[PATCH v4 2/5] drm/edid: Add a function to match EDID with identity

[Hsin-Yi Wang]

On Wed, Mar 6, 2024 at 1:17 AM Jani Nikula <jani.nikula at linux.intel.com> wrote:
>
> On Tue, 05 Mar 2024, Hsin-Yi Wang <hsinyi at chromium.org> wrote:
> > Create a type drm_edid_ident as the identity of an EDID. Currently it
> > contains panel id and monitor name.
> >
> > Create a function that can match a given EDID and an identity:
> > 1. Reject if the panel id doesn't match.
> > 2. If name is not null in identity, try to match it in the detailed timing
> >    blocks. Note that some panel vendors put the monitor name after
> >    EDID_DETAIL_MONITOR_STRING.
> >
> > Signed-off-by: Hsin-Yi Wang <hsinyi at chromium.org>
> > ---
> > v3->v4:
> > 1. add a type drm_edid_ident
> > 2. match name -> match identity. Modify function to use edid iterators.
> > ---
> >  drivers/gpu/drm/drm_edid.c | 76 ++++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_edid.h     |  8 ++++
> >  2 files changed, 84 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index f9e09f327f81..5e7e69e0e345 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -102,6 +102,11 @@ struct detailed_mode_closure {
> >       int modes;
> >  };
> >
> > +struct drm_edid_ident_closure {
> > +     const struct drm_edid_ident *ident;
> > +     bool matched;
> > +};
>
> More like drm_edid_match_closure.
>
> > +
> >  #define LEVEL_DMT    0
> >  #define LEVEL_GTF    1
> >  #define LEVEL_GTF2   2
> > @@ -5455,6 +5460,77 @@ drm_parse_hdmi_vsdb_audio(struct drm_connector *connector, const u8 *db)
> >                   connector->audio_latency[0], connector->audio_latency[1]);
> >  }
> >
> > +static void
> > +match_identity(const struct detailed_timing *timing, void *data)
> > +{
> > +     struct drm_edid_ident_closure *closure = data;
> > +     unsigned int i, j;
> > +     const char *str = closure->ident->name;
> > +     unsigned int buflen = strlen(str);
> > +     unsigned int size = ARRAY_SIZE(timing->data.other_data.data.str.str);
> > +
> > +     if (buflen > size ||
> > +         !(is_display_descriptor(timing, EDID_DETAIL_MONITOR_NAME) ||
> > +           is_display_descriptor(timing, EDID_DETAIL_MONITOR_STRING)))
> > +             return;
> > +
> > +     for (i = 0; i < buflen; i++) {
> > +             char c = timing->data.other_data.data.str.str[i];
> > +
> > +             if (c != str[i] ||  c == '\n')
> > +                     break;
> > +             }
> > +
> > +             if (i == buflen) {
>
> This will never be true.

It should be

for (i = 0; i < buflen; i++) {
...
}

if (i==buflen) {
...
}

But okay we can use strcmp.

>
> > +                     /* Allow trailing white spaces. */
> > +                     for (j = i; j < size; j++) {
> > +                             char c = timing->data.other_data.data.str.str[j];
> > +
> > +                             if (c == '\n') {
> > +                                     closure->matched = true;
> > +                                     return;
> > +                             } else if (c != ' ') {
> > +                                     break;
> > +                             }
> > +                     }
> > +                     if (j == size) {
> > +                             closure->matched = true;
> > +                             return;
> > +                     }
> > +     }
>
> Please let's use strcmp and friends instead of reinventing our own:
>
> const char *name = closure->ident->name;
> int name_len = strlen(name);
> const char *desc = timing->data.other_data.data.str.str;
> int desc_len = ARRAY_SIZE(timing->data.other_data.data.str.str);
>
> if (name_len > desc_len)
>         return;
>
> if (strncmp(name, desc, name_en))
>         return;
>
> for (i = name_len; i < desc_len; i++) {
>         if (!isspace(desc[i]) && !desc[i])
>                 return;
> }
>
> closure->matched = true;
>
>
> > +}
> > +
> > +/**
> > + * drm_edid_match_identity - match drm_edid with given identity
> > + * @drm_edid: EDID
> > + * @ident: the EDID identity to match with
> > + *
> > + * Check if the EDID matches with the given identity.
> > + *
> > + * Return: True if the given identity matched with EDID, false otherwise.
> > + */
> > +bool drm_edid_match_identity(const struct drm_edid *drm_edid,
> > +                          const struct drm_edid_ident *ident)
>
> Can we please just call this drm_edid_match()? Is the _identity in the
> name somehow helpful?
>
> > +{
> > +     if (!drm_edid || edid_extract_panel_id(drm_edid->edid) != ident->panel_id)
> > +             return false;
>
> Side note, edid_extract_panel_id() could now be made to take struct
> drm_edid.
>
> > +
> > +     /* Match with name only if it's not NULL. */
> > +     if (ident->name) {
> > +             struct drm_edid_ident_closure closure = {
> > +                     .ident = ident,
> > +                     .matched = false,
> > +             };
> > +
> > +             drm_for_each_detailed_block(drm_edid, match_identity, &closure);
> > +
> > +             return closure.matched;
> > +     }
> > +
> > +     return true;
> > +}
> > +EXPORT_SYMBOL(drm_edid_match_identity);
> > +
> >  static void
> >  monitor_name(const struct detailed_timing *timing, void *data)
> >  {
> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> > index 9686a7cee6a6..01825a8954b6 100644
> > --- a/include/drm/drm_edid.h
> > +++ b/include/drm/drm_edid.h
> > @@ -312,6 +312,12 @@ struct edid {
> >       u8 checksum;
> >  } __packed;
> >
> > +/* EDID matching */
> > +struct drm_edid_ident {
> > +     u32 panel_id;
> > +     const char *name;
> > +};
> > +
> >  #define EDID_PRODUCT_ID(e) ((e)->prod_code[0] | ((e)->prod_code[1] << 8))
> >
> >  /* Short Audio Descriptor */
> > @@ -412,6 +418,8 @@ struct edid *drm_get_edid(struct drm_connector *connector,
> >                         struct i2c_adapter *adapter);
> >  const struct drm_edid *drm_edid_read_base_block(struct i2c_adapter *adapter);
> >  u32 drm_edid_get_panel_id(const struct drm_edid *drm_edid);
> > +bool drm_edid_match_identity(const struct drm_edid *drm_edid,
> > +                          const struct drm_edid_ident *ident);
> >  struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
> >                                    struct i2c_adapter *adapter);
> >  struct edid *drm_edid_duplicate(const struct edid *edid);
>
> --
> Jani Nikula, Intel