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

  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.

Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
---
 drivers/gpu/drm/panel/panel-edp.c | 52 +++++++++++++++++++++++++++----
 1 file changed, 46 insertions(+), 6 deletions(-)
  

Hi,

On Fri, Feb 23, 2024 at 2:40 PM 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. 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?


> @@ -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?


> @@ -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.



-Doug
  

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

Hi,

On Mon, Feb 26, 2024 at 2:39 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> On Mon, Feb 26, 2024 at 2:29 PM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Fri, Feb 23, 2024 at 2:40 PM 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. 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.

Ah, in (yet another) offline comment, someone also pointed out bytes
12-15 should also be ignored for the CRC. Those are the serial number.

-Doug
  

On Sat, 24 Feb 2024 at 00:40, 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. 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.
>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
>  drivers/gpu/drm/panel/panel-edp.c | 52 +++++++++++++++++++++++++++----
>  1 file changed, 46 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
> index f6ddbaa103b5..42c430036846 100644
> --- a/drivers/gpu/drm/panel/panel-edp.c
> +++ b/drivers/gpu/drm/panel/panel-edp.c
> @@ -21,6 +21,7 @@
>   * DEALINGS IN THE SOFTWARE.
>   */
>
> +#include <linux/crc32.h>
>  #include <linux/debugfs.h>
>  #include <linux/delay.h>
>  #include <linux/gpio/consumer.h>
> @@ -213,6 +214,9 @@ struct edp_panel_entry {
>         /** @panel_id: 32-bit ID for panel, encoded with drm_edid_encode_panel_id(). */
>         u32 panel_id;
>
> +       /** @panel_hash: the CRC32 hash of the EDID base block. */
> +       u32 panel_hash;
> +
>         /** @delay: The power sequencing delays needed for this panel. */
>         const struct panel_delay *delay;
>
> @@ -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;
>                 kfree(base_block);
>         } else {
>                 dev_err(dev, "Couldn't identify panel via EDID\n");
>                 ret = -EIO;
>                 goto exit;
>         }
> +
>         drm_edid_decode_panel_id(panel_id, vend, &product_id);
>
> -       panel->detected_panel = find_edp_panel(panel_id);
> +       panel->detected_panel = find_edp_panel(panel_id, panel_hash);
>
>         /*
>          * We're using non-optimized timings and want it really obvious that
> @@ -1006,6 +1012,19 @@ static const struct panel_desc auo_b101ean01 = {
>         },
>  };
>
> +static const struct drm_display_mode auo_b116xa3_mode = {
> +       .clock = 70589,
> +       .hdisplay = 1366,
> +       .hsync_start = 1366 + 40,
> +       .hsync_end = 1366 + 40 + 40,
> +       .htotal = 1366 + 40 + 40 + 32,
> +       .vdisplay = 768,
> +       .vsync_start = 768 + 10,
> +       .vsync_end = 768 + 10 + 12,
> +       .vtotal = 768 + 10 + 12 + 6,
> +       .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
> +};
> +
>  static const struct drm_display_mode auo_b116xak01_mode = {
>         .clock = 69300,
>         .hdisplay = 1366,
> @@ -1926,11 +1945,13 @@ static const struct panel_delay delay_200_500_e50_po2e200 = {
>         .delay = _delay \
>  }
>
> -#define EDP_PANEL_ENTRY2(vend_chr_0, vend_chr_1, vend_chr_2, product_id, _delay, _name, _mode) \
> +#define EDP_PANEL_ENTRY2(vend_chr_0, vend_chr_1, vend_chr_2, product_id, \
> +                        _hash, _delay, _name, _mode) \
>  { \
>         .name = _name, \
>         .panel_id = drm_edid_encode_panel_id(vend_chr_0, vend_chr_1, vend_chr_2, \
>                                              product_id), \
> +       .panel_hash = _hash, \
>         .delay = _delay, \
>         .override_edid_mode = _mode \
>  }
> @@ -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.
> + */
> +static const struct edp_panel_entry edp_panels_variants[] = {

I'd suggest keeping these entries in the main table. Otherwise it
becomes harder to note, which setting gets used.

> +       EDP_PANEL_ENTRY2('A', 'U', 'O', 0x405c, 0xa9951478, &auo_b116xak01.delay,
> +                        "B116XAK01.0", &auo_b116xa3_mode),
> +       EDP_PANEL_ENTRY2('A', 'U', 'O', 0x615c, 0x109b75b3, &delay_200_500_e50,
> +                        "B116XAN06.1", &auo_b116xa3_mode),
> +
> +       { /* sentinal */ }
> +};
> +
> +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;
>
> +       for (panel = edp_panels_variants; panel->panel_id; panel++)
> +               if (panel->panel_id == panel_id && panel->panel_hash == panel_hash)
> +                       return panel;
> +
>         for (panel = edp_panels; panel->panel_id; panel++)
>                 if (panel->panel_id == panel_id)
>                         return panel;
> --
> 2.44.0.rc0.258.g7320e95886-goog
>