[PATCH v4] DRM/KMS/EDID: Consolidate EDID Error Handling (v4)

[Egbert Eich <eich@suse.com>]

Consolidate the null_edid_counter and the bad_edid_counter
into EDID error state flags which for the last EDID read
are accessible from user.
Errors are looged it the same error has not been present
in the previous read of the EDID. This will reset the
EDID error status for example when the monitor is changed
but still prevents permanent EDID errors from piling up
the the kernel logs.

v2: Fixed conflits due to reordering of commits.
    Set error state where missing.
v3: Don't update cache when returning block from cache.
v4: Fixed drm_edid_is_valid() to return the actual error
    state.
    Included results of extension block checking in the
    overall error state.
    Removed drm_edid_block_valid() as there is noone
    using it.

Signed-off-by: Egbert Eich <eich@suse.com>
---
 drivers/gpu/drm/drm_edid.c                 |  136 ++++++++++++++++------------
 drivers/gpu/drm/radeon/radeon_atombios.c   |    2 +-
 drivers/gpu/drm/radeon/radeon_connectors.c |    2 +-
 include/drm/drm_crtc.h                     |    6 +-
 include/drm/drm_edid.h                     |   10 ++
 5 files changed, 94 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index dd0df60..6cedd46 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -157,6 +157,17 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
 }
 EXPORT_SYMBOL(drm_edid_header_is_valid);
 
+static bool drm_edid_is_zero(u8 *in_edid, int length)
+{
+	int i;
+	u32 *raw_edid = (u32 *)in_edid;
+
+	for (i = 0; i < length / 4; i++)
+		if (*(raw_edid + i) != 0)
+			return false;
+	return true;
+}
+
 static int edid_fixup __read_mostly = 6;
 module_param_named(edid_fixup, edid_fixup, int, 0400);
 MODULE_PARM_DESC(edid_fixup,
@@ -166,11 +177,13 @@ MODULE_PARM_DESC(edid_fixup,
  * Sanity check the EDID block (base or extension).  Return 0 if the block
  * doesn't check out, or 1 if it's valid.
  */
-bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
+unsigned
+drm_edid_block_check_error(u8 *raw_edid, int block, unsigned last_error_flags)
 {
 	int i;
 	u8 csum = 0;
 	struct edid *edid = (struct edid *)raw_edid;
+	unsigned result = 0;
 
 	if (edid_fixup > 8 || edid_fixup < 0)
 		edid_fixup = 6;
@@ -182,27 +195,33 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
 			DRM_DEBUG("Fixing EDID header, your hardware may be failing\n");
 			memcpy(raw_edid, edid_header, sizeof(edid_header));
 		} else {
-			goto bad;
+			result |= EDID_ERR_NO_BLOCK0;
+			if (drm_edid_is_zero(raw_edid, EDID_LENGTH)) {
+				result |= EDID_ERR_NULL;
+				goto bad;
+			}
 		}
 	}
 
 	for (i = 0; i < EDID_LENGTH; i++)
 		csum += raw_edid[i];
 	if (csum) {
-		if (print_bad_edid) {
+		if ((last_error_flags & EDID_ERR_CSUM) == 0)
 			DRM_ERROR("EDID checksum is invalid, remainder is %d\n", csum);
-		}
 
 		/* allow CEA to slide through, switches mangle this */
 		if (raw_edid[0] != 0x02)
-			goto bad;
+			result |= EDID_ERR_CSUM;
 	}
+	if (result)
+		goto bad;
 
 	/* per-block-type checks */
 	switch (raw_edid[0]) {
 	case 0: /* base */
 		if (edid->version != 1) {
 			DRM_ERROR("EDID has major version %d, instead of 1\n", edid->version);
+			result |= EDID_ERR_UNSUPPORTED_VERSION;
 			goto bad;
 		}
 
@@ -214,17 +233,16 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
 		break;
 	}
 
-	return 1;
+	return 0;
 
 bad:
-	if (raw_edid && print_bad_edid) {
+	if (raw_edid && last_error_flags != result) {
 		printk(KERN_ERR "Raw EDID:\n");
 		print_hex_dump(KERN_ERR, " \t", DUMP_PREFIX_NONE, 16, 1,
 			       raw_edid, EDID_LENGTH, false);
 	}
-	return 0;
+	return result;
 }
-EXPORT_SYMBOL(drm_edid_block_valid);
 
 /**
  * drm_edid_is_valid - sanity check EDID data
@@ -232,19 +250,21 @@ EXPORT_SYMBOL(drm_edid_block_valid);
  *
  * Sanity-check an entire EDID record (including extensions)
  */
-bool drm_edid_is_valid(struct edid *edid)
+unsigned drm_edid_is_valid(struct edid *edid, unsigned last_error_flags)
 {
 	int i;
+	unsigned result;
 	u8 *raw = (u8 *)edid;
 
 	if (!edid)
-		return false;
+		return EDID_ERR_NO_DATA;
 
-	for (i = 0; i <= edid->extensions; i++)
-		if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true))
-			return false;
-
-	return true;
+	for (i = 0; i <= edid->extensions; i++) {
+		result = drm_edid_block_check_error(raw + i * EDID_LENGTH, i, last_error_flags);
+		if (result)
+			return result;
+	}
+	return 0;
 }
 EXPORT_SYMBOL(drm_edid_is_valid);
 
@@ -310,17 +330,6 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
 	return ret == xfers ? 0 : -1;
 }
 
-static bool drm_edid_is_zero(u8 *in_edid, int length)
-{
-	int i;
-	u32 *raw_edid = (u32 *)in_edid;
-
-	for (i = 0; i < length / 4; i++)
-		if (*(raw_edid + i) != 0)
-			return false;
-	return true;
-}
-
 static void
 fix_map(u8 *block, int cnt)
 {
@@ -456,37 +465,40 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 {
 	int i, j = 0, valid_extensions = 0;
 	u8 *block, *new;
-	bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
+	int last_error_flags = (drm_debug & DRM_UT_KMS) ? 0 : connector->last_edid_error_flags;
+	unsigned result = EDID_ERR_NO_DATA;
 
 #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
 	/* check if the user has specified a 'firmware' EDID file */
 	block = (u8 *)drm_load_edid_firmware(connector);
 	if (block) {
 		drm_cache_edid(connector, NULL);
+		connector->last_edid_error_flags = 0;
 		return block;
 	}
 #endif
-
-	if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
+	block = kmalloc(EDID_LENGTH, GFP_KERNEL);
+	if (block == NULL) {
+		result = EDID_ERR_NO_MEM;
 		goto error;
+	}
 
 	/* base block fetch */
 	for (i = 0; i < 4; i++) {
 		if (drm_do_probe_ddc_edid(adapter, block, 0, EDID_LENGTH))
 			goto error_free;
-		if (drm_edid_block_valid(block, 0, print_bad_edid))
+		result = drm_edid_block_check_error(block, 0, last_error_flags);
+		if (!result)
 			break;
-		if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) {
-			connector->null_edid_counter++;
+		if (i == 0 && result & EDID_ERR_NULL)
 			goto error_carp;
-		}
 	}
-	if (i == 4)
+	if (result)
 		goto error_carp;
 
 	/* if there are no extensions, we're done - don't bother caching */
 	if (block[EDID_EXTENSION_FLAG_OFFSET] == 0)
-		goto done;
+		goto done_update_cache;
 
 	/* don't expect extension blocks in EDID Versions < 1.3: return base block with correct extension flag */
 	if (block[EDID_VERSION_MINOR_OFFSET] < 3)
@@ -494,7 +506,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 
 	/* see if EDID is in the cache - no need to read all extension blocks */
 	if (compare_get_edid_from_cache(connector, (struct edid **)&block))
-		return block;
+		goto done;
 
 	new = krealloc(block, (block[EDID_EXTENSION_FLAG_OFFSET] + 1) * EDID_LENGTH, GFP_KERNEL);
 	if (!new) {
@@ -506,13 +518,18 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 
 	for (j = 1; j <= block[EDID_EXTENSION_FLAG_OFFSET]; j++) {
 		for (i = 0; i < 4; i++) {
+			unsigned err;
 			if (drm_do_probe_ddc_edid(adapter,
 				  block + (valid_extensions + 1) * EDID_LENGTH,
 				  j, EDID_LENGTH)) {
 				valid_extensions = 0;
 				goto done_fix_extension_count;
 			}
-			if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) {
+			err = drm_edid_block_check_error(
+				block + (valid_extensions + 1) * EDID_LENGTH,
+				j, last_error_flags);
+			result |= err;
+			if (!err) {
 				valid_extensions++;
 				/* If extension block 2 is identical to the base block the display is probably
 				 * not EDDC cabable - despite of what the extension flag says - as it doesn't
@@ -535,22 +552,23 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 
 done_fix_extension_count:
 	fixup_edid(&block, valid_extensions);
-done:
+done_update_cache:
 	drm_cache_edid(connector, (valid_extensions > 0) ? (struct edid *)block : NULL);
+done:
+	connector->last_edid_error_flags = 0;
 
 	return block;
 
 error_carp:
-	if (print_bad_edid) {
+	if (last_error_flags != result) {
 		dev_warn(connector->dev->dev, "%s: EDID block %d invalid.\n",
 			 drm_get_connector_name(connector), j);
 	}
-	connector->bad_edid_counter++;
-
 error_free:
 	kfree(block);
 error:
 	drm_cache_edid(connector, NULL);
+	connector->last_edid_error_flags = result;
 	return NULL;
 }
 
@@ -567,26 +585,31 @@ int
 drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len)
 {
 	int n_blocks = len / EDID_LENGTH;
-	int valid_extensions = 0, ret = 0;
-	bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
+	int valid_extensions = 0, ret = -EINVAL;
+	int last_error_flags = (drm_debug & DRM_UT_KMS) ? 0 : connector->last_edid_error_flags;
+	unsigned result = EDID_ERR_NO_DATA;
 
-	if (!blockp || !*blockp)
-		ret = -EINVAL;
-	else if (!n_blocks || !drm_edid_block_valid(*blockp, 0, print_bad_edid)) {
-		kfree(*blockp);
-		*blockp = NULL;
-		ret = -EINVAL;
+	if (blockp && *blockp) {
+		if (n_blocks)
+			result = drm_edid_block_check_error(*blockp, 0, last_error_flags);
+		if (result) {
+			kfree(*blockp);
+			*blockp = NULL;
+		}
 	}
-	if (!ret) {
+	if (!result) {
 		int cnt = 0;
 		n_blocks--;
 		if ((*blockp)[EDID_EXTENSION_FLAG_OFFSET] < n_blocks)
 			n_blocks = (*blockp)[EDID_EXTENSION_FLAG_OFFSET];
 
 		while (n_blocks--) {
+			unsigned err;
 			cnt++;
-			if (drm_edid_block_valid(*blockp + cnt * EDID_LENGTH,
-						 valid_extensions + 1, print_bad_edid)) {
+			err = drm_edid_block_check_error(*blockp + cnt * EDID_LENGTH,
+							 valid_extensions + 1, last_error_flags);
+			result |= err;
+			if (!err) {
 				valid_extensions++;
 				if (cnt != valid_extensions)
 					memcpy(*blockp + valid_extensions * EDID_LENGTH,
@@ -594,8 +617,9 @@ drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len)
 			}
 		}
 		fixup_edid(blockp, valid_extensions);
-	} else
-		connector->bad_edid_counter++;
+		ret = 0;
+	}
+	connector->last_edid_error_flags = result;
 	return ret;
 }
 
@@ -2187,7 +2211,7 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
 	if (edid == NULL) {
 		return 0;
 	}
-	if (!drm_edid_is_valid(edid)) {
+	if (drm_edid_is_valid(edid, 0)) {
 		dev_warn(connector->dev->dev, "%s: EDID invalid.\n",
 			 drm_get_connector_name(connector));
 		return 0;
diff --git a/drivers/gpu/drm/radeon/radeon_atombios.c b/drivers/gpu/drm/radeon/radeon_atombios.c
index f22eb57..2de0d5b 100644
--- a/drivers/gpu/drm/radeon/radeon_atombios.c
+++ b/drivers/gpu/drm/radeon/radeon_atombios.c
@@ -1644,7 +1644,7 @@ struct radeon_encoder_atom_dig *radeon_atombios_get_lvds_info(struct
 							memcpy((u8 *)edid, (u8 *)&fake_edid_record->ucFakeEDIDString[0],
 							       fake_edid_record->ucFakeEDIDLength);
 
-							if (drm_edid_is_valid(edid)) {
+							if (drm_edid_is_valid(edid,0) == 0) {
 								rdev->mode_info.bios_hardcoded_edid = edid;
 								rdev->mode_info.bios_hardcoded_edid_size = edid_size;
 							} else
diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
index b884c36..e80ba63 100644
--- a/drivers/gpu/drm/radeon/radeon_connectors.c
+++ b/drivers/gpu/drm/radeon/radeon_connectors.c
@@ -961,7 +961,7 @@ radeon_dvi_detect(struct drm_connector *connector, bool force)
 					drm_get_connector_name(connector));
 			/* rs690 seems to have a problem with connectors not existing and always
 			 * return a block of 0's. If we see this just stop polling on this output */
-			if ((rdev->family == CHIP_RS690 || rdev->family == CHIP_RS740) && radeon_connector->base.null_edid_counter) {
+			if ((rdev->family == CHIP_RS690 || rdev->family == CHIP_RS740) && radeon_connector->base.last_edid_error_flags & EDID_ERR_NULL) {
 				ret = connector_status_disconnected;
 				DRM_ERROR("%s: detected RS690 floating bus bug, stopping ddc detect\n", drm_get_connector_name(connector));
 				radeon_connector->ddc_bus = NULL;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 6a1054c..7a3ccbf 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -608,8 +608,7 @@ struct drm_connector {
 	bool latency_present[2];
 	int video_latency[2];	/* [0]: progressive, [1]: interlaced */
 	int audio_latency[2];
-	int null_edid_counter; /* needed to workaround some HW bugs where we get all 0s */
-	unsigned bad_edid_counter;
+	unsigned last_edid_error_flags;
 };
 
 /**
@@ -1056,8 +1055,7 @@ extern int drm_add_modes_noedid(struct drm_connector *connector,
 				int hdisplay, int vdisplay);
 
 extern int drm_edid_header_is_valid(const u8 *raw_edid);
-extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid);
-extern bool drm_edid_is_valid(struct edid *edid);
+extern unsigned drm_edid_is_valid(struct edid *edid, unsigned last_error_flags);
 struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
 					   int hsize, int vsize, int fresh,
 					   bool rb);
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index c880510..6bcaee5 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -244,6 +244,15 @@ struct edid {
 
 #define EDID_PRODUCT_ID(e) ((e)->prod_code[0] | ((e)->prod_code[1] << 8))
 
+enum edid_error {
+	EDID_ERR_NO_BLOCK0 = 1 << 0,
+	EDID_ERR_NULL      = 1 << 1,
+	EDID_ERR_CSUM      = 1 << 2,
+	EDID_ERR_UNSUPPORTED_VERSION  = 1 << 3,
+	EDID_ERR_NO_DATA   = 1 << 4,
+	EDID_ERR_NO_MEM    = 1 << 5,
+};
+
 struct drm_encoder;
 struct drm_connector;
 struct drm_display_mode;
@@ -257,5 +266,6 @@ struct edid *drm_load_edid_firmware(struct drm_connector *connector);
 #endif
 int drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len);
 void drm_cache_edid(struct drm_connector *connector, struct edid *edid);
+unsigned drm_edid_block_check_error(u8 *raw_edid, int block, unsigned last_error_flags);
 
 #endif /* __DRM_EDID_H__ */
-- 
1.7.7