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

[Christian König]

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.

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);