Received-SPF: pass (google.com: domain of linux-kernel+bounces-87098-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: Jani Nikula <jani.nikula@linux.intel.com>
To: Doug Anderson <dianders@chromium.org>, Hsin-Yi Wang <hsinyi@chromium.org>
Cc: 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 1/2] drm_edid: Add a function to get EDID base block
In-Reply-To: <CAD=FV=WU-2yystd40e+g9VNDNTiv5c=nP0uQg-AR03o7UGMTdA@mail.gmail.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
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>
Date: Thu, 29 Feb 2024 18:43:23 +0200
Message-ID: <87bk7z6x1w.fsf@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

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@linux.i=
ntel.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 same p=
anel id
>> > > although their EDID and names are different. Besides panel id, now w=
e need
>> > > the hash of entire EDID base block to distinguish these panel varian=
ts.
>> > >
>> > > Add drm_edid_get_base_block to returns the EDID base block, so calle=
r 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.or=
g/
>>
>> 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.

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_pa=
nel_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?


BR,
Jani.


--=20
Jani Nikula, Intel