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

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

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