On Tue, Mar 11, 2025 at 09:56:24PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> I don't think there should be any need for us to call any of
> pci_enable_device(), pci_disable_device() or pci_set_master()
> from the suspend/resume paths. The config space save/restore should
> take care of all of this.
Cc: Karthik Poosa <karthik.poosa@xxxxxxxxx>
Karthik, I believe we should do the same on Xe and test this against that strange D3Cold bug on BMG
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_driver.c | 31 ------------------------------
> 1 file changed, 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index 503f1b6b694f..d3d1b2d082dd 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -1092,7 +1092,6 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
> {
> 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);
> struct intel_runtime_pm *rpm = &dev_priv->runtime_pm;
> struct intel_gt *gt;
> int ret, i;
> @@ -1113,21 +1112,10 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
> if (ret) {
> drm_err(&dev_priv->drm, "Suspend complete failed: %d\n", ret);
> intel_display_power_resume_early(display);
> -
> - goto fail;
> }
>
> enable_rpm_wakeref_asserts(rpm);
>
> - if (!dev_priv->uncore.user_forcewake_count)
> - intel_runtime_pm_driver_release(rpm);
> -
> - pci_disable_device(pdev);
> -
> - return 0;
> -
> -fail:
> - enable_rpm_wakeref_asserts(rpm);
> if (!dev_priv->uncore.user_forcewake_count)
> intel_runtime_pm_driver_release(rpm);
>
> @@ -1278,7 +1266,6 @@ static int i915_drm_resume_early(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);
> struct intel_gt *gt;
> int ret, i;
>
> @@ -1292,24 +1279,6 @@ static int i915_drm_resume_early(struct drm_device *dev)
> * similar so that power domains can be employed.
> */
>
> - /*
> - * Note that pci_enable_device() first enables any parent bridge
> - * device and only then sets the power state for this device. The
> - * bridge enabling is a nop though, since bridge devices are resumed
> - * first. The order of enabling power and enabling the device is
> - * imposed by the PCI core as described above, so here we preserve the
> - * same order for the freeze/thaw phases.
> - *
> - * TODO: eventually we should remove pci_disable_device() /
> - * pci_enable_enable_device() from suspend/resume. Due to how they
> - * depend on the device enable refcount we can't anyway depend on them
> - * disabling/enabling the device.
> - */
> - if (pci_enable_device(pdev))
> - return -EIO;
> -
> - pci_set_master(pdev);
We need more tests, but let's try it and clean things up.
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> -
> disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
>
> ret = vlv_resume_prepare(dev_priv, false);
> --
> 2.45.3
>
