Re: [PATCH v3 2/4] drm/edid: Add a function to check monitor string
On Tue, 05 Mar 2024, Hsin-Yi Wang wrote: > On Tue, Mar 5, 2024 at 11:25 AM Doug Anderson wrote: >> Hmm. As Hsin-Yi pointed out to me offline. Somehow we'll need to get >> the actual panel ID out. Right now in panel-edp.c we have: >> >> dev_warn(dev, >> "Unknown panel %s %#06x, using conservative timings\n", >> vend, product_id); >> >> Where "vend" and "product_id" come from the panel ID of a panel that >> we didn't recognize. For instance: >> >> Unknown panel BOE 0x0731, using conservative timings >> >> We need to still be able to print this message for unrecognized >> panels. Then when we see field reports including this message we know >> that somehow we ended up shipping an unrecognized panel. >> >> Any suggestions on what abstraction you'd like to see to enable us to >> print that message if everything is opaque? > > Sent v4 here: > https://lore.kernel.org/lkml/20240306004347.974304-1-hsi...@chromium.org/ > > Besides that it still keeps drm_edid_get_panel_id() to be used on the > kernel warning when no panel is matched, other parts I think are > following the comments. Yeah we can keep that for now. BR, Jani. -- Jani Nikula, Intel
Re: [PATCH v3 2/4] drm/edid: Add a function to check monitor string
On Tue, Mar 5, 2024 at 11:25 AM Doug Anderson wrote: > > Hi, > > On Tue, Mar 5, 2024 at 12:17 AM Jani Nikula > wrote: > > > > On Mon, 04 Mar 2024, Doug Anderson wrote: > > > Hi, > > > > > > On Mon, Mar 4, 2024 at 4:19 PM Hsin-Yi Wang wrote: > > >> > > >> > > 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. > > >> > > > >> drm_panel prints the panel_id info on whether the panel is detected or > > >> not. > > >> https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/gpu/drm/panel/panel-edp.c#L792 > > >> > > >> Is it okay to remove this information? > > > > > > Hmmm, I guess it also is exported via debugfs, actually. See > > > detected_panel_show() in panel-edp.c. We probably don't want to remove > > > that... > > > > You currently print the information via panel->detected_panel, which is > > a struct edp_panel_entry *. That doesn't change. It'll be slightly > > restructured to contain a struct drm_edid_ident, which will not be an > > opaque type. > > Hmm. As Hsin-Yi pointed out to me offline. Somehow we'll need to get > the actual panel ID out. Right now in panel-edp.c we have: > > dev_warn(dev, > "Unknown panel %s %#06x, using conservative timings\n", > vend, product_id); > > Where "vend" and "product_id" come from the panel ID of a panel that > we didn't recognize. For instance: > > Unknown panel BOE 0x0731, using conservative timings > > We need to still be able to print this message for unrecognized > panels. Then when we see field reports including this message we know > that somehow we ended up shipping an unrecognized panel. > > Any suggestions on what abstraction you'd like to see to enable us to > print that message if everything is opaque? Sent v4 here: https://lore.kernel.org/lkml/20240306004347.974304-1-hsi...@chromium.org/ Besides that it still keeps drm_edid_get_panel_id() to be used on the kernel warning when no panel is matched, other parts I think are following the comments. Thanks. > > -Doug
Re: [PATCH v3 2/4] drm/edid: Add a function to check monitor string
Hi, On Tue, Mar 5, 2024 at 12:17 AM Jani Nikula wrote: > > On Mon, 04 Mar 2024, Doug Anderson wrote: > > Hi, > > > > On Mon, Mar 4, 2024 at 4:19 PM Hsin-Yi Wang wrote: > >> > >> > > 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. > >> > > >> drm_panel prints the panel_id info on whether the panel is detected or not. > >> https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/gpu/drm/panel/panel-edp.c#L792 > >> > >> Is it okay to remove this information? > > > > Hmmm, I guess it also is exported via debugfs, actually. See > > detected_panel_show() in panel-edp.c. We probably don't want to remove > > that... > > You currently print the information via panel->detected_panel, which is > a struct edp_panel_entry *. That doesn't change. It'll be slightly > restructured to contain a struct drm_edid_ident, which will not be an > opaque type. Hmm. As Hsin-Yi pointed out to me offline. Somehow we'll need to get the actual panel ID out. Right now in panel-edp.c we have: dev_warn(dev, "Unknown panel %s %#06x, using conservative timings\n", vend, product_id); Where "vend" and "product_id" come from the panel ID of a panel that we didn't recognize. For instance: Unknown panel BOE 0x0731, using conservative timings We need to still be able to print this message for unrecognized panels. Then when we see field reports including this message we know that somehow we ended up shipping an unrecognized panel. Any suggestions on what abstraction you'd like to see to enable us to print that message if everything is opaque? -Doug
Re: [PATCH v3 2/4] drm/edid: Add a function to check monitor string
On Mon, 04 Mar 2024, Doug Anderson wrote: > Hi, > > On Mon, Mar 4, 2024 at 4:19 PM Hsin-Yi Wang wrote: >> >> > > 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. >> > >> drm_panel prints the panel_id info on whether the panel is detected or not. >> https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/gpu/drm/panel/panel-edp.c#L792 >> >> Is it okay to remove this information? > > Hmmm, I guess it also is exported via debugfs, actually. See > detected_panel_show() in panel-edp.c. We probably don't want to remove > that... You currently print the information via panel->detected_panel, which is a struct edp_panel_entry *. That doesn't change. It'll be slightly restructured to contain a struct drm_edid_ident, which will not be an opaque type. BR, Jani. -- Jani Nikula, Intel
Re: [PATCH v3 2/4] drm/edid: Add a function to check monitor string
On Mon, 04 Mar 2024, Hsin-Yi Wang wrote: > On Mon, Mar 4, 2024 at 4:09 PM Jani Nikula > wrote: >> >> On Mon, 04 Mar 2024, Hsin-Yi Wang wrote: >> > 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. >> > > Sorry I might be misunderstanding about the requests here: > > If drm_edid should remain opaque, then struct drm_edid remains opaque, > drm_edid_match() should take struct edid *edid as a parameter? just as > other exposed functions in drm_edid. No, it should take struct drm_edid *. > If panel edp doesn't hold drm_edid returned from > drm_edid_read_base_block(), what should it use to iterate the > edp_panels array? Panel edp can hold a *pointer* to struct drm_edid * without knowing the full type. This is one of the points of struct drm_edid. Focus more of the EDID parsing within drm_edid.c instead of having everyone parse it to varying degrees of correctness. > > for (panel = edp_panels; panel->panel_id; panel++) > if(drm_edid_match(drm_edid, panel->ident)) > ... > BR, Jani. -- Jani Nikula, Intel
Re: [PATCH v3 2/4] drm/edid: Add a function to check monitor string
On Mon, Mar 4, 2024 at 4:09 PM Jani Nikula wrote:
>
> On Mon, 04 Mar 2024, Hsin-Yi Wang wrote:
> > On Mon, Mar 4, 2024 at 12:38 PM Jani Nikula
> > wrote:
> >>
> >> On Mon, 04 Mar 2024, Hsin-Yi Wang 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
> >> > ---
> >> > 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
Re: [PATCH v3 2/4] drm/edid: Add a function to check monitor string
Hi, On Mon, Mar 4, 2024 at 4:19 PM Hsin-Yi Wang wrote: > > > > 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. > > > drm_panel prints the panel_id info on whether the panel is detected or not. > https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/gpu/drm/panel/panel-edp.c#L792 > > Is it okay to remove this information? Hmmm, I guess it also is exported via debugfs, actually. See detected_panel_show() in panel-edp.c. We probably don't want to remove that...
Re: [PATCH v3 2/4] drm/edid: Add a function to check monitor string
On Mon, Mar 4, 2024 at 4:09 PM Jani Nikula wrote:
>
> On Mon, 04 Mar 2024, Hsin-Yi Wang wrote:
> > On Mon, Mar 4, 2024 at 12:38 PM Jani Nikula
> > wrote:
> >>
> >> On Mon, 04 Mar 2024, Hsin-Yi Wang 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
> >> > ---
> >> > 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
Re: [PATCH v3 2/4] drm/edid: Add a function to check monitor string
On Mon, 04 Mar 2024, Hsin-Yi Wang wrote:
> On Mon, Mar 4, 2024 at 12:38 PM Jani Nikula
> wrote:
>>
>> On Mon, 04 Mar 2024, Hsin-Yi Wang 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
>> > ---
>> > 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
Re: [PATCH v3 2/4] drm/edid: Add a function to check monitor string
On Mon, 4 Mar 2024 at 22:38, Jani Nikula wrote:
>
> On Mon, 04 Mar 2024, Hsin-Yi Wang 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
> > ---
> > 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.
That sounds perfect!
>
> - 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
Re: [PATCH v3 2/4] drm/edid: Add a function to check monitor string
On Mon, Mar 4, 2024 at 12:38 PM Jani Nikula wrote:
>
> On Mon, 04 Mar 2024, Hsin-Yi Wang 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
> > ---
> > 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
Re: [PATCH v3 2/4] drm/edid: Add a function to check monitor string
On Mon, 04 Mar 2024, Hsin-Yi Wang 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
> ---
> 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.
- Remove function:
u32 drm_edid_get_panel_id(struct i2c_adapter *adapter);
- Refactor edid_quirk_list to use the same id struct and match function
and mechanism within drm_edid.c (can b
