linux-kernel - Re: [PATCH 1/2] drm_edid: Add a function to get EDID base block

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 28 Feb 2024 16:20:31 -0800
From: Doug Anderson <dianders@...omium.org>
To: Hsin-Yi Wang <hsinyi@...omium.org>
Cc: Jani Nikula <jani.nikula@...ux.intel.com>, 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
Subject: Re: [PATCH 1/2] drm_edid: Add a function to get EDID base block

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@...ux.intel.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?



-Doug