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

[Predut, Marius]

Or the best way to fix it ...
outside branches add :
"
count -= (count-start) & 3;
if (cont == 0)
   return;
"
and inside branches remove:
" count -= (count-start)%4; "
and " count -= (count-start) & 3; "

?


> -----Original Message-----
> From: mesa-dev [mailto:mesa-dev-bounces at lists.freedesktop.org] On Behalf Of
> Predut, Marius
> Sent: Monday, September 14, 2015 11:25 AM
> To: Ian Romanick; Ilia Mirkin
> 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
> 
> Ok ,
> So I propose this code to be included into the patch:
> 
> count -= (count - start) % 4;
> if (cont == 0)
>    return;
> 
> What do you think?
> 
> 
> 
> > -----Original Message-----
> > From: Ian Romanick [mailto:idr at freedesktop.org]
> > Sent: Saturday, September 12, 2015 4:46 AM
> > To: Ilia Mirkin; Predut, Marius
> > 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 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.
> >
> > > 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.
> >
> > >       /* Emit whole number of quads in total.  dmasz is already a multiple
> > >        * of 4.
> > >        */
> > >       count -= (count-start)%4;
> > >
> > > Which seems more accurate.
> > >
> > >> +      if(count == 0) return;
> > >
> > > if (count == 0)
> > >
> > > note the space. That's the style used in all of mesa.
> > >
> > > $ git grep '\sif(' | wc -l
> > > 1076
> > > $ git grep '\sif (' | wc -l
> > > 58071
> > >
> > > I guess a few 'if(' instances snuck through, mostly in src/gallium.
> > > But the overwhelming majority are of the 'if (' style.
> > >
> > >> +
> > >>        INIT(GL_TRIANGLES);
> > >>
> > >>        for (j = start; j < count-3; j += 4) {
> > >> --
> > >> 1.9.1
> > >>
> > >> _______________________________________________
> > >> mesa-dev mailing list
> > >> mesa-dev at lists.freedesktop.org
> > >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > >>
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev