On Thu, 04 Jun 2026, Michał Grzelak <[email protected]> wrote:
> Add accessor function for LT to read requested table from VBT #57.
> Parse the requested table and transform data into port's buffer.
>
> Add helper for checking if devdata is safe for dereference. Proceed with
> default values if not.
>
> Add helper to check if VS/PE-O buffer has been allocated during
> allocate_vswing_preemph_override(). Proceed with default values if not.
>
> LT's VS/PE-O tables have less columns than xe3plpd_lt_phy_buf_trans
> contains fields. Thus copy txswing and txswing_level from default VS/PE
> values onto VS/PE-O tables.
>
> Use 6th table if encoder supports DP 2.0 or higher. Otherwise use 5th
> table for DP.
>
> There are no changes to intel_ddi_dp_level() since selection of correct
> row of intel_ddi_buf_trans_entry is same as when no override request has
> been done.
>
> Tables 1-4 are not used at all and are most likely to be zeroed. 5th
> table is used for any mode below DP 2.0 (exclusive). 6th table is used
> for any mode above DP 2.0 (inclusive).
>
> Indices for other tables have not yet been observed to be used as of
> now.
>
> v5->v6
> - remove drm_WARN_ONCE (Suraj)
> - pass default VS/PE tables to LT's VBT accessor (Suraj)
> - set txswing & _level from default VS/PE tables (Suraj)
> - add helper checking if VS/PE-O has been allocated (Suraj)
> - check if devdata is not NULL
>
> v4->v5
> - add if-ladder instead of function pointer
> - blend index computation with table parsing
> - remove WARN and debug messages
> - remove enums entirely
> - add spaces around operators (Suraj)
> - remove spaces after type casting (Suraj)
> - remove INTEL_DISPLAY_STATE_WARN (Suraj)
>
> v3->v4
> - stick to solely changing VBT data into current structures (Jani)
> - move iterator declaration to declaration block (Suraj)
>
> v2->v3
> - remove unnecessary braces from if block (Suraj)
> - return -EINVAL instead of -1 (Suraj)
>
> Signed-off-by: Michał Grzelak <[email protected]>
> ---
> drivers/gpu/drm/i915/display/intel_bios.c | 40 +++++++++++++++++++
> drivers/gpu/drm/i915/display/intel_bios.h | 6 +++
> .../drm/i915/display/intel_ddi_buf_trans.c | 37 ++++++++++++++++-
> 3 files changed, 82 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c
> b/drivers/gpu/drm/i915/display/intel_bios.c
> index bc48ed9a7cbf5..302a9465a637b 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -3846,6 +3846,11 @@ int intel_bios_hdmi_ddc_pin(const struct
> intel_bios_encoder_data *devdata)
> return map_ddc_pin(devdata->display, devdata->child.ddc_pin);
> }
>
> +bool intel_bios_encoder_allocated_vspeo(const struct intel_bios_encoder_data
> *devdata)
> +{
> + return !!devdata->vspeo;
> +}
Please don't add this function. Just return NULL from get call below and
handle it in the caller.
> +
> bool intel_bios_encoder_requests_vspeo(const struct intel_bios_encoder_data
> *devdata)
> {
> return devdata->display->vbt.version >= 218 &&
> devdata->child.use_vbt_vswing;
> @@ -3861,6 +3866,41 @@ bool intel_bios_encoder_supports_tbt(const struct
> intel_bios_encoder_data *devda
> return devdata->display->vbt.version >= 209 && devdata->child.tbt;
> }
>
> +const struct intel_ddi_buf_trans *
> +intel_bios_get_lt_vspeo(const struct intel_bios_encoder_data *devdata,
> + const struct intel_ddi_buf_trans *buf_trans,
> + int idx)
> +{
> + struct intel_display *display = devdata->display;
> + struct intel_ddi_buf_trans *vspeo = (void *)devdata->vspeo;
> + union intel_ddi_buf_trans_entry *entries = (void *)vspeo->entries;
What's with the casts?
> + const u32 *tables = display->vbt.vspeo.tables;
> + int num_columns = display->vbt.vspeo.num_columns;
> + int num_rows = display->vbt.vspeo.num_rows;
> + size_t offset = 0;
> + int level;
> +
> + offset += idx * num_rows * num_columns;
I think the division of responsibilities is not great if the caller
passes in an index that's tied to the VBT data format.
> +
> + for (level = 0; level < num_rows; level++) {
> + u8 txswing = buf_trans->entries[level].lt.txswing;
> + u8 txswing_level = buf_trans->entries[level].lt.txswing_level;
> + u32 main_cursor = tables[offset];
> + u32 pre_cursor = tables[offset + 1];
> + u32 post_cursor = tables[offset + 2];
> +
> + entries[level].lt.txswing = txswing;
> + entries[level].lt.txswing_level = txswing_level;
> + entries[level].lt.main_cursor = main_cursor;
> + entries[level].lt.pre_cursor = pre_cursor;
> + entries[level].lt.post_cursor = post_cursor;
> +
> + offset += num_columns;
> + }
> +
> + return vspeo;
> +}
> +
> bool intel_bios_encoder_is_dedicated_external(const struct
> intel_bios_encoder_data *devdata)
> {
> return devdata->display->vbt.version >= 264 &&
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.h
> b/drivers/gpu/drm/i915/display/intel_bios.h
> index 7a50a272cd27d..1a9b27d8e5789 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.h
> +++ b/drivers/gpu/drm/i915/display/intel_bios.h
> @@ -73,6 +73,12 @@ bool intel_bios_get_dsc_params(struct intel_encoder
> *encoder,
> const struct intel_bios_encoder_data *
> intel_bios_encoder_data_lookup(struct intel_display *display, enum port
> port);
>
> +const struct intel_ddi_buf_trans *
> +intel_bios_get_lt_vspeo(const struct intel_bios_encoder_data *devdata,
> + const struct intel_ddi_buf_trans *buf_trans,
> + int idx);
> +
> +bool intel_bios_encoder_allocated_vspeo(const struct intel_bios_encoder_data
> *devdata);
> bool intel_bios_encoder_requests_vspeo(const struct intel_bios_encoder_data
> *devdata);
> bool intel_bios_encoder_supports_dvi(const struct intel_bios_encoder_data
> *devdata);
> bool intel_bios_encoder_supports_hdmi(const struct intel_bios_encoder_data
> *devdata);
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.c
> b/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.c
> index 4cd1e4d76c7af..f936868d6113a 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.c
> @@ -1784,6 +1784,24 @@ xe3plpd_get_lt_buf_trans(struct intel_encoder *encoder,
> return intel_get_buf_trans(&xe3plpd_lt_trans_dp14, n_entries);
> }
>
> +static const struct intel_ddi_buf_trans *
> +xe3plpd_get_lt_vspeo_buf_trans(struct intel_encoder *encoder,
> + const struct intel_crtc_state *crtc_state,
> + int *n_entries)
> +{
> + const struct intel_ddi_buf_trans *buf_trans;
> +
> + buf_trans = encoder->get_buf_trans(encoder, crtc_state, n_entries);
> + if (intel_crtc_has_dp_encoder(crtc_state)) {
> + if (intel_dp_is_uhbr(crtc_state))
> + return intel_bios_get_lt_vspeo(encoder->devdata,
> buf_trans, 5);
> + else
> + return intel_bios_get_lt_vspeo(encoder->devdata,
> buf_trans, 4);
> + }
> +
> + return buf_trans;
> +}
I need to think about this.
Basically this approach is duplicating the platform if-else ladders
*and* the CRTC type and port clock etc. checks already existing in this
file. It's not great for maintainability, and it's a lot of code for the
feature.
Plus there's all the added code in intel_bios.c too.
> +
> void intel_ddi_buf_trans_init(struct intel_encoder *encoder)
> {
> struct intel_display *display = to_intel_display(encoder);
> @@ -1857,5 +1875,22 @@ const struct intel_ddi_buf_trans
> *intel_ddi_buf_trans_get(struct intel_encoder *
> const struct
> intel_crtc_state *crtc_state,
> int *n_entries)
> {
> - return encoder->get_buf_trans(encoder, crtc_state, n_entries);
> + struct intel_display *display = to_intel_display(encoder);
> + const struct intel_ddi_buf_trans *buf_trans;
> +
> + if (!encoder->devdata)
The intel_bios_* functions need to check that, not here.
> + return encoder->get_buf_trans(encoder, crtc_state, n_entries);
> +
> + if (!intel_bios_encoder_requests_vspeo(encoder->devdata))
> + return encoder->get_buf_trans(encoder, crtc_state, n_entries);
> +
> + if (!intel_bios_encoder_allocated_vspeo(encoder->devdata))
> + return encoder->get_buf_trans(encoder, crtc_state, n_entries);
Ditto for the allocation, don't check here.
> +
> + if (HAS_LT_PHY(display))
> + buf_trans = xe3plpd_get_lt_vspeo_buf_trans(encoder, crtc_state,
> n_entries);
> + else
> + buf_trans = encoder->get_buf_trans(encoder, crtc_state,
> n_entries);
> +
> + return intel_get_buf_trans(buf_trans, n_entries);
This function has four paths to call encoder->get_buf_trans(). There
*must* be only one.
> }
--
Jani Nikula, Intel
