[PATCH v3 0/4] Match panel with id and name

[PATCH v3 0/4] 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
v2: https://lore.kernel.org/lkml/20240228011133.1238439-1-hsinyi@chromium.org

Hsin-Yi Wang (4):
  drm_edid: Add a function to get EDID base block
  drm/edid: Add a function to check monitor string
  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        | 107 +++++++++++++++++++++++-------
 drivers/gpu/drm/panel/panel-edp.c |  42 +++++++++---
 include/drm/drm_edid.h            |   8 ++-
 3 files changed, 123 insertions(+), 34 deletions(-)

-- 
2.44.0.rc1.240.g4c46232300-goog

[PATCH v3 1/4] 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
more information from the EDID base block to distinguish these panel
variants.

Add drm_edid_get_base_block() to return the EDID base block, which is
introduced as a new type edid_base_block with the same layout as edid.

Caller can further use it to get panel id or check if the block contains
certain strings, such as panel name.

Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
---
v2->v3: change back to return only the first block.
---
 drivers/gpu/drm/drm_edid.c        | 58 ++++++++++++++++++-------------
 drivers/gpu/drm/panel/panel-edp.c |  8 +++--
 include/drm/drm_edid.h            |  7 +++-
 3 files changed, 45 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 923c4423151c..13454bc64ca2 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2770,58 +2770,66 @@ 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_block: 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(struct edid_base_block *base_block)
+{
+	return edid_extract_panel_id(&base_block->edid);
+}
+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.
+ *
+ * Return: Pointer to allocated EDID base block, or NULL if failed.
  */
-
-u32 drm_edid_get_panel_id(struct i2c_adapter *adapter)
+struct edid_base_block *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);
+		kfree(base_block);
+		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 745f3e48f02a..fc2d648fd3ab 100644
--- a/drivers/gpu/drm/panel/panel-edp.c
+++ b/drivers/gpu/drm/panel/panel-edp.c
@@ -766,6 +766,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;
+	struct edid_base_block *base_block;
 	u32 panel_id;
 	char vend[4];
 	u16 product_id;
@@ -795,8 +796,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..2455d6ab2221 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -312,6 +312,10 @@ struct edid {
 	u8 checksum;
 } __packed;
 
+struct edid_base_block {
+	struct edid edid;
+} __packed;
+
 #define EDID_PRODUCT_ID(e) ((e)->prod_code[0] | ((e)->prod_code[1] << 8))
 
 /* Short Audio Descriptor */
@@ -410,7 +414,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);
+struct edid_base_block *drm_edid_get_base_block(struct i2c_adapter *adapter);
+u32 drm_edid_get_panel_id(struct edid_base_block *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.rc1.240.g4c46232300-goog

[PATCH v3 2/4] drm/edid: Add a function to check monitor string

[Hsin-Yi Wang]

Add a function to check if the EDID base block contains a given string.

One of the use cases is fetching panel from a list of panel names, since
some panel vendors put the monitor name after EDID_DETAIL_MONITOR_STRING
instead of EDID_DETAIL_MONITOR_NAME.

Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
---
v2->v3: move string matching to drm_edid
---
 drivers/gpu/drm/drm_edid.c | 49 ++++++++++++++++++++++++++++++++++++++
 include/drm/drm_edid.h     |  1 +
 2 files changed, 50 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 13454bc64ca2..fcdc2bd143dd 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2789,6 +2789,55 @@ u32 drm_edid_get_panel_id(struct edid_base_block *base_block)
 }
 EXPORT_SYMBOL(drm_edid_get_panel_id);
 
+/**
+ * drm_edid_has_monitor_string - Check if a EDID base block has certain string.
+ * @base_block: EDID base block to check.
+ * @str: pointer to a character array to hold the string to be checked.
+ *
+ * Check if the detailed timings section of a EDID base block has the given
+ * string.
+ *
+ * Return: True if the EDID base block contains the string, false otherwise.
+ */
+bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str)
+{
+	unsigned int i, j, k, buflen = strlen(str);
+
+	for (i = 0; i < EDID_DETAILED_TIMINGS; i++) {
+		struct detailed_timing *timing = &base_block->edid.detailed_timings[i];
+		unsigned int size = ARRAY_SIZE(timing->data.other_data.data.str.str);
+
+		if (buflen > size || 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 < buflen; j++) {
+			char c = timing->data.other_data.data.str.str[j];
+
+			if (c != str[j] ||  c == '\n')
+				break;
+		}
+
+		if (j == buflen) {
+			/* Allow trailing white spaces. */
+			for (k = j; k < size; k++) {
+				char c = timing->data.other_data.data.str.str[k];
+
+				if (c == '\n')
+					return true;
+				else if (c != ' ')
+					break;
+			}
+			if (k == size)
+				return true;
+		}
+	}
+
+	return false;
+}
+
 /**
  * drm_edid_get_base_block - Get a panel's EDID base block
  * @adapter: I2C adapter to use for DDC
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 2455d6ab2221..248ddb0a6b5d 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -416,6 +416,7 @@ struct edid *drm_get_edid(struct drm_connector *connector,
 			  struct i2c_adapter *adapter);
 struct edid_base_block *drm_edid_get_base_block(struct i2c_adapter *adapter);
 u32 drm_edid_get_panel_id(struct edid_base_block *base_block);
+bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str);
 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 v3 3/4] 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>
---
v2->v3: move string matching to drm_edid
---
 drivers/gpu/drm/panel/panel-edp.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
index fc2d648fd3ab..e3044e34c5f8 100644
--- a/drivers/gpu/drm/panel/panel-edp.c
+++ b/drivers/gpu/drm/panel/panel-edp.c
@@ -761,7 +761,8 @@ 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,
+						    struct edid_base_block *base_block);
 
 static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
 {
@@ -799,7 +800,6 @@ 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);
-		kfree(base_block);
 	} else {
 		dev_err(dev, "Couldn't identify panel via EDID\n");
 		ret = -EIO;
@@ -807,7 +807,9 @@ static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
 	}
 	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, base_block);
+
+	kfree(base_block);
 
 	/*
 	 * We're using non-optimized timings and want it really obvious that
@@ -2087,13 +2089,18 @@ static const struct edp_panel_entry edp_panels[] = {
 	{ /* sentinal */ }
 };
 
-static const struct edp_panel_entry *find_edp_panel(u32 panel_id)
+static const struct edp_panel_entry *find_edp_panel(u32 panel_id,
+						    struct edid_base_block *base_block)
 {
 	const struct edp_panel_entry *panel;
 
-	if (!panel_id)
-		return NULL;
+	 /* Match with both panel_id and name */
+	for (panel = edp_panels; panel->panel_id; panel++)
+		if (panel->panel_id == panel_id &&
+		    drm_edid_has_monitor_string(base_block, 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;
-- 
2.44.0.rc1.240.g4c46232300-goog

[PATCH v3 4/4] 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->v3: no change.
---
 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 e3044e34c5f8..d2e181efff98 100644
--- a/drivers/gpu/drm/panel/panel-edp.c
+++ b/drivers/gpu/drm/panel/panel-edp.c
@@ -1011,6 +1011,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,
@@ -1966,7 +1979,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 v3 2/4] drm/edid: Add a function to check monitor string

[Jani Nikula]

On Mon, 04 Mar 2024, Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> Add a function to check if the EDID base block contains a given string.
>
> One of the use cases is fetching panel from a list of panel names, since
> some panel vendors put the monitor name after EDID_DETAIL_MONITOR_STRING
> instead of EDID_DETAIL_MONITOR_NAME.
>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
> v2->v3: move string matching to drm_edid
> ---
>  drivers/gpu/drm/drm_edid.c | 49 ++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_edid.h     |  1 +
>  2 files changed, 50 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 13454bc64ca2..fcdc2bd143dd 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2789,6 +2789,55 @@ u32 drm_edid_get_panel_id(struct edid_base_block *base_block)
>  }
>  EXPORT_SYMBOL(drm_edid_get_panel_id);
>  
> +/**
> + * drm_edid_has_monitor_string - Check if a EDID base block has certain string.
> + * @base_block: EDID base block to check.
> + * @str: pointer to a character array to hold the string to be checked.
> + *
> + * Check if the detailed timings section of a EDID base block has the given
> + * string.
> + *
> + * Return: True if the EDID base block contains the string, false otherwise.
> + */
> +bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str)
> +{
> +	unsigned int i, j, k, buflen = strlen(str);
> +
> +	for (i = 0; i < EDID_DETAILED_TIMINGS; i++) {
> +		struct detailed_timing *timing = &base_block->edid.detailed_timings[i];
> +		unsigned int size = ARRAY_SIZE(timing->data.other_data.data.str.str);
> +
> +		if (buflen > size || 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 < buflen; j++) {
> +			char c = timing->data.other_data.data.str.str[j];
> +
> +			if (c != str[j] ||  c == '\n')
> +				break;
> +		}
> +
> +		if (j == buflen) {
> +			/* Allow trailing white spaces. */
> +			for (k = j; k < size; k++) {
> +				char c = timing->data.other_data.data.str.str[k];
> +
> +				if (c == '\n')
> +					return true;
> +				else if (c != ' ')
> +					break;
> +			}
> +			if (k == size)
> +				return true;
> +		}
> +	}
> +
> +	return false;
> +}
> +

So we've put a lot of effort into converting from struct edid to struct
drm_edid, passing that around in drm_edid.c, with the allocation size it
provides, and generally cleaning stuff up.

I'm not at all happy to see *another* struct added just for the base
block, and detailed timing iteration as well as monitor name parsing
duplicated.

With struct drm_edid you can actually return an EDID that only has the
base block and size 128, even if the EDID indicates more
extensions. Because the whole thing is *designed* to handle that
gracefully. The allocated size matters, not what the blob originating
outside of the kernel tells you.

What I'm thinking is:

- Add some struct drm_edid_ident or similar. Add all the information
  that's needed to identify a panel there. I guess initially that's
  panel_id and name.

    struct drm_edid_ident {
        u32 panel_id;
        const char *name;
    };

- Add function:

    bool drm_edid_match(const struct drm_edid *drm_edid, const struct drm_edid_ident *ident);

  Check if stuff in ident matches drm_edid. You can use and extend the
  existing drm_edid based iteration etc. in
  drm_edid.c. Straightforward. The fields in ident can trivially be
  extended later, and the stuff can be useful for other drivers and
  quirks etc.

- Restructure struct edp_panel_entry to contain struct
  drm_edid_ident. Change the iteration of edp_panels array to use
  drm_edid_match() on the array elements and the edid.

- Add a function to read the EDID base block *but* make it return const
  struct drm_edid *. Add warnings in the comments that it's only for
  panel and for transition until it switches to reading full EDIDs.

    const struct drm_edid *drm_edid_read_base_block(struct i2c_adapter *adapter);

  This is the *only* hackish part of the whole thing, and it's nicely
  isolated. For the most part you can use drm_edid_get_panel_id() code
  for this, just return the blob wrapped in a struct drm_edid envelope.

- Remove function:

    u32 drm_edid_get_panel_id(struct i2c_adapter *adapter);

- Refactor edid_quirk_list to use the same id struct and match function
  and mechanism within drm_edid.c (can be follow-up too).

- Once you change the panel code to read the whole EDID using
  drm_edid_read family of functions in the future, you don't have to
  change *anything* about the iteration or matching or anything, because
  it's already passing struct drm_edid around.


I hope this covers everything.

BR,
Jani.


>  /**
>   * drm_edid_get_base_block - Get a panel's EDID base block
>   * @adapter: I2C adapter to use for DDC
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 2455d6ab2221..248ddb0a6b5d 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -416,6 +416,7 @@ struct edid *drm_get_edid(struct drm_connector *connector,
>  			  struct i2c_adapter *adapter);
>  struct edid_base_block *drm_edid_get_base_block(struct i2c_adapter *adapter);
>  u32 drm_edid_get_panel_id(struct edid_base_block *base_block);
> +bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str);
>  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 v3 2/4] drm/edid: Add a function to check monitor string

[Hsin-Yi Wang]

On Mon, Mar 4, 2024 at 12:38 PM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
> On Mon, 04 Mar 2024, Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> > Add a function to check if the EDID base block contains a given string.
> >
> > One of the use cases is fetching panel from a list of panel names, since
> > some panel vendors put the monitor name after EDID_DETAIL_MONITOR_STRING
> > instead of EDID_DETAIL_MONITOR_NAME.
> >
> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > ---
> > v2->v3: move string matching to drm_edid
> > ---
> >  drivers/gpu/drm/drm_edid.c | 49 ++++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_edid.h     |  1 +
> >  2 files changed, 50 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 13454bc64ca2..fcdc2bd143dd 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -2789,6 +2789,55 @@ u32 drm_edid_get_panel_id(struct edid_base_block *base_block)
> >  }
> >  EXPORT_SYMBOL(drm_edid_get_panel_id);
> >
> > +/**
> > + * drm_edid_has_monitor_string - Check if a EDID base block has certain string.
> > + * @base_block: EDID base block to check.
> > + * @str: pointer to a character array to hold the string to be checked.
> > + *
> > + * Check if the detailed timings section of a EDID base block has the given
> > + * string.
> > + *
> > + * Return: True if the EDID base block contains the string, false otherwise.
> > + */
> > +bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str)
> > +{
> > +     unsigned int i, j, k, buflen = strlen(str);
> > +
> > +     for (i = 0; i < EDID_DETAILED_TIMINGS; i++) {
> > +             struct detailed_timing *timing = &base_block->edid.detailed_timings[i];
> > +             unsigned int size = ARRAY_SIZE(timing->data.other_data.data.str.str);
> > +
> > +             if (buflen > size || 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 < buflen; j++) {
> > +                     char c = timing->data.other_data.data.str.str[j];
> > +
> > +                     if (c != str[j] ||  c == '\n')
> > +                             break;
> > +             }
> > +
> > +             if (j == buflen) {
> > +                     /* Allow trailing white spaces. */
> > +                     for (k = j; k < size; k++) {
> > +                             char c = timing->data.other_data.data.str.str[k];
> > +
> > +                             if (c == '\n')
> > +                                     return true;
> > +                             else if (c != ' ')
> > +                                     break;
> > +                     }
> > +                     if (k == size)
> > +                             return true;
> > +             }
> > +     }
> > +
> > +     return false;
> > +}
> > +
>
> So we've put a lot of effort into converting from struct edid to struct
> drm_edid, passing that around in drm_edid.c, with the allocation size it
> provides, and generally cleaning stuff up.
>
> I'm not at all happy to see *another* struct added just for the base
> block, and detailed timing iteration as well as monitor name parsing
> duplicated.
>
> With struct drm_edid you can actually return an EDID that only has the
> base block and size 128, even if the EDID indicates more
> extensions. Because the whole thing is *designed* to handle that
> gracefully. The allocated size matters, not what the blob originating
> outside of the kernel tells you.
>
> What I'm thinking is:
>
> - Add some struct drm_edid_ident or similar. Add all the information
>   that's needed to identify a panel there. I guess initially that's
>   panel_id and name.
>
>     struct drm_edid_ident {
>         u32 panel_id;
>         const char *name;
>     };
>
> - Add function:
>
>     bool drm_edid_match(const struct drm_edid *drm_edid, const struct drm_edid_ident *ident);
>
>   Check if stuff in ident matches drm_edid. You can use and extend the
>   existing drm_edid based iteration etc. in
>   drm_edid.c. Straightforward. The fields in ident can trivially be
>   extended later, and the stuff can be useful for other drivers and
>   quirks etc.
>
> - Restructure struct edp_panel_entry to contain struct
>   drm_edid_ident. Change the iteration of edp_panels array to use
>   drm_edid_match() on the array elements and the edid.
>
> - Add a function to read the EDID base block *but* make it return const
>   struct drm_edid *. Add warnings in the comments that it's only for
>   panel and for transition until it switches to reading full EDIDs.
>
>     const struct drm_edid *drm_edid_read_base_block(struct i2c_adapter *adapter);
>
>   This is the *only* hackish part of the whole thing, and it's nicely
>   isolated. For the most part you can use drm_edid_get_panel_id() code
>   for this, just return the blob wrapped in a struct drm_edid envelope.

