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 13:28 +0200, Danilo Krummrich wrote:
> On 9/9/25 1:24 PM, Alice Ryhl wrote:
> > On Tue, Sep 9, 2025 at 1:11 PM Thomas Hellström
> > <thomas.hellstrom@linux.intel.com> wrote:
> > > 
> > > 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:
> > > > > > > > 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.
> > 
> > I don't think evict list empty means that there are no evicted
> > GEMs.
> > It's possible for an extobj to be missing from the evict list in
> > some
> > scenarios. That's why drm_gpuvm_prepare_objects_locked() checks
> > evicted on the extobj list to ensure that the evicted list is
> > up-to-date when you call into drm_gpuvm_validate_locked().
> 
> Indeed, though I would expect that Xe considers that? It was Thomas
> who proposed
> the logic you describe here back then IIRC. :)
> 

Yeah I don't think that eviction-while-validating could happen to
extobjs, but rather to local objects, if it happens at all anymore,
we've made a lot of changes in that area.

But moving forward both the extobj scenario and local object scenarios
will probably have to be considered in OOM situations, but then we'd of
course need to suggest suitable additions to drm_gpuvm to handle that.

/Thomas