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

[Predut, Marius]

> -----Original Message-----
> From: mesa-dev [mailto:mesa-dev-bounces at lists.freedesktop.org] On Behalf Of
> Ilia Mirkin
> Sent: Saturday, September 12, 2015 8:40 AM
> To: Ian Romanick
> Cc: mesa-dev at lists.freedesktop.org; Romanick, Ian D
> Subject: Re: [Mesa-dev] [PATCH v3] i915: fixing driver crashes if too few
> vertices are submitted
> 
> On Fri, Sep 11, 2015 at 10:45 PM, Ian Romanick <idr at freedesktop.org> wrote:
> > On 09/11/2015 10:55 AM, Ilia Mirkin wrote:
> >> On Fri, Sep 11, 2015 at 12:36 PM, Marius Predut <marius.predut at intel.com>
> 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.
> >>> v2: Ian Romanick: glDrawArrays(GL_QUADS, 0, (n * 4) + k) fail , k < 4.
> >>>
> >>> 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 | 7 +++++++
> >>>  1 file changed, 7 insertions(+)
> >>>
> >>> diff --git a/src/mesa/tnl_dd/t_dd_dmatmp.h
> >>> b/src/mesa/tnl_dd/t_dd_dmatmp.h index 7be3954..f99d977 100644
> >>> --- a/src/mesa/tnl_dd/t_dd_dmatmp.h
> >>> +++ b/src/mesa/tnl_dd/t_dd_dmatmp.h
> >>> @@ -627,6 +627,13 @@ static void TAG(render_quads_verts)( struct
> gl_context *ctx,
> >>>        LOCAL_VARS;
> >>>        GLuint j;
> >>>
> >>> +      /* 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.
> >>> +       */
> >>> +      count = (count / 4) * 4;
> >>
> >> Might be just me, but I'd find
> >>
> >> count &= ~0x3
> >>
> >> to be a lot clearer. Don't know if the compiler can make such an
> >> optimization.
> >
> > I think it can if count is unsigned.  Of course, GLsizei is not
> > unsigned.  It is already invalid for count < 0, so your optimization
> > is safe.
> 
> Actually count is a GLuint, so you're probably right that the compiler can
> work it out. I definitely have to think about what it's doing though, whereas
> with something like & ~3 it's pretty obvious. Perhaps I've been in bit-land
> too long.
> 
> >
> >> However this seems wrong... you're supposed to draw start..count, so
> >> that's the value that has to be div-by-4. Further up, when there's
> >> native quad support, the logic does:
> >
> > I don't think that's right.  Count is the number of vertices, not the
> > index of the last vertex.  Calling
> >
> >     glDrawArrays(GL_QUADS, 47000, 4);
> >
> > still draws one quad.
> >
> > Look at the pseudocode on page 28 (page 42 of the PDF) of the OpenGL
> > 2.1
> > spec:
> >
> >     "The command
> >
> >         void DrawArrays(enum mode, int first, sizei count);
> >
> >     constructs a sequence of geometric primitives using elements first
> >     through first + count − 1 of each enabled array. mode specifies
> >     what kind of primitives are constructed; it accepts the same token
> >     values as the mode parameter of the Begin command. The effect of
> >
> >         DrawArrays(mode, first, count);
> >
> >     is the same as the effect of the command sequence
> >
> >         if (mode or count is invalid)
> >             generate appropriate error
> >         else {
> >             Begin(mode);
> >             for (int i = 0; i < count; i++)
> >                 ArrayElement(first + i);
> >             End();
> >         }"
> >
> > Combining that with the language previously quoted, I think this
> > change is right.
> 
> Well, the code in question is
> 
>       for (j = start; j < count-3; j += 4) {
>          void *tmp = ALLOC_VERTS( 6 );
>          /* Send v0, v1, v3
>           */
>          tmp = EMIT_VERTS(ctx, j,     2, tmp);
>          tmp = EMIT_VERTS(ctx, j + 3, 1, tmp);
>          /* Send v1, v2, v3
>           */
>          tmp = EMIT_VERTS(ctx, j + 1, 3, tmp);
>          (void) tmp;
>       }
> 
> If count worked the way you're suggesting, then this would never work for
> start != 0. I think "count" is really "end" in this case. Here is one of the
> callers of this function:
> 
> tab[prim & PRIM_MODE_MASK]( ctx, start, start + length, prim );
> 
> The fact that the variable is called 'count' is actively misleading of course,
> but that doesn't make the code any more right. The HAVE_QUADS and HAVE_ELTS
> cases both have:
> 
>       count -= (count-start)%4;
> 
> which I believe further confirms my analysis.

Definitely you are right here, It was escaped, I have to consider also the start variable! 

> 
>   -ilia
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev