| Message ID | 75b98802-5906-49c2-8c04-9f613d328ec2@HUB1.rwth-ad.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 20 Nov 2014, Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote: > Checksumming was disabled for CEA blocks by > > commit 4a638b4e38234233f5c7e6705662fbc0b58d80c2 > Author: Adam Jackson <ajax@redhat.com> > Date: Tue May 25 16:33:09 2010 -0400 > > drm/edid: Allow non-fatal checksum errors in CEA blocks > > If only the checksum is wrong, reading twice should result in identical > data, whereas a bad transfer will most likely corrupt different bytes. > Comparing checksums is not sufficient, as there is a considerable chance > of two bad transfers having the same checksum. > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> > --- > drivers/gpu/drm/drm_edid.c | 22 +++++++++++++++++++++- > 1 file changed, 21 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 3a10f3f..55963d5 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -1203,6 +1203,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) > { > int i, j = 0, valid_extensions = 0; > u8 *block, *new; > + u8 *saved_block = NULL; > bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS); > > if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL) > @@ -1233,12 +1234,27 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) > > for (j = 1; j <= block[0x7e]; j++) { > u8 *ext_block = block + (valid_extensions + 1) * EDID_LENGTH; > + u8 csum, last_csum = 0; > for (i = 0; i < 4; i++) { > if (drm_do_probe_ddc_edid(adapter, ext_block, j, EDID_LENGTH)) > goto out; > - if (drm_edid_block_valid(ext_block, j, print_bad_edid)) { > + csum = drm_edid_block_csum(ext_block); > + if (!csum) { > valid_extensions++; > break; > + } else if (ext_block[0] == CEA_EXT) { > + /* > + * Some switches mangle CEA contents without fixing the checksum. > + * Accept CEA blocks when two reads return identical data. > + */ > + if (saved_block && csum == last_csum && > + !memcmp(ext_block, saved_block, EDID_LENGTH)) { > + valid_extensions++; > + break; > + } > + kfree(saved_block); > + saved_block = kmemdup(ext_block, EDID_LENGTH, GFP_KERNEL); > + last_csum = csum; I still feel like this should be embedded in drm_edid_block_valid somehow. Who's going to print the bad edid now? Is it simply no longer printed in this loop? I'll defer to others; any better suggestions? Jani. > } > } > > @@ -1249,6 +1265,9 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) > > connector->bad_edid_counter++; > } > + > + kfree(saved_block); > + saved_block = NULL; > } > > if (valid_extensions != block[0x7e]) { > @@ -1270,6 +1289,7 @@ carp: > connector->bad_edid_counter++; > > out: > + kfree(saved_block); > kfree(block); > return NULL; > } > -- > 2.1.2 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Friday 21 November 2014 15:39:40 you wrote: > I still feel like this should be embedded in drm_edid_block_valid > somehow. Who's going to print the bad edid now? Is it simply no longer > printed in this loop? > > I'll defer to others; any better suggestions? > > Jani. > This can not be embedded in drm_edid_block_valid, as the function is completely "passive", but we must be able to trigger a retransmission of the bad block. Regarding printing, this should be added. Thinking about it, maybe factor out the printing routine, and add the zero block handling to it? Kind regards, Stefan
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 3a10f3f..55963d5 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1203,6 +1203,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) { int i, j = 0, valid_extensions = 0; u8 *block, *new; + u8 *saved_block = NULL; bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS); if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL) @@ -1233,12 +1234,27 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) for (j = 1; j <= block[0x7e]; j++) { u8 *ext_block = block + (valid_extensions + 1) * EDID_LENGTH; + u8 csum, last_csum = 0; for (i = 0; i < 4; i++) { if (drm_do_probe_ddc_edid(adapter, ext_block, j, EDID_LENGTH)) goto out; - if (drm_edid_block_valid(ext_block, j, print_bad_edid)) { + csum = drm_edid_block_csum(ext_block); + if (!csum) { valid_extensions++; break; + } else if (ext_block[0] == CEA_EXT) { + /* + * Some switches mangle CEA contents without fixing the checksum. + * Accept CEA blocks when two reads return identical data. + */ + if (saved_block && csum == last_csum && + !memcmp(ext_block, saved_block, EDID_LENGTH)) { + valid_extensions++; + break; + } + kfree(saved_block); + saved_block = kmemdup(ext_block, EDID_LENGTH, GFP_KERNEL); + last_csum = csum; } } @@ -1249,6 +1265,9 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) connector->bad_edid_counter++; } + + kfree(saved_block); + saved_block = NULL; } if (valid_extensions != block[0x7e]) { @@ -1270,6 +1289,7 @@ carp: connector->bad_edid_counter++; out: + kfree(saved_block); kfree(block); return NULL; }
Checksumming was disabled for CEA blocks by commit 4a638b4e38234233f5c7e6705662fbc0b58d80c2 Author: Adam Jackson <ajax@redhat.com> Date: Tue May 25 16:33:09 2010 -0400 drm/edid: Allow non-fatal checksum errors in CEA blocks If only the checksum is wrong, reading twice should result in identical data, whereas a bad transfer will most likely corrupt different bytes. Comparing checksums is not sufficient, as there is a considerable chance of two bad transfers having the same checksum. Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> --- drivers/gpu/drm/drm_edid.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)