Re: [PATCH 1/3] drm/xe/forcewake: add delayed release with hysteresis

["Bai, Zongyao" <zongyao.bai@intel.com>]

Thank you Vivi for the reviewing!

On 11/9/2026 12:55 PM, Rodrigo Vivi wrote:
> On Sat, Oct 17, 2026 at 03:18:54AM +0000, Zongyao Bai wrote:
>> Add xe_force_wake_put_delay(), which keeps a domain awake for a short hold
>> time after its last reference is dropped instead of putting it to sleep
>> immediately. A xe_force_wake_get() within that time reuses the awake domain
>> without a wake request and ACK wait, which makes repeated short accesses
>> cheaper.
>>
>> The domain is put to sleep from a delayed work, in process context, so
>> the sleep ACK can be waited for in the same way as a regular put. The work
>> sleeps only the domains whose hold time expired and which still have no
>> reference, and requeues itself for the rest. The hold time is a couple of
>> jiffies.
>>
>> Signed-off-by: Zongyao Bai <zongyao.bai@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_force_wake.c       | 184 ++++++++++++++++++++---
>>   drivers/gpu/drm/xe/xe_force_wake.h       |  21 ++-
>>   drivers/gpu/drm/xe/xe_force_wake_types.h |  14 ++
>>   drivers/gpu/drm/xe/xe_gt.c               |   5 +-
>>   4 files changed, 197 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_force_wake.c b/drivers/gpu/drm/xe/xe_force_wake.c
>> index 197e2197bd0a..71517bc4ffe6 100644
>> --- a/drivers/gpu/drm/xe/xe_force_wake.c
>> +++ b/drivers/gpu/drm/xe/xe_force_wake.c
>> @@ -6,16 +6,22 @@
>>   #include "xe_force_wake.h"
>>   
>>   #include <drm/drm_util.h>
>> +#include <linux/device.h>
>> +#include <linux/workqueue.h>
>>   
>>   #include "regs/xe_gt_regs.h"
>>   #include "regs/xe_reg_defs.h"
>>   #include "xe_gt.h"
>>   #include "xe_gt_printk.h"
>>   #include "xe_mmio.h"
>> +#include "xe_pm.h"
>>   #include "xe_sriov.h"
>>   
>>   #define XE_FORCE_WAKE_ACK_TIMEOUT_MS	50
>>   
>> +/* Minimum hold of a delayed release, in jiffies. */
>> +#define XE_FORCE_WAKE_HOLD_JIFFIES	2
> please define in msec and use msec_to_jiffies()

Got it.
will change to: XE_FORCE_WAKE_HOLD_MS

>
>> +
>>   static const char *str_wake_sleep(bool wake)
>>   {
>>   	return wake ? "wake" : "sleep";
>> @@ -42,12 +48,22 @@ static void init_domain(struct xe_force_wake *fw,
>>   	mark_domain_initialized(fw, id);
>>   }
>>   
>> -void xe_force_wake_init_gt(struct xe_gt *gt, struct xe_force_wake *fw)
>> +static void xe_force_wake_disable_work(void *arg)
>> +{
>> +	struct xe_force_wake *fw = arg;
>> +
>> +	disable_delayed_work_sync(&fw->delayed_release_work);
>> +}
>> +
>> +static void xe_force_wake_delayed_release_work(struct work_struct *work);
>> +
>> +int xe_force_wake_init_gt(struct xe_gt *gt, struct xe_force_wake *fw)
>>   {
>>   	struct xe_device *xe = gt_to_xe(gt);
>>   
>>   	fw->gt = gt;
>>   	spin_lock_init(&fw->lock);
>> +	INIT_DELAYED_WORK(&fw->delayed_release_work, xe_force_wake_delayed_release_work);
>>   
>>   	if (xe->info.graphics_verx100 >= 1270) {
>>   		init_domain(fw, XE_FW_DOMAIN_ID_GT,
>> @@ -58,6 +74,8 @@ void xe_force_wake_init_gt(struct xe_gt *gt, struct xe_force_wake *fw)
>>   			    FORCEWAKE_GT,
>>   			    FORCEWAKE_ACK_GT);
>>   	}
>> +
>> +	return devm_add_action_or_reset(xe->drm.dev, xe_force_wake_disable_work, fw);
>>   }
>>   
>>   void xe_force_wake_init_engines(struct xe_gt *gt, struct xe_force_wake *fw)
>> @@ -187,8 +205,13 @@ unsigned int __must_check xe_force_wake_get(struct xe_force_wake *fw,
>>   	spin_lock_irqsave(&fw->lock, flags);
>>   	for_each_fw_domain_masked(domain, ref_rqst, fw, tmp) {
>>   		if (!domain->ref++) {
>> -			awake_rqst |= BIT(domain->id);
>> -			domain_wake(gt, domain);
>> +			if (fw->delayed_domains & BIT(domain->id)) {
>> +				/* Still awake from a delayed release: reuse it. */
>> +				fw->delayed_domains &= ~BIT(domain->id);
>> +			} else {
>> +				awake_rqst |= BIT(domain->id);
>> +				domain_wake(gt, domain);
>> +			}
>>   		}
>>   		ref_incr |= BIT(domain->id);
>>   	}
>> @@ -212,23 +235,46 @@ unsigned int __must_check xe_force_wake_get(struct xe_force_wake *fw,
>>   	return ref_incr;
>>   }
>>   
>> -/**
>> - * xe_force_wake_put - Decrement the refcount and put domain to sleep if refcount becomes 0
>> - * @fw: Pointer to the force wake structure
>> - * @fw_ref: return of xe_force_wake_get()
>> - *
>> - * This function reduces the reference counts for domains in fw_ref. If
>> - * refcount for any of the specified domain reaches 0, it puts the domain to sleep
>> - * and waits for acknowledgment for domain to sleep within 50 milisec timeout.
>> - * Warns in case of timeout of ack from domain.
>> +/*
>> + * Put the @mask domains to sleep and wait for the acknowledgment. Must be
>> + * called with fw->lock held. Returns the mask of domains that failed to
>> + * acknowledge.
>>    */
>> -void xe_force_wake_put(struct xe_force_wake *fw, unsigned int fw_ref)
>> +static unsigned int xe_force_wake_sleep_locked(struct xe_force_wake *fw,
>> +					       unsigned int mask)
>>   {
>>   	struct xe_gt *gt = fw->gt;
>>   	struct xe_force_wake_domain *domain;
>> -	unsigned int tmp, sleep = 0;
>> +	unsigned int tmp, ack_fail = 0;
>> +
>> +	lockdep_assert_held(&fw->lock);
>> +
>> +	for_each_fw_domain_masked(domain, mask, fw, tmp)
>> +		domain_sleep(gt, domain);
>> +
>> +	for_each_fw_domain_masked(domain, mask, fw, tmp) {
>> +		if (domain_sleep_wait(gt, domain) == 0)
>> +			fw->awake_domains &= ~BIT(domain->id);
>> +		else
>> +			ack_fail |= BIT(domain->id);
>> +	}
>> +
>> +	return ack_fail;
>> +}
>> +
>> +static void xe_force_wake_warn_sleep_fail(struct xe_gt *gt, unsigned int ack_fail)
>> +{
>> +	xe_gt_WARN(gt, ack_fail, "Forcewake domain%s %#x failed to acknowledge sleep request\n",
>> +		   str_plural(hweight_long(ack_fail)), ack_fail);
>> +}
>> +
>> +static void __xe_force_wake_put(struct xe_force_wake *fw, unsigned int fw_ref,
>> +				bool delayed)
> perhaps instead of bool we pass the msec of delay instead?

Get it.  Will change to:

  __xe_force_wake_put(fw, ref, unsigned int hold_ms)
hold_ms = 0 /* sleep immediately */
hold_ms = XE_FORCE_WAKE_HOLD_MS   /* delayed release */

>
>> +{
>> +	struct xe_gt *gt = fw->gt;
>> +	struct xe_force_wake_domain *domain;
>> +	unsigned int tmp, sleep = 0, ack_fail;
>>   	unsigned long flags;
>> -	int ack_fail = 0;
>>   
>>   	/*
>>   	 * Avoid unnecessary lock and unlock when the function is called
>> @@ -244,21 +290,111 @@ void xe_force_wake_put(struct xe_force_wake *fw, unsigned int fw_ref)
>>   	for_each_fw_domain_masked(domain, fw_ref, fw, tmp) {
>>   		xe_gt_assert(gt, domain->ref);
>>   
>> -		if (!--domain->ref) {
>> +		if (--domain->ref)
>> +			continue;
>> +
>> +		if (delayed) {
>> +			domain->hold_until = jiffies + XE_FORCE_WAKE_HOLD_JIFFIES;
>> +			fw->delayed_domains |= BIT(domain->id);
>> +			/*
>> +			 * queue_delayed_work() returns false both when the work
>> +			 * is already pending and when flush/stop has disabled
>> +			 * it. A pending worker releases the domain later, a
>> +			 * disabled one never will, so release it right away.
>> +			 *
>> +			 * If the worker starts and clears PENDING between the
>> +			 * two checks, it is misread as disabled and released
>> +			 * early. That is safe: the delayed bit is cleared
>> +			 * under fw->lock, so the worker does not see the
>> +			 * domain and does not sleep it again. Only this one
>> +			 * hold is lost.
>> +			 */
>> +			if (!queue_delayed_work(system_percpu_wq, &fw->delayed_release_work,
>> +						XE_FORCE_WAKE_HOLD_JIFFIES) &&
>> +			    !delayed_work_pending(&fw->delayed_release_work)) {
> This is confusing... perhaps a fw->delayed_enabled bool that is set and cleared
> under fw->lock by stop, is cleaner removing race and comment above...

Yeah, it's not worth to save a parameter here.

Got it, will change to:

+        if (hold_ms && fw->delayed_enabled) {
+            unsigned long hold = msecs_to_jiffies(hold_ms);
+
+            domain->hold_until = jiffies + hold;
              fw->delayed_domains |= BIT(domain->id);
+            queue_delayed_work(system_percpu_wq, 
&fw->delayed_release_work, hold);

And init it as true, and
in  xe_force_wake_stop()  ->  fw->delayed_enabled = false;
in xe_force_wake_disable_work() ->  fw->delayed_enabled = false;

>
>> +				fw->delayed_domains &= ~BIT(domain->id);
>> +				sleep |= BIT(domain->id);
>> +			}
>> +		} else {
>>   			sleep |= BIT(domain->id);
>> -			domain_sleep(gt, domain);
>>   		}
>>   	}
>> -	for_each_fw_domain_masked(domain, sleep, fw, tmp) {
>> -		if (domain_sleep_wait(gt, domain) == 0)
>> -			fw->awake_domains &= ~BIT(domain->id);
>> -		else
>> -			ack_fail |= BIT(domain->id);
>> +	ack_fail = xe_force_wake_sleep_locked(fw, sleep);
>> +	spin_unlock_irqrestore(&fw->lock, flags);
>> +
>> +	xe_force_wake_warn_sleep_fail(gt, ack_fail);
>> +}
>> +
>> +/**
>> + * xe_force_wake_put - Decrement the refcount and put domain to sleep if refcount becomes 0
>> + * @fw: Pointer to the force wake structure
>> + * @fw_ref: return of xe_force_wake_get()
>> + *
>> + * This function reduces the reference counts for domains in fw_ref. If
>> + * refcount for any of the specified domain reaches 0, it puts the domain to sleep
>> + * and waits for acknowledgment for domain to sleep within 50 milisec timeout.
>> + * Warns in case of timeout of ack from domain.
>> + */
>> +void xe_force_wake_put(struct xe_force_wake *fw, unsigned int fw_ref)
>> +{
>> +	__xe_force_wake_put(fw, fw_ref, false);
>> +}
>> +
>> +/**
>> + * xe_force_wake_put_delay - Decrement the refcount, delaying the sleep request
>> + * @fw: Pointer to the force wake structure
>> + * @fw_ref: return of xe_force_wake_get()
>> + *
>> + * Like xe_force_wake_put(), but a domain whose refcount reaches 0 is kept
>> + * awake for a short hold time instead of being put to sleep right away. An
>> + * xe_force_wake_get() during that time reuses the awake domain without a
>> + * wake request, which makes repeated short accesses cheaper. The domain is
>> + * put to sleep by a worker once the hold time expires without a new user.
>> + *
>> + * Meant for opt-in hot paths that are expected to be repeated shortly after.
>> + */
>> +void xe_force_wake_put_delay(struct xe_force_wake *fw, unsigned int fw_ref)
>> +{
>> +	__xe_force_wake_put(fw, fw_ref, true);
>> +}
>> +
>> +/*
>> + * Put the delayed domains whose hold time expired to sleep, and requeue for
>> + * the remaining ones.
>> + */
>> +static void xe_force_wake_delayed_release_work(struct work_struct *work)
>> +{
>> +	struct xe_force_wake *fw = container_of(to_delayed_work(work),
>> +						struct xe_force_wake, delayed_release_work);
>> +	struct xe_gt *gt = fw->gt;
>> +	struct xe_force_wake_domain *domain;
>> +	unsigned int tmp, expired = 0, ack_fail;
>> +	unsigned long flags, next = 0;
>> +	bool remaining = false;
>> +
>> +	xe_gt_assert(gt, !xe_pm_runtime_suspended(gt_to_xe(gt)));
> what about a get_if_active variant or something like that instead this
> fragile assert?

Agree! It's much better, will change to:

+   if (!xe_pm_runtime_get_if_active(xe))
+       return;

>
>> +
>> +	spin_lock_irqsave(&fw->lock, flags);
>> +	for_each_fw_domain_masked(domain, fw->delayed_domains, fw, tmp) {
>> +		if (time_before(jiffies, domain->hold_until)) {
>> +			if (!remaining || time_before(domain->hold_until, next))
>> +				next = domain->hold_until;
>> +			remaining = true;
>> +			continue;
>> +		}
>> +
>> +		expired |= BIT(domain->id);
>> +		fw->delayed_domains &= ~BIT(domain->id);
>>   	}
>> +
>> +	ack_fail = xe_force_wake_sleep_locked(fw, expired);
>> +
>> +	if (remaining)
>> +		queue_delayed_work(system_percpu_wq, &fw->delayed_release_work,
>> +				   next - jiffies);
>>   	spin_unlock_irqrestore(&fw->lock, flags);
>>   
>> -	xe_gt_WARN(gt, ack_fail, "Forcewake domain%s %#x failed to acknowledge sleep request\n",
>> -		   str_plural(hweight_long(ack_fail)), ack_fail);
>> +	xe_force_wake_warn_sleep_fail(gt, ack_fail);
>>   }
>>   
>>   const char *xe_force_wake_domain_to_str(enum xe_force_wake_domain_id id)
>> diff --git a/drivers/gpu/drm/xe/xe_force_wake.h b/drivers/gpu/drm/xe/xe_force_wake.h
>> index e2721f205d6c..35a2252c762a 100644
>> --- a/drivers/gpu/drm/xe/xe_force_wake.h
>> +++ b/drivers/gpu/drm/xe/xe_force_wake.h
>> @@ -11,13 +11,14 @@
>>   
>>   struct xe_gt;
>>   
>> -void xe_force_wake_init_gt(struct xe_gt *gt,
>> -			   struct xe_force_wake *fw);
>> +int xe_force_wake_init_gt(struct xe_gt *gt,
>> +			  struct xe_force_wake *fw);
>>   void xe_force_wake_init_engines(struct xe_gt *gt,
>>   				struct xe_force_wake *fw);
>>   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);
>> +void xe_force_wake_put_delay(struct xe_force_wake *fw, unsigned int fw_ref);
>>   
>>   const char *xe_force_wake_domain_to_str(enum xe_force_wake_domain_id id);
>>   
>> @@ -103,6 +104,22 @@ DEFINE_CLASS(xe_force_wake, struct xe_force_wake_ref,
>>   #define xe_with_force_wake(ref, fw, domains) \
>>   	__xe_with_force_wake(ref, fw, domains, __UNIQUE_ID(done))
>>   
>> +/*
>> + * Same as the xe_force_wake class, but the domains are released with
>> + * xe_force_wake_put_delay().
>> + */
>> +DEFINE_CLASS(xe_force_wake_delay, struct xe_force_wake_ref,
>> +	     xe_force_wake_put_delay(_T.fw, _T.domains),
>> +	     xe_force_wake_constructor(fw, domains),
>> +	     struct xe_force_wake *fw, unsigned int domains);
>> +
>> +#define __xe_with_force_wake_delay(ref, fw, domains, done) \
>> +	for (CLASS(xe_force_wake_delay, ref)(fw, domains), *(done) = NULL; \
>> +	     !(done); (done) = (void *)1)
>> +
>> +#define xe_with_force_wake_delay(ref, fw, domains) \
>> +	__xe_with_force_wake_delay(ref, fw, domains, __UNIQUE_ID(done))
>> +
>>   /*
>>    * Used when xe_force_wake_constructor() has already been called by another
>>    * function and the current function is responsible for releasing the forcewake
>> diff --git a/drivers/gpu/drm/xe/xe_force_wake_types.h b/drivers/gpu/drm/xe/xe_force_wake_types.h
>> index 14b7b86e801b..5c16511c6f36 100644
>> --- a/drivers/gpu/drm/xe/xe_force_wake_types.h
>> +++ b/drivers/gpu/drm/xe/xe_force_wake_types.h
>> @@ -8,6 +8,7 @@
>>   
>>   #include <linux/mutex.h>
>>   #include <linux/types.h>
>> +#include <linux/workqueue.h>
>>   
>>   #include "regs/xe_reg_defs.h"
>>   
>> @@ -82,6 +83,11 @@ struct xe_force_wake_domain {
>>   	u32 mask;
>>   	/** @ref: domain reference */
>>   	u32 ref;
>> +	/**
>> +	 * @hold_until: jiffies value until which a delayed release keeps the
>> +	 * domain awake. Protected by &xe_force_wake.lock.
>> +	 */
>> +	unsigned long hold_until;
>>   };
>>   
>>   /**
>> @@ -101,6 +107,14 @@ struct xe_force_wake {
>>   	spinlock_t lock;
>>   	/** @awake_domains: mask of all domains awake */
>>   	unsigned int awake_domains;
>> +	/**
>> +	 * @delayed_domains: mask of unreferenced domains that are kept awake
>> +	 * until @delayed_release_work puts them to sleep. A subset of
>> +	 * @awake_domains.
>> +	 */
>> +	unsigned int delayed_domains;
>> +	/** @delayed_release_work: puts expired @delayed_domains to sleep */
>> +	struct delayed_work delayed_release_work;
>>   	/** @initialized_domains: mask of all initialized domains */
>>   	unsigned int initialized_domains;
>>   	/** @domains: force wake domains */
>> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
>> index 775c826b68b4..5ed96807fe34 100644
>> --- a/drivers/gpu/drm/xe/xe_gt.c
>> +++ b/drivers/gpu/drm/xe/xe_gt.c
>> @@ -515,7 +515,10 @@ int xe_gt_init_early(struct xe_gt *gt)
>>   
>>   	xe_wa_process_gt_oob(gt);
>>   
>> -	xe_force_wake_init_gt(gt, gt_to_fw(gt));
>> +	err = xe_force_wake_init_gt(gt, gt_to_fw(gt));
>> +	if (err)
>> +		return err;
>> +
>>   	spin_lock_init(&gt->global_invl_lock);
>>   
>>   	err = xe_gt_tlb_inval_init_early(gt);
>> -- 
>> 2.43.0
>>