[Intel-gfx] [SKL-DMC-BUGFIX 2/5] drm/i915/skl: Making DC6 entry is the last call in suspend flow.
Animesh Manna
animesh.manna at intel.com
Thu Aug 6 07:38:58 PDT 2015
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 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 | 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 at lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list