| Message ID | 1438619136-18268-3-git-send-email-animesh.manna@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Monday 03 August 2015 09:55 PM, 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: commit message updated based on comment from Vathsala. > > 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> > --- > drivers/gpu/drm/i915/i915_drv.c | 12 ++++++------ > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > drivers/gpu/drm/i915/intel_runtime_pm.c | 33 ++++++++------------------------- > 3 files changed, 16 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 0d6775a..e1d0102 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1017,14 +1017,11 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv) > { > /* Enabling DC6 is not a hard requirement to enter runtime D3 */ > > - /* > - * This is to ensure that CSR isn't identified as loaded before > - * CSR-loading program is called during runtime-resume. > - */ > - intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED); > - > skl_uninit_cdclk(dev_priv); > > + if (intel_csr_load_status_get(dev_priv) == FW_LOADED) > + skl_enable_dc6(dev_priv); > + > return 0; > } > > @@ -1071,6 +1068,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 47cef0e..06f346f 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1117,6 +1117,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 6393b76..c660245 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -532,7 +532,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; > > @@ -549,7 +549,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; > > @@ -610,10 +610,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. > @@ -621,8 +621,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); > @@ -642,24 +640,9 @@ 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)) && > - power_well->data == SKL_DISP_PW_2) { > - enum csr_state state; > - /* 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) > - 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); > - } > + if (GEN9_ENABLE_DC5(dev) && > + power_well->data == SKL_DISP_PW_2) > + gen9_enable_dc5(dev_priv); > } > } > Same comment as before: This is really right change. With this we will go back to our original design. Deepest possible display state will be triggered by respective suspend complete. for example in BXT we trigger DC9 from bxt_suspend_complete and works fine with rpm integrated. Same case here for SKL for DC6 which uses DMC. Right bug fix. Reviewed-by: A.Sunil Kamath <sunil.kamath@intel.com>
On Mon, Aug 03, 2015 at 09:55:33PM +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: commit message updated based on comment from Vathsala. > > 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> > --- > drivers/gpu/drm/i915/i915_drv.c | 12 ++++++------ > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > drivers/gpu/drm/i915/intel_runtime_pm.c | 33 ++++++++------------------------- > 3 files changed, 16 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 0d6775a..e1d0102 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1017,14 +1017,11 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv) > { > /* Enabling DC6 is not a hard requirement to enter runtime D3 */ > > - /* > - * This is to ensure that CSR isn't identified as loaded before > - * CSR-loading program is called during runtime-resume. > - */ > - intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED); Seems like an unrelated hunk. Separate patch (in the dmc loader rework series) or an explanation why we need this. > - > skl_uninit_cdclk(dev_priv); > > + if (intel_csr_load_status_get(dev_priv) == FW_LOADED) > + skl_enable_dc6(dev_priv); > + > return 0; > } > > @@ -1071,6 +1068,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 47cef0e..06f346f 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1117,6 +1117,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 6393b76..c660245 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -532,7 +532,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; > > @@ -549,7 +549,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; Everything above seems to roughly be matching your patch description, but not perfectly: You talk about suspend flow but also touch resume flow. But the hunks below are completely unexplained magic afaict. Either this needs a separate patch or it needs seriously more explanation of what's going on. > > @@ -610,10 +610,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. > @@ -621,8 +621,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); > @@ -642,24 +640,9 @@ 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)) && > - power_well->data == SKL_DISP_PW_2) { > - enum csr_state state; > - /* 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) > - 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); > - } > + if (GEN9_ENABLE_DC5(dev) && > + power_well->data == SKL_DISP_PW_2) > + 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
On Tue, Aug 04, 2015 at 04:55:59PM +0530, Sunil Kamath wrote: > On Monday 03 August 2015 09:55 PM, 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: commit message updated based on comment from Vathsala. > > > >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> > >--- > > drivers/gpu/drm/i915/i915_drv.c | 12 ++++++------ > > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > > drivers/gpu/drm/i915/intel_runtime_pm.c | 33 ++++++++------------------------- > > 3 files changed, 16 insertions(+), 31 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > >index 0d6775a..e1d0102 100644 > >--- a/drivers/gpu/drm/i915/i915_drv.c > >+++ b/drivers/gpu/drm/i915/i915_drv.c > >@@ -1017,14 +1017,11 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv) > > { > > /* Enabling DC6 is not a hard requirement to enter runtime D3 */ > >- /* > >- * This is to ensure that CSR isn't identified as loaded before > >- * CSR-loading program is called during runtime-resume. > >- */ > >- intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED); > >- > > skl_uninit_cdclk(dev_priv); > >+ if (intel_csr_load_status_get(dev_priv) == FW_LOADED) > >+ skl_enable_dc6(dev_priv); > >+ > > return 0; > > } > >@@ -1071,6 +1068,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 47cef0e..06f346f 100644 > >--- a/drivers/gpu/drm/i915/intel_drv.h > >+++ b/drivers/gpu/drm/i915/intel_drv.h > >@@ -1117,6 +1117,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 6393b76..c660245 100644 > >--- a/drivers/gpu/drm/i915/intel_runtime_pm.c > >+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > >@@ -532,7 +532,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; > >@@ -549,7 +549,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; > >@@ -610,10 +610,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. > >@@ -621,8 +621,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); > >@@ -642,24 +640,9 @@ 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)) && > >- power_well->data == SKL_DISP_PW_2) { > >- enum csr_state state; > >- /* 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) > >- 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); > >- } > >+ if (GEN9_ENABLE_DC5(dev) && > >+ power_well->data == SKL_DISP_PW_2) > >+ gen9_enable_dc5(dev_priv); > > } > > } > Same comment as before: > > This is really right change. > With this we will go back to our original design. If this reverts something ("go back to our original design") then we need to mention the patches this undoes in the commit message. -Daniel > Deepest possible display state will be triggered by respective suspend > complete. > for example in BXT we trigger DC9 from bxt_suspend_complete and works fine > with rpm integrated. > Same case here for SKL for DC6 which uses DMC. > > Right bug fix. > > Reviewed-by: A.Sunil Kamath <sunil.kamath@intel.com> > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 8/5/2015 2:35 PM, Daniel Vetter wrote: > On Mon, Aug 03, 2015 at 09:55:33PM +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: commit message updated based on comment from Vathsala. >> >> 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> >> --- >> drivers/gpu/drm/i915/i915_drv.c | 12 ++++++------ >> drivers/gpu/drm/i915/intel_drv.h | 2 ++ >> drivers/gpu/drm/i915/intel_runtime_pm.c | 33 ++++++++------------------------- >> 3 files changed, 16 insertions(+), 31 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c >> index 0d6775a..e1d0102 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.c >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> @@ -1017,14 +1017,11 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv) >> { >> /* Enabling DC6 is not a hard requirement to enter runtime D3 */ >> >> - /* >> - * This is to ensure that CSR isn't identified as loaded before >> - * CSR-loading program is called during runtime-resume. >> - */ >> - intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED); > Seems like an unrelated hunk. Separate patch (in the dmc loader rework > series) or an explanation why we need this. In the same skl_suspend_complete() later we are checking if firmware is loaded, based on that we trigger dc6, so the above hunk has to be removed in this patch. I will add explanation in my next patchset. On the other hand firmware team confirmed that one time firmware loading during driver loading is sufficient, no need to load firmware in csr-address-space every suspend (dc6 entry) - resume (dc6 exit) flow, dmc will take care of it which eliminate any chance of regression. - Animesh > >> - >> skl_uninit_cdclk(dev_priv); >> >> + if (intel_csr_load_status_get(dev_priv) == FW_LOADED) >> + skl_enable_dc6(dev_priv); >> + >> return 0; >> } >> >> @@ -1071,6 +1068,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 47cef0e..06f346f 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -1117,6 +1117,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 6393b76..c660245 100644 >> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c >> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c >> @@ -532,7 +532,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; >> >> @@ -549,7 +549,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; > Everything above seems to roughly be matching your patch description, but > not perfectly: You talk about suspend flow but also touch resume flow. > > But the hunks below are completely unexplained magic afaict. Either this > needs a separate patch or it needs seriously more explanation of what's > going on. > >> >> @@ -610,10 +610,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. >> @@ -621,8 +621,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); >> @@ -642,24 +640,9 @@ 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)) && >> - power_well->data == SKL_DISP_PW_2) { >> - enum csr_state state; >> - /* 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) >> - 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); >> - } >> + if (GEN9_ENABLE_DC5(dev) && >> + power_well->data == SKL_DISP_PW_2) >> + 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
On Thu, Aug 06, 2015 at 02:47:22PM +0530, Animesh Manna wrote: > > > On 8/5/2015 2:35 PM, Daniel Vetter wrote: > >On Mon, Aug 03, 2015 at 09:55:33PM +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: commit message updated based on comment from Vathsala. > >> > >>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> > >>--- > >> drivers/gpu/drm/i915/i915_drv.c | 12 ++++++------ > >> drivers/gpu/drm/i915/intel_drv.h | 2 ++ > >> drivers/gpu/drm/i915/intel_runtime_pm.c | 33 ++++++++------------------------- > >> 3 files changed, 16 insertions(+), 31 deletions(-) > >> > >>diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > >>index 0d6775a..e1d0102 100644 > >>--- a/drivers/gpu/drm/i915/i915_drv.c > >>+++ b/drivers/gpu/drm/i915/i915_drv.c > >>@@ -1017,14 +1017,11 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv) > >> { > >> /* Enabling DC6 is not a hard requirement to enter runtime D3 */ > >>- /* > >>- * This is to ensure that CSR isn't identified as loaded before > >>- * CSR-loading program is called during runtime-resume. > >>- */ > >>- intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED); > >Seems like an unrelated hunk. Separate patch (in the dmc loader rework > >series) or an explanation why we need this. > > In the same skl_suspend_complete() later we are checking if firmware is loaded, > based on that we trigger dc6, so the above hunk has to be removed in this patch. > I will add explanation in my next patchset. I know that later on we'll replace this with something else, but you can't remove old code before the new code is there. And you can't do changes like this here in unrelated patches. > On the other hand firmware team confirmed that one time firmware loading > during driver loading is sufficient, no need to load firmware in > csr-address-space every suspend (dc6 entry) - resume (dc6 exit) flow, dmc will > take care of it which eliminate any chance of regression. And what about hibernate? -Daniel > > - Animesh > > > > >>- > >> skl_uninit_cdclk(dev_priv); > >>+ if (intel_csr_load_status_get(dev_priv) == FW_LOADED) > >>+ skl_enable_dc6(dev_priv); > >>+ > >> return 0; > >> } > >>@@ -1071,6 +1068,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 47cef0e..06f346f 100644 > >>--- a/drivers/gpu/drm/i915/intel_drv.h > >>+++ b/drivers/gpu/drm/i915/intel_drv.h > >>@@ -1117,6 +1117,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 6393b76..c660245 100644 > >>--- a/drivers/gpu/drm/i915/intel_runtime_pm.c > >>+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > >>@@ -532,7 +532,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; > >>@@ -549,7 +549,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; > >Everything above seems to roughly be matching your patch description, but > >not perfectly: You talk about suspend flow but also touch resume flow. > > > >But the hunks below are completely unexplained magic afaict. Either this > >needs a separate patch or it needs seriously more explanation of what's > >going on. > > > >>@@ -610,10 +610,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. > >>@@ -621,8 +621,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); > >>@@ -642,24 +640,9 @@ 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)) && > >>- power_well->data == SKL_DISP_PW_2) { > >>- enum csr_state state; > >>- /* 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) > >>- 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); > >>- } > >>+ if (GEN9_ENABLE_DC5(dev) && > >>+ power_well->data == SKL_DISP_PW_2) > >>+ 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 >
On 8/6/2015 6:48 PM, Daniel Vetter wrote: > On Thu, Aug 06, 2015 at 02:47:22PM +0530, Animesh Manna wrote: >> >> On 8/5/2015 2:35 PM, Daniel Vetter wrote: >>> On Mon, Aug 03, 2015 at 09:55:33PM +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: commit message updated based on comment from Vathsala. >>>> >>>> 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> >>>> --- >>>> drivers/gpu/drm/i915/i915_drv.c | 12 ++++++------ >>>> drivers/gpu/drm/i915/intel_drv.h | 2 ++ >>>> drivers/gpu/drm/i915/intel_runtime_pm.c | 33 ++++++++------------------------- >>>> 3 files changed, 16 insertions(+), 31 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c >>>> index 0d6775a..e1d0102 100644 >>>> --- a/drivers/gpu/drm/i915/i915_drv.c >>>> +++ b/drivers/gpu/drm/i915/i915_drv.c >>>> @@ -1017,14 +1017,11 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv) >>>> { >>>> /* Enabling DC6 is not a hard requirement to enter runtime D3 */ >>>> - /* >>>> - * This is to ensure that CSR isn't identified as loaded before >>>> - * CSR-loading program is called during runtime-resume. >>>> - */ >>>> - intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED); >>> Seems like an unrelated hunk. Separate patch (in the dmc loader rework >>> series) or an explanation why we need this. >> In the same skl_suspend_complete() later we are checking if firmware is loaded, >> based on that we trigger dc6, so the above hunk has to be removed in this patch. > I know that later on we'll replace this with something else, but you can't > remove old code before the new code is there. And you can't do changes > like this here in unrelated patches. code snippet added in this patch: + if (intel_csr_load_status_get(dev_priv) == FW_LOADED) + skl_enable_dc6(dev_priv); We can add uninitilized call here after this which will be related changes. After dmc redesign will remove it completely the function call for csr uninitialization. > >> On the other hand firmware team confirmed that one time firmware loading >> during driver loading is sufficient, no need to load firmware in >> csr-address-space every suspend (dc6 entry) - resume (dc6 exit) flow, dmc will >> take care of it which eliminate any chance of regression. > And what about hibernate? Yes, during hibernate I assumed os will restore the ram content from disk. But specially for csr program we need to load it again. Thanks for the pointer. -Animesh > -Daniel >> - Animesh >> >>>> - >>>> skl_uninit_cdclk(dev_priv); >>>> + if (intel_csr_load_status_get(dev_priv) == FW_LOADED) >>>> + skl_enable_dc6(dev_priv); >>>> + >>>> return 0; >>>> } >>>> @@ -1071,6 +1068,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 47cef0e..06f346f 100644 >>>> --- a/drivers/gpu/drm/i915/intel_drv.h >>>> +++ b/drivers/gpu/drm/i915/intel_drv.h >>>> @@ -1117,6 +1117,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 6393b76..c660245 100644 >>>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c >>>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c >>>> @@ -532,7 +532,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; >>>> @@ -549,7 +549,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; >>> Everything above seems to roughly be matching your patch description, but >>> not perfectly: You talk about suspend flow but also touch resume flow. >>> >>> But the hunks below are completely unexplained magic afaict. Either this >>> needs a separate patch or it needs seriously more explanation of what's >>> going on. >>> >>>> @@ -610,10 +610,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. >>>> @@ -621,8 +621,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); >>>> @@ -642,24 +640,9 @@ 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)) && >>>> - power_well->data == SKL_DISP_PW_2) { >>>> - enum csr_state state; >>>> - /* 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) >>>> - 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); >>>> - } >>>> + if (GEN9_ENABLE_DC5(dev) && >>>> + power_well->data == SKL_DISP_PW_2) >>>> + 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
On Thu, Aug 06, 2015 at 08:08:58PM +0530, Animesh Manna wrote: > > > On 8/6/2015 6:48 PM, Daniel Vetter wrote: > >On Thu, Aug 06, 2015 at 02:47:22PM +0530, Animesh Manna wrote: > >> > >>On 8/5/2015 2:35 PM, Daniel Vetter wrote: > >>>On Mon, Aug 03, 2015 at 09:55:33PM +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: commit message updated based on comment from Vathsala. > >>>> > >>>>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> > >>>>--- > >>>> drivers/gpu/drm/i915/i915_drv.c | 12 ++++++------ > >>>> drivers/gpu/drm/i915/intel_drv.h | 2 ++ > >>>> drivers/gpu/drm/i915/intel_runtime_pm.c | 33 ++++++++------------------------- > >>>> 3 files changed, 16 insertions(+), 31 deletions(-) > >>>> > >>>>diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > >>>>index 0d6775a..e1d0102 100644 > >>>>--- a/drivers/gpu/drm/i915/i915_drv.c > >>>>+++ b/drivers/gpu/drm/i915/i915_drv.c > >>>>@@ -1017,14 +1017,11 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv) > >>>> { > >>>> /* Enabling DC6 is not a hard requirement to enter runtime D3 */ > >>>>- /* > >>>>- * This is to ensure that CSR isn't identified as loaded before > >>>>- * CSR-loading program is called during runtime-resume. > >>>>- */ > >>>>- intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED); > >>>Seems like an unrelated hunk. Separate patch (in the dmc loader rework > >>>series) or an explanation why we need this. > >>In the same skl_suspend_complete() later we are checking if firmware is loaded, > >>based on that we trigger dc6, so the above hunk has to be removed in this patch. > >I know that later on we'll replace this with something else, but you can't > >remove old code before the new code is there. And you can't do changes > >like this here in unrelated patches. > > code snippet added in this patch: > + if (intel_csr_load_status_get(dev_priv) == FW_LOADED) > + skl_enable_dc6(dev_priv); > > We can add uninitilized call here after this which will be related changes. > After dmc redesign will remove it completely the function call for csr uninitialization. Oh right I was getting a bit confused here, that change does make sense. But the patch should explain better why/how this all gets changed. Obviously if we keep on setting status to UNITIALIZED then this check here will always fall over. Might be good to split up this patch into parts 1. Remove the UNITIALIZED from suspend path since dmc stays around. Also handle re-loading on hibernate resume correctly. 2. Adjust ordering between dc5 and dc6 for dmc. I think there's still more parts in this patch, e.g. you also remove the wait_for ... > >>On the other hand firmware team confirmed that one time firmware loading > >>during driver loading is sufficient, no need to load firmware in > >>csr-address-space every suspend (dc6 entry) - resume (dc6 exit) flow, dmc will > >>take care of it which eliminate any chance of regression. > >And what about hibernate? > > Yes, during hibernate I assumed os will restore the ram content from disk. > But specially for csr program we need to load it again. Thanks for the pointer. Hibernate is really tricky, so please try to test that fully. The big risk is resume: - first we load a normal kernel, including i915 and everything - then that first kernel loads the hibernate image - then all hw gets shut down like with a normal suspend call - then we resume everything again Given all this we might need to check the hw state itself whether dmc is loaded already and only load it if that's the case. Otherwise bad stuff might happen. -Daniel > > > -Animesh > > >-Daniel > >>- Animesh > >> > >>>>- > >>>> skl_uninit_cdclk(dev_priv); > >>>>+ if (intel_csr_load_status_get(dev_priv) == FW_LOADED) > >>>>+ skl_enable_dc6(dev_priv); > >>>>+ > >>>> return 0; > >>>> } > >>>>@@ -1071,6 +1068,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 47cef0e..06f346f 100644 > >>>>--- a/drivers/gpu/drm/i915/intel_drv.h > >>>>+++ b/drivers/gpu/drm/i915/intel_drv.h > >>>>@@ -1117,6 +1117,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 6393b76..c660245 100644 > >>>>--- a/drivers/gpu/drm/i915/intel_runtime_pm.c > >>>>+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > >>>>@@ -532,7 +532,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; > >>>>@@ -549,7 +549,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; > >>>Everything above seems to roughly be matching your patch description, but > >>>not perfectly: You talk about suspend flow but also touch resume flow. > >>> > >>>But the hunks below are completely unexplained magic afaict. Either this > >>>needs a separate patch or it needs seriously more explanation of what's > >>>going on. > >>> > >>>>@@ -610,10 +610,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. > >>>>@@ -621,8 +621,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); > >>>>@@ -642,24 +640,9 @@ 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)) && > >>>>- power_well->data == SKL_DISP_PW_2) { > >>>>- enum csr_state state; > >>>>- /* 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) > >>>>- 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); > >>>>- } > >>>>+ if (GEN9_ENABLE_DC5(dev) && > >>>>+ power_well->data == SKL_DISP_PW_2) > >>>>+ 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 >
On ma, 2015-08-03 at 21:55 +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. The MMIO range BSpec-ID 0527 refers to is the DMC MMIO range. The driver uses this MMIO range only to program the FW image, it doesn't access it afterwards. So that's not a good justification to keep DC6 disabled. That BSpec-ID also mentions that DC6 together with DC5 needs to be disabled around modesets, which we need to address by disabling both DC5/6, but only around modesets. Later discussions with HW people revealed that there is an open issue related to DC6, see [1]. The suggested workaround for that issue is to keep DC6 disabled all the time and use a manual sequence to enable deeper power states (see "Sequence for Software to Allow Package C9-C10" in BSpec). Based on this what we need to do in this patch is simply disable DC6. As a follow-up to this patchset we need to: - Add the manual PC9/10 sequence. - Prevent DC5/6 during modesets (and DPAUX transfers). - Move out the remaining parts of the display init/uninit sequence from the suspend/resume path. - Add a module options to select between enabling DC6 and running the manual PC9/10 sequence. People still would like to experiment with DC6 and we may end up enabling it in the end if all the open issues get resolved. For that case enabling DC6 should still happen at the place where it happens now, that is after disabling PW2. Please see my corresponding comments inlined below. [1] http://lists.freedesktop.org/archives/intel-gfx/2015-October/077669.html > > v1: Initial version. > > v2: commit message updated based on comment from Vathsala. > > 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> > --- > drivers/gpu/drm/i915/i915_drv.c | 12 ++++++------ > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > drivers/gpu/drm/i915/intel_runtime_pm.c | 33 ++++++++------------------------- > 3 files changed, 16 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 0d6775a..e1d0102 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1017,14 +1017,11 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv) > { > /* Enabling DC6 is not a hard requirement to enter runtime D3 */ > > - /* > - * This is to ensure that CSR isn't identified as loaded before > - * CSR-loading program is called during runtime-resume. > - */ > - intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED); > - > skl_uninit_cdclk(dev_priv); > > + if (intel_csr_load_status_get(dev_priv) == FW_LOADED) > + skl_enable_dc6(dev_priv); > + Not needed, atm we shouldn't enable DC6 ever due to open issues under investigation. Even if those investigations reveal that we can use DC6 it shouldn't be enabled here, rather at its current place after disabling PW2. > return 0; > } > > @@ -1071,6 +1068,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); > + Not needed based on the previous comment. > 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 47cef0e..06f346f 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1117,6 +1117,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 6393b76..c660245 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -532,7 +532,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; > > @@ -549,7 +549,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; > > @@ -610,10 +610,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. > @@ -621,8 +621,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); Instead of this please just redefine GEN9_ENABLE_DC5 and SKL_ENABLE_DC6 accordingly. We want to keep the possibility to use DC6, (adding a module option for it as a follow-up), and then this is still the right place to disable it. > } > } > I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask); > @@ -642,24 +640,9 @@ 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)) && > - power_well->data == SKL_DISP_PW_2) { > - enum csr_state state; > - /* 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) > - 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); > - } > + if (GEN9_ENABLE_DC5(dev) && > + power_well->data == SKL_DISP_PW_2) > + gen9_enable_dc5(dev_priv); Removing the blocking wait is ok, though you should have mentioned in the commit log why and have this change in a separate patch. Instead of removing skl_enable_dc6() please redefine the macros as explained above. --Imre > } > } >
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 0d6775a..e1d0102 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1017,14 +1017,11 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv) { /* Enabling DC6 is not a hard requirement to enter runtime D3 */ - /* - * This is to ensure that CSR isn't identified as loaded before - * CSR-loading program is called during runtime-resume. - */ - intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED); - skl_uninit_cdclk(dev_priv); + if (intel_csr_load_status_get(dev_priv) == FW_LOADED) + skl_enable_dc6(dev_priv); + return 0; } @@ -1071,6 +1068,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 47cef0e..06f346f 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1117,6 +1117,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 6393b76..c660245 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -532,7 +532,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; @@ -549,7 +549,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; @@ -610,10 +610,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. @@ -621,8 +621,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); @@ -642,24 +640,9 @@ 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)) && - power_well->data == SKL_DISP_PW_2) { - enum csr_state state; - /* 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) - 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); - } + if (GEN9_ENABLE_DC5(dev) && + power_well->data == SKL_DISP_PW_2) + gen9_enable_dc5(dev_priv); } }