Re: [PATCH v5 4/4] drm/amdgpu: track bo memory stats at runtime

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

Am 22.10.24 um 17:17 schrieb Li, Yunxiang (Teddy):
> [Public]
>
>>> +static uint32_t fold_memtype(uint32_t memtype) {
>> In general please add prefixes to even static functions, e.g. amdgpu_vm_ or
>> amdgpu_bo_.
>>
>>> +   /* Squash private placements into 'cpu' to keep the legacy userspace view.
>> */
>>> +   switch (mem_type) {
>>> +   case TTM_PL_VRAM:
>>> +   case TTM_PL_TT:
>>> +           return memtype
>>> +   default:
>>> +           return TTM_PL_SYSTEM;
>>> +   }
>>> +}
>>> +
>>> +static uint32_t bo_get_memtype(struct amdgpu_bo *bo) {
>> That whole function belongs into amdgpu_bo.c
> Do you mean bo_get_memtype or fold_memtype? I debated whether bo_get_memtype should go into amdgpu_vm.c or amdgpu_bo.c, and since it's using fold_memtype and only useful for memory stats because of folding the private placements I just left them here together with the other mem stats code.
>
> I can move it to amdgpu_bo.c make it return the memtype verbatim and just fold it when I do the accounting.

I think that folding GDS, GWS and OA into system is also a bug. We 
should really not doing that.

Just wanted to point out for this round that the code to query the 
current placement from a BO should probably go into amdgpu_bo.c and not 
amdgpu_vm.c

>
>>> +   struct ttm_resource *res = bo->tbo.resource;
>>> +   const uint32_t domain_to_pl[] = {
>>> +           [ilog2(AMDGPU_GEM_DOMAIN_CPU)]      = TTM_PL_SYSTEM,
>>> +           [ilog2(AMDGPU_GEM_DOMAIN_GTT)]      = TTM_PL_TT,
>>> +           [ilog2(AMDGPU_GEM_DOMAIN_VRAM)]     = TTM_PL_VRAM,
>>> +           [ilog2(AMDGPU_GEM_DOMAIN_GDS)]      = AMDGPU_PL_GDS,
>>> +           [ilog2(AMDGPU_GEM_DOMAIN_GWS)]      = AMDGPU_PL_GWS,
>>> +           [ilog2(AMDGPU_GEM_DOMAIN_OA)]       = AMDGPU_PL_OA,
>>> +           [ilog2(AMDGPU_GEM_DOMAIN_DOORBELL)] =
>> AMDGPU_PL_DOORBELL,
>>> +   };
>>> +   uint32_t domain;
>>> +
>>> +   if (res)
>>> +           return fold_memtype(res->mem_type);
>>> +
>>> +   /*
>>> +    * If no backing store use one of the preferred domain for basic
>>> +    * stats. We take the MSB since that should give a reasonable
>>> +    * view.
>>> +    */
>>> +   BUILD_BUG_ON(TTM_PL_VRAM < TTM_PL_TT || TTM_PL_VRAM <
>> TTM_PL_SYSTEM);
>>> +   domain = fls(bo->preferred_domains & AMDGPU_GEM_DOMAIN_MASK);
>>> +   if (drm_WARN_ON_ONCE(&adev->ddev,
>>> +                        domain == 0 || --domain >= ARRAY_SIZE(domain_to_pl)))
>> It's perfectly legal to create a BO without a placement. That one just won't have a
>> backing store.
>>
> This is lifted from the previous change I'm rebasing onto. I think what it’s trying to do is if the BO doesn't have a placement, use the "biggest" (VRAM > TT > SYSTEM) preferred placement for the purpose of accounting. Previously we just ignore BOs that doesn't have a placement. I guess there's argument for going with either approaches.

I was not arguing, I'm simply pointing out a bug. It's perfectly valid 
for bo->preferred_domains to be 0.

So the following WARN_ON() that no bit is set is incorrect.

>
>>> +           return 0;
>>> +   return fold_memtype(domain_to_pl[domain])
>> That would need specular execution mitigation if I'm not completely mistaken.
>>
>> Better use a switch/case statement.
>>
> Do you mean change the array indexing to a switch statement?

Yes.

Regards,
Christian.

>
>
> Regards,
> Teddy