[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
> Eirik Byrkjeflot Anonsen
> Sent: Saturday, September 12, 2015 8:54 AM
> To: Ilia Mirkin; 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
> 
> Ilia Mirkin <imirkin at alum.mit.edu> writes:
> 
> > 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.
> 
> So we have three main branches and one failure branch, and all three main
> branches tries to do the exact same thing (looping from start to count in
> steps of 4). Maybe the common stuff should be moved out of the branches?
> 
> (Actually, the first two also differ in their style of setting dmasz and
> currentsz to a multiple of 4. Maybe they should be made consistent for easier
> reading?)
> 

On HAVE_ELTS branches we have 
count -= (count-start) & 3; so it can’t be general, or something escape to me?



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