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().
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
>
