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...@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