To clarify:
struct drm_edid currently is only internal to drm_edid.c. So with
change we will have to move it to the header drm_edid.h

>
> - Remove function:
>
>     u32 drm_edid_get_panel_id(struct i2c_adapter *adapter);
>

Probably change to u32 drm_edid_get_panel_id(const struct drm_edid
*);? Given that we still need to parse id from
drm_edid_read_base_block().

> - Refactor edid_quirk_list to use the same id struct and match function
>   and mechanism within drm_edid.c (can be follow-up too).
>

edid_quirk currently doesn't have panel names in it, and it might be a
bit difficult to get all the correct names of these panels without
having the datasheets.
One way is to leave the name as null and if the name is empty and skip
matching the name in drm_edid_match().

> - Once you change the panel code to read the whole EDID using
>   drm_edid_read family of functions in the future, you don't have to
>   change *anything* about the iteration or matching or anything, because
>   it's already passing struct drm_edid around.
>
>
> I hope this covers everything.
>
> BR,
> Jani.
>
>
> >  /**
> >   * drm_edid_get_base_block - Get a panel's EDID base block
> >   * @adapter: I2C adapter to use for DDC
> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> > index 2455d6ab2221..248ddb0a6b5d 100644
> > --- a/include/drm/drm_edid.h
> > +++ b/include/drm/drm_edid.h
> > @@ -416,6 +416,7 @@ struct edid *drm_get_edid(struct drm_connector *connector,
> >                         struct i2c_adapter *adapter);
> >  struct edid_base_block *drm_edid_get_base_block(struct i2c_adapter *adapter);
> >  u32 drm_edid_get_panel_id(struct edid_base_block *base_block);
> > +bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str);
> >  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 v3 2/4] drm/edid: Add a function to check monitor string

[Dmitry Baryshkov]

On Mon, 4 Mar 2024 at 22:38, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
> On Mon, 04 Mar 2024, Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> > Add a function to check if the EDID base block contains a given string.
> >
> > One of the use cases is fetching panel from a list of panel names, since
> > some panel vendors put the monitor name after EDID_DETAIL_MONITOR_STRING
> > instead of EDID_DETAIL_MONITOR_NAME.
> >
> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > ---
> > v2->v3: move string matching to drm_edid
> > ---
> >  drivers/gpu/drm/drm_edid.c | 49 ++++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_edid.h     |  1 +
> >  2 files changed, 50 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 13454bc64ca2..fcdc2bd143dd 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -2789,6 +2789,55 @@ u32 drm_edid_get_panel_id(struct edid_base_block *base_block)
> >  }
> >  EXPORT_SYMBOL(drm_edid_get_panel_id);
> >
> > +/**
> > + * drm_edid_has_monitor_string - Check if a EDID base block has certain string.
> > + * @base_block: EDID base block to check.
> > + * @str: pointer to a character array to hold the string to be checked.
> > + *
> > + * Check if the detailed timings section of a EDID base block has the given
> > + * string.
> > + *
> > + * Return: True if the EDID base block contains the string, false otherwise.
> > + */
> > +bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str)
> > +{
> > +     unsigned int i, j, k, buflen = strlen(str);
> > +
> > +     for (i = 0; i < EDID_DETAILED_TIMINGS; i++) {
> > +             struct detailed_timing *timing = &base_block->edid.detailed_timings[i];
> > +             unsigned int size = ARRAY_SIZE(timing->data.other_data.data.str.str);
> > +
> > +             if (buflen > size || 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 < buflen; j++) {
> > +                     char c = timing->data.other_data.data.str.str[j];
> > +
> > +                     if (c != str[j] ||  c == '\n')
> > +                             break;
> > +             }
> > +
> > +             if (j == buflen) {
> > +                     /* Allow trailing white spaces. */
> > +                     for (k = j; k < size; k++) {
> > +                             char c = timing->data.other_data.data.str.str[k];
> > +
> > +                             if (c == '\n')
> > +                                     return true;
> > +                             else if (c != ' ')
> > +                                     break;
> > +                     }
> > +                     if (k == size)
> > +                             return true;
> > +             }
> > +     }
> > +
> > +     return false;
> > +}
> > +
>
> So we've put a lot of effort into converting from struct edid to struct
> drm_edid, passing that around in drm_edid.c, with the allocation size it
> provides, and generally cleaning stuff up.
>
> I'm not at all happy to see *another* struct added just for the base
> block, and detailed timing iteration as well as monitor name parsing
> duplicated.
>
> With struct drm_edid you can actually return an EDID that only has the
> base block and size 128, even if the EDID indicates more
> extensions. Because the whole thing is *designed* to handle that
> gracefully. The allocated size matters, not what the blob originating
> outside of the kernel tells you.
>
> What I'm thinking is:
>
> - Add some struct drm_edid_ident or similar. Add all the information
>   that's needed to identify a panel there. I guess initially that's
>   panel_id and name.
>
>     struct drm_edid_ident {
>         u32 panel_id;
>         const char *name;
>     };
>
> - Add function:
>
>     bool drm_edid_match(const struct drm_edid *drm_edid, const struct drm_edid_ident *ident);
>
>   Check if stuff in ident matches drm_edid. You can use and extend the
>   existing drm_edid based iteration etc. in
>   drm_edid.c. Straightforward. The fields in ident can trivially be
>   extended later, and the stuff can be useful for other drivers and
>   quirks etc.

That sounds perfect!

>
> - Restructure struct edp_panel_entry to contain struct
>   drm_edid_ident. Change the iteration of edp_panels array to use
>   drm_edid_match() on the array elements and the edid.
>
> - Add a function to read the EDID base block *but* make it return const
>   struct drm_edid *. Add warnings in the comments that it's only for
>   panel and for transition until it switches to reading full EDIDs.
>
>     const struct drm_edid *drm_edid_read_base_block(struct i2c_adapter *adapter);
>
>   This is the *only* hackish part of the whole thing, and it's nicely
>   isolated. For the most part you can use drm_edid_get_panel_id() code
>   for this, just return the blob wrapped in a struct drm_edid envelope.
>
> - Remove function:
>
>     u32 drm_edid_get_panel_id(struct i2c_adapter *adapter);
>
> - Refactor edid_quirk_list to use the same id struct and match function
>   and mechanism within drm_edid.c (can be follow-up too).

I wonder if we can take one step further and merge edp_panels to
edid_quirk_list (as one of the follow-ups). Maybe just some bits of
it.


> - Once you change the panel code to read the whole EDID using
>   drm_edid_read family of functions in the future, you don't have to
>   change *anything* about the iteration or matching or anything, because
>   it's already passing struct drm_edid around.
>
>
> I hope this covers everything.
>
> BR,
> Jani.
>
>
> >  /**
> >   * drm_edid_get_base_block - Get a panel's EDID base block
> >   * @adapter: I2C adapter to use for DDC
> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> > index 2455d6ab2221..248ddb0a6b5d 100644
> > --- a/include/drm/drm_edid.h
> > +++ b/include/drm/drm_edid.h
> > @@ -416,6 +416,7 @@ struct edid *drm_get_edid(struct drm_connector *connector,
> >                         struct i2c_adapter *adapter);
> >  struct edid_base_block *drm_edid_get_base_block(struct i2c_adapter *adapter);
> >  u32 drm_edid_get_panel_id(struct edid_base_block *base_block);
> > +bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str);
> >  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



-- 
With best wishes
Dmitry

Re: [PATCH v3 2/4] drm/edid: Add a function to check monitor string

[Jani Nikula]

On Mon, 04 Mar 2024, Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> On Mon, Mar 4, 2024 at 12:38 PM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>
>> On Mon, 04 Mar 2024, Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>> > Add a function to check if the EDID base block contains a given string.
>> >
>> > One of the use cases is fetching panel from a list of panel names, since
>> > some panel vendors put the monitor name after EDID_DETAIL_MONITOR_STRING
>> > instead of EDID_DETAIL_MONITOR_NAME.
>> >
>> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
>> > ---
>> > v2->v3: move string matching to drm_edid
>> > ---
>> >  drivers/gpu/drm/drm_edid.c | 49 ++++++++++++++++++++++++++++++++++++++
>> >  include/drm/drm_edid.h     |  1 +
>> >  2 files changed, 50 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> > index 13454bc64ca2..fcdc2bd143dd 100644
>> > --- a/drivers/gpu/drm/drm_edid.c
>> > +++ b/drivers/gpu/drm/drm_edid.c
>> > @@ -2789,6 +2789,55 @@ u32 drm_edid_get_panel_id(struct edid_base_block *base_block)
>> >  }
>> >  EXPORT_SYMBOL(drm_edid_get_panel_id);
>> >
>> > +/**
>> > + * drm_edid_has_monitor_string - Check if a EDID base block has certain string.
>> > + * @base_block: EDID base block to check.
>> > + * @str: pointer to a character array to hold the string to be checked.
>> > + *
>> > + * Check if the detailed timings section of a EDID base block has the given
>> > + * string.
>> > + *
>> > + * Return: True if the EDID base block contains the string, false otherwise.
>> > + */
>> > +bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str)
>> > +{
>> > +     unsigned int i, j, k, buflen = strlen(str);
>> > +
>> > +     for (i = 0; i < EDID_DETAILED_TIMINGS; i++) {
>> > +             struct detailed_timing *timing = &base_block->edid.detailed_timings[i];
>> > +             unsigned int size = ARRAY_SIZE(timing->data.other_data.data.str.str);
>> > +
>> > +             if (buflen > size || 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 < buflen; j++) {
>> > +                     char c = timing->data.other_data.data.str.str[j];
>> > +
>> > +                     if (c != str[j] ||  c == '\n')
>> > +                             break;
>> > +             }
>> > +
>> > +             if (j == buflen) {
>> > +                     /* Allow trailing white spaces. */
>> > +                     for (k = j; k < size; k++) {
>> > +                             char c = timing->data.other_data.data.str.str[k];
>> > +
>> > +                             if (c == '\n')
>> > +                                     return true;
>> > +                             else if (c != ' ')
>> > +                                     break;
>> > +                     }
>> > +                     if (k == size)
>> > +                             return true;
>> > +             }
>> > +     }
>> > +
>> > +     return false;
>> > +}
>> > +
>>
>> So we've put a lot of effort into converting from struct edid to struct
>> drm_edid, passing that around in drm_edid.c, with the allocation size it
>> provides, and generally cleaning stuff up.
>>
>> I'm not at all happy to see *another* struct added just for the base
>> block, and detailed timing iteration as well as monitor name parsing
>> duplicated.
>>
>> With struct drm_edid you can actually return an EDID that only has the
>> base block and size 128, even if the EDID indicates more
>> extensions. Because the whole thing is *designed* to handle that
>> gracefully. The allocated size matters, not what the blob originating
>> outside of the kernel tells you.
>>
>> What I'm thinking is:
>>
>> - Add some struct drm_edid_ident or similar. Add all the information
>>   that's needed to identify a panel there. I guess initially that's
>>   panel_id and name.
>>
>>     struct drm_edid_ident {
>>         u32 panel_id;
>>         const char *name;
>>     };
>>
>> - Add function:
>>
>>     bool drm_edid_match(const struct drm_edid *drm_edid, const struct drm_edid_ident *ident);
>>
>>   Check if stuff in ident matches drm_edid. You can use and extend the
>>   existing drm_edid based iteration etc. in
>>   drm_edid.c. Straightforward. The fields in ident can trivially be
>>   extended later, and the stuff can be useful for other drivers and
>>   quirks etc.
>>
>> - Restructure struct edp_panel_entry to contain struct
>>   drm_edid_ident. Change the iteration of edp_panels array to use
>>   drm_edid_match() on the array elements and the edid.
>>
>> - Add a function to read the EDID base block *but* make it return const
>>   struct drm_edid *. Add warnings in the comments that it's only for
>>   panel and for transition until it switches to reading full EDIDs.
>>
>>     const struct drm_edid *drm_edid_read_base_block(struct i2c_adapter *adapter);
>>
>>   This is the *only* hackish part of the whole thing, and it's nicely
>>   isolated. For the most part you can use drm_edid_get_panel_id() code
>>   for this, just return the blob wrapped in a struct drm_edid envelope.
>
> To clarify:
> struct drm_edid currently is only internal to drm_edid.c. So with
> change we will have to move it to the header drm_edid.h

