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: 4461 bytes --]

On Fri, 2014-08-08 at 14:29 +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)

Yes, this is correct. The DPM core takes a runtime PM reference before
calling the system suspend handler. So you have to call any needed parts
explicitly from the system suspend handler that is shared with the
runtime PM code. But the platform checks could be contained all within
the called common handler and you'd call this handler from the system
suspend handler unconditionally. I think this would be closer what
Daniel suggested.

--Imre

> <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
> 
> 
> thanks,
> Sagar
> 
> 


[-- 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