[Intel-gfx] [DMC_BUGFIX_SKL_V2 3/5] drm/i915/skl: Making DC6 entry is the last call in suspend flow.
Daniel Vetter
daniel at ffwll.ch
Tue Sep 29 02:01:35 PDT 2015
On Tue, Sep 29, 2015 at 11:08:25AM +0530, Animesh Manna wrote:
>
>
> On 09/28/2015 12:51 PM, Daniel Vetter wrote:
> >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.
>
> I mentioned in my commit message that we have a h/w workaround.
> "Mmio register access after dc6/dc5 entry is not allowed when
> DC6 power states are enabled"
>
> This patch is mandatory to solve the pc10 entry issue for skylake.
> Planning to send again after rebase on top of tree.
The problem isn't that the patch didn't apply cleanly, but that it seems
to have fundamental issues:
- We already grab power well references around suspend/resume code, see
the call to intel_power_domains_init_hw and the calls to
intel_display_set_init_power. That code is supposed to ensure that while
suspend/resume is going on _all_ power wells are on (and hence dc6/5 are
forbidden). It's a bit a big hammer, but we already have the code to do
exactly what you're trying to do here. It could be though that for skl
it's in the wrong order (but then the commit message must state very
clearly what's the exact depency, the gpu is assembled from a pile of
various different blocks which are all mostly independent).
- Your patch directly calls skl_enable/disable_dc6 instead of going
through the power well abstraction. Breaking these abstraction needs a
really good reason, adequate assurance that the refcounts are ok and
nothing breaks (using WARN_ON) and an explanation why the refcount
interface for display power management doesn't work.
- You also do changes to the power well code itself, which looks like it
might break dc6 for runtime pm (by only going into dc5 at most). That
needs to be a separate patch or have a much better explanation.
We probably need to set up a meeting to resolve this sooner than the few
months it took to figure out the resume issue ...
Thanks, Daniel
> >>
> >>v2: Based on review comment from Daniel,
> >>- created a seperate patch for csr uninitialization set call.
> >>
> >>Cc: Daniel Vetter <daniel.vetter at intel.com>
> >>Cc: Damien Lespiau <damien.lespiau at intel.com>
> >>Cc: Imre Deak <imre.deak at intel.com>
> >>Cc: Sunil Kamath <sunil.kamath at intel.com>
> >>Signed-off-by: Animesh Manna <animesh.manna at intel.com>
> >>Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju at intel.com>
> >>Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj at 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 at lists.freedesktop.org
> >>http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list