On Mon, Feb 26, 2024 at 2:29 PM Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
>
> Hi,
>
> On Fri, Feb 23, 2024 at 2:40 PM 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. 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
