diff mbox series

[2/6] drm/i915/pm: Hoist pci_save_state()+pci_set_power_state() to the end of pm _late() hook

Message ID 20240925144526.2482-3-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/pm: Clean up the hibernate vs. PCI D3 quirk | expand

Commit Message

Ville Syrjälä Sept. 25, 2024, 2:45 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

driver/pci does the pci_save_state()+pci_set_power_state() from the
_noirq() pm hooks. Move our manual calls (needed for the hibernate+D3
workaround with buggy BIOSes) towards that same point. We currently
have no _noirq() hooks, so end of _late() hooks is the best we can
do right now.

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: linux-pci@vger.kernel.org
Cc: intel-gfx@lists.freedesktop.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_driver.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

Comments

Rodrigo Vivi Sept. 26, 2024, 2:43 p.m. UTC | #1 
On Wed, Sep 25, 2024 at 05:45:22PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> driver/pci does the pci_save_state()+pci_set_power_state() from the
> _noirq() pm hooks. Move our manual calls (needed for the hibernate+D3
> workaround with buggy BIOSes) towards that same point. We currently
> have no _noirq() hooks, so end of _late() hooks is the best we can
> do right now.
> 
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: linux-pci@vger.kernel.org
> Cc: intel-gfx@lists.freedesktop.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_driver.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index 6dc0104a3e36..9d557ff8adf5 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -1015,7 +1015,6 @@ static int i915_drm_suspend(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_display *display = &dev_priv->display;
> -	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
>  	pci_power_t opregion_target_state;
>  
>  	disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
> @@ -1029,8 +1028,6 @@ static int i915_drm_suspend(struct drm_device *dev)
>  		intel_display_driver_disable_user_access(dev_priv);
>  	}
>  
> -	pci_save_state(pdev);
> -
>  	intel_display_driver_suspend(dev_priv);
>  
>  	intel_dp_mst_suspend(dev_priv);
> @@ -1090,10 +1087,16 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
>  		drm_err(&dev_priv->drm, "Suspend complete failed: %d\n", ret);
>  		intel_power_domains_resume(dev_priv);
>  
> -		goto out;
> +		goto fail;
>  	}
>  
> +	enable_rpm_wakeref_asserts(rpm);
> +
> +	if (!dev_priv->uncore.user_forcewake_count)
> +		intel_runtime_pm_driver_release(rpm);
> +

why do we need this?
probably deserves a separate patch?

>  	pci_disable_device(pdev);
> +
>  	/*
>  	 * During hibernation on some platforms the BIOS may try to access
>  	 * the device even though it's already in D3 and hang the machine. So
> @@ -1105,11 +1108,17 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
>  	 * Lenovo Thinkpad X301, X61s, X60, T60, X41
>  	 * Fujitsu FSC S7110
>  	 * Acer Aspire 1830T
> +	 *
> +	 * pci_save_state() is needed to prevent driver/pci from
> +	 * automagically putting the device into D3.
>  	 */

I'm still not convinced that this would automagically prevent the D3,
specially in this part of the code.

I would prefer to simply remove this call, or keep it and move it
here to be consistent with other drivers, but also add the restore
portion of it for consistency and alignment...

> +	pci_save_state(pdev);
>  	if (!(hibernation && GRAPHICS_VER(dev_priv) < 6))
>  		pci_set_power_state(pdev, PCI_D3hot);
>  
> -out:
> +	return 0;
> +
> +fail:
>  	enable_rpm_wakeref_asserts(rpm);
>  	if (!dev_priv->uncore.user_forcewake_count)
>  		intel_runtime_pm_driver_release(rpm);
> -- 
> 2.44.2
>
Ville Syrjälä Sept. 26, 2024, 3:41 p.m. UTC | #2 
On Thu, Sep 26, 2024 at 10:43:21AM -0400, Rodrigo Vivi wrote:
> On Wed, Sep 25, 2024 at 05:45:22PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > driver/pci does the pci_save_state()+pci_set_power_state() from the
> > _noirq() pm hooks. Move our manual calls (needed for the hibernate+D3
> > workaround with buggy BIOSes) towards that same point. We currently
> > have no _noirq() hooks, so end of _late() hooks is the best we can
> > do right now.
> > 
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: linux-pci@vger.kernel.org
> > Cc: intel-gfx@lists.freedesktop.org
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_driver.c | 19 ++++++++++++++-----
> >  1 file changed, 14 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> > index 6dc0104a3e36..9d557ff8adf5 100644
> > --- a/drivers/gpu/drm/i915/i915_driver.c
> > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > @@ -1015,7 +1015,6 @@ static int i915_drm_suspend(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct intel_display *display = &dev_priv->display;
> > -	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
> >  	pci_power_t opregion_target_state;
> >  
> >  	disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
> > @@ -1029,8 +1028,6 @@ static int i915_drm_suspend(struct drm_device *dev)
> >  		intel_display_driver_disable_user_access(dev_priv);
> >  	}
> >  
> > -	pci_save_state(pdev);
> > -
> >  	intel_display_driver_suspend(dev_priv);
> >  
> >  	intel_dp_mst_suspend(dev_priv);
> > @@ -1090,10 +1087,16 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
> >  		drm_err(&dev_priv->drm, "Suspend complete failed: %d\n", ret);
> >  		intel_power_domains_resume(dev_priv);
> >  
> > -		goto out;
> > +		goto fail;
> >  	}
> >  
> > +	enable_rpm_wakeref_asserts(rpm);
> > +
> > +	if (!dev_priv->uncore.user_forcewake_count)
> > +		intel_runtime_pm_driver_release(rpm);
> > +
> 
> why do we need this?
> probably deserves a separate patch?

It was there already.

> 
> >  	pci_disable_device(pdev);
> > +
> >  	/*
> >  	 * During hibernation on some platforms the BIOS may try to access
> >  	 * the device even though it's already in D3 and hang the machine. So
> > @@ -1105,11 +1108,17 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
> >  	 * Lenovo Thinkpad X301, X61s, X60, T60, X41
> >  	 * Fujitsu FSC S7110
> >  	 * Acer Aspire 1830T
> > +	 *
> > +	 * pci_save_state() is needed to prevent driver/pci from
> > +	 * automagically putting the device into D3.
> >  	 */
> 
> I'm still not convinced that this would automagically prevent the D3,
> specially in this part of the code.

You need to read pci_pm_poweroff_noirq()

> 
> I would prefer to simply remove this call, or keep it and move it
> here to be consistent with other drivers, but also add the restore
> portion of it for consistency and alignment...
> 
> > +	pci_save_state(pdev);
> >  	if (!(hibernation && GRAPHICS_VER(dev_priv) < 6))
> >  		pci_set_power_state(pdev, PCI_D3hot);
> >  
> > -out:
> > +	return 0;
> > +
> > +fail:
> >  	enable_rpm_wakeref_asserts(rpm);
> >  	if (!dev_priv->uncore.user_forcewake_count)
> >  		intel_runtime_pm_driver_release(rpm);
> > -- 
> > 2.44.2
> >
Rodrigo Vivi Sept. 26, 2024, 4:40 p.m. UTC | #3 
On Thu, Sep 26, 2024 at 06:41:17PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 26, 2024 at 10:43:21AM -0400, Rodrigo Vivi wrote:
> > On Wed, Sep 25, 2024 at 05:45:22PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > driver/pci does the pci_save_state()+pci_set_power_state() from the
> > > _noirq() pm hooks. Move our manual calls (needed for the hibernate+D3
> > > workaround with buggy BIOSes) towards that same point. We currently
> > > have no _noirq() hooks, so end of _late() hooks is the best we can
> > > do right now.
> > > 
> > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: linux-pci@vger.kernel.org
> > > Cc: intel-gfx@lists.freedesktop.org
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_driver.c | 19 ++++++++++++++-----
> > >  1 file changed, 14 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> > > index 6dc0104a3e36..9d557ff8adf5 100644
> > > --- a/drivers/gpu/drm/i915/i915_driver.c
> > > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > > @@ -1015,7 +1015,6 @@ static int i915_drm_suspend(struct drm_device *dev)
> > >  {
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > >  	struct intel_display *display = &dev_priv->display;
> > > -	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
> > >  	pci_power_t opregion_target_state;
> > >  
> > >  	disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
> > > @@ -1029,8 +1028,6 @@ static int i915_drm_suspend(struct drm_device *dev)
> > >  		intel_display_driver_disable_user_access(dev_priv);
> > >  	}
> > >  
> > > -	pci_save_state(pdev);
> > > -
> > >  	intel_display_driver_suspend(dev_priv);
> > >  
> > >  	intel_dp_mst_suspend(dev_priv);
> > > @@ -1090,10 +1087,16 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
> > >  		drm_err(&dev_priv->drm, "Suspend complete failed: %d\n", ret);
> > >  		intel_power_domains_resume(dev_priv);
> > >  
> > > -		goto out;
> > > +		goto fail;
> > >  	}
> > >  
> > > +	enable_rpm_wakeref_asserts(rpm);
> > > +
> > > +	if (!dev_priv->uncore.user_forcewake_count)
> > > +		intel_runtime_pm_driver_release(rpm);
> > > +
> > 
> > why do we need this?
> > probably deserves a separate patch?
> 
> It was there already.

yes, but it was done in a later stage and now it is called earlier
in the regular path. I believe it deserves a 'why' that is not clear
in this commit message.

> 
> > 
> > >  	pci_disable_device(pdev);
> > > +
> > >  	/*
> > >  	 * During hibernation on some platforms the BIOS may try to access
> > >  	 * the device even though it's already in D3 and hang the machine. So
> > > @@ -1105,11 +1108,17 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
> > >  	 * Lenovo Thinkpad X301, X61s, X60, T60, X41
> > >  	 * Fujitsu FSC S7110
> > >  	 * Acer Aspire 1830T
> > > +	 *
> > > +	 * pci_save_state() is needed to prevent driver/pci from
> > > +	 * automagically putting the device into D3.
> > >  	 */
> > 
> > I'm still not convinced that this would automagically prevent the D3,
> > specially in this part of the code.
> 
> You need to read pci_pm_poweroff_noirq()
> 
> > 
> > I would prefer to simply remove this call, or keep it and move it
> > here to be consistent with other drivers, but also add the restore
> > portion of it for consistency and alignment...
> > 
> > > +	pci_save_state(pdev);
> > >  	if (!(hibernation && GRAPHICS_VER(dev_priv) < 6))
> > >  		pci_set_power_state(pdev, PCI_D3hot);
> > >  
> > > -out:
> > > +	return 0;
> > > +
> > > +fail:
> > >  	enable_rpm_wakeref_asserts(rpm);
> > >  	if (!dev_priv->uncore.user_forcewake_count)
> > >  		intel_runtime_pm_driver_release(rpm);
> > > -- 
> > > 2.44.2
> > > 
> 
> -- 
> Ville Syrjälä
> Intel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index 6dc0104a3e36..9d557ff8adf5 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -1015,7 +1015,6 @@  static int i915_drm_suspend(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_display *display = &dev_priv->display;
-	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
 	pci_power_t opregion_target_state;
 
 	disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
@@ -1029,8 +1028,6 @@  static int i915_drm_suspend(struct drm_device *dev)
 		intel_display_driver_disable_user_access(dev_priv);
 	}
 
-	pci_save_state(pdev);
-
 	intel_display_driver_suspend(dev_priv);
 
 	intel_dp_mst_suspend(dev_priv);
@@ -1090,10 +1087,16 @@  static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
 		drm_err(&dev_priv->drm, "Suspend complete failed: %d\n", ret);
 		intel_power_domains_resume(dev_priv);
 
-		goto out;
+		goto fail;
 	}
 
+	enable_rpm_wakeref_asserts(rpm);
+
+	if (!dev_priv->uncore.user_forcewake_count)
+		intel_runtime_pm_driver_release(rpm);
+
 	pci_disable_device(pdev);
+
 	/*
 	 * During hibernation on some platforms the BIOS may try to access
 	 * the device even though it's already in D3 and hang the machine. So
@@ -1105,11 +1108,17 @@  static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
 	 * Lenovo Thinkpad X301, X61s, X60, T60, X41
 	 * Fujitsu FSC S7110
 	 * Acer Aspire 1830T
+	 *
+	 * pci_save_state() is needed to prevent driver/pci from
+	 * automagically putting the device into D3.
 	 */
+	pci_save_state(pdev);
 	if (!(hibernation && GRAPHICS_VER(dev_priv) < 6))
 		pci_set_power_state(pdev, PCI_D3hot);
 
-out:
+	return 0;
+
+fail:
 	enable_rpm_wakeref_asserts(rpm);
 	if (!dev_priv->uncore.user_forcewake_count)
 		intel_runtime_pm_driver_release(rpm);