| Message ID | 20240304195214.14563-3-hsinyi@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Match panel with id and name | expand |
On Mon, 04 Mar 2024, Hsin-Yi Wang <hsinyi@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 <hsinyi@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. - 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 be follow-up too). - 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);
On Mon, Mar 4, 2024 at 12:38 PM Jani Nikula <jani.nikula@linux.intel.com> wrote: > > On Mon, 04 Mar 2024, Hsin-Yi Wang <hsinyi@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 <hsinyi@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 > > - 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
On Mon, 4 Mar 2024 at 22:38, Jani Nikula <jani.nikula@linux.intel.com> wrote: > > On Mon, 04 Mar 2024, Hsin-Yi Wang <hsinyi@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 <hsinyi@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. 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. 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 be follow-up too). I wonder if we can take one step further and merge edp_panels to edid_quirk_list (as one of the follow-ups). Maybe just some bits of it. > - 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
On Mon, 04 Mar 2024, Hsin-Yi Wang <hsinyi@chromium.org> wrote: > On Mon, Mar 4, 2024 at 12:38 PM Jani Nikula <jani.nikula@linux.intel.com> wrote: >> >> On Mon, 04 Mar 2024, Hsin-Yi Wang <hsinyi@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 <hsinyi@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
On Mon, Mar 4, 2024 at 4:09 PM Jani Nikula <jani.nikula@linux.intel.com> wrote: > > On Mon, 04 Mar 2024, Hsin-Yi Wang <hsinyi@chromium.org> wrote: > > On Mon, Mar 4, 2024 at 12:38 PM Jani Nikula <jani.nikula@linux.intel.com> wrote: > >> > >> On Mon, 04 Mar 2024, Hsin-Yi Wang <hsinyi@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 <hsinyi@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. > 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? > > > >> - 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
Hi, On Mon, Mar 4, 2024 at 4:19 PM Hsin-Yi Wang <hsinyi@chromium.org> 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...
On Mon, Mar 4, 2024 at 4:09 PM Jani Nikula <jani.nikula@linux.intel.com> wrote: > > On Mon, 04 Mar 2024, Hsin-Yi Wang <hsinyi@chromium.org> wrote: > > On Mon, Mar 4, 2024 at 12:38 PM Jani Nikula <jani.nikula@linux.intel.com> wrote: > >> > >> On Mon, 04 Mar 2024, Hsin-Yi Wang <hsinyi@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 <hsinyi@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. > 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. 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? for (panel = edp_panels; panel->panel_id; panel++) if(drm_edid_match(drm_edid, panel->ident)) ... > > > >> > >> - 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
On Mon, 04 Mar 2024, Hsin-Yi Wang <hsinyi@chromium.org> wrote: > On Mon, Mar 4, 2024 at 4:09 PM Jani Nikula <jani.nikula@linux.intel.com> wrote: >> >> On Mon, 04 Mar 2024, Hsin-Yi Wang <hsinyi@chromium.org> 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.
On Mon, 04 Mar 2024, Doug Anderson <dianders@chromium.org> wrote: > Hi, > > On Mon, Mar 4, 2024 at 4:19 PM Hsin-Yi Wang <hsinyi@chromium.org> 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.
Hi, On Tue, Mar 5, 2024 at 12:17 AM Jani Nikula <jani.nikula@linux.intel.com> wrote: > > On Mon, 04 Mar 2024, Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > > > On Mon, Mar 4, 2024 at 4:19 PM Hsin-Yi Wang <hsinyi@chromium.org> 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
On Tue, Mar 5, 2024 at 11:25 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Tue, Mar 5, 2024 at 12:17 AM Jani Nikula <jani.nikula@linux.intel.com> wrote: > > > > On Mon, 04 Mar 2024, Doug Anderson <dianders@chromium.org> wrote: > > > Hi, > > > > > > On Mon, Mar 4, 2024 at 4:19 PM Hsin-Yi Wang <hsinyi@chromium.org> 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-hsinyi@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
On Tue, 05 Mar 2024, Hsin-Yi Wang <hsinyi@chromium.org> wrote: > On Tue, Mar 5, 2024 at 11:25 AM Doug Anderson <dianders@chromium.org> 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-hsinyi@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.
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; +} + /** * 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);
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@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(+)