| Message ID | 1443504719-27794-1-git-send-email-animesh.manna@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On ti, 2015-09-29 at 11:01 +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. > > v1: Initial version. > > v2: Based on review comment from Daniel, > - created a seperate patch for csr uninitialization set call. > > v3: Rebased on top of latest code. > > 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> > Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com> Acked-by: Imre Deak <imre.deak@intel.com> Suggestion for the commit message: Currently we keep DC6 enabled during modesets and DPAUX transfers, which is not allowed according to the specification. This can lead at least to PLL locking failures, DPAUX timeouts and prevent deeper package power states (PC9/10). Fix this for now by enabling DC6 only when we know the above events (modeset, DPAUX) can't happen. This a temporary solution as some issues are still unsolved as described in [1] and [2], we'll address those as a follow-up. [1] http://lists.freedesktop.org/archives/intel-gfx/2015-October/077669.html [2] http://lists.freedesktop.org/archives/intel-gfx/2015-October/077787.html > --- > 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 1cb6b82..51075d5 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1049,10 +1049,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; > } > > @@ -1099,6 +1109,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 c96289d..990161d 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1143,6 +1143,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 d8e9416..d6b4f61 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -551,7 +551,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; > > @@ -568,7 +568,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; > > @@ -629,10 +629,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. > @@ -640,8 +640,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); > @@ -667,7 +665,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv, > 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 > @@ -680,10 +678,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv, > DRM_DEBUG("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); > } > } > }
On Fri, Oct 16, 2015 at 03:22:45PM +0300, Imre Deak wrote: > On ti, 2015-09-29 at 11:01 +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. > > > > v1: Initial version. > > > > v2: Based on review comment from Daniel, > > - created a seperate patch for csr uninitialization set call. > > > > v3: Rebased on top of latest code. > > > > 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> > > Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com> > > Acked-by: Imre Deak <imre.deak@intel.com> > > Suggestion for the commit message: > > Currently we keep DC6 enabled during modesets and DPAUX transfers, which > is not allowed according to the specification. This can lead at least to > PLL locking failures, DPAUX timeouts and prevent deeper package power > states (PC9/10). Fix this for now by enabling DC6 only when we know the > above events (modeset, DPAUX) can't happen. > > This a temporary solution as some issues are still unsolved as described > in [1] and [2], we'll address those as a follow-up. > > [1] > http://lists.freedesktop.org/archives/intel-gfx/2015-October/077669.html > [2] > http://lists.freedesktop.org/archives/intel-gfx/2015-October/077787.html Queued for -next, thanks for the patch. -Daniel > > > > --- > > 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 1cb6b82..51075d5 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -1049,10 +1049,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; > > } > > > > @@ -1099,6 +1109,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 c96289d..990161d 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -1143,6 +1143,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 d8e9416..d6b4f61 100644 > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > > @@ -551,7 +551,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; > > > > @@ -568,7 +568,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; > > > > @@ -629,10 +629,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. > > @@ -640,8 +640,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); > > @@ -667,7 +665,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv, > > 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 > > @@ -680,10 +678,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv, > > DRM_DEBUG("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); > > } > > } > > } > > > _______________________________________________ > 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 1cb6b82..51075d5 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1049,10 +1049,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; } @@ -1099,6 +1109,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 c96289d..990161d 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1143,6 +1143,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 d8e9416..d6b4f61 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -551,7 +551,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; @@ -568,7 +568,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; @@ -629,10 +629,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. @@ -640,8 +640,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); @@ -667,7 +665,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv, 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 @@ -680,10 +678,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv, DRM_DEBUG("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); } } }