> -----Original Message----- > From: mesa-dev [mailto:[email protected]] On Behalf Of > Ilia Mirkin > Sent: Saturday, September 12, 2015 8:40 AM > To: Ian Romanick > Cc: [email protected]; 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 <[email protected]> wrote: > > On 09/11/2015 10:55 AM, Ilia Mirkin wrote: > >> On Fri, Sep 11, 2015 at 12:36 PM, Marius Predut <[email protected]> > 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 <[email protected]> > >>> --- > >>> 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 > [email protected] > http://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
