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

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

On Fri, 25 Jul 2025, Suraj Kandpal <suraj.kandpal@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@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.

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

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

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

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