On Wed, 28 Feb 2024, Doug Anderson <dianders@xxxxxxxxxxxx> wrote: > Hi, > > On Tue, Feb 27, 2024 at 5:27 PM Hsin-Yi Wang <hsinyi@xxxxxxxxxxxx> wrote: >> >> On Tue, Feb 27, 2024 at 1:09 AM Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote: >> > >> > On Fri, 23 Feb 2024, Hsin-Yi Wang <hsinyi@xxxxxxxxxxxx> 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@xxxxxxxxxxxx/ >> >> 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
