Re: [PATCH drm-misc-next 4/5] drm/gpuvm: fall back to drm_exec_lock_obj()

[Christian König]

Am 25.11.23 um 00:40 schrieb Danilo Krummrich:
> Hi Christian,
>
> do you think it makes sense to handle this in drm_exec_prepare_obj() or
> even dma_resv_reserve_fences() even?

IIRC for drm_exec the discussion has gone a bit back and forth between 
handling 0 and having a separate function which doesn't allocate fences.

We ended up with the solution using separate calls since you either know 
that you don't need fences (because you for example only need to look 
something up) or you need fences and eventually calculate the number you 
need dynamically and if you then end up with 0 it's a bug.

So to sum it up the conclusion was that it's more defensive to complain 
about 0 fences to reserve (which reminds me that 
dma_resv_reserve_fences() still needs to get a warning for 0 fences as 
well).

Regards,
Christian.

> - Danilo
>
> On 11/25/23 00:36, Danilo Krummrich wrote:
> > Fall back to drm_exec_lock_obj() if num_fences is zero for the
> > drm_gpuvm_prepare_* function family.
> >
> > Otherwise dma_resv_reserve_fences() would actually allocate slots even
> > though num_fences is zero.
> >
> > Cc: Christian König <christian.koenig@xxxxxxx>
> > Signed-off-by: Danilo Krummrich <dakr@xxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/drm_gpuvm.c | 36 +++++++++++++++++++++++++++++++++---
> >   include/drm/drm_gpuvm.h     | 23 +++--------------------
> >   2 files changed, 36 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
> > index 54f5e8851de5..d1d1c2379e44 100644
> > --- a/drivers/gpu/drm/drm_gpuvm.c
> > +++ b/drivers/gpu/drm/drm_gpuvm.c
> > @@ -1085,6 +1085,36 @@ drm_gpuvm_put(struct drm_gpuvm *gpuvm)
> >   }
> >   EXPORT_SYMBOL_GPL(drm_gpuvm_put);
> >   +static int
> > +exec_prepare_obj(struct drm_exec *exec, struct drm_gem_object *obj,
> > +         unsigned int num_fences)
> > +{
> > +    return num_fences ? drm_exec_prepare_obj(exec, obj, num_fences) :
> > +                drm_exec_lock_obj(exec, obj);
> > +}
> > +
> > +/**
> > + * drm_gpuvm_prepare_vm() - prepare the GPUVMs common dma-resv
> > + * @gpuvm: the &drm_gpuvm
> > + * @exec: the &drm_exec context
> > + * @num_fences: the amount of &dma_fences to reserve
> > + *
> > + * Calls drm_exec_prepare_obj() for the GPUVMs dummy &drm_gem_object.
> > + *
> > + * Using this function directly, it is the drivers responsibility to 
> > call
> > + * drm_exec_init() and drm_exec_fini() accordingly.
> > + *
> > + * Returns: 0 on success, negative error code on failure.
> > + */
> > +int
> > +drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm,
> > +             struct drm_exec *exec,
> > +             unsigned int num_fences)
> > +{
> > +    return exec_prepare_obj(exec, gpuvm->r_obj, num_fences);
> > +}
> > +EXPORT_SYMBOL_GPL(drm_gpuvm_prepare_vm);
> > +
> >   static int
> >   __drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm,
> >                   struct drm_exec *exec,
> > @@ -1095,7 +1125,7 @@ __drm_gpuvm_prepare_objects(struct drm_gpuvm 
> > *gpuvm,
> >       int ret = 0;
> >         for_each_vm_bo_in_list(gpuvm, extobj, &extobjs, vm_bo) {
> > -        ret = drm_exec_prepare_obj(exec, vm_bo->obj, num_fences);
> > +        ret = exec_prepare_obj(exec, vm_bo->obj, num_fences);
> >           if (ret)
> >               break;
> >       }
> > @@ -1116,7 +1146,7 @@ drm_gpuvm_prepare_objects_locked(struct 
> > drm_gpuvm *gpuvm,
> >         drm_gpuvm_resv_assert_held(gpuvm);
> >       list_for_each_entry(vm_bo, &gpuvm->extobj.list, 
> > list.entry.extobj) {
> > -        ret = drm_exec_prepare_obj(exec, vm_bo->obj, num_fences);
> > +        ret = exec_prepare_obj(exec, vm_bo->obj, num_fences);
> >           if (ret)
> >               break;
> >   @@ -1186,7 +1216,7 @@ drm_gpuvm_prepare_range(struct drm_gpuvm 
> > *gpuvm, struct drm_exec *exec,
> >       drm_gpuvm_for_each_va_range(va, gpuvm, addr, end) {
> >           struct drm_gem_object *obj = va->gem.obj;
> >   -        ret = drm_exec_prepare_obj(exec, obj, num_fences);
> > +        ret = exec_prepare_obj(exec, obj, num_fences);
> >           if (ret)
> >               return ret;
> >       }
> > diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h
> > index f94fec9a8517..b3f82ec7fb17 100644
> > --- a/include/drm/drm_gpuvm.h
> > +++ b/include/drm/drm_gpuvm.h
> > @@ -544,26 +544,9 @@ struct drm_gpuvm_exec {
> >       } extra;
> >   };
> >   -/**
> > - * drm_gpuvm_prepare_vm() - prepare the GPUVMs common dma-resv
> > - * @gpuvm: the &drm_gpuvm
> > - * @exec: the &drm_exec context
> > - * @num_fences: the amount of &dma_fences to reserve
> > - *
> > - * Calls drm_exec_prepare_obj() for the GPUVMs dummy &drm_gem_object.
> > - *
> > - * Using this function directly, it is the drivers responsibility to 
> > call
> > - * drm_exec_init() and drm_exec_fini() accordingly.
> > - *
> > - * Returns: 0 on success, negative error code on failure.
> > - */
> > -static inline int
> > -drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm,
> > -             struct drm_exec *exec,
> > -             unsigned int num_fences)
> > -{
> > -    return drm_exec_prepare_obj(exec, gpuvm->r_obj, num_fences);
> > -}
> > +int drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm,
> > +             struct drm_exec *exec,
> > +             unsigned int num_fences);
> >     int drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm,
> >                     struct drm_exec *exec,