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

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

On Fri, Sep 11, 2026 at 04:02:39PM -0700, Belgaumkar, Vinay wrote:
> 
> On 9/11/2026 2:58 PM, Rodrigo Vivi wrote:
> > 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.
> 
> Sure, we can only check inside _suspend call to simplify. In that case,
> there is no need to check for forcewake bits, right? Checking for C6 is
> sufficient since it cannot happen without the other.

ack

> 
> Thanks,
> 
> Vinay.
> 
> > 
> > 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
> > > > >