Re: [PATCH v2 2/3] drm/i915: gmch: set SR WMs to valid values before enabling them

[Vijay Purushothaman <vijay.a.purushothaman@intel.com>]

On 6/13/2014 5:24 PM, Imre Deak wrote:
> Atm it's possible that we enable the memory self-refresh mode before the
> watermark levels used by this mode are programmed with valid values. So
> move the enabling after we programmed the WM levels.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_pm.c | 21 ++++++++++++++++++---
>   1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index e55622e..c9ee1aa 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1332,6 +1332,7 @@ static void valleyview_update_wm(struct drm_crtc *crtc)
>   	int plane_sr, cursor_sr;
>   	int ignore_plane_sr, ignore_cursor_sr;
>   	unsigned int enabled = 0;
> +	bool cxsr_enabled;
>
>   	vlv_update_drain_latency(dev);
>
> @@ -1358,8 +1359,9 @@ static void valleyview_update_wm(struct drm_crtc *crtc)
>   			     &valleyview_wm_info,
>   			     &valleyview_cursor_wm_info,
>   			     &ignore_plane_sr, &cursor_sr)) {
> -		intel_set_memory_cxsr(dev_priv, true);
> +		cxsr_enabled = true;
>   	} else {
> +		cxsr_enabled = false;
>   		intel_set_memory_cxsr(dev_priv, false);
>   		plane_sr = cursor_sr = 0;
>   	}
> @@ -1380,6 +1382,9 @@ static void valleyview_update_wm(struct drm_crtc *crtc)
>   	I915_WRITE(DSPFW3,
>   		   (I915_READ(DSPFW3) & ~DSPFW_CURSOR_SR_MASK) |
>   		   (cursor_sr << DSPFW_CURSOR_SR_SHIFT));
> +
> +	if (cxsr_enabled)
> +		intel_set_memory_cxsr(dev_priv, true);
>   }
>
>   static void g4x_update_wm(struct drm_crtc *crtc)
> @@ -1390,6 +1395,7 @@ static void g4x_update_wm(struct drm_crtc *crtc)
>   	int planea_wm, planeb_wm, cursora_wm, cursorb_wm;
>   	int plane_sr, cursor_sr;
>   	unsigned int enabled = 0;
> +	bool cxsr_enabled;
>
>   	if (g4x_compute_wm0(dev, PIPE_A,
>   			    &g4x_wm_info, latency_ns,
> @@ -1409,8 +1415,9 @@ static void g4x_update_wm(struct drm_crtc *crtc)
>   			     &g4x_wm_info,
>   			     &g4x_cursor_wm_info,
>   			     &plane_sr, &cursor_sr)) {
> -		intel_set_memory_cxsr(dev_priv, true);
> +		cxsr_enabled = true;
>   	} else {
> +		cxsr_enabled = false;
>   		intel_set_memory_cxsr(dev_priv, false);
>   		plane_sr = cursor_sr = 0;
>   	}
> @@ -1432,6 +1439,9 @@ static void g4x_update_wm(struct drm_crtc *crtc)
>   	I915_WRITE(DSPFW3,
>   		   (I915_READ(DSPFW3) & ~(DSPFW_HPLL_SR_EN | DSPFW_CURSOR_SR_MASK)) |
>   		   (cursor_sr << DSPFW_CURSOR_SR_SHIFT));
> +
> +	if (cxsr_enabled)
> +		intel_set_memory_cxsr(dev_priv, true);
>   }
>
>   static void i965_update_wm(struct drm_crtc *unused_crtc)
> @@ -1441,6 +1451,7 @@ static void i965_update_wm(struct drm_crtc *unused_crtc)
>   	struct drm_crtc *crtc;
>   	int srwm = 1;
>   	int cursor_sr = 16;
> +	bool cxsr_enabled;
>
>   	/* Calc sr entries for one plane configs */
>   	crtc = single_enabled_crtc(dev);
> @@ -1482,8 +1493,9 @@ static void i965_update_wm(struct drm_crtc *unused_crtc)
>   		DRM_DEBUG_KMS("self-refresh watermark: display plane %d "
>   			      "cursor %d\n", srwm, cursor_sr);
>
> -		intel_set_memory_cxsr(dev_priv, true);
> +		cxsr_enabled = true;
>   	} else {
> +		cxsr_enabled = false;
>   		/* Turn off self refresh if both pipes are enabled */
>   		intel_set_memory_cxsr(dev_priv, false);
>   	}
> @@ -1497,6 +1509,9 @@ static void i965_update_wm(struct drm_crtc *unused_crtc)
>   	I915_WRITE(DSPFW2, (8 << 8) | (8 << 0));
>   	/* update cursor SR watermark */
>   	I915_WRITE(DSPFW3, (cursor_sr << DSPFW_CURSOR_SR_SHIFT));
> +
> +	if (cxsr_enabled)
> +		intel_set_memory_cxsr(dev_priv, true);
>   }
>
>   static void i9xx_update_wm(struct drm_crtc *unused_crtc)
>

Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com>