Re: [PATCH 1/4] drm/amdgpu: add the evf attached gem obj resv dump

["Christian König" <christian.koenig@amd.com>]

Am 16.04.25 um 14:54 schrieb Liang, Prike:
> [Public]
>
>> From: Koenig, Christian <Christian.Koenig@amd.com>
>> Sent: Wednesday, April 16, 2025 7:01 PM
>> To: Liang, Prike <Prike.Liang@amd.com>; amd-gfx@lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
>> Subject: Re: [PATCH 1/4] drm/amdgpu: add the evf attached gem obj resv dump
>>
>> Am 16.04.25 um 10:50 schrieb Prike Liang:
>>> This debug dump will help on debugging the evf attached gem obj fence
>>> related issue.
>> That looks like overkill to me and will just massively spam the debug log.
>>
>> Christian.
>>
> How about putting the evf attached resv obj dump in a trace point?

Well that's better, but I still don't see the value in it for the eviction fence handling.

See the dma_resv object is just a container for fences. it's completely irrelevant for the eviction fence what other fences are in the resv object.

On the other hand adding the dma_resv_describe() to amdgpu_bo_print_info() is probably quite nice to have and perfectly valid.

Thanks,
Christian.

>
> Thanks,
> Prike
>
>>> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c | 13 +++++++++++++
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c         |  6 +++++-
>>>  2 files changed, 18 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
>>> index 0075469550b0..7030d721196b 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
>>> @@ -86,6 +86,19 @@ amdgpu_eviction_fence_replace_fence(struct
>> amdgpu_eviction_fence_mgr *evf_mgr,
>>>             if (ret) {
>>>                     DRM_ERROR("Failed to attch new eviction fence\n");
>>>                     goto free_err;
>>> +           } else {
>>> +                   struct dma_resv_iter cursor;
>>> +                   struct dma_fence *fence;
>>> +                   static const char *usage[] = { "kernel", "write", "read",
>>> +"bookkeep" };
>>> +
>>> +                   dma_resv_for_each_fence(&cursor, obj->resv,
>> DMA_RESV_USAGE_READ, fence) {
>>> +                           DRM_DEBUG("after attach evf the resv dump
>> usage:%s\n"
>>> +                                      "after attach evf name:%s timeline
>> name:%s seq:%lld %ssingned\n",
>>> +                                   usage[dma_resv_iter_usage(&cursor)],
>>> +                                   fence->ops->get_driver_name(fence),
>>> +                                   fence->ops->get_timeline_name(fence),
>> fence->seqno,
>>> +                                   dma_fence_is_signaled(fence) ? "" : "un");
>>> +                   }
>>>             }
>>>     }
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> index d09db052e282..1e73ce30d4d7 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> @@ -1675,7 +1675,11 @@ u64 amdgpu_bo_print_info(int id, struct amdgpu_bo
>> *bo, struct seq_file *m)
>>>     amdgpu_bo_print_flag(m, bo, VRAM_CONTIGUOUS);
>>>     amdgpu_bo_print_flag(m, bo, VM_ALWAYS_VALID);
>>>     amdgpu_bo_print_flag(m, bo, EXPLICIT_SYNC);
>>> -
>>> +   /* Add the gem obj resv fence dump*/
>>> +   if (dma_resv_trylock(bo->tbo.base.resv)) {
>>> +           dma_resv_describe(bo->tbo.base.resv, m);
>>> +           dma_resv_unlock(bo->tbo.base.resv);
>>> +   }
>>>     seq_puts(m, "\n");
>>>
>>>     return size;