[PATCH v2 0/3] Match panel with id and name

[PATCH v2 0/3] Match panel with id and name

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

Match the panel for both name and id. If not found, fallback to match id.

v1: https://lore.kernel.org/lkml/20240223223958.3887423-1-hsinyi@chromium.org/T/

Hsin-Yi Wang (3):
  drm_edid: Support getting EDID through ddc without connector
  drm/panel: panel-edp: Match edp_panels with panel name
  drm/panel: panel-edp: Fix AUO 0x405c panel naming and add a variant

 drivers/gpu/drm/drm_edid.c        | 114 ++++++++++++------------------
 drivers/gpu/drm/panel/panel-edp.c |  61 ++++++++++++++--
 include/drm/drm_edid.h            |   3 +-
 3 files changed, 104 insertions(+), 74 deletions(-)

-- 
2.44.0.rc1.240.g4c46232300-goog

[PATCH v2 1/3] drm_edid: Support getting EDID through ddc without connector

[Hsin-Yi Wang]

Some panels are interested in the EDID during early probe when connector
is still unknown.

Add a function drm_get_edid_no_connector() to get edid without connector.
No functional change for existing usage.

Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
---
v1->v2:
add a function to return the entire edid without updating connector.
---
 drivers/gpu/drm/drm_edid.c | 45 ++++++++++++++++++++++++++++----------
 include/drm/drm_edid.h     |  1 +
 2 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 1ad94473e400..15b97c8ed993 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2364,7 +2364,7 @@ static struct edid *_drm_do_get_edid(struct drm_connector *connector,
 	struct edid *edid, *new;
 	size_t alloc_size = EDID_LENGTH;
 
-	override = drm_edid_override_get(connector);
+	override = connector ? drm_edid_override_get(connector) : false;
 	if (override) {
 		alloc_size = override->size;
 		edid = kmemdup(override->edid, alloc_size, GFP_KERNEL);
@@ -2385,18 +2385,20 @@ static struct edid *_drm_do_get_edid(struct drm_connector *connector,
 	if (status == EDID_BLOCK_READ_FAIL)
 		goto fail;
 
-	/* FIXME: Clarify what a corrupt EDID actually means. */
-	if (status == EDID_BLOCK_OK || status == EDID_BLOCK_VERSION)
-		connector->edid_corrupt = false;
-	else
-		connector->edid_corrupt = true;
+	if (connector) {
+		/* FIXME: Clarify what a corrupt EDID actually means. */
+		if (status == EDID_BLOCK_OK || status == EDID_BLOCK_VERSION)
+			connector->edid_corrupt = false;
+		else
+			connector->edid_corrupt = true;
 
-	if (!edid_block_status_valid(status, edid_block_tag(edid))) {
-		if (status == EDID_BLOCK_ZERO)
-			connector->null_edid_counter++;
+		if (!edid_block_status_valid(status, edid_block_tag(edid))) {
+			if (status == EDID_BLOCK_ZERO)
+				connector->null_edid_counter++;
 
-		connector_bad_edid(connector, edid, 1);
-		goto fail;
+			connector_bad_edid(connector, edid, 1);
+			goto fail;
+		}
 	}
 
 	if (!edid_extension_block_count(edid))
@@ -2444,7 +2446,8 @@ static struct edid *_drm_do_get_edid(struct drm_connector *connector,
 	}
 
 	if (invalid_blocks) {
-		connector_bad_edid(connector, edid, num_blocks);
+		if (connector)
+			connector_bad_edid(connector, edid, num_blocks);
 
 		edid = edid_filter_invalid_blocks(edid, &alloc_size);
 	}
@@ -2637,6 +2640,24 @@ struct edid *drm_get_edid(struct drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_get_edid);
 
+/**
+ * drm_get_edid_no_connector - get EDID data without updating connector status
+ * @adapter: I2C adapter to use for DDC
+ *
+ * Similar to drm_edid_read_ddc(), but not checking any connector status. Use
+ * this function to get EDID when connector is still unknown.
+ *
+ * Return: Pointer to valid EDID or NULL if we couldn't find any.
+ */
+struct edid *drm_get_edid_no_connector(struct i2c_adapter *adapter)
+{
+	if (!drm_probe_ddc(adapter))
+		return NULL;
+
+	return _drm_do_get_edid(NULL, drm_do_probe_ddc_edid, adapter, NULL);
+}
+EXPORT_SYMBOL(drm_get_edid_no_connector);
+
 /**
  * drm_edid_read_custom - Read EDID data using given EDID block read function
  * @connector: Connector to use
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 70ae6c290bdc..80c9e32ff80e 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -565,6 +565,7 @@ 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);
+struct edid *drm_get_edid_no_connector(struct i2c_adapter *adapter);
 u32 drm_edid_get_panel_id(struct i2c_adapter *adapter);
 struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
 				     struct i2c_adapter *adapter);
-- 
2.44.0.rc1.240.g4c46232300-goog

[PATCH v2 2/3] drm/panel: panel-edp: Match edp_panels with panel name

[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. When matching generic edp
panels, we should first match with both panel id and panel name by checking
if edid contains the name string. If not found, match with panel id only.

Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
---
v1->v2:
match with panel name instead of crc hash.
Note that we can't directly use drm_edid_get_monitor_name(), because some
panel store the name after EDID_DETAIL_MONITOR_STRING instead of
EDID_DETAIL_MONITOR_NAME.
---
 drivers/gpu/drm/drm_edid.c        | 69 +++++++------------------------
 drivers/gpu/drm/panel/panel-edp.c | 44 +++++++++++++++++---
 include/drm/drm_edid.h            |  2 +-
 3 files changed, 54 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 15b97c8ed993..c4126475ff0a 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2764,7 +2764,19 @@ const struct drm_edid *drm_edid_read(struct drm_connector *connector)
 }
 EXPORT_SYMBOL(drm_edid_read);
 
-static u32 edid_extract_panel_id(const struct edid *edid)
+/**
+ * edid_extract_panel_id - Extract a panel's ID from EDID
+ * @edid: EDID used to extract the panel's ID.
+ *
+ * Extract panel ID from EDID.
+ *
+ * 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 edid_extract_panel_id(const struct edid *edid)
 {
 	/*
 	 * We represent the ID as a 32-bit number so it can easily be compared
@@ -2783,60 +2795,7 @@ static u32 edid_extract_panel_id(const struct edid *edid)
 	       (u32)edid->mfg_id[1] << 16   |
 	       (u32)EDID_PRODUCT_ID(edid);
 }
-
-/**
- * drm_edid_get_panel_id - Get a panel's ID through DDC
- * @adapter: I2C adapter to use for DDC
- *
- * This function reads 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.
- *
- * 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).
- *
- * 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.
- */
-
-u32 drm_edid_get_panel_id(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;
-
-	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
-		edid_block_dump(KERN_NOTICE, base_block, 0);
-
-	kfree(base_block);
-
-	return panel_id;
-}
-EXPORT_SYMBOL(drm_edid_get_panel_id);
+EXPORT_SYMBOL(edid_extract_panel_id);
 
 /**
  * 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 3fb5fcd326a4..72ad552bff24 100644
--- a/drivers/gpu/drm/panel/panel-edp.c
+++ b/drivers/gpu/drm/panel/panel-edp.c
@@ -761,11 +761,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 bool edid_has_name(struct edid *edid, const char *panel_name);
+static const struct edp_panel_entry *find_edp_panel(u32 panel_id, struct edid *edid);
 
 static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
 {
 	struct panel_desc *desc;
+	struct edid *edid;
 	u32 panel_id;
 	char vend[4];
 	u16 product_id;
@@ -795,15 +797,19 @@ 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) {
+	edid = drm_get_edid_no_connector(panel->ddc);
+	if (!edid) {
 		dev_err(dev, "Couldn't identify panel via EDID\n");
 		ret = -EIO;
 		goto exit;
 	}
+	panel_id = edid_extract_panel_id(edid);
 	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, edid);
+
+	/* Read EDID again in panel_edp_get_modes() when connector is known. */
+	kfree(edid);
 
 	/*
 	 * We're using non-optimized timings and want it really obvious that
@@ -2107,13 +2113,41 @@ static const struct edp_panel_entry edp_panels[] = {
 	{ /* sentinal */ }
 };
 
-static const struct edp_panel_entry *find_edp_panel(u32 panel_id)
+static bool edid_has_name(struct edid *edid, const char *panel_name)
+{
+	int i, j;
+	char buf[13];
+
+	for (i = 0; i < 4; i++) {
+		strncpy(buf, edid->detailed_timings[i].data.other_data.data.str.str,
+			sizeof(buf));
+		for (j = 0; j < 13; j++) {
+			if (buf[j] == 0x0a) {
+				buf[j] = '\0';
+				break;
+			}
+		}
+		buf[12] = '\0';
+		if (strncmp(panel_name, buf, strlen(panel_name)) == 0)
+			return true;
+	}
+
+	return false;
+}
+
+static const struct edp_panel_entry *find_edp_panel(u32 panel_id, struct edid *edid)
 {
 	const struct edp_panel_entry *panel;
 
 	if (!panel_id)
 		return NULL;
 
+	/* Match with both panel_id and name. Find if EDID contains name. */
+	for (panel = edp_panels; panel->panel_id; panel++)
+		if (panel->panel_id == panel_id && edid_has_name(edid, panel->name))
+			return panel;
+
+	/* Match with only panel_id */
 	for (panel = edp_panels; panel->panel_id; panel++)
 		if (panel->panel_id == panel_id)
 			return panel;
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 80c9e32ff80e..4cefff357789 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -566,7 +566,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 struct edid *drm_get_edid(struct drm_connector *connector,
 			  struct i2c_adapter *adapter);
 struct edid *drm_get_edid_no_connector(struct i2c_adapter *adapter);
-u32 drm_edid_get_panel_id(struct i2c_adapter *adapter);
+u32 edid_extract_panel_id(const struct edid *edid);
 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.rc1.240.g4c46232300-goog

[PATCH v2 3/3] drm/panel: panel-edp: Fix AUO 0x405c panel naming and add a variant

[Hsin-Yi Wang]

There are 2 different AUO panels using the same panel id. 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.

Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
---
v2: new
---
 drivers/gpu/drm/panel/panel-edp.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
index 72ad552bff24..e39af92342e8 100644
--- a/drivers/gpu/drm/panel/panel-edp.c
+++ b/drivers/gpu/drm/panel/panel-edp.c
@@ -1013,6 +1013,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,
@@ -1990,7 +2003,9 @@ static const struct edp_panel_entry edp_panels[] = {
 	EDP_PANEL_ENTRY('A', 'U', 'O', 0x239b, &delay_200_500_e50, "B116XAN06.1"),
 	EDP_PANEL_ENTRY('A', 'U', 'O', 0x255c, &delay_200_500_e50, "B116XTN02.5"),
 	EDP_PANEL_ENTRY('A', 'U', 'O', 0x403d, &delay_200_500_e50, "B140HAN04.0"),
-	EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0"),
+	EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAN04.0 "),
+	EDP_PANEL_ENTRY2('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0 ",
+			 &auo_b116xa3_mode),
 	EDP_PANEL_ENTRY('A', 'U', 'O', 0x435c, &delay_200_500_e50, "Unknown"),
 	EDP_PANEL_ENTRY('A', 'U', 'O', 0x582d, &delay_200_500_e50, "B133UAN01.0"),
 	EDP_PANEL_ENTRY('A', 'U', 'O', 0x615c, &delay_200_500_e50, "B116XAN06.1"),
-- 
2.44.0.rc1.240.g4c46232300-goog

Re: [PATCH v2 1/3] drm_edid: Support getting EDID through ddc without connector

[Hsin-Yi Wang]

On Tue, Feb 27, 2024 at 5:11 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> Some panels are interested in the EDID during early probe when connector
> is still unknown.
>
> Add a function drm_get_edid_no_connector() to get edid without connector.
> No functional change for existing usage.
>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
> v1->v2:
> add a function to return the entire edid without updating connector.
> ---
>  drivers/gpu/drm/drm_edid.c | 45 ++++++++++++++++++++++++++++----------
>  include/drm/drm_edid.h     |  1 +
>  2 files changed, 34 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 1ad94473e400..15b97c8ed993 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2364,7 +2364,7 @@ static struct edid *_drm_do_get_edid(struct drm_connector *connector,
>         struct edid *edid, *new;
>         size_t alloc_size = EDID_LENGTH;
>
> -       override = drm_edid_override_get(connector);
> +       override = connector ? drm_edid_override_get(connector) : false;

typo: should be NULL here. I'll update in the next version with other comments.

>         if (override) {
>                 alloc_size = override->size;
>                 edid = kmemdup(override->edid, alloc_size, GFP_KERNEL);
> @@ -2385,18 +2385,20 @@ static struct edid *_drm_do_get_edid(struct drm_connector *connector,
>         if (status == EDID_BLOCK_READ_FAIL)
>                 goto fail;
>
> -       /* FIXME: Clarify what a corrupt EDID actually means. */
> -       if (status == EDID_BLOCK_OK || status == EDID_BLOCK_VERSION)
> -               connector->edid_corrupt = false;
> -       else
> -               connector->edid_corrupt = true;
> +       if (connector) {
> +               /* FIXME: Clarify what a corrupt EDID actually means. */
> +               if (status == EDID_BLOCK_OK || status == EDID_BLOCK_VERSION)
> +                       connector->edid_corrupt = false;
> +               else
> +                       connector->edid_corrupt = true;
>
> -       if (!edid_block_status_valid(status, edid_block_tag(edid))) {
> -               if (status == EDID_BLOCK_ZERO)
> -                       connector->null_edid_counter++;
> +               if (!edid_block_status_valid(status, edid_block_tag(edid))) {
> +                       if (status == EDID_BLOCK_ZERO)
> +                               connector->null_edid_counter++;
>
> -               connector_bad_edid(connector, edid, 1);
> -               goto fail;
> +                       connector_bad_edid(connector, edid, 1);
> +                       goto fail;
> +               }
>         }
>
>         if (!edid_extension_block_count(edid))
> @@ -2444,7 +2446,8 @@ static struct edid *_drm_do_get_edid(struct drm_connector *connector,
>         }
>
>         if (invalid_blocks) {
> -               connector_bad_edid(connector, edid, num_blocks);
> +               if (connector)
> +                       connector_bad_edid(connector, edid, num_blocks);
>
>                 edid = edid_filter_invalid_blocks(edid, &alloc_size);
>         }
> @@ -2637,6 +2640,24 @@ struct edid *drm_get_edid(struct drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_get_edid);
>
> +/**
> + * drm_get_edid_no_connector - get EDID data without updating connector status
> + * @adapter: I2C adapter to use for DDC
> + *
> + * Similar to drm_edid_read_ddc(), but not checking any connector status. Use
> + * this function to get EDID when connector is still unknown.
> + *
> + * Return: Pointer to valid EDID or NULL if we couldn't find any.
> + */
> +struct edid *drm_get_edid_no_connector(struct i2c_adapter *adapter)
> +{
> +       if (!drm_probe_ddc(adapter))
> +               return NULL;
> +
> +       return _drm_do_get_edid(NULL, drm_do_probe_ddc_edid, adapter, NULL);
> +}
> +EXPORT_SYMBOL(drm_get_edid_no_connector);
> +
>  /**
>   * drm_edid_read_custom - Read EDID data using given EDID block read function
>   * @connector: Connector to use
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 70ae6c290bdc..80c9e32ff80e 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -565,6 +565,7 @@ 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);
> +struct edid *drm_get_edid_no_connector(struct i2c_adapter *adapter);
>  u32 drm_edid_get_panel_id(struct i2c_adapter *adapter);
>  struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
>                                      struct i2c_adapter *adapter);
> --
> 2.44.0.rc1.240.g4c46232300-goog
>

Re: [PATCH v2 2/3] drm/panel: panel-edp: Match edp_panels with panel name

[Dmitry Baryshkov]

On Wed, 28 Feb 2024 at 03:11, 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. When matching generic edp
> panels, we should first match with both panel id and panel name by checking
> if edid contains the name string. If not found, match with panel id only.
>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
> v1->v2:
> match with panel name instead of crc hash.
> Note that we can't directly use drm_edid_get_monitor_name(), because some
> panel store the name after EDID_DETAIL_MONITOR_STRING instead of
> EDID_DETAIL_MONITOR_NAME.
> ---
>  drivers/gpu/drm/drm_edid.c        | 69 +++++++------------------------
>  drivers/gpu/drm/panel/panel-edp.c | 44 +++++++++++++++++---
>  include/drm/drm_edid.h            |  2 +-
>  3 files changed, 54 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 15b97c8ed993..c4126475ff0a 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2764,7 +2764,19 @@ const struct drm_edid *drm_edid_read(struct drm_connector *connector)
>  }
>  EXPORT_SYMBOL(drm_edid_read);
>
> -static u32 edid_extract_panel_id(const struct edid *edid)
> +/**
> + * edid_extract_panel_id - Extract a panel's ID from EDID
> + * @edid: EDID used to extract the panel's ID.
> + *
> + * Extract panel ID from EDID.
> + *
> + * 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 edid_extract_panel_id(const struct edid *edid)

drm_edid_extract_panel_id(), please

>  {
>         /*
>          * We represent the ID as a 32-bit number so it can easily be compared

[skipeed the rest, LGTM]

-- 
With best wishes
Dmitry

Re: [PATCH v2 1/3] drm_edid: Support getting EDID through ddc without connector

[Doug Anderson]

Hi,

On Tue, Feb 27, 2024 at 5:11 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> Some panels are interested in the EDID during early probe when connector
> is still unknown.
>
> Add a function drm_get_edid_no_connector() to get edid without connector.
> No functional change for existing usage.
>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
> v1->v2:
> add a function to return the entire edid without updating connector.
> ---
>  drivers/gpu/drm/drm_edid.c | 45 ++++++++++++++++++++++++++++----------
>  include/drm/drm_edid.h     |  1 +
>  2 files changed, 34 insertions(+), 12 deletions(-)

I'll respond in the discussion in v1 too, but overall I'm not a fan of
reading the whole EDID twice at bootup. Personally I'd love to see us
to back to just reading the base block like in v1, but I guess we can
see what Jani and others say.


> @@ -2385,18 +2385,20 @@ static struct edid *_drm_do_get_edid(struct drm_connector *connector,
>         if (status == EDID_BLOCK_READ_FAIL)
>                 goto fail;
>
> -       /* FIXME: Clarify what a corrupt EDID actually means. */
> -       if (status == EDID_BLOCK_OK || status == EDID_BLOCK_VERSION)
> -               connector->edid_corrupt = false;
> -       else
> -               connector->edid_corrupt = true;
> +       if (connector) {
> +               /* FIXME: Clarify what a corrupt EDID actually means. */
> +               if (status == EDID_BLOCK_OK || status == EDID_BLOCK_VERSION)
> +                       connector->edid_corrupt = false;
> +               else
> +                       connector->edid_corrupt = true;
>
> -       if (!edid_block_status_valid(status, edid_block_tag(edid))) {
> -               if (status == EDID_BLOCK_ZERO)
> -                       connector->null_edid_counter++;
> +               if (!edid_block_status_valid(status, edid_block_tag(edid))) {
> +                       if (status == EDID_BLOCK_ZERO)
> +                               connector->null_edid_counter++;
>
> -               connector_bad_edid(connector, edid, 1);
> -               goto fail;
> +                       connector_bad_edid(connector, edid, 1);
> +                       goto fail;

This "goto fail" is now only run "if (connector)" which means that
you're not properly checking if the EDID is valid when "connector ==
NULL", right? That seems like a bug unless I missed something...

Re: [PATCH v2 2/3] drm/panel: panel-edp: Match edp_panels with panel name

[Doug Anderson]

Hi,

On Tue, Feb 27, 2024 at 5:11 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> @@ -2107,13 +2113,41 @@ static const struct edp_panel_entry edp_panels[] = {
>         { /* sentinal */ }
>  };
>
> -static const struct edp_panel_entry *find_edp_panel(u32 panel_id)
> +static bool edid_has_name(struct edid *edid, const char *panel_name)
> +{
> +       int i, j;
> +       char buf[13];
> +

Should have some comment about why this can't use
drm_edid_get_monitor_name(). AKA because panels seem to be storing the
monitor name tagged as raw strings instead of as the name. Should
probably also have some of the other checks from
is_display_descriptor() like checking for clock of 0 and pad1 of 0.


> +       for (i = 0; i < 4; i++) {

Instead of 4, I think this can be ARRAY_SIZE(edid->detailed_timings), right?


> +               strncpy(buf, edid->detailed_timings[i].data.other_data.data.str.str,
> +                       sizeof(buf));

I can never quite remember which of the strXcpy() routines are frowned
upon and which are the golden children at the moment. ...but I don't
think we really need the copy nor the local buffer anyway, right?
You're already going through this thing 1 byte at a time so just
compare it straight away.


> +               for (j = 0; j < 13; j++) {
> +                       if (buf[j] == 0x0a) {

Instead of hardcoding 0x0a, I think you want '\n', no?


> +                               buf[j] = '\0';
> +                               break;
> +                       }
> +               }
> +               buf[12] = '\0';
> +               if (strncmp(panel_name, buf, strlen(panel_name)) == 0)
> +                       return true;

Untested, but I think with my suggestions above the function becomes this:

static bool edid_has_name(struct edid *edid, const char *panel_name)
{
    int i, j;

    /*
     * We can't use drm_edid_get_monitor_name() since many eDP panels store
     * their name as a raw string. We'll accept either a string or name
     * match as long as the panel ID also matches.
     */
    for (i = 0; i < ARRAY_SIZE(edid->detailed_timings); i++) {
        struct detailed_timing *timing = &edid->detailed_timings[i];

        if (timing->pixel_clock != 0 ||
            timing->data.other_data.pad1 != 0 ||
            (timing->data.other_data.type != EDID_DETAIL_MONITOR_NAME &&
             timing->data.other_data.type != EDID_DETAIL_MONITOR_STRING))
                continue;

        for (j = 0; j < ARRAY_SIZE(timing->data.other_data.data.str.str); j++) {
            const char *str = timing->data.other_data.data.str.str;

            if (panel_name[j] == '\0') {
                if (str[j] == '\0' || str[j] == '\n')
                    return true;
                break;
            }
        }
        if (j == ARRAY_SIZE(timing->data.other_data.data.str.str) &&
            panel_name[j] == '\0')
            return true;
    }

    return false;
}

Re: [PATCH v2 3/3] drm/panel: panel-edp: Fix AUO 0x405c panel naming and add a variant

[Doug Anderson]

Hi,

On Tue, Feb 27, 2024 at 5:11 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> There are 2 different AUO panels using the same panel id. 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.
>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
> v2: new
> ---
>  drivers/gpu/drm/panel/panel-edp.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)

The previous version of this patch that we reverted also had an
override for AUO 0x615c. Is that one no longer needed?


> @@ -1990,7 +2003,9 @@ static const struct edp_panel_entry edp_panels[] = {
>         EDP_PANEL_ENTRY('A', 'U', 'O', 0x239b, &delay_200_500_e50, "B116XAN06.1"),
>         EDP_PANEL_ENTRY('A', 'U', 'O', 0x255c, &delay_200_500_e50, "B116XTN02.5"),
>         EDP_PANEL_ENTRY('A', 'U', 'O', 0x403d, &delay_200_500_e50, "B140HAN04.0"),
> -       EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0"),
> +       EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAN04.0 "),
> +       EDP_PANEL_ENTRY2('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0 ",
> +                        &auo_b116xa3_mode),

The name string now has a space at the end of it. I _guess_ that's OK.
Hmmm, but I guess you should update the kernel doc for "struct
edp_panel_entry". The name field is described as "Name of this panel
(for printing to logs)". Now it should include that it's also used for
matching EDIDs in some cases too.

Re: [PATCH v2 1/3] drm_edid: Support getting EDID through ddc without connector

[Hsin-Yi Wang]

On Wed, Feb 28, 2024 at 4:21 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, Feb 27, 2024 at 5:11 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> >
> > Some panels are interested in the EDID during early probe when connector
> > is still unknown.
> >
> > Add a function drm_get_edid_no_connector() to get edid without connector.
> > No functional change for existing usage.
> >
> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > ---
> > v1->v2:
> > add a function to return the entire edid without updating connector.
> > ---
> >  drivers/gpu/drm/drm_edid.c | 45 ++++++++++++++++++++++++++++----------
> >  include/drm/drm_edid.h     |  1 +
> >  2 files changed, 34 insertions(+), 12 deletions(-)
>
> I'll respond in the discussion in v1 too, but overall I'm not a fan of
> reading the whole EDID twice at bootup. Personally I'd love to see us
> to back to just reading the base block like in v1, but I guess we can
> see what Jani and others say.
>
>
> > @@ -2385,18 +2385,20 @@ static struct edid *_drm_do_get_edid(struct drm_connector *connector,
> >         if (status == EDID_BLOCK_READ_FAIL)
> >                 goto fail;
> >
> > -       /* FIXME: Clarify what a corrupt EDID actually means. */
> > -       if (status == EDID_BLOCK_OK || status == EDID_BLOCK_VERSION)
> > -               connector->edid_corrupt = false;
> > -       else
> > -               connector->edid_corrupt = true;
> > +       if (connector) {
> > +               /* FIXME: Clarify what a corrupt EDID actually means. */
> > +               if (status == EDID_BLOCK_OK || status == EDID_BLOCK_VERSION)
> > +                       connector->edid_corrupt = false;
> > +               else
> > +                       connector->edid_corrupt = true;
> >
> > -       if (!edid_block_status_valid(status, edid_block_tag(edid))) {
> > -               if (status == EDID_BLOCK_ZERO)
> > -                       connector->null_edid_counter++;
> > +               if (!edid_block_status_valid(status, edid_block_tag(edid))) {
> > +                       if (status == EDID_BLOCK_ZERO)
> > +                               connector->null_edid_counter++;
> >
> > -               connector_bad_edid(connector, edid, 1);
> > -               goto fail;
> > +                       connector_bad_edid(connector, edid, 1);
> > +                       goto fail;
>
> This "goto fail" is now only run "if (connector)" which means that
> you're not properly checking if the EDID is valid when "connector ==
> NULL", right? That seems like a bug unless I missed something...

We can't check with connector_bad_edid() since there's no connector.
But we still check with edid_block_read() status, similar to what the
original drm_edid_get_panel_id() checks.

Re: [PATCH v2 3/3] drm/panel: panel-edp: Fix AUO 0x405c panel naming and add a variant

[Hsin-Yi Wang]

On Wed, Feb 28, 2024 at 4:22 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, Feb 27, 2024 at 5:11 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> >
> > There are 2 different AUO panels using the same panel id. 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.
> >
> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > ---
> > v2: new
> > ---
> >  drivers/gpu/drm/panel/panel-edp.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
>
> The previous version of this patch that we reverted also had an
> override for AUO 0x615c. Is that one no longer needed?
>
>
> > @@ -1990,7 +2003,9 @@ static const struct edp_panel_entry edp_panels[] = {
> >         EDP_PANEL_ENTRY('A', 'U', 'O', 0x239b, &delay_200_500_e50, "B116XAN06.1"),
> >         EDP_PANEL_ENTRY('A', 'U', 'O', 0x255c, &delay_200_500_e50, "B116XTN02.5"),
> >         EDP_PANEL_ENTRY('A', 'U', 'O', 0x403d, &delay_200_500_e50, "B140HAN04.0"),
> > -       EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0"),
> > +       EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAN04.0 "),
> > +       EDP_PANEL_ENTRY2('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0 ",
> > +                        &auo_b116xa3_mode),
>
> The name string now has a space at the end of it. I _guess_ that's OK.
> Hmmm, but I guess you should update the kernel doc for "struct
> edp_panel_entry". The name field is described as "Name of this panel
> (for printing to logs)". Now it should include that it's also used for
> matching EDIDs in some cases too.

The space here is because in the EDID, there is space at the end,
before 0x0a (\n).
Okay I will update the kernel doc to mention that the same should be
exactly the same as the panel name.

Re: [PATCH v2 3/3] drm/panel: panel-edp: Fix AUO 0x405c panel naming and add a variant

[Dmitry Baryshkov]

On Thu, 29 Feb 2024 at 03:05, Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> On Wed, Feb 28, 2024 at 4:22 PM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Tue, Feb 27, 2024 at 5:11 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> > >
> > > There are 2 different AUO panels using the same panel id. 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.
> > >
> > > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > > ---
> > > v2: new
> > > ---
> > >  drivers/gpu/drm/panel/panel-edp.c | 17 ++++++++++++++++-
> > >  1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > The previous version of this patch that we reverted also had an
> > override for AUO 0x615c. Is that one no longer needed?
> >
> >
> > > @@ -1990,7 +2003,9 @@ static const struct edp_panel_entry edp_panels[] = {
> > >         EDP_PANEL_ENTRY('A', 'U', 'O', 0x239b, &delay_200_500_e50, "B116XAN06.1"),
> > >         EDP_PANEL_ENTRY('A', 'U', 'O', 0x255c, &delay_200_500_e50, "B116XTN02.5"),
> > >         EDP_PANEL_ENTRY('A', 'U', 'O', 0x403d, &delay_200_500_e50, "B140HAN04.0"),
> > > -       EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0"),
> > > +       EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAN04.0 "),
> > > +       EDP_PANEL_ENTRY2('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0 ",
> > > +                        &auo_b116xa3_mode),
> >
> > The name string now has a space at the end of it. I _guess_ that's OK.
> > Hmmm, but I guess you should update the kernel doc for "struct
> > edp_panel_entry". The name field is described as "Name of this panel
> > (for printing to logs)". Now it should include that it's also used for
> > matching EDIDs in some cases too.
>
> The space here is because in the EDID, there is space at the end,
> before 0x0a (\n).
> Okay I will update the kernel doc to mention that the same should be
> exactly the same as the panel name.

Maybe it would be better to strip all the whitespace on the right?

-- 
With best wishes
Dmitry

Re: [PATCH v2 3/3] drm/panel: panel-edp: Fix AUO 0x405c panel naming and add a variant

[Hsin-Yi Wang]

On Wed, Feb 28, 2024 at 5:13 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Thu, 29 Feb 2024 at 03:05, Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> >
> > On Wed, Feb 28, 2024 at 4:22 PM Doug Anderson <dianders@chromium.org> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, Feb 27, 2024 at 5:11 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> > > >
> > > > There are 2 different AUO panels using the same panel id. 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.
> > > >
> > > > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > > > ---
> > > > v2: new
> > > > ---
> > > >  drivers/gpu/drm/panel/panel-edp.c | 17 ++++++++++++++++-
> > > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > >
> > > The previous version of this patch that we reverted also had an
> > > override for AUO 0x615c. Is that one no longer needed?
> > >
> > >
> > > > @@ -1990,7 +2003,9 @@ static const struct edp_panel_entry edp_panels[] = {
> > > >         EDP_PANEL_ENTRY('A', 'U', 'O', 0x239b, &delay_200_500_e50, "B116XAN06.1"),
> > > >         EDP_PANEL_ENTRY('A', 'U', 'O', 0x255c, &delay_200_500_e50, "B116XTN02.5"),
> > > >         EDP_PANEL_ENTRY('A', 'U', 'O', 0x403d, &delay_200_500_e50, "B140HAN04.0"),
> > > > -       EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0"),
> > > > +       EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAN04.0 "),
> > > > +       EDP_PANEL_ENTRY2('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0 ",
> > > > +                        &auo_b116xa3_mode),
> > >
> > > The name string now has a space at the end of it. I _guess_ that's OK.
> > > Hmmm, but I guess you should update the kernel doc for "struct
> > > edp_panel_entry". The name field is described as "Name of this panel
> > > (for printing to logs)". Now it should include that it's also used for
> > > matching EDIDs in some cases too.
> >
> > The space here is because in the EDID, there is space at the end,
> > before 0x0a (\n).
> > Okay I will update the kernel doc to mention that the same should be
> > exactly the same as the panel name.
>
> Maybe it would be better to strip all the whitespace on the right?
>

Sounds good too.

> --
> With best wishes
> Dmitry

Re: [PATCH v2 3/3] drm/panel: panel-edp: Fix AUO 0x405c panel naming and add a variant

[Hsin-Yi Wang]

On Wed, Feb 28, 2024 at 4:22 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, Feb 27, 2024 at 5:11 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> >
> > There are 2 different AUO panels using the same panel id. 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.
> >
> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > ---
> > v2: new
> > ---
> >  drivers/gpu/drm/panel/panel-edp.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
>
> The previous version of this patch that we reverted also had an
> override for AUO 0x615c. Is that one no longer needed?
>

I currently don't have the exact panel to verify at hand. If the
dependent code is agreed and accepted, I will send out a patch for
this panel later.


>
> > @@ -1990,7 +2003,9 @@ static const struct edp_panel_entry edp_panels[] = {
> >         EDP_PANEL_ENTRY('A', 'U', 'O', 0x239b, &delay_200_500_e50, "B116XAN06.1"),
> >         EDP_PANEL_ENTRY('A', 'U', 'O', 0x255c, &delay_200_500_e50, "B116XTN02.5"),
> >         EDP_PANEL_ENTRY('A', 'U', 'O', 0x403d, &delay_200_500_e50, "B140HAN04.0"),
> > -       EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0"),
> > +       EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAN04.0 "),
> > +       EDP_PANEL_ENTRY2('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0 ",
> > +                        &auo_b116xa3_mode),
>
> The name string now has a space at the end of it. I _guess_ that's OK.
> Hmmm, but I guess you should update the kernel doc for "struct
> edp_panel_entry". The name field is described as "Name of this panel
> (for printing to logs)". Now it should include that it's also used for
> matching EDIDs in some cases too.