Re: [PATCH 1/2] drm_gem: add mutex to drm_gem_object.gpuva

[Alice Ryhl <aliceryhl@google.com>]

On Thu, Aug 14, 2025 at 4:59 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Thu Aug 14, 2025 at 3:53 PM CEST, Alice Ryhl wrote:
> > There are two main ways that GPUVM might be used:
> >
> > * staged mode, where VM_BIND ioctls update the GPUVM immediately so that
> >   the GPUVM reflects the state of the VM *including* staged changes that
> >   are not yet applied to the GPU's virtual address space.
> > * immediate mode, where the GPUVM state is updated during run_job(),
> >   i.e., in the DMA fence signalling critical path, to ensure that the
> >   GPUVM and the GPU's virtual address space has the same state at all
> >   times.
> >
> > Currently, only Panthor uses GPUVM in immediate mode, but the Rust
> > drivers Tyr and Nova will also use GPUVM in immediate mode, so it is
> > worth to support both staged and immediate mode well in GPUVM. To use
> > immediate mode, the GEMs gpuva list must be modified during the fence
> > signalling path, which means that it must be protected by a lock that is
> > fence signalling safe.
> >
> > For this reason, a mutex is added to struct drm_gem_object that is
> > intended to achieve this purpose. Adding it directly in the GEM object
> > both makes it easier to use GPUVM in immediate mode, but also makes it
> > possible to take the gpuva lock from core drm code.
> >
> > As a follow-up, another change that should probably be made to support
> > immediate mode is a mechanism to postpone cleanup of vm_bo objects, as
> > dropping a vm_bo object in the fence signalling path is problematic for
> > two reasons:
> >
> > * When using DRM_GPUVM_RESV_PROTECTED, you cannot remove the vm_bo from
> >   the extobj/evicted lists during the fence signalling path.
> > * Dropping a vm_bo could lead to the GEM object getting destroyed.
> >   The requirement that GEM object cleanup is fence signalling safe is
> >   dubious and likely to be violated in practice.
> >
> > Panthor already has its own custom implementation of postponing vm_bo
> > cleanup.
> >
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > ---
> >  drivers/gpu/drm/drm_gem.c | 2 ++
> >  include/drm/drm_gem.h     | 4 +++-
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index 6a44351e58b7741c358406c8a576b6660b5ca904..24c109ab3fadd5af2e5d9de3fe330b105217a9ce 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -187,6 +187,7 @@ void drm_gem_private_object_init(struct drm_device *dev,
> >       kref_init(&obj->refcount);
> >       obj->handle_count = 0;
> >       obj->size = size;
> > +     mutex_init(&obj->gpuva.lock);
> >       dma_resv_init(&obj->_resv);
> >       if (!obj->resv)
> >               obj->resv = &obj->_resv;
> > @@ -1057,6 +1058,7 @@ drm_gem_object_free(struct kref *kref)
> >       if (WARN_ON(!obj->funcs->free))
> >               return;
> >
> > +     mutex_destroy(&obj->gpuva.lock);
> >       obj->funcs->free(obj);
>
> I really can't think of a valid case where we need to access this mutex from the
> GEM's free() callback, yet it probably doesn't hurt to mention it in the
> documentation of struct drm_gem_object_funcs.

I had wanted to move it below free(), but we can't do that since
free() also performs the kfree() call.

> >  }
> >  EXPORT_SYMBOL(drm_gem_object_free);
> > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> > index d3a7b43e2c637b164eba5af7cc2fc8ef09d4f0a4..5934d8dc267a65aaf62d2d025869221cd110b325 100644
> > --- a/include/drm/drm_gem.h
> > +++ b/include/drm/drm_gem.h
> > @@ -403,11 +403,13 @@ struct drm_gem_object {
> >        * Provides the list of GPU VAs attached to this GEM object.
> >        *
> >        * Drivers should lock list accesses with the GEMs &dma_resv lock
> > -      * (&drm_gem_object.resv) or a custom lock if one is provided.
> > +      * (&drm_gem_object.resv) or a custom lock if one is provided. The
> > +      * mutex inside this struct may be used as the custom lock.
> >        */
> >       struct {
> >               struct list_head list;
> >
> > +             struct mutex lock;
> >  #ifdef CONFIG_LOCKDEP
> >               struct lockdep_map *lock_dep_map;
> >  #endif
>
> We should remove this and the corresponding functions (i.e.
> drm_gem_gpuva_set_lock(), drm_gem_gpuva_assert_lock_held()) in a subsequent
> patch and let GPUVM assert for this mutex directly rather than for the
> lockdep_map.

Agreed.

Alice