Absolutely not, struct drm_edid must remain an opaque type. The point is
that you ask drm_edid.c if there's a match or not, and the panel code
does not need to care what's inside struct drm_edid.

>
>>
>> - Remove function:
>>
>>     u32 drm_edid_get_panel_id(struct i2c_adapter *adapter);
>>
>
> Probably change to u32 drm_edid_get_panel_id(const struct drm_edid
> *);? Given that we still need to parse id from
> drm_edid_read_base_block().

No, we no longer need to parse the id outside of drm_edid.c. You'll have
the id's in panel code in the form of struct drm_edid_ident (or
whatever), and use the match function to see if the opaque drm_edid
matches.

>
>> - Refactor edid_quirk_list to use the same id struct and match function
>>   and mechanism within drm_edid.c (can be follow-up too).
>>
>
> edid_quirk currently doesn't have panel names in it, and it might be a
> bit difficult to get all the correct names of these panels without
> having the datasheets.
> One way is to leave the name as null and if the name is empty and skip
> matching the name in drm_edid_match().

Exactly. NULL in drm_edid_ident would mean "don't care". I think most of
the ones in panel code also won't use the name for matching.

BR,
Jani.

>
>> - Once you change the panel code to read the whole EDID using
>>   drm_edid_read family of functions in the future, you don't have to
>>   change *anything* about the iteration or matching or anything, because
>>   it's already passing struct drm_edid around.
>>
>>
>> I hope this covers everything.
>>
>> BR,
>> Jani.
>>
>>
>> >  /**
>> >   * drm_edid_get_base_block - Get a panel's EDID base block
>> >   * @adapter: I2C adapter to use for DDC
>> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>> > index 2455d6ab2221..248ddb0a6b5d 100644
>> > --- a/include/drm/drm_edid.h
>> > +++ b/include/drm/drm_edid.h
>> > @@ -416,6 +416,7 @@ struct edid *drm_get_edid(struct drm_connector *connector,
>> >                         struct i2c_adapter *adapter);
>> >  struct edid_base_block *drm_edid_get_base_block(struct i2c_adapter *adapter);
>> >  u32 drm_edid_get_panel_id(struct edid_base_block *base_block);
>> > +bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str);
>> >  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

-- 
Jani Nikula, Intel

Re: [PATCH v3 2/4] drm/edid: Add a function to check monitor string

[Hsin-Yi Wang]

On Mon, Mar 4, 2024 at 4:09 PM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
> On Mon, 04 Mar 2024, Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> > On Mon, Mar 4, 2024 at 12:38 PM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >>
> >> On Mon, 04 Mar 2024, Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> >> > Add a function to check if the EDID base block contains a given string.
> >> >
> >> > One of the use cases is fetching panel from a list of panel names, since
> >> > some panel vendors put the monitor name after EDID_DETAIL_MONITOR_STRING
> >> > instead of EDID_DETAIL_MONITOR_NAME.
> >> >
> >> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> >> > ---
> >> > v2->v3: move string matching to drm_edid
> >> > ---
> >> >  drivers/gpu/drm/drm_edid.c | 49 ++++++++++++++++++++++++++++++++++++++
> >> >  include/drm/drm_edid.h     |  1 +
> >> >  2 files changed, 50 insertions(+)
> >> >
> >> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >> > index 13454bc64ca2..fcdc2bd143dd 100644
> >> > --- a/drivers/gpu/drm/drm_edid.c
> >> > +++ b/drivers/gpu/drm/drm_edid.c
> >> > @@ -2789,6 +2789,55 @@ u32 drm_edid_get_panel_id(struct edid_base_block *base_block)
> >> >  }
> >> >  EXPORT_SYMBOL(drm_edid_get_panel_id);
> >> >
> >> > +/**
> >> > + * drm_edid_has_monitor_string - Check if a EDID base block has certain string.
> >> > + * @base_block: EDID base block to check.
> >> > + * @str: pointer to a character array to hold the string to be checked.
> >> > + *
> >> > + * Check if the detailed timings section of a EDID base block has the given
> >> > + * string.
> >> > + *
> >> > + * Return: True if the EDID base block contains the string, false otherwise.
> >> > + */
> >> > +bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str)
> >> > +{
> >> > +     unsigned int i, j, k, buflen = strlen(str);
> >> > +
> >> > +     for (i = 0; i < EDID_DETAILED_TIMINGS; i++) {
> >> > +             struct detailed_timing *timing = &base_block->edid.detailed_timings[i];
> >> > +             unsigned int size = ARRAY_SIZE(timing->data.other_data.data.str.str);
> >> > +
> >> > +             if (buflen > size || 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 < buflen; j++) {
> >> > +                     char c = timing->data.other_data.data.str.str[j];
> >> > +
> >> > +                     if (c != str[j] ||  c == '\n')
> >> > +                             break;
> >> > +             }
> >> > +
> >> > +             if (j == buflen) {
> >> > +                     /* Allow trailing white spaces. */
> >> > +                     for (k = j; k < size; k++) {
> >> > +                             char c = timing->data.other_data.data.str.str[k];
> >> > +
> >> > +                             if (c == '\n')
> >> > +                                     return true;
> >> > +                             else if (c != ' ')
> >> > +                                     break;
> >> > +                     }
> >> > +                     if (k == size)
> >> > +                             return true;
> >> > +             }
> >> > +     }
> >> > +
> >> > +     return false;
> >> > +}
> >> > +
> >>
> >> So we've put a lot of effort into converting from struct edid to struct
> >> drm_edid, passing that around in drm_edid.c, with the allocation size it
> >> provides, and generally cleaning stuff up.
> >>
> >> I'm not at all happy to see *another* struct added just for the base
> >> block, and detailed timing iteration as well as monitor name parsing
> >> duplicated.
> >>
> >> With struct drm_edid you can actually return an EDID that only has the
> >> base block and size 128, even if the EDID indicates more
> >> extensions. Because the whole thing is *designed* to handle that
> >> gracefully. The allocated size matters, not what the blob originating
> >> outside of the kernel tells you.
> >>
> >> What I'm thinking is:
> >>
> >> - Add some struct drm_edid_ident or similar. Add all the information
> >>   that's needed to identify a panel there. I guess initially that's
> >>   panel_id and name.
> >>
> >>     struct drm_edid_ident {
> >>         u32 panel_id;
> >>         const char *name;
> >>     };
> >>
> >> - Add function:
> >>
> >>     bool drm_edid_match(const struct drm_edid *drm_edid, const struct drm_edid_ident *ident);
> >>
> >>   Check if stuff in ident matches drm_edid. You can use and extend the
> >>   existing drm_edid based iteration etc. in
> >>   drm_edid.c. Straightforward. The fields in ident can trivially be
> >>   extended later, and the stuff can be useful for other drivers and
> >>   quirks etc.
> >>
> >> - Restructure struct edp_panel_entry to contain struct
> >>   drm_edid_ident. Change the iteration of edp_panels array to use
> >>   drm_edid_match() on the array elements and the edid.
> >>
> >> - Add a function to read the EDID base block *but* make it return const
> >>   struct drm_edid *. Add warnings in the comments that it's only for
> >>   panel and for transition until it switches to reading full EDIDs.
> >>
> >>     const struct drm_edid *drm_edid_read_base_block(struct i2c_adapter *adapter);
> >>
> >>   This is the *only* hackish part of the whole thing, and it's nicely
> >>   isolated. For the most part you can use drm_edid_get_panel_id() code
> >>   for this, just return the blob wrapped in a struct drm_edid envelope.
> >
> > To clarify:
> > struct drm_edid currently is only internal to drm_edid.c. So with
> > change we will have to move it to the header drm_edid.h
>
> Absolutely not, struct drm_edid must remain an opaque type. The point is
> that you ask drm_edid.c if there's a match or not, and the panel code
> does not need to care what's inside struct drm_edid.
>
> >
> >>
> >> - Remove function:
> >>
> >>     u32 drm_edid_get_panel_id(struct i2c_adapter *adapter);
> >>
> >
> > Probably change to u32 drm_edid_get_panel_id(const struct drm_edid
> > *);? Given that we still need to parse id from
> > drm_edid_read_base_block().
>
> No, we no longer need to parse the id outside of drm_edid.c. You'll have
> the id's in panel code in the form of struct drm_edid_ident (or
> whatever), and use the match function to see if the opaque drm_edid
> matches.
>
drm_panel prints the panel_id info on whether the panel is detected or not.
https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/gpu/drm/panel/panel-edp.c#L792

Is it okay to remove this information?


> >
> >> - Refactor edid_quirk_list to use the same id struct and match function
> >>   and mechanism within drm_edid.c (can be follow-up too).
> >>
> >
> > edid_quirk currently doesn't have panel names in it, and it might be a
> > bit difficult to get all the correct names of these panels without
> > having the datasheets.
> > One way is to leave the name as null and if the name is empty and skip
> > matching the name in drm_edid_match().
>
> Exactly. NULL in drm_edid_ident would mean "don't care". I think most of
> the ones in panel code also won't use the name for matching.
>
> BR,
> Jani.
>
> >
> >> - Once you change the panel code to read the whole EDID using
> >>   drm_edid_read family of functions in the future, you don't have to
> >>   change *anything* about the iteration or matching or anything, because
> >>   it's already passing struct drm_edid around.
> >>
> >>
> >> I hope this covers everything.
> >>
> >> BR,
> >> Jani.
> >>
> >>
> >> >  /**
> >> >   * drm_edid_get_base_block - Get a panel's EDID base block
> >> >   * @adapter: I2C adapter to use for DDC
> >> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> >> > index 2455d6ab2221..248ddb0a6b5d 100644
> >> > --- a/include/drm/drm_edid.h
> >> > +++ b/include/drm/drm_edid.h
> >> > @@ -416,6 +416,7 @@ struct edid *drm_get_edid(struct drm_connector *connector,
> >> >                         struct i2c_adapter *adapter);
> >> >  struct edid_base_block *drm_edid_get_base_block(struct i2c_adapter *adapter);
> >> >  u32 drm_edid_get_panel_id(struct edid_base_block *base_block);
> >> > +bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str);
> >> >  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
>
> --
> Jani Nikula, Intel

Re: [PATCH v3 2/4] drm/edid: Add a function to check monitor string

[Doug Anderson]

Hi,

On Mon, Mar 4, 2024 at 4:19 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> > > Probably change to u32 drm_edid_get_panel_id(const struct drm_edid
> > > *);? Given that we still need to parse id from
> > > drm_edid_read_base_block().
> >
> > No, we no longer need to parse the id outside of drm_edid.c. You'll have
> > the id's in panel code in the form of struct drm_edid_ident (or
> > whatever), and use the match function to see if the opaque drm_edid
> > matches.
> >
> drm_panel prints the panel_id info on whether the panel is detected or not.
> https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/gpu/drm/panel/panel-edp.c#L792
>
> Is it okay to remove this information?

Hmmm, I guess it also is exported via debugfs, actually. See
detected_panel_show() in panel-edp.c. We probably don't want to remove
that...

Re: [PATCH v3 2/4] drm/edid: Add a function to check monitor string

[Hsin-Yi Wang]

On Mon, Mar 4, 2024 at 4:09 PM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
> On Mon, 04 Mar 2024, Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> > On Mon, Mar 4, 2024 at 12:38 PM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >>
> >> On Mon, 04 Mar 2024, Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> >> > Add a function to check if the EDID base block contains a given string.
> >> >
> >> > One of the use cases is fetching panel from a list of panel names, since
> >> > some panel vendors put the monitor name after EDID_DETAIL_MONITOR_STRING
> >> > instead of EDID_DETAIL_MONITOR_NAME.
> >> >
> >> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> >> > ---
> >> > v2->v3: move string matching to drm_edid
> >> > ---
> >> >  drivers/gpu/drm/drm_edid.c | 49 ++++++++++++++++++++++++++++++++++++++
> >> >  include/drm/drm_edid.h     |  1 +
> >> >  2 files changed, 50 insertions(+)
> >> >
> >> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >> > index 13454bc64ca2..fcdc2bd143dd 100644
> >> > --- a/drivers/gpu/drm/drm_edid.c
> >> > +++ b/drivers/gpu/drm/drm_edid.c
> >> > @@ -2789,6 +2789,55 @@ u32 drm_edid_get_panel_id(struct edid_base_block *base_block)
> >> >  }
> >> >  EXPORT_SYMBOL(drm_edid_get_panel_id);
> >> >
> >> > +/**
> >> > + * drm_edid_has_monitor_string - Check if a EDID base block has certain string.
> >> > + * @base_block: EDID base block to check.
> >> > + * @str: pointer to a character array to hold the string to be checked.
> >> > + *
> >> > + * Check if the detailed timings section of a EDID base block has the given
> >> > + * string.
> >> > + *
> >> > + * Return: True if the EDID base block contains the string, false otherwise.
> >> > + */
> >> > +bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str)
> >> > +{
> >> > +     unsigned int i, j, k, buflen = strlen(str);
> >> > +
> >> > +     for (i = 0; i < EDID_DETAILED_TIMINGS; i++) {
> >> > +             struct detailed_timing *timing = &base_block->edid.detailed_timings[i];
> >> > +             unsigned int size = ARRAY_SIZE(timing->data.other_data.data.str.str);
> >> > +
> >> > +             if (buflen > size || 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 < buflen; j++) {
> >> > +                     char c = timing->data.other_data.data.str.str[j];
> >> > +
> >> > +                     if (c != str[j] ||  c == '\n')
> >> > +                             break;
> >> > +             }
> >> > +
> >> > +             if (j == buflen) {
> >> > +                     /* Allow trailing white spaces. */
> >> > +                     for (k = j; k < size; k++) {
> >> > +                             char c = timing->data.other_data.data.str.str[k];
> >> > +
> >> > +                             if (c == '\n')
> >> > +                                     return true;
> >> > +                             else if (c != ' ')
> >> > +                                     break;
> >> > +                     }
> >> > +                     if (k == size)
> >> > +                             return true;
> >> > +             }
> >> > +     }
> >> > +
> >> > +     return false;
> >> > +}
> >> > +
> >>
> >> So we've put a lot of effort into converting from struct edid to struct
> >> drm_edid, passing that around in drm_edid.c, with the allocation size it
> >> provides, and generally cleaning stuff up.
> >>
> >> I'm not at all happy to see *another* struct added just for the base
> >> block, and detailed timing iteration as well as monitor name parsing
> >> duplicated.
> >>
> >> With struct drm_edid you can actually return an EDID that only has the
> >> base block and size 128, even if the EDID indicates more
> >> extensions. Because the whole thing is *designed* to handle that
> >> gracefully. The allocated size matters, not what the blob originating
> >> outside of the kernel tells you.
> >>
> >> What I'm thinking is:
> >>
> >> - Add some struct drm_edid_ident or similar. Add all the information
> >>   that's needed to identify a panel there. I guess initially that's
> >>   panel_id and name.
> >>
> >>     struct drm_edid_ident {
> >>         u32 panel_id;
> >>         const char *name;
> >>     };
> >>
> >> - Add function:
> >>
> >>     bool drm_edid_match(const struct drm_edid *drm_edid, const struct drm_edid_ident *ident);
> >>
> >>   Check if stuff in ident matches drm_edid. You can use and extend the
> >>   existing drm_edid based iteration etc. in
> >>   drm_edid.c. Straightforward. The fields in ident can trivially be
> >>   extended later, and the stuff can be useful for other drivers and
> >>   quirks etc.
> >>
> >> - Restructure struct edp_panel_entry to contain struct
> >>   drm_edid_ident. Change the iteration of edp_panels array to use
> >>   drm_edid_match() on the array elements and the edid.
> >>
> >> - Add a function to read the EDID base block *but* make it return const
> >>   struct drm_edid *. Add warnings in the comments that it's only for
> >>   panel and for transition until it switches to reading full EDIDs.
> >>
> >>     const struct drm_edid *drm_edid_read_base_block(struct i2c_adapter *adapter);
> >>
> >>   This is the *only* hackish part of the whole thing, and it's nicely
> >>   isolated. For the most part you can use drm_edid_get_panel_id() code
> >>   for this, just return the blob wrapped in a struct drm_edid envelope.
> >
> > To clarify:
> > struct drm_edid currently is only internal to drm_edid.c. So with
> > change we will have to move it to the header drm_edid.h
>
> Absolutely not, struct drm_edid must remain an opaque type. The point is
> that you ask drm_edid.c if there's a match or not, and the panel code
> does not need to care what's inside struct drm_edid.
>

