Re: [PATCH v10 6/8] drm/i915: override Snps's VS/PE when requested

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

On Thu, 02 Jul 2026, Michał Grzelak <michal.grzelak@intel.com> wrote:
> Add accessor function for Snps to read requested table from VBT #57.
> Parse the requested table and transform data into port's buffer.
>
> Choose appropriate accessor function in intel_ddi_buf_trans_get() based
> on display version and PHY type.
>
> For C20, use 6th table if encoder supports DP 2.0 or higher. Otherwise
> use 5th table for DP.
>
> For C20, 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).
>
> For C10, use 2nd table for external DP if encoder supports any mode
> beyond or including HBR2. Use 1st table if external DP encoder supports
> anything lower than HBR2. For eDP, use 4th table if encoder supports
> HBR3. Otherwise use 3rd table for eDP.
>
> For C10, 1st table is used for external DP with modes below HBR2
> (exclusive). 1st table is also used as a fallback for non-DPs. 2nd
> table is used for external DP with modes higher than HBR2 (inclusive).
> 3rd table is used for eDP with modes lower than HBR3 (exclusive). 4th
> table is used for eDP with modes higher than HBR3 (inclusive).
>
> Indices for other tables have not yet been observed to be used as of
> now.
>
> 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.
>
> v9->v10
> - call dedicated VS/PE-O vfunc
> - drop deconstifying default tables (Suraj, Jani)
> - cache `entries` into const field after data is overwritten (Jani)
>
> v8->v9
> - init vspeo before using it
> - deconstify intel_ddi_buf_trans_entry
>
> v7->v8
> - remove comments (Suraj)
> - add check for LT (Suraj)
>
> v6->v7
> - handle VS/PE-O's VBT details in intel_bios_* functions (Jani)
> - remove vspeo's cast to (void *) (Jani)
> - check devdata->vspeo if VS/PE-O was requested
> - call encoder->get_buf_trans() once (Jani)
> - return NULL from intel_bios_get_* when using default (Jani)
> - validate VS/PE-O in intel_bios.c (Jani)
> - inline mtl_{c10,c20}_get_vspeo_buf_trans()
> - remove temporarily LT
>
> v4->v5
> - blend index computation with table parsing
> - remove enums entirely
> - change funcs prefix from snps_ to mtl_ (Suraj)
> - 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>
> Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bios.c     | 104 ++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_bios.h     |   7 ++
>  .../drm/i915/display/intel_ddi_buf_trans.c    |  37 ++++++-
>  3 files changed, 146 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index a491b8500611..f897ac067585 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -3881,6 +3881,110 @@ bool intel_bios_encoder_supports_tbt(const struct intel_bios_encoder_data *devda
>  	return devdata->display->vbt.version >= 209 && devdata->child.tbt;
>  }
>  
> +static bool
> +validate_vspeo(const struct intel_bios_encoder_data *devdata, bool has_dp)
> +{
> +	struct intel_ddi_buf_trans *vspeo;
> +
> +	if (!devdata)
> +		return false;

When would this be NULL?

> +
> +	vspeo = devdata->vspeo;
> +	if (!vspeo)
> +		return false;
> +
> +	if (!has_dp)
> +		return false;
> +
> +	return true;
> +}
> +
> +const struct intel_ddi_buf_trans *
> +intel_bios_get_c20_vspeo(const struct intel_bios_encoder_data *devdata,
> +			 bool has_dp, bool is_uhbr)
> +{
> +	struct intel_display *display;
> +	union intel_ddi_buf_trans_entry *entries;
> +	int num_columns, num_rows, level, idx;
> +	struct intel_ddi_buf_trans *vspeo;
> +	const u32 *tables;
> +	size_t offset = 0;
> +
> +	if (!validate_vspeo(devdata, has_dp))
> +		return NULL;
> +
> +	display = devdata->display;
> +	vspeo = devdata->vspeo;
> +	entries = devdata->entries;
> +	tables = display->vbt.vspeo.tables;
> +	num_columns = display->vbt.vspeo.num_columns;
> +	num_rows = display->vbt.vspeo.num_rows;
> +	idx = is_uhbr ? 5 : 4;

All of these should just be initialized at declaration.

> +
> +	offset += idx * num_rows * num_columns;
> +
> +	for (level = 0; level < num_rows; level++) {
> +		u32 vswing = tables[offset];
> +		u32 pre_cursor = tables[offset + 1];
> +		u32 post_cursor = tables[offset + 2];
> +
> +		entries[level].snps.vswing = vswing;
> +		entries[level].snps.pre_cursor = pre_cursor;
> +		entries[level].snps.post_cursor = post_cursor;

The struct dg2_snps_phy_buf_trans (snps) have u8 members. Is the data in
VBT in only the lowest byte of the u32? Or how is it arranged?

> +
> +		offset += num_columns;
> +	}
> +
> +	vspeo->entries = entries;
> +	vspeo->num_entries = num_rows;

It's common to have a blank line before return.

> +	return vspeo;
> +}
> +
> +const struct intel_ddi_buf_trans *
> +intel_bios_get_c10_vspeo(const struct intel_bios_encoder_data *devdata,
> +			 bool has_dp, int port_clock, bool has_edp)
> +{
> +	struct intel_display *display;
> +	union intel_ddi_buf_trans_entry *entries;
> +	int num_columns, num_rows, level, idx;
> +	struct intel_ddi_buf_trans *vspeo;
> +	const u32 *tables;
> +	size_t offset = 0;
> +
> +	if (!validate_vspeo(devdata, has_dp))
> +		return NULL;
> +
> +	display = devdata->display;
> +	vspeo = devdata->vspeo;
> +	entries = devdata->entries;
> +	tables = display->vbt.vspeo.tables;
> +	num_columns = display->vbt.vspeo.num_columns;
> +	num_rows = display->vbt.vspeo.num_rows;

Ditto about initialization.

> +
> +	idx = port_clock > 270000 ? 1 : 0;
> +	if (has_edp)
> +		idx = port_clock > 540000 ? 3 : 2;

if (has_edp)
	...
else
	...

seems more idiomatic than assigning twice for has_edp.

> +
> +	offset += idx * num_rows * num_columns;
> +
> +	for (level = 0; level < num_rows; level++) {
> +		u32 vswing = tables[offset];
> +		u32 pre_cursor = tables[offset + 1];
> +		u32 post_cursor = tables[offset + 2];
> +
> +		entries[level].snps.vswing = vswing;
> +		entries[level].snps.pre_cursor = pre_cursor;
> +		entries[level].snps.post_cursor = post_cursor;

Ditto about u8 vs u32.

> +
> +		offset += num_columns;
> +	}
> +
> +	vspeo->entries = entries;
> +	vspeo->num_entries = num_rows;
> +
> +	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 7a50a272cd27..49acf8c405e2 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.h
> +++ b/drivers/gpu/drm/i915/display/intel_bios.h
> @@ -73,6 +73,13 @@ 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_c20_vspeo(const struct intel_bios_encoder_data *devdata,
> +			 bool has_dp, bool is_uhbr);
> +const struct intel_ddi_buf_trans *
> +intel_bios_get_c10_vspeo(const struct intel_bios_encoder_data *devdata,
> +			 bool has_dp, int port_clock, bool has_edp);
> +
>  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 f31283a0331b..92c0d0f933ab 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,36 @@ 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 *
> +mtl_get_c10_buf_trans_override(struct intel_encoder *encoder,
> +			       const struct intel_crtc_state *crtc_state,
> +			       int *n_entries)
> +{
> +	const struct intel_bios_encoder_data *devdata = encoder->devdata;
> +	bool has_edp, has_dp;
> +	int port_clock;
> +
> +	has_edp = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_EDP);
> +	has_dp = intel_crtc_has_dp_encoder(crtc_state);
> +	port_clock = crtc_state->port_clock;
> +
> +	return intel_bios_get_c10_vspeo(devdata, has_dp, port_clock, has_edp);
> +}
> +
> +static const struct intel_ddi_buf_trans *
> +mtl_get_c20_buf_trans_override(struct intel_encoder *encoder,
> +			       const struct intel_crtc_state *crtc_state,
> +			       int *n_entries)
> +{
> +	const struct intel_bios_encoder_data *devdata = encoder->devdata;
> +	bool has_dp, is_uhbr;
> +
> +	has_dp = intel_crtc_has_dp_encoder(crtc_state);
> +	is_uhbr = intel_dp_is_uhbr(crtc_state);
> +
> +	return intel_bios_get_c20_vspeo(devdata, has_dp, is_uhbr);
> +}
> +
>  void intel_ddi_buf_trans_init(struct intel_encoder *encoder)
>  {
>  	struct intel_display *display = to_intel_display(encoder);
> @@ -1791,10 +1821,13 @@ void intel_ddi_buf_trans_init(struct intel_encoder *encoder)
>  	if (HAS_LT_PHY(display)) {
>  		encoder->get_buf_trans = xe3plpd_get_lt_buf_trans;
>  	} else if (DISPLAY_VER(display) >= 14) {
> -		if (intel_encoder_is_c10phy(encoder))
> +		if (intel_encoder_is_c10phy(encoder)) {
>  			encoder->get_buf_trans = mtl_get_c10_buf_trans;
> -		else
> +			encoder->get_buf_trans_override = mtl_get_c10_buf_trans_override;
> +		} else {
>  			encoder->get_buf_trans = mtl_get_c20_buf_trans;
> +			encoder->get_buf_trans_override = mtl_get_c20_buf_trans_override;
> +		}
>  	} else if (display->platform.dg2) {
>  		encoder->get_buf_trans = dg2_get_snps_buf_trans;
>  	} else if (display->platform.alderlake_p) {

-- 
Jani Nikula, Intel