Message-ID: <CAD=FV=Wzm9Y7m9Q6KqO7yWdnc1xToaRMb2f1s2TQMJqpqVYLOg@mail.gmail.com>
Date: Thu, 29 Feb 2024 09:11:39 -0800
From: Doug Anderson <dianders@...omium.org>
To: Jani Nikula <jani.nikula@...ux.intel.com>
Cc: Hsin-Yi Wang <hsinyi@...omium.org>, Neil Armstrong <neil.armstrong@...aro.org>, 
	Jessica Zhang <quic_jesszhan@...cinc.com>, Sam Ravnborg <sam@...nborg.org>, 
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, 
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>, 
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org, 
	Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Subject: Re: [PATCH 1/2] drm_edid: Add a function to get EDID base block

Hi,

On Thu, Feb 29, 2024 at 8:43 AM Jani Nikula <jani.nikula@...ux.intel.com> wrote:
>
> On Wed, 28 Feb 2024, Doug Anderson <dianders@...omium.org> wrote:
> > Hi,
> >
> > On Tue, Feb 27, 2024 at 5:27 PM Hsin-Yi Wang <hsinyi@...omium.org> wrote:
> >>
> >> On Tue, Feb 27, 2024 at 1:09 AM Jani Nikula <jani.nikula@...uxintel.com> wrote:
> >> >
> >> > On Fri, 23 Feb 2024, Hsin-Yi Wang <hsinyi@...omium.org> wrote:
> >> > > It's found that some panels have variants that they share the same panel id
> >> > > although their EDID and names are different. Besides panel id, now we need
> >> > > the hash of entire EDID base block to distinguish these panel variants.
> >> > >
> >> > > Add drm_edid_get_base_block to returns the EDID base block, so caller can
> >> > > further use it to get panel id and/or the hash.
> >> >
> >> > Please reconsider the whole approach here.
> >> >
> >> > Please let's not add single-use special case functions to read an EDID
> >> > base block.
> >> >
> >> > Please consider reading the whole EDID, using the regular EDID reading
> >> > functions, and use that instead.
> >> >
> >> > Most likely you'll only have 1-2 blocks anyway. And you might consider
> >> > caching the EDID in struct panel_edp if reading the entire EDID is too
> >> > slow. (And if it is, this is probably sensible even if the EDID only
> >> > consists of one block.)
> >> >
> >> > Anyway, please do *not* merge this as-is.
> >> >
> >>
> >> hi Jani,
> >>
> >> I sent a v2 here implementing this method:
> >> https://lore.kernel.org/lkml/20240228011133.1238439-2-hsinyi@chromium.org/
> >>
> >> We still have to read edid twice due to:
> >> 1. The first caller is in panel probe, at that time, connector is
> >> still unknown, so we can't update connector status (eg. update
> >> edid_corrupt).
> >> 2. It's possible that the connector can have some override
> >> (drm_edid_override_get) to EDID, that is still unknown during the
> >> first read.
> >
> > I'll also comment in Hsin-Yi's v2, but given Hsin-Yi's digging and the
> > fact that we can't cache the EDID (because we don't yet have a
> > "drm_connector"), I'd much prefer Hsin-Yi's solution here from v1 that
> > allows reading just the first block. If we try to boot a device with a
> > multi-block EDID we're now wastefully reading all the blocks of the
> > EDID twice at bootup which will slow boot time.
> >
> > If you can see a good solution to avoid reading the EDID twice then
> > that would be amazing, but if not it seems like we should go back to
> > what's here in v1. What do you think? Anyone else have any opinions?
>
> I haven't replied so far, because I've been going back and forth with
> this. I'm afraid I don't really like either approach now. Handling the
> no connector case in v2 is a bit ugly too. :(
>
> Seems like you only need this to extend the panel ID with a hash. And
> panel-edp.c is the only user of drm_edid_get_panel_id(). And EDID quirks
> in drm_edid.c could theoretically hit the same problem you're solving.

Good point. That would imply that more of the logic could go into
"drm_edid.c" in case the EDID quirks code eventually needs it.


> So maybe something like:
>
>         u32 drm_edid_get_panel_id(struct i2c_adapter *adapter, u32 *hash);
>
> or if you want to be fancy add a struct capturing both id and hash:
>
>         bool drm_edid_get_panel_id(struct i2c_adapter *adapter, struct drm_edid_panel_id *id);
>
> And put the hash (or whatever mechanism you have) computation in
> drm_edid.c. Just hide it all in drm_edid.c, and keep the EDID interfaces
> neat.
>
> How would that work for you?

The problem is that Dmitry didn't like the idea of using a hash and in
v2 Hsin-Yi has moved to using the name of the display. ...except of
course that eDP panels don't always properly specify
"EDID_DETAIL_MONITOR_NAME". See the discussion [1]. If you want to see
some of the EDIDs involved, you can see Hsin-Yi's post [2]. The panels
included stuff like this:

    Alphanumeric Data String: 'AUO'
    Alphanumeric Data String: 'B116XAN04.0 '

The fact that there is more than one string in there makes it hard to
just "return" the display name in a generic way. The way Hsin-Yi's
code was doing it was that it would consider it a match if the panel
name was in any of the strings...

How about this as a solution: we change drm_edid_get_panel_id() to
return an opaque type (struct drm_edid_panel_id_blob) that's really
just the first block of the EDID but we can all pretend that it isn't.
Then we can add a function in drm_edid.c that takes this opaque blob,
a 32-bit integer (as per drm_edid_encode_panel_id()), and a string
name and it can tell us if the blob matches?


[1] https://lore.kernel.org/r/CAD=FV=VMVr+eJ7eyuLGa671fMgH6ZX9zPOkbKzYJ0H79MZ2k9A@mail.gmail.com
[2] https://lore.kernel.org/r/CAJMQK-gfKbdPhYJeCJ5UX0dNrx3y-EmLsTiv9nj+U3Rmej38pw@mail.gmail.com

Powered by blists - more mailing lists