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