[Mesa-dev] [PATCH v3] i915: fixing driver crashes if too few vertices are submitted
Predut, Marius
marius.predut at intel.com
Mon Sep 14 07:26:29 PDT 2015
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 at 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-bounces at 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 at 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-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
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list