[PATCH 00/12] drm/edid: low level EDID block read refactoring etc.

[PATCH 00/12] drm/edid: low level EDID block read refactoring etc.

[Jani Nikula]

Ever so slowly moving towards cleaner EDID reading.

Jani Nikula (12):
  drm/edid: convert edid_is_zero() to edid_block_is_zero() for blocks
  drm/edid: have edid_block_check() detect blocks that are all zero
  drm/edid: refactor EDID block status printing
  drm/edid: add a helper to log dump an EDID block
  drm/edid: pass struct edid to connector_bad_edid()
  drm/edid: add typedef for block read function
  drm/edid: abstract an EDID block read helper
  drm/edid: use EDID block read helper in drm_do_get_edid()
  drm/edid: convert extension block read to EDID block read helper
  drm/edid: drop extra local var
  drm/edid: add single point of return to drm_do_get_edid()
  drm/edid: add EDID block count and size helpers

 drivers/gpu/drm/drm_edid.c | 350 ++++++++++++++++++++++++-------------
 1 file changed, 225 insertions(+), 125 deletions(-)

-- 
2.30.2

[PATCH 01/12] drm/edid: convert edid_is_zero() to edid_block_is_zero() for blocks

[Jani Nikula]

As edid_is_zero() is only ever used on EDID blocks, convert it to
edid_block_is_zero() with implicit block size.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 04e818ecd662..f4da3f92f41b 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1632,9 +1632,9 @@ static int edid_block_tag(const void *_block)
 	return block[0];
 }
 
-static bool edid_is_zero(const void *edid, int length)
+static bool edid_block_is_zero(const void *edid)
 {
-	return !memchr_inv(edid, 0, length);
+	return !memchr_inv(edid, 0, EDID_LENGTH);
 }
 
 /**
@@ -1785,7 +1785,7 @@ bool drm_edid_block_valid(u8 *_block, int block_num, bool print_bad_edid,
 	}
 
 	if (!valid && print_bad_edid) {
-		if (edid_is_zero(block, EDID_LENGTH)) {
+		if (edid_block_is_zero(block)) {
 			pr_notice("EDID block is all zeroes\n");
 		} else {
 			pr_notice("Raw EDID:\n");
@@ -1942,7 +1942,7 @@ static void connector_bad_edid(struct drm_connector *connector,
 		u8 *block = edid + i * EDID_LENGTH;
 		char prefix[20];
 
-		if (edid_is_zero(block, EDID_LENGTH))
+		if (edid_block_is_zero(block))
 			sprintf(prefix, "\t[%02x] ZERO ", i);
 		else if (!drm_edid_block_valid(block, i, false, NULL))
 			sprintf(prefix, "\t[%02x] BAD  ", i);
@@ -2019,7 +2019,7 @@ static struct edid *drm_do_get_edid_base_block(struct drm_connector *connector,
 			goto out;
 		if (drm_edid_block_valid(edid, 0, false, edid_corrupt))
 			break;
-		if (try == 0 && edid_is_zero(edid, EDID_LENGTH)) {
+		if (try == 0 && edid_block_is_zero(edid)) {
 			if (null_edid_counter)
 				(*null_edid_counter)++;
 			goto carp;
-- 
2.30.2

[PATCH 02/12] drm/edid: have edid_block_check() detect blocks that are all zero

[Jani Nikula]

We have the check function, have it also detect blocks that are all zero
instead of leaving that to callers.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index f4da3f92f41b..f062d1715ec3 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1671,6 +1671,7 @@ EXPORT_SYMBOL(drm_edid_are_equal);
 enum edid_block_status {
 	EDID_BLOCK_OK = 0,
 	EDID_BLOCK_NULL,
+	EDID_BLOCK_ZERO,
 	EDID_BLOCK_HEADER_CORRUPT,
 	EDID_BLOCK_HEADER_REPAIR,
 	EDID_BLOCK_HEADER_FIXED,
@@ -1689,15 +1690,23 @@ static enum edid_block_status edid_block_check(const void *_block,
 	if (is_base_block) {
 		int score = drm_edid_header_is_valid(block);
 
-		if (score < clamp(edid_fixup, 0, 8))
-			return EDID_BLOCK_HEADER_CORRUPT;
+		if (score < clamp(edid_fixup, 0, 8)) {
+			if (edid_block_is_zero(block))
+				return EDID_BLOCK_ZERO;
+			else
+				return EDID_BLOCK_HEADER_CORRUPT;
+		}
 
 		if (score < 8)
 			return EDID_BLOCK_HEADER_REPAIR;
 	}
 
-	if (edid_block_compute_checksum(block) != edid_block_get_checksum(block))
-		return EDID_BLOCK_CHECKSUM;
+	if (edid_block_compute_checksum(block) != edid_block_get_checksum(block)) {
+		if (edid_block_is_zero(block))
+			return EDID_BLOCK_ZERO;
+		else
+			return EDID_BLOCK_CHECKSUM;
+	}
 
 	if (is_base_block) {
 		if (block->version != 1)
@@ -1785,7 +1794,7 @@ bool drm_edid_block_valid(u8 *_block, int block_num, bool print_bad_edid,
 	}
 
 	if (!valid && print_bad_edid) {
-		if (edid_block_is_zero(block)) {
+		if (status == EDID_BLOCK_ZERO) {
 			pr_notice("EDID block is all zeroes\n");
 		} else {
 			pr_notice("Raw EDID:\n");
-- 
2.30.2

[PATCH 03/12] drm/edid: refactor EDID block status printing

[Jani Nikula]

Split out a function to log EDID block status. The printouts get changed
slightly.

Unfortunately, not all users will have struct drm_device available, so
we convert to pr_* debug logging instead of drm device based logging.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 75 ++++++++++++++++++++++++++------------
 1 file changed, 51 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index f062d1715ec3..3d04d63464ba 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1729,6 +1729,50 @@ static bool edid_block_valid(const void *block, bool base)
 				       edid_block_tag(block));
 }
 
+static void edid_block_status_print(enum edid_block_status status,
+				    const struct edid *block,
+				    int block_num)
+{
+	switch (status) {
+	case EDID_BLOCK_OK:
+		break;
+	case EDID_BLOCK_NULL:
+		pr_debug("EDID block %d pointer is NULL\n", block_num);
+		break;
+	case EDID_BLOCK_ZERO:
+		pr_notice("EDID block %d is all zeroes\n", block_num);
+		break;
+	case EDID_BLOCK_HEADER_CORRUPT:
+		pr_notice("EDID has corrupt header\n");
+		break;
+	case EDID_BLOCK_HEADER_REPAIR:
+		pr_debug("EDID corrupt header needs repair\n");
+		break;
+	case EDID_BLOCK_HEADER_FIXED:
+		pr_debug("EDID corrupt header fixed\n");
+		break;
+	case EDID_BLOCK_CHECKSUM:
+		if (edid_block_status_valid(status, edid_block_tag(block))) {
+			pr_debug("EDID block %d (tag 0x%02x) checksum is invalid, remainder is %d, ignoring\n",
+				 block_num, edid_block_tag(block),
+				 edid_block_compute_checksum(block));
+		} else {
+			pr_notice("EDID block %d (tag 0x%02x) checksum is invalid, remainder is %d\n",
+				  block_num, edid_block_tag(block),
+				  edid_block_compute_checksum(block));
+		}
+		break;
+	case EDID_BLOCK_VERSION:
+		pr_notice("EDID has major version %d, instead of 1\n",
+			  block->version);
+		break;
+	default:
+		pr_debug("EDID block %d unknown edid block status code %d\n",
+			 block_num, status);
+		break;
+	}
+}
+
 /**
  * drm_edid_block_valid - Sanity check the EDID block (base or extension)
  * @raw_edid: pointer to raw EDID block
@@ -1775,33 +1819,16 @@ bool drm_edid_block_valid(u8 *_block, int block_num, bool print_bad_edid,
 			*edid_corrupt = true;
 	}
 
+	edid_block_status_print(status, block, block_num);
+
 	/* Determine whether we can use this block with this status. */
 	valid = edid_block_status_valid(status, edid_block_tag(block));
 
-	/* Some fairly random status printouts. */
-	if (status == EDID_BLOCK_CHECKSUM) {
-		if (valid) {
-			DRM_DEBUG("EDID block checksum is invalid, remainder is %d\n",
-				  edid_block_compute_checksum(block));
-			DRM_DEBUG("Assuming a KVM switch modified the block but left the original checksum\n");
-		} else if (print_bad_edid) {
-			DRM_NOTE("EDID block checksum is invalid, remainder is %d\n",
-				 edid_block_compute_checksum(block));
-		}
-	} else if (status == EDID_BLOCK_VERSION) {
-		DRM_NOTE("EDID has major version %d, instead of 1\n",
-			 block->version);
-	}
-
-	if (!valid && print_bad_edid) {
-		if (status == EDID_BLOCK_ZERO) {
-			pr_notice("EDID block is all zeroes\n");
-		} else {
-			pr_notice("Raw EDID:\n");
-			print_hex_dump(KERN_NOTICE,
-				       " \t", DUMP_PREFIX_NONE, 16, 1,
-				       block, EDID_LENGTH, false);
-		}
+	if (!valid && print_bad_edid && status != EDID_BLOCK_ZERO) {
+		pr_notice("Raw EDID:\n");
+		print_hex_dump(KERN_NOTICE,
+			       " \t", DUMP_PREFIX_NONE, 16, 1,
+			       block, EDID_LENGTH, false);
 	}
 
 	return valid;
-- 
2.30.2

[PATCH 04/12] drm/edid: add a helper to log dump an EDID block

[Jani Nikula]

Unify debug log dumping. There's duplication in the error paths for EDID
block validity checks, but this should be neglible.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 3d04d63464ba..8638e54e0879 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1773,6 +1773,23 @@ static void edid_block_status_print(enum edid_block_status status,
 	}
 }
 
+static void edid_block_dump(const char *level, const void *block, int block_num)
+{
+	enum edid_block_status status;
+	char prefix[20];
+
+	status = edid_block_check(block, block_num == 0);
+	if (status == EDID_BLOCK_ZERO)
+		sprintf(prefix, "\t[%02x] ZERO ", block_num);
+	else if (!edid_block_status_valid(status, edid_block_tag(block)))
+		sprintf(prefix, "\t[%02x] BAD  ", block_num);
+	else
+		sprintf(prefix, "\t[%02x] GOOD ", block_num);
+
+	print_hex_dump(level, prefix, DUMP_PREFIX_NONE, 16, 1,
+		       block, EDID_LENGTH, false);
+}
+
 /**
  * drm_edid_block_valid - Sanity check the EDID block (base or extension)
  * @raw_edid: pointer to raw EDID block
@@ -1826,9 +1843,7 @@ bool drm_edid_block_valid(u8 *_block, int block_num, bool print_bad_edid,
 
 	if (!valid && print_bad_edid && status != EDID_BLOCK_ZERO) {
 		pr_notice("Raw EDID:\n");
-		print_hex_dump(KERN_NOTICE,
-			       " \t", DUMP_PREFIX_NONE, 16, 1,
-			       block, EDID_LENGTH, false);
+		edid_block_dump(KERN_NOTICE, block, block_num);
 	}
 
 	return valid;
@@ -1976,18 +1991,8 @@ static void connector_bad_edid(struct drm_connector *connector,
 	drm_dbg_kms(connector->dev, "%s: EDID is invalid:\n", connector->name);
 	for (i = 0; i < num_blocks; i++) {
 		u8 *block = edid + i * EDID_LENGTH;
-		char prefix[20];
-
-		if (edid_block_is_zero(block))
-			sprintf(prefix, "\t[%02x] ZERO ", i);
-		else if (!drm_edid_block_valid(block, i, false, NULL))
-			sprintf(prefix, "\t[%02x] BAD  ", i);
-		else
-			sprintf(prefix, "\t[%02x] GOOD ", i);
 
-		print_hex_dump(KERN_DEBUG,
-			       prefix, DUMP_PREFIX_NONE, 16, 1,
-			       block, EDID_LENGTH, false);
+		edid_block_dump(KERN_DEBUG, block, i);
 	}
 }
 
-- 
2.30.2

[PATCH 05/12] drm/edid: pass struct edid to connector_bad_edid()

[Jani Nikula]

Avoid casting here and there, and make it const.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 8638e54e0879..ba54701f91f6 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1967,7 +1967,7 @@ drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned int block, size_t len)
 }
 
 static void connector_bad_edid(struct drm_connector *connector,
-			       u8 *edid, int num_blocks)
+			       const struct edid *edid, int num_blocks)
 {
 	int i;
 	u8 last_block;
@@ -1978,22 +1978,19 @@ static void connector_bad_edid(struct drm_connector *connector,
 	 * of 0x7e in the EDID of the _index_ of the last block in the
 	 * combined chunk of memory.
 	 */
-	last_block = edid[0x7e];
+	last_block = edid->extensions;
 
 	/* Calculate real checksum for the last edid extension block data */
 	if (last_block < num_blocks)
 		connector->real_edid_checksum =
-			edid_block_compute_checksum(edid + last_block * EDID_LENGTH);
+			edid_block_compute_checksum(edid + last_block);
 
 	if (connector->bad_edid_counter++ && !drm_debug_enabled(DRM_UT_KMS))
 		return;
 
 	drm_dbg_kms(connector->dev, "%s: EDID is invalid:\n", connector->name);
-	for (i = 0; i < num_blocks; i++) {
-		u8 *block = edid + i * EDID_LENGTH;
-
-		edid_block_dump(KERN_DEBUG, block, i);
-	}
+	for (i = 0; i < num_blocks; i++)
+		edid_block_dump(KERN_DEBUG, edid + i, i);
 }
 
 /* Get override or firmware EDID */
@@ -2139,7 +2136,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 	}
 
 	if (invalid_blocks) {
-		connector_bad_edid(connector, (u8 *)edid, edid->extensions + 1);
+		connector_bad_edid(connector, edid, edid->extensions + 1);
 
 		edid = edid_filter_invalid_blocks(edid, invalid_blocks);
 	}
-- 
2.30.2

[PATCH 06/12] drm/edid: add typedef for block read function

[Jani Nikula]

Make the callback a bit easier on the eye.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index ba54701f91f6..926ffe5cd97e 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2037,10 +2037,11 @@ int drm_add_override_edid_modes(struct drm_connector *connector)
 }
 EXPORT_SYMBOL(drm_add_override_edid_modes);
 
