Re: [Intel-gfx] [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 
> Cc: Damien Lespiau 
> Cc: Imre Deak 
> Cc: Sunil Kamath 
> Signed-off-by: Animesh Manna 
> Signed-off-by: Vathsala Nagaraju 
> ---
>  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

Re: [Intel-gfx] [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.vet...@intel.com
Cc: Damien Lespiau damien.lesp...@intel.com
Cc: Imre Deak imre.d...@intel.com
Cc: Sunil Kamath sunil.kam...@intel.com
Signed-off-by: Animesh Manna animesh.ma...@intel.com
Signed-off-by: Vathsala Nagaraju vathsala.nagar...@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: [Intel-gfx] [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.vet...@intel.com
 Cc: Damien Lespiau damien.lesp...@intel.com
 Cc: Imre Deak imre.d...@intel.com
 Cc: Sunil Kamath sunil.kam...@intel.com
 Signed-off-by: Animesh Manna animesh.ma...@intel.com
 Signed-off-by: Vathsala Nagaraju vathsala.nagar...@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: [Intel-gfx] [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.vet...@intel.com
Cc: Damien Lespiau damien.lesp...@intel.com
Cc: Imre Deak imre.d...@intel.com
Cc: Sunil Kamath sunil.kam...@intel.com
Signed-off-by: Animesh Manna animesh.ma...@intel.com
Signed-off-by: Vathsala Nagaraju vathsala.nagar...@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.kam...@intel.com
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx