| Message ID | 44ca22d7-d28e-4757-b915-0e45fe864900@HUB2.rwth-ad.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, 16 Nov 2014, Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote: > 4a638b4e38234233f5c7e6705662fbc0b58d80c2 disabled the checksumming Please include the subject of the referenced commit, e.g. 4a638b4e3823 drm/edid: Allow non-fatal checksum errors in CEA blocks or 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 > for CEA blocks. If only the checksum is wrong, reading twice should > result in identical data, whereas a bad transfer will most likely > corrupt diffent bytes. > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> > --- > drivers/gpu/drm/drm_edid.c | 25 +++++++++++++++++++++---- > 1 file changed, 21 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 505960e..9b6b65e 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -1200,6 +1200,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) > @@ -1234,15 +1235,29 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) > block = new; > > for (j = 1; j <= block[0x7e]; j++) { > + u8 *ext_block = block + (valid_extensions + 1) * EDID_LENGTH; This bit could be a non-functional prep patch. > + u8 csum, last_csum = 0; > for (i = 0; i < 4; i++) { > - if (drm_do_probe_ddc_edid(adapter, > - block + (valid_extensions + 1) * EDID_LENGTH, > - j, EDID_LENGTH)) > + if (drm_do_probe_ddc_edid(adapter, ext_block, j, EDID_LENGTH)) > goto out; > - if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) { Now you skip drm_edid_block_valid for all blocks? > + if ((csum = drm_edid_block_checksum(ext_block)) == 0) { Please don't assign within the if condition. > valid_extensions++; > break; > + } else if ((ext_block[0] == CEA_EXT) && (csum == last_csum)) { Too many braces. Perhaps it would be sufficient to just check checksums instead of comparing the data? *shrug*. > + /* > + * Some switches mangle CEA contents without fixing the checksum. > + * Accept CEA blocks when two reads return identical data. > + */ So you end up here on the 2nd time you get identical checksums, but you don't have the saved_block for the 1st attempt. Therefore you end up saving the 2nd attempt and go for a 3rd attempt to end up here. > + if (!saved_block) > + saved_block = kmalloc(EDID_LENGTH, GFP_KERNEL); > + if (saved_block && !memcmp(ext_block, saved_block, EDID_LENGTH)) { On the 2nd attempt you compare ext_block to whatever kmalloc returned you. > + valid_extensions++; > + break; > + } > + if (saved_block) > + memcpy(saved_block, ext_block, EDID_LENGTH); You should use kmemdup, and rearrange the code to do what it says on the commit message. BR, Jani. > } > + last_csum = csum; > } > > if (i == 4 && print_bad_edid) { > @@ -1263,6 +1278,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) > block = new; > } > > + kfree(saved_block); > return block; > > carp: > @@ -1273,6 +1289,7 @@ carp: > connector->bad_edid_counter++; > > out: > + kfree(saved_block); > kfree(block); > return NULL; > } > -- > 1.8.4.5 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Monday, November 17, 2014 11:08:48 Jani Nikula wrote: > On Sun, 16 Nov 2014, Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote: > > 4a638b4e38234233f5c7e6705662fbc0b58d80c2 disabled the checksumming > > Please include the subject of the referenced commit, e.g. > > 4a638b4e3823 drm/edid: Allow non-fatal checksum errors in CEA blocks > > or > > 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 OK > > > for CEA blocks. If only the checksum is wrong, reading twice should > > result in identical data, whereas a bad transfer will most likely > > corrupt diffent bytes. > > > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> > > --- > > > > drivers/gpu/drm/drm_edid.c | 25 +++++++++++++++++++++---- > > 1 file changed, 21 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > > index 505960e..9b6b65e 100644 > > --- a/drivers/gpu/drm/drm_edid.c > > +++ b/drivers/gpu/drm/drm_edid.c > > @@ -1200,6 +1200,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) > > > > @@ -1234,15 +1235,29 @@ drm_do_get_edid(struct drm_connector *connector, > > struct i2c_adapter *adapter)> > > block = new; > > > > for (j = 1; j <= block[0x7e]; j++) { > > > > + u8 *ext_block = block + (valid_extensions + 1) * EDID_LENGTH; > > This bit could be a non-functional prep patch. OK > > + u8 csum, last_csum = 0; > > > > for (i = 0; i < 4; i++) { > > > > - if (drm_do_probe_ddc_edid(adapter, > > - block + (valid_extensions + 1) * EDID_LENGTH, > > - j, EDID_LENGTH)) > > + if (drm_do_probe_ddc_edid(adapter, ext_block, j, EDID_LENGTH)) > > > > goto out; > > > > - if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, > > j, print_bad_edid)) { > Now you skip drm_edid_block_valid for all blocks? drm_edid_block_valid does a bunch of things, depending on the block number. For blocks after the first block, it is essentially just checksumming the block, but also ignores any errors for CEA blocks, which causes problems later on. Therefor the use of the new checksum helper. > > + if ((csum = drm_edid_block_checksum(ext_block)) == 0) { > > Please don't assign within the if condition. OK > > valid_extensions++; > > break; > > > > + } else if ((ext_block[0] == CEA_EXT) && (csum == last_csum)) { > > Too many braces. > > Perhaps it would be sufficient to just check checksums instead of > comparing the data? *shrug*. As the EDID checksum is just a linear checksum over all bytes there are a lot of error combinations having the same checksum, but different data. I think a byte-for-byte comparision is a must here (although the checksum-only would be an improvement). > > + /* > > + * Some switches mangle CEA contents without fixing the checksum. > > + * Accept CEA blocks when two reads return identical data. > > + */ > > So you end up here on the 2nd time you get identical checksums, but you > don't have the saved_block for the 1st attempt. Therefore you end up > saving the 2nd attempt and go for a 3rd attempt to end up here. Good catch, this is obviously wrong. How about the following: ... } else if (ext_block[0] == CEA_EXT) { 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; } ... This should also address the other remarks. Kind regards, Stefan
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 505960e..9b6b65e 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1200,6 +1200,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) @@ -1234,15 +1235,29 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) block = new; 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, - block + (valid_extensions + 1) * EDID_LENGTH, - j, EDID_LENGTH)) + if (drm_do_probe_ddc_edid(adapter, ext_block, j, EDID_LENGTH)) goto out; - if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) { + if ((csum = drm_edid_block_checksum(ext_block)) == 0) { valid_extensions++; break; + } else if ((ext_block[0] == CEA_EXT) && (csum == last_csum)) { + /* + * Some switches mangle CEA contents without fixing the checksum. + * Accept CEA blocks when two reads return identical data. + */ + if (!saved_block) + saved_block = kmalloc(EDID_LENGTH, GFP_KERNEL); + if (saved_block && !memcmp(ext_block, saved_block, EDID_LENGTH)) { + valid_extensions++; + break; + } + if (saved_block) + memcpy(saved_block, ext_block, EDID_LENGTH); } + last_csum = csum; } if (i == 4 && print_bad_edid) { @@ -1263,6 +1278,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) block = new; } + kfree(saved_block); return block; carp: @@ -1273,6 +1289,7 @@ carp: connector->bad_edid_counter++; out: + kfree(saved_block); kfree(block); return NULL; }
4a638b4e38234233f5c7e6705662fbc0b58d80c2 disabled the checksumming for CEA blocks. If only the checksum is wrong, reading twice should result in identical data, whereas a bad transfer will most likely corrupt diffent bytes. Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> --- drivers/gpu/drm/drm_edid.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-)