[Mesa-dev] [PATCH] llvmpipe, i915: add back NEW_RASTERIZER dependency when computing vertex info

[Jose Fonseca]

On 20/01/16 19:43, sroland at vmware.com wrote:
> From: Roland Scheidegger <sroland at vmware.com>
>
> I removed this mistakenly in 2dbc20e45689e09766552517a74e2270e49817b5. I
> actually thought it should not be necessary and a piglit run didn't show
> any differences, but this shouldn't have been in there.
> draw_prepare_shader_outputs() is in fact dependent on NEW_RASTERIZER.
> The new polygon-mode-facing test indeed shows why this is necessary, there's
> lots of invalid reads and writes with valgrind (also crashes without
> valgrind), because the pre-pipeline vertex size doesn't match the
> post-pipeline vertex size (note this won't help much with stages which don't
> have the prepare hook which can grow the vertex size, in particular the wide
> point stage, but this isn't used by llvmpipe). The test still won't pass, of
> course, but it is only usage of uninitialized values now, which is much
> less dangerous...
> (Albeit I'm pretty sure for i915 it really is not needed anymore as it
> doesn't care about the extra outputs and doesn't call
> draw_prepare_shader_outputs().)
> ---
>   src/gallium/auxiliary/draw/draw_llvm.c          | 6 ++++++
>   src/gallium/drivers/i915/i915_state_derived.c   | 2 +-
>   src/gallium/drivers/llvmpipe/lp_state_derived.c | 3 ++-
>   3 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/src/gallium/auxiliary/draw/draw_llvm.c b/src/gallium/auxiliary/draw/draw_llvm.c
> index 142d78a..b48bdcc 100644
> --- a/src/gallium/auxiliary/draw/draw_llvm.c
> +++ b/src/gallium/auxiliary/draw/draw_llvm.c
> @@ -1618,6 +1618,12 @@ draw_llvm_generate(struct draw_llvm *llvm, struct draw_llvm_variant *variant,
>      context_ptr               = LLVMGetParam(variant_func, 0);
>      io_ptr                    = LLVMGetParam(variant_func, 1);
>      vbuffers_ptr              = LLVMGetParam(variant_func, 2);
> +   /*
> +    * XXX: stride is actually unused. The stride we use is strictly calculated
> +    * from the number of outputs (including the draw_extra outputs).
> +    * Should probably fix some day (we need a new vs just because of extra
> +    * outputs which the generated vs won't touch).
> +    */
>      stride                    = LLVMGetParam(variant_func, 5 + (elts ? 1 : 0));
>      vb_ptr                    = LLVMGetParam(variant_func, 6 + (elts ? 1 : 0));
>      system_values.instance_id = LLVMGetParam(variant_func, 7 + (elts ? 1 : 0));
> diff --git a/src/gallium/drivers/i915/i915_state_derived.c b/src/gallium/drivers/i915/i915_state_derived.c
> index bd0f448..177b854 100644
> --- a/src/gallium/drivers/i915/i915_state_derived.c
> +++ b/src/gallium/drivers/i915/i915_state_derived.c
> @@ -184,7 +184,7 @@ static void calculate_vertex_layout(struct i915_context *i915)
>   struct i915_tracked_state i915_update_vertex_layout = {
>      "vertex_layout",
>      calculate_vertex_layout,
> -   I915_NEW_FS | I915_NEW_VS
> +   I915_NEW_RASTERIZER | I915_NEW_FS | I915_NEW_VS
>   };
>
>
> diff --git a/src/gallium/drivers/llvmpipe/lp_state_derived.c b/src/gallium/drivers/llvmpipe/lp_state_derived.c
> index 34961cb..fa3d725 100644
> --- a/src/gallium/drivers/llvmpipe/lp_state_derived.c
> +++ b/src/gallium/drivers/llvmpipe/lp_state_derived.c
> @@ -191,7 +191,8 @@ void llvmpipe_update_derived( struct llvmpipe_context *llvmpipe )
>         llvmpipe->dirty |= LP_NEW_SAMPLER_VIEW;
>      }
>
> -   if (llvmpipe->dirty & (LP_NEW_FS |
> +   if (llvmpipe->dirty & (LP_NEW_RASTERIZER |
> +                          LP_NEW_FS |
>                             LP_NEW_VS))
>         compute_vertex_info(llvmpipe);
>
>

Given it's not obvious, let's add a comment here explaining why 
NEW_RASTERIZER is necessary.

With that, this is

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