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

[Todd Previte <tprevite@gmail.com>]

On 12/12/14 1:25 PM, Paulo Zanoni wrote:
> 2014-12-10 21:53 GMT-02: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.
>>
>> V2:
>> - Addressed previous mailing list feedback
>> - Fixed compilation issue (struct members declared in a later patch)
>> - Updated debug messages to be more accurate
>> - Added status checks for the DPCD read/write calls
>> - Removed excess comments and debug messages
>> - Fixed debug message compilation warnings
>> - Fixed compilation issue with missing variables
>> - Updated link training autotest to ACK
>>
>> Signed-off-by: Todd Previte<tprevite@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c  | 72 +++++++++++++++++++++++++++++++++++++---
>>   drivers/gpu/drm/i915/intel_drv.h |  4 +++
>>   2 files changed, 72 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 3fc3296..3dc92a3 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3744,11 +3744,75 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>>          return true;
>>   }
>>
>> -static void
>> -intel_dp_handle_test_request(struct intel_dp *intel_dp)
>> +static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
>> +{
>> +       uint8_t test_result = DP_TEST_ACK;
>> +       return test_result;
>> +}
>> +
>> +static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
>> +{
>> +       uint8_t test_result = DP_TEST_NAK;
>> +       return test_result;
>> +}
>> +
>> +static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
>> +{
>> +       uint8_t test_result = DP_TEST_NAK;
>> +       return test_result;
>> +}
>> +
>> +static uint8_t intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp)
>> +{
>> +       uint8_t test_result = DP_TEST_NAK;
>> +       return test_result;
>> +}
>> +
>> +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;
>> +
>> +       status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_REQUEST, &rxdata, 1);
>> +       if (status != 0) {
> Why are we checking for zero here? In the "happy case", shouldn't this
> function return 1? To my understanding, we would be ignoring all test
> requests from the users, which means you wouldn't be able to test
> anything in your series at all... I see that you don't change this
> line at all in the rest of your series, so maybe I'm just crazy and
> failing to notice some detail...
>
>
>> +               response = DP_TEST_NAK;
>> +               DRM_DEBUG_KMS("Could not read test request from sink\n");
> You assign a value to "response" but don't do anything to it.
> Shouldn't we be trying to send the NAK in this case? If yes, then the
> code is missing, if no, then I guess we can remove the "response"
> assignment (well, we could remove it in both cases since it's already
> initialized to DP_TEST_NAK anyway).
Good catches on these two - thanks Paulo. They've been fixed in V3.

>> +               return;
>> +       }
>> +
>> +       switch (rxdata) {
>> +       case DP_TEST_LINK_TRAINING:
>> +               DRM_DEBUG_KMS("LINK_TRAINING test requested\n");
>> +               response = intel_dp_autotest_link_training(intel_dp);
>> +               break;
>> +       case DP_TEST_LINK_VIDEO_PATTERN:
>> +               DRM_DEBUG_KMS("TEST_PATTERN test requested\n");
>> +               response = intel_dp_autotest_video_pattern(intel_dp);
>> +               break;
>> +       case DP_TEST_LINK_EDID_READ:
>> +               DRM_DEBUG_KMS("EDID test requested\n");
>> +               response = intel_dp_autotest_edid(intel_dp);
>> +               break;
>> +       case DP_TEST_LINK_PHY_TEST_PATTERN:
>> +               DRM_DEBUG_KMS("PHY_PATTERN test requested\n");
>> +               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;
>> +       default:
>> +               DRM_DEBUG_KMS("Invalid test request '%02x'\n", rxdata);
>> +               break;
>> +       }
>> +       status = drm_dp_dpcd_write(&intel_dp->aux,
>> +                                  DP_TEST_RESPONSE,
>> +                                  &response, 1);
>> +       if (status != 0)
> Same here...
>
Same as above. Fixed in V3.
>> +               DRM_DEBUG_KMS("Could not write test response to sink\n");
>> +
>> +       intel_dp->compliance_testing_active = 0;
>>   }
>>
>>   static int
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 588b618..d1a807a 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -638,6 +638,10 @@ struct intel_dp {
>>                  struct mutex mutex;
>>          } drrs_state;
>>
>> +       /* Displayport compliance testing */
>> +       unsigned long compliance_test_data;
>> +       bool compliance_testing_active;
> Not a change request, but just a note: usually it's better to just add
> new field/members in the patches that actually start using them.
> Because sometimes we merge the first patches before the others, and we
> may decide to change the later patches so they stop using those
> fields, so we risk ending with unused space. Also, adding a field just
> in the patch that uses it allows the reviewer to check if the chosen
> type, name and location are appropriate, etc.
>
Ok I'll keep this in mind moving forward. Thanks Paulo!

>> +
>>   };
>>
>>   struct intel_digital_port {
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

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