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

[Gupta, Anshuman]

> -----Original Message-----
> From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of Rodrigo
> Vivi
> Sent: Friday, September 13, 2024 9:14 PM
> To: Ville Syrjälä <ville.syrjala at linux.intel.com>; rafael at kernel.org
> Cc: intel-xe at lists.freedesktop.org
> Subject: Re: [PATCH] drm/xe: Restore pci state upon resume
> 
> 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 at 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.
I observed below pci dbg logs which indicates, save/restore done by pci core during suspend/resume.

[  414.365382] i915 0000:03:00.0: saving config space at offset 0x0 (reading 0x56938086)
[  414.373199] i915 0000:03:00.0: saving config space at offset 0x4 (reading 0x100007)
[  414.380852] i915 0000:03:00.0: saving config space at offset 0x8 (reading 0x3000004)
[  414.388587] i915 0000:03:00.0: saving config space at offset 0xc (reading 0x0)
[  414.395806] i915 0000:03:00.0: saving config space at offset 0x10 (reading 0x60000004)
[  414.403725] i915 0000:03:00.0: saving config space at offset 0x14 (reading 0x0)
[  414.411022] i915 0000:03:00.0: saving config space at offset 0x18 (reading 0x8000000c)
[  414.418928] i915 0000:03:00.0: saving config space at offset 0x1c (reading 0x40)
[  414.426319] i915 0000:03:00.0: saving config space at offset 0x20 (reading 0x0)
[  414.433628] i915 0000:03:00.0: saving config space at offset 0x24 (reading 0x0)
[  414.440926] i915 0000:03:00.0: saving config space at offset 0x28 (reading 0x0)
[  414.448235] i915 0000:03:00.0: saving config space at offset 0x2c (reading 0x49058086)
[  414.456147] i915 0000:03:00.0: saving config space at offset 0x30 (reading 0xffe00000)
[  414.464057] i915 0000:03:00.0: saving config space at offset 0x34 (reading 0x40)
[  414.471449] i915 0000:03:00.0: saving config space at offset 0x38 (reading 0x0)
[  414.478753] i915 0000:03:00.0: saving config space at offset 0x3c (reading 0xff)
--------------------------------------------------------------------------------------------------------------------------
[  444.303318] i915 0000:03:00.0: restoring config space at offset 0x3c (was 0xffffffff, writing 0xff)
[  444.312456] i915 0000:03:00.0: restoring config space at offset 0x38 (was 0xffffffff, writing 0x0)
[  444.321402] i915 0000:03:00.0: restoring config space at offset 0x34 (was 0xffffffff, writing 0x40)
[  444.330430] i915 0000:03:00.0: restoring config space at offset 0x30 (was 0xffffffff, writing 0xffe00000)
[  444.339978] i915 0000:03:00.0: restoring config space at offset 0x2c (was 0xffffffff, writing 0x49058086)
[  444.349525] i915 0000:03:00.0: restoring config space at offset 0x28 (was 0xffffffff, writing 0x0)
[  444.358469] i915 0000:03:00.0: restoring config space at offset 0x24 (was 0xffffffff, writing 0x0)
[  444.368417] i915 0000:03:00.0: restoring config space at offset 0x24 (was 0xffffffff, writing 0x0)
[  444.378371] i915 0000:03:00.0: restoring config space at offset 0x24 (was 0xffffffff, writing 0x0)
[  444.388318] i915 0000:03:00.0: restoring config space at offset 0x24 (was 0xffffffff, writing 0x0)
[  444.398266] i915 0000:03:00.0: restoring config space at offset 0x24 (was 0xffffffff, writing 0x0)
[  444.408207] i915 0000:03:00.0: restoring config space at offset 0x24 (was 0xffffffff, writing 0x0)
[  444.418192] i915 0000:03:00.0: restoring config space at offset 0x24 (was 0xffffffff, writing 0x0)
[  444.428138] i915 0000:03:00.0: restoring config space at offset 0x24 (was 0xffffffff, writing 0x0)
[  444.438085] i915 0000:03:00.0: restoring config space at offset 0x24 (was 0xffffffff, writing 0x0)
[  444.448026] i915 0000:03:00.0: restoring config space at offset 0x24 (was 0xffffffff, writing 0x0)
[  444.457970] i915 0000:03:00.0: restoring config space at offset 0x24 (was 0xffffffff, writing 0x0)
[  444.466912] i915 0000:03:00.0: restoring config space at offset 0x20 (was 0xffffffff, writing 0x0)
[  444.476858] i915 0000:03:00.0: restoring config space at offset 0x20 (was 0xffffffff, writing 0x0)
Thanks,
Anshuman
> 
> 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.
> 
> 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 at kernel.org>
> 
> >
> > > +
> > >  	err = pci_enable_device(pdev);
> > >  	if (err)
> > >  		return err;
> > > --
> > > 2.46.0
> >
> > --
> > Ville Syrjälä
> > Intel