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

[Paulo Zanoni]

Hi

Since I already commented about the coding style on the previous
reviews, I'll ignore that aspect for the comments below.

2014-10-09 12:38 GMT-03:00 Todd Previte <tprevite at 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 at 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.

> +
>  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.


> +       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.


> +               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?


> +       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.

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



-- 
Paulo Zanoni