Re: [PATCH 1/1] drm/i915: Adding Gfx Clock, Wake and Gunit save/restore logic in PM suspend/resume paths.

[Imre Deak <imre.deak@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5905 bytes --]

On Fri, 2014-08-08 at 15:54 +0530, Sagar Arun Kamble wrote:
> On Fri, 2014-08-08 at 11:15 +0200, Daniel Vetter wrote:
> > On Fri, Aug 08, 2014 at 02:29:38PM +0530, Sagar Arun Kamble wrote:
> > > On Fri, 2014-08-08 at 09:42 +0200, Daniel Vetter wrote:
> > > > On Fri, Aug 08, 2014 at 12:22:44PM +0530, Sagar Arun Kamble wrote:
> > > > > Hi Daniel,
> > > > > On Mon, 2014-08-04 at 10:07 +0200, Daniel Vetter wrote:
> > > > > > On Fri, Aug 01, 2014 at 12:34:56PM +0530, sagar.a.kamble@intel.com wrote:
> > > > > > > From: Sagar Kamble <sagar.a.kamble@intel.com>
> > > > > > > @@ -562,7 +567,12 @@ static int i915_drm_freeze(struct drm_device *dev)
> > > > > > >  
> > > > > > >  	intel_display_set_init_power(dev_priv, false);
> > > > > > >  
> > > > > > > -	return 0;
> > > > > > > +	/* Save Gunit State and clear wake - Need to make sure
> > > > > > > +	 * changes in vlv_runtime_suspend path don't impact this path */
> > > > > > > +	if (IS_VALLEYVIEW(dev))
> > > > > > > +		ret = vlv_runtime_suspend(dev_priv);
> > > > > > 
> > > > > > Maybe I wasn't clear, but I absolutely don't want any IS_VLV additions to
> > > > > > core resume/thaw code. This should be shovelled into the runtime pm
> > > > > > handling code, which should be reused in the suspend/resume code.
> > > > > This piece of code does not fit into any of the power well get/put path.
> > > > > Its specific sequence that need to be followed in VLV when Gunit gets
> > > > > power gated. So we have to keep this IS_VLV related functionality in
> > > > > both runtime and pm suspend/resume.
> > > > 
> > > > Well we support S0ix now. Which means our system suspend/resume code
> > > > actually calls into the runtime pm code. So either that design is broken
> > > > (and we need to fix this) or something else is amiss. Or we don't need
> > > > this code any more.
> > > > 
> > > > But duplicating it is not the right approach. And yeah the power domain
> > > > stuff might not be the right place, I've just used that as a place-holder
> > > > for all the runtime pm code we have.
> > > > -Daniel
> > > While entering S0iX we are getting following call flow:
> > > intel_runtime_resume followed by i915_drm_freeze 
> > > and While resuming back from S0iX:
> > > i915_drm_thaw
> > > 
> > > How can we share that wake control, gunit save/restore functionality?
> > > 
> > > I am observing issue given below:
> > > 
> > > 
> > > Here is how I am achieving S0ix:
> > > 1. echo mem > /sys/power/state
> > > 2. (Assuming we have removed all wake_locks in /sys/powe/wake_lock)
> > >         If device is runtime suspend it is getting resumed with
> > > runtime_resume
> > > 3. PM core triggers this sequence for each device
> > > (prepare, suspend, suspend_late, suspend_noirq)
> > > 4. Then pm_suspend happens for Gfx.
> > > 
> > > After all devices are suspended S0i0->S0iR->S0i1->S0i2->S0i3 happens.
> > > Gunit gets power gated.
> > > 
> > > While resuming back (with user action/power button press)
> > > 1. PM core triggers following sequenc for each device:
> > > (resume_noirq, resume_early, resume, complete)
> > > 
> > > After call to i915_drm_thaw_early(called through resume_early),
> > > during i915_drm_thaw(called through resume) we were seeing following
> > > issue during ring initialization.
> > > This is happening since wake is not enabled and Gunit registers are not
> > > restored(which is done in runtime_resume but that path is not taken here
> > > since this is resume from pm_suspend)
> > 
> > We have intel_runtime_pm_get/put calls in the suspend/resume paths which
> > should achieve this. Maybe they're not at the right place or not in the
> > right ordering, but they're there. So on platforms with runtime pm support
> > we _do_ call all the relevant runtime pm callbacks from system
> > suspend/resume.
> > -Daniel
> I am seeing following commit removed get/put calls from suspend resume
> paths:
> commit 395a5abbd97d7d76a7a26da52f33daebe279b3b3
> Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> Date:   Wed Jun 18 09:52:56 2014 -0700
> 
>     drm/i915: don't take runtime PM reference around freeze/thaw
>     
> 
> That ordering of get/put was also incorrect probably. Even though we do
> rpm_get before freeze after S0i3 we need to explicitely do wake
> contro/gunit save restore.
> 
> 
> In the current patch, the rpm handlers are called directly instead of
> (intel_runtime_pm_get/put).
> 
> valleyview_runtime_suspend at the end of i915_drm_freeze and 
> valleyview_runtime_resume at the beginning of i915_drm_thaw_early.
> 
> Do I need to replace them with intel_display_power_get/put? Will DPM
> core allow rpm calls when system pm suspend/resume is in progress?

No, you can't make the device runtime suspend while the system suspend
handler runs since DPM takes an RPM reference before calling this
handler. The reference will be dropped only after returning from the
corresponding system resume handler.

> > > <6>[ 3152.991399] PM: device[controlD64] driver[drm] resume enter
> > > <3>[ 3153.976759] [drm:stop_ring] *ERROR* render ring :timed out trying
> > > to stop ring
> > > <3>[ 3154.980061] [drm:stop_ring] *ERROR* render ring :timed out trying
> > > to stop ring
> > > <3>[ 3154.980067] [drm:init_ring_common] *ERROR* failed to set render
> > > ring head to zero ctl 00000000 head 00000000 tail 00000000 start
> > > 00000000
> > > <3>[ 3155.983364] [drm:stop_ring] *ERROR* bsd ring :timed out trying to
> > > stop ring
> > > <3>[ 3156.986668] [drm:stop_ring] *ERROR* bsd ring :timed out trying to
> > > stop ring
> > > <3>[ 3156.986673] [drm:init_ring_common] *ERROR* failed to set bsd ring
> > > head to zero ctl 00000000 head 00000000 tail 00000000 start 00000000
> > > <3>[ 3157.989982] [drm:stop_ring] *ERROR* render ring :timed out trying
> > > to stop ring
> 
> 


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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