[Intel-gfx] [PATCH 02/17] drm/i915: Update intel_dp_check_link_status() for Displayport compliance testing

[Paulo Zanoni]

2014-12-10 21:53 GMT-02:00 Todd Previte <tprevite at gmail.com>:
> Move the DPCD read to the top and check for an interrupt from the sink to catch
> Displayport automated testing requests necessary to support Displayport compliance
> testing. The checks for active connectors and link status are moved below the
> check for the interrupt.

Why exactly is this needed?

>
> Adds a check at the top to verify that the device is connected. This is necessary
> for DP compliance testing to ensure that test requests are captured and acknowledged.

Why exactly? Can you please describe in terms of how the code is
executed in each case and what is missing?

Since there appears to be 2 different changes, shouldn't this patch be
split into 2 different patches?

> If a test request is present during a connected->disconnected transition,
> the test code will attempt to execute even though the connection has been disabled,
> resulting in a faied test.
>
> Signed-off-by: Todd Previte <tprevite at gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 32 +++++++++++++++++++-------------
>  1 file changed, 19 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 3dc92a3..1b452cc 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3890,21 +3890,14 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>
>         WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>
> -       if (!intel_encoder->connectors_active)
> -               return;
> -
> -       if (WARN_ON(!intel_encoder->base.crtc))
> -               return;
> -
> -       if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> -               return;
> -
> -       /* Try to read receiver status if the link appears to be up */
> -       if (!intel_dp_get_link_status(intel_dp, link_status)) {
> +       /* Bail if not connected */

Bikeshed: I don't see the reason for the comment above :)

Other than that, the patch looks fine. Let's see what PRTS will say about it.


> +       if (intel_dp->attached_connector->base.status !=
> +           connector_status_connected) {
> +               DRM_DEBUG_KMS("Not connected\n");
>                 return;
>         }
>
> -       /* Now read the DPCD to see if it's actually running */
> +       /* Attempt to read the DPCD */
>         if (!intel_dp_get_dpcd(intel_dp)) {
>                 return;
>         }
> @@ -3916,13 +3909,26 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>                 drm_dp_dpcd_writeb(&intel_dp->aux,
>                                    DP_DEVICE_SERVICE_IRQ_VECTOR,
>                                    sink_irq_vector);
> -
>                 if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
>                         intel_dp_handle_test_request(intel_dp);
>                 if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ))
>                         DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
>         }
>
> +       if (!intel_encoder->connectors_active)
> +               return;
> +
> +       if (WARN_ON(!intel_encoder->base.crtc))
> +               return;
> +
> +       if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> +               return;
> +
> +       /* Try to read receiver status if the link appears to be up */
> +       if (!intel_dp_get_link_status(intel_dp, link_status)) {
> +               return;
> +       }
> +
>         if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) {
>                 DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
>                               intel_encoder->base.name);
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni