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

[Ville Syrjälä]

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