+typedef int read_block_fn(void *context, u8 *buf, unsigned int block, size_t len);
+
 static struct edid *drm_do_get_edid_base_block(struct drm_connector *connector,
-	int (*get_edid_block)(void *data, u8 *buf, unsigned int block,
-			      size_t len),
-	void *data)
+					       read_block_fn read_block,
+					       void *context)
 {
 	int *null_edid_counter = connector ? &connector->null_edid_counter : NULL;
 	bool *edid_corrupt = connector ? &connector->edid_corrupt : NULL;
@@ -2053,7 +2054,7 @@ static struct edid *drm_do_get_edid_base_block(struct drm_connector *connector,
 
 	/* base block fetch */
 	for (try = 0; try < 4; try++) {
-		if (get_edid_block(data, edid, 0, EDID_LENGTH))
+		if (read_block(context, edid, 0, EDID_LENGTH))
 			goto out;
 		if (drm_edid_block_valid(edid, 0, false, edid_corrupt))
 			break;
@@ -2097,9 +2098,8 @@ static struct edid *drm_do_get_edid_base_block(struct drm_connector *connector,
  * Return: Pointer to valid EDID or NULL if we couldn't find any.
  */
 struct edid *drm_do_get_edid(struct drm_connector *connector,
-	int (*get_edid_block)(void *data, u8 *buf, unsigned int block,
-			      size_t len),
-	void *data)
+			     read_block_fn read_block,
+			     void *context)
 {
 	int j, invalid_blocks = 0;
 	struct edid *edid, *new, *override;
@@ -2108,7 +2108,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 	if (override)
 		return override;
 
-	edid = drm_do_get_edid_base_block(connector, get_edid_block, data);
+	edid = drm_do_get_edid_base_block(connector, read_block, context);
 	if (!edid)
 		return NULL;
 
@@ -2125,7 +2125,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 		int try;
 
 		for (try = 0; try < 4; try++) {
-			if (get_edid_block(data, block, j, EDID_LENGTH))
+			if (read_block(context, block, j, EDID_LENGTH))
 				goto out;
 			if (drm_edid_block_valid(block, j, false, NULL))
 				break;
-- 
2.30.2

[PATCH 07/12] drm/edid: abstract an EDID block read helper

[Jani Nikula]

We have an abstraction for the EDID base block read, yet duplicating the
retries and error handling for extension block reads. Introduce a more
generic EDID block read helper.

Switch to the helper piecemeal, starting with drm_edid_get_panel_id(),
which doesn't need or have access to the connector anyway.

The subtle change is switching from drm_edid_block_valid() to
edid_block_check(). We also status print once, not for every attempt.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 58 +++++++++++++++++++++++++++++++++-----
 1 file changed, 51 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 926ffe5cd97e..162f0a959376 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1670,6 +1670,7 @@ EXPORT_SYMBOL(drm_edid_are_equal);
 
 enum edid_block_status {
 	EDID_BLOCK_OK = 0,
+	EDID_BLOCK_READ_FAIL,
 	EDID_BLOCK_NULL,
 	EDID_BLOCK_ZERO,
 	EDID_BLOCK_HEADER_CORRUPT,
@@ -1736,6 +1737,9 @@ static void edid_block_status_print(enum edid_block_status status,
 	switch (status) {
 	case EDID_BLOCK_OK:
 		break;
+	case EDID_BLOCK_READ_FAIL:
+		pr_debug("EDID block %d read failed\n", block_num);
+		break;
 	case EDID_BLOCK_NULL:
 		pr_debug("EDID block %d pointer is NULL\n", block_num);
 		break;
@@ -2039,6 +2043,39 @@ EXPORT_SYMBOL(drm_add_override_edid_modes);
 
 typedef int read_block_fn(void *context, u8 *buf, unsigned int block, size_t len);
 
+static enum edid_block_status edid_block_read(void *block, unsigned int block_num,
+					      read_block_fn read_block,
+					      void *context)
+{
+	enum edid_block_status status;
+	bool is_base_block = block_num == 0;
+	int try;
+
+	for (try = 0; try < 4; try++) {
+		if (read_block(context, block, block_num, EDID_LENGTH))
+			return EDID_BLOCK_READ_FAIL;
+
+		status = edid_block_check(block, is_base_block);
+		if (status == EDID_BLOCK_HEADER_REPAIR) {
+			edid_header_fix(block);
+
+			/* Retry with fixed header, update status if that worked. */
+			status = edid_block_check(block, is_base_block);
+			if (status == EDID_BLOCK_OK)
+				status = EDID_BLOCK_HEADER_FIXED;
+		}
+
+		if (edid_block_status_valid(status, edid_block_tag(block)))
+			break;
+
+		/* Fail early for unrepairable base block all zeros. */
+		if (try == 0 && is_base_block && status == EDID_BLOCK_ZERO)
+			break;
+	}
+
+	return status;
+}
+
 static struct edid *drm_do_get_edid_base_block(struct drm_connector *connector,
 					       read_block_fn read_block,
 					       void *context)
@@ -2237,20 +2274,27 @@ static u32 edid_extract_panel_id(const struct edid *edid)
 
 u32 drm_edid_get_panel_id(struct i2c_adapter *adapter)
 {
-	const struct edid *edid;
-	u32 panel_id;
-
-	edid = drm_do_get_edid_base_block(NULL, drm_do_probe_ddc_edid, 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.
 	 */
-	if (!edid)
+
+	base_block = kmalloc(EDID_LENGTH, GFP_KERNEL);
+	if (!base_block)
 		return 0;
 
-	panel_id = edid_extract_panel_id(edid);
-	kfree(edid);
+	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);
+
+	kfree(base_block);
 
 	return panel_id;
 }
-- 
2.30.2

[PATCH 08/12] drm/edid: use EDID block read helper in drm_do_get_edid()

[Jani Nikula]

Convert drm_do_get_edit() from the base block read helper to the generic
block read helper. There's quite a bit going on here, as the corrupt and
null EDID information is moved back to the caller. As we see, they were
not all that clear to begin with, and this change underlines that.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 62 ++++++++++++++------------------------
 1 file changed, 23 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 162f0a959376..bffb8a599cda 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2076,44 +2076,6 @@ static enum edid_block_status edid_block_read(void *block, unsigned int block_nu
 	return status;
 }
 
-static struct edid *drm_do_get_edid_base_block(struct drm_connector *connector,
-					       read_block_fn read_block,
-					       void *context)
-{
-	int *null_edid_counter = connector ? &connector->null_edid_counter : NULL;
-	bool *edid_corrupt = connector ? &connector->edid_corrupt : NULL;
-	void *edid;
-	int try;
-
-	edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
-	if (edid == NULL)
-		return NULL;
-
-	/* base block fetch */
-	for (try = 0; try < 4; try++) {
-		if (read_block(context, edid, 0, EDID_LENGTH))
-			goto out;
-		if (drm_edid_block_valid(edid, 0, false, edid_corrupt))
-			break;
-		if (try == 0 && edid_block_is_zero(edid)) {
-			if (null_edid_counter)
-				(*null_edid_counter)++;
-			goto carp;
-		}
-	}
-	if (try == 4)
-		goto carp;
-
-	return edid;
-
-carp:
-	if (connector)
-		connector_bad_edid(connector, edid, 1);
-out:
-	kfree(edid);
-	return NULL;
-}
-
 /**
  * drm_do_get_edid - get EDID data using a custom EDID block read function
  * @connector: connector we're probing
@@ -2138,6 +2100,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 			     read_block_fn read_block,
 			     void *context)
 {
+	enum edid_block_status status;
 	int j, invalid_blocks = 0;
 	struct edid *edid, *new, *override;
 
@@ -2145,10 +2108,31 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 	if (override)
 		return override;
 
-	edid = drm_do_get_edid_base_block(connector, read_block, context);
+	edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
 	if (!edid)
 		return NULL;
 
+	status = edid_block_read(edid, 0, read_block, context);
+
+	edid_block_status_print(status, edid, 0);
+
+	if (status == EDID_BLOCK_READ_FAIL)
+		goto out;
+
+	/* 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++;
+
+		connector_bad_edid(connector, edid, 1);
+		goto out;
+	}
+
 	if (edid->extensions == 0)
 		return edid;
 
-- 
2.30.2

[PATCH 09/12] drm/edid: convert extension block read to EDID block read helper

[Jani Nikula]

Use the EDID block read helper also for extension block reads, making
edid_block_read() the only place with the read retry logic.

Note: We observe that drm_do_get_edid() does not use invalid extension
blocks to flag the EDID as corrupt.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index bffb8a599cda..6fa17084af02 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2143,17 +2143,16 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 
 	for (j = 1; j <= edid->extensions; j++) {
 		void *block = edid + j;
-		int try;
 
-		for (try = 0; try < 4; try++) {
-			if (read_block(context, block, j, EDID_LENGTH))
-				goto out;
-			if (drm_edid_block_valid(block, j, false, NULL))
-				break;
-		}
+		status = edid_block_read(block, j, read_block, context);
 
-		if (try == 4)
+		edid_block_status_print(status, block, j);
+
+		if (!edid_block_status_valid(status, edid_block_tag(block))) {
+			if (status == EDID_BLOCK_READ_FAIL)
+				goto out;
 			invalid_blocks++;
+		}
 	}
 
 	if (invalid_blocks) {
-- 
2.30.2

[PATCH 10/12] drm/edid: drop extra local var

[Jani Nikula]

We don't need override as a variable for anything.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 6fa17084af02..767639364bd9 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2102,11 +2102,11 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 {
 	enum edid_block_status status;
 	int j, invalid_blocks = 0;
-	struct edid *edid, *new, *override;
+	struct edid *edid, *new;
 
-	override = drm_get_override_edid(connector);
-	if (override)
-		return override;
+	edid = drm_get_override_edid(connector);
+	if (edid)
+		return edid;
 
 	edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
 	if (!edid)
-- 
2.30.2

[PATCH 11/12] drm/edid: add single point of return to drm_do_get_edid()

[Jani Nikula]

This will be useful in the future. Use fail label for fail exit.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 767639364bd9..90615e30eaf5 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2106,7 +2106,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 
 	edid = drm_get_override_edid(connector);
 	if (edid)
-		return edid;
+		goto ok;
 
 	edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
 	if (!edid)
@@ -2117,7 +2117,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 	edid_block_status_print(status, edid, 0);
 
 	if (status == EDID_BLOCK_READ_FAIL)
-		goto out;
+		goto fail;
 
 	/* FIXME: Clarify what a corrupt EDID actually means. */
 	if (status == EDID_BLOCK_OK || status == EDID_BLOCK_VERSION)
@@ -2130,15 +2130,15 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 			connector->null_edid_counter++;
 
 		connector_bad_edid(connector, edid, 1);
-		goto out;
+		goto fail;
 	}
 
 	if (edid->extensions == 0)
-		return edid;
+		goto ok;
 
 	new = krealloc(edid, (edid->extensions + 1) * EDID_LENGTH, GFP_KERNEL);
 	if (!new)
-		goto out;
+		goto fail;
 	edid = new;
 
 	for (j = 1; j <= edid->extensions; j++) {
@@ -2150,7 +2150,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 
 		if (!edid_block_status_valid(status, edid_block_tag(block))) {
 			if (status == EDID_BLOCK_READ_FAIL)
-				goto out;
+				goto fail;
 			invalid_blocks++;
 		}
 	}
@@ -2161,9 +2161,10 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 		edid = edid_filter_invalid_blocks(edid, invalid_blocks);
 	}
 
+ok:
 	return edid;
 
-out:
+fail:
 	kfree(edid);
 	return NULL;
 }
-- 
2.30.2

[PATCH 12/12] drm/edid: add EDID block count and size helpers

[Jani Nikula]

Add some helpers to figure out the EDID extension block count, block
count, size, pointers to blocks.

Unfortunately, we'll need to cast away the const in a few places where
we actually need to access the data.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 78 +++++++++++++++++++++++++++-----------
 1 file changed, 56 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 90615e30eaf5..53a57ff5f4a1 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1568,6 +1568,38 @@ static const struct drm_display_mode edid_4k_modes[] = {
 
 /*** DDC fetch and block validation ***/
 
+static int edid_extension_block_count(const struct edid *edid)
+{
+	return edid->extensions;
+}
+
+static int edid_block_count(const struct edid *edid)
+{
+	return edid_extension_block_count(edid) + 1;
+}
+
+static int edid_size_by_blocks(int num_blocks)
+{
+	return num_blocks * EDID_LENGTH;
+}
+
+static int edid_size(const struct edid *edid)
+{
+	return edid_size_by_blocks(edid_block_count(edid));
+}
+
+static const void *edid_block_data(const struct edid *edid, int index)
+{
+	BUILD_BUG_ON(sizeof(*edid) != EDID_LENGTH);
+
+	return edid + index;
+}
+
+static const void *edid_extension_block_data(const struct edid *edid, int index)
+{
+	return edid_block_data(edid, index + 1);
+}
+
 static const u8 edid_header[] = {
 	0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00
 };
@@ -1654,8 +1686,8 @@ bool drm_edid_are_equal(const struct edid *edid1, const struct edid *edid2)
 		return false;
 
 	if (edid1) {
-		edid1_len = EDID_LENGTH * (1 + edid1->extensions);
-		edid2_len = EDID_LENGTH * (1 + edid2->extensions);
+		edid1_len = edid_size(edid1);
+		edid2_len = edid_size(edid2);
 
 		if (edid1_len != edid2_len)
 			return false;
@@ -1865,14 +1897,16 @@ EXPORT_SYMBOL(drm_edid_block_valid);
 bool drm_edid_is_valid(struct edid *edid)
 {
 	int i;
-	u8 *raw = (u8 *)edid;
 
 	if (!edid)
 		return false;
 
-	for (i = 0; i <= edid->extensions; i++)
-		if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true, NULL))
+	for (i = 0; i < edid_block_count(edid); i++) {
+		void *block = (void *)edid_block_data(edid, i);
+
+		if (!drm_edid_block_valid(block, i, true, NULL))
 			return false;
+	}
 
 	return true;
 }
@@ -1885,13 +1919,13 @@ static struct edid *edid_filter_invalid_blocks(const struct edid *edid,
 	int valid_extensions = edid->extensions - invalid_blocks;
 	int i;
 
-	new = kmalloc_array(valid_extensions + 1, EDID_LENGTH, GFP_KERNEL);
+	new = kmalloc(edid_size_by_blocks(valid_extensions + 1), GFP_KERNEL);
 	if (!new)
 		goto out;
 
 	dest_block = new;
-	for (i = 0; i <= edid->extensions; i++) {
-		const void *block = edid + i;
+	for (i = 0; i < edid_block_count(edid); i++) {
+		const void *block = edid_block_data(edid, i);
 
 		if (edid_block_valid(block, i == 0))
 			memcpy(dest_block++, block, EDID_LENGTH);
@@ -2101,7 +2135,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 			     void *context)
 {
 	enum edid_block_status status;
-	int j, invalid_blocks = 0;
+	int i, invalid_blocks = 0;
 	struct edid *edid, *new;
 
 	edid = drm_get_override_edid(connector);
@@ -2133,18 +2167,18 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 		goto fail;
 	}
 
-	if (edid->extensions == 0)
+	if (!edid_extension_block_count(edid) == 0)
 		goto ok;
 
-	new = krealloc(edid, (edid->extensions + 1) * EDID_LENGTH, GFP_KERNEL);
+	new = krealloc(edid, edid_size(edid), GFP_KERNEL);
 	if (!new)
 		goto fail;
 	edid = new;
 
-	for (j = 1; j <= edid->extensions; j++) {
-		void *block = edid + j;
+	for (i = 1; i < edid_block_count(edid); i++) {
+		void *block = (void *)edid_block_data(edid, i);
 
-		status = edid_block_read(block, j, read_block, context);
+		status = edid_block_read(block, i, read_block, context);
 
 		edid_block_status_print(status, block, j);
 
@@ -2156,7 +2190,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 	}
 
 	if (invalid_blocks) {
-		connector_bad_edid(connector, edid, edid->extensions + 1);
+		connector_bad_edid(connector, edid, edid_block_count(edid));
 
 		edid = edid_filter_invalid_blocks(edid, invalid_blocks);
 	}
@@ -2321,7 +2355,7 @@ EXPORT_SYMBOL(drm_get_edid_switcheroo);
  */
 struct edid *drm_edid_duplicate(const struct edid *edid)
 {
-	return kmemdup(edid, (edid->extensions + 1) * EDID_LENGTH, GFP_KERNEL);
+	return kmemdup(edid, edid_size(edid), GFP_KERNEL);
 }
 EXPORT_SYMBOL(drm_edid_duplicate);
 
@@ -2505,8 +2539,8 @@ drm_for_each_detailed_block(const struct edid *edid, detailed_cb *cb, void *clos
 	for (i = 0; i < EDID_DETAILED_TIMINGS; i++)
 		cb(&(edid->detailed_timings[i]), closure);
 
-	for (i = 1; i <= edid->extensions; i++) {
-		const u8 *ext = (const u8 *)edid + (i * EDID_LENGTH);
+	for (i = 0; i < edid_extension_block_count(edid); i++) {
+		const u8 *ext = edid_extension_block_data(edid, i);
 
 		switch (*ext) {
 		case CEA_EXT:
@@ -3476,17 +3510,17 @@ const u8 *drm_find_edid_extension(const struct edid *edid,
 	int i;
 
 	/* No EDID or EDID extensions */
-	if (edid == NULL || edid->extensions == 0)
+	if (!edid || !edid_extension_block_count(edid))
 		return NULL;
 
 	/* Find CEA extension */
-	for (i = *ext_index; i < edid->extensions; i++) {
-		edid_ext = (const u8 *)edid + EDID_LENGTH * (i + 1);
+	for (i = *ext_index; i < edid_extension_block_count(edid); i++) {
+		edid_ext = edid_extension_block_data(edid, i);
 		if (edid_block_tag(edid_ext) == ext_id)
 			break;
 	}
 
-	if (i >= edid->extensions)
+	if (i >= edid_extension_block_count(edid))
 		return NULL;
 
 	*ext_index = i + 1;
-- 
2.30.2

Re: [PATCH 03/12] drm/edid: refactor EDID block status printing

[Ville Syrjälä]

On Thu, Apr 07, 2022 at 12:14:29PM +0300, Jani Nikula wrote:
> Split out a function to log EDID block status. The printouts get changed
> slightly.
> 
> Unfortunately, not all users will have struct drm_device available, so
> we convert to pr_* debug logging instead of drm device based logging.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 75 ++++++++++++++++++++++++++------------
>  1 file changed, 51 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index f062d1715ec3..3d04d63464ba 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1729,6 +1729,50 @@ static bool edid_block_valid(const void *block, bool base)
>  				       edid_block_tag(block));
>  }
>  
> +static void edid_block_status_print(enum edid_block_status status,
> +				    const struct edid *block,
> +				    int block_num)
> +{
> +	switch (status) {
> +	case EDID_BLOCK_OK:
> +		break;
> +	case EDID_BLOCK_NULL:
> +		pr_debug("EDID block %d pointer is NULL\n", block_num);
> +		break;
> +	case EDID_BLOCK_ZERO:
> +		pr_notice("EDID block %d is all zeroes\n", block_num);
> +		break;
> +	case EDID_BLOCK_HEADER_CORRUPT:
> +		pr_notice("EDID has corrupt header\n");
> +		break;
> +	case EDID_BLOCK_HEADER_REPAIR:
> +		pr_debug("EDID corrupt header needs repair\n");
> +		break;
> +	case EDID_BLOCK_HEADER_FIXED:
> +		pr_debug("EDID corrupt header fixed\n");
> +		break;
> +	case EDID_BLOCK_CHECKSUM:
> +		if (edid_block_status_valid(status, edid_block_tag(block))) {
> +			pr_debug("EDID block %d (tag 0x%02x) checksum is invalid, remainder is %d, ignoring\n",
> +				 block_num, edid_block_tag(block),
> +				 edid_block_compute_checksum(block));
> +		} else {
> +			pr_notice("EDID block %d (tag 0x%02x) checksum is invalid, remainder is %d\n",
> +				  block_num, edid_block_tag(block),
> +				  edid_block_compute_checksum(block));
> +		}
> +		break;
> +	case EDID_BLOCK_VERSION:
> +		pr_notice("EDID has major version %d, instead of 1\n",
> +			  block->version);
> +		break;
> +	default:
> +		pr_debug("EDID block %d unknown edid block status code %d\n",
> +			 block_num, status);

Maybe this should complaing a bit more loudly. Indicates a bug in the
code no?

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +		break;
> +	}
> +}
> +
>  /**
>   * drm_edid_block_valid - Sanity check the EDID block (base or extension)
>   * @raw_edid: pointer to raw EDID block
> @@ -1775,33 +1819,16 @@ bool drm_edid_block_valid(u8 *_block, int block_num, bool print_bad_edid,
>  			*edid_corrupt = true;
>  	}
>  
> +	edid_block_status_print(status, block, block_num);
> +
>  	/* Determine whether we can use this block with this status. */
>  	valid = edid_block_status_valid(status, edid_block_tag(block));
>  
> -	/* Some fairly random status printouts. */
> -	if (status == EDID_BLOCK_CHECKSUM) {
> -		if (valid) {
> -			DRM_DEBUG("EDID block checksum is invalid, remainder is %d\n",
> -				  edid_block_compute_checksum(block));
> -			DRM_DEBUG("Assuming a KVM switch modified the block but left the original checksum\n");
> -		} else if (print_bad_edid) {
> -			DRM_NOTE("EDID block checksum is invalid, remainder is %d\n",
> -				 edid_block_compute_checksum(block));
> -		}
> -	} else if (status == EDID_BLOCK_VERSION) {
> -		DRM_NOTE("EDID has major version %d, instead of 1\n",
> -			 block->version);
> -	}
> -
> -	if (!valid && print_bad_edid) {
> -		if (status == EDID_BLOCK_ZERO) {
> -			pr_notice("EDID block is all zeroes\n");
> -		} else {
> -			pr_notice("Raw EDID:\n");
> -			print_hex_dump(KERN_NOTICE,
> -				       " \t", DUMP_PREFIX_NONE, 16, 1,
> -				       block, EDID_LENGTH, false);
> -		}
> +	if (!valid && print_bad_edid && status != EDID_BLOCK_ZERO) {
> +		pr_notice("Raw EDID:\n");
> +		print_hex_dump(KERN_NOTICE,
> +			       " \t", DUMP_PREFIX_NONE, 16, 1,
> +			       block, EDID_LENGTH, false);
>  	}
>  
>  	return valid;
> -- 
> 2.30.2

-- 
Ville Syrjälä
Intel

Re: [PATCH 05/12] drm/edid: pass struct edid to connector_bad_edid()

[Ville Syrjälä]

On Thu, Apr 07, 2022 at 12:14:31PM +0300, Jani Nikula wrote:
> Avoid casting here and there, and make it const.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 8638e54e0879..ba54701f91f6 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1967,7 +1967,7 @@ drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned int block, size_t len)
>  }
>  
>  static void connector_bad_edid(struct drm_connector *connector,
> -			       u8 *edid, int num_blocks)
> +			       const struct edid *edid, int num_blocks)
>  {
>  	int i;
>  	u8 last_block;
> @@ -1978,22 +1978,19 @@ static void connector_bad_edid(struct drm_connector *connector,
>  	 * of 0x7e in the EDID of the _index_ of the last block in the
>  	 * combined chunk of memory.
>  	 */
> -	last_block = edid[0x7e];
> +	last_block = edid->extensions;
>  
>  	/* Calculate real checksum for the last edid extension block data */
>  	if (last_block < num_blocks)
>  		connector->real_edid_checksum =
> -			edid_block_compute_checksum(edid + last_block * EDID_LENGTH);
> +			edid_block_compute_checksum(edid + last_block);
>  
>  	if (connector->bad_edid_counter++ && !drm_debug_enabled(DRM_UT_KMS))
>  		return;
>  
>  	drm_dbg_kms(connector->dev, "%s: EDID is invalid:\n", connector->name);
> -	for (i = 0; i < num_blocks; i++) {
> -		u8 *block = edid + i * EDID_LENGTH;
> -
> -		edid_block_dump(KERN_DEBUG, block, i);
> -	}
> +	for (i = 0; i < num_blocks; i++)
> +		edid_block_dump(KERN_DEBUG, edid + i, i);

nit: I'm not a big fan of pointer arithmetic in general. IMO it
makes it a bit too easy to miss the fact that it's not counting
bytes but rather potentially something much bigger. So I tend to
prefer array notation for such things. But looks like most of 
these go away at the end anyway with the block_data() stuff.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  }
>  
>  /* Get override or firmware EDID */
> @@ -2139,7 +2136,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>  	}
>  
>  	if (invalid_blocks) {
> -		connector_bad_edid(connector, (u8 *)edid, edid->extensions + 1);
> +		connector_bad_edid(connector, edid, edid->extensions + 1);
>  
>  		edid = edid_filter_invalid_blocks(edid, invalid_blocks);
>  	}
> -- 
> 2.30.2

-- 
Ville Syrjälä
Intel

Re: [PATCH 06/12] drm/edid: add typedef for block read function

[Ville Syrjälä]

On Thu, Apr 07, 2022 at 12:14:32PM +0300, Jani Nikula wrote:
> Make the callback a bit easier on the eye.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index ba54701f91f6..926ffe5cd97e 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2037,10 +2037,11 @@ int drm_add_override_edid_modes(struct drm_connector *connector)
>  }
>  EXPORT_SYMBOL(drm_add_override_edid_modes);
>  
> +typedef int read_block_fn(void *context, u8 *buf, unsigned int block, size_t len);
> +

The header still has the eyesore. Maybe we want to put the typedef
there?

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  static struct edid *drm_do_get_edid_base_block(struct drm_connector *connector,
> -	int (*get_edid_block)(void *data, u8 *buf, unsigned int block,
> -			      size_t len),
> -	void *data)
> +					       read_block_fn read_block,
> +					       void *context)
>  {
>  	int *null_edid_counter = connector ? &connector->null_edid_counter : NULL;
>  	bool *edid_corrupt = connector ? &connector->edid_corrupt : NULL;
> @@ -2053,7 +2054,7 @@ static struct edid *drm_do_get_edid_base_block(struct drm_connector *connector,
>  
>  	/* base block fetch */
>  	for (try = 0; try < 4; try++) {
> -		if (get_edid_block(data, edid, 0, EDID_LENGTH))
> +		if (read_block(context, edid, 0, EDID_LENGTH))
>  			goto out;
>  		if (drm_edid_block_valid(edid, 0, false, edid_corrupt))
>  			break;
> @@ -2097,9 +2098,8 @@ static struct edid *drm_do_get_edid_base_block(struct drm_connector *connector,
>   * Return: Pointer to valid EDID or NULL if we couldn't find any.
>   */
>  struct edid *drm_do_get_edid(struct drm_connector *connector,
> -	int (*get_edid_block)(void *data, u8 *buf, unsigned int block,
> -			      size_t len),
> -	void *data)
> +			     read_block_fn read_block,
> +			     void *context)
>  {
>  	int j, invalid_blocks = 0;
>  	struct edid *edid, *new, *override;
> @@ -2108,7 +2108,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>  	if (override)
>  		return override;
>  
> -	edid = drm_do_get_edid_base_block(connector, get_edid_block, data);
> +	edid = drm_do_get_edid_base_block(connector, read_block, context);
>  	if (!edid)
>  		return NULL;
>  
> @@ -2125,7 +2125,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>  		int try;
>  
>  		for (try = 0; try < 4; try++) {
> -			if (get_edid_block(data, block, j, EDID_LENGTH))
> +			if (read_block(context, block, j, EDID_LENGTH))
>  				goto out;
>  			if (drm_edid_block_valid(block, j, false, NULL))
>  				break;
> -- 
> 2.30.2

-- 
Ville Syrjälä
Intel

Re: [PATCH 00/12] drm/edid: low level EDID block read refactoring etc.

[Ville Syrjälä]

On Thu, Apr 07, 2022 at 12:14:26PM +0300, Jani Nikula wrote:
> Ever so slowly moving towards cleaner EDID reading.
> 
> Jani Nikula (12):
>   drm/edid: convert edid_is_zero() to edid_block_is_zero() for blocks
>   drm/edid: have edid_block_check() detect blocks that are all zero
>   drm/edid: refactor EDID block status printing
>   drm/edid: add a helper to log dump an EDID block
>   drm/edid: pass struct edid to connector_bad_edid()
>   drm/edid: add typedef for block read function
>   drm/edid: abstract an EDID block read helper
>   drm/edid: use EDID block read helper in drm_do_get_edid()
>   drm/edid: convert extension block read to EDID block read helper
>   drm/edid: drop extra local var
>   drm/edid: add single point of return to drm_do_get_edid()
>   drm/edid: add EDID block count and size helpers

Some of the stuff was a bit hard to follow, but I suppose that's mostly
an indication of the messy state of the current code. Didn't spot
anything obviously wrong at least.

Series is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
>  drivers/gpu/drm/drm_edid.c | 350 ++++++++++++++++++++++++-------------
>  1 file changed, 225 insertions(+), 125 deletions(-)
> 
> -- 
> 2.30.2

-- 
Ville Syrjälä
Intel

[PATCH v2] drm/edid: add EDID block count and size helpers

[Jani Nikula]

Add some helpers to figure out the EDID extension block count, block
count, size, pointers to blocks.

Unfortunately, we'll need to cast away the const in a few places where
we actually need to access the data.

v2: fix s/j/i/ introduced in a rebase

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 80 +++++++++++++++++++++++++++-----------
 1 file changed, 57 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 90615e30eaf5..c09ff1efdbc8 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1568,6 +1568,38 @@ static const struct drm_display_mode edid_4k_modes[] = {
 
 /*** DDC fetch and block validation ***/
 
+static int edid_extension_block_count(const struct edid *edid)
+{
+	return edid->extensions;
+}
+
+static int edid_block_count(const struct edid *edid)
+{
+	return edid_extension_block_count(edid) + 1;
+}
+
+static int edid_size_by_blocks(int num_blocks)
+{
+	return num_blocks * EDID_LENGTH;
+}
+
+static int edid_size(const struct edid *edid)
+{
+	return edid_size_by_blocks(edid_block_count(edid));
+}
+
+static const void *edid_block_data(const struct edid *edid, int index)
+{
+	BUILD_BUG_ON(sizeof(*edid) != EDID_LENGTH);
+
+	return edid + index;
+}
+
+static const void *edid_extension_block_data(const struct edid *edid, int index)
+{
+	return edid_block_data(edid, index + 1);
+}
+
 static const u8 edid_header[] = {
 	0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00
 };
@@ -1654,8 +1686,8 @@ bool drm_edid_are_equal(const struct edid *edid1, const struct edid *edid2)
 		return false;
 
 	if (edid1) {
-		edid1_len = EDID_LENGTH * (1 + edid1->extensions);
-		edid2_len = EDID_LENGTH * (1 + edid2->extensions);
+		edid1_len = edid_size(edid1);
+		edid2_len = edid_size(edid2);
 
 		if (edid1_len != edid2_len)
 			return false;
@@ -1865,14 +1897,16 @@ EXPORT_SYMBOL(drm_edid_block_valid);
 bool drm_edid_is_valid(struct edid *edid)
 {
 	int i;
-	u8 *raw = (u8 *)edid;
 
 	if (!edid)
 		return false;
 
-	for (i = 0; i <= edid->extensions; i++)
-		if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true, NULL))
+	for (i = 0; i < edid_block_count(edid); i++) {
+		void *block = (void *)edid_block_data(edid, i);
+
+		if (!drm_edid_block_valid(block, i, true, NULL))
 			return false;
+	}
 
 	return true;
 }
@@ -1885,13 +1919,13 @@ static struct edid *edid_filter_invalid_blocks(const struct edid *edid,
 	int valid_extensions = edid->extensions - invalid_blocks;
 	int i;
 
-	new = kmalloc_array(valid_extensions + 1, EDID_LENGTH, GFP_KERNEL);
+	new = kmalloc(edid_size_by_blocks(valid_extensions + 1), GFP_KERNEL);
 	if (!new)
 		goto out;
 
 	dest_block = new;
-	for (i = 0; i <= edid->extensions; i++) {
-		const void *block = edid + i;
+	for (i = 0; i < edid_block_count(edid); i++) {
+		const void *block = edid_block_data(edid, i);
 
 		if (edid_block_valid(block, i == 0))
 			memcpy(dest_block++, block, EDID_LENGTH);
@@ -2101,7 +2135,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 			     void *context)
 {
 	enum edid_block_status status;
-	int j, invalid_blocks = 0;
+	int i, invalid_blocks = 0;
 	struct edid *edid, *new;
 
 	edid = drm_get_override_edid(connector);
@@ -2133,20 +2167,20 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 		goto fail;
 	}
 
-	if (edid->extensions == 0)
+	if (!edid_extension_block_count(edid) == 0)
 		goto ok;
 
-	new = krealloc(edid, (edid->extensions + 1) * EDID_LENGTH, GFP_KERNEL);
+	new = krealloc(edid, edid_size(edid), GFP_KERNEL);
 	if (!new)
 		goto fail;
 	edid = new;
 
-	for (j = 1; j <= edid->extensions; j++) {
-		void *block = edid + j;
+	for (i = 1; i < edid_block_count(edid); i++) {
+		void *block = (void *)edid_block_data(edid, i);
 
-		status = edid_block_read(block, j, read_block, context);
+		status = edid_block_read(block, i, read_block, context);
 
-		edid_block_status_print(status, block, j);
+		edid_block_status_print(status, block, i);
 
 		if (!edid_block_status_valid(status, edid_block_tag(block))) {
 			if (status == EDID_BLOCK_READ_FAIL)
@@ -2156,7 +2190,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 	}
 
 	if (invalid_blocks) {
-		connector_bad_edid(connector, edid, edid->extensions + 1);
+		connector_bad_edid(connector, edid, edid_block_count(edid));
 
 		edid = edid_filter_invalid_blocks(edid, invalid_blocks);
 	}
@@ -2321,7 +2355,7 @@ EXPORT_SYMBOL(drm_get_edid_switcheroo);
  */
 struct edid *drm_edid_duplicate(const struct edid *edid)
 {
-	return kmemdup(edid, (edid->extensions + 1) * EDID_LENGTH, GFP_KERNEL);
+	return kmemdup(edid, edid_size(edid), GFP_KERNEL);
 }
 EXPORT_SYMBOL(drm_edid_duplicate);
 
@@ -2505,8 +2539,8 @@ drm_for_each_detailed_block(const struct edid *edid, detailed_cb *cb, void *clos
 	for (i = 0; i < EDID_DETAILED_TIMINGS; i++)
 		cb(&(edid->detailed_timings[i]), closure);
 
-	for (i = 1; i <= edid->extensions; i++) {
-		const u8 *ext = (const u8 *)edid + (i * EDID_LENGTH);
+	for (i = 0; i < edid_extension_block_count(edid); i++) {
+		const u8 *ext = edid_extension_block_data(edid, i);
 
 		switch (*ext) {
 		case CEA_EXT:
@@ -3476,17 +3510,17 @@ const u8 *drm_find_edid_extension(const struct edid *edid,
 	int i;
 
 	/* No EDID or EDID extensions */
-	if (edid == NULL || edid->extensions == 0)
+	if (!edid || !edid_extension_block_count(edid))
 		return NULL;
 
 	/* Find CEA extension */
-	for (i = *ext_index; i < edid->extensions; i++) {
-		edid_ext = (const u8 *)edid + EDID_LENGTH * (i + 1);
+	for (i = *ext_index; i < edid_extension_block_count(edid); i++) {
+		edid_ext = edid_extension_block_data(edid, i);
 		if (edid_block_tag(edid_ext) == ext_id)
 			break;
 	}
 
-	if (i >= edid->extensions)
+	if (i >= edid_extension_block_count(edid))
 		return NULL;
 
 	*ext_index = i + 1;
-- 
2.30.2

Re: [PATCH 06/12] drm/edid: add typedef for block read function

[Jani Nikula]

On Thu, 07 Apr 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Apr 07, 2022 at 12:14:32PM +0300, Jani Nikula wrote:
>> Make the callback a bit easier on the eye.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/drm_edid.c | 18 +++++++++---------
>>  1 file changed, 9 insertions(+), 9 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index ba54701f91f6..926ffe5cd97e 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -2037,10 +2037,11 @@ int drm_add_override_edid_modes(struct drm_connector *connector)
>>  }
>>  EXPORT_SYMBOL(drm_add_override_edid_modes);
>>  
>> +typedef int read_block_fn(void *context, u8 *buf, unsigned int block, size_t len);
>> +
>
> The header still has the eyesore. Maybe we want to put the typedef
> there?

Decided to leave that for the future, possibly with a s/u8*/void*/
change.

Thanks,
Jani.


>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>>  static struct edid *drm_do_get_edid_base_block(struct drm_connector *connector,
>> -	int (*get_edid_block)(void *data, u8 *buf, unsigned int block,
>> -			      size_t len),
>> -	void *data)
>> +					       read_block_fn read_block,
>> +					       void *context)
>>  {
>>  	int *null_edid_counter = connector ? &connector->null_edid_counter : NULL;
>>  	bool *edid_corrupt = connector ? &connector->edid_corrupt : NULL;
>> @@ -2053,7 +2054,7 @@ static struct edid *drm_do_get_edid_base_block(struct drm_connector *connector,
>>  
>>  	/* base block fetch */
>>  	for (try = 0; try < 4; try++) {
>> -		if (get_edid_block(data, edid, 0, EDID_LENGTH))
>> +		if (read_block(context, edid, 0, EDID_LENGTH))
>>  			goto out;
>>  		if (drm_edid_block_valid(edid, 0, false, edid_corrupt))
>>  			break;
>> @@ -2097,9 +2098,8 @@ static struct edid *drm_do_get_edid_base_block(struct drm_connector *connector,
>>   * Return: Pointer to valid EDID or NULL if we couldn't find any.
>>   */
>>  struct edid *drm_do_get_edid(struct drm_connector *connector,
>> -	int (*get_edid_block)(void *data, u8 *buf, unsigned int block,
>> -			      size_t len),
>> -	void *data)
>> +			     read_block_fn read_block,
>> +			     void *context)
>>  {
>>  	int j, invalid_blocks = 0;
>>  	struct edid *edid, *new, *override;
>> @@ -2108,7 +2108,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>>  	if (override)
>>  		return override;
>>  
>> -	edid = drm_do_get_edid_base_block(connector, get_edid_block, data);
>> +	edid = drm_do_get_edid_base_block(connector, read_block, context);
>>  	if (!edid)
>>  		return NULL;
>>  
>> @@ -2125,7 +2125,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>>  		int try;
>>  
>>  		for (try = 0; try < 4; try++) {
>> -			if (get_edid_block(data, block, j, EDID_LENGTH))
>> +			if (read_block(context, block, j, EDID_LENGTH))
>>  				goto out;
>>  			if (drm_edid_block_valid(block, j, false, NULL))
>>  				break;
>> -- 
>> 2.30.2

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH 03/12] drm/edid: refactor EDID block status printing

[Jani Nikula]

On Thu, 07 Apr 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Apr 07, 2022 at 12:14:29PM +0300, Jani Nikula wrote:
>> Split out a function to log EDID block status. The printouts get changed
>> slightly.
>> 
>> Unfortunately, not all users will have struct drm_device available, so
>> we convert to pr_* debug logging instead of drm device based logging.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/drm_edid.c | 75 ++++++++++++++++++++++++++------------
>>  1 file changed, 51 insertions(+), 24 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index f062d1715ec3..3d04d63464ba 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -1729,6 +1729,50 @@ static bool edid_block_valid(const void *block, bool base)
>>  				       edid_block_tag(block));
>>  }
>>  
>> +static void edid_block_status_print(enum edid_block_status status,
>> +				    const struct edid *block,
>> +				    int block_num)
>> +{
>> +	switch (status) {
>> +	case EDID_BLOCK_OK:
>> +		break;
>> +	case EDID_BLOCK_NULL:
>> +		pr_debug("EDID block %d pointer is NULL\n", block_num);
>> +		break;
>> +	case EDID_BLOCK_ZERO:
>> +		pr_notice("EDID block %d is all zeroes\n", block_num);
>> +		break;
>> +	case EDID_BLOCK_HEADER_CORRUPT:
>> +		pr_notice("EDID has corrupt header\n");
>> +		break;
>> +	case EDID_BLOCK_HEADER_REPAIR:
>> +		pr_debug("EDID corrupt header needs repair\n");
>> +		break;
>> +	case EDID_BLOCK_HEADER_FIXED:
>> +		pr_debug("EDID corrupt header fixed\n");
>> +		break;
>> +	case EDID_BLOCK_CHECKSUM:
>> +		if (edid_block_status_valid(status, edid_block_tag(block))) {
>> +			pr_debug("EDID block %d (tag 0x%02x) checksum is invalid, remainder is %d, ignoring\n",
>> +				 block_num, edid_block_tag(block),
>> +				 edid_block_compute_checksum(block));
>> +		} else {
>> +			pr_notice("EDID block %d (tag 0x%02x) checksum is invalid, remainder is %d\n",
>> +				  block_num, edid_block_tag(block),
>> +				  edid_block_compute_checksum(block));
>> +		}
>> +		break;
>> +	case EDID_BLOCK_VERSION:
>> +		pr_notice("EDID has major version %d, instead of 1\n",
>> +			  block->version);
>> +		break;
>> +	default:
>> +		pr_debug("EDID block %d unknown edid block status code %d\n",
>> +			 block_num, status);
>
> Maybe this should complaing a bit more loudly. Indicates a bug in the
> code no?

Sent v2 with WARN().

Thanks,
Jani


>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>> +		break;
>> +	}
>> +}
>> +
>>  /**
>>   * drm_edid_block_valid - Sanity check the EDID block (base or extension)
>>   * @raw_edid: pointer to raw EDID block
>> @@ -1775,33 +1819,16 @@ bool drm_edid_block_valid(u8 *_block, int block_num, bool print_bad_edid,
>>  			*edid_corrupt = true;
>>  	}
>>  
>> +	edid_block_status_print(status, block, block_num);
>> +
>>  	/* Determine whether we can use this block with this status. */
>>  	valid = edid_block_status_valid(status, edid_block_tag(block));
>>  
>> -	/* Some fairly random status printouts. */
>> -	if (status == EDID_BLOCK_CHECKSUM) {
>> -		if (valid) {
>> -			DRM_DEBUG("EDID block checksum is invalid, remainder is %d\n",
>> -				  edid_block_compute_checksum(block));
>> -			DRM_DEBUG("Assuming a KVM switch modified the block but left the original checksum\n");
>> -		} else if (print_bad_edid) {
>> -			DRM_NOTE("EDID block checksum is invalid, remainder is %d\n",
>> -				 edid_block_compute_checksum(block));
>> -		}
>> -	} else if (status == EDID_BLOCK_VERSION) {
>> -		DRM_NOTE("EDID has major version %d, instead of 1\n",
>> -			 block->version);
>> -	}
>> -
>> -	if (!valid && print_bad_edid) {
>> -		if (status == EDID_BLOCK_ZERO) {
>> -			pr_notice("EDID block is all zeroes\n");
>> -		} else {
>> -			pr_notice("Raw EDID:\n");
>> -			print_hex_dump(KERN_NOTICE,
>> -				       " \t", DUMP_PREFIX_NONE, 16, 1,
>> -				       block, EDID_LENGTH, false);
>> -		}
>> +	if (!valid && print_bad_edid && status != EDID_BLOCK_ZERO) {
>> +		pr_notice("Raw EDID:\n");
>> +		print_hex_dump(KERN_NOTICE,
>> +			       " \t", DUMP_PREFIX_NONE, 16, 1,
>> +			       block, EDID_LENGTH, false);
>>  	}
>>  
>>  	return valid;
>> -- 
>> 2.30.2

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH 00/12] drm/edid: low level EDID block read refactoring etc.

[Jani Nikula]

On Thu, 07 Apr 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Apr 07, 2022 at 12:14:26PM +0300, Jani Nikula wrote:
>> Ever so slowly moving towards cleaner EDID reading.
>> 
>> Jani Nikula (12):
>>   drm/edid: convert edid_is_zero() to edid_block_is_zero() for blocks
>>   drm/edid: have edid_block_check() detect blocks that are all zero
>>   drm/edid: refactor EDID block status printing
>>   drm/edid: add a helper to log dump an EDID block
>>   drm/edid: pass struct edid to connector_bad_edid()
>>   drm/edid: add typedef for block read function
>>   drm/edid: abstract an EDID block read helper
>>   drm/edid: use EDID block read helper in drm_do_get_edid()
>>   drm/edid: convert extension block read to EDID block read helper
>>   drm/edid: drop extra local var
>>   drm/edid: add single point of return to drm_do_get_edid()
>>   drm/edid: add EDID block count and size helpers
>
> Some of the stuff was a bit hard to follow, but I suppose that's mostly
> an indication of the messy state of the current code. Didn't spot
> anything obviously wrong at least.

I'm painfully aware. I'm just hoping the end result has more clarity. At
least highlights the warts in the logic that I tried to preserve...

> Series is
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Again, thanks a bunch!

BR,
Jani.


>
>> 
>>  drivers/gpu/drm/drm_edid.c | 350 ++++++++++++++++++++++++-------------
>>  1 file changed, 225 insertions(+), 125 deletions(-)
>> 
>> -- 
>> 2.30.2

-- 
Jani Nikula, Intel Open Source Graphics Center