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
