[PATCH 1/2] drm/amdgpu: add a spinlock to wb allocation

[Alex Deucher]

On Tue, Apr 23, 2024 at 2:57 AM Christian König
<ckoenig.leichtzumerken at gmail.com> wrote:
>
> Am 22.04.24 um 16:37 schrieb Alex Deucher:
> > As we use wb slots more dynamically, we need to lock
> > access to avoid racing on allocation or free.
>
> Wait a second. Why are we using the wb slots dynamically?
>

See patch 2.  I needed a way to allocate small GPU accessible memory
locations on the fly.  Using WB seems like a good solution.

Alex

> The number of slots made available is statically calculated, when this
> is suddenly used dynamically we have quite a bug here.
>
> Regards,
> Christian.
>
> >
> > Signed-off-by: Alex Deucher <alexander.deucher at amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  1 +
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 11 ++++++++++-
> >   2 files changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > index cac0ca64367b..f87d53e183c3 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > @@ -502,6 +502,7 @@ struct amdgpu_wb {
> >       uint64_t                gpu_addr;
> >       u32                     num_wb; /* Number of wb slots actually reserved for amdgpu. */
> >       unsigned long           used[DIV_ROUND_UP(AMDGPU_MAX_WB, BITS_PER_LONG)];
> > +     spinlock_t              lock;
> >   };
> >
> >   int amdgpu_device_wb_get(struct amdgpu_device *adev, u32 *wb);
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index f8a34db5d9e3..869256394136 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -1482,13 +1482,17 @@ static int amdgpu_device_wb_init(struct amdgpu_device *adev)
> >    */
> >   int amdgpu_device_wb_get(struct amdgpu_device *adev, u32 *wb)
> >   {
> > -     unsigned long offset = find_first_zero_bit(adev->wb.used, adev->wb.num_wb);
> > +     unsigned long flags, offset;
> >
> > +     spin_lock_irqsave(&adev->wb.lock, flags);
> > +     offset = find_first_zero_bit(adev->wb.used, adev->wb.num_wb);
> >       if (offset < adev->wb.num_wb) {
> >               __set_bit(offset, adev->wb.used);
> > +             spin_unlock_irqrestore(&adev->wb.lock, flags);
> >               *wb = offset << 3; /* convert to dw offset */
> >               return 0;
> >       } else {
> > +             spin_unlock_irqrestore(&adev->wb.lock, flags);
> >               return -EINVAL;
> >       }
> >   }
> > @@ -1503,9 +1507,13 @@ int amdgpu_device_wb_get(struct amdgpu_device *adev, u32 *wb)
> >    */
> >   void amdgpu_device_wb_free(struct amdgpu_device *adev, u32 wb)
> >   {
> > +     unsigned long flags;
> > +
> >       wb >>= 3;
> > +     spin_lock_irqsave(&adev->wb.lock, flags);
> >       if (wb < adev->wb.num_wb)
> >               __clear_bit(wb, adev->wb.used);
> > +     spin_unlock_irqrestore(&adev->wb.lock, flags);
> >   }
> >
> >   /**
> > @@ -4061,6 +4069,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> >       spin_lock_init(&adev->se_cac_idx_lock);
> >       spin_lock_init(&adev->audio_endpt_idx_lock);
> >       spin_lock_init(&adev->mm_stats.lock);
> > +     spin_lock_init(&adev->wb.lock);
> >
> >       INIT_LIST_HEAD(&adev->shadow_list);
> >       mutex_init(&adev->shadow_list_lock);
>