Received-SPF: pass (google.com: domain of linux-kernel+bounces-94460-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240306004347.974304-1-hsinyi@chromium.org> <20240306004347.974304-3-hsinyi@chromium.org>
 <87sf13zpmc.fsf@intel.com>
In-Reply-To: <87sf13zpmc.fsf@intel.com>
From: Hsin-Yi Wang <hsinyi@chromium.org>
Date: Wed, 6 Mar 2024 11:08:27 -0800
Message-ID: <CAJMQK-gTuPRCJ2y4ErGcv6T74JgGjtzuWyeDxU2uX+=0+yYq9Q@mail.gmail.com>
Subject: Re: [PATCH v4 2/5] drm/edid: Add a function to match EDID with identity
To: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, Douglas Anderson <dianders@chromium.org>, 
	Neil Armstrong <neil.armstrong@linaro.org>, Jessica Zhang <quic_jesszhan@quicinc.com>, 
	Sam Ravnborg <sam@ravnborg.org>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, 
	Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>, 
	David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>, dri-devel@lists.freedesktop.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Mar 6, 2024 at 1:17=E2=80=AFAM Jani Nikula <jani.nikula@linux.intel=
com> wrote:
>
> On Tue, 05 Mar 2024, Hsin-Yi Wang <hsinyi@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 tim=
ing
> >    blocks. Note that some panel vendors put the monitor name after
> >    EDID_DETAIL_MONITOR_STRING.
> >
> > Signed-off-by: Hsin-Yi Wang <hsinyi@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 =3D data;
> > +     unsigned int i, j;
> > +     const char *str =3D closure->ident->name;
> > +     unsigned int buflen =3D strlen(str);
> > +     unsigned int size =3D 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 =3D 0; i < buflen; i++) {
> > +             char c =3D timing->data.other_data.data.str.str[i];
> > +
> > +             if (c !=3D str[i] ||  c =3D=3D '\n')
> > +                     break;
> > +             }
> > +
> > +             if (i =3D=3D buflen) {
>
> This will never be true.

It should be

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

if (i=3D=3Dbuflen) {
..
}

But okay we can use strcmp.

>
> > +                     /* Allow trailing white spaces. */
> > +                     for (j =3D i; j < size; j++) {
> > +                             char c =3D timing->data.other_data.data.s=
tr.str[j];
> > +
> > +                             if (c =3D=3D '\n') {
> > +                                     closure->matched =3D true;
> > +                                     return;
> > +                             } else if (c !=3D ' ') {
> > +                                     break;
> > +                             }
> > +                     }
> > +                     if (j =3D=3D size) {
> > +                             closure->matched =3D true;
> > +                             return;
> > +                     }
> > +     }
>
> Please let's use strcmp and friends instead of reinventing our own:
>
> const char *name =3D closure->ident->name;
> int name_len =3D strlen(name);
> const char *desc =3D timing->data.other_data.data.str.str;
> int desc_len =3D ARRAY_SIZE(timing->data.other_data.data.str.str);
>
> if (name_len > desc_len)
>         return;
>
> if (strncmp(name, desc, name_en))
>         return;
>
> for (i =3D name_len; i < desc_len; i++) {
>         if (!isspace(desc[i]) && !desc[i])
>                 return;
> }
>
> closure->matched =3D 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 otherwi=
se.
> > + */
> > +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) !=3D 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 =3D {
> > +                     .ident =3D ident,
> > +                     .matched =3D false,
> > +             };
> > +
> > +             drm_for_each_detailed_block(drm_edid, match_identity, &cl=
osure);
> > +
> > +             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 *con=
nector,
> >                         struct i2c_adapter *adapter);
> >  const struct drm_edid *drm_edid_read_base_block(struct i2c_adapter *ad=
apter);
> >  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