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

[Tvrtko Ursulin <tvrtko.ursulin@igalia.com>]

On 26/04/2024 23:24, Felix Kuehling wrote:
> 
> On 2024-04-26 12:43, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@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?

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);

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?

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@igalia.com>
>> Fixes: b453e42a6e8b ("drm/amdgpu: Add new placement for preemptible SG 
>> BOs")
>> Cc: Felix Kuehling <Felix.Kuehling@amd.com>
>> Cc: Christian König <christian.koenig@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: