[Mesa-dev] [PATCH v2] i965: allocate a SGVS element when VertexID or InstanceID are read

[Iago Toral]

Ken, do you have any comments about this patch? I'd like to push it
otherwise.

Iago

On Thu, 2018-01-04 at 14:24 -0800, Jason Ekstrand wrote:
> Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>
> 
> Ken?
> 
> On Wed, Jan 3, 2018 at 6:55 PM, Iago Toral Quiroga <itoral at igalia.com
> > wrote:
> > Although on gen8+ platforms we can in theory use 3DSTATE_VF_SGVS
> > 
> > to put these beyond the last vertex element it seems that we still
> > 
> > need to allocate the SVGS element, otherwise we have observed cases
> > 
> > where we end up reading garbage. Specifically, the CTS test
> > mentioned
> > 
> > below was flaky with a fail rate of ~1% on some gen9+ platforms
> > caused
> > 
> > by reading garbage for the gl_InstanceID value. The flakyness goes
> > 
> > away as soon as we start allocating the SVGS element.
> > 
> > 
> > 
> > v2:
> > 
> >   - Do this for gen8+, not just gen9+, and pull the boolean
> > 
> >     outside the #if block (Jason)
> > 
> > 
> > 
> > Fixes flaky test:
> > 
> > KHR-GL45.vertex_attrib_64bit.limits_test
> > 
> > 
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104335
> > 
> > ---
> > 
> >  src/mesa/drivers/dri/i965/genX_state_upload.c | 17 ++-------------
> > --
> > 
> >  1 file changed, 2 insertions(+), 15 deletions(-)
> > 
> > 
> > 
> > diff --git a/src/mesa/drivers/dri/i965/genX_state_upload.c
> > b/src/mesa/drivers/dri/i965/genX_state_upload.c
> > 
> > index 50ac5bc59f..d0a980f973 100644
> > 
> > --- a/src/mesa/drivers/dri/i965/genX_state_upload.c
> > 
> > +++ b/src/mesa/drivers/dri/i965/genX_state_upload.c
> > 
> > @@ -486,26 +486,13 @@ genX(emit_vertices)(struct brw_context *brw)
> > 
> >     } else {
> > 
> >        brw_batch_emit(brw, GENX(3DSTATE_VF_SGVS), vfs);
> > 
> >     }
> > 
> > +#endif
> > 
> > 
> > 
> > -   /* Normally we don't need an element for the SGVS attribute
> > because the
> > 
> > -    * 3DSTATE_VF_SGVS instruction lets you store the generated
> > attribute in an
> > 
> > -    * element that is past the list in 3DSTATE_VERTEX_ELEMENTS.
> > However if
> > 
> > -    * we're using draw parameters then we need an element for the
> > those
> > 
> > -    * 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.
> > 
> > -    */
> > 
> > -   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));
> > 
> > -#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);
> > 
> > -#endif
> > 
> > +
> > 
> >     unsigned nr_elements =
> > 
> >        brw->vb.nr_enabled + needs_sgvs_element + vs_prog_data-
> > >uses_drawid;
> > 
> > 
> > 
> > --
> > 
> > 2.11.0
> > 
> > 
> > 
> > 
> 
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180110/e1026bc5/attachment-0001.html>