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

[Sagar Arun Kamble <sagar.a.kamble@intel.com>]

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?




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