On 4/15/2015 1:25 PM, Paulo Zanoni wrote:
> 2015-04-15 14:15 GMT-03:00 Todd Previte <tprevite at gmail.com>:
>> 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
>> code never sees the invalid checksum. This results in a failure to pass the
>> compliance test.
>>
>> To correct this issue, a checksum is generated when the EDID header is
>> detected
>> as corrupted. If the checksum is invalid, it sets the header_corrupt flag and
>> logs the errors. In the case of a more seriously damaged header (fixup score
>> less than the threshold) the code does not generate the checksum but does set
>> the header_corrupt flag.
>>
>> 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
>>
>> Signed-off-by: Todd Previte <tprevite at gmail.com>
>> Cc: dri-devel at lists.freedesktop.org
>> ---
>> drivers/gpu/drm/drm_edid.c | 30 ++++++++++++++++++++++++++----
>> drivers/gpu/drm/drm_edid_load.c | 7 +++++--
>> drivers/gpu/drm/i915/intel_dp.c | 6 +++++-
>> include/drm/drm_crtc.h | 8 +++++++-
>> 4 files changed, 43 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 53bc7a6..1ed18f5 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
>> + * @header_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 *header_corrupt)
>> {
>> u8 csum;
>> struct edid *edid = (struct edid *)raw_edid;
>> @@ -1062,9 +1064,25 @@ bool drm_edid_block_valid(u8 *raw_edid, int block,
>> bool print_bad_edid)
>> int score = drm_edid_header_is_valid(raw_edid);
>> if (score == 8) ;
>> else if (score >= edid_fixup) {
>> + /* Displayport Link CTS Core 1.2 rev1.1 test 4.2.2.6
>> + * In order to properly generate the invalid checksum
>> + * required for this test, it must be generated using
>> + * the raw EDID data. Otherwise, the fix-up code here
>> + * will correct the problem, the checksum is correct
>> + * and the test fails
>> + */
>> + csum = drm_edid_block_checksum(raw_edid);
>> + if (csum) {
>> + if (header_corrupt)
>> + *header_corrupt = 1;
>> + }
>> DRM_DEBUG("Fixing EDID header, your hardware may be
>> failing\n");
>> memcpy(raw_edid, edid_header, sizeof(edid_header));
>> } else {
>> + if (header_corrupt) {
>> + DRM_DEBUG_DRIVER("Invalid EDID header\n");
> Still using DRM_DEBUG_DRIVER. See include/drm/drmP.h for a description
> of each macro. BTW, since we're just going to dump the whole EDID
> anyway, the message is also not very informative.
Fair enough. Removed for the next patch.
>
>> + *header_corrupt = 1;
>> + }
>> goto bad;
>> }
>> }
>> @@ -1129,7 +1147,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 +1250,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_header_corrupt))
>> break;
>> if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) {
>> connector->null_edid_counter++;
>> @@ -1257,7 +1276,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..48b48e8 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_header_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/drivers/gpu/drm/i915/intel_dp.c
>> b/drivers/gpu/drm/i915/intel_dp.c
>> index b120d59..ee41e10 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4005,6 +4005,7 @@ static uint8_t intel_dp_autotest_phy_pattern(struct
>> intel_dp *intel_dp)
>>
>> static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>> {
>> + struct drm_connector *connector =
>> &intel_dp->attached_connector->base;
>> uint8_t response = DP_TEST_NAK;
>> uint8_t rxdata = 0;
>> int status = 0;
>> @@ -4051,6 +4052,9 @@ update_status:
>> &response, 1);
>> if (status <= 0)
>> DRM_DEBUG_KMS("Could not write test response to sink\n");
>> +
>> + /* Clear EDID corruption flag now */
>> + connector->edid_header_corrupt = 0;
> I don't think it makes sense to clear this inside i915.ko. We're
> adding the feature to drm.ko, so It makes sense to clear it there.
Good point, that makes a lot of sense. Clearing the flag in the DRM
module is the best place for it. Thanks Paulo.
> Isn't it possible to just clear it inside drm_edid_block_valid()?
> Something like:
>
> if (score == 8) {
> if (header_corrupt)
> *header_corrupt = 0;
> } else if (score >= edid_fixup) {
> ... etc ...
> }
> }
That seems the most logical place for it. Doesn't seem to cause any
issues with the tests so this works for me. Updated patch will be out
shortly.
> If not, why?
As I said above, the only issue would be if it gets cleared before the
test handlers see it, since they're the ones that care about it. But it
doesn't so it's a non-issue. :)
>
>> }
>>
>> static int
>> @@ -4136,7 +4140,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp,
>> bool long_hpd)
>> */
>> if (long_hpd) {
>> edid_read = drm_get_edid(connector, adapter);
>> - if (!edid_read) {
>> + if (!edid_read || connector->edid_header_corrupt == 1) {
>> DRM_DEBUG_DRIVER("Invalid EDID detected\n");
>> } else {
>> kfree(edid_read);
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index d4e4b82..f7a4a92 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_header_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 *header_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
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
