[PATCH v2 1/3] drm/i915: gmch: factor out intel_set_memory_cxsr

[PATCH v2 1/3] drm/i915: gmch: factor out intel_set_memory_cxsr

[Imre Deak]

This functionality will be also needed by an upcoming patch, so factor
it out. As a bonus this also makes things a bit more uniform across
platforms. Note that this also changes the register read-modify-write
to a simple write during disabling. This is what we do during enabling
anyway and according to the spec all the relevant bits are reserved-MBZ
or reserved with a 0 default value.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  2 ++
 drivers/gpu/drm/i915/intel_pm.c | 75 ++++++++++++++++++++---------------------
 2 files changed, 39 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f36d9eb..211a173 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2590,6 +2590,8 @@ extern void gen6_set_rps(struct drm_device *dev, u8 val);
 extern void valleyview_set_rps(struct drm_device *dev, u8 val);
 extern int valleyview_rps_max_freq(struct drm_i915_private *dev_priv);
 extern int valleyview_rps_min_freq(struct drm_i915_private *dev_priv);
+extern void intel_set_memory_cxsr(struct drm_i915_private *dev_priv,
+				  bool enable);
 extern void intel_detect_pch(struct drm_device *dev);
 extern int intel_trans_dp_port_sel(struct drm_crtc *crtc);
 extern int intel_enable_rc6(const struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0b088fe..e55622e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -789,12 +789,33 @@ static const struct cxsr_latency *intel_get_cxsr_latency(int is_desktop,
 	return NULL;
 }
 
-static void pineview_disable_cxsr(struct drm_device *dev)
+void intel_set_memory_cxsr(struct drm_i915_private *dev_priv, bool enable)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_device *dev = dev_priv->dev;
+	u32 val;
 
-	/* deactivate cxsr */
-	I915_WRITE(DSPFW3, I915_READ(DSPFW3) & ~PINEVIEW_SELF_REFRESH_EN);
+	if (IS_VALLEYVIEW(dev)) {
+		I915_WRITE(FW_BLC_SELF_VLV, enable ? FW_CSPWRDWNEN : 0);
+	} else if (IS_G4X(dev) || IS_CRESTLINE(dev)) {
+		I915_WRITE(FW_BLC_SELF, enable ? FW_BLC_SELF_EN : 0);
+	} else if (IS_PINEVIEW(dev)) {
+		val = I915_READ(DSPFW3) & ~PINEVIEW_SELF_REFRESH_EN;
+		val |= enable ? PINEVIEW_SELF_REFRESH_EN : 0;
+		I915_WRITE(DSPFW3, val);
+	} else if (IS_I945G(dev) || IS_I945GM(dev)) {
+		val = enable ? _MASKED_BIT_ENABLE(FW_BLC_SELF_EN) :
+			       _MASKED_BIT_DISABLE(FW_BLC_SELF_EN);
+		I915_WRITE(FW_BLC_SELF, val);
+	} else if (IS_I915GM(dev)) {
+		val = enable ? _MASKED_BIT_ENABLE(INSTPM_SELF_EN) :
+			       _MASKED_BIT_DISABLE(INSTPM_SELF_EN);
+		I915_WRITE(INSTPM, val);
+	} else {
+		return;
+	}
+
+	DRM_DEBUG_KMS("memory self-refresh is %s\n",
+		      enable ? "enabled" : "disabled");
 }
 
 /*
@@ -1033,7 +1054,7 @@ static void pineview_update_wm(struct drm_crtc *unused_crtc)
 					 dev_priv->fsb_freq, dev_priv->mem_freq);
 	if (!latency) {
 		DRM_DEBUG_KMS("Unknown FSB/MEM found, disable CxSR\n");
-		pineview_disable_cxsr(dev);
+		intel_set_memory_cxsr(dev_priv, false);
 		return;
 	}
 
@@ -1085,12 +1106,7 @@ static void pineview_update_wm(struct drm_crtc *unused_crtc)
 		DRM_DEBUG_KMS("DSPFW3 register is %x\n", reg);
 
 		/* activate cxsr */
-		I915_WRITE(DSPFW3,
-			   I915_READ(DSPFW3) | PINEVIEW_SELF_REFRESH_EN);
-		DRM_DEBUG_KMS("Self-refresh is enabled\n");
-	} else {
-		pineview_disable_cxsr(dev);
-		DRM_DEBUG_KMS("Self-refresh is disabled\n");
+		intel_set_memory_cxsr(dev_priv, true);
 	}
 }
 
@@ -1342,10 +1358,9 @@ static void valleyview_update_wm(struct drm_crtc *crtc)
 			     &valleyview_wm_info,
 			     &valleyview_cursor_wm_info,
 			     &ignore_plane_sr, &cursor_sr)) {
-		I915_WRITE(FW_BLC_SELF_VLV, FW_CSPWRDWNEN);
+		intel_set_memory_cxsr(dev_priv, true);
 	} else {
-		I915_WRITE(FW_BLC_SELF_VLV,
-			   I915_READ(FW_BLC_SELF_VLV) & ~FW_CSPWRDWNEN);
+		intel_set_memory_cxsr(dev_priv, false);
 		plane_sr = cursor_sr = 0;
 	}
 
@@ -1394,10 +1409,9 @@ static void g4x_update_wm(struct drm_crtc *crtc)
 			     &g4x_wm_info,
 			     &g4x_cursor_wm_info,
 			     &plane_sr, &cursor_sr)) {
-		I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN);
+		intel_set_memory_cxsr(dev_priv, true);
 	} else {
-		I915_WRITE(FW_BLC_SELF,
-			   I915_READ(FW_BLC_SELF) & ~FW_BLC_SELF_EN);
+		intel_set_memory_cxsr(dev_priv, false);
 		plane_sr = cursor_sr = 0;
 	}
 
@@ -1468,13 +1482,10 @@ 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);
 
-		if (IS_CRESTLINE(dev))
-			I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN);
+		intel_set_memory_cxsr(dev_priv, true);
 	} else {
 		/* Turn off self refresh if both pipes are enabled */
-		if (IS_CRESTLINE(dev))
-			I915_WRITE(FW_BLC_SELF, I915_READ(FW_BLC_SELF)
-				   & ~FW_BLC_SELF_EN);
+		intel_set_memory_cxsr(dev_priv, false);
 	}
 
 	DRM_DEBUG_KMS("Setting FIFO watermarks - A: 8, B: 8, C: 8, SR %d\n",
@@ -1560,10 +1571,7 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
 	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, _MASKED_BIT_DISABLE(INSTPM_SELF_EN));
+	intel_set_memory_cxsr(dev_priv, false);
 
 	/* Calc sr entries for one plane configs */
 	if (HAS_FW_BLC(dev) && enabled) {
@@ -1609,17 +1617,8 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
 	I915_WRITE(FW_BLC, fwater_lo);
 	I915_WRITE(FW_BLC2, fwater_hi);
 
-	if (HAS_FW_BLC(dev)) {
-		if (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, _MASKED_BIT_ENABLE(INSTPM_SELF_EN));
-			DRM_DEBUG_KMS("memory self refresh enabled\n");
-		} else
-			DRM_DEBUG_KMS("memory self refresh disabled\n");
-	}
+	if (enabled)
+		intel_set_memory_cxsr(dev_priv, true);
 }
 
 static void i845_update_wm(struct drm_crtc *unused_crtc)
@@ -6662,7 +6661,7 @@ void intel_init_pm(struct drm_device *dev)
 				 (dev_priv->is_ddr3 == 1) ? "3" : "2",
 				 dev_priv->fsb_freq, dev_priv->mem_freq);
 			/* Disable CxSR and never update its watermark again */
-			pineview_disable_cxsr(dev);
+			intel_set_memory_cxsr(dev_priv, false);
 			dev_priv->display.update_wm = NULL;
 		} else
 			dev_priv->display.update_wm = pineview_update_wm;
-- 
1.8.4

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

[Imre Deak]

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)
-- 
1.8.4

[PATCH v2 3/3] drm/i915: gmch: fix stuck primary plane due to memory self-refresh mode

[Imre Deak]

Blanking/unblanking the console in a loop on an Asus T100 sometimes
leaves the console blank. After some digging I found that applying

commit 61bc95c1fbbb6a08b55bbe161fdf1ea5493fc595
Author: Egbert Eich <eich@suse.com>
Date:   Mon Mar 4 09:24:38 2013 -0500

    DRM/i915: On G45 enable cursor plane briefly after enabling the display plane.

fixed VLV too.

In my case the problem seemed to happen already during the previous crtc
disabling and went away if I disabled self-refresh mode before disabling
the primary plane.

The root cause for this is that updates from the shadow to live plane
control register are blocked at vblank time if the memory self-refresh
mode (aka max-fifo mode on VLV) is active at that moment. The controller
checks at frame start time if the CPU is in C0 and the self-refresh mode
enable bit is set and if so activates self-reresh mode, otherwise
deactivates it. So to make sure that the plane truly gets disabled before
pipe-off we have to:

1. disable memory self-refresh mode
2. disable plane
3. wait for vblank
4. disable pipe
5. wait for pipe-off

v2:
- add explanation for the root cause from HW team (Cesar Mancini et al)
- remove note about the CPU C7S state, in my latest tests disabling it
  alone didn't make a difference
- add vblank between disabling plane and pipe (Ville)
- apply the same workaround for all gmch platforms (Ville)

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b9251c8..5eb8afe 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4799,6 +4799,16 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	if (IS_GEN2(dev))
 		intel_set_cpu_fifo_underrun_reporting(dev, pipe, false);
 
+	/*
+	 * Vblank time updates from the shadow to live plane control register
+	 * are blocked if the memory self-refresh mode is active at that
+	 * moment. So to make sure the plane gets truly disabled, disable
+	 * first the self-refresh mode. The self-refresh enable bit in turn
+	 * will be checked/applied by the HW only at the next frame start
+	 * event which is after the vblank start event, so we need to have a
+	 * wait-for-vblank between disabling the plane and the pipe.
+	 */
+	intel_set_memory_cxsr(dev_priv, false);
 	intel_crtc_disable_planes(crtc);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
@@ -4807,9 +4817,10 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	/*
 	 * On gen2 planes are double buffered but the pipe isn't, so we must
 	 * wait for planes to fully turn off before disabling the pipe.
+	 * We also need to wait on all gmch platforms because of the
+	 * self-refresh mode constraint explained above.
 	 */
-	if (IS_GEN2(dev))
-		intel_wait_for_vblank(dev, pipe);
+	intel_wait_for_vblank(dev, pipe);
 
 	intel_disable_pipe(dev_priv, pipe);
 
-- 
1.8.4

Re: [PATCH v2 3/3] drm/i915: gmch: fix stuck primary plane due to memory self-refresh mode

[Daniel Vetter]

Adding Egbert since he's done the original hack here. Imre please keep
him on cc.
-Daniel

On Fri, Jun 13, 2014 at 1:54 PM, Imre Deak <imre.deak@intel.com> wrote:
> Blanking/unblanking the console in a loop on an Asus T100 sometimes
> leaves the console blank. After some digging I found that applying
>
> commit 61bc95c1fbbb6a08b55bbe161fdf1ea5493fc595
> Author: Egbert Eich <eich@suse.com>
> Date:   Mon Mar 4 09:24:38 2013 -0500
>
>     DRM/i915: On G45 enable cursor plane briefly after enabling the display plane.
>
> fixed VLV too.
>
> In my case the problem seemed to happen already during the previous crtc
> disabling and went away if I disabled self-refresh mode before disabling
> the primary plane.
>
> The root cause for this is that updates from the shadow to live plane
> control register are blocked at vblank time if the memory self-refresh
> mode (aka max-fifo mode on VLV) is active at that moment. The controller
> checks at frame start time if the CPU is in C0 and the self-refresh mode
> enable bit is set and if so activates self-reresh mode, otherwise
> deactivates it. So to make sure that the plane truly gets disabled before
> pipe-off we have to:
>
> 1. disable memory self-refresh mode
> 2. disable plane
> 3. wait for vblank
> 4. disable pipe
> 5. wait for pipe-off
>
> v2:
> - add explanation for the root cause from HW team (Cesar Mancini et al)
> - remove note about the CPU C7S state, in my latest tests disabling it
>   alone didn't make a difference
> - add vblank between disabling plane and pipe (Ville)
> - apply the same workaround for all gmch platforms (Ville)
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b9251c8..5eb8afe 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4799,6 +4799,16 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>         if (IS_GEN2(dev))
>                 intel_set_cpu_fifo_underrun_reporting(dev, pipe, false);
>
> +       /*
> +        * Vblank time updates from the shadow to live plane control register
> +        * are blocked if the memory self-refresh mode is active at that
> +        * moment. So to make sure the plane gets truly disabled, disable
> +        * first the self-refresh mode. The self-refresh enable bit in turn
> +        * will be checked/applied by the HW only at the next frame start
> +        * event which is after the vblank start event, so we need to have a
> +        * wait-for-vblank between disabling the plane and the pipe.
> +        */
> +       intel_set_memory_cxsr(dev_priv, false);
>         intel_crtc_disable_planes(crtc);
>
>         for_each_encoder_on_crtc(dev, crtc, encoder)
> @@ -4807,9 +4817,10 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>         /*
>          * On gen2 planes are double buffered but the pipe isn't, so we must
>          * wait for planes to fully turn off before disabling the pipe.
> +        * We also need to wait on all gmch platforms because of the
> +        * self-refresh mode constraint explained above.
>          */
> -       if (IS_GEN2(dev))
> -               intel_wait_for_vblank(dev, pipe);
> +       intel_wait_for_vblank(dev, pipe);
>
>         intel_disable_pipe(dev_priv, pipe);
>
> --
> 1.8.4
>
> _______________________________________________
> 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

Re: [PATCH v2 3/3] drm/i915: gmch: fix stuck primary plane due to memory self-refresh mode

[Egbert Eich]

Hi Daniel, hi Imre,

Daniel Vetter writes:
 > Adding Egbert since he's done the original hack here. Imre please keep
 > him on cc.
 > -Daniel

I finally managed to get this set of patches tested on the platform that
exhibited the intermittent blanking problem when terminating the Xserver.

I can confirm that Imre's patches resolve the issue and that g4x_fixup_plane()
which I had introduced after extensive experiments is no longer needed to
prevent the blanking from happening.
If you want I can provide a patch to back this out with the appropriate
comments once Imre's patches are in.

Cheers,
	Egbert.

Re: [PATCH v2 3/3] drm/i915: gmch: fix stuck primary plane due to memory self-refresh mode

[Chris Wilson]

On Fri, Jun 27, 2014 at 12:07:47AM +0200, Egbert Eich wrote:
> 
> Hi Daniel, hi Imre,
> 
> Daniel Vetter writes:
>  > Adding Egbert since he's done the original hack here. Imre please keep
>  > him on cc.
>  > -Daniel
> 
> I finally managed to get this set of patches tested on the platform that
> exhibited the intermittent blanking problem when terminating the Xserver.
> 
> I can confirm that Imre's patches resolve the issue and that g4x_fixup_plane()
> which I had introduced after extensive experiments is no longer needed to
> prevent the blanking from happening.
> If you want I can provide a patch to back this out with the appropriate
> comments once Imre's patches are in.

That would be ideal.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v2 3/3] drm/i915: gmch: fix stuck primary plane due to memory self-refresh mode

[Egbert Eich]

Chris Wilson writes:
 > On Fri, Jun 27, 2014 at 12:07:47AM +0200, Egbert Eich wrote:
 > > 
 > > Hi Daniel, hi Imre,
 > > 
 > > Daniel Vetter writes:
 > >  > Adding Egbert since he's done the original hack here. Imre please keep
 > >  > him on cc.
 > >  > -Daniel
 > > 
 > > I finally managed to get this set of patches tested on the platform that
 > > exhibited the intermittent blanking problem when terminating the Xserver.
 > > 
 > > I can confirm that Imre's patches resolve the issue and that g4x_fixup_plane()
 > > which I had introduced after extensive experiments is no longer needed to
 > > prevent the blanking from happening.
 > > If you want I can provide a patch to back this out with the appropriate
 > > comments once Imre's patches are in.
 > 
 > That would be ideal.

Is there a chance that Imre's patches will go into the
Intel repo any time soon? Then I could use the commit Id in
the patch description.

Cheers,
	Egbert.

Re: [PATCH v2 3/3] drm/i915: gmch: fix stuck primary plane due to memory self-refresh mode

[Imre Deak]

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

Hi Egbert,

On Fri, 2014-06-27 at 15:55 +0200, Egbert Eich wrote:
> Chris Wilson writes:
>  > On Fri, Jun 27, 2014 at 12:07:47AM +0200, Egbert Eich wrote:
>  > > 
>  > > Hi Daniel, hi Imre,
>  > > 
>  > > Daniel Vetter writes:
>  > >  > Adding Egbert since he's done the original hack here. Imre please keep
>  > >  > him on cc.
>  > >  > -Daniel
>  > > 
>  > > I finally managed to get this set of patches tested on the platform that
>  > > exhibited the intermittent blanking problem when terminating the Xserver.
>  > > 
>  > > I can confirm that Imre's patches resolve the issue and that g4x_fixup_plane()
>  > > which I had introduced after extensive experiments is no longer needed to
>  > > prevent the blanking from happening.
>  > > If you want I can provide a patch to back this out with the appropriate
>  > > comments once Imre's patches are in.
>  > 
>  > That would be ideal.
> 
> Is there a chance that Imre's patches will go into the
> Intel repo any time soon? Then I could use the commit Id in
> the patch description.

Yes, Deepak promised to review it early next week, so it could be
applied after that. I guess we could add your Tested-by too.

--Imre

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 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 v2 2/3] drm/i915: gmch: set SR WMs to valid values before enabling them

[Vijay Purushothaman]

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>

Re: [PATCH v2 3/3] drm/i915: gmch: fix stuck primary plane due to memory self-refresh mode

[Vijay Purushothaman]

On 6/13/2014 5:24 PM, Imre Deak wrote:
> Blanking/unblanking the console in a loop on an Asus T100 sometimes
> leaves the console blank. After some digging I found that applying
>
> commit 61bc95c1fbbb6a08b55bbe161fdf1ea5493fc595
> Author: Egbert Eich <eich@suse.com>
> Date:   Mon Mar 4 09:24:38 2013 -0500
>
>      DRM/i915: On G45 enable cursor plane briefly after enabling the display plane.
>
> fixed VLV too.
>
> In my case the problem seemed to happen already during the previous crtc
> disabling and went away if I disabled self-refresh mode before disabling
> the primary plane.
>
> The root cause for this is that updates from the shadow to live plane
> control register are blocked at vblank time if the memory self-refresh
> mode (aka max-fifo mode on VLV) is active at that moment. The controller
> checks at frame start time if the CPU is in C0 and the self-refresh mode
> enable bit is set and if so activates self-reresh mode, otherwise
> deactivates it. So to make sure that the plane truly gets disabled before
> pipe-off we have to:
>
> 1. disable memory self-refresh mode
> 2. disable plane
> 3. wait for vblank
> 4. disable pipe
> 5. wait for pipe-off
>
> v2:
> - add explanation for the root cause from HW team (Cesar Mancini et al)
> - remove note about the CPU C7S state, in my latest tests disabling it
>    alone didn't make a difference
> - add vblank between disabling plane and pipe (Ville)
> - apply the same workaround for all gmch platforms (Ville)
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b9251c8..5eb8afe 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4799,6 +4799,16 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>   	if (IS_GEN2(dev))
>   		intel_set_cpu_fifo_underrun_reporting(dev, pipe, false);
>
> +	/*
> +	 * Vblank time updates from the shadow to live plane control register
> +	 * are blocked if the memory self-refresh mode is active at that
> +	 * moment. So to make sure the plane gets truly disabled, disable
> +	 * first the self-refresh mode. The self-refresh enable bit in turn
> +	 * will be checked/applied by the HW only at the next frame start
> +	 * event which is after the vblank start event, so we need to have a
> +	 * wait-for-vblank between disabling the plane and the pipe.
> +	 */
> +	intel_set_memory_cxsr(dev_priv, false);
>   	intel_crtc_disable_planes(crtc);
>
>   	for_each_encoder_on_crtc(dev, crtc, encoder)
> @@ -4807,9 +4817,10 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>   	/*
>   	 * On gen2 planes are double buffered but the pipe isn't, so we must
>   	 * wait for planes to fully turn off before disabling the pipe.
> +	 * We also need to wait on all gmch platforms because of the
> +	 * self-refresh mode constraint explained above.
>   	 */
> -	if (IS_GEN2(dev))
> -		intel_wait_for_vblank(dev, pipe);
> +	intel_wait_for_vblank(dev, pipe);
>
>   	intel_disable_pipe(dev_priv, pipe);
>
>

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

Re: [PATCH v2 1/3] drm/i915: gmch: factor out intel_set_memory_cxsr

[Deepak S]

On Friday 13 June 2014 05:24 PM, Imre Deak wrote:
> This functionality will be also needed by an upcoming patch, so factor
> it out. As a bonus this also makes things a bit more uniform across
> platforms. Note that this also changes the register read-modify-write
> to a simple write during disabling. This is what we do during enabling
> anyway and according to the spec all the relevant bits are reserved-MBZ
> or reserved with a 0 default value.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h |  2 ++
>   drivers/gpu/drm/i915/intel_pm.c | 75 ++++++++++++++++++++---------------------
>   2 files changed, 39 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f36d9eb..211a173 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2590,6 +2590,8 @@ extern void gen6_set_rps(struct drm_device *dev, u8 val);
>   extern void valleyview_set_rps(struct drm_device *dev, u8 val);
>   extern int valleyview_rps_max_freq(struct drm_i915_private *dev_priv);
>   extern int valleyview_rps_min_freq(struct drm_i915_private *dev_priv);
> +extern void intel_set_memory_cxsr(struct drm_i915_private *dev_priv,
> +				  bool enable);
>   extern void intel_detect_pch(struct drm_device *dev);
>   extern int intel_trans_dp_port_sel(struct drm_crtc *crtc);
>   extern int intel_enable_rc6(const struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 0b088fe..e55622e 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -789,12 +789,33 @@ static const struct cxsr_latency *intel_get_cxsr_latency(int is_desktop,
>   	return NULL;
>   }
>   
> -static void pineview_disable_cxsr(struct drm_device *dev)
> +void intel_set_memory_cxsr(struct drm_i915_private *dev_priv, bool enable)
>   {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_device *dev = dev_priv->dev;
> +	u32 val;
>   
> -	/* deactivate cxsr */
> -	I915_WRITE(DSPFW3, I915_READ(DSPFW3) & ~PINEVIEW_SELF_REFRESH_EN);
> +	if (IS_VALLEYVIEW(dev)) {
> +		I915_WRITE(FW_BLC_SELF_VLV, enable ? FW_CSPWRDWNEN : 0);
> +	} else if (IS_G4X(dev) || IS_CRESTLINE(dev)) {
> +		I915_WRITE(FW_BLC_SELF, enable ? FW_BLC_SELF_EN : 0);
> +	} else if (IS_PINEVIEW(dev)) {
> +		val = I915_READ(DSPFW3) & ~PINEVIEW_SELF_REFRESH_EN;
> +		val |= enable ? PINEVIEW_SELF_REFRESH_EN : 0;
> +		I915_WRITE(DSPFW3, val);
> +	} else if (IS_I945G(dev) || IS_I945GM(dev)) {
> +		val = enable ? _MASKED_BIT_ENABLE(FW_BLC_SELF_EN) :
> +			       _MASKED_BIT_DISABLE(FW_BLC_SELF_EN);
> +		I915_WRITE(FW_BLC_SELF, val);
> +	} else if (IS_I915GM(dev)) {
> +		val = enable ? _MASKED_BIT_ENABLE(INSTPM_SELF_EN) :
> +			       _MASKED_BIT_DISABLE(INSTPM_SELF_EN);
> +		I915_WRITE(INSTPM, val);
> +	} else {
> +		return;
> +	}
> +
> +	DRM_DEBUG_KMS("memory self-refresh is %s\n",
> +		      enable ? "enabled" : "disabled");
>   }
>   
>   /*
> @@ -1033,7 +1054,7 @@ static void pineview_update_wm(struct drm_crtc *unused_crtc)
>   					 dev_priv->fsb_freq, dev_priv->mem_freq);
>   	if (!latency) {
>   		DRM_DEBUG_KMS("Unknown FSB/MEM found, disable CxSR\n");
> -		pineview_disable_cxsr(dev);
> +		intel_set_memory_cxsr(dev_priv, false);
>   		return;
>   	}
>   
> @@ -1085,12 +1106,7 @@ static void pineview_update_wm(struct drm_crtc *unused_crtc)
>   		DRM_DEBUG_KMS("DSPFW3 register is %x\n", reg);
>   
>   		/* activate cxsr */
> -		I915_WRITE(DSPFW3,
> -			   I915_READ(DSPFW3) | PINEVIEW_SELF_REFRESH_EN);
> -		DRM_DEBUG_KMS("Self-refresh is enabled\n");
> -	} else {
> -		pineview_disable_cxsr(dev);
> -		DRM_DEBUG_KMS("Self-refresh is disabled\n");
> +		intel_set_memory_cxsr(dev_priv, true);

I think we need to pass "false" here to disable cxsr right?

Apart for this everything else looks good.

Reviewed-by: Deepak S<deepak.s@linux.intel.com>

>   	}
>   }
>   
> @@ -1342,10 +1358,9 @@ static void valleyview_update_wm(struct drm_crtc *crtc)
>   			     &valleyview_wm_info,
>   			     &valleyview_cursor_wm_info,
>   			     &ignore_plane_sr, &cursor_sr)) {
> -		I915_WRITE(FW_BLC_SELF_VLV, FW_CSPWRDWNEN);
> +		intel_set_memory_cxsr(dev_priv, true);
>   	} else {
> -		I915_WRITE(FW_BLC_SELF_VLV,
> -			   I915_READ(FW_BLC_SELF_VLV) & ~FW_CSPWRDWNEN);
> +		intel_set_memory_cxsr(dev_priv, false);
>   		plane_sr = cursor_sr = 0;
>   	}
>   
> @@ -1394,10 +1409,9 @@ static void g4x_update_wm(struct drm_crtc *crtc)
>   			     &g4x_wm_info,
>   			     &g4x_cursor_wm_info,
>   			     &plane_sr, &cursor_sr)) {
> -		I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN);
> +		intel_set_memory_cxsr(dev_priv, true);
>   	} else {
> -		I915_WRITE(FW_BLC_SELF,
> -			   I915_READ(FW_BLC_SELF) & ~FW_BLC_SELF_EN);
> +		intel_set_memory_cxsr(dev_priv, false);
>   		plane_sr = cursor_sr = 0;
>   	}
>   
> @@ -1468,13 +1482,10 @@ 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);
>   
> -		if (IS_CRESTLINE(dev))
> -			I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN);
> +		intel_set_memory_cxsr(dev_priv, true);
>   	} else {
>   		/* Turn off self refresh if both pipes are enabled */
> -		if (IS_CRESTLINE(dev))
> -			I915_WRITE(FW_BLC_SELF, I915_READ(FW_BLC_SELF)
> -				   & ~FW_BLC_SELF_EN);
> +		intel_set_memory_cxsr(dev_priv, false);
>   	}
>   
>   	DRM_DEBUG_KMS("Setting FIFO watermarks - A: 8, B: 8, C: 8, SR %d\n",
> @@ -1560,10 +1571,7 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
>   	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, _MASKED_BIT_DISABLE(INSTPM_SELF_EN));
> +	intel_set_memory_cxsr(dev_priv, false);
>   
>   	/* Calc sr entries for one plane configs */
>   	if (HAS_FW_BLC(dev) && enabled) {
> @@ -1609,17 +1617,8 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
>   	I915_WRITE(FW_BLC, fwater_lo);
>   	I915_WRITE(FW_BLC2, fwater_hi);
>   
> -	if (HAS_FW_BLC(dev)) {
> -		if (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, _MASKED_BIT_ENABLE(INSTPM_SELF_EN));
> -			DRM_DEBUG_KMS("memory self refresh enabled\n");
> -		} else
> -			DRM_DEBUG_KMS("memory self refresh disabled\n");
> -	}
> +	if (enabled)
> +		intel_set_memory_cxsr(dev_priv, true);
>   }
>   
>   static void i845_update_wm(struct drm_crtc *unused_crtc)
> @@ -6662,7 +6661,7 @@ void intel_init_pm(struct drm_device *dev)
>   				 (dev_priv->is_ddr3 == 1) ? "3" : "2",
>   				 dev_priv->fsb_freq, dev_priv->mem_freq);
>   			/* Disable CxSR and never update its watermark again */
> -			pineview_disable_cxsr(dev);
> +			intel_set_memory_cxsr(dev_priv, false);
>   			dev_priv->display.update_wm = NULL;
>   		} else
>   			dev_priv->display.update_wm = pineview_update_wm;

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

[Deepak S]

On Friday 13 June 2014 05: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: Deepak S<deepak.s@linux.intel.com>

Re: [PATCH v2 3/3] drm/i915: gmch: fix stuck primary plane due to memory self-refresh mode

[Deepak S]

On Friday 13 June 2014 05:24 PM, Imre Deak wrote:
> Blanking/unblanking the console in a loop on an Asus T100 sometimes
> leaves the console blank. After some digging I found that applying
>
> commit 61bc95c1fbbb6a08b55bbe161fdf1ea5493fc595
> Author: Egbert Eich <eich@suse.com>
> Date:   Mon Mar 4 09:24:38 2013 -0500
>
>      DRM/i915: On G45 enable cursor plane briefly after enabling the display plane.
>
> fixed VLV too.
>
> In my case the problem seemed to happen already during the previous crtc
> disabling and went away if I disabled self-refresh mode before disabling
> the primary plane.
>
> The root cause for this is that updates from the shadow to live plane
> control register are blocked at vblank time if the memory self-refresh
> mode (aka max-fifo mode on VLV) is active at that moment. The controller
> checks at frame start time if the CPU is in C0 and the self-refresh mode
> enable bit is set and if so activates self-reresh mode, otherwise
> deactivates it. So to make sure that the plane truly gets disabled before
> pipe-off we have to:
>
> 1. disable memory self-refresh mode
> 2. disable plane
> 3. wait for vblank
> 4. disable pipe
> 5. wait for pipe-off
>
> v2:
> - add explanation for the root cause from HW team (Cesar Mancini et al)
> - remove note about the CPU C7S state, in my latest tests disabling it
>    alone didn't make a difference
> - add vblank between disabling plane and pipe (Ville)
> - apply the same workaround for all gmch platforms (Ville)
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b9251c8..5eb8afe 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4799,6 +4799,16 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>   	if (IS_GEN2(dev))
>   		intel_set_cpu_fifo_underrun_reporting(dev, pipe, false);
>   
> +	/*
> +	 * Vblank time updates from the shadow to live plane control register
> +	 * are blocked if the memory self-refresh mode is active at that
> +	 * moment. So to make sure the plane gets truly disabled, disable
> +	 * first the self-refresh mode. The self-refresh enable bit in turn
> +	 * will be checked/applied by the HW only at the next frame start
> +	 * event which is after the vblank start event, so we need to have a
> +	 * wait-for-vblank between disabling the plane and the pipe.
> +	 */
> +	intel_set_memory_cxsr(dev_priv, false);
>   	intel_crtc_disable_planes(crtc);
>   
>   	for_each_encoder_on_crtc(dev, crtc, encoder)
> @@ -4807,9 +4817,10 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>   	/*
>   	 * On gen2 planes are double buffered but the pipe isn't, so we must
>   	 * wait for planes to fully turn off before disabling the pipe.
> +	 * We also need to wait on all gmch platforms because of the
> +	 * self-refresh mode constraint explained above.
>   	 */
> -	if (IS_GEN2(dev))
> -		intel_wait_for_vblank(dev, pipe);
> +	intel_wait_for_vblank(dev, pipe);
>   
>   	intel_disable_pipe(dev_priv, pipe);
>   

Reviewed-by: Deepak S<deepak.s@linux.intel.com>

Re: [PATCH v2 1/3] drm/i915: gmch: factor out intel_set_memory_cxsr

[Imre Deak]

On Tue, 2014-07-01 at 09:18 +0530, Deepak S wrote:
> On Friday 13 June 2014 05:24 PM, Imre Deak wrote:
> > This functionality will be also needed by an upcoming patch, so factor
> > it out. As a bonus this also makes things a bit more uniform across
> > platforms. Note that this also changes the register read-modify-write
> > to a simple write during disabling. This is what we do during enabling
> > anyway and according to the spec all the relevant bits are reserved-MBZ
> > or reserved with a 0 default value.
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_drv.h |  2 ++
> >   drivers/gpu/drm/i915/intel_pm.c | 75 ++++++++++++++++++++---------------------
> >   2 files changed, 39 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index f36d9eb..211a173 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2590,6 +2590,8 @@ extern void gen6_set_rps(struct drm_device *dev, u8 val);
> >   extern void valleyview_set_rps(struct drm_device *dev, u8 val);
> >   extern int valleyview_rps_max_freq(struct drm_i915_private *dev_priv);
> >   extern int valleyview_rps_min_freq(struct drm_i915_private *dev_priv);
> > +extern void intel_set_memory_cxsr(struct drm_i915_private *dev_priv,
> > +				  bool enable);
> >   extern void intel_detect_pch(struct drm_device *dev);
> >   extern int intel_trans_dp_port_sel(struct drm_crtc *crtc);
> >   extern int intel_enable_rc6(const struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 0b088fe..e55622e 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -789,12 +789,33 @@ static const struct cxsr_latency *intel_get_cxsr_latency(int is_desktop,
> >   	return NULL;
> >   }
> >   
> > -static void pineview_disable_cxsr(struct drm_device *dev)
> > +void intel_set_memory_cxsr(struct drm_i915_private *dev_priv, bool enable)
> >   {
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct drm_device *dev = dev_priv->dev;
> > +	u32 val;
> >   
> > -	/* deactivate cxsr */
> > -	I915_WRITE(DSPFW3, I915_READ(DSPFW3) & ~PINEVIEW_SELF_REFRESH_EN);
> > +	if (IS_VALLEYVIEW(dev)) {
> > +		I915_WRITE(FW_BLC_SELF_VLV, enable ? FW_CSPWRDWNEN : 0);
> > +	} else if (IS_G4X(dev) || IS_CRESTLINE(dev)) {
> > +		I915_WRITE(FW_BLC_SELF, enable ? FW_BLC_SELF_EN : 0);
> > +	} else if (IS_PINEVIEW(dev)) {
> > +		val = I915_READ(DSPFW3) & ~PINEVIEW_SELF_REFRESH_EN;
> > +		val |= enable ? PINEVIEW_SELF_REFRESH_EN : 0;
> > +		I915_WRITE(DSPFW3, val);
> > +	} else if (IS_I945G(dev) || IS_I945GM(dev)) {
> > +		val = enable ? _MASKED_BIT_ENABLE(FW_BLC_SELF_EN) :
> > +			       _MASKED_BIT_DISABLE(FW_BLC_SELF_EN);
> > +		I915_WRITE(FW_BLC_SELF, val);
> > +	} else if (IS_I915GM(dev)) {
> > +		val = enable ? _MASKED_BIT_ENABLE(INSTPM_SELF_EN) :
> > +			       _MASKED_BIT_DISABLE(INSTPM_SELF_EN);
> > +		I915_WRITE(INSTPM, val);
> > +	} else {
> > +		return;
> > +	}
> > +
> > +	DRM_DEBUG_KMS("memory self-refresh is %s\n",
> > +		      enable ? "enabled" : "disabled");
> >   }
> >   
> >   /*
> > @@ -1033,7 +1054,7 @@ static void pineview_update_wm(struct drm_crtc *unused_crtc)
> >   					 dev_priv->fsb_freq, dev_priv->mem_freq);
> >   	if (!latency) {
> >   		DRM_DEBUG_KMS("Unknown FSB/MEM found, disable CxSR\n");
> > -		pineview_disable_cxsr(dev);
> > +		intel_set_memory_cxsr(dev_priv, false);
> >   		return;
> >   	}
> >   
> > @@ -1085,12 +1106,7 @@ static void pineview_update_wm(struct drm_crtc *unused_crtc)
> >   		DRM_DEBUG_KMS("DSPFW3 register is %x\n", reg);
> >   
> >   		/* activate cxsr */
> > -		I915_WRITE(DSPFW3,
> > -			   I915_READ(DSPFW3) | PINEVIEW_SELF_REFRESH_EN);
> > -		DRM_DEBUG_KMS("Self-refresh is enabled\n");
> > -	} else {
> > -		pineview_disable_cxsr(dev);
> > -		DRM_DEBUG_KMS("Self-refresh is disabled\n");
> > +		intel_set_memory_cxsr(dev_priv, true);
> 
> I think we need to pass "false" here to disable cxsr right?

Yes, in the else branch, which I left out for some reason.. Thanks a lot
for spotting it, I'll send an updated patch.

--Imre

> 
> Apart for this everything else looks good.
> 
> Reviewed-by: Deepak S<deepak.s@linux.intel.com>
> 
> >   	}
> >   }
> >   
> > @@ -1342,10 +1358,9 @@ static void valleyview_update_wm(struct drm_crtc *crtc)
> >   			     &valleyview_wm_info,
> >   			     &valleyview_cursor_wm_info,
> >   			     &ignore_plane_sr, &cursor_sr)) {
> > -		I915_WRITE(FW_BLC_SELF_VLV, FW_CSPWRDWNEN);
> > +		intel_set_memory_cxsr(dev_priv, true);
> >   	} else {
> > -		I915_WRITE(FW_BLC_SELF_VLV,
> > -			   I915_READ(FW_BLC_SELF_VLV) & ~FW_CSPWRDWNEN);
> > +		intel_set_memory_cxsr(dev_priv, false);
> >   		plane_sr = cursor_sr = 0;
> >   	}
> >   
> > @@ -1394,10 +1409,9 @@ static void g4x_update_wm(struct drm_crtc *crtc)
> >   			     &g4x_wm_info,
> >   			     &g4x_cursor_wm_info,
> >   			     &plane_sr, &cursor_sr)) {
> > -		I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN);
> > +		intel_set_memory_cxsr(dev_priv, true);
> >   	} else {
> > -		I915_WRITE(FW_BLC_SELF,
> > -			   I915_READ(FW_BLC_SELF) & ~FW_BLC_SELF_EN);
> > +		intel_set_memory_cxsr(dev_priv, false);
> >   		plane_sr = cursor_sr = 0;
> >   	}
> >   
> > @@ -1468,13 +1482,10 @@ 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);
> >   
> > -		if (IS_CRESTLINE(dev))
> > -			I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN);
> > +		intel_set_memory_cxsr(dev_priv, true);
> >   	} else {
> >   		/* Turn off self refresh if both pipes are enabled */
> > -		if (IS_CRESTLINE(dev))
> > -			I915_WRITE(FW_BLC_SELF, I915_READ(FW_BLC_SELF)
> > -				   & ~FW_BLC_SELF_EN);
> > +		intel_set_memory_cxsr(dev_priv, false);
> >   	}
> >   
> >   	DRM_DEBUG_KMS("Setting FIFO watermarks - A: 8, B: 8, C: 8, SR %d\n",
> > @@ -1560,10 +1571,7 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
> >   	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, _MASKED_BIT_DISABLE(INSTPM_SELF_EN));
> > +	intel_set_memory_cxsr(dev_priv, false);
> >   
> >   	/* Calc sr entries for one plane configs */
> >   	if (HAS_FW_BLC(dev) && enabled) {
> > @@ -1609,17 +1617,8 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
> >   	I915_WRITE(FW_BLC, fwater_lo);
> >   	I915_WRITE(FW_BLC2, fwater_hi);
> >   
> > -	if (HAS_FW_BLC(dev)) {
> > -		if (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, _MASKED_BIT_ENABLE(INSTPM_SELF_EN));
> > -			DRM_DEBUG_KMS("memory self refresh enabled\n");
> > -		} else
> > -			DRM_DEBUG_KMS("memory self refresh disabled\n");
> > -	}
> > +	if (enabled)
> > +		intel_set_memory_cxsr(dev_priv, true);
> >   }
> >   
> >   static void i845_update_wm(struct drm_crtc *unused_crtc)
> > @@ -6662,7 +6661,7 @@ void intel_init_pm(struct drm_device *dev)
> >   				 (dev_priv->is_ddr3 == 1) ? "3" : "2",
> >   				 dev_priv->fsb_freq, dev_priv->mem_freq);
> >   			/* Disable CxSR and never update its watermark again */
> > -			pineview_disable_cxsr(dev);
> > +			intel_set_memory_cxsr(dev_priv, false);
> >   			dev_priv->display.update_wm = NULL;
> >   		} else
> >   			dev_priv->display.update_wm = pineview_update_wm;
> 

[PATCH v3 1/3] drm/i915: gmch: factor out intel_set_memory_cxsr

[Imre Deak]

This functionality will be also needed by an upcoming patch, so factor
it out. As a bonus this also makes things a bit more uniform across
platforms. Note that this also changes the register read-modify-write
to a simple write during disabling. This is what we do during enabling
anyway and according to the spec all the relevant bits are reserved-MBZ
or reserved with a 0 default value.

v2:
- unchanged
v3:
- fix missing cxsr disabling on pineview (Deepak)

Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Deepak S <deepak.s@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  2 ++
 drivers/gpu/drm/i915/intel_pm.c | 76 ++++++++++++++++++++---------------------
 2 files changed, 40 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8cea596..c1e306e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2648,6 +2648,8 @@ extern void gen6_set_rps(struct drm_device *dev, u8 val);
 extern void valleyview_set_rps(struct drm_device *dev, u8 val);
 extern int valleyview_rps_max_freq(struct drm_i915_private *dev_priv);
 extern int valleyview_rps_min_freq(struct drm_i915_private *dev_priv);
+extern void intel_set_memory_cxsr(struct drm_i915_private *dev_priv,
+				  bool enable);
 extern void intel_detect_pch(struct drm_device *dev);
 extern int intel_trans_dp_port_sel(struct drm_crtc *crtc);
 extern int intel_enable_rc6(const struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a90fdbd..ddc22c1 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -792,12 +792,33 @@ static const struct cxsr_latency *intel_get_cxsr_latency(int is_desktop,
 	return NULL;
 }
 
-static void pineview_disable_cxsr(struct drm_device *dev)
+void intel_set_memory_cxsr(struct drm_i915_private *dev_priv, bool enable)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_device *dev = dev_priv->dev;
+	u32 val;
 
-	/* deactivate cxsr */
-	I915_WRITE(DSPFW3, I915_READ(DSPFW3) & ~PINEVIEW_SELF_REFRESH_EN);
+	if (IS_VALLEYVIEW(dev)) {
+		I915_WRITE(FW_BLC_SELF_VLV, enable ? FW_CSPWRDWNEN : 0);
+	} else if (IS_G4X(dev) || IS_CRESTLINE(dev)) {
+		I915_WRITE(FW_BLC_SELF, enable ? FW_BLC_SELF_EN : 0);
+	} else if (IS_PINEVIEW(dev)) {
+		val = I915_READ(DSPFW3) & ~PINEVIEW_SELF_REFRESH_EN;
+		val |= enable ? PINEVIEW_SELF_REFRESH_EN : 0;
+		I915_WRITE(DSPFW3, val);
+	} else if (IS_I945G(dev) || IS_I945GM(dev)) {
+		val = enable ? _MASKED_BIT_ENABLE(FW_BLC_SELF_EN) :
+			       _MASKED_BIT_DISABLE(FW_BLC_SELF_EN);
+		I915_WRITE(FW_BLC_SELF, val);
+	} else if (IS_I915GM(dev)) {
+		val = enable ? _MASKED_BIT_ENABLE(INSTPM_SELF_EN) :
+			       _MASKED_BIT_DISABLE(INSTPM_SELF_EN);
+		I915_WRITE(INSTPM, val);
+	} else {
+		return;
+	}
+
+	DRM_DEBUG_KMS("memory self-refresh is %s\n",
+		      enable ? "enabled" : "disabled");
 }
 
 /*
@@ -1036,7 +1057,7 @@ static void pineview_update_wm(struct drm_crtc *unused_crtc)
 					 dev_priv->fsb_freq, dev_priv->mem_freq);
 	if (!latency) {
 		DRM_DEBUG_KMS("Unknown FSB/MEM found, disable CxSR\n");
-		pineview_disable_cxsr(dev);
+		intel_set_memory_cxsr(dev_priv, false);
 		return;
 	}
 
@@ -1087,13 +1108,9 @@ static void pineview_update_wm(struct drm_crtc *unused_crtc)
 		I915_WRITE(DSPFW3, reg);
 		DRM_DEBUG_KMS("DSPFW3 register is %x\n", reg);
 
-		/* activate cxsr */
-		I915_WRITE(DSPFW3,
-			   I915_READ(DSPFW3) | PINEVIEW_SELF_REFRESH_EN);
-		DRM_DEBUG_KMS("Self-refresh is enabled\n");
+		intel_set_memory_cxsr(dev_priv, true);
 	} else {
-		pineview_disable_cxsr(dev);
-		DRM_DEBUG_KMS("Self-refresh is disabled\n");
+		intel_set_memory_cxsr(dev_priv, false);
 	}
 }
 
@@ -1345,10 +1362,9 @@ static void valleyview_update_wm(struct drm_crtc *crtc)
 			     &valleyview_wm_info,
 			     &valleyview_cursor_wm_info,
 			     &ignore_plane_sr, &cursor_sr)) {
-		I915_WRITE(FW_BLC_SELF_VLV, FW_CSPWRDWNEN);
+		intel_set_memory_cxsr(dev_priv, true);
 	} else {
-		I915_WRITE(FW_BLC_SELF_VLV,
-			   I915_READ(FW_BLC_SELF_VLV) & ~FW_CSPWRDWNEN);
+		intel_set_memory_cxsr(dev_priv, false);
 		plane_sr = cursor_sr = 0;
 	}
 
@@ -1397,10 +1413,9 @@ static void g4x_update_wm(struct drm_crtc *crtc)
 			     &g4x_wm_info,
 			     &g4x_cursor_wm_info,
 			     &plane_sr, &cursor_sr)) {
-		I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN);
+		intel_set_memory_cxsr(dev_priv, true);
 	} else {
-		I915_WRITE(FW_BLC_SELF,
-			   I915_READ(FW_BLC_SELF) & ~FW_BLC_SELF_EN);
+		intel_set_memory_cxsr(dev_priv, false);
 		plane_sr = cursor_sr = 0;
 	}
 
@@ -1471,13 +1486,10 @@ 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);
 
-		if (IS_CRESTLINE(dev))
-			I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN);
+		intel_set_memory_cxsr(dev_priv, true);
 	} else {
 		/* Turn off self refresh if both pipes are enabled */
-		if (IS_CRESTLINE(dev))
-			I915_WRITE(FW_BLC_SELF, I915_READ(FW_BLC_SELF)
-				   & ~FW_BLC_SELF_EN);
+		intel_set_memory_cxsr(dev_priv, false);
 	}
 
 	DRM_DEBUG_KMS("Setting FIFO watermarks - A: 8, B: 8, C: 8, SR %d\n",
@@ -1563,10 +1575,7 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
 	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, _MASKED_BIT_DISABLE(INSTPM_SELF_EN));
+	intel_set_memory_cxsr(dev_priv, false);
 
 	/* Calc sr entries for one plane configs */
 	if (HAS_FW_BLC(dev) && enabled) {
@@ -1612,17 +1621,8 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
 	I915_WRITE(FW_BLC, fwater_lo);
 	I915_WRITE(FW_BLC2, fwater_hi);
 
-	if (HAS_FW_BLC(dev)) {
-		if (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, _MASKED_BIT_ENABLE(INSTPM_SELF_EN));
-			DRM_DEBUG_KMS("memory self refresh enabled\n");
-		} else
-			DRM_DEBUG_KMS("memory self refresh disabled\n");
-	}
+	if (enabled)
+		intel_set_memory_cxsr(dev_priv, true);
 }
 
 static void i845_update_wm(struct drm_crtc *unused_crtc)
@@ -6660,7 +6660,7 @@ void intel_init_pm(struct drm_device *dev)
 				 (dev_priv->is_ddr3 == 1) ? "3" : "2",
 				 dev_priv->fsb_freq, dev_priv->mem_freq);
 			/* Disable CxSR and never update its watermark again */
-			pineview_disable_cxsr(dev);
+			intel_set_memory_cxsr(dev_priv, false);
 			dev_priv->display.update_wm = NULL;
 		} else
 			dev_priv->display.update_wm = pineview_update_wm;
-- 
1.8.4

Re: [PATCH v2 3/3] drm/i915: gmch: fix stuck primary plane due to memory self-refresh mode

[Daniel Vetter]

On Fri, Jun 27, 2014 at 09:38:52PM +0300, Imre Deak wrote:
> Hi Egbert,
> 
> On Fri, 2014-06-27 at 15:55 +0200, Egbert Eich wrote:
> > Chris Wilson writes:
> >  > On Fri, Jun 27, 2014 at 12:07:47AM +0200, Egbert Eich wrote:
> >  > > 
> >  > > Hi Daniel, hi Imre,
> >  > > 
> >  > > Daniel Vetter writes:
> >  > >  > Adding Egbert since he's done the original hack here. Imre please keep
> >  > >  > him on cc.
> >  > >  > -Daniel
> >  > > 
> >  > > I finally managed to get this set of patches tested on the platform that
> >  > > exhibited the intermittent blanking problem when terminating the Xserver.
> >  > > 
> >  > > I can confirm that Imre's patches resolve the issue and that g4x_fixup_plane()
> >  > > which I had introduced after extensive experiments is no longer needed to
> >  > > prevent the blanking from happening.
> >  > > If you want I can provide a patch to back this out with the appropriate
> >  > > comments once Imre's patches are in.
> >  > 
> >  > That would be ideal.
> > 
> > Is there a chance that Imre's patches will go into the
> > Intel repo any time soon? Then I could use the commit Id in
> > the patch description.
> 
> Yes, Deepak promised to review it early next week, so it could be
> applied after that. I guess we could add your Tested-by too.

Back from my vacation and patches are merged. Egbert, please submit your
patch so that I can merge it.

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