[Mesa-dev] [PATCH][RFC] i965: allocate a SGVS element when VertexID or InstanceID are read in gen9+

[Jason Ekstrand]

On January 3, 2018 03:21:51 Iago Toral Quiroga <itoral at igalia.com> wrote:

> Although in theory this should not be necessary it seems that doing this
> fixes a spurious low rate failure of ~1% in a CTS test that seems to happen
> in some gen9+ platforms only.
>
> Fixes flakyness in:
> KHR-GL45.vertex_attrib_64bit.limits_test
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104335
> ---
>
> I am not sure why this fixes the spurious fails, but it clearly does, at 
> least for
> me. I have not seen any workarounds related to this in the PRMs so maybe
> this is just papering over some other problem in the end. It would be great if
> someone else could test this and verify that it fixes the spurious fails 
> for them
> as well and I'd also appreciate any thoughts on what could be causing this 
> problem
> and why this seems to fix it. There is more detail in the bug report.
>
>  src/mesa/drivers/dri/i965/genX_state_upload.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/i965/genX_state_upload.c 
> b/src/mesa/drivers/dri/i965/genX_state_upload.c
> index 50ac5bc59f..523e9688a6 100644
> --- a/src/mesa/drivers/dri/i965/genX_state_upload.c
> +++ b/src/mesa/drivers/dri/i965/genX_state_upload.c
> @@ -494,12 +494,25 @@ genX(emit_vertices)(struct brw_context *brw)
>      * values.  Additionally if there is an edge flag element then the SGVS
>      * can't be inserted past that so we need a dummy element to ensure that
>      * the edge flag is the last one.
> +    *
> +    * In gen9+, the CTS test KHR-GL45.vertex_attrib_64bit.limits_test fails
> +    * with a fail rate of ~1% for the cases where it reads gl_InstanceID 
> but no
> +    * SGVS element is programmed. Programming the SGVS element in this case
> +    * seems to make the problem go away.
>      */
> +#if GEN_GEN >= 9
> +   const bool needs_sgvs_element = (vs_prog_data->uses_basevertex ||
> +                                    vs_prog_data->uses_baseinstance ||
> +                                    vs_prog_data->uses_instanceid ||
> +                                    vs_prog_data->uses_vertexid);
> +#else
>     const bool needs_sgvs_element = (vs_prog_data->uses_basevertex ||
>                                      vs_prog_data->uses_baseinstance ||
>                                      ((vs_prog_data->uses_instanceid ||
>                                        vs_prog_data->uses_vertexid)
>                                       && uses_edge_flag));

Something smells fishy here.  Why are the last to && with uses_edge_flag in 
the first place?  (That's a rhetorical question; I went and read the code.) 
 It seems to me as if we want the element even if we're just going to put 
instance or vertex id in it.  In particular, I'm not sure that emitting 
3DSTATE_VERTEX_ELEMENTS actually clears out the elements not specified so 
we may have some random vertex element from a previous draw that we're 
overwriting. When you combine that with the following two restrictions, 
things could get very weird:

 It is INVALID to use this command to overwrite any portion of a 64-bit 
vertex element component.
 It is INVALID to use this command to overwrite a EdgeFlag vertex element 
component or any vertex
element beyond it.

I think this patch is probably more-or-less doing the right thing though I 
would argue we should probably do it on all gen8+ and we could pull 
needs_sgvs_element out of the #if entirely.  Good work hunting this down.

--Jason

> +#endif
> +
>  #else
>     const bool needs_sgvs_element = (vs_prog_data->uses_basevertex ||
>                                      vs_prog_data->uses_baseinstance ||
> --
> 2.11.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev