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

[Matthew Brost <matthew.brost@intel.com>]

On Wed, Jun 10, 2026 at 08:09:33PM -0700, Matthew Brost wrote:

Ugh, I realized I replied to wrong version but I think most of comments
are still relavent to v2, so let's continue the discussion here.

Matt 

> On Mon, Jun 01, 2026 at 09:38:04PM +0000, Zongyao Bai wrote:
> > Add delayed-release optimization:
> >     - Add domain sleep 200us 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
> > 
> 
> I think this concept makes sense, as MMIO read operations are relatively
> expensive in terms of time cost (perhaps ~5 µs). However, the downside
> is increased power usage.
> 
> Should we make delayed release an optional call—for example,
> xe_force_wake_put_delay—and only use it in specific critical paths? For
> instance, we could limit its use to paths tied to Level Zero calls like
> zeDeviceGetGlobalTimestamps.
> 
> This gets a bit tricky if xe_force_wake_put_delay is called and is not
> the last reference, followed by xe_force_wake_put being the final
> reference. However, it should be straightforward to track that an
> delayed put was requested and have the final xe_force_wake_put issue the
> delay.
> 
> > Signed-off-by: Zongyao Bai <zongyao.bai@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_force_wake.c       | 111 +++++++++++++++++------
> >  drivers/gpu/drm/xe/xe_force_wake.h       |   4 +-
> >  drivers/gpu/drm/xe/xe_force_wake_types.h |  11 +++
> >  drivers/gpu/drm/xe/xe_gt.c               |   4 +-
> >  4 files changed, 99 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_force_wake.c b/drivers/gpu/drm/xe/xe_force_wake.c
> > index 197e2197bd0a..183a17fa6d68 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
> 
> How did you choose this value? It seems like it should be something
> configurable via Kconfig or configfs.
> 
> >  
> >  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);
> 
> Is the warning above actually valid? It seems fairly normal for a fini
> to race with a delayed fw put. I would drop this.
> 
> > +		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,36 @@ 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);
> 
> I'd use guard(spinlock_irqsave) here rather manually unlock this.
> hrtimer_forward_now should be safe under fw->lock unless I'm missing
> something.
> 
> > +
> > +	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 +240,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 +271,25 @@ 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.
> > + * Mirroring i915's fw_domains_put(), the deferred sleep is fire-and-forget:
> > + * no sleep ACK is polled, since the next wake re-waits for the wake ACK.
> 
> Let's not mention the i915 in Xe code.
> 
> >   */
> >  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.
> > -	 */
> 
> Why delete this comment?
> 
> >  	if (!fw_ref)
> >  		return;
> >  
> > @@ -245,20 +301,19 @@ 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)
> > -			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);
> 
> This deleted code for domain_sleep_wait / error probably needs to be in
> xe_force_wake_domain_timer.
> 
> >  }
> >  
> >  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..19679b923dca 100644
> > --- a/drivers/gpu/drm/xe/xe_force_wake.h
> > +++ b/drivers/gpu/drm/xe/xe_force_wake.h
> > @@ -11,8 +11,8 @@
> >  
> >  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,
> > 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 */
> 
> Protected fw_back->lock.
> 
> > +	bool timer_rearm;
> 
> In general, I’d reorganize the layout so that structs are at the top of
> xe_force_wake_domain, followed by u32 fields, and finally the bool
> fields.
> 
> > +	/** @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..43a79698cd04 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;
> 
> I'd add newline here.
> 
> Matt
> 
> >  	spin_lock_init(&gt->global_invl_lock);
> >  
> >  	err = xe_gt_tlb_inval_init_early(gt);
> > -- 
> > 2.43.0
> >