[PATCH] drm/edid: Only print the bad edid when aborting

[PATCH] drm/edid: Only print the bad edid when aborting

[Chris Wilson]

Currently, if drm.debug is enabled, we get a DRM_ERROR message on the
intermediate edid reads. This causes transient failures in CI which
flags up the sporadic EDID read failures, which are recovered by
rereading the EDID automatically. This patch combines the reporting done
by drm_do_get_edid() itself with the bad block printing from
get_edid_block(), into a single warning associated with the connector
once all attempts to retrieve the EDID fail.

References: https://bugs.freedesktop.org/show_bug.cgi?id=98228
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/drm_edid.c | 82 +++++++++++++++++++++++++---------------------
 1 file changed, 44 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index ec77bd3e1f08..51dd10c65b53 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1260,6 +1260,26 @@ drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned int block, size_t len)
 	return ret == xfers ? 0 : -1;
 }
 
+static void connector_add_bad_edid(struct drm_connector *connector,
+				   u8 *block, int num)
+{
+	if (connector->bad_edid_counter++ && !(drm_debug & DRM_UT_KMS))
+		return;
+
+	if (drm_edid_is_zero(block, EDID_LENGTH)) {
+		dev_warn(connector->dev->dev,
+			 "%s: EDID block %d is all zeroes.\n",
+			 connector->name, num);
+	} else {
+		dev_warn(connector->dev->dev,
+			 "%s: EDID block %d invalid:\n",
+			 connector->name, num);
+		print_hex_dump(KERN_WARNING,
+			       " \t", DUMP_PREFIX_NONE, 16, 1,
+			       block, EDID_LENGTH, false);
+	}
+}
+
 /**
  * drm_do_get_edid - get EDID data using a custom EDID block read function
  * @connector: connector we're probing
@@ -1282,20 +1302,19 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 	void *data)
 {
 	int i, j = 0, valid_extensions = 0;
-	u8 *block, *new;
-	bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
+	u8 *edid, *new;
 
-	if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
+	if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
 		return NULL;
 
 	/* base block fetch */
 	for (i = 0; i < 4; i++) {
-		if (get_edid_block(data, block, 0, EDID_LENGTH))
+		if (get_edid_block(data, edid, 0, EDID_LENGTH))
 			goto out;
-		if (drm_edid_block_valid(block, 0, print_bad_edid,
+		if (drm_edid_block_valid(edid, 0, false,
 					 &connector->edid_corrupt))
 			break;
-		if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) {
+		if (i == 0 && drm_edid_is_zero(edid, EDID_LENGTH)) {
 			connector->null_edid_counter++;
 			goto carp;
 		}
@@ -1304,58 +1323,45 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 		goto carp;
 
 	/* if there's no extensions, we're done */
-	if (block[0x7e] == 0)
-		return (struct edid *)block;
+	if (edid[0x7e] == 0)
+		return (struct edid *)edid;
 
-	new = krealloc(block, (block[0x7e] + 1) * EDID_LENGTH, GFP_KERNEL);
+	new = krealloc(edid, (edid[0x7e] + 1) * EDID_LENGTH, GFP_KERNEL);
 	if (!new)
 		goto out;
-	block = new;
+	edid = new;
+
+	for (j = 1; j <= edid[0x7e]; j++) {
+		u8 *block = edid + (valid_extensions + 1) * EDID_LENGTH;
 
-	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))
+			if (get_edid_block(data, block, j, EDID_LENGTH))
 				goto out;
-			if (drm_edid_block_valid(block + (valid_extensions + 1)
-						 * EDID_LENGTH, j,
-						 print_bad_edid,
-						 NULL)) {
+			if (drm_edid_block_valid(block, j, false, NULL)) {
 				valid_extensions++;
 				break;
 			}
 		}
 
-		if (i == 4 && print_bad_edid) {
-			dev_warn(connector->dev->dev,
-			 "%s: Ignoring invalid EDID block %d.\n",
-			 connector->name, j);
-
-			connector->bad_edid_counter++;
-		}
+		if (i == 4)
+			connector_add_bad_edid(connector, block, j);
 	}
 
-	if (valid_extensions != block[0x7e]) {
-		block[EDID_LENGTH-1] += block[0x7e] - valid_extensions;
-		block[0x7e] = valid_extensions;
-		new = krealloc(block, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
+	if (valid_extensions != edid[0x7e]) {
+		edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
+		edid[0x7e] = valid_extensions;
+		new = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
 		if (!new)
 			goto out;
-		block = new;
+		edid = new;
 	}
 
-	return (struct edid *)block;
+	return (struct edid *)edid;
 
 carp:
-	if (print_bad_edid) {
-		dev_warn(connector->dev->dev, "%s: EDID block %d invalid.\n",
-			 connector->name, j);
-	}
-	connector->bad_edid_counter++;
-
+	connector_add_bad_edid(connector, edid, 0);
 out:
-	kfree(block);
+	kfree(edid);
 	return NULL;
 }
 EXPORT_SYMBOL_GPL(drm_do_get_edid);
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/edid: Only print the bad edid when aborting

[Ville Syrjälä]

On Thu, Oct 13, 2016 at 08:43:55PM +0100, Chris Wilson wrote:
> Currently, if drm.debug is enabled, we get a DRM_ERROR message on the
> intermediate edid reads. This causes transient failures in CI which
> flags up the sporadic EDID read failures, which are recovered by
> rereading the EDID automatically. This patch combines the reporting done
> by drm_do_get_edid() itself with the bad block printing from
> get_edid_block(), into a single warning associated with the connector
> once all attempts to retrieve the EDID fail.

One question is why have been getting more of these corrupt EDID reads
recently. Due to your gmbus change, or the live status revert perhaps?

> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=98228
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/drm_edid.c | 82 +++++++++++++++++++++++++---------------------
>  1 file changed, 44 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index ec77bd3e1f08..51dd10c65b53 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1260,6 +1260,26 @@ drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned int block, size_t len)
>  	return ret == xfers ? 0 : -1;
>  }
>  
> +static void connector_add_bad_edid(struct drm_connector *connector,
> +				   u8 *block, int num)
> +{
> +	if (connector->bad_edid_counter++ && !(drm_debug & DRM_UT_KMS))
> +		return;
> +
> +	if (drm_edid_is_zero(block, EDID_LENGTH)) {
> +		dev_warn(connector->dev->dev,
> +			 "%s: EDID block %d is all zeroes.\n",
> +			 connector->name, num);
> +	} else {
> +		dev_warn(connector->dev->dev,
> +			 "%s: EDID block %d invalid:\n",
> +			 connector->name, num);
> +		print_hex_dump(KERN_WARNING,
> +			       " \t", DUMP_PREFIX_NONE, 16, 1,
> +			       block, EDID_LENGTH, false);
> +	}
> +}
> +
>  /**
>   * drm_do_get_edid - get EDID data using a custom EDID block read function
>   * @connector: connector we're probing
> @@ -1282,20 +1302,19 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>  	void *data)
>  {
>  	int i, j = 0, valid_extensions = 0;
> -	u8 *block, *new;
> -	bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
> +	u8 *edid, *new;
>  
> -	if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
> +	if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
>  		return NULL;
>  
>  	/* base block fetch */
>  	for (i = 0; i < 4; i++) {
> -		if (get_edid_block(data, block, 0, EDID_LENGTH))
> +		if (get_edid_block(data, edid, 0, EDID_LENGTH))
>  			goto out;
> -		if (drm_edid_block_valid(block, 0, print_bad_edid,
> +		if (drm_edid_block_valid(edid, 0, false,
>  					 &connector->edid_corrupt))
>  			break;
> -		if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) {
> +		if (i == 0 && drm_edid_is_zero(edid, EDID_LENGTH)) {
>  			connector->null_edid_counter++;
>  			goto carp;
>  		}
> @@ -1304,58 +1323,45 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>  		goto carp;
>  
>  	/* if there's no extensions, we're done */
> -	if (block[0x7e] == 0)
> -		return (struct edid *)block;
> +	if (edid[0x7e] == 0)
> +		return (struct edid *)edid;
>  
> -	new = krealloc(block, (block[0x7e] + 1) * EDID_LENGTH, GFP_KERNEL);
> +	new = krealloc(edid, (edid[0x7e] + 1) * EDID_LENGTH, GFP_KERNEL);
>  	if (!new)
>  		goto out;
> -	block = new;
> +	edid = new;
> +
> +	for (j = 1; j <= edid[0x7e]; j++) {
> +		u8 *block = edid + (valid_extensions + 1) * EDID_LENGTH;
>  
> -	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))
> +			if (get_edid_block(data, block, j, EDID_LENGTH))
>  				goto out;
> -			if (drm_edid_block_valid(block + (valid_extensions + 1)
> -						 * EDID_LENGTH, j,
> -						 print_bad_edid,
> -						 NULL)) {
> +			if (drm_edid_block_valid(block, j, false, NULL)) {
>  				valid_extensions++;
>  				break;
>  			}
>  		}
>  
> -		if (i == 4 && print_bad_edid) {
> -			dev_warn(connector->dev->dev,
> -			 "%s: Ignoring invalid EDID block %d.\n",
> -			 connector->name, j);
> -
> -			connector->bad_edid_counter++;
> -		}
> +		if (i == 4)
> +			connector_add_bad_edid(connector, block, j);
>  	}
>  
> -	if (valid_extensions != block[0x7e]) {
> -		block[EDID_LENGTH-1] += block[0x7e] - valid_extensions;
> -		block[0x7e] = valid_extensions;
> -		new = krealloc(block, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
> +	if (valid_extensions != edid[0x7e]) {
> +		edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
> +		edid[0x7e] = valid_extensions;
> +		new = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
>  		if (!new)
>  			goto out;
> -		block = new;
> +		edid = new;
>  	}
>  
> -	return (struct edid *)block;
> +	return (struct edid *)edid;
>  
>  carp:
> -	if (print_bad_edid) {
> -		dev_warn(connector->dev->dev, "%s: EDID block %d invalid.\n",
> -			 connector->name, j);
> -	}
> -	connector->bad_edid_counter++;
> -
> +	connector_add_bad_edid(connector, edid, 0);
>  out:
> -	kfree(block);
> +	kfree(edid);
>  	return NULL;
>  }
>  EXPORT_SYMBOL_GPL(drm_do_get_edid);
> -- 
> 2.9.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH] drm/edid: Only print the bad edid when aborting

[Chris Wilson]

On Fri, Oct 14, 2016 at 01:46:37PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 13, 2016 at 08:43:55PM +0100, Chris Wilson wrote:
> > Currently, if drm.debug is enabled, we get a DRM_ERROR message on the
> > intermediate edid reads. This causes transient failures in CI which
> > flags up the sporadic EDID read failures, which are recovered by
> > rereading the EDID automatically. This patch combines the reporting done
> > by drm_do_get_edid() itself with the bad block printing from
> > get_edid_block(), into a single warning associated with the connector
> > once all attempts to retrieve the EDID fail.
> 
> One question is why have been getting more of these corrupt EDID reads
> recently. Due to your gmbus change, or the live status revert perhaps?

The recent reports are from a new Ironlake setup aiui.
-Chris

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

Re: [PATCH] drm/edid: Only print the bad edid when aborting

[Daniel Vetter]

On Thu, Oct 13, 2016 at 08:43:55PM +0100, Chris Wilson wrote:
> Currently, if drm.debug is enabled, we get a DRM_ERROR message on the
> intermediate edid reads. This causes transient failures in CI which
> flags up the sporadic EDID read failures, which are recovered by
> rereading the EDID automatically. This patch combines the reporting done
> by drm_do_get_edid() itself with the bad block printing from
> get_edid_block(), into a single warning associated with the connector
> once all attempts to retrieve the EDID fail.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=98228
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Can I haz a version of this patch without the s/block/edid/? It confuses
me this early. If you want it, please split out.
-Daniel

> ---
>  drivers/gpu/drm/drm_edid.c | 82 +++++++++++++++++++++++++---------------------
>  1 file changed, 44 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index ec77bd3e1f08..51dd10c65b53 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1260,6 +1260,26 @@ drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned int block, size_t len)
>  	return ret == xfers ? 0 : -1;
>  }
>  
> +static void connector_add_bad_edid(struct drm_connector *connector,
> +				   u8 *block, int num)
> +{
> +	if (connector->bad_edid_counter++ && !(drm_debug & DRM_UT_KMS))
> +		return;
> +
> +	if (drm_edid_is_zero(block, EDID_LENGTH)) {
> +		dev_warn(connector->dev->dev,
> +			 "%s: EDID block %d is all zeroes.\n",
> +			 connector->name, num);
> +	} else {
> +		dev_warn(connector->dev->dev,
> +			 "%s: EDID block %d invalid:\n",
> +			 connector->name, num);
> +		print_hex_dump(KERN_WARNING,
> +			       " \t", DUMP_PREFIX_NONE, 16, 1,
> +			       block, EDID_LENGTH, false);
> +	}
> +}
> +
>  /**
>   * drm_do_get_edid - get EDID data using a custom EDID block read function
>   * @connector: connector we're probing
> @@ -1282,20 +1302,19 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>  	void *data)
>  {
>  	int i, j = 0, valid_extensions = 0;
> -	u8 *block, *new;
> -	bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
> +	u8 *edid, *new;
>  
> -	if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
> +	if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
>  		return NULL;
>  
>  	/* base block fetch */
>  	for (i = 0; i < 4; i++) {
> -		if (get_edid_block(data, block, 0, EDID_LENGTH))
> +		if (get_edid_block(data, edid, 0, EDID_LENGTH))
>  			goto out;
> -		if (drm_edid_block_valid(block, 0, print_bad_edid,
> +		if (drm_edid_block_valid(edid, 0, false,
>  					 &connector->edid_corrupt))
>  			break;
> -		if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) {
> +		if (i == 0 && drm_edid_is_zero(edid, EDID_LENGTH)) {
>  			connector->null_edid_counter++;
>  			goto carp;
>  		}
> @@ -1304,58 +1323,45 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>  		goto carp;
>  
>  	/* if there's no extensions, we're done */
> -	if (block[0x7e] == 0)
> -		return (struct edid *)block;
> +	if (edid[0x7e] == 0)
> +		return (struct edid *)edid;
>  
> -	new = krealloc(block, (block[0x7e] + 1) * EDID_LENGTH, GFP_KERNEL);
> +	new = krealloc(edid, (edid[0x7e] + 1) * EDID_LENGTH, GFP_KERNEL);
>  	if (!new)
>  		goto out;
> -	block = new;
> +	edid = new;
> +
> +	for (j = 1; j <= edid[0x7e]; j++) {
> +		u8 *block = edid + (valid_extensions + 1) * EDID_LENGTH;
>  
> -	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))
> +			if (get_edid_block(data, block, j, EDID_LENGTH))
>  				goto out;
> -			if (drm_edid_block_valid(block + (valid_extensions + 1)
> -						 * EDID_LENGTH, j,
> -						 print_bad_edid,
> -						 NULL)) {
> +			if (drm_edid_block_valid(block, j, false, NULL)) {
>  				valid_extensions++;
>  				break;
>  			}
>  		}
>  
> -		if (i == 4 && print_bad_edid) {
> -			dev_warn(connector->dev->dev,
> -			 "%s: Ignoring invalid EDID block %d.\n",
> -			 connector->name, j);
> -
> -			connector->bad_edid_counter++;
> -		}
> +		if (i == 4)
> +			connector_add_bad_edid(connector, block, j);
>  	}
>  
> -	if (valid_extensions != block[0x7e]) {
> -		block[EDID_LENGTH-1] += block[0x7e] - valid_extensions;
> -		block[0x7e] = valid_extensions;
> -		new = krealloc(block, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
> +	if (valid_extensions != edid[0x7e]) {
> +		edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
> +		edid[0x7e] = valid_extensions;
> +		new = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
>  		if (!new)
>  			goto out;
> -		block = new;
> +		edid = new;
>  	}
>  
> -	return (struct edid *)block;
> +	return (struct edid *)edid;
>  
>  carp:
> -	if (print_bad_edid) {
> -		dev_warn(connector->dev->dev, "%s: EDID block %d invalid.\n",
> -			 connector->name, j);
> -	}
> -	connector->bad_edid_counter++;
> -
> +	connector_add_bad_edid(connector, edid, 0);
>  out:
> -	kfree(block);
> +	kfree(edid);
>  	return NULL;
>  }
>  EXPORT_SYMBOL_GPL(drm_do_get_edid);
> -- 
> 2.9.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/3] drm/edid: Rename local variable block to edid

[Chris Wilson]

The "block" variable points to the entire edid, not individual blocks
despite it being named such.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/drm_edid.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index ec77bd3e1f08..3b4ac28f509e 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1282,20 +1282,20 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 	void *data)
 {
 	int i, j = 0, valid_extensions = 0;
-	u8 *block, *new;
+	u8 *edid, *new;
 	bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
 
-	if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
+	if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
 		return NULL;
 
 	/* base block fetch */
 	for (i = 0; i < 4; i++) {
-		if (get_edid_block(data, block, 0, EDID_LENGTH))
+		if (get_edid_block(data, edid, 0, EDID_LENGTH))
 			goto out;
-		if (drm_edid_block_valid(block, 0, print_bad_edid,
+		if (drm_edid_block_valid(edid, 0, print_bad_edid,
 					 &connector->edid_corrupt))
 			break;
-		if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) {
+		if (i == 0 && drm_edid_is_zero(edid, EDID_LENGTH)) {
 			connector->null_edid_counter++;
 			goto carp;
 		}
@@ -1304,21 +1304,21 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 		goto carp;
 
 	/* if there's no extensions, we're done */
-	if (block[0x7e] == 0)
-		return (struct edid *)block;
+	if (edid[0x7e] == 0)
+		return (struct edid *)edid;
 
-	new = krealloc(block, (block[0x7e] + 1) * EDID_LENGTH, GFP_KERNEL);
+	new = krealloc(edid, (edid[0x7e] + 1) * EDID_LENGTH, GFP_KERNEL);
 	if (!new)
 		goto out;
-	block = new;
+	edid = new;
 
-	for (j = 1; j <= block[0x7e]; j++) {
+	for (j = 1; j <= edid[0x7e]; j++) {
 		for (i = 0; i < 4; i++) {
 			if (get_edid_block(data,
-				  block + (valid_extensions + 1) * EDID_LENGTH,
+				  edid + (valid_extensions + 1) * EDID_LENGTH,
 				  j, EDID_LENGTH))
 				goto out;
-			if (drm_edid_block_valid(block + (valid_extensions + 1)
+			if (drm_edid_block_valid(edid + (valid_extensions + 1)
 						 * EDID_LENGTH, j,
 						 print_bad_edid,
 						 NULL)) {
@@ -1336,16 +1336,16 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 		}
 	}
 
-	if (valid_extensions != block[0x7e]) {
-		block[EDID_LENGTH-1] += block[0x7e] - valid_extensions;
-		block[0x7e] = valid_extensions;
-		new = krealloc(block, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
+	if (valid_extensions != edid[0x7e]) {
+		edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
+		edid[0x7e] = valid_extensions;
+		new = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
 		if (!new)
 			goto out;
-		block = new;
+		edid = new;
 	}
 
-	return (struct edid *)block;
+	return (struct edid *)edid;
 
 carp:
 	if (print_bad_edid) {
@@ -1355,7 +1355,7 @@ carp:
 	connector->bad_edid_counter++;
 
 out:
-	kfree(block);
+	kfree(edid);
 	return NULL;
 }
 EXPORT_SYMBOL_GPL(drm_do_get_edid);
-- 
2.9.3

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

[PATCH 2/3] drm/edid: Use block local to refer to the block

[Chris Wilson]

Now that we have the name "block" free once more, we can use it to point
to the start of a block within the edid.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/drm_edid.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 3b4ac28f509e..95de47ba1e77 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1313,15 +1313,13 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 	edid = new;
 
 	for (j = 1; j <= edid[0x7e]; j++) {
+		u8 *block = edid + (valid_extensions + 1) * EDID_LENGTH;
+
 		for (i = 0; i < 4; i++) {
-			if (get_edid_block(data,
-				  edid + (valid_extensions + 1) * EDID_LENGTH,
-				  j, EDID_LENGTH))
+			if (get_edid_block(data, block, j, EDID_LENGTH))
 				goto out;
-			if (drm_edid_block_valid(edid + (valid_extensions + 1)
-						 * EDID_LENGTH, j,
-						 print_bad_edid,
-						 NULL)) {
+			if (drm_edid_block_valid(block, j,
+						 print_bad_edid, NULL)) {
 				valid_extensions++;
 				break;
 			}
-- 
2.9.3

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

[PATCH 3/3] drm/edid: Only print the bad edid when aborting

[Chris Wilson]

Currently, if drm.debug is enabled, we get a DRM_ERROR message on the
intermediate edid reads. This causes transient failures in CI which
flags up the sporadic EDID read failures, which are recovered by
rereading the EDID automatically. This patch combines the reporting done
by drm_do_get_edid() itself with the bad block printing from
get_edid_block(), into a single warning associated with the connector
once all attempts to retrieve the EDID fail.

References: https://bugs.freedesktop.org/show_bug.cgi?id=98228
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/drm_edid.c | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 95de47ba1e77..51dd10c65b53 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1260,6 +1260,26 @@ drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned int block, size_t len)
 	return ret == xfers ? 0 : -1;
 }
 
+static void connector_add_bad_edid(struct drm_connector *connector,
+				   u8 *block, int num)
+{
+	if (connector->bad_edid_counter++ && !(drm_debug & DRM_UT_KMS))
+		return;
+
+	if (drm_edid_is_zero(block, EDID_LENGTH)) {
+		dev_warn(connector->dev->dev,
+			 "%s: EDID block %d is all zeroes.\n",
+			 connector->name, num);
+	} else {
+		dev_warn(connector->dev->dev,
+			 "%s: EDID block %d invalid:\n",
+			 connector->name, num);
+		print_hex_dump(KERN_WARNING,
+			       " \t", DUMP_PREFIX_NONE, 16, 1,
+			       block, EDID_LENGTH, false);
+	}
+}
+
 /**
  * drm_do_get_edid - get EDID data using a custom EDID block read function
  * @connector: connector we're probing
@@ -1283,7 +1303,6 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 {
 	int i, j = 0, valid_extensions = 0;
 	u8 *edid, *new;
-	bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
 
 	if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
 		return NULL;
@@ -1292,7 +1311,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 	for (i = 0; i < 4; i++) {
 		if (get_edid_block(data, edid, 0, EDID_LENGTH))
 			goto out;
-		if (drm_edid_block_valid(edid, 0, print_bad_edid,
+		if (drm_edid_block_valid(edid, 0, false,
 					 &connector->edid_corrupt))
 			break;
 		if (i == 0 && drm_edid_is_zero(edid, EDID_LENGTH)) {
@@ -1318,20 +1337,14 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 		for (i = 0; i < 4; i++) {
 			if (get_edid_block(data, block, j, EDID_LENGTH))
 				goto out;
-			if (drm_edid_block_valid(block, j,
-						 print_bad_edid, NULL)) {
+			if (drm_edid_block_valid(block, j, false, NULL)) {
 				valid_extensions++;
 				break;
 			}
 		}
 
-		if (i == 4 && print_bad_edid) {
-			dev_warn(connector->dev->dev,
-			 "%s: Ignoring invalid EDID block %d.\n",
-			 connector->name, j);
-
-			connector->bad_edid_counter++;
-		}
+		if (i == 4)
+			connector_add_bad_edid(connector, block, j);
 	}
 
 	if (valid_extensions != edid[0x7e]) {
@@ -1346,12 +1359,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 	return (struct edid *)edid;
 
 carp:
-	if (print_bad_edid) {
-		dev_warn(connector->dev->dev, "%s: EDID block %d invalid.\n",
-			 connector->name, j);
-	}
-	connector->bad_edid_counter++;
-
+	connector_add_bad_edid(connector, edid, 0);
 out:
 	kfree(edid);
 	return NULL;
-- 
2.9.3

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

Re: [PATCH 3/3] drm/edid: Only print the bad edid when aborting

[Ville Syrjälä]

On Mon, Oct 17, 2016 at 09:35:14AM +0100, Chris Wilson wrote:
> Currently, if drm.debug is enabled, we get a DRM_ERROR message on the
> intermediate edid reads. This causes transient failures in CI which
> flags up the sporadic EDID read failures, which are recovered by
> rereading the EDID automatically. This patch combines the reporting done
> by drm_do_get_edid() itself with the bad block printing from
> get_edid_block(), into a single warning associated with the connector
> once all attempts to retrieve the EDID fail.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=98228
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/drm_edid.c | 42 +++++++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 95de47ba1e77..51dd10c65b53 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1260,6 +1260,26 @@ drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned int block, size_t len)
>  	return ret == xfers ? 0 : -1;
>  }
>  
> +static void connector_add_bad_edid(struct drm_connector *connector,
> +				   u8 *block, int num)
> +{
> +	if (connector->bad_edid_counter++ && !(drm_debug & DRM_UT_KMS))
> +		return;
> +
> +	if (drm_edid_is_zero(block, EDID_LENGTH)) {
> +		dev_warn(connector->dev->dev,
> +			 "%s: EDID block %d is all zeroes.\n",
> +			 connector->name, num);
> +	} else {
> +		dev_warn(connector->dev->dev,
> +			 "%s: EDID block %d invalid:\n",
> +			 connector->name, num);
> +		print_hex_dump(KERN_WARNING,
> +			       " \t", DUMP_PREFIX_NONE, 16, 1,
> +			       block, EDID_LENGTH, false);
> +	}
> +}
> +
>  /**
>   * drm_do_get_edid - get EDID data using a custom EDID block read function
>   * @connector: connector we're probing
> @@ -1283,7 +1303,6 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>  {
>  	int i, j = 0, valid_extensions = 0;
>  	u8 *edid, *new;
> -	bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
>  
>  	if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
>  		return NULL;
> @@ -1292,7 +1311,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>  	for (i = 0; i < 4; i++) {
>  		if (get_edid_block(data, edid, 0, EDID_LENGTH))
>  			goto out;
> -		if (drm_edid_block_valid(edid, 0, print_bad_edid,
> +		if (drm_edid_block_valid(edid, 0, false,
>  					 &connector->edid_corrupt))
>  			break;
>  		if (i == 0 && drm_edid_is_zero(edid, EDID_LENGTH)) {
> @@ -1318,20 +1337,14 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>  		for (i = 0; i < 4; i++) {
>  			if (get_edid_block(data, block, j, EDID_LENGTH))
>  				goto out;
> -			if (drm_edid_block_valid(block, j,
> -						 print_bad_edid, NULL)) {
> +			if (drm_edid_block_valid(block, j, false, NULL)) {
>  				valid_extensions++;
>  				break;
>  			}
>  		}
>  
> -		if (i == 4 && print_bad_edid) {
> -			dev_warn(connector->dev->dev,
> -			 "%s: Ignoring invalid EDID block %d.\n",
> -			 connector->name, j);
> -
> -			connector->bad_edid_counter++;
> -		}
> +		if (i == 4)
> +			connector_add_bad_edid(connector, block, j);

Hmm. So this will only print the first bad block we find. Should we
perhaps just dump out the entire EDID at the end, with the bad blocks
clearly marked as such?

>  	}
>  
>  	if (valid_extensions != edid[0x7e]) {
> @@ -1346,12 +1359,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>  	return (struct edid *)edid;
>  
>  carp:
> -	if (print_bad_edid) {
> -		dev_warn(connector->dev->dev, "%s: EDID block %d invalid.\n",
> -			 connector->name, j);
> -	}
> -	connector->bad_edid_counter++;
> -
> +	connector_add_bad_edid(connector, edid, 0);
>  out:
>  	kfree(edid);
>  	return NULL;
> -- 
> 2.9.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 2/3] drm/edid: Use block local to refer to the block

[Daniel Vetter]

On Mon, Oct 17, 2016 at 09:35:13AM +0100, Chris Wilson wrote:
> Now that we have the name "block" free once more, we can use it to point
> to the start of a block within the edid.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Merged the 2 prep patches to drm-misc for now. I'll wait with 3/3 until
you&Ville reach some agreement.

Thanks, Daniel

> ---
>  drivers/gpu/drm/drm_edid.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 3b4ac28f509e..95de47ba1e77 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1313,15 +1313,13 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>  	edid = new;
>  
>  	for (j = 1; j <= edid[0x7e]; j++) {
> +		u8 *block = edid + (valid_extensions + 1) * EDID_LENGTH;
> +
>  		for (i = 0; i < 4; i++) {
> -			if (get_edid_block(data,
> -				  edid + (valid_extensions + 1) * EDID_LENGTH,
> -				  j, EDID_LENGTH))
> +			if (get_edid_block(data, block, j, EDID_LENGTH))
>  				goto out;
> -			if (drm_edid_block_valid(edid + (valid_extensions + 1)
> -						 * EDID_LENGTH, j,
> -						 print_bad_edid,
> -						 NULL)) {
> +			if (drm_edid_block_valid(block, j,
> +						 print_bad_edid, NULL)) {
>  				valid_extensions++;
>  				break;
>  			}
> -- 
> 2.9.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2] drm/edid: Only print the bad edid when aborting

[Chris Wilson]

Currently, if drm.debug is enabled, we get a DRM_ERROR message on the
intermediate edid reads. This causes transient failures in CI which
flags up the sporadic EDID read failures, which are recovered by
rereading the EDID automatically. This patch combines the reporting done
by drm_do_get_edid() itself with the bad block printing from
get_edid_block(), into a single warning associated with the connector
once all attempts to retrieve the EDID fail.

v2: Print the whole EDID, marking up the bad/zero blocks. This requires
recording the whole of the raw edid, then a second pass to reduce it to
the valid extensions.

References: https://bugs.freedesktop.org/show_bug.cgi?id=98228
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/drm_edid.c | 78 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 55 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 95de47ba1e77..2789eb0b9162 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1260,6 +1260,34 @@ drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned int block, size_t len)
 	return ret == xfers ? 0 : -1;
 }
 
+static void connector_bad_edid(struct drm_connector *connector,
+			       u8 *edid, int num_blocks)
+{
+	int i;
+
+	if (connector->bad_edid_counter++ && !(drm_debug & DRM_UT_KMS))
+		return;
+
+	dev_warn(connector->dev->dev,
+		 "%s: EDID is invalid:\n",
+		 connector->name);
+	for (i = 0; i < num_blocks; i++) {
+		u8 *block = edid + i * EDID_LENGTH;
+		char prefix[20];
+
+		if (drm_edid_is_zero(block, EDID_LENGTH))
+			sprintf(prefix, "\t[%02x] ZERO ", i);
+		else if (!drm_edid_block_valid(block, i, false, NULL))
+			sprintf(prefix, "\t[%02x] BAD  ", i);
+		else
+			sprintf(prefix, "\t[%02x] GOOD ", i);
+
+		print_hex_dump(KERN_WARNING,
+			       prefix, DUMP_PREFIX_NONE, 16, 1,
+			       block, EDID_LENGTH, false);
+	}
+}
+
 /**
  * drm_do_get_edid - get EDID data using a custom EDID block read function
  * @connector: connector we're probing
@@ -1283,7 +1311,6 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 {
 	int i, j = 0, valid_extensions = 0;
 	u8 *edid, *new;
-	bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
 
 	if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
 		return NULL;
@@ -1292,7 +1319,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 	for (i = 0; i < 4; i++) {
 		if (get_edid_block(data, edid, 0, EDID_LENGTH))
 			goto out;
-		if (drm_edid_block_valid(edid, 0, print_bad_edid,
+		if (drm_edid_block_valid(edid, 0, false,
 					 &connector->edid_corrupt))
 			break;
 		if (i == 0 && drm_edid_is_zero(edid, EDID_LENGTH)) {
@@ -1304,54 +1331,59 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 		goto carp;
 
 	/* if there's no extensions, we're done */
-	if (edid[0x7e] == 0)
+	valid_extensions = edid[0x7e];
+	if (valid_extensions == 0)
 		return (struct edid *)edid;
 
-	new = krealloc(edid, (edid[0x7e] + 1) * EDID_LENGTH, GFP_KERNEL);
+	new = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
 	if (!new)
 		goto out;
 	edid = new;
 
 	for (j = 1; j <= edid[0x7e]; j++) {
-		u8 *block = edid + (valid_extensions + 1) * EDID_LENGTH;
+		u8 *block = edid + j * EDID_LENGTH;
 
 		for (i = 0; i < 4; i++) {
 			if (get_edid_block(data, block, j, EDID_LENGTH))
 				goto out;
-			if (drm_edid_block_valid(block, j,
-						 print_bad_edid, NULL)) {
-				valid_extensions++;
+			if (drm_edid_block_valid(block, j, false, NULL)) {
+				valid_extensions--;
 				break;
 			}
 		}
-
-		if (i == 4 && print_bad_edid) {
-			dev_warn(connector->dev->dev,
-			 "%s: Ignoring invalid EDID block %d.\n",
-			 connector->name, j);
-
-			connector->bad_edid_counter++;
-		}
 	}
 
 	if (valid_extensions != edid[0x7e]) {
+		u8 *base;
+
+		connector_bad_edid(connector, edid, edid[0x7e] + 1);
+
 		edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
 		edid[0x7e] = valid_extensions;
-		new = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
+
+		new = kmalloc((valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
 		if (!new)
 			goto out;
+
+		base = new;
+		for (i = 0; i <= edid[0x7e]; i++) {
+			u8 *block = edid + i * EDID_LENGTH;
+
+			if (!drm_edid_block_valid(block, i, false, NULL))
+				continue;
+
+			memcpy(base, block, EDID_LENGTH);
+			base += EDID_LENGTH;
+		}
+
+		kfree(edid);
 		edid = new;
 	}
 
 	return (struct edid *)edid;
 
 carp:
-	if (print_bad_edid) {
-		dev_warn(connector->dev->dev, "%s: EDID block %d invalid.\n",
-			 connector->name, j);
-	}
-	connector->bad_edid_counter++;
-
+	connector_bad_edid(connector, edid, 1);
 out:
 	kfree(edid);
 	return NULL;
-- 
2.10.1

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

Re: [PATCH v2] drm/edid: Only print the bad edid when aborting

[Chris Wilson]

On Mon, Oct 24, 2016 at 12:33:41PM +0100, Chris Wilson wrote:
>  	for (j = 1; j <= edid[0x7e]; j++) {
> -		u8 *block = edid + (valid_extensions + 1) * EDID_LENGTH;
> +		u8 *block = edid + j * EDID_LENGTH;
>  
>  		for (i = 0; i < 4; i++) {
>  			if (get_edid_block(data, block, j, EDID_LENGTH))
>  				goto out;
> -			if (drm_edid_block_valid(block, j,
> -						 print_bad_edid, NULL)) {
> -				valid_extensions++;
> +			if (drm_edid_block_valid(block, j, false, NULL)) {
> +				valid_extensions--;

Inverted.
-Chris

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

[PATCH v3] drm/edid: Only print the bad edid when aborting

[Chris Wilson]

Currently, if drm.debug is enabled, we get a DRM_ERROR message on the
intermediate edid reads. This causes transient failures in CI which
flags up the sporadic EDID read failures, which are recovered by
rereading the EDID automatically. This patch combines the reporting done
by drm_do_get_edid() itself with the bad block printing from
get_edid_block(), into a single warning associated with the connector
once all attempts to retrieve the EDID fail.

v2: Print the whole EDID, marking up the bad/zero blocks. This requires
recording the whole of the raw edid, then a second pass to reduce it to
the valid extensions.
v3: Fix invalid/valid extension fumble.

References: https://bugs.freedesktop.org/show_bug.cgi?id=98228
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_edid.c | 79 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 56 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 95de47ba1e77..9506933b41cd 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1260,6 +1260,34 @@ drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned int block, size_t len)
 	return ret == xfers ? 0 : -1;
 }
 
+static void connector_bad_edid(struct drm_connector *connector,
+			       u8 *edid, int num_blocks)
+{
+	int i;
+
+	if (connector->bad_edid_counter++ && !(drm_debug & DRM_UT_KMS))
+		return;
+
+	dev_warn(connector->dev->dev,
+		 "%s: EDID is invalid:\n",
+		 connector->name);
+	for (i = 0; i < num_blocks; i++) {
+		u8 *block = edid + i * EDID_LENGTH;
+		char prefix[20];
+
+		if (drm_edid_is_zero(block, EDID_LENGTH))
+			sprintf(prefix, "\t[%02x] ZERO ", i);
+		else if (!drm_edid_block_valid(block, i, false, NULL))
+			sprintf(prefix, "\t[%02x] BAD  ", i);
+		else
+			sprintf(prefix, "\t[%02x] GOOD ", i);
+
+		print_hex_dump(KERN_WARNING,
+			       prefix, DUMP_PREFIX_NONE, 16, 1,
+			       block, EDID_LENGTH, false);
+	}
+}
+
 /**
  * drm_do_get_edid - get EDID data using a custom EDID block read function
  * @connector: connector we're probing
@@ -1283,7 +1311,6 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 {
 	int i, j = 0, valid_extensions = 0;
 	u8 *edid, *new;
-	bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
 
 	if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
 		return NULL;
@@ -1292,7 +1319,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 	for (i = 0; i < 4; i++) {
 		if (get_edid_block(data, edid, 0, EDID_LENGTH))
 			goto out;
-		if (drm_edid_block_valid(edid, 0, print_bad_edid,
+		if (drm_edid_block_valid(edid, 0, false,
 					 &connector->edid_corrupt))
 			break;
 		if (i == 0 && drm_edid_is_zero(edid, EDID_LENGTH)) {
@@ -1304,54 +1331,60 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 		goto carp;
 
 	/* if there's no extensions, we're done */
-	if (edid[0x7e] == 0)
+	valid_extensions = edid[0x7e];
+	if (valid_extensions == 0)
 		return (struct edid *)edid;
 
-	new = krealloc(edid, (edid[0x7e] + 1) * EDID_LENGTH, GFP_KERNEL);
+	new = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
 	if (!new)
 		goto out;
 	edid = new;
 
 	for (j = 1; j <= edid[0x7e]; j++) {
-		u8 *block = edid + (valid_extensions + 1) * EDID_LENGTH;
+		u8 *block = edid + j * EDID_LENGTH;
 
 		for (i = 0; i < 4; i++) {
 			if (get_edid_block(data, block, j, EDID_LENGTH))
 				goto out;
-			if (drm_edid_block_valid(block, j,
-						 print_bad_edid, NULL)) {
-				valid_extensions++;
+			if (drm_edid_block_valid(block, j, false, NULL))
 				break;
-			}
 		}
 
-		if (i == 4 && print_bad_edid) {
-			dev_warn(connector->dev->dev,
-			 "%s: Ignoring invalid EDID block %d.\n",
-			 connector->name, j);
-
-			connector->bad_edid_counter++;
-		}
+		if (i == 4)
+			valid_extensions--;
 	}
 
 	if (valid_extensions != edid[0x7e]) {
+		u8 *base;
+
+		connector_bad_edid(connector, edid, edid[0x7e] + 1);
+
 		edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
 		edid[0x7e] = valid_extensions;
-		new = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
+
+		new = kmalloc((valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
 		if (!new)
 			goto out;
+
+		base = new;
+		for (i = 0; i <= edid[0x7e]; i++) {
+			u8 *block = edid + i * EDID_LENGTH;
+
+			if (!drm_edid_block_valid(block, i, false, NULL))
+				continue;
+
+			memcpy(base, block, EDID_LENGTH);
+			base += EDID_LENGTH;
+		}
+
+		kfree(edid);
 		edid = new;
 	}
 
 	return (struct edid *)edid;
 
 carp:
-	if (print_bad_edid) {
-		dev_warn(connector->dev->dev, "%s: EDID block %d invalid.\n",
-			 connector->name, j);
-	}
-	connector->bad_edid_counter++;
-
+	connector_bad_edid(connector, edid, 1);
 out:
 	kfree(edid);
 	return NULL;
-- 
2.10.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/edid: Only print the bad edid when aborting

[Sean Paul]

On Mon, Oct 24, 2016 at 7:38 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Currently, if drm.debug is enabled, we get a DRM_ERROR message on the
> intermediate edid reads. This causes transient failures in CI which
> flags up the sporadic EDID read failures, which are recovered by
> rereading the EDID automatically. This patch combines the reporting done
> by drm_do_get_edid() itself with the bad block printing from
> get_edid_block(), into a single warning associated with the connector
> once all attempts to retrieve the EDID fail.
>
> v2: Print the whole EDID, marking up the bad/zero blocks. This requires
> recording the whole of the raw edid, then a second pass to reduce it to
> the valid extensions.
> v3: Fix invalid/valid extension fumble.
>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=98228
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>

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

> ---
>  drivers/gpu/drm/drm_edid.c | 79 ++++++++++++++++++++++++++++++++--------------
>  1 file changed, 56 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 95de47ba1e77..9506933b41cd 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1260,6 +1260,34 @@ drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned int block, size_t len)
>         return ret == xfers ? 0 : -1;
>  }
>
> +static void connector_bad_edid(struct drm_connector *connector,
> +                              u8 *edid, int num_blocks)
> +{
> +       int i;
> +
> +       if (connector->bad_edid_counter++ && !(drm_debug & DRM_UT_KMS))
> +               return;
> +
> +       dev_warn(connector->dev->dev,
> +                "%s: EDID is invalid:\n",
> +                connector->name);
> +       for (i = 0; i < num_blocks; i++) {
> +               u8 *block = edid + i * EDID_LENGTH;
> +               char prefix[20];
> +
> +               if (drm_edid_is_zero(block, EDID_LENGTH))
> +                       sprintf(prefix, "\t[%02x] ZERO ", i);
> +               else if (!drm_edid_block_valid(block, i, false, NULL))
> +                       sprintf(prefix, "\t[%02x] BAD  ", i);
> +               else
> +                       sprintf(prefix, "\t[%02x] GOOD ", i);
> +
> +               print_hex_dump(KERN_WARNING,
> +                              prefix, DUMP_PREFIX_NONE, 16, 1,
> +                              block, EDID_LENGTH, false);
> +       }
> +}
> +
>  /**
>   * drm_do_get_edid - get EDID data using a custom EDID block read function
>   * @connector: connector we're probing
> @@ -1283,7 +1311,6 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>  {
>         int i, j = 0, valid_extensions = 0;
>         u8 *edid, *new;
> -       bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
>
>         if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
>                 return NULL;
> @@ -1292,7 +1319,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>         for (i = 0; i < 4; i++) {
>                 if (get_edid_block(data, edid, 0, EDID_LENGTH))
>                         goto out;
> -               if (drm_edid_block_valid(edid, 0, print_bad_edid,
> +               if (drm_edid_block_valid(edid, 0, false,
>                                          &connector->edid_corrupt))
>                         break;
>                 if (i == 0 && drm_edid_is_zero(edid, EDID_LENGTH)) {
> @@ -1304,54 +1331,60 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>                 goto carp;
>
>         /* if there's no extensions, we're done */
> -       if (edid[0x7e] == 0)
> +       valid_extensions = edid[0x7e];
> +       if (valid_extensions == 0)
>                 return (struct edid *)edid;
>
> -       new = krealloc(edid, (edid[0x7e] + 1) * EDID_LENGTH, GFP_KERNEL);
> +       new = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
>         if (!new)
>                 goto out;
>         edid = new;
>
>         for (j = 1; j <= edid[0x7e]; j++) {
> -               u8 *block = edid + (valid_extensions + 1) * EDID_LENGTH;
> +               u8 *block = edid + j * EDID_LENGTH;
>
>                 for (i = 0; i < 4; i++) {
>                         if (get_edid_block(data, block, j, EDID_LENGTH))
>                                 goto out;
> -                       if (drm_edid_block_valid(block, j,
> -                                                print_bad_edid, NULL)) {
> -                               valid_extensions++;
> +                       if (drm_edid_block_valid(block, j, false, NULL))
>                                 break;
> -                       }
>                 }
>
> -               if (i == 4 && print_bad_edid) {
> -                       dev_warn(connector->dev->dev,
> -                        "%s: Ignoring invalid EDID block %d.\n",
> -                        connector->name, j);
> -
> -                       connector->bad_edid_counter++;
> -               }
> +               if (i == 4)
> +                       valid_extensions--;
>         }
>
>         if (valid_extensions != edid[0x7e]) {
> +               u8 *base;
> +
> +               connector_bad_edid(connector, edid, edid[0x7e] + 1);
> +
>                 edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
>                 edid[0x7e] = valid_extensions;
> -               new = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
> +
> +               new = kmalloc((valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
>                 if (!new)
>                         goto out;
> +
> +               base = new;
> +               for (i = 0; i <= edid[0x7e]; i++) {
> +                       u8 *block = edid + i * EDID_LENGTH;
> +
> +                       if (!drm_edid_block_valid(block, i, false, NULL))
> +                               continue;
> +
> +                       memcpy(base, block, EDID_LENGTH);
> +                       base += EDID_LENGTH;
> +               }
> +
> +               kfree(edid);
>                 edid = new;
>         }
>
>         return (struct edid *)edid;
>
>  carp:
> -       if (print_bad_edid) {
> -               dev_warn(connector->dev->dev, "%s: EDID block %d invalid.\n",
> -                        connector->name, j);
> -       }
> -       connector->bad_edid_counter++;
> -
> +       connector_bad_edid(connector, edid, 1);
>  out:
>         kfree(edid);
>         return NULL;
> --
> 2.10.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx