Re: [PATCH] drm/i915: Drop vblank wait from intel_dp_link_down

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 26 Nov 2014, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2014-11-24 13:54 GMT-02:00 Daniel Vetter <daniel.vetter@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@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@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@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.

BR,
Jani.



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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx