[PATCH 0/8] Patches for Sandybridge suspend/resume fixes

[PATCH 0/8] Patches for Sandybridge suspend/resume fixes

[Zhenyu Wang]

Below patches aim to fix https://bugs.freedesktop.org/show_bug.cgi?id=35462.
With them I've been able to run many cycles of S3 tests on two SNB/CPT boards
here, which would hang immediately after S3 resume with upstream kernel.

It still needs more testing, I'd like to get them reviewed first.

Thanks.

[PATCH 1/8] drm/i915: clock gating fix for gen5 and gen6

[Zhenyu Wang]

Some bits should only be set when enable FBC.

Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      |    4 +++-
 drivers/gpu/drm/i915/intel_display.c |   27 ++++++++++++++-------------
 2 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 2abe240..b0aabe4 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2771,8 +2771,10 @@
 #define  ILK_eDP_A_DISABLE		(1<<24)
 #define  ILK_DESKTOP			(1<<23)
 #define ILK_DSPCLK_GATE		0x42020
-#define  ILK_DPARB_CLK_GATE	(1<<5)
+#define  ILK_DPFC_CLK_GATE	(1<<9)
+#define  ILK_DPFCR_CLK_GATE	(1<<8)
 #define  ILK_DPFD_CLK_GATE	(1<<7)
+#define  ILK_DPARB_CLK_GATE	(1<<5)
 
 /* According to spec this bit 7/8/9 of 0x42020 should be set to enable FBC */
 #define   ILK_CLK_FBC		(1<<7)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 232c4ec..13b17e0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6461,7 +6461,7 @@ void intel_enable_clock_gating(struct drm_device *dev)
 				   ILK_FBCQ_DIS);
 			I915_WRITE(ILK_DISPLAY_CHICKEN2,
 				   I915_READ(ILK_DISPLAY_CHICKEN2) |
-				   ILK_DPARB_GATE);
+				   ILK_DPARB_GATE | ILK_ELPIN_409_SELECT);
 			I915_WRITE(ILK_DSPCLK_GATE,
 				   I915_READ(ILK_DSPCLK_GATE) |
 				   ILK_DPFC_DIS1 |
@@ -6469,10 +6469,6 @@ void intel_enable_clock_gating(struct drm_device *dev)
 				   ILK_CLK_FBC);
 		}
 
-		I915_WRITE(ILK_DISPLAY_CHICKEN2,
-			   I915_READ(ILK_DISPLAY_CHICKEN2) |
-			   ILK_ELPIN_409_SELECT);
-
 		if (IS_GEN5(dev)) {
 			I915_WRITE(_3D_CHICKEN2,
 				   _3D_CHICKEN2_WM_READ_PIPELINED << 16 |
@@ -6493,16 +6489,21 @@ void intel_enable_clock_gating(struct drm_device *dev)
 			 * The bit14 of 0x70180
 			 * The bit14 of 0x71180
 			 */
-			I915_WRITE(ILK_DISPLAY_CHICKEN1,
-				   I915_READ(ILK_DISPLAY_CHICKEN1) |
-				   ILK_FBCQ_DIS | ILK_PABSTRETCH_DIS);
+			if (I915_HAS_FBC(dev) && i915_powersave) {
+				I915_WRITE(ILK_DISPLAY_CHICKEN1,
+					   I915_READ(ILK_DISPLAY_CHICKEN1) |
+					   ILK_FBCQ_DIS | ILK_PABSTRETCH_DIS);
+				I915_WRITE(ILK_DSPCLK_GATE,
+					   I915_READ(ILK_DSPCLK_GATE) |
+					   ILK_DPFC_CLK_GATE | ILK_DPFCR_CLK_GATE);
+			} else
+				I915_WRITE(ILK_DISPLAY_CHICKEN1,
+					   I915_READ(ILK_DISPLAY_CHICKEN1) |
+					   ILK_PABSTRETCH_DIS);
+
 			I915_WRITE(ILK_DISPLAY_CHICKEN2,
 				   I915_READ(ILK_DISPLAY_CHICKEN2) |
-				   ILK_DPARB_GATE | ILK_VSDPFD_FULL);
-			I915_WRITE(ILK_DSPCLK_GATE,
-				   I915_READ(ILK_DSPCLK_GATE) |
-				   ILK_DPARB_CLK_GATE  |
-				   ILK_DPFD_CLK_GATE);
+				   ILK_VSDPFD_FULL | ILK_ELPIN_409_SELECT);
 
 			I915_WRITE(DSPACNTR,
 				   I915_READ(DSPACNTR) |
-- 
1.7.4.1

[PATCH 2/8] drm/i915: remove LBB config save/restore

[Zhenyu Wang]

we don't use it anywhere.

Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_suspend.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index 0521ecf..422981b 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -795,8 +795,6 @@ int i915_save_state(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int i;
 
-	pci_read_config_byte(dev->pdev, LBB, &dev_priv->saveLBB);
-
 	/* Hardware status page */
 	dev_priv->saveHWS = I915_READ(HWS_PGA);
 
@@ -844,8 +842,6 @@ int i915_restore_state(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int i;
 
-	pci_write_config_byte(dev->pdev, LBB, dev_priv->saveLBB);
-
 	/* Hardware status page */
 	I915_WRITE(HWS_PGA, dev_priv->saveHWS);
 
-- 
1.7.4.1

Re: [PATCH 2/8] drm/i915: remove LBB config save/restore

[Jesse Barnes]

On Mon, 21 Mar 2011 17:27:13 +0800
Zhenyu Wang <zhenyuw@linux.intel.com> wrote:

> we don't use it anywhere.
> 
> Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_suspend.c |    4 ----
>  1 files changed, 0 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> index 0521ecf..422981b 100644
> --- a/drivers/gpu/drm/i915/i915_suspend.c
> +++ b/drivers/gpu/drm/i915/i915_suspend.c
> @@ -795,8 +795,6 @@ int i915_save_state(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int i;
>  
> -	pci_read_config_byte(dev->pdev, LBB, &dev_priv->saveLBB);
> -
>  	/* Hardware status page */
>  	dev_priv->saveHWS = I915_READ(HWS_PGA);
>  
> @@ -844,8 +842,6 @@ int i915_restore_state(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int i;
>  
> -	pci_write_config_byte(dev->pdev, LBB, dev_priv->saveLBB);
> -
>  	/* Hardware status page */
>  	I915_WRITE(HWS_PGA, dev_priv->saveHWS);

This may affect older platforms and prevent backlight save/restore?  At
least I think that's why we added it in the first place...

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/8] drm/i915: remove LBB config save/restore

[Zhenyu Wang]

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

On 2011.03.21 10:36:01 -0700, Jesse Barnes wrote:
> On Mon, 21 Mar 2011 17:27:13 +0800
> Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> 
> > we don't use it anywhere.
> > 
> > Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_suspend.c |    4 ----
> >  1 files changed, 0 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> > index 0521ecf..422981b 100644
> > --- a/drivers/gpu/drm/i915/i915_suspend.c
> > +++ b/drivers/gpu/drm/i915/i915_suspend.c
> > @@ -795,8 +795,6 @@ int i915_save_state(struct drm_device *dev)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	int i;
> >  
> > -	pci_read_config_byte(dev->pdev, LBB, &dev_priv->saveLBB);
> > -
> >  	/* Hardware status page */
> >  	dev_priv->saveHWS = I915_READ(HWS_PGA);
> >  
> > @@ -844,8 +842,6 @@ int i915_restore_state(struct drm_device *dev)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	int i;
> >  
> > -	pci_write_config_byte(dev->pdev, LBB, dev_priv->saveLBB);
> > -
> >  	/* Hardware status page */
> >  	I915_WRITE(HWS_PGA, dev_priv->saveHWS);
> 
> This may affect older platforms and prevent backlight save/restore?  At
> least I think that's why we added it in the first place...
> 

oh, I just grep it's not used any more, or we plan to add it back?

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 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 2/8] drm/i915: remove LBB config save/restore

[Jesse Barnes]

On Tue, 22 Mar 2011 09:29:14 +0800
Zhenyu Wang <zhenyuw@linux.intel.com> wrote:

> On 2011.03.21 10:36:01 -0700, Jesse Barnes wrote:
> > On Mon, 21 Mar 2011 17:27:13 +0800
> > Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> > 
> > > we don't use it anywhere.
> > > 
> > > Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_suspend.c |    4 ----
> > >  1 files changed, 0 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> > > index 0521ecf..422981b 100644
> > > --- a/drivers/gpu/drm/i915/i915_suspend.c
> > > +++ b/drivers/gpu/drm/i915/i915_suspend.c
> > > @@ -795,8 +795,6 @@ int i915_save_state(struct drm_device *dev)
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > >  	int i;
> > >  
> > > -	pci_read_config_byte(dev->pdev, LBB, &dev_priv->saveLBB);
> > > -
> > >  	/* Hardware status page */
> > >  	dev_priv->saveHWS = I915_READ(HWS_PGA);
> > >  
> > > @@ -844,8 +842,6 @@ int i915_restore_state(struct drm_device *dev)
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > >  	int i;
> > >  
> > > -	pci_write_config_byte(dev->pdev, LBB, dev_priv->saveLBB);
> > > -
> > >  	/* Hardware status page */
> > >  	I915_WRITE(HWS_PGA, dev_priv->saveHWS);
> > 
> > This may affect older platforms and prevent backlight save/restore?  At
> > least I think that's why we added it in the first place...
> > 
> 
> oh, I just grep it's not used any more, or we plan to add it back?

I thought we just re-introduced legacy mode so we modify the reg
again...  Either way we may need to save/restore it even if the BIOS is
the one that whacks it when we call ACPI.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 3/8] drm/i915: save/restore DSPARB only for chip before gen4

[Zhenyu Wang]

Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_suspend.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index 422981b..7f00f8b 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -603,7 +603,8 @@ void i915_save_display(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	/* Display arbitration control */
-	dev_priv->saveDSPARB = I915_READ(DSPARB);
+	if (dev_priv->info->gen < 4)
+		dev_priv->saveDSPARB = I915_READ(DSPARB);
 
 	/* This is only meaningful in non-KMS mode */
 	/* Don't save them in KMS mode */
@@ -695,7 +696,8 @@ void i915_restore_display(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	/* Display arbitration */
-	I915_WRITE(DSPARB, dev_priv->saveDSPARB);
+	if (dev_priv->info->gen < 4)
+		I915_WRITE(DSPARB, dev_priv->saveDSPARB);
 
 	/* Display port ratios (must be done before clock is set) */
 	if (SUPPORTS_INTEGRATED_DP(dev)) {
@@ -795,9 +797,6 @@ int i915_save_state(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int i;
 
-	/* Hardware status page */
-	dev_priv->saveHWS = I915_READ(HWS_PGA);
-
 	i915_save_display(dev);
 
 	/* Interrupt state */
@@ -842,9 +841,6 @@ int i915_restore_state(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int i;
 
-	/* Hardware status page */
-	I915_WRITE(HWS_PGA, dev_priv->saveHWS);
-
 	i915_restore_display(dev);
 
 	/* Interrupt state */
-- 
1.7.4.1

Re: [PATCH 3/8] drm/i915: save/restore DSPARB only for chip before gen4

[Jesse Barnes]

On Mon, 21 Mar 2011 17:27:14 +0800
Zhenyu Wang <zhenyuw@linux.intel.com> wrote:

> Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_suspend.c |   12 ++++--------
>  1 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> index 422981b..7f00f8b 100644
> --- a/drivers/gpu/drm/i915/i915_suspend.c
> +++ b/drivers/gpu/drm/i915/i915_suspend.c
> @@ -603,7 +603,8 @@ void i915_save_display(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	/* Display arbitration control */
> -	dev_priv->saveDSPARB = I915_READ(DSPARB);
> +	if (dev_priv->info->gen < 4)
> +		dev_priv->saveDSPARB = I915_READ(DSPARB);
>  
>  	/* This is only meaningful in non-KMS mode */
>  	/* Don't save them in KMS mode */
> @@ -695,7 +696,8 @@ void i915_restore_display(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	/* Display arbitration */
> -	I915_WRITE(DSPARB, dev_priv->saveDSPARB);
> +	if (dev_priv->info->gen < 4)
> +		I915_WRITE(DSPARB, dev_priv->saveDSPARB);

Was it Cantiga that started doing automatic FIFO sizing for us?
Checking the docs...  Yeah, only Bearlake-C (G33?) and Cantiga do this
automatically.  So a blanket gen4 check probably isn't right.

>  
>  	/* Display port ratios (must be done before clock is set) */
>  	if (SUPPORTS_INTEGRATED_DP(dev)) {
> @@ -795,9 +797,6 @@ int i915_save_state(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int i;
>  
> -	/* Hardware status page */
> -	dev_priv->saveHWS = I915_READ(HWS_PGA);
> -
>  	i915_save_display(dev);
>  
>  	/* Interrupt state */
> @@ -842,9 +841,6 @@ int i915_restore_state(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int i;
>  
> -	/* Hardware status page */
> -	I915_WRITE(HWS_PGA, dev_priv->saveHWS);
> -
>  	i915_restore_display(dev);
>  
>  	/* Interrupt state */


These hunks are unrelated to DSPARB; assuming we actually want to do
this it should be a separate patch.  Based on earlier reports, it
sounds like we're not properly idling across suspend/resume, in that
we're trying to execute commands at resume before setting up the HWS
again.


-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 3/8] drm/i915: save/restore DSPARB only for chip before gen4

[Zhenyu Wang]

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

On 2011.03.21 10:39:54 -0700, Jesse Barnes wrote:
> On Mon, 21 Mar 2011 17:27:14 +0800
> Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> 
> > Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_suspend.c |   12 ++++--------
> >  1 files changed, 4 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> > index 422981b..7f00f8b 100644
> > --- a/drivers/gpu/drm/i915/i915_suspend.c
> > +++ b/drivers/gpu/drm/i915/i915_suspend.c
> > @@ -603,7 +603,8 @@ void i915_save_display(struct drm_device *dev)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> >  	/* Display arbitration control */
> > -	dev_priv->saveDSPARB = I915_READ(DSPARB);
> > +	if (dev_priv->info->gen < 4)
> > +		dev_priv->saveDSPARB = I915_READ(DSPARB);
> >  
> >  	/* This is only meaningful in non-KMS mode */
> >  	/* Don't save them in KMS mode */
> > @@ -695,7 +696,8 @@ void i915_restore_display(struct drm_device *dev)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> >  	/* Display arbitration */
> > -	I915_WRITE(DSPARB, dev_priv->saveDSPARB);
> > +	if (dev_priv->info->gen < 4)
> > +		I915_WRITE(DSPARB, dev_priv->saveDSPARB);
> 
> Was it Cantiga that started doing automatic FIFO sizing for us?
> Checking the docs...  Yeah, only Bearlake-C (G33?) and Cantiga do this
> automatically.  So a blanket gen4 check probably isn't right.
> 

I just saw we only use it for gen < 4 chips, also including G33 (gen=3)...


> >  
> >  	/* Display port ratios (must be done before clock is set) */
> >  	if (SUPPORTS_INTEGRATED_DP(dev)) {
> > @@ -795,9 +797,6 @@ int i915_save_state(struct drm_device *dev)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	int i;
> >  
> > -	/* Hardware status page */
> > -	dev_priv->saveHWS = I915_READ(HWS_PGA);
> > -
> >  	i915_save_display(dev);
> >  
> >  	/* Interrupt state */
> > @@ -842,9 +841,6 @@ int i915_restore_state(struct drm_device *dev)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	int i;
> >  
> > -	/* Hardware status page */
> > -	I915_WRITE(HWS_PGA, dev_priv->saveHWS);
> > -
> >  	i915_restore_display(dev);
> >  
> >  	/* Interrupt state */
> 
> 
> These hunks are unrelated to DSPARB; assuming we actually want to do
> this it should be a separate patch.  Based on earlier reports, it
> sounds like we're not properly idling across suspend/resume, in that
> we're trying to execute commands at resume before setting up the HWS
> again.
> 

sorry, it's already in Chris's tree, but not in Linus's yet.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 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

[PATCH 4/8] drm/i915: remove CACHE_MODE_0 save/restore

[Zhenyu Wang]

we don't use it anywhere.

Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_suspend.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index 7f00f8b..11e61a3 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -819,9 +819,6 @@ int i915_save_state(struct drm_device *dev)
 	if (IS_GEN6(dev))
 		gen6_disable_rps(dev);
 
-	/* Cache mode state */
-	dev_priv->saveCACHE_MODE_0 = I915_READ(CACHE_MODE_0);
-
 	/* Memory Arbitration state */
 	dev_priv->saveMI_ARB_STATE = I915_READ(MI_ARB_STATE);
 
@@ -867,9 +864,6 @@ int i915_restore_state(struct drm_device *dev)
 	if (IS_GEN6(dev))
 		gen6_enable_rps(dev_priv);
 
-	/* Cache mode state */
-	I915_WRITE (CACHE_MODE_0, dev_priv->saveCACHE_MODE_0 | 0xffff0000);
-
 	/* Memory arbitration state */
 	I915_WRITE (MI_ARB_STATE, dev_priv->saveMI_ARB_STATE | 0xffff0000);
 
-- 
1.7.4.1

Re: [PATCH 4/8] drm/i915: remove CACHE_MODE_0 save/restore

[Chris Wilson]

On Mon, 21 Mar 2011 17:27:15 +0800, Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> we don't use it anywhere.

But we still need to restore the value after resume, right? Or are
guaranteed that it will be reset to the default?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/8] drm/i915: remove CACHE_MODE_0 save/restore

[Jesse Barnes]

On Mon, 21 Mar 2011 17:27:15 +0800
Zhenyu Wang <zhenyuw@linux.intel.com> wrote:

> we don't use it anywhere.
> 
> Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_suspend.c |    6 ------
>  1 files changed, 0 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> index 7f00f8b..11e61a3 100644
> --- a/drivers/gpu/drm/i915/i915_suspend.c
> +++ b/drivers/gpu/drm/i915/i915_suspend.c
> @@ -819,9 +819,6 @@ int i915_save_state(struct drm_device *dev)
>  	if (IS_GEN6(dev))
>  		gen6_disable_rps(dev);
>  
> -	/* Cache mode state */
> -	dev_priv->saveCACHE_MODE_0 = I915_READ(CACHE_MODE_0);
> -
>  	/* Memory Arbitration state */
>  	dev_priv->saveMI_ARB_STATE = I915_READ(MI_ARB_STATE);
>  
> @@ -867,9 +864,6 @@ int i915_restore_state(struct drm_device *dev)
>  	if (IS_GEN6(dev))
>  		gen6_enable_rps(dev_priv);
>  
> -	/* Cache mode state */
> -	I915_WRITE (CACHE_MODE_0, dev_priv->saveCACHE_MODE_0 | 0xffff0000);
> -
>  	/* Memory arbitration state */
>  	I915_WRITE (MI_ARB_STATE, dev_priv->saveMI_ARB_STATE | 0xffff0000);

I'd be fine with this if we actually set CACHE_MODE_0 to a reasonable
value somewhere else.  But right now we rely on the BIOS settings and
for the BIOS to reprogram it at resume, which may or may not be
reliable.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 4/8] drm/i915: remove CACHE_MODE_0 save/restore

[Zhenyu Wang]

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

On 2011.03.21 10:40:51 -0700, Jesse Barnes wrote:
> On Mon, 21 Mar 2011 17:27:15 +0800
> Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> 
> > we don't use it anywhere.
> > 
> > Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_suspend.c |    6 ------
> >  1 files changed, 0 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> > index 7f00f8b..11e61a3 100644
> > --- a/drivers/gpu/drm/i915/i915_suspend.c
> > +++ b/drivers/gpu/drm/i915/i915_suspend.c
> > @@ -819,9 +819,6 @@ int i915_save_state(struct drm_device *dev)
> >  	if (IS_GEN6(dev))
> >  		gen6_disable_rps(dev);
> >  
> > -	/* Cache mode state */
> > -	dev_priv->saveCACHE_MODE_0 = I915_READ(CACHE_MODE_0);
> > -
> >  	/* Memory Arbitration state */
> >  	dev_priv->saveMI_ARB_STATE = I915_READ(MI_ARB_STATE);
> >  
> > @@ -867,9 +864,6 @@ int i915_restore_state(struct drm_device *dev)
> >  	if (IS_GEN6(dev))
> >  		gen6_enable_rps(dev_priv);
> >  
> > -	/* Cache mode state */
> > -	I915_WRITE (CACHE_MODE_0, dev_priv->saveCACHE_MODE_0 | 0xffff0000);
> > -
> >  	/* Memory arbitration state */
> >  	I915_WRITE (MI_ARB_STATE, dev_priv->saveMI_ARB_STATE | 0xffff0000);
> 
> I'd be fine with this if we actually set CACHE_MODE_0 to a reasonable
> value somewhere else.  But right now we rely on the BIOS settings and
> for the BIOS to reprogram it at resume, which may or may not be
> reliable.
> 

I think if we don't touch it in driver we might just keep as it,
but your concern seems reasonable as well... I'll test to see if this
really impacts on suspend issue.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 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

[PATCH 5/8] drm/i915: save/restore MI_ARB_STATE only for gen3

[Zhenyu Wang]

Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_suspend.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index 11e61a3..26387c3 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -820,7 +820,8 @@ int i915_save_state(struct drm_device *dev)
 		gen6_disable_rps(dev);
 
 	/* Memory Arbitration state */
-	dev_priv->saveMI_ARB_STATE = I915_READ(MI_ARB_STATE);
+	if (IS_GEN3(dev))
+		dev_priv->saveMI_ARB_STATE = I915_READ(MI_ARB_STATE);
 
 	/* Scratch space */
 	for (i = 0; i < 16; i++) {
@@ -865,7 +866,8 @@ int i915_restore_state(struct drm_device *dev)
 		gen6_enable_rps(dev_priv);
 
 	/* Memory arbitration state */
-	I915_WRITE (MI_ARB_STATE, dev_priv->saveMI_ARB_STATE | 0xffff0000);
+	if (IS_GEN3(dev))
+		I915_WRITE (MI_ARB_STATE, dev_priv->saveMI_ARB_STATE | 0xffff0000);
 
 	for (i = 0; i < 16; i++) {
 		I915_WRITE(SWF00 + (i << 2), dev_priv->saveSWF0[i]);
-- 
1.7.4.1

Re: [PATCH 5/8] drm/i915: save/restore MI_ARB_STATE only for gen3

[Jesse Barnes]

On Mon, 21 Mar 2011 17:27:16 +0800
Zhenyu Wang <zhenyuw@linux.intel.com> wrote:

> Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_suspend.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> index 11e61a3..26387c3 100644
> --- a/drivers/gpu/drm/i915/i915_suspend.c
> +++ b/drivers/gpu/drm/i915/i915_suspend.c
> @@ -820,7 +820,8 @@ int i915_save_state(struct drm_device *dev)
>  		gen6_disable_rps(dev);
>  
>  	/* Memory Arbitration state */
> -	dev_priv->saveMI_ARB_STATE = I915_READ(MI_ARB_STATE);
> +	if (IS_GEN3(dev))
> +		dev_priv->saveMI_ARB_STATE = I915_READ(MI_ARB_STATE);
>  
>  	/* Scratch space */
>  	for (i = 0; i < 16; i++) {
> @@ -865,7 +866,8 @@ int i915_restore_state(struct drm_device *dev)
>  		gen6_enable_rps(dev_priv);
>  
>  	/* Memory arbitration state */
> -	I915_WRITE (MI_ARB_STATE, dev_priv->saveMI_ARB_STATE | 0xffff0000);
> +	if (IS_GEN3(dev))
> +		I915_WRITE (MI_ARB_STATE, dev_priv->saveMI_ARB_STATE | 0xffff0000);
>  
>  	for (i = 0; i < 16; i++) {
>  		I915_WRITE(SWF00 + (i << 2), dev_priv->saveSWF0[i]);

This register exists on gen4 as well, though as with CACHE_MODE_0, we
should probably be setting it to a good value rather than simply
trusting the BIOS.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 6/8] drm/i915: change force wake order for GT read

[Zhenyu Wang]

Move FORCEWAKE_ACK == 0 wait after setting force wake to 0. This is
another way to set force wake for GT read from spec. And also this
makes RC6 setting order strictly follow spec.

Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index dd8bd5c..a55b71d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -264,10 +264,6 @@ void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
 {
 	int count;
 
-	count = 0;
-	while (count++ < 50 && (I915_READ_NOTRACE(FORCEWAKE_ACK) & 1))
-		udelay(10);
-
 	I915_WRITE_NOTRACE(FORCEWAKE, 1);
 	POSTING_READ(FORCEWAKE);
 
@@ -278,8 +274,15 @@ void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
 
 void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
 {
+	int count;
+
 	I915_WRITE_NOTRACE(FORCEWAKE, 0);
 	POSTING_READ(FORCEWAKE);
+
+	count = 0;
+	while (count++ < 50 && (I915_READ_NOTRACE(FORCEWAKE_ACK) & 1))
+		udelay(10);
+
 }
 
 void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
-- 
1.7.4.1

Re: [PATCH 6/8] drm/i915: change force wake order for GT read

[Chris Wilson]

On Mon, 21 Mar 2011 17:27:17 +0800, Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> Move FORCEWAKE_ACK == 0 wait after setting force wake to 0. This is
> another way to set force wake for GT read from spec. And also this
> makes RC6 setting order strictly follow spec.

The spec does indeed give both methods. So do you have any reason to
change to the slower variant?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 6/8] drm/i915: change force wake order for GT read

[Zhenyu Wang]

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

On 2011.03.21 09:46:20 +0000, Chris Wilson wrote:
> On Mon, 21 Mar 2011 17:27:17 +0800, Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> > Move FORCEWAKE_ACK == 0 wait after setting force wake to 0. This is
> > another way to set force wake for GT read from spec. And also this
> > makes RC6 setting order strictly follow spec.
> 
> The spec does indeed give both methods. So do you have any reason to
> change to the slower variant?

Just mean to follow the doc, it matches the sequence of RC6 enabling steps
from GT PM programming doc, not sure if it's strictly required.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 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 6/8] drm/i915: change force wake order for GT read

[Chris Wilson]

On Tue, 22 Mar 2011 09:27:51 +0800, Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> Just mean to follow the doc, it matches the sequence of RC6 enabling steps
> from GT PM programming doc, not sure if it's strictly required.

But as you point out, the docs do also outline the current method as well.
;-)

Meanwhile a better solution for _gen6_gt_wait_for_fifo() is desperately
sought. It is the new clflush. [Ok, most of that could be mitigated by
improving the ddx...]
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 7/8] drm/i915: move sandybridge RC6 enable in resume after ring initialization

[Zhenyu Wang]

Move RC6 enable after we reset rings for all regines, as RC6 on
Sandybridge would setup ring idle state, so I assume we should have
setup rings already.

Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c     |    4 ++++
 drivers/gpu/drm/i915/i915_suspend.c |    3 ---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a55b71d..b40f3ee 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -384,6 +384,10 @@ static int i915_drm_thaw(struct drm_device *dev)
 
 		if (IS_IRONLAKE_M(dev))
 			ironlake_enable_rc6(dev);
+
+		if (IS_GEN6(dev))
+			gen6_enable_rps(dev_priv);
+
 	}
 
 	intel_opregion_init(dev);
diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index 26387c3..8e6aa8c 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -862,9 +862,6 @@ int i915_restore_state(struct drm_device *dev)
 		intel_init_emon(dev);
 	}
 
-	if (IS_GEN6(dev))
-		gen6_enable_rps(dev_priv);
-
 	/* Memory arbitration state */
 	if (IS_GEN3(dev))
 		I915_WRITE (MI_ARB_STATE, dev_priv->saveMI_ARB_STATE | 0xffff0000);
-- 
1.7.4.1

Re: [PATCH 7/8] drm/i915: move sandybridge RC6 enable in resume after ring initialization

[Jesse Barnes]

On Mon, 21 Mar 2011 17:27:18 +0800
Zhenyu Wang <zhenyuw@linux.intel.com> wrote:

> Move RC6 enable after we reset rings for all regines, as RC6 on
> Sandybridge would setup ring idle state, so I assume we should have
> setup rings already.

So you're seeing the idle value get clobbered?  Otherwise it's it
better to set the ring idle value before actually starting the ring?

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 7/8] drm/i915: move sandybridge RC6 enable in resume after ring initialization

[Zhenyu Wang]

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

On 2011.03.21 10:46:53 -0700, Jesse Barnes wrote:
> On Mon, 21 Mar 2011 17:27:18 +0800
> Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> 
> > Move RC6 enable after we reset rings for all regines, as RC6 on
> > Sandybridge would setup ring idle state, so I assume we should have
> > setup rings already.
> 
> So you're seeing the idle value get clobbered?  Otherwise it's it
> better to set the ring idle value before actually starting the ring?
> 

Don't know, but I can ask if we should do like that or not. I had more
stable test result with this.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 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

[PATCH 8/8] drm/i915: only save/restore VGA clock regs for non-KMS case

[Zhenyu Wang]

For KMS we want to disable VGA anyway, do so by using correct setting
order in i915_disable_vga().

Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |    1 +
 drivers/gpu/drm/i915/i915_suspend.c  |   39 ++++++++++++++++++++--------------
 drivers/gpu/drm/i915/intel_display.c |    2 +-
 3 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index eaee7f1..c5dc79c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1276,6 +1276,7 @@ extern void ironlake_enable_rc6(struct drm_device *dev);
 extern void gen6_set_rps(struct drm_device *dev, u8 val);
 extern void intel_detect_pch (struct drm_device *dev);
 extern int intel_trans_dp_port_sel (struct drm_crtc *crtc);
+extern void i915_disable_vga(struct drm_device *dev);
 
 /* overlay */
 #ifdef CONFIG_DEBUG_FS
diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index 8e6aa8c..07f4fc9 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -680,13 +680,15 @@ void i915_save_display(struct drm_device *dev)
 	}
 
 	/* VGA state */
-	dev_priv->saveVGA0 = I915_READ(VGA0);
-	dev_priv->saveVGA1 = I915_READ(VGA1);
-	dev_priv->saveVGA_PD = I915_READ(VGA_PD);
-	if (HAS_PCH_SPLIT(dev))
-		dev_priv->saveVGACNTRL = I915_READ(CPU_VGACNTRL);
-	else
-		dev_priv->saveVGACNTRL = I915_READ(VGACNTRL);
+	if (!drm_core_check_feature(dev, DRIVER_MODESET)) {
+		dev_priv->saveVGA0 = I915_READ(VGA0);
+		dev_priv->saveVGA1 = I915_READ(VGA1);
+		dev_priv->saveVGA_PD = I915_READ(VGA_PD);
+		if (HAS_PCH_SPLIT(dev))
+			dev_priv->saveVGACNTRL = I915_READ(CPU_VGACNTRL);
+		else
+			dev_priv->saveVGACNTRL = I915_READ(VGACNTRL);
+	}
 
 	i915_save_vga(dev);
 }
@@ -779,17 +781,22 @@ void i915_restore_display(struct drm_device *dev)
 		}
 	}
 	/* VGA state */
-	if (HAS_PCH_SPLIT(dev))
-		I915_WRITE(CPU_VGACNTRL, dev_priv->saveVGACNTRL);
-	else
-		I915_WRITE(VGACNTRL, dev_priv->saveVGACNTRL);
-	I915_WRITE(VGA0, dev_priv->saveVGA0);
-	I915_WRITE(VGA1, dev_priv->saveVGA1);
-	I915_WRITE(VGA_PD, dev_priv->saveVGA_PD);
-	POSTING_READ(VGA_PD);
-	udelay(150);
+	if (!drm_core_check_feature(dev, DRIVER_MODESET)) {
+		if (HAS_PCH_SPLIT(dev))
+			I915_WRITE(CPU_VGACNTRL, dev_priv->saveVGACNTRL);
+		else
+			I915_WRITE(VGACNTRL, dev_priv->saveVGACNTRL);
+		I915_WRITE(VGA0, dev_priv->saveVGA0);
+		I915_WRITE(VGA1, dev_priv->saveVGA1);
+		I915_WRITE(VGA_PD, dev_priv->saveVGA_PD);
+		POSTING_READ(VGA_PD);
+		udelay(150);
+	}
 
 	i915_restore_vga(dev);
+
+	if (drm_core_check_feature(dev, DRIVER_MODESET))
+		i915_disable_vga(dev);
 }
 
 int i915_save_state(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 13b17e0..13d45b8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6815,7 +6815,7 @@ static void intel_init_quirks(struct drm_device *dev)
 }
 
 /* Disable the VGA plane that we never use */
-static void i915_disable_vga(struct drm_device *dev)
+void i915_disable_vga(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u8 sr1;
-- 
1.7.4.1

Re: [PATCH 8/8] drm/i915: only save/restore VGA clock regs for non-KMS case

[Chris Wilson]

On Mon, 21 Mar 2011 17:27:19 +0800, Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> For KMS we want to disable VGA anyway, do so by using correct setting
> order in i915_disable_vga().

Whilst we are here... Can anybody explain the distinction between those
VGA registers and i915_save_vga()? Might this be a good point to merge
those code blocks and add some comments?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 8/8] drm/i915: only save/restore VGA clock regs for non-KMS case

[Jesse Barnes]

On Mon, 21 Mar 2011 09:49:23 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Mon, 21 Mar 2011 17:27:19 +0800, Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> > For KMS we want to disable VGA anyway, do so by using correct setting
> > order in i915_disable_vga().
> 
> Whilst we are here... Can anybody explain the distinction between those
> VGA registers and i915_save_vga()? Might this be a good point to merge
> those code blocks and add some comments?

And/or just move this block into the modeset_regs function, since it
already checks for modesetting.

Not sure if the divisor regs are aliases for the CRTC regs or not; the
VGA plane control reg is definitely separate though.

-- 
Jesse Barnes, Intel Open Source Technology Center