On 3/17/2025 9:09 PM, Christian König wrote:
Hi Sathish,
Am 17.03.25 um 15:40 schrieb Sundararaju, Sathishkumar:
Hi Christian,
On 3/17/2025 7:33 PM, Christian König wrote:
Am 16.03.25 um 16:36 schrieb Sathishkumar S:
Wait for vm page table updates to finish before resuming user queues.
Resume must follow after completion of pte updates to avoid page faults.
amdgpu: [gfxhub] page fault (src_id:0 ring:40 vmid:10 pasid:32771)
amdgpu: in process pid 0 thread pid 0)
amdgpu: in page starting at address 0x0000800105405000 from client 10
amdgpu: GCVM_L2_PROTECTION_FAULT_STATUS:0x00A41051
amdgpu: Faulty UTCL2 client ID: TCP (0x8)
amdgpu: MORE_FAULTS: 0x1
amdgpu: WALKER_ERROR: 0x0
amdgpu: PERMISSION_FAULTS: 0x5
amdgpu: MAPPING_ERROR: 0x0
amdgpu: RW: 0x1
amdgpu: [gfxhub] page fault (src_id:0 ring:40 vmid:10 pasid:32771)
amdgpu: in process pid 0 thread pid 0)
amdgpu: in page starting at address 0x0000800105404000 from client 10
Signed-off-by: Sathishkumar S <sathishkumar.sundararaju@xxxxxxx>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
index f1d4e29772a5..4c3edd988a05 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
@@ -541,10 +541,23 @@ amdgpu_userqueue_validate_bos(struct amdgpu_userq_mgr *uq_mgr)
static void amdgpu_userqueue_resume_worker(struct work_struct *work)
{
struct amdgpu_userq_mgr *uq_mgr = work_to_uq_mgr(work, resume_work.work);
+ struct amdgpu_fpriv *fpriv = uq_mgr_to_fpriv(uq_mgr);
+ struct amdgpu_eviction_fence_mgr *evf_mgr = &fpriv->evf_mgr;
+ struct amdgpu_eviction_fence *ev_fence;
int ret;
mutex_lock(&uq_mgr->userq_mutex);
+ spin_lock(&evf_mgr->ev_fence_lock);
+ ev_fence = evf_mgr->ev_fence;
+ spin_unlock(&evf_mgr->ev_fence_lock);
+ if (ev_fence && dma_fence_is_signaled(&ev_fence->base)) {
+ /* Wait for the prior vm updates to complete before proceeding with resume */
+ dma_resv_wait_timeout(fpriv->vm.root.bo->tbo.base.resv,
+ DMA_RESV_USAGE_BOOKKEEP,
+ true,
+ msecs_to_jiffies(AMDGPU_FENCE_JIFFIES_TIMEOUT));
+ }
In general I agree that we need for PTE updates before resuming userqueues, but this here is just nonsense.
Okay, but the reason for adding this is, I was able to verify that corresponding vm sdma job fence is signaled few microseconds after the resume queue call in the timeline in tracing. I verified this by adding resume trace point after amdgpu_userqueue_validate_bos has completed (hoping this should do the waiting part), but observed that resume timestamp is before the sdma job fence signal timestamp.
Whereas after adding the dma_resv_wait on root.bo, the resume happens after sdma job fence is signalled, and was also able to see that page faults stopped after adding this check.
Good catch, but you seem to have a bunch of misunderstandings.
Okay Christian, Thank you for explaining the details below, will try to
understand further based on your inputs, that's a lot of valuable
information to ponder about and understand for me, thanks again.
See the queues are suspended because some BOs are moved to a location where they shouldn't be (e.g. VRAM into GTT or even SYSTEM and GTT into SYSTEM, etc...).
The function amdgpu_userqueue_validate_bos() moves the BOs back so that they are accessible by the GPU again, updates the PTEs and then waits for the PTE updates to finish.
It is perfectly possible that there is a bug in amdgpu_userqueue_validate_bos() and we don't wait for the PTE updates to finish. You should probably double check that.
Okay got it , will double check this code and will also try to
understand the below 2 points and think from that perspective.
Regards,
Sathish
Alternatively it is possible that PTE are made while the queues are disabled, but we don't need to wait for those to finish before re-starting the queues since they should be explicitly waited for by userspace.
In the kernel we only need to wait for all implicit PTE updates triggered by kernel BO moves to finish.
Regards,
Christian.
Regards, Sathish
ret = amdgpu_userqueue_validate_bos(uq_mgr);
This call here is validating the BOs, updating the PTEs *and* making sure that we wait for this update to finish.
Waiting before that just doesn't make any sense as far as I can see.
Regards,
Christian.
if (ret) {
DRM_ERROR("Failed to validate BOs to restore\n");
