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

[Jani Nikula]

On Mon, 09 Feb 2015, Jani Nikula <jani.nikula at linux.intel.com> wrote:
> On Mon, 09 Feb 2015, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
>> On Mon, Feb 09, 2015 at 03:15:56PM +0200, Jani Nikula wrote:
>>> On Wed, 26 Nov 2014, Paulo Zanoni <przanoni at gmail.com> wrote:
>>> > 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?
>>> 
>>> Ugh. So the spec says, "When disabling the port, software must
>>> temporarily enable the port with transcoder select (bit #30) cleared to
>>> ‘0’ after disabling the port."
>>> 
>>> IIUC we should disable like we normally do, and do the w/a by enabling
>>> and disabling the port with DP_PIPEB_SELECT cleared *after* that. I
>>> think the current code is wrong, the patch is wrong, what Paulo suggests
>>> is wrong, and also intel_disable_hdmi() is wrong.
>>
>> This code has been bugging me for a long time as well. IIRC I even had
>> cooked up some patches to do the re-enable as you suggest since I
>> read the spec the same way. But I never had enough time to test it. And
>> in order to really test it I would first like to actually reproduce the
>> problem that the workaround is supposed to fix. How else would you know
>> if the workaround is correct after all.
>
> *sigh* an alternative is to apply Daniel's patch and add a comment
> there's something fishy.

I've done just that, with cc: stable for v3.19. I referenced this
discussion from the commit message. Thanks for the patch, review, and
bikeshedding.

BR,
Jani.


>
> Jani.
>
>>
>> -- 
>> Ville Syrjälä
>> Intel OTC
>
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center