Received-SPF: pass (google.com: domain of linux-kernel+bounces-89911-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240223223958.3887423-1-hsinyi@chromium.org> <20240223223958.3887423-2-hsinyi@chromium.org>
 <87wmqqjmt9.fsf@intel.com> <CAJMQK-jSPg6vU3SLmRy7zwNHJ4yqO2hT6RaiYxA4ifZ7CzwD9Q@mail.gmail.com>
 <CAD=FV=WU-2yystd40e+g9VNDNTiv5c=nP0uQg-AR03o7UGMTdA@mail.gmail.com>
 <87bk7z6x1w.fsf@intel.com> <CAD=FV=Wzm9Y7m9Q6KqO7yWdnc1xToaRMb2f1s2TQMJqpqVYLOg@mail.gmail.com>
In-Reply-To: <CAD=FV=Wzm9Y7m9Q6KqO7yWdnc1xToaRMb2f1s2TQMJqpqVYLOg@mail.gmail.com>
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Date: Sun, 3 Mar 2024 23:30:40 +0200
Message-ID: <CAA8EJpqHJTbc+TCpkccjx_eQH36zaNgcQ9QssecNeQUQgfYApQ@mail.gmail.com>
Subject: Re: [PATCH 1/2] drm_edid: Add a function to get EDID base block
To: Doug Anderson <dianders@chromium.org>
Cc: Jani Nikula <jani.nikula@linux.intel.com>, Hsin-Yi Wang <hsinyi@chromium.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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, 29 Feb 2024 at 19:11, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Feb 29, 2024 at 8:43=E2=80=AFAM Jani Nikula <jani.nikula@linux.in=
tel.com> wrote:
> >
> > On Wed, 28 Feb 2024, Doug Anderson <dianders@chromium.org> wrote:
> > > Hi,
> > >
> > > On Tue, Feb 27, 2024 at 5:27=E2=80=AFPM Hsin-Yi Wang <hsinyi@chromium=
org> wrote:
> > >>
> > >> On Tue, Feb 27, 2024 at 1:09=E2=80=AFAM Jani Nikula <jani.nikula@lin=
ux.intel.com> wrote:
> > >> >
> > >> > On Fri, 23 Feb 2024, Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> > >> > > It's found that some panels have variants that they share the sa=
me panel id
> > >> > > although their EDID and names are different. Besides panel id, n=
ow we need
> > >> > > the hash of entire EDID base block to distinguish these panel va=
riants.
> > >> > >
> > >> > > Add drm_edid_get_base_block to returns the EDID base block, so c=
aller 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 rea=
ding
> > >> > functions, and use that instead.
> > >> >
> > >> > Most likely you'll only have 1-2 blocks anyway. And you might cons=
ider
> > >> > 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 on=
ly
> > >> > 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@chromiu=
m.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 th=
e
> > > 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 tha=
t
> > > 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 quirk=
s
> > 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 *has=
h);
> >
> > 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 interface=
s
> > 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?

Would it be easier to push drm_edid_match to drm_edid.c? It looks way
more simpler than the opaque blob.

> [1] https://lore.kernel.org/r/CAD=3DFV=3DVMVr+eJ7eyuLGa671fMgH6ZX9zPOkbKz=
YJ0H79MZ2k9A@mail.gmail.com
> [2] https://lore.kernel.org/r/CAJMQK-gfKbdPhYJeCJ5UX0dNrx3y-EmLsTiv9nj+U3=
Rmej38pw@mail.gmail.com


--=20
With best wishes
Dmitry