On Thu, Nov 22, 2012 at 07:28:44PM +0100, Egbert Eich wrote:
> Ville Syrj� 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
