[PATCH 2/3] drm/i915/bios: Add function to check if edp data override is needed

[Kandpal, Suraj]

> -----Original Message-----
> From: Jani Nikula <jani.nikula at linux.intel.com>
> Sent: Monday, July 28, 2025 6:28 PM
> To: Kandpal, Suraj <suraj.kandpal at intel.com>; intel-xe at lists.freedesktop.org;
> intel-gfx at lists.freedesktop.org
> Cc: Nautiyal, Ankit K <ankit.k.nautiyal at intel.com>; Kandpal, Suraj
> <suraj.kandpal at intel.com>
> Subject: Re: [PATCH 2/3] drm/i915/bios: Add function to check if edp data
> override is needed
> 
> On Fri, 25 Jul 2025, Suraj Kandpal <suraj.kandpal at intel.com> wrote:
> > Add a function that helps identify if the rate provided needs to be
> > overridden. For this we need a function that compares the rate
> > provided and bitmask of rates provided in VBT.
> >
> > Signed-off-by: Suraj Kandpal <suraj.kandpal at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_bios.c | 58
> > +++++++++++++++++++++++  drivers/gpu/drm/i915/display/intel_bios.h |
> > 2 +
> >  2 files changed, 60 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c
> > b/drivers/gpu/drm/i915/display/intel_bios.c
> > index 8337ebe0f2c8..3eac804b2449 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> > @@ -2480,6 +2480,54 @@ static int
> parse_bdb_216_dp_max_link_rate(const int vbt_max_link_rate)
> >  	}
> >  }
> >
> > +static bool parse_bdb_263_edp_data_override(const u32
> > +edp_data_override_mask, int rate)
> 
> We don't really use const like this anywhere.

Sure will remove this

> 
> > +{
> > +	u32 val;
> > +
> > +	switch (rate) {
> > +	case 2000000:
> > +		val = BDB_263_VBT_EDP_LINK_RATE_20;
> > +		break;
> > +	case 1350000:
> > +		val = BDB_263_VBT_EDP_LINK_RATE_13_5;
> > +		break;
> > +	case 1000000:
> > +		val = BDB_263_VBT_EDP_LINK_RATE_10;
> > +		break;
> > +	case 810000:
> > +		val = BDB_263_VBT_EDP_LINK_RATE_8_1;
> > +		break;
> > +	case 675000:
> > +		val = BDB_263_VBT_EDP_LINK_RATE_6_75;
> > +		break;
> > +	case 540000:
> > +		val = BDB_263_VBT_EDP_LINK_RATE_5_4;
> > +		break;
> > +	case 432000:
> > +		val = BDB_263_VBT_EDP_LINK_RATE_4_32;
> > +		break;
> > +	case 324000:
> > +		val = BDB_263_VBT_EDP_LINK_RATE_3_24;
> > +		break;
> > +	case 270000:
> > +		val = BDB_263_VBT_EDP_LINK_RATE_2_7;
> > +		break;
> > +	case 243000:
> > +		val = BDB_263_VBT_EDP_LINK_RATE_2_43;
> > +		break;
> > +	case 216000:
> > +		val = BDB_263_VBT_EDP_LINK_RATE_2_16;
> > +		break;
> > +	case 162000:
> > +		val = BDB_263_VBT_EDP_LINK_RATE_1_62;
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	return edp_data_override_mask & val;
> 
> I think you're better off having a helper function that returns the mask for a
> rate, possibly 0, that you don't pass edp_data_override_mask at all.
> 

Sure will fix that

> > +}
> > +
> >  int intel_bios_dp_max_link_rate(const struct intel_bios_encoder_data
> > *devdata)  {
> >  	if (!devdata || devdata->display->vbt.version < 216) @@ -2499,6
> > +2547,16 @@ int intel_bios_dp_max_lane_count(const struct
> intel_bios_encoder_data *devdata)
> >  	return devdata->child.dp_max_lane_count + 1;  }
> >
> > +bool
> > +intel_bios_need_edp_data_override(const struct intel_bios_encoder_data
> *devdata,
> > +				  int rate)
> 
> intel_bios_encoder_supports_edp_rate() or something, "need edp data
> override" doesn't mean anything.

Sure

> 
> > +{
> > +	if (!devdata || devdata->display->vbt.version < 263)
> > +		return false;
> > +
> > +	return
> > +parse_bdb_263_edp_data_override(devdata->child.edp_data_override,
> > +rate);
> 
> This would become:
> 
> 	return devdata->child.edp_data_override &
> edp_rate_override_mask(rate);
> 
> except negated with the positive function naming.
> 
> Also child.edp_data_override is a misleading name, because it's about data
> *rate* override, not some random data override.

Sure will amend the name to edp_data_rate_override.

> 
> > +}
> > +
> >  static void sanitize_device_type(struct intel_bios_encoder_data *devdata,
> >  				 enum port port)
> >  {
> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.h
> > b/drivers/gpu/drm/i915/display/intel_bios.h
> > index 6cd7a011b8c4..aeba0e9b5228 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bios.h
> > +++ b/drivers/gpu/drm/i915/display/intel_bios.h
> > @@ -274,5 +274,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_need_edp_data_override(const struct
> > +intel_bios_encoder_data *devdata, int rate);
> 
> Place this next to the other intel_bios_encoder_supports_* declarations.
> 
> BR,
> Jani.
> 
> >
> >  #endif /* _INTEL_BIOS_H_ */
> 
> --
> Jani Nikula, Intel