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

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

[Daniel Vetter]

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);
+		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

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

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  367/367              367/367
ILK                 -2              375/375              373/375
SNB                                  450/450              450/450
IVB                 -2              503/503              501/503
BYT                                  289/289              289/289
HSW                 -4              567/567              563/567
BDW                                  417/417              417/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 ILK  igt_gem_reset_stats_close-pending-fork-render      TIMEOUT(8, M37M26)PASS(1, M26)      TIMEOUT(1, M37)
 ILK  igt_kms_flip_vblank-vs-hang      TIMEOUT(8, M37M26)PASS(1, M26)      TIMEOUT(1, M37)
 IVB  igt_gem_bad_reloc_negative-reloc      NSPT(14, M34M21M4)PASS(1, M21)      NSPT(1, M21)
 IVB  igt_gem_bad_reloc_negative-reloc-lut      NSPT(3, M21M34M4)PASS(15, M21M34M4)      NSPT(1, M21)
 HSW  igt_gem_bad_reloc_negative-reloc-lut      NSPT(24, M40M20M19)PASS(1, M20)      NSPT(1, M20)
*HSW  igt_kms_fence_pin_leak      PASS(2, M20)      DMESG_WARN(1, M20)
*HSW  igt_kms_rotation_crc_primary-rotation      PASS(23, M20M40M19)      DMESG_WARN(1, M20)
*HSW  igt_pm_rc6_residency_rc6-accuracy      PASS(25, M20M40M19)      FAIL(1, M20)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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?


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

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

[Jani Nikula]

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

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

[Ville Syrjälä]

On Mon, Feb 09, 2015 at 03:15:56PM +0200, Jani Nikula wrote:
> 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.

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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

On Mon, 09 Feb 2015, Ville Syrjälä <ville.syrjala@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@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.
>
> 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.

Jani.

>
> -- 
> Ville Syrjälä
> Intel OTC

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

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

[Jani Nikula]

On Mon, 09 Feb 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Mon, 09 Feb 2015, Ville Syrjälä <ville.syrjala@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@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.
>>
>> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx