Re: [Mesa-dev] [PATCH v3] i915: fixing driver crashes if too few vertices are submitted
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 writes: > > > > > On Fri, Sep 11, 2015 at 10:45 PM, Ian Romanick > > > > wrote: > > >> On 09/11/2015 10:55 AM, Ilia Mirkin wrote: > > >>> On Fri, Sep 11, 2015 at 12:36 PM, Marius Predut > > >>> > > 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 > > >>>> --- > > >>>> 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
Re: [Mesa-dev] [PATCH v3] i915: fixing driver crashes if too few vertices are submitted
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-boun...@lists.freedesktop.org] On Behalf Of
> Predut, Marius
> Sent: Monday, September 14, 2015 11:25 AM
> To: Ian Romanick; Ilia Mirkin
> 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
>
> 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:i...@freedesktop.org]
> > Sent: Saturday, September 12, 2015 4:46 AM
> > To: Ilia Mirkin; Predut, Marius
> > 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
> >
> > On 09/11/2015 10:55 AM, Ilia Mirkin wrote:
> > > On Fri, Sep 11, 2015 at 12:36 PM, Marius Predut
> > >
> > 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
> > >> ---
> > >> 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 qu
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 writes: > > > > > On Fri, Sep 11, 2015 at 10:45 PM, Ian Romanick > wrote: > > >> On 09/11/2015 10:55 AM, Ilia Mirkin wrote: > > >>> On Fri, Sep 11, 2015 at 12:36 PM, Marius Predut > > >>> > > 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 > > >>>> --- > > >>>> 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 > > >>
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:i...@freedesktop.org]
> Sent: Saturday, September 12, 2015 4:46 AM
> To: Ilia Mirkin; Predut, Marius
> 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
>
> On 09/11/2015 10:55 AM, Ilia Mirkin wrote:
> > On Fri, Sep 11, 2015 at 12:36 PM, Marius Predut
> 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
> >> ---
> >> 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@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
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
> Ilia Mirkin
> Sent: Saturday, September 12, 2015 8:40 AM
> To: 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
>
> On Fri, Sep 11, 2015 at 10:45 PM, Ian Romanick wrote:
> > On 09/11/2015 10:55 AM, Ilia Mirkin wrote:
> >> On Fri, Sep 11, 2015 at 12:36 PM, Marius Predut
> 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
> >>> ---
> >>> 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
> 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
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 writes:
>
> > On Fri, Sep 11, 2015 at 10:45 PM, Ian Romanick wrote:
> >> On 09/11/2015 10:55 AM, Ilia Mirkin wrote:
> >>> On Fri, Sep 11, 2015 at 12:36 PM, Marius Predut
> 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
> >>>> ---
> >>>> 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) {
> >
Re: [Mesa-dev] [PATCH v3] i915: fixing driver crashes if too few vertices are submitted
Ilia Mirkin writes:
> On Fri, Sep 11, 2015 at 10:45 PM, Ian Romanick wrote:
>> On 09/11/2015 10:55 AM, Ilia Mirkin wrote:
>>> On Fri, Sep 11, 2015 at 12:36 PM, Marius Predut
>>> 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
---
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?)
eirik
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
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 wrote:
> On 09/11/2015 10:55 AM, Ilia Mirkin wrote:
>> On Fri, Sep 11, 2015 at 12:36 PM, Marius Predut
>> 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
>>> ---
>>> 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.
-ilia
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
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
> 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
>> ---
>> 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@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
Re: [Mesa-dev] [PATCH v3] i915: fixing driver crashes if too few vertices are submitted
On Fri, Sep 11, 2015 at 12:36 PM, Marius Predut 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
> ---
> 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. 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:
/* 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@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] [PATCH v3] i915: fixing driver crashes if too few vertices are submitted
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
---
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;
+ if(count == 0) return;
+
INIT(GL_TRIANGLES);
for (j = start; j < count-3; j += 4) {
--
1.9.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
