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 <[email protected]> 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 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
