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
Date: Thu, 29 Feb 2024 18:43:23 +0200 [thread overview]
Message-ID: <87bk7z6x1w.fsf@intel.com> (raw)
In-Reply-To: <CAD=FV=WU-2yystd40e+g9VNDNTiv5c=nP0uQg-AR03o7UGMTdA@mail.gmail.com>
On Wed, 28 Feb 2024, Doug Anderson <dianders@chromium.org> wrote:
> Hi,
>
> On Tue, Feb 27, 2024 at 5:27 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>>
>> On Tue, Feb 27, 2024 at 1:09 AM Jani Nikula <jani.nikula@linux.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 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.
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?
BR,
Jani.
--
Jani Nikula, Intel
next prev parent reply other threads:[~2024-02-29 16:43 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-23 22:29 [PATCH 0/2] Match panel hash for overridden mode Hsin-Yi Wang
2024-02-23 22:29 ` [PATCH 1/2] drm_edid: Add a function to get EDID base block Hsin-Yi Wang
2024-02-26 22:29 ` Doug Anderson
2024-02-27 9:09 ` Jani Nikula
2024-02-27 16:50 ` Doug Anderson
2024-02-28 1:27 ` Hsin-Yi Wang
2024-02-29 0:20 ` Doug Anderson
2024-02-29 16:43 ` Jani Nikula [this message]
2024-02-29 17:11 ` Doug Anderson
2024-03-03 21:30 ` Dmitry Baryshkov
2024-03-04 16:17 ` Doug Anderson
2024-03-04 19:55 ` Hsin-Yi Wang
2024-03-04 20:44 ` Jani Nikula
2024-02-23 22:29 ` [PATCH 2/2] drm/panel: panel-edp: Match with panel hash for overridden modes Hsin-Yi Wang
2024-02-26 22:29 ` Doug Anderson
2024-02-26 22:38 ` Hsin-Yi Wang
2024-02-27 0:24 ` Doug Anderson
2024-02-27 2:28 ` Dmitry Baryshkov
2024-02-27 0:37 ` [PATCH 0/2] Match panel hash for overridden mode Dmitry Baryshkov
2024-02-27 1:00 ` Doug Anderson
2024-02-27 2:18 ` Dmitry Baryshkov
2024-02-27 1:09 ` Hsin-Yi Wang
2024-02-27 2:26 ` Dmitry Baryshkov
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=87bk7z6x1w.fsf@intel.com \
--to=jani.nikula@linux.intel.com \
--cc=airlied@gmail.com \
--cc=daniel@ffwll.ch \
--cc=dianders@chromium.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.