On Mon, Sep 08, 2025 at 09:11:40AM +0200, Boris Brezillon wrote: > Hi Alice, > > On Sun, 7 Sep 2025 11:39:41 +0000 > Alice Ryhl <alicer...@google.com> wrote: > > > Yeah I guess we could have unlink remove the gpuva, but then allow the > > end-user to attach the gpuva to a list of gpuvas to kfree deferred. That > > way, the drm_gpuva_unlink() is not deferred but any resources it has can > > be. > > This ^. > > > > > Of course, this approach also makes deferred gpuva cleanup somewhat > > orthogonal to this patch. > > Well, yes and no, because if you go for gpuva deferred cleanup, you > don't really need the fancy kref_put() you have in this patch, it's > just a regular vm_bo_put() that's called in the deferred gpuva path on > the vm_bo attached to the gpuva being released.
Ok, so what you suggest is that on gpuva_unlink() we remove the gpuva from the vm_bo's list, but then instead of putting the vm_bo's refcount, we add the gpuva to a list, and in the deferred cleanup codepath we iterate gpuvas and drop vm_bo refcounts *at that point*. Is that understood correctly? That means we don't immediately remove the vm_bo from the gem.gpuva list, but the gpuva list in the vm_bo will be empty. I guess you already have to handle such vm_bos anyway since you can already have an empty vm_bo in between vm_bo_obtain() and the first call to gpuva_link(). One disadvantage is that we might end up preparing or unevicting a GEM object that doesn't have any VAs left, which the current approach avoids. > > One annoying part is that we don't have an gpuvm ops operation for > > freeing gpuva, and if we add one for this, it would *only* be used in > > this case as most drivers explicitly kfree gpuvas, which could be > > confusing for end-users. > > Also not sure ::vm_bo_free() was meant to be used like that. It was for > drivers that need to control the drm_gpuvm_bo allocation, not those > that rely on the default implementation (kmalloc). Given how things > are described in the the doc, it feels weird to have a ::vm_bo_free() > without ::vm_bo_alloc(). So, if we decide to go this way (which I'm > still not convinced we should, given ultimately we might want to defer > gpuvas cleanup), the ::vm_bo_free() doc should be extended to cover > this 'deferred vm_bo free' case. I can implement vm_bo_alloc() too, but I think it seems like a pretty natural way to use vm_bo_free(). Alice