Sorry I might be misunderstanding about the requests here:

If drm_edid should remain opaque, then struct drm_edid remains opaque,
drm_edid_match() should take struct edid *edid as a parameter? just as
other exposed functions in drm_edid.

If panel edp doesn't hold drm_edid returned from
drm_edid_read_base_block(), what should it use to iterate the
edp_panels array?

for (panel = edp_panels; panel->panel_id; panel++)
    if(drm_edid_match(drm_edid, panel->ident))
        ...

> >
> >>
> >> - Remove function:
> >>
> >>     u32 drm_edid_get_panel_id(struct i2c_adapter *adapter);
> >>
> >
> > Probably change to u32 drm_edid_get_panel_id(const struct drm_edid
> > *);? Given that we still need to parse id from
> > drm_edid_read_base_block().
>
> No, we no longer need to parse the id outside of drm_edid.c. You'll have
> the id's in panel code in the form of struct drm_edid_ident (or
> whatever), and use the match function to see if the opaque drm_edid
> matches.
>
> >
> >> - Refactor edid_quirk_list to use the same id struct and match function
> >>   and mechanism within drm_edid.c (can be follow-up too).
> >>
> >
> > edid_quirk currently doesn't have panel names in it, and it might be a
> > bit difficult to get all the correct names of these panels without
> > having the datasheets.
> > One way is to leave the name as null and if the name is empty and skip
> > matching the name in drm_edid_match().
>
> Exactly. NULL in drm_edid_ident would mean "don't care". I think most of
> the ones in panel code also won't use the name for matching.
>
> BR,
> Jani.
>
> >
> >> - Once you change the panel code to read the whole EDID using
> >>   drm_edid_read family of functions in the future, you don't have to
> >>   change *anything* about the iteration or matching or anything, because
> >>   it's already passing struct drm_edid around.
> >>
> >>
> >> I hope this covers everything.
> >>
> >> BR,
> >> Jani.
> >>
> >>
> >> >  /**
> >> >   * drm_edid_get_base_block - Get a panel's EDID base block
> >> >   * @adapter: I2C adapter to use for DDC
> >> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> >> > index 2455d6ab2221..248ddb0a6b5d 100644
> >> > --- a/include/drm/drm_edid.h
> >> > +++ b/include/drm/drm_edid.h
> >> > @@ -416,6 +416,7 @@ struct edid *drm_get_edid(struct drm_connector *connector,
> >> >                         struct i2c_adapter *adapter);
> >> >  struct edid_base_block *drm_edid_get_base_block(struct i2c_adapter *adapter);
> >> >  u32 drm_edid_get_panel_id(struct edid_base_block *base_block);
> >> > +bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str);
> >> >  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
>
> --
> Jani Nikula, Intel

Re: [PATCH v3 2/4] drm/edid: Add a function to check monitor string

[Jani Nikula]

On Mon, 04 Mar 2024, Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> On Mon, Mar 4, 2024 at 4:09 PM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>
>> On Mon, 04 Mar 2024, Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>> > To clarify:
>> > struct drm_edid currently is only internal to drm_edid.c. So with
>> > change we will have to move it to the header drm_edid.h
>>
>> Absolutely not, struct drm_edid must remain an opaque type. The point is
>> that you ask drm_edid.c if there's a match or not, and the panel code
>> does not need to care what's inside struct drm_edid.
>>
>
> Sorry I might be misunderstanding about the requests here:
>
> If drm_edid should remain opaque, then struct drm_edid remains opaque,
> drm_edid_match() should take struct edid *edid as a parameter? just as
> other exposed functions in drm_edid.

No, it should take struct drm_edid *.

> If panel edp doesn't hold drm_edid returned from
> drm_edid_read_base_block(), what should it use to iterate the
> edp_panels array?

Panel edp can hold a *pointer* to struct drm_edid * without knowing the
full type. This is one of the points of struct drm_edid. Focus more of
the EDID parsing within drm_edid.c instead of having everyone parse it
to varying degrees of correctness.

>
> for (panel = edp_panels; panel->panel_id; panel++)
>     if(drm_edid_match(drm_edid, panel->ident))
>         ...
>

BR,
Jani.

-- 
Jani Nikula, Intel

Re: [PATCH v3 2/4] drm/edid: Add a function to check monitor string

[Jani Nikula]

On Mon, 04 Mar 2024, Doug Anderson <dianders@chromium.org> wrote:
> Hi,
>
> On Mon, Mar 4, 2024 at 4:19 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>>
>> > > Probably change to u32 drm_edid_get_panel_id(const struct drm_edid
>> > > *);? Given that we still need to parse id from
>> > > drm_edid_read_base_block().
>> >
>> > No, we no longer need to parse the id outside of drm_edid.c. You'll have
>> > the id's in panel code in the form of struct drm_edid_ident (or
>> > whatever), and use the match function to see if the opaque drm_edid
>> > matches.
>> >
>> drm_panel prints the panel_id info on whether the panel is detected or not.
>> https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/gpu/drm/panel/panel-edp.c#L792
>>
>> Is it okay to remove this information?
>
> Hmmm, I guess it also is exported via debugfs, actually. See
> detected_panel_show() in panel-edp.c. We probably don't want to remove
> that...

You currently print the information via panel->detected_panel, which is
a struct edp_panel_entry *. That doesn't change. It'll be slightly
restructured to contain a struct drm_edid_ident, which will not be an
opaque type.

BR,
Jani.


-- 
Jani Nikula, Intel

Re: [PATCH v3 2/4] drm/edid: Add a function to check monitor string

[Doug Anderson]

Hi,

On Tue, Mar 5, 2024 at 12:17 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
> On Mon, 04 Mar 2024, Doug Anderson <dianders@chromium.org> wrote:
> > Hi,
> >
> > On Mon, Mar 4, 2024 at 4:19 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> >>
> >> > > Probably change to u32 drm_edid_get_panel_id(const struct drm_edid
> >> > > *);? Given that we still need to parse id from
> >> > > drm_edid_read_base_block().
> >> >
> >> > No, we no longer need to parse the id outside of drm_edid.c. You'll have
> >> > the id's in panel code in the form of struct drm_edid_ident (or
> >> > whatever), and use the match function to see if the opaque drm_edid
> >> > matches.
> >> >
> >> drm_panel prints the panel_id info on whether the panel is detected or not.
> >> https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/gpu/drm/panel/panel-edp.c#L792
> >>
> >> Is it okay to remove this information?
> >
> > Hmmm, I guess it also is exported via debugfs, actually. See
> > detected_panel_show() in panel-edp.c. We probably don't want to remove
> > that...
>
> You currently print the information via panel->detected_panel, which is
> a struct edp_panel_entry *. That doesn't change. It'll be slightly
> restructured to contain a struct drm_edid_ident, which will not be an
> opaque type.

Hmm. As Hsin-Yi pointed out to me offline. Somehow we'll need to get
the actual panel ID out. Right now in panel-edp.c we have:

dev_warn(dev,
  "Unknown panel %s %#06x, using conservative timings\n",
  vend, product_id);

Where "vend" and "product_id" come from the panel ID of a panel that
we didn't recognize. For instance:

  Unknown panel BOE 0x0731, using conservative timings

We need to still be able to print this message for unrecognized
panels. Then when we see field reports including this message we know
that somehow we ended up shipping an unrecognized panel.

Any suggestions on what abstraction you'd like to see to enable us to
print that message if everything is opaque?

-Doug

Re: [PATCH v3 2/4] drm/edid: Add a function to check monitor string

[Hsin-Yi Wang]

On Tue, Mar 5, 2024 at 11:25 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, Mar 5, 2024 at 12:17 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >
> > On Mon, 04 Mar 2024, Doug Anderson <dianders@chromium.org> wrote:
> > > Hi,
> > >
> > > On Mon, Mar 4, 2024 at 4:19 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> > >>
> > >> > > Probably change to u32 drm_edid_get_panel_id(const struct drm_edid
> > >> > > *);? Given that we still need to parse id from
> > >> > > drm_edid_read_base_block().
> > >> >
> > >> > No, we no longer need to parse the id outside of drm_edid.c. You'll have
> > >> > the id's in panel code in the form of struct drm_edid_ident (or
> > >> > whatever), and use the match function to see if the opaque drm_edid
> > >> > matches.
> > >> >
> > >> drm_panel prints the panel_id info on whether the panel is detected or not.
> > >> https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/gpu/drm/panel/panel-edp.c#L792
> > >>
> > >> Is it okay to remove this information?
> > >
> > > Hmmm, I guess it also is exported via debugfs, actually. See
> > > detected_panel_show() in panel-edp.c. We probably don't want to remove
> > > that...
> >
> > You currently print the information via panel->detected_panel, which is
> > a struct edp_panel_entry *. That doesn't change. It'll be slightly
> > restructured to contain a struct drm_edid_ident, which will not be an
> > opaque type.
>
> Hmm. As Hsin-Yi pointed out to me offline. Somehow we'll need to get
> the actual panel ID out. Right now in panel-edp.c we have:
>
> dev_warn(dev,
>   "Unknown panel %s %#06x, using conservative timings\n",
>   vend, product_id);
>
> Where "vend" and "product_id" come from the panel ID of a panel that
> we didn't recognize. For instance:
>
>   Unknown panel BOE 0x0731, using conservative timings
>
> We need to still be able to print this message for unrecognized
> panels. Then when we see field reports including this message we know
> that somehow we ended up shipping an unrecognized panel.
>
> Any suggestions on what abstraction you'd like to see to enable us to
> print that message if everything is opaque?

Sent v4 here: https://lore.kernel.org/lkml/20240306004347.974304-1-hsinyi@chromium.org/

Besides that it still keeps drm_edid_get_panel_id() to be used on the
kernel warning when no panel is matched, other parts I think are
following the comments.

Thanks.
>
> -Doug

Re: [PATCH v3 2/4] drm/edid: Add a function to check monitor string

[Jani Nikula]

On Tue, 05 Mar 2024, Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> On Tue, Mar 5, 2024 at 11:25 AM Doug Anderson <dianders@chromium.org> wrote:
>> Hmm. As Hsin-Yi pointed out to me offline. Somehow we'll need to get
>> the actual panel ID out. Right now in panel-edp.c we have:
>>
>> dev_warn(dev,
>>   "Unknown panel %s %#06x, using conservative timings\n",
>>   vend, product_id);
>>
>> Where "vend" and "product_id" come from the panel ID of a panel that
>> we didn't recognize. For instance:
>>
>>   Unknown panel BOE 0x0731, using conservative timings
>>
>> We need to still be able to print this message for unrecognized
>> panels. Then when we see field reports including this message we know
>> that somehow we ended up shipping an unrecognized panel.
>>
>> Any suggestions on what abstraction you'd like to see to enable us to
>> print that message if everything is opaque?
>
> Sent v4 here: https://lore.kernel.org/lkml/20240306004347.974304-1-hsinyi@chromium.org/
>
> Besides that it still keeps drm_edid_get_panel_id() to be used on the
> kernel warning when no panel is matched, other parts I think are
> following the comments.

Yeah we can keep that for now.

BR,
Jani.


-- 
Jani Nikula, Intel