2015-04-22 17:20 GMT-03:00 Alex Deucher <alexdeucher at gmail.com>: > On Tue, Apr 21, 2015 at 2:09 PM, Todd Previte <tprevite at gmail.com> wrote: >> Displayport compliance test 4.2.2.6 requires that a source device be capable >> of >> detecting a corrupt EDID. The test specification states that the sink device >> sets up the EDID with an invalid checksum. To do this, the sink sets up an >> invalid EDID header, expecting the source device to generate the checksum and >> compare it to the value stored in the last byte of the block data. >> >> Unfortunately, the DRM EDID reading and parsing functions are actually too >> good >> in this case; the header is fixed before the checksum is computed and thus >> the >> test never sees the invalid checksum. This results in a failure to pass the >> compliance test. >> >> To correct this issue, when the EDID code detects that the header is invalid, >> a flag is set to indicate that the EDID is corrupted. In this case, it sets >> edid_corrupt flag and continues with its fix-up code. This flag is also set >> in >> the case of a more seriously damaged header (fixup score less than the >> threshold). For consistency, the edid_corrupt flag is also set when the >> checksum is invalid as well. >> >> V2: >> - Removed the static bool global >> - Added a bool to the drm_connector struct to reaplce the static one for >> holding the status of raw edid header corruption detection >> - Modified the function signature of the is_valid function to take an >> additional parameter to store the corruption detected value >> - Fixed the other callers of the above is_valid function >> V3: >> - Updated the commit message to be more clear about what and why this >> patch does what it does. >> - Added comment in code to clarify the operations there >> - Removed compliance variable and check_link_status update; those >> have been moved to a later patch >> - Removed variable assignment from the bottom of the test handler >> V4: >> - Removed i915 tag from subject line as the patch is not i915-specific >> V5: >> - Moved code causing a compilation error to this patch where the variable >> is actually declared >> - Maintained blank lines / spacing so as to not contaminate the patch >> V6: >> - Removed extra debug messages >> - Added documentation to for the added parameter on drm_edid_block_valid >> - Fixed more whitespace issues in check_link_status >> - Added a clear of the header_corrupt flag to the end of the test handler >> in intel_dp.c >> - Changed the usage of the new function prototype in several places to use >> NULL where it is not needed by compliance testing >> V7: >> - Updated to account for long_pulse flag propagation >> V8: >> - Removed clearing of header_corrupt flag from the test handler in intel_dp.c >> - Added clearing of header_corrupt flag in the drm_edid_block_valid function >> V9: >> - Renamed header_corrupt flag to edid_corrupt to more accurately reflect its >> value and purpose >> - Updated commit message >> V10: >> - Updated for versioning and patch swizzle >> - Revised the title to more accurately reflect the nature and contents of >> the patch >> - Fixed formatting/whitespace problems >> - Added set flag when computed checksum is invalid >> >> Signed-off-by: Todd Previte <tprevite at gmail.com> >> Cc: dri-devel at lists.freedesktop.org > > Seems reasonable. > > Reviewed-by: Alex Deucher <alexander.deucher at amd.com> Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com> (since I made comments on the previous versions, this is my documentation that the patch looks fine to me now)
> >> --- >> drivers/gpu/drm/drm_edid.c | 32 ++++++++++++++++++++++++++------ >> drivers/gpu/drm/drm_edid_load.c | 7 +++++-- >> include/drm/drm_crtc.h | 8 +++++++- >> 3 files changed, 38 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c >> index 53bc7a6..be6713c 100644 >> --- a/drivers/gpu/drm/drm_edid.c >> +++ b/drivers/gpu/drm/drm_edid.c >> @@ -1041,13 +1041,15 @@ static bool drm_edid_is_zero(const u8 *in_edid, int >> length) >> * @raw_edid: pointer to raw EDID block >> * @block: type of block to validate (0 for base, extension otherwise) >> * @print_bad_edid: if true, dump bad EDID blocks to the console >> + * @edid_corrupt: if true, the header or checksum is invalid >> * >> * Validate a base or extension EDID block and optionally dump bad blocks to >> * the console. >> * >> * Return: True if the block is valid, false otherwise. >> */ >> -bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) >> +bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid, >> + bool *edid_corrupt) >> { >> u8 csum; >> struct edid *edid = (struct edid *)raw_edid; >> @@ -1060,11 +1062,22 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, >> bool print_bad_edid) >> >> if (block == 0) { >> int score = drm_edid_header_is_valid(raw_edid); >> - if (score == 8) ; >> - else if (score >= edid_fixup) { >> + if (score == 8) { >> + if (edid_corrupt) >> + *edid_corrupt = 0; >> + } else if (score >= edid_fixup) { >> + /* Displayport Link CTS Core 1.2 rev1.1 test 4.2.2.6 >> + * The corrupt flag needs to be set here otherwise, >> the >> + * fix-up code here will correct the problem, the >> + * checksum is correct and the test fails >> + */ >> + if (edid_corrupt) >> + *edid_corrupt = 1; >> DRM_DEBUG("Fixing EDID header, your hardware may be >> failing\n"); >> memcpy(raw_edid, edid_header, sizeof(edid_header)); >> } else { >> + if (edid_corrupt) >> + *edid_corrupt = 1; >> goto bad; >> } >> } >> @@ -1075,6 +1088,9 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, >> bool print_bad_edid) >> DRM_ERROR("EDID checksum is invalid, remainder is >> %d\n", csum); >> } >> >> + if (edid_corrupt) >> + *edid_corrupt = 1; >> + >> /* allow CEA to slide through, switches mangle this */ >> if (raw_edid[0] != 0x02) >> goto bad; >> @@ -1129,7 +1145,7 @@ bool drm_edid_is_valid(struct edid *edid) >> return false; >> >> for (i = 0; i <= edid->extensions; i++) >> - if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true)) >> + if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true, >> NULL)) >> return false; >> >> return true; >> @@ -1232,7 +1248,8 @@ struct edid *drm_do_get_edid(struct drm_connector >> *connector, >> for (i = 0; i < 4; i++) { >> if (get_edid_block(data, block, 0, EDID_LENGTH)) >> goto out; >> - if (drm_edid_block_valid(block, 0, print_bad_edid)) >> + if (drm_edid_block_valid(block, 0, print_bad_edid, >> + &connector->edid_corrupt)) >> break; >> if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) { >> connector->null_edid_counter++; >> @@ -1257,7 +1274,10 @@ struct edid *drm_do_get_edid(struct drm_connector >> *connector, >> block + (valid_extensions + 1) * >> EDID_LENGTH, >> j, EDID_LENGTH)) >> goto out; >> - if (drm_edid_block_valid(block + (valid_extensions + >> 1) * EDID_LENGTH, j, print_bad_edid)) { >> + if (drm_edid_block_valid(block + (valid_extensions + >> 1) >> + * EDID_LENGTH, j, >> + print_bad_edid, >> + NULL)) { >> valid_extensions++; >> break; >> } >> diff --git a/drivers/gpu/drm/drm_edid_load.c >> b/drivers/gpu/drm/drm_edid_load.c >> index 4c0aa97..c5605fe 100644 >> --- a/drivers/gpu/drm/drm_edid_load.c >> +++ b/drivers/gpu/drm/drm_edid_load.c >> @@ -216,7 +216,8 @@ static void *edid_load(struct drm_connector *connector, >> const char *name, >> goto out; >> } >> >> - if (!drm_edid_block_valid(edid, 0, print_bad_edid)) { >> + if (!drm_edid_block_valid(edid, 0, print_bad_edid, >> + &connector->edid_corrupt)) { >> connector->bad_edid_counter++; >> DRM_ERROR("Base block of EDID firmware \"%s\" is invalid ", >> name); >> @@ -229,7 +230,9 @@ static void *edid_load(struct drm_connector *connector, >> const char *name, >> if (i != valid_extensions + 1) >> memcpy(edid + (valid_extensions + 1) * EDID_LENGTH, >> edid + i * EDID_LENGTH, EDID_LENGTH); >> - if (drm_edid_block_valid(edid + i * EDID_LENGTH, i, >> print_bad_edid)) >> + if (drm_edid_block_valid(edid + i * EDID_LENGTH, i, >> + print_bad_edid, >> + NULL)) >> valid_extensions++; >> } >> >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h >> index d4e4b82..8bc2724 100644 >> --- a/include/drm/drm_crtc.h >> +++ b/include/drm/drm_crtc.h >> @@ -719,6 +719,11 @@ struct drm_connector { >> int null_edid_counter; /* needed to workaround some HW bugs where we >> get all 0s */ >> unsigned bad_edid_counter; >> >> + /* Flag for raw EDID header corruption - used in Displayport >> + * compliance testing - * Displayport Link CTS Core 1.2 rev1.1 >> 4.2.2.6 >> + */ >> + bool edid_corrupt; >> + >> struct dentry *debugfs_entry; >> >> struct drm_connector_state *state; >> @@ -1443,7 +1448,8 @@ extern void drm_set_preferred_mode(struct >> drm_connector *connector, >> int hpref, int vpref); >> >> extern int drm_edid_header_is_valid(const u8 *raw_edid); >> -extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool >> print_bad_edid); >> +extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool >> print_bad_edid, >> + bool *edid_corrupt); >> extern bool drm_edid_is_valid(struct edid *edid); >> >> extern struct drm_tile_group *drm_mode_create_tile_group(struct drm_device >> *dev, >> -- >> 1.9.1 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel at lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel > _______________________________________________ > dri-devel mailing list > dri-devel at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Paulo Zanoni