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

[Christian König]

Am 23.10.24 um 13:37 schrieb Tvrtko Ursulin:
>
> On 23/10/2024 10:14, Christian König wrote:
>> Am 23.10.24 um 09:38 schrieb Tvrtko Ursulin:
>>>
>>> On 22/10/2024 17:24, Christian König wrote:
>>>> 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.
>>>
>>> Did you mean array_index_nospec?
>>
>> Yes.
>>
>>> Domain is not a direct userspace input and is calculated from the 
>>> mask which sanitized to allowed values prior to this call. So I 
>>> *think* switch is an overkill but don't mind it either. Just 
>>> commenting FWIW.
>>
>> I missed that the mask is applied.
>>
>> Thinking more about it I'm not sure if we should do this conversion 
>> in the first place. IIRC Tvrtko you once suggested a patch which 
>> switched a bunch of code to use the TTM placement instead of the UAPI 
>> flags.
>
> Maybe 8fb0efb10184 ("drm/amdgpu: Reduce mem_type to domain double 
> indirection") is what are you thinking of?

Yes, exactly that one.

>
>> Going more into this direction I think when we want to look at the 
>> current placement we should probably also use the TTM PL enumeration 
>> directly.
>
> It does this already. The placement flags are just to "invent" a TTM 
> PL enum when bo->tbo.resource == NULL.

Ah, good point! I though we would do the mapping the other way around.

In this case that is even more something we should probably not do at all.

When bo->tbo.resource is NULL then this BO isn't resident at all, so it 
should not account to resident memory.

> Again, based of the same enum. Not sure if you have something other in 
> mind or you are happy with that?

I think that for drm-total-* we should use the GEM flags and for 
drm-resident-* we should use the TTM placement.

>
> Then what Teddy does is IMO only tangential, he just changes when 
> stats are collected and not this aspect.

Yeah, right but we should probably fix it up in the right way while on it.

>
> To fold or not the special placements (GWS, GDS & co) is also 
> tangential. In my patch I just preserved the legacy behaviour so it 
> can easily be tweaked on top.

Yeah, but again the original behavior is completely broken.

GWS, GDS and OA are counted in blocks of HW units (multiplied by 
PAGE_SIZE IIRC to avoid some GEM&TTM warnings).

When you accumulate that anywhere in the memory stats then that is just 
completely off.

Regards,
Christian.

>
> Regards,
>
> Tvrtko
>
>>
>> Regards,
>> Christian.
>>
>>>
>>> Regards,
>>>
>>> Tvrtko
>>