On Wed, Jun 04, 2025 at 01:02:57PM +0300, Jani Nikula wrote:
> 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.
The reason for the change is to add a way to define quirks based on the
panel name, also referred to in the commit message. I can drop it from
this patchset, adding it only later.
>
> > 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?
The quirk defined by the standard is panel name based. Imo the new macro
to add a quirk should allow for matching any panel ID _or_ any panel
name and I came up with the way above. But I'll drop it from this
patchset and use only EDID_QUIRK adding the DPCD probe quirk needed for
now.
> > };
> >
> > +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.
The internal quirks are not listed in an enum. That conversion is a
bigger change, but I agree it's a better way to define quirks. Did that
now, will follow up with it, this also allows using a single quirk list
as you suggested.
> >
> > 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().
Ok, will add a comment to quirks about this.
> > + */
> > + 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
