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

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

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.

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