[PATCH 3/3] drm/amdgpu: Fix pinned GART area accounting and fdinfo reporting

[Tvrtko Ursulin]

On 29/04/2024 15:43, Felix Kuehling wrote:
> On 2024-04-29 5:43, Tvrtko Ursulin wrote:
>>
>> On 26/04/2024 23:24, Felix Kuehling wrote:
>>>
>>> On 2024-04-26 12:43, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>
>>>>
>>>> When commit b453e42a6e8b ("drm/amdgpu: Add new placement for 
>>>> preemptible
>>>> SG BOs") added a new TTM region it missed to notice the conceptual
>>>> imbalance in GART pin size accounting as done in amdgpu_bo_pin/unpin.
>>>>
>>>> That imbalance leads to such objects getting accounted against the
>>>> resource, but are not un-accounted when unpinned.
>>>
>>> AMDGPU_PL_PREEMPT is mostly used for userptr BOs, which cannot be 
>>> pinned. In any case you should make sure that the accounting is 
>>> consistent between amdgpu_bo_pin_restricted and amdgpu_bo_unpin. This 
>>> patch breaks that consistency.
>>
>> You mean amdgpu_bo_pin(_restricted) and amdgpu_bo_unpin do not run for 
>> such objects, or something else?
> 
> Right. amdgpu_bo_pin_restricted will return an error for userptr BOs:
> 
>          if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm))
>                  return -EPERM;

Ah I missed that, thank you!

>>
>> If they run, then at the end of pin there is:
>>
>>      domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
>> ...
>>      } else if (domain == AMDGPU_GEM_DOMAIN_GTT) {
>>          atomic64_add(amdgpu_bo_size(bo), &adev->gart_pin_size);
> 
> You changed that in your patch 2:
> 
> -    } else if (domain == AMDGPU_GEM_DOMAIN_GTT) {
> +    } else if (bo->tbo.resource->mem_type == TTM_PL_TT ||
> +           bo->tbo.resource->mem_type == AMDGPU_PL_PREEMPT) {
>           atomic64_add(amdgpu_bo_size(bo), &adev->gart_pin_size);
>       }
> 
> I was suggesting you just change this in patch 2 like this, so it 
> matches what's done on unpin:
> 
> -    } else if (domain == AMDGPU_GEM_DOMAIN_GTT) {
> +    } else if (bo->tbo.resource->mem_type == TTM_PL_TT) {
>           atomic64_add(amdgpu_bo_size(bo), &adev->gart_pin_size);
>       }
> 
> 
>>
>> And unpin has no handling for AMDGPU_PL_PREEMPT.
>>
>> Ah I see.. does it rely on amdgpu_mem_type_to_domain returning 0 for 
>> AMDGPU_PL_PREEMPT? My confusion was I misread the pinning check as 
>> checking the domain as stored in the bo at creation time.
>>
>> Although I am still confused by the statement userptr BOs are not 
>> pinned. It is not needed to map them via GART on AMD hardware for GPU 
>> to be able to access them?
>>>> Fix by extending the accounting criteria in amdgpu_bo_unpin.
>>>>
>>>> What also aappears needs fixing is not reporting their size from the
>>>> amdgpu_bo_get_memory, which is used to implement fdinfo stats, so 
>>>> they are
>>>> not mixed with the regular userspace created and driver owned objects.
>>>
>>> I think that's true. It's a very fine distinction. AMDGPU_PL_PREEMPT 
>>> does use system memory and it is GPU accessible, just like GTT. The 
>>> only difference is, that it's not subject to the GTT limits because 
>>> their eviction is handled by callbacks other than TTM evictions and 
>>> doesn't need to wait for fences.
>>
>> As in you think those two hunks of the patch are correct?
> 
> Yes. It seems, Christian agrees but wants to show preemptible memory 
> separately in debugfs instead of not showing it at all.

Okay, I've posted a respin with a name 'PREEMPTIBLE' to start the naming 
discussion. :)

If I did not get confused again, it is only the last patch in the series 
moves the reporting for those objects from 'CPU' to this new label.

This is because both the current code:

                domain = 
amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
                switch (domain) {
...
                default:
                         placement = "CPU";
                         break;

And with my 2/3:

                switch (bo->tbo.resource->mem_type) {
...
                case TTM_PL_SYSTEM:
                default:
                         placement = "CPU";
                         break;

They end up in the CPU bucket. Things only change with 3/3:

Regards,

Tvrtko

> 
> Regards,
>    Felix
> 
> 
>>
>> Regards,
>>
>> Tvrtko
>>
>>
>>> Regards,
>>>    Felix
>>>
>>>
>>>>
>>>> And also amdgpu_bo_print_info for debugfs reporting.
>>>>
>>>> Note that the patch depends on the previous one which broke down the
>>>> relevant checks from the domain based to placement based.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>
>>>> Fixes: b453e42a6e8b ("drm/amdgpu: Add new placement for preemptible 
>>>> SG BOs")
>>>> Cc: Felix Kuehling <Felix.Kuehling at amd.com>
>>>> Cc: Christian König <christian.koenig at amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 5 ++---
>>>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>> index fb984669fc3a..5a2bbc793953 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>> @@ -1032,7 +1032,8 @@ void amdgpu_bo_unpin(struct amdgpu_bo *bo)
>>>>           atomic64_sub(amdgpu_bo_size(bo), &adev->vram_pin_size);
>>>>           atomic64_sub(amdgpu_vram_mgr_bo_visible_size(bo),
>>>>                    &adev->visible_pin_size);
>>>> -    } else if (bo->tbo.resource->mem_type == TTM_PL_TT) {
>>>> +    } else if (bo->tbo.resource->mem_type == TTM_PL_TT ||
>>>> +           bo->tbo.resource->mem_type == AMDGPU_PL_PREEMPT) {
>>>>           atomic64_sub(amdgpu_bo_size(bo), &adev->gart_pin_size);
>>>>       }
>>>> @@ -1298,7 +1299,6 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
>>>>               stats->vram_shared += size;
>>>>           break;
>>>>       case TTM_PL_TT:
>>>> -    case AMDGPU_PL_PREEMPT:
>>>>           stats->gtt += size;
>>>>           if (shared)
>>>>               stats->gtt_shared += size;
>>>> @@ -1599,7 +1599,6 @@ u64 amdgpu_bo_print_info(int id, struct 
>>>> amdgpu_bo *bo, struct seq_file *m)
>>>>                   placement = "VRAM";
>>>>               break;
>>>>           case TTM_PL_TT:
>>>> -        case AMDGPU_PL_PREEMPT:
>>>>               placement = "GTT";
>>>>               break;
>>>>           case TTM_PL_SYSTEM: