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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] drm/xe: delayed forcewake domains stranded when
  rpm_get_if_active fails
- [Low] drm/xe/forcewake: xe_force_wake_assert_held() passes on delayed
  domains
--

commit 4a71af2399dc0153d2185a60b2aa48a76a02006c
Author: Zongyao Bai <zongyao.bai@intel.com>

drm/xe/forcewake: add delayed release with hysteresis

This commit adds xe_force_wake_put_delay() and a delayed release worker to
keep forcewake domains awake for a short hold period after their last
reference is dropped.

> diff --git a/drivers/gpu/drm/xe/xe_force_wake.c b/drivers/gpu/drm/xe/xe_force_wake.c
> index 197e2197bd0a3..d09d138c10840 100644
> --- a/drivers/gpu/drm/xe/xe_force_wake.c
> +++ b/drivers/gpu/drm/xe/xe_force_wake.c

[ ... ]

> @@ -244,21 +307,101 @@ 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 (hold_ms && fw->delayed_enabled) {
> +			unsigned long hold = xe_force_wake_hold_jiffies(hold_ms);
> +
> +			domain->hold_until = jiffies + hold;
> +			fw->delayed_domains |= BIT(domain->id);
> +			/* No-op if pending, the worker requeues for the latest hold. */
> +			queue_delayed_work(system_percpu_wq,
> +					   &fw->delayed_release_work, hold);
> +		} else {
>  			sleep |= BIT(domain->id);
> -			domain_sleep(gt, domain);
>  		}
>  	}

[Severity: Low]
Does keeping BIT(domain->id) set in fw->awake_domains while a domain is in
fw->delayed_domains cause xe_force_wake_assert_held() to pass even when no
caller holds a reference?

drivers/gpu/drm/xe/xe_force_wake.h:xe_force_wake_assert_held() {
	xe_gt_assert(fw->gt, domain != XE_FORCEWAKE_ALL);
	xe_gt_assert(fw->gt, fw->awake_domains & domain);
}

During the delayed hold window, domain->ref is 0 and
xe_force_wake_delayed_release_work() can put the domain to sleep on
another CPU at any time, but xe_force_wake_assert_held() only checks
fw->awake_domains & domain without excluding fw->delayed_domains.

> +	ack_fail = xe_force_wake_sleep_locked(fw, sleep);
>  	spin_unlock_irqrestore(&fw->lock, flags);
>  
> +	xe_force_wake_warn_sleep_fail(gt, ack_fail);
> +}

[ ... ]

> +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_device *xe = gt_to_xe(gt);
> +	struct xe_force_wake_domain *domain;
> +	unsigned int tmp, expired = 0, ack_fail;
> +	unsigned long flags, next = 0;
> +	bool remaining = false;
> +
> +	/* Never resume from here, the suspend flush settles delayed domains. */
> +	if (!xe_pm_runtime_get_if_active(xe))
> +		return;

[Severity: Medium]
Can returning early here without sleeping expired domains or requeuing
fw->delayed_release_work leave fw->delayed_domains stranded awake?

Although commit "drm/xe/forcewake: flush delayed release at power and
teardown boundaries" flushes delayed domains in xe_gt_suspend() and
xe_gt_runtime_suspend(), xe_pm_runtime_get_if_active() returns false as
soon as rpm_suspend() sets dev->power.runtime_status to RPM_SUSPENDING.
If xe_force_wake_delayed_release_work() runs while xe_pm_runtime_suspend()
is in xe_pxp_pm_suspend(), xe_bo_evict_all(), or an earlier GT's suspend,
and that step fails and jumps to out_resume, the remaining GTs are never
flushed and runtime_status returns to RPM_ACTIVE with their delayed
domains still awake and no worker pending, preventing C6 entry.  In
addition, when CONFIG_PM is disabled, pm_runtime_get_if_active() returns
-EINVAL, so xe_pm_runtime_get_if_active() is always false and delayed
domains are never released.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110220542.432595-1-zongyao.bai@intel.com?part=1