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

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

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