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

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

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.

> 
> 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