Re: [SKL-DMC-BUGFIX 2/5] drm/i915/skl: Making DC6 entry is the last call in suspend flow.

[Animesh Manna <animesh.manna@intel.com>]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx