Eirik, Apologize for typo. -----Original Message----- From: Predut, Marius Sent: Monday, September 14, 2015 12:32 PM To: Predut, Marius; Eirik Byrkjeflot Anonsen; Ilia Mirkin; Ian Romanick Cc: mesa-dev@lists.freedesktop.org; Romanick, Ian D Subject: RE: [Mesa-dev] [PATCH v3] i915: fixing driver crashes if too few vertices are submitted
Sorry Eric, Now I see the formulas are equivalent , So I can move it outside branches :=) > -----Original Message----- > From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On > Behalf Of Predut, Marius > Sent: Monday, September 14, 2015 11:18 AM > To: Eirik Byrkjeflot Anonsen; Ilia Mirkin; Ian Romanick > Cc: mesa-dev@lists.freedesktop.org; Romanick, Ian D > Subject: Re: [Mesa-dev] [PATCH v3] i915: fixing driver crashes if too > few vertices are submitted > > > > -----Original Message----- > > From: mesa-dev [mailto:mesa-dev-boun...@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@lists.freedesktop.org; Romanick, Ian D > > Subject: Re: [Mesa-dev] [PATCH v3] i915: fixing driver crashes if > > too few vertices are submitted > > > > Ilia Mirkin <imir...@alum.mit.edu> writes: > > > > > On Fri, Sep 11, 2015 at 10:45 PM, Ian Romanick > > > <i...@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.pre...@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.pre...@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@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
