Re: [PATCH v3 2/4] drm/edid: Add a function to check monitor string

[Hsin-Yi Wang]

On Mon, Mar 4, 2024 at 12:38 PM Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
>
> On Mon, 04 Mar 2024, Hsin-Yi Wang <hsinyi@xxxxxxxxxxxx> wrote:
> > Add a function to check if the EDID base block contains a given string.
> >
> > One of the use cases is fetching panel from a list of panel names, since
> > some panel vendors put the monitor name after EDID_DETAIL_MONITOR_STRING
> > instead of EDID_DETAIL_MONITOR_NAME.
> >
> > Signed-off-by: Hsin-Yi Wang <hsinyi@xxxxxxxxxxxx>
> > ---
> > v2->v3: move string matching to drm_edid
> > ---
> >  drivers/gpu/drm/drm_edid.c | 49 ++++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_edid.h     |  1 +
> >  2 files changed, 50 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 13454bc64ca2..fcdc2bd143dd 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -2789,6 +2789,55 @@ u32 drm_edid_get_panel_id(struct edid_base_block *base_block)
> >  }
> >  EXPORT_SYMBOL(drm_edid_get_panel_id);
> >
> > +/**
> > + * drm_edid_has_monitor_string - Check if a EDID base block has certain string.
> > + * @base_block: EDID base block to check.
> > + * @str: pointer to a character array to hold the string to be checked.
> > + *
> > + * Check if the detailed timings section of a EDID base block has the given
> > + * string.
> > + *
> > + * Return: True if the EDID base block contains the string, false otherwise.
> > + */
> > +bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str)
> > +{
> > +     unsigned int i, j, k, buflen = strlen(str);
> > +
> > +     for (i = 0; i < EDID_DETAILED_TIMINGS; i++) {
> > +             struct detailed_timing *timing = &base_block->edid.detailed_timings[i];
> > +             unsigned int size = ARRAY_SIZE(timing->data.other_data.data.str.str);
> > +
> > +             if (buflen > size || timing->pixel_clock != 0 ||
> > +                 timing->data.other_data.pad1 != 0 ||
> > +                 (timing->data.other_data.type != EDID_DETAIL_MONITOR_NAME &&
> > +                  timing->data.other_data.type != EDID_DETAIL_MONITOR_STRING))
> > +                     continue;
> > +
> > +             for (j = 0; j < buflen; j++) {
> > +                     char c = timing->data.other_data.data.str.str[j];
> > +
> > +                     if (c != str[j] ||  c == '\n')
> > +                             break;
> > +             }
> > +
> > +             if (j == buflen) {
> > +                     /* Allow trailing white spaces. */
> > +                     for (k = j; k < size; k++) {
> > +                             char c = timing->data.other_data.data.str.str[k];
> > +
> > +                             if (c == '\n')
> > +                                     return true;
> > +                             else if (c != ' ')
> > +                                     break;
> > +                     }
> > +                     if (k == size)
> > +                             return true;
> > +             }
> > +     }
> > +
> > +     return false;
> > +}
> > +
>
> So we've put a lot of effort into converting from struct edid to struct
> drm_edid, passing that around in drm_edid.c, with the allocation size it
> provides, and generally cleaning stuff up.
>
> I'm not at all happy to see *another* struct added just for the base
> block, and detailed timing iteration as well as monitor name parsing
> duplicated.
>
> With struct drm_edid you can actually return an EDID that only has the
> base block and size 128, even if the EDID indicates more
> extensions. Because the whole thing is *designed* to handle that
> gracefully. The allocated size matters, not what the blob originating
> outside of the kernel tells you.
>
> What I'm thinking is:
>
> - Add some struct drm_edid_ident or similar. Add all the information
>   that's needed to identify a panel there. I guess initially that's
>   panel_id and name.
>
>     struct drm_edid_ident {
>         u32 panel_id;
>         const char *name;
>     };
>
> - Add function:
>
>     bool drm_edid_match(const struct drm_edid *drm_edid, const struct drm_edid_ident *ident);
>
>   Check if stuff in ident matches drm_edid. You can use and extend the
>   existing drm_edid based iteration etc. in
>   drm_edid.c. Straightforward. The fields in ident can trivially be
>   extended later, and the stuff can be useful for other drivers and
>   quirks etc.
>
> - Restructure struct edp_panel_entry to contain struct
>   drm_edid_ident. Change the iteration of edp_panels array to use
>   drm_edid_match() on the array elements and the edid.
>
> - Add a function to read the EDID base block *but* make it return const
>   struct drm_edid *. Add warnings in the comments that it's only for
>   panel and for transition until it switches to reading full EDIDs.
>
>     const struct drm_edid *drm_edid_read_base_block(struct i2c_adapter *adapter);
>
>   This is the *only* hackish part of the whole thing, and it's nicely
>   isolated. For the most part you can use drm_edid_get_panel_id() code
>   for this, just return the blob wrapped in a struct drm_edid envelope.

To clarify:
struct drm_edid currently is only internal to drm_edid.c. So with
change we will have to move it to the header drm_edid.h

>
> - Remove function:
>
>     u32 drm_edid_get_panel_id(struct i2c_adapter *adapter);
>

Probably change to u32 drm_edid_get_panel_id(const struct drm_edid
*);? Given that we still need to parse id from
drm_edid_read_base_block().

> - Refactor edid_quirk_list to use the same id struct and match function
>   and mechanism within drm_edid.c (can be follow-up too).
>

edid_quirk currently doesn't have panel names in it, and it might be a
bit difficult to get all the correct names of these panels without
having the datasheets.
One way is to leave the name as null and if the name is empty and skip
matching the name in drm_edid_match().

> - Once you change the panel code to read the whole EDID using
>   drm_edid_read family of functions in the future, you don't have to
>   change *anything* about the iteration or matching or anything, because
>   it's already passing struct drm_edid around.
>
>
> I hope this covers everything.
>
> BR,
> Jani.
>
>
> >  /**
> >   * drm_edid_get_base_block - Get a panel's EDID base block
> >   * @adapter: I2C adapter to use for DDC
> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> > index 2455d6ab2221..248ddb0a6b5d 100644
> > --- a/include/drm/drm_edid.h
> > +++ b/include/drm/drm_edid.h
> > @@ -416,6 +416,7 @@ struct edid *drm_get_edid(struct drm_connector *connector,
> >                         struct i2c_adapter *adapter);
> >  struct edid_base_block *drm_edid_get_base_block(struct i2c_adapter *adapter);
> >  u32 drm_edid_get_panel_id(struct edid_base_block *base_block);
> > +bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str);
> >  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