On Wed, 04 Jun 2025, Imre Deak <imre.d...@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.syrj...@linux.intel.com>
> Cc: Jani Nikula <jani.nik...@linux.intel.com>
> Signed-off-by: Imre Deak <imre.d...@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
