[PATCH] allow 945 to control self refresh automatically

[PATCH] allow 945 to control self refresh automatically

[Alexander Lam]

[-- Attachment #1: Type: text/plain, Size: 1089 bytes --]


Using 2.6.35.7 (this patch is against drm-intel-next 7dcd249, but untested),
I changed 945's self refresh to work without the need for the driver to
enable/disable self refresh manually based on the idle state of the gpu.
This is much better than enabling/disabling self refresh for various
reasons, including staying in a lower power state for more time and
avoiding the need for cpu cycles.

Something must have been fixed in the driver between the initial
implementation of 945 self refresh and now.
(maybe 944001201ca0196bcdb088129e5866a9f379d08c: drm/i915: enable low
power render writes on GEN3 hardware?)

This patch probably doesn't cover all cases concerning if SR should
be enabled/disabled, not to mention doing things in the wrong order or
in the wrong place.

It only works with printk statements or replacing the printk's
with DRM_DEBUG and turning drm debugging on (I don't understand what is
going on there...).
---
 drivers/gpu/drm/i915/intel_display.c |   37 ++++++++++-----------------------
 1 files changed, 11 insertions(+), 26 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-allow-945-to-control-self-refresh-automatically.patch --]
[-- Type: text/x-patch; name="0001-allow-945-to-control-self-refresh-automatically.patch", Size: 3785 bytes --]

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 69c54c5..97f4d86 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3256,7 +3256,7 @@ static void i9xx_update_wm(struct drm_device *dev, int planea_clock,
 	int planea_wm, planeb_wm;
 	struct intel_watermark_params planea_params, planeb_params;
 	unsigned long line_time_us;
-	int sr_clock, sr_entries = 0;
+	int sr_clock, sr_entries = 0, sr_enabled = 0;
 
 	/* Create copies of the base settings for each pipe */
 	if (IS_CRESTLINE(dev) || IS_I945GM(dev))
@@ -3303,8 +3303,11 @@ static void i9xx_update_wm(struct drm_device *dev, int planea_clock,
 		if (srwm < 0)
 			srwm = 1;
 
-		if (IS_I945G(dev) || IS_I945GM(dev))
+		if (IS_I945G(dev) || IS_I945GM(dev)){
 			I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_FIFO_MASK | (srwm & 0xff));
+			printk("enable memory self refresh on 945\n");
+			sr_enabled = 1;
+		}
 		else if (IS_I915GM(dev)) {
 			/* 915M has a smaller SRWM field */
 			I915_WRITE(FW_BLC_SELF, srwm & 0x3f);
@@ -3313,6 +3316,8 @@ static void i9xx_update_wm(struct drm_device *dev, int planea_clock,
 	} else {
 		/* Turn off self refresh if both pipes are enabled */
 		if (IS_I945G(dev) || IS_I945GM(dev)) {
+			printk("disable memory self refresh on 945\n");
+			sr_enabled = 0;
 			I915_WRITE(FW_BLC_SELF, I915_READ(FW_BLC_SELF)
 				   & ~FW_BLC_SELF_EN);
 		} else if (IS_I915GM(dev)) {
@@ -3332,6 +3337,8 @@ static void i9xx_update_wm(struct drm_device *dev, int planea_clock,
 
 	I915_WRITE(FW_BLC, fwater_lo);
 	I915_WRITE(FW_BLC2, fwater_hi);
+	if (sr_enabled)
+		I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN_MASK | FW_BLC_SELF_EN);
 }
 
 static void i830_update_wm(struct drm_device *dev, int planea_clock, int unused,
@@ -4828,7 +4835,6 @@ static void intel_idle_update(struct work_struct *work)
 	struct drm_device *dev = dev_priv->dev;
 	struct drm_crtc *crtc;
 	struct intel_crtc *intel_crtc;
-	int enabled = 0;
 
 	if (!i915_powersave)
 		return;
@@ -4842,16 +4848,11 @@ static void intel_idle_update(struct work_struct *work)
 		if (!crtc->fb)
 			continue;
 
-		enabled++;
 		intel_crtc = to_intel_crtc(crtc);
 		if (!intel_crtc->busy)
 			intel_decrease_pllclock(crtc);
 	}
 
-	if ((enabled == 1) && (IS_I945G(dev) || IS_I945GM(dev))) {
-		DRM_DEBUG_DRIVER("enable memory self refresh on 945\n");
-		I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN_MASK | FW_BLC_SELF_EN);
-	}
 
 	mutex_unlock(&dev->struct_mutex);
 }
@@ -4876,17 +4877,9 @@ void intel_mark_busy(struct drm_device *dev, struct drm_gem_object *obj)
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return;
 
-	if (!dev_priv->busy) {
-		if (IS_I945G(dev) || IS_I945GM(dev)) {
-			u32 fw_blc_self;
-
-			DRM_DEBUG_DRIVER("disable memory self refresh on 945\n");
-			fw_blc_self = I915_READ(FW_BLC_SELF);
-			fw_blc_self &= ~FW_BLC_SELF_EN;
-			I915_WRITE(FW_BLC_SELF, fw_blc_self | FW_BLC_SELF_EN_MASK);
-		}
+	if (!dev_priv->busy)
 		dev_priv->busy = true;
-	} else
+	else
 		mod_timer(&dev_priv->idle_timer, jiffies +
 			  msecs_to_jiffies(GPU_IDLE_TIMEOUT));
 
@@ -4898,14 +4891,6 @@ void intel_mark_busy(struct drm_device *dev, struct drm_gem_object *obj)
 		intel_fb = to_intel_framebuffer(crtc->fb);
 		if (intel_fb->obj == obj) {
 			if (!intel_crtc->busy) {
-				if (IS_I945G(dev) || IS_I945GM(dev)) {
-					u32 fw_blc_self;
-
-					DRM_DEBUG_DRIVER("disable memory self refresh on 945\n");
-					fw_blc_self = I915_READ(FW_BLC_SELF);
-					fw_blc_self &= ~FW_BLC_SELF_EN;
-					I915_WRITE(FW_BLC_SELF, fw_blc_self | FW_BLC_SELF_EN_MASK);
-				}
 				/* Non-busy -> busy, upclock */
 				intel_increase_pllclock(crtc);
 				intel_crtc->busy = true;

[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] allow 945 to control self refresh automatically

[Li Peng]

[-- Attachment #1.1: Type: text/plain, Size: 5553 bytes --]

Hi,  Alexander:

I think you are right that GEN3 hardware CXSR requires enabling low
power render writes.

This patch is OK for me, but DRM_DEBUG_KMS is better than printk.

Acked-by : Li Peng <peng.li@linux.intel.com>

On Mon, 2010-10-04 at 19:31 -0400, Alexander Lam wrote:

> Using 2.6.35.7 (this patch is against drm-intel-next 7dcd249, but untested),
> I changed 945's self refresh to work without the need for the driver to
> enable/disable self refresh manually based on the idle state of the gpu.
> This is much better than enabling/disabling self refresh for various
> reasons, including staying in a lower power state for more time and
> avoiding the need for cpu cycles.
> 
> Something must have been fixed in the driver between the initial
> implementation of 945 self refresh and now.
> (maybe 944001201ca0196bcdb088129e5866a9f379d08c: drm/i915: enable low
> power render writes on GEN3 hardware?)
> 
> This patch probably doesn't cover all cases concerning if SR should
> be enabled/disabled, not to mention doing things in the wrong order or
> in the wrong place.
> 
> It only works with printk statements or replacing the printk's
> with DRM_DEBUG and turning drm debugging on (I don't understand what is
> going on there...).
> ---
>  drivers/gpu/drm/i915/intel_display.c |   37 ++++++++++-----------------------
>  1 files changed, 11 insertions(+), 26 deletions(-)
> 
> 
> differences between files attachment
> (0001-allow-945-to-control-self-refresh-automatically.patch)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 69c54c5..97f4d86 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3256,7 +3256,7 @@ static void i9xx_update_wm(struct drm_device *dev, int planea_clock,
>  	int planea_wm, planeb_wm;
>  	struct intel_watermark_params planea_params, planeb_params;
>  	unsigned long line_time_us;
> -	int sr_clock, sr_entries = 0;
> +	int sr_clock, sr_entries = 0, sr_enabled = 0;
>  
>  	/* Create copies of the base settings for each pipe */
>  	if (IS_CRESTLINE(dev) || IS_I945GM(dev))
> @@ -3303,8 +3303,11 @@ static void i9xx_update_wm(struct drm_device *dev, int planea_clock,
>  		if (srwm < 0)
>  			srwm = 1;
>  
> -		if (IS_I945G(dev) || IS_I945GM(dev))
> +		if (IS_I945G(dev) || IS_I945GM(dev)){
>  			I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_FIFO_MASK | (srwm & 0xff));
> +			printk("enable memory self refresh on 945\n");
> +			sr_enabled = 1;
> +		}
>  		else if (IS_I915GM(dev)) {
>  			/* 915M has a smaller SRWM field */
>  			I915_WRITE(FW_BLC_SELF, srwm & 0x3f);
> @@ -3313,6 +3316,8 @@ static void i9xx_update_wm(struct drm_device *dev, int planea_clock,
>  	} else {
>  		/* Turn off self refresh if both pipes are enabled */
>  		if (IS_I945G(dev) || IS_I945GM(dev)) {
> +			printk("disable memory self refresh on 945\n");
> +			sr_enabled = 0;
>  			I915_WRITE(FW_BLC_SELF, I915_READ(FW_BLC_SELF)
>  				   & ~FW_BLC_SELF_EN);
>  		} else if (IS_I915GM(dev)) {
> @@ -3332,6 +3337,8 @@ static void i9xx_update_wm(struct drm_device *dev, int planea_clock,
>  
>  	I915_WRITE(FW_BLC, fwater_lo);
>  	I915_WRITE(FW_BLC2, fwater_hi);
> +	if (sr_enabled)
> +		I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN_MASK | FW_BLC_SELF_EN);
>  }
>  
>  static void i830_update_wm(struct drm_device *dev, int planea_clock, int unused,
> @@ -4828,7 +4835,6 @@ static void intel_idle_update(struct work_struct *work)
>  	struct drm_device *dev = dev_priv->dev;
>  	struct drm_crtc *crtc;
>  	struct intel_crtc *intel_crtc;
> -	int enabled = 0;
>  
>  	if (!i915_powersave)
>  		return;
> @@ -4842,16 +4848,11 @@ static void intel_idle_update(struct work_struct *work)
>  		if (!crtc->fb)
>  			continue;
>  
> -		enabled++;
>  		intel_crtc = to_intel_crtc(crtc);
>  		if (!intel_crtc->busy)
>  			intel_decrease_pllclock(crtc);
>  	}
>  
> -	if ((enabled == 1) && (IS_I945G(dev) || IS_I945GM(dev))) {
> -		DRM_DEBUG_DRIVER("enable memory self refresh on 945\n");
> -		I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN_MASK | FW_BLC_SELF_EN);
> -	}
>  
>  	mutex_unlock(&dev->struct_mutex);
>  }
> @@ -4876,17 +4877,9 @@ void intel_mark_busy(struct drm_device *dev, struct drm_gem_object *obj)
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return;
>  
> -	if (!dev_priv->busy) {
> -		if (IS_I945G(dev) || IS_I945GM(dev)) {
> -			u32 fw_blc_self;
> -
> -			DRM_DEBUG_DRIVER("disable memory self refresh on 945\n");
> -			fw_blc_self = I915_READ(FW_BLC_SELF);
> -			fw_blc_self &= ~FW_BLC_SELF_EN;
> -			I915_WRITE(FW_BLC_SELF, fw_blc_self | FW_BLC_SELF_EN_MASK);
> -		}
> +	if (!dev_priv->busy)
>  		dev_priv->busy = true;
> -	} else
> +	else
>  		mod_timer(&dev_priv->idle_timer, jiffies +
>  			  msecs_to_jiffies(GPU_IDLE_TIMEOUT));
>  
> @@ -4898,14 +4891,6 @@ void intel_mark_busy(struct drm_device *dev, struct drm_gem_object *obj)
>  		intel_fb = to_intel_framebuffer(crtc->fb);
>  		if (intel_fb->obj == obj) {
>  			if (!intel_crtc->busy) {
> -				if (IS_I945G(dev) || IS_I945GM(dev)) {
> -					u32 fw_blc_self;
> -
> -					DRM_DEBUG_DRIVER("disable memory self refresh on 945\n");
> -					fw_blc_self = I915_READ(FW_BLC_SELF);
> -					fw_blc_self &= ~FW_BLC_SELF_EN;
> -					I915_WRITE(FW_BLC_SELF, fw_blc_self | FW_BLC_SELF_EN_MASK);
> -				}
>  				/* Non-busy -> busy, upclock */
>  				intel_increase_pllclock(crtc);
>  				intel_crtc->busy = true;
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



[-- Attachment #1.2: Type: text/html, Size: 6022 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] allow 945 to control self refresh automatically

[Chris Wilson]

On Fri, 08 Oct 2010 16:05:34 +0800, Li Peng <peng.li@linux.intel.com> wrote:
> Hi,  Alexander:
> 
> I think you are right that GEN3 hardware CXSR requires enabling low
> power render writes.
> 
> This patch is OK for me, but DRM_DEBUG_KMS is better than printk.

Alexander, if you don't mind respinning your patch to fixup the bare
printks and adding your signed-off-by and Li's ack, I'll apply this to
next and give it some testing.

Thanks,
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] allow 945 to control self refresh automatically

[Alexander Lam]

I changed 945's self refresh to work without the need for the driver to
enable/disable self refresh manually based on the idle state of the gpu.
This is much better than enabling/disabling self refresh for various
reasons, including staying in a lower power state for more time and
avoiding the need for cpu cycles.

Something must have been fixed in the driver between the initial
implementation of 945 self refresh and now.
(maybe 944001201ca0196bcdb088129e5866a9f379d08c: drm/i915: enable low
power render writes on GEN3 hardware?)

This patch probably doesn't cover all cases concerning if SR should
be enabled/disabled, not to mention doing things in the wrong order or
in the wrong place.

Signed-off-by: Alexander Lam <lambchop468@gmail.com>
Acked-by : Li Peng <peng.li@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   37 ++++++++++-----------------------
 1 files changed, 11 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fe56cb3..46f6878 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3316,7 +3316,7 @@ static void i9xx_update_wm(struct drm_device *dev, int planea_clock,
 	int planea_wm, planeb_wm;
 	struct intel_watermark_params planea_params, planeb_params;
 	unsigned long line_time_us;
-	int sr_clock, sr_entries = 0;
+	int sr_clock, sr_entries = 0, sr_enabled = 0;
 
 	/* Create copies of the base settings for each pipe */
 	if (IS_CRESTLINE(dev) || IS_I945GM(dev))
@@ -3363,8 +3363,11 @@ static void i9xx_update_wm(struct drm_device *dev, int planea_clock,
 		if (srwm < 0)
 			srwm = 1;
 
-		if (IS_I945G(dev) || IS_I945GM(dev))
+		if (IS_I945G(dev) || IS_I945GM(dev)){
 			I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_FIFO_MASK | (srwm & 0xff));
+			DRM_DEBUG_KMS("enable memory self refresh on 945\n");
+			sr_enabled = 1;
+		}
 		else if (IS_I915GM(dev)) {
 			/* 915M has a smaller SRWM field */
 			I915_WRITE(FW_BLC_SELF, srwm & 0x3f);
@@ -3373,6 +3376,8 @@ static void i9xx_update_wm(struct drm_device *dev, int planea_clock,
 	} else {
 		/* Turn off self refresh if both pipes are enabled */
 		if (IS_I945G(dev) || IS_I945GM(dev)) {
+			DRM_DEBUG_KMS("disable memory self refresh on 945\n");
+			sr_enabled = 0;
 			I915_WRITE(FW_BLC_SELF, I915_READ(FW_BLC_SELF)
 				   & ~FW_BLC_SELF_EN);
 		} else if (IS_I915GM(dev)) {
@@ -3392,6 +3397,8 @@ static void i9xx_update_wm(struct drm_device *dev, int planea_clock,
 
 	I915_WRITE(FW_BLC, fwater_lo);
 	I915_WRITE(FW_BLC2, fwater_hi);
+	if (sr_enabled)
+		I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN_MASK | FW_BLC_SELF_EN);
 }
 
 static void i830_update_wm(struct drm_device *dev, int planea_clock, int unused,
@@ -5125,7 +5132,6 @@ static void intel_idle_update(struct work_struct *work)
 	struct drm_device *dev = dev_priv->dev;
 	struct drm_crtc *crtc;
 	struct intel_crtc *intel_crtc;
-	int enabled = 0;
 
 	if (!i915_powersave)
 		return;
@@ -5139,16 +5145,11 @@ static void intel_idle_update(struct work_struct *work)
 		if (!crtc->fb)
 			continue;
 
-		enabled++;
 		intel_crtc = to_intel_crtc(crtc);
 		if (!intel_crtc->busy)
 			intel_decrease_pllclock(crtc);
 	}
 
-	if ((enabled == 1) && (IS_I945G(dev) || IS_I945GM(dev))) {
-		DRM_DEBUG_DRIVER("enable memory self refresh on 945\n");
-		I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN_MASK | FW_BLC_SELF_EN);
-	}
 
 	mutex_unlock(&dev->struct_mutex);
 }
@@ -5173,17 +5174,9 @@ void intel_mark_busy(struct drm_device *dev, struct drm_i915_gem_object *obj)
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return;
 
-	if (!dev_priv->busy) {
-		if (IS_I945G(dev) || IS_I945GM(dev)) {
-			u32 fw_blc_self;
-
-			DRM_DEBUG_DRIVER("disable memory self refresh on 945\n");
-			fw_blc_self = I915_READ(FW_BLC_SELF);
-			fw_blc_self &= ~FW_BLC_SELF_EN;
-			I915_WRITE(FW_BLC_SELF, fw_blc_self | FW_BLC_SELF_EN_MASK);
-		}
+	if (!dev_priv->busy)
 		dev_priv->busy = true;
-	} else
+	else
 		mod_timer(&dev_priv->idle_timer, jiffies +
 			  msecs_to_jiffies(GPU_IDLE_TIMEOUT));
 
@@ -5195,14 +5188,6 @@ void intel_mark_busy(struct drm_device *dev, struct drm_i915_gem_object *obj)
 		intel_fb = to_intel_framebuffer(crtc->fb);
 		if (intel_fb->obj == obj) {
 			if (!intel_crtc->busy) {
-				if (IS_I945G(dev) || IS_I945GM(dev)) {
-					u32 fw_blc_self;
-
-					DRM_DEBUG_DRIVER("disable memory self refresh on 945\n");
-					fw_blc_self = I915_READ(FW_BLC_SELF);
-					fw_blc_self &= ~FW_BLC_SELF_EN;
-					I915_WRITE(FW_BLC_SELF, fw_blc_self | FW_BLC_SELF_EN_MASK);
-				}
 				/* Non-busy -> busy, upclock */
 				intel_increase_pllclock(crtc);
 				intel_crtc->busy = true;
-- 
1.7.3.4

Re: [PATCH] allow 945 to control self refresh automatically

[Chris Wilson]

On Mon,  3 Jan 2011 13:28:56 -0500, Alexander Lam <lambchop468@gmail.com> wrote:
> I changed 945's self refresh to work without the need for the driver to
> enable/disable self refresh manually based on the idle state of the gpu.
> This is much better than enabling/disabling self refresh for various
> reasons, including staying in a lower power state for more time and
> avoiding the need for cpu cycles.
> 
> Something must have been fixed in the driver between the initial
> implementation of 945 self refresh and now.
> (maybe 944001201ca0196bcdb088129e5866a9f379d08c: drm/i915: enable low
> power render writes on GEN3 hardware?)

Considering that there is no rationale in the git history as why it was
done manually, maybe you could explain the reason why it could not have
been automatically before? Was it simply causing hangs? Do you have any
measurements that show power/performance impacts of the switch?
 
> This patch probably doesn't cover all cases concerning if SR should
> be enabled/disabled, not to mention doing things in the wrong order or
> in the wrong place.

Does this patch introduce bugs? Certainly sounds like it does, but that may
have just been badly phrased.

Reading the patch did raise one concern:
>  		/* Turn off self refresh if both pipes are enabled */
>  		if (IS_I945G(dev) || IS_I945GM(dev)) {
> +			DRM_DEBUG_KMS("disable memory self refresh on 945\n");
> +			sr_enabled = 0;
>  			I915_WRITE(FW_BLC_SELF, I915_READ(FW_BLC_SELF)
>  				   & ~FW_BLC_SELF_EN);

This looks wrong, as elsewhere to disable self-refresh we do:

I915_WRITE(FW_BLC_SELF, (I915_READ(FW_BLC_SELF) | FW_BLC_SELF_EN_MASK) & ~FB_BLC_SELF_EN);

This looks to be a bug in the original code, 33c5fd12, but does it mean
that upon applying your patch that we never disable SR, even when it is
not supported by the hardware configuration?
-Chrs

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] allow 945 to control self refresh automatically

[Jesse Barnes]

On Mon, 03 Jan 2011 19:33:05 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Mon,  3 Jan 2011 13:28:56 -0500, Alexander Lam <lambchop468@gmail.com> wrote:
> > I changed 945's self refresh to work without the need for the driver to
> > enable/disable self refresh manually based on the idle state of the gpu.
> > This is much better than enabling/disabling self refresh for various
> > reasons, including staying in a lower power state for more time and
> > avoiding the need for cpu cycles.
> > 
> > Something must have been fixed in the driver between the initial
> > implementation of 945 self refresh and now.
> > (maybe 944001201ca0196bcdb088129e5866a9f379d08c: drm/i915: enable low
> > power render writes on GEN3 hardware?)
> 
> Considering that there is no rationale in the git history as why it was
> done manually, maybe you could explain the reason why it could not have
> been automatically before? Was it simply causing hangs? Do you have any
> measurements that show power/performance impacts of the switch?

Yes, it would hang before, but only on some platforms.  It's likely
related to the low power render writes issue...

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] allow 945 to control self refresh automatically

[Alexander Lam]

Hi,

I probably should mention that the patch is in reply to: (I got Li's ack here)
http://lists.freedesktop.org/archives/intel-gfx/2010-October/008302.html

School kept me busy since then and I haven't been able to find any free
time until this winter break to respin. (College unexpectedly took more time
than I imagined)

On Mon, Jan 3, 2011 at 2:33 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> On Mon,  3 Jan 2011 13:28:56 -0500, Alexander Lam <lambchop468@gmail.com> wrote:
> > I changed 945's self refresh to work without the need for the driver to
> > enable/disable self refresh manually based on the idle state of the gpu.
> > This is much better than enabling/disabling self refresh for various
> > reasons, including staying in a lower power state for more time and
> > avoiding the need for cpu cycles.
> >
> > Something must have been fixed in the driver between the initial
> > implementation of 945 self refresh and now.
> > (maybe 944001201ca0196bcdb088129e5866a9f379d08c: drm/i915: enable low
> > power render writes on GEN3 hardware?)
>
> Considering that there is no rationale in the git history as why it was
> done manually, maybe you could explain the reason why it could not have
> been automatically before? Was it simply causing hangs? Do you have any
> measurements that show power/performance impacts of the switch?

It is explained in commit ee980b80
(same as Jesse says)

I don't have any measurements (although the absolute idle power savings
is the same before and after). The problem is that I don't really have a way
to reliably reproduce a workload situation for this and measure average
power consumption (I guess I could throw together a test, but I don't think
I have time for that). Anyway, this would totally solve the problem fixed
by 060e645a.

> > This patch probably doesn't cover all cases concerning if SR should
> > be enabled/disabled, not to mention doing things in the wrong order or
> > in the wrong place.
>
> Does this patch introduce bugs? Certainly sounds like it does, but that may
> have just been badly phrased.

What that really is is a disclaimer that I might be programming the
hardware's bits
in the wrong order (i.e. I don't know if we are allowed to write FW_BLC_SELF_EN
before writing the actual watermarks) because I don't have the hardware docs.

> Reading the patch did raise one concern:
> >               /* Turn off self refresh if both pipes are enabled */
> >               if (IS_I945G(dev) || IS_I945GM(dev)) {
> > +                     DRM_DEBUG_KMS("disable memory self refresh on 945\n");
> > +                     sr_enabled = 0;
> >                       I915_WRITE(FW_BLC_SELF, I915_READ(FW_BLC_SELF)
> >                                  & ~FW_BLC_SELF_EN);
>
> This looks wrong, as elsewhere to disable self-refresh we do:
>
> I915_WRITE(FW_BLC_SELF, (I915_READ(FW_BLC_SELF) | FW_BLC_SELF_EN_MASK) & ~FB_BLC_SELF_EN);
>
> This looks to be a bug in the original code, 33c5fd12, but does it mean
> that upon applying your patch that we never disable SR, even when it is
> not supported by the hardware configuration?

ee980b80 uses both methods, so I'm not sure. I figured going with the
original code would be best, but I'm not entirely sure without
docs. I didn't consider that the original code was incorrect, so it
may be because
the manual enable/disable could have masked the bug. I am not sure why this
issue didn't show up in testing....should I respin?


Also, is it possible for me to move
I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN_MASK | FW_BLC_SELF_EN);
to before the watermark is written (to avoid needing sr_enabled; I
used sr_enabled
to keep the ordering of writing these bits the same compared to prior
to this patch)

> -Chrs
>
> --
> Chris Wilson, Intel Open Source Technology Centre

Thanks,

--
Alexander Lam

[PATCH] drm/i915: allow 945 to control self refresh (CxSR) automatically

[Chris Wilson]

From: Alexander Lam <lambchop468@gmail.com>

I changed 945's self refresh to work without the need for the driver to
enable/disable self refresh manually based on the idle state of the gpu.
This is much better than enabling/disabling self refresh for various
reasons, including staying in a lower power state for more time and
avoiding the need for cpu cycles.

This was originally done manually to workaround issues with the hardware
hanging. However, since 944001201: drm/i915: enable low power render
writes on GEN3 hardware, automatic CxSR seems stable.

Signed-off-by: Alexander Lam <lambchop468@gmail.com>
Acked-by : Li Peng <peng.li@linux.intel.com>
[ickle: play safe with the ordering and disable CxSR before tweaking any
watermark and enable afterwards.]
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |   62 +++++++++++++---------------------
 1 files changed, 24 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 14ac352..09d3eaa 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3316,7 +3316,7 @@ static void i9xx_update_wm(struct drm_device *dev, int planea_clock,
 	int planea_wm, planeb_wm;
 	struct intel_watermark_params planea_params, planeb_params;
 	unsigned long line_time_us;
-	int sr_clock, sr_entries = 0;
+	int sr_clock, sr_entries = 0, sr_enabled = 0;
 
 	/* Create copies of the base settings for each pipe */
 	if (IS_CRESTLINE(dev) || IS_I945GM(dev))
@@ -3345,6 +3345,12 @@ static void i9xx_update_wm(struct drm_device *dev, int planea_clock,
 	 */
 	cwm = 2;
 
+	/* Play safe and disable self-refresh before adjusting watermarks. */
+	if (IS_I945G(dev) || IS_I945GM(dev))
+		I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN_MASK | 0);
+	else if (IS_I915GM(dev))
+		I915_WRITE(INSTPM, I915_READ(INSTPM) & ~INSTPM_SELF_EN);
+
 	/* Calc sr entries for one plane configs */
 	if (HAS_FW_BLC(dev) && sr_hdisplay &&
 	    (!planea_clock || !planeb_clock)) {
@@ -3364,20 +3370,12 @@ static void i9xx_update_wm(struct drm_device *dev, int planea_clock,
 			srwm = 1;
 
 		if (IS_I945G(dev) || IS_I945GM(dev))
-			I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_FIFO_MASK | (srwm & 0xff));
-		else if (IS_I915GM(dev)) {
-			/* 915M has a smaller SRWM field */
+			I915_WRITE(FW_BLC_SELF,
+				   FW_BLC_SELF_FIFO_MASK | (srwm & 0xff));
+		else if (IS_I915GM(dev))
 			I915_WRITE(FW_BLC_SELF, srwm & 0x3f);
-			I915_WRITE(INSTPM, I915_READ(INSTPM) | INSTPM_SELF_EN);
-		}
-	} else {
-		/* Turn off self refresh if both pipes are enabled */
-		if (IS_I945G(dev) || IS_I945GM(dev)) {
-			I915_WRITE(FW_BLC_SELF, I915_READ(FW_BLC_SELF)
-				   & ~FW_BLC_SELF_EN);
-		} else if (IS_I915GM(dev)) {
-			I915_WRITE(INSTPM, I915_READ(INSTPM) & ~INSTPM_SELF_EN);
-		}
+
+		sr_enabled = 1;
 	}
 
 	DRM_DEBUG_KMS("Setting FIFO watermarks - A: %d, B: %d, C: %d, SR %d\n",
@@ -3392,6 +3390,16 @@ static void i9xx_update_wm(struct drm_device *dev, int planea_clock,
 
 	I915_WRITE(FW_BLC, fwater_lo);
 	I915_WRITE(FW_BLC2, fwater_hi);
+
+	if (sr_enabled) {
+		if (IS_I945G(dev) || IS_I945GM(dev))
+			I915_WRITE(FW_BLC_SELF,
+				   FW_BLC_SELF_EN_MASK | FW_BLC_SELF_EN);
+		else if (IS_I915GM(dev))
+			I915_WRITE(INSTPM, I915_READ(INSTPM) | INSTPM_SELF_EN);
+		DRM_DEBUG_KMS("memory self refresh enabled\n");
+	} else
+		DRM_DEBUG_KMS("memory self refresh disabled\n");
 }
 
 static void i830_update_wm(struct drm_device *dev, int planea_clock, int unused,
@@ -5132,7 +5140,6 @@ static void intel_idle_update(struct work_struct *work)
 	struct drm_device *dev = dev_priv->dev;
 	struct drm_crtc *crtc;
 	struct intel_crtc *intel_crtc;
-	int enabled = 0;
 
 	if (!i915_powersave)
 		return;
@@ -5146,16 +5153,11 @@ static void intel_idle_update(struct work_struct *work)
 		if (!crtc->fb)
 			continue;
 
-		enabled++;
 		intel_crtc = to_intel_crtc(crtc);
 		if (!intel_crtc->busy)
 			intel_decrease_pllclock(crtc);
 	}
 
-	if ((enabled == 1) && (IS_I945G(dev) || IS_I945GM(dev))) {
-		DRM_DEBUG_DRIVER("enable memory self refresh on 945\n");
-		I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN_MASK | FW_BLC_SELF_EN);
-	}
 
 	mutex_unlock(&dev->struct_mutex);
 }
@@ -5180,17 +5182,9 @@ void intel_mark_busy(struct drm_device *dev, struct drm_i915_gem_object *obj)
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return;
 
-	if (!dev_priv->busy) {
-		if (IS_I945G(dev) || IS_I945GM(dev)) {
-			u32 fw_blc_self;
-
-			DRM_DEBUG_DRIVER("disable memory self refresh on 945\n");
-			fw_blc_self = I915_READ(FW_BLC_SELF);
-			fw_blc_self &= ~FW_BLC_SELF_EN;
-			I915_WRITE(FW_BLC_SELF, fw_blc_self | FW_BLC_SELF_EN_MASK);
-		}
+	if (!dev_priv->busy)
 		dev_priv->busy = true;
-	} else
+	else
 		mod_timer(&dev_priv->idle_timer, jiffies +
 			  msecs_to_jiffies(GPU_IDLE_TIMEOUT));
 
@@ -5202,14 +5196,6 @@ void intel_mark_busy(struct drm_device *dev, struct drm_i915_gem_object *obj)
 		intel_fb = to_intel_framebuffer(crtc->fb);
 		if (intel_fb->obj == obj) {
 			if (!intel_crtc->busy) {
-				if (IS_I945G(dev) || IS_I945GM(dev)) {
-					u32 fw_blc_self;
-
-					DRM_DEBUG_DRIVER("disable memory self refresh on 945\n");
-					fw_blc_self = I915_READ(FW_BLC_SELF);
-					fw_blc_self &= ~FW_BLC_SELF_EN;
-					I915_WRITE(FW_BLC_SELF, fw_blc_self | FW_BLC_SELF_EN_MASK);
-				}
 				/* Non-busy -> busy, upclock */
 				intel_increase_pllclock(crtc);
 				intel_crtc->busy = true;
-- 
1.7.2.3