| Message ID | 1437850839-16782-5-git-send-email-animesh.manna@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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);