Re: [PATCH v2 1/2] drm/xe: Inspect all forcewake bits for runtime idleness

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

On Thu, Sep 10, 2026 at 04:00:40PM -0700, Belgaumkar, Vinay wrote:
> 
> On 9/10/2026 3:17 PM, Rodrigo Vivi wrote:
> > On Thu, Sep 10, 2026 at 02:28:22PM -0700, Vinay Belgaumkar wrote:
> > > Inspect all GT force wake bits in the idle check for runtime suspend.
> > > This includes the ones not typically used by Xe KMD as well. Skip suspend
> > > when any of these bits are set, since it could mean someone is trying to
> > > keep the GT awake.
> > > 
> > > Also add a call to pm_runtime_autosuspend() inside the idle check. This
> > > ensures a PM subsystem retry at a later time.
> > > 
> > > v2: Only check the GT domain FW register. Other domains require a GT force
> > > wake applied before we can read their registers. Retry if idleness check
> > > fails (Sashiko)
> > > 
> > > Cc: Badal Nilawar <badal.nilawar@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Assisted-by: Claude:Claude-Sonnet-5
> > > Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
> > > ---
> > >   drivers/gpu/drm/xe/xe_force_wake.c | 25 +++++++++++++++++++++++++
> > >   drivers/gpu/drm/xe/xe_force_wake.h |  1 +
> > >   drivers/gpu/drm/xe/xe_pci.c        |  6 +++++-
> > >   drivers/gpu/drm/xe/xe_pm.c         | 19 +++++++++++++++++++
> > >   drivers/gpu/drm/xe/xe_pm.h         |  1 +
> > >   5 files changed, 51 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_force_wake.c b/drivers/gpu/drm/xe/xe_force_wake.c
> > > index 197e2197bd0a..0acb796c756c 100644
> > > --- a/drivers/gpu/drm/xe/xe_force_wake.c
> > > +++ b/drivers/gpu/drm/xe/xe_force_wake.c
> > > @@ -148,6 +148,31 @@ static int domain_sleep_wait(struct xe_gt *gt,
> > >   	return __domain_wait(gt, domain, false);
> > >   }
> > > +/**
> > > + * xe_force_wake_any_bit_set - Return true if any force wake bit is set
> > > + * @fw: struct xe_force_wake
> > > + * @id: Force wake domain id
> > > + *
> > > + * Check if any multithread force wake bit is set for a given domain ID.
> > > + *
> > > + * Return: true if any bit in the force wake reg is set
> > > + */
> > > +bool xe_force_wake_any_bit_set(struct xe_force_wake *fw, enum xe_force_wake_domain_id id)
> > > +{
> > > +	struct xe_gt *gt = fw->gt;
> > > +	struct xe_force_wake_domain *domain = &fw->domains[id];
> > > +	u32 val;
> > > +
> > > +	val = xe_mmio_read32(&gt->mmio, domain->reg_ctl);
> > > +	if (val & 0xFFFF) {
> > > +		xe_gt_dbg(gt, "FW bit set in domain: %d, reg: 0x%x, val: 0x%x",
> > > +			  domain->id, domain->reg_ctl.addr, val);
> > > +		return true;
> > > +	}
> > > +
> > > +	return false;
> > > +}
> > > +
> > >   /**
> > >    * xe_force_wake_get() : Increase the domain refcount
> > >    * @fw: struct xe_force_wake
> > > diff --git a/drivers/gpu/drm/xe/xe_force_wake.h b/drivers/gpu/drm/xe/xe_force_wake.h
> > > index e2721f205d6c..53941e5a0fbe 100644
> > > --- a/drivers/gpu/drm/xe/xe_force_wake.h
> > > +++ b/drivers/gpu/drm/xe/xe_force_wake.h
> > > @@ -18,6 +18,7 @@ void xe_force_wake_init_engines(struct xe_gt *gt,
> > >   unsigned int __must_check xe_force_wake_get(struct xe_force_wake *fw,
> > >   					    enum xe_force_wake_domains domains);
> > >   void xe_force_wake_put(struct xe_force_wake *fw, unsigned int fw_ref);
> > > +bool xe_force_wake_any_bit_set(struct xe_force_wake *fw, enum xe_force_wake_domain_id id);
> > >   const char *xe_force_wake_domain_to_str(enum xe_force_wake_domain_id id);
> > > diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> > > index ab4da1d9a9f1..a79d928ad75a 100644
> > > --- a/drivers/gpu/drm/xe/xe_pci.c
> > > +++ b/drivers/gpu/drm/xe/xe_pci.c
> > > @@ -1438,10 +1438,14 @@ static int xe_pci_runtime_idle(struct device *dev)
> > >   {
> > >   	struct pci_dev *pdev = to_pci_dev(dev);
> > >   	struct xe_device *xe = pdev_to_xe_device(pdev);
> > > +	int ret;
> > >   	xe_pm_d3cold_allowed_toggle(xe);
> > > -	return 0;
> > > +	ret = xe_pm_check_runtime_idle(xe);
> > > +	pm_runtime_autosuspend(dev);
> > do we really need this autosuspend here?
> > I believe this is what may have confused Sashiko.
> > And if needed it probably worth a separate patch with explanation.
> 
> It was Sashiko's suggestion :). If we fail runtime_idle check due to someone
> holding a forcewake, there is no mechanism to re-trigger the autosuspend
> when they release the forcewake. I believe it is only retriggered on a
> pm_ref put(). So, system will be stuck in D0 state even after the forcewake
> has been released (until some execution happens and a pm_ref count goes to
> zero again). We only need the autosuspend when the check fails, so should be
> part of the same patch? I added it to both paths since it is a noop in the
> case where we succeed in the idle check (as it returns 0 to the PM
> subsystem).

Well, Sashiko is right about something that I had never realized,
runtime_suspend failure does schedule itself, the idle indeed doesn't.
That's awkward in the rpm infra, but not a driver job to do this call.

Also, I don't believe it is forever anyway. In the next put/get pair
it will be rearmed.

So, we can either accept this delayed rearm, we can move this check
to inside runtime_suspend and avoid the _idle or we can try to work
with linux core kernel to ensure _idle is rearmed.

But this call itself needs to be removed from here.

Thanks,
Rodrigo.

> 
> Thanks,
> 
> Vinay.
> 
> > 
> > > +
> > > +	return ret;
> > >   }
> > >   static const struct dev_pm_ops xe_pm_ops = {
> > > diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> > > index f517bf453b54..e8d4cdbaef88 100644
> > > --- a/drivers/gpu/drm/xe/xe_pm.c
> > > +++ b/drivers/gpu/drm/xe/xe_pm.c
> > > @@ -17,6 +17,8 @@
> > >   #include "xe_bo.h"
> > >   #include "xe_bo_evict.h"
> > >   #include "xe_device.h"
> > > +#include "xe_force_wake.h"
> > > +#include "xe_force_wake_types.h"
> > >   #include "xe_ggtt.h"
> > >   #include "xe_gt.h"
> > >   #include "xe_gt_idle.h"
> > > @@ -1030,6 +1032,23 @@ void xe_pm_d3cold_allowed_toggle(struct xe_device *xe)
> > >   	mutex_unlock(&xe->d3cold.lock);
> > >   }
> > > +/**
> > > + * xe_pm_check_runtime_idle() - Perform checks before signaling runtime idle.
> > > + *
> > > + * Return: 0 on success, non-zero on finding device not idle
> > > + */
> > > +int xe_pm_check_runtime_idle(struct xe_device *xe)
> > > +{
> > > +	struct xe_gt *gt;
> > > +	u8 id;
> > > +
> > > +	for_each_gt(gt, xe, id)
> > > +		if (xe_force_wake_any_bit_set(gt_to_fw(gt), XE_FW_DOMAIN_ID_GT))
> > > +			return -EBUSY;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >   /**
> > >    * xe_pm_module_init() - Perform xe_pm specific module initialization.
> > >    *
> > > diff --git a/drivers/gpu/drm/xe/xe_pm.h b/drivers/gpu/drm/xe/xe_pm.h
> > > index 6d5ab09cb769..d1f6a694b7c4 100644
> > > --- a/drivers/gpu/drm/xe/xe_pm.h
> > > +++ b/drivers/gpu/drm/xe/xe_pm.h
> > > @@ -38,6 +38,7 @@ struct task_struct *xe_pm_read_callback_task(struct xe_device *xe);
> > >   int xe_pm_block_on_suspend(struct xe_device *xe);
> > >   void xe_pm_might_block_on_suspend(void);
> > >   int xe_pm_module_init(void);
> > > +int xe_pm_check_runtime_idle(struct xe_device *xe);
> > >   static inline void __xe_pm_runtime_noop(struct xe_device *xe) {}
> > > -- 
> > > 2.38.1
> > >