[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.
> > #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
