Re: [PATCH v2 2/4] drm/edid: Add support for quirks visible to DRM core and drivers

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

On Wed, 04 Jun 2025, Imre Deak <imre.deak@intel.com> wrote:
> Add support for EDID based quirks which can be queried outside of the
> EDID parser iteself by DRM core and drivers. There are at least two such
> quirks applicable to all drivers: the DPCD register access probe quirk
> and the 128b/132b DPRX Lane Count Conversion quirk (see 3.5.2.16.3 in
> the v2.1a DP Standard). The latter quirk applies to panels with specific
> EDID panel names, accordingly add support for defining quirks based on
> the EDID panel name.
>
> v2: Reset global_quirks in drm_reset_display_info().
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c  | 34 +++++++++++++++++++++++++++-------
>  include/drm/drm_connector.h |  5 +++++
>  include/drm/drm_edid.h      |  5 +++++
>  3 files changed, 37 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 74e77742b2bd4..5d3a25cbc4d36 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -114,15 +114,21 @@ struct drm_edid_match_closure {
>  #define LEVEL_GTF2	2
>  #define LEVEL_CVT	3
>  
> -#define EDID_QUIRK(vend_chr_0, vend_chr_1, vend_chr_2, product_id, _quirks) \
> +#define PANEL_NAME_ANY	NULL
> +
> +#define DRM_EDID_QUIRK(_panel_id, _panel_name, _quirks) \
>  { \
>  	.ident = { \
> -		.panel_id = drm_edid_encode_panel_id(vend_chr_0, vend_chr_1, \
> -						     vend_chr_2, product_id), \
> +		.panel_id = _panel_id, \
> +		.name = _panel_name, \
>  	}, \
>  	.quirks = _quirks \
>  }
>  
> +#define EDID_QUIRK(vend_chr_0, vend_chr_1, vend_chr_2, product_id, _quirks) \
> +	DRM_EDID_QUIRK(drm_edid_encode_panel_id(vend_chr_0, vend_chr_1, vend_chr_2, product_id), \
> +		       PANEL_NAME_ANY, _quirks)
> +

I'm not sure why this has to change. It's not explained in the commit
message.

>  static const struct edid_quirk {
>  	const struct drm_edid_ident ident;
>  	u32 quirks;
> @@ -248,6 +254,9 @@ static const struct edid_quirk {
>  	EDID_QUIRK('A', 'U', 'O', 0x1111, EDID_QUIRK_NON_DESKTOP),

Don't we want the other quirk list also be this concise?

>  };
>  
> +static const struct edid_quirk global_edid_quirk_list[] = {
> +};
> +
>  /*
>   * Autogenerated from the DMT spec.
>   * This table is copied from xfree86/modes/xf86EdidModes.c.
> @@ -2937,13 +2946,14 @@ EXPORT_SYMBOL(drm_edid_duplicate);
>   *
>   * Return: A u32 represents the quirks to apply.
>   */
> -static u32 edid_get_quirks(const struct drm_edid *drm_edid)
> +static u32 edid_get_quirks(const struct drm_edid *drm_edid,
> +			   const struct edid_quirk *quirk_list, int quirk_list_size)
>  {
>  	const struct edid_quirk *quirk;
>  	int i;
>  
> -	for (i = 0; i < ARRAY_SIZE(edid_quirk_list); i++) {
> -		quirk = &edid_quirk_list[i];
> +	for (i = 0; i < quirk_list_size; i++) {
> +		quirk = &quirk_list[i];
>  		if (drm_edid_match(drm_edid, &quirk->ident))
>  			return quirk->quirks;
>  	}
> @@ -6614,6 +6624,7 @@ static void drm_reset_display_info(struct drm_connector *connector)
>  	info->vics_len = 0;
>  
>  	info->quirks = 0;
> +	info->global_quirks = 0;

So I am not sure if we really need or want two lists.

I think we could have

drm_edid.h:

enum drm_edid_quirk {
	/* ... */
	DRM_EDID_QUIRK_MAX,
};

drm_edid.c:

enum drm_edid_internal_quirk {
	FOO_QUIRK = DRM_EDID_QUIRK_MAX,
        /* etc ... */
};

And just make info->quirks big enough. I think it feels simpler overall.

>  
>  	info->source_physical_address = CEC_PHYS_ADDR_INVALID;
>  }
> @@ -6660,7 +6671,10 @@ static void update_display_info(struct drm_connector *connector,
>  
>  	edid = drm_edid->edid;
>  
> -	info->quirks = edid_get_quirks(drm_edid);
> +	info->quirks = edid_get_quirks(drm_edid, edid_quirk_list,
> +				       ARRAY_SIZE(edid_quirk_list));
> +	info->global_quirks = edid_get_quirks(drm_edid, global_edid_quirk_list,
> +					      ARRAY_SIZE(global_edid_quirk_list));
>  
>  	info->width_mm = edid->width_cm * 10;
>  	info->height_mm = edid->height_cm * 10;
> @@ -7566,3 +7580,9 @@ bool drm_edid_is_digital(const struct drm_edid *drm_edid)
>  		drm_edid->edid->input & DRM_EDID_INPUT_DIGITAL;
>  }
>  EXPORT_SYMBOL(drm_edid_is_digital);
> +
> +bool drm_edid_has_quirk(struct drm_connector *connector, enum drm_edid_quirk quirk)
> +{
> +	return connector->display_info.global_quirks & BIT(quirk);
> +}
> +EXPORT_SYMBOL(drm_edid_has_quirk);
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 73903c3c842f3..996ecf229f8c7 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -847,6 +847,11 @@ struct drm_display_info {
>  	 */
>  	u32 quirks;
>  
> +	/**
> +	 * @global_quirks: EDID based quirks. Can be queried by DRM core and drivers.

Might mention that you should not access directly, but using
drm_edid_has_quirk().

> +	 */
> +	u32 global_quirks;
> +
>  	/**
>  	 * @source_physical_address: Source Physical Address from HDMI
>  	 * Vendor-Specific Data Block, for CEC usage.
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index b38409670868d..3d8e168521c82 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -109,6 +109,10 @@ struct detailed_data_string {
>  #define DRM_EDID_CVT_FLAGS_STANDARD_BLANKING (1 << 3)
>  #define DRM_EDID_CVT_FLAGS_REDUCED_BLANKING  (1 << 4)
>  
> +enum drm_edid_quirk {
> +	DRM_EDID_QUIRK_NONE,
> +};
> +
>  struct detailed_data_monitor_range {
>  	u8 min_vfreq;
>  	u8 max_vfreq;
> @@ -476,5 +480,6 @@ void drm_edid_print_product_id(struct drm_printer *p,
>  u32 drm_edid_get_panel_id(const struct drm_edid *drm_edid);
>  bool drm_edid_match(const struct drm_edid *drm_edid,
>  		    const struct drm_edid_ident *ident);
> +bool drm_edid_has_quirk(struct drm_connector *connector, enum drm_edid_quirk quirk);
>  
>  #endif /* __DRM_EDID_H__ */

-- 
Jani Nikula, Intel