Re: [PATCH 2/5] drm: Add edid_corrupt flag for Displayport Link CTS 4.2.2.6

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



2015-04-22 17:20 GMT-03:00 Alex Deucher <alexdeucher@xxxxxxxxx>:
> On Tue, Apr 21, 2015 at 2:09 PM, Todd Previte <tprevite@xxxxxxxxx> 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@xxxxxxxxx>
>> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
>
> Seems reasonable.
>
> Reviewed-by: Alex Deucher <alexander.deucher@xxxxxxx>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
(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@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Paulo Zanoni
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux