[PATCH v3 1/2] drm/amdgpu: Add a max ibs per submission limit.

[Bas Nieuwenhuizen]

On Wed, Apr 12, 2023 at 2:46 PM Christian König
<ckoenig.leichtzumerken at gmail.com> wrote:
>
> Am 12.04.23 um 14:18 schrieb Bas Nieuwenhuizen:
> > And ensure each ring supports that many submissions. This makes
> > sure that we don't get surprises after the submission has been
> > scheduled where the ring allocation actually gets rejected.
> >
> > Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2498
> > Signed-off-by: Bas Nieuwenhuizen <bas at basnieuwenhuizen.nl>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c   |  3 +++
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 25 ++++++++++++++++++++++++
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h |  1 +
> >   3 files changed, 29 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > index 7af3041ccd0e..8362738974c8 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > @@ -110,6 +110,9 @@ static int amdgpu_cs_p1_ib(struct amdgpu_cs_parser *p,
> >       if (r < 0)
> >               return r;
> >
> > +     if (num_ibs[r] >= amdgpu_ring_max_ibs(chunk_ib->ip_type))
> > +             return -EINVAL;
> > +
> >       ++(num_ibs[r]);
> >       p->gang_leader_idx = r;
> >       return 0;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> > index dc474b809604..abd70d2f26f6 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> > @@ -49,6 +49,25 @@
> >    * them until the pointers are equal again.
> >    */
> >
> > +/**
> > + * amdgpu_ring_max_ibs - Return max IBs that fit in a single submission.
> > + *
> > + * @type: ring type for which to return the limit.
> > + */
> > +unsigned int amdgpu_ring_max_ibs(enum amdgpu_ring_type type)
> > +{
> > +     switch (type) {
> > +     case AMDGPU_RING_TYPE_GFX:
> > +     case AMDGPU_RING_TYPE_COMPUTE:
> > +             /* gfx/compute are often used more extensively and radv
> > +              * has historically assumed the limit was 192.
> > +              */
> > +             return 192;
> > +     default:
> > +             return 50;
> > +     }
> > +}
> > +
> >   /**
> >    * amdgpu_ring_alloc - allocate space on the ring buffer
> >    *
> > @@ -182,6 +201,7 @@ int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring,
> >       int sched_hw_submission = amdgpu_sched_hw_submission;
> >       u32 *num_sched;
> >       u32 hw_ip;
> > +     unsigned int max_ibs_dw;
> >
> >       /* Set the hw submission limit higher for KIQ because
> >        * it's used for a number of gfx/compute tasks by both
> > @@ -290,6 +310,11 @@ int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring,
> >               return r;
> >       }
> >
> > +     max_ibs_dw = ring->funcs->emit_frame_size +
> > +                  amdgpu_ring_max_ibs(ring->funcs->type) * ring->funcs->emit_ib_size;
> > +     max_ibs_dw = (max_ibs_dw + ring->funcs->align_mask) & ~ring->funcs->align_mask;
> > +     max_dw = max(max_dw, max_ibs_dw);
>
> I think something like "if (WARN_ON(max_ibs_dw > max_dw) max_dw =
> max_ibs_dw;" would be more appropriate.

I really like it this way because it automatically does the right
thing. In comparison it is difficult getting the test matrix together
for a WARN_ON on something that differs every generation, and I don't
know what we'd gain from doing it that way?


>
> Apart from that this looks really good to me,
> Christian.
>
> > +
> >       ring->ring_size = roundup_pow_of_two(max_dw * 4 * sched_hw_submission);
> >
> >       ring->buf_mask = (ring->ring_size / 4) - 1;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> > index 3989e755a5b4..e6e672727529 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> > @@ -317,6 +317,7 @@ struct amdgpu_ring {
> >   #define amdgpu_ring_patch_cond_exec(r,o) (r)->funcs->patch_cond_exec((r),(o))
> >   #define amdgpu_ring_preempt_ib(r) (r)->funcs->preempt_ib(r)
> >
> > +unsigned int amdgpu_ring_max_ibs(enum amdgpu_ring_type type);
> >   int amdgpu_ring_alloc(struct amdgpu_ring *ring, unsigned ndw);
> >   void amdgpu_ring_ib_begin(struct amdgpu_ring *ring);
> >   void amdgpu_ring_ib_end(struct amdgpu_ring *ring);
>