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

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

Am 26.04.24 um 18:43 schrieb Tvrtko Ursulin:
> 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.
>
> 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.
>
> 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) {

Good catch, but please separate that one from the other changes since we 
probably want to backport it.

>   		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:

Yeah, that makes sense as well. But we need a case for AMDGPU_PL_PREEMPT 
here as well then.

Regards,
Christian.

>   			placement = "GTT";
>   			break;
>   		case TTM_PL_SYSTEM: