[0/2] Match panel hash for overridden mode

  This series is a follow up for 1a5e81de180e ("Revert "drm/panel-edp: Add
auo_b116xa3_mode""). It's found that 2 different AUO panels use the same
product id. One of them requires an overridden mode, while the other should
use the mode directly from edid.

Since product id match is no longer sufficient, EDP_PANEL_ENTRY2 is extended
to check the crc hash of the entire edid base block.

Hsin-Yi Wang (2):
  drm_edid: Add a function to get EDID base block
  drm/panel: panel-edp: Match with panel hash for overridden modes

 drivers/gpu/drm/drm_edid.c        | 55 +++++++++++++++-------------
 drivers/gpu/drm/panel/panel-edp.c | 60 ++++++++++++++++++++++++++-----
 include/drm/drm_edid.h            |  3 +-
 3 files changed, 84 insertions(+), 34 deletions(-)
  

On Sat, 24 Feb 2024 at 00:40, Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> This series is a follow up for 1a5e81de180e ("Revert "drm/panel-edp: Add
> auo_b116xa3_mode""). It's found that 2 different AUO panels use the same
> product id. One of them requires an overridden mode, while the other should
> use the mode directly from edid.
>
> Since product id match is no longer sufficient, EDP_PANEL_ENTRY2 is extended
> to check the crc hash of the entire edid base block.

Do you have these EDIDs posted somewhere? Can we use something less
cryptic than hash for matching the panel, e.g. strings from Monitor
Descriptors?

>
> Hsin-Yi Wang (2):
>   drm_edid: Add a function to get EDID base block
>   drm/panel: panel-edp: Match with panel hash for overridden modes
>
>  drivers/gpu/drm/drm_edid.c        | 55 +++++++++++++++-------------
>  drivers/gpu/drm/panel/panel-edp.c | 60 ++++++++++++++++++++++++++-----
>  include/drm/drm_edid.h            |  3 +-
>  3 files changed, 84 insertions(+), 34 deletions(-)
>
> --
> 2.44.0.rc0.258.g7320e95886-goog
>
  

Hi,

On Mon, Feb 26, 2024 at 4:37 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Sat, 24 Feb 2024 at 00:40, Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> >
> > This series is a follow up for 1a5e81de180e ("Revert "drm/panel-edp: Add
> > auo_b116xa3_mode""). It's found that 2 different AUO panels use the same
> > product id. One of them requires an overridden mode, while the other should
> > use the mode directly from edid.
> >
> > Since product id match is no longer sufficient, EDP_PANEL_ENTRY2 is extended
> > to check the crc hash of the entire edid base block.
>
> Do you have these EDIDs posted somewhere? Can we use something less
> cryptic than hash for matching the panel, e.g. strings from Monitor
> Descriptors?

We could try it if need be. I guess I'm worried that if panel vendors
ended up re-using the panel ID for two different panels that they
might also re-use the name field too. Hashing the majority of the
descriptor's base block makes us more likely not to mix two panels up.
In general it feels like the goal is that if there is any doubt that
we shouldn't override the mode and including more fields in the hash
works towards that goal.

I guess one thing that might help would be to make it a policy that
any time a panel is added to this list that a full EDID is included in
the commit message. That would mean that if we ever needed to change
things we could. What do you think?

That being said, if everyone thinks that the "name" field is enough,
we could do it. I think that in the one case that we ran into it would
have been enough...

-Doug
  

On Mon, Feb 26, 2024 at 4:37 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Sat, 24 Feb 2024 at 00:40, Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> >
> > This series is a follow up for 1a5e81de180e ("Revert "drm/panel-edp: Add
> > auo_b116xa3_mode""). It's found that 2 different AUO panels use the same
> > product id. One of them requires an overridden mode, while the other should
> > use the mode directly from edid.
> >
> > Since product id match is no longer sufficient, EDP_PANEL_ENTRY2 is extended
> > to check the crc hash of the entire edid base block.
>
> Do you have these EDIDs posted somewhere? Can we use something less
> cryptic than hash for matching the panel, e.g. strings from Monitor
> Descriptors?
>

Panel 1:

00 ff ff ff ff ff ff 00 06 af 5c 40 00 00 00 00
00 1a 01 04 95 1a 0e 78 02 99 85 95 55 56 92 28
22 50 54 00 00 00 01 01 01 01 01 01 01 01 01 01
01 01 01 01 01 01 12 1b 56 5a 50 00 19 30 30 20
46 00 00 90 10 00 00 18 00 00 00 0f 00 00 00 00
00 00 00 00 00 00 00 00 00 20 00 00 00 fe 00 41
55 4f 0a 20 20 20 20 20 20 20 20 20 00 00 00 fe
00 42 31 31 36 58 41 4b 30 31 2e 30 20 0a 00 cc

----------------

Block 0, Base EDID:
  EDID Structure Version & Revision: 1.4
  Vendor & Product Identification:
    Manufacturer: AUO
    Model: 16476
    Made in: 2016
  Basic Display Parameters & Features:
    Digital display
    Bits per primary color channel: 6
    DisplayPort interface
    Maximum image size: 26 cm x 14 cm
    Gamma: 2.20
    Supported color formats: RGB 4:4:4
    First detailed timing includes the native pixel format and
preferred refresh rate
  Color Characteristics:
    Red  : 0.5839, 0.3330
    Green: 0.3378, 0.5712
    Blue : 0.1582, 0.1328
    White: 0.3134, 0.3291
  Established Timings I & II: none
  Standard Timings: none
  Detailed Timing Descriptors:
    DTD 1:  1366x768    60.020 Hz 683:384  47.596 kHz   69.300 MHz
(256 mm x 144 mm)
                 Hfront   48 Hsync  32 Hback  10 Hpol N
                 Vfront    4 Vsync   6 Vback  15 Vpol N
    Manufacturer-Specified Display Descriptor (0x0f): 00 0f 00 00 00
00 00 00 00 00 00 00 00 00 00 20 '............... '
    Alphanumeric Data String: 'AUO'
    Alphanumeric Data String: 'B116XAK01.0 '
Checksum: 0xcc


Panel 2:

00 ff ff ff ff ff ff 00 06 af 5c 40 00 00 00 00
00 19 01 04 95 1a 0e 78 02 99 85 95 55 56 92 28
22 50 54 00 00 00 01 01 01 01 01 01 01 01 01 01
01 01 01 01 01 01 ce 1d 56 ea 50 00 1a 30 30 20
46 00 00 90 10 00 00 18 d4 17 56 ea 50 00 1a 30
30 20 46 00 00 90 10 00 00 18 00 00 00 fe 00 41
55 4f 0a 20 20 20 20 20 20 20 20 20 00 00 00 fe
00 42 31 31 36 58 41 4e 30 34 2e 30 20 0a 00 94

----------------

Block 0, Base EDID:
  EDID Structure Version & Revision: 1.4
  Vendor & Product Identification:
    Manufacturer: AUO
    Model: 16476
    Made in: 2015
  Basic Display Parameters & Features:
    Digital display
    Bits per primary color channel: 6
    DisplayPort interface
    Maximum image size: 26 cm x 14 cm
    Gamma: 2.20
    Supported color formats: RGB 4:4:4
    First detailed timing includes the native pixel format and
preferred refresh rate
  Color Characteristics:
    Red  : 0.5839, 0.3330
    Green: 0.3378, 0.5712
    Blue : 0.1582, 0.1328
    White: 0.3134, 0.3291
  Established Timings I & II: none
  Standard Timings: none
  Detailed Timing Descriptors:
    DTD 1:  1366x768    60.059824 Hz 683:384   47.688 kHz
76.300000 MHz (256 mm x 144 mm)
                 Hfront   48 Hsync  32 Hback  154 Hpol N
                 Vfront    4 Vsync   6 Vback   16 Vpol N
    DTD 2:  1366x768    48.016373 Hz 683:384   38.125 kHz
61.000000 MHz (256 mm x 144 mm)
                 Hfront   48 Hsync  32 Hback  154 Hpol N
                 Vfront    4 Vsync   6 Vback   16 Vpol N
    Alphanumeric Data String: 'AUO'
    Alphanumeric Data String: 'B116XAN04.0 '
Checksum: 0x94

In this example, Descriptors can also be used to distinguish. But it's
possible that the name field is also reused by mistake, for the same
reason as model id is reused.


> >
> > Hsin-Yi Wang (2):
> >   drm_edid: Add a function to get EDID base block
> >   drm/panel: panel-edp: Match with panel hash for overridden modes
> >
> >  drivers/gpu/drm/drm_edid.c        | 55 +++++++++++++++-------------
> >  drivers/gpu/drm/panel/panel-edp.c | 60 ++++++++++++++++++++++++++-----
> >  include/drm/drm_edid.h            |  3 +-
> >  3 files changed, 84 insertions(+), 34 deletions(-)
> >
> > --
> > 2.44.0.rc0.258.g7320e95886-goog
> >
>
>
> --
> With best wishes
> Dmitry
  

On Tue, 27 Feb 2024 at 03:00, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Feb 26, 2024 at 4:37 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Sat, 24 Feb 2024 at 00:40, Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> > >
> > > This series is a follow up for 1a5e81de180e ("Revert "drm/panel-edp: Add
> > > auo_b116xa3_mode""). It's found that 2 different AUO panels use the same
> > > product id. One of them requires an overridden mode, while the other should
> > > use the mode directly from edid.
> > >
> > > Since product id match is no longer sufficient, EDP_PANEL_ENTRY2 is extended
> > > to check the crc hash of the entire edid base block.
> >
> > Do you have these EDIDs posted somewhere? Can we use something less
> > cryptic than hash for matching the panel, e.g. strings from Monitor
> > Descriptors?
>
> We could try it if need be. I guess I'm worried that if panel vendors
> ended up re-using the panel ID for two different panels that they
> might also re-use the name field too. Hashing the majority of the
> descriptor's base block makes us more likely not to mix two panels up.
> In general it feels like the goal is that if there is any doubt that
> we shouldn't override the mode and including more fields in the hash
> works towards that goal.

My main concern is that hash (or crc) is a non-obvious thing: even if
we have EDID in the commit message, it takes some effort to calculate
the value. If for any reason we decide to change the hashed bytes
(e.g. by dropping any of the fields) it will be an error-prone process
to update existing hash values. On the contrary, the 'strings' are
easy to check / compare without any additional tools.

>
> I guess one thing that might help would be to make it a policy that
> any time a panel is added to this list that a full EDID is included in
> the commit message. That would mean that if we ever needed to change
> things we could. What do you think?

Definitely, that sounds like a good idea.

> That being said, if everyone thinks that the "name" field is enough,
> we could do it. I think that in the one case that we ran into it would
> have been enough...

Yes, I'd suggest using the string unless at some point we see two
panels sharing both panel_id and names.
  

On Tue, 27 Feb 2024 at 03:10, Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> On Mon, Feb 26, 2024 at 4:37 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Sat, 24 Feb 2024 at 00:40, Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> > >
> > > This series is a follow up for 1a5e81de180e ("Revert "drm/panel-edp: Add
> > > auo_b116xa3_mode""). It's found that 2 different AUO panels use the same
> > > product id. One of them requires an overridden mode, while the other should
> > > use the mode directly from edid.
> > >
> > > Since product id match is no longer sufficient, EDP_PANEL_ENTRY2 is extended
> > > to check the crc hash of the entire edid base block.
> >
> > Do you have these EDIDs posted somewhere? Can we use something less
> > cryptic than hash for matching the panel, e.g. strings from Monitor
> > Descriptors?
> >
>
> Panel 1:
>
> 00 ff ff ff ff ff ff 00 06 af 5c 40 00 00 00 00
> 00 1a 01 04 95 1a 0e 78 02 99 85 95 55 56 92 28
> 22 50 54 00 00 00 01 01 01 01 01 01 01 01 01 01
> 01 01 01 01 01 01 12 1b 56 5a 50 00 19 30 30 20
> 46 00 00 90 10 00 00 18 00 00 00 0f 00 00 00 00
> 00 00 00 00 00 00 00 00 00 20 00 00 00 fe 00 41
> 55 4f 0a 20 20 20 20 20 20 20 20 20 00 00 00 fe
> 00 42 31 31 36 58 41 4b 30 31 2e 30 20 0a 00 cc
>
> ----------------
>
> Block 0, Base EDID:
>   EDID Structure Version & Revision: 1.4
>   Vendor & Product Identification:
>     Manufacturer: AUO
>     Model: 16476
>     Made in: 2016
>   Basic Display Parameters & Features:
>     Digital display
>     Bits per primary color channel: 6
>     DisplayPort interface
>     Maximum image size: 26 cm x 14 cm
>     Gamma: 2.20
>     Supported color formats: RGB 4:4:4
>     First detailed timing includes the native pixel format and
> preferred refresh rate
>   Color Characteristics:
>     Red  : 0.5839, 0.3330
>     Green: 0.3378, 0.5712
>     Blue : 0.1582, 0.1328
>     White: 0.3134, 0.3291
>   Established Timings I & II: none
>   Standard Timings: none
>   Detailed Timing Descriptors:
>     DTD 1:  1366x768    60.020 Hz 683:384  47.596 kHz   69.300 MHz
> (256 mm x 144 mm)
>                  Hfront   48 Hsync  32 Hback  10 Hpol N
>                  Vfront    4 Vsync   6 Vback  15 Vpol N
>     Manufacturer-Specified Display Descriptor (0x0f): 00 0f 00 00 00
> 00 00 00 00 00 00 00 00 00 00 20 '............... '
>     Alphanumeric Data String: 'AUO'
>     Alphanumeric Data String: 'B116XAK01.0 '
> Checksum: 0xcc
>
>
> Panel 2:
>
> 00 ff ff ff ff ff ff 00 06 af 5c 40 00 00 00 00
> 00 19 01 04 95 1a 0e 78 02 99 85 95 55 56 92 28
> 22 50 54 00 00 00 01 01 01 01 01 01 01 01 01 01
> 01 01 01 01 01 01 ce 1d 56 ea 50 00 1a 30 30 20
> 46 00 00 90 10 00 00 18 d4 17 56 ea 50 00 1a 30
> 30 20 46 00 00 90 10 00 00 18 00 00 00 fe 00 41
> 55 4f 0a 20 20 20 20 20 20 20 20 20 00 00 00 fe
> 00 42 31 31 36 58 41 4e 30 34 2e 30 20 0a 00 94
>
> ----------------
>
> Block 0, Base EDID:
>   EDID Structure Version & Revision: 1.4
>   Vendor & Product Identification:
>     Manufacturer: AUO
>     Model: 16476
>     Made in: 2015
>   Basic Display Parameters & Features:
>     Digital display
>     Bits per primary color channel: 6
>     DisplayPort interface
>     Maximum image size: 26 cm x 14 cm
>     Gamma: 2.20
>     Supported color formats: RGB 4:4:4
>     First detailed timing includes the native pixel format and
> preferred refresh rate
>   Color Characteristics:
>     Red  : 0.5839, 0.3330
>     Green: 0.3378, 0.5712
>     Blue : 0.1582, 0.1328
>     White: 0.3134, 0.3291
>   Established Timings I & II: none
>   Standard Timings: none
>   Detailed Timing Descriptors:
>     DTD 1:  1366x768    60.059824 Hz 683:384   47.688 kHz
> 76.300000 MHz (256 mm x 144 mm)
>                  Hfront   48 Hsync  32 Hback  154 Hpol N
>                  Vfront    4 Vsync   6 Vback   16 Vpol N
>     DTD 2:  1366x768    48.016373 Hz 683:384   38.125 kHz
> 61.000000 MHz (256 mm x 144 mm)
>                  Hfront   48 Hsync  32 Hback  154 Hpol N
>                  Vfront    4 Vsync   6 Vback   16 Vpol N
>     Alphanumeric Data String: 'AUO'
>     Alphanumeric Data String: 'B116XAN04.0 '
> Checksum: 0x94
>
> In this example, Descriptors can also be used to distinguish. But it's
> possible that the name field is also reused by mistake, for the same
> reason as model id is reused.

Thank you! Let's settle the discussion at the cover letter.

>
>
> > >
> > > Hsin-Yi Wang (2):
> > >   drm_edid: Add a function to get EDID base block
> > >   drm/panel: panel-edp: Match with panel hash for overridden modes
> > >
> > >  drivers/gpu/drm/drm_edid.c        | 55 +++++++++++++++-------------
> > >  drivers/gpu/drm/panel/panel-edp.c | 60 ++++++++++++++++++++++++++-----
> > >  include/drm/drm_edid.h            |  3 +-
> > >  3 files changed, 84 insertions(+), 34 deletions(-)
> > >
> > > --
> > > 2.44.0.rc0.258.g7320e95886-goog
> > >
> >
> >
> > --
> > With best wishes
> > Dmitry