[PATCH 2/3] drm/amdgpu: Reduce mem_type to domain double indirection

[Tvrtko Ursulin]

Hi Christian,

On 29/04/2024 12:03, Christian König wrote:
> Am 26.04.24 um 18:43 schrieb Tvrtko Ursulin:
>> From: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>
>>
>> All apart from AMDGPU_GEM_DOMAIN_GTT memory domains map 1:1 to TTM
>> placements. And the former be either AMDGPU_PL_PREEMPT or TTM_PL_TT,
>> depending on AMDGPU_GEM_CREATE_PREEMPTIBLE.
>>
>> Simplify a few places in the code which convert the TTM placement into
>> a domain by checking against the current placement directly.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>
> 
> Reviewed-by: Christian König <christian.koenig at amd.com>

Thanks! I am however a bit unsure now, read below..

>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c |  4 +--
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  | 30 ++++++++++-----------
>>   2 files changed, 16 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>> index 055ba2ea4c12..ff83f8d8628c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>> @@ -165,8 +165,8 @@ static struct sg_table *amdgpu_dma_buf_map(struct 
>> dma_buf_attachment *attach,
>>           if (r)
>>               return ERR_PTR(r);
>> -    } else if (!(amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type) &
>> -             AMDGPU_GEM_DOMAIN_GTT)) {
>> +    } else if (bo->tbo.resource->mem_type != TTM_PL_TT &&
>> +           bo->tbo.resource->mem_type != AMDGPU_PL_PREEMPT) {
>>           return ERR_PTR(-EBUSY);

... this for instance (one hunk below too).

Because what we are discussing in 3/3, 
amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type) does not return a 
valid domain for AMDGPU_PL_PREEMPT. Which could make mentioning it here 
misleading. So I am not really sure what the design is supposed to be.

Is this a weakness in the current code? Maybe best to leave the 
discussion for 3/3.

Regards,

Tvrtko

>>       }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> index 8bc79924d171..fb984669fc3a 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> @@ -976,12 +976,12 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo 
>> *bo, u32 domain,
>>       ttm_bo_pin(&bo->tbo);
>> -    domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
>> -    if (domain == AMDGPU_GEM_DOMAIN_VRAM) {
>> +    if (bo->tbo.resource->mem_type == TTM_PL_VRAM) {
>>           atomic64_add(amdgpu_bo_size(bo), &adev->vram_pin_size);
>>           atomic64_add(amdgpu_vram_mgr_bo_visible_size(bo),
>>                    &adev->visible_pin_size);
>> -    } 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);
>>       }
>> @@ -1280,7 +1280,6 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
>>   {
>>       uint64_t size = amdgpu_bo_size(bo);
>>       struct drm_gem_object *obj;
>> -    unsigned int domain;
>>       bool shared;
>>       /* Abort if the BO doesn't currently have a backing store */
>> @@ -1290,21 +1289,21 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
>>       obj = &bo->tbo.base;
>>       shared = drm_gem_object_is_shared_for_memory_stats(obj);
>> -    domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
>> -    switch (domain) {
>> -    case AMDGPU_GEM_DOMAIN_VRAM:
>> +    switch (bo->tbo.resource->mem_type) {
>> +    case TTM_PL_VRAM:
>>           stats->vram += size;
>>           if (amdgpu_bo_in_cpu_visible_vram(bo))
>>               stats->visible_vram += size;
>>           if (shared)
>>               stats->vram_shared += size;
>>           break;
>> -    case AMDGPU_GEM_DOMAIN_GTT:
>> +    case TTM_PL_TT:
>> +    case AMDGPU_PL_PREEMPT:
>>           stats->gtt += size;
>>           if (shared)
>>               stats->gtt_shared += size;
>>           break;
>> -    case AMDGPU_GEM_DOMAIN_CPU:
>> +    case TTM_PL_SYSTEM:
>>       default:
>>           stats->cpu += size;
>>           if (shared)
>> @@ -1317,7 +1316,7 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
>>           if (bo->flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED)
>>               stats->requested_visible_vram += size;
>> -        if (domain != AMDGPU_GEM_DOMAIN_VRAM) {
>> +        if (bo->tbo.resource->mem_type != TTM_PL_VRAM) {
>>               stats->evicted_vram += size;
>>               if (bo->flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED)
>>                   stats->evicted_visible_vram += size;
>> @@ -1592,19 +1591,18 @@ u64 amdgpu_bo_print_info(int id, struct 
>> amdgpu_bo *bo, struct seq_file *m)
>>       u64 size;
>>       if (dma_resv_trylock(bo->tbo.base.resv)) {
>> -        unsigned int domain;
>> -        domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
>> -        switch (domain) {
>> -        case AMDGPU_GEM_DOMAIN_VRAM:
>> +        switch (bo->tbo.resource->mem_type) {
>> +        case TTM_PL_VRAM:
>>               if (amdgpu_bo_in_cpu_visible_vram(bo))
>>                   placement = "VRAM VISIBLE";
>>               else
>>                   placement = "VRAM";
>>               break;
>> -        case AMDGPU_GEM_DOMAIN_GTT:
>> +        case TTM_PL_TT:
>> +        case AMDGPU_PL_PREEMPT:
>>               placement = "GTT";
>>               break;
>> -        case AMDGPU_GEM_DOMAIN_CPU:
>> +        case TTM_PL_SYSTEM:
>>           default:
>>               placement = "CPU";
>>               break;
>