Re: [PATCH v2 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] Resource Accounting Leak / Throttle Bypass via loop state reset
- [High] Eviction Ping-Pong and Mass Locking via blind `drm_exec` iteration
--

commit 55fa3886644544ae8f5e3d97c8a80ceba452b3ca
Author: Natalie Vock <nat@pixelcluster.dev>
Subject: drm/amdgpu: use drm_exec during BO validation

This commit updates the BO validation in amdgpu_cs to use drm_exec, aiming
to prevent deadlocks caused by resource constraints and avoid sporadic
-ENOMEM issues when using GDS.

> 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
> @@ -793,7 +793,7 @@ static int amdgpu_cs_bo_validate(void *param, struct amdgpu_bo *bo)
>  	struct ttm_operation_ctx ctx = {
>  		.interruptible = true,
>  		.no_wait_gpu = false,
> -		.resv = bo->tbo.base.resv
> +		.exec = &p->exec,
>  	};
>  	uint32_t domain;
>  	int r;

[Severity: High]
Does setting .exec to &p->exec cause TTM to add evicted victim buffers to the
transaction's locked object list?

If so, later when drm_exec_for_each_locked_object() iterates over all
objects in p->exec, will it inadvertently re-validate these victim BOs
and attempt to move them back into VRAM?

[ ... ]

> @@ -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: High]
Is it intentional to unconditionally reset these accumulators inside the
retry loop?

Since this block is inside the drm_exec_until_all_locked() loop, if a later
validation causes WW lock contention and triggers a retry, the loop will
restart and these counters will be zeroed.

Because TTM does not rollback physical moves when dropping locks, this
appears to discard the accounting for BOs that were already moved,
potentially bypassing the migration bandwidth throttle.

> -	drm_exec_for_each_locked_object(&p->exec, obj) {
> -		r = amdgpu_cs_bo_validate(p, gem_to_amdgpu_bo(obj));
> -		if (unlikely(r))
> +		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));
> +			drm_exec_retry_on_contention(&p->exec);
> +			if (unlikely(r))
> +				goto out_free_user_pages;
> +		}

[Severity: High]
Could this iteration cause an eviction ping-pong?

Because TTM eviction adds victim buffers to p->exec, this loop will
iterate over those newly added victims. amdgpu_cs_bo_validate() might
then see the victim's preferred domains and try to move it back into
VRAM, forcing another eviction and trapping the validation in an infinite
loop until the threshold is hit or VRAM is exhausted.

> +
> +		amdgpu_cs_report_moved_bytes(p->adev, p->bytes_moved,
> +					     p->bytes_moved_vis);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706-ttm_2_drm_exec-v2-0-4bf6bfc0d320@pixelcluster.dev?part=10