[Mesa-dev] [PATCH v2] i915: fixing driver crashes if too few vertices are submitted

[Ville Syrjälä]

On Wed, Sep 09, 2015 at 11:53:54AM -0700, Ian Romanick wrote:
> On 09/09/2015 11:16 AM, Marius Predut wrote:
> > Comparison with a signed expression and unsigned value
> > is converted to unsigned value, reason for minus value is interpreted
> > as a big unsigned value. For this case the "for" loop
> > is going into unexpected behavior.
> > 
> > v1:Brian Paul: code style fix.
> 
> I don't think you really did...
> 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=38109
> > Signed-off-by: Marius Predut <marius.predut at intel.com>
> > ---
> >  src/mesa/tnl_dd/t_dd_dmatmp.h | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/mesa/tnl_dd/t_dd_dmatmp.h b/src/mesa/tnl_dd/t_dd_dmatmp.h
> > index 7be3954..79de224 100644
> > --- a/src/mesa/tnl_dd/t_dd_dmatmp.h
> > +++ b/src/mesa/tnl_dd/t_dd_dmatmp.h
> > @@ -627,6 +627,9 @@ static void TAG(render_quads_verts)( struct gl_context *ctx,
> >        LOCAL_VARS;
> >        GLuint j;
> >  
> > +      if(count % 4 != 0)
>            ^
> ...because I'm quite sure Brian's code had a space here, per Mesa's
> coding standards.
> 
> Also, this change is incorrect.  If an application does
> glDrawArrays(GL_QUADS, 0, (n * 4) + 3), this change will cause zero
> quads to be drawn when n quads should be drawn.

Due to the other check added to validate_render() we would never even
get here since we would have dropped off the fast path entrirely. Which
is not a nice thing to do simply due to an extra vertex hanging around
somewhere.

> 
> Page 18 (page 32 of the PDF) of the OpenGL 2.1 spec says:
> 
>     "The total number of vertices between Begin and End is 4n + k,
>     where 0 ≤ k ≤ 3; if k is not zero, the final k vertices are
>     ignored."
> 
> We probably don't have a piglit test for this scenario, so one should be
> added.  You can CC me on that patch. :)
> 
> I think the correct change is to trim count such that (count % 4) == 0.
>  If the modified value of count is zero, bail out.  With that change,
> the other hunk (below) is unnecessary.

I would suggest having one function that gets passed the primitive and
vertex count and have it return the trimmed vertex count. That could
then be called from both intel_run_render() and and TAG(validate_render())
to skip any extra vertices. That way none of the actual render functions
in t_dd_dmatmp.h need worry about any extra vertices.

> 
> > +          return;
> > +
> >        INIT(GL_TRIANGLES);
> >  
> >        for (j = start; j < count-3; j += 4) {
> > @@ -1248,7 +1251,7 @@ static GLboolean TAG(validate_render)( struct gl_context *ctx,
> >  	    ok = (GLint) count < GET_SUBSEQUENT_VB_MAX_ELTS();
> >  	 }
> >  	 else {
> > -	    ok = HAVE_TRIANGLES; /* flatshading is ok. */
> > +	    ok = HAVE_TRIANGLES && (count % 4 == 0); /* flatshading is ok. */
> >  	 }
> >  	 break;
> >        default:
> > 
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev

-- 
Ville Syrjälä
Intel OTC