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

[Paulo Zanoni]

2014-12-10 21:53 GMT-02: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.
>
> 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 at 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).


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


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


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



-- 
Paulo Zanoni