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

[Raag Jadav <raag.jadav@intel.com>]

On Tue, Mar 11, 2025 at 09:56:20PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> drivers/pci does the pci_save_state()+pci_set_power_state() from
> the _noirq() pm hooks. Move our manual calls (needed for the
> hibernate vs. 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: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 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 be232caef9df..e06f2956382c 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -1045,7 +1045,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);
> @@ -1059,8 +1058,6 @@ static int i915_drm_suspend(struct drm_device *dev)
>  		intel_display_driver_disable_user_access(display);
>  	}
>  
> -	pci_save_state(pdev);
> -
>  	intel_display_driver_suspend(display);
>  
>  	intel_irq_suspend(dev_priv);
> @@ -1117,10 +1114,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_display_power_resume_early(display);
>  
> -		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
> @@ -1132,11 +1135,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() prevents drivers/pci from
> +	 * automagically putting the device into D3.
>  	 */
> +	pci_save_state(pdev);

I might be missing something, but looking at the comment it seems like
the problem is with certain gens and their BIOS? Not sure why we need
to do it for all gens.

Shouldn't we let PCI PM take care of it, atleast for devices that don't
have this problem?

>  	if (!(hibernation && GRAPHICS_VER(dev_priv) < 6))
>  		pci_set_power_state(pdev, PCI_D3hot);

Raag