[PATCH] drm/i915/bdw: Add 42ms delay for IPS disable

[PATCH] drm/i915/bdw: Add 42ms delay for IPS disable

[Ben Widawsky]

From: Ben Widawsky <benjamin.widawsky@linux.intel.com>

This is a requirement added to the spec. This patch will present
persistent corruption on the display.

Cc: Art Runyan <arthur.j.runyan@intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_display.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7f02444..ebc84ee 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3583,7 +3583,7 @@ void hsw_disable_ips(struct intel_crtc *crtc)
 		return;
 
 	assert_plane_enabled(dev_priv, crtc->plane);
-	if (IS_BROADWELL(crtc->base.dev)) {
+	if (IS_BROADWELL(dev)) {
 		mutex_lock(&dev_priv->rps.hw_lock);
 		WARN_ON(sandybridge_pcode_write(dev_priv, DISPLAY_IPS_CONTROL, 0));
 		mutex_unlock(&dev_priv->rps.hw_lock);
@@ -3594,6 +3594,10 @@ void hsw_disable_ips(struct intel_crtc *crtc)
 
 	/* We need to wait for a vblank before we can disable the plane. */
 	intel_wait_for_vblank(dev, crtc->pipe);
+
+	/* wait for pcode to finish disabling IPS, which may take up to 42ms */
+	if (IS_BROADWELL(dev))
+		msleep(42);
 }
 
 /** Loads the palette/gamma unit for the CRTC with the prepared values */
-- 
1.9.1

Re: [PATCH] drm/i915/bdw: Add 42ms delay for IPS disable

[Chris Wilson]

On Thu, Apr 10, 2014 at 11:04:17AM -0700, Ben Widawsky wrote:
> From: Ben Widawsky <benjamin.widawsky@linux.intel.com>
> 
> This is a requirement added to the spec. This patch will present
> persistent corruption on the display.

Prevent? I don't think we want to start showing corruption to the user.
 
> Cc: Art Runyan <arthur.j.runyan@intel.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7f02444..ebc84ee 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3583,7 +3583,7 @@ void hsw_disable_ips(struct intel_crtc *crtc)
>  		return;
>  
>  	assert_plane_enabled(dev_priv, crtc->plane);
> -	if (IS_BROADWELL(crtc->base.dev)) {
> +	if (IS_BROADWELL(dev)) {
>  		mutex_lock(&dev_priv->rps.hw_lock);
>  		WARN_ON(sandybridge_pcode_write(dev_priv, DISPLAY_IPS_CONTROL, 0));
>  		mutex_unlock(&dev_priv->rps.hw_lock);
> @@ -3594,6 +3594,10 @@ void hsw_disable_ips(struct intel_crtc *crtc)
>  
>  	/* We need to wait for a vblank before we can disable the plane. */
>  	intel_wait_for_vblank(dev, crtc->pipe);
> +
> +	/* wait for pcode to finish disabling IPS, which may take up to 42ms */
> +	if (IS_BROADWELL(dev))
> +		msleep(42);

Reading too much Douglas Adams?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915/bdw: Add 42ms delay for IPS disable

[Ben Widawsky]

On Thu, Apr 10, 2014 at 07:12:46PM +0100, Chris Wilson wrote:
> On Thu, Apr 10, 2014 at 11:04:17AM -0700, Ben Widawsky wrote:
> > From: Ben Widawsky <benjamin.widawsky@linux.intel.com>
> > 
> > This is a requirement added to the spec. This patch will present
> > persistent corruption on the display.

s/present/prevent

> 
> Prevent? I don't think we want to start showing corruption to the user.
>  
> > Cc: Art Runyan <arthur.j.runyan@intel.com>
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 7f02444..ebc84ee 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3583,7 +3583,7 @@ void hsw_disable_ips(struct intel_crtc *crtc)
> >  		return;
> >  
> >  	assert_plane_enabled(dev_priv, crtc->plane);
> > -	if (IS_BROADWELL(crtc->base.dev)) {
> > +	if (IS_BROADWELL(dev)) {
> >  		mutex_lock(&dev_priv->rps.hw_lock);
> >  		WARN_ON(sandybridge_pcode_write(dev_priv, DISPLAY_IPS_CONTROL, 0));
> >  		mutex_unlock(&dev_priv->rps.hw_lock);
> > @@ -3594,6 +3594,10 @@ void hsw_disable_ips(struct intel_crtc *crtc)
> >  
> >  	/* We need to wait for a vblank before we can disable the plane. */
> >  	intel_wait_for_vblank(dev, crtc->pipe);
> > +
> > +	/* wait for pcode to finish disabling IPS, which may take up to 42ms */
> > +	if (IS_BROADWELL(dev))
> > +		msleep(42);
> 
> Reading too much Douglas Adams?
> -Chris
> 

It's those damn display arTchitects

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915/bdw: Add 42ms delay for IPS disable

[Runyan, Arthur J]

>-----Original Message-----
>From: Ben Widawsky [mailto:benjamin.widawsky@linux.intel.com]
>Sent: Thursday, April 10, 2014 11:41 AM
>To: Chris Wilson; Widawsky, Benjamin; Intel GFX; Runyan, Arthur J
>Subject: Re: [Intel-gfx] [PATCH] drm/i915/bdw: Add 42ms delay for IPS disable
>
>On Thu, Apr 10, 2014 at 07:12:46PM +0100, Chris Wilson wrote:
>> On Thu, Apr 10, 2014 at 11:04:17AM -0700, Ben Widawsky wrote:
>> > From: Ben Widawsky <benjamin.widawsky@linux.intel.com>
>> >
>> > This is a requirement added to the spec. This patch will present
>> > persistent corruption on the display.
>
>s/present/prevent
>
>>
>> Prevent? I don't think we want to start showing corruption to the user.
>>
>> > Cc: Art Runyan <arthur.j.runyan@intel.com>
>> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>> > ---
>> >  drivers/gpu/drm/i915/intel_display.c | 6 +++++-
>> >  1 file changed, 5 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > index 7f02444..ebc84ee 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -3583,7 +3583,7 @@ void hsw_disable_ips(struct intel_crtc *crtc)
>> >  		return;
>> >
>> >  	assert_plane_enabled(dev_priv, crtc->plane);
>> > -	if (IS_BROADWELL(crtc->base.dev)) {
>> > +	if (IS_BROADWELL(dev)) {
>> >  		mutex_lock(&dev_priv->rps.hw_lock);
>> >  		WARN_ON(sandybridge_pcode_write(dev_priv, DISPLAY_IPS_CONTROL,
>0));
>> >  		mutex_unlock(&dev_priv->rps.hw_lock);
>> > @@ -3594,6 +3594,10 @@ void hsw_disable_ips(struct intel_crtc *crtc)
>> >
>> >  	/* We need to wait for a vblank before we can disable the plane. */
>> >  	intel_wait_for_vblank(dev, crtc->pipe);
>> > +
>> > +	/* wait for pcode to finish disabling IPS, which may take up to 42ms */
>> > +	if (IS_BROADWELL(dev))
>> > +		msleep(42);

The added delay for broadwell should come before the wait for vblank.
Also, you should check for IPS_CTL bit 31 (Enable IPS) to become 0, just in case the 42ms was a lie, or something got stuck.  

>>
>> Reading too much Douglas Adams?
>> -Chris
>>
>
>It's those damn display arTchitects

Hah.... I am an Art, but not one of those lofty architects.

>
>--
>Ben Widawsky, Intel Open Source Technology Center

[PATCH] [v2] drm/i915/bdw: Add 42ms delay for IPS disable

[Ben Widawsky]

From: Ben Widawsky <benjamin.widawsky@linux.intel.com>

This is a requirement added to the spec. This patch will prevent
persistent corruption on the display.

v2: Make the wait before the vblank wait. (Art)
Try to finish early by polling the register
s/present/prevent (Chris)

Cc: Art Runyan <arthur.j.runyan@intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_display.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7f02444..05c60b1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3583,10 +3583,13 @@ void hsw_disable_ips(struct intel_crtc *crtc)
 		return;
 
 	assert_plane_enabled(dev_priv, crtc->plane);
-	if (IS_BROADWELL(crtc->base.dev)) {
+	if (IS_BROADWELL(dev)) {
 		mutex_lock(&dev_priv->rps.hw_lock);
 		WARN_ON(sandybridge_pcode_write(dev_priv, DISPLAY_IPS_CONTROL, 0));
 		mutex_unlock(&dev_priv->rps.hw_lock);
+		/* wait for pcode to finish disabling IPS, which may take up to 42ms */
+		if (wait_for((I915_READ(IPS_CTL) & IPS_ENABLE) == 0, 42))
+			DRM_DEBUG_KMS("Timed out waiting for IPS disable\n");
 	} else {
 		I915_WRITE(IPS_CTL, 0);
 		POSTING_READ(IPS_CTL);
-- 
1.9.1

Re: [PATCH] [v2] drm/i915/bdw: Add 42ms delay for IPS disable

[Runyan, Arthur J]

Ben explained some of the fine details of the code to me, and I'm happy.
Reviewed-by: Art Runyan <arthur.j.runyan@intel.com>

>
>From: Ben Widawsky <benjamin.widawsky@linux.intel.com>
>
>This is a requirement added to the spec. This patch will prevent
>persistent corruption on the display.
>
>v2: Make the wait before the vblank wait. (Art)
>Try to finish early by polling the register
>s/present/prevent (Chris)
>
>Cc: Art Runyan <arthur.j.runyan@intel.com>
>Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>---
> drivers/gpu/drm/i915/intel_display.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>index 7f02444..05c60b1 100644
>--- a/drivers/gpu/drm/i915/intel_display.c
>+++ b/drivers/gpu/drm/i915/intel_display.c
>@@ -3583,10 +3583,13 @@ void hsw_disable_ips(struct intel_crtc *crtc)
> 		return;
>
> 	assert_plane_enabled(dev_priv, crtc->plane);
>-	if (IS_BROADWELL(crtc->base.dev)) {
>+	if (IS_BROADWELL(dev)) {
> 		mutex_lock(&dev_priv->rps.hw_lock);
> 		WARN_ON(sandybridge_pcode_write(dev_priv, DISPLAY_IPS_CONTROL,
>0));
> 		mutex_unlock(&dev_priv->rps.hw_lock);
>+		/* wait for pcode to finish disabling IPS, which may take up to 42ms */
>+		if (wait_for((I915_READ(IPS_CTL) & IPS_ENABLE) == 0, 42))
>+			DRM_DEBUG_KMS("Timed out waiting for IPS disable\n");
> 	} else {
> 		I915_WRITE(IPS_CTL, 0);
> 		POSTING_READ(IPS_CTL);
>--
>1.9.1

Re: [PATCH] [v2] drm/i915/bdw: Add 42ms delay for IPS disable

[Daniel Vetter]

On Thu, Apr 10, 2014 at 11:38:21PM +0000, Runyan, Arthur J wrote:
> Ben explained some of the fine details of the code to me, and I'm happy.
> Reviewed-by: Art Runyan <arthur.j.runyan@intel.com>
> 
> >
> >From: Ben Widawsky <benjamin.widawsky@linux.intel.com>
> >
> >This is a requirement added to the spec. This patch will prevent
> >persistent corruption on the display.
> >
> >v2: Make the wait before the vblank wait. (Art)
> >Try to finish early by polling the register
> >s/present/prevent (Chris)
> >
> >Cc: Art Runyan <arthur.j.runyan@intel.com>
> >Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Queued for -next, thanks for the patch.
> >---
> > drivers/gpu/drm/i915/intel_display.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >index 7f02444..05c60b1 100644
> >--- a/drivers/gpu/drm/i915/intel_display.c
> >+++ b/drivers/gpu/drm/i915/intel_display.c
> >@@ -3583,10 +3583,13 @@ void hsw_disable_ips(struct intel_crtc *crtc)
> > 		return;
> >
> > 	assert_plane_enabled(dev_priv, crtc->plane);
> >-	if (IS_BROADWELL(crtc->base.dev)) {
> >+	if (IS_BROADWELL(dev)) {
> > 		mutex_lock(&dev_priv->rps.hw_lock);
> > 		WARN_ON(sandybridge_pcode_write(dev_priv, DISPLAY_IPS_CONTROL,
> >0));
> > 		mutex_unlock(&dev_priv->rps.hw_lock);
> >+		/* wait for pcode to finish disabling IPS, which may take up to 42ms */
> >+		if (wait_for((I915_READ(IPS_CTL) & IPS_ENABLE) == 0, 42))
> >+			DRM_DEBUG_KMS("Timed out waiting for IPS disable\n");

I've upgraded this to an ERROR to make sure we'll get noticed if it
happens.
-Daniel

> > 	} else {
> > 		I915_WRITE(IPS_CTL, 0);
> > 		POSTING_READ(IPS_CTL);
> >--
> >1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch