[PATCH 00/17] DRM/KMS/EDID: Various EDID handling related fixes.

[PATCH 00/17] DRM/KMS/EDID: Various EDID handling related fixes.

[Egbert Eich]

This patchset fixes issues about EDID and EDID Extension block handling:

1. We currently don't return any EDID information if there are issues with
   reading extension blocks. There are devices however which announce to
   have EDID extension blocks although these are not readable.
   If reading extension blocks fails we should at least return the base
   block (with a corrected extension blcok flag).
2. There are broken legacy DDC devices around which wrongly announce to have ED
   extension blocks. Since the segment address is ignored on these devices
   we will get a base block returned for any uneven block number. Detect this
   situation and abort reading further blocks.
3. When we read EDID extension blocks but fail to validate them we discard
   them and correct the EDID block count in the base block. There may however
   be a block map in the first extesion block which we should also fix.
   Similarly, don't fail if we cannot allocate enough contiguous memory for
   all EDID extension blocks: rather than failing completely, it is better
   to return at least the base block and if possible all extension blocks
   for which there is memory.
4. EDID 'firmware'-files:
   * Treat the, the same way as EDIDs read from DDC channels:
    - use the same routines to validate and fix them.
    - feed them when reading
   * Support different EDID files for different connectors.
5. Consolidate EDID error handling:
   Store information about the error status of the last DDC read in the
   drm_connector structure. This data can be used:
   - in the driver to detect certain issues with DDC read out.
   - provide a finer control over repeated error logging - instead
     of disabling EDID error logging entirely if an EDID error has
     been detected, only disable it for those errors which have been
     seen also on the previous DDC read cycle.
6. Move EDID-related functions to drm_edid.h
   The handling of EDID related DRM functions seemed to be a bit arbitrary:
   although drm_edid.h had been created, new functions were often added to
   drm_crtc.h


Egbert Eich (17):
  DRM/KMS/EDID: Mask out Segment Bits when calculating Offset.
  DRM/KMS/EDID: 0x7e -> EDID_EXTENSION_FLAG_OFFSET
  DRM/KMS/EDID: Return Base EDID block if reading EEDID Blocks fails.
  DRM/KMS/EDID: Test EDDC if EDID announces more than one Extension
    Block.
  DRM/KMS/EDID: Fix up EEDID Map Blocks if Extension block count has
    changed.
  DRM/exynos: Fix Memory Leak: free EDID blcok returned by
    drm_get_edid().
  DRM/KMS/EDID: Move drm_edid_load.o to drm.ko
  DRM/KMS/EDID: Use Extension Block Fixup Code also for 'firmware'
    EDID.
  DRM/KMS/EDID: Feed 'firmware' supplied EDID blocks whenever the EDID
    is read.
  DRM/KMS/EDID: Cache EDID blobs with extensions.
  DRM/KMS/EDID: Avoid failing on krealloc on EDID blocks.
  DRM/KMS/EDID: Consolidate EDID Error Handling.
  DRM/KMS/EDID: Allow for multiple Connectors when specifying
    'firmware'-EDID Files.
  DRM/KMS/ast: Include drm_edid.h in file using drm_get_edid().
  DRM/KMS/gma500: Include drm_edid.h in file using drm_get_edid().
  DRM/KMS/mgag200: Include drm_edid.h in file using drm_get_edid().
  DRM/KMS/EDID: Move EDID related Functions to drm_edid.h.

 drivers/gpu/drm/Makefile                   |    2 +-
 drivers/gpu/drm/ast/ast_mode.c             |    1 +
 drivers/gpu/drm/drm_crtc.c                 |    1 +
 drivers/gpu/drm/drm_crtc_helper.c          |    6 +-
 drivers/gpu/drm/drm_edid.c                 |  303 ++++++++++++++++++++++++----
 drivers/gpu/drm/drm_edid_load.c            |  115 +++++------
 drivers/gpu/drm/exynos/exynos_hdmi.c       |    1 +
 drivers/gpu/drm/gma500/cdv_intel_dp.c      |    1 +
 drivers/gpu/drm/gma500/oaktrail_lvds.c     |    1 +
 drivers/gpu/drm/gma500/psb_intel_modes.c   |    1 +
 drivers/gpu/drm/mgag200/mgag200_mode.c     |    1 +
 drivers/gpu/drm/radeon/radeon_connectors.c |    2 +-
 include/drm/drm_crtc.h                     |   13 +-
 include/drm/drm_edid.h                     |   24 ++-
 14 files changed, 348 insertions(+), 124 deletions(-)

-- 
1.7.7

[PATCH 01/17] DRM/KMS/EDID: Mask out Segment Bits when calculating Offset.

[Egbert Eich]

This patch is a bit cosmetic as the variable size will truncate
the start address anyway but for readability it should be made
explicite that the lowest bit in the EDID block number determines
the I2C start address.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/drm_edid.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index fadcd44..049fa52 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -258,7 +258,7 @@ static int
 drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
 		      int block, int len)
 {
-	unsigned char start = block * EDID_LENGTH;
+	unsigned char start = (block & 0x01) * EDID_LENGTH;
 	unsigned char segment = block >> 1;
 	unsigned char xfers = segment ? 3 : 2;
 	int ret, retries = 5;
-- 
1.7.7

[PATCH 03/17] DRM/KMS/EDID: Return Base EDID block if reading EEDID Blocks fails.

[Egbert Eich]

If I2C readout fails for an extension block but we have read a
valid base block, don't fail completely but at least return the
base block.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/drm_edid.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index f90d968..b258646 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -355,8 +355,10 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 		for (i = 0; i < 4; i++) {
 			if (drm_do_probe_ddc_edid(adapter,
 				  block + (valid_extensions + 1) * EDID_LENGTH,
-				  j, EDID_LENGTH))
-				goto out;
+				  j, EDID_LENGTH)) {
+				valid_extensions = 0;
+				goto no_more;
+			}
 			if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) {
 				valid_extensions++;
 				break;
@@ -368,6 +370,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 			 drm_get_connector_name(connector), j);
 	}
 
+ no_more:
 	if (valid_extensions != block[EDID_EXTENSION_FLAG_OFFSET]) {
 		block[EDID_CHECKSUM_OFFSET] += block[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions;
 		block[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions;
-- 
1.7.7

[PATCH 04/17] DRM/KMS/EDID: Test EDDC if EDID announces more than one Extension Block.

[Egbert Eich]

There are displays which announce EDID extension blocks in the
Extension Flag of the EDID base block although they are not EDDC
capable (ie. take a segment address at I2C slave address 0x30).
We test this by looking for an EDID header which is only possible
in the base block.
If the segment address is not taken into account, this block will
be identical to the base block in which case we stop reading further
EEDID blocks, correct the extension flag and just return the base
block.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/drm_edid.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index b258646..ec6f3bb 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -361,6 +361,19 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 			}
 			if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) {
 				valid_extensions++;
+				/* Test if base block announced extension blocks although
+				 * display is not EDDC capable.
+				 */
+				if (j == 2) {
+					int k;
+					for (k = 0; k < sizeof(edid_header); k++)
+						if (block[(EDID_LENGTH * 2) + k] != edid_header[k])
+							break;
+					if (k == sizeof(edid_header)) {
+						valid_extensions = 0;
+						goto no_more;
+					}
+				}
 				break;
 			}
 		}
-- 
1.7.7

[PATCH 05/17] DRM/KMS/EDID: Fix up EEDID Map Blocks if Extension block count has changed.

[Egbert Eich]

EEDID v1.3 mandates map blocks if more than one EDID extension block
is used while in v1.4 they are optional.
If the extension count has been changed (because some extension
blocks were not readable) those map blocks need fixing.
In case of v1.4 or higher we simply eliminate all map blocks as
this is less time consuming. For v1.3 we scrap any exsisting map
blocks and recreate them from scratch.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/drm_edid.c |  105 +++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 100 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index ec6f3bb..d1b9d67 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -44,7 +44,11 @@
 #define EDID_DETAILED_TIMINGS 4
 
 #define EDID_EXTENSION_FLAG_OFFSET  ((size_t)&((struct edid *)0)->extensions)
-#define EDID_CHECKSUM_OFFSET  ((size_t)&((struct edid *)0)->checksum)
+#define EDID_CHECKSUM_OFFSET	    ((size_t)&((struct edid *)0)->checksum)
+#define EDID_VERSION_MAJOR_OFFSET   ((size_t)&((struct edid *)0)->version)
+#define EDID_VERSION_MINOR_OFFSET   ((size_t)&((struct edid *)0)->revision)
+#define EEDID_BLOCK_MAP_FLAG      0xF0
+
 /*
  * EDID blocks out in the wild have a variety of bugs, try to collect
  * them here (note that userspace may work around broken monitors first,
@@ -318,6 +322,92 @@ static bool drm_edid_is_zero(u8 *in_edid, int length)
 	return true;
 }
 
+static void
+fix_map(u8 *block, int cnt)
+{
+	int i;
+	u8 csum;
+
+       if (--cnt > 127)
+	       cnt = 127;
+       memset(block, 0, 128);
+       block[0] = EEDID_BLOCK_MAP_FLAG;
+       csum = block[0];
+
+       for (i = 1; i < cnt; i++) {
+	       block[i] = block[i * EDID_LENGTH];
+	       csum += block[i];
+       }
+       block[127] = (u8)(0x100 - csum);
+}
+
+static int
+fixup_blockmaps(u8 **blockp, int eblock_cnt)
+{
+	int i;
+	u8 *block = *blockp;
+
+	if (block[EDID_VERSION_MAJOR_OFFSET] > 1)
+		return 0;
+	if (block[EDID_VERSION_MINOR_OFFSET] < 3)
+		return 0;
+	if (eblock_cnt == 1) {
+		if (block[EDID_LENGTH] == EEDID_BLOCK_MAP_FLAG)
+			return 0;
+		else
+			return 1;
+	}
+	if (block[EDID_VERSION_MINOR_OFFSET] >= 4) {
+		/* blockmaps are optional: simply toss them */
+		for (i = 1; i <= eblock_cnt; i++) {
+			if (block[i * EDID_LENGTH] == EEDID_BLOCK_MAP_FLAG) {
+				if (i < eblock_cnt)
+					memmove(&block[i * EDID_LENGTH],
+						&block[(i + 1) * EDID_LENGTH],
+						(eblock_cnt-i) * EDID_LENGTH);
+				i--;
+				eblock_cnt--;
+			}
+		}
+	} else {
+		int total_cnt = block[EDID_EXTENSION_FLAG_OFFSET];
+		for (i = 1; i <= eblock_cnt; i++) {
+			if (block[i * EDID_LENGTH] == EEDID_BLOCK_MAP_FLAG) {
+				if (i == 1 || i == 128) /* correct map block locations */
+					continue;
+				if (i < eblock_cnt)
+					memmove(&block[i * EDID_LENGTH],
+						&block[(i + 1) * EDID_LENGTH],
+						(eblock_cnt-i) * EDID_LENGTH);
+				i--;
+				eblock_cnt--;
+				continue;
+			} else if (i == 1 || i == 128) {
+				if (eblock_cnt >= total_cnt) {
+					u8 *tmp_p;
+					tmp_p = krealloc(block, (eblock_cnt + 2) * EDID_LENGTH, GFP_KERNEL);
+					if (!tmp_p)
+						return -ENOMEM;
+					*blockp = block = tmp_p;
+					total_cnt = eblock_cnt + 1;
+				}
+				eblock_cnt++;
+				memmove(&block[(i + 1) * EDID_LENGTH],
+					&block[i * EDID_LENGTH],
+					(eblock_cnt-i) * EDID_LENGTH);
+			}
+		}
+		fix_map(&block[EDID_LENGTH], eblock_cnt);
+
+		if (eblock_cnt == 129)
+			return 128;
+
+		if (eblock_cnt > 129)
+		       fix_map(&block[EDID_LENGTH * 128], eblock_cnt - 127);
+	}
+	return eblock_cnt;
+}
+
 static u8 *
 drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 {
@@ -385,10 +475,15 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 
  no_more:
 	if (valid_extensions != block[EDID_EXTENSION_FLAG_OFFSET]) {
-		block[EDID_CHECKSUM_OFFSET] += block[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions;
-		block[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions;
-		new = krealloc(block, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
-		if (!new)
+		if (valid_extensions)
+			valid_extensions = fixup_blockmaps(&block, valid_extensions);
+		if (valid_extensions >= 0) {
+			block[EDID_CHECKSUM_OFFSET] += block[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions;
+			block[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions;
+			new = krealloc(block, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
+			if (!new)
+				goto out;
+		} else
 			goto out;
 		block = new;
 	}
-- 
1.7.7

[PATCH 06/17] DRM/exynos: Fix Memory Leak: free EDID blcok returned by drm_get_edid().

[Egbert Eich]

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/exynos/exynos_hdmi.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index 2c115f8..bc87bca 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -1293,6 +1293,7 @@ static int hdmi_get_edid(void *ctx, struct drm_connector *connector,
 		DRM_DEBUG_KMS("%s : width[%d] x height[%d]\n",
 			(hdata->dvi_mode ? "dvi monitor" : "hdmi monitor"),
 			raw_edid->width_cm, raw_edid->height_cm);
+		kfree(raw_edid);
 	} else {
 		return -ENODEV;
 	}
-- 
1.7.7

[PATCH 07/17] DRM/KMS/EDID: Move drm_edid_load.o to drm.ko

[Egbert Eich]

EDIDs are an integral concept of connectors, connectors are a concept
of drm core also drm_edid.o is already part of this drm core.
Overridden, 'firmware-supplied' EDIDs should be treated exactly the
same as device supplied ones.
Therefore move drm_edid_load from the helper level to the drm core.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 2ff5cef..76ca269 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -16,11 +16,11 @@ drm-y       :=	drm_auth.o drm_buffer.o drm_bufs.o drm_cache.o \
 
 drm-$(CONFIG_COMPAT) += drm_ioc32.o
 drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
+drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
 
 drm-usb-y   := drm_usb.o
 
 drm_kms_helper-y := drm_fb_helper.o drm_crtc_helper.o drm_dp_i2c_helper.o
-drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
 drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
 
 obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
-- 
1.7.7

[PATCH 08/17] DRM/KMS/EDID: Use Extension Block Fixup Code also for 'firmware' EDID.

[Egbert Eich]

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/drm_edid.c      |   77 ++++++++++++++++++++++++++++++++-------
 drivers/gpu/drm/drm_edid_load.c |   54 ++++++---------------------
 include/drm/drm_edid.h          |    1 +
 3 files changed, 77 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index d1b9d67..7bdae6e 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -408,6 +408,29 @@ fixup_blockmaps(u8 **blockp, int eblock_cnt)
 	return eblock_cnt;
 }
 
+static int
+fixup_edid(u8 **blockp, int valid_extensions)
+{
+	u8 *new = NULL;
+
+	if (valid_extensions != (*blockp)[EDID_EXTENSION_FLAG_OFFSET]) {
+
+		if (valid_extensions)
+			valid_extensions = fixup_blockmaps(blockp, valid_extensions);
+
+		if (valid_extensions >= 0) {
+			(*blockp)[EDID_CHECKSUM_OFFSET] += (*blockp)[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions;
+			(*blockp)[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions;
+			new = krealloc(*blockp, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
+		}
+		if (!new)
+			kfree(*blockp);
+
+		*blockp = new;
+	}
+	return (new ? valid_extensions : -ENOMEM);
+}
+
 static u8 *
 drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 {
@@ -474,19 +497,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 	}
 
  no_more:
-	if (valid_extensions != block[EDID_EXTENSION_FLAG_OFFSET]) {
-		if (valid_extensions)
-			valid_extensions = fixup_blockmaps(&block, valid_extensions);
-		if (valid_extensions >= 0) {
-			block[EDID_CHECKSUM_OFFSET] += block[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions;
-			block[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions;
-			new = krealloc(block, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
-			if (!new)
-				goto out;
-		} else
-			goto out;
-		block = new;
-	}
+	fixup_edid(&block, valid_extensions);
 
 	return block;
 
@@ -503,6 +514,46 @@ out:
 }
 
 /**
+ * Validate an entire EDID blob.
+ * \param connector: drm_connector struct of the used connector.
+ * \param blockp: pointer to address of an raw EDID data block.
+ * \param len: size if block in bytes.
+ *
+ * validate block and return corrected block in \param block.
+ * \return: number of valid extensions or -errno if unsuccessful.
+ */
+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);
+
+	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 (!ret) {
+		n_blocks--;
+		if ((*blockp)[EDID_EXTENSION_FLAG_OFFSET] < n_blocks)
+			n_blocks = (*blockp)[EDID_EXTENSION_FLAG_OFFSET];
+
+		while (n_blocks--) {
+			if (drm_edid_block_valid(*blockp + (valid_extensions + 1) * EDID_LENGTH,
+						 valid_extensions + 1, print_bad_edid))
+				valid_extensions++;
+		}
+		ret = fixup_edid(blockp, valid_extensions);
+	}
+	if (ret < 0)
+		connector->bad_edid_counter++;
+	return ret;
+}
+
+/**
  * Probe DDC presence.
  *
  * \param adapter : i2c device adaptor
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 38d3943..6541c1f 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -119,11 +119,10 @@ static u8 *edid_load(struct drm_connector *connector, char *name,
 {
 	const struct firmware *fw;
 	struct platform_device *pdev;
-	u8 *fwdata = NULL, *edid, *new_edid;
+	u8 *fwdata = NULL;
+	struct edid *edid;
 	int fwsize, expected;
 	int builtin = 0, err = 0;
-	int i, valid_extensions = 0;
-	bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
 
 	pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
 	if (IS_ERR(pdev)) {
@@ -137,7 +136,7 @@ static u8 *edid_load(struct drm_connector *connector, char *name,
 	platform_device_unregister(pdev);
 
 	if (err) {
-		i = 0;
+		int i = 0;
 		while (i < GENERIC_EDIDS && strcmp(name, generic_edid_name[i]))
 			i++;
 		if (i < GENERIC_EDIDS) {
@@ -174,49 +173,20 @@ static u8 *edid_load(struct drm_connector *connector, char *name,
 	}
 	memcpy(edid, fwdata, fwsize);
 
-	if (!drm_edid_block_valid(edid, 0, print_bad_edid)) {
-		connector->bad_edid_counter++;
-		DRM_ERROR("Base block of EDID firmware \"%s\" is invalid ",
-		    name);
-		kfree(edid);
-		err = -EINVAL;
-		goto relfw_out;
-	}
-
-	for (i = 1; i <= edid[0x7e]; i++) {
-		if (i != valid_extensions + 1)
-			memcpy(edid + (valid_extensions + 1) * EDID_LENGTH,
-			    edid + i * EDID_LENGTH, EDID_LENGTH);
-		if (drm_edid_block_valid(edid + i * EDID_LENGTH, i, print_bad_edid))
-			valid_extensions++;
-	}
-
-	if (valid_extensions != edid[0x7e]) {
-		edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
-		DRM_INFO("Found %d valid extensions instead of %d in EDID data "
-		    "\"%s\" for connector \"%s\"\n", valid_extensions,
-		    edid[0x7e], name, connector_name);
-		edid[0x7e] = valid_extensions;
-		new_edid = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH,
-		    GFP_KERNEL);
-		if (new_edid == NULL) {
-			err = -ENOMEM;
-			kfree(edid);
-			goto relfw_out;
-		}
-		edid = new_edid;
-	}
-
-	DRM_INFO("Got %s EDID base block and %d extension%s from "
-	    "\"%s\" for connector \"%s\"\n", builtin ? "built-in" :
-	    "external", valid_extensions, valid_extensions == 1 ? "" : "s",
-	    name, connector_name);
+	err = drm_validate_edid_blob(connector, (u8 **)&edid, fwsize);
+	if (err < 0)
+		DRM_ERROR("EDID firmware \"%s\" is invalid ", name);
+	else
+		DRM_INFO("Got %s EDID base block and %d extension%s from "
+			 "\"%s\" for connector \"%s\"\n", builtin ? "built-in" :
+			 "external", edid->extensions, edid->extensions == 1 ? "" : "s",
+			 name, connector_name);
 
 relfw_out:
 	release_firmware(fw);
 
 out:
-	if (err)
+	if (err < 0)
 		return ERR_PTR(err);
 
 	return edid;
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 0cac551..3e8ef06 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -253,5 +253,6 @@ int drm_av_sync_delay(struct drm_connector *connector,
 struct drm_connector *drm_select_eld(struct drm_encoder *encoder,
 				     struct drm_display_mode *mode);
 int drm_load_edid_firmware(struct drm_connector *connector);
+int drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len);
 
 #endif /* __DRM_EDID_H__ */
-- 
1.7.7

[PATCH 09/17] DRM/KMS/EDID: Feed 'firmware' supplied EDID blocks whenever the EDID is read.

[Egbert Eich]

Firmware supplied EDIDs where fed in in drm_helper_probe_single_connector_modes()
in place of calling the driver supplied get_modes() function.
This has two problems:
1. Drivers don't call drm_get_edid() only from within get_modes().
2. The get_modes() replacement in drm_load_edid_firmware() provided only
   the least common denominator of what driver provided get_modes() callbacks
   do.
This patch now supplies a user provided 'firmware' EDID whenever drm_get_edid()
is called if one is available. The user supplied EDID is thus used the same way
as a display provided one (except for detecting the presence of a device thru
EDID). Also it does so not on the helper level any more, the possibility for
EDID loading now happens on the DRM KMS core level.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/drm_crtc_helper.c |    6 +-----
 drivers/gpu/drm/drm_edid.c        |    7 +++++++
 drivers/gpu/drm/drm_edid_load.c   |   22 +++++++++-------------
 include/drm/drm_edid.h            |    4 +++-
 4 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 1227adf..bc99595 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -119,11 +119,7 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 		goto prune;
 	}
 
-#ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
-	count = drm_load_edid_firmware(connector);
-	if (count == 0)
-#endif
-		count = (*connector_funcs->get_modes)(connector);
+	count = (*connector_funcs->get_modes)(connector);
 
 	if (count == 0 && connector->status == connector_status_connected)
 		count = drm_add_modes_noedid(connector, 1024, 768);
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 7bdae6e..3e7df61 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -438,6 +438,13 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 	u8 *block, *new;
 	bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
 
+#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)
+		return block;
+#endif
+
 	if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
 		return NULL;
 
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 6541c1f..1475c6f 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -114,7 +114,7 @@ static u8 generic_edid[GENERIC_EDIDS][128] = {
 	},
 };
 
-static u8 *edid_load(struct drm_connector *connector, char *name,
+static struct edid *edid_load(struct drm_connector *connector, char *name,
 			char *connector_name)
 {
 	const struct firmware *fw;
@@ -192,36 +192,32 @@ out:
 	return edid;
 }
 
-int drm_load_edid_firmware(struct drm_connector *connector)
+struct edid *
+drm_load_edid_firmware(struct drm_connector *connector)
 {
 	char *connector_name = drm_get_connector_name(connector);
 	char *edidname = edid_firmware, *last, *colon;
-	int ret;
 	struct edid *edid;
 
 	if (*edidname == '\0')
-		return 0;
+		return NULL;
 
 	colon = strchr(edidname, ':');
 	if (colon != NULL) {
 		if (strncmp(connector_name, edidname, colon - edidname))
-			return 0;
+			return NULL;
 		edidname = colon + 1;
 		if (*edidname == '\0')
-			return 0;
+			return NULL;
 	}
 
 	last = edidname + strlen(edidname) - 1;
 	if (*last == '\n')
 		*last = '\0';
 
-	edid = (struct edid *) edid_load(connector, edidname, connector_name);
+	edid = edid_load(connector, edidname, connector_name);
 	if (IS_ERR_OR_NULL(edid))
-		return 0;
+		return NULL;
 
-	drm_mode_connector_update_edid_property(connector, edid);
-	ret = drm_add_edid_modes(connector, edid);
-	kfree(edid);
-
-	return ret;
+	return edid;
 }
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 3e8ef06..53c619c 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -252,7 +252,9 @@ int drm_av_sync_delay(struct drm_connector *connector,
 		      struct drm_display_mode *mode);
 struct drm_connector *drm_select_eld(struct drm_encoder *encoder,
 				     struct drm_display_mode *mode);
-int drm_load_edid_firmware(struct drm_connector *connector);
+#ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
+struct edid *drm_load_edid_firmware(struct drm_connector *connector);
+#endif
 int drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len);
 
 #endif /* __DRM_EDID_H__ */
-- 
1.7.7

[PATCH 10/17] DRM/KMS/EDID: Cache EDID blobs with extensions.

[Egbert Eich]

According the the VESA specs there can be up to 254 EEDID extension blocks.
Since we may read the EDID (including extensions) in 10 second intervals to
probe for display hotplugging (at least in cases where no hardware hotplug
detection exists) and I2C transfer is rather slow we may end up consuming
a considerable amount on CPU time for just that.
This patch caches the EDID block if it contains at least one extension.
To determine if the blocks match we only tranfer the base block, on a match
we use the cached data.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/drm_crtc.c |    1 +
 drivers/gpu/drm/drm_edid.c |   43 ++++++++++++++++++++++++++++++++++++++++++-
 include/drm/drm_crtc.h     |    1 +
 include/drm/drm_edid.h     |    1 +
 4 files changed, 45 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 3533609..e283355 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -598,6 +598,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
 		drm_mode_remove(connector, mode);
 
 	mutex_lock(&dev->mode_config.mutex);
+	drm_cache_edid(connector, NULL);
 	drm_mode_object_put(dev, &connector->base);
 	list_del(&connector->head);
 	dev->mode_config.num_connector--;
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 3e7df61..410a54d 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -431,6 +431,41 @@ fixup_edid(u8 **blockp, int valid_extensions)
 	return (new ? valid_extensions : -ENOMEM);
 }
 
+static bool 
+compare_get_edid_from_cache(struct drm_connector *connector, struct edid **edidp)
+{
+	if (connector->edid_cache &&
+	    connector->edid_cache->prod_code[0] == (*edidp)->prod_code[0] &&
+	    connector->edid_cache->prod_code[1] == (*edidp)->prod_code[1] &&
+	    connector->edid_cache->serial == (*edidp)->serial &&
+	    connector->edid_cache->input == (*edidp)->input) {
+		int size = (connector->edid_cache->extensions + 1) * EDID_LENGTH;
+		struct edid *new = kmalloc(size, GFP_KERNEL);
+		if (!new)
+			return false;
+		DRM_DEBUG_KMS("Got EDID for %s from cache.\n",drm_get_connector_name(connector));
+		memcpy(new, connector->edid_cache, size);
+		kfree(*edidp);
+		*edidp = new;
+		return true;
+	}
+	return false;
+}
+
+void
+drm_cache_edid(struct drm_connector *connector, struct edid *edid)
+{
+	struct edid *new = NULL;
+	kfree(connector->edid_cache);
+	if (edid) {
+		int size = (edid->extensions + 1) * EDID_LENGTH;
+		new = kmalloc(size, GFP_KERNEL);
+		if (new)
+		  memcpy(new, edid, size);
+	}
+	connector->edid_cache = new;
+}
+
 static u8 *
 drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 {
@@ -462,10 +497,14 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 	if (i == 4)
 		goto carp;
 
-	/* if there's no extensions, we're done */
+	/* if there are no extensions, we're done - don't bother caching */
 	if (block[EDID_EXTENSION_FLAG_OFFSET] == 0)
 		return block;
 
+	/* 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;
+
 	new = krealloc(block, (block[EDID_EXTENSION_FLAG_OFFSET] + 1) * EDID_LENGTH, GFP_KERNEL);
 	if (!new)
 		goto out;
@@ -505,6 +544,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 
  no_more:
 	fixup_edid(&block, valid_extensions);
+	drm_cache_edid(connector, (struct edid *)block);
 
 	return block;
 
@@ -513,6 +553,7 @@ carp:
 		dev_warn(connector->dev->dev, "%s: EDID block %d invalid.\n",
 			 drm_get_connector_name(connector), j);
 	}
+	drm_cache_edid(connector, NULL);
 	connector->bad_edid_counter++;
 
 out:
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 49dd8c2..6a1054c 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -601,6 +601,7 @@ struct drm_connector {
 	struct drm_encoder *encoder; /* currently active encoder */
 
 	/* EDID bits */
+	struct edid *edid_cache;
 	uint8_t eld[MAX_ELD_BYTES];
 	bool dvi_dual;
 	int max_tmds_clock;	/* in MHz */
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 53c619c..c880510 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -256,5 +256,6 @@ struct drm_connector *drm_select_eld(struct drm_encoder *encoder,
 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);
 
 #endif /* __DRM_EDID_H__ */
-- 
1.7.7

[PATCH 11/17] DRM/KMS/EDID: Avoid failing on krealloc on EDID blocks.

[Egbert Eich]

There is no point to call krealloc() to shrink the amount of
memory for EDID blocks: the implementation of krealloc() will
ignore the size change and return the same chunk of memory
anyway.
So far we failed reading EDIDs when krealloc() failed.
This may happen if the size required to store an EDID is larger
than one page and the kernel is not able to find consecutive
pages to allocate.
In this case it is still better to return what we already have
(in most cases the base block) and correct the extension block
flag and the checksum accordingly than to fail completely.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/drm_edid.c |   34 +++++++++++++---------------------
 1 files changed, 13 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 410a54d..16c06d6 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -387,7 +387,7 @@ fixup_blockmaps(u8 **blockp, int eblock_cnt)
 					u8 *tmp_p;
 					tmp_p = krealloc(block, (eblock_cnt + 2) * EDID_LENGTH, GFP_KERNEL);
 					if (!tmp_p)
-						return -ENOMEM;
+						return eblock_cnt;
 					*blockp = block = tmp_p;
 					total_cnt = eblock_cnt + 1;
 				}
@@ -408,27 +408,16 @@ fixup_blockmaps(u8 **blockp, int eblock_cnt)
 	return eblock_cnt;
 }
 
-static int
+static void
 fixup_edid(u8 **blockp, int valid_extensions)
 {
-	u8 *new = NULL;
-
-	if (valid_extensions != (*blockp)[EDID_EXTENSION_FLAG_OFFSET]) {
-
+  if (valid_extensions != (*blockp)[EDID_EXTENSION_FLAG_OFFSET]) {
 		if (valid_extensions)
 			valid_extensions = fixup_blockmaps(blockp, valid_extensions);
 
-		if (valid_extensions >= 0) {
-			(*blockp)[EDID_CHECKSUM_OFFSET] += (*blockp)[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions;
-			(*blockp)[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions;
-			new = krealloc(*blockp, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
-		}
-		if (!new)
-			kfree(*blockp);
-
-		*blockp = new;
+		(*blockp)[EDID_CHECKSUM_OFFSET] += (*blockp)[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions;
+		(*blockp)[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions;
 	}
-	return (new ? valid_extensions : -ENOMEM);
 }
 
 static bool 
@@ -506,8 +495,12 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 		return block;
 
 	new = krealloc(block, (block[EDID_EXTENSION_FLAG_OFFSET] + 1) * EDID_LENGTH, GFP_KERNEL);
-	if (!new)
-		goto out;
+	if (!new) {
+		dev_warn(connector->dev->dev, "%s: cannot allocate memory for %d EDID blocks: trunkating.\n",
+			 drm_get_connector_name(connector), block[EDID_EXTENSION_FLAG_OFFSET] + 1);
+		valid_extensions = 0;
+		goto no_more;
+	}
 	block = new;
 
 	for (j = 1; j <= block[EDID_EXTENSION_FLAG_OFFSET]; j++) {
@@ -594,9 +587,8 @@ drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len)
 						 valid_extensions + 1, print_bad_edid))
 				valid_extensions++;
 		}
-		ret = fixup_edid(blockp, valid_extensions);
-	}
-	if (ret < 0)
+		fixup_edid(blockp, valid_extensions);
+	} else
 		connector->bad_edid_counter++;
 	return ret;
 }
-- 
1.7.7

[PATCH 12/17] DRM/KMS/EDID: Consolidate EDID Error Handling.

[Egbert Eich]

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.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/drm_edid.c                 |  101 +++++++++++++++++-----------
 drivers/gpu/drm/radeon/radeon_connectors.c |    2 +-
 include/drm/drm_crtc.h                     |    4 +-
 include/drm/drm_edid.h                     |    9 +++
 4 files changed, 72 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 16c06d6..6144150 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -158,6 +158,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,
@@ -167,11 +178,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;
@@ -183,27 +196,34 @@ 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;
 		}
 
@@ -215,15 +235,23 @@ 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;
+}
+
+bool
+drm_edid_block_valid(u8 *raw_edid, int block, unsigned last_error_flags)
+{
+	if (!drm_edid_block_check_error(raw_edid, block, last_error_flags))
+		return true;
+	return false;
 }
 EXPORT_SYMBOL(drm_edid_block_valid);
 
@@ -242,7 +270,7 @@ bool drm_edid_is_valid(struct edid *edid)
 		return false;
 
 	for (i = 0; i <= edid->extensions; i++)
-		if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true))
+		if (drm_edid_block_check_error(raw + i * EDID_LENGTH, i, true))
 			return false;
 
 	return true;
@@ -311,17 +339,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)
 {
@@ -460,7 +477,8 @@ 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 = 0;
 
 #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
 	/* check if the user has specified a 'firmware' EDID file */
@@ -476,12 +494,11 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 	for (i = 0; i < 4; i++) {
 		if (drm_do_probe_ddc_edid(adapter, block, 0, EDID_LENGTH))
 			goto out;
-		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 carp;
-		}
 	}
 	if (i == 4)
 		goto carp;
@@ -511,7 +528,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 				valid_extensions = 0;
 				goto no_more;
 			}
-			if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) {
+			if (!drm_edid_block_check_error(block + (valid_extensions + 1) * EDID_LENGTH, j, last_error_flags)) {
 				valid_extensions++;
 				/* Test if base block announced extension blocks although
 				 * display is not EDDC capable.
@@ -538,16 +555,17 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
  no_more:
 	fixup_edid(&block, valid_extensions);
 	drm_cache_edid(connector, (struct edid *)block);
+	connector->last_edid_error_flags = 0;
 
 	return block;
 
 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);
 	}
 	drm_cache_edid(connector, NULL);
-	connector->bad_edid_counter++;
+	connector->last_edid_error_flags = result;
 
 out:
 	kfree(block);
@@ -567,29 +585,32 @@ 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) {
 		n_blocks--;
 		if ((*blockp)[EDID_EXTENSION_FLAG_OFFSET] < n_blocks)
 			n_blocks = (*blockp)[EDID_EXTENSION_FLAG_OFFSET];
 
 		while (n_blocks--) {
-			if (drm_edid_block_valid(*blockp + (valid_extensions + 1) * EDID_LENGTH,
-						 valid_extensions + 1, print_bad_edid))
+			if (!drm_edid_block_check_error(*blockp + (valid_extensions + 1) * EDID_LENGTH,
+						 valid_extensions + 1, last_error_flags))
 				valid_extensions++;
 		}
 		fixup_edid(blockp, valid_extensions);
-	} else
-		connector->bad_edid_counter++;
+		ret = 0;
+	}
+	connector->last_edid_error_flags = result;
 	return ret;
 }
 
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..171aa33 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,7 +1055,6 @@ 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);
 struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
 					   int hsize, int vsize, int fresh,
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index c880510..bf3b5d5 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -244,6 +244,14 @@ 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,
+};
+
 struct drm_encoder;
 struct drm_connector;
 struct drm_display_mode;
@@ -257,5 +265,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

[PATCH 13/17] DRM/KMS/EDID: Allow for multiple Connectors when specifying 'firmware'-EDID Files.

[Egbert Eich]

So far it was only possible to load an EDID for a single connector (unless
no connector was specified at all in which case the same EDID file was used
for all).
This patch extends the EDID loader so that EDID files can be specified for
more than one connector. A semicolon is used to separate the different connector
sections.
The option now looks like this:
   edid_firmware="[<connector_0>:]<edid_file_0>[;<connector_n>:<edid_file_1>]..."

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/drm_edid_load.c |   45 ++++++++++++++++++++++++++------------
 1 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 1475c6f..a40a88505 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -196,26 +196,43 @@ struct edid *
 drm_load_edid_firmware(struct drm_connector *connector)
 {
 	char *connector_name = drm_get_connector_name(connector);
-	char *edidname = edid_firmware, *last, *colon;
-	struct edid *edid;
+	char *edidname, *last, *colon;
+	struct edid *edid = NULL;
+	char *next, *mem;
+	bool many = false;
 
-	if (*edidname == '\0')
+	if (*edid_firmware == '\0')
 		return NULL;
 
-	colon = strchr(edidname, ':');
-	if (colon != NULL) {
-		if (strncmp(connector_name, edidname, colon - edidname))
-			return NULL;
-		edidname = colon + 1;
-		if (*edidname == '\0')
-			return NULL;
+	edidname = mem = kstrndup(edid_firmware, PATH_MAX, GFP_KERNEL);
+	do {
+		next = strchr(edidname, ';');
+		if (next)
+			*(next++) = '\0';
+		colon = strchr(edidname, ':');
+		if (colon == NULL) {
+			if (next || many)
+				edidname = NULL;
+			break;
+		} else if (!strncmp(connector_name, edidname, colon - edidname)) {
+			edidname = colon + 1;
+			break;
+		}
+		edidname = next;
+		many = true;
+	} while (edidname);
+
+	if (edidname && *edidname != '\0') {
+		last = edidname + strlen(edidname) - 1;
+		if (*last == '\n')
+			*last = '\0';
+
+		if (strlen(edidname) > 0)
+			edid = edid_load(connector, edidname, connector_name);
 	}
 
-	last = edidname + strlen(edidname) - 1;
-	if (*last == '\n')
-		*last = '\0';
+	kfree(mem);
 
-	edid = edid_load(connector, edidname, connector_name);
 	if (IS_ERR_OR_NULL(edid))
 		return NULL;
 
-- 
1.7.7

[PATCH 14/17] DRM/KMS/ast: Include drm_edid.h in file using drm_get_edid().

[Egbert Eich]

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/ast/ast_mode.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 7fc9f72..c27aa8d 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -31,6 +31,7 @@
 #include <drm/drmP.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_edid.h>
 #include "ast_drv.h"
 
 #include "ast_tables.h"
-- 
1.7.7

[PATCH 15/17] DRM/KMS/gma500: Include drm_edid.h in file using drm_get_edid().

[Egbert Eich]

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/gma500/cdv_intel_dp.c    |    1 +
 drivers/gpu/drm/gma500/oaktrail_lvds.c   |    1 +
 drivers/gpu/drm/gma500/psb_intel_modes.c |    1 +
 3 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/gma500/cdv_intel_dp.c b/drivers/gpu/drm/gma500/cdv_intel_dp.c
index e3a3978..37cad73 100644
--- a/drivers/gpu/drm/gma500/cdv_intel_dp.c
+++ b/drivers/gpu/drm/gma500/cdv_intel_dp.c
@@ -34,6 +34,7 @@
 #include "psb_intel_drv.h"
 #include "psb_intel_reg.h"
 #include <drm/drm_dp_helper.h>
+#include <drm/drm_edid.h>
 
 #define _wait_for(COND, MS, W) ({ \
         unsigned long timeout__ = jiffies + msecs_to_jiffies(MS);       \
diff --git a/drivers/gpu/drm/gma500/oaktrail_lvds.c b/drivers/gpu/drm/gma500/oaktrail_lvds.c
index 558c77f..f7e20f5 100644
--- a/drivers/gpu/drm/gma500/oaktrail_lvds.c
+++ b/drivers/gpu/drm/gma500/oaktrail_lvds.c
@@ -22,6 +22,7 @@
 
 #include <linux/i2c.h>
 #include <drm/drmP.h>
+#include <drm/drm_edid.h>
 #include <asm/mrst.h>
 
 #include "intel_bios.h"
diff --git a/drivers/gpu/drm/gma500/psb_intel_modes.c b/drivers/gpu/drm/gma500/psb_intel_modes.c
index 4fca0d6..1da014d 100644
--- a/drivers/gpu/drm/gma500/psb_intel_modes.c
+++ b/drivers/gpu/drm/gma500/psb_intel_modes.c
@@ -20,6 +20,7 @@
 #include <linux/i2c.h>
 #include <linux/fb.h>
 #include <drm/drmP.h>
+#include <drm/drm_edid.h>
 #include "psb_intel_drv.h"
 
 /**
-- 
1.7.7

[PATCH 16/17] DRM/KMS/mgag200: Include drm_edid.h in file using drm_get_edid().

[Egbert Eich]

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index d3d99a2..f89a0c1 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -15,6 +15,7 @@
 
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_edid.h>
 
 #include "mgag200_drv.h"
 
-- 
1.7.7

[PATCH 17/17] DRM/KMS/EDID: Move EDID related Functions to drm_edid.h.

[Egbert Eich]

Signed-off-by: Egbert Eich <eich@suse.de>
---
 include/drm/drm_crtc.h |    8 --------
 include/drm/drm_edid.h |    9 +++++++++
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 171aa33..06254bc 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -878,9 +878,6 @@ extern char *drm_get_tv_subconnector_name(int val);
 extern char *drm_get_tv_select_name(int val);
 extern void drm_fb_release(struct drm_file *file_priv);
 extern int drm_mode_group_init_legacy_group(struct drm_device *dev, struct drm_mode_group *group);
-extern bool drm_probe_ddc(struct i2c_adapter *adapter);
-extern struct edid *drm_get_edid(struct drm_connector *connector,
-				 struct i2c_adapter *adapter);
 extern int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
 extern void drm_mode_probed_add(struct drm_connector *connector, struct drm_display_mode *mode);
 extern void drm_mode_remove(struct drm_connector *connector, struct drm_display_mode *mode);
@@ -1036,9 +1033,6 @@ extern int drm_mode_gamma_get_ioctl(struct drm_device *dev,
 				    void *data, struct drm_file *file_priv);
 extern int drm_mode_gamma_set_ioctl(struct drm_device *dev,
 				    void *data, struct drm_file *file_priv);
-extern u8 *drm_find_cea_extension(struct edid *edid);
-extern bool drm_detect_hdmi_monitor(struct edid *edid);
-extern bool drm_detect_monitor_audio(struct edid *edid);
 extern int drm_mode_page_flip_ioctl(struct drm_device *dev,
 				    void *data, struct drm_file *file_priv);
 extern struct drm_display_mode *drm_cvt_mode(struct drm_device *dev,
@@ -1054,8 +1048,6 @@ extern struct drm_display_mode *drm_gtf_mode_complex(struct drm_device *dev,
 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_is_valid(struct edid *edid);
 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 bf3b5d5..a18a8b9 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -267,4 +267,13 @@ 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);
 
+extern bool drm_probe_ddc(struct i2c_adapter *adapter);
+extern struct edid *drm_get_edid(struct drm_connector *connector,
+				 struct i2c_adapter *adapter);
+extern u8 *drm_find_cea_extension(struct edid *edid);
+extern bool drm_detect_hdmi_monitor(struct edid *edid);
+extern bool drm_detect_monitor_audio(struct edid *edid);
+extern int drm_edid_header_is_valid(const u8 *raw_edid);
+extern bool drm_edid_is_valid(struct edid *edid);
+
 #endif /* __DRM_EDID_H__ */
-- 
1.7.7

Re: [PATCH 06/17] DRM/exynos: Fix Memory Leak: free EDID blcok returned by drm_get_edid().

[Inki Dae]

[-- Attachment #1.1: Type: text/plain, Size: 1149 bytes --]

Could you please re-send this patch updating like below?

For example,

[PATCH ] drm/exynos: fix memory leak to edid

some comments.

Thanks,
Inki Dae

2012/11/20 Egbert Eich <eich@suse.de>

> Signed-off-by: Egbert Eich <eich@suse.de>
> ---
>  drivers/gpu/drm/exynos/exynos_hdmi.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c
> b/drivers/gpu/drm/exynos/exynos_hdmi.c
> index 2c115f8..bc87bca 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
> @@ -1293,6 +1293,7 @@ static int hdmi_get_edid(void *ctx, struct
> drm_connector *connector,
>                 DRM_DEBUG_KMS("%s : width[%d] x height[%d]\n",
>                         (hdata->dvi_mode ? "dvi monitor" : "hdmi monitor"),
>                         raw_edid->width_cm, raw_edid->height_cm);
> +               kfree(raw_edid);
>         } else {
>                 return -ENODEV;
>         }
> --
> 1.7.7
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

[-- Attachment #1.2: Type: text/html, Size: 1764 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 05/17] DRM/KMS/EDID: Fix up EEDID Map Blocks if Extension block count has changed.

[Takashi Iwai]

At Mon, 19 Nov 2012 15:23:06 -0500,
"Egbert Eich " <"eich@novell.com> wrote:
> 
> EEDID v1.3 mandates map blocks if more than one EDID extension block
> is used while in v1.4 they are optional.
> If the extension count has been changed (because some extension
> blocks were not readable) those map blocks need fixing.
> In case of v1.4 or higher we simply eliminate all map blocks as
> this is less time consuming. For v1.3 we scrap any exsisting map
> blocks and recreate them from scratch.
> 
> Signed-off-by: Egbert Eich <eich@suse.de>
> ---
>  drivers/gpu/drm/drm_edid.c |  105 +++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 100 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index ec6f3bb..d1b9d67 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -44,7 +44,11 @@
>  #define EDID_DETAILED_TIMINGS 4
>  
>  #define EDID_EXTENSION_FLAG_OFFSET  ((size_t)&((struct edid *)0)->extensions)
> -#define EDID_CHECKSUM_OFFSET  ((size_t)&((struct edid *)0)->checksum)
> +#define EDID_CHECKSUM_OFFSET	    ((size_t)&((struct edid *)0)->checksum)
> +#define EDID_VERSION_MAJOR_OFFSET   ((size_t)&((struct edid *)0)->version)
> +#define EDID_VERSION_MINOR_OFFSET   ((size_t)&((struct edid *)0)->revision)

Better to use offsetof().


Takashi

Re: [PATCH 08/17] DRM/KMS/EDID: Use Extension Block Fixup Code also for 'firmware' EDID.

[Takashi Iwai]

At Mon, 19 Nov 2012 15:23:09 -0500,
"Egbert Eich " <"eich@novell.com> wrote:
> 
> Signed-off-by: Egbert Eich <eich@suse.de>
> ---
>  drivers/gpu/drm/drm_edid.c      |   77 ++++++++++++++++++++++++++++++++-------
>  drivers/gpu/drm/drm_edid_load.c |   54 ++++++---------------------
>  include/drm/drm_edid.h          |    1 +
>  3 files changed, 77 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index d1b9d67..7bdae6e 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -408,6 +408,29 @@ fixup_blockmaps(u8 **blockp, int eblock_cnt)
>  	return eblock_cnt;
>  }
>  
> +static int
> +fixup_edid(u8 **blockp, int valid_extensions)
> +{
> +	u8 *new = NULL;
> +
> +	if (valid_extensions != (*blockp)[EDID_EXTENSION_FLAG_OFFSET]) {
> +
> +		if (valid_extensions)
> +			valid_extensions = fixup_blockmaps(blockp, valid_extensions);
> +
> +		if (valid_extensions >= 0) {
> +			(*blockp)[EDID_CHECKSUM_OFFSET] += (*blockp)[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions;
> +			(*blockp)[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions;
> +			new = krealloc(*blockp, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
> +		}
> +		if (!new)
> +			kfree(*blockp);
> +
> +		*blockp = new;
> +	}
> +	return (new ? valid_extensions : -ENOMEM);

So this function returns -ENOMEM if valid_extensions is equal with
block[EDID_EXTENSION_FLAG_OFFSET]...?


Takashi

> +}
> +
>  static u8 *
>  drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  {
> @@ -474,19 +497,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  	}
>  
>   no_more:
> -	if (valid_extensions != block[EDID_EXTENSION_FLAG_OFFSET]) {
> -		if (valid_extensions)
> -			valid_extensions = fixup_blockmaps(&block, valid_extensions);
> -		if (valid_extensions >= 0) {
> -			block[EDID_CHECKSUM_OFFSET] += block[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions;
> -			block[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions;
> -			new = krealloc(block, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
> -			if (!new)
> -				goto out;
> -		} else
> -			goto out;
> -		block = new;
> -	}
> +	fixup_edid(&block, valid_extensions);
>  
>  	return block;
>  
> @@ -503,6 +514,46 @@ out:
>  }
>  
>  /**
> + * Validate an entire EDID blob.
> + * \param connector: drm_connector struct of the used connector.
> + * \param blockp: pointer to address of an raw EDID data block.
> + * \param len: size if block in bytes.
> + *
> + * validate block and return corrected block in \param block.
> + * \return: number of valid extensions or -errno if unsuccessful.
> + */
> +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);
> +
> +	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 (!ret) {
> +		n_blocks--;
> +		if ((*blockp)[EDID_EXTENSION_FLAG_OFFSET] < n_blocks)
> +			n_blocks = (*blockp)[EDID_EXTENSION_FLAG_OFFSET];
> +
> +		while (n_blocks--) {
> +			if (drm_edid_block_valid(*blockp + (valid_extensions + 1) * EDID_LENGTH,
> +						 valid_extensions + 1, print_bad_edid))
> +				valid_extensions++;
> +		}
> +		ret = fixup_edid(blockp, valid_extensions);
> +	}
> +	if (ret < 0)
> +		connector->bad_edid_counter++;
> +	return ret;
> +}
> +
> +/**
>   * Probe DDC presence.
>   *
>   * \param adapter : i2c device adaptor
> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
> index 38d3943..6541c1f 100644
> --- a/drivers/gpu/drm/drm_edid_load.c
> +++ b/drivers/gpu/drm/drm_edid_load.c
> @@ -119,11 +119,10 @@ static u8 *edid_load(struct drm_connector *connector, char *name,
>  {
>  	const struct firmware *fw;
>  	struct platform_device *pdev;
> -	u8 *fwdata = NULL, *edid, *new_edid;
> +	u8 *fwdata = NULL;
> +	struct edid *edid;
>  	int fwsize, expected;
>  	int builtin = 0, err = 0;
> -	int i, valid_extensions = 0;
> -	bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
>  
>  	pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
>  	if (IS_ERR(pdev)) {
> @@ -137,7 +136,7 @@ static u8 *edid_load(struct drm_connector *connector, char *name,
>  	platform_device_unregister(pdev);
>  
>  	if (err) {
> -		i = 0;
> +		int i = 0;
>  		while (i < GENERIC_EDIDS && strcmp(name, generic_edid_name[i]))
>  			i++;
>  		if (i < GENERIC_EDIDS) {
> @@ -174,49 +173,20 @@ static u8 *edid_load(struct drm_connector *connector, char *name,
>  	}
>  	memcpy(edid, fwdata, fwsize);
>  
> -	if (!drm_edid_block_valid(edid, 0, print_bad_edid)) {
> -		connector->bad_edid_counter++;
> -		DRM_ERROR("Base block of EDID firmware \"%s\" is invalid ",
> -		    name);
> -		kfree(edid);
> -		err = -EINVAL;
> -		goto relfw_out;
> -	}
> -
> -	for (i = 1; i <= edid[0x7e]; i++) {
> -		if (i != valid_extensions + 1)
> -			memcpy(edid + (valid_extensions + 1) * EDID_LENGTH,
> -			    edid + i * EDID_LENGTH, EDID_LENGTH);
> -		if (drm_edid_block_valid(edid + i * EDID_LENGTH, i, print_bad_edid))
> -			valid_extensions++;
> -	}
> -
> -	if (valid_extensions != edid[0x7e]) {
> -		edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
> -		DRM_INFO("Found %d valid extensions instead of %d in EDID data "
> -		    "\"%s\" for connector \"%s\"\n", valid_extensions,
> -		    edid[0x7e], name, connector_name);
> -		edid[0x7e] = valid_extensions;
> -		new_edid = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH,
> -		    GFP_KERNEL);
> -		if (new_edid == NULL) {
> -			err = -ENOMEM;
> -			kfree(edid);
> -			goto relfw_out;
> -		}
> -		edid = new_edid;
> -	}
> -
> -	DRM_INFO("Got %s EDID base block and %d extension%s from "
> -	    "\"%s\" for connector \"%s\"\n", builtin ? "built-in" :
> -	    "external", valid_extensions, valid_extensions == 1 ? "" : "s",
> -	    name, connector_name);
> +	err = drm_validate_edid_blob(connector, (u8 **)&edid, fwsize);
> +	if (err < 0)
> +		DRM_ERROR("EDID firmware \"%s\" is invalid ", name);
> +	else
> +		DRM_INFO("Got %s EDID base block and %d extension%s from "
> +			 "\"%s\" for connector \"%s\"\n", builtin ? "built-in" :
> +			 "external", edid->extensions, edid->extensions == 1 ? "" : "s",
> +			 name, connector_name);
>  
>  relfw_out:
>  	release_firmware(fw);
>  
>  out:
> -	if (err)
> +	if (err < 0)
>  		return ERR_PTR(err);
>  
>  	return edid;
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 0cac551..3e8ef06 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -253,5 +253,6 @@ int drm_av_sync_delay(struct drm_connector *connector,
>  struct drm_connector *drm_select_eld(struct drm_encoder *encoder,
>  				     struct drm_display_mode *mode);
>  int drm_load_edid_firmware(struct drm_connector *connector);
> +int drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len);
>  
>  #endif /* __DRM_EDID_H__ */
> -- 
> 1.7.7
> 

Re: [PATCH 07/17] DRM/KMS/EDID: Move drm_edid_load.o to drm.ko

[Takashi Iwai]

At Mon, 19 Nov 2012 15:23:08 -0500,
"Egbert Eich " <"eich@novell.com> wrote:
> 
> EDIDs are an integral concept of connectors, connectors are a concept
> of drm core also drm_edid.o is already part of this drm core.
> Overridden, 'firmware-supplied' EDIDs should be treated exactly the
> same as device supplied ones.
> Therefore move drm_edid_load from the helper level to the drm core.
> 
> Signed-off-by: Egbert Eich <eich@suse.de>

You need to fix Kconfig as well.
And I think the dependency on CONFIG_FW_LOADER is missing there, too.
It should have "select FW_LOADER".


Takashi

> ---
>  drivers/gpu/drm/Makefile |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 2ff5cef..76ca269 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -16,11 +16,11 @@ drm-y       :=	drm_auth.o drm_buffer.o drm_bufs.o drm_cache.o \
>  
>  drm-$(CONFIG_COMPAT) += drm_ioc32.o
>  drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
> +drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
>  
>  drm-usb-y   := drm_usb.o
>  
>  drm_kms_helper-y := drm_fb_helper.o drm_crtc_helper.o drm_dp_i2c_helper.o
> -drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
>  drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
>  
>  obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
> -- 
> 1.7.7
> 

Re: [PATCH 10/17] DRM/KMS/EDID: Cache EDID blobs with extensions.

[Takashi Iwai]

At Mon, 19 Nov 2012 15:23:11 -0500,
"Egbert Eich " <"eich@novell.com> wrote:
> 
> According the the VESA specs there can be up to 254 EEDID extension blocks.
> Since we may read the EDID (including extensions) in 10 second intervals to
> probe for display hotplugging (at least in cases where no hardware hotplug
> detection exists) and I2C transfer is rather slow we may end up consuming
> a considerable amount on CPU time for just that.
> This patch caches the EDID block if it contains at least one extension.
> To determine if the blocks match we only tranfer the base block, on a match
> we use the cached data.
> 
> Signed-off-by: Egbert Eich <eich@suse.de>
> ---
>  drivers/gpu/drm/drm_crtc.c |    1 +
>  drivers/gpu/drm/drm_edid.c |   43 ++++++++++++++++++++++++++++++++++++++++++-
>  include/drm/drm_crtc.h     |    1 +
>  include/drm/drm_edid.h     |    1 +
>  4 files changed, 45 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 3533609..e283355 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -598,6 +598,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
>  		drm_mode_remove(connector, mode);
>  
>  	mutex_lock(&dev->mode_config.mutex);
> +	drm_cache_edid(connector, NULL);
>  	drm_mode_object_put(dev, &connector->base);
>  	list_del(&connector->head);
>  	dev->mode_config.num_connector--;
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 3e7df61..410a54d 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -431,6 +431,41 @@ fixup_edid(u8 **blockp, int valid_extensions)
>  	return (new ? valid_extensions : -ENOMEM);
>  }
>  
> +static bool 
> +compare_get_edid_from_cache(struct drm_connector *connector, struct edid **edidp)
> +{
> +	if (connector->edid_cache &&
> +	    connector->edid_cache->prod_code[0] == (*edidp)->prod_code[0] &&
> +	    connector->edid_cache->prod_code[1] == (*edidp)->prod_code[1] &&
> +	    connector->edid_cache->serial == (*edidp)->serial &&
> +	    connector->edid_cache->input == (*edidp)->input) {
> +		int size = (connector->edid_cache->extensions + 1) * EDID_LENGTH;
> +		struct edid *new = kmalloc(size, GFP_KERNEL);
> +		if (!new)
> +			return false;
> +		DRM_DEBUG_KMS("Got EDID for %s from cache.\n",drm_get_connector_name(connector));
> +		memcpy(new, connector->edid_cache, size);
> +		kfree(*edidp);

You can use kmemdup().

> +		*edidp = new;
> +		return true;
> +	}
> +	return false;
> +}
> +
> +void
> +drm_cache_edid(struct drm_connector *connector, struct edid *edid)
> +{
> +	struct edid *new = NULL;
> +	kfree(connector->edid_cache);
> +	if (edid) {
> +		int size = (edid->extensions + 1) * EDID_LENGTH;
> +		new = kmalloc(size, GFP_KERNEL);
> +		if (new)
> +		  memcpy(new, edid, size);

Ditto.


Takashi

> +	}
> +	connector->edid_cache = new;
> +}
> +
>  static u8 *
>  drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  {
> @@ -462,10 +497,14 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  	if (i == 4)
>  		goto carp;
>  
> -	/* if there's no extensions, we're done */
> +	/* if there are no extensions, we're done - don't bother caching */
>  	if (block[EDID_EXTENSION_FLAG_OFFSET] == 0)
>  		return block;
>  
> +	/* 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;
> +
>  	new = krealloc(block, (block[EDID_EXTENSION_FLAG_OFFSET] + 1) * EDID_LENGTH, GFP_KERNEL);
>  	if (!new)
>  		goto out;
> @@ -505,6 +544,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  
>   no_more:
>  	fixup_edid(&block, valid_extensions);
> +	drm_cache_edid(connector, (struct edid *)block);
>  
>  	return block;
>  
> @@ -513,6 +553,7 @@ carp:
>  		dev_warn(connector->dev->dev, "%s: EDID block %d invalid.\n",
>  			 drm_get_connector_name(connector), j);
>  	}
> +	drm_cache_edid(connector, NULL);
>  	connector->bad_edid_counter++;
>  
>  out:
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 49dd8c2..6a1054c 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -601,6 +601,7 @@ struct drm_connector {
>  	struct drm_encoder *encoder; /* currently active encoder */
>  
>  	/* EDID bits */
> +	struct edid *edid_cache;
>  	uint8_t eld[MAX_ELD_BYTES];
>  	bool dvi_dual;
>  	int max_tmds_clock;	/* in MHz */
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 53c619c..c880510 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -256,5 +256,6 @@ struct drm_connector *drm_select_eld(struct drm_encoder *encoder,
>  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);
>  
>  #endif /* __DRM_EDID_H__ */
> -- 
> 1.7.7
> 

Re: [PATCH 13/17] DRM/KMS/EDID: Allow for multiple Connectors when specifying 'firmware'-EDID Files.

[Takashi Iwai]

At Mon, 19 Nov 2012 15:23:14 -0500,
"Egbert Eich " <"eich@novell.com> wrote:
> 
> So far it was only possible to load an EDID for a single connector (unless
> no connector was specified at all in which case the same EDID file was used
> for all).
> This patch extends the EDID loader so that EDID files can be specified for
> more than one connector. A semicolon is used to separate the different connector
> sections.
> The option now looks like this:
>    edid_firmware="[<connector_0>:]<edid_file_0>[;<connector_n>:<edid_file_1>]..."
> 
> Signed-off-by: Egbert Eich <eich@suse.de>
> ---
>  drivers/gpu/drm/drm_edid_load.c |   45 ++++++++++++++++++++++++++------------
>  1 files changed, 31 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
> index 1475c6f..a40a88505 100644
> --- a/drivers/gpu/drm/drm_edid_load.c
> +++ b/drivers/gpu/drm/drm_edid_load.c
> @@ -196,26 +196,43 @@ struct edid *
>  drm_load_edid_firmware(struct drm_connector *connector)
>  {
>  	char *connector_name = drm_get_connector_name(connector);
> -	char *edidname = edid_firmware, *last, *colon;
> -	struct edid *edid;
> +	char *edidname, *last, *colon;
> +	struct edid *edid = NULL;
> +	char *next, *mem;
> +	bool many = false;
>  
> -	if (*edidname == '\0')
> +	if (*edid_firmware == '\0')
>  		return NULL;
>  
> -	colon = strchr(edidname, ':');
> -	if (colon != NULL) {
> -		if (strncmp(connector_name, edidname, colon - edidname))
> -			return NULL;
> -		edidname = colon + 1;
> -		if (*edidname == '\0')
> -			return NULL;
> +	edidname = mem = kstrndup(edid_firmware, PATH_MAX, GFP_KERNEL);
> +	do {

Missing NULL check of edidname.


Takashi

> +		next = strchr(edidname, ';');
> +		if (next)
> +			*(next++) = '\0';
> +		colon = strchr(edidname, ':');
> +		if (colon == NULL) {
> +			if (next || many)
> +				edidname = NULL;
> +			break;
> +		} else if (!strncmp(connector_name, edidname, colon - edidname)) {
> +			edidname = colon + 1;
> +			break;
> +		}
> +		edidname = next;
> +		many = true;
> +	} while (edidname);
> +
> +	if (edidname && *edidname != '\0') {
> +		last = edidname + strlen(edidname) - 1;
> +		if (*last == '\n')
> +			*last = '\0';
> +
> +		if (strlen(edidname) > 0)
> +			edid = edid_load(connector, edidname, connector_name);
>  	}
>  
> -	last = edidname + strlen(edidname) - 1;
> -	if (*last == '\n')
> -		*last = '\0';
> +	kfree(mem);
>  
> -	edid = edid_load(connector, edidname, connector_name);
>  	if (IS_ERR_OR_NULL(edid))
>  		return NULL;
>  
> -- 
> 1.7.7
> 

[PATCH] drm/exynos: fix memory leak: free EDID block

[Egbert Eich]

drm_get_edid() returns a pointer to an EDID block. The caller
is responsible to free this pointer itself.
Here the pointer gets assigned to the local variable raw_edid.
Therefore it should be freed before the variable goes out of
scope.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/exynos/exynos_hdmi.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index 2c115f8..bc87bca 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -1293,6 +1293,7 @@ static int hdmi_get_edid(void *ctx, struct drm_connector *connector,
 		DRM_DEBUG_KMS("%s : width[%d] x height[%d]\n",
 			(hdata->dvi_mode ? "dvi monitor" : "hdmi monitor"),
 			raw_edid->width_cm, raw_edid->height_cm);
+		kfree(raw_edid);
 	} else {
 		return -ENODEV;
 	}
-- 
1.7.7

Re: [PATCH] drm/exynos: fix memory leak: free EDID block

[Sean Paul]

On Tue, Nov 20, 2012 at 4:30 AM, Egbert Eich <eich@suse.de> wrote:
> drm_get_edid() returns a pointer to an EDID block. The caller
> is responsible to free this pointer itself.
> Here the pointer gets assigned to the local variable raw_edid.
> Therefore it should be freed before the variable goes out of
> scope.
>
> Signed-off-by: Egbert Eich <eich@suse.de>
> ---
>  drivers/gpu/drm/exynos/exynos_hdmi.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
> index 2c115f8..bc87bca 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
> @@ -1293,6 +1293,7 @@ static int hdmi_get_edid(void *ctx, struct drm_connector *connector,
>                 DRM_DEBUG_KMS("%s : width[%d] x height[%d]\n",
>                         (hdata->dvi_mode ? "dvi monitor" : "hdmi monitor"),
>                         raw_edid->width_cm, raw_edid->height_cm);
> +               kfree(raw_edid);

This will actually cause the memory to be freed twice.

The reason this happens is drm_get_edid attaches this to
connector->display_info.raw_edid, which is then freed in the
exynos_drm_connector function that gets the edid.

The whole thing is ugly, and needs to be revised. I've uploaded a
patch to refactor this against the chromium tree, but haven't yet
rebased against upstream. See
https://gerrit.chromium.org/gerrit/#/c/38406/

For now, please drop this patch.

Sean

>         } else {
>                 return -ENODEV;
>         }
> --
> 1.7.7
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/exynos: fix memory leak: free EDID block

[Egbert Eich]

Sean Paul writes:
 > On Tue, Nov 20, 2012 at 4:30 AM, Egbert Eich <eich@suse.de> wrote:
 > > drm_get_edid() returns a pointer to an EDID block. The caller
 > > is responsible to free this pointer itself.
 > > Here the pointer gets assigned to the local variable raw_edid.
 > > Therefore it should be freed before the variable goes out of
 > > scope.
 > >
 > > Signed-off-by: Egbert Eich <eich@suse.de>
 > > ---
 > >  drivers/gpu/drm/exynos/exynos_hdmi.c |    1 +
 > >  1 files changed, 1 insertions(+), 0 deletions(-)
 > >
 > > diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
 > > index 2c115f8..bc87bca 100644
 > > --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
 > > +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
 > > @@ -1293,6 +1293,7 @@ static int hdmi_get_edid(void *ctx, struct drm_connector *connector,
 > >                 DRM_DEBUG_KMS("%s : width[%d] x height[%d]\n",
 > >                         (hdata->dvi_mode ? "dvi monitor" : "hdmi monitor"),
 > >                         raw_edid->width_cm, raw_edid->height_cm);
 > > +               kfree(raw_edid);
 > 
 > This will actually cause the memory to be freed twice.
 > 
 > The reason this happens is drm_get_edid attaches this to
 > connector->display_info.raw_edid, which is then freed in the
 > exynos_drm_connector function that gets the edid.

No, the raw_edid member is gone from struct drm_display_info.
All other drivers free the edid data returned by drm_get_edid() 
themselves.
I would actually prefer if the edid data would be freed by
drm_connector_destroy rather than by the drivers as this would
allow us to cache an edid without creating copies to pass to
the drivers. However some drivers store the pointer to returned 
edid block somewhere in their own data structures, so if we
do this it can easily get out of sync when we reread the
edid and it has changed for whatever reason in which case
the driver will have a stale and invalid pointer.

 > 
 > The whole thing is ugly, and needs to be revised. I've uploaded a
 > patch to refactor this against the chromium tree, but haven't yet
 > rebased against upstream. See
 > https://gerrit.chromium.org/gerrit/#/c/38406/
 > 
 > For now, please drop this patch.
 > 

I've looked at the code in the exynos driver in the drm_next
branch - there's nothing that destroys this edid data any more.

Cheers,
	Egbert.

Re: [PATCH] drm/exynos: fix memory leak: free EDID block

[Sean Paul]

On Tue, Nov 20, 2012 at 3:29 PM, Egbert Eich <eich@suse.com> wrote:
> Sean Paul writes:
>  > On Tue, Nov 20, 2012 at 4:30 AM, Egbert Eich <eich@suse.de> wrote:
>  > > drm_get_edid() returns a pointer to an EDID block. The caller
>  > > is responsible to free this pointer itself.
>  > > Here the pointer gets assigned to the local variable raw_edid.
>  > > Therefore it should be freed before the variable goes out of
>  > > scope.
>  > >
>  > > Signed-off-by: Egbert Eich <eich@suse.de>

Reviewed-by: Sean Paul <seanpaul@chromium.org>

>  > > ---
>  > >  drivers/gpu/drm/exynos/exynos_hdmi.c |    1 +
>  > >  1 files changed, 1 insertions(+), 0 deletions(-)
>  > >
>  > > diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
>  > > index 2c115f8..bc87bca 100644
>  > > --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
>  > > +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
>  > > @@ -1293,6 +1293,7 @@ static int hdmi_get_edid(void *ctx, struct drm_connector *connector,
>  > >                 DRM_DEBUG_KMS("%s : width[%d] x height[%d]\n",
>  > >                         (hdata->dvi_mode ? "dvi monitor" : "hdmi monitor"),
>  > >                         raw_edid->width_cm, raw_edid->height_cm);
>  > > +               kfree(raw_edid);
>  >
>  > This will actually cause the memory to be freed twice.
>  >
>  > The reason this happens is drm_get_edid attaches this to
>  > connector->display_info.raw_edid, which is then freed in the
>  > exynos_drm_connector function that gets the edid.
>
> No, the raw_edid member is gone from struct drm_display_info.
> All other drivers free the edid data returned by drm_get_edid()
> themselves.

A ha! I should have checked first, my apologies.

> I would actually prefer if the edid data would be freed by
> drm_connector_destroy rather than by the drivers as this would
> allow us to cache an edid without creating copies to pass to
> the drivers. However some drivers store the pointer to returned
> edid block somewhere in their own data structures, so if we
> do this it can easily get out of sync when we reread the
> edid and it has changed for whatever reason in which case
> the driver will have a stale and invalid pointer.
>

Agreed, that would be nice.

>  >
>  > The whole thing is ugly, and needs to be revised. I've uploaded a
>  > patch to refactor this against the chromium tree, but haven't yet
>  > rebased against upstream. See
>  > https://gerrit.chromium.org/gerrit/#/c/38406/
>  >
>  > For now, please drop this patch.
>  >
>
> I've looked at the code in the exynos driver in the drm_next
> branch - there's nothing that destroys this edid data any more.
>

Yep, right you are. Your patch does the right thing, thanks for
setting me straight :)

I hope we won't need to live with this double-allocation + copy for too long.

Sean



> Cheers,
>         Egbert.

Re: [PATCH] drm/exynos: fix memory leak: free EDID block

[Inki Dae]

[-- Attachment #1.1: Type: text/plain, Size: 1373 bytes --]

Ok, I modifed the subject of your patch like below and applied.

drm/exynos: fix memory lean to EDID block


Thanks,
Inki Dae

2012/11/20 Egbert Eich <eich@suse.de>

> drm_get_edid() returns a pointer to an EDID block. The caller
> is responsible to free this pointer itself.
> Here the pointer gets assigned to the local variable raw_edid.
> Therefore it should be freed before the variable goes out of
> scope.
>
> Signed-off-by: Egbert Eich <eich@suse.de>
> ---
>  drivers/gpu/drm/exynos/exynos_hdmi.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c
> b/drivers/gpu/drm/exynos/exynos_hdmi.c
> index 2c115f8..bc87bca 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
> @@ -1293,6 +1293,7 @@ static int hdmi_get_edid(void *ctx, struct
> drm_connector *connector,
>                 DRM_DEBUG_KMS("%s : width[%d] x height[%d]\n",
>                         (hdata->dvi_mode ? "dvi monitor" : "hdmi monitor"),
>                         raw_edid->width_cm, raw_edid->height_cm);
> +               kfree(raw_edid);
>         } else {
>                 return -ENODEV;
>         }
> --
> 1.7.7
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

[-- Attachment #1.2: Type: text/html, Size: 1998 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 00/18] DRM/KMS/EDID: Various EDID handling related fixes.

[Egbert Eich]

The patches have been reordered and the changes suggested by
Takashi Iwai have been worked in.

Egbert Eich (18):

1. Make error handling of EDID extension blocks a bit more fault
   tolorant:
   * Don't fail when EDID extension blocks cannot be read or memory
     cannot be allocated.
   * Don't read EDID extension blocks there are no blocks to expect,
     either for EDID versions < 1.3
     or when block 3 is equal to the base block (in which case
     the monitor is not EDDC capable and doesn't accept the segment
     address.

  DRM/KMS/EDID: Mask out Segment Bits when calculating Offset.
  DRM/KMS/EDID: 0x7e -> EDID_EXTENSION_FLAG_OFFSET (v2)
  DRM/KMS/EDID: Return Base EDID block if reading EEDID Blocks fails (v2)
  DRM/KMS/EDID: Don't fail when failing to allocate memory for EDID extensions.
  DRM/KMS/EDID: Test EDDC if EDID announces more than one Extension Block (v2)
  DRM/KMS/EDID: Don't expect extension blocks for EDID Versions < 1.3.
  DRM/KMS/EDID: Don't reallocate EDID blob when size has shrunk.

2. Fix EDID block maps:
   We skip invalid EEDID blocks, if we do so however we should fix
   the block maps to keep the EDID consistent.

  DRM/KMS/EDID: Fix up EEDID Map Blogs if Extension Block Count has changed (v2)

3. Improve handling of 'firmware'-supplied EDIDs:
   * Move EDID loading from a helper level to DRM core.
   * If there is a 'firmware'-supplied EDID matching the connector
     supply it whenever drm_get_edid() is called.
   * Allow to specify firmware for multiple connectors.
   * Use the same EDID fault handling as for DDC read EDIDs.

  DRM/KMS/EDID: Move drm_edid_load.o to drm.ko (v2)
  DRM/KMS/EDID: Feed 'firmware' supplied EDID blocks whenever the EDID is read (v2)
  DRM/KMS/EDID: Allow for multiple Connectors when specifying
    'firmware'-EDID Files (v2)
  DRM/KMS/EDID: Use Extension Block Fixup Code also for 'firmware' EDID (v2)

4. Cache EDIDs with extension blocks.
   I2C transfer of EDIDs with several extension blocks can be quite
   time consuming. In many cases the EDID has not changed for a
   given connector since the last read. We detect this by comparing
   data from the base block. If a match is found we copy the EDID
   data from cache. This way only the base block needs to be transferred.

  DRM/KMS/EDID: Cache EDID blobs with extensions (v2)

5. Consolidate EDID fatal error handling:
   Store information about the error status of the last DDC read in the
   drm_connector structure.
   It combines the handling of all zero EDIDs (needed by the radeon
   driver) and the suppression of consecutive logging of the same
   EDID error. It will log again when the error pattern has changed
   (for inststance if the monitor has been exchanged).

  DRM/KMS/EDID: Consolidate EDID Error Handling (v2)

6. Move EDID-related functions to drm_edid.h
   The handling of EDID related DRM functions seemed to be a bit arbitrary:
   although drm_edid.h had been created, new functions were often added to
   drm_crtc.h

  DRM/KMS/ast: Include drm_edid.h in file using drm_get_edid().
  DRM/KMS/gma500: Include drm_edid.h in file using drm_get_edid().
  DRM/KMS/mgag200: Include drm_edid.h in file using drm_get_edid().
  DRM/KMS/EDID: Move EDID related Functions to drm_edid.h.

 drivers/gpu/drm/Kconfig                    |    3 +-
 drivers/gpu/drm/Makefile                   |    2 +-
 drivers/gpu/drm/ast/ast_mode.c             |    1 +
 drivers/gpu/drm/drm_crtc.c                 |    1 +
 drivers/gpu/drm/drm_crtc_helper.c          |    6 +-
 drivers/gpu/drm/drm_edid.c                 |  333 ++++++++++++++++++++++++----
 drivers/gpu/drm/drm_edid_load.c            |  115 ++++------
 drivers/gpu/drm/gma500/cdv_intel_dp.c      |    1 +
 drivers/gpu/drm/gma500/oaktrail_lvds.c     |    1 +
 drivers/gpu/drm/gma500/psb_intel_modes.c   |    1 +
 drivers/gpu/drm/mgag200/mgag200_mode.c     |    1 +
 drivers/gpu/drm/radeon/radeon_connectors.c |    2 +-
 include/drm/drm_crtc.h                     |   13 +-
 include/drm/drm_edid.h                     |   25 ++-
 14 files changed, 370 insertions(+), 135 deletions(-)

-- 
1.7.7

[PATCH v2 01/18] DRM/KMS/EDID: Mask out Segment Bits when calculating Offset.

[Egbert Eich]

This patch is a bit cosmetic as the variable size will truncate
the start address anyway but for readability it should be made
explicite that the lowest bit in the EDID block number determines
the I2C start address.

Signed-off-by: Egbert Eich <eich@suse.com>
---
 drivers/gpu/drm/drm_edid.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index fadcd44..049fa52 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -258,7 +258,7 @@ static int
 drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
 		      int block, int len)
 {
-	unsigned char start = block * EDID_LENGTH;
+	unsigned char start = (block & 0x01) * EDID_LENGTH;
 	unsigned char segment = block >> 1;
 	unsigned char xfers = segment ? 3 : 2;
 	int ret, retries = 5;
-- 
1.7.7

[PATCH v2 02/18] DRM/KMS/EDID: 0x7e -> EDID_EXTENSION_FLAG_OFFSET (v2)

[Egbert Eich]

v2: Use offsetof().

Signed-off-by: Egbert Eich <eich@suse.com>
---
 drivers/gpu/drm/drm_edid.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 049fa52..9e64069 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -43,6 +43,8 @@
 #define EDID_STD_TIMINGS 8
 #define EDID_DETAILED_TIMINGS 4
 
+#define EDID_EXTENSION_FLAG_OFFSET  offsetof(struct edid, extensions)
+#define EDID_CHECKSUM_OFFSET	    offsetof(struct edid, checksum)
 /*
  * EDID blocks out in the wild have a variety of bugs, try to collect
  * them here (note that userspace may work around broken monitors first,
@@ -341,15 +343,15 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 		goto carp;
 
 	/* if there's no extensions, we're done */
-	if (block[0x7e] == 0)
+	if (block[EDID_EXTENSION_FLAG_OFFSET] == 0)
 		return block;
 
-	new = krealloc(block, (block[0x7e] + 1) * EDID_LENGTH, GFP_KERNEL);
+	new = krealloc(block, (block[EDID_EXTENSION_FLAG_OFFSET] + 1) * EDID_LENGTH, GFP_KERNEL);
 	if (!new)
 		goto out;
 	block = new;
 
-	for (j = 1; j <= block[0x7e]; j++) {
+	for (j = 1; j <= block[EDID_EXTENSION_FLAG_OFFSET]; j++) {
 		for (i = 0; i < 4; i++) {
 			if (drm_do_probe_ddc_edid(adapter,
 				  block + (valid_extensions + 1) * EDID_LENGTH,
@@ -366,9 +368,9 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 			 drm_get_connector_name(connector), j);
 	}
 
-	if (valid_extensions != block[0x7e]) {
-		block[EDID_LENGTH-1] += block[0x7e] - valid_extensions;
-		block[0x7e] = valid_extensions;
+	if (valid_extensions != block[EDID_EXTENSION_FLAG_OFFSET]) {
+		block[EDID_CHECKSUM_OFFSET] += block[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions;
+		block[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions;
 		new = krealloc(block, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
 		if (!new)
 			goto out;
@@ -601,7 +603,7 @@ drm_for_each_detailed_block(u8 *raw_edid, detailed_cb *cb, void *closure)
 	for (i = 0; i < EDID_DETAILED_TIMINGS; i++)
 		cb(&(edid->detailed_timings[i]), closure);
 
-	for (i = 1; i <= raw_edid[0x7e]; i++) {
+	for (i = 1; i <= raw_edid[EDID_EXTENSION_FLAG_OFFSET]; i++) {
 		u8 *ext = raw_edid + (i * EDID_LENGTH);
 		switch (*ext) {
 		case CEA_EXT:
-- 
1.7.7

[PATCH v2 03/18] DRM/KMS/EDID: Return Base EDID block if reading EEDID Blocks fails (v2)

[Egbert Eich]

If I2C readout fails for an extension block but we have read a
valid base block, don't fail completely but at least return the
base block.

v2: Make goto target names more telling.

Signed-off-by: Egbert Eich <eich@suse.com>
---
 drivers/gpu/drm/drm_edid.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 9e64069..043ba42 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -355,8 +355,10 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 		for (i = 0; i < 4; i++) {
 			if (drm_do_probe_ddc_edid(adapter,
 				  block + (valid_extensions + 1) * EDID_LENGTH,
-				  j, EDID_LENGTH))
-				goto out;
+				  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)) {
 				valid_extensions++;
 				break;
@@ -368,6 +370,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 			 drm_get_connector_name(connector), j);
 	}
 
+done_fix_extension_count:
 	if (valid_extensions != block[EDID_EXTENSION_FLAG_OFFSET]) {
 		block[EDID_CHECKSUM_OFFSET] += block[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions;
 		block[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions;
-- 
1.7.7

[PATCH v2 04/18] DRM/KMS/EDID: Don't fail when failing to allocate memory for EDID extensions.

[Egbert Eich]

When we fail to allocate space for EDID extensions we should just
warn, fix up the EDID block count and return the base block instead
of failing.

Signed-off-by: Egbert Eich <eich@suse.com>
---
 drivers/gpu/drm/drm_edid.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 043ba42..a952cfe 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -347,8 +347,11 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 		return block;
 
 	new = krealloc(block, (block[EDID_EXTENSION_FLAG_OFFSET] + 1) * EDID_LENGTH, GFP_KERNEL);
-	if (!new)
-		goto out;
+	if (!new) {
+		dev_warn(connector->dev->dev, "%s: cannot allocate memory for %d EDID blocks: truncating.\n",
+			 drm_get_connector_name(connector), block[EDID_EXTENSION_FLAG_OFFSET] + 1);
+		goto done_fix_extension_count;
+	}
 	block = new;
 
 	for (j = 1; j <= block[EDID_EXTENSION_FLAG_OFFSET]; j++) {
-- 
1.7.7

[PATCH v2 05/18] DRM/KMS/EDID: Test EDDC if EDID announces more than one Extension Block (v2)

[Egbert Eich]

There are displays which announce EDID extension blocks in the
Extension Flag of the EDID base block although they are not EDDC
capable (ie. take a segment address at I2C slave address 0x30).
We test this by looking for an EDID header which is only possible
in the base block.
If the segment address is not taken into account, this block will
be identical to the base block in which case we stop reading further
EEDID blocks, correct the extension flag and just return the base
block.

v2: Split up EDID fixup code into separate commit.

Signed-off-by: Egbert Eich <eich@suse.com>
---
 drivers/gpu/drm/drm_edid.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index a952cfe..5a0e331 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -364,6 +364,19 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 			}
 			if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) {
 				valid_extensions++;
+				/* Test if base block announced extension blocks although
+				 * display is not EDDC capable.
+				 */
+				if (j == 2) {
+					int k;
+					for (k = 0; k < sizeof(edid_header); k++)
+						if (block[(EDID_LENGTH * 2) + k] != edid_header[k])
+							break;
+					if (k == sizeof(edid_header)) {
+						valid_extensions = 0;
+						goto done_fix_extension_count;
+					}
+				}
 				break;
 			}
 		}
-- 
1.7.7

[PATCH v2 06/18] DRM/KMS/EDID: Don't expect extension blocks for EDID Versions < 1.3.

[Egbert Eich]

EDID extension blogs are only expected for EDIDs version 1.3 or higher.
If an EDID with a lower version is found fix the block count in the
extension flags and return the base block.
This should help to avoid issues with older displays with broken
DDC implementations.

Signed-off-by: Egbert Eich <eich@suse.com>
---
 drivers/gpu/drm/drm_edid.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 5a0e331..da2f7fa 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -45,6 +45,8 @@
 
 #define EDID_EXTENSION_FLAG_OFFSET  offsetof(struct edid, extensions)
 #define EDID_CHECKSUM_OFFSET	    offsetof(struct edid, checksum)
+#define EDID_VERSION_MAJOR_OFFSET   offsetof(struct edid, version)
+#define EDID_VERSION_MINOR_OFFSET   offsetof(struct edid, revision)
 /*
  * EDID blocks out in the wild have a variety of bugs, try to collect
  * them here (note that userspace may work around broken monitors first,
@@ -346,6 +348,10 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 	if (block[EDID_EXTENSION_FLAG_OFFSET] == 0)
 		return block;
 
+	/* don't expect extension blocks in EDID Versions < 1.3: return base block with correct extension flag */
+	if (block[EDID_VERSION_MINOR_OFFSET] < 3)
+		goto done_fix_extension_count;
+
 	new = krealloc(block, (block[EDID_EXTENSION_FLAG_OFFSET] + 1) * EDID_LENGTH, GFP_KERNEL);
 	if (!new) {
 		dev_warn(connector->dev->dev, "%s: cannot allocate memory for %d EDID blocks: truncating.\n",
-- 
1.7.7

[PATCH v2 07/18] DRM/KMS/EDID: Don't reallocate EDID blob when size has shrunk.

[Egbert Eich]

valid_extensions (the number of EDID extensions found to be valid)
can never be > block[EDID_EXTENSION_FLAG_OFFSET].
There is no point of reallocating the block in this case: the
extra blocks at the end of the EDID structure will not hurt,
also the implementation of krealloc() will just return the same
block.

Signed-off-by: Egbert Eich <eich@suse.com>
---
 drivers/gpu/drm/drm_edid.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index da2f7fa..0fe61fb 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -396,10 +396,6 @@ done_fix_extension_count:
 	if (valid_extensions != block[EDID_EXTENSION_FLAG_OFFSET]) {
 		block[EDID_CHECKSUM_OFFSET] += block[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions;
 		block[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions;
-		new = krealloc(block, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
-		if (!new)
-			goto out;
-		block = new;
 	}
 
 	return block;
-- 
1.7.7

[PATCH v2 08/18] DRM/KMS/EDID: Fix up EEDID Map Blogs if Extension Block Count has changed (v2)

[Egbert Eich]

EEDID v1.3 mandates map blogs if more than one EDID extension block
is used while in v1.4 they are optional.
If the extension count has been changed (because some extension
blocks were not readable) those map blocks need fixing.
In case of v1.4 or higher we simply eliminate all map blogs as
this is less time consuming. For v1.3 we scrap any exsisting map
blocks and recreate them from scratch.

v2: Fixed conflits due to reordering of commits.

Signed-off-by: Egbert Eich <eich@suse.com>
---
 drivers/gpu/drm/drm_edid.c |   89 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 89 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 0fe61fb..9b298fc 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -47,6 +47,7 @@
 #define EDID_CHECKSUM_OFFSET	    offsetof(struct edid, checksum)
 #define EDID_VERSION_MAJOR_OFFSET   offsetof(struct edid, version)
 #define EDID_VERSION_MINOR_OFFSET   offsetof(struct edid, revision)
+#define EEDID_BLOCK_MAP_FLAG      0xF0
 /*
  * EDID blocks out in the wild have a variety of bugs, try to collect
  * them here (note that userspace may work around broken monitors first,
@@ -320,6 +321,92 @@ static bool drm_edid_is_zero(u8 *in_edid, int length)
 	return true;
 }
 
+static void
+fix_map(u8 *block, int cnt)
+{
+	int i;
+	u8 csum;
+
+	if (--cnt > 127)
+		cnt = 127;
+	memset(block, 0, 128);
+	block[0] = EEDID_BLOCK_MAP_FLAG;
+	csum = block[0];
+
+	for (i = 1; i < cnt; i++) {
+		block[i] = block[i * EDID_LENGTH];
+		csum += block[i];
+	}
+	block[127] = (u8)(0x100 - csum);
+}
+
+static int
+fixup_blockmaps(u8 **blockp, int eblock_cnt)
+{
+	int i;
+	u8 *block = *blockp;
+
+	if (block[EDID_VERSION_MAJOR_OFFSET] > 1)
+		return 0;
+	if (block[EDID_VERSION_MINOR_OFFSET] < 3)
+		return 0;
+	if (eblock_cnt == 1) {
+		if (block[EDID_LENGTH] == EEDID_BLOCK_MAP_FLAG)
+			return 0;
+		else
+			return 1;
+	}
+	if (block[EDID_VERSION_MINOR_OFFSET] >= 4) {
+		/* blockmaps are optional: simply toss them */
+		for (i = 1; i <= eblock_cnt; i++) {
+			if (block[i * EDID_LENGTH] == EEDID_BLOCK_MAP_FLAG) {
+				if (i < eblock_cnt)
+					memmove(&block[i * EDID_LENGTH],
+						&block[(i + 1) * EDID_LENGTH],
+						(eblock_cnt-i) * EDID_LENGTH);
+				i--;
+				eblock_cnt--;
+			}
+		}
+	} else {
+		int total_cnt = block[EDID_EXTENSION_FLAG_OFFSET];
+		for (i = 1; i <= eblock_cnt; i++) {
+			if (block[i * EDID_LENGTH] == EEDID_BLOCK_MAP_FLAG) {
+				if (i == 1 || i == 128) /* correct map block locations */
+					continue;
+				if (i < eblock_cnt)
+					memmove(&block[i * EDID_LENGTH],
+						&block[(i + 1) * EDID_LENGTH],
+						(eblock_cnt-i) * EDID_LENGTH);
+				i--;
+				eblock_cnt--;
+				continue;
+			} else if (i == 1 || i == 128) {
+				if (eblock_cnt >= total_cnt) {
+					u8 *tmp_p;
+					tmp_p = krealloc(block, (eblock_cnt + 2) * EDID_LENGTH, GFP_KERNEL);
+					if (!tmp_p)
+						return eblock_cnt;
+					*blockp = block = tmp_p;
+					total_cnt = eblock_cnt + 1;
+				}
+				eblock_cnt++;
+				memmove(&block[(i + 1) * EDID_LENGTH],
+					&block[i * EDID_LENGTH],
+					(eblock_cnt-i) * EDID_LENGTH);
+			}
+		}
+		fix_map(&block[EDID_LENGTH], eblock_cnt);
+
+		if (eblock_cnt == 129)
+			return 128;
+
+		if (eblock_cnt > 129)
+			fix_map(&block[EDID_LENGTH * 128], eblock_cnt - 127);
+	}
+	return eblock_cnt;
+}
+
 static u8 *
 drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 {
@@ -394,6 +481,8 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 
 done_fix_extension_count:
 	if (valid_extensions != block[EDID_EXTENSION_FLAG_OFFSET]) {
+		if (valid_extensions)
+			valid_extensions = fixup_blockmaps(&block, valid_extensions);
 		block[EDID_CHECKSUM_OFFSET] += block[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions;
 		block[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions;
 	}
-- 
1.7.7

[PATCH v2 09/18] DRM/KMS/EDID: Move drm_edid_load.o to drm.ko (v2)

[Egbert Eich]

EDIDs are an integral concept of connectors, connectors are a concept
of drm core also drm_edid.o is already part of this drm core.
Overridden, 'firmware-supplied' EDIDs should be treated exactly the
same as device supplied ones.
Therefore move drm_edid_load from the helper level to the drm core.

v2: a. Also adapt Kconfig
    b. Add missing 'select FW_LOADER'

Signed-off-by: Egbert Eich <eich@suse.com>
---
 drivers/gpu/drm/Kconfig  |    3 ++-
 drivers/gpu/drm/Makefile |    2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 18321b68b..4bc239b 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -36,7 +36,8 @@ config DRM_KMS_HELPER
 
 config DRM_LOAD_EDID_FIRMWARE
 	bool "Allow to specify an EDID data set instead of probing for it"
-	depends on DRM_KMS_HELPER
+	depends on DRM
+	select FW_LOADER
 	help
 	  Say Y here, if you want to use EDID data to be loaded from the
 	  /lib/firmware directory or one of the provided built-in
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 2ff5cef..76ca269 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -16,11 +16,11 @@ drm-y       :=	drm_auth.o drm_buffer.o drm_bufs.o drm_cache.o \
 
 drm-$(CONFIG_COMPAT) += drm_ioc32.o
 drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
+drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
 
 drm-usb-y   := drm_usb.o
 
 drm_kms_helper-y := drm_fb_helper.o drm_crtc_helper.o drm_dp_i2c_helper.o
-drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
 drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
 
 obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
-- 
1.7.7

[PATCH v2 10/18] DRM/KMS/EDID: Feed 'firmware' supplied EDID blocks whenever the EDID is read (v2)

[Egbert Eich]

Firmware supplied EDIDs where fed in in drm_helper_probe_single_connector_modes()
in place of calling the driver supplied get_modes() function.
This has two problems:
1. Drivers don't call drm_get_edid() only from within get_modes().
2. The get_modes() replacement in drm_load_edid_firmware() provided only
   the least common denominator of what driver provided get_modes() callbacks
   do.
This patch now supplies a user provided 'firmware' EDID whenever drm_get_edid()
is called if one is available. The user supplied EDID is thus used the same way
as a display provided one (except for detecting the presence of a device thru
EDID). Also it does so not on the helper level any more, the possibility for
EDID loading now happens on the DRM KMS core level.

v2: Fixed formatting and conflicts due to reordering of commits.

Signed-off-by: Egbert Eich <eich@suse.com>
---
 drivers/gpu/drm/drm_crtc_helper.c |    6 +-----
 drivers/gpu/drm/drm_edid.c        |    7 +++++++
 drivers/gpu/drm/drm_edid_load.c   |   22 +++++++++-------------
 include/drm/drm_edid.h            |    4 +++-
 4 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 1227adf..bc99595 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -119,11 +119,7 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 		goto prune;
 	}
 
-#ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
-	count = drm_load_edid_firmware(connector);
-	if (count == 0)
-#endif
-		count = (*connector_funcs->get_modes)(connector);
+	count = (*connector_funcs->get_modes)(connector);
 
 	if (count == 0 && connector->status == connector_status_connected)
 		count = drm_add_modes_noedid(connector, 1024, 768);
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 9b298fc..8239c42 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -414,6 +414,13 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 	u8 *block, *new;
 	bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
 
+#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)
+		return block;
+#endif
+
 	if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
 		return NULL;
 
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 38d3943..748f63b 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -114,7 +114,7 @@ static u8 generic_edid[GENERIC_EDIDS][128] = {
 	},
 };
 
-static u8 *edid_load(struct drm_connector *connector, char *name,
+static struct edid *edid_load(struct drm_connector *connector, char *name,
 			char *connector_name)
 {
 	const struct firmware *fw;
@@ -222,36 +222,32 @@ out:
 	return edid;
 }
 
-int drm_load_edid_firmware(struct drm_connector *connector)
+struct edid *
+drm_load_edid_firmware(struct drm_connector *connector)
 {
 	char *connector_name = drm_get_connector_name(connector);
 	char *edidname = edid_firmware, *last, *colon;
-	int ret;
 	struct edid *edid;
 
 	if (*edidname == '\0')
-		return 0;
+		return NULL;
 
 	colon = strchr(edidname, ':');
 	if (colon != NULL) {
 		if (strncmp(connector_name, edidname, colon - edidname))
-			return 0;
+			return NULL;
 		edidname = colon + 1;
 		if (*edidname == '\0')
-			return 0;
+			return NULL;
 	}
 
 	last = edidname + strlen(edidname) - 1;
 	if (*last == '\n')
 		*last = '\0';
 
-	edid = (struct edid *) edid_load(connector, edidname, connector_name);
+	edid = edid_load(connector, edidname, connector_name);
 	if (IS_ERR_OR_NULL(edid))
-		return 0;
+		return NULL;
 
-	drm_mode_connector_update_edid_property(connector, edid);
-	ret = drm_add_edid_modes(connector, edid);
-	kfree(edid);
-
-	return ret;
+	return edid;
 }
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 0cac551..c0a77bd 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -252,6 +252,8 @@ int drm_av_sync_delay(struct drm_connector *connector,
 		      struct drm_display_mode *mode);
 struct drm_connector *drm_select_eld(struct drm_encoder *encoder,
 				     struct drm_display_mode *mode);
-int drm_load_edid_firmware(struct drm_connector *connector);
+#ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
+struct edid *drm_load_edid_firmware(struct drm_connector *connector);
+#endif
 
 #endif /* __DRM_EDID_H__ */
-- 
1.7.7

[PATCH v2 11/18] DRM/KMS/EDID: Allow for multiple Connectors when specifying 'firmware'-EDID Files (v2)

[Egbert Eich]

So far it was only possible to load an EDID for a single connector (unless
no connector was specified at all in which case the same EDID file was used
for all).
This patch extends the EDID loader so that EDID files can be specified for
more than one connector. A semicolon is used to separate the different connector
sections.
The option now looks like this:
   edid_firmware="[<connector_0>:]<edid_file_0>[;<connector_n>:<edid_file_1>]..."

v2: Check for edidname != NULL before entering loop.

Signed-off-by: Egbert Eich <eich@suse.com>
---
 drivers/gpu/drm/drm_edid_load.c |   45 ++++++++++++++++++++++++++------------
 1 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 748f63b..9c15c4a 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -226,26 +226,43 @@ struct edid *
 drm_load_edid_firmware(struct drm_connector *connector)
 {
 	char *connector_name = drm_get_connector_name(connector);
-	char *edidname = edid_firmware, *last, *colon;
-	struct edid *edid;
+	char *edidname, *last, *colon;
+	struct edid *edid = NULL;
+	char *next, *mem;
+	bool many = false;
 
-	if (*edidname == '\0')
+	if (*edid_firmware == '\0')
 		return NULL;
 
-	colon = strchr(edidname, ':');
-	if (colon != NULL) {
-		if (strncmp(connector_name, edidname, colon - edidname))
-			return NULL;
-		edidname = colon + 1;
-		if (*edidname == '\0')
-			return NULL;
+	edidname = mem = kstrndup(edid_firmware, PATH_MAX, GFP_KERNEL);
+	while (edidname) {
+		next = strchr(edidname, ';');
+		if (next)
+			*(next++) = '\0';
+		colon = strchr(edidname, ':');
+		if (colon == NULL) {
+			if (next || many)
+				edidname = NULL;
+			break;
+		} else if (!strncmp(connector_name, edidname, colon - edidname)) {
+			edidname = colon + 1;
+			break;
+		}
+		edidname = next;
+		many = true;
+	}
+
+	if (edidname && *edidname != '\0') {
+		last = edidname + strlen(edidname) - 1;
+		if (*last == '\n')
+			*last = '\0';
+
+		if (strlen(edidname) > 0)
+			edid = edid_load(connector, edidname, connector_name);
 	}
 
-	last = edidname + strlen(edidname) - 1;
-	if (*last == '\n')
-		*last = '\0';
+	kfree(mem);
 
-	edid = edid_load(connector, edidname, connector_name);
 	if (IS_ERR_OR_NULL(edid))
 		return NULL;
 
-- 
1.7.7

[PATCH v2 12/18] DRM/KMS/EDID: Use Extension Block Fixup Code also for 'firmware' EDID (v2)

[Egbert Eich]

in drm_edid.c there's now code to fix extension blockmaps if the number
of extensions has changed. This code also rearranges the EDID blocks.
Replace the exisiting EDID rearrange code with a call to this code.

v2: Make adjustments required by patch reordering, add missing memcpy().

Signed-off-by: Egbert Eich <eich@suse.com>
---
 drivers/gpu/drm/drm_edid.c      |   64 +++++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/drm_edid_load.c |   54 +++++++-------------------------
 include/drm/drm_edid.h          |    1 +
 3 files changed, 71 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 8239c42..a47fa7f 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -407,6 +407,18 @@ fixup_blockmaps(u8 **blockp, int eblock_cnt)
 	return eblock_cnt;
 }
 
+static void
+fixup_edid(u8 **blockp, int valid_extensions)
+{
+	if (valid_extensions != (*blockp)[EDID_EXTENSION_FLAG_OFFSET]) {
+		if (valid_extensions)
+			valid_extensions = fixup_blockmaps(blockp, valid_extensions);
+
+		(*blockp)[EDID_CHECKSUM_OFFSET] += (*blockp)[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions;
+		(*blockp)[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions;
+	}
+}
+
 static u8 *
 drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 {
@@ -487,12 +499,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 	}
 
 done_fix_extension_count:
-	if (valid_extensions != block[EDID_EXTENSION_FLAG_OFFSET]) {
-		if (valid_extensions)
-			valid_extensions = fixup_blockmaps(&block, valid_extensions);
-		block[EDID_CHECKSUM_OFFSET] += block[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions;
-		block[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions;
-	}
+	fixup_edid(&block, valid_extensions);
 
 	return block;
 
@@ -509,6 +516,51 @@ out:
 }
 
 /**
+ * Validate an entire EDID blob.
+ * \param connector: drm_connector struct of the used connector.
+ * \param blockp: pointer to address of an raw EDID data block.
+ * \param len: size if block in bytes.
+ *
+ * validate block and return corrected block in \param block.
+ * \return: number of valid extensions or -errno if unsuccessful.
+ */
+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);
+
+	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 (!ret) {
+		int cnt = 0;
+		n_blocks--;
+		if ((*blockp)[EDID_EXTENSION_FLAG_OFFSET] < n_blocks)
+			n_blocks = (*blockp)[EDID_EXTENSION_FLAG_OFFSET];
+
+		while (n_blocks--) {
+			cnt++;
+			if (drm_edid_block_valid(*blockp + cnt * EDID_LENGTH,
+						 valid_extensions + 1, print_bad_edid)) {
+				valid_extensions++;
+				if (cnt != valid_extensions)
+					memcpy(*blockp + valid_extensions * EDID_LENGTH,
+					       *blockp + cnt * EDID_LENGTH, EDID_LENGTH);
+			}
+		}
+		fixup_edid(blockp, valid_extensions);
+	} else
+		connector->bad_edid_counter++;
+	return ret;
+}
+
+/**
  * Probe DDC presence.
  *
  * \param adapter : i2c device adaptor
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 9c15c4a..8f26790 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -119,11 +119,10 @@ static struct edid *edid_load(struct drm_connector *connector, char *name,
 {
 	const struct firmware *fw;
 	struct platform_device *pdev;
-	u8 *fwdata = NULL, *edid, *new_edid;
+	u8 *fwdata = NULL;
+	struct edid *edid;
 	int fwsize, expected;
 	int builtin = 0, err = 0;
-	int i, valid_extensions = 0;
-	bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
 
 	pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
 	if (IS_ERR(pdev)) {
@@ -137,7 +136,7 @@ static struct edid *edid_load(struct drm_connector *connector, char *name,
 	platform_device_unregister(pdev);
 
 	if (err) {
-		i = 0;
+		int i = 0;
 		while (i < GENERIC_EDIDS && strcmp(name, generic_edid_name[i]))
 			i++;
 		if (i < GENERIC_EDIDS) {
@@ -174,49 +173,20 @@ static struct edid *edid_load(struct drm_connector *connector, char *name,
 	}
 	memcpy(edid, fwdata, fwsize);
 
-	if (!drm_edid_block_valid(edid, 0, print_bad_edid)) {
-		connector->bad_edid_counter++;
-		DRM_ERROR("Base block of EDID firmware \"%s\" is invalid ",
-		    name);
-		kfree(edid);
-		err = -EINVAL;
-		goto relfw_out;
-	}
-
-	for (i = 1; i <= edid[0x7e]; i++) {
-		if (i != valid_extensions + 1)
-			memcpy(edid + (valid_extensions + 1) * EDID_LENGTH,
-			    edid + i * EDID_LENGTH, EDID_LENGTH);
-		if (drm_edid_block_valid(edid + i * EDID_LENGTH, i, print_bad_edid))
-			valid_extensions++;
-	}
-
-	if (valid_extensions != edid[0x7e]) {
-		edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
-		DRM_INFO("Found %d valid extensions instead of %d in EDID data "
-		    "\"%s\" for connector \"%s\"\n", valid_extensions,
-		    edid[0x7e], name, connector_name);
-		edid[0x7e] = valid_extensions;
-		new_edid = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH,
-		    GFP_KERNEL);
-		if (new_edid == NULL) {
-			err = -ENOMEM;
-			kfree(edid);
-			goto relfw_out;
-		}
-		edid = new_edid;
-	}
-
-	DRM_INFO("Got %s EDID base block and %d extension%s from "
-	    "\"%s\" for connector \"%s\"\n", builtin ? "built-in" :
-	    "external", valid_extensions, valid_extensions == 1 ? "" : "s",
-	    name, connector_name);
+	err = drm_validate_edid_blob(connector, (u8 **)&edid, fwsize);
+	if (err < 0)
+		DRM_ERROR("EDID firmware \"%s\" is invalid ", name);
+	else
+		DRM_INFO("Got %s EDID base block and %d extension%s from "
+			 "\"%s\" for connector \"%s\"\n", builtin ? "built-in" :
+			 "external", edid->extensions, edid->extensions == 1 ? "" : "s",
+			 name, connector_name);
 
 relfw_out:
 	release_firmware(fw);
 
 out:
-	if (err)
+	if (err < 0)
 		return ERR_PTR(err);
 
 	return edid;
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index c0a77bd..53c619c 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -255,5 +255,6 @@ struct drm_connector *drm_select_eld(struct drm_encoder *encoder,
 #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
 struct edid *drm_load_edid_firmware(struct drm_connector *connector);
 #endif
+int drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len);
 
 #endif /* __DRM_EDID_H__ */
-- 
1.7.7

[PATCH v2 13/18] DRM/KMS/EDID: Cache EDID blobs with extensions (v2)

[Egbert Eich]

According the the VESA specs there can be up to 254 EEDID extension blocks.
Since we may read the EDID (including extensions) in 10 second intervals to
probe for display hotplugging (at least in cases where no hardware hotplug
detection exists) and I2C transfer is rather slow we may end up consuming
a considerable amount on CPU time for just that.
This patch caches the EDID block if it contains at least one extension.
To determine if the blocks match we only tranfer the base block, on a match
we use the cached data.

V2: Use kmemdup() instead of a kmalloc()/memcpy() combo,
    erase cache when reading a 'firmware'-supplied EDID or on error.

Signed-off-by: Egbert Eich <eich@suse.com>
---
 drivers/gpu/drm/drm_crtc.c |    1 +
 drivers/gpu/drm/drm_edid.c |   57 +++++++++++++++++++++++++++++++++++++------
 include/drm/drm_crtc.h     |    1 +
 include/drm/drm_edid.h     |    1 +
 4 files changed, 52 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 3533609..e283355 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -598,6 +598,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
 		drm_mode_remove(connector, mode);
 
 	mutex_lock(&dev->mode_config.mutex);
+	drm_cache_edid(connector, NULL);
 	drm_mode_object_put(dev, &connector->base);
 	list_del(&connector->head);
 	dev->mode_config.num_connector--;
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index a47fa7f..28b877c 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -419,6 +419,38 @@ fixup_edid(u8 **blockp, int valid_extensions)
 	}
 }
 
+static bool
+compare_get_edid_from_cache(struct drm_connector *connector, struct edid **edidp)
+{
+	if (connector->edid_cache &&
+	    connector->edid_cache->prod_code[0] == (*edidp)->prod_code[0] &&
+	    connector->edid_cache->prod_code[1] == (*edidp)->prod_code[1] &&
+	    connector->edid_cache->serial == (*edidp)->serial &&
+	    connector->edid_cache->input == (*edidp)->input) {
+		int size = (connector->edid_cache->extensions + 1) * EDID_LENGTH;
+		struct edid *new = kmemdup(connector->edid_cache, size, GFP_KERNEL);
+		if (!new)
+			return false;
+		DRM_DEBUG_KMS("Got EDID for %s from cache.\n", drm_get_connector_name(connector));
+		kfree(*edidp);
+		*edidp = new;
+		return true;
+	}
+	return false;
+}
+
+void
+drm_cache_edid(struct drm_connector *connector, struct edid *edid)
+{
+	struct edid *new = NULL;
+	kfree(connector->edid_cache);
+	if (edid) {
+		int size = (edid->extensions + 1) * EDID_LENGTH;
+		new = kmemdup(edid, size, GFP_KERNEL);
+	}
+	connector->edid_cache = new;
+}
+
 static u8 *
 drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 {
@@ -429,28 +461,30 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 #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)
+	if (block) {
+		drm_cache_edid(connector, NULL);
 		return block;
+	}
 #endif
 
 	if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
-		return NULL;
+		goto error;
 
 	/* base block fetch */
 	for (i = 0; i < 4; i++) {
 		if (drm_do_probe_ddc_edid(adapter, block, 0, EDID_LENGTH))
-			goto out;
+			goto error_free;
 		if (drm_edid_block_valid(block, 0, print_bad_edid))
 			break;
 		if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) {
 			connector->null_edid_counter++;
-			goto carp;
+			goto error_carp;
 		}
 	}
 	if (i == 4)
-		goto carp;
+		goto error_carp;
 
-	/* if there's no extensions, we're done */
+	/* if there are no extensions, we're done - don't bother caching */
 	if (block[EDID_EXTENSION_FLAG_OFFSET] == 0)
 		return block;
 
@@ -458,6 +492,10 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 	if (block[EDID_VERSION_MINOR_OFFSET] < 3)
 		goto done_fix_extension_count;
 
+	/* 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;
+
 	new = krealloc(block, (block[EDID_EXTENSION_FLAG_OFFSET] + 1) * EDID_LENGTH, GFP_KERNEL);
 	if (!new) {
 		dev_warn(connector->dev->dev, "%s: cannot allocate memory for %d EDID blocks: truncating.\n",
@@ -500,18 +538,21 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 
 done_fix_extension_count:
 	fixup_edid(&block, valid_extensions);
+	drm_cache_edid(connector, (valid_extensions > 0) ? (struct edid *)block : NULL);
 
 	return block;
 
-carp:
+error_carp:
 	if (print_bad_edid) {
 		dev_warn(connector->dev->dev, "%s: EDID block %d invalid.\n",
 			 drm_get_connector_name(connector), j);
 	}
 	connector->bad_edid_counter++;
 
-out:
+error_free:
 	kfree(block);
+error:
+	drm_cache_edid(connector, NULL);
 	return NULL;
 }
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 49dd8c2..6a1054c 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -601,6 +601,7 @@ struct drm_connector {
 	struct drm_encoder *encoder; /* currently active encoder */
 
 	/* EDID bits */
+	struct edid *edid_cache;
 	uint8_t eld[MAX_ELD_BYTES];
 	bool dvi_dual;
 	int max_tmds_clock;	/* in MHz */
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 53c619c..c880510 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -256,5 +256,6 @@ struct drm_connector *drm_select_eld(struct drm_encoder *encoder,
 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);
 
 #endif /* __DRM_EDID_H__ */
-- 
1.7.7

[PATCH v2 14/18] DRM/KMS/EDID: Consolidate EDID Error Handling (v2)

[Egbert Eich]

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.

Signed-off-by: Egbert Eich <eich@suse.com>
---
 drivers/gpu/drm/drm_edid.c                 |  115 +++++++++++++++++-----------
 drivers/gpu/drm/radeon/radeon_connectors.c |    2 +-
 include/drm/drm_crtc.h                     |    4 +-
 include/drm/drm_edid.h                     |   10 +++
 4 files changed, 81 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 28b877c..7d8363e 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,15 +233,23 @@ 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;
+}
+
+bool
+drm_edid_block_valid(u8 *raw_edid, int block, unsigned last_error_flags)
+{
+	if (!drm_edid_block_check_error(raw_edid, block, last_error_flags))
+		return true;
+	return false;
 }
 EXPORT_SYMBOL(drm_edid_block_valid);
 
@@ -241,7 +268,7 @@ bool drm_edid_is_valid(struct edid *edid)
 		return false;
 
 	for (i = 0; i <= edid->extensions; i++)
-		if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true))
+		if (drm_edid_block_check_error(raw + i * EDID_LENGTH, i, true))
 			return false;
 
 	return true;
@@ -310,17 +337,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 +472,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)
 		goto error_carp;
 
 	/* if there are no extensions, we're done - don't bother caching */
 	if (block[EDID_EXTENSION_FLAG_OFFSET] == 0)
-		return block;
+		goto done;
 
 	/* 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 +513,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) {
@@ -512,7 +531,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 				valid_extensions = 0;
 				goto done_fix_extension_count;
 			}
-			if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) {
+			if (!drm_edid_block_check_error(block + (valid_extensions + 1) * EDID_LENGTH, j, last_error_flags)) {
 				valid_extensions++;
 				/* Test if base block announced extension blocks although
 				 * display is not EDDC capable.
@@ -538,21 +557,22 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 
 done_fix_extension_count:
 	fixup_edid(&block, valid_extensions);
+done:
 	drm_cache_edid(connector, (valid_extensions > 0) ? (struct edid *)block : NULL);
+	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;
 }
 
@@ -569,17 +589,19 @@ 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)
@@ -587,8 +609,8 @@ drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len)
 
 		while (n_blocks--) {
 			cnt++;
-			if (drm_edid_block_valid(*blockp + cnt * EDID_LENGTH,
-						 valid_extensions + 1, print_bad_edid)) {
+			if (!drm_edid_block_check_error(*blockp + cnt * EDID_LENGTH,
+							valid_extensions + 1, last_error_flags)) {
 				valid_extensions++;
 				if (cnt != valid_extensions)
 					memcpy(*blockp + valid_extensions * EDID_LENGTH,
@@ -596,8 +618,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;
 }
 
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..d402b3b 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,7 +1055,6 @@ 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);
 struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
 					   int hsize, int vsize, int fresh,
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

[PATCH v2 15/18] DRM/KMS/ast: Include drm_edid.h in file using drm_get_edid().

[Egbert Eich]

Signed-off-by: Egbert Eich <eich@suse.com>
---
 drivers/gpu/drm/ast/ast_mode.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 7fc9f72..c27aa8d 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -31,6 +31,7 @@
 #include <drm/drmP.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_edid.h>
 #include "ast_drv.h"
 
 #include "ast_tables.h"
-- 
1.7.7

[PATCH v2 16/18] DRM/KMS/gma500: Include drm_edid.h in file using drm_get_edid().

[Egbert Eich]

Signed-off-by: Egbert Eich <eich@suse.com>
---
 drivers/gpu/drm/gma500/cdv_intel_dp.c    |    1 +
 drivers/gpu/drm/gma500/oaktrail_lvds.c   |    1 +
 drivers/gpu/drm/gma500/psb_intel_modes.c |    1 +
 3 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/gma500/cdv_intel_dp.c b/drivers/gpu/drm/gma500/cdv_intel_dp.c
index e3a3978..37cad73 100644
--- a/drivers/gpu/drm/gma500/cdv_intel_dp.c
+++ b/drivers/gpu/drm/gma500/cdv_intel_dp.c
@@ -34,6 +34,7 @@
 #include "psb_intel_drv.h"
 #include "psb_intel_reg.h"
 #include <drm/drm_dp_helper.h>
+#include <drm/drm_edid.h>
 
 #define _wait_for(COND, MS, W) ({ \
         unsigned long timeout__ = jiffies + msecs_to_jiffies(MS);       \
diff --git a/drivers/gpu/drm/gma500/oaktrail_lvds.c b/drivers/gpu/drm/gma500/oaktrail_lvds.c
index 558c77f..f7e20f5 100644
--- a/drivers/gpu/drm/gma500/oaktrail_lvds.c
+++ b/drivers/gpu/drm/gma500/oaktrail_lvds.c
@@ -22,6 +22,7 @@
 
 #include <linux/i2c.h>
 #include <drm/drmP.h>
+#include <drm/drm_edid.h>
 #include <asm/mrst.h>
 
 #include "intel_bios.h"
diff --git a/drivers/gpu/drm/gma500/psb_intel_modes.c b/drivers/gpu/drm/gma500/psb_intel_modes.c
index 4fca0d6..1da014d 100644
--- a/drivers/gpu/drm/gma500/psb_intel_modes.c
+++ b/drivers/gpu/drm/gma500/psb_intel_modes.c
@@ -20,6 +20,7 @@
 #include <linux/i2c.h>
 #include <linux/fb.h>
 #include <drm/drmP.h>
+#include <drm/drm_edid.h>
 #include "psb_intel_drv.h"
 
 /**
-- 
1.7.7

[PATCH v2 17/18] DRM/KMS/mgag200: Include drm_edid.h in file using drm_get_edid().

[Egbert Eich]

Signed-off-by: Egbert Eich <eich@suse.com>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index d3d99a2..f89a0c1 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -15,6 +15,7 @@
 
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_edid.h>
 
 #include "mgag200_drv.h"
 
-- 
1.7.7

[PATCH v2 18/18] DRM/KMS/EDID: Move EDID related Functions to drm_edid.h.

[Egbert Eich]

Signed-off-by: Egbert Eich <eich@suse.com>
---
 include/drm/drm_crtc.h |    8 --------
 include/drm/drm_edid.h |    9 +++++++++
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index d402b3b..7eed9bd 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -878,9 +878,6 @@ extern char *drm_get_tv_subconnector_name(int val);
 extern char *drm_get_tv_select_name(int val);
 extern void drm_fb_release(struct drm_file *file_priv);
 extern int drm_mode_group_init_legacy_group(struct drm_device *dev, struct drm_mode_group *group);
-extern bool drm_probe_ddc(struct i2c_adapter *adapter);
-extern struct edid *drm_get_edid(struct drm_connector *connector,
-				 struct i2c_adapter *adapter);
 extern int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
 extern void drm_mode_probed_add(struct drm_connector *connector, struct drm_display_mode *mode);
 extern void drm_mode_remove(struct drm_connector *connector, struct drm_display_mode *mode);
@@ -1036,9 +1033,6 @@ extern int drm_mode_gamma_get_ioctl(struct drm_device *dev,
 				    void *data, struct drm_file *file_priv);
 extern int drm_mode_gamma_set_ioctl(struct drm_device *dev,
 				    void *data, struct drm_file *file_priv);
-extern u8 *drm_find_cea_extension(struct edid *edid);
-extern bool drm_detect_hdmi_monitor(struct edid *edid);
-extern bool drm_detect_monitor_audio(struct edid *edid);
 extern int drm_mode_page_flip_ioctl(struct drm_device *dev,
 				    void *data, struct drm_file *file_priv);
 extern struct drm_display_mode *drm_cvt_mode(struct drm_device *dev,
@@ -1054,8 +1048,6 @@ extern struct drm_display_mode *drm_gtf_mode_complex(struct drm_device *dev,
 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_is_valid(struct edid *edid);
 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 6bcaee5..72328bc 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -268,4 +268,13 @@ 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);
 
+extern bool drm_probe_ddc(struct i2c_adapter *adapter);
+extern struct edid *drm_get_edid(struct drm_connector *connector,
+				 struct i2c_adapter *adapter);
+extern u8 *drm_find_cea_extension(struct edid *edid);
+extern bool drm_detect_hdmi_monitor(struct edid *edid);
+extern bool drm_detect_monitor_audio(struct edid *edid);
+extern int drm_edid_header_is_valid(const u8 *raw_edid);
+extern bool drm_edid_is_valid(struct edid *edid);
+
 #endif /* __DRM_EDID_H__ */
-- 
1.7.7

Re: [PATCH v2 05/18] DRM/KMS/EDID: Test EDDC if EDID announces more than one Extension Block (v2)

[Ville Syrjälä]

On Thu, Nov 22, 2012 at 05:22:55AM -0500, Egbert Eich wrote:
> There are displays which announce EDID extension blocks in the
> Extension Flag of the EDID base block although they are not EDDC
> capable (ie. take a segment address at I2C slave address 0x30).
> We test this by looking for an EDID header which is only possible
> in the base block.
> If the segment address is not taken into account, this block will
> be identical to the base block in which case we stop reading further
> EEDID blocks, correct the extension flag and just return the base
> block.
> 
> v2: Split up EDID fixup code into separate commit.
> 
> Signed-off-by: Egbert Eich <eich@suse.com>
> ---
>  drivers/gpu/drm/drm_edid.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index a952cfe..5a0e331 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -364,6 +364,19 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  			}
>  			if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) {
>  				valid_extensions++;
> +				/* Test if base block announced extension blocks although
> +				 * display is not EDDC capable.
> +				 */
> +				if (j == 2) {
> +					int k;
> +					for (k = 0; k < sizeof(edid_header); k++)
> +						if (block[(EDID_LENGTH * 2) + k] != edid_header[k])
> +							break;
> +					if (k == sizeof(edid_header)) {
> +						valid_extensions = 0;
> +						goto done_fix_extension_count;
> +					}

memcmp()? Also couldn't we just memcmp() the whole block against the base
block, instead of just the header part?

Also the comment is somehow misleading. It talks about the base block
even though we're looking at the extension block.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v2 05/18] DRM/KMS/EDID: Test EDDC if EDID announces more than one Extension Block (v2)

[Egbert Eich]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=unknown, Size: 2927 bytes --]

Ville Syrjälä writes:
 > On Thu, Nov 22, 2012 at 05:22:55AM -0500, Egbert Eich wrote:
 > > There are displays which announce EDID extension blocks in the
 > > Extension Flag of the EDID base block although they are not EDDC
 > > capable (ie. take a segment address at I2C slave address 0x30).
 > > We test this by looking for an EDID header which is only possible
 > > in the base block.
 > > If the segment address is not taken into account, this block will
 > > be identical to the base block in which case we stop reading further
 > > EEDID blocks, correct the extension flag and just return the base
 > > block.
 > > 
 > > v2: Split up EDID fixup code into separate commit.
 > > 
 > > Signed-off-by: Egbert Eich <eich@suse.com>
 > > ---
 > >  drivers/gpu/drm/drm_edid.c |   13 +++++++++++++
 > >  1 files changed, 13 insertions(+), 0 deletions(-)
 > > 
 > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
 > > index a952cfe..5a0e331 100644
 > > --- a/drivers/gpu/drm/drm_edid.c
 > > +++ b/drivers/gpu/drm/drm_edid.c
 > > @@ -364,6 +364,19 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 > >  			}
 > >  			if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) {
 > >  				valid_extensions++;
 > > +				/* Test if base block announced extension blocks although
 > > +				 * display is not EDDC capable.
 > > +				 */
 > > +				if (j == 2) {
 > > +					int k;
 > > +					for (k = 0; k < sizeof(edid_header); k++)
 > > +						if (block[(EDID_LENGTH * 2) + k] != edid_header[k])
 > > +							break;
 > > +					if (k == sizeof(edid_header)) {
 > > +						valid_extensions = 0;
 > > +						goto done_fix_extension_count;
 > > +					}
 > 
 > memcmp()? Also couldn't we just memcmp() the whole block against the base
 > block, instead of just the header part?

I don't see an advantage of comparing the entire block with the base block:
the signature should already be unique. However I don't insist ;)
Regarding memcmp() you are definitely right, I will change the code.

 > 
 > Also the comment is somehow misleading. It talks about the base block
 > even though we're looking at the extension block.


Reason for this patch:
I had a bug report for a monitor announcing extension blocks in the extension
block flag of the base block (over 200!) although it wasn't EDDC capable. 
For some reason it got past the ACK check when the segment number was written
to address 0x30 and happily transferred the base block for any odd numbered 
block and some garbage for even ones.
The only reliable way we found to catch this condition early was to check if 
block 2 had the header of a base block which will happen when the display
cannot deal with the segment number.

This was what I tried to summarize in very few words - maybe i should reword
it a bit.

Thanks!

	Egbert.

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 05/18] DRM/KMS/EDID: Test EDDC if EDID announces more than one Extension Block (v2)

[Ville Syrjälä]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 3985 bytes --]

On Thu, Nov 22, 2012 at 01:07:28PM +0100, Egbert Eich wrote:
> Ville Syrjälä writes:
>  > On Thu, Nov 22, 2012 at 05:22:55AM -0500, Egbert Eich wrote:
>  > > There are displays which announce EDID extension blocks in the
>  > > Extension Flag of the EDID base block although they are not EDDC
>  > > capable (ie. take a segment address at I2C slave address 0x30).
>  > > We test this by looking for an EDID header which is only possible
>  > > in the base block.
>  > > If the segment address is not taken into account, this block will
>  > > be identical to the base block in which case we stop reading further
>  > > EEDID blocks, correct the extension flag and just return the base
>  > > block.
>  > > 
>  > > v2: Split up EDID fixup code into separate commit.
>  > > 
>  > > Signed-off-by: Egbert Eich <eich@suse.com>
>  > > ---
>  > >  drivers/gpu/drm/drm_edid.c |   13 +++++++++++++
>  > >  1 files changed, 13 insertions(+), 0 deletions(-)
>  > > 
>  > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>  > > index a952cfe..5a0e331 100644
>  > > --- a/drivers/gpu/drm/drm_edid.c
>  > > +++ b/drivers/gpu/drm/drm_edid.c
>  > > @@ -364,6 +364,19 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  > >  			}
>  > >  			if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) {
>  > >  				valid_extensions++;
>  > > +				/* Test if base block announced extension blocks although
>  > > +				 * display is not EDDC capable.
>  > > +				 */
>  > > +				if (j == 2) {
>  > > +					int k;
>  > > +					for (k = 0; k < sizeof(edid_header); k++)
>  > > +						if (block[(EDID_LENGTH * 2) + k] != edid_header[k])
>  > > +							break;
>  > > +					if (k == sizeof(edid_header)) {
>  > > +						valid_extensions = 0;
>  > > +						goto done_fix_extension_count;
>  > > +					}
>  > 
>  > memcmp()? Also couldn't we just memcmp() the whole block against the base
>  > block, instead of just the header part?
> 
> I don't see an advantage of comparing the entire block with the base block:
> the signature should already be unique. However I don't insist ;)

Me neither. I just figured it might reduce the chance of false
positives. But if you say that can't happen, I'll take your word
for it.

> Regarding memcmp() you are definitely right, I will change the code.
> 
>  > 
>  > Also the comment is somehow misleading. It talks about the base block
>  > even though we're looking at the extension block.
> 
> 
> Reason for this patch:
> I had a bug report for a monitor announcing extension blocks in the extension
> block flag of the base block (over 200!) although it wasn't EDDC capable. 
> For some reason it got past the ACK check when the segment number was written
> to address 0x30 and happily transferred the base block for any odd numbered 
> block and some garbage for even ones.

That's nasty. When I saw your patch I was immediately thinking that this
is caused by the IGNORE_NAK flag, but then I read the current code, and
realized that we're not using that flag. It was used in the original
EDDC patch, and I was worried that this kind of bad behaviour would be
possible if use the flag. But it seems I underestimated how crappy the
monitor hardwar can be.

> The only reliable way we found to catch this condition early was to check if 
> block 2 had the header of a base block which will happen when the display
> cannot deal with the segment number.
> 
> This was what I tried to summarize in very few words - maybe i should reword
> it a bit.

Right. The idea seems reasonable, I just found the comment somehow a
bit confusing when I was reading the code following it.

So maybe something like 'Test if base block ..., by checking whether the
extension block is a duplicate the base block.' Although the use of
memcmp() will already make things much clearer.

-- 
Ville Syrjälä
Intel OTC

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 05/18] DRM/KMS/EDID: Test EDDC if EDID announces more than one Extension Block (v2)

[Egbert Eich]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=unknown, Size: 2782 bytes --]

Ville Syrjälä writes:
 > 
 > Me neither. I just figured it might reduce the chance of false
 > positives. But if you say that can't happen, I'll take your word
 > for it.
 > 
 > > Regarding memcmp() you are definitely right, I will change the code.
 > > 
 > >  > 
 > >  > Also the comment is somehow misleading. It talks about the base block
 > >  > even though we're looking at the extension block.
 > > 
 > > 
 > > Reason for this patch:
 > > I had a bug report for a monitor announcing extension blocks in the extension
 > > block flag of the base block (over 200!) although it wasn't EDDC capable. 
 > > For some reason it got past the ACK check when the segment number was written
 > > to address 0x30 and happily transferred the base block for any odd numbered 
 > > block and some garbage for even ones.
 > 
 > That's nasty. When I saw your patch I was immediately thinking that this
 > is caused by the IGNORE_NAK flag, but then I read the current code, and
 > realized that we're not using that flag. It was used in the original
 > EDDC patch, and I was worried that this kind of bad behaviour would be
 > possible if use the flag. But it seems I underestimated how crappy the
 > monitor hardwar can be.

Right. 
This flag would only make sense for the base block (to reset 
an EDDC capable monitor but not fail on non-EDDC capable ones). 
In fact I had a patch to add this, but then I realized that none
of the I2C implemenations in the gfx drivers honored this flag.
But then I reread the VESA spec for EDDC, this requires that the
segment address in the monitor has to be reset on a STOP condition.

So if there ever is the case that a monitor has a segment > 0 
selected when drm_read_edid() is called for the first time (because 
of a system crash for instance) the validation will fail and causes 
a 2nd read attempt for which the segment address should be reset by 
the previous transfer.
Regarding crappiness of monitor hardware: catch me at a bar and I
can tell you more stories ... ;p

 > 
 > > The only reliable way we found to catch this condition early was to check if 
 > > block 2 had the header of a base block which will happen when the display
 > > cannot deal with the segment number.
 > > 
 > > This was what I tried to summarize in very few words - maybe i should reword
 > > it a bit.
 > 
 > Right. The idea seems reasonable, I just found the comment somehow a
 > bit confusing when I was reading the code following it.
 > 
 > So maybe something like 'Test if base block ..., by checking whether the
 > extension block is a duplicate the base block.' Although the use of
 > memcmp() will already make things much clearer.

Right. I've already reworked it. Will send out.

Cheers,
	Egbert.

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 13/18] DRM/KMS/EDID: Cache EDID blobs with extensions (v2)

[Ville Syrjälä]

On Thu, Nov 22, 2012 at 05:23:03AM -0500, Egbert Eich wrote:
> According the the VESA specs there can be up to 254 EEDID extension blocks.
> Since we may read the EDID (including extensions) in 10 second intervals to
> probe for display hotplugging (at least in cases where no hardware hotplug
> detection exists) and I2C transfer is rather slow we may end up consuming
> a considerable amount on CPU time for just that.
> This patch caches the EDID block if it contains at least one extension.
> To determine if the blocks match we only tranfer the base block, on a match
> we use the cached data.
> 
> V2: Use kmemdup() instead of a kmalloc()/memcpy() combo,
>     erase cache when reading a 'firmware'-supplied EDID or on error.
> 
> Signed-off-by: Egbert Eich <eich@suse.com>
> ---
>  drivers/gpu/drm/drm_crtc.c |    1 +
>  drivers/gpu/drm/drm_edid.c |   57 +++++++++++++++++++++++++++++++++++++------
>  include/drm/drm_crtc.h     |    1 +
>  include/drm/drm_edid.h     |    1 +
>  4 files changed, 52 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 3533609..e283355 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -598,6 +598,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
>  		drm_mode_remove(connector, mode);
>  
>  	mutex_lock(&dev->mode_config.mutex);
> +	drm_cache_edid(connector, NULL);
>  	drm_mode_object_put(dev, &connector->base);
>  	list_del(&connector->head);
>  	dev->mode_config.num_connector--;
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index a47fa7f..28b877c 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -419,6 +419,38 @@ fixup_edid(u8 **blockp, int valid_extensions)
>  	}
>  }
>  
> +static bool
> +compare_get_edid_from_cache(struct drm_connector *connector, struct edid **edidp)
> +{
> +	if (connector->edid_cache &&
> +	    connector->edid_cache->prod_code[0] == (*edidp)->prod_code[0] &&
> +	    connector->edid_cache->prod_code[1] == (*edidp)->prod_code[1] &&
> +	    connector->edid_cache->serial == (*edidp)->serial &&
> +	    connector->edid_cache->input == (*edidp)->input) {
> +		int size = (connector->edid_cache->extensions + 1) * EDID_LENGTH;
> +		struct edid *new = kmemdup(connector->edid_cache, size, GFP_KERNEL);
> +		if (!new)
> +			return false;
> +		DRM_DEBUG_KMS("Got EDID for %s from cache.\n", drm_get_connector_name(connector));
> +		kfree(*edidp);
> +		*edidp = new;
> +		return true;
> +	}
> +	return false;
> +}
> +
> +void
> +drm_cache_edid(struct drm_connector *connector, struct edid *edid)
> +{
> +	struct edid *new = NULL;
> +	kfree(connector->edid_cache);
> +	if (edid) {
> +		int size = (edid->extensions + 1) * EDID_LENGTH;
> +		new = kmemdup(edid, size, GFP_KERNEL);
> +	}
> +	connector->edid_cache = new;
> +}
> +
>  static u8 *
>  drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  {
> @@ -429,28 +461,30 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  #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)
> +	if (block) {
> +		drm_cache_edid(connector, NULL);
>  		return block;
> +	}
>  #endif
>  
>  	if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
> -		return NULL;
> +		goto error;
>  
>  	/* base block fetch */
>  	for (i = 0; i < 4; i++) {
>  		if (drm_do_probe_ddc_edid(adapter, block, 0, EDID_LENGTH))
> -			goto out;
> +			goto error_free;
>  		if (drm_edid_block_valid(block, 0, print_bad_edid))
>  			break;
>  		if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) {
>  			connector->null_edid_counter++;
> -			goto carp;
> +			goto error_carp;
>  		}
>  	}
>  	if (i == 4)
> -		goto carp;
> +		goto error_carp;
>  
> -	/* if there's no extensions, we're done */
> +	/* if there are no extensions, we're done - don't bother caching */
>  	if (block[EDID_EXTENSION_FLAG_OFFSET] == 0)
>  		return block;

Shouldn't you erase the cache here too?

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v2 13/18] DRM/KMS/EDID: Cache EDID blobs with extensions (v2)

[Egbert Eich]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=unknown, Size: 459 bytes --]

Ville Syrjälä writes:
 > On Thu, Nov 22, 2012 at 05:23:03AM -0500, Egbert Eich wrote:
 > >  
 > > -	/* if there's no extensions, we're done */
 > > +	/* if there are no extensions, we're done - don't bother caching */
 > >  	if (block[EDID_EXTENSION_FLAG_OFFSET] == 0)
 > >  		return block;
 > 
 > Shouldn't you erase the cache here too?
 > 

Indeed. And I had fixed it. It went into the wrong patch however :(

Will correct.

Thanks,
	Egbert.

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3] DRM/KMS/EDID: Test EDDC if EDID announces more than one Extension Block (v3)

[Egbert Eich]

There are displays which announce EDID extension blocks in the
Extension Flag of the EDID base block although they are not EDDC
capable (ie. take a segment address at I2C slave address 0x30).
We test this by looking for an EDID header which is only possible
in the base block.
If the segment address is not taken into account, this block will
be identical to the base block in which case we stop reading further
EEDID blocks, correct the extension flag and just return the base
block.

v2: Split up EDID fixup code into separate commit.
v3: Worked in changes suggested by Ville Syrjälä <ville.syrjala@linux.intel.com>:
    Reworded comment,
    Used memcmp(),
    Compared entire base block instead of signature only.

Signed-off-by: Egbert Eich <eich@suse.com>
---
 drivers/gpu/drm/drm_edid.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index a952cfe..b42fbc0 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -364,6 +364,16 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 			}
 			if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) {
 				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
+				 * select the correct segment address: detect this condition and bail early.
+				 */
+				if (j == 2) {
+					if (memcmp(block + EDID_LENGTH * 2, block, EDID_LENGTH) == 0) {
+						valid_extensions = 0;
+						goto done_fix_extension_count;
+					}
+				}
 				break;
 			}
 		}
-- 
1.7.7

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3] DRM/KMS/EDID: Cache EDID blobs with extensions (v3)

[Egbert Eich]

According the the VESA specs there can be up to 254 EEDID extension blocks.
Since we may read the EDID (including extensions) in 10 second intervals to
probe for display hotplugging (at least in cases where no hardware hotplug
detection exists) and I2C transfer is rather slow we may end up consuming
a considerable amount on CPU time for just that.
This patch caches the EDID block if it contains at least one extension.
To determine if the blocks match we only tranfer the base block, on a match
we use the cached data.

V2: Use kmemdup() instead of a kmalloc()/memcpy() combo,
    erase cache when reading a 'firmware'-supplied EDID or on error.
V3: Uncache when only one extension block is found. This chunk had
    accidentally gone into the next patch of the series. Found by
    Ville Syrjälä <ville.syrjala@linux.intel.com>.

Signed-off-by: Egbert Eich <eich@suse.com>
---
 drivers/gpu/drm/drm_crtc.c |    1 +
 drivers/gpu/drm/drm_edid.c |   60 +++++++++++++++++++++++++++++++++++++------
 include/drm/drm_crtc.h     |    1 +
 include/drm/drm_edid.h     |    1 +
 4 files changed, 54 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 3533609..e283355 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -598,6 +598,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
 		drm_mode_remove(connector, mode);
 
 	mutex_lock(&dev->mode_config.mutex);
+	drm_cache_edid(connector, NULL);
 	drm_mode_object_put(dev, &connector->base);
 	list_del(&connector->head);
 	dev->mode_config.num_connector--;
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index e269739..dd0df60 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -419,6 +419,38 @@ fixup_edid(u8 **blockp, int valid_extensions)
 	}
 }
 
+static bool
+compare_get_edid_from_cache(struct drm_connector *connector, struct edid **edidp)
+{
+	if (connector->edid_cache &&
+	    connector->edid_cache->prod_code[0] == (*edidp)->prod_code[0] &&
+	    connector->edid_cache->prod_code[1] == (*edidp)->prod_code[1] &&
+	    connector->edid_cache->serial == (*edidp)->serial &&
+	    connector->edid_cache->input == (*edidp)->input) {
+		int size = (connector->edid_cache->extensions + 1) * EDID_LENGTH;
+		struct edid *new = kmemdup(connector->edid_cache, size, GFP_KERNEL);
+		if (!new)
+			return false;
+		DRM_DEBUG_KMS("Got EDID for %s from cache.\n", drm_get_connector_name(connector));
+		kfree(*edidp);
+		*edidp = new;
+		return true;
+	}
+	return false;
+}
+
+void
+drm_cache_edid(struct drm_connector *connector, struct edid *edid)
+{
+	struct edid *new = NULL;
+	kfree(connector->edid_cache);
+	if (edid) {
+		int size = (edid->extensions + 1) * EDID_LENGTH;
+		new = kmemdup(edid, size, GFP_KERNEL);
+	}
+	connector->edid_cache = new;
+}
+
 static u8 *
 drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 {
@@ -429,35 +461,41 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 #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)
+	if (block) {
+		drm_cache_edid(connector, NULL);
 		return block;
+	}
 #endif
 
 	if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
-		return NULL;
+		goto error;
 
 	/* base block fetch */
 	for (i = 0; i < 4; i++) {
 		if (drm_do_probe_ddc_edid(adapter, block, 0, EDID_LENGTH))
-			goto out;
+			goto error_free;
 		if (drm_edid_block_valid(block, 0, print_bad_edid))
 			break;
 		if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) {
 			connector->null_edid_counter++;
-			goto carp;
+			goto error_carp;
 		}
 	}
 	if (i == 4)
-		goto carp;
+		goto error_carp;
 
-	/* if there's no extensions, we're done */
+	/* if there are no extensions, we're done - don't bother caching */
 	if (block[EDID_EXTENSION_FLAG_OFFSET] == 0)
-		return block;
+		goto done;
 
 	/* don't expect extension blocks in EDID Versions < 1.3: return base block with correct extension flag */
 	if (block[EDID_VERSION_MINOR_OFFSET] < 3)
 		goto done_fix_extension_count;
 
+	/* 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;
+
 	new = krealloc(block, (block[EDID_EXTENSION_FLAG_OFFSET] + 1) * EDID_LENGTH, GFP_KERNEL);
 	if (!new) {
 		dev_warn(connector->dev->dev, "%s: cannot allocate memory for %d EDID blocks: truncating.\n",
@@ -497,18 +535,22 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 
 done_fix_extension_count:
 	fixup_edid(&block, valid_extensions);
+done:
+	drm_cache_edid(connector, (valid_extensions > 0) ? (struct edid *)block : NULL);
 
 	return block;
 
-carp:
+error_carp:
 	if (print_bad_edid) {
 		dev_warn(connector->dev->dev, "%s: EDID block %d invalid.\n",
 			 drm_get_connector_name(connector), j);
 	}
 	connector->bad_edid_counter++;
 
-out:
+error_free:
 	kfree(block);
+error:
+	drm_cache_edid(connector, NULL);
 	return NULL;
 }
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 49dd8c2..6a1054c 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -601,6 +601,7 @@ struct drm_connector {
 	struct drm_encoder *encoder; /* currently active encoder */
 
 	/* EDID bits */
+	struct edid *edid_cache;
 	uint8_t eld[MAX_ELD_BYTES];
 	bool dvi_dual;
 	int max_tmds_clock;	/* in MHz */
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 53c619c..c880510 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -256,5 +256,6 @@ struct drm_connector *drm_select_eld(struct drm_encoder *encoder,
 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);
 
 #endif /* __DRM_EDID_H__ */
-- 
1.7.7

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Egbert Eich]

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.

Signed-off-by: Egbert Eich <eich@suse.com>
---
 drivers/gpu/drm/drm_edid.c                 |  117 +++++++++++++++++-----------
 drivers/gpu/drm/radeon/radeon_connectors.c |    2 +-
 include/drm/drm_crtc.h                     |    4 +-
 include/drm/drm_edid.h                     |   10 +++
 4 files changed, 82 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index dd0df60..aa9b34d 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,15 +233,23 @@ 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;
+}
+
+bool
+drm_edid_block_valid(u8 *raw_edid, int block, unsigned last_error_flags)
+{
+	if (!drm_edid_block_check_error(raw_edid, block, last_error_flags))
+		return true;
+	return false;
 }
 EXPORT_SYMBOL(drm_edid_block_valid);
 
@@ -241,7 +268,7 @@ bool drm_edid_is_valid(struct edid *edid)
 		return false;
 
 	for (i = 0; i <= edid->extensions; i++)
-		if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true))
+		if (drm_edid_block_check_error(raw + i * EDID_LENGTH, i, true))
 			return false;
 
 	return true;
@@ -310,17 +337,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 +472,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)
 		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 +513,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) {
@@ -512,7 +531,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 				valid_extensions = 0;
 				goto done_fix_extension_count;
 			}
-			if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) {
+			if (!drm_edid_block_check_error(block + (valid_extensions + 1) * EDID_LENGTH, j, last_error_flags)) {
 				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 +554,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,17 +587,19 @@ 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)
@@ -585,8 +607,8 @@ drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len)
 
 		while (n_blocks--) {
 			cnt++;
-			if (drm_edid_block_valid(*blockp + cnt * EDID_LENGTH,
-						 valid_extensions + 1, print_bad_edid)) {
+			if (!drm_edid_block_check_error(*blockp + cnt * EDID_LENGTH,
+							valid_extensions + 1, last_error_flags)) {
 				valid_extensions++;
 				if (cnt != valid_extensions)
 					memcpy(*blockp + valid_extensions * EDID_LENGTH,
@@ -594,8 +616,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;
 }
 
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..d402b3b 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,7 +1055,6 @@ 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);
 struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
 					   int hsize, int vsize, int fresh,
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

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

[Ville Syrjälä]

On Thu, Nov 22, 2012 at 09:44:42AM -0500, Egbert Eich wrote:
> 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.
> 
> Signed-off-by: Egbert Eich <eich@suse.com>
> ---
>  drivers/gpu/drm/drm_edid.c                 |  117 +++++++++++++++++-----------
>  drivers/gpu/drm/radeon/radeon_connectors.c |    2 +-
>  include/drm/drm_crtc.h                     |    4 +-
>  include/drm/drm_edid.h                     |   10 +++
>  4 files changed, 82 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index dd0df60..aa9b34d 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;

You could use memchr_inv() here. But the compiler can't optimize it
since it's not inline, so I suppose it might make it slower.

> +}
> +
>  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,15 +233,23 @@ 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;
> +}
> +
> +bool
> +drm_edid_block_valid(u8 *raw_edid, int block, unsigned last_error_flags)
> +{
> +	if (!drm_edid_block_check_error(raw_edid, block, last_error_flags))
> +		return true;
> +	return false;

return !drm_edid_block_check_error();

>  }
>  EXPORT_SYMBOL(drm_edid_block_valid);
>  
> @@ -241,7 +268,7 @@ bool drm_edid_is_valid(struct edid *edid)
>  		return false;
>  
>  	for (i = 0; i <= edid->extensions; i++)
> -		if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true))
> +		if (drm_edid_block_check_error(raw + i * EDID_LENGTH, i, true))
                                                                         ^^^^

That looks wrong. Also the 's/!drm_edid_block_valid/drm_edid_block_check_error'
change seems superfluous since you're not using the more detailed return
value from drm_edid_block_check_error().

>  			return false;
>  
>  	return true;
> @@ -310,17 +337,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 +472,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)
>  		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 +513,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) {
> @@ -512,7 +531,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  				valid_extensions = 0;
>  				goto done_fix_extension_count;
>  			}
> -			if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) {
> +			if (!drm_edid_block_check_error(block + (valid_extensions + 1) * EDID_LENGTH, j, last_error_flags)) {

Again the change of function seems superfluous.

-- 
Ville Syrjälä
Intel OTC

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

[Egbert Eich]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=unknown, Size: 2870 bytes --]

Ville Syrjälä writes:
 > > 
 > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
 > > index dd0df60..aa9b34d 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;

[...]
 > 
 > You could use memchr_inv() here. But the compiler can't optimize it
 > since it's not inline, so I suppose it might make it slower.


 > > +
 > > +bool
 > > +drm_edid_block_valid(u8 *raw_edid, int block, unsigned last_error_flags)
 > > +{
 > > +	if (!drm_edid_block_check_error(raw_edid, block, last_error_flags))
 > > +		return true;
 > > +	return false;
 > 
 > return !drm_edid_block_check_error();

Right. 
It's stupid anyway. See below.

 > 
 > >  }
 > >  EXPORT_SYMBOL(drm_edid_block_valid);
 > >  
 > > @@ -241,7 +268,7 @@ bool drm_edid_is_valid(struct edid *edid)
 > >  		return false;
 > >  
 > >  	for (i = 0; i <= edid->extensions; i++)
 > > -		if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true))
 > > +		if (drm_edid_block_check_error(raw + i * EDID_LENGTH, i, true))
 >                                                                          ^^^^
 > 
 > That looks wrong. Also the 's/!drm_edid_block_valid/drm_edid_block_check_error'

Oops, right. Leftover from old code.

 > change seems superfluous since you're not using the more detailed return
 > value from drm_edid_block_check_error().

This should probably be changed. 
For the drm_edid_block_valid() I'm already using the previous error state
as argument - but I don't tell the result of this read. 
Doesn't make much sense :(

Something similar should be done for drm_edid_is_valid() - even if the 
driver doesn't bother (for instance because this function is only called
once when the device structures are initialized).

The current code ignores the error state for extension blocks i guess it
should not if we want to avoid having repreated logging of errors in the
extension blocks.
What should be done is:
        unsigned err = drm_edid_block_check_error();
	result |= err;
	if (!err) {
	   valid_extensions++;
 	   ...
        }

 > > -			if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) {
 > > +			if (!drm_edid_block_check_error(block + (valid_extensions + 1) * EDID_LENGTH, j, last_error_flags)) {
 > 
 > Again the change of function seems superfluous.
 > 

Exactly. Please see above.

Thanks for looking at the patches!

Cheers,
	Egbert.

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 3161 bytes --]

On Thu, Nov 22, 2012 at 07:28:44PM +0100, Egbert Eich wrote:
> Ville Syrjälä writes:
>  > > 
>  > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>  > > index dd0df60..aa9b34d 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;
> 
> [...]
>  > 
>  > You could use memchr_inv() here. But the compiler can't optimize it
>  > since it's not inline, so I suppose it might make it slower.
> 
> 
>  > > +
>  > > +bool
>  > > +drm_edid_block_valid(u8 *raw_edid, int block, unsigned last_error_flags)
>  > > +{
>  > > +	if (!drm_edid_block_check_error(raw_edid, block, last_error_flags))
>  > > +		return true;
>  > > +	return false;
>  > 
>  > return !drm_edid_block_check_error();
> 
> Right. 
> It's stupid anyway. See below.
> 
>  > 
>  > >  }
>  > >  EXPORT_SYMBOL(drm_edid_block_valid);
>  > >  
>  > > @@ -241,7 +268,7 @@ bool drm_edid_is_valid(struct edid *edid)
>  > >  		return false;
>  > >  
>  > >  	for (i = 0; i <= edid->extensions; i++)
>  > > -		if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true))
>  > > +		if (drm_edid_block_check_error(raw + i * EDID_LENGTH, i, true))
>  >                                                                          ^^^^
>  > 
>  > That looks wrong. Also the 's/!drm_edid_block_valid/drm_edid_block_check_error'
> 
> Oops, right. Leftover from old code.
> 
>  > change seems superfluous since you're not using the more detailed return
>  > value from drm_edid_block_check_error().
> 
> This should probably be changed. 
> For the drm_edid_block_valid() I'm already using the previous error state
> as argument - but I don't tell the result of this read. 
> Doesn't make much sense :(
> 
> Something similar should be done for drm_edid_is_valid() - even if the 
> driver doesn't bother (for instance because this function is only called
> once when the device structures are initialized).
> 
> The current code ignores the error state for extension blocks i guess it
> should not if we want to avoid having repreated logging of errors in the
> extension blocks.

I'm not sure. The current code dump all failed extension block, doesn't
it? Maybe we actually do want that to happen, at least w/ debugs
enabled.

Then there are various retry loops in the code, which may or may not be
necessary. I have a feeling some of them may have been attempts at
papering over a bug that I fixed [1] in the i2c bitbanging code. But if
they are necessary, I'm not sure we really appreciate repeated dumps of
the same block.

[1] https://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=8ee161ce5e0cfc689eb677f227a6248191165fac

-- 
Ville Syrjälä
Intel OTC

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Egbert Eich]

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

[PATCH v4] DRM/KMS/EDID: Move EDID related Functions to drm_edid.h (v2)

[Egbert Eich]

v2: Adjusted to apply cleanly.

Signed-off-by: Egbert Eich <eich@suse.com>
---
 include/drm/drm_crtc.h |    8 --------
 include/drm/drm_edid.h |    9 +++++++++
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 7a3ccbf..7eed9bd 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -878,9 +878,6 @@ extern char *drm_get_tv_subconnector_name(int val);
 extern char *drm_get_tv_select_name(int val);
 extern void drm_fb_release(struct drm_file *file_priv);
 extern int drm_mode_group_init_legacy_group(struct drm_device *dev, struct drm_mode_group *group);
-extern bool drm_probe_ddc(struct i2c_adapter *adapter);
-extern struct edid *drm_get_edid(struct drm_connector *connector,
-				 struct i2c_adapter *adapter);
 extern int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
 extern void drm_mode_probed_add(struct drm_connector *connector, struct drm_display_mode *mode);
 extern void drm_mode_remove(struct drm_connector *connector, struct drm_display_mode *mode);
@@ -1036,9 +1033,6 @@ extern int drm_mode_gamma_get_ioctl(struct drm_device *dev,
 				    void *data, struct drm_file *file_priv);
 extern int drm_mode_gamma_set_ioctl(struct drm_device *dev,
 				    void *data, struct drm_file *file_priv);
-extern u8 *drm_find_cea_extension(struct edid *edid);
-extern bool drm_detect_hdmi_monitor(struct edid *edid);
-extern bool drm_detect_monitor_audio(struct edid *edid);
 extern int drm_mode_page_flip_ioctl(struct drm_device *dev,
 				    void *data, struct drm_file *file_priv);
 extern struct drm_display_mode *drm_cvt_mode(struct drm_device *dev,
@@ -1054,8 +1048,6 @@ extern struct drm_display_mode *drm_gtf_mode_complex(struct drm_device *dev,
 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 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 6bcaee5..2c0d39a 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -268,4 +268,13 @@ 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);
 
+extern bool drm_probe_ddc(struct i2c_adapter *adapter);
+extern struct edid *drm_get_edid(struct drm_connector *connector,
+				 struct i2c_adapter *adapter);
+extern u8 *drm_find_cea_extension(struct edid *edid);
+extern bool drm_detect_hdmi_monitor(struct edid *edid);
+extern bool drm_detect_monitor_audio(struct edid *edid);
+extern int drm_edid_header_is_valid(const u8 *raw_edid);
+extern unsigned drm_edid_is_valid(struct edid *edid, unsigned last_error_flags);
+
 #endif /* __DRM_EDID_H__ */
-- 
1.7.7

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

[Egbert Eich]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=unknown, Size: 1761 bytes --]

Ville Syrjälä writes:
 > On Thu, Nov 22, 2012 at 07:28:44PM +0100, Egbert Eich wrote:
 > > 
 > > Something similar should be done for drm_edid_is_valid() - even if the 
 > > driver doesn't bother (for instance because this function is only called
 > > once when the device structures are initialized).
 > > 
 > > The current code ignores the error state for extension blocks i guess it
 > > should not if we want to avoid having repreated logging of errors in the
 > > extension blocks.
 > 
 > I'm not sure. The current code dump all failed extension block, doesn't
 > it? Maybe we actually do want that to happen, at least w/ debugs
 > enabled.

Yes, it does. It drops them silently. I was a bit unclear - what I 
meant was: we ignore the result for the final error state we keep 
to be able to track which errors we still want to log.
 > 
 > Then there are various retry loops in the code, which may or may not be
 > necessary. I have a feeling some of them may have been attempts at
 > papering over a bug that I fixed [1] in the i2c bitbanging code. But if
 > they are necessary, I'm not sure we really appreciate repeated dumps of
 > the same block.
 > 
 > [1] https://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=8ee161ce5e0cfc689eb677f227a6248191165fac

Right, I know this patch :)
I'm also not sure about the retries. I guess for the base block we want at
least one retry in case the display is in an intermediate state where it
will not send the base block.
Apart from this I've seen many EDID failures however I don't recall seeing
one which got fixed by repeated reads. However to see if those loops
fix any issue we may want to keep all messages enabled for the time being.

Cheers,
	Egbert.

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Egbert Eich]

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: Inspired by a discussion with Ville Syrjälä
    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.
v5: Added missing radeon/radeon_combios.c.

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_combios.c    |    2 +-
 drivers/gpu/drm/radeon/radeon_connectors.c |    2 +-
 include/drm/drm_crtc.h                     |    6 +-
 include/drm/drm_edid.h                     |   10 ++
 6 files changed, 95 insertions(+), 63 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_combios.c b/drivers/gpu/drm/radeon/radeon_combios.c
index 45b660b..42cf702 100644
--- a/drivers/gpu/drm/radeon/radeon_combios.c
+++ b/drivers/gpu/drm/radeon/radeon_combios.c
@@ -463,7 +463,7 @@ bool radeon_combios_check_hardcoded_edid(struct radeon_device *rdev)
 
 	memcpy((unsigned char *)edid, raw, size);
 
-	if (!drm_edid_is_valid(edid)) {
+	if (drm_edid_is_valid(edid,0) != 0) {
 		kfree(edid);
 		return false;
 	}
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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/exynos: fix memory leak: free EDID block

[Jani Nikula]

On Tue, 20 Nov 2012, Sean Paul <seanpaul@chromium.org> wrote:
> On Tue, Nov 20, 2012 at 4:30 AM, Egbert Eich <eich@suse.de> wrote:
>> drm_get_edid() returns a pointer to an EDID block. The caller
>> is responsible to free this pointer itself.
>> Here the pointer gets assigned to the local variable raw_edid.
>> Therefore it should be freed before the variable goes out of
>> scope.
>>
>> Signed-off-by: Egbert Eich <eich@suse.de>
>> ---
>>  drivers/gpu/drm/exynos/exynos_hdmi.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
>> index 2c115f8..bc87bca 100644
>> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
>> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
>> @@ -1293,6 +1293,7 @@ static int hdmi_get_edid(void *ctx, struct drm_connector *connector,
>>                 DRM_DEBUG_KMS("%s : width[%d] x height[%d]\n",
>>                         (hdata->dvi_mode ? "dvi monitor" : "hdmi monitor"),
>>                         raw_edid->width_cm, raw_edid->height_cm);
>> +               kfree(raw_edid);
>
> This will actually cause the memory to be freed twice.
>
> The reason this happens is drm_get_edid attaches this to
> connector->display_info.raw_edid, which is then freed in the
> exynos_drm_connector function that gets the edid.
>
> The whole thing is ugly, and needs to be revised. I've uploaded a
> patch to refactor this against the chromium tree, but haven't yet
> rebased against upstream. See
> https://gerrit.chromium.org/gerrit/#/c/38406/

The patch is good. connector->display_info.raw_edid is gone since

commit 451023dc32d4542c21b52ad1692e6e01cb75b099
Author: Jani Nikula <jani.nikula@intel.com>
Date:   Wed Aug 15 09:32:39 2012 +0000

    drm: remove the raw_edid field from struct drm_display_info

BR,
Jani.


>
> For now, please drop this patch.
>
> Sean
>
>>         } else {
>>                 return -ENODEV;
>>         }
>> --
>> 1.7.7
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel