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(-)
--
2.44.0.rc0.258.g7320e95886-goog
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 <[email protected]>
---
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[] = {
+ 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
Hi,
On Fri, Feb 23, 2024 at 2:40 PM Hsin-Yi Wang <[email protected]> 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 <[email protected]> wrote:
>
> Hi,
>
> On Fri, Feb 23, 2024 at 2:40 PM Hsin-Yi Wang <[email protected]> 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 <[email protected]> wrote:
>
> On Mon, Feb 26, 2024 at 2:29 PM Doug Anderson <[email protected]> wrote:
> >
> > Hi,
> >
> > On Fri, Feb 23, 2024 at 2:40 PM Hsin-Yi Wang <[email protected]> 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 <[email protected]> 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
>
--
With best wishes
Dmitry
Hi,
On Mon, Feb 26, 2024 at 4:37 PM Dmitry Baryshkov
<[email protected]> wrote:
>
> On Sat, 24 Feb 2024 at 00:40, Hsin-Yi Wang <[email protected]> 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
<[email protected]> wrote:
>
> On Sat, 24 Feb 2024 at 00:40, Hsin-Yi Wang <[email protected]> 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 <[email protected]> wrote:
>
> Hi,
>
> On Mon, Feb 26, 2024 at 4:37 PM Dmitry Baryshkov
> <[email protected]> wrote:
> >
> > On Sat, 24 Feb 2024 at 00:40, Hsin-Yi Wang <[email protected]> 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.
--
With best wishes
Dmitry
On Sat, 24 Feb 2024 at 00:40, Hsin-Yi Wang <[email protected]> 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 <[email protected]>
> ---
> 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
>
--
With best wishes
Dmitry
On Tue, 27 Feb 2024 at 03:10, Hsin-Yi Wang <[email protected]> wrote:
>
> On Mon, Feb 26, 2024 at 4:37 PM Dmitry Baryshkov
> <[email protected]> wrote:
> >
> > On Sat, 24 Feb 2024 at 00:40, Hsin-Yi Wang <[email protected]> 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
--
With best wishes
Dmitry