[PATCH] drm: Only warn about an invalid EDID block prior to rejection

[PATCH] drm: Only warn about an invalid EDID block prior to rejection

[Chris Wilson]

On a noisy link, we may retry the EDID reads multiple times per block
and still succeed. In such cases, we don't want to flood the kernel logs
with *ERROR* messages as we then succeed. Refactor the EDID dumping and
push it into the caller rather than the validator so we can suppress the
warnings from transient failures. In the process, we want to refactor
drm_load_edid_firmware() to use the common drm_do_get_edid() to share
the same logic (since it already duplicates it).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
References: https://bugs.freedesktop.org/show_bug.cgi?id=89454
---
 drivers/gpu/drm/drm_edid.c      | 64 +++++++++++++++++++++++++++--------------
 drivers/gpu/drm/drm_edid_load.c | 60 +++++++++++---------------------------
 include/drm/drm_crtc.h          |  2 +-
 3 files changed, 61 insertions(+), 65 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 53bc7a628909..024b47ffb065 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1040,14 +1040,13 @@ static bool drm_edid_is_zero(const u8 *in_edid, int length)
  * drm_edid_block_valid - Sanity check the EDID block (base or extension)
  * @raw_edid: pointer to raw EDID block
  * @block: type of block to validate (0 for base, extension otherwise)
- * @print_bad_edid: if true, dump bad EDID blocks to the console
  *
  * Validate a base or extension EDID block and optionally dump bad blocks to
  * the console.
  *
  * Return: True if the block is valid, false otherwise.
  */
-bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
+bool drm_edid_block_valid(u8 *raw_edid, int block)
 {
 	u8 csum;
 	struct edid *edid = (struct edid *)raw_edid;
@@ -1071,10 +1070,6 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
 
 	csum = drm_edid_block_checksum(raw_edid);
 	if (csum) {
-		if (print_bad_edid) {
-			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;
@@ -1099,19 +1094,29 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
 	return true;
 
 bad:
-	if (print_bad_edid) {
-		if (drm_edid_is_zero(raw_edid, EDID_LENGTH)) {
-			printk(KERN_ERR "EDID block is all zeroes\n");
-		} else {
-			printk(KERN_ERR "Raw EDID:\n");
-			print_hex_dump(KERN_ERR, " \t", DUMP_PREFIX_NONE, 16, 1,
-			       raw_edid, EDID_LENGTH, false);
-		}
-	}
 	return false;
 }
 EXPORT_SYMBOL(drm_edid_block_valid);
 
+static void drm_edid_block_dump(u8 *raw_edid, int block)
+{
+	u8 csum;
+
+	if (drm_edid_is_zero(raw_edid, EDID_LENGTH)) {
+		printk(KERN_NOTICE "EDID block %d is all zeroes.\n", block);
+		return;
+	}
+
+	if ((csum = drm_edid_block_checksum(raw_edid)))
+		printk(KERN_NOTICE "EDID block %d checksum is invalid, remainder is %d\n", block, csum);
+	else
+		printk(KERN_NOTICE "EDID block %d invalid.\n", block);
+
+	printk(KERN_NOTICE "Raw EDID:\n");
+	print_hex_dump(KERN_NOTICE, " \t", DUMP_PREFIX_NONE, 16, 1,
+		       raw_edid, EDID_LENGTH, false);
+}
+
 /**
  * drm_edid_is_valid - sanity check EDID data
  * @edid: EDID data
@@ -1129,8 +1134,10 @@ 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_valid(raw + i * EDID_LENGTH, i)) {
+			drm_edid_block_dump(raw + i * EDID_LENGTH, i);
 			return false;
+		}
 
 	return true;
 }
@@ -1221,7 +1228,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 			      size_t len),
 	void *data)
 {
-	int i, j = 0, valid_extensions = 0;
+	int i, j = 0, valid_extensions = -1;
 	u8 *block, *new;
 	bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
 
@@ -1232,7 +1239,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 	for (i = 0; i < 4; i++) {
 		if (get_edid_block(data, block, 0, EDID_LENGTH))
 			goto out;
-		if (drm_edid_block_valid(block, 0, print_bad_edid))
+		if (drm_edid_block_valid(block, 0))
 			break;
 		if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) {
 			connector->null_edid_counter++;
@@ -1251,13 +1258,14 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 		goto out;
 	block = new;
 
+	valid_extensions = 0;
 	for (j = 1; j <= block[0x7e]; j++) {
 		for (i = 0; i < 4; i++) {
 			if (get_edid_block(data,
 				  block + (valid_extensions + 1) * EDID_LENGTH,
 				  j, EDID_LENGTH))
 				goto out;
-			if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) {
+			if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j)) {
 				valid_extensions++;
 				break;
 			}
@@ -1265,14 +1273,25 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 
 		if (i == 4 && print_bad_edid) {
 			dev_warn(connector->dev->dev,
-			 "%s: Ignoring invalid EDID block %d.\n",
-			 connector->name, j);
+				 "%s: Ignoring invalid EDID block %d.\n",
+				 connector->name, j);
+
+			drm_edid_block_dump(block + (valid_extensions+1) * EDID_LENGTH,
+					    j);
 
 			connector->bad_edid_counter++;
 		}
 	}
 
 	if (valid_extensions != block[0x7e]) {
+		if (print_bad_edid) {
+			dev_warn(connector->dev->dev,
+				 "Found %d valid extensions instead of %d in "
+				 "EDID for connector \"%s\"\n",
+				 valid_extensions,
+				 block[0x7e], connector->name);
+		}
+
 		block[EDID_LENGTH-1] += block[0x7e] - valid_extensions;
 		block[0x7e] = valid_extensions;
 		new = krealloc(block, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
@@ -1287,6 +1306,9 @@ carp:
 	if (print_bad_edid) {
 		dev_warn(connector->dev->dev, "%s: EDID block %d invalid.\n",
 			 connector->name, j);
+
+		drm_edid_block_dump(block + (valid_extensions+1) * EDID_LENGTH,
+				    j);
 	}
 	connector->bad_edid_counter++;
 
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 732cb6f8e653..f6a4408cb096 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -160,15 +160,20 @@ static int edid_size(const u8 *edid, int data_size)
 	return (edid[0x7e] + 1) * EDID_LENGTH;
 }
 
+static int get_edid_block(void *data, u8 *buf, unsigned int block, size_t len)
+{
+	memcpy(buf, data + block * EDID_LENGTH, EDID_LENGTH);
+	return 0;
+}
+
 static void *edid_load(struct drm_connector *connector, const char *name,
 			const char *connector_name)
 {
 	const struct firmware *fw = NULL;
 	const u8 *fwdata;
-	u8 *edid;
+	struct edid *edid;
 	int fwsize, builtin;
-	int i, valid_extensions = 0;
-	bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
+	int i;
 
 	builtin = 0;
 	for (i = 0; i < GENERIC_EDIDS; i++) {
@@ -210,49 +215,18 @@ static void *edid_load(struct drm_connector *connector, const char *name,
 		goto out;
 	}
 
-	edid = kmemdup(fwdata, fwsize, GFP_KERNEL);
-	if (edid == NULL) {
-		edid = ERR_PTR(-ENOMEM);
-		goto out;
-	}
-
-	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);
+	edid = drm_do_get_edid(connector, get_edid_block, (void *)fwdata);
+	if (edid) {
+		DRM_INFO("Got %s EDID base block and %d extension%s from "
+			 "\"%s\" for connector \"%s\"\n",
+			 builtin ? "built-in" : "external",
+			 ((u8 *)edid)[0x7e], ((u8 *)edid)[0x7e] == 1 ? "" : "s",
+			 name, connector_name);
+	} else {
+		DRM_ERROR("EDID firmware \"%s\" is invalid ", name);
 		edid = ERR_PTR(-EINVAL);
-		goto 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]) {
-		u8 *new_edid;
-
-		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)
-			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);
-
 out:
 	release_firmware(fw);
 	return edid;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index da83d39e37d4..15278fd14739 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1441,7 +1441,7 @@ extern void drm_set_preferred_mode(struct drm_connector *connector,
 				   int hpref, int vpref);
 
 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_block_valid(u8 *raw_edid, int block);
 extern bool drm_edid_is_valid(struct edid *edid);
 
 extern struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev,
-- 
2.1.4

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

Re: [PATCH] drm: Only warn about an invalid EDID block prior to rejection

[Ville Syrjälä]

On Mon, Mar 09, 2015 at 11:44:05AM +0000, Chris Wilson wrote:
> On a noisy link, we may retry the EDID reads multiple times per block
> and still succeed. In such cases, we don't want to flood the kernel logs
> with *ERROR* messages as we then succeed. Refactor the EDID dumping and
> push it into the caller rather than the validator so we can suppress the
> warnings from transient failures. In the process, we want to refactor
> drm_load_edid_firmware() to use the common drm_do_get_edid() to share
> the same logic (since it already duplicates it).
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=89454

Could we get it to dump the number of retries it had to make to get a
valid EDID? The retries could be due to driver bugs so we may want to
know that they in fact occured. Otherwise we might end up with eg.
large delays during resume without any indication what caused them.

> ---
>  drivers/gpu/drm/drm_edid.c      | 64 +++++++++++++++++++++++++++--------------
>  drivers/gpu/drm/drm_edid_load.c | 60 +++++++++++---------------------------
>  include/drm/drm_crtc.h          |  2 +-
>  3 files changed, 61 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 53bc7a628909..024b47ffb065 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1040,14 +1040,13 @@ static bool drm_edid_is_zero(const u8 *in_edid, int length)
>   * drm_edid_block_valid - Sanity check the EDID block (base or extension)
>   * @raw_edid: pointer to raw EDID block
>   * @block: type of block to validate (0 for base, extension otherwise)
> - * @print_bad_edid: if true, dump bad EDID blocks to the console
>   *
>   * Validate a base or extension EDID block and optionally dump bad blocks to
>   * the console.
>   *
>   * Return: True if the block is valid, false otherwise.
>   */
> -bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
> +bool drm_edid_block_valid(u8 *raw_edid, int block)
>  {
>  	u8 csum;
>  	struct edid *edid = (struct edid *)raw_edid;
> @@ -1071,10 +1070,6 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
>  
>  	csum = drm_edid_block_checksum(raw_edid);
>  	if (csum) {
> -		if (print_bad_edid) {
> -			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;
> @@ -1099,19 +1094,29 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
>  	return true;
>  
>  bad:
> -	if (print_bad_edid) {
> -		if (drm_edid_is_zero(raw_edid, EDID_LENGTH)) {
> -			printk(KERN_ERR "EDID block is all zeroes\n");
> -		} else {
> -			printk(KERN_ERR "Raw EDID:\n");
> -			print_hex_dump(KERN_ERR, " \t", DUMP_PREFIX_NONE, 16, 1,
> -			       raw_edid, EDID_LENGTH, false);
> -		}
> -	}
>  	return false;
>  }
>  EXPORT_SYMBOL(drm_edid_block_valid);
>  
> +static void drm_edid_block_dump(u8 *raw_edid, int block)
> +{
> +	u8 csum;
> +
> +	if (drm_edid_is_zero(raw_edid, EDID_LENGTH)) {
> +		printk(KERN_NOTICE "EDID block %d is all zeroes.\n", block);
> +		return;
> +	}
> +
> +	if ((csum = drm_edid_block_checksum(raw_edid)))
> +		printk(KERN_NOTICE "EDID block %d checksum is invalid, remainder is %d\n", block, csum);
> +	else
> +		printk(KERN_NOTICE "EDID block %d invalid.\n", block);
> +
> +	printk(KERN_NOTICE "Raw EDID:\n");
> +	print_hex_dump(KERN_NOTICE, " \t", DUMP_PREFIX_NONE, 16, 1,
> +		       raw_edid, EDID_LENGTH, false);
> +}
> +
>  /**
>   * drm_edid_is_valid - sanity check EDID data
>   * @edid: EDID data
> @@ -1129,8 +1134,10 @@ 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_valid(raw + i * EDID_LENGTH, i)) {
> +			drm_edid_block_dump(raw + i * EDID_LENGTH, i);
>  			return false;
> +		}
>  
>  	return true;
>  }
> @@ -1221,7 +1228,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>  			      size_t len),
>  	void *data)
>  {
> -	int i, j = 0, valid_extensions = 0;
> +	int i, j = 0, valid_extensions = -1;
>  	u8 *block, *new;
>  	bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
>  
> @@ -1232,7 +1239,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>  	for (i = 0; i < 4; i++) {
>  		if (get_edid_block(data, block, 0, EDID_LENGTH))
>  			goto out;
> -		if (drm_edid_block_valid(block, 0, print_bad_edid))
> +		if (drm_edid_block_valid(block, 0))
>  			break;
>  		if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) {
>  			connector->null_edid_counter++;
> @@ -1251,13 +1258,14 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>  		goto out;
>  	block = new;
>  
> +	valid_extensions = 0;
>  	for (j = 1; j <= block[0x7e]; j++) {
>  		for (i = 0; i < 4; i++) {
>  			if (get_edid_block(data,
>  				  block + (valid_extensions + 1) * EDID_LENGTH,
>  				  j, EDID_LENGTH))
>  				goto out;
> -			if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) {
> +			if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j)) {
>  				valid_extensions++;
>  				break;
>  			}
> @@ -1265,14 +1273,25 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>  
>  		if (i == 4 && print_bad_edid) {
>  			dev_warn(connector->dev->dev,
> -			 "%s: Ignoring invalid EDID block %d.\n",
> -			 connector->name, j);
> +				 "%s: Ignoring invalid EDID block %d.\n",
> +				 connector->name, j);
> +
> +			drm_edid_block_dump(block + (valid_extensions+1) * EDID_LENGTH,
> +					    j);
>  
>  			connector->bad_edid_counter++;
>  		}
>  	}
>  
>  	if (valid_extensions != block[0x7e]) {
> +		if (print_bad_edid) {
> +			dev_warn(connector->dev->dev,
> +				 "Found %d valid extensions instead of %d in "
> +				 "EDID for connector \"%s\"\n",
> +				 valid_extensions,
> +				 block[0x7e], connector->name);
> +		}
> +
>  		block[EDID_LENGTH-1] += block[0x7e] - valid_extensions;
>  		block[0x7e] = valid_extensions;
>  		new = krealloc(block, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
> @@ -1287,6 +1306,9 @@ carp:
>  	if (print_bad_edid) {
>  		dev_warn(connector->dev->dev, "%s: EDID block %d invalid.\n",
>  			 connector->name, j);
> +
> +		drm_edid_block_dump(block + (valid_extensions+1) * EDID_LENGTH,
> +				    j);
>  	}
>  	connector->bad_edid_counter++;
>  
> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
> index 732cb6f8e653..f6a4408cb096 100644
> --- a/drivers/gpu/drm/drm_edid_load.c
> +++ b/drivers/gpu/drm/drm_edid_load.c
> @@ -160,15 +160,20 @@ static int edid_size(const u8 *edid, int data_size)
>  	return (edid[0x7e] + 1) * EDID_LENGTH;
>  }
>  
> +static int get_edid_block(void *data, u8 *buf, unsigned int block, size_t len)
> +{
> +	memcpy(buf, data + block * EDID_LENGTH, EDID_LENGTH);
> +	return 0;
> +}
> +
>  static void *edid_load(struct drm_connector *connector, const char *name,
>  			const char *connector_name)
>  {
>  	const struct firmware *fw = NULL;
>  	const u8 *fwdata;
> -	u8 *edid;
> +	struct edid *edid;
>  	int fwsize, builtin;
> -	int i, valid_extensions = 0;
> -	bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
> +	int i;
>  
>  	builtin = 0;
>  	for (i = 0; i < GENERIC_EDIDS; i++) {
> @@ -210,49 +215,18 @@ static void *edid_load(struct drm_connector *connector, const char *name,
>  		goto out;
>  	}
>  
> -	edid = kmemdup(fwdata, fwsize, GFP_KERNEL);
> -	if (edid == NULL) {
> -		edid = ERR_PTR(-ENOMEM);
> -		goto out;
> -	}
> -
> -	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);
> +	edid = drm_do_get_edid(connector, get_edid_block, (void *)fwdata);
> +	if (edid) {
> +		DRM_INFO("Got %s EDID base block and %d extension%s from "
> +			 "\"%s\" for connector \"%s\"\n",
> +			 builtin ? "built-in" : "external",
> +			 ((u8 *)edid)[0x7e], ((u8 *)edid)[0x7e] == 1 ? "" : "s",
> +			 name, connector_name);
> +	} else {
> +		DRM_ERROR("EDID firmware \"%s\" is invalid ", name);
>  		edid = ERR_PTR(-EINVAL);
> -		goto 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]) {
> -		u8 *new_edid;
> -
> -		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)
> -			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);
> -
>  out:
>  	release_firmware(fw);
>  	return edid;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index da83d39e37d4..15278fd14739 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1441,7 +1441,7 @@ extern void drm_set_preferred_mode(struct drm_connector *connector,
>  				   int hpref, int vpref);
>  
>  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_block_valid(u8 *raw_edid, int block);
>  extern bool drm_edid_is_valid(struct edid *edid);
>  
>  extern struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev,
> -- 
> 2.1.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Only warn about an invalid EDID block prior to rejection

[Chris Wilson]

On Mon, Mar 09, 2015 at 05:29:44PM +0200, Ville Syrjälä wrote:
> On Mon, Mar 09, 2015 at 11:44:05AM +0000, Chris Wilson wrote:
> > On a noisy link, we may retry the EDID reads multiple times per block
> > and still succeed. In such cases, we don't want to flood the kernel logs
> > with *ERROR* messages as we then succeed. Refactor the EDID dumping and
> > push it into the caller rather than the validator so we can suppress the
> > warnings from transient failures. In the process, we want to refactor
> > drm_load_edid_firmware() to use the common drm_do_get_edid() to share
> > the same logic (since it already duplicates it).
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=89454
> 
> Could we get it to dump the number of retries it had to make to get a
> valid EDID? The retries could be due to driver bugs so we may want to
> know that they in fact occured. Otherwise we might end up with eg.
> large delays during resume without any indication what caused them.

We also log the number of times the edid was bad and the edid was zero.
Knowing how many times we retried would be nice as well. Seems like it
should be possible to output that to debugfs/.../connector/edid_info ?

What interface do you envisage being useful? I'm not convinced adding it
to the user dmesg would be useful, but can a debugfs be integrated into
whatever test you have in mind?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Only warn about an invalid EDID block prior to rejection

[Ville Syrjälä]

On Mon, Mar 09, 2015 at 03:39:01PM +0000, Chris Wilson wrote:
> On Mon, Mar 09, 2015 at 05:29:44PM +0200, Ville Syrjälä wrote:
> > On Mon, Mar 09, 2015 at 11:44:05AM +0000, Chris Wilson wrote:
> > > On a noisy link, we may retry the EDID reads multiple times per block
> > > and still succeed. In such cases, we don't want to flood the kernel logs
> > > with *ERROR* messages as we then succeed. Refactor the EDID dumping and
> > > push it into the caller rather than the validator so we can suppress the
> > > warnings from transient failures. In the process, we want to refactor
> > > drm_load_edid_firmware() to use the common drm_do_get_edid() to share
> > > the same logic (since it already duplicates it).
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > References: https://bugs.freedesktop.org/show_bug.cgi?id=89454
> > 
> > Could we get it to dump the number of retries it had to make to get a
> > valid EDID? The retries could be due to driver bugs so we may want to
> > know that they in fact occured. Otherwise we might end up with eg.
> > large delays during resume without any indication what caused them.
> 
> We also log the number of times the edid was bad and the edid was zero.

I don't see where we log those. We do count them though.

> Knowing how many times we retried would be nice as well. Seems like it
> should be possible to output that to debugfs/.../connector/edid_info ?
> 
> What interface do you envisage being useful? I'm not convinced adding it
> to the user dmesg would be useful, but can a debugfs be integrated into
> whatever test you have in mind?

Well I just figure having it in the dmesg would be OK. That way when we
get a bug titled 'Resume takes forever' we can just ask for the dmesg as
usual, and we'd be able to see that something fishy was going on with
the EDID reads.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Only warn about an invalid EDID block prior to rejection

[Chris Wilson]

On Mon, Mar 09, 2015 at 06:04:46PM +0200, Ville Syrjälä wrote:
> On Mon, Mar 09, 2015 at 03:39:01PM +0000, Chris Wilson wrote:
> > On Mon, Mar 09, 2015 at 05:29:44PM +0200, Ville Syrjälä wrote:
> > > On Mon, Mar 09, 2015 at 11:44:05AM +0000, Chris Wilson wrote:
> > > > On a noisy link, we may retry the EDID reads multiple times per block
> > > > and still succeed. In such cases, we don't want to flood the kernel logs
> > > > with *ERROR* messages as we then succeed. Refactor the EDID dumping and
> > > > push it into the caller rather than the validator so we can suppress the
> > > > warnings from transient failures. In the process, we want to refactor
> > > > drm_load_edid_firmware() to use the common drm_do_get_edid() to share
> > > > the same logic (since it already duplicates it).
> > > > 
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > References: https://bugs.freedesktop.org/show_bug.cgi?id=89454
> > > 
> > > Could we get it to dump the number of retries it had to make to get a
> > > valid EDID? The retries could be due to driver bugs so we may want to
> > > know that they in fact occured. Otherwise we might end up with eg.
> > > large delays during resume without any indication what caused them.
> > 
> > We also log the number of times the edid was bad and the edid was zero.
> 
> I don't see where we log those. We do count them though.
> 
> > Knowing how many times we retried would be nice as well. Seems like it
> > should be possible to output that to debugfs/.../connector/edid_info ?
> > 
> > What interface do you envisage being useful? I'm not convinced adding it
> > to the user dmesg would be useful, but can a debugfs be integrated into
> > whatever test you have in mind?
> 
> Well I just figure having it in the dmesg would be OK. That way when we
> get a bug titled 'Resume takes forever' we can just ask for the dmesg as
> usual, and we'd be able to see that something fishy was going on with
> the EDID reads.

Wouldn't we be better off with the timestamps on log functions calls
during resume. (I thought there was already an option to provide that.)
Alternatively, Daniel was looking at adding a big lart to anything
triggering an edid read. Similarly, we should be able to get better
information using ftrace. The counter argument is that they are all
substantially more difficult to setup than just asking the user to
enable drm.debug.

How about something like adding:
DRM_INFO("Reading EDID for connector->name took %s (%d extension blocks,
%d total blocks read)\n");
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm: Only warn about an invalid EDID block prior to rejection

[Daniel Vetter]

On Mon, Mar 09, 2015 at 05:29:44PM +0200, Ville Syrjälä wrote:
> On Mon, Mar 09, 2015 at 11:44:05AM +0000, Chris Wilson wrote:
> > On a noisy link, we may retry the EDID reads multiple times per block
> > and still succeed. In such cases, we don't want to flood the kernel logs
> > with *ERROR* messages as we then succeed. Refactor the EDID dumping and
> > push it into the caller rather than the validator so we can suppress the
> > warnings from transient failures. In the process, we want to refactor
> > drm_load_edid_firmware() to use the common drm_do_get_edid() to share
> > the same logic (since it already duplicates it).
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=89454
> 
> Could we get it to dump the number of retries it had to make to get a
> valid EDID? The retries could be due to driver bugs so we may want to
> know that they in fact occured. Otherwise we might end up with eg.
> large delays during resume without any indication what caused them.

Just aside I'm hacking around on latencytop stuff so that edid reads show
up as latency sources. Often they're done as cpu spinning (so don't show
up at all). And if not there's huge issues with piles of retries (like
here) or just very short sleeps (gmbus can only transfer 4 bytes a time).
But that would just augment whatever other debug information we dump into
dmesg.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Only warn about an invalid EDID block prior to rejection

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5917
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  282/282              282/282
ILK                                  308/308              308/308
SNB                                  307/307              307/307
IVB                 -1              375/375              374/375
BYT                                  294/294              294/294
HSW                                  385/385              385/385
BDW                                  315/315              315/315
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*IVB  igt_gem_userptr_blits_minor-normal-sync      PASS(3)      DMESG_WARN(2)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx