Re: [PATCH v6 5/8] drm/i915: override LT's VS/PE when requested

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 04 Jun 2026, Michał Grzelak <michal.grzelak@intel.com> 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 <michal.grzelak@intel.com>
> ---
>  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