[Intel-gfx] [PATCH 11/17] drm/i915: Update the EDID automated compliance test function

[Todd Previte]

On 12/17/14 9:25 AM, Paulo Zanoni wrote:
> 2014-12-10 21:53 GMT-02:00 Todd Previte<tprevite at gmail.com>:
>> Updates the EDID compliance test function to perform the EDID read as
>> required by the tests. This read needs to take place in the kernel for
>> reasons of speed and efficiency. The results of the EDID read are handed
>> off to userspace so that the remainder of the test can be conducted there.
>>
>> V2:
>> - Addressed mailing list feedback
>> - Removed excess debug messages
>> - Removed extraneous comments
>> - Fixed formatting issues (line length > 80)
>> - Updated the debug message in compute_edid_checksum to output hex values
>>    instead of decimal
>>
>> Signed-off-by: Todd Previte<tprevite at gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c | 72 ++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 71 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index b6f5a72..2a13124 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -40,6 +40,13 @@
>>
>>   #define DP_LINK_CHECK_TIMEOUT  (10 * 1000)
>>
>> +/* Compliance test status bits  */
>> +#define  INTEL_DP_EDID_OK              (0<<0)
>> +#define  INTEL_DP_EDID_CORRUPT         (1<<0)
>> +#define  INTEL_DP_RESOLUTION_PREFERRED (1<<2)
>> +#define  INTEL_DP_RESOLUTION_STANDARD  (1<<3)
>> +#define  INTEL_DP_RESOLUTION_FAILSAFE  (1<<4)
>> +
>>   struct dp_link_dpll {
>>          int link_bw;
>>          struct dpll dpll;
>> @@ -3761,9 +3768,72 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
>>          return test_result;
>>   }
>>
>> +static bool intel_dp_compute_edid_checksum(uint8_t *edid_data,
>> +                                          uint8_t *edid_checksum)
>> +{
>> +       uint32_t byte_total = 0;
>> +       uint8_t i = 0;
>> +       bool edid_ok = true;
>> +
>> +       /* Don't include last byte (the checksum) in the computation */
>> +       for (i = 0; i < EDID_LENGTH - 2; i++)
> Shouldn't this be "i < EDID_LENGHT - 1"?
Nope. :) EDID_LENGTH is defined as 128, so 128 would be off the end of 
the array. EDID_LENGTH - 1 would be the checksum itself, which I don't 
want to include when computing the checksum. So the last valid byte is 
actually byte 126, which is EDID_LENGTH - 2.
>> +               byte_total += edid_data[i];
>> +
>> +       *edid_checksum = 256 - (byte_total % 256);
>> +
>> +       if (*edid_checksum != edid_data[EDID_LENGTH - 1]) {
>> +               DRM_DEBUG_KMS("Invalid EDID checksum %02x, should be %02x\n",
>> +                             edid_data[EDID_LENGTH - 40 - 1], *edid_checksum);
>> +               edid_ok = false;
>> +       }
>> +
>> +       return edid_ok;
>> +}
>> +
>>   static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
>>   {
>> -       uint8_t test_result = DP_TEST_NAK;
>> +       struct drm_connector *connector = &intel_dp->attached_connector->base;
>> +       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
>> +       struct edid *edid_read = NULL;
>> +       uint8_t *edid_data = NULL;
>> +       uint8_t test_result = DP_TEST_NAK, checksum = 0;
>> +       uint32_t ret = 0;
>> +
>> +       intel_dp->aux.i2c_nack_count = 0;
>> +       intel_dp->aux.i2c_defer_count = 0;
>> +
>> +       edid_read = drm_get_edid(connector, adapter);
>> +
>> +       if (edid_read == NULL) {
>> +               /* Check for NACKs/DEFERs, use failsafe if detected
>> +                  (DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5) */
>> +               if (intel_dp->aux.i2c_nack_count > 0 ||
>> +                       intel_dp->aux.i2c_defer_count > 0)
>> +                       DRM_DEBUG_KMS("EDID read had %d NACKs, %d DEFERs\n",
>> +                                     intel_dp->aux.i2c_nack_count,
>> +                                     intel_dp->aux.i2c_defer_count);
> Don't we need to use these _count values somehow, instead of just
> printing them in the logs?
>
> Everything else looks fine.
In this instance, no. All that is required is for the source device to 
appropriately respond to receiving an I2C DEFER or NACK from the sink 
device. If those counters are above 0 for purposes of compliance 
testing, that triggers the corrupt/invalid EDID response from the test 
mechanism.

In reality, those counters could be tracked for a variety of reasons; 
one example is that they could used to slow down I2C transactions for 
sink devices with high response latencies, so that the DEFERs or NACKs 
could be reduced or avoided entirely.
>> +               intel_dp->compliance_test_data = INTEL_DP_EDID_CORRUPT |
>> +                                                INTEL_DP_RESOLUTION_FAILSAFE;
>> +       } else {
>> +               edid_data = (uint8_t *) edid_read;
>> +
>> +               if (intel_dp_compute_edid_checksum(edid_data, &checksum)) {
>> +                       ret = drm_dp_dpcd_write(&intel_dp->aux,
>> +                                               DP_TEST_EDID_CHECKSUM,
>> +                                               &edid_read->checksum, 1);
>> +                       test_result = DP_TEST_ACK |
>> +                                     DP_TEST_EDID_CHECKSUM_WRITE;
>> +                       intel_dp->compliance_test_data =
>> +                               INTEL_DP_EDID_OK |
>> +                               INTEL_DP_RESOLUTION_PREFERRED;
>> +               } else {
>> +                       /* Invalid checksum - EDID corruption detection */
>> +                       intel_dp->compliance_test_data =
>> +                               INTEL_DP_EDID_CORRUPT |
>> +                               INTEL_DP_RESOLUTION_FAILSAFE;
>> +               }
>> +       }
>> +
>>          return test_result;
>>   }
>>
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>