[Mesa-dev] [PATCH] draw: fix incorrect vertex size computation in LLVM drawing code

[Jose Fonseca]

Good catch Brian. Change looks good to me AFAICT.

We do need more this sort of assertions in the draw module.  I never fail to get surprised at how intricate and sensitive draw module is -- mostly due the fact that it reaches out to the upstream gallium interface from within the driver.  I do hope one day the state trackers just use geometry shaders for all those things, so that the draw module is a straighforward VS/GS processor, and does not need to create its own fragment sahders.  (It is unfortunate that with util/u_blitter we replicate the bad example of reaching out to gallium interface from inside the driver.)


Jose


----- Original Message -----
> We were calling draw_total_vs_outputs() too early.  The call to
> draw_pt_emit_prepare() could result in the vertex size changing.
> So call draw_total_vs_outputs() after draw_pt_emit_prepare().
> 
> This fix would seem to be needed for the non-LLVM code as well,
> but it's not obvious.  Instead, I added an assertion there to
> try to catch this problem if it were to occur there.
> 
> Bugzilla:
> https://urldefense.proofpoint.com/v1/url?u=https://bugs.freedesktop.org/show_bug.cgi?id%3D72926&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=NMr9uy2iTjWVixC0wOcYCWEIYhfo80qKwRgdodpoDzA%3D%0A&m=91zldQ8lO5OhCBRVy4vcYnfNUSKL3Bo1lcxmHhpMqWQ%3D%0A&s=2dc25b5d2d9cdcc89cb242f8bdd09f59761026dd200cea7eb9dd3278928e3603
> Cc: 10.0 <mesa-stable at lists.freedesktop.org>
> ---
>  .../auxiliary/draw/draw_pt_fetch_shade_pipeline.c  |    7 +++++--
>  .../draw/draw_pt_fetch_shade_pipeline_llvm.c       |   22
>  ++++++++++++--------
>  2 files changed, 18 insertions(+), 11 deletions(-)
> 
> 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 8fcc170..2c5c4cd 100644
> --- a/src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline.c
> +++ b/src/gallium/auxiliary/draw/draw_pt_fetch_shade_pipeline.c
> @@ -72,8 +72,8 @@ static void fetch_pipeline_prepare( struct
> draw_pt_middle_end *middle,
>  
>     const unsigned gs_out_prim = (gs ? gs->output_primitive :
>                                   u_assembled_prim(prim));
> -   unsigned nr = MAX2( vs->info.num_inputs,
> -		       draw_total_vs_outputs(draw) );
> +   unsigned nr_vs_outputs = draw_total_vs_outputs(draw);
> +   unsigned nr = MAX2(vs->info.num_inputs, nr_vs_outputs);
>  
>     if (gs) {
>        nr = MAX2(nr, gs->info.num_outputs + 1);
> @@ -129,6 +129,9 @@ static void fetch_pipeline_prepare( struct
> draw_pt_middle_end *middle,
>     /* No need to prepare the shader.
>      */
>     vs->prepare(vs, draw);
> +
> +   /* Make sure that the vertex size didn't change at any point above */
> +   assert(nr_vs_outputs == draw_total_vs_outputs(draw));
>  }
>  
>  
> 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 9f17241..60ec528 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
> @@ -141,19 +141,11 @@ llvm_middle_end_prepare( struct draw_pt_middle_end
> *middle,
>     struct draw_geometry_shader *gs = draw->gs.geometry_shader;
>     const unsigned out_prim = gs ? gs->output_primitive :
>        u_assembled_prim(in_prim);
> -   const unsigned nr = MAX2(vs->info.num_inputs,
> -                            draw_total_vs_outputs(draw));
> +   unsigned nr;
>  
>     fpme->input_prim = in_prim;
>     fpme->opt = opt;
>  
> -   /* Always leave room for the vertex header whether we need it or
> -    * not.  It's hard to get rid of it in particular because of the
> -    * viewport code in draw_pt_post_vs.c.
> -    */
> -   fpme->vertex_size = sizeof(struct vertex_header) + nr * 4 *
> sizeof(float);
> -
> -
>     draw_pt_post_vs_prepare( fpme->post_vs,
>                              draw->clip_xy,
>                              draw->clip_z,
> @@ -177,6 +169,18 @@ llvm_middle_end_prepare( struct draw_pt_middle_end
> *middle,
>        *max_vertices = 4096;
>     }
>  
> +   /* Get the number of float[4] attributes per vertex.
> +    * Note: this must be done after draw_pt_emit_prepare() since that
> +    * can effect the vertex size.
> +    */
> +   nr = MAX2(vs->info.num_inputs, draw_total_vs_outputs(draw));
> +
> +   /* Always leave room for the vertex header whether we need it or
> +    * not.  It's hard to get rid of it in particular because of the
> +    * viewport code in draw_pt_post_vs.c.
> +    */
> +   fpme->vertex_size = sizeof(struct vertex_header) + nr * 4 *
> sizeof(float);
> +
>     /* return even number */
>     *max_vertices = *max_vertices & ~1;
>  
> --
> 1.7.10.4
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://urldefense.proofpoint.com/v1/url?u=http://lists.freedesktop.org/mailman/listinfo/mesa-dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=NMr9uy2iTjWVixC0wOcYCWEIYhfo80qKwRgdodpoDzA%3D%0A&m=91zldQ8lO5OhCBRVy4vcYnfNUSKL3Bo1lcxmHhpMqWQ%3D%0A&s=beda6ecc8e4e1c8c7778d049ee75198ed00145f1ec140f1a3233a2dc5a93c732
>