[Intel-gfx] Preserving DPMS state over suspend/resume
Daniel Vetter
daniel at ffwll.ch
Thu Nov 13 15:40:56 CET 2014
Hi all,
Why is this discussion happening in private? Please _always_ cc a
relevant mailing list to keep everyone else in the loop. Public
intel-gfx seems appropriate here, so adding that. Please don't drop
that from replies.
Thanks, Daniel
On Thu, Nov 13, 2014 at 11:15 AM, Imre Deak <imre.deak at intel.com> wrote:
> On Thu, 2014-11-13 at 09:20 +0200, Goel, Akash wrote:
>> Checked by registering the driver's prepare callback. Whenever the
>> driver's prepare callback is invoked, device is in the runtime resumed
>> state only (as there is a call to pm_runtime_resume in the parent
>> function)
>
> What kernel are you using? As I mentioned earlier in the upstream kernel
> there won't be a runtime resume call in the parent. If you don't have
> the relevant commit, then you have to rebase to a newer kernel version,
> or backport that change:
>
> 7cd0602d783 - "PCI / PM: Resume runtime-suspended devices later during
> system suspend"
>
>> These are the value of some of the fields inside the device's
>> power structure, when prepare callback is invoked.
>> [drm:i915_pm_prepare] *ERROR* name 0000:00:02.0, runtime_auto 1,
>> disable_depth 0, runtime_status 0, use_autosuspend 1, usage_count 2,
>> ignore_children 0
>>
>> Also as per the code, if we return a nonzero value from the prepare
>> callback (except -EAGAIN), the suspend operation itself would be aborted.
>
> No, returning >0 only results in skipping the later steps during
> suspend, with the system suspend operation succeeding. This is what we
> need and it's described in
>
> Documentation/power/runtime_pm.txt
>
> and
>
> Documentation/power/devices.txt
>
> describing the pm.prepare callback.
>
> --Imre
>
>> Can we put the device back into runtime suspended state (if display state
>> is DPMS OFF) & return -EAGAIN from the prepare callback function ?
>>
>> Best regards
>> Akash
>>
>> -----Original Message-----
>> From: Kamble, Sagar A
>> Sent: Wednesday, November 12, 2014 10:48 PM
>> To: Goel, Akash; Deak, Imre
>> Cc: Nikkanen, Kimmo; Daniel Vetter; Srinivas, Vidya
>> Subject: RE: Preserving DPMS state over suspend/resume
>>
>> Right. My mistake.
>>
>> Somehow our code is not matching Kernel doc specification about prepare then.
>> https://www.kernel.org/doc/Documentation/power/devices.txt
>>
>>
>> The prepare phase is meant to prevent races by preventing new devices
>> from being registered; the PM core would never know that all the
>> children of a device had been suspended if new children could be
>> registered at will. (By contrast, devices may be unregistered at any
>> time.) Unlike the other suspend-related phases, during the prepare
>> phase the device tree is traversed top-down.
>>
>> After the prepare callback method returns, no new children may be
>> registered below the device. The method may also prepare the device or
>> driver in some way for the upcoming system power transition, but it
>> should not put the device into a low-power state.
>>
>> For devices supporting runtime power management, the return value of the
>> prepare callback can be used to indicate to the PM core that it may
>> safely leave the device in runtime suspend (if runtime-suspended
>> already), provided that all of the device's descendants are also left in
>> runtime suspend. Namely, if the prepare callback returns a positive
>> number and that happens for all of the descendants of the device too,
>> and all of them (including the device itself) are runtime-suspended, the
>> PM core will skip the suspend, suspend_late and suspend_noirq suspend
>> phases as well as the resume_noirq, resume_early and resume phases of
>> the following system resume for all of these devices. In that case,
>> the complete callback will be called directly after the prepare callback
>> and is entirely responsible for bringing the device back to the
>> functional state as appropriate.
>>
>>
>> -----Original Message-----
>> From: Goel, Akash
>> Sent: Wednesday, November 12, 2014 10:41 PM
>> To: Deak, Imre; Kamble, Sagar A
>> Cc: Nikkanen, Kimmo; Daniel Vetter; Srinivas, Vidya
>> Subject: RE: Preserving DPMS state over suspend/resume
>>
>> Yes pm_runtime_get_noresume() would not result in a resume, just the usage count will be increased, which in a way will forbid the runtime suspend of the device. Same is used when queuing the delayed work item for RC6/Turbo enabling, as runtime suspend should be prevented, till RC6 is enabled.
>>
>> static inline void pm_runtime_get_noresume(struct device *dev) {
>> atomic_inc(&dev->power.usage_count);
>> }
>>
>> -----Original Message-----
>> From: Deak, Imre
>> Sent: Wednesday, November 12, 2014 10:37 PM
>> To: Kamble, Sagar A
>> Cc: Goel, Akash; Nikkanen, Kimmo; Daniel Vetter; Srinivas, Vidya
>> Subject: Re: Preserving DPMS state over suspend/resume
>>
>> pm_runtime_get_noresume() shouldn't resume the device if it's suspended.
>>
>> On Wed, 2014-11-12 at 19:04 +0200, Kamble, Sagar A wrote:
>> > I think runtime resume happens from
>> >
>> > pm_runtime_get_noresume called from device prepare
>> >
>> > And device gets runtime suspended again if prepare callback returns non-null value:
>> >
>> > if (error)
>> > pm_runtime_put(dev);
>> >
>> >
>> > However need to verify this.
>> >
>> > Thanks
>> > Sagar
>> >
>> >
>> > -----Original Message-----
>> > From: Deak, Imre
>> > Sent: Wednesday, November 12, 2014 8:51 PM
>> > To: Goel, Akash
>> > Cc: Kamble, Sagar A; Nikkanen, Kimmo; Daniel Vetter; Srinivas, Vidya
>> > Subject: Re: Preserving DPMS state over suspend/resume
>> >
>> > Ok, I wasn't aware of this. But I haven't seen any wakeups on my device and I guess the reason is the following upstream commit:
>> >
>> > 7cd0602d783 - "PCI / PM: Resume runtime-suspended devices later during system suspend"
>> >
>> > On Wed, 2014-11-12 at 17:11 +0200, Goel, Akash wrote:
>> > > Hi Imre,
>> > >
>> > > There is a call to 'pm_runtime_resume' from the pci_pm_prepare
>> > > callback function. Won't it runtime resume the GFX device, before
>> > > the prepare callback is executed. Can we trigger the runtime
>> > > suspend of the device from the Driver's prepare callback. We know
>> > > that user space & kernel threads have all been frozen already.
>> > >
>> > > static int pci_pm_prepare(struct device *dev) {
>> > > struct device_driver *drv = dev->driver;
>> > > int error = 0;
>> > > /*
>> > > * PCI devices suspended at run time need to be resumed at this
>> > > * point, because in general it is necessary to reconfigure them for
>> > > * system suspend. Namely, if the device is supposed to wake up the
>> > > * system from the sleep state, we may need to reconfigure it for this
>> > > * purpose. In turn, if the device is not supposed to wake up the
>> > > * system from the sleep state, we'll have to prevent it from signaling
>> > > * wake-up.
>> > > */
>> > > pm_runtime_resume(dev);
>> > >
>> > > if (drv && drv->pm && drv->pm->prepare)
>> > > error = drv->pm->prepare(dev);
>> > > return error;
>> > > }
>> > >
>> > > Best regards
>> > > Akash
>> > > -----Original Message-----
>> > > From: Deak, Imre
>> > > Sent: Wednesday, November 12, 2014 7:18 PM
>> > > To: Kamble, Sagar A
>> > > Cc: Goel, Akash; Nikkanen, Kimmo; Daniel Vetter; Srinivas, Vidya
>> > > Subject: Re: Preserving DPMS state over suspend/resume
>> > >
>> > > I'm not sure yet. In theory the device should be able to resume from the runtime suspended state w/o any special actions needed in .complete().
>> > > But this will need some more testing on different platforms. At least I managed to runtime resume on BYT after S3 w/o problems. But I guess if we do need any extra steps resuming from S3,S0ix those steps should be part of the runtime resume handler, rather than the .complete handler.
>> > >
>> > > On Wed, 2014-11-12 at 15:36 +0200, Kamble, Sagar A wrote:
>> > > > Agree. This will help.
>> > > >
>> > > > Do we need to add support for complete callback in resume path?
>> > > >
>> > > > + .complete = intel_runtime_resume,
>> > > >
>> > > >
>> > > > Thanks
>> > > > Sagar
>> > > >
>> > > > -----Original Message-----
>> > > > From: Goel, Akash
>> > > > Sent: Wednesday, November 12, 2014 6:19 PM
>> > > > To: Deak, Imre
>> > > > Cc: Nikkanen, Kimmo; Daniel Vetter; Kamble, Sagar A; Srinivas,
>> > > > Vidya
>> > > > Subject: RE: Preserving DPMS state over suspend/resume
>> > > >
>> > > > Hi Imre,
>> > > >
>> > > > Thanks so much, this looks a very attractive solution :).
>> > > >
>> > > > So if we can ensure that post DPMS Off, GFX device gets runtime suspended before its system suspend phase is initiated, then this use case would get handled conveniently. As you said having a lower value of auto-suspend delay would ensure that this happens on a regular basis.
>> > > >
>> > > > Best regards
>> > > > Akash
>> > > >
>> > > > -----Original Message-----
>> > > > From: Deak, Imre
>> > > > Sent: Wednesday, November 12, 2014 5:42 PM
>> > > > To: Goel, Akash
>> > > > Cc: Nikkanen, Kimmo; Daniel Vetter; Kamble, Sagar A; Srinivas,
>> > > > Vidya
>> > > > Subject: Re: Preserving DPMS state over suspend/resume
>> > > >
>> > > > On Wed, 2014-11-12 at 05:10 +0200, Goel, Akash wrote:
>> > > > > Hi Imre,
>> > > > >
>> > > > > We are presuming that on intermediate wakeups, if Display isn't
>> > > > > being turned on, then there may not be any GT side activity also
>> > > > > (initiated by User space). So thought we can altogether skip the
>> > > > > resume of the GFX device when Display is off.
>> > > >
>> > > > In general this isn't guaranteed anyhow and the kernel can't trust user space to follow this rule. Also for GPGPU it's a valid use case to have the display off, but the GT side active.
>> > > >
>> > > > > We have tried these changes.
>> > > > > 1. Deferring the un-gating of Display from early resume to resume.
>> > > > > 2. Skipping the modeset call from the resume sequence, based on DPMS
>> > > > > state. This avoids the issue of mismatch between SW state & HW state.
>> > > > > 3. Putting the Display back into D3 at the end of resume.
>> > > > > 4. Enabling RC6 immediately in resume sequence.
>> > > > >
>> > > > > But still we haven't been able to achieve the desired power savings :(.
>> > > > > Only if we completely bypass suspend/resume of GFX device on
>> > > > > intermediate wakeups, we get the required savings.
>> > > >
>> > > > Ok, interesting. This is a separate issue from not restoring the DPMS state then, but it should be fixed nevertheless. Do you also enable runtime PM for i915? If so, there is actually an existing mechanism to avoid system s/r if the device is already runtime suspended, we should take this into use. This would be actually useful for making system and runtime s/r more unified eventually, by forcing the device to idle additionally in i915_pm_prepare(). Could you give a try to the patch below? You'd also have to reduce the runtime suspend timeout of course to get good results:
>> > > >
>> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
>> > > > b/drivers/gpu/drm/i915/i915_drv.c index 2404b2b..d8a6951 100644
>> > > > --- a/drivers/gpu/drm/i915/i915_drv.c
>> > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
>> > > > @@ -926,6 +926,15 @@ i915_pci_remove(struct pci_dev *pdev)
>> > > > drm_put_dev(dev);
>> > > > }
>> > > >
>> > > > +static int i915_pm_prepare(struct device *dev) {
>> > > > + struct pci_dev *pdev = to_pci_dev(dev);
>> > > > + struct drm_device *drm_dev = pci_get_drvdata(pdev);
>> > > > + struct drm_i915_private *dev_priv = drm_dev->dev_private;
>> > > > +
>> > > > + return dev_priv->pm.suspended ? 1 : 0; }
>> > > > +
>> > > > static int i915_pm_suspend(struct device *dev) {
>> > > > struct pci_dev *pdev = to_pci_dev(dev); @@ -1499,6 +1508,8 @@
>> > > > static int intel_suspend_complete(struct drm_i915_private
>> > > > *dev_priv) }
>> > > >
>> > > > static const struct dev_pm_ops i915_pm_ops = {
>> > > > + .prepare = i915_pm_prepare,
>> > > > +
>> > > > /*
>> > > > * S0ix (via system suspend) and S3 event handlers [PMSG_SUSPEND,
>> > > > * PMSG_RESUME]
>> > > >
>> > > >
>> > > >
>> > > > >
>> > > > > Best regards
>> > > > > Akash
>> > > > >
>> > > > > -----Original Message-----
>> > > > > From: Deak, Imre
>> > > > > Sent: Wednesday, November 12, 2014 1:32 AM
>> > > > > To: Goel, Akash
>> > > > > Cc: Nikkanen, Kimmo; Daniel Vetter; Kamble, Sagar A; Srinivas,
>> > > > > Vidya
>> > > > > Subject: Re: Preserving DPMS state over suspend/resume
>> > > > >
>> > > > > On Tue, 2014-11-11 at 17:03 +0200, Goel, Akash wrote:
>> > > > > > Hi Imre,
>> > > > > >
>> > > > > > Please could you review the attached patch file, which we have
>> > > > > > prepared as an interim solution to address the concerned
>> > > > > > problem of repeated suspend/resume of GFX device on intermediate wakeups.
>> > > > >
>> > > > > We can't skip resuming the whole i915 device based only on the
>> > > > > display state, you would have to account for the GT side too. So
>> > > > > for example you'd have to check in each IOCTL/debugfs etc. entry
>> > > > > point which can access the HW and make sure the driver is not system suspended.
>> > > > > Another issue with this approach is that in case you have two
>> > > > > display outputs, one in DPMS-ON the other in DPMS-OFF state
>> > > > > before suspend, you would still resume and turn on the second output.
>> > > > >
>> > > > > The underlying issue is that if there is non-NULL FB modeset for
>> > > > > a pipe (the pipe's SW state is enabled) the DPMS-OFF state won't
>> > > > > be preserved, but will be forced to DPMS-ON during resume. So
>> > > > > imo this needs to be fixed. intel_modeset_affected_pipes()
>> > > > > calculates the masks for pipes to be enabled/disabled, so this
>> > > > > function would need to take into account each connector's DPMS
>> > > > > state too in addition to the pipe's enabled state (intel_crtc->new_enabled).
>> > > > > Now
>> > > > > connector->dpms will be updated by
>> > > > > intel_modeset_readout_hw_state() to reflect the current DPMS
>> > > > > state, which may be altered by BIOS across a suspend/resume, so
>> > > > > I'd also add a
>> > > > > connector->new_dpms field. This would be set to the desired DPMS
>> > > > > connector->state
>> > > > > for each modeset: during a normal user modeset DPMS-ON for each
>> > > > > enabled connector, during resume the DPMS state that was set for
>> > > > > the connector before resume.
>> > > > >
>> > > > > --Imre
>> > > > >
>> > > > > >
>> > > > > > Best regards
>> > > > > > Akash
>> > > > > >
>> > > > > > -----Original Message-----
>> > > > > > From: Deak, Imre
>> > > > > > Sent: Tuesday, November 11, 2014 4:34 PM
>> > > > > > To: Nikkanen, Kimmo
>> > > > > > Cc: Goel, Akash; Daniel Vetter
>> > > > > > Subject: Preserving DPMS state over suspend/resume
>> > > > > >
>> > > > > > Hi,
>> > > > > >
>> > > > > > Akash contacted me a week ago about an issue they saw where
>> > > > > > the display comes back on after a suspend/resume cycle
>> > > > > > although it was turned off before explicitly by user space. We
>> > > > > > tracked down this to the following
>> > > > > > sequence:
>> > > > > >
>> > > > > > dpms-off -> system suspend -> system-resume
>> > > > > >
>> > > > > > After this the expectation is that the display stays in dpms-off state, the usecase being that system-resume happens due to a reason unrelated to the display. An example is waking for network packet handling from s0ix state, where the display should really stay off.
>> > > > > >
>> > > > > > The root cause for this is that the driver doesn't preserve the user DPMS state across s/r, in effect resetting it to DPMS-ON during resume.
>> > > > > > This could be a valid thing to do in certain cases/platforms, for example where we can't determine the wake reason. For a power button press for example it's the correct thing to do to turn on the display (again, given that we wouldn't be able to distinguish this event from other wake events). But on some platforms the wake reason is clear, so there the driver should leave the display off and leave it up to user space to act on the wake event. So perhaps we would need some driver capability, or other knob to set this behavior.
>> > > > > >
>> > > > > > My question is how to go about solving this issue. I promised to Akash to look into this, but I don't think I have time for it (sorry Akash, for the 1 week wasted). I also saw Daniel's bigger plans for restructuring the DPMS code, so maybe that would solve this issue.
>> > > > > >
>> > > > > > --Imre
>> > > > > >
>> > > > >
>> > > > >
>> > > >
>> > > >
>> > >
>> > >
>> >
>> >
>>
>>
>
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
More information about the Intel-gfx
mailing list