You believe suspend w/ DC6 should work due to the unconditional power-well reference, or you don't know and just couldn't merge since the patch didn’t apply?
Gavin Hindman Senior Program Manager SSG/OTC – Open Source Technology Center -----Original Message----- From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf Of Daniel Vetter Sent: Monday, September 28, 2015 12:22 AM To: Manna, Animesh <animesh.ma...@intel.com> Cc: Bhardwaj, Rajneesh <rajneesh.bhard...@intel.com>; intel-gfx@lists.freedesktop.org; Vetter, Daniel <daniel.vet...@intel.com> Subject: Re: [Intel-gfx] [DMC_BUGFIX_SKL_V2 3/5] drm/i915/skl: Making DC6 entry is the last call in suspend flow. On Wed, Aug 26, 2015 at 01:36:07AM +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. We unconditionaly grab a power well reference for everything in our suspend/resume functions, which means dc6/5 should be prevented for long enough. Also since dc6/5 are part of the power well framework you can't just enable/disable them directly, instead you need to get/put the corresponding power wells. Finally this patch doesn't just do that, but also frobs around a lot in set power well code itself, and I have no idea what it does there and why. It does smell a bit like you're just breaking dc6 for runtime pm though, which wouldn't be good. Anyway I decided to just merge this since this patch series has been floating around since forever, but then the patch didn't apply cleanly and so dropped it. -Daniel > > v1: Initial version. > > v2: Based on review comment from Daniel, > - created a seperate patch for csr uninitialization set call. > > 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> > Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhard...@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 13 +++++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > drivers/gpu/drm/i915/intel_runtime_pm.c | 19 +++++++------------ > 3 files changed, 22 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > b/drivers/gpu/drm/i915/i915_drv.c index 478101c..fa66162 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1013,10 +1013,20 @@ static int i915_pm_resume(struct device *dev) > > static int skl_suspend_complete(struct drm_i915_private *dev_priv) { > + enum csr_state state; > /* Enabling DC6 is not a hard requirement to enter runtime D3 */ > > skl_uninit_cdclk(dev_priv); > > + /* 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) > + skl_enable_dc6(dev_priv); > + > return 0; > } > > @@ -1063,6 +1073,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 81b7d77..9cb7d4e 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1118,6 +1118,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 821644d..23a3aa3 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -548,7 +548,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; > > @@ -565,7 +565,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; > > @@ -626,10 +626,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. > @@ -637,8 +637,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); @@ > -658,7 +656,7 > @@ 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)) && > + if (GEN9_ENABLE_DC5(dev) && > power_well->data == SKL_DISP_PW_2) { > enum csr_state state; > /* TODO: wait for a completion event or @@ > -671,10 +669,7 @@ > static void skl_set_power_well(struct drm_i915_private *dev_priv, > 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); > + 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
