Re: [PATCH] drm/xe/d3cold: Set power state to D3Cold during s2idle/s3

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

On Sat, Mar 29, 2025 at 07:20:37AM +0200, Raag Jadav wrote:
> On Fri, Mar 28, 2025 at 12:02:17PM -0400, Rodrigo Vivi wrote:
> > On Thu, Mar 27, 2025 at 01:14:09PM -0400, Rodrigo Vivi wrote:
> > > On Thu, Mar 27, 2025 at 10:02:29PM +0530, Nilawar, Badal wrote:
> > > > 
> > > > On 27-03-2025 21:49, Badal Nilawar wrote:
> > > > Hi Rodrigo,
> > > > 
> > > > > According to pci core guidelines, pci_save_config is recommended when the
> > > > > driver explicitly needs to set the pci power state. As of now xe kmd is
> > > > > only doing pci_save_config while entering to s2idle/s3 state, which makes
> > > > > pci core think that device driver has already applied required pci power
> > > > > state. This leads to GPU remain in D0 state. To fix the issue setting
> > > > > the pci power state to D3Cold.
> > > > > 
> > > > > Fixes:dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
> > > > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> > > > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > > > > ---
> > > > >   drivers/gpu/drm/xe/xe_pci.c | 1 +
> > > > >   1 file changed, 1 insertion(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> > > > > index 7046e7e9a6c7..3317d475be79 100644
> > > > > --- a/drivers/gpu/drm/xe/xe_pci.c
> > > > > +++ b/drivers/gpu/drm/xe/xe_pci.c
> > > > > @@ -932,6 +932,7 @@ static int xe_pci_suspend(struct device *dev)
> > > > >   	pci_save_state(pdev);
> > > > >   	pci_disable_device(pdev);
> > > > > +	pci_set_power_state(pdev, PCI_D3cold);
> > > > 
> > > > Another approach to avoid calling pci_save_state and pci_set_power_state,
> > > > allowing the PCI core to manage this.
> > > > Currently, the above change aligns with the Xe RPM suspend flow.
> > > 
> > > Either way is fine it seems. Or we don't save the state and let pci subsystem
> > > handle that for us or we save and set explicitly. So, let's move quickly
> > > with this option here that is already fixing our current issue.
> 
> On our way to become another i915 now, are we?

could you please expand on this?

A simple git grep on pci_set_power_state and on pci_save_state
return many more entries than i915.

what am I missing?

> 
> While this might be a good enough band-aid, we should probably explore
> how DPM_FLAG_SMART_* work and stop mixing/matching approaches to hide
> issues.

perhaps this indeed...
perhaps the ones doing power state themselves are the ones declaring 'smart'?

Well, on a dumb script here it looks we have over 110 drivers using pci_save_state
and only 6 of those using DPM_FLAG_SMART_*

So, I agree that it might be a good idea to explore things here and find the
optimal settings.

> 
> Raag