[DMC_BUGFIX_SKL_V2,3/5] drm/i915/skl: Making DC6 entry is the last call in suspend flow.

Commit Message

Manna, Animesh Aug. 25, 2015, 8:06 p.m. UTC

Mmio register access after dc6/dc5 entry is not allowed when
DC6 power states are enabled according to bspec (bspec-id 0527),
so enabling dc6 as the last call in suspend flow.

v1: Initial version.

v2: Based on review comment from Daniel,
- created a seperate patch for csr uninitialization set call.

Cc: Daniel Vetter <[e-mail]>
Cc: Damien Lespiau <[e-mail]>
Cc: Imre Deak <[e-mail]>
Cc: Sunil Kamath <[e-mail]>
Signed-off-by: Animesh Manna <[e-mail]>
Signed-off-by: Vathsala Nagaraju <[e-mail]>
Signed-off-by: Rajneesh Bhardwaj <[e-mail]>
---
 drivers/gpu/drm/i915/i915_drv.c         | 13 +++++++++++++
 drivers/gpu/drm/i915/intel_drv.h        |  2 ++
 drivers/gpu/drm/i915/intel_runtime_pm.c | 19 +++++++------------
 3 files changed, 22 insertions(+), 12 deletions(-)

Comments

Kamath, Sunil Sept. 7, 2015, 11:06 a.m. UTC | #1

On Wednesday 26 August 2015 01:36 AM, Animesh Manna wrote:
> Mmio register access after dc6/dc5 entry is not allowed when
> DC6 power states are enabled according to bspec (bspec-id 0527),
> so enabling dc6 as the last call in suspend flow.
>
> v1: Initial version.
>
> v2: Based on review comment from Daniel,
> - created a seperate patch for csr uninitialization set call.
>
> Cc: Daniel Vetter <[e-mail]>
> Cc: Damien Lespiau <[e-mail]>
> Cc: Imre Deak <[e-mail]>
> Cc: Sunil Kamath <[e-mail]>
> Signed-off-by: Animesh Manna <[e-mail]>
> Signed-off-by: Vathsala Nagaraju <[e-mail]>
> Signed-off-by: Rajneesh Bhardwaj <[e-mail]>
> ---
>   drivers/gpu/drm/i915/i915_drv.c         | 13 +++++++++++++
>   drivers/gpu/drm/i915/intel_drv.h        |  2 ++
>   drivers/gpu/drm/i915/intel_runtime_pm.c | 19 +++++++------------
>   3 files changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 478101c..fa66162 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1013,10 +1013,20 @@ static int i915_pm_resume(struct device *dev)
>   
>   static int skl_suspend_complete(struct drm_i915_private *dev_priv)
>   {
> +	enum csr_state state;
>   	/* Enabling DC6 is not a hard requirement to enter runtime D3 */
>   
>   	skl_uninit_cdclk(dev_priv);
>   
> +	/* TODO: wait for a completion event or
> +	 * similar here instead of busy
> +	 * waiting using wait_for function.
> +	 */
> +	wait_for((state = intel_csr_load_status_get(dev_priv)) !=
> +			FW_UNINITIALIZED, 1000);
> +	if (state == FW_LOADED)
> +		skl_enable_dc6(dev_priv);
> +
>   	return 0;
>   }
>   
> @@ -1063,6 +1073,9 @@ static int skl_resume_prepare(struct drm_i915_private *dev_priv)
>   {
>   	struct drm_device *dev = dev_priv->dev;
>   
> +	if (intel_csr_load_status_get(dev_priv) == FW_LOADED)
> +		skl_disable_dc6(dev_priv);
> +
>   	skl_init_cdclk(dev_priv);
>   	intel_csr_load_program(dev);
>   
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 81b7d77..9cb7d4e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1118,6 +1118,8 @@ void bxt_enable_dc9(struct drm_i915_private *dev_priv);
>   void bxt_disable_dc9(struct drm_i915_private *dev_priv);
>   void skl_init_cdclk(struct drm_i915_private *dev_priv);
>   void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
> +void skl_enable_dc6(struct drm_i915_private *dev_priv);
> +void skl_disable_dc6(struct drm_i915_private *dev_priv);
>   void intel_dp_get_m_n(struct intel_crtc *crtc,
>   		      struct intel_crtc_state *pipe_config);
>   void intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set m_n);
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 821644d..23a3aa3 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -548,7 +548,7 @@ static void assert_can_disable_dc6(struct drm_i915_private *dev_priv)
>   		"DC6 already programmed to be disabled.\n");
>   }
>   
> -static void skl_enable_dc6(struct drm_i915_private *dev_priv)
> +void skl_enable_dc6(struct drm_i915_private *dev_priv)
>   {
>   	uint32_t val;
>   
> @@ -565,7 +565,7 @@ static void skl_enable_dc6(struct drm_i915_private *dev_priv)
>   	POSTING_READ(DC_STATE_EN);
>   }
>   
> -static void skl_disable_dc6(struct drm_i915_private *dev_priv)
> +void skl_disable_dc6(struct drm_i915_private *dev_priv)
>   {
>   	uint32_t val;
>   
> @@ -626,10 +626,10 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>   				!I915_READ(HSW_PWR_WELL_BIOS),
>   				"Invalid for power well status to be enabled, unless done by the BIOS, \
>   				when request is to disable!\n");
> -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> -				power_well->data == SKL_DISP_PW_2) {
> +			if (power_well->data == SKL_DISP_PW_2) {
> +				if (GEN9_ENABLE_DC5(dev))
> +					gen9_disable_dc5(dev_priv);
>   				if (SKL_ENABLE_DC6(dev)) {
> -					skl_disable_dc6(dev_priv);
>   					/*
>   					 * DDI buffer programming unnecessary during driver-load/resume
>   					 * as it's already done during modeset initialization then.
> @@ -637,8 +637,6 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>   					 */
>   					if (!dev_priv->power_domains.initializing)
>   						intel_prepare_ddi(dev);
> -				} else {
> -					gen9_disable_dc5(dev_priv);
>   				}
>   			}
>   			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
> @@ -658,7 +656,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>   			POSTING_READ(HSW_PWR_WELL_DRIVER);
>   			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
>   
> -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> +			if (GEN9_ENABLE_DC5(dev) &&
>   				power_well->data == SKL_DISP_PW_2) {
>   				enum csr_state state;
>   				/* TODO: wait for a completion event or
> @@ -671,10 +669,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>   					DRM_ERROR("CSR firmware not ready (%d)\n",
>   							state);
>   				else
> -					if (SKL_ENABLE_DC6(dev))
> -						skl_enable_dc6(dev_priv);
> -					else
> -						gen9_enable_dc5(dev_priv);
> +					gen9_enable_dc5(dev_priv);
>   			}
>   		}
>   	}

Valid fix and patch is ready for merge now.

Reviewed-by: A.Sunil Kamath <[e-mail]> 
<mailto:[e-mail]>

Daniel Vetter Sept. 28, 2015, 7:21 a.m. UTC | #2

On Wed, Aug 26, 2015 at 01:36:07AM +0530, Animesh Manna wrote:
> Mmio register access after dc6/dc5 entry is not allowed when
> DC6 power states are enabled according to bspec (bspec-id 0527),
> so enabling dc6 as the last call in suspend flow.

We unconditionaly grab a power well reference for everything in our
suspend/resume functions, which means dc6/5 should be prevented for long
enough.

Also since dc6/5 are part of the power well framework you can't just
enable/disable them directly, instead you need to get/put the
corresponding power wells.

Finally this patch doesn't just do that, but also frobs around a lot in
set power well code itself, and I have no idea what it does there and why.
It does smell a bit like you're just breaking dc6 for runtime pm though,
which wouldn't be good.

Anyway I decided to just merge this since this patch series has been
floating around since forever, but then the patch didn't apply cleanly and
so dropped it.
-Daniel

> 
> v1: Initial version.
> 
> v2: Based on review comment from Daniel,
> - created a seperate patch for csr uninitialization set call.
> 
> Cc: Daniel Vetter <[e-mail]>
> Cc: Damien Lespiau <[e-mail]>
> Cc: Imre Deak <[e-mail]>
> Cc: Sunil Kamath <[e-mail]>
> Signed-off-by: Animesh Manna <[e-mail]>
> Signed-off-by: Vathsala Nagaraju <[e-mail]>
> Signed-off-by: Rajneesh Bhardwaj <[e-mail]>
> ---
>  drivers/gpu/drm/i915/i915_drv.c         | 13 +++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h        |  2 ++
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 19 +++++++------------
>  3 files changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 478101c..fa66162 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1013,10 +1013,20 @@ static int i915_pm_resume(struct device *dev)
>  
>  static int skl_suspend_complete(struct drm_i915_private *dev_priv)
>  {
> +	enum csr_state state;
>  	/* Enabling DC6 is not a hard requirement to enter runtime D3 */
>  
>  	skl_uninit_cdclk(dev_priv);
>  
> +	/* TODO: wait for a completion event or
> +	 * similar here instead of busy
> +	 * waiting using wait_for function.
> +	 */
> +	wait_for((state = intel_csr_load_status_get(dev_priv)) !=
> +			FW_UNINITIALIZED, 1000);
> +	if (state == FW_LOADED)
> +		skl_enable_dc6(dev_priv);
> +
>  	return 0;
>  }
>  
> @@ -1063,6 +1073,9 @@ static int skl_resume_prepare(struct drm_i915_private *dev_priv)
>  {
>  	struct drm_device *dev = dev_priv->dev;
>  
> +	if (intel_csr_load_status_get(dev_priv) == FW_LOADED)
> +		skl_disable_dc6(dev_priv);
> +
>  	skl_init_cdclk(dev_priv);
>  	intel_csr_load_program(dev);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 81b7d77..9cb7d4e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1118,6 +1118,8 @@ void bxt_enable_dc9(struct drm_i915_private *dev_priv);
>  void bxt_disable_dc9(struct drm_i915_private *dev_priv);
>  void skl_init_cdclk(struct drm_i915_private *dev_priv);
>  void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
> +void skl_enable_dc6(struct drm_i915_private *dev_priv);
> +void skl_disable_dc6(struct drm_i915_private *dev_priv);
>  void intel_dp_get_m_n(struct intel_crtc *crtc,
>  		      struct intel_crtc_state *pipe_config);
>  void intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set m_n);
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 821644d..23a3aa3 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -548,7 +548,7 @@ static void assert_can_disable_dc6(struct drm_i915_private *dev_priv)
>  		"DC6 already programmed to be disabled.\n");
>  }
>  
> -static void skl_enable_dc6(struct drm_i915_private *dev_priv)
> +void skl_enable_dc6(struct drm_i915_private *dev_priv)
>  {
>  	uint32_t val;
>  
> @@ -565,7 +565,7 @@ static void skl_enable_dc6(struct drm_i915_private *dev_priv)
>  	POSTING_READ(DC_STATE_EN);
>  }
>  
> -static void skl_disable_dc6(struct drm_i915_private *dev_priv)
> +void skl_disable_dc6(struct drm_i915_private *dev_priv)
>  {
>  	uint32_t val;
>  
> @@ -626,10 +626,10 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  				!I915_READ(HSW_PWR_WELL_BIOS),
>  				"Invalid for power well status to be enabled, unless done by the BIOS, \
>  				when request is to disable!\n");
> -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> -				power_well->data == SKL_DISP_PW_2) {
> +			if (power_well->data == SKL_DISP_PW_2) {
> +				if (GEN9_ENABLE_DC5(dev))
> +					gen9_disable_dc5(dev_priv);
>  				if (SKL_ENABLE_DC6(dev)) {
> -					skl_disable_dc6(dev_priv);
>  					/*
>  					 * DDI buffer programming unnecessary during driver-load/resume
>  					 * as it's already done during modeset initialization then.
> @@ -637,8 +637,6 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  					 */
>  					if (!dev_priv->power_domains.initializing)
>  						intel_prepare_ddi(dev);
> -				} else {
> -					gen9_disable_dc5(dev_priv);
>  				}
>  			}
>  			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
> @@ -658,7 +656,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  			POSTING_READ(HSW_PWR_WELL_DRIVER);
>  			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
>  
> -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> +			if (GEN9_ENABLE_DC5(dev) &&
>  				power_well->data == SKL_DISP_PW_2) {
>  				enum csr_state state;
>  				/* TODO: wait for a completion event or
> @@ -671,10 +669,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  					DRM_ERROR("CSR firmware not ready (%d)\n",
>  							state);
>  				else
> -					if (SKL_ENABLE_DC6(dev))
> -						skl_enable_dc6(dev_priv);
> -					else
> -						gen9_enable_dc5(dev_priv);
> +					gen9_enable_dc5(dev_priv);
>  			}
>  		}
>  	}
> -- 
> 2.0.2
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Hindman, Gavin Sept. 28, 2015, 6:49 p.m. UTC | #3

You believe suspend w/ DC6 should work due to the unconditional power-well reference, or you don't know and just couldn't merge since the patch didn’t apply?

Gavin Hindman
Senior Program Manager
SSG/OTC – Open Source Technology Center

-----Original Message-----
From: Intel-gfx [mailto:[e-mail]] On Behalf Of Daniel Vetter

Sent: Monday, September 28, 2015 12:22 AM
To: Manna, Animesh <[e-mail]>
Cc: Bhardwaj, Rajneesh <[e-mail]>; [e-mail]; Vetter, Daniel <[e-mail]>
Subject: Re: [Intel-gfx] [DMC_BUGFIX_SKL_V2 3/5] drm/i915/skl: Making DC6 entry is the last call in suspend flow.

On Wed, Aug 26, 2015 at 01:36:07AM +0530, Animesh Manna wrote:
> Mmio register access after dc6/dc5 entry is not allowed when

> DC6 power states are enabled according to bspec (bspec-id 0527), so 

> enabling dc6 as the last call in suspend flow.


We unconditionaly grab a power well reference for everything in our suspend/resume functions, which means dc6/5 should be prevented for long enough.

Also since dc6/5 are part of the power well framework you can't just enable/disable them directly, instead you need to get/put the corresponding power wells.

Finally this patch doesn't just do that, but also frobs around a lot in set power well code itself, and I have no idea what it does there and why.
It does smell a bit like you're just breaking dc6 for runtime pm though, which wouldn't be good.

Anyway I decided to just merge this since this patch series has been floating around since forever, but then the patch didn't apply cleanly and so dropped it.
-Daniel

> 

> v1: Initial version.

> 

> v2: Based on review comment from Daniel,

> - created a seperate patch for csr uninitialization set call.

> 

> Cc: Daniel Vetter <[e-mail]>

> Cc: Damien Lespiau <[e-mail]>

> Cc: Imre Deak <[e-mail]>

> Cc: Sunil Kamath <[e-mail]>

> Signed-off-by: Animesh Manna <[e-mail]>

> Signed-off-by: Vathsala Nagaraju <[e-mail]>

> Signed-off-by: Rajneesh Bhardwaj <[e-mail]>

> ---

>  drivers/gpu/drm/i915/i915_drv.c         | 13 +++++++++++++

>  drivers/gpu/drm/i915/intel_drv.h        |  2 ++

>  drivers/gpu/drm/i915/intel_runtime_pm.c | 19 +++++++------------

>  3 files changed, 22 insertions(+), 12 deletions(-)

> 

> diff --git a/drivers/gpu/drm/i915/i915_drv.c 

> b/drivers/gpu/drm/i915/i915_drv.c index 478101c..fa66162 100644

> --- a/drivers/gpu/drm/i915/i915_drv.c

> +++ b/drivers/gpu/drm/i915/i915_drv.c

> @@ -1013,10 +1013,20 @@ static int i915_pm_resume(struct device *dev)

>  

>  static int skl_suspend_complete(struct drm_i915_private *dev_priv)  {

> +	enum csr_state state;

>  	/* Enabling DC6 is not a hard requirement to enter runtime D3 */

>  

>  	skl_uninit_cdclk(dev_priv);

>  

> +	/* TODO: wait for a completion event or

> +	 * similar here instead of busy

> +	 * waiting using wait_for function.

> +	 */

> +	wait_for((state = intel_csr_load_status_get(dev_priv)) !=

> +			FW_UNINITIALIZED, 1000);

> +	if (state == FW_LOADED)

> +		skl_enable_dc6(dev_priv);

> +

>  	return 0;

>  }

>  

> @@ -1063,6 +1073,9 @@ static int skl_resume_prepare(struct 

> drm_i915_private *dev_priv)  {

>  	struct drm_device *dev = dev_priv->dev;

>  

> +	if (intel_csr_load_status_get(dev_priv) == FW_LOADED)

> +		skl_disable_dc6(dev_priv);

> +

>  	skl_init_cdclk(dev_priv);

>  	intel_csr_load_program(dev);

>  

> diff --git a/drivers/gpu/drm/i915/intel_drv.h 

> b/drivers/gpu/drm/i915/intel_drv.h

> index 81b7d77..9cb7d4e 100644

> --- a/drivers/gpu/drm/i915/intel_drv.h

> +++ b/drivers/gpu/drm/i915/intel_drv.h

> @@ -1118,6 +1118,8 @@ void bxt_enable_dc9(struct drm_i915_private 

> *dev_priv);  void bxt_disable_dc9(struct drm_i915_private *dev_priv);  

> void skl_init_cdclk(struct drm_i915_private *dev_priv);  void 

> skl_uninit_cdclk(struct drm_i915_private *dev_priv);

> +void skl_enable_dc6(struct drm_i915_private *dev_priv); void 

> +skl_disable_dc6(struct drm_i915_private *dev_priv);

>  void intel_dp_get_m_n(struct intel_crtc *crtc,

>  		      struct intel_crtc_state *pipe_config);  void 

> intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set m_n); diff 

> --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 

> b/drivers/gpu/drm/i915/intel_runtime_pm.c

> index 821644d..23a3aa3 100644

> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c

> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c

> @@ -548,7 +548,7 @@ static void assert_can_disable_dc6(struct drm_i915_private *dev_priv)

>  		"DC6 already programmed to be disabled.\n");  }

>  

> -static void skl_enable_dc6(struct drm_i915_private *dev_priv)

> +void skl_enable_dc6(struct drm_i915_private *dev_priv)

>  {

>  	uint32_t val;

>  

> @@ -565,7 +565,7 @@ static void skl_enable_dc6(struct drm_i915_private *dev_priv)

>  	POSTING_READ(DC_STATE_EN);

>  }

>  

> -static void skl_disable_dc6(struct drm_i915_private *dev_priv)

> +void skl_disable_dc6(struct drm_i915_private *dev_priv)

>  {

>  	uint32_t val;

>  

> @@ -626,10 +626,10 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,

>  				!I915_READ(HSW_PWR_WELL_BIOS),

>  				"Invalid for power well status to be enabled, unless done by the BIOS, \

>  				when request is to disable!\n");

> -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&

> -				power_well->data == SKL_DISP_PW_2) {

> +			if (power_well->data == SKL_DISP_PW_2) {

> +				if (GEN9_ENABLE_DC5(dev))

> +					gen9_disable_dc5(dev_priv);

>  				if (SKL_ENABLE_DC6(dev)) {

> -					skl_disable_dc6(dev_priv);

>  					/*

>  					 * DDI buffer programming unnecessary during driver-load/resume

>  					 * as it's already done during modeset initialization then.

> @@ -637,8 +637,6 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,

>  					 */

>  					if (!dev_priv->power_domains.initializing)

>  						intel_prepare_ddi(dev);

> -				} else {

> -					gen9_disable_dc5(dev_priv);

>  				}

>  			}

>  			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask); @@ -658,7 +656,7 

> @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,

>  			POSTING_READ(HSW_PWR_WELL_DRIVER);

>  			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);

>  

> -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&

> +			if (GEN9_ENABLE_DC5(dev) &&

>  				power_well->data == SKL_DISP_PW_2) {

>  				enum csr_state state;

>  				/* TODO: wait for a completion event or @@ -671,10 +669,7 @@ 

> static void skl_set_power_well(struct drm_i915_private *dev_priv,

>  					DRM_ERROR("CSR firmware not ready (%d)\n",

>  							state);

>  				else

> -					if (SKL_ENABLE_DC6(dev))

> -						skl_enable_dc6(dev_priv);

> -					else

> -						gen9_enable_dc5(dev_priv);

> +					gen9_enable_dc5(dev_priv);

>  			}

>  		}

>  	}

> --

> 2.0.2

> 

> _______________________________________________

> Intel-gfx mailing list

> [e-mail]

> http://lists.freedesktop.org/mailman/listinfo/intel-gfx


--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
[e-mail]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Manna, Animesh Sept. 29, 2015, 5:38 a.m. UTC | #4

On 09/28/2015 12:51 PM, Daniel Vetter wrote:
> On Wed, Aug 26, 2015 at 01:36:07AM +0530, Animesh Manna wrote:
>> Mmio register access after dc6/dc5 entry is not allowed when
>> DC6 power states are enabled according to bspec (bspec-id 0527),
>> so enabling dc6 as the last call in suspend flow.
> We unconditionaly grab a power well reference for everything in our
> suspend/resume functions, which means dc6/5 should be prevented for long
> enough.
>
> Also since dc6/5 are part of the power well framework you can't just
> enable/disable them directly, instead you need to get/put the
> corresponding power wells.
>
> Finally this patch doesn't just do that, but also frobs around a lot in
> set power well code itself, and I have no idea what it does there and why.
> It does smell a bit like you're just breaking dc6 for runtime pm though,
> which wouldn't be good.
>
> Anyway I decided to just merge this since this patch series has been
> floating around since forever, but then the patch didn't apply cleanly and
> so dropped it.

I mentioned in my commit message that we have a h/w workaround.
"Mmio register access after dc6/dc5 entry is not allowed when
DC6 power states are enabled"

This patch is mandatory to solve the pc10 entry issue for skylake.
Planning to send again after rebase on top of tree.

-Animesh

> -Daniel
>
>> v1: Initial version.
>>
>> v2: Based on review comment from Daniel,
>> - created a seperate patch for csr uninitialization set call.
>>
>> Cc: Daniel Vetter <[e-mail]>
>> Cc: Damien Lespiau <[e-mail]>
>> Cc: Imre Deak <[e-mail]>
>> Cc: Sunil Kamath <[e-mail]>
>> Signed-off-by: Animesh Manna <[e-mail]>
>> Signed-off-by: Vathsala Nagaraju <[e-mail]>
>> Signed-off-by: Rajneesh Bhardwaj <[e-mail]>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.c         | 13 +++++++++++++
>>   drivers/gpu/drm/i915/intel_drv.h        |  2 ++
>>   drivers/gpu/drm/i915/intel_runtime_pm.c | 19 +++++++------------
>>   3 files changed, 22 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 478101c..fa66162 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -1013,10 +1013,20 @@ static int i915_pm_resume(struct device *dev)
>>   
>>   static int skl_suspend_complete(struct drm_i915_private *dev_priv)
>>   {
>> +	enum csr_state state;
>>   	/* Enabling DC6 is not a hard requirement to enter runtime D3 */
>>   
>>   	skl_uninit_cdclk(dev_priv);
>>   
>> +	/* TODO: wait for a completion event or
>> +	 * similar here instead of busy
>> +	 * waiting using wait_for function.
>> +	 */
>> +	wait_for((state = intel_csr_load_status_get(dev_priv)) !=
>> +			FW_UNINITIALIZED, 1000);
>> +	if (state == FW_LOADED)
>> +		skl_enable_dc6(dev_priv);
>> +
>>   	return 0;
>>   }
>>   
>> @@ -1063,6 +1073,9 @@ static int skl_resume_prepare(struct drm_i915_private *dev_priv)
>>   {
>>   	struct drm_device *dev = dev_priv->dev;
>>   
>> +	if (intel_csr_load_status_get(dev_priv) == FW_LOADED)
>> +		skl_disable_dc6(dev_priv);
>> +
>>   	skl_init_cdclk(dev_priv);
>>   	intel_csr_load_program(dev);
>>   
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 81b7d77..9cb7d4e 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1118,6 +1118,8 @@ void bxt_enable_dc9(struct drm_i915_private *dev_priv);
>>   void bxt_disable_dc9(struct drm_i915_private *dev_priv);
>>   void skl_init_cdclk(struct drm_i915_private *dev_priv);
>>   void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
>> +void skl_enable_dc6(struct drm_i915_private *dev_priv);
>> +void skl_disable_dc6(struct drm_i915_private *dev_priv);
>>   void intel_dp_get_m_n(struct intel_crtc *crtc,
>>   		      struct intel_crtc_state *pipe_config);
>>   void intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set m_n);
>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> index 821644d..23a3aa3 100644
>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> @@ -548,7 +548,7 @@ static void assert_can_disable_dc6(struct drm_i915_private *dev_priv)
>>   		"DC6 already programmed to be disabled.\n");
>>   }
>>   
>> -static void skl_enable_dc6(struct drm_i915_private *dev_priv)
>> +void skl_enable_dc6(struct drm_i915_private *dev_priv)
>>   {
>>   	uint32_t val;
>>   
>> @@ -565,7 +565,7 @@ static void skl_enable_dc6(struct drm_i915_private *dev_priv)
>>   	POSTING_READ(DC_STATE_EN);
>>   }
>>   
>> -static void skl_disable_dc6(struct drm_i915_private *dev_priv)
>> +void skl_disable_dc6(struct drm_i915_private *dev_priv)
>>   {
>>   	uint32_t val;
>>   
>> @@ -626,10 +626,10 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>>   				!I915_READ(HSW_PWR_WELL_BIOS),
>>   				"Invalid for power well status to be enabled, unless done by the BIOS, \
>>   				when request is to disable!\n");
>> -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
>> -				power_well->data == SKL_DISP_PW_2) {
>> +			if (power_well->data == SKL_DISP_PW_2) {
>> +				if (GEN9_ENABLE_DC5(dev))
>> +					gen9_disable_dc5(dev_priv);
>>   				if (SKL_ENABLE_DC6(dev)) {
>> -					skl_disable_dc6(dev_priv);
>>   					/*
>>   					 * DDI buffer programming unnecessary during driver-load/resume
>>   					 * as it's already done during modeset initialization then.
>> @@ -637,8 +637,6 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>>   					 */
>>   					if (!dev_priv->power_domains.initializing)
>>   						intel_prepare_ddi(dev);
>> -				} else {
>> -					gen9_disable_dc5(dev_priv);
>>   				}
>>   			}
>>   			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
>> @@ -658,7 +656,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>>   			POSTING_READ(HSW_PWR_WELL_DRIVER);
>>   			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
>>   
>> -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
>> +			if (GEN9_ENABLE_DC5(dev) &&
>>   				power_well->data == SKL_DISP_PW_2) {
>>   				enum csr_state state;
>>   				/* TODO: wait for a completion event or
>> @@ -671,10 +669,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>>   					DRM_ERROR("CSR firmware not ready (%d)\n",
>>   							state);
>>   				else
>> -					if (SKL_ENABLE_DC6(dev))
>> -						skl_enable_dc6(dev_priv);
>> -					else
>> -						gen9_enable_dc5(dev_priv);
>> +					gen9_enable_dc5(dev_priv);
>>   			}
>>   		}
>>   	}
>> -- 
>> 2.0.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> [e-mail]
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Vetter Sept. 29, 2015, 9:01 a.m. UTC | #5

On Tue, Sep 29, 2015 at 11:08:25AM +0530, Animesh Manna wrote:
> 
> 
> On 09/28/2015 12:51 PM, Daniel Vetter wrote:
> >On Wed, Aug 26, 2015 at 01:36:07AM +0530, Animesh Manna wrote:
> >>Mmio register access after dc6/dc5 entry is not allowed when
> >>DC6 power states are enabled according to bspec (bspec-id 0527),
> >>so enabling dc6 as the last call in suspend flow.
> >We unconditionaly grab a power well reference for everything in our
> >suspend/resume functions, which means dc6/5 should be prevented for long
> >enough.
> >
> >Also since dc6/5 are part of the power well framework you can't just
> >enable/disable them directly, instead you need to get/put the
> >corresponding power wells.
> >
> >Finally this patch doesn't just do that, but also frobs around a lot in
> >set power well code itself, and I have no idea what it does there and why.
> >It does smell a bit like you're just breaking dc6 for runtime pm though,
> >which wouldn't be good.
> >
> >Anyway I decided to just merge this since this patch series has been
> >floating around since forever, but then the patch didn't apply cleanly and
> >so dropped it.
> 
> I mentioned in my commit message that we have a h/w workaround.
> "Mmio register access after dc6/dc5 entry is not allowed when
> DC6 power states are enabled"
> 
> This patch is mandatory to solve the pc10 entry issue for skylake.
> Planning to send again after rebase on top of tree.

The problem isn't that the patch didn't apply cleanly, but that it seems
to have fundamental issues:
- We already grab power well references around suspend/resume code, see
  the call to intel_power_domains_init_hw and the calls to
  intel_display_set_init_power. That code is supposed to ensure that while
  suspend/resume is going on _all_ power wells are on (and hence dc6/5 are
  forbidden). It's a bit a big hammer, but we already have the code to do
  exactly what you're trying to do here. It could be though that for skl
  it's in the wrong order (but then the commit message must state very
  clearly what's the exact depency, the gpu is assembled from a pile of
  various different blocks which are all mostly independent).

- Your patch directly calls skl_enable/disable_dc6 instead of going
  through the power well abstraction. Breaking these abstraction needs a
  really good reason, adequate assurance that the refcounts are ok and
  nothing breaks (using WARN_ON) and an explanation why the refcount
  interface for display power management doesn't work.

- You also do changes to the power well code itself, which looks like it
  might break dc6 for runtime pm (by only going into dc5 at most). That
  needs to be a separate patch or have a much better explanation.

We probably need to set up a meeting to resolve this sooner than the few
months it took to figure out the resume issue ...

Thanks, Daniel
> >>
> >>v2: Based on review comment from Daniel,
> >>- created a seperate patch for csr uninitialization set call.
> >>
> >>Cc: Daniel Vetter <[e-mail]>
> >>Cc: Damien Lespiau <[e-mail]>
> >>Cc: Imre Deak <[e-mail]>
> >>Cc: Sunil Kamath <[e-mail]>
> >>Signed-off-by: Animesh Manna <[e-mail]>
> >>Signed-off-by: Vathsala Nagaraju <[e-mail]>
> >>Signed-off-by: Rajneesh Bhardwaj <[e-mail]>
> >>---
> >>  drivers/gpu/drm/i915/i915_drv.c         | 13 +++++++++++++
> >>  drivers/gpu/drm/i915/intel_drv.h        |  2 ++
> >>  drivers/gpu/drm/i915/intel_runtime_pm.c | 19 +++++++------------
> >>  3 files changed, 22 insertions(+), 12 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >>index 478101c..fa66162 100644
> >>--- a/drivers/gpu/drm/i915/i915_drv.c
> >>+++ b/drivers/gpu/drm/i915/i915_drv.c
> >>@@ -1013,10 +1013,20 @@ static int i915_pm_resume(struct device *dev)
> >>  static int skl_suspend_complete(struct drm_i915_private *dev_priv)
> >>  {
> >>+	enum csr_state state;
> >>  	/* Enabling DC6 is not a hard requirement to enter runtime D3 */
> >>  	skl_uninit_cdclk(dev_priv);
> >>+	/* TODO: wait for a completion event or
> >>+	 * similar here instead of busy
> >>+	 * waiting using wait_for function.
> >>+	 */
> >>+	wait_for((state = intel_csr_load_status_get(dev_priv)) !=
> >>+			FW_UNINITIALIZED, 1000);
> >>+	if (state == FW_LOADED)
> >>+		skl_enable_dc6(dev_priv);
> >>+
> >>  	return 0;
> >>  }
> >>@@ -1063,6 +1073,9 @@ static int skl_resume_prepare(struct drm_i915_private *dev_priv)
> >>  {
> >>  	struct drm_device *dev = dev_priv->dev;
> >>+	if (intel_csr_load_status_get(dev_priv) == FW_LOADED)
> >>+		skl_disable_dc6(dev_priv);
> >>+
> >>  	skl_init_cdclk(dev_priv);
> >>  	intel_csr_load_program(dev);
> >>diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >>index 81b7d77..9cb7d4e 100644
> >>--- a/drivers/gpu/drm/i915/intel_drv.h
> >>+++ b/drivers/gpu/drm/i915/intel_drv.h
> >>@@ -1118,6 +1118,8 @@ void bxt_enable_dc9(struct drm_i915_private *dev_priv);
> >>  void bxt_disable_dc9(struct drm_i915_private *dev_priv);
> >>  void skl_init_cdclk(struct drm_i915_private *dev_priv);
> >>  void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
> >>+void skl_enable_dc6(struct drm_i915_private *dev_priv);
> >>+void skl_disable_dc6(struct drm_i915_private *dev_priv);
> >>  void intel_dp_get_m_n(struct intel_crtc *crtc,
> >>  		      struct intel_crtc_state *pipe_config);
> >>  void intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set m_n);
> >>diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> >>index 821644d..23a3aa3 100644
> >>--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> >>+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> >>@@ -548,7 +548,7 @@ static void assert_can_disable_dc6(struct drm_i915_private *dev_priv)
> >>  		"DC6 already programmed to be disabled.\n");
> >>  }
> >>-static void skl_enable_dc6(struct drm_i915_private *dev_priv)
> >>+void skl_enable_dc6(struct drm_i915_private *dev_priv)
> >>  {
> >>  	uint32_t val;
> >>@@ -565,7 +565,7 @@ static void skl_enable_dc6(struct drm_i915_private *dev_priv)
> >>  	POSTING_READ(DC_STATE_EN);
> >>  }
> >>-static void skl_disable_dc6(struct drm_i915_private *dev_priv)
> >>+void skl_disable_dc6(struct drm_i915_private *dev_priv)
> >>  {
> >>  	uint32_t val;
> >>@@ -626,10 +626,10 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> >>  				!I915_READ(HSW_PWR_WELL_BIOS),
> >>  				"Invalid for power well status to be enabled, unless done by the BIOS, \
> >>  				when request is to disable!\n");
> >>-			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> >>-				power_well->data == SKL_DISP_PW_2) {
> >>+			if (power_well->data == SKL_DISP_PW_2) {
> >>+				if (GEN9_ENABLE_DC5(dev))
> >>+					gen9_disable_dc5(dev_priv);
> >>  				if (SKL_ENABLE_DC6(dev)) {
> >>-					skl_disable_dc6(dev_priv);
> >>  					/*
> >>  					 * DDI buffer programming unnecessary during driver-load/resume
> >>  					 * as it's already done during modeset initialization then.
> >>@@ -637,8 +637,6 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> >>  					 */
> >>  					if (!dev_priv->power_domains.initializing)
> >>  						intel_prepare_ddi(dev);
> >>-				} else {
> >>-					gen9_disable_dc5(dev_priv);
> >>  				}
> >>  			}
> >>  			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
> >>@@ -658,7 +656,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> >>  			POSTING_READ(HSW_PWR_WELL_DRIVER);
> >>  			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
> >>-			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> >>+			if (GEN9_ENABLE_DC5(dev) &&
> >>  				power_well->data == SKL_DISP_PW_2) {
> >>  				enum csr_state state;
> >>  				/* TODO: wait for a completion event or
> >>@@ -671,10 +669,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> >>  					DRM_ERROR("CSR firmware not ready (%d)\n",
> >>  							state);
> >>  				else
> >>-					if (SKL_ENABLE_DC6(dev))
> >>-						skl_enable_dc6(dev_priv);
> >>-					else
> >>-						gen9_enable_dc5(dev_priv);
> >>+					gen9_enable_dc5(dev_priv);
> >>  			}
> >>  		}
> >>  	}
> >>-- 
> >>2.0.2
> >>
> >>_______________________________________________
> >>Intel-gfx mailing list
> >>[e-mail]
> >>http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

Patrik Jakobsson Sept. 29, 2015, 12:35 p.m. UTC | #6

On Tue, Sep 29, 2015 at 11:01:35AM +0200, Daniel Vetter wrote:
> On Tue, Sep 29, 2015 at 11:08:25AM +0530, Animesh Manna wrote:
> > 
> > 
> > On 09/28/2015 12:51 PM, Daniel Vetter wrote:
> > >On Wed, Aug 26, 2015 at 01:36:07AM +0530, Animesh Manna wrote:
> > >>Mmio register access after dc6/dc5 entry is not allowed when
> > >>DC6 power states are enabled according to bspec (bspec-id 0527),
> > >>so enabling dc6 as the last call in suspend flow.
> > >We unconditionaly grab a power well reference for everything in our
> > >suspend/resume functions, which means dc6/5 should be prevented for long
> > >enough.
> > >
> > >Also since dc6/5 are part of the power well framework you can't just
> > >enable/disable them directly, instead you need to get/put the
> > >corresponding power wells.
> > >
> > >Finally this patch doesn't just do that, but also frobs around a lot in
> > >set power well code itself, and I have no idea what it does there and why.
> > >It does smell a bit like you're just breaking dc6 for runtime pm though,
> > >which wouldn't be good.
> > >
> > >Anyway I decided to just merge this since this patch series has been
> > >floating around since forever, but then the patch didn't apply cleanly and
> > >so dropped it.
> > 
> > I mentioned in my commit message that we have a h/w workaround.
> > "Mmio register access after dc6/dc5 entry is not allowed when
> > DC6 power states are enabled"
> > 
> > This patch is mandatory to solve the pc10 entry issue for skylake.
> > Planning to send again after rebase on top of tree.

Hi Animesh and Daniel

I'm trying to shed some light on this. It seems rather confusing atm. Animesh,
please correct me if I'm wrong below.

> The problem isn't that the patch didn't apply cleanly, but that it seems
> to have fundamental issues:
> - We already grab power well references around suspend/resume code, see
>   the call to intel_power_domains_init_hw and the calls to
>   intel_display_set_init_power. That code is supposed to ensure that while
>   suspend/resume is going on _all_ power wells are on (and hence dc6/5 are
>   forbidden). It's a bit a big hammer, but we already have the code to do
>   exactly what you're trying to do here. It could be though that for skl
>   it's in the wrong order (but then the commit message must state very
>   clearly what's the exact depency, the gpu is assembled from a pile of
>   various different blocks which are all mostly independent).

DC6 can only be enabled when _all_ power wells are disabled. Hence this patch
moves DC6 enabled/disabled to the runtime suspend/resume callbacks. Previously 
we only disabled DC6 when PW2 was enabled.

The DC state table says:
- Everything off: DC6 enabled
- PW0 enabled: DC5 enabled but DC6 is disabled
- PW1 enabled: DC5 and DC6 disabled 
- PW2 enabled: DC5 and DC6 disabled 

> - Your patch directly calls skl_enable/disable_dc6 instead of going
>   through the power well abstraction. Breaking these abstraction needs a
>   really good reason, adequate assurance that the refcounts are ok and
>   nothing breaks (using WARN_ON) and an explanation why the refcount
>   interface for display power management doesn't work.

This is indeed a hack but since all we need is the guarantee that a power well
is in fact enabled we can see the operation as: enable pw + disable dc state.
This is currently hacked into skl_set_power_well(). It's not nice but it takes
away the complexity of modeling DC states as power wells.

The alternative would be to turn DC5 and DC6 into power wells and add proper
dependencies. What I've discovered is that it's quite a tight fit.

> - You also do changes to the power well code itself, which looks like it
>   might break dc6 for runtime pm (by only going into dc5 at most). That
>   needs to be a separate patch or have a much better explanation.

If I understood correctly, that is the entire point of this patch. Don't allow
DC6 unless all power wells are disabled. My question though is why we don't
disable DC5 for PW1 as well?

> We probably need to set up a meeting to resolve this sooner than the few
> months it took to figure out the resume issue ...
> 
> Thanks, Daniel

I'd like to join if possible.

Thanks
Patrik

> > >>
> > >>v2: Based on review comment from Daniel,
> > >>- created a seperate patch for csr uninitialization set call.
> > >>
> > >>Cc: Daniel Vetter <[e-mail]>
> > >>Cc: Damien Lespiau <[e-mail]>
> > >>Cc: Imre Deak <[e-mail]>
> > >>Cc: Sunil Kamath <[e-mail]>
> > >>Signed-off-by: Animesh Manna <[e-mail]>
> > >>Signed-off-by: Vathsala Nagaraju <[e-mail]>
> > >>Signed-off-by: Rajneesh Bhardwaj <[e-mail]>
> > >>---
> > >>  drivers/gpu/drm/i915/i915_drv.c         | 13 +++++++++++++
> > >>  drivers/gpu/drm/i915/intel_drv.h        |  2 ++
> > >>  drivers/gpu/drm/i915/intel_runtime_pm.c | 19 +++++++------------
> > >>  3 files changed, 22 insertions(+), 12 deletions(-)
> > >>
> > >>diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > >>index 478101c..fa66162 100644
> > >>--- a/drivers/gpu/drm/i915/i915_drv.c
> > >>+++ b/drivers/gpu/drm/i915/i915_drv.c
> > >>@@ -1013,10 +1013,20 @@ static int i915_pm_resume(struct device *dev)
> > >>  static int skl_suspend_complete(struct drm_i915_private *dev_priv)
> > >>  {
> > >>+	enum csr_state state;
> > >>  	/* Enabling DC6 is not a hard requirement to enter runtime D3 */
> > >>  	skl_uninit_cdclk(dev_priv);
> > >>+	/* TODO: wait for a completion event or
> > >>+	 * similar here instead of busy
> > >>+	 * waiting using wait_for function.
> > >>+	 */
> > >>+	wait_for((state = intel_csr_load_status_get(dev_priv)) !=
> > >>+			FW_UNINITIALIZED, 1000);
> > >>+	if (state == FW_LOADED)
> > >>+		skl_enable_dc6(dev_priv);
> > >>+
> > >>  	return 0;
> > >>  }
> > >>@@ -1063,6 +1073,9 @@ static int skl_resume_prepare(struct drm_i915_private *dev_priv)
> > >>  {
> > >>  	struct drm_device *dev = dev_priv->dev;
> > >>+	if (intel_csr_load_status_get(dev_priv) == FW_LOADED)
> > >>+		skl_disable_dc6(dev_priv);
> > >>+
> > >>  	skl_init_cdclk(dev_priv);
> > >>  	intel_csr_load_program(dev);
> > >>diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > >>index 81b7d77..9cb7d4e 100644
> > >>--- a/drivers/gpu/drm/i915/intel_drv.h
> > >>+++ b/drivers/gpu/drm/i915/intel_drv.h
> > >>@@ -1118,6 +1118,8 @@ void bxt_enable_dc9(struct drm_i915_private *dev_priv);
> > >>  void bxt_disable_dc9(struct drm_i915_private *dev_priv);
> > >>  void skl_init_cdclk(struct drm_i915_private *dev_priv);
> > >>  void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
> > >>+void skl_enable_dc6(struct drm_i915_private *dev_priv);
> > >>+void skl_disable_dc6(struct drm_i915_private *dev_priv);
> > >>  void intel_dp_get_m_n(struct intel_crtc *crtc,
> > >>  		      struct intel_crtc_state *pipe_config);
> > >>  void intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set m_n);
> > >>diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > >>index 821644d..23a3aa3 100644
> > >>--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > >>+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > >>@@ -548,7 +548,7 @@ static void assert_can_disable_dc6(struct drm_i915_private *dev_priv)
> > >>  		"DC6 already programmed to be disabled.\n");
> > >>  }
> > >>-static void skl_enable_dc6(struct drm_i915_private *dev_priv)
> > >>+void skl_enable_dc6(struct drm_i915_private *dev_priv)
> > >>  {
> > >>  	uint32_t val;
> > >>@@ -565,7 +565,7 @@ static void skl_enable_dc6(struct drm_i915_private *dev_priv)
> > >>  	POSTING_READ(DC_STATE_EN);
> > >>  }
> > >>-static void skl_disable_dc6(struct drm_i915_private *dev_priv)
> > >>+void skl_disable_dc6(struct drm_i915_private *dev_priv)
> > >>  {
> > >>  	uint32_t val;
> > >>@@ -626,10 +626,10 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> > >>  				!I915_READ(HSW_PWR_WELL_BIOS),
> > >>  				"Invalid for power well status to be enabled, unless done by the BIOS, \
> > >>  				when request is to disable!\n");
> > >>-			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> > >>-				power_well->data == SKL_DISP_PW_2) {
> > >>+			if (power_well->data == SKL_DISP_PW_2) {
> > >>+				if (GEN9_ENABLE_DC5(dev))
> > >>+					gen9_disable_dc5(dev_priv);
> > >>  				if (SKL_ENABLE_DC6(dev)) {
> > >>-					skl_disable_dc6(dev_priv);
> > >>  					/*
> > >>  					 * DDI buffer programming unnecessary during driver-load/resume
> > >>  					 * as it's already done during modeset initialization then.
> > >>@@ -637,8 +637,6 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> > >>  					 */
> > >>  					if (!dev_priv->power_domains.initializing)
> > >>  						intel_prepare_ddi(dev);
> > >>-				} else {
> > >>-					gen9_disable_dc5(dev_priv);
> > >>  				}
> > >>  			}
> > >>  			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
> > >>@@ -658,7 +656,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> > >>  			POSTING_READ(HSW_PWR_WELL_DRIVER);
> > >>  			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
> > >>-			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> > >>+			if (GEN9_ENABLE_DC5(dev) &&
> > >>  				power_well->data == SKL_DISP_PW_2) {
> > >>  				enum csr_state state;
> > >>  				/* TODO: wait for a completion event or
> > >>@@ -671,10 +669,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> > >>  					DRM_ERROR("CSR firmware not ready (%d)\n",
> > >>  							state);
> > >>  				else
> > >>-					if (SKL_ENABLE_DC6(dev))
> > >>-						skl_enable_dc6(dev_priv);
> > >>-					else
> > >>-						gen9_enable_dc5(dev_priv);
> > >>+					gen9_enable_dc5(dev_priv);
> > >>  			}
> > >>  		}
> > >>  	}
> > >>-- 
> > >>2.0.2
> > >>
> > >>_______________________________________________
> > >>Intel-gfx mailing list
> > >>[e-mail]
> > >>http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Vetter Sept. 29, 2015, 1:01 p.m. UTC | #7

On Tue, Sep 29, 2015 at 2:35 PM, Patrik Jakobsson
<[e-mail]> wrote:
> On Tue, Sep 29, 2015 at 11:01:35AM +0200, Daniel Vetter wrote:
>> On Tue, Sep 29, 2015 at 11:08:25AM +0530, Animesh Manna wrote:
>> >
>> >
>> > On 09/28/2015 12:51 PM, Daniel Vetter wrote:
>> > >On Wed, Aug 26, 2015 at 01:36:07AM +0530, Animesh Manna wrote:
>> > >>Mmio register access after dc6/dc5 entry is not allowed when
>> > >>DC6 power states are enabled according to bspec (bspec-id 0527),
>> > >>so enabling dc6 as the last call in suspend flow.
>> > >We unconditionaly grab a power well reference for everything in our
>> > >suspend/resume functions, which means dc6/5 should be prevented for long
>> > >enough.
>> > >
>> > >Also since dc6/5 are part of the power well framework you can't just
>> > >enable/disable them directly, instead you need to get/put the
>> > >corresponding power wells.
>> > >
>> > >Finally this patch doesn't just do that, but also frobs around a lot in
>> > >set power well code itself, and I have no idea what it does there and why.
>> > >It does smell a bit like you're just breaking dc6 for runtime pm though,
>> > >which wouldn't be good.
>> > >
>> > >Anyway I decided to just merge this since this patch series has been
>> > >floating around since forever, but then the patch didn't apply cleanly and
>> > >so dropped it.
>> >
>> > I mentioned in my commit message that we have a h/w workaround.
>> > "Mmio register access after dc6/dc5 entry is not allowed when
>> > DC6 power states are enabled"
>> >
>> > This patch is mandatory to solve the pc10 entry issue for skylake.
>> > Planning to send again after rebase on top of tree.
>
> Hi Animesh and Daniel
>
> I'm trying to shed some light on this. It seems rather confusing atm. Animesh,
> please correct me if I'm wrong below.
>
>> The problem isn't that the patch didn't apply cleanly, but that it seems
>> to have fundamental issues:
>> - We already grab power well references around suspend/resume code, see
>>   the call to intel_power_domains_init_hw and the calls to
>>   intel_display_set_init_power. That code is supposed to ensure that while
>>   suspend/resume is going on _all_ power wells are on (and hence dc6/5 are
>>   forbidden). It's a bit a big hammer, but we already have the code to do
>>   exactly what you're trying to do here. It could be though that for skl
>>   it's in the wrong order (but then the commit message must state very
>>   clearly what's the exact depency, the gpu is assembled from a pile of
>>   various different blocks which are all mostly independent).
>
> DC6 can only be enabled when _all_ power wells are disabled. Hence this patch
> moves DC6 enabled/disabled to the runtime suspend/resume callbacks. Previously
> we only disabled DC6 when PW2 was enabled.
>
> The DC state table says:
> - Everything off: DC6 enabled
> - PW0 enabled: DC5 enabled but DC6 is disabled
> - PW1 enabled: DC5 and DC6 disabled
> - PW2 enabled: DC5 and DC6 disabled

Ok, so what the patch does (and that is at least self-consistent) is
to move the DC6 enable/disable from PW2 to the top-level domain. That
doesn't quite match what you're describing here since we still have
PW1 managed by the driver, but that's short-circuited now.

>> - Your patch directly calls skl_enable/disable_dc6 instead of going
>>   through the power well abstraction. Breaking these abstraction needs a
>>   really good reason, adequate assurance that the refcounts are ok and
>>   nothing breaks (using WARN_ON) and an explanation why the refcount
>>   interface for display power management doesn't work.
>
> This is indeed a hack but since all we need is the guarantee that a power well
> is in fact enabled we can see the operation as: enable pw + disable dc state.
> This is currently hacked into skl_set_power_well(). It's not nice but it takes
> away the complexity of modeling DC states as power wells.
>
> The alternative would be to turn DC5 and DC6 into power wells and add proper
> dependencies. What I've discovered is that it's quite a tight fit.

Nah, if the goal is to move just DC6 the patch seems fine. I was just
really confused that the commit message talked about both dc6 _and_
dc5 being impossible to do, and then only moving half of the story
around. And I was also a bit confused with runtime pm vs. system
suspend (again).

So if we can confirm that this is really only about DC6 and not also
about dc5 like the commit message claims, then I can clean up the
commit message and apply the patch.

>> - You also do changes to the power well code itself, which looks like it
>>   might break dc6 for runtime pm (by only going into dc5 at most). That
>>   needs to be a separate patch or have a much better explanation.
>
> If I understood correctly, that is the entire point of this patch. Don't allow
> DC6 unless all power wells are disabled. My question though is why we don't
> disable DC5 for PW1 as well?

Afaiui dmc manages pw1 for us and we shouldn't ever touch it from the
driver side. I'm not entirely sure about that though since the current
fix is a hack. If we really shouldn't touch pw1 on skl then we should
remove that power domain from our skl code entirely.
-Daniel

Ville Syrjälä Sept. 29, 2015, 1:23 p.m. UTC | #8

On Tue, Sep 29, 2015 at 03:01:57PM +0200, Daniel Vetter wrote:
> On Tue, Sep 29, 2015 at 2:35 PM, Patrik Jakobsson
> <[e-mail]> wrote:
> > On Tue, Sep 29, 2015 at 11:01:35AM +0200, Daniel Vetter wrote:
> >> On Tue, Sep 29, 2015 at 11:08:25AM +0530, Animesh Manna wrote:
> >> >
> >> >
> >> > On 09/28/2015 12:51 PM, Daniel Vetter wrote:
> >> > >On Wed, Aug 26, 2015 at 01:36:07AM +0530, Animesh Manna wrote:
> >> > >>Mmio register access after dc6/dc5 entry is not allowed when
> >> > >>DC6 power states are enabled according to bspec (bspec-id 0527),
> >> > >>so enabling dc6 as the last call in suspend flow.
> >> > >We unconditionaly grab a power well reference for everything in our
> >> > >suspend/resume functions, which means dc6/5 should be prevented for long
> >> > >enough.
> >> > >
> >> > >Also since dc6/5 are part of the power well framework you can't just
> >> > >enable/disable them directly, instead you need to get/put the
> >> > >corresponding power wells.
> >> > >
> >> > >Finally this patch doesn't just do that, but also frobs around a lot in
> >> > >set power well code itself, and I have no idea what it does there and why.
> >> > >It does smell a bit like you're just breaking dc6 for runtime pm though,
> >> > >which wouldn't be good.
> >> > >
> >> > >Anyway I decided to just merge this since this patch series has been
> >> > >floating around since forever, but then the patch didn't apply cleanly and
> >> > >so dropped it.
> >> >
> >> > I mentioned in my commit message that we have a h/w workaround.
> >> > "Mmio register access after dc6/dc5 entry is not allowed when
> >> > DC6 power states are enabled"
> >> >
> >> > This patch is mandatory to solve the pc10 entry issue for skylake.
> >> > Planning to send again after rebase on top of tree.
> >
> > Hi Animesh and Daniel
> >
> > I'm trying to shed some light on this. It seems rather confusing atm. Animesh,
> > please correct me if I'm wrong below.
> >
> >> The problem isn't that the patch didn't apply cleanly, but that it seems
> >> to have fundamental issues:
> >> - We already grab power well references around suspend/resume code, see
> >>   the call to intel_power_domains_init_hw and the calls to
> >>   intel_display_set_init_power. That code is supposed to ensure that while
> >>   suspend/resume is going on _all_ power wells are on (and hence dc6/5 are
> >>   forbidden). It's a bit a big hammer, but we already have the code to do
> >>   exactly what you're trying to do here. It could be though that for skl
> >>   it's in the wrong order (but then the commit message must state very
> >>   clearly what's the exact depency, the gpu is assembled from a pile of
> >>   various different blocks which are all mostly independent).
> >
> > DC6 can only be enabled when _all_ power wells are disabled. Hence this patch
> > moves DC6 enabled/disabled to the runtime suspend/resume callbacks. Previously
> > we only disabled DC6 when PW2 was enabled.
> >
> > The DC state table says:
> > - Everything off: DC6 enabled
> > - PW0 enabled: DC5 enabled but DC6 is disabled
> > - PW1 enabled: DC5 and DC6 disabled
> > - PW2 enabled: DC5 and DC6 disabled
> 
> Ok, so what the patch does (and that is at least self-consistent) is
> to move the DC6 enable/disable from PW2 to the top-level domain. That
> doesn't quite match what you're describing here since we still have
> PW1 managed by the driver, but that's short-circuited now.

Hmm. Why are we going back and forth on this all the time? Was there
some problem with the plan [1] that Imre and I hatched?

[1] http://lists.freedesktop.org/archives/intel-gfx/2015-September/076612.html

> 
> >> - Your patch directly calls skl_enable/disable_dc6 instead of going
> >>   through the power well abstraction. Breaking these abstraction needs a
> >>   really good reason, adequate assurance that the refcounts are ok and
> >>   nothing breaks (using WARN_ON) and an explanation why the refcount
> >>   interface for display power management doesn't work.
> >
> > This is indeed a hack but since all we need is the guarantee that a power well
> > is in fact enabled we can see the operation as: enable pw + disable dc state.
> > This is currently hacked into skl_set_power_well(). It's not nice but it takes
> > away the complexity of modeling DC states as power wells.
> >
> > The alternative would be to turn DC5 and DC6 into power wells and add proper
> > dependencies. What I've discovered is that it's quite a tight fit.
> 
> Nah, if the goal is to move just DC6 the patch seems fine. I was just
> really confused that the commit message talked about both dc6 _and_
> dc5 being impossible to do, and then only moving half of the story
> around. And I was also a bit confused with runtime pm vs. system
> suspend (again).
> 
> So if we can confirm that this is really only about DC6 and not also
> about dc5 like the commit message claims, then I can clean up the
> commit message and apply the patch.
> 
> >> - You also do changes to the power well code itself, which looks like it
> >>   might break dc6 for runtime pm (by only going into dc5 at most). That
> >>   needs to be a separate patch or have a much better explanation.
> >
> > If I understood correctly, that is the entire point of this patch. Don't allow
> > DC6 unless all power wells are disabled. My question though is why we don't
> > disable DC5 for PW1 as well?
> 
> Afaiui dmc manages pw1 for us and we shouldn't ever touch it from the
> driver side. I'm not entirely sure about that though since the current
> fix is a hack. If we really shouldn't touch pw1 on skl then we should
> remove that power domain from our skl code entirely.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> [phone] - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Vetter Sept. 29, 2015, 2 p.m. UTC | #9

On Tue, Sep 29, 2015 at 3:23 PM, Ville Syrjälä
<[e-mail]> wrote:
> Hmm. Why are we going back and forth on this all the time? Was there
> some problem with the plan [1] that Imre and I hatched?
>
> [1] http://lists.freedesktop.org/archives/intel-gfx/2015-September/076612.html

Well this is just the interim bugfix with a confusing commit message.
Compared to your overall plan we now have dc5&pw2 fused together, pw1
is still there but a no-op and dc6 is now (with this patch) fused with
with pw0 and overall device d3 (or whatever we use to kill the entire
thing and still appease the firmware).

So at least matches the overall plan.
-Daniel

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 478101c..fa66162 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1013,10 +1013,20 @@  static int i915_pm_resume(struct device *dev)
 
 static int skl_suspend_complete(struct drm_i915_private *dev_priv)
 {
+	enum csr_state state;
 	/* Enabling DC6 is not a hard requirement to enter runtime D3 */
 
 	skl_uninit_cdclk(dev_priv);
 
+	/* TODO: wait for a completion event or
+	 * similar here instead of busy
+	 * waiting using wait_for function.
+	 */
+	wait_for((state = intel_csr_load_status_get(dev_priv)) !=
+			FW_UNINITIALIZED, 1000);
+	if (state == FW_LOADED)
+		skl_enable_dc6(dev_priv);
+
 	return 0;
 }
 
@@ -1063,6 +1073,9 @@  static int skl_resume_prepare(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
 
+	if (intel_csr_load_status_get(dev_priv) == FW_LOADED)
+		skl_disable_dc6(dev_priv);
+
 	skl_init_cdclk(dev_priv);
 	intel_csr_load_program(dev);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 81b7d77..9cb7d4e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1118,6 +1118,8 @@  void bxt_enable_dc9(struct drm_i915_private *dev_priv);
 void bxt_disable_dc9(struct drm_i915_private *dev_priv);
 void skl_init_cdclk(struct drm_i915_private *dev_priv);
 void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
+void skl_enable_dc6(struct drm_i915_private *dev_priv);
+void skl_disable_dc6(struct drm_i915_private *dev_priv);
 void intel_dp_get_m_n(struct intel_crtc *crtc,
 		      struct intel_crtc_state *pipe_config);
 void intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set m_n);
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 821644d..23a3aa3 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -548,7 +548,7 @@  static void assert_can_disable_dc6(struct drm_i915_private *dev_priv)
 		"DC6 already programmed to be disabled.\n");
 }
 
-static void skl_enable_dc6(struct drm_i915_private *dev_priv)
+void skl_enable_dc6(struct drm_i915_private *dev_priv)
 {
 	uint32_t val;
 
@@ -565,7 +565,7 @@  static void skl_enable_dc6(struct drm_i915_private *dev_priv)
 	POSTING_READ(DC_STATE_EN);
 }
 
-static void skl_disable_dc6(struct drm_i915_private *dev_priv)
+void skl_disable_dc6(struct drm_i915_private *dev_priv)
 {
 	uint32_t val;
 
@@ -626,10 +626,10 @@  static void skl_set_power_well(struct drm_i915_private *dev_priv,
 				!I915_READ(HSW_PWR_WELL_BIOS),
 				"Invalid for power well status to be enabled, unless done by the BIOS, \
 				when request is to disable!\n");
-			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
-				power_well->data == SKL_DISP_PW_2) {
+			if (power_well->data == SKL_DISP_PW_2) {
+				if (GEN9_ENABLE_DC5(dev))
+					gen9_disable_dc5(dev_priv);
 				if (SKL_ENABLE_DC6(dev)) {
-					skl_disable_dc6(dev_priv);
 					/*
 					 * DDI buffer programming unnecessary during driver-load/resume
 					 * as it's already done during modeset initialization then.
@@ -637,8 +637,6 @@  static void skl_set_power_well(struct drm_i915_private *dev_priv,
 					 */
 					if (!dev_priv->power_domains.initializing)
 						intel_prepare_ddi(dev);
-				} else {
-					gen9_disable_dc5(dev_priv);
 				}
 			}
 			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
@@ -658,7 +656,7 @@  static void skl_set_power_well(struct drm_i915_private *dev_priv,
 			POSTING_READ(HSW_PWR_WELL_DRIVER);
 			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
 
-			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
+			if (GEN9_ENABLE_DC5(dev) &&
 				power_well->data == SKL_DISP_PW_2) {
 				enum csr_state state;
 				/* TODO: wait for a completion event or
@@ -671,10 +669,7 @@  static void skl_set_power_well(struct drm_i915_private *dev_priv,
 					DRM_ERROR("CSR firmware not ready (%d)\n",
 							state);
 				else
-					if (SKL_ENABLE_DC6(dev))
-						skl_enable_dc6(dev_priv);
-					else
-						gen9_enable_dc5(dev_priv);
+					gen9_enable_dc5(dev_priv);
 			}
 		}
 	}