Re: [PATCH] drm/xe/pm: Enable RPM via PCIe PM capability

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

On Wed, Jul 30, 2025 at 12:39:42PM +0000, Gupta, Anshuman wrote:
> 
> 
> > -----Original Message-----
> > From: K V P, Satyanarayana <satyanarayana.k.v.p@intel.com>
> > Sent: Wednesday, July 30, 2025 6:03 PM
> > To: intel-xe@lists.freedesktop.org
> > Cc: K V P, Satyanarayana <satyanarayana.k.v.p@intel.com>; Wajdeczko, Michal
> > <Michal.Wajdeczko@intel.com>; Brost, Matthew <matthew.brost@intel.com>;
> > Winiarski, Michal <michal.winiarski@intel.com>; Gupta, Anshuman
> > <anshuman.gupta@intel.com>
> > Subject: [PATCH] drm/xe/pm: Enable RPM via PCIe PM capability
> > 
> > Enable Runtime Power Management (RPM) for PCI Express devices by utilizing
> > their native Power Management (PM) capabilities.

Why?! The commit message needs to explain what it is doing, but it also
needs to tell why we are doing it.

> > 
> > Signed-off-by: Satyanarayana K V P <satyanarayana.k.v.p@intel.com>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Matthew Brost <matthew.brost@intel.com>
> > Cc: Michał Winiarski <michal.winiarski@intel.com>
> > Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_device_types.h | 2 ++
> >  drivers/gpu/drm/xe/xe_pm.c           | 9 +++++++++
> >  drivers/gpu/drm/xe/xe_pm.h           | 4 ++++
> >  3 files changed, 15 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_device_types.h
> > b/drivers/gpu/drm/xe/xe_device_types.h
> > index 38c8329b4d2c..3bbfc46044a0 100644
> > --- a/drivers/gpu/drm/xe/xe_device_types.h
> > +++ b/drivers/gpu/drm/xe/xe_device_types.h
> > @@ -285,6 +285,8 @@ struct xe_device {
> >  		 * pcode mailbox commands.
> >  		 */
> >  		u8 has_mbx_power_limits:1;
> > +		/** @info.has_pm_capability: Device has PCI pm capability */
> > +		u8 has_pm_capability:1;
> >  		/** @info.has_pxp: Device has PXP support */
> >  		u8 has_pxp:1;
> >  		/** @info.has_range_tlb_invalidation: Has range based TLB
> > invalidations */ diff --git a/drivers/gpu/drm/xe/xe_pm.c
> > b/drivers/gpu/drm/xe/xe_pm.c index 44aaf154ddf7..5e6311964685 100644
> > --- a/drivers/gpu/drm/xe/xe_pm.c
> > +++ b/drivers/gpu/drm/xe/xe_pm.c
> > @@ -244,6 +244,9 @@ static void xe_pm_runtime_init(struct xe_device *xe)  {
> >  	struct device *dev = xe->drm.dev;
> > 
> > +	if (!IS_RPM_SUPPORTED(xe))
> > +		return;
> > +
> >  	/*
> >  	 * Disable the system suspend direct complete optimization.
> >  	 * We need to ensure that the regular device suspend/resume
> > functions @@ -265,6 +268,7 @@ static void xe_pm_runtime_init(struct
> > xe_device *xe)
> > 
> >  int xe_pm_init_early(struct xe_device *xe)  {
> > +	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
> >  	int err;
> > 
> >  	INIT_LIST_HEAD(&xe->mem_access.vram_userfault.list);
> > @@ -278,6 +282,8 @@ int xe_pm_init_early(struct xe_device *xe)
> >  		return err;
> > 
> >  	xe->d3cold.capable = xe_pm_pci_d3cold_capable(xe);
> > +	xe->info.has_pm_capability = !!pdev->pm_cap;
> > +
> >  	return 0;
> >  }
> >  ALLOW_ERROR_INJECTION(xe_pm_init_early, ERRNO); /* See xe_pci_probe()
> > */ @@ -364,6 +370,9 @@ static void xe_pm_runtime_fini(struct xe_device *xe)
> > {
> >  	struct device *dev = xe->drm.dev;
> > 
> > +	if (!IS_RPM_SUPPORTED(xe))
> > +		return;
> > +
> >  	pm_runtime_get_sync(dev);
> >  	pm_runtime_forbid(dev);
> >  }
> > diff --git a/drivers/gpu/drm/xe/xe_pm.h b/drivers/gpu/drm/xe/xe_pm.h index
> > 59678b310e55..55b65728f522 100644
> > --- a/drivers/gpu/drm/xe/xe_pm.h
> > +++ b/drivers/gpu/drm/xe/xe_pm.h
> > @@ -9,6 +9,10 @@
> >  #include <linux/pm_runtime.h>
> > 
> >  #define DEFAULT_VRAM_THRESHOLD 300 /* in MB */
> > +#define IS_RPM_SUPPORTED(xe) ({ \
> > +		struct xe_device *___xe = (xe); \
> > +		___xe->info.has_pm_capability; \
> > +		})
> IMHO the correct method to let PCIe core get the permanent ref count if pdev->pm_cap is false.

that's correct, if we need to take pm_cap into account, getting the permanent
ref count would be the right way... but....

> @Vivi, Rodrigo what your thought, I think XeKMD should not take any decision based upon pdev->pm_cap.

right again... I'm seeing absolutely no driver using the pm_cap to take rpm
decisions, why would we deviate from that? Something is not right with the
whole goal here.

Thanks,
Rodrigo.

> Thanks,
> Anshuman 
> > 
> >  struct xe_device;
> > 
> > --
> > 2.43.0
>