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

[Egbert Eich]

Ville Syrjälä writes:
 > > 
 > > 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.


 > > +
 > > +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();

Right. 
It's stupid anyway. See below.

 > 
 > >  }
 > >  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'

Oops, right. Leftover from old code.

 > change seems superfluous since you're not using the more detailed return
 > value from drm_edid_block_check_error().

This should probably be changed. 
For the drm_edid_block_valid() I'm already using the previous error state
as argument - but I don't tell the result of this read. 
Doesn't make much sense :(

Something similar should be done for drm_edid_is_valid() - even if the 
driver doesn't bother (for instance because this function is only called
once when the device structures are initialized).

The current code ignores the error state for extension blocks i guess it
should not if we want to avoid having repreated logging of errors in the
extension blocks.
What should be done is:
        unsigned err = drm_edid_block_check_error();
	result |= err;
	if (!err) {
	   valid_extensions++;
 	   ...
        }

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

Exactly. Please see above.

Thanks for looking at the patches!

Cheers,
	Egbert.