On Wed, Sep 25, 2024 at 02:28:42PM -0500, Bjorn Helgaas wrote:
> On Wed, Sep 25, 2024 at 05:45:21PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > On some older laptops i915 needs to leave the GPU in
> > D0 when hibernating the system, or else the BIOS
> > hangs somewhere. Currently that is achieved by calling
> > pci_save_state() ahead of time, which then skips the
> > whole pci_prepare_to_sleep() stuff.
>
> IIUC this refers to pci_pm_suspend_noirq(), which has this:
>
> if (!pci_dev->state_saved) {
> pci_save_state(pci_dev);
> if (!pci_dev->skip_bus_pm && pci_power_manageable(pci_dev))
> pci_prepare_to_sleep(pci_dev);
> }
>
> Would be good if the commit log included the name of the function
> where pci_prepare_to_sleep() is skipped.
>
> If there's a general requirement to leave all devices in D0 when
> hibernating, it would be nice to have have some documentation like an
> ACPI spec reference.
>
> Or if this is some i915-specific thing, maybe a pointer to history
> like a lore or bugzilla reference.
>
> > It feels to me that this approach could lead to unintended
> > side effects as it causes the pci code to deviate from the
> > standard path in various ways. In order to keep i915
> > behaviour more standard it seems preferrable to use
> > pci_dev->skip_bus_pm here. Duplicate the relevant logic
> > from pci_pm_suspend_noirq() in pci_pm_poweroff_noirq().
> >
> > It also looks like the current code is may put the parent
> > bridge into D3 despite leaving the device in D0. Though
> > perhaps the host bridge (which is where the integrated
> > GPU lives) always has subordinates, which would make
> > this a non-issue for i915. But maybe this could be a
> > problem for other devices. Utilizing skip_bus_pm will
> > make the behaviour of leaving the bridge in D0 a bit
> > more explicit if nothing else.
>
> s/is may/may/
>
> Rewrap to fill 75 columns. Could apply to all patches in the series.
>
> Will need an ack from Rafael, author of:
>
> d491f2b75237 ("PCI: PM: Avoid possible suspend-to-idle issue")
> 3e26c5feed2a ("PCI: PM: Skip devices in D0 for suspend-to-idle")
>
> which added .skip_bus_pm and its use in pci_pm_suspend_noirq().
Rafael, any thoughts on this stuff?
>
> IIUC this is a cleanup that doesn't fix any known problem? The
> overall diffstat doesn't make it look like a simplification, although
> it might certainly be cleaner somehow:
>
> > drivers/gpu/drm/i915/i915_driver.c | 121 +++++++++++++++++++----------
> > drivers/pci/pci-driver.c | 16 +++-
> > 2 files changed, 94 insertions(+), 43 deletions(-)
>
> > Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> > Cc: "Rafael J. Wysocki" <rafael@xxxxxxxxxx>
> > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > Cc: linux-pci@xxxxxxxxxxxxxxx
> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > drivers/pci/pci-driver.c | 16 +++++++++++++++-
> > 1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > index f412ef73a6e4..ef436895939c 100644
> > --- a/drivers/pci/pci-driver.c
> > +++ b/drivers/pci/pci-driver.c
> > @@ -1142,6 +1142,8 @@ static int pci_pm_poweroff(struct device *dev)
> > struct pci_dev *pci_dev = to_pci_dev(dev);
> > const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> >
> > + pci_dev->skip_bus_pm = false;
> > +
> > if (pci_has_legacy_pm_support(pci_dev))
> > return pci_legacy_suspend(dev, PMSG_HIBERNATE);
> >
> > @@ -1206,9 +1208,21 @@ static int pci_pm_poweroff_noirq(struct device *dev)
> > return error;
> > }
> >
> > - if (!pci_dev->state_saved && !pci_has_subordinate(pci_dev))
> > + if (!pci_dev->state_saved && !pci_dev->skip_bus_pm &&
> > + !pci_has_subordinate(pci_dev))
> > pci_prepare_to_sleep(pci_dev);
> >
> > + if (pci_dev->current_state == PCI_D0) {
> > + pci_dev->skip_bus_pm = true;
> > + /*
> > + * Per PCI PM r1.2, table 6-1, a bridge must be in D0 if any
> > + * downstream device is in D0, so avoid changing the power state
> > + * of the parent bridge by setting the skip_bus_pm flag for it.
> > + */
> > + if (pci_dev->bus->self)
> > + pci_dev->bus->self->skip_bus_pm = true;
> > + }
> > +
> > /*
> > * The reason for doing this here is the same as for the analogous code
> > * in pci_pm_suspend_noirq().
> > --
> > 2.44.2
> >
--
Ville Syrjälä
Intel
