On Mon, 04 Mar 2024, Hsin-Yi Wang <hsi...@chromium.org> wrote:
> On Mon, Mar 4, 2024 at 12:38 PM Jani Nikula <jani.nik...@linux.intel.com>
> wrote:
>>
>> On Mon, 04 Mar 2024, Hsin-Yi Wang <hsi...@chromium.org> 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 <hsi...@chromium.org>
>> > ---
>> > 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
Absolutely not, struct drm_edid must remain an opaque type. The point is
that you ask drm_edid.c if there's a match or not, and the panel code
does not need to care what's inside struct drm_edid.
>
>>
>> - 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().
No, we no longer need to parse the id outside of drm_edid.c. You'll have
the id's in panel code in the form of struct drm_edid_ident (or
whatever), and use the match function to see if the opaque drm_edid
matches.
>
>> - 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().
Exactly. NULL in drm_edid_ident would mean "don't care". I think most of
the ones in panel code also won't use the name for matching.
BR,
Jani.
>
>> - 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
--
Jani Nikula, Intel
