RE: [PATCH v2] drm/amdgpu/userq: fix lock missing for userq fence error set

AMD General

Regards,
      Prike

> -----Original Message-----
> From: Alex Deucher <[email protected]>
> Sent: Friday, August 21, 2026 9:02 PM
> To: Liang, Prike <[email protected]>
> Cc: [email protected]; Deucher, Alexander
> <[email protected]>; Koenig, Christian <[email protected]>
> Subject: Re: [PATCH v2] drm/amdgpu/userq: fix lock missing for userq fence error
> set
>
> On Fri, Aug 21, 2026 at 5:09 AM Liang, Prike <[email protected]> wrote:
> >
> > AMD General
> >
> > Regards,
> >       Prike
> >
> > > -----Original Message-----
> > > From: Alex Deucher <[email protected]>
> > > Sent: Wednesday, August 19, 2026 11:52 PM
> > > To: Liang, Prike <[email protected]>
> > > Cc: [email protected]; Deucher, Alexander
> > > <[email protected]>; Koenig, Christian
> > > <[email protected]>
> > > Subject: Re: [PATCH v2] drm/amdgpu/userq: fix lock missing for userq
> > > fence error set
> > >
> > > On Thu, Aug 13, 2026 at 11:50 PM Prike Liang <[email protected]> wrote:
> > > >
> > > > amdgpu_userq_fence_driver() and
> > > > amdgpu_userq_fence_driver_destroy()
> > > > don't acquire the dma_fence spinlock, so locking the dma_fence
> > > > lock before test the signaled state and set error state.
> > > >
> > > > Signed-off-by: Prike Liang <[email protected]>
> > > > ---
> > > >  .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.c   | 20 ++++++++++++-------
> > > >  1 file changed, 13 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> > > > index 19af1adc2921..09e2079cfdbd 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> > > > @@ -191,14 +191,15 @@ void
> > > > amdgpu_userq_fence_driver_destroy(struct kref
> > > *ref)
> > > >         struct dma_fence *f;
> > > >
> > > >         spin_lock_irqsave(&fence_drv->fence_list_lock, flags);
> > > > +       lockdep_assert_held(&fence_drv->fence_list_lock);
> > > >         list_for_each_entry_safe(fence, tmp, &fence_drv->fences, link) {
> > > >                 f = &fence->base;
> > > > -
> > > > -               if (!dma_fence_is_signaled(f)) {
> > > > +               spin_lock(dma_fence_spinlock(f));
> > > > +               if (!dma_fence_is_signaled_locked(f)) {
> > > >                         dma_fence_set_error(f, -ECANCELED);
> > > > -                       dma_fence_signal(f);
> > > > +                       dma_fence_signal_locked(f);
> > > >                 }
> > > > -
> > > > +               spin_unlock(dma_fence_spinlock(f));
> > > >                 list_del(&fence->link);
> > > >                 dma_fence_put(f);
> > > >         }
> > > > @@ -423,11 +424,16 @@ amdgpu_userq_fence_driver_set_error(struct
> > > amdgpu_userq_fence *fence,
> > > >         struct dma_fence *f;
> > > >
> > > >         spin_lock_irqsave(&fence_drv->fence_list_lock, flags);
> > > > -
> > > > +       lockdep_assert_held(&fence_drv->fence_list_lock);
> > > >         f = rcu_dereference_protected(&fence->base,
> > > >                                       lockdep_is_held(&fence_drv->fence_list_lock));
> > > > -       if (f && !dma_fence_is_signaled_locked(f))
> > > > -               dma_fence_set_error(f, error);
> > > > +       if (f) {
> > > > +               /* nest f->lock inside fence_list_lock */
> > > > +               spin_lock(dma_fence_spinlock(f));
> > >
> > > Why do we need to take the fence spinlock for userqs, but not for kernel
> queues?
> > > See amdgpu_fence.c
> >
> > For the kernel queue ring fence, we also need to hold the dma_fence lock when
> calling dma_fence_is_signaled_locked(). The reason that does not assert the lock
> missing for the kernel ring fence is the .signaled() callback is not implemented, so
> dma_fence_signal_locked() will never be invoked.
> >
>
> Sounds like we need to fix up the kernel queue fences as well.
>
> Acked-by: Alex Deucher <[email protected]>

Yeah, I will handle the kernel queue fences lock separately.

>
> > Thanks,
> > Prike
> >
> > > Alex
> > >
> > > > +               if (!dma_fence_is_signaled_locked(f))
> > > > +                       dma_fence_set_error(f, error);
> > > > +               spin_unlock(dma_fence_spinlock(f));
> > > > +       }
> > > >         spin_unlock_irqrestore(&fence_drv->fence_list_lock,
> > > > flags);  }
> > > >
> > > > --
> > > > 2.34.1
> > > >