diff mbox

[1/2] drm/i915: remove unexplained vblank wait in the DP off code

Message ID 1397251542-6857-1-git-send-email-jbarnes@virtuousgeek.org (mailing list archive)
State New, archived
Headers show

Commit Message

Jesse Barnes April 11, 2014, 9:25 p.m. UTC
I don't think this is necessary; at least it doesn't appear to be on my
BYT.  Dropping it speeds up our shutdown code a little, in some cases
resulting in faster init times.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_dp.c | 3 ---
 1 file changed, 3 deletions(-)

Comments

Chris Wilson April 12, 2014, 7:03 a.m. UTC | #1
On Fri, Apr 11, 2014 at 02:25:41PM -0700, Jesse Barnes wrote:
> I don't think this is necessary; at least it doesn't appear to be on my
> BYT.  Dropping it speeds up our shutdown code a little, in some cases
> resulting in faster init times.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Also not required on my IVB.
-Chris
Ville Syrjälä April 17, 2014, 12:21 p.m. UTC | #2
On Fri, Apr 11, 2014 at 02:25:41PM -0700, Jesse Barnes wrote:
> I don't think this is necessary; at least it doesn't appear to be on my
> BYT.  Dropping it speeds up our shutdown code a little, in some cases
> resulting in faster init times.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e48d47c..728a5db 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2756,9 +2756,6 @@ intel_dp_link_down(struct intel_dp *intel_dp)
>  	}
>  	POSTING_READ(intel_dp->output_reg);
>  
> -	/* We don't really know why we're doing this */
> -	intel_wait_for_vblank(dev, intel_crtc->pipe);
> -

Maybe this was here to guarantee we send the magic five idle patterns
specified in the DP spec. But since we're going to be turning off the
port anyway I don't see why we switch to transmitting the idle pattern
at all.

I guess switching to the idle pattern might make sense for the IBX
transcoder select workaround to avoid sending some garbage on the main
link. Although we don't seem to be doing that workaround quite according
to spec. The spec says we should first disable the port, and then
re-enable it temporarily w/ transcoder A. What we do is switch the port
over to transcoder A while it's still enabled, and only then disable it.

So I guess killing the wait here is fine, but looks like the IBX
workaround stuff needs a better look. I can try to clean it up a bit.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  	if (HAS_PCH_IBX(dev) &&
>  	    I915_READ(intel_dp->output_reg) & DP_PIPEB_SELECT) {
>  		struct drm_crtc *crtc = intel_dig_port->base.base.crtc;
> -- 
> 1.8.4.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter April 22, 2014, 7:17 p.m. UTC | #3
On Thu, Apr 17, 2014 at 03:21:27PM +0300, Ville Syrjälä wrote:
> On Fri, Apr 11, 2014 at 02:25:41PM -0700, Jesse Barnes wrote:
> > I don't think this is necessary; at least it doesn't appear to be on my
> > BYT.  Dropping it speeds up our shutdown code a little, in some cases
> > resulting in faster init times.
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index e48d47c..728a5db 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -2756,9 +2756,6 @@ intel_dp_link_down(struct intel_dp *intel_dp)
> >  	}
> >  	POSTING_READ(intel_dp->output_reg);
> >  
> > -	/* We don't really know why we're doing this */
> > -	intel_wait_for_vblank(dev, intel_crtc->pipe);
> > -
> 
> Maybe this was here to guarantee we send the magic five idle patterns
> specified in the DP spec. But since we're going to be turning off the
> port anyway I don't see why we switch to transmitting the idle pattern
> at all.
> 
> I guess switching to the idle pattern might make sense for the IBX
> transcoder select workaround to avoid sending some garbage on the main
> link. Although we don't seem to be doing that workaround quite according
> to spec. The spec says we should first disable the port, and then
> re-enable it temporarily w/ transcoder A. What we do is switch the port
> over to transcoder A while it's still enabled, and only then disable it.
> 
> So I guess killing the wait here is fine, but looks like the IBX
> workaround stuff needs a better look. I can try to clean it up a bit.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Queued for -next, thanks for the patch.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e48d47c..728a5db 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2756,9 +2756,6 @@  intel_dp_link_down(struct intel_dp *intel_dp)
 	}
 	POSTING_READ(intel_dp->output_reg);
 
-	/* We don't really know why we're doing this */
-	intel_wait_for_vblank(dev, intel_crtc->pipe);
-
 	if (HAS_PCH_IBX(dev) &&
 	    I915_READ(intel_dp->output_reg) & DP_PIPEB_SELECT) {
 		struct drm_crtc *crtc = intel_dig_port->base.base.crtc;