Re: [PATCH 5/6] drm/i915/pm: Do pci_restore_state() in switcheroo resume hook

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

On Thu, Sep 26, 2024 at 06:36:13PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 26, 2024 at 10:48:41AM -0400, Rodrigo Vivi wrote:
> > On Wed, Sep 25, 2024 at 05:45:25PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Since this switcheroo stuff bypasses all the core pm we
> > > have to manually manage the pci state. To that end add the
> > > missing pci_restore_state() to the switcheroo resume hook.
> > > We already have the pci_save_state() counterpart on the
> > > suspend side.
> > > 
> > > I suppose this might not matter in practice as the
> > > integrated GPU probably won't lose any state in D3,
> > > and I presume there are no machines where this code
> > > would come into play with an Intel discrete GPU.
> > > 
> > > Arguably none of this code should exist in the driver
> > > in the first place, and instead the entire switcheroo
> > > mechanism should be rewritten and properly integrated into
> > > core pm code...
> > > 
> > > 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 | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> > > index fe7c34045794..c3e7225ea1ba 100644
> > > --- a/drivers/gpu/drm/i915/i915_driver.c
> > > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > > @@ -1311,6 +1311,8 @@ int i915_driver_resume_switcheroo(struct drm_i915_private *i915)
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	pci_restore_state(pdev);
> > 
> > then why not simply call that inside the resume, for a better alignment
> > with the save counterpart?
> 
> This is switcheroo resume. And the counterpart is in switcheroo suspend.
> 
> For the core pm hooks I'm getting rid of both save and restore.

With this I totally agree. I probably missed something when just
reading the patches... I had to apply them all to see the final version.

So,

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> > 
> > > +
> > >  	ret = i915_drm_resume_early(&i915->drm);
> > >  	if (ret)
> > >  		return ret;
> > > -- 
> > > 2.44.2
> > > 
> 
> -- 
> Ville Syrjälä
> Intel