On Thu, Nov 22, 2012 at 01:07:28PM +0100, Egbert Eich wrote:
> Ville Syrj?l? writes:
> > On Thu, Nov 22, 2012 at 05:22:55AM -0500, Egbert Eich wrote:
> > > There are displays which announce EDID extension blocks in the
> > > Extension Flag of the EDID base block although they are not EDDC
> > > capable (ie. take a segment address at I2C slave address 0x30).
> > > We test this by looking for an EDID header which is only possible
> > > in the base block.
> > > If the segment address is not taken into account, this block will
> > > be identical to the base block in which case we stop reading further
> > > EEDID blocks, correct the extension flag and just return the base
> > > block.
> > >
> > > v2: Split up EDID fixup code into separate commit.
> > >
> > > Signed-off-by: Egbert Eich <eich at suse.com>
> > > ---
> > > drivers/gpu/drm/drm_edid.c | 13 +++++++++++++
> > > 1 files changed, 13 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > index a952cfe..5a0e331 100644
> > > --- a/drivers/gpu/drm/drm_edid.c
> > > +++ b/drivers/gpu/drm/drm_edid.c
> > > @@ -364,6 +364,19 @@ drm_do_get_edid(struct drm_connector *connector,
> struct i2c_adapter *adapter)
> > > }
> > > if (drm_edid_block_valid(block +
> (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) {
> > > valid_extensions++;
> > > + /* Test if base block announced
> extension blocks although
> > > + * display is not EDDC capable.
> > > + */
> > > + if (j == 2) {
> > > + int k;
> > > + for (k = 0; k <
> sizeof(edid_header); k++)
> > > + if (block[(EDID_LENGTH
> * 2) + k] != edid_header[k])
> > > + break;
> > > + if (k == sizeof(edid_header)) {
> > > + valid_extensions = 0;
> > > + goto
> done_fix_extension_count;
> > > + }
> >
> > memcmp()? Also couldn't we just memcmp() the whole block against the base
> > block, instead of just the header part?
>
> I don't see an advantage of comparing the entire block with the base block:
> the signature should already be unique. However I don't insist ;)
Me neither. I just figured it might reduce the chance of false
positives. But if you say that can't happen, I'll take your word
for it.
> Regarding memcmp() you are definitely right, I will change the code.
>
> >
> > Also the comment is somehow misleading. It talks about the base block
> > even though we're looking at the extension block.
>
>
> Reason for this patch:
> I had a bug report for a monitor announcing extension blocks in the extension
> block flag of the base block (over 200!) although it wasn't EDDC capable.
> For some reason it got past the ACK check when the segment number was written
> to address 0x30 and happily transferred the base block for any odd numbered
> block and some garbage for even ones.
That's nasty. When I saw your patch I was immediately thinking that this
is caused by the IGNORE_NAK flag, but then I read the current code, and
realized that we're not using that flag. It was used in the original
EDDC patch, and I was worried that this kind of bad behaviour would be
possible if use the flag. But it seems I underestimated how crappy the
monitor hardwar can be.
> The only reliable way we found to catch this condition early was to check if
> block 2 had the header of a base block which will happen when the display
> cannot deal with the segment number.
>
> This was what I tried to summarize in very few words - maybe i should reword
> it a bit.
Right. The idea seems reasonable, I just found the comment somehow a
bit confusing when I was reading the code following it.
So maybe something like 'Test if base block ..., by checking whether the
extension block is a duplicate the base block.' Although the use of
memcmp() will already make things much clearer.
--
Ville Syrj??l??
Intel OTC
