From: Jani Nikula <jani.nikula@linux.intel.com>
To: Hsin-Yi Wang <hsinyi@chromium.org>
Cc: Doug Anderson <dianders@chromium.org>,
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
Neil Armstrong <neil.armstrong@linaro.org>,
Jessica Zhang <quic_jesszhan@quicinc.com>,
Sam Ravnborg <sam@ravnborg.org>,
Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
Maxime Ripard <mripard@kernel.org>,
Thomas Zimmermann <tzimmermann@suse.de>,
David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 3/6] drm/edid: Add a function to match EDID with identity
Date: Fri, 08 Mar 2024 00:36:11 +0200 [thread overview]
Message-ID: <87plw5vfdw.fsf@intel.com> (raw)
In-Reply-To: <CAJMQK-igm-OXa=L-Bb0hdm5+KL98sk9UAznvAR7SptP9iwWAoA@mail.gmail.com>
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
--
Jani Nikula, Intel
next prev parent reply other threads:[~2024-03-07 22:36 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-06 19:55 [PATCH v5 0/6] Match panel with identity Hsin-Yi Wang
2024-03-06 19:55 ` [PATCH v5 1/6] drm_edid: Add a function to get EDID base block Hsin-Yi Wang
2024-03-06 23:29 ` Doug Anderson
2024-03-07 12:50 ` Jani Nikula
2024-03-06 19:55 ` [PATCH v5 2/6] drm/edid: Clean up drm_edid_get_panel_id() Hsin-Yi Wang
2024-03-06 23:29 ` Doug Anderson
2024-03-07 12:51 ` Jani Nikula
2024-03-06 19:55 ` [PATCH v5 3/6] drm/edid: Add a function to match EDID with identity Hsin-Yi Wang
2024-03-06 23:29 ` Doug Anderson
2024-03-07 0:20 ` Hsin-Yi Wang
2024-03-07 0:37 ` Doug Anderson
2024-03-07 13:20 ` Jani Nikula
2024-03-07 19:34 ` Hsin-Yi Wang
2024-03-07 22:36 ` Jani Nikula [this message]
2024-03-06 19:55 ` [PATCH v5 4/6] drm/edid: Match edid quirks " Hsin-Yi Wang
2024-03-06 23:29 ` Doug Anderson
2024-03-06 19:55 ` [PATCH v5 5/6] drm/panel-edp: Match edp_panels with panel identity Hsin-Yi Wang
2024-03-06 23:30 ` Doug Anderson
2024-03-06 19:55 ` [PATCH v5 6/6] drm/panel-edp: Fix AUO 0x405c panel naming and add a variant Hsin-Yi Wang
2024-03-06 23:30 ` Doug Anderson
2024-03-07 13:28 ` Jani Nikula
2024-03-07 20:18 ` Hsin-Yi Wang
2024-03-07 20:27 ` Jani Nikula
2024-03-07 21:46 ` Doug Anderson
2024-03-07 22:35 ` Jani Nikula
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87plw5vfdw.fsf@intel.com \
--to=jani.nikula@linux.intel.com \
--cc=airlied@gmail.com \
--cc=daniel@ffwll.ch \
--cc=dianders@chromium.org \
--cc=dmitry.baryshkov@linaro.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=hsinyi@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maarten.lankhorst@linux.intel.com \
--cc=mripard@kernel.org \
--cc=neil.armstrong@linaro.org \
--cc=quic_jesszhan@quicinc.com \
--cc=sam@ravnborg.org \
--cc=tzimmermann@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).