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

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 3161 bytes --]

On Thu, Nov 22, 2012 at 07:28:44PM +0100, Egbert Eich wrote:
> 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.

I'm not sure. The current code dump all failed extension block, doesn't
it? Maybe we actually do want that to happen, at least w/ debugs
enabled.

Then there are various retry loops in the code, which may or may not be
necessary. I have a feeling some of them may have been attempts at
papering over a bug that I fixed [1] in the i2c bitbanging code. But if
they are necessary, I'm not sure we really appreciate repeated dumps of
the same block.

[1] https://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=8ee161ce5e0cfc689eb677f227a6248191165fac

-- 
Ville Syrjälä
Intel OTC

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel