RE: [PATCH v5 3/3] drm/i915/edp: eDP Data Overrride

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

On Tue, 19 Aug 2025, "Kandpal, Suraj" <suraj.kandpal@intel.com> wrote:
>> 
>> On Tue, 19 Aug 2025, Suraj Kandpal <suraj.kandpal@intel.com> wrote:
>> > We need override certain link rates in favour of the next available
>> > higher link rate. The Link rates that need to be overridden are
>> > indicated by a mask in VBT. To make sure these modes are skipped we
>> > don't add them in them in the sink rates array.
>> >
>> > --v2
>> > -Update the link rates after we have a final set of link rates [Ankit]
>> > -Break this patch up [Ankit] -Optimize the assingment during loop
>> > [Ankit]
>> >
>> > --v3
>> > -Add protection against broken VBTs [Jani]
>> >
>> > --v4
>> > -Fix build errors
>> > -Create a seprate function to check if edp data override is selected
>> > and using the correct vbt
>> >
>> > --v5
>> > -Use correct number to check the num of edp rates [Ankit]
>> >
>> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/display/intel_bios.c | 15 ++++++++++++++-
>> > drivers/gpu/drm/i915/display/intel_bios.h |  2 ++
>> >  drivers/gpu/drm/i915/display/intel_dp.c   | 22 ++++++++++++++++++++++
>> >  3 files changed, 38 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c
>> > b/drivers/gpu/drm/i915/display/intel_bios.c
>> > index 444ed54f7c35..05a74c3bc9af 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
>> > @@ -2521,11 +2521,24 @@ int intel_bios_dp_max_lane_count(const struct
>> > intel_bios_encoder_data *devdata)  bool
>> > intel_bios_encoder_reject_edp_rate(const struct intel_bios_encoder_data
>> *devdata,
>> >  				   int rate)
>> > +{
>> > +	return devdata->child.edp_data_rate_override &
>> > +edp_rate_override_mask(rate); }
>> > +
>> > +bool
>> > +intel_bios_vbt_supports_edp_data_override(const struct
>> > +intel_bios_encoder_data *devdata)
>> 
>> Why are you adding this function? Earlier versions didn't have it, and the reason
>> for its existence isn't explained or clear.
>> 
>
> This function was added in v3 (I did add a small mention in the commit message I can maybe elaborate more there)
>  in response to one of your comments in which you mentioned the
> Possibility of ending up with a broken vbt that masked all the link rates causing us to have no link rate
> To drive the edp with this function now checks if we have at least one edp link rate using which we can
> Drive edp if not we do not modify the list of sink rates at all.

You can check for that scenario in intel_bios_encoder_reject_edp_rate(),
and return false for all rates if all of the bits are set.

We don't have to leak these checks into callers.


BR,
Jani.


>
> Regards,
> Suraj Kandpal
>
>> BR,
>> Jani.
>> 
>> 
>> >  {
>> >  	if (!devdata || devdata->display->vbt.version < 263)
>> >  		return false;
>> >
>> > -	return devdata->child.edp_data_rate_override &
>> edp_rate_override_mask(rate);
>> > +	/*
>> > +	 * This means the VBT ends up asking us to override every possible rate
>> > +	 * indicating the VBT is broken so skip this
>> > +	 */
>> > +	if (hweight32(devdata->child.edp_data_rate_override) >=
>> BDB_263_VBT_EDP_NUM_RATES)
>> > +		return false;
>> > +
>> > +	return true;
>> >  }
>> >
>> >  static void sanitize_device_type(struct intel_bios_encoder_data
>> > *devdata, diff --git a/drivers/gpu/drm/i915/display/intel_bios.h
>> > b/drivers/gpu/drm/i915/display/intel_bios.h
>> > index 781e08f7eeb2..d24660bcc7f3 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_bios.h
>> > +++ b/drivers/gpu/drm/i915/display/intel_bios.h
>> > @@ -276,5 +276,7 @@ void intel_bios_for_each_encoder(struct intel_display
>> *display,
>> >  					      const struct
>> intel_bios_encoder_data *devdata));
>> >
>> >  void intel_bios_debugfs_register(struct intel_display *display);
>> > +bool
>> > +intel_bios_vbt_supports_edp_data_override(const struct
>> > +intel_bios_encoder_data *devdata);
>> >
>> >  #endif /* _INTEL_BIOS_H_ */
>> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
>> > b/drivers/gpu/drm/i915/display/intel_dp.c
>> > index 54d88f24b689..f6fad42182ae 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> > @@ -4277,6 +4277,26 @@ static void intel_edp_mso_init(struct intel_dp
>> *intel_dp)
>> >  	intel_dp->mso_pixel_overlap = mso ? info->mso_pixel_overlap : 0;  }
>> >
>> > +static void
>> > +intel_edp_set_data_override_rates(struct intel_dp *intel_dp) {
>> > +	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
>> > +	int *sink_rates = intel_dp->sink_rates;
>> > +	int i, j = 0;
>> > +
>> > +	if (!intel_bios_vbt_supports_edp_data_override(encoder->devdata))
>> > +		return;
>> > +
>> > +	for (i = 0; i < intel_dp->num_sink_rates; i++) {
>> > +		if (intel_bios_encoder_reject_edp_rate(encoder->devdata,
>> > +						       intel_dp->sink_rates[i]))
>> > +			continue;
>> > +
>> > +		sink_rates[j++] = intel_dp->sink_rates[i];
>> > +	}
>> > +	intel_dp->num_sink_rates = j;
>> > +}
>> > +
>> >  static void
>> >  intel_edp_set_sink_rates(struct intel_dp *intel_dp)  { @@ -4327,6
>> > +4347,8 @@ intel_edp_set_sink_rates(struct intel_dp *intel_dp)
>> >  		intel_dp->use_rate_select = true;
>> >  	else
>> >  		intel_dp_set_sink_rates(intel_dp);
>> > +
>> > +	intel_edp_set_data_override_rates(intel_dp);
>> >  }
>> >
>> >  static bool
>> 
>> --
>> Jani Nikula, Intel

-- 
Jani Nikula, Intel