Re: [PATCH] drm/xe: Restore pci state upon resume

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

On Fri, Sep 13, 2024 at 07:54:34PM +0300, Ville Syrjälä wrote:
> On Fri, Sep 13, 2024 at 11:43:52AM -0400, Rodrigo Vivi wrote:
> > On Fri, Sep 13, 2024 at 02:01:49PM +0300, Ville Syrjälä wrote:
> > > On Thu, Sep 12, 2024 at 03:05:30PM -0400, Rodrigo Vivi wrote:
> > > > The pci state was saved, but not restored. Restore
> > > > right after the power state transition request like
> > > > every other driver.
> > > > 
> > > > Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/xe/xe_pci.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> > > > index 5ba4ec229494..6d29ef4b396f 100644
> > > > --- a/drivers/gpu/drm/xe/xe_pci.c
> > > > +++ b/drivers/gpu/drm/xe/xe_pci.c
> > > > @@ -949,6 +949,8 @@ static int xe_pci_resume(struct device *dev)
> > > >  	if (err)
> > > >  		return err;
> > > >  
> > > > +	pci_restore_state(pdev);
> > > 
> > > Why is xe even doing this stuff by hand instead of letting
> > > the pci core handle it?
> > 
> > That's a fair question, given that there's not much documentation
> > around it.
> > 
> > Looking the pci code, it looks that the pci core is not calling itself
> > for the restoration of the config space anywhere and looking to
> > other drivers around it looks like a safe thing to do.
> > 
> > And the pci_restore_state is paired with the pci_save_state.
> > Both i915 and Xe are doing the pci_save_state and not restoring
> > it.
> 
> i915 needs it because (as a side effect) it prevents the pci
> code from automagically sticking the device into D3, which
> apparently breaks hibernation on some old crappy laptops.
> But xe shouldn't need that.

Hmm, doing some archaeology here, it looks like the
both pci_save and pci_restore were added together on
regular system suspend-resume by Jesse from the very
beginning:

ba8bbcf6ff46 ("i915: add suspend/resume support")

Then, later pci_restore was removed by Zhenyu on
b7e53aba2f0e ("drm/i915: remove restore in resume")
because it was hanging some platforms.

The only reference to d3 related issues that I could find
was this one:
https://lore.kernel.org/intel-gfx/1497281047-25204-5-git-send-email-animesh.manna@intel.com/

but that was trying to add the support to the the save/restore
in the runtime pm side and not here in the regular system suspend/resume.

Am I missing anything?

Empirically Anshuman showed us that PCI subsystem is indeed taking
care of the save/restore.

Ville, my question to you now is: can I go ahead and simply remove
the pci_save_state() call from i915? Or you still believe some
hibernation somewhere could be broken?

I believe we should either remove both save and restore for both
drivers or add both to both. But staying with a lingering stand
alone case is bad. Also it kinds of block some of the refactor
code that I'm going around intel_display suspend/resume when
trying to reconcile xe and i915 calls.

Thanks a lot for all the inputs,
Rodrigo.

> 
> > 
> > So, we either add the restore state to both, or we remove the
> > save state. Adding seems to be the most conservative approach,
> > hence this patch (i915 is queued in a branch with other display
> > refactor).
> > 
> > Rafael, any guidance here?
> > 
> > Cc: Rafael J. Wysocki <rafael@kernel.org>
> > 
> > > 
> > > > +
> > > >  	err = pci_enable_device(pdev);
> > > >  	if (err)
> > > >  		return err;
> > > > -- 
> > > > 2.46.0
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> 
> -- 
> Ville Syrjälä
> Intel