| Message ID | 20240306200353.1436198-4-hsinyi@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Match panel with identity | expand |
Hi, On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote: > > +static void > +match_identity(const struct detailed_timing *timing, void *data) > +{ > + struct drm_edid_match_closure *closure = data; > + unsigned int i; > + const char *name = closure->ident->name; > + unsigned int name_len = strlen(name); > + const char *desc = timing->data.other_data.data.str.str; > + unsigned int desc_len = ARRAY_SIZE(timing->data.other_data.data.str.str); > + > + if (name_len > desc_len || > + !(is_display_descriptor(timing, EDID_DETAIL_MONITOR_NAME) || > + is_display_descriptor(timing, EDID_DETAIL_MONITOR_STRING))) > + return; > + > + if (strncmp(name, desc, name_len)) > + return; > + > + /* Allow trailing white spaces and \0. */ > + for (i = name_len; i < desc_len; i++) { > + if (desc[i] == '\n') > + break; > + if (!isspace(desc[i]) && !desc[i]) > + return; > + } If my code analysis is correct, I think you'll reject the case where: name = "foo" desc[13] = "foo \0zzzzzzzz" ...but you'll accept these cases: desc[13] = "foo \nzzzzzzzz" desc[13] = "foo \0\0\0\0\0\0\0\0\0" It somehow seems weird to me that a '\n' terminates the string but not a '\0'. I would have done: for (i = name_len; i < desc_len; i++) { /* Consider \n or \0 to terminate the string */ if (desc[i] == '\n' || desc[i] == '\0') break; /* OK for spaces at the end, but non-space is a fail */ if (!isspace(desc[i])) return; } > @@ -367,6 +367,12 @@ struct edid { > u8 checksum; > } __attribute__((packed)); > > +/* EDID matching */ > +struct drm_edid_ident { > + u32 panel_id; > + const char *name; Might not hurt to have a comment for panel_id saying that it's encoded by drm_edid_encode_panel_id() so it's obvious what this random u32 is. -Doug
On Wed, Mar 6, 2024 at 3:30 PM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote: > > > > +static void > > +match_identity(const struct detailed_timing *timing, void *data) > > +{ > > + struct drm_edid_match_closure *closure = data; > > + unsigned int i; > > + const char *name = closure->ident->name; > > + unsigned int name_len = strlen(name); > > + const char *desc = timing->data.other_data.data.str.str; > > + unsigned int desc_len = ARRAY_SIZE(timing->data.other_data.data.str.str); > > + > > + if (name_len > desc_len || > > + !(is_display_descriptor(timing, EDID_DETAIL_MONITOR_NAME) || > > + is_display_descriptor(timing, EDID_DETAIL_MONITOR_STRING))) > > + return; > > + > > + if (strncmp(name, desc, name_len)) > > + return; > > + > > + /* Allow trailing white spaces and \0. */ > > + for (i = name_len; i < desc_len; i++) { > > + if (desc[i] == '\n') > > + break; > > + if (!isspace(desc[i]) && !desc[i]) > > + return; > > + } > > If my code analysis is correct, I think you'll reject the case where: > > name = "foo" > desc[13] = "foo \0zzzzzzzz" > > ...but you'll accept these cases: > > desc[13] = "foo \nzzzzzzzz" > desc[13] = "foo \0\0\0\0\0\0\0\0\0" > > It somehow seems weird to me that a '\n' terminates the string but not a '\0'. I'm also not sure about \0... based on https://git.linuxtv.org/edid-decode.git/tree/parse-base-block.cpp#n493, they use \n as terminator. Maybe we should also reject \0 before\n? Since it's not printable. > > I would have done: > > for (i = name_len; i < desc_len; i++) { > /* Consider \n or \0 to terminate the string */ > if (desc[i] == '\n' || desc[i] == '\0') > break; > /* OK for spaces at the end, but non-space is a fail */ > if (!isspace(desc[i])) > return; > } > > > > @@ -367,6 +367,12 @@ struct edid { > > u8 checksum; > > } __attribute__((packed)); > > > > +/* EDID matching */ > > +struct drm_edid_ident { > > + u32 panel_id; > > + const char *name; > > Might not hurt to have a comment for panel_id saying that it's encoded > by drm_edid_encode_panel_id() so it's obvious what this random u32 is. > > > -Doug
Hi, On Wed, Mar 6, 2024 at 4:20 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote: > > On Wed, Mar 6, 2024 at 3:30 PM Doug Anderson <dianders@chromium.org> wrote: > > > > Hi, > > > > On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote: > > > > > > +static void > > > +match_identity(const struct detailed_timing *timing, void *data) > > > +{ > > > + struct drm_edid_match_closure *closure = data; > > > + unsigned int i; > > > + const char *name = closure->ident->name; > > > + unsigned int name_len = strlen(name); > > > + const char *desc = timing->data.other_data.data.str.str; > > > + unsigned int desc_len = ARRAY_SIZE(timing->data.other_data.data.str.str); > > > + > > > + if (name_len > desc_len || > > > + !(is_display_descriptor(timing, EDID_DETAIL_MONITOR_NAME) || > > > + is_display_descriptor(timing, EDID_DETAIL_MONITOR_STRING))) > > > + return; > > > + > > > + if (strncmp(name, desc, name_len)) > > > + return; > > > + > > > + /* Allow trailing white spaces and \0. */ > > > + for (i = name_len; i < desc_len; i++) { > > > + if (desc[i] == '\n') > > > + break; > > > + if (!isspace(desc[i]) && !desc[i]) > > > + return; > > > + } > > > > If my code analysis is correct, I think you'll reject the case where: > > > > name = "foo" > > desc[13] = "foo \0zzzzzzzz" > > > > ...but you'll accept these cases: > > > > desc[13] = "foo \nzzzzzzzz" > > desc[13] = "foo \0\0\0\0\0\0\0\0\0" > > > > It somehow seems weird to me that a '\n' terminates the string but not a '\0'. > > I'm also not sure about \0... based on > https://git.linuxtv.org/edid-decode.git/tree/parse-base-block.cpp#n493, > they use \n as terminator. Maybe we should also reject \0 before\n? > Since it's not printable. Ah, OK. I guess the EDID spec simply doesn't allow for '\0' in there. I guess in that case I'd prefer simply removing the code to handle '\0' instead of treating it like space until we see some actual need for it. So just get rid of the "!desc[i]" case? -Doug
On Wed, 06 Mar 2024, Doug Anderson <dianders@chromium.org> wrote: > Hi, > > On Wed, Mar 6, 2024 at 4:20 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote: >> >> On Wed, Mar 6, 2024 at 3:30 PM Doug Anderson <dianders@chromium.org> wrote: >> > >> > Hi, >> > >> > On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote: >> > > >> > > +static void >> > > +match_identity(const struct detailed_timing *timing, void *data) >> > > +{ >> > > + struct drm_edid_match_closure *closure = data; >> > > + unsigned int i; >> > > + const char *name = closure->ident->name; >> > > + unsigned int name_len = strlen(name); >> > > + const char *desc = timing->data.other_data.data.str.str; >> > > + unsigned int desc_len = ARRAY_SIZE(timing->data.other_data.data.str.str); >> > > + >> > > + if (name_len > desc_len || >> > > + !(is_display_descriptor(timing, EDID_DETAIL_MONITOR_NAME) || >> > > + is_display_descriptor(timing, EDID_DETAIL_MONITOR_STRING))) >> > > + return; >> > > + >> > > + if (strncmp(name, desc, name_len)) >> > > + return; >> > > + >> > > + /* Allow trailing white spaces and \0. */ >> > > + for (i = name_len; i < desc_len; i++) { >> > > + if (desc[i] == '\n') >> > > + break; >> > > + if (!isspace(desc[i]) && !desc[i]) >> > > + return; >> > > + } >> > >> > If my code analysis is correct, I think you'll reject the case where: >> > >> > name = "foo" >> > desc[13] = "foo \0zzzzzzzz" >> > >> > ...but you'll accept these cases: >> > >> > desc[13] = "foo \nzzzzzzzz" >> > desc[13] = "foo \0\0\0\0\0\0\0\0\0" >> > >> > It somehow seems weird to me that a '\n' terminates the string but not a '\0'. >> >> I'm also not sure about \0... based on >> https://git.linuxtv.org/edid-decode.git/tree/parse-base-block.cpp#n493, >> they use \n as terminator. Maybe we should also reject \0 before\n? >> Since it's not printable. > > Ah, OK. I guess the EDID spec simply doesn't allow for '\0' in there. > I guess in that case I'd prefer simply removing the code to handle > '\0' instead of treating it like space until we see some actual need > for it. So just get rid of the "!desc[i]" case? The spec text, similar for both EDID_DETAIL_MONITOR_NAME and EDID_DETAIL_MONITOR_STRING: Up to 13 alphanumeric characters (using ASCII codes) may be used to define the model name of the display product. The data shall be sequenced such that the 1st byte (ASCII code) = the 1st character, the 2nd byte (ASCII code) = the 2nd character, etc. If there are less than 13 characters in the string, then terminate the display product name string with ASCII code ‘0Ah’ (line feed) and pad the unused bytes in the field with ASCII code ‘20h’ (space). In theory, only checking for '\n' for termination should be enough, and this is what drm_edid_get_monitor_name() does. If there's a space *before* that, it should be considered part of the name, and not ignored. (So my suggestion in reply to the previous version is wrong.) However, since the match name uses NUL termination, maybe we should ignore NULs *before* '\n'? Like so: for (i = name_len; i < desc_len; i++) { if (desc[i] == '\n') break; if (!desc[i]) return; } BR, Jani. > > -Doug
On Thu, Mar 7, 2024 at 5:20 AM Jani Nikula <jani.nikula@linux.intel.com> wrote: > > On Wed, 06 Mar 2024, Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > > > On Wed, Mar 6, 2024 at 4:20 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote: > >> > >> On Wed, Mar 6, 2024 at 3:30 PM Doug Anderson <dianders@chromium.org> wrote: > >> > > >> > Hi, > >> > > >> > On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote: > >> > > > >> > > +static void > >> > > +match_identity(const struct detailed_timing *timing, void *data) > >> > > +{ > >> > > + struct drm_edid_match_closure *closure = data; > >> > > + unsigned int i; > >> > > + const char *name = closure->ident->name; > >> > > + unsigned int name_len = strlen(name); > >> > > + const char *desc = timing->data.other_data.data.str.str; > >> > > + unsigned int desc_len = ARRAY_SIZE(timing->data.other_data.data.str.str); > >> > > + > >> > > + if (name_len > desc_len || > >> > > + !(is_display_descriptor(timing, EDID_DETAIL_MONITOR_NAME) || > >> > > + is_display_descriptor(timing, EDID_DETAIL_MONITOR_STRING))) > >> > > + return; > >> > > + > >> > > + if (strncmp(name, desc, name_len)) > >> > > + return; > >> > > + > >> > > + /* Allow trailing white spaces and \0. */ > >> > > + for (i = name_len; i < desc_len; i++) { > >> > > + if (desc[i] == '\n') > >> > > + break; > >> > > + if (!isspace(desc[i]) && !desc[i]) > >> > > + return; > >> > > + } > >> > > >> > If my code analysis is correct, I think you'll reject the case where: > >> > > >> > name = "foo" > >> > desc[13] = "foo \0zzzzzzzz" > >> > > >> > ...but you'll accept these cases: > >> > > >> > desc[13] = "foo \nzzzzzzzz" > >> > desc[13] = "foo \0\0\0\0\0\0\0\0\0" > >> > > >> > It somehow seems weird to me that a '\n' terminates the string but not a '\0'. > >> > >> I'm also not sure about \0... based on > >> https://git.linuxtv.org/edid-decode.git/tree/parse-base-block.cpp#n493, > >> they use \n as terminator. Maybe we should also reject \0 before\n? > >> Since it's not printable. > > > > Ah, OK. I guess the EDID spec simply doesn't allow for '\0' in there. > > I guess in that case I'd prefer simply removing the code to handle > > '\0' instead of treating it like space until we see some actual need > > for it. So just get rid of the "!desc[i]" case? > > The spec text, similar for both EDID_DETAIL_MONITOR_NAME and > EDID_DETAIL_MONITOR_STRING: > > Up to 13 alphanumeric characters (using ASCII codes) may be used > to define the model name of the display product. The data shall > be sequenced such that the 1st byte (ASCII code) = the 1st > character, the 2nd byte (ASCII code) = the 2nd character, > etc. If there are less than 13 characters in the string, then > terminate the display product name string with ASCII code ‘0Ah’ > (line feed) and pad the unused bytes in the field with ASCII > code ‘20h’ (space). > > In theory, only checking for '\n' for termination should be enough, and > this is what drm_edid_get_monitor_name() does. If there's a space > *before* that, it should be considered part of the name, and not > ignored. (So my suggestion in reply to the previous version is wrong.) > > However, since the match name uses NUL termination, maybe we should > ignore NULs *before* '\n'? Like so: > > for (i = name_len; i < desc_len; i++) { > if (desc[i] == '\n') > break; > if (!desc[i]) > return; > } > Allow trailing white spaces so we don't need to add the trailing white space in edp_panel_entry. https://lore.kernel.org/lkml/CAA8EJpr7LHvqeGXhbFQ8KNn0LGDuv19cw0i04qVUz51TJeSQrA@mail.gmail.com/ > > BR, > Jani. > > > > > > -Doug > > -- > Jani Nikula, Intel
On Thu, 07 Mar 2024, Hsin-Yi Wang <hsinyi@chromium.org> wrote: > On Thu, Mar 7, 2024 at 5:20 AM Jani Nikula <jani.nikula@linux.intel.com> wrote: >> >> On Wed, 06 Mar 2024, Doug Anderson <dianders@chromium.org> wrote: >> > Hi, >> > >> > On Wed, Mar 6, 2024 at 4:20 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote: >> >> >> >> On Wed, Mar 6, 2024 at 3:30 PM Doug Anderson <dianders@chromium.org> wrote: >> >> > >> >> > Hi, >> >> > >> >> > On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote: >> >> > > >> >> > > +static void >> >> > > +match_identity(const struct detailed_timing *timing, void *data) >> >> > > +{ >> >> > > + struct drm_edid_match_closure *closure = data; >> >> > > + unsigned int i; >> >> > > + const char *name = closure->ident->name; >> >> > > + unsigned int name_len = strlen(name); >> >> > > + const char *desc = timing->data.other_data.data.str.str; >> >> > > + unsigned int desc_len = ARRAY_SIZE(timing->data.other_data.data.str.str); >> >> > > + >> >> > > + if (name_len > desc_len || >> >> > > + !(is_display_descriptor(timing, EDID_DETAIL_MONITOR_NAME) || >> >> > > + is_display_descriptor(timing, EDID_DETAIL_MONITOR_STRING))) >> >> > > + return; >> >> > > + >> >> > > + if (strncmp(name, desc, name_len)) >> >> > > + return; >> >> > > + >> >> > > + /* Allow trailing white spaces and \0. */ >> >> > > + for (i = name_len; i < desc_len; i++) { >> >> > > + if (desc[i] == '\n') >> >> > > + break; >> >> > > + if (!isspace(desc[i]) && !desc[i]) >> >> > > + return; >> >> > > + } >> >> > >> >> > If my code analysis is correct, I think you'll reject the case where: >> >> > >> >> > name = "foo" >> >> > desc[13] = "foo \0zzzzzzzz" >> >> > >> >> > ...but you'll accept these cases: >> >> > >> >> > desc[13] = "foo \nzzzzzzzz" >> >> > desc[13] = "foo \0\0\0\0\0\0\0\0\0" >> >> > >> >> > It somehow seems weird to me that a '\n' terminates the string but not a '\0'. >> >> >> >> I'm also not sure about \0... based on >> >> https://git.linuxtv.org/edid-decode.git/tree/parse-base-block.cpp#n493, >> >> they use \n as terminator. Maybe we should also reject \0 before\n? >> >> Since it's not printable. >> > >> > Ah, OK. I guess the EDID spec simply doesn't allow for '\0' in there. >> > I guess in that case I'd prefer simply removing the code to handle >> > '\0' instead of treating it like space until we see some actual need >> > for it. So just get rid of the "!desc[i]" case? >> >> The spec text, similar for both EDID_DETAIL_MONITOR_NAME and >> EDID_DETAIL_MONITOR_STRING: >> >> Up to 13 alphanumeric characters (using ASCII codes) may be used >> to define the model name of the display product. The data shall >> be sequenced such that the 1st byte (ASCII code) = the 1st >> character, the 2nd byte (ASCII code) = the 2nd character, >> etc. If there are less than 13 characters in the string, then >> terminate the display product name string with ASCII code ‘0Ah’ >> (line feed) and pad the unused bytes in the field with ASCII >> code ‘20h’ (space). >> >> In theory, only checking for '\n' for termination should be enough, and >> this is what drm_edid_get_monitor_name() does. If there's a space >> *before* that, it should be considered part of the name, and not >> ignored. (So my suggestion in reply to the previous version is wrong.) >> >> However, since the match name uses NUL termination, maybe we should >> ignore NULs *before* '\n'? Like so: >> >> for (i = name_len; i < desc_len; i++) { >> if (desc[i] == '\n') >> break; >> if (!desc[i]) >> return; >> } >> > Allow trailing white spaces so we don't need to add the trailing white > space in edp_panel_entry. Just so it's clear here too: Agreed. > > https://lore.kernel.org/lkml/CAA8EJpr7LHvqeGXhbFQ8KNn0LGDuv19cw0i04qVUz51TJeSQrA@mail.gmail.com/ > >> >> BR, >> Jani. >> >> >> > >> > -Doug >> >> -- >> Jani Nikula, Intel
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index febe374fa969..29ef35ebee32 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -100,6 +100,11 @@ struct detailed_mode_closure { int modes; }; +struct drm_edid_match_closure { + const struct drm_edid_ident *ident; + bool matched; +}; + #define LEVEL_DMT 0 #define LEVEL_GTF 1 #define LEVEL_GTF2 2 @@ -5405,6 +5410,66 @@ drm_parse_hdmi_vsdb_audio(struct drm_connector *connector, const u8 *db) connector->audio_latency[0], connector->audio_latency[1]); } +static void +match_identity(const struct detailed_timing *timing, void *data) +{ + struct drm_edid_match_closure *closure = data; + unsigned int i; + const char *name = closure->ident->name; + unsigned int name_len = strlen(name); + const char *desc = timing->data.other_data.data.str.str; + unsigned int desc_len = ARRAY_SIZE(timing->data.other_data.data.str.str); + + if (name_len > desc_len || + !(is_display_descriptor(timing, EDID_DETAIL_MONITOR_NAME) || + is_display_descriptor(timing, EDID_DETAIL_MONITOR_STRING))) + return; + + if (strncmp(name, desc, name_len)) + return; + + /* Allow trailing white spaces and \0. */ + for (i = name_len; i < desc_len; i++) { + if (desc[i] == '\n') + break; + if (!isspace(desc[i]) && !desc[i]) + return; + } + + closure->matched = true; +} + +/** + * drm_edid_match - match drm_edid with given identity + * @drm_edid: EDID + * @ident: the EDID identity to match with + * + * Check if the EDID matches with the given identity. + * + * Return: True if the given identity matched with EDID, false otherwise. + */ +bool drm_edid_match(const struct drm_edid *drm_edid, + const struct drm_edid_ident *ident) +{ + if (!drm_edid || drm_edid_get_panel_id(drm_edid) != ident->panel_id) + return false; + + /* Match with name only if it's not NULL. */ + if (ident->name) { + struct drm_edid_match_closure closure = { + .ident = ident, + .matched = false, + }; + + drm_for_each_detailed_block(drm_edid, match_identity, &closure); + + return closure.matched; + } + + return true; +} +EXPORT_SYMBOL(drm_edid_match); + static void monitor_name(const struct detailed_timing *timing, void *data) { diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index 8b233865b085..28f05a7b2f7b 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -367,6 +367,12 @@ struct edid { u8 checksum; } __attribute__((packed)); +/* EDID matching */ +struct drm_edid_ident { + u32 panel_id; + const char *name; +}; + #define EDID_PRODUCT_ID(e) ((e)->prod_code[0] | ((e)->prod_code[1] << 8)) /* Short Audio Descriptor */ @@ -567,6 +573,8 @@ struct edid *drm_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter); const struct drm_edid *drm_edid_read_base_block(struct i2c_adapter *adapter); u32 drm_edid_get_panel_id(const struct drm_edid *drm_edid); +bool drm_edid_match(const struct drm_edid *drm_edid, + const struct drm_edid_ident *ident); struct edid *drm_get_edid_switcheroo(struct drm_connector *connector, struct i2c_adapter *adapter); struct edid *drm_edid_duplicate(const struct edid *edid);
Create a type drm_edid_ident as the identity of an EDID. Currently it contains panel id and monitor name. Create a function that can match a given EDID and an identity: 1. Reject if the panel id doesn't match. 2. If name is not null in identity, try to match it in the detailed timing blocks. Note that some panel vendors put the monitor name after EDID_DETAIL_MONITOR_STRING. Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org> --- v4->v5: use strncmp, change function/variable names. --- drivers/gpu/drm/drm_edid.c | 65 ++++++++++++++++++++++++++++++++++++++ include/drm/drm_edid.h | 8 +++++ 2 files changed, 73 insertions(+)