Re: [PATCH] drm/xe: Don't fail runtime suspend permanently on VRAM eviction -ENOMEM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] drm/xe: missing pinned BO rollback on -ENOMEM in xe_pm_runtime_suspend()
--

commit c1d7f6ee67a2e6a9eb3953b3524ff03d2efde556
Author: Thomas Hellström <thomas.hellstrom@linux.intel.com>

drm/xe: Don't fail runtime suspend permanently on VRAM eviction -ENOMEM

This commit converts -ENOMEM from xe_bo_evict_all() into -EAGAIN during
runtime suspend to D3cold so that the runtime PM core treats the memory
shortage as transient, keeps the device active, and retries runtime
suspend later.

> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> index 4f1035ab1eb2b..5652511294bc6 100644
> --- a/drivers/gpu/drm/xe/xe_pm.c
> +++ b/drivers/gpu/drm/xe/xe_pm.c
> @@ -649,8 +649,16 @@ int xe_pm_runtime_suspend(struct xe_device *xe)
>  
>  	if (xe->d3cold.allowed) {
>  		err = xe_bo_evict_all(xe);
> -		if (err)
> +		if (err) {
> +			/*
> +			 * Runtime PM treats errors other than -EAGAIN and
> +			 * -EBUSY as fatal, so report memory shortage as
> +			 * transient to allow a later retry.
> +			 */
> +			if (err == -ENOMEM)
> +				err = -EAGAIN;
>  			goto out_resume;
> +		}
>  	}
>  
>  	for_each_gt(gt, xe, id) {

[Severity: Critical]
When xe_bo_evict_all() fails with -ENOMEM and xe_pm_runtime_suspend()
returns -EAGAIN via out_resume, what rolls back the pinned BOs that were
already backed up and moved to the evicted lists before the allocation
failure?

Unlike system suspend, runtime suspend does not run the PM notifier
(xe_bo_notifier_prepare_all_pinned()), so xe_bo_evict_all() allocates
bo->backup_obj on the fly inside xe_bo_evict_pinned() using
xe_bo_init_locked():

drivers/gpu/drm/xe/xe_bo_evict.c:xe_bo_evict_all() {
	...
	ret = xe_bo_apply_to_pinned(xe, &xe->pinned.late.external,
				    &xe->pinned.late.external, xe_bo_evict_pinned);

	if (!ret)
		ret = xe_bo_apply_to_pinned(xe, &xe->pinned.late.kernel_bo_present,
					    &xe->pinned.late.evicted, xe_bo_evict_pinned);
	...
	return xe_bo_apply_to_pinned(xe, &xe->pinned.early.kernel_bo_present,
				     &xe->pinned.early.evicted,
				     xe_bo_evict_pinned);
}

In xe_bo_apply_to_pinned(), each BO that succeeds is moved to
still_in_list with bo->backup_obj populated (and backup->parent_obj
holding a reference to bo). If a subsequent BO fails in
xe_bo_evict_pinned() with -ENOMEM, only that failing BO is put back on
pinned_list, while all previously processed BOs in still_in_list are
spliced onto new_list (xe->pinned.late.evicted or
xe->pinned.early.evicted):

drivers/gpu/drm/xe/xe_bo_evict.c:xe_bo_apply_to_pinned() {
		...
		ret = pinned_fn(bo);
		if (ret && pinned_list != new_list) {
			spin_lock(&xe->pinned.lock);
			if (!list_empty(&bo->pinned_link))
				list_move(&bo->pinned_link, pinned_list);
			spin_unlock(&xe->pinned.lock);
		}
		xe_bo_put(bo);
		spin_lock(&xe->pinned.lock);
	}
	list_splice_tail(&still_in_list, new_list);
	spin_unlock(&xe->pinned.lock);
}

Because returning -EAGAIN leaves the device active in D0 without calling
xe_pm_runtime_resume(), neither xe_bo_restore_early() nor
xe_bo_restore_late() runs, and out_resume in xe_pm_runtime_suspend() does
not restore or unprepare those pinned BOs:

drivers/gpu/drm/xe/xe_pm.c:xe_pm_runtime_suspend() {
	...
out_resume:
	xe_pm_update_pme_enabled(xe, false);
	xe_display_pm_runtime_resume(xe);
	xe_pxp_pm_resume(xe->pxp);
}

While the device stays active in D0 and later retries suspend, can this
cause three issues?

1. While the device remains active in D0, the GPU, GuC, and CPU continue
modifying pinned kernel BOs in VRAM. When runtime suspend is later
retried (or system suspend runs), xe_bo_evict_all() only iterates
xe->pinned.late.kernel_bo_present and xe->pinned.early.kernel_bo_present,
skipping all kernel BOs left on xe->pinned.late.evicted and
xe->pinned.early.evicted. When that later suspend succeeds and the device
subsequently resumes from D3cold, won't xe_bo_restore_early() and
xe_bo_restore_late() overwrite modified VRAM with the stale bo->backup_obj
snapshots from the earlier aborted runtime suspend?

2. For external BOs on xe->pinned.late.external that succeeded before
-ENOMEM, bo->backup_obj remains set with a mutual reference between
bo->backup_obj and backup->parent_obj. If an external BO is unpinned while
the device is active via xe_bo_unpin_external():

drivers/gpu/drm/xe/xe_bo.c:xe_bo_unpin_external() {
	...
	xe_bo_unpin_account(bo);
	ttm_bo_move_to_lru_tail_unlocked(&bo->ttm);
}

it is removed from xe->pinned.late.external without releasing
bo->backup_obj (unlike xe_bo_unpin()). Doesn't that leave an unbreakable
reference cycle leaking both bo and bo->backup_obj?

3. If system suspend or hibernation runs after an aborted runtime suspend,
xe_bo_notifier_prepare_all_pinned() -> xe_bo_notifier_prepare_pinned()
iterates xe->pinned.late.external:

drivers/gpu/drm/xe/xe_bo.c:xe_bo_notifier_prepare_pinned() {
	...
		xe_assert(xe, !bo->backup_obj);
	...
		backup->parent_obj = xe_bo_get(bo); /* Released by bo_destroy */
		xe_bo_pin_account(backup);
		bo->backup_obj = backup;
}

Won't any external BO that already had bo->backup_obj allocated during the
failed runtime suspend trigger xe_assert(xe, !bo->backup_obj) and leak the
previous backup_obj when bo->backup_obj is overwritten?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020100545.46715-1-thomas.hellstrom@linux.intel.com?part=1