[Mesa-dev] [PATCH 13/18] winsys/amdgpu: start with smaller IBs, growing as necessary

[Marek Olšák]

On Tue, May 10, 2016 at 1:21 AM, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>
> This avoids allocating giant IBs from the outset, especially for CE and DMA.
>
> With this change, we also never flush prematurely due to the CE IB: as long
> as there is space in the buffer, we will use it.
> ---
>  src/gallium/winsys/amdgpu/drm/amdgpu_cs.c | 55 +++++++++++++++++++++++++------
>  src/gallium/winsys/amdgpu/drm/amdgpu_cs.h |  1 +
>  2 files changed, 46 insertions(+), 10 deletions(-)
>
> diff --git a/src/gallium/winsys/amdgpu/drm/amdgpu_cs.c b/src/gallium/winsys/amdgpu/drm/amdgpu_cs.c
> index a318670..546f224 100644
> --- a/src/gallium/winsys/amdgpu/drm/amdgpu_cs.c
> +++ b/src/gallium/winsys/amdgpu/drm/amdgpu_cs.c
> @@ -335,11 +335,31 @@ static unsigned amdgpu_cs_add_buffer(struct radeon_winsys_cs *rcs,
>     return index;
>  }
>
> -static bool amdgpu_ib_new_buffer(struct radeon_winsys *ws, struct amdgpu_ib *ib,
> -                                 unsigned buffer_size)
> +static bool amdgpu_ib_new_buffer(struct radeon_winsys *ws, struct amdgpu_ib *ib)
>  {
>     struct pb_buffer *pb;
>     uint8_t *mapped;
> +   unsigned buffer_size;
> +
> +   /* Always create a buffer that is at least as large as the largest IB
> +    * seen so far (multiplied by a factor to reduce internal fragmentation),
> +    * but never more than the maximum IB size supported by the hardware.
> +    */
> +   buffer_size = 4 << MIN2(19, 2 + util_last_bit(ib->max_ib_size));

Would you please use something more readable? I think it's equal or
very similar to this expression:

MIN2(2 * 1024 * 1024, 4 * 4 * util_next_power_of_two(ib->max_ib_size))

And a comment explaining those numbers would be useful. For example,
"2MB is the maximum IB size allowed by the winsys" (I think the hw
limit is 4 MB actually) "and we always allocate 4 times more space
than the maximum seen IB size aligned to 2^n".


> +
> +   switch (ib->ib_type) {
> +   case IB_CONST_PREAMBLE:
> +      buffer_size = MAX2(buffer_size, 4 * 1024);
> +      break;
> +   case IB_CONST:
> +      buffer_size = MAX2(buffer_size, 16 * 1024 * 4);
> +      break;
> +   case IB_MAIN:
> +      buffer_size = MAX2(buffer_size, 8 * 1024 * 4);
> +      break;
> +   default:
> +      unreachable("unhandled IB type");
> +   }
>
>     pb = ws->buffer_create(ws, buffer_size, 4096, RADEON_DOMAIN_GTT,
>                            RADEON_FLAG_CPU_ACCESS);
> @@ -370,35 +390,34 @@ static bool amdgpu_get_new_ib(struct radeon_winsys *ws, struct amdgpu_cs *cs,
>      */
>     struct amdgpu_ib *ib = NULL;
>     struct amdgpu_cs_ib_info *info = &cs->csc->ib[ib_type];
> -   unsigned buffer_size, ib_size;
> +   unsigned ib_size = 0;
>
>     switch (ib_type) {
>     case IB_CONST_PREAMBLE:
>        ib = &cs->const_preamble_ib;
> -      buffer_size = 4 * 1024 * 4;
> -      ib_size = 1024 * 4;
> +      ib_size = 256 * 4;
>        break;
>     case IB_CONST:
>        ib = &cs->const_ib;
> -      buffer_size = 512 * 1024 * 4;
> -      ib_size = 128 * 1024 * 4;
> +      ib_size = 8 * 1024 * 4;
>        break;
>     case IB_MAIN:
>        ib = &cs->main;
> -      buffer_size = 128 * 1024 * 4;
> -      ib_size = 20 * 1024 * 4;
> +      ib_size = 4 * 1024 * 4;
>        break;
>     default:
>        unreachable("unhandled IB type");
>     }
>
> +   ib_size = MAX2(ib_size, 4 << MIN2(19, util_last_bit(ib->max_ib_size)));

This is an expression similar to the one above. Some unification would be nice.

Marek