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

[Christian König]

Am 20.06.24 um 16:30 schrieb Li, Yunxiang (Teddy):
> [Public]
>
>>> +   dma_resv_lock(bo->tbo.base.resv, NULL);
>> Why do you grab the BO lock to update the stats? That doesn't seem to make
>> any sense.
>>
>>> +   update_stats = !(bo->flags & AMDGPU_GEM_WAS_EXPORTED);
>>> +   if (update_stats)
>>> +           amdgpu_bo_add_memory(bo, &stats);
>>> +   else
>>> +           dma_resv_unlock(bo->tbo.base.resv);
>>> +
>>>      buf = drm_gem_prime_export(gobj, flags);
>>> -   if (!IS_ERR(buf))
>>> +   if (!IS_ERR(buf)) {
>>>              buf->ops = &amdgpu_dmabuf_ops;
>>> +           if (update_stats) {
>>> +                   for (base = bo->vm_bo; base; base = base->next) {
>>> +                           spin_lock(&base->vm->status_lock);
>>> +                           base->vm->stats.vram_shared += stats.vram;
>>> +                           base->vm->stats.gtt_shared += stats.gtt;
>>> +                           base->vm->stats.cpu_shared += stats.cpu;
>>> +                           spin_unlock(&base->vm->status_lock);
>>> +                   }
>>> +                   bo->flags |= AMDGPU_GEM_WAS_EXPORTED;
>>> +                   dma_resv_unlock(bo->tbo.base.resv);
> The thought here is that I don't want two exports of the same buffer to race here and increase the stats twice. But if BO can only be exported once then yes this is not needed.
>
>> Don't touch any VM internals from the BO code.
>> Don't touch any VM internals in TTM code.
> What would be the preferred approach? I can put a small helper in amdgpu_vm or amdgpu_bo I suppose.

In general the VM should already have most of the necessary functionality.

amdgpu_vm_bo_add() is called when a BO is initially added to the VM.
amdgpu_vm_bo_del() is called when a BO is removed from the VM.

amdgpu_vm_bo_invalidate() is called when a BO moves. This needs to be 
improved to take old_mem/new_mem as well.

Then you only need another function which signals that the BO is 
exported and you should be done.

Regards,
Christian.


>
>>>    #define AMDGPU_GEM_CREATE_GFX12_DCC               (1 << 16)
>>>
>>> +/* Flag that BO was exported at one point and counts torwards the
>> "shared"
>>> + * memory stats. Once set it does not get cleared until the BO is destroyed.
>>> + */
>>> +#define AMDGPU_GEM_WAS_EXPORTED            (1 << 17)
>>> +
>> Absolutely clear NAK to that approach. This is not even remotely an allocation
>> flag but some status.
>>
>> Additional to that completely unnecessary since BOs are usually only exported
>> once.
> If BOs can only be exported once then we don't need this kind of marker, but I think user space is free to export as many times as they wish right? I first tried to handle the unshare case as well, but I don't see any where in that path we can easily hook into. I can give it another try.
>
> Regards,
> Teddy