Re: [PATCH 07/12] drm/edid: split drm_edid_block_valid() to check and act parts

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Mar 29, 2022 at 09:42:14PM +0300, Jani Nikula wrote:
> Add edid_block_check() that only checks the EDID block validity, without
> any actions. Turns out it's simple and crystal clear.
> 
> Rewrite drm_edid_block_valid() around it, keeping all the functionality
> fairly closely the same, warts and all. Turns out it's incredibly
> complicated for a function you'd expect to be simple, with all the
> fixing and printing and special casing. (Maybe we'll want to simplify it
> in the future.)
> 
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> ---
>  drivers/gpu/drm/drm_edid.c | 150 ++++++++++++++++++++++---------------
>  1 file changed, 88 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 481643751d10..04eb6949c9c8 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1668,10 +1668,55 @@ bool drm_edid_are_equal(const struct edid *edid1, const struct edid *edid2)
>  }
>  EXPORT_SYMBOL(drm_edid_are_equal);
>  
> +enum edid_block_status {
> +	EDID_BLOCK_OK = 0,
> +	EDID_BLOCK_NULL,
> +	EDID_BLOCK_HEADER_CORRUPT,
> +	EDID_BLOCK_HEADER_REPAIR,
> +	EDID_BLOCK_HEADER_FIXED,
> +	EDID_BLOCK_CHECKSUM,
> +	EDID_BLOCK_VERSION,
> +};
> +
> +static enum edid_block_status edid_block_check(const void *_block, bool base)

s/base/is_base_block/ or something?

> +{
> +	const struct edid *block = _block;
> +
> +	if (!block)
> +		return EDID_BLOCK_NULL;
> +
> +	if (base) {
> +		int score = drm_edid_header_is_valid(block);
> +
> +		if (score < clamp(edid_fixup, 6, 8))

That should clamp to 0-8?

Might be nicer to just define a .set() op for the modparam
and check it there, but that's clearly material for a separate patch.

> +			return EDID_BLOCK_HEADER_CORRUPT;
> +
> +		if (score < 8)
> +			return EDID_BLOCK_HEADER_REPAIR;
> +	}
> +
> +	if (edid_block_compute_checksum(block) != edid_block_get_checksum(block))
> +		return EDID_BLOCK_CHECKSUM;
> +
> +	if (base) {
> +		if (block->version != 1)
> +			return EDID_BLOCK_VERSION;
> +	}
> +
> +	return EDID_BLOCK_OK;
> +}
> +
> +static bool edid_block_status_valid(enum edid_block_status status, int tag)
> +{
> +	return status == EDID_BLOCK_OK ||
> +		status == EDID_BLOCK_HEADER_FIXED ||
> +		(status == EDID_BLOCK_CHECKSUM && tag == CEA_EXT);
> +}
> +
>  /**
>   * 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)
> + * @block_num: type of block to validate (0 for base, extension otherwise)
>   * @print_bad_edid: if true, dump bad EDID blocks to the console
>   * @edid_corrupt: if true, the header or checksum is invalid
>   *
> @@ -1680,88 +1725,69 @@ EXPORT_SYMBOL(drm_edid_are_equal);
>   *
>   * 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 *_block, int block_num, bool print_bad_edid,
>  			  bool *edid_corrupt)
>  {
> -	u8 csum;
> -	struct edid *edid = (struct edid *)raw_edid;
> +	struct edid *block = (struct edid *)_block;
> +	enum edid_block_status status;
> +	bool base = block_num == 0;
> +	bool valid;
>  
> -	if (WARN_ON(!raw_edid))
> +	if (WARN_ON(!block))
>  		return false;
>  
> -	if (edid_fixup > 8 || edid_fixup < 0)
> -		edid_fixup = 6;
> -
> -	if (block == 0) {
> -		int score = drm_edid_header_is_valid(raw_edid);
> +	status = edid_block_check(block, base);
> +	if (status == EDID_BLOCK_HEADER_REPAIR) {
> +		DRM_DEBUG("Fixing EDID header, your hardware may be failing\n");
> +		edid_header_fix(block);
>  
> -		if (score == 8) {
> -			if (edid_corrupt)
> -				*edid_corrupt = false;
> -		} else if (score >= edid_fixup) {
> -			/* Displayport Link CTS Core 1.2 rev1.1 test 4.2.2.6
> -			 * The corrupt flag needs to be set here otherwise, the
> -			 * fix-up code here will correct the problem, the
> -			 * checksum is correct and the test fails
> -			 */
> -			if (edid_corrupt)
> -				*edid_corrupt = true;
> -			DRM_DEBUG("Fixing EDID header, your hardware may be failing\n");
> -			edid_header_fix(raw_edid);
> -		} else {
> -			if (edid_corrupt)
> -				*edid_corrupt = true;
> -			goto bad;
> -		}
> +		/* Retry with fixed header, update status if that worked. */
> +		status = edid_block_check(block, base);
> +		if (status == EDID_BLOCK_OK)
> +			status = EDID_BLOCK_HEADER_FIXED;
>  	}
>  
> -	csum = edid_block_compute_checksum(raw_edid);
> -	if (csum != edid_block_get_checksum(raw_edid)) {
> -		if (edid_corrupt)
> +	if (edid_corrupt) {
> +		/*
> +		 * Unknown major version isn't corrupt but we can't use it. Only
> +		 * the base block can reset edid_corrupt to false.
> +		 */
> +		if (base && (status == EDID_BLOCK_OK || status == EDID_BLOCK_VERSION))
> +			*edid_corrupt = false;
> +		else if (status != EDID_BLOCK_OK)
>  			*edid_corrupt = true;
> -
> -		/* allow CEA to slide through, switches mangle this */
> -		if (edid_block_tag(raw_edid) == CEA_EXT) {
> -			DRM_DEBUG("EDID checksum is invalid, remainder is %d\n", csum);
> -			DRM_DEBUG("Assuming a KVM switch modified the CEA block but left the original checksum\n");
> -		} else {
> -			if (print_bad_edid)
> -				DRM_NOTE("EDID checksum is invalid, remainder is %d\n", csum);
> -
> -			goto bad;
> -		}
>  	}
>  
> -	/* per-block-type checks */
> -	switch (edid_block_tag(raw_edid)) {
> -	case 0: /* base */
> -		if (edid->version != 1) {
> -			DRM_NOTE("EDID has major version %d, instead of 1\n", edid->version);
> -			goto bad;
> +	/* Determine whether we can use this block with this status. */
> +	valid = edid_block_status_valid(status, edid_block_tag(block));
> +
> +	/* Some fairly random status printouts. */
> +	if (status == EDID_BLOCK_CHECKSUM) {
> +		if (valid) {
> +			DRM_DEBUG("EDID block checksum is invalid, remainder is %d\n",
> +				  edid_block_compute_checksum(block));
> +			DRM_DEBUG("Assuming a KVM switch modified the block but left the original checksum\n");
> +		} else if (print_bad_edid) {
> +			DRM_NOTE("EDID block checksum is invalid, remainder is %d\n",
> +				 edid_block_compute_checksum(block));
>  		}
> -
> -		if (edid->revision > 4)
> -			DRM_DEBUG("EDID minor > 4, assuming backward compatibility\n");

This debug message seems to disappear. Intentional?

> -		break;
> -
> -	default:
> -		break;
> +	} else if (status == EDID_BLOCK_VERSION) {
> +		DRM_NOTE("EDID has major version %d, instead of 1\n",
> +			 block->version);
>  	}
>  
> -	return true;
> -
> -bad:
> -	if (print_bad_edid) {
> -		if (edid_is_zero(raw_edid, EDID_LENGTH)) {
> +	if (!valid && print_bad_edid) {
> +		if (edid_is_zero(block, EDID_LENGTH)) {
>  			pr_notice("EDID block is all zeroes\n");
>  		} else {
>  			pr_notice("Raw EDID:\n");
>  			print_hex_dump(KERN_NOTICE,
>  				       " \t", DUMP_PREFIX_NONE, 16, 1,
> -				       raw_edid, EDID_LENGTH, false);
> +				       block, EDID_LENGTH, false);
>  		}
>  	}
> -	return false;
> +
> +	return valid;
>  }
>  EXPORT_SYMBOL(drm_edid_block_valid);
>  
> -- 
> 2.30.2

-- 
Ville Syrjälä
Intel



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux