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

From: Li Peng <peng.li@linux.intel.com>
To: Alexander Lam <lambchop468@gmail.com>
Cc: intel-gfx@lists.freedesktop.org, peng.li@intel.com
Subject: Re: [PATCH] allow 945 to control self refresh automatically
Date: Fri, 08 Oct 2010 16:05:34 +0800	[thread overview]
Message-ID: <1286525134.3746.18.camel@pli1-desktop> (raw)
In-Reply-To: <1286235085-29700-1-git-send-email-lambchop468@gmail.com>

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