[04/18] drm/i915/gen9: block disable call for pw1 if dmc firmware is present.
diff mbox

Message ID 1437850839-16782-5-git-send-email-animesh.manna@intel.com
State New
Headers show

Commit Message

Animesh Manna July 25, 2015, 7 p.m. UTC
Grabbing a runtime pm reference with intel_runtime_pm_get will only
prevent device D3. But dmc firmware is required even earlier (namely
for the skl power well 1). DMC is responsible to save the status of
power well 1 and shut off the power well when panel is self refresh
mode of display is completely off.

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.

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 | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Daniel Vetter July 27, 2015, 8:48 a.m. UTC | #1
On Sun, Jul 26, 2015 at 12:30:25AM +0530, Animesh Manna wrote:
> Grabbing a runtime pm reference with intel_runtime_pm_get will only
> prevent device D3. But dmc firmware is required even earlier (namely
> for the skl power well 1). DMC is responsible to save the status of
> power well 1 and shut off the power well when panel is self refresh
> mode of display is completely off.
> 
> 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.
> 
> 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>

Please use the approach I've laid out in my original patch series with
"drm/i915: use correct power domain for csr loading" and "drm/i915: Only
allow rpm on gen9+ with dmc loaded". Your approach here requires a
flush_work in the runtime pm callbacks which can deadlock.

If you want to make dmc optional on bxt then you need to adjust the 2nd
patch a bit to no longer leak the runtime pm reference. Your approach here
is an inversion of control and that doesn't work well since control
inversions very easily lead to locking inversions.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 6393b76..e6156d5 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1141,8 +1141,10 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>  
>  	for_each_power_well_rev(i, power_well, BIT(domain), power_domains) {
>  		WARN_ON(!power_well->count);
> -
> -		if (!--power_well->count && i915.disable_power_well) {
> +		if (IS_SKYLAKE(dev_priv) && (power_well->data == SKL_DISP_PW_1) &&
> +			(intel_csr_load_status_get(dev_priv) == FW_LOADED))
> +			DRM_DEBUG_KMS("dmc will disable pw1");
> +		else if (!--power_well->count && i915.disable_power_well) {
>  			DRM_DEBUG_KMS("disabling %s\n", power_well->name);
>  			power_well->hw_enabled = false;
>  			power_well->ops->disable(dev_priv, power_well);
> -- 
> 2.0.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter July 28, 2015, 7:57 a.m. UTC | #2
On Mon, Jul 27, 2015 at 10:48:16AM +0200, Daniel Vetter wrote:
> On Sun, Jul 26, 2015 at 12:30:25AM +0530, Animesh Manna wrote:
> > Grabbing a runtime pm reference with intel_runtime_pm_get will only
> > prevent device D3. But dmc firmware is required even earlier (namely
> > for the skl power well 1). DMC is responsible to save the status of
> > power well 1 and shut off the power well when panel is self refresh
> > mode of display is completely off.
> > 
> > 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.
> > 
> > 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>
> 
> Please use the approach I've laid out in my original patch series with
> "drm/i915: use correct power domain for csr loading" and "drm/i915: Only
> allow rpm on gen9+ with dmc loaded". Your approach here requires a
> flush_work in the runtime pm callbacks which can deadlock.
> 
> If you want to make dmc optional on bxt then you need to adjust the 2nd
> patch a bit to no longer leak the runtime pm reference. Your approach here
> is an inversion of control and that doesn't work well since control
> inversions very easily lead to locking inversions.

Summary of what we just discussed offline:

Ok I was confused here with the intel_csr_load_status_get() check. If we
apply the above to patch from me first then we don't need that check any
more, and the patch itself looks good as a bugfix for skl dmc firmware
assumptions.
-Daniel
Kamath, Sunil July 28, 2015, 10:28 a.m. UTC | #3
On Tuesday 28 July 2015 01:27 PM, Daniel Vetter wrote:
> On Mon, Jul 27, 2015 at 10:48:16AM +0200, Daniel Vetter wrote:
>> On Sun, Jul 26, 2015 at 12:30:25AM +0530, Animesh Manna wrote:
>>> Grabbing a runtime pm reference with intel_runtime_pm_get will only
>>> prevent device D3. But dmc firmware is required even earlier (namely
>>> for the skl power well 1). DMC is responsible to save the status of
>>> power well 1 and shut off the power well when panel is self refresh
>>> mode of display is completely off.
>>>
>>> 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.
>>>
>>> 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>
>> Please use the approach I've laid out in my original patch series with
>> "drm/i915: use correct power domain for csr loading" and "drm/i915: Only
>> allow rpm on gen9+ with dmc loaded". Your approach here requires a
>> flush_work in the runtime pm callbacks which can deadlock.
>>
>> If you want to make dmc optional on bxt then you need to adjust the 2nd
>> patch a bit to no longer leak the runtime pm reference. Your approach here
>> is an inversion of control and that doesn't work well since control
>> inversions very easily lead to locking inversions.
> Summary of what we just discussed offline:
>
> Ok I was confused here with the intel_csr_load_status_get() check. If we
> apply the above to patch from me first then we don't need that check any
> more, and the patch itself looks good as a bugfix for skl dmc firmware
> assumptions.
> -Daniel
Thanks alot Damien.

Animesh,
As discussed in same call, bug fix should go initial patches in this series.

also its good to add the check in skl_set_power_well function itself 
than intel_display_power_put

- Sunil

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 6393b76..e6156d5 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1141,8 +1141,10 @@  void intel_display_power_put(struct drm_i915_private *dev_priv,
 
 	for_each_power_well_rev(i, power_well, BIT(domain), power_domains) {
 		WARN_ON(!power_well->count);
-
-		if (!--power_well->count && i915.disable_power_well) {
+		if (IS_SKYLAKE(dev_priv) && (power_well->data == SKL_DISP_PW_1) &&
+			(intel_csr_load_status_get(dev_priv) == FW_LOADED))
+			DRM_DEBUG_KMS("dmc will disable pw1");
+		else if (!--power_well->count && i915.disable_power_well) {
 			DRM_DEBUG_KMS("disabling %s\n", power_well->name);
 			power_well->hw_enabled = false;
 			power_well->ops->disable(dev_priv, power_well);