[Mesa-dev] [PATCH] vbo: Use alloca for _vbo_draw_indirect.

[Brian Paul]

On 03/28/2018 04:35 AM, Mathias.Froehlich at gmx.net wrote:
> From: Mathias Fröhlich <mathias.froehlich at web.de>
> 
> 
> Marek,
> 
> you mean with the below patch as the 9-th change in the series?
> I would like to keep that change seprarate from #3 since patch #3
> just moves the already existing impelentation to the driver_functions
> level using the exactly identical implementation except calling into
> struct driver_functions instead of the vbo module draw function.
> 
> Also I do not want to call just blindly into alloca with possibly
> large counts. So, the implementation uses an upper bound when to use
> malloc instead of alloca.
> 
> Ok, with that?
> 
> best
> 
> Mathias
> 
> 
> 
> Avoid using malloc in the draw path of mesa.
> Since the draw_count is a user api input, fall back to malloc if
> the amount of consumed stack space may get too high.
> 
> Signed-off-by: Mathias Fröhlich <Mathias.Froehlich at web.de>
> ---
>   src/mesa/vbo/vbo_context.c | 70 +++++++++++++++++++++++++++++++---------------
>   1 file changed, 47 insertions(+), 23 deletions(-)
> 
> diff --git a/src/mesa/vbo/vbo_context.c b/src/mesa/vbo/vbo_context.c
> index b8c28ceffb..06b8f820ee 100644
> --- a/src/mesa/vbo/vbo_context.c
> +++ b/src/mesa/vbo/vbo_context.c
> @@ -233,25 +233,17 @@ _vbo_DestroyContext(struct gl_context *ctx)
>   }
>   
>   
> -void
> -_vbo_draw_indirect(struct gl_context *ctx, GLuint mode,
> -                        struct gl_buffer_object *indirect_data,
> -                        GLsizeiptr indirect_offset, unsigned draw_count,
> -                        unsigned stride,
> -                        struct gl_buffer_object *indirect_draw_count_buffer,
> -                        GLsizeiptr indirect_draw_count_offset,
> -                        const struct _mesa_index_buffer *ib)
> +static void
> +draw_indirect(struct gl_context *ctx, GLuint mode,
> +              struct gl_buffer_object *indirect_data,
> +              GLsizeiptr indirect_offset, unsigned draw_count,
> +              unsigned stride,
> +              struct gl_buffer_object *indirect_draw_count_buffer,
> +              GLsizeiptr indirect_draw_count_offset,
> +              const struct _mesa_index_buffer *ib,
> +              struct _mesa_prim *space)

Can you just rename 'space' to 'prim' and rm the prim = space assignment 
below?

Also, could you put a comment on this function to explain the draw_count 
and space/prim parameters, at least?

Other than that, the series looks good.

Reviewed-by: Brian Paul <brianp at vmware.com>

Sorry for the slow review, busy with other things.

-Brian


>   {
> -   struct _mesa_prim *prim;
> -
> -   prim = calloc(draw_count, sizeof(*prim));
> -   if (prim == NULL) {
> -      _mesa_error(ctx, GL_OUT_OF_MEMORY, "gl%sDraw%sIndirect%s",
> -                  (draw_count > 1) ? "Multi" : "",
> -                  ib ? "Elements" : "Arrays",
> -                  indirect_data ? "CountARB" : "");
> -      return;
> -   }
> +   struct _mesa_prim *prim = space;
>   
>      prim[0].begin = 1;
>      prim[draw_count - 1].end = 1;
> @@ -266,10 +258,42 @@ _vbo_draw_indirect(struct gl_context *ctx, GLuint mode,
>      /* This should always be true at this time */
>      assert(indirect_data == ctx->DrawIndirectBuffer);
>   
> -   ctx->Driver.Draw(ctx, prim, draw_count,
> -                   ib, false, 0, ~0,
> -                   NULL, 0,
> -                   indirect_data);
> +   ctx->Driver.Draw(ctx, prim, draw_count, ib, false, 0u, ~0u,
> +                    NULL, 0, indirect_data);
> +}
> +
>   
> -   free(prim);
> +void
> +_vbo_draw_indirect(struct gl_context *ctx, GLuint mode,
> +                   struct gl_buffer_object *indirect_data,
> +                   GLsizeiptr indirect_offset, unsigned draw_count,
> +                   unsigned stride,
> +                   struct gl_buffer_object *indirect_draw_count_buffer,
> +                   GLsizeiptr indirect_draw_count_offset,
> +                   const struct _mesa_index_buffer *ib)
> +{
> +   /* Use alloca for the prim space if we are somehow in bounds. */
> +   if (draw_count*sizeof(struct _mesa_prim) < 1024) {
> +      struct _mesa_prim *space = alloca(draw_count*sizeof(struct _mesa_prim));
> +      memset(space, 0, draw_count*sizeof(struct _mesa_prim));
> +
> +      draw_indirect(ctx, mode, indirect_data, indirect_offset, draw_count,
> +                    stride, indirect_draw_count_buffer,
> +                    indirect_draw_count_offset, ib, space);
> +   } else {
> +      struct _mesa_prim *space = calloc(draw_count, sizeof(struct _mesa_prim));
> +      if (space == NULL) {
> +         _mesa_error(ctx, GL_OUT_OF_MEMORY, "gl%sDraw%sIndirect%s",
> +                     (draw_count > 1) ? "Multi" : "",
> +                     ib ? "Elements" : "Arrays",
> +                     indirect_data ? "CountARB" : "");
> +         return;
> +      }
> +
> +      draw_indirect(ctx, mode, indirect_data, indirect_offset, draw_count,
> +                    stride, indirect_draw_count_buffer,
> +                    indirect_draw_count_offset, ib, space);
> +
> +      free(space);
> +   }
>   }
>