On Fri, Aug 08, 2014 at 02:34:37PM +0300, Imre Deak wrote: > 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@xxxxxxxxx wrote: > > > > > > > > From: Sagar Kamble <sagar.a.kamble@xxxxxxxxx> > > > > > > > > @@ -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@xxxxxxxxxxxxxxxx> > > 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. Blergh, I always forget that little piece of nonsense. Then we just need to call the relevant stuff directly, but not the individual platform functions but the generic interfaces. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
