Re: [PATCH v2] drm/xe/forcewake: add delayed-release optimization

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

On 6/11/2026 4:59 AM, Maarten Lankhorst wrote:
> Hey,
>
> Why not used delayed_work instead of introducing a hrtimer?
> Is the 200 µs that time critical?
>
> Regardless, can we not configure the delay in hardware instead of software?
>
> Kind regards,
> ~Maarten Lankhorst

Hi Maarten,

     Thank you for review.

1. Why not used delayed_work instead of introducing a hrtimer?
    Is the 200 µs that time critical?
    This optimization is target for back-to-back access to avoid 
frequently sleep/wake up actions.
    A tiny short hold time is enough for that. Introducing a hrtimer 
could make it precise with less jitter.
    delayed_work may make this tiny-short time > 1ms.
    200us is validated for zeDeviceGetGlobalTimestamps benchmark.
    But yes, we could test more values, like 100us/50us/...
    The shorter the better for the power consume.
    I'll update the result here later.

2. Regardless, can we not configure the delay in hardware instead of software?
    I do not see a generic, driver-visible hardware control for programming a
    forcewake auto-release delay in the current xe forcewake interface or register.
    And it likes a kind of software policy for optimization, there is no hard-limitation here.
    so the delayed release is implemented in software.

>
> On 6/11/26 03:03, Zongyao Bai wrote:
>> Add delayed-release optimization:
>>   - Add domain sleep XE_FORCE_WAKE_HOLD_DELAY_US after xe_force_wake_put()
>>   - Skip MMIO wake in xe_force_wake_get() if domain still awake.
>> Reduces frequent wake/sleep cycles for back-to-back operations.
>> Examples of scenarios: zeDeviceGetGlobalTimestamps read by VTune, PTI
>>
>> v2:
>>   - Add xe_force_wake_flush() and call it on the runtime/system
>>     suspend paths to ensure no deferred-sleep runs after GT power-off.
>>     (Sashiko)
>>
>> Assisted-by: GitHub-Copilot:claude-sonnet-4.6
>> Assisted-by: GitHub-Copilot:claude-opus-4.8 #v2
>> Signed-off-by: Zongyao Bai <zongyao.bai@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_force_wake.c       | 146 +++++++++++++++++++----
>>   drivers/gpu/drm/xe/xe_force_wake.h       |  23 +++-
>>   drivers/gpu/drm/xe/xe_force_wake_types.h |  11 ++
>>   drivers/gpu/drm/xe/xe_gt.c               |  26 ++--
>>   4 files changed, 171 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_force_wake.c b/drivers/gpu/drm/xe/xe_force_wake.c
>> index 197e2197bd0a..b7c674f5f69a 100644
>> --- a/drivers/gpu/drm/xe/xe_force_wake.c
>> +++ b/drivers/gpu/drm/xe/xe_force_wake.c
>> @@ -6,15 +6,20 @@
>>   #include "xe_force_wake.h"
>>   
>>   #include <drm/drm_util.h>
>> +#include <linux/device.h>
>> +#include <linux/hrtimer.h>
>>   
>>   #include "regs/xe_gt_regs.h"
>>   #include "regs/xe_reg_defs.h"
>> +#include "xe_device.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
>> +#define XE_FORCE_WAKE_HOLD_DELAY_US	200
>>   
>>   static const char *str_wake_sleep(bool wake)
>>   {
>> @@ -27,6 +32,8 @@ static void mark_domain_initialized(struct xe_force_wake *fw,
>>   	fw->initialized_domains |= BIT(id);
>>   }
>>   
>> +static enum hrtimer_restart xe_force_wake_domain_timer(struct hrtimer *timer);
>> +
>>   static void init_domain(struct xe_force_wake *fw,
>>   			enum xe_force_wake_domain_id id,
>>   			struct xe_reg reg, struct xe_reg ack)
>> @@ -38,11 +45,29 @@ static void init_domain(struct xe_force_wake *fw,
>>   	domain->reg_ack = ack;
>>   	domain->val = FORCEWAKE_MT(FORCEWAKE_KERNEL);
>>   	domain->mask = FORCEWAKE_MT_MASK(FORCEWAKE_KERNEL);
>> +	domain->fw_back = fw;
>> +	hrtimer_setup(&domain->timer, xe_force_wake_domain_timer,
>> +		      CLOCK_MONOTONIC, HRTIMER_MODE_REL);
>>   
>>   	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_fini(void *arg)
>> +{
>> +	struct xe_force_wake *fw = arg;
>> +	struct xe_gt *gt = fw->gt;
>> +	struct xe_force_wake_domain *domain;
>> +	unsigned int tmp;
>> +
>> +	for_each_fw_domain(domain, fw, tmp) {
>> +		xe_gt_WARN(gt, domain->ref,
>> +			   "Forcewake domain %d still referenced (%u) at teardown\n",
>> +			   domain->id, domain->ref);
>> +		hrtimer_cancel(&domain->timer);
>> +	}
>> +}
>> +
>> +int xe_force_wake_init_gt(struct xe_gt *gt, struct xe_force_wake *fw)
>>   {
>>   	struct xe_device *xe = gt_to_xe(gt);
>>   
>> @@ -58,6 +83,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_fini, fw);
>>   }
>>   
>>   void xe_force_wake_init_engines(struct xe_gt *gt, struct xe_force_wake *fw)
>> @@ -142,10 +169,37 @@ static void domain_sleep(struct xe_gt *gt, struct xe_force_wake_domain *domain)
>>   	__domain_ctl(gt, domain, false);
>>   }
>>   
>> -static int domain_sleep_wait(struct xe_gt *gt,
>> -			     struct xe_force_wake_domain *domain)
>> +static enum hrtimer_restart xe_force_wake_domain_timer(struct hrtimer *timer)
>>   {
>> -	return __domain_wait(gt, domain, false);
>> +	struct xe_force_wake_domain *domain =
>> +		container_of(timer, struct xe_force_wake_domain, timer);
>> +	struct xe_force_wake *fw = domain->fw_back;
>> +	struct xe_gt *gt = fw->gt;
>> +	unsigned long flags;
>> +
>> +	xe_gt_assert(gt, !xe_pm_runtime_suspended(gt_to_xe(gt)));
>> +
>> +	spin_lock_irqsave(&fw->lock, flags);
>> +
>> +	if (!(fw->timer_domains & BIT(domain->id)) || domain->ref) {
>> +		spin_unlock_irqrestore(&fw->lock, flags);
>> +		return HRTIMER_NORESTART;
>> +	}
>> +	if (domain->timer_rearm) {
>> +		domain->timer_rearm = false;
>> +		spin_unlock_irqrestore(&fw->lock, flags);
>> +		hrtimer_forward_now(timer,
>> +				    ns_to_ktime(XE_FORCE_WAKE_HOLD_DELAY_US *
>> +						NSEC_PER_USEC));
>> +		return HRTIMER_RESTART;
>> +	}
>> +
>> +	fw->timer_domains &= ~BIT(domain->id);
>> +	domain_sleep(gt, domain);
>> +	fw->awake_domains &= ~BIT(domain->id);
>> +	spin_unlock_irqrestore(&fw->lock, flags);
>> +
>> +	return HRTIMER_NORESTART;
>>   }
>>   
>>   /**
>> @@ -187,8 +241,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->awake_domains & BIT(domain->id)) {
>> +				fw->timer_domains &= ~BIT(domain->id);
>> +				hrtimer_try_to_cancel(&domain->timer);
>> +			} else {
>> +				awake_rqst |= BIT(domain->id);
>> +				domain_wake(gt, domain);
>> +			}
>>   		}
>>   		ref_incr |= BIT(domain->id);
>>   	}
>> @@ -213,27 +272,23 @@ unsigned int __must_check xe_force_wake_get(struct xe_force_wake *fw,
>>   }
>>   
>>   /**
>> - * xe_force_wake_put - Decrement the refcount and put domain to sleep if refcount becomes 0
>> + * xe_force_wake_put - Decrement the refcount and arm the delayed-sleep timer
>>    * @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.
>> + * This function reduces the reference counts for domains in fw_ref.  When a
>> + * domain's refcount reaches 0 the sleep request is not issued immediately;
>> + * instead a hrtimer is armed for XE_FORCE_WAKE_HOLD_DELAY_US so that a rapid
>> + * xe_force_wake_get() can reuse the still-awake domain at zero MMIO cost.  On
>> + * timer expiry, if the domain is still idle, the sleep request is written.
>>    */
>>   void xe_force_wake_put(struct xe_force_wake *fw, unsigned int fw_ref)
>>   {
>>   	struct xe_gt *gt = fw->gt;
>>   	struct xe_force_wake_domain *domain;
>> -	unsigned int tmp, sleep = 0;
>> +	unsigned int tmp;
>>   	unsigned long flags;
>> -	int ack_fail = 0;
>>   
>> -	/*
>> -	 * Avoid unnecessary lock and unlock when the function is called
>> -	 * in error path of individual domains.
>> -	 */
>>   	if (!fw_ref)
>>   		return;
>>   
>> @@ -245,20 +300,59 @@ void xe_force_wake_put(struct xe_force_wake *fw, unsigned int fw_ref)
>>   		xe_gt_assert(gt, domain->ref);
>>   
>>   		if (!--domain->ref) {
>> -			sleep |= BIT(domain->id);
>> -			domain_sleep(gt, domain);
>> +			fw->timer_domains |= BIT(domain->id);
>> +			if (hrtimer_callback_running(&domain->timer)) {
>> +				domain->timer_rearm = true;
>> +			} else {
>> +				domain->timer_rearm = false;
>> +				hrtimer_start(&domain->timer,
>> +					      ns_to_ktime(XE_FORCE_WAKE_HOLD_DELAY_US *
>> +							  NSEC_PER_USEC),
>> +				      HRTIMER_MODE_REL);
>> +			}
>>   		}
>>   	}
>> -	for_each_fw_domain_masked(domain, sleep, fw, tmp) {
>> -		if (domain_sleep_wait(gt, domain) == 0)
>> +	spin_unlock_irqrestore(&fw->lock, flags);
>> +}
>> +
>> +/**
>> + * xe_force_wake_flush - Cancel pending delayed-sleep timers and settle domains
>> + * @fw: Pointer to the force wake structure
>> + *
>> + * The delayed-release optimization in xe_force_wake_put() arms a short hrtimer
>> + * instead of putting an idle domain to sleep immediately.  That deferred sleep
>> + * performs MMIO and must not be allowed to run after the device has been
>> + * powered down.  Callers on the runtime/system suspend path must therefore
>> + * flush any outstanding timers once forcewake has been fully released, before
>> + * the hardware is powered off.
>> + *
>> + * For every domain this cancels the pending timer (synchronously waiting for an
>> + * in-flight callback to finish) and, if the domain is still idle with an
>> + * outstanding deferred sleep, performs the sleep request right away so the
>> + * domain is left in a consistent, fully-asleep state.  hrtimer_cancel() must be
>> + * called without holding fw->lock, since the timer callback takes that same
>> + * lock.
>> + */
>> +void xe_force_wake_flush(struct xe_force_wake *fw)
>> +{
>> +	struct xe_gt *gt = fw->gt;
>> +	struct xe_force_wake_domain *domain;
>> +	unsigned int tmp;
>> +	unsigned long flags;
>> +
>> +	for_each_fw_domain(domain, fw, tmp)
>> +		hrtimer_cancel(&domain->timer);
>> +
>> +	spin_lock_irqsave(&fw->lock, flags);
>> +	for_each_fw_domain(domain, fw, tmp) {
>> +		domain->timer_rearm = false;
>> +		if ((fw->timer_domains & BIT(domain->id)) && !domain->ref) {
>> +			fw->timer_domains &= ~BIT(domain->id);
>> +			domain_sleep(gt, domain);
>>   			fw->awake_domains &= ~BIT(domain->id);
>> -		else
>> -			ack_fail |= BIT(domain->id);
>> +		}
>>   	}
>>   	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);
>>   }
>>   
>>   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..1b6d5efead38 100644
>> --- a/drivers/gpu/drm/xe/xe_force_wake.h
>> +++ b/drivers/gpu/drm/xe/xe_force_wake.h
>> @@ -11,13 +11,32 @@
>>   
>>   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_flush(struct xe_force_wake *fw);
>> +
>> +/**
>> + * xe_force_wake_put_and_flush - Release forcewake and settle any deferred sleep
>> + * @fw: Pointer to the force wake structure
>> + * @fw_ref: return of xe_force_wake_get()
>> + *
>> + * Convenience helper for paths that must guarantee the domain is fully asleep
>> + * before returning (typically right before the GT is powered off on
>> + * runtime/system suspend). Because the flush has to run after the final put
>> + * but before HW power-off, scope-based cleanup via CLASS(xe_force_wake, ...)
>> + * cannot be used here.
>> + */
>> +static inline void
>> +xe_force_wake_put_and_flush(struct xe_force_wake *fw, unsigned int fw_ref)
>> +{
>> +	xe_force_wake_put(fw, fw_ref);
>> +	xe_force_wake_flush(fw);
>> +}
>>   
>>   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_types.h b/drivers/gpu/drm/xe/xe_force_wake_types.h
>> index 14b7b86e801b..ee5675069fe0 100644
>> --- a/drivers/gpu/drm/xe/xe_force_wake_types.h
>> +++ b/drivers/gpu/drm/xe/xe_force_wake_types.h
>> @@ -6,6 +6,7 @@
>>   #ifndef _XE_FORCE_WAKE_TYPES_H_
>>   #define _XE_FORCE_WAKE_TYPES_H_
>>   
>> +#include <linux/hrtimer.h>
>>   #include <linux/mutex.h>
>>   #include <linux/types.h>
>>   
>> @@ -51,6 +52,8 @@ enum xe_force_wake_domains {
>>   	XE_FORCEWAKE_ALL	= BIT(XE_FW_DOMAIN_ID_COUNT)
>>   };
>>   
>> +struct xe_force_wake;
>> +
>>   /**
>>    * struct xe_force_wake_domain - Xe force wake power domain
>>    *
>> @@ -82,6 +85,12 @@ struct xe_force_wake_domain {
>>   	u32 mask;
>>   	/** @ref: domain reference */
>>   	u32 ref;
>> +	/** @timer_rearm: put() ran while callback was in-flight; callback must restart timer */
>> +	bool timer_rearm;
>> +	/** @timer: hrtimer for delayed sleep request */
>> +	struct hrtimer timer;
>> +	/** @fw_back: back pointer to parent xe_force_wake */
>> +	struct xe_force_wake *fw_back;
>>   };
>>   
>>   /**
>> @@ -101,6 +110,8 @@ struct xe_force_wake {
>>   	spinlock_t lock;
>>   	/** @awake_domains: mask of all domains awake */
>>   	unsigned int awake_domains;
>> +	/** @timer_domains: mask of domains with an outstanding delayed-sleep timer */
>> +	unsigned int timer_domains;
>>   	/** @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 783eb6d631b5..bba51a655b28 100644
>> --- a/drivers/gpu/drm/xe/xe_gt.c
>> +++ b/drivers/gpu/drm/xe/xe_gt.c
>> @@ -511,7 +511,9 @@ 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);
>> @@ -998,13 +1000,15 @@ void xe_gt_suspend_prepare(struct xe_gt *gt)
>>   
>>   int xe_gt_suspend(struct xe_gt *gt)
>>   {
>> +	unsigned int fw_ref;
>>   	int err;
>>   
>>   	xe_gt_dbg(gt, "suspending\n");
>>   	xe_gt_sanitize(gt);
>>   
>> -	CLASS(xe_force_wake, fw_ref)(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>> -	if (!xe_force_wake_ref_has_domain(fw_ref.domains, XE_FORCEWAKE_ALL)) {
>> +	fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>> +	if (!xe_force_wake_ref_has_domain(fw_ref, XE_FORCEWAKE_ALL)) {
>> +		xe_force_wake_put(gt_to_fw(gt), fw_ref);
>>   		xe_gt_err(gt, "suspend failed (%pe)\n", ERR_PTR(-ETIMEDOUT));
>>   		return -ETIMEDOUT;
>>   	}
>> @@ -1012,7 +1016,7 @@ int xe_gt_suspend(struct xe_gt *gt)
>>   	err = xe_uc_suspend(&gt->uc);
>>   	if (err) {
>>   		xe_gt_err(gt, "suspend failed (%pe)\n", ERR_PTR(err));
>> -		return err;
>> +		goto err_force_wake;
>>   	}
>>   
>>   	xe_gt_idle_disable_pg(gt);
>> @@ -1021,7 +1025,10 @@ int xe_gt_suspend(struct xe_gt *gt)
>>   
>>   	xe_gt_dbg(gt, "suspended\n");
>>   
>> -	return 0;
>> +err_force_wake:
>> +	xe_force_wake_put_and_flush(gt_to_fw(gt), fw_ref);
>> +
>> +	return err;
>>   }
>>   
>>   void xe_gt_shutdown(struct xe_gt *gt)
>> @@ -1080,10 +1087,13 @@ int xe_gt_resume(struct xe_gt *gt)
>>    */
>>   int xe_gt_runtime_suspend(struct xe_gt *gt)
>>   {
>> +	unsigned int fw_ref;
>> +
>>   	xe_gt_dbg(gt, "runtime suspending\n");
>>   
>> -	CLASS(xe_force_wake, fw_ref)(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>> -	if (!xe_force_wake_ref_has_domain(fw_ref.domains, XE_FORCEWAKE_ALL)) {
>> +	fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>> +	if (!xe_force_wake_ref_has_domain(fw_ref, XE_FORCEWAKE_ALL)) {
>> +		xe_force_wake_put(gt_to_fw(gt), fw_ref);
>>   		xe_gt_err(gt, "runtime suspend failed (%pe)\n", ERR_PTR(-ETIMEDOUT));
>>   		return -ETIMEDOUT;
>>   	}
>> @@ -1093,6 +1103,8 @@ int xe_gt_runtime_suspend(struct xe_gt *gt)
>>   
>>   	xe_gt_dbg(gt, "runtime suspended\n");
>>   
>> +	xe_force_wake_put_and_flush(gt_to_fw(gt), fw_ref);
>> +
>>   	return 0;
>>   }
>>