Re: [PATCH 1/2] drm/gpuvm: add deferred vm_bo cleanup

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On Tue, 2025-09-09 at 12:47 +0200, Danilo Krummrich wrote:
> On 9/9/25 12:39 PM, Thomas Hellström wrote:
> > On 9/8/25 14:20, Danilo Krummrich wrote:
> > > On 9/8/25 2:11 PM, Boris Brezillon wrote:
> > > > On Mon, 08 Sep 2025 13:11:32 +0200
> > > > "Danilo Krummrich" <dakr@kernel.org> wrote:
> > > > > I'm saying exactly what you say: "has to be a special unlink
> > > > > function" ->
> > > > > drm_gpuva_unlink_defer_put(). :)
> > > > I don't see how calling drm_gpuva_unlink() instead of
> > > > drm_gpuva_unlink_defer_put() would leak the vm_bo though.
> > > Initially (i.e. a few mails back), it sounded to me as if you'd
> > > propose to drop
> > > the drm_gpuva's vm_bo reference only when it is freed.
> > > 
> > > > > No, drivers can't iterate the evict/extobj lists directly; or
> > > > > at least this is
> > > > > not intended by GPUVM's API and if drivers do so, this is
> > > > > considered peeking
> > > > > into GPUVM internals, so drivers are on their own anyways.
> > > > > 
> > > > > Iterators, such as for_each_vm_bo_in_list() are not exposed
> > > > > to drivers.
> > > > Okay, that's a good thing. I thought Xe was doing some funky
> > > > stuff with
> > > > the list...
> > > Maybe, I don't know. If they do so, the should send patches
> > > adding the
> > > corresponding iterators and provide a rationale why drivers need
> > > to access those
> > > lists directly and why we can't provide an API that handles the
> > > overall
> > > use-case, such as drm_gpuvm_prepare_objects(), etc.
> > 
> > We're using the drm_gpuvm_*for_each* macros in drm_gpuvm.h,
> > assuming from name
> > and docs they are driver api.
> > 
> > Also the drm_gem_for_each_gpuvm_bo(), although this usage could
> > easily be
> > converted to a helper.
> 
> We were talking about the extobj/evict lists, the ones you mention
> are fine of
> course. :)
> 

Hmm. Now on closer inspection it looks like we're checking for evict
list empty, It looks like rebinding after validation may in theory
evict some bos to system memory and then we'd rerun the validation step
if the evict list was not empty.

We could of course add a helper for that or if there are better
suggestions to handle that situation, that'd be fine as well.

Thanks,
Thomas