[Mesa-dev] [PATCH] draw: force draw pipeline if there's more than 65535 vertices

[Jose Fonseca]

On 22/07/18 00:09, sroland at vmware.com wrote:
> From: Roland Scheidegger <sroland at vmware.com>
> 
> The pt emit path can only handle 65535 - the number of vertices is
> truncated to a ushort, resulting in a too small buffer allocation, which
> will crash.
> 
> Forcing the pipeline path looks suboptimal, then again this bug is
> probably there ever since GS is supported, so it seems it's not
> happening often. (Note that the vertex_id in the vertex header is 16
> bit too, however this is only used by the draw pipeline, and it denotes
> the emit vertex nr, and that uses vbuf code, which will only emit smaller
> chunks, so should be fine I think.)
> Other solutions would be to simply allow 32bit counts for vertex
> allocation, however 65535 is already larger than this was intended for
> (the idea being it should be more cache friendly). Or could try to teach
> the pt emit path to split the emit in smaller chunks (only the non-index
> path can be affected, since gs output is always linear), but it's a bit
> tricky (we don't know the primitive boundaries up-front).
> 
> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=107295
> 
> Cc: <mesa-stable at lists.freedesktop.org>
> ---
>   src/gallium/auxiliary/draw/draw_pt_emit.c                      |  2 ++
>   src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline.c      | 10 ++++++++++
>   src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline_llvm.c |  9 +++++++++
>   3 files changed, 21 insertions(+)
> 
> diff --git a/src/gallium/auxiliary/draw/draw_pt_emit.c b/src/gallium/auxiliary/draw/draw_pt_emit.c
> index 6fb630b549..984c76fdf9 100644
> --- a/src/gallium/auxiliary/draw/draw_pt_emit.c
> +++ b/src/gallium/auxiliary/draw/draw_pt_emit.c
> @@ -158,6 +158,7 @@ draw_pt_emit(struct pt_emit *emit,
>       */
>      render->set_primitive(draw->render, prim_info->prim);
>   
> +   assert(vertex_count <= 65535);
>      render->allocate_vertices(render,
>                                (ushort)translate->key.output_stride,
>                                (ushort)vertex_count);
> @@ -229,6 +230,7 @@ draw_pt_emit_linear(struct pt_emit *emit,
>       */
>      render->set_primitive(draw->render, prim_info->prim);
>   
> +   assert(count <= 65535);
>      if (!render->allocate_vertices(render,
>                                     (ushort)translate->key.output_stride,
>                                     (ushort)count))
> diff --git a/src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline.c b/src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline.c
> index aa20b918f5..f76e022994 100644
> --- a/src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline.c
> +++ b/src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline.c
> @@ -299,6 +299,16 @@ fetch_pipeline_generic(struct draw_pt_middle_end *middle,
>         FREE(vert_info->verts);
>         vert_info = &gs_vert_info;
>         prim_info = &gs_prim_info;
> +
> +      /*
> +       * pt emit can only handle ushort number of vertices (see
> +       * render->allocate_vertices).
> +       * vsplit guarantees there's never more than 4096, however GS can
> +       * easily blow this up (by a factor of 256 (or even 1024) max).
> +       */
> +      if (vert_info->count > 65535) {
> +         opt |= PT_PIPELINE;
> +      }
>      } else {
>         if (draw_prim_assembler_is_required(draw, prim_info, vert_info)) {
>            draw_prim_assembler_run(draw, prim_info, vert_info,
> diff --git a/src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline_llvm.c b/src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline_llvm.c
> index 5e0c562256..91c9360cce 100644
> --- a/src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline_llvm.c
> +++ b/src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline_llvm.c
> @@ -428,6 +428,15 @@ llvm_pipeline_generic(struct draw_pt_middle_end *middle,
>         FREE(vert_info->verts);
>         vert_info = &gs_vert_info;
>         prim_info = &gs_prim_info;
> +      /*
> +       * pt emit can only handle ushort number of vertices (see
> +       * render->allocate_vertices).
> +       * vsplit guarantees there's never more than 4096, however GS can
> +       * easily blow this up (by a factor of 256 (or even 1024) max).
> +       */
> +      if (vert_info->count > 65535) {
> +         opt |= PT_PIPELINE;
> +      }
>      } else {
>         if (draw_prim_assembler_is_required(draw, prim_info, vert_info)) {
>            draw_prim_assembler_run(draw, prim_info, vert_info,
> 

Sounds good to me.

Reviewed-by: Jose Fonseca <jfonseca at vmware.com>