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 <[email protected]> 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/[email protected]?part=10
