[SKL-DMC-BUGFIX 0/5] SKL PC10 entry fixes.

[SKL-DMC-BUGFIX 0/5] SKL PC10 entry fixes.

[Animesh Manna]

The following patches helps to solve PC10 entry issue for SKL.
Detailed description about the changes done to solve the issue
is mentioned in commit message of each patch.

All these patches are send earlier for review as part of "Redesign of
dmc firmware loading" patch series. Now skl specific bug-fixes are
seperated out and sending as seperate patch series to make it simple.

Animesh Manna (5):
  drm/i915/gen9: Removed byte swapping for csr firmware
  drm/i915/skl: Making DC6 entry is the last call in suspend flow.
  drm/i915/skl: Do not disable cdclk PLL if csr firmware is present.
  drm/i915/skl: Block disable call for pw1 if dmc firmware is present.
  drm/i915/skl: Removed csr firmware load in resume path.

 drivers/gpu/drm/i915/i915_drv.c         | 13 +++++-----
 drivers/gpu/drm/i915/i915_drv.h         |  2 +-
 drivers/gpu/drm/i915/intel_csr.c        | 16 +++---------
 drivers/gpu/drm/i915/intel_display.c    | 11 +++++---
 drivers/gpu/drm/i915/intel_drv.h        |  2 ++
 drivers/gpu/drm/i915/intel_runtime_pm.c | 45 +++++++++++++--------------------
 6 files changed, 37 insertions(+), 52 deletions(-)

-- 
2.0.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[SKL-DMC-BUGFIX 1/5] drm/i915/gen9: Removed byte swapping for csr firmware

[Animesh Manna]

This patch contains the changes to remove the byte
swapping logic introduced with old dmc firmware.
While debugging PC10 entry issue for skylake found
with latest dmc firmware version 1.18 without byte
swapping dmc is working fine and able to enter PC10.

v1: Initial version.

v2: Corrected firmware size during memcpy(). (Suggested by Sunil)

Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  2 +-
 drivers/gpu/drm/i915/intel_csr.c | 16 ++++------------
 2 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b94ada9..9d0ee58 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -742,7 +742,7 @@ enum csr_state {
 
 struct intel_csr {
 	const char *fw_path;
-	__be32 *dmc_payload;
+	uint32_t *dmc_payload;
 	uint32_t dmc_fw_size;
 	uint32_t mmio_count;
 	uint32_t mmioaddr[8];
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 6d8a7bf..ba1ae03 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -244,7 +244,7 @@ void intel_csr_load_status_set(struct drm_i915_private *dev_priv,
 void intel_csr_load_program(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	__be32 *payload = dev_priv->csr.dmc_payload;
+	u32 *payload = dev_priv->csr.dmc_payload;
 	uint32_t i, fw_size;
 
 	if (!IS_GEN9(dev)) {
@@ -256,7 +256,7 @@ void intel_csr_load_program(struct drm_device *dev)
 	fw_size = dev_priv->csr.dmc_fw_size;
 	for (i = 0; i < fw_size; i++)
 		I915_WRITE(CSR_PROGRAM_BASE + i * 4,
-			(u32 __force)payload[i]);
+			payload[i]);
 
 	for (i = 0; i < dev_priv->csr.mmio_count; i++) {
 		I915_WRITE(dev_priv->csr.mmioaddr[i],
@@ -279,7 +279,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 	char substepping = intel_get_substepping(dev);
 	uint32_t dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes;
 	uint32_t i;
-	__be32 *dmc_payload;
+	uint32_t *dmc_payload;
 	bool fw_loaded = false;
 
 	if (!fw) {
@@ -375,15 +375,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 	}
 
 	dmc_payload = csr->dmc_payload;
-	for (i = 0; i < dmc_header->fw_size; i++) {
-		uint32_t *tmp = (u32 *)&fw->data[readcount + i * 4];
-		/*
-		 * The firmware payload is an array of 32 bit words stored in
-		 * little-endian format in the firmware image and programmed
-		 * as 32 bit big-endian format to memory.
-		 */
-		dmc_payload[i] = cpu_to_be32(*tmp);
-	}
+	memcpy(dmc_payload, &fw->data[readcount], nbytes);
 
 	/* load csr program during system boot, as needed for DC states */
 	intel_csr_load_program(dev);
-- 
2.0.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [SKL-DMC-BUGFIX 1/5] drm/i915/gen9: Removed byte swapping for csr firmware

[Nagaraju, Vathsala]

"This patch contains the changes to remove the byte swapping logic introduced with old dmc firmware."
Which version of DMC need reversal  logic?  Atleast , 4,1.16,1.18 follow the same format.

-----Original Message-----
From: Manna, Animesh 
Sent: Monday, August 3, 2015 9:56 PM
To: intel-gfx@lists.freedesktop.org
Cc: Manna, Animesh; Vetter, Daniel; Lespiau, Damien; Deak, Imre; Kamath, Sunil; Nagaraju, Vathsala
Subject: [SKL-DMC-BUGFIX 1/5] drm/i915/gen9: Removed byte swapping for csr firmware

This patch contains the changes to remove the byte swapping logic introduced with old dmc firmware.
While debugging PC10 entry issue for skylake found with latest dmc firmware version 1.18 without byte swapping dmc is working fine and able to enter PC10.

v1: Initial version.

v2: Corrected firmware size during memcpy(). (Suggested by Sunil)

Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  2 +-  drivers/gpu/drm/i915/intel_csr.c | 16 ++++------------
 2 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b94ada9..9d0ee58 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -742,7 +742,7 @@ enum csr_state {
 
 struct intel_csr {
 	const char *fw_path;
-	__be32 *dmc_payload;
+	uint32_t *dmc_payload;
 	uint32_t dmc_fw_size;
 	uint32_t mmio_count;
 	uint32_t mmioaddr[8];
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 6d8a7bf..ba1ae03 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -244,7 +244,7 @@ void intel_csr_load_status_set(struct drm_i915_private *dev_priv,  void intel_csr_load_program(struct drm_device *dev)  {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	__be32 *payload = dev_priv->csr.dmc_payload;
+	u32 *payload = dev_priv->csr.dmc_payload;
 	uint32_t i, fw_size;
 
 	if (!IS_GEN9(dev)) {
@@ -256,7 +256,7 @@ void intel_csr_load_program(struct drm_device *dev)
 	fw_size = dev_priv->csr.dmc_fw_size;
 	for (i = 0; i < fw_size; i++)
 		I915_WRITE(CSR_PROGRAM_BASE + i * 4,
-			(u32 __force)payload[i]);
+			payload[i]);
 
 	for (i = 0; i < dev_priv->csr.mmio_count; i++) {
 		I915_WRITE(dev_priv->csr.mmioaddr[i],
@@ -279,7 +279,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 	char substepping = intel_get_substepping(dev);
 	uint32_t dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes;
 	uint32_t i;
-	__be32 *dmc_payload;
+	uint32_t *dmc_payload;
 	bool fw_loaded = false;
 
 	if (!fw) {
@@ -375,15 +375,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 	}
 
 	dmc_payload = csr->dmc_payload;
-	for (i = 0; i < dmc_header->fw_size; i++) {
-		uint32_t *tmp = (u32 *)&fw->data[readcount + i * 4];
-		/*
-		 * The firmware payload is an array of 32 bit words stored in
-		 * little-endian format in the firmware image and programmed
-		 * as 32 bit big-endian format to memory.
-		 */
-		dmc_payload[i] = cpu_to_be32(*tmp);
-	}
+	memcpy(dmc_payload, &fw->data[readcount], nbytes);
 
 	/* load csr program during system boot, as needed for DC states */
 	intel_csr_load_program(dev);
--
2.0.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [SKL-DMC-BUGFIX 1/5] drm/i915/gen9: Removed byte swapping for csr firmware

[Animesh Manna]

On 8/4/2015 9:16 AM, Nagaraju, Vathsala wrote:
> "This patch contains the changes to remove the byte swapping logic introduced with old dmc firmware."
> Which version of DMC need reversal  logic?  Atleast , 4,1.16,1.18 follow the same format.

Packaging of firmware binary completely changed after api version 1.0, so by old firmware I want to mean
the initial version before dmc 1.0.

>
> -----Original Message-----
> From: Manna, Animesh
> Sent: Monday, August 3, 2015 9:56 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Manna, Animesh; Vetter, Daniel; Lespiau, Damien; Deak, Imre; Kamath, Sunil; Nagaraju, Vathsala
> Subject: [SKL-DMC-BUGFIX 1/5] drm/i915/gen9: Removed byte swapping for csr firmware
>
> This patch contains the changes to remove the byte swapping logic introduced with old dmc firmware.
> While debugging PC10 entry issue for skylake found with latest dmc firmware version 1.18 without byte swapping dmc is working fine and able to enter PC10.
>
> v1: Initial version.
>
> v2: Corrected firmware size during memcpy(). (Suggested by Sunil)
>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Sunil Kamath <sunil.kamath@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h  |  2 +-  drivers/gpu/drm/i915/intel_csr.c | 16 ++++------------
>   2 files changed, 5 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b94ada9..9d0ee58 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -742,7 +742,7 @@ enum csr_state {
>   
>   struct intel_csr {
>   	const char *fw_path;
> -	__be32 *dmc_payload;
> +	uint32_t *dmc_payload;
>   	uint32_t dmc_fw_size;
>   	uint32_t mmio_count;
>   	uint32_t mmioaddr[8];
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index 6d8a7bf..ba1ae03 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -244,7 +244,7 @@ void intel_csr_load_status_set(struct drm_i915_private *dev_priv,  void intel_csr_load_program(struct drm_device *dev)  {
>   	struct drm_i915_private *dev_priv = dev->dev_private;
> -	__be32 *payload = dev_priv->csr.dmc_payload;
> +	u32 *payload = dev_priv->csr.dmc_payload;
>   	uint32_t i, fw_size;
>   
>   	if (!IS_GEN9(dev)) {
> @@ -256,7 +256,7 @@ void intel_csr_load_program(struct drm_device *dev)
>   	fw_size = dev_priv->csr.dmc_fw_size;
>   	for (i = 0; i < fw_size; i++)
>   		I915_WRITE(CSR_PROGRAM_BASE + i * 4,
> -			(u32 __force)payload[i]);
> +			payload[i]);
>   
>   	for (i = 0; i < dev_priv->csr.mmio_count; i++) {
>   		I915_WRITE(dev_priv->csr.mmioaddr[i],
> @@ -279,7 +279,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>   	char substepping = intel_get_substepping(dev);
>   	uint32_t dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes;
>   	uint32_t i;
> -	__be32 *dmc_payload;
> +	uint32_t *dmc_payload;
>   	bool fw_loaded = false;
>   
>   	if (!fw) {
> @@ -375,15 +375,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>   	}
>   
>   	dmc_payload = csr->dmc_payload;
> -	for (i = 0; i < dmc_header->fw_size; i++) {
> -		uint32_t *tmp = (u32 *)&fw->data[readcount + i * 4];
> -		/*
> -		 * The firmware payload is an array of 32 bit words stored in
> -		 * little-endian format in the firmware image and programmed
> -		 * as 32 bit big-endian format to memory.
> -		 */
> -		dmc_payload[i] = cpu_to_be32(*tmp);
> -	}
> +	memcpy(dmc_payload, &fw->data[readcount], nbytes);
>   
>   	/* load csr program during system boot, as needed for DC states */
>   	intel_csr_load_program(dev);
> --
> 2.0.2
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [SKL-DMC-BUGFIX 1/5] drm/i915/gen9: Removed byte swapping for csr firmware

[Daniel Vetter]

On Tue, Aug 04, 2015 at 11:25:40AM +0530, Animesh Manna wrote:
> 
> 
> On 8/4/2015 9:16 AM, Nagaraju, Vathsala wrote:
> >"This patch contains the changes to remove the byte swapping logic introduced with old dmc firmware."
> >Which version of DMC need reversal  logic?  Atleast , 4,1.16,1.18 follow the same format.
> 
> Packaging of firmware binary completely changed after api version 1.0, so by old firmware I want to mean
> the initial version before dmc 1.0.

This kind of information really must be included in the commit message.
Very likely someone with old firmware will bisect to this commit, and if
you don't include that people need latest dmc firmware there will be
confusion.

Commit message _must_ be complete and contain everything that was
discussed while reviewing and developing a patch.

I added a note while merging the patch.
-Daniel

> 
> >
> >-----Original Message-----
> >From: Manna, Animesh
> >Sent: Monday, August 3, 2015 9:56 PM
> >To: intel-gfx@lists.freedesktop.org
> >Cc: Manna, Animesh; Vetter, Daniel; Lespiau, Damien; Deak, Imre; Kamath, Sunil; Nagaraju, Vathsala
> >Subject: [SKL-DMC-BUGFIX 1/5] drm/i915/gen9: Removed byte swapping for csr firmware
> >
> >This patch contains the changes to remove the byte swapping logic introduced with old dmc firmware.
> >While debugging PC10 entry issue for skylake found with latest dmc firmware version 1.18 without byte swapping dmc is working fine and able to enter PC10.
> >
> >v1: Initial version.
> >
> >v2: Corrected firmware size during memcpy(). (Suggested by Sunil)
> >
> >Cc: Daniel Vetter <daniel.vetter@intel.com>
> >Cc: Damien Lespiau <damien.lespiau@intel.com>
> >Cc: Imre Deak <imre.deak@intel.com>
> >Cc: Sunil Kamath <sunil.kamath@intel.com>
> >Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> >Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> >---
> >  drivers/gpu/drm/i915/i915_drv.h  |  2 +-  drivers/gpu/drm/i915/intel_csr.c | 16 ++++------------
> >  2 files changed, 5 insertions(+), 13 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b94ada9..9d0ee58 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.h
> >+++ b/drivers/gpu/drm/i915/i915_drv.h
> >@@ -742,7 +742,7 @@ enum csr_state {
> >  struct intel_csr {
> >  	const char *fw_path;
> >-	__be32 *dmc_payload;
> >+	uint32_t *dmc_payload;
> >  	uint32_t dmc_fw_size;
> >  	uint32_t mmio_count;
> >  	uint32_t mmioaddr[8];
> >diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> >index 6d8a7bf..ba1ae03 100644
> >--- a/drivers/gpu/drm/i915/intel_csr.c
> >+++ b/drivers/gpu/drm/i915/intel_csr.c
> >@@ -244,7 +244,7 @@ void intel_csr_load_status_set(struct drm_i915_private *dev_priv,  void intel_csr_load_program(struct drm_device *dev)  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >-	__be32 *payload = dev_priv->csr.dmc_payload;
> >+	u32 *payload = dev_priv->csr.dmc_payload;
> >  	uint32_t i, fw_size;
> >  	if (!IS_GEN9(dev)) {
> >@@ -256,7 +256,7 @@ void intel_csr_load_program(struct drm_device *dev)
> >  	fw_size = dev_priv->csr.dmc_fw_size;
> >  	for (i = 0; i < fw_size; i++)
> >  		I915_WRITE(CSR_PROGRAM_BASE + i * 4,
> >-			(u32 __force)payload[i]);
> >+			payload[i]);
> >  	for (i = 0; i < dev_priv->csr.mmio_count; i++) {
> >  		I915_WRITE(dev_priv->csr.mmioaddr[i],
> >@@ -279,7 +279,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
> >  	char substepping = intel_get_substepping(dev);
> >  	uint32_t dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes;
> >  	uint32_t i;
> >-	__be32 *dmc_payload;
> >+	uint32_t *dmc_payload;
> >  	bool fw_loaded = false;
> >  	if (!fw) {
> >@@ -375,15 +375,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
> >  	}
> >  	dmc_payload = csr->dmc_payload;
> >-	for (i = 0; i < dmc_header->fw_size; i++) {
> >-		uint32_t *tmp = (u32 *)&fw->data[readcount + i * 4];
> >-		/*
> >-		 * The firmware payload is an array of 32 bit words stored in
> >-		 * little-endian format in the firmware image and programmed
> >-		 * as 32 bit big-endian format to memory.
> >-		 */
> >-		dmc_payload[i] = cpu_to_be32(*tmp);
> >-	}
> >+	memcpy(dmc_payload, &fw->data[readcount], nbytes);
> >  	/* load csr program during system boot, as needed for DC states */
> >  	intel_csr_load_program(dev);
> >--
> >2.0.2
> >
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [SKL-DMC-BUGFIX 1/5] drm/i915/gen9: Removed byte swapping for csr firmware

[Animesh Manna]

On 8/5/2015 2:31 PM, Daniel Vetter wrote:
> On Tue, Aug 04, 2015 at 11:25:40AM +0530, Animesh Manna wrote:
>>
>> On 8/4/2015 9:16 AM, Nagaraju, Vathsala wrote:
>>> "This patch contains the changes to remove the byte swapping logic introduced with old dmc firmware."
>>> Which version of DMC need reversal  logic?  Atleast , 4,1.16,1.18 follow the same format.
>> Packaging of firmware binary completely changed after api version 1.0, so by old firmware I want to mean
>> the initial version before dmc 1.0.
> This kind of information really must be included in the commit message.
> Very likely someone with old firmware will bisect to this commit, and if
> you don't include that people need latest dmc firmware there will be
> confusion.
>
> Commit message _must_ be complete and contain everything that was
> discussed while reviewing and developing a patch.
>
> I added a note while merging the patch.
> -Daniel

Thanks Daniel. Sure, will follow your suggestion in future.

-Animesh


>>> -----Original Message-----
>>> From: Manna, Animesh
>>> Sent: Monday, August 3, 2015 9:56 PM
>>> To: intel-gfx@lists.freedesktop.org
>>> Cc: Manna, Animesh; Vetter, Daniel; Lespiau, Damien; Deak, Imre; Kamath, Sunil; Nagaraju, Vathsala
>>> Subject: [SKL-DMC-BUGFIX 1/5] drm/i915/gen9: Removed byte swapping for csr firmware
>>>
>>> This patch contains the changes to remove the byte swapping logic introduced with old dmc firmware.
>>> While debugging PC10 entry issue for skylake found with latest dmc firmware version 1.18 without byte swapping dmc is working fine and able to enter PC10.
>>>
>>> v1: Initial version.
>>>
>>> v2: Corrected firmware size during memcpy(). (Suggested by Sunil)
>>>
>>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>>> Cc: Damien Lespiau <damien.lespiau@intel.com>
>>> Cc: Imre Deak <imre.deak@intel.com>
>>> Cc: Sunil Kamath <sunil.kamath@intel.com>
>>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>>> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_drv.h  |  2 +-  drivers/gpu/drm/i915/intel_csr.c | 16 ++++------------
>>>   2 files changed, 5 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b94ada9..9d0ee58 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -742,7 +742,7 @@ enum csr_state {
>>>   struct intel_csr {
>>>   	const char *fw_path;
>>> -	__be32 *dmc_payload;
>>> +	uint32_t *dmc_payload;
>>>   	uint32_t dmc_fw_size;
>>>   	uint32_t mmio_count;
>>>   	uint32_t mmioaddr[8];
>>> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>>> index 6d8a7bf..ba1ae03 100644
>>> --- a/drivers/gpu/drm/i915/intel_csr.c
>>> +++ b/drivers/gpu/drm/i915/intel_csr.c
>>> @@ -244,7 +244,7 @@ void intel_csr_load_status_set(struct drm_i915_private *dev_priv,  void intel_csr_load_program(struct drm_device *dev)  {
>>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>>> -	__be32 *payload = dev_priv->csr.dmc_payload;
>>> +	u32 *payload = dev_priv->csr.dmc_payload;
>>>   	uint32_t i, fw_size;
>>>   	if (!IS_GEN9(dev)) {
>>> @@ -256,7 +256,7 @@ void intel_csr_load_program(struct drm_device *dev)
>>>   	fw_size = dev_priv->csr.dmc_fw_size;
>>>   	for (i = 0; i < fw_size; i++)
>>>   		I915_WRITE(CSR_PROGRAM_BASE + i * 4,
>>> -			(u32 __force)payload[i]);
>>> +			payload[i]);
>>>   	for (i = 0; i < dev_priv->csr.mmio_count; i++) {
>>>   		I915_WRITE(dev_priv->csr.mmioaddr[i],
>>> @@ -279,7 +279,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>>>   	char substepping = intel_get_substepping(dev);
>>>   	uint32_t dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes;
>>>   	uint32_t i;
>>> -	__be32 *dmc_payload;
>>> +	uint32_t *dmc_payload;
>>>   	bool fw_loaded = false;
>>>   	if (!fw) {
>>> @@ -375,15 +375,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>>>   	}
>>>   	dmc_payload = csr->dmc_payload;
>>> -	for (i = 0; i < dmc_header->fw_size; i++) {
>>> -		uint32_t *tmp = (u32 *)&fw->data[readcount + i * 4];
>>> -		/*
>>> -		 * The firmware payload is an array of 32 bit words stored in
>>> -		 * little-endian format in the firmware image and programmed
>>> -		 * as 32 bit big-endian format to memory.
>>> -		 */
>>> -		dmc_payload[i] = cpu_to_be32(*tmp);
>>> -	}
>>> +	memcpy(dmc_payload, &fw->data[readcount], nbytes);
>>>   	/* load csr program during system boot, as needed for DC states */
>>>   	intel_csr_load_program(dev);
>>> --
>>> 2.0.2
>>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [SKL-DMC-BUGFIX 1/5] drm/i915/gen9: Removed byte swapping for csr firmware

[Mika Kuoppala]

Daniel Vetter <daniel@ffwll.ch> writes:

> On Tue, Aug 04, 2015 at 11:25:40AM +0530, Animesh Manna wrote:
>> 
>> 
>> On 8/4/2015 9:16 AM, Nagaraju, Vathsala wrote:
>> >"This patch contains the changes to remove the byte swapping logic introduced with old dmc firmware."
>> >Which version of DMC need reversal  logic?  Atleast , 4,1.16,1.18 follow the same format.
>> 
>> Packaging of firmware binary completely changed after api version 1.0, so by old firmware I want to mean
>> the initial version before dmc 1.0.
>
> This kind of information really must be included in the commit message.
> Very likely someone with old firmware will bisect to this commit, and if
> you don't include that people need latest dmc firmware there will be
> confusion.
>
> Commit message _must_ be complete and contain everything that was
> discussed while reviewing and developing a patch.
>
> I added a note while merging the patch.
> -Daniel
>

Hi,

There is problem with gem_exec_nop throwing gpu hang with GPU HANG:
ecode 9:0:0xfffffffe on some skls. Looks like faster ones are
more suspectible.

My bisect pointed to this commit. As this looks like commit
to fix the loading, one could assume that at this point the firmware
started to actually do something.

So my failure hypothesis is that our driver runtime power well
bookkeeping and handover to dmc is racy. Lots of speculation here but
i suspect that during initialization, when the gpu is busy with the
workarounds and null state batch, handover to dmc happens
and something bad with it, like render side power toggles
while the dmc initializes?

I haven't yet tested the redesign series for cure/clue, but changing
the firmware from 1.19 to 1.21 makes it very hard to reproduce
the issue.

On a related note, we need the firmware version to be part of
error state. Also the driver could warn if not-new-enough firmware
is found, to atleast push the unlucky bisecter to a right
direction.

- Mika

>> 
>> >
>> >-----Original Message-----
>> >From: Manna, Animesh
>> >Sent: Monday, August 3, 2015 9:56 PM
>> >To: intel-gfx@lists.freedesktop.org
>> >Cc: Manna, Animesh; Vetter, Daniel; Lespiau, Damien; Deak, Imre; Kamath, Sunil; Nagaraju, Vathsala
>> >Subject: [SKL-DMC-BUGFIX 1/5] drm/i915/gen9: Removed byte swapping for csr firmware
>> >
>> >This patch contains the changes to remove the byte swapping logic introduced with old dmc firmware.
>> >While debugging PC10 entry issue for skylake found with latest dmc firmware version 1.18 without byte swapping dmc is working fine and able to enter PC10.
>> >
>> >v1: Initial version.
>> >
>> >v2: Corrected firmware size during memcpy(). (Suggested by Sunil)
>> >
>> >Cc: Daniel Vetter <daniel.vetter@intel.com>
>> >Cc: Damien Lespiau <damien.lespiau@intel.com>
>> >Cc: Imre Deak <imre.deak@intel.com>
>> >Cc: Sunil Kamath <sunil.kamath@intel.com>
>> >Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>> >Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
>> >---
>> >  drivers/gpu/drm/i915/i915_drv.h  |  2 +-  drivers/gpu/drm/i915/intel_csr.c | 16 ++++------------
>> >  2 files changed, 5 insertions(+), 13 deletions(-)
>> >
>> >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b94ada9..9d0ee58 100644
>> >--- a/drivers/gpu/drm/i915/i915_drv.h
>> >+++ b/drivers/gpu/drm/i915/i915_drv.h
>> >@@ -742,7 +742,7 @@ enum csr_state {
>> >  struct intel_csr {
>> >  	const char *fw_path;
>> >-	__be32 *dmc_payload;
>> >+	uint32_t *dmc_payload;
>> >  	uint32_t dmc_fw_size;
>> >  	uint32_t mmio_count;
>> >  	uint32_t mmioaddr[8];
>> >diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>> >index 6d8a7bf..ba1ae03 100644
>> >--- a/drivers/gpu/drm/i915/intel_csr.c
>> >+++ b/drivers/gpu/drm/i915/intel_csr.c
>> >@@ -244,7 +244,7 @@ void intel_csr_load_status_set(struct drm_i915_private *dev_priv,  void intel_csr_load_program(struct drm_device *dev)  {
>> >  	struct drm_i915_private *dev_priv = dev->dev_private;
>> >-	__be32 *payload = dev_priv->csr.dmc_payload;
>> >+	u32 *payload = dev_priv->csr.dmc_payload;
>> >  	uint32_t i, fw_size;
>> >  	if (!IS_GEN9(dev)) {
>> >@@ -256,7 +256,7 @@ void intel_csr_load_program(struct drm_device *dev)
>> >  	fw_size = dev_priv->csr.dmc_fw_size;
>> >  	for (i = 0; i < fw_size; i++)
>> >  		I915_WRITE(CSR_PROGRAM_BASE + i * 4,
>> >-			(u32 __force)payload[i]);
>> >+			payload[i]);
>> >  	for (i = 0; i < dev_priv->csr.mmio_count; i++) {
>> >  		I915_WRITE(dev_priv->csr.mmioaddr[i],
>> >@@ -279,7 +279,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>> >  	char substepping = intel_get_substepping(dev);
>> >  	uint32_t dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes;
>> >  	uint32_t i;
>> >-	__be32 *dmc_payload;
>> >+	uint32_t *dmc_payload;
>> >  	bool fw_loaded = false;
>> >  	if (!fw) {
>> >@@ -375,15 +375,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>> >  	}
>> >  	dmc_payload = csr->dmc_payload;
>> >-	for (i = 0; i < dmc_header->fw_size; i++) {
>> >-		uint32_t *tmp = (u32 *)&fw->data[readcount + i * 4];
>> >-		/*
>> >-		 * The firmware payload is an array of 32 bit words stored in
>> >-		 * little-endian format in the firmware image and programmed
>> >-		 * as 32 bit big-endian format to memory.
>> >-		 */
>> >-		dmc_payload[i] = cpu_to_be32(*tmp);
>> >-	}
>> >+	memcpy(dmc_payload, &fw->data[readcount], nbytes);
>> >  	/* load csr program during system boot, as needed for DC states */
>> >  	intel_csr_load_program(dev);
>> >--
>> >2.0.2
>> >
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[REGRESSION] drm/i915/gen9: Removed byte swapping for csr firmware

[Daniel Vetter]

On Fri, Sep 11, 2015 at 06:29:19PM +0300, Mika Kuoppala wrote:
> Daniel Vetter <daniel@ffwll.ch> writes:
> 
> > On Tue, Aug 04, 2015 at 11:25:40AM +0530, Animesh Manna wrote:
> >> 
> >> 
> >> On 8/4/2015 9:16 AM, Nagaraju, Vathsala wrote:
> >> >"This patch contains the changes to remove the byte swapping logic introduced with old dmc firmware."
> >> >Which version of DMC need reversal  logic?  Atleast , 4,1.16,1.18 follow the same format.
> >> 
> >> Packaging of firmware binary completely changed after api version 1.0, so by old firmware I want to mean
> >> the initial version before dmc 1.0.
> >
> > This kind of information really must be included in the commit message.
> > Very likely someone with old firmware will bisect to this commit, and if
> > you don't include that people need latest dmc firmware there will be
> > confusion.
> >
> > Commit message _must_ be complete and contain everything that was
> > discussed while reviewing and developing a patch.
> >
> > I added a note while merging the patch.
> > -Daniel
> >
> 
> Hi,
> 
> There is problem with gem_exec_nop throwing gpu hang with GPU HANG:
> ecode 9:0:0xfffffffe on some skls. Looks like faster ones are
> more suspectible.
> 
> My bisect pointed to this commit. As this looks like commit
> to fix the loading, one could assume that at this point the firmware
> started to actually do something.
> 
> So my failure hypothesis is that our driver runtime power well
> bookkeeping and handover to dmc is racy. Lots of speculation here but
> i suspect that during initialization, when the gpu is busy with the
> workarounds and null state batch, handover to dmc happens
> and something bad with it, like render side power toggles
> while the dmc initializes?
> 
> I haven't yet tested the redesign series for cure/clue, but changing
> the firmware from 1.19 to 1.21 makes it very hard to reproduce
> the issue.
> 
> On a related note, we need the firmware version to be part of
> error state. Also the driver could warn if not-new-enough firmware
> is found, to atleast push the unlucky bisecter to a right
> direction.

Does a revert remedy the problem?
-Daniel

> 
> - Mika
> 
> >> 
> >> >
> >> >-----Original Message-----
> >> >From: Manna, Animesh
> >> >Sent: Monday, August 3, 2015 9:56 PM
> >> >To: intel-gfx@lists.freedesktop.org
> >> >Cc: Manna, Animesh; Vetter, Daniel; Lespiau, Damien; Deak, Imre; Kamath, Sunil; Nagaraju, Vathsala
> >> >Subject: [SKL-DMC-BUGFIX 1/5] drm/i915/gen9: Removed byte swapping for csr firmware
> >> >
> >> >This patch contains the changes to remove the byte swapping logic introduced with old dmc firmware.
> >> >While debugging PC10 entry issue for skylake found with latest dmc firmware version 1.18 without byte swapping dmc is working fine and able to enter PC10.
> >> >
> >> >v1: Initial version.
> >> >
> >> >v2: Corrected firmware size during memcpy(). (Suggested by Sunil)
> >> >
> >> >Cc: Daniel Vetter <daniel.vetter@intel.com>
> >> >Cc: Damien Lespiau <damien.lespiau@intel.com>
> >> >Cc: Imre Deak <imre.deak@intel.com>
> >> >Cc: Sunil Kamath <sunil.kamath@intel.com>
> >> >Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> >> >Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> >> >---
> >> >  drivers/gpu/drm/i915/i915_drv.h  |  2 +-  drivers/gpu/drm/i915/intel_csr.c | 16 ++++------------
> >> >  2 files changed, 5 insertions(+), 13 deletions(-)
> >> >
> >> >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b94ada9..9d0ee58 100644
> >> >--- a/drivers/gpu/drm/i915/i915_drv.h
> >> >+++ b/drivers/gpu/drm/i915/i915_drv.h
> >> >@@ -742,7 +742,7 @@ enum csr_state {
> >> >  struct intel_csr {
> >> >  	const char *fw_path;
> >> >-	__be32 *dmc_payload;
> >> >+	uint32_t *dmc_payload;
> >> >  	uint32_t dmc_fw_size;
> >> >  	uint32_t mmio_count;
> >> >  	uint32_t mmioaddr[8];
> >> >diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> >> >index 6d8a7bf..ba1ae03 100644
> >> >--- a/drivers/gpu/drm/i915/intel_csr.c
> >> >+++ b/drivers/gpu/drm/i915/intel_csr.c
> >> >@@ -244,7 +244,7 @@ void intel_csr_load_status_set(struct drm_i915_private *dev_priv,  void intel_csr_load_program(struct drm_device *dev)  {
> >> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >> >-	__be32 *payload = dev_priv->csr.dmc_payload;
> >> >+	u32 *payload = dev_priv->csr.dmc_payload;
> >> >  	uint32_t i, fw_size;
> >> >  	if (!IS_GEN9(dev)) {
> >> >@@ -256,7 +256,7 @@ void intel_csr_load_program(struct drm_device *dev)
> >> >  	fw_size = dev_priv->csr.dmc_fw_size;
> >> >  	for (i = 0; i < fw_size; i++)
> >> >  		I915_WRITE(CSR_PROGRAM_BASE + i * 4,
> >> >-			(u32 __force)payload[i]);
> >> >+			payload[i]);
> >> >  	for (i = 0; i < dev_priv->csr.mmio_count; i++) {
> >> >  		I915_WRITE(dev_priv->csr.mmioaddr[i],
> >> >@@ -279,7 +279,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
> >> >  	char substepping = intel_get_substepping(dev);
> >> >  	uint32_t dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes;
> >> >  	uint32_t i;
> >> >-	__be32 *dmc_payload;
> >> >+	uint32_t *dmc_payload;
> >> >  	bool fw_loaded = false;
> >> >  	if (!fw) {
> >> >@@ -375,15 +375,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
> >> >  	}
> >> >  	dmc_payload = csr->dmc_payload;
> >> >-	for (i = 0; i < dmc_header->fw_size; i++) {
> >> >-		uint32_t *tmp = (u32 *)&fw->data[readcount + i * 4];
> >> >-		/*
> >> >-		 * The firmware payload is an array of 32 bit words stored in
> >> >-		 * little-endian format in the firmware image and programmed
> >> >-		 * as 32 bit big-endian format to memory.
> >> >-		 */
> >> >-		dmc_payload[i] = cpu_to_be32(*tmp);
> >> >-	}
> >> >+	memcpy(dmc_payload, &fw->data[readcount], nbytes);
> >> >  	/* load csr program during system boot, as needed for DC states */
> >> >  	intel_csr_load_program(dev);
> >> >--
> >> >2.0.2
> >> >
> >> 
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [REGRESSION] drm/i915/gen9: Removed byte swapping for csr firmware

[Mika Kuoppala]

Daniel Vetter <daniel@ffwll.ch> writes:

> On Fri, Sep 11, 2015 at 06:29:19PM +0300, Mika Kuoppala wrote:
>> Daniel Vetter <daniel@ffwll.ch> writes:
>> 
>> > On Tue, Aug 04, 2015 at 11:25:40AM +0530, Animesh Manna wrote:
>> >> 
>> >> 
>> >> On 8/4/2015 9:16 AM, Nagaraju, Vathsala wrote:
>> >> >"This patch contains the changes to remove the byte swapping logic introduced with old dmc firmware."
>> >> >Which version of DMC need reversal  logic?  Atleast , 4,1.16,1.18 follow the same format.
>> >> 
>> >> Packaging of firmware binary completely changed after api version 1.0, so by old firmware I want to mean
>> >> the initial version before dmc 1.0.
>> >
>> > This kind of information really must be included in the commit message.
>> > Very likely someone with old firmware will bisect to this commit, and if
>> > you don't include that people need latest dmc firmware there will be
>> > confusion.
>> >
>> > Commit message _must_ be complete and contain everything that was
>> > discussed while reviewing and developing a patch.
>> >
>> > I added a note while merging the patch.
>> > -Daniel
>> >
>> 
>> Hi,
>> 
>> There is problem with gem_exec_nop throwing gpu hang with GPU HANG:
>> ecode 9:0:0xfffffffe on some skls. Looks like faster ones are
>> more suspectible.
>> 
>> My bisect pointed to this commit. As this looks like commit
>> to fix the loading, one could assume that at this point the firmware
>> started to actually do something.
>> 
>> So my failure hypothesis is that our driver runtime power well
>> bookkeeping and handover to dmc is racy. Lots of speculation here but
>> i suspect that during initialization, when the gpu is busy with the
>> workarounds and null state batch, handover to dmc happens
>> and something bad with it, like render side power toggles
>> while the dmc initializes?
>> 
>> I haven't yet tested the redesign series for cure/clue, but changing
>> the firmware from 1.19 to 1.21 makes it very hard to reproduce
>> the issue.
>> 
>> On a related note, we need the firmware version to be part of
>> error state. Also the driver could warn if not-new-enough firmware
>> is found, to atleast push the unlucky bisecter to a right
>> direction.
>
> Does a revert remedy the problem?
> -Daniel

Yes. By making the loading fail and firmware not starting.

With current knowledge I would say that upgrading from 1.19
to 1.21 is better than revert.

-Mika

>
>> 
>> - Mika
>> 
>> >> 
>> >> >
>> >> >-----Original Message-----
>> >> >From: Manna, Animesh
>> >> >Sent: Monday, August 3, 2015 9:56 PM
>> >> >To: intel-gfx@lists.freedesktop.org
>> >> >Cc: Manna, Animesh; Vetter, Daniel; Lespiau, Damien; Deak, Imre; Kamath, Sunil; Nagaraju, Vathsala
>> >> >Subject: [SKL-DMC-BUGFIX 1/5] drm/i915/gen9: Removed byte swapping for csr firmware
>> >> >
>> >> >This patch contains the changes to remove the byte swapping logic introduced with old dmc firmware.
>> >> >While debugging PC10 entry issue for skylake found with latest dmc firmware version 1.18 without byte swapping dmc is working fine and able to enter PC10.
>> >> >
>> >> >v1: Initial version.
>> >> >
>> >> >v2: Corrected firmware size during memcpy(). (Suggested by Sunil)
>> >> >
>> >> >Cc: Daniel Vetter <daniel.vetter@intel.com>
>> >> >Cc: Damien Lespiau <damien.lespiau@intel.com>
>> >> >Cc: Imre Deak <imre.deak@intel.com>
>> >> >Cc: Sunil Kamath <sunil.kamath@intel.com>
>> >> >Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>> >> >Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
>> >> >---
>> >> >  drivers/gpu/drm/i915/i915_drv.h  |  2 +-  drivers/gpu/drm/i915/intel_csr.c | 16 ++++------------
>> >> >  2 files changed, 5 insertions(+), 13 deletions(-)
>> >> >
>> >> >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b94ada9..9d0ee58 100644
>> >> >--- a/drivers/gpu/drm/i915/i915_drv.h
>> >> >+++ b/drivers/gpu/drm/i915/i915_drv.h
>> >> >@@ -742,7 +742,7 @@ enum csr_state {
>> >> >  struct intel_csr {
>> >> >  	const char *fw_path;
>> >> >-	__be32 *dmc_payload;
>> >> >+	uint32_t *dmc_payload;
>> >> >  	uint32_t dmc_fw_size;
>> >> >  	uint32_t mmio_count;
>> >> >  	uint32_t mmioaddr[8];
>> >> >diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>> >> >index 6d8a7bf..ba1ae03 100644
>> >> >--- a/drivers/gpu/drm/i915/intel_csr.c
>> >> >+++ b/drivers/gpu/drm/i915/intel_csr.c
>> >> >@@ -244,7 +244,7 @@ void intel_csr_load_status_set(struct drm_i915_private *dev_priv,  void intel_csr_load_program(struct drm_device *dev)  {
>> >> >  	struct drm_i915_private *dev_priv = dev->dev_private;
>> >> >-	__be32 *payload = dev_priv->csr.dmc_payload;
>> >> >+	u32 *payload = dev_priv->csr.dmc_payload;
>> >> >  	uint32_t i, fw_size;
>> >> >  	if (!IS_GEN9(dev)) {
>> >> >@@ -256,7 +256,7 @@ void intel_csr_load_program(struct drm_device *dev)
>> >> >  	fw_size = dev_priv->csr.dmc_fw_size;
>> >> >  	for (i = 0; i < fw_size; i++)
>> >> >  		I915_WRITE(CSR_PROGRAM_BASE + i * 4,
>> >> >-			(u32 __force)payload[i]);
>> >> >+			payload[i]);
>> >> >  	for (i = 0; i < dev_priv->csr.mmio_count; i++) {
>> >> >  		I915_WRITE(dev_priv->csr.mmioaddr[i],
>> >> >@@ -279,7 +279,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>> >> >  	char substepping = intel_get_substepping(dev);
>> >> >  	uint32_t dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes;
>> >> >  	uint32_t i;
>> >> >-	__be32 *dmc_payload;
>> >> >+	uint32_t *dmc_payload;
>> >> >  	bool fw_loaded = false;
>> >> >  	if (!fw) {
>> >> >@@ -375,15 +375,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>> >> >  	}
>> >> >  	dmc_payload = csr->dmc_payload;
>> >> >-	for (i = 0; i < dmc_header->fw_size; i++) {
>> >> >-		uint32_t *tmp = (u32 *)&fw->data[readcount + i * 4];
>> >> >-		/*
>> >> >-		 * The firmware payload is an array of 32 bit words stored in
>> >> >-		 * little-endian format in the firmware image and programmed
>> >> >-		 * as 32 bit big-endian format to memory.
>> >> >-		 */
>> >> >-		dmc_payload[i] = cpu_to_be32(*tmp);
>> >> >-	}
>> >> >+	memcpy(dmc_payload, &fw->data[readcount], nbytes);
>> >> >  	/* load csr program during system boot, as needed for DC states */
>> >> >  	intel_csr_load_program(dev);
>> >> >--
>> >> >2.0.2
>> >> >
>> >> 
>> >> _______________________________________________
>> >> Intel-gfx mailing list
>> >> Intel-gfx@lists.freedesktop.org
>> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >
>> > -- 
>> > Daniel Vetter
>> > Software Engineer, Intel Corporation
>> > http://blog.ffwll.ch
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [SKL-DMC-BUGFIX 1/5] drm/i915/gen9: Removed byte swapping for csr firmware

[Sunil Kamath]

On Monday 03 August 2015 09:55 PM, Animesh Manna wrote:
> This patch contains the changes to remove the byte
> swapping logic introduced with old dmc firmware.
> While debugging PC10 entry issue for skylake found
> with latest dmc firmware version 1.18 without byte
> swapping dmc is working fine and able to enter PC10.
>
> v1: Initial version.
>
> v2: Corrected firmware size during memcpy(). (Suggested by Sunil)
>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Sunil Kamath <sunil.kamath@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h  |  2 +-
>   drivers/gpu/drm/i915/intel_csr.c | 16 ++++------------
>   2 files changed, 5 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b94ada9..9d0ee58 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -742,7 +742,7 @@ enum csr_state {
>   
>   struct intel_csr {
>   	const char *fw_path;
> -	__be32 *dmc_payload;
> +	uint32_t *dmc_payload;
>   	uint32_t dmc_fw_size;
>   	uint32_t mmio_count;
>   	uint32_t mmioaddr[8];
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index 6d8a7bf..ba1ae03 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -244,7 +244,7 @@ void intel_csr_load_status_set(struct drm_i915_private *dev_priv,
>   void intel_csr_load_program(struct drm_device *dev)
>   {
>   	struct drm_i915_private *dev_priv = dev->dev_private;
> -	__be32 *payload = dev_priv->csr.dmc_payload;
> +	u32 *payload = dev_priv->csr.dmc_payload;
>   	uint32_t i, fw_size;
>   
>   	if (!IS_GEN9(dev)) {
> @@ -256,7 +256,7 @@ void intel_csr_load_program(struct drm_device *dev)
>   	fw_size = dev_priv->csr.dmc_fw_size;
>   	for (i = 0; i < fw_size; i++)
>   		I915_WRITE(CSR_PROGRAM_BASE + i * 4,
> -			(u32 __force)payload[i]);
> +			payload[i]);
>   
>   	for (i = 0; i < dev_priv->csr.mmio_count; i++) {
>   		I915_WRITE(dev_priv->csr.mmioaddr[i],
> @@ -279,7 +279,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>   	char substepping = intel_get_substepping(dev);
>   	uint32_t dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes;
>   	uint32_t i;
> -	__be32 *dmc_payload;
> +	uint32_t *dmc_payload;
>   	bool fw_loaded = false;
>   
>   	if (!fw) {
> @@ -375,15 +375,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>   	}
>   
>   	dmc_payload = csr->dmc_payload;
> -	for (i = 0; i < dmc_header->fw_size; i++) {
> -		uint32_t *tmp = (u32 *)&fw->data[readcount + i * 4];
> -		/*
> -		 * The firmware payload is an array of 32 bit words stored in
> -		 * little-endian format in the firmware image and programmed
> -		 * as 32 bit big-endian format to memory.
> -		 */
> -		dmc_payload[i] = cpu_to_be32(*tmp);
> -	}
> +	memcpy(dmc_payload, &fw->data[readcount], nbytes);
>   
>   	/* load csr program during system boot, as needed for DC states */
>   	intel_csr_load_program(dev);
Thanks for addressing review comments.
Small change can be done in Gerrit message to generalize the issue to fw 
1.0+. Else fine.

Valid bug fix.

Reviewed-by: A.Sunil Kamath <sunil.kamath@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[SKL-DMC-BUGFIX 2/5] drm/i915/skl: Making DC6 entry is the last call in suspend flow.

[Animesh Manna]

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: commit message updated based on comment from Vathsala.

Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c         | 12 ++++++------
 drivers/gpu/drm/i915/intel_drv.h        |  2 ++
 drivers/gpu/drm/i915/intel_runtime_pm.c | 33 ++++++++-------------------------
 3 files changed, 16 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 0d6775a..e1d0102 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1017,14 +1017,11 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv)
 {
 	/* Enabling DC6 is not a hard requirement to enter runtime D3 */
 
-	/*
-	 * This is to ensure that CSR isn't identified as loaded before
-	 * CSR-loading program is called during runtime-resume.
-	 */
-	intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED);
-
 	skl_uninit_cdclk(dev_priv);
 
+	if (intel_csr_load_status_get(dev_priv) == FW_LOADED)
+		skl_enable_dc6(dev_priv);
+
 	return 0;
 }
 
@@ -1071,6 +1068,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 47cef0e..06f346f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1117,6 +1117,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 6393b76..c660245 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -532,7 +532,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;
 
@@ -549,7 +549,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;
 
@@ -610,10 +610,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.
@@ -621,8 +621,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);
@@ -642,24 +640,9 @@ 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)) &&
-				power_well->data == SKL_DISP_PW_2) {
-				enum csr_state state;
-				/* 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)
-					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);
-			}
+			if (GEN9_ENABLE_DC5(dev) &&
+				power_well->data == SKL_DISP_PW_2)
+					gen9_enable_dc5(dev_priv);
 		}
 	}
 
-- 
2.0.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [SKL-DMC-BUGFIX 2/5] drm/i915/skl: Making DC6 entry is the last call in suspend flow.

[Sunil Kamath]

On Monday 03 August 2015 09:55 PM, 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: commit message updated based on comment from Vathsala.
>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Sunil Kamath <sunil.kamath@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.c         | 12 ++++++------
>   drivers/gpu/drm/i915/intel_drv.h        |  2 ++
>   drivers/gpu/drm/i915/intel_runtime_pm.c | 33 ++++++++-------------------------
>   3 files changed, 16 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 0d6775a..e1d0102 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1017,14 +1017,11 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv)
>   {
>   	/* Enabling DC6 is not a hard requirement to enter runtime D3 */
>   
> -	/*
> -	 * This is to ensure that CSR isn't identified as loaded before
> -	 * CSR-loading program is called during runtime-resume.
> -	 */
> -	intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED);
> -
>   	skl_uninit_cdclk(dev_priv);
>   
> +	if (intel_csr_load_status_get(dev_priv) == FW_LOADED)
> +		skl_enable_dc6(dev_priv);
> +
>   	return 0;
>   }
>   
> @@ -1071,6 +1068,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 47cef0e..06f346f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1117,6 +1117,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 6393b76..c660245 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -532,7 +532,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;
>   
> @@ -549,7 +549,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;
>   
> @@ -610,10 +610,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.
> @@ -621,8 +621,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);
> @@ -642,24 +640,9 @@ 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)) &&
> -				power_well->data == SKL_DISP_PW_2) {
> -				enum csr_state state;
> -				/* 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)
> -					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);
> -			}
> +			if (GEN9_ENABLE_DC5(dev) &&
> +				power_well->data == SKL_DISP_PW_2)
> +					gen9_enable_dc5(dev_priv);
>   		}
>   	}
>   
Same comment as before:

This is really right change.
With this we will go back to our original design.
Deepest possible display state will be triggered by respective suspend 
complete.
for example in BXT we trigger DC9 from bxt_suspend_complete and works 
fine with rpm integrated.
Same case here for SKL for DC6 which uses DMC.

Right bug fix.

Reviewed-by: A.Sunil Kamath <sunil.kamath@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [SKL-DMC-BUGFIX 2/5] drm/i915/skl: Making DC6 entry is the last call in suspend flow.

[Daniel Vetter]

On Tue, Aug 04, 2015 at 04:55:59PM +0530, Sunil Kamath wrote:
> On Monday 03 August 2015 09:55 PM, 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: commit message updated based on comment from Vathsala.
> >
> >Cc: Daniel Vetter <daniel.vetter@intel.com>
> >Cc: Damien Lespiau <damien.lespiau@intel.com>
> >Cc: Imre Deak <imre.deak@intel.com>
> >Cc: Sunil Kamath <sunil.kamath@intel.com>
> >Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> >Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> >Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> >---
> >  drivers/gpu/drm/i915/i915_drv.c         | 12 ++++++------
> >  drivers/gpu/drm/i915/intel_drv.h        |  2 ++
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 33 ++++++++-------------------------
> >  3 files changed, 16 insertions(+), 31 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >index 0d6775a..e1d0102 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.c
> >+++ b/drivers/gpu/drm/i915/i915_drv.c
> >@@ -1017,14 +1017,11 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv)
> >  {
> >  	/* Enabling DC6 is not a hard requirement to enter runtime D3 */
> >-	/*
> >-	 * This is to ensure that CSR isn't identified as loaded before
> >-	 * CSR-loading program is called during runtime-resume.
> >-	 */
> >-	intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED);
> >-
> >  	skl_uninit_cdclk(dev_priv);
> >+	if (intel_csr_load_status_get(dev_priv) == FW_LOADED)
> >+		skl_enable_dc6(dev_priv);
> >+
> >  	return 0;
> >  }
> >@@ -1071,6 +1068,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 47cef0e..06f346f 100644
> >--- a/drivers/gpu/drm/i915/intel_drv.h
> >+++ b/drivers/gpu/drm/i915/intel_drv.h
> >@@ -1117,6 +1117,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 6393b76..c660245 100644
> >--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> >+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> >@@ -532,7 +532,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;
> >@@ -549,7 +549,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;
> >@@ -610,10 +610,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.
> >@@ -621,8 +621,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);
> >@@ -642,24 +640,9 @@ 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)) &&
> >-				power_well->data == SKL_DISP_PW_2) {
> >-				enum csr_state state;
> >-				/* 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)
> >-					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);
> >-			}
> >+			if (GEN9_ENABLE_DC5(dev) &&
> >+				power_well->data == SKL_DISP_PW_2)
> >+					gen9_enable_dc5(dev_priv);
> >  		}
> >  	}
> Same comment as before:
> 
> This is really right change.
> With this we will go back to our original design.

If this reverts something ("go back to our original design") then we need
to mention the patches this undoes in the commit message.
-Daniel

> Deepest possible display state will be triggered by respective suspend
> complete.
> for example in BXT we trigger DC9 from bxt_suspend_complete and works fine
> with rpm integrated.
> Same case here for SKL for DC6 which uses DMC.
> 
> Right bug fix.
> 
> Reviewed-by: A.Sunil Kamath <sunil.kamath@intel.com>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [SKL-DMC-BUGFIX 2/5] drm/i915/skl: Making DC6 entry is the last call in suspend flow.

[Daniel Vetter]

On Mon, Aug 03, 2015 at 09:55:33PM +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.
> 
> v1: Initial version.
> 
> v2: commit message updated based on comment from Vathsala.
> 
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Sunil Kamath <sunil.kamath@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c         | 12 ++++++------
>  drivers/gpu/drm/i915/intel_drv.h        |  2 ++
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 33 ++++++++-------------------------
>  3 files changed, 16 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 0d6775a..e1d0102 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1017,14 +1017,11 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv)
>  {
>  	/* Enabling DC6 is not a hard requirement to enter runtime D3 */
>  
> -	/*
> -	 * This is to ensure that CSR isn't identified as loaded before
> -	 * CSR-loading program is called during runtime-resume.
> -	 */
> -	intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED);

Seems like an unrelated hunk. Separate patch (in the dmc loader rework
series) or an explanation why we need this.

> -
>  	skl_uninit_cdclk(dev_priv);
>  
> +	if (intel_csr_load_status_get(dev_priv) == FW_LOADED)
> +		skl_enable_dc6(dev_priv);
> +
>  	return 0;
>  }
>  
> @@ -1071,6 +1068,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 47cef0e..06f346f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1117,6 +1117,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 6393b76..c660245 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -532,7 +532,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;
>  
> @@ -549,7 +549,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;

Everything above seems to roughly be matching your patch description, but
not perfectly: You talk about suspend flow but also touch resume flow.

But the hunks below are completely unexplained magic afaict. Either this
needs a separate patch or it needs seriously more explanation of what's
going on.

>  
> @@ -610,10 +610,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.
> @@ -621,8 +621,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);
> @@ -642,24 +640,9 @@ 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)) &&
> -				power_well->data == SKL_DISP_PW_2) {
> -				enum csr_state state;
> -				/* 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)
> -					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);
> -			}
> +			if (GEN9_ENABLE_DC5(dev) &&
> +				power_well->data == SKL_DISP_PW_2)
> +					gen9_enable_dc5(dev_priv);
>  		}
>  	}
>  
> -- 
> 2.0.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [SKL-DMC-BUGFIX 2/5] drm/i915/skl: Making DC6 entry is the last call in suspend flow.

[Animesh Manna]

On 8/5/2015 2:35 PM, Daniel Vetter wrote:
> On Mon, Aug 03, 2015 at 09:55:33PM +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.
>>
>> v1: Initial version.
>>
>> v2: commit message updated based on comment from Vathsala.
>>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Damien Lespiau <damien.lespiau@intel.com>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Cc: Sunil Kamath <sunil.kamath@intel.com>
>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
>> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.c         | 12 ++++++------
>>   drivers/gpu/drm/i915/intel_drv.h        |  2 ++
>>   drivers/gpu/drm/i915/intel_runtime_pm.c | 33 ++++++++-------------------------
>>   3 files changed, 16 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 0d6775a..e1d0102 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -1017,14 +1017,11 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv)
>>   {
>>   	/* Enabling DC6 is not a hard requirement to enter runtime D3 */
>>   
>> -	/*
>> -	 * This is to ensure that CSR isn't identified as loaded before
>> -	 * CSR-loading program is called during runtime-resume.
>> -	 */
>> -	intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED);
> Seems like an unrelated hunk. Separate patch (in the dmc loader rework
> series) or an explanation why we need this.

In the same skl_suspend_complete() later we are checking if firmware is loaded,
based on that we trigger dc6, so the above hunk has to be removed in this patch.
I will add explanation in my next patchset.
On the other hand firmware team confirmed that one time firmware loading
during driver loading is sufficient, no need to load firmware in
csr-address-space every suspend (dc6 entry) - resume (dc6 exit) flow, dmc will
take care of it which eliminate any chance of regression.

- Animesh

>
>> -
>>   	skl_uninit_cdclk(dev_priv);
>>   
>> +	if (intel_csr_load_status_get(dev_priv) == FW_LOADED)
>> +		skl_enable_dc6(dev_priv);
>> +
>>   	return 0;
>>   }
>>   
>> @@ -1071,6 +1068,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 47cef0e..06f346f 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1117,6 +1117,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 6393b76..c660245 100644
>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> @@ -532,7 +532,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;
>>   
>> @@ -549,7 +549,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;
> Everything above seems to roughly be matching your patch description, but
> not perfectly: You talk about suspend flow but also touch resume flow.
>
> But the hunks below are completely unexplained magic afaict. Either this
> needs a separate patch or it needs seriously more explanation of what's
> going on.
>
>>   
>> @@ -610,10 +610,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.
>> @@ -621,8 +621,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);
>> @@ -642,24 +640,9 @@ 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)) &&
>> -				power_well->data == SKL_DISP_PW_2) {
>> -				enum csr_state state;
>> -				/* 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)
>> -					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);
>> -			}
>> +			if (GEN9_ENABLE_DC5(dev) &&
>> +				power_well->data == SKL_DISP_PW_2)
>> +					gen9_enable_dc5(dev_priv);
>>   		}
>>   	}
>>   
>> -- 
>> 2.0.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/5] drm/i915/skl: Making DC6 entry is the last call in suspend flow.

[Animesh Manna]

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. 

Before triggering dc6 a condition check added to check firmware
loading status. Removed the set call for firmware loading state
to uninitilaze. As dmc will handle firmware loading in dc6 exit
flow, so we do not need the uninitialize call at all.

v1: Initial version.

v2: commit message updated based on comment from Vathsala.

v3: Added explanation in commit description for removed
intel_csr_load_status_set call in skl_suspend_complete()
(Suggested by Daniel).

Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c         | 12 ++++++------
 drivers/gpu/drm/i915/intel_drv.h        |  2 ++
 drivers/gpu/drm/i915/intel_runtime_pm.c | 33 ++++++++-------------------------
 3 files changed, 16 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 0d6775a..e1d0102 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1017,14 +1017,11 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv)
 {
 	/* Enabling DC6 is not a hard requirement to enter runtime D3 */
 
-	/*
-	 * This is to ensure that CSR isn't identified as loaded before
-	 * CSR-loading program is called during runtime-resume.
-	 */
-	intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED);
-
 	skl_uninit_cdclk(dev_priv);
 
+	if (intel_csr_load_status_get(dev_priv) == FW_LOADED)
+		skl_enable_dc6(dev_priv);
+
 	return 0;
 }
 
@@ -1071,6 +1068,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 47cef0e..06f346f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1117,6 +1117,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 6393b76..c660245 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -532,7 +532,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;
 
@@ -549,7 +549,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;
 
@@ -610,10 +610,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.
@@ -621,8 +621,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);
@@ -642,24 +640,9 @@ 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)) &&
-				power_well->data == SKL_DISP_PW_2) {
-				enum csr_state state;
-				/* 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)
-					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);
-			}
+			if (GEN9_ENABLE_DC5(dev) &&
+				power_well->data == SKL_DISP_PW_2)
+					gen9_enable_dc5(dev_priv);
 		}
 	}
 
-- 
2.0.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [SKL-DMC-BUGFIX 2/5] drm/i915/skl: Making DC6 entry is the last call in suspend flow.

[Daniel Vetter]

On Thu, Aug 06, 2015 at 02:47:22PM +0530, Animesh Manna wrote:
> 
> 
> On 8/5/2015 2:35 PM, Daniel Vetter wrote:
> >On Mon, Aug 03, 2015 at 09:55:33PM +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.
> >>
> >>v1: Initial version.
> >>
> >>v2: commit message updated based on comment from Vathsala.
> >>
> >>Cc: Daniel Vetter <daniel.vetter@intel.com>
> >>Cc: Damien Lespiau <damien.lespiau@intel.com>
> >>Cc: Imre Deak <imre.deak@intel.com>
> >>Cc: Sunil Kamath <sunil.kamath@intel.com>
> >>Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> >>Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> >>Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> >>---
> >>  drivers/gpu/drm/i915/i915_drv.c         | 12 ++++++------
> >>  drivers/gpu/drm/i915/intel_drv.h        |  2 ++
> >>  drivers/gpu/drm/i915/intel_runtime_pm.c | 33 ++++++++-------------------------
> >>  3 files changed, 16 insertions(+), 31 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >>index 0d6775a..e1d0102 100644
> >>--- a/drivers/gpu/drm/i915/i915_drv.c
> >>+++ b/drivers/gpu/drm/i915/i915_drv.c
> >>@@ -1017,14 +1017,11 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv)
> >>  {
> >>  	/* Enabling DC6 is not a hard requirement to enter runtime D3 */
> >>-	/*
> >>-	 * This is to ensure that CSR isn't identified as loaded before
> >>-	 * CSR-loading program is called during runtime-resume.
> >>-	 */
> >>-	intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED);
> >Seems like an unrelated hunk. Separate patch (in the dmc loader rework
> >series) or an explanation why we need this.
> 
> In the same skl_suspend_complete() later we are checking if firmware is loaded,
> based on that we trigger dc6, so the above hunk has to be removed in this patch.
> I will add explanation in my next patchset.

I know that later on we'll replace this with something else, but you can't
remove old code before the new code is there. And you can't do changes
like this here in unrelated patches.

> On the other hand firmware team confirmed that one time firmware loading
> during driver loading is sufficient, no need to load firmware in
> csr-address-space every suspend (dc6 entry) - resume (dc6 exit) flow, dmc will
> take care of it which eliminate any chance of regression.

And what about hibernate?
-Daniel
> 
> - Animesh
> 
> >
> >>-
> >>  	skl_uninit_cdclk(dev_priv);
> >>+	if (intel_csr_load_status_get(dev_priv) == FW_LOADED)
> >>+		skl_enable_dc6(dev_priv);
> >>+
> >>  	return 0;
> >>  }
> >>@@ -1071,6 +1068,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 47cef0e..06f346f 100644
> >>--- a/drivers/gpu/drm/i915/intel_drv.h
> >>+++ b/drivers/gpu/drm/i915/intel_drv.h
> >>@@ -1117,6 +1117,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 6393b76..c660245 100644
> >>--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> >>+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> >>@@ -532,7 +532,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;
> >>@@ -549,7 +549,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;
> >Everything above seems to roughly be matching your patch description, but
> >not perfectly: You talk about suspend flow but also touch resume flow.
> >
> >But the hunks below are completely unexplained magic afaict. Either this
> >needs a separate patch or it needs seriously more explanation of what's
> >going on.
> >
> >>@@ -610,10 +610,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.
> >>@@ -621,8 +621,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);
> >>@@ -642,24 +640,9 @@ 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)) &&
> >>-				power_well->data == SKL_DISP_PW_2) {
> >>-				enum csr_state state;
> >>-				/* 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)
> >>-					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);
> >>-			}
> >>+			if (GEN9_ENABLE_DC5(dev) &&
> >>+				power_well->data == SKL_DISP_PW_2)
> >>+					gen9_enable_dc5(dev_priv);
> >>  		}
> >>  	}
> >>-- 
> >>2.0.2
> >>
> >>_______________________________________________
> >>Intel-gfx mailing list
> >>Intel-gfx@lists.freedesktop.org
> >>http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

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

Re: [SKL-DMC-BUGFIX 2/5] drm/i915/skl: Making DC6 entry is the last call in suspend flow.

[Animesh Manna]

On 8/6/2015 6:48 PM, Daniel Vetter wrote:
> On Thu, Aug 06, 2015 at 02:47:22PM +0530, Animesh Manna wrote:
>>
>> On 8/5/2015 2:35 PM, Daniel Vetter wrote:
>>> On Mon, Aug 03, 2015 at 09:55:33PM +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.
>>>>
>>>> v1: Initial version.
>>>>
>>>> v2: commit message updated based on comment from Vathsala.
>>>>
>>>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>>>> Cc: Damien Lespiau <damien.lespiau@intel.com>
>>>> Cc: Imre Deak <imre.deak@intel.com>
>>>> Cc: Sunil Kamath <sunil.kamath@intel.com>
>>>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>>>> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
>>>> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/i915_drv.c         | 12 ++++++------
>>>>   drivers/gpu/drm/i915/intel_drv.h        |  2 ++
>>>>   drivers/gpu/drm/i915/intel_runtime_pm.c | 33 ++++++++-------------------------
>>>>   3 files changed, 16 insertions(+), 31 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>>>> index 0d6775a..e1d0102 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>>> @@ -1017,14 +1017,11 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv)
>>>>   {
>>>>   	/* Enabling DC6 is not a hard requirement to enter runtime D3 */
>>>> -	/*
>>>> -	 * This is to ensure that CSR isn't identified as loaded before
>>>> -	 * CSR-loading program is called during runtime-resume.
>>>> -	 */
>>>> -	intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED);
>>> Seems like an unrelated hunk. Separate patch (in the dmc loader rework
>>> series) or an explanation why we need this.
>> In the same skl_suspend_complete() later we are checking if firmware is loaded,
>> based on that we trigger dc6, so the above hunk has to be removed in this patch.
> I know that later on we'll replace this with something else, but you can't
> remove old code before the new code is there. And you can't do changes
> like this here in unrelated patches.

code snippet added in this patch:
+	if (intel_csr_load_status_get(dev_priv) == FW_LOADED)
+		skl_enable_dc6(dev_priv);

We can add uninitilized call here after this which will be related changes.
After dmc redesign will remove it completely the function call for csr uninitialization.

>
>> On the other hand firmware team confirmed that one time firmware loading
>> during driver loading is sufficient, no need to load firmware in
>> csr-address-space every suspend (dc6 entry) - resume (dc6 exit) flow, dmc will
>> take care of it which eliminate any chance of regression.
> And what about hibernate?

Yes, during hibernate I assumed os will restore the ram content from disk.
But specially for csr program we need to load it again. Thanks for the pointer.


-Animesh

> -Daniel
>> - Animesh
>>
>>>> -
>>>>   	skl_uninit_cdclk(dev_priv);
>>>> +	if (intel_csr_load_status_get(dev_priv) == FW_LOADED)
>>>> +		skl_enable_dc6(dev_priv);
>>>> +
>>>>   	return 0;
>>>>   }
>>>> @@ -1071,6 +1068,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 47cef0e..06f346f 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -1117,6 +1117,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 6393b76..c660245 100644
>>>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>>>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>>> @@ -532,7 +532,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;
>>>> @@ -549,7 +549,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;
>>> Everything above seems to roughly be matching your patch description, but
>>> not perfectly: You talk about suspend flow but also touch resume flow.
>>>
>>> But the hunks below are completely unexplained magic afaict. Either this
>>> needs a separate patch or it needs seriously more explanation of what's
>>> going on.
>>>
>>>> @@ -610,10 +610,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.
>>>> @@ -621,8 +621,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);
>>>> @@ -642,24 +640,9 @@ 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)) &&
>>>> -				power_well->data == SKL_DISP_PW_2) {
>>>> -				enum csr_state state;
>>>> -				/* 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)
>>>> -					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);
>>>> -			}
>>>> +			if (GEN9_ENABLE_DC5(dev) &&
>>>> +				power_well->data == SKL_DISP_PW_2)
>>>> +					gen9_enable_dc5(dev_priv);
>>>>   		}
>>>>   	}
>>>> -- 
>>>> 2.0.2
>>>>
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx@lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [SKL-DMC-BUGFIX 2/5] drm/i915/skl: Making DC6 entry is the last call in suspend flow.

[Daniel Vetter]

On Thu, Aug 06, 2015 at 08:08:58PM +0530, Animesh Manna wrote:
> 
> 
> On 8/6/2015 6:48 PM, Daniel Vetter wrote:
> >On Thu, Aug 06, 2015 at 02:47:22PM +0530, Animesh Manna wrote:
> >>
> >>On 8/5/2015 2:35 PM, Daniel Vetter wrote:
> >>>On Mon, Aug 03, 2015 at 09:55:33PM +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.
> >>>>
> >>>>v1: Initial version.
> >>>>
> >>>>v2: commit message updated based on comment from Vathsala.
> >>>>
> >>>>Cc: Daniel Vetter <daniel.vetter@intel.com>
> >>>>Cc: Damien Lespiau <damien.lespiau@intel.com>
> >>>>Cc: Imre Deak <imre.deak@intel.com>
> >>>>Cc: Sunil Kamath <sunil.kamath@intel.com>
> >>>>Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> >>>>Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> >>>>Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> >>>>---
> >>>>  drivers/gpu/drm/i915/i915_drv.c         | 12 ++++++------
> >>>>  drivers/gpu/drm/i915/intel_drv.h        |  2 ++
> >>>>  drivers/gpu/drm/i915/intel_runtime_pm.c | 33 ++++++++-------------------------
> >>>>  3 files changed, 16 insertions(+), 31 deletions(-)
> >>>>
> >>>>diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >>>>index 0d6775a..e1d0102 100644
> >>>>--- a/drivers/gpu/drm/i915/i915_drv.c
> >>>>+++ b/drivers/gpu/drm/i915/i915_drv.c
> >>>>@@ -1017,14 +1017,11 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv)
> >>>>  {
> >>>>  	/* Enabling DC6 is not a hard requirement to enter runtime D3 */
> >>>>-	/*
> >>>>-	 * This is to ensure that CSR isn't identified as loaded before
> >>>>-	 * CSR-loading program is called during runtime-resume.
> >>>>-	 */
> >>>>-	intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED);
> >>>Seems like an unrelated hunk. Separate patch (in the dmc loader rework
> >>>series) or an explanation why we need this.
> >>In the same skl_suspend_complete() later we are checking if firmware is loaded,
> >>based on that we trigger dc6, so the above hunk has to be removed in this patch.
> >I know that later on we'll replace this with something else, but you can't
> >remove old code before the new code is there. And you can't do changes
> >like this here in unrelated patches.
> 
> code snippet added in this patch:
> +	if (intel_csr_load_status_get(dev_priv) == FW_LOADED)
> +		skl_enable_dc6(dev_priv);
> 
> We can add uninitilized call here after this which will be related changes.
> After dmc redesign will remove it completely the function call for csr uninitialization.

Oh right I was getting a bit confused here, that change does make sense.
But the patch should explain better why/how this all gets changed.
Obviously if we keep on setting status to UNITIALIZED then this check here
will always fall over.

Might be good to split up this patch into parts
1. Remove the UNITIALIZED from suspend path since dmc stays around. Also
handle re-loading on hibernate resume correctly.

2. Adjust ordering between dc5 and dc6 for dmc.

I think there's still more parts in this patch, e.g. you also remove the
wait_for ...

> >>On the other hand firmware team confirmed that one time firmware loading
> >>during driver loading is sufficient, no need to load firmware in
> >>csr-address-space every suspend (dc6 entry) - resume (dc6 exit) flow, dmc will
> >>take care of it which eliminate any chance of regression.
> >And what about hibernate?
> 
> Yes, during hibernate I assumed os will restore the ram content from disk.
> But specially for csr program we need to load it again. Thanks for the pointer.

Hibernate is really tricky, so please try to test that fully. The big risk
is resume:
- first we load a normal kernel, including i915 and everything
- then that first kernel loads the hibernate image
- then all hw gets shut down like with a normal suspend call
- then we resume everything again

Given all this we might need to check the hw state itself whether dmc is
loaded already and only load it if that's the case. Otherwise bad stuff
might happen.
-Daniel

> 
> 
> -Animesh
> 
> >-Daniel
> >>- Animesh
> >>
> >>>>-
> >>>>  	skl_uninit_cdclk(dev_priv);
> >>>>+	if (intel_csr_load_status_get(dev_priv) == FW_LOADED)
> >>>>+		skl_enable_dc6(dev_priv);
> >>>>+
> >>>>  	return 0;
> >>>>  }
> >>>>@@ -1071,6 +1068,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 47cef0e..06f346f 100644
> >>>>--- a/drivers/gpu/drm/i915/intel_drv.h
> >>>>+++ b/drivers/gpu/drm/i915/intel_drv.h
> >>>>@@ -1117,6 +1117,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 6393b76..c660245 100644
> >>>>--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> >>>>+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> >>>>@@ -532,7 +532,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;
> >>>>@@ -549,7 +549,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;
> >>>Everything above seems to roughly be matching your patch description, but
> >>>not perfectly: You talk about suspend flow but also touch resume flow.
> >>>
> >>>But the hunks below are completely unexplained magic afaict. Either this
> >>>needs a separate patch or it needs seriously more explanation of what's
> >>>going on.
> >>>
> >>>>@@ -610,10 +610,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.
> >>>>@@ -621,8 +621,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);
> >>>>@@ -642,24 +640,9 @@ 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)) &&
> >>>>-				power_well->data == SKL_DISP_PW_2) {
> >>>>-				enum csr_state state;
> >>>>-				/* 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)
> >>>>-					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);
> >>>>-			}
> >>>>+			if (GEN9_ENABLE_DC5(dev) &&
> >>>>+				power_well->data == SKL_DISP_PW_2)
> >>>>+					gen9_enable_dc5(dev_priv);
> >>>>  		}
> >>>>  	}
> >>>>-- 
> >>>>2.0.2
> >>>>
> >>>>_______________________________________________
> >>>>Intel-gfx mailing list
> >>>>Intel-gfx@lists.freedesktop.org
> >>>>http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

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

Re: [SKL-DMC-BUGFIX 2/5] drm/i915/skl: Making DC6 entry is the last call in suspend flow.

[Imre Deak]

On ma, 2015-08-03 at 21:55 +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.

The MMIO range BSpec-ID 0527 refers to is the DMC MMIO range. The driver
uses this MMIO range only to program the FW image, it doesn't access it
afterwards. So that's not a good justification to keep DC6 disabled.

That BSpec-ID also mentions that DC6 together with DC5 needs to be
disabled around modesets, which we need to address by disabling both
DC5/6, but only around modesets.

Later discussions with HW people revealed that there is an open issue
related to DC6, see [1]. The suggested workaround for that issue is to
keep DC6 disabled all the time and use a manual sequence to enable
deeper power states (see "Sequence for Software to Allow Package C9-C10"
in BSpec).

Based on this what we need to do in this patch is simply disable DC6. As
a follow-up to this patchset we need to:
- Add the manual PC9/10 sequence.
- Prevent DC5/6 during modesets (and DPAUX transfers).
- Move out the remaining parts of the display init/uninit sequence from
the suspend/resume path.
- Add a module options to select between enabling DC6 and running the
manual PC9/10 sequence. People still would like to experiment with DC6
and we may end up enabling it in the end if all the open issues get
resolved. For that case enabling DC6 should still happen at the place
where it happens now, that is after disabling PW2.

Please see my corresponding comments inlined below.

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

> 
> v1: Initial version.
> 
> v2: commit message updated based on comment from Vathsala.
> 
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Sunil Kamath <sunil.kamath@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c         | 12 ++++++------
>  drivers/gpu/drm/i915/intel_drv.h        |  2 ++
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 33 ++++++++-------------------------
>  3 files changed, 16 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 0d6775a..e1d0102 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1017,14 +1017,11 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv)
>  {
>  	/* Enabling DC6 is not a hard requirement to enter runtime D3 */
>  
> -	/*
> -	 * This is to ensure that CSR isn't identified as loaded before
> -	 * CSR-loading program is called during runtime-resume.
> -	 */
> -	intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED);
> -
>  	skl_uninit_cdclk(dev_priv);
>  
> +	if (intel_csr_load_status_get(dev_priv) == FW_LOADED)
> +		skl_enable_dc6(dev_priv);
> +

Not needed, atm we shouldn't enable DC6 ever due to open issues under
investigation. Even if those investigations reveal that we can use DC6
it shouldn't be enabled here, rather at its current place after
disabling PW2.

>  	return 0;
>  }
>  
> @@ -1071,6 +1068,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);
> +

Not needed based on the previous comment.

>  	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 47cef0e..06f346f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1117,6 +1117,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 6393b76..c660245 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -532,7 +532,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;
>  
> @@ -549,7 +549,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;
>  
> @@ -610,10 +610,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.
> @@ -621,8 +621,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);

Instead of this please just redefine GEN9_ENABLE_DC5 and SKL_ENABLE_DC6
accordingly. We want to keep the possibility to use DC6, (adding a
module option for it as a follow-up), and then this is still the right
place to disable it.

>  				}
>  			}
>  			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
> @@ -642,24 +640,9 @@ 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)) &&
> -				power_well->data == SKL_DISP_PW_2) {
> -				enum csr_state state;
> -				/* 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)
> -					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);
> -			}
> +			if (GEN9_ENABLE_DC5(dev) &&
> +				power_well->data == SKL_DISP_PW_2)
> +					gen9_enable_dc5(dev_priv);

Removing the blocking wait is ok, though you should have mentioned in
the commit log why and have this change in a separate patch. Instead of
removing skl_enable_dc6() please redefine the macros as explained above.

--Imre

>  		}
>  	}
>  


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Disable DC6 for now.

[Rodrigo Vivi]

There is an intermitent RAM corruption happening with DMC
micro-controler when in DC6 transitioning to PC9/10. So
the recoomendation is to use DC5 as the deeper DC state
for now until this issue is being investigated at firmware
level.

This macros must be re-worked in order to allow us to use
module parameters to allow max DC states. However let's do
this simple approach first before products out there start
facing this corruption. Also this is the easiest one to be
backported by products.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index ec010ee..7332cc0 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -49,8 +49,8 @@
  * present for a given platform.
  */
 
-#define GEN9_ENABLE_DC5(dev) 0
-#define SKL_ENABLE_DC6(dev) IS_SKYLAKE(dev)
+#define GEN9_ENABLE_DC5(dev) IS_SKYLAKE(dev)
+#define SKL_ENABLE_DC6(dev) 0
 
 #define for_each_power_well(i, power_well, domain_mask, power_domains)	\
 	for (i = 0;							\
-- 
2.4.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Disable DC6 for now.

[Hindman, Gavin]

The current DC6 functionality is stable enough for development and is badly needed for working down other platform power issues.  I'm fine if you want to disable it by default, but please only do so in conjunction with i915 kernel override flags to reenable it at runtime.

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

-----Original Message-----
From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Rodrigo Vivi
Sent: Monday, October 12, 2015 8:43 AM
To: intel-gfx@lists.freedesktop.org
Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>
Subject: [Intel-gfx] [PATCH] drm/i915: Disable DC6 for now.

There is an intermitent RAM corruption happening with DMC micro-controler when in DC6 transitioning to PC9/10. So the recoomendation is to use DC5 as the deeper DC state for now until this issue is being investigated at firmware level.

This macros must be re-worked in order to allow us to use module parameters to allow max DC states. However let's do this simple approach first before products out there start facing this corruption. Also this is the easiest one to be backported by products.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index ec010ee..7332cc0 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -49,8 +49,8 @@
  * present for a given platform.
  */
 
-#define GEN9_ENABLE_DC5(dev) 0
-#define SKL_ENABLE_DC6(dev) IS_SKYLAKE(dev)
+#define GEN9_ENABLE_DC5(dev) IS_SKYLAKE(dev) #define 
+SKL_ENABLE_DC6(dev) 0
 
 #define for_each_power_well(i, power_well, domain_mask, power_domains)	\
 	for (i = 0;							\
--
2.4.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[SKL-DMC-BUGFIX 3/5] drm/i915/skl: Do not disable cdclk PLL if csr firmware is present.

[Animesh Manna]

While display engine entering into low power state no need to disable
cdclk pll as CSR firmware of dmc will take care. If pll is already
enabled firmware execution sequence will be blocked. This is one
of the criteria for dmc to work properly.

Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
Signed-off-bt: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index af0bcfe..ef2ef4d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5675,10 +5675,13 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
 	if (I915_READ(DBUF_CTL) & DBUF_POWER_STATE)
 		DRM_ERROR("DBuf power disable timeout\n");
 
-	/* disable DPLL0 */
-	I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) & ~LCPLL_PLL_ENABLE);
-	if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
-		DRM_ERROR("Couldn't disable DPLL0\n");
+	if (dev_priv->csr.dmc_payload) {
+		/* disable DPLL0 */
+		I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) &
+					~LCPLL_PLL_ENABLE);
+		if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
+			DRM_ERROR("Couldn't disable DPLL0\n");
+	}
 
 	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
 }
-- 
2.0.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [SKL-DMC-BUGFIX 3/5] drm/i915/skl: Do not disable cdclk PLL if csr firmware is present.

[Sunil Kamath]

On Monday 03 August 2015 09:55 PM, Animesh Manna wrote:
> While display engine entering into low power state no need to disable
> cdclk pll as CSR firmware of dmc will take care. If pll is already
> enabled firmware execution sequence will be blocked. This is one
> of the criteria for dmc to work properly.
>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Sunil Kamath <sunil.kamath@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> Signed-off-bt: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index af0bcfe..ef2ef4d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5675,10 +5675,13 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
>   	if (I915_READ(DBUF_CTL) & DBUF_POWER_STATE)
>   		DRM_ERROR("DBuf power disable timeout\n");
>   
> -	/* disable DPLL0 */
> -	I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) & ~LCPLL_PLL_ENABLE);
> -	if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
> -		DRM_ERROR("Couldn't disable DPLL0\n");
> +	if (dev_priv->csr.dmc_payload) {
> +		/* disable DPLL0 */
> +		I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) &
> +					~LCPLL_PLL_ENABLE);
> +		if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
> +			DRM_ERROR("Couldn't disable DPLL0\n");
> +	}
>   
>   	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
>   }
Right bug fix.

Reviewed-by: A.Sunil Kamath <sunil.kamath@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [SKL-DMC-BUGFIX 3/5] drm/i915/skl: Do not disable cdclk PLL if csr firmware is present.

[Daniel Vetter]

On Mon, Aug 03, 2015 at 09:55:34PM +0530, Animesh Manna wrote:
> While display engine entering into low power state no need to disable
> cdclk pll as CSR firmware of dmc will take care. If pll is already
> enabled firmware execution sequence will be blocked. This is one
> of the criteria for dmc to work properly.
> 
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Sunil Kamath <sunil.kamath@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> Signed-off-bt: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index af0bcfe..ef2ef4d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5675,10 +5675,13 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
>  	if (I915_READ(DBUF_CTL) & DBUF_POWER_STATE)
>  		DRM_ERROR("DBuf power disable timeout\n");
>  
> -	/* disable DPLL0 */
> -	I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) & ~LCPLL_PLL_ENABLE);
> -	if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
> -		DRM_ERROR("Couldn't disable DPLL0\n");
> +	if (dev_priv->csr.dmc_payload) {
> +		/* disable DPLL0 */

Imo this needs a proper comment (and the current one is useless since it
just states exactly what the code does and is redundant). What about

	/* DMC assumes ownership of LCPLL and will get confused if we
	 * touch it. */

instead before the if?
-Daniel

> +		I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) &
> +					~LCPLL_PLL_ENABLE);
> +		if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
> +			DRM_ERROR("Couldn't disable DPLL0\n");
> +	}
>  
>  	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
>  }
> -- 
> 2.0.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [SKL-DMC-BUGFIX 3/5] drm/i915/skl: Do not disable cdclk PLL if csr firmware is present.

[Animesh Manna]

On 8/5/2015 2:42 PM, Daniel Vetter wrote:
> On Mon, Aug 03, 2015 at 09:55:34PM +0530, Animesh Manna wrote:
>> While display engine entering into low power state no need to disable
>> cdclk pll as CSR firmware of dmc will take care. If pll is already
>> enabled firmware execution sequence will be blocked. This is one
>> of the criteria for dmc to work properly.
>>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Damien Lespiau <damien.lespiau@intel.com>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Cc: Sunil Kamath <sunil.kamath@intel.com>
>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>> Signed-off-bt: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
>> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index af0bcfe..ef2ef4d 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -5675,10 +5675,13 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
>>   	if (I915_READ(DBUF_CTL) & DBUF_POWER_STATE)
>>   		DRM_ERROR("DBuf power disable timeout\n");
>>   
>> -	/* disable DPLL0 */
>> -	I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) & ~LCPLL_PLL_ENABLE);
>> -	if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
>> -		DRM_ERROR("Couldn't disable DPLL0\n");
>> +	if (dev_priv->csr.dmc_payload) {
>> +		/* disable DPLL0 */
> Imo this needs a proper comment (and the current one is useless since it
> just states exactly what the code does and is redundant). What about
>
> 	/* DMC assumes ownership of LCPLL and will get confused if we
> 	 * touch it. */
>
> instead before the if?
> -Daniel

Agree, I will add it in my next patch.
(Current comment came from existing code.)

- Animesh

>
>> +		I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) &
>> +					~LCPLL_PLL_ENABLE);
>> +		if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
>> +			DRM_ERROR("Couldn't disable DPLL0\n");
>> +	}
>>   
>>   	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
>>   }
>> -- 
>> 2.0.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[SKL-DMC-BUGFIX 3/5] drm/i915/skl: Do not disable cdclk PLL if csr firmware is present

[Animesh Manna]

While display engine entering into low power state no need to disable
cdclk pll as CSR firmware of dmc will take care. If pll is already
enabled firmware execution sequence will be blocked. This is one
of the criteria for dmc to work properly.

v1: Initial version.

v2: Based on review comment from Daniel added code commnent.

Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
Signed-off-bt: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index af0bcfe..1f8d704 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5675,10 +5675,16 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
 	if (I915_READ(DBUF_CTL) & DBUF_POWER_STATE)
 		DRM_ERROR("DBuf power disable timeout\n");
 
-	/* disable DPLL0 */
-	I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) & ~LCPLL_PLL_ENABLE);
-	if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
-		DRM_ERROR("Couldn't disable DPLL0\n");
+	/* 
+	 * DMC assumes ownership of LCPLL and will get confused if we touch it. 
+	 */
+	if (dev_priv->csr.dmc_payload) {
+		/* disable DPLL0 */
+		I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) &
+					~LCPLL_PLL_ENABLE);
+		if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
+			DRM_ERROR("Couldn't disable DPLL0\n");
+	}
 
 	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
 }
-- 
2.0.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [SKL-DMC-BUGFIX 3/5] drm/i915/skl: Do not disable cdclk PLL if csr firmware is present.

[Imre Deak]

On ma, 2015-08-03 at 21:55 +0530, Animesh Manna wrote:
> While display engine entering into low power state no need to disable
> cdclk pll as CSR firmware of dmc will take care. If pll is already
> enabled firmware execution sequence will be blocked. This is one
> of the criteria for dmc to work properly.
> 
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Sunil Kamath <sunil.kamath@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> Signed-off-bt: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index af0bcfe..ef2ef4d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5675,10 +5675,13 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
>  	if (I915_READ(DBUF_CTL) & DBUF_POWER_STATE)
>  		DRM_ERROR("DBuf power disable timeout\n");

My understanding is that DBUF_CTL is also handled by the firmware and so
we shouldn't need to disable it either manually. I guess that could be
addressed as a follow-up.

>  
> -	/* disable DPLL0 */
> -	I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) & ~LCPLL_PLL_ENABLE);
> -	if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
> -		DRM_ERROR("Couldn't disable DPLL0\n");
> +	if (dev_priv->csr.dmc_payload) {
> +		/* disable DPLL0 */
> +		I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) &
> +					~LCPLL_PLL_ENABLE);
> +		if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
> +			DRM_ERROR("Couldn't disable DPLL0\n");
> +	}
>  
>  	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
>  }


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [SKL-DMC-BUGFIX 3/5] drm/i915/skl: Do not disable cdclk PLL if csr firmware is present.

[Imre Deak]

On ma, 2015-10-12 at 16:37 +0300, Imre Deak wrote:
> On ma, 2015-08-03 at 21:55 +0530, Animesh Manna wrote:
> > While display engine entering into low power state no need to disable
> > cdclk pll as CSR firmware of dmc will take care. If pll is already
> > enabled firmware execution sequence will be blocked. This is one
> > of the criteria for dmc to work properly.
> > 
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Damien Lespiau <damien.lespiau@intel.com>
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Sunil Kamath <sunil.kamath@intel.com>
> > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > Signed-off-bt: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> > Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index af0bcfe..ef2ef4d 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5675,10 +5675,13 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
> >  	if (I915_READ(DBUF_CTL) & DBUF_POWER_STATE)
> >  		DRM_ERROR("DBuf power disable timeout\n");
> 
> My understanding is that DBUF_CTL is also handled by the firmware and so
> we shouldn't need to disable it either manually. I guess that could be
> addressed as a follow-up.
> 
> >  
> > -	/* disable DPLL0 */
> > -	I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) & ~LCPLL_PLL_ENABLE);
> > -	if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
> > -		DRM_ERROR("Couldn't disable DPLL0\n");
> > +	if (dev_priv->csr.dmc_payload) {
> > +		/* disable DPLL0 */
> > +		I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) &
> > +					~LCPLL_PLL_ENABLE);
> > +		if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
> > +			DRM_ERROR("Couldn't disable DPLL0\n");
> > +	}
> >  
> >  	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);

Not introduced in this patch, but the above put looks incorrect. We get
here on the runtime suspend path, where all RPM and hence display power
domain references should be dropped already. So not sure how this can
even work atm. This is for someone to look into as a follow-up.

> >  }
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [SKL-DMC-BUGFIX 3/5] drm/i915/skl: Do not disable cdclk PLL if csr firmware is present.

[Patrik Jakobsson]

On Mon, Oct 12, 2015 at 05:07:13PM +0300, Imre Deak wrote:
> On ma, 2015-10-12 at 16:37 +0300, Imre Deak wrote:
> > On ma, 2015-08-03 at 21:55 +0530, Animesh Manna wrote:
> > > While display engine entering into low power state no need to disable
> > > cdclk pll as CSR firmware of dmc will take care. If pll is already
> > > enabled firmware execution sequence will be blocked. This is one
> > > of the criteria for dmc to work properly.
> > > 
> > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > Cc: Damien Lespiau <damien.lespiau@intel.com>
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Cc: Sunil Kamath <sunil.kamath@intel.com>
> > > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > > Signed-off-bt: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> > > Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 11 +++++++----
> > >  1 file changed, 7 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index af0bcfe..ef2ef4d 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -5675,10 +5675,13 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
> > >  	if (I915_READ(DBUF_CTL) & DBUF_POWER_STATE)
> > >  		DRM_ERROR("DBuf power disable timeout\n");
> > 
> > My understanding is that DBUF_CTL is also handled by the firmware and so
> > we shouldn't need to disable it either manually. I guess that could be
> > addressed as a follow-up.
> > 
> > >  
> > > -	/* disable DPLL0 */
> > > -	I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) & ~LCPLL_PLL_ENABLE);
> > > -	if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
> > > -		DRM_ERROR("Couldn't disable DPLL0\n");
> > > +	if (dev_priv->csr.dmc_payload) {
> > > +		/* disable DPLL0 */
> > > +		I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) &
> > > +					~LCPLL_PLL_ENABLE);
> > > +		if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
> > > +			DRM_ERROR("Couldn't disable DPLL0\n");
> > > +	}
> > >  
> > >  	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
> 
> Not introduced in this patch, but the above put looks incorrect. We get
> here on the runtime suspend path, where all RPM and hence display power
> domain references should be dropped already. So not sure how this can
> even work atm. This is for someone to look into as a follow-up.

Hmm, I thought that was fixed already. This seems to be the last comment on
Paulos attempt at fixing it:

http://lists.freedesktop.org/archives/intel-gfx/2015-August/073122.html

> > >  }
> > 
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [SKL-DMC-BUGFIX 3/5] drm/i915/skl: Do not disable cdclk PLL if csr firmware is present.

[Imre Deak]

On ma, 2015-10-12 at 16:46 +0200, Patrik Jakobsson wrote:
> On Mon, Oct 12, 2015 at 05:07:13PM +0300, Imre Deak wrote:
> > On ma, 2015-10-12 at 16:37 +0300, Imre Deak wrote:
> > > On ma, 2015-08-03 at 21:55 +0530, Animesh Manna wrote:
> > > > While display engine entering into low power state no need to disable
> > > > cdclk pll as CSR firmware of dmc will take care. If pll is already
> > > > enabled firmware execution sequence will be blocked. This is one
> > > > of the criteria for dmc to work properly.
> > > > 
> > > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > > Cc: Damien Lespiau <damien.lespiau@intel.com>
> > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > Cc: Sunil Kamath <sunil.kamath@intel.com>
> > > > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > > > Signed-off-bt: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> > > > Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.c | 11 +++++++----
> > > >  1 file changed, 7 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index af0bcfe..ef2ef4d 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -5675,10 +5675,13 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
> > > >  	if (I915_READ(DBUF_CTL) & DBUF_POWER_STATE)
> > > >  		DRM_ERROR("DBuf power disable timeout\n");
> > > 
> > > My understanding is that DBUF_CTL is also handled by the firmware and so
> > > we shouldn't need to disable it either manually. I guess that could be
> > > addressed as a follow-up.
> > > 
> > > >  
> > > > -	/* disable DPLL0 */
> > > > -	I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) & ~LCPLL_PLL_ENABLE);
> > > > -	if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
> > > > -		DRM_ERROR("Couldn't disable DPLL0\n");
> > > > +	if (dev_priv->csr.dmc_payload) {
> > > > +		/* disable DPLL0 */
> > > > +		I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) &
> > > > +					~LCPLL_PLL_ENABLE);
> > > > +		if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
> > > > +			DRM_ERROR("Couldn't disable DPLL0\n");
> > > > +	}
> > > >  
> > > >  	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
> > 
> > Not introduced in this patch, but the above put looks incorrect. We get
> > here on the runtime suspend path, where all RPM and hence display power
> > domain references should be dropped already. So not sure how this can
> > even work atm. This is for someone to look into as a follow-up.
> 
> Hmm, I thought that was fixed already. This seems to be the last comment on
> Paulos attempt at fixing it:
> 
> http://lists.freedesktop.org/archives/intel-gfx/2015-August/073122.html

Yes, that would solve this issue. One note about it is that we only want
to manually toggle PW1 and Misc IO if DC6 is disabled (via a module
option for example). And if so, toggling of PW1 and Misc IO would be
part of the bigger
"Sequence for Software to Allow/Disallow Package C9-C10".

> > > >  }
> > > 
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[SKL-DMC-BUGFIX 4/5] drm/i915/skl: Block disable call for pw1 if dmc firmware is present.

[Animesh Manna]

Another interesting criteria to work dmc as expected is pw1 to be
enabled by driver and dmc will shut it off in its execution
sequence. If already disabled by driver dmc will get confuse and
behave differently than expected found during pc10 entry issue
for skl.

So berfore we disable power-well 1, added check if dmc firmware is
present and driver will not disable power well 1, but for any reason
if firmware is not present of failed to load we can shut off the
power well 1 which will save some power.

As skl is currently fully dependent on dmc to go in lowest possible
power state (dc6) but the same is not applicable for bxt. Display
engine can enter into dc9 without dmc, hence unblocking disable call.

v1: Initial version.

v2: Based on revire commnents from Sunil,
- condition check for pw1 is moved in skl_set_power_well.

Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index c660245..00cd4ff 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -636,9 +636,15 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
 		}
 	} else {
 		if (enable_requested) {
-			I915_WRITE(HSW_PWR_WELL_DRIVER,	tmp & ~req_mask);
-			POSTING_READ(HSW_PWR_WELL_DRIVER);
-			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
+			if (IS_SKYLAKE(dev) &&
+				(power_well->data == SKL_DISP_PW_1) &&
+				(intel_csr_load_status_get(dev_priv) == FW_LOADED))
+				DRM_DEBUG_KMS("Not Disabling PW1, dmc will handle\n");
+			else {
+				I915_WRITE(HSW_PWR_WELL_DRIVER,	tmp & ~req_mask);
+				POSTING_READ(HSW_PWR_WELL_DRIVER);
+				DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
+			}
 
 			if (GEN9_ENABLE_DC5(dev) &&
 				power_well->data == SKL_DISP_PW_2)
-- 
2.0.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [SKL-DMC-BUGFIX 4/5] drm/i915/skl: Block disable call for pw1 if dmc firmware is present.

[Sunil Kamath]

On Monday 03 August 2015 09:55 PM, Animesh Manna wrote:
> Another interesting criteria to work dmc as expected is pw1 to be
> enabled by driver and dmc will shut it off in its execution
> sequence. If already disabled by driver dmc will get confuse and
> behave differently than expected found during pc10 entry issue
> for skl.
>
> So berfore we disable power-well 1, added check if dmc firmware is
> present and driver will not disable power well 1, but for any reason
> if firmware is not present of failed to load we can shut off the
> power well 1 which will save some power.
>
> As skl is currently fully dependent on dmc to go in lowest possible
> power state (dc6) but the same is not applicable for bxt. Display
> engine can enter into dc9 without dmc, hence unblocking disable call.
>
> v1: Initial version.
>
> v2: Based on revire commnents from Sunil,
> - condition check for pw1 is moved in skl_set_power_well.
>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Sunil Kamath <sunil.kamath@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_runtime_pm.c | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index c660245..00cd4ff 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -636,9 +636,15 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>   		}
>   	} else {
>   		if (enable_requested) {
> -			I915_WRITE(HSW_PWR_WELL_DRIVER,	tmp & ~req_mask);
> -			POSTING_READ(HSW_PWR_WELL_DRIVER);
> -			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
> +			if (IS_SKYLAKE(dev) &&
> +				(power_well->data == SKL_DISP_PW_1) &&
> +				(intel_csr_load_status_get(dev_priv) == FW_LOADED))
> +				DRM_DEBUG_KMS("Not Disabling PW1, dmc will handle\n");
> +			else {
> +				I915_WRITE(HSW_PWR_WELL_DRIVER,	tmp & ~req_mask);
> +				POSTING_READ(HSW_PWR_WELL_DRIVER);
> +				DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
> +			}
>   
>   			if (GEN9_ENABLE_DC5(dev) &&
>   				power_well->data == SKL_DISP_PW_2)
Right bug fix.

Reviewed-by: A.Sunil Kamath <sunil.kamath@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [SKL-DMC-BUGFIX 4/5] drm/i915/skl: Block disable call for pw1 if dmc firmware is present.

[Daniel Vetter]

On Mon, Aug 03, 2015 at 09:55:35PM +0530, Animesh Manna wrote:
> Another interesting criteria to work dmc as expected is pw1 to be
> enabled by driver and dmc will shut it off in its execution
> sequence. If already disabled by driver dmc will get confuse and
> behave differently than expected found during pc10 entry issue
> for skl.
> 
> So berfore we disable power-well 1, added check if dmc firmware is
> present and driver will not disable power well 1, but for any reason
> if firmware is not present of failed to load we can shut off the
> power well 1 which will save some power.
> 
> As skl is currently fully dependent on dmc to go in lowest possible
> power state (dc6) but the same is not applicable for bxt. Display
> engine can enter into dc9 without dmc, hence unblocking disable call.
> 
> v1: Initial version.
> 
> v2: Based on revire commnents from Sunil,
> - condition check for pw1 is moved in skl_set_power_well.

There's another patch from Damien/Paulo to do some similar fumbling
between LCPLL and PW1. We probably want to completely take away PW1 from
being controlled by the power wells code and push it all into the rpm code
(where we either disable pw1, pw-misc and lcpll in one go or leave it all
to the dmc firmware).
-Daniel

> 
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Sunil Kamath <sunil.kamath@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index c660245..00cd4ff 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -636,9 +636,15 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  		}
>  	} else {
>  		if (enable_requested) {
> -			I915_WRITE(HSW_PWR_WELL_DRIVER,	tmp & ~req_mask);
> -			POSTING_READ(HSW_PWR_WELL_DRIVER);
> -			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
> +			if (IS_SKYLAKE(dev) &&
> +				(power_well->data == SKL_DISP_PW_1) &&
> +				(intel_csr_load_status_get(dev_priv) == FW_LOADED))
> +				DRM_DEBUG_KMS("Not Disabling PW1, dmc will handle\n");
> +			else {
> +				I915_WRITE(HSW_PWR_WELL_DRIVER,	tmp & ~req_mask);
> +				POSTING_READ(HSW_PWR_WELL_DRIVER);
> +				DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
> +			}
>  
>  			if (GEN9_ENABLE_DC5(dev) &&
>  				power_well->data == SKL_DISP_PW_2)
> -- 
> 2.0.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [SKL-DMC-BUGFIX 4/5] drm/i915/skl: Block disable call for pw1 if dmc firmware is present.

[Animesh Manna]

On 8/5/2015 2:44 PM, Daniel Vetter wrote:
> On Mon, Aug 03, 2015 at 09:55:35PM +0530, Animesh Manna wrote:
>> Another interesting criteria to work dmc as expected is pw1 to be
>> enabled by driver and dmc will shut it off in its execution
>> sequence. If already disabled by driver dmc will get confuse and
>> behave differently than expected found during pc10 entry issue
>> for skl.
>>
>> So berfore we disable power-well 1, added check if dmc firmware is
>> present and driver will not disable power well 1, but for any reason
>> if firmware is not present of failed to load we can shut off the
>> power well 1 which will save some power.
>>
>> As skl is currently fully dependent on dmc to go in lowest possible
>> power state (dc6) but the same is not applicable for bxt. Display
>> engine can enter into dc9 without dmc, hence unblocking disable call.
>>
>> v1: Initial version.
>>
>> v2: Based on revire commnents from Sunil,
>> - condition check for pw1 is moved in skl_set_power_well.
> There's another patch from Damien/Paulo to do some similar fumbling
> between LCPLL and PW1. We probably want to completely take away PW1 from
> being controlled by the power wells code and push it all into the rpm code
> (where we either disable pw1, pw-misc and lcpll in one go or leave it all
> to the dmc firmware).
> -Daniel

Patch from Damien/Paulo submitted in intel-gfx mailing list? I have not seen the
actual implementation.

skl_set_power_well() is the function where enable/disable operation for all power-wells
is implemented. Imo, till we can add different condition check for taking care
special cases like dmc, its better to put power-well related enable/disable
code in same place - by this we will avoid code duplication and code readability
will be better. Send me your suggestion, accordingly if needed I will add required
changes.

- Animesh

>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Damien Lespiau <damien.lespiau@intel.com>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Cc: Sunil Kamath <sunil.kamath@intel.com>
>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_runtime_pm.c | 12 +++++++++---
>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> index c660245..00cd4ff 100644
>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> @@ -636,9 +636,15 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>>   		}
>>   	} else {
>>   		if (enable_requested) {
>> -			I915_WRITE(HSW_PWR_WELL_DRIVER,	tmp & ~req_mask);
>> -			POSTING_READ(HSW_PWR_WELL_DRIVER);
>> -			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
>> +			if (IS_SKYLAKE(dev) &&
>> +				(power_well->data == SKL_DISP_PW_1) &&
>> +				(intel_csr_load_status_get(dev_priv) == FW_LOADED))
>> +				DRM_DEBUG_KMS("Not Disabling PW1, dmc will handle\n");
>> +			else {
>> +				I915_WRITE(HSW_PWR_WELL_DRIVER,	tmp & ~req_mask);
>> +				POSTING_READ(HSW_PWR_WELL_DRIVER);
>> +				DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
>> +			}
>>   
>>   			if (GEN9_ENABLE_DC5(dev) &&
>>   				power_well->data == SKL_DISP_PW_2)
>> -- 
>> 2.0.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [SKL-DMC-BUGFIX 4/5] drm/i915/skl: Block disable call for pw1 if dmc firmware is present.

[Imre Deak]

On ma, 2015-08-03 at 21:55 +0530, Animesh Manna wrote:
> Another interesting criteria to work dmc as expected is pw1 to be
> enabled by driver and dmc will shut it off in its execution
> sequence. If already disabled by driver dmc will get confuse and
> behave differently than expected found during pc10 entry issue
> for skl.
> 
> So berfore we disable power-well 1, added check if dmc firmware is
> present and driver will not disable power well 1, but for any reason
> if firmware is not present of failed to load we can shut off the
> power well 1 which will save some power.
> 
> As skl is currently fully dependent on dmc to go in lowest possible
> power state (dc6) but the same is not applicable for bxt. Display
> engine can enter into dc9 without dmc, hence unblocking disable call.
> 
> v1: Initial version.
> 
> v2: Based on revire commnents from Sunil,
> - condition check for pw1 is moved in skl_set_power_well.
> 
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Sunil Kamath <sunil.kamath@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index c660245..00cd4ff 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -636,9 +636,15 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  		}
>  	} else {
>  		if (enable_requested) {
> -			I915_WRITE(HSW_PWR_WELL_DRIVER,	tmp & ~req_mask);
> -			POSTING_READ(HSW_PWR_WELL_DRIVER);
> -			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
> +			if (IS_SKYLAKE(dev) &&
> +				(power_well->data == SKL_DISP_PW_1) &&
> +				(intel_csr_load_status_get(dev_priv) == FW_LOADED))

We'll only ever get here with the firmware loaded, so no need to check
for it. 

> +				DRM_DEBUG_KMS("Not Disabling PW1, dmc will handle\n");
> +			else {
> +				I915_WRITE(HSW_PWR_WELL_DRIVER,	tmp & ~req_mask);
> +				POSTING_READ(HSW_PWR_WELL_DRIVER);
> +				DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
> +			}

Not disabling PW1 here is a good solution for now imo, but ideally we
should move both PW1 enabling and disabling out from this code and do
these as part of the display init/uninit sequence. This could be done as
a follow-up to this patchset.

>  
>  			if (GEN9_ENABLE_DC5(dev) &&
>  				power_well->data == SKL_DISP_PW_2)


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[SKL-DMC-BUGFIX 5/5] drm/i915/skl: Removed csr firmware load in resume path

[Animesh Manna]

As csr firmware is taking care of loading the firmware,
so no need for driver to load again.

Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e1d0102..02019e9 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1066,13 +1066,10 @@ static int bxt_resume_prepare(struct drm_i915_private *dev_priv)
 
 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);
 
 	return 0;
 }
-- 
2.0.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [SKL-DMC-BUGFIX 5/5] drm/i915/skl: Removed csr firmware load in resume path

[Sunil Kamath]

On Monday 03 August 2015 09:55 PM, Animesh Manna wrote:
> As csr firmware is taking care of loading the firmware,
> so no need for driver to load again.
>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Sunil Kamath <sunil.kamath@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.c | 3 ---
>   1 file changed, 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e1d0102..02019e9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1066,13 +1066,10 @@ static int bxt_resume_prepare(struct drm_i915_private *dev_priv)
>   
>   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);

Same comment as before.
The context save and restore program is reset on cold boot, warm reset, 
PCI function level reset, and hibernate/suspend.

Need valid reason to do this change. If reading DC5/DC6 counters is a 
concern, lets use this as just debug patch.

Dont hurry on this patch.
Need to close on the above opens.

- Sunil
>   
>   	return 0;
>   }

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [SKL-DMC-BUGFIX 5/5] drm/i915/skl: Removed csr firmware load in resume path

[Animesh Manna]

On 8/4/2015 4:50 PM, Sunil Kamath wrote:
> On Monday 03 August 2015 09:55 PM, Animesh Manna wrote:
>> As csr firmware is taking care of loading the firmware,
>> so no need for driver to load again.
>>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Damien Lespiau <damien.lespiau@intel.com>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Cc: Sunil Kamath <sunil.kamath@intel.com>
>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c 
>> b/drivers/gpu/drm/i915/i915_drv.c
>> index e1d0102..02019e9 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -1066,13 +1066,10 @@ static int bxt_resume_prepare(struct 
>> drm_i915_private *dev_priv)
>>     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);
>
> Same comment as before.
> The context save and restore program is reset on cold boot, warm 
> reset, PCI function level reset, and hibernate/suspend.
>
> Need valid reason to do this change. If reading DC5/DC6 counters is a 
> concern, lets use this as just debug patch.
>
> Dont hurry on this patch.
> Need to close on the above opens.
>
> - Sunil

Agree, I want to add this patch as part of this patch series, already started communication with firmware team.
Waiting for suggestion, will followup and proceed further based on suggestion.


Regards,
Animesh
>>         return 0;
>>   }
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [SKL-DMC-BUGFIX 5/5] drm/i915/skl: Removed csr firmware load in resume path

[Animesh Manna]

On 8/4/2015 5:03 PM, Animesh Manna wrote:
>
>
> On 8/4/2015 4:50 PM, Sunil Kamath wrote:
>> On Monday 03 August 2015 09:55 PM, Animesh Manna wrote:
>>> As csr firmware is taking care of loading the firmware,
>>> so no need for driver to load again.
>>>
>>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>>> Cc: Damien Lespiau <damien.lespiau@intel.com>
>>> Cc: Imre Deak <imre.deak@intel.com>
>>> Cc: Sunil Kamath <sunil.kamath@intel.com>
>>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>>> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_drv.c | 3 ---
>>>   1 file changed, 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c 
>>> b/drivers/gpu/drm/i915/i915_drv.c
>>> index e1d0102..02019e9 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>> @@ -1066,13 +1066,10 @@ static int bxt_resume_prepare(struct 
>>> drm_i915_private *dev_priv)
>>>     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);
>>
>> Same comment as before.
>> The context save and restore program is reset on cold boot, warm 
>> reset, PCI function level reset, and hibernate/suspend.
>>
>> Need valid reason to do this change. If reading DC5/DC6 counters is a 
>> concern, lets use this as just debug patch.
>>
>> Dont hurry on this patch.
>> Need to close on the above opens.
>>
>> - Sunil
>
> Agree, I want to add this patch as part of this patch series, already 
> started communication with firmware team.
> Waiting for suggestion, will followup and proceed further based on 
> suggestion.
>
>
> Regards,
> Animesh

Firmware team confirmed that one time firmware loading during driver loading is sufficient, no need
to load firmware in csr-address-space every suspend (dc6 entry) - resume (dc6 exit) flow, dmc will
take care of it.

- Animesh

>>>         return 0;
>>>   }
>>
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [SKL-DMC-BUGFIX 5/5] drm/i915/skl: Removed csr firmware load in resume path

[Imre Deak]

On ma, 2015-08-03 at 21:55 +0530, Animesh Manna wrote:
> As csr firmware is taking care of loading the firmware,
> so no need for driver to load again.

I'd think that it's some other (PUnit, PCode) firmware that would take
care of reprogramming the CSR(DMC) firmware, rather than the CSR
firmware itself.

> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Sunil Kamath <sunil.kamath@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e1d0102..02019e9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1066,13 +1066,10 @@ static int bxt_resume_prepare(struct drm_i915_private *dev_priv)
>  
>  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);

This also means that we don't reprogram the firmware during S3 and S4
resume. I can see how this would work during S3 resume, but I can't see
how it would work during S4 resume, especially if the kernel loading the
hibernation image doesn't load the firmware itself (possible if it
doesn't have the i915 driver). If you confirmed that we don't need to
reprogram the firmware ever, please add this to the commit message
mentioning explicitly the D3, S0ix, S3, S4 states.

>  
>  	return 0;
>  }


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [SKL-DMC-BUGFIX 0/5] SKL PC10 entry fixes.

[Zanoni, Paulo R]

Em Seg, 2015-08-03 às 21:55 +0530, Animesh Manna escreveu:
> The following patches helps to solve PC10 entry issue for SKL.
> Detailed description about the changes done to solve the issue
> is mentioned in commit message of each patch.
> 
> All these patches are send earlier for review as part of "Redesign of
> dmc firmware loading" patch series. Now skl specific bug-fixes are
> seperated out and sending as seperate patch series to make it simple.

Hello

I find interesting that even with this series, igt/pm_rpm/rte fails. We
don't seem to be doing runtime PM, yet we allow PC10 somehow. Maybe it
would be nice to have some kind of documentation explaining what's
different on SKL and how things are supposed to work.

Also, since this feature is not relying on runtime PM, it would be
really nice to elaborate some IGT tests for it. You could even try to
automate PC10 entry checks by reading the appropriate MSR regsiters -
just like we do with PC8 on HSW.

Thanks,
Paulo

> 
> Animesh Manna (5):
>   drm/i915/gen9: Removed byte swapping for csr firmware
>   drm/i915/skl: Making DC6 entry is the last call in suspend flow.
>   drm/i915/skl: Do not disable cdclk PLL if csr firmware is present.
>   drm/i915/skl: Block disable call for pw1 if dmc firmware is 
> present.
>   drm/i915/skl: Removed csr firmware load in resume path.
> 
>  drivers/gpu/drm/i915/i915_drv.c         | 13 +++++-----
>  drivers/gpu/drm/i915/i915_drv.h         |  2 +-
>  drivers/gpu/drm/i915/intel_csr.c        | 16 +++---------
>  drivers/gpu/drm/i915/intel_display.c    | 11 +++++---
>  drivers/gpu/drm/i915/intel_drv.h        |  2 ++
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 45 +++++++++++++----------
> ----------
>  6 files changed, 37 insertions(+), 52 deletions(-)
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [SKL-DMC-BUGFIX 0/5] SKL PC10 entry fixes.

[Sunil Kamath]

On Tuesday 04 August 2015 12:17 AM, Zanoni, Paulo R wrote:
> Em Seg, 2015-08-03 às 21:55 +0530, Animesh Manna escreveu:
>> The following patches helps to solve PC10 entry issue for SKL.
>> Detailed description about the changes done to solve the issue
>> is mentioned in commit message of each patch.
>>
>> All these patches are send earlier for review as part of "Redesign of
>> dmc firmware loading" patch series. Now skl specific bug-fixes are
>> seperated out and sending as seperate patch series to make it simple.
> Hello
>
> I find interesting that even with this series, igt/pm_rpm/rte fails. We
> don't seem to be doing runtime PM, yet we allow PC10 somehow. Maybe it
> would be nice to have some kind of documentation explaining what's
> different on SKL and how things are supposed to work.

Why do you say that rpm is not done for SKL?
Even in SKL its runtime PM that triggers DC6 enable then PKGC10.

Vathsala, Rajneesh,
please add your comments and observations here.

- Sunil

>
> Also, since this feature is not relying on runtime PM, it would be
> really nice to elaborate some IGT tests for it. You could even try to
> automate PC10 entry checks by reading the appropriate MSR regsiters -
> just like we do with PC8 on HSW.
>
> Thanks,
> Paulo
>
>> Animesh Manna (5):
>>    drm/i915/gen9: Removed byte swapping for csr firmware
>>    drm/i915/skl: Making DC6 entry is the last call in suspend flow.
>>    drm/i915/skl: Do not disable cdclk PLL if csr firmware is present.
>>    drm/i915/skl: Block disable call for pw1 if dmc firmware is
>> present.
>>    drm/i915/skl: Removed csr firmware load in resume path.
>>
>>   drivers/gpu/drm/i915/i915_drv.c         | 13 +++++-----
>>   drivers/gpu/drm/i915/i915_drv.h         |  2 +-
>>   drivers/gpu/drm/i915/intel_csr.c        | 16 +++---------
>>   drivers/gpu/drm/i915/intel_display.c    | 11 +++++---
>>   drivers/gpu/drm/i915/intel_drv.h        |  2 ++
>>   drivers/gpu/drm/i915/intel_runtime_pm.c | 45 +++++++++++++----------
>> ----------
>>   6 files changed, 37 insertions(+), 52 deletions(-)
>>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [SKL-DMC-BUGFIX 0/5] SKL PC10 entry fixes.

[Zanoni, Paulo R]

Em Ter, 2015-08-04 às 17:01 +0530, Sunil Kamath escreveu:
> On Tuesday 04 August 2015 12:17 AM, Zanoni, Paulo R wrote:
> > Em Seg, 2015-08-03 às 21:55 +0530, Animesh Manna escreveu:
> > > The following patches helps to solve PC10 entry issue for SKL.
> > > Detailed description about the changes done to solve the issue
> > > is mentioned in commit message of each patch.
> > > 
> > > All these patches are send earlier for review as part of 
> > > "Redesign of
> > > dmc firmware loading" patch series. Now skl specific bug-fixes 
> > > are
> > > seperated out and sending as seperate patch series to make it 
> > > simple.
> > Hello
> > 
> > I find interesting that even with this series, igt/pm_rpm/rte 
> > fails. We
> > don't seem to be doing runtime PM, yet we allow PC10 somehow. Maybe 
> > it
> > would be nice to have some kind of documentation explaining what's
> > different on SKL and how things are supposed to work.
> 
> Why do you say that rpm is not done for SKL?

> Even in SKL its runtime PM that triggers DC6 enable then PKGC10.
> 
> Vathsala, Rajneesh,
> please add your comments and observations here.

Every single RPM IGT test case fails on current -nightly, even with
these patches applied. We just don't ever runtime suspend on SKL.

OTOH I already submitted "[PATCH 3/4] drm/i915: Make turning on/off PW1
and Misc I/O part of the init/fini sequences", which makes SKL actually
runtime suspend, so most of the tests start passing - although we still
have some failures.

OTOH, since you claim we actually reach PC10 without [patch 3/4], then
maybe we don't need RPM at all? Still, we'd need IGT tests for that.

> 
> - Sunil
> 
> > 
> > Also, since this feature is not relying on runtime PM, it would be
> > really nice to elaborate some IGT tests for it. You could even try 
> > to
> > automate PC10 entry checks by reading the appropriate MSR regsiters 
> > -
> > just like we do with PC8 on HSW.
> > 
> > Thanks,
> > Paulo
> > 
> > > Animesh Manna (5):
> > >    drm/i915/gen9: Removed byte swapping for csr firmware
> > >    drm/i915/skl: Making DC6 entry is the last call in suspend 
> > > flow.
> > >    drm/i915/skl: Do not disable cdclk PLL if csr firmware is 
> > > present.
> > >    drm/i915/skl: Block disable call for pw1 if dmc firmware is
> > > present.
> > >    drm/i915/skl: Removed csr firmware load in resume path.
> > > 
> > >   drivers/gpu/drm/i915/i915_drv.c         | 13 +++++-----
> > >   drivers/gpu/drm/i915/i915_drv.h         |  2 +-
> > >   drivers/gpu/drm/i915/intel_csr.c        | 16 +++---------
> > >   drivers/gpu/drm/i915/intel_display.c    | 11 +++++---
> > >   drivers/gpu/drm/i915/intel_drv.h        |  2 ++
> > >   drivers/gpu/drm/i915/intel_runtime_pm.c | 45 +++++++++++++-----
> > > -----
> > > ----------
> > >   6 files changed, 37 insertions(+), 52 deletions(-)
> > > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx