On Thu, May 15, 2025 at 2:06 AM Danilo Krummrich <d...@kernel.org> wrote:
>
> On Thu, May 15, 2025 at 10:54:27AM +0200, Danilo Krummrich wrote:
> > Hi Rob,
> >
> > Can you please CC me on patches for GPUVM?
> >
> > On Wed, May 14, 2025 at 10:53:15AM -0700, Rob Clark wrote:
> > > From: Rob Clark <robdcl...@chromium.org>
> > >
> > > See commit a414fe3a2129 ("drm/msm/gem: Drop obj lock in
> > > msm_gem_free_object()") for justification.
> >
> > Please write a proper commit message that explains the problem and the
> > solution.
> > Please don't just refer to another commit and leave it to the reviewer of
> > the
> > patch to figure this out.
> >
> > > Signed-off-by: Rob Clark <robdcl...@chromium.org>
> > > ---
> > > drivers/gpu/drm/drm_gpuvm.c | 7 +++++--
> > > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> >
> > > diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
> > > index f9eb56f24bef..1e89a98caad4 100644
> > > --- a/drivers/gpu/drm/drm_gpuvm.c
> > > +++ b/drivers/gpu/drm/drm_gpuvm.c
> > > @@ -1511,7 +1511,9 @@ drm_gpuvm_bo_destroy(struct kref *kref)
> > > drm_gpuvm_bo_list_del(vm_bo, extobj, lock);
> > > drm_gpuvm_bo_list_del(vm_bo, evict, lock);
> > >
> > > - drm_gem_gpuva_assert_lock_held(obj);
> > > + if (kref_read(&obj->refcount) > 0)
> > > + drm_gem_gpuva_assert_lock_held(obj);
> > > +
> > > list_del(&vm_bo->list.entry.gem);
> >
> > This seems wrong.
> >
> > A VM_BO object keeps a reference of the underlying GEM object, so this
> > should
> > never happen.
> >
> > This function calls drm_gem_object_put() before it returns.
>
> I noticed your subsequent patch that allows VM_BO structures to have weak
> references to GEM objects.
>
> However, even with that this seems wrong. If the reference count of the GEM
> object is zero when drm_gpuvm_bo_destroy() is called it means that the GEM
> object is dead. However, until drm_gpuvm_bo_destroy() is called the GEM object
> potentially remains to be on the extobj and eviced list, which means that
> other
> code paths might fetch it from those lists and consider it to be a valid GEM
> object.
We only iterate extobj or evicted in VM_BIND mode, where we aren't
using WEAK_REF. I suppose some WARN_ON()s or BUG_ON()s could make
this more clear.
BR,
-R
