Re: [PATCH 10/10] drm/amdgpu: use drm_exec during BO validation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Iterating over `p->exec` objects for validation inadvertently validates victim BOs that were just evicted, breaking VRAM overcommit.
- [Medium] Buffer migration budget is bypassed when a submission retries due to lock contention.
--

commit 106f3922010dddea6e74f4d9b1260b3b5ba91adf
Author: Christian König <christian.koenig@amd.com>

drm/amdgpu: use drm_exec during BO validation

This commit modifies the CS parser BO validation sequence to use the
drm_exec infrastructure, aiming to detect and handle deadlocks caused by
resource constraints during eviction.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index c2e6495a28bc5..052d41013f7a0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c

[ ... ]

> @@ -922,47 +923,53 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>  			if (unlikely(r))
>  				goto out_free_user_pages;
>  		}

[ ... ]

> -	amdgpu_cs_get_threshold_for_moves(p->adev, &p->bytes_moved_threshold,
> -					  &p->bytes_moved_vis_threshold);
> -	p->bytes_moved = 0;
> -	p->bytes_moved_vis = 0;
> -
> -	r = amdgpu_vm_validate(p->adev, &fpriv->vm, NULL,
> -			       amdgpu_cs_bo_validate, p);
> -	if (r) {
> -		drm_err(adev_to_drm(p->adev), "amdgpu_vm_validate() failed.\n");
> -		goto out_free_user_pages;
> -	}
> +		amdgpu_cs_get_threshold_for_moves(p->adev, &p->bytes_moved_threshold,
> +						  &p->bytes_moved_vis_threshold);
> +		p->bytes_moved = 0;
> +		p->bytes_moved_vis = 0;

[Severity: Medium]
If a submission physically moves buffers but then hits lock contention later
in the execution, drm_exec_retry_on_contention() will restart the
drm_exec_until_all_locked() loop.

Does resetting p->bytes_moved and p->bytes_moved_vis to 0 here cause the
driver to lose track of the bytes already moved during the aborted pass?

When the retry finally succeeds, it appears amdgpu_cs_report_moved_bytes()
will only report the final pass, which might bypass the migration budget
throttling logic and leak the physical migration costs already incurred.

> +		r = amdgpu_vm_validate(p->adev, &fpriv->vm, NULL,
> +				       amdgpu_cs_bo_validate, p);
> +		drm_exec_retry_on_contention(&p->exec);
> +		if (r) {
> +			drm_err(adev_to_drm(p->adev), "amdgpu_vm_validate() failed.\n");
>  			goto out_free_user_pages;
> +		}
> +
> +		drm_exec_for_each_locked_object(&p->exec, obj) {
> +			r = amdgpu_cs_bo_validate(p, gem_to_amdgpu_bo(obj));

[Severity: High]
When TTM evicts a buffer, does it use ctx.exec (which now points to p->exec
after this patch) to lock the victim BO?

If so, it seems the victim BO gets added to the p->exec locked objects list.
Since this drm_exec_for_each_locked_object() loop blindly iterates over all
objects in p->exec, won't it inadvertently validate the newly evicted victim
BO as well?

Calling amdgpu_cs_bo_validate() on the victim could attempt to move it back
to its preferred domain in VRAM, which seems like it would defeat the eviction
entirely and potentially break VRAM overcommit.

> +			drm_exec_retry_on_contention(&p->exec);
> +			if (unlikely(r))
> +				goto out_free_user_pages;
> +		}
> +
> +		amdgpu_cs_report_moved_bytes(p->adev, p->bytes_moved,
> +					     p->bytes_moved_vis);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-ttm_2_drm_exec-v1-0-43685ac1286b@gmx.de?part=10