Re: [PATCH v4 2/5] drm/edid: Add a function to match EDID with identity
From: Hsin-Yi Wang
Date: Wed Mar 06 2024 - 14:09:10 EST
On Wed, Mar 6, 2024 at 1:17 AM Jani Nikula <jani.nikula@linux.intelcom> wrote:
>
> On Tue, 05 Mar 2024, Hsin-Yi Wang <hsinyi@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > ---
> > 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.strstr);
> > +
> > + 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