On Thu, 29 Feb 2024 at 19:11, Doug Anderson <dianders@xxxxxxxxxxxx> wrote: > > Hi, > > On Thu, Feb 29, 2024 at 8:43 AM Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote: > > > > 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. > > 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 *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? > > 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=FV=VMVr+eJ7eyuLGa671fMgH6ZX9zPOkbKzYJ0H79MZ2k9A@xxxxxxxxxxxxxx > [2] https://lore.kernel.org/r/CAJMQK-gfKbdPhYJeCJ5UX0dNrx3y-EmLsTiv9nj+U3Rmej38pw@xxxxxxxxxxxxxx -- With best wishes Dmitry
