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 >