Re: [PATCH v2 05/18] DRM/KMS/EDID: Test EDDC if EDID announces more than one Extension Block (v2)

From: Egbert Eich <eich@suse.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: Egbert Eich <eich@suse.com>,
	tiwai@suse.com, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2 05/18] DRM/KMS/EDID: Test EDDC if EDID announces more than one Extension Block (v2)
Date: Thu, 22 Nov 2012 14:18:11 +0100	[thread overview]
Message-ID: <20654.9747.55686.261628@linux-qknr.site> (raw)
In-Reply-To: ville.syrjala@linux.intel.com wrote on Thursday, 22 November 2012 at 14:29:45 +0200

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

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

Right. 
This flag would only make sense for the base block (to reset 
an EDDC capable monitor but not fail on non-EDDC capable ones). 
In fact I had a patch to add this, but then I realized that none
of the I2C implemenations in the gfx drivers honored this flag.
But then I reread the VESA spec for EDDC, this requires that the
segment address in the monitor has to be reset on a STOP condition.

So if there ever is the case that a monitor has a segment > 0 
selected when drm_read_edid() is called for the first time (because 
of a system crash for instance) the validation will fail and causes 
a 2nd read attempt for which the segment address should be reset by 
the previous transfer.
Regarding crappiness of monitor hardware: catch me at a bar and I
can tell you more stories ... ;p

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

Right. I've already reworked it. Will send out.

Cheers,
	Egbert.

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