Re: [RFC v1 11/11] drm/i915/mtl: add V/P Override support for mtl+

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

On Sun, 08 Mar 2026, Michał Grzelak <michal.grzelak@intel.com> wrote:
> Add macros to reflect the layout of vswing/preemphasis override tables
> for mtl. Add separate macros for C10 & C20.
>
> Add separate intel_ddi_buf_trans_entry to be overridden for mtl onward.
> Set & return it when port requests to override default table. Use same
> setter for both C10 & C20 cases.
>
> Set the value by extracting the lowest byte from entry from the table.
>
> 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.
>
> Signed-off-by: Michał Grzelak <michal.grzelak@intel.com>
> ---
>  .../drm/i915/display/intel_ddi_buf_trans.c    | 99 ++++++++++++++++++-
>  1 file changed, 96 insertions(+), 3 deletions(-)
>
> 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 1b30c9888f95..7798320a4968 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.c
> @@ -12,6 +12,17 @@
>  #include "intel_dp.h"
>  #include "intel_lt_phy.h"
>  
> +#define MTL_C10_VS_PE_DP14_RBR_HBR 0
> +#define MTL_C10_VS_PE_DP14_HBR2_HBR3 1
> +#define MTL_C10_VS_PE_EDP_NON_HBR3 2
> +#define MTL_C10_VS_PE_EDP_HBR3 3
> +
> +#define MTL_C20_VS_PE_DP14 4
> +#define MTL_C20_VS_PE_DP20 5
> +
> +#define MTL_CX0_VS_PE_COL 3
> +#define MTL_CX0_VS_PE_ROW 16

I don't know what any of these mean, and it's really hard to figure out
from the code.

> +
>  #define XE3P_VS_PE_EDP 3
>  #define XE3P_VS_PE_DP14 4
>  #define XE3P_VS_PE_DP21 5
> @@ -1109,6 +1120,25 @@ static const union intel_ddi_buf_trans_entry _mtl_c20_trans_hdmi[] = {
>  	{ .snps = { 32, 4, 12 } },      /* preset 4 */
>  };
>  
> +static union intel_ddi_buf_trans_entry _mtl_cx0_trans_override[] = {
> +	{ .snps = { 0, 0, 0 } },
> +	{ .snps = { 0, 0, 0 } },
> +	{ .snps = { 0, 0, 0 } },
> +	{ .snps = { 0, 0, 0 } },
> +	{ .snps = { 0, 0, 0 } },
> +	{ .snps = { 0, 0, 0 } },
> +	{ .snps = { 0, 0, 0 } },
> +	{ .snps = { 0, 0, 0 } },
> +	{ .snps = { 0, 0, 0 } },
> +	{ .snps = { 0, 0, 0 } },
> +	{ .snps = { 0, 0, 0 } },
> +	{ .snps = { 0, 0, 0 } },
> +	{ .snps = { 0, 0, 0 } },
> +	{ .snps = { 0, 0, 0 } },
> +	{ .snps = { 0, 0, 0 } },
> +	{ .snps = { 0, 0, 0 } },
> +};
> +

Using mutable static data for this is not possible. It's module
specific, which means having two devices in the same system will trample
each other's data.

>  static const struct intel_ddi_buf_trans mtl_c20_trans_hdmi = {
>  	.entries = _mtl_c20_trans_hdmi,
>  	.num_entries = ARRAY_SIZE(_mtl_c20_trans_hdmi),
> @@ -1126,6 +1156,11 @@ static const struct intel_ddi_buf_trans mtl_c20_trans_uhbr = {
>  	.num_entries = ARRAY_SIZE(_mtl_c20_trans_uhbr),
>  };
>  
> +static struct intel_ddi_buf_trans mtl_cx0_trans_override = {
> +	.entries = _mtl_cx0_trans_override,
> +	.num_entries = ARRAY_SIZE(_mtl_cx0_trans_override),
> +};
> +
>  /* DP1.4 */
>  static const union intel_ddi_buf_trans_entry _xe3plpd_lt_trans_dp14[] = {
>  	{ .lt = { 21, 0, 0 , 1, 0 } },
> @@ -1815,11 +1850,60 @@ dg2_get_snps_buf_trans(struct intel_encoder *encoder,
>  		return intel_get_buf_trans(&dg2_snps_trans, n_entries);
>  }
>  
> +static const struct intel_ddi_buf_trans *
> +mtl_set_cx0_buf_trans(struct intel_encoder *encoder,
> +		      int idx,
> +		      int *n_entries)
> +{
> +	struct intel_display *display = to_intel_display(encoder);
> +	struct intel_ddi_buf_trans *buf_trans = &mtl_cx0_trans_override;
> +	union intel_ddi_buf_trans_entry *entries, *entry;
> +	const u32 *tables = display->vbt.override_vswing;
> +	const u32 *vals;
> +
> +	entries = (union intel_ddi_buf_trans_entry *) buf_trans->entries;
> +	for (int row = 0; row < MTL_CX0_VS_PE_ROW; row++) {
> +		entry = &entries[row];
> +		vals = find_row(tables,
> +				idx,
> +				row,
> +				MTL_CX0_VS_PE_ROW,
> +				MTL_CX0_VS_PE_COL);
> +
> +		entry->snps.vswing = LOW(vals[0]);
> +		entry->snps.pre_cursor = LOW(vals[1]);
> +		entry->snps.post_cursor = LOW(vals[2]);
> +	}
> +
> +	return intel_get_buf_trans(&mtl_cx0_trans_override, n_entries);
> +}

But why do we have to "set" the data in the first place?

Is it not possible to arrange the buf trans data that we have in the
same format as VBT? And then instead of returning the data from our
predefined tables, we'd return the data from VBT. Without conversions.

If that's not feasible, then parsing of VBT data belongs to
intel_bios.c. That's the place where we convert VBT input into the
format our driver understands, and the rest of the driver doesn't need
to jump through hoops to parse it.

> +
>  static const struct intel_ddi_buf_trans *
>  mtl_get_c10_buf_trans(struct intel_encoder *encoder,
>  		      const struct intel_crtc_state *crtc_state,
>  		      int *n_entries)
>  {
> +	if (intel_bios_encoder_overrides_vswing(encoder->devdata)) {
> +		if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP)) {
> +			if (intel_dp_above_hbr1(crtc_state))
> +				return mtl_set_cx0_buf_trans(encoder,
> +							     MTL_C10_VS_PE_DP14_HBR2_HBR3,
> +							     n_entries);
> +			else
> +				return mtl_set_cx0_buf_trans(encoder,
> +							     MTL_C10_VS_PE_DP14_RBR_HBR,
> +							     n_entries);
> +		} else if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_EDP)){
> +			if (intel_edp_above_hbr2(crtc_state))
> +				return mtl_set_cx0_buf_trans(encoder,
> +							     MTL_C10_VS_PE_EDP_HBR3,
> +							     n_entries);
> +			else
> +				return mtl_set_cx0_buf_trans(encoder,
> +							     MTL_C10_VS_PE_EDP_NON_HBR3,
> +							     n_entries);
> +		}
> +	}

Can we not do this in a more generic fashion instead of sprinkling these
override things and if-ladders absolutely everywhere? I mean the whole
file is already a bunch of if-ladders, but do we really need to add
another set for VBT stuff?

>  	return intel_get_buf_trans(&mtl_c10_trans_dp14, n_entries);
>  }
>  
> @@ -1828,12 +1912,21 @@ mtl_get_c20_buf_trans(struct intel_encoder *encoder,
>  		      const struct intel_crtc_state *crtc_state,
>  		      int *n_entries)
>  {
> -	if (intel_crtc_has_dp_encoder(crtc_state) && intel_dp_is_uhbr(crtc_state))
> +	if (intel_crtc_has_dp_encoder(crtc_state) && intel_dp_is_uhbr(crtc_state)) {
> +		if (intel_bios_encoder_overrides_vswing(encoder->devdata))
> +			return mtl_set_cx0_buf_trans(encoder,
> +						     MTL_C20_VS_PE_DP20,
> +						     n_entries);
>  		return intel_get_buf_trans(&mtl_c20_trans_uhbr, n_entries);
> -	else if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
> +	} else if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
>  		return intel_get_buf_trans(&mtl_c20_trans_hdmi, n_entries);
> -	else
> +	} else {
> +		if (intel_bios_encoder_overrides_vswing(encoder->devdata))
> +			return mtl_set_cx0_buf_trans(encoder,
> +						     MTL_C20_VS_PE_DP14,
> +						     n_entries);
>  		return intel_get_buf_trans(&mtl_c20_trans_dp14, n_entries);
> +	}
>  }
>  
>  static const struct intel_ddi_buf_trans *

-- 
Jani Nikula, Intel