[Intel-gfx] [PATCH] drm/i915: Drop vblank wait from intel_dp_link_down

[Paulo Zanoni]

2014-11-24 13:54 GMT-02:00 Daniel Vetter <daniel.vetter at ffwll.ch>:
> Nothing in Bspec seems to indicate that we actually needs this, and it
> looks like can't work since by this point the pipe is off and so
> vblanks won't really happen any more.
>
> Note that Bspec mentions that it takes a vblank for this bit to
> change, but _only_ when enabling.
>
> Dropping this code quenches an annoying backtrace introduced by the
> more anal checking since
>
> commit 51e31d49c89055299e34b8f44d13f70e19aaaad1
> Author: Daniel Vetter <daniel.vetter at ffwll.ch>
> Date:   Mon Sep 15 12:36:02 2014 +0200
>
>     drm/i915: Use generic vblank wait
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=86095
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 17 +----------------
>  1 file changed, 1 insertion(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 46731da407c0..63fcdbf90652 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3514,8 +3514,6 @@ intel_dp_link_down(struct intel_dp *intel_dp)
>         enum port port = intel_dig_port->port;
>         struct drm_device *dev = intel_dig_port->base.base.dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       struct intel_crtc *intel_crtc =
> -               to_intel_crtc(intel_dig_port->base.base.crtc);
>         uint32_t DP = intel_dp->DP;
>
>         if (WARN_ON(HAS_DDI(dev)))
> @@ -3540,8 +3538,6 @@ intel_dp_link_down(struct intel_dp *intel_dp)
>
>         if (HAS_PCH_IBX(dev) &&
>             I915_READ(intel_dp->output_reg) & DP_PIPEB_SELECT) {
> -               struct drm_crtc *crtc = intel_dig_port->base.base.crtc;
> -
>                 /* Hardware workaround: leaving our transcoder select
>                  * set to transcoder B while it's off will prevent the
>                  * corresponding HDMI output on transcoder A.
> @@ -3552,18 +3548,7 @@ intel_dp_link_down(struct intel_dp *intel_dp)
>                  */
>                 DP &= ~DP_PIPEB_SELECT;
>                 I915_WRITE(intel_dp->output_reg, DP);
> -
> -               /* Changes to enable or select take place the vblank
> -                * after being written.
> -                */
> -               if (WARN_ON(crtc == NULL)) {
> -                       /* We should never try to disable a port without a crtc
> -                        * attached. For paranoia keep the code around for a
> -                        * bit. */
> -                       POSTING_READ(intel_dp->output_reg);
> -                       msleep(50);
> -               } else
> -                       intel_wait_for_vblank(dev, intel_crtc->pipe);

What I can guess is that we have the vblank wait here because the
DP_PORT_EN bit is still enabled at this point. It would make some
sense to have it if the pipe were not off... So removing the waits
looks sane: Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>

But when I read the spec, it makes me think that maybe doing the
I915_WRITE above is also wrong, since the port is still enabled. Maybe
we should only clear bit 30 in the same write as the one that clears
bit 31?


> +               POSTING_READ(intel_dp->output_reg);
>         }
>
>         DP &= ~DP_AUDIO_OUTPUT_ENABLE;
> --
> 2.1.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni