Re: [PATCH 06/13] drm: Add supporting structure for Displayport Link CTS test 4.2.2.6

[Todd Previte <tprevite@gmail.com>]

On 4/13/15 3:18 PM, Paulo Zanoni wrote:
> 2015-04-13 11:53 GMT-03:00 Todd Previte <tprevite@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
>>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> Cc: dri-devel@lists.freedesktop.org
>> ---
>>   drivers/gpu/drm/drm_edid.c        | 31 ++++++++++++++++++++++++++-----
>>   drivers/gpu/drm/drm_edid_load.c   |  7 +++++--
>>   drivers/gpu/drm/i2c/tda998x_drv.c |  4 ++--
>>   drivers/gpu/drm/i915/intel_dp.c   |  2 +-
>>   include/drm/drm_crtc.h            |  8 +++++++-
>>   5 files changed, 41 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 53bc7a6..12e5be7 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -1005,7 +1005,6 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
>>          for (i = 0; i < sizeof(edid_header); i++)
>>                  if (raw_edid[i] == edid_header[i])
>>                          score++;
>> -
>>          return score;
>>   }
>>   EXPORT_SYMBOL(drm_edid_header_is_valid);
> Bad chunk...
Fixed
>
>> @@ -1047,7 +1046,8 @@ static bool drm_edid_is_zero(const u8 *in_edid, int length)
>>    *
>>    * 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)
> Need to add the new parameter description to the documentation above.
Done.
>
>>   {
>>          u8 csum;
>>          struct edid *edid = (struct edid *)raw_edid;
>> @@ -1062,9 +1062,27 @@ 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 then correct
>> +                        * and the test fails
>> +                        */
>> +                       csum = drm_edid_block_checksum(raw_edid);
>> +                       if (csum) {
>> +                               DRM_DEBUG_DRIVER("Invalid EDID header, score = %d\n", score);
>> +                               DRM_DEBUG_DRIVER("Invalid EDID checksum %d\n", csum);
> No one on this file uses DRM_DEBUG_DRIVER (you use 2 calls here and one below).
>
> Also, during "normal operation" we try to calculate the checksum based
> on the fixed EDID header, so if we also print these messages here
> we're always going to have a message complaining about invalid
> checksum: either this one or the other that's already there. My
> bikeshed would be to just remove the messages you added here and below
> to not confuse users. Let's assume that the bad header is due to some
> communication/corruption error, and the HW manufacturers did not
> program an EDID with a bad header and a correct checksum based on bad
> header :)
Works for me. Messages have been removed.
>
>> +                               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");
>> +                               *header_corrupt = 1;
>> +                       }
> We don't ever set header_corrupt back to false, so if we ever get a
> corrupt header once, the header_corrupt flag will stay there even if
> it gets fixed somehow (such as a DP compliance tester starting to
> submit the correct header).
This is fixed as well. The flag now is reset to 0 at the end of the test 
handler.
>
>>                          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,9 @@ 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,
>> +                                                &connector->edid_header_corrupt)) {
> You can use NULL here since we're not handling block 0 anymore.
Fixed.
>
>>                                  valid_extensions++;
>>                                  break;
>>                          }
>> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
>> index 732cb6f..1505494 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,
>> +                                        &connector->edid_header_corrupt))
> Same here: not iterating over block 0.
Fixed
> Btw, why can't we just use NULL on edid_load?
I think it's at too high level again and the fix up code will get in the 
way.
>
>>                          valid_extensions++;
>>          }
>>
>> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
>> index a9041d1..9c3d6b3 100644
>> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
>> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
>> @@ -1106,7 +1106,7 @@ static uint8_t *do_get_edid(struct tda998x_priv *priv)
>>          if (read_edid_block(priv, block, 0))
>>                  goto fail;
>>
>> -       if (!drm_edid_block_valid(block, 0, print_bad_edid))
>> +       if (!drm_edid_block_valid(block, 0, print_bad_edid, NULL))
>>                  goto fail;
>>
>>          /* if there's no extensions, we're done */
>> @@ -1123,7 +1123,7 @@ static uint8_t *do_get_edid(struct tda998x_priv *priv)
>>                  if (read_edid_block(priv, ext_block, j))
>>                          goto fail;
>>
>> -               if (!drm_edid_block_valid(ext_block, j, print_bad_edid))
>> +               if (!drm_edid_block_valid(ext_block, j, print_bad_edid, NULL))
>>                          goto fail;
>>
>>                  valid_extensions++;
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index e1d6e79..77b6b15 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3922,7 +3922,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>           * 4.2.2.1 - EDID read required for all HPD events
>>            */
>>           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");
>>           }
>>
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index 0261417..e31a4b3 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;
>> @@ -1436,7 +1441,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
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx