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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Returning -EAGAIN from `xe_pci_runtime_idle` aborts the runtime PM idle transition but fails to schedule a retry, permanently preventing runtime suspend.
- [Medium] Using `xe_gt_info` in a runtime PM path will cause severe log spam when forcewake is held.
--

commit 987ddc78bc72644b422f8d130f4407129128a641
Author: Vinay Belgaumkar <vinay.belgaumkar@intel.com>

drm/xe: Inspect all forcewake bits for runtime idleness

This commit updates the runtime idleness check to inspect all forcewake
bits across all domains. If any bit is set, it returns -EAGAIN to skip 
the suspend operation.

> diff --git a/drivers/gpu/drm/xe/xe_force_wake.c b/drivers/gpu/drm/xe/xe_force_wake.c
[ ... ]
> +bool xe_force_wake_any_bit_set(struct xe_force_wake *fw)
> +{
> +	struct xe_gt *gt = fw->gt;
> +	struct xe_force_wake_domain *domain;
> +	unsigned int tmp;
> +	u32 val;
> +
> +	for_each_fw_domain(domain, fw, tmp) {
> +		val = xe_mmio_read32(&gt->mmio, domain->reg_ctl);
> +		if (val & 0xFFFF) {
> +			xe_gt_info(gt, "FW bit set in domain: %d, val: 0x%x",
> +				   domain->id, val);

[Severity: Medium]
Will this cause severe log spam in high-frequency runtime PM paths?

Forcewake can be legitimately held for long periods (for example, by
userspace profiling tools, IGT tests, or debugfs readers). Every time the
driver drops its PM reference and triggers a runtime idle check, this
unconditional info message will print if the external agent is still holding
forcewake. 

Should this be a debug-level log (xe_gt_dbg) or rate-limited instead?

> +			return true;
> +		}
> +	}
> +
> +	return false;
> +}

> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
[ ... ]
> @@ -1437,9 +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);
> +	struct xe_gt *gt;
> +	u8 id;
>  
>  	xe_pm_d3cold_allowed_toggle(xe);
>  
> +	for_each_gt(gt, xe, id)
> +		if (xe_force_wake_any_bit_set(gt_to_fw(gt)))
> +			return -EAGAIN;

[Severity: High]
Will returning -EAGAIN here permanently prevent runtime suspend?

When the PM core's rpm_idle() function receives a non-zero return value, it
aborts the idle transition and bypasses the subsequent call to rpm_suspend().
Returning an error code does not automatically instruct the PM core to poll
or reschedule an autosuspend.

If a forcewake bit happens to be set at the moment the PM usage count drops
to 0, it seems the device will remain stranded in the active (D0) state
indefinitely. 

Should this explicitly schedule a retry, perhaps by calling
pm_runtime_mark_last_busy() and pm_runtime_autosuspend() before returning?

>  	return 0;
>  }
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909013712.1962521-1-vinay.belgaumkar@intel.com?part=1