RE: [PATCH v7 5/8] drm/i915: override Snps's VS/PE when requested

["Michał Grzelak" <michal.grzelak@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 9653 bytes --]

On Mon, 15 Jun 2026, Kandpal, Suraj wrote:
>> Subject: RE: [PATCH v7 5/8] drm/i915: override Snps's VS/PE when requested
>>
>>
>>> Subject: [PATCH v7 5/8] drm/i915: override Snps's VS/PE when requested
>>>
>>> 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()
>>> basing on
>>
>> * based
>>
>>> 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.
>>>
>>> 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>
>>> ---
>>>  drivers/gpu/drm/i915/display/intel_bios.c     | 100 ++++++++++++++++++
>>>  drivers/gpu/drm/i915/display/intel_bios.h     |   7 ++
>>>  .../drm/i915/display/intel_ddi_buf_trans.c    |  21 ++++
>>>  3 files changed, 128 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c
>>> b/drivers/gpu/drm/i915/display/intel_bios.c
>>> index 3d8864374cac..59ffb5bc8848 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_bios.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
>>> @@ -3861,6 +3861,106 @@ 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) {
>>
>> Ahh ohkay so you deal with that allocation problem this way. This seems good
>> too.
>>
>>
>>> +	struct intel_ddi_buf_trans *vspeo;
>>> +
>>> +	if (!devdata)
>>> +		return false;
>>> +
>>> +	/* if vspeo is allocated then VS/PE-O was requested */
>>> +	vspeo = devdata->vspeo;
>>> +	if (!vspeo)
>>> +		return false;
>>> +
>>> +	/* if crtc_state has eDP it also has DP */
>>> +	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;
>>> +	entries = (void *)vspeo->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;
>>> +
>>> +	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;
>>> +
>>> +		offset += num_columns;
>>> +	}
>>> +
>>> +	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 = (void *)vspeo->entries;
>>> +	tables = display->vbt.vspeo.tables;
>>> +	num_columns = display->vbt.vspeo.num_columns;
>>> +	num_rows = display->vbt.vspeo.num_rows;
>>> +
>>> +	idx = port_clock > 270000 ? 1 : 0;
>>> +	if (has_edp)
>>> +		idx = port_clock > 540000 ? 3 : 2;
>>> +
>>> +	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;
>>> +
>>> +		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 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 4cd1e4d76c7a..b4120b9c49b2 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.c
>>> @@ -1857,5 +1857,26 @@ const struct intel_ddi_buf_trans
>>> *intel_ddi_buf_trans_get(struct intel_encoder *
>>>  							  const struct
>>> intel_crtc_state *crtc_state,
>>>  							  int *n_entries)
>>>  {
>>> +	struct intel_display *display = to_intel_display(encoder);
>>> +	const struct intel_bios_encoder_data *devdata = encoder->devdata;
>>> +	const struct intel_ddi_buf_trans *buf_trans = NULL;
>>> +	bool has_edp, has_dp, is_uhbr;
>>> +	int port_clock;
>>> +
>>> +
>>> +	is_uhbr = intel_dp_is_uhbr(crtc_state);
>>> +	port_clock = crtc_state->port_clock;
>>
>> We don't need a separate port_clock variable its just called at one place just
>> directly use crtc_state->port_clock.

Later patches also use port_clock for eg. Combo, so I think it
could be better to retain it in current state instead of overwritting
it. Assuming that it is right approach of course.

BR,
Michał

>>
>>> +
>>> +	if (DISPLAY_VER(display) >= 14) {
>>
>> This needs to use >= 14 && < 35 so that this does not spill into LT PHY territory
>>
>>> +		if (intel_encoder_is_c10phy(encoder))
>>> +			buf_trans = intel_bios_get_c10_vspeo(devdata,
>>> has_dp, port_clock, has_edp);
>>> +		else
>>> +			buf_trans = intel_bios_get_c20_vspeo(devdata,
>>> has_dp, is_uhbr);
>>> +	}
>>> +
>>> +	if (buf_trans)
>>> +		return intel_get_buf_trans(buf_trans, n_entries);
>>
>> I think this code block belongs inside the above if block guarded by
>> DISPLAY_VER check
>
> Or maybe not since this later becomes a else if ladder you can ignore this comment.
>
> Regards,
> Suraj Kandpal
>
>>
>> Regards,
>> Suraj Kandpal
>>
>>> +
>>>  	return encoder->get_buf_trans(encoder, crtc_state, n_entries);  }
>>> --
>>> 2.45.2
>
>