[PATCH 2/2] drm/panel: panel-edp: Match with panel hash for overridden modes

[Hsin-Yi Wang]

On Mon, Feb 26, 2024 at 2:29 PM Doug Anderson <dianders at chromium.org> wrote:
>
> Hi,
>
> On Fri, Feb 23, 2024 at 2:40 PM Hsin-Yi Wang <hsinyi at 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. One of the variants requires
> > using overridden modes to resolve glitching issue as described in commit
> > 70e0d5550f5c ("drm/panel-edp: Add auo_b116xa3_mode"). Other variants should
> > use the modes parsed from EDID.
> >
> > For example, AUO 0x405c B116XAK01.0, it has at least 2 different variants
> > that EDID and panel name are different, but using the same panel id. One of
> > the variants require using overridden mode. Same case for AUO 0x615c
> > B116XAN06.1.
> >
> > Add such entries and use the hash of the EDID to match the panel needs the
> > overridden modes.
>
> As pointed out in an offline discussion, it's possible that we might
> want to "ignore" some of these bytes for the purpose of the CRC.
> Specifically, we might want to ignore:
> * byte 16 - Week of manufacture
> * byte 17 - Year of manufacture
> * byte 127 - Checksum
>
> That way if a manufacturer actually is updating those numbers in
> production we can still have one hash that captures all the panels. I
> have no idea if manufacturers actually are, but IMO the hash of the
> rest of the base block should be sufficient to differentiate between
> different panels anyway. It would be easy to just zero out those 3
> bytes before computing the CRC.
>
> What do you think?

Agreed that we can zero out these fields.

>
>
> > @@ -758,13 +762,13 @@ static void panel_edp_parse_panel_timing_node(struct device *dev,
> >                 dev_err(dev, "Reject override mode: No display_timing found\n");
> >  }
> >
> > -static const struct edp_panel_entry *find_edp_panel(u32 panel_id);
> > +static const struct edp_panel_entry *find_edp_panel(u32 panel_id, u32 panel_hash);
> >
> >  static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
> >  {
> >         struct panel_desc *desc;
> >         void *base_block;
> > -       u32 panel_id;
> > +       u32 panel_id, panel_hash;
> >         char vend[4];
> >         u16 product_id;
> >         u32 reliable_ms = 0;
> > @@ -796,15 +800,17 @@ static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
> >         base_block = drm_edid_get_base_block(panel->ddc);
> >         if (base_block) {
> >                 panel_id = drm_edid_get_panel_id(base_block);
> > +               panel_hash = crc32_le(~0, base_block, EDID_LENGTH) ^ 0xffffffff;
>
> Any reason you need to XOR with 0xffffffff?
>
To be consistent with the crc32[1] command. It's more convenient to be
able to verify it with userspace tools.

[1] https://www.commandlinux.com/man-page/man1/crc32.1.html

>
> > @@ -2077,13 +2098,32 @@ static const struct edp_panel_entry edp_panels[] = {
> >         { /* sentinal */ }
> >  };
> >
> > -static const struct edp_panel_entry *find_edp_panel(u32 panel_id)
> > +/*
> > + * Similar to edp_panels, this table lists panel variants that require using
> > + * overridden modes but have the same panel id as one of the entries in edp_panels.
> > + *
> > + * Sort first by vendor, then by product ID.
>
> Add ", then by hash" just in case we need it.
>
>
> > +static const struct edp_panel_entry *find_edp_panel(u32 panel_id, u32 panel_hash)
> >  {
> >         const struct edp_panel_entry *panel;
> >
> > -       if (!panel_id)
> > +       if (!panel_id || !panel_hash)
> >                 return NULL;
>
> IMO just remove the check above. Not sure why it was there in the
> first place. Maybe I had it from some older version of the code?
> Callers shouldn't be calling us with a panel ID / hash of 0 anyway,
> and if they do they'll go through the loop and return NULL anyway.
>

Sure.

>
>
> -Doug