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

[Mathias Fröhlich]

Hi,

> This adds uses of alloca, without a corresponding include of alloca.h.
> 
> Perhaps something like the attached is needed?

Yes, sorry!

Reviewed-by: Mathias Fröhlich 

best

Mathias


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Jon Turney]

On 28/03/2018 11:35, mathias.froehl...@gmx.net wrote:


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 
---
  src/mesa/vbo/vbo_context.c | 70 +++---
  1 file changed, 47 insertions(+), 23 deletions(-)


This adds uses of alloca, without a corresponding include of alloca.h.

Perhaps something like the attached is needed?

See also:
https://lists.freedesktop.org/archives/mesa-dev/2018-January/184030.html
https://lists.freedesktop.org/archives/mesa-dev/2017-December/180073.html
https://lists.freedesktop.org/archives/mesa-dev/2016-July/122346.html

From 8599fd9109d59adc541ab06570732776e2c098d6 Mon Sep 17 00:00:00 2001
From: Jon Turney 
Date: Tue, 3 Apr 2018 17:52:56 +0100
Subject: [PATCH] Fix use of alloca() without #include 

Fix use of alloca() without #include  in 1da345e5

vbo/vbo_context.c: In function '_vbo_draw_indirect':
vbo/vbo_context.c:284:34: error: implicit declaration of function 'alloca' 
[-Werror=implicit-function-declaration]
   struct _mesa_prim *space = alloca(draw_count*sizeof(struct _mesa_prim));
  ^~
vbo/vbo_context.c:284:34: warning: initialization makes pointer from integer 
without a cast [-Wint-conversion]

Signed-off-by: Jon Turney 
---
 src/mesa/vbo/vbo_context.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/mesa/vbo/vbo_context.c b/src/mesa/vbo/vbo_context.c
index e50cee7a8c..f698fd0f41 100644
--- a/src/mesa/vbo/vbo_context.c
+++ b/src/mesa/vbo/vbo_context.c
@@ -25,6 +25,7 @@
  *Keith Whitwell 
  */
 
+#include "c99_alloca.h"
 #include "main/mtypes.h"
 #include "main/bufferobj.h"
 #include "math/m_eval.h"
-- 
2.16.2

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Mathias Fröhlich]

Hi Brian,

> 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?
I'll try my best!

> Other than that, the series looks good.
> 
> Reviewed-by: Brian Paul 
Thanks for the review!!!

> Sorry for the slow review, busy with other things.
No worries. I can make use of the wait time in between :-)
But now, today is mesa time for me ...
Thank you!

best

Mathias


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Brian Paul]

On 03/28/2018 04:35 AM, mathias.froehl...@gmx.net wrote:

From: Mathias Fröhlich 


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

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



___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org

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

[Marek Olšák]

Yes, it looks good.

Marek

On Wed, Mar 28, 2018 at 6:35 AM,  wrote:

> From: Mathias Fröhlich 
>
>
> 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 
> ---
>  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)
>  {
> -   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);
> +   }
>  }
> --
> 2.14.3
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Mathias . Froehlich]

From: Mathias Fröhlich 


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 
---
 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)
 {
-   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);
+   }
 }
-- 
2.14.3

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev