Re: [PATCH v2 6/6] drm/i915/pm: Drop redundant pci stuff from suspend/resume paths

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Sep 12, 2025 at 09:44:15AM +0000, Hogander, Jouni wrote:
> On Tue, 2025-03-11 at 21:56 +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > 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.
> 
> I couldn't find out what save/restore you are referring here. At least
> driver/pci isn't doing these in it's suspend/resume paths. Can you
> please point me out?

The save/restore of config space. IIRC the io, mem, and
busmaster enable bits all live in the pci command register.

> 
> BR,
> 
> Jouni Högander
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  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);
> > -
> >  	disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
> >  
> >  	ret = vlv_resume_prepare(dev_priv, false);
> 

-- 
Ville Syrjälä
Intel