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

On Tue, 11 Aug 2026, Jani Nikula wrote:
> On Thu, 02 Jul 2026, MichaÂł Grzelak <[email protected]> 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 <[email protected]>
>> Reviewed-by: Suraj Kandpal <[email protected]>
>> ---
>>  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?

Never. I missed that your suggestion makes null pointer dereference seen
earlier [1] no longer possible on Haswell. Will remove it.

>
>> +
>> +	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?

Yes, data in VBT is stored in only lowest byte of u32. Even if other
bytes are set, they should be discarded; that's my reasoning behind this
implicit conversion.

I probably should include it in the commit message.

BR,
MichaÂł

[1] https://lore.kernel.org/intel-gfx/178042917831.36136.4204902639552043116@6beec6c84f66/

>
>> +
>> +		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
>