Re: [PATCH 01/10] drm/i915: Add automated testing support for Displayport compliance testing

[Todd Previte <tprevite@gmail.com>]

Version 2 in process now. Responses in line below.


On 10/20/2014 10:48 AM, Paulo Zanoni wrote:
> Hi
>
> Since I already commented about the coding style on the previous
> reviews, I'll ignore that aspect for the comments below.
I think all the style issues have been resolved. Checkpatch.pl has 0 
errors, 0 warnings, so that should be good to go. Excess comments have 
also been removed. If I missed anything though, please let me know.

>
> 2014-10-09 12:38 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>> Add the skeleton framework for supporting automation for Displayport compliance
>> testing. This patch adds the necessary framework for the source device to appropriately
>> respond to test automation requests from a sink device.
>>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c | 82 ++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 80 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 64c8e04..f7d4119 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3959,11 +3959,89 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>>          return true;
>>   }
>>
>> +/* Displayport compliance testing - Link training */
>> +static uint8_t
>> +intel_dp_autotest_link_training(struct intel_dp *intel_dp)
>> +{
>> +       uint8_t test_result = DP_TEST_NAK;
>> +       return test_result;
>> +}
>> +
>> +/* Displayport compliance testing - Video pattern testing */
>> +static uint8_t
>> +intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
>> +{
>> +       uint8_t test_result = DP_TEST_NAK;
>> +       return test_result;
>> +}
>> +
>> +/* Displayport compliance testing - EDID operations */
>> +static uint8_t
>> +intel_dp_autotest_edid(struct intel_dp *intel_dp)
>> +{
>> +       uint8_t test_result = DP_TEST_NAK;
>> +       return test_result;
>> +}
>> +
>> +/* Displayport compliance testing - PHY pattern testing */
>> +static uint8_t
>> +intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp)
>> +{
>> +       uint8_t test_result = DP_TEST_NAK;
>> +       return test_result;
>> +}
> I guess a lot of people would have just made the code return NAK
> without even defining/calling these functions above.

It came down to two ways of implementing this initial patch. I could 
have NAK'd in the handler and omitted these functions until they were 
fully implemented. Or I could do what I did here, which was to put the 
skeleton in place and add the flesh to bones when ready. To me, this 
seems to make more sense to me because it gets the structure in place 
and builds on it in subsequent patches.

>
>> +
>>   static void
>>   intel_dp_handle_test_request(struct intel_dp *intel_dp)
>>   {
>> -       /* NAK by default */
>> -       drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_RESPONSE, DP_TEST_NAK);
>> +       uint8_t response = DP_TEST_NAK;
>> +       uint8_t rxdata = 0;
>> +       int status = 0;
>> +
>> +       DRM_DEBUG_KMS("Received automated test request\n");
>> +       status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_REQUEST, &rxdata, 1);
>> +
>> +       /* ACK/NAK response based on test function response
>> +          Unimplemented/unsupported tests will NAK by default */
>> +       switch (rxdata) {
> You're reading rxdata without checking for "status" first.
Good catch. Fixed. The status check at the end should have been up at 
the top as well.
>
>
>> +       case DP_TEST_LINK_TRAINING:
>> +               DRM_DEBUG_KMS("Executing LINK_TRAINING request\n");
> As I said on the previous review of the same patch: we're lying here.
> We won't execute anything yet.
I reread the previous comments and after reviewing the code, I can see 
how this could be misleading or confusing.
The message has been changed to "<test name> test requested".


>> +               intel_dp->compliance_test_data = DP_TEST_LINK_TRAINING;
>> +               response = intel_dp_autotest_link_training(intel_dp);
>> +               break;
>> +       case DP_TEST_LINK_VIDEO_PATTERN:
>> +               DRM_DEBUG_KMS("Executing TEST_PATTERN request\n");
>> +               intel_dp->compliance_test_data = DP_TEST_LINK_VIDEO_PATTERN;
>> +               response = intel_dp_autotest_video_pattern(intel_dp);
>> +               break;
>> +       case DP_TEST_LINK_EDID_READ:
>> +               DRM_DEBUG_KMS("Executing EDID request\n");
>> +               intel_dp->compliance_test_data = DP_TEST_LINK_EDID_READ;
>> +               response = intel_dp_autotest_edid(intel_dp);
>> +               break;
>> +       case DP_TEST_LINK_PHY_TEST_PATTERN:
>> +               DRM_DEBUG_KMS("Executing PHY_PATTERN request\n");
>> +               intel_dp->compliance_test_data = DP_TEST_LINK_PHY_TEST_PATTERN;
>> +               response = intel_dp_autotest_phy_pattern(intel_dp);
>> +               break;
>> +               /* FAUX is optional in DP 1.2*/
>> +       case DP_TEST_LINK_FAUX_PATTERN:
>> +               DRM_DEBUG_KMS("FAUX_PATTERN testing not supported\n");
>> +               break;
>> +       /* Unsupported test case or something went wrong */
> Is there a way to differentiate the "unsupported" and the "went wrong"
> cases on dmesg?
I gave this one some thought and after looking at the spec and the code, 
the default case is there to catch any invalid test request. So I 
changed the debug message to reflect that - it now states that it has 
encountered and invalid test request and properly NAKs it.
>
>
>> +       default:
>> +               DRM_DEBUG_KMS("Unhandled test request\n");
>> +               break;
>> +       }
>> +       if (status != 0) {
>> +               response = DP_TEST_NAK;
>> +               DRM_DEBUG_KMS("Error %d processing test request\n", status);
>> +       }
>> +       status = drm_dp_dpcd_write(&intel_dp->aux,
>> +                                  DP_TEST_RESPONSE,
>> +                                  &response, 1);
>> +       intel_dp->compliance_testing_active = 0;
>> +
>>   }
> And the most important thing: the patch doesn't compile. Please make
> sure each patch in the series compiles and works. We do tons of git
> bisections on our tree, that's really important.
Yeah that was an artifact of multiple rebases and moving code around. 
I'll scrub the whole series again when I post version 2.

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