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

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 31 Mar 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 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@linux.intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  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?

Okay.

>
>> +{
>> +	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?

Indeed, thanks!

> 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.

Yes.

BR,
Jani.

>
>> +			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

-- 
Jani Nikula, Intel Open Source Graphics Center