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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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