[PATCH 0/2] Match panel hash for overridden mode

[PATCH 0/2] Match panel hash for overridden mode

[Hsin-Yi Wang]

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

[PATCH 1/2] drm_edid: Add a function to get EDID base block

[Hsin-Yi Wang]

It's found that some panels have variants that they share the same panel id
although their EDID and names are different. Besides panel id, now we need
the hash of entire EDID base block to distinguish these panel variants.

Add drm_edid_get_base_block to returns the EDID base block, so caller can
further use it to get panel id and/or the hash.

Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
---
 drivers/gpu/drm/drm_edid.c        | 55 +++++++++++++++++--------------
 drivers/gpu/drm/panel/panel-edp.c |  8 +++--
 include/drm/drm_edid.h            |  3 +-
 3 files changed, 38 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 923c4423151c..55598ca4a5d1 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2770,58 +2770,63 @@ static u32 edid_extract_panel_id(const struct edid *edid)
 }
 
 /**
- * drm_edid_get_panel_id - Get a panel's ID through DDC
- * @adapter: I2C adapter to use for DDC
+ * drm_edid_get_panel_id - Get a panel's ID from EDID base block
+ * @base_bock: EDID base block that contains panel ID.
  *
- * This function reads the first block of the EDID of a panel and (assuming
+ * This function uses the first block of the EDID of a panel and (assuming
  * that the EDID is valid) extracts the ID out of it. The ID is a 32-bit value
  * (16 bits of manufacturer ID and 16 bits of per-manufacturer ID) that's
  * supposed to be different for each different modem of panel.
  *
+ * Return: A 32-bit ID that should be different for each make/model of panel.
+ *         See the functions drm_edid_encode_panel_id() and
+ *         drm_edid_decode_panel_id() for some details on the structure of this
+ *         ID.
+ */
+u32 drm_edid_get_panel_id(void *base_block)
+{
+	return edid_extract_panel_id(base_block);
+}
+EXPORT_SYMBOL(drm_edid_get_panel_id);
+
+/**
+ * drm_edid_get_base_block - Get a panel's EDID base block
+ * @adapter: I2C adapter to use for DDC
+ *
+ * This function returns the first block of the EDID of a panel.
+ *
  * This function is intended to be used during early probing on devices where
  * more than one panel might be present. Because of its intended use it must
- * assume that the EDID of the panel is correct, at least as far as the ID
- * is concerned (in other words, we don't process any overrides here).
+ * assume that the EDID of the panel is correct, at least as far as the base
+ * block is concerned (in other words, we don't process any overrides here).
  *
  * NOTE: it's expected that this function and drm_do_get_edid() will both
  * be read the EDID, but there is no caching between them. Since we're only
  * reading the first block, hopefully this extra overhead won't be too big.
  *
- * Return: A 32-bit ID that should be different for each make/model of panel.
- *         See the functions drm_edid_encode_panel_id() and
- *         drm_edid_decode_panel_id() for some details on the structure of this
- *         ID.
+ * Caller should free the base block after use.
  */
-
-u32 drm_edid_get_panel_id(struct i2c_adapter *adapter)
+void *drm_edid_get_base_block(struct i2c_adapter *adapter)
 {
 	enum edid_block_status status;
 	void *base_block;
-	u32 panel_id = 0;
-
-	/*
-	 * There are no manufacturer IDs of 0, so if there is a problem reading
-	 * the EDID then we'll just return 0.
-	 */
 
 	base_block = kzalloc(EDID_LENGTH, GFP_KERNEL);
 	if (!base_block)
-		return 0;
+		return NULL;
 
 	status = edid_block_read(base_block, 0, drm_do_probe_ddc_edid, adapter);
 
 	edid_block_status_print(status, base_block, 0);
 
-	if (edid_block_status_valid(status, edid_block_tag(base_block)))
-		panel_id = edid_extract_panel_id(base_block);
-	else
+	if (!edid_block_status_valid(status, edid_block_tag(base_block))) {
 		edid_block_dump(KERN_NOTICE, base_block, 0);
+		return NULL;
+	}
 
-	kfree(base_block);
-
-	return panel_id;
+	return base_block;
 }
-EXPORT_SYMBOL(drm_edid_get_panel_id);
+EXPORT_SYMBOL(drm_edid_get_base_block);
 
 /**
  * drm_get_edid_switcheroo - get EDID data for a vga_switcheroo output
diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
index bd71d239272a..f6ddbaa103b5 100644
--- a/drivers/gpu/drm/panel/panel-edp.c
+++ b/drivers/gpu/drm/panel/panel-edp.c
@@ -763,6 +763,7 @@ static const struct edp_panel_entry *find_edp_panel(u32 panel_id);
 static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
 {
 	struct panel_desc *desc;
+	void *base_block;
 	u32 panel_id;
 	char vend[4];
 	u16 product_id;
@@ -792,8 +793,11 @@ static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
 		goto exit;
 	}
 
-	panel_id = drm_edid_get_panel_id(panel->ddc);
-	if (!panel_id) {
+	base_block = drm_edid_get_base_block(panel->ddc);
+	if (base_block) {
+		panel_id = drm_edid_get_panel_id(base_block);
+		kfree(base_block);
+	} else {
 		dev_err(dev, "Couldn't identify panel via EDID\n");
 		ret = -EIO;
 		goto exit;
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 7923bc00dc7a..56b60f9204d3 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -410,7 +410,8 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 	void *data);
 struct edid *drm_get_edid(struct drm_connector *connector,
 			  struct i2c_adapter *adapter);
-u32 drm_edid_get_panel_id(struct i2c_adapter *adapter);
+void *drm_edid_get_base_block(struct i2c_adapter *adapter);
+u32 drm_edid_get_panel_id(void *base_block);
 struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
 				     struct i2c_adapter *adapter);
 struct edid *drm_edid_duplicate(const struct edid *edid);
-- 
2.44.0.rc0.258.g7320e95886-goog

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

[Hsin-Yi Wang]

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[] = {
+	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

Re: [PATCH 1/2] drm_edid: Add a function to get EDID base block

[Doug Anderson]

Hi,

On Fri, Feb 23, 2024 at 2:40 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> @@ -2770,58 +2770,63 @@ static u32 edid_extract_panel_id(const struct edid *edid)
>  }
>
>  /**
> - * drm_edid_get_panel_id - Get a panel's ID through DDC
> - * @adapter: I2C adapter to use for DDC
> + * drm_edid_get_panel_id - Get a panel's ID from EDID base block
> + * @base_bock: EDID base block that contains panel ID.

s/base_bock/base_block, as identified by:

scripts/kernel-doc -v drivers/gpu/drm/drm_edid.c | less 2>&1

drivers/gpu/drm/drm_edid.c:2787: warning: Function parameter or struct
member 'base_block' not described in 'drm_edid_get_panel_id'
drivers/gpu/drm/drm_edid.c:2787: warning: Excess function parameter
'base_bock' description in 'drm_edid_get_panel_id'


>   *
> - * This function reads the first block of the EDID of a panel and (assuming
> + * This function uses the first block of the EDID of a panel and (assuming
>   * that the EDID is valid) extracts the ID out of it. The ID is a 32-bit value
>   * (16 bits of manufacturer ID and 16 bits of per-manufacturer ID) that's
>   * supposed to be different for each different modem of panel.
>   *
> + * Return: A 32-bit ID that should be different for each make/model of panel.
> + *         See the functions drm_edid_encode_panel_id() and
> + *         drm_edid_decode_panel_id() for some details on the structure of this
> + *         ID.
> + */
> +u32 drm_edid_get_panel_id(void *base_block)
> +{
> +       return edid_extract_panel_id(base_block);
> +}
> +EXPORT_SYMBOL(drm_edid_get_panel_id);
> +
> +/**
> + * drm_edid_get_base_block - Get a panel's EDID base block
> + * @adapter: I2C adapter to use for DDC
> + *
> + * This function returns the first block of the EDID of a panel.
> + *
>   * This function is intended to be used during early probing on devices where
>   * more than one panel might be present. Because of its intended use it must
> - * assume that the EDID of the panel is correct, at least as far as the ID
> - * is concerned (in other words, we don't process any overrides here).
> + * assume that the EDID of the panel is correct, at least as far as the base
> + * block is concerned (in other words, we don't process any overrides here).
>   *
>   * NOTE: it's expected that this function and drm_do_get_edid() will both
>   * be read the EDID, but there is no caching between them. Since we're only
>   * reading the first block, hopefully this extra overhead won't be too big.
>   *
> - * Return: A 32-bit ID that should be different for each make/model of panel.
> - *         See the functions drm_edid_encode_panel_id() and
> - *         drm_edid_decode_panel_id() for some details on the structure of this
> - *         ID.
> + * Caller should free the base block after use.

Don't you need a "Return:" clause here to document what you're returning?


Other than the kernel-doc nits, this looks fine to me.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

It'll probably need at least an Ack from someone else in the DRM
community before it can land, though, since this is touching a core
file.


-Doug

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

[Doug Anderson]

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

Re: [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@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

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

[Doug Anderson]

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

Re: [PATCH 0/2] Match panel hash for overridden mode

[Dmitry Baryshkov]

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
>


-- 
With best wishes
Dmitry

Re: [PATCH 0/2] Match panel hash for overridden mode

[Doug Anderson]

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

Re: [PATCH 0/2] Match panel hash for overridden mode

[Hsin-Yi Wang]

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

Re: [PATCH 0/2] Match panel hash for overridden mode

[Dmitry Baryshkov]

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.

-- 
With best wishes
Dmitry

Re: [PATCH 0/2] Match panel hash for overridden mode

[Dmitry Baryshkov]

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



-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

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
>


-- 
With best wishes
Dmitry

Re: [PATCH 1/2] drm_edid: Add a function to get EDID base block

[Jani Nikula]

On Fri, 23 Feb 2024, 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. Besides panel id, now we need
> the hash of entire EDID base block to distinguish these panel variants.
>
> Add drm_edid_get_base_block to returns the EDID base block, so caller can
> further use it to get panel id and/or the hash.

Please reconsider the whole approach here.

Please let's not add single-use special case functions to read an EDID
base block.

Please consider reading the whole EDID, using the regular EDID reading
functions, and use that instead.

Most likely you'll only have 1-2 blocks anyway. And you might consider
caching the EDID in struct panel_edp if reading the entire EDID is too
slow. (And if it is, this is probably sensible even if the EDID only
consists of one block.)

Anyway, please do *not* merge this as-is.

BR,
Jani.

>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
>  drivers/gpu/drm/drm_edid.c        | 55 +++++++++++++++++--------------
>  drivers/gpu/drm/panel/panel-edp.c |  8 +++--
>  include/drm/drm_edid.h            |  3 +-
>  3 files changed, 38 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 923c4423151c..55598ca4a5d1 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2770,58 +2770,63 @@ static u32 edid_extract_panel_id(const struct edid *edid)
>  }
>  
>  /**
> - * drm_edid_get_panel_id - Get a panel's ID through DDC
> - * @adapter: I2C adapter to use for DDC
> + * drm_edid_get_panel_id - Get a panel's ID from EDID base block
> + * @base_bock: EDID base block that contains panel ID.
>   *
> - * This function reads the first block of the EDID of a panel and (assuming
> + * This function uses the first block of the EDID of a panel and (assuming
>   * that the EDID is valid) extracts the ID out of it. The ID is a 32-bit value
>   * (16 bits of manufacturer ID and 16 bits of per-manufacturer ID) that's
>   * supposed to be different for each different modem of panel.
>   *
> + * Return: A 32-bit ID that should be different for each make/model of panel.
> + *         See the functions drm_edid_encode_panel_id() and
> + *         drm_edid_decode_panel_id() for some details on the structure of this
> + *         ID.
> + */
> +u32 drm_edid_get_panel_id(void *base_block)
> +{
> +	return edid_extract_panel_id(base_block);
> +}
> +EXPORT_SYMBOL(drm_edid_get_panel_id);
> +
> +/**
> + * drm_edid_get_base_block - Get a panel's EDID base block
> + * @adapter: I2C adapter to use for DDC
> + *
> + * This function returns the first block of the EDID of a panel.
> + *
>   * This function is intended to be used during early probing on devices where
>   * more than one panel might be present. Because of its intended use it must
> - * assume that the EDID of the panel is correct, at least as far as the ID
> - * is concerned (in other words, we don't process any overrides here).
> + * assume that the EDID of the panel is correct, at least as far as the base
> + * block is concerned (in other words, we don't process any overrides here).
>   *
>   * NOTE: it's expected that this function and drm_do_get_edid() will both
>   * be read the EDID, but there is no caching between them. Since we're only
>   * reading the first block, hopefully this extra overhead won't be too big.
>   *
> - * Return: A 32-bit ID that should be different for each make/model of panel.
> - *         See the functions drm_edid_encode_panel_id() and
> - *         drm_edid_decode_panel_id() for some details on the structure of this
> - *         ID.
> + * Caller should free the base block after use.
>   */
> -
> -u32 drm_edid_get_panel_id(struct i2c_adapter *adapter)
> +void *drm_edid_get_base_block(struct i2c_adapter *adapter)
>  {
>  	enum edid_block_status status;
>  	void *base_block;
> -	u32 panel_id = 0;
> -
> -	/*
> -	 * There are no manufacturer IDs of 0, so if there is a problem reading
> -	 * the EDID then we'll just return 0.
> -	 */
>  
>  	base_block = kzalloc(EDID_LENGTH, GFP_KERNEL);
>  	if (!base_block)
> -		return 0;
> +		return NULL;
>  
>  	status = edid_block_read(base_block, 0, drm_do_probe_ddc_edid, adapter);
>  
>  	edid_block_status_print(status, base_block, 0);
>  
> -	if (edid_block_status_valid(status, edid_block_tag(base_block)))
> -		panel_id = edid_extract_panel_id(base_block);
> -	else
> +	if (!edid_block_status_valid(status, edid_block_tag(base_block))) {
>  		edid_block_dump(KERN_NOTICE, base_block, 0);
> +		return NULL;
> +	}
>  
> -	kfree(base_block);
> -
> -	return panel_id;
> +	return base_block;
>  }
> -EXPORT_SYMBOL(drm_edid_get_panel_id);
> +EXPORT_SYMBOL(drm_edid_get_base_block);
>  
>  /**
>   * drm_get_edid_switcheroo - get EDID data for a vga_switcheroo output
> diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
> index bd71d239272a..f6ddbaa103b5 100644
> --- a/drivers/gpu/drm/panel/panel-edp.c
> +++ b/drivers/gpu/drm/panel/panel-edp.c
> @@ -763,6 +763,7 @@ static const struct edp_panel_entry *find_edp_panel(u32 panel_id);
>  static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
>  {
>  	struct panel_desc *desc;
> +	void *base_block;
>  	u32 panel_id;
>  	char vend[4];
>  	u16 product_id;
> @@ -792,8 +793,11 @@ static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
>  		goto exit;
>  	}
>  
> -	panel_id = drm_edid_get_panel_id(panel->ddc);
> -	if (!panel_id) {
> +	base_block = drm_edid_get_base_block(panel->ddc);
> +	if (base_block) {
> +		panel_id = drm_edid_get_panel_id(base_block);
> +		kfree(base_block);
> +	} else {
>  		dev_err(dev, "Couldn't identify panel via EDID\n");
>  		ret = -EIO;
>  		goto exit;
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 7923bc00dc7a..56b60f9204d3 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -410,7 +410,8 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>  	void *data);
>  struct edid *drm_get_edid(struct drm_connector *connector,
>  			  struct i2c_adapter *adapter);
> -u32 drm_edid_get_panel_id(struct i2c_adapter *adapter);
> +void *drm_edid_get_base_block(struct i2c_adapter *adapter);
> +u32 drm_edid_get_panel_id(void *base_block);
>  struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
>  				     struct i2c_adapter *adapter);
>  struct edid *drm_edid_duplicate(const struct edid *edid);

-- 
Jani Nikula, Intel

Re: [PATCH 1/2] drm_edid: Add a function to get EDID base block

[Doug Anderson]

Hi,

On Tue, Feb 27, 2024 at 1:09 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
> On Fri, 23 Feb 2024, 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. Besides panel id, now we need
> > the hash of entire EDID base block to distinguish these panel variants.
> >
> > Add drm_edid_get_base_block to returns the EDID base block, so caller can
> > further use it to get panel id and/or the hash.
>
> Please reconsider the whole approach here.
>
> Please let's not add single-use special case functions to read an EDID
> base block.
>
> Please consider reading the whole EDID, using the regular EDID reading
> functions, and use that instead.
>
> Most likely you'll only have 1-2 blocks anyway. And you might consider
> caching the EDID in struct panel_edp if reading the entire EDID is too
> slow. (And if it is, this is probably sensible even if the EDID only
> consists of one block.)

That makes a lot of sense! Not quite sure why I didn't just read the
whole EDID in the first place when trying to get the panel ID.

-Doug

Re: [PATCH 1/2] drm_edid: Add a function to get EDID base block

[Hsin-Yi Wang]

On Tue, Feb 27, 2024 at 1:09 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
> On Fri, 23 Feb 2024, 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. Besides panel id, now we need
> > the hash of entire EDID base block to distinguish these panel variants.
> >
> > Add drm_edid_get_base_block to returns the EDID base block, so caller can
> > further use it to get panel id and/or the hash.
>
> Please reconsider the whole approach here.
>
> Please let's not add single-use special case functions to read an EDID
> base block.
>
> Please consider reading the whole EDID, using the regular EDID reading
> functions, and use that instead.
>
> Most likely you'll only have 1-2 blocks anyway. And you might consider
> caching the EDID in struct panel_edp if reading the entire EDID is too
> slow. (And if it is, this is probably sensible even if the EDID only
> consists of one block.)
>
> Anyway, please do *not* merge this as-is.
>

hi Jani,

I sent a v2 here implementing this method:
https://lore.kernel.org/lkml/20240228011133.1238439-2-hsinyi@chromium.org/

We still have to read edid twice due to:
1. The first caller is in panel probe, at that time, connector is
still unknown, so we can't update connector status (eg. update
edid_corrupt).
2. It's possible that the connector can have some override
(drm_edid_override_get) to EDID, that is still unknown during the
first read.

> BR,
> Jani.
>
> >
> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > ---
> >  drivers/gpu/drm/drm_edid.c        | 55 +++++++++++++++++--------------
> >  drivers/gpu/drm/panel/panel-edp.c |  8 +++--
> >  include/drm/drm_edid.h            |  3 +-
> >  3 files changed, 38 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 923c4423151c..55598ca4a5d1 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -2770,58 +2770,63 @@ static u32 edid_extract_panel_id(const struct edid *edid)
> >  }
> >
> >  /**
> > - * drm_edid_get_panel_id - Get a panel's ID through DDC
> > - * @adapter: I2C adapter to use for DDC
> > + * drm_edid_get_panel_id - Get a panel's ID from EDID base block
> > + * @base_bock: EDID base block that contains panel ID.
> >   *
> > - * This function reads the first block of the EDID of a panel and (assuming
> > + * This function uses the first block of the EDID of a panel and (assuming
> >   * that the EDID is valid) extracts the ID out of it. The ID is a 32-bit value
> >   * (16 bits of manufacturer ID and 16 bits of per-manufacturer ID) that's
> >   * supposed to be different for each different modem of panel.
> >   *
> > + * Return: A 32-bit ID that should be different for each make/model of panel.
> > + *         See the functions drm_edid_encode_panel_id() and
> > + *         drm_edid_decode_panel_id() for some details on the structure of this
> > + *         ID.
> > + */
> > +u32 drm_edid_get_panel_id(void *base_block)
> > +{
> > +     return edid_extract_panel_id(base_block);
> > +}
> > +EXPORT_SYMBOL(drm_edid_get_panel_id);
> > +
> > +/**
> > + * drm_edid_get_base_block - Get a panel's EDID base block
> > + * @adapter: I2C adapter to use for DDC
> > + *
> > + * This function returns the first block of the EDID of a panel.
> > + *
> >   * This function is intended to be used during early probing on devices where
> >   * more than one panel might be present. Because of its intended use it must
> > - * assume that the EDID of the panel is correct, at least as far as the ID
> > - * is concerned (in other words, we don't process any overrides here).
> > + * assume that the EDID of the panel is correct, at least as far as the base
> > + * block is concerned (in other words, we don't process any overrides here).
> >   *
> >   * NOTE: it's expected that this function and drm_do_get_edid() will both
> >   * be read the EDID, but there is no caching between them. Since we're only
> >   * reading the first block, hopefully this extra overhead won't be too big.
> >   *
> > - * Return: A 32-bit ID that should be different for each make/model of panel.
> > - *         See the functions drm_edid_encode_panel_id() and
> > - *         drm_edid_decode_panel_id() for some details on the structure of this
> > - *         ID.
> > + * Caller should free the base block after use.
> >   */
> > -
> > -u32 drm_edid_get_panel_id(struct i2c_adapter *adapter)
> > +void *drm_edid_get_base_block(struct i2c_adapter *adapter)
> >  {
> >       enum edid_block_status status;
> >       void *base_block;
> > -     u32 panel_id = 0;
> > -
> > -     /*
> > -      * There are no manufacturer IDs of 0, so if there is a problem reading
> > -      * the EDID then we'll just return 0.
> > -      */
> >
> >       base_block = kzalloc(EDID_LENGTH, GFP_KERNEL);
> >       if (!base_block)
> > -             return 0;
> > +             return NULL;
> >
> >       status = edid_block_read(base_block, 0, drm_do_probe_ddc_edid, adapter);
> >
> >       edid_block_status_print(status, base_block, 0);
> >
> > -     if (edid_block_status_valid(status, edid_block_tag(base_block)))
> > -             panel_id = edid_extract_panel_id(base_block);
> > -     else
> > +     if (!edid_block_status_valid(status, edid_block_tag(base_block))) {
> >               edid_block_dump(KERN_NOTICE, base_block, 0);
> > +             return NULL;
> > +     }
> >
> > -     kfree(base_block);
> > -
> > -     return panel_id;
> > +     return base_block;
> >  }
> > -EXPORT_SYMBOL(drm_edid_get_panel_id);
> > +EXPORT_SYMBOL(drm_edid_get_base_block);
> >
> >  /**
> >   * drm_get_edid_switcheroo - get EDID data for a vga_switcheroo output
> > diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
> > index bd71d239272a..f6ddbaa103b5 100644
> > --- a/drivers/gpu/drm/panel/panel-edp.c
> > +++ b/drivers/gpu/drm/panel/panel-edp.c
> > @@ -763,6 +763,7 @@ static const struct edp_panel_entry *find_edp_panel(u32 panel_id);
> >  static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
> >  {
> >       struct panel_desc *desc;
> > +     void *base_block;
> >       u32 panel_id;
> >       char vend[4];
> >       u16 product_id;
> > @@ -792,8 +793,11 @@ static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
> >               goto exit;
> >       }
> >
> > -     panel_id = drm_edid_get_panel_id(panel->ddc);
> > -     if (!panel_id) {
> > +     base_block = drm_edid_get_base_block(panel->ddc);
> > +     if (base_block) {
> > +             panel_id = drm_edid_get_panel_id(base_block);
> > +             kfree(base_block);
> > +     } else {
> >               dev_err(dev, "Couldn't identify panel via EDID\n");
> >               ret = -EIO;
> >               goto exit;
> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> > index 7923bc00dc7a..56b60f9204d3 100644
> > --- a/include/drm/drm_edid.h
> > +++ b/include/drm/drm_edid.h
> > @@ -410,7 +410,8 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
> >       void *data);
> >  struct edid *drm_get_edid(struct drm_connector *connector,
> >                         struct i2c_adapter *adapter);
> > -u32 drm_edid_get_panel_id(struct i2c_adapter *adapter);
> > +void *drm_edid_get_base_block(struct i2c_adapter *adapter);
> > +u32 drm_edid_get_panel_id(void *base_block);
> >  struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
> >                                    struct i2c_adapter *adapter);
> >  struct edid *drm_edid_duplicate(const struct edid *edid);
>
> --
> Jani Nikula, Intel

Re: [PATCH 1/2] drm_edid: Add a function to get EDID base block

[Doug Anderson]

Hi,

On Tue, Feb 27, 2024 at 5:27 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> On Tue, Feb 27, 2024 at 1:09 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >
> > On Fri, 23 Feb 2024, 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. Besides panel id, now we need
> > > the hash of entire EDID base block to distinguish these panel variants.
> > >
> > > Add drm_edid_get_base_block to returns the EDID base block, so caller can
> > > further use it to get panel id and/or the hash.
> >
> > Please reconsider the whole approach here.
> >
> > Please let's not add single-use special case functions to read an EDID
> > base block.
> >
> > Please consider reading the whole EDID, using the regular EDID reading
> > functions, and use that instead.
> >
> > Most likely you'll only have 1-2 blocks anyway. And you might consider
> > caching the EDID in struct panel_edp if reading the entire EDID is too
> > slow. (And if it is, this is probably sensible even if the EDID only
> > consists of one block.)
> >
> > Anyway, please do *not* merge this as-is.
> >
>
> hi Jani,
>
> I sent a v2 here implementing this method:
> https://lore.kernel.org/lkml/20240228011133.1238439-2-hsinyi@chromium.org/
>
> We still have to read edid twice due to:
> 1. The first caller is in panel probe, at that time, connector is
> still unknown, so we can't update connector status (eg. update
> edid_corrupt).
> 2. It's possible that the connector can have some override
> (drm_edid_override_get) to EDID, that is still unknown during the
> first read.

I'll also comment in Hsin-Yi's v2, but given Hsin-Yi's digging and the
fact that we can't cache the EDID (because we don't yet have a
"drm_connector"), I'd much prefer Hsin-Yi's solution here from v1 that
allows reading just the first block. If we try to boot a device with a
multi-block EDID we're now wastefully reading all the blocks of the
EDID twice at bootup which will slow boot time.

If you can see a good solution to avoid reading the EDID twice then
that would be amazing, but if not it seems like we should go back to
what's here in v1. What do you think? Anyone else have any opinions?



-Doug

Re: [PATCH 1/2] drm_edid: Add a function to get EDID base block

[Jani Nikula]

On Wed, 28 Feb 2024, Doug Anderson <dianders@chromium.org> wrote:
> Hi,
>
> On Tue, Feb 27, 2024 at 5:27 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>>
>> On Tue, Feb 27, 2024 at 1:09 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> >
>> > On Fri, 23 Feb 2024, 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. Besides panel id, now we need
>> > > the hash of entire EDID base block to distinguish these panel variants.
>> > >
>> > > Add drm_edid_get_base_block to returns the EDID base block, so caller can
>> > > further use it to get panel id and/or the hash.
>> >
>> > Please reconsider the whole approach here.
>> >
>> > Please let's not add single-use special case functions to read an EDID
>> > base block.
>> >
>> > Please consider reading the whole EDID, using the regular EDID reading
>> > functions, and use that instead.
>> >
>> > Most likely you'll only have 1-2 blocks anyway. And you might consider
>> > caching the EDID in struct panel_edp if reading the entire EDID is too
>> > slow. (And if it is, this is probably sensible even if the EDID only
>> > consists of one block.)
>> >
>> > Anyway, please do *not* merge this as-is.
>> >
>>
>> hi Jani,
>>
>> I sent a v2 here implementing this method:
>> https://lore.kernel.org/lkml/20240228011133.1238439-2-hsinyi@chromium.org/
>>
>> We still have to read edid twice due to:
>> 1. The first caller is in panel probe, at that time, connector is
>> still unknown, so we can't update connector status (eg. update
>> edid_corrupt).
>> 2. It's possible that the connector can have some override
>> (drm_edid_override_get) to EDID, that is still unknown during the
>> first read.
>
> I'll also comment in Hsin-Yi's v2, but given Hsin-Yi's digging and the
> fact that we can't cache the EDID (because we don't yet have a
> "drm_connector"), I'd much prefer Hsin-Yi's solution here from v1 that
> allows reading just the first block. If we try to boot a device with a
> multi-block EDID we're now wastefully reading all the blocks of the
> EDID twice at bootup which will slow boot time.
>
> If you can see a good solution to avoid reading the EDID twice then
> that would be amazing, but if not it seems like we should go back to
> what's here in v1. What do you think? Anyone else have any opinions?

I haven't replied so far, because I've been going back and forth with
this. I'm afraid I don't really like either approach now. Handling the
no connector case in v2 is a bit ugly too. :(

Seems like you only need this to extend the panel ID with a hash. And
panel-edp.c is the only user of drm_edid_get_panel_id(). And EDID quirks
in drm_edid.c could theoretically hit the same problem you're solving.

So maybe something like:

	u32 drm_edid_get_panel_id(struct i2c_adapter *adapter, u32 *hash);

or if you want to be fancy add a struct capturing both id and hash:

	bool drm_edid_get_panel_id(struct i2c_adapter *adapter, struct drm_edid_panel_id *id);

And put the hash (or whatever mechanism you have) computation in
drm_edid.c. Just hide it all in drm_edid.c, and keep the EDID interfaces
neat.

How would that work for you?


BR,
Jani.


-- 
Jani Nikula, Intel

Re: [PATCH 1/2] drm_edid: Add a function to get EDID base block

[Doug Anderson]

Hi,

On Thu, Feb 29, 2024 at 8:43 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
> On Wed, 28 Feb 2024, Doug Anderson <dianders@chromium.org> wrote:
> > Hi,
> >
> > On Tue, Feb 27, 2024 at 5:27 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> >>
> >> On Tue, Feb 27, 2024 at 1:09 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >> >
> >> > On Fri, 23 Feb 2024, 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. Besides panel id, now we need
> >> > > the hash of entire EDID base block to distinguish these panel variants.
> >> > >
> >> > > Add drm_edid_get_base_block to returns the EDID base block, so caller can
> >> > > further use it to get panel id and/or the hash.
> >> >
> >> > Please reconsider the whole approach here.
> >> >
> >> > Please let's not add single-use special case functions to read an EDID
> >> > base block.
> >> >
> >> > Please consider reading the whole EDID, using the regular EDID reading
> >> > functions, and use that instead.
> >> >
> >> > Most likely you'll only have 1-2 blocks anyway. And you might consider
> >> > caching the EDID in struct panel_edp if reading the entire EDID is too
> >> > slow. (And if it is, this is probably sensible even if the EDID only
> >> > consists of one block.)
> >> >
> >> > Anyway, please do *not* merge this as-is.
> >> >
> >>
> >> hi Jani,
> >>
> >> I sent a v2 here implementing this method:
> >> https://lore.kernel.org/lkml/20240228011133.1238439-2-hsinyi@chromium.org/
> >>
> >> We still have to read edid twice due to:
> >> 1. The first caller is in panel probe, at that time, connector is
> >> still unknown, so we can't update connector status (eg. update
> >> edid_corrupt).
> >> 2. It's possible that the connector can have some override
> >> (drm_edid_override_get) to EDID, that is still unknown during the
> >> first read.
> >
> > I'll also comment in Hsin-Yi's v2, but given Hsin-Yi's digging and the
> > fact that we can't cache the EDID (because we don't yet have a
> > "drm_connector"), I'd much prefer Hsin-Yi's solution here from v1 that
> > allows reading just the first block. If we try to boot a device with a
> > multi-block EDID we're now wastefully reading all the blocks of the
> > EDID twice at bootup which will slow boot time.
> >
> > If you can see a good solution to avoid reading the EDID twice then
> > that would be amazing, but if not it seems like we should go back to
> > what's here in v1. What do you think? Anyone else have any opinions?
>
> I haven't replied so far, because I've been going back and forth with
> this. I'm afraid I don't really like either approach now. Handling the
> no connector case in v2 is a bit ugly too. :(
>
> Seems like you only need this to extend the panel ID with a hash. And
> panel-edp.c is the only user of drm_edid_get_panel_id(). And EDID quirks
> in drm_edid.c could theoretically hit the same problem you're solving.

Good point. That would imply that more of the logic could go into
"drm_edid.c" in case the EDID quirks code eventually needs it.


> So maybe something like:
>
>         u32 drm_edid_get_panel_id(struct i2c_adapter *adapter, u32 *hash);
>
> or if you want to be fancy add a struct capturing both id and hash:
>
>         bool drm_edid_get_panel_id(struct i2c_adapter *adapter, struct drm_edid_panel_id *id);
>
> And put the hash (or whatever mechanism you have) computation in
> drm_edid.c. Just hide it all in drm_edid.c, and keep the EDID interfaces
> neat.
>
> How would that work for you?

The problem is that Dmitry didn't like the idea of using a hash and in
v2 Hsin-Yi has moved to using the name of the display. ...except of
course that eDP panels don't always properly specify
"EDID_DETAIL_MONITOR_NAME". See the discussion [1]. If you want to see
some of the EDIDs involved, you can see Hsin-Yi's post [2]. The panels
included stuff like this:

    Alphanumeric Data String: 'AUO'
    Alphanumeric Data String: 'B116XAN04.0 '

The fact that there is more than one string in there makes it hard to
just "return" the display name in a generic way. The way Hsin-Yi's
code was doing it was that it would consider it a match if the panel
name was in any of the strings...

How about this as a solution: we change drm_edid_get_panel_id() to
return an opaque type (struct drm_edid_panel_id_blob) that's really
just the first block of the EDID but we can all pretend that it isn't.
Then we can add a function in drm_edid.c that takes this opaque blob,
a 32-bit integer (as per drm_edid_encode_panel_id()), and a string
name and it can tell us if the blob matches?


[1] https://lore.kernel.org/r/CAD=FV=VMVr+eJ7eyuLGa671fMgH6ZX9zPOkbKzYJ0H79MZ2k9A@mail.gmail.com
[2] https://lore.kernel.org/r/CAJMQK-gfKbdPhYJeCJ5UX0dNrx3y-EmLsTiv9nj+U3Rmej38pw@mail.gmail.com

Re: [PATCH 1/2] drm_edid: Add a function to get EDID base block

[Dmitry Baryshkov]

On Thu, 29 Feb 2024 at 19:11, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Feb 29, 2024 at 8:43 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >
> > On Wed, 28 Feb 2024, Doug Anderson <dianders@chromium.org> wrote:
> > > Hi,
> > >
> > > On Tue, Feb 27, 2024 at 5:27 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> > >>
> > >> On Tue, Feb 27, 2024 at 1:09 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > >> >
> > >> > On Fri, 23 Feb 2024, 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. Besides panel id, now we need
> > >> > > the hash of entire EDID base block to distinguish these panel variants.
> > >> > >
> > >> > > Add drm_edid_get_base_block to returns the EDID base block, so caller can
> > >> > > further use it to get panel id and/or the hash.
> > >> >
> > >> > Please reconsider the whole approach here.
> > >> >
> > >> > Please let's not add single-use special case functions to read an EDID
> > >> > base block.
> > >> >
> > >> > Please consider reading the whole EDID, using the regular EDID reading
> > >> > functions, and use that instead.
> > >> >
> > >> > Most likely you'll only have 1-2 blocks anyway. And you might consider
> > >> > caching the EDID in struct panel_edp if reading the entire EDID is too
> > >> > slow. (And if it is, this is probably sensible even if the EDID only
> > >> > consists of one block.)
> > >> >
> > >> > Anyway, please do *not* merge this as-is.
> > >> >
> > >>
> > >> hi Jani,
> > >>
> > >> I sent a v2 here implementing this method:
> > >> https://lore.kernel.org/lkml/20240228011133.1238439-2-hsinyi@chromium.org/
> > >>
> > >> We still have to read edid twice due to:
> > >> 1. The first caller is in panel probe, at that time, connector is
> > >> still unknown, so we can't update connector status (eg. update
> > >> edid_corrupt).
> > >> 2. It's possible that the connector can have some override
> > >> (drm_edid_override_get) to EDID, that is still unknown during the
> > >> first read.
> > >
> > > I'll also comment in Hsin-Yi's v2, but given Hsin-Yi's digging and the
> > > fact that we can't cache the EDID (because we don't yet have a
> > > "drm_connector"), I'd much prefer Hsin-Yi's solution here from v1 that
> > > allows reading just the first block. If we try to boot a device with a
> > > multi-block EDID we're now wastefully reading all the blocks of the
> > > EDID twice at bootup which will slow boot time.
> > >
> > > If you can see a good solution to avoid reading the EDID twice then
> > > that would be amazing, but if not it seems like we should go back to
> > > what's here in v1. What do you think? Anyone else have any opinions?
> >
> > I haven't replied so far, because I've been going back and forth with
> > this. I'm afraid I don't really like either approach now. Handling the
> > no connector case in v2 is a bit ugly too. :(
> >
> > Seems like you only need this to extend the panel ID with a hash. And
> > panel-edp.c is the only user of drm_edid_get_panel_id(). And EDID quirks
> > in drm_edid.c could theoretically hit the same problem you're solving.
>
> Good point. That would imply that more of the logic could go into
> "drm_edid.c" in case the EDID quirks code eventually needs it.
>
>
> > So maybe something like:
> >
> >         u32 drm_edid_get_panel_id(struct i2c_adapter *adapter, u32 *hash);
> >
> > or if you want to be fancy add a struct capturing both id and hash:
> >
> >         bool drm_edid_get_panel_id(struct i2c_adapter *adapter, struct drm_edid_panel_id *id);
> >
> > And put the hash (or whatever mechanism you have) computation in
> > drm_edid.c. Just hide it all in drm_edid.c, and keep the EDID interfaces
> > neat.
> >
> > How would that work for you?
>
> The problem is that Dmitry didn't like the idea of using a hash and in
> v2 Hsin-Yi has moved to using the name of the display. ...except of
> course that eDP panels don't always properly specify
> "EDID_DETAIL_MONITOR_NAME". See the discussion [1]. If you want to see
> some of the EDIDs involved, you can see Hsin-Yi's post [2]. The panels
> included stuff like this:
>
>     Alphanumeric Data String: 'AUO'
>     Alphanumeric Data String: 'B116XAN04.0 '
>
> The fact that there is more than one string in there makes it hard to
> just "return" the display name in a generic way. The way Hsin-Yi's
> code was doing it was that it would consider it a match if the panel
> name was in any of the strings...
>
> How about this as a solution: we change drm_edid_get_panel_id() to
> return an opaque type (struct drm_edid_panel_id_blob) that's really
> just the first block of the EDID but we can all pretend that it isn't.
> Then we can add a function in drm_edid.c that takes this opaque blob,
> a 32-bit integer (as per drm_edid_encode_panel_id()), and a string
> name and it can tell us if the blob matches?

Would it be easier to push drm_edid_match to drm_edid.c? It looks way
more simpler than the opaque blob.

> [1] https://lore.kernel.org/r/CAD=FV=VMVr+eJ7eyuLGa671fMgH6ZX9zPOkbKzYJ0H79MZ2k9A@mail.gmail.com
> [2] https://lore.kernel.org/r/CAJMQK-gfKbdPhYJeCJ5UX0dNrx3y-EmLsTiv9nj+U3Rmej38pw@mail.gmail.com



-- 
With best wishes
Dmitry

Re: [PATCH 1/2] drm_edid: Add a function to get EDID base block

[Doug Anderson]

Hi,

On Sun, Mar 3, 2024 at 1:30 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> > The problem is that Dmitry didn't like the idea of using a hash and in
> > v2 Hsin-Yi has moved to using the name of the display. ...except of
> > course that eDP panels don't always properly specify
> > "EDID_DETAIL_MONITOR_NAME". See the discussion [1]. If you want to see
> > some of the EDIDs involved, you can see Hsin-Yi's post [2]. The panels
> > included stuff like this:
> >
> >     Alphanumeric Data String: 'AUO'
> >     Alphanumeric Data String: 'B116XAN04.0 '
> >
> > The fact that there is more than one string in there makes it hard to
> > just "return" the display name in a generic way. The way Hsin-Yi's
> > code was doing it was that it would consider it a match if the panel
> > name was in any of the strings...
> >
> > How about this as a solution: we change drm_edid_get_panel_id() to
> > return an opaque type (struct drm_edid_panel_id_blob) that's really
> > just the first block of the EDID but we can all pretend that it isn't.
> > Then we can add a function in drm_edid.c that takes this opaque blob,
> > a 32-bit integer (as per drm_edid_encode_panel_id()), and a string
> > name and it can tell us if the blob matches?
>
> Would it be easier to push drm_edid_match to drm_edid.c? It looks way
> more simpler than the opaque blob.

Yeah, that sounds reasonable / cleaner to me. Good idea! Maybe Hsin-Yi
will be able to try this out and see if there's a reason it wouldn't
work.

-Doug

Re: [PATCH 1/2] drm_edid: Add a function to get EDID base block

[Hsin-Yi Wang]

On Mon, Mar 4, 2024 at 8:17 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Sun, Mar 3, 2024 at 1:30 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > > The problem is that Dmitry didn't like the idea of using a hash and in
> > > v2 Hsin-Yi has moved to using the name of the display. ...except of
> > > course that eDP panels don't always properly specify
> > > "EDID_DETAIL_MONITOR_NAME". See the discussion [1]. If you want to see
> > > some of the EDIDs involved, you can see Hsin-Yi's post [2]. The panels
> > > included stuff like this:
> > >
> > >     Alphanumeric Data String: 'AUO'
> > >     Alphanumeric Data String: 'B116XAN04.0 '
> > >
> > > The fact that there is more than one string in there makes it hard to
> > > just "return" the display name in a generic way. The way Hsin-Yi's
> > > code was doing it was that it would consider it a match if the panel
> > > name was in any of the strings...
> > >
> > > How about this as a solution: we change drm_edid_get_panel_id() to
> > > return an opaque type (struct drm_edid_panel_id_blob) that's really
> > > just the first block of the EDID but we can all pretend that it isn't.
> > > Then we can add a function in drm_edid.c that takes this opaque blob,
> > > a 32-bit integer (as per drm_edid_encode_panel_id()), and a string
> > > name and it can tell us if the blob matches?
> >
> > Would it be easier to push drm_edid_match to drm_edid.c? It looks way
> > more simpler than the opaque blob.
>
> Yeah, that sounds reasonable / cleaner to me. Good idea! Maybe Hsin-Yi
> will be able to try this out and see if there's a reason it wouldn't
> work.

Thanks for all the suggestions. I sent out v3, which still has a blob
type since we need
1. get panel id
2. do the string matching.

And I felt that packing these 2 steps into one function may make that
function do multiple tasks?

But let me know if it's preferred in this way.

v3: https://lore.kernel.org/lkml/20240304195214.14563-1-hsinyi@chromium.org/

>
> -Doug

Re: [PATCH 1/2] drm_edid: Add a function to get EDID base block

[Jani Nikula]

On Mon, 04 Mar 2024, Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> On Mon, Mar 4, 2024 at 8:17 AM Doug Anderson <dianders@chromium.org> wrote:
>>
>> Hi,
>>
>> On Sun, Mar 3, 2024 at 1:30 PM Dmitry Baryshkov
>> <dmitry.baryshkov@linaro.org> wrote:
>> >
>> > > The problem is that Dmitry didn't like the idea of using a hash and in
>> > > v2 Hsin-Yi has moved to using the name of the display. ...except of
>> > > course that eDP panels don't always properly specify
>> > > "EDID_DETAIL_MONITOR_NAME". See the discussion [1]. If you want to see
>> > > some of the EDIDs involved, you can see Hsin-Yi's post [2]. The panels
>> > > included stuff like this:
>> > >
>> > >     Alphanumeric Data String: 'AUO'
>> > >     Alphanumeric Data String: 'B116XAN04.0 '
>> > >
>> > > The fact that there is more than one string in there makes it hard to
>> > > just "return" the display name in a generic way. The way Hsin-Yi's
>> > > code was doing it was that it would consider it a match if the panel
>> > > name was in any of the strings...
>> > >
>> > > How about this as a solution: we change drm_edid_get_panel_id() to
>> > > return an opaque type (struct drm_edid_panel_id_blob) that's really
>> > > just the first block of the EDID but we can all pretend that it isn't.
>> > > Then we can add a function in drm_edid.c that takes this opaque blob,
>> > > a 32-bit integer (as per drm_edid_encode_panel_id()), and a string
>> > > name and it can tell us if the blob matches?
>> >
>> > Would it be easier to push drm_edid_match to drm_edid.c? It looks way
>> > more simpler than the opaque blob.
>>
>> Yeah, that sounds reasonable / cleaner to me. Good idea! Maybe Hsin-Yi
>> will be able to try this out and see if there's a reason it wouldn't
>> work.
>
> Thanks for all the suggestions. I sent out v3, which still has a blob
> type since we need
> 1. get panel id
> 2. do the string matching.
>
> And I felt that packing these 2 steps into one function may make that
> function do multiple tasks?
>
> But let me know if it's preferred in this way.
>
> v3: https://lore.kernel.org/lkml/20240304195214.14563-1-hsinyi@chromium.org/

I still don't like it, but incorporating all the ideas here, and in the
previous patches, I think I have a suggestion that covers all cases in a
reasonable manner [1].

Sorry about being so inflexible about this. I've just put 140+ commits
worth of effort in drm_edid.c in the past couple of years, and I'm keen
on keeping it nice and tidy. :)


BR,
Jani.


[1] https://lore.kernel.org/r/87a5nd4tsg.fsf@intel.com


>
>>
>> -Doug

-- 
Jani Nikula, Intel