| Message ID | 1437850839-16782-15-git-send-email-animesh.manna@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Jul 26, 2015 at 12:30:35AM +0530, Animesh Manna wrote: > As power well 1 is superset of power well 2 and always pw2 > will be disabled first and then pw1. On the other hand dmc > is responsible to save & restore back pw1 when display > engine goes and come back from low power state. Before > disabling pw1 dmc must be loaded, so adding flush_work() > while disabling pw2 which ensure that firmware will be > available before disabling pw1 in suspend flow. > > 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> > --- > drivers/gpu/drm/i915/i915_drv.c | 2 -- > drivers/gpu/drm/i915/intel_csr.c | 2 ++ > drivers/gpu/drm/i915/intel_runtime_pm.c | 1 + > 3 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 98343eb..ddf8a25 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -995,8 +995,6 @@ static int i915_pm_resume(struct device *dev) > > static int skl_suspend_complete(struct drm_i915_private *dev_priv) > { > - /* Enabling DC6 is not a hard requirement to enter runtime D3 */ Don't we need a flush_work here to make sure dmc is ready before going into suspend? > - > skl_uninit_cdclk(dev_priv); > > return 0; > diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c > index 36386324..1858f02 100644 > --- a/drivers/gpu/drm/i915/intel_csr.c > +++ b/drivers/gpu/drm/i915/intel_csr.c > @@ -417,5 +417,7 @@ void intel_csr_ucode_fini(struct drm_i915_private *dev_priv) > if (!HAS_CSR(dev_priv)) > return; > > + flush_work(&dev_priv->csr.work); > + > kfree(dev_priv->csr.dmc_payload); > } > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > index 5f1ae23..a5059e8 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -652,6 +652,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv, > > if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) && > power_well->data == SKL_DISP_PW_2) { > + flush_work(&dev_priv->csr.work); This flush_work here has two problems: - It's an inversion of control since this is low-level code blocking on high-level. I think this is here is safe but in general it's very easy to create deadlocks and hence this pattern is discouraged. - dmc loading might take a while (especially on cros/android where i915 is built-in but fw needs to wait for the full system before it can be loaded). We do it asynchronously so that i915 init can proceed and we can enable outputs really early, but the modeset code also does power well get/put calls and might end up in here, resulting in a stall. If we instead prevent execution of this low-level power-well code by grabbing a power_domain reference then both problems are solved: - No flush_work needed since we'll only get into this code when the last power domain references is dropped, which necessarily means dmc loader has completed. - No blocking of modesets while dmc loading is still in progress since the get/put power_domain calls will simply be no-ops too. Long story short: If you apply the two patches I mentioned in my reply to patch 4 there shouldn't be a need for this flush_work here in skl_set_power_well. Thanks, Daniel > if (SKL_ENABLE_DC6(dev)) > skl_enable_dc6(dev_priv); > else > -- > 2.0.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 98343eb..ddf8a25 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -995,8 +995,6 @@ static int i915_pm_resume(struct device *dev) static int skl_suspend_complete(struct drm_i915_private *dev_priv) { - /* Enabling DC6 is not a hard requirement to enter runtime D3 */ - skl_uninit_cdclk(dev_priv); return 0; diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c index 36386324..1858f02 100644 --- a/drivers/gpu/drm/i915/intel_csr.c +++ b/drivers/gpu/drm/i915/intel_csr.c @@ -417,5 +417,7 @@ void intel_csr_ucode_fini(struct drm_i915_private *dev_priv) if (!HAS_CSR(dev_priv)) return; + flush_work(&dev_priv->csr.work); + kfree(dev_priv->csr.dmc_payload); } diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index 5f1ae23..a5059e8 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -652,6 +652,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv, if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) && power_well->data == SKL_DISP_PW_2) { + flush_work(&dev_priv->csr.work); if (SKL_ENABLE_DC6(dev)) skl_enable_dc6(dev_priv); else
As power well 1 is superset of power well 2 and always pw2 will be disabled first and then pw1. On the other hand dmc is responsible to save & restore back pw1 when display engine goes and come back from low power state. Before disabling pw1 dmc must be loaded, so adding flush_work() while disabling pw2 which ensure that firmware will be available before disabling pw1 in suspend flow. 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> --- drivers/gpu/drm/i915/i915_drv.c | 2 -- drivers/gpu/drm/i915/intel_csr.c | 2 ++ drivers/gpu/drm/i915/intel_runtime_pm.c | 1 + 3 files changed, 3 insertions(+), 2 deletions(-)