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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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
>