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

2015-09-11 Thread Predut, Marius 


> -Original Message-
> From: Ian Romanick [mailto:i...@freedesktop.org]
> Sent: Wednesday, September 09, 2015 8:54 PM
> To: Predut, Marius; mesa-dev@lists.freedesktop.org
> Subject: Re: [Mesa-dev] [PATCH v2] i915: fixing driver crashes if too few
> vertices are submitted
> 
> On 09/09/2015 11:16 AM, 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.
> 
> I don't think you really did...
> 
> > 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 | 5 -
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/mesa/tnl_dd/t_dd_dmatmp.h b/src/mesa/tnl_dd/t_dd_dmatmp.h
> > index 7be3954..79de224 100644
> > --- a/src/mesa/tnl_dd/t_dd_dmatmp.h
> > +++ b/src/mesa/tnl_dd/t_dd_dmatmp.h
> > @@ -627,6 +627,9 @@ static void TAG(render_quads_verts)( struct gl_context
> *ctx,
> >LOCAL_VARS;
> >GLuint j;
> >
> > +  if(count % 4 != 0)
>^
> ...because I'm quite sure Brian's code had a space here, per Mesa's
> coding standards.

 So code style is:   if( count % 4 != 0 )? 
I think Brian used:  if(count % 4 != 0)

> 
> Also, this change is incorrect.  If an application does
> glDrawArrays(GL_QUADS, 0, (n * 4) + 3), this change will cause zero
> quads to be drawn when n quads should be drawn.
> 
> 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."
> 
> We probably don't have a piglit test for this scenario, so one should be
> added.  You can CC me on that patch. :)


Ok

> 
> I think the correct change is to trim count such that (count % 4) == 0.
>  If the modified value of count is zero, bail out.  With that change,
> the other hunk (below) is unnecessary.
> 
> > +  return;
> > +
> >INIT(GL_TRIANGLES);
> >
> >for (j = start; j < count-3; j += 4) {
> > @@ -1248,7 +1251,7 @@ static GLboolean TAG(validate_render)( struct
> gl_context *ctx,
> > ok = (GLint) count < GET_SUBSEQUENT_VB_MAX_ELTS();
> >  }
> >  else {
> > -   ok = HAVE_TRIANGLES; /* flatshading is ok. */
> > +   ok = HAVE_TRIANGLES && (count % 4 == 0); /* flatshading is ok. */
> >  }
> >  break;
> >default:
> >

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2015-09-11 Thread Ian Romanick 
On 09/11/2015 03:41 AM, Predut, Marius wrote:
> 
> 
>> -Original Message-
>> From: Ian Romanick [mailto:i...@freedesktop.org]
>> Sent: Wednesday, September 09, 2015 8:54 PM
>> To: Predut, Marius; mesa-dev@lists.freedesktop.org
>> Subject: Re: [Mesa-dev] [PATCH v2] i915: fixing driver crashes if too few
>> vertices are submitted
>>
>> On 09/09/2015 11:16 AM, 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.
>>
>> I don't think you really did...
>>
>>> 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 | 5 -
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/mesa/tnl_dd/t_dd_dmatmp.h b/src/mesa/tnl_dd/t_dd_dmatmp.h
>>> index 7be3954..79de224 100644
>>> --- a/src/mesa/tnl_dd/t_dd_dmatmp.h
>>> +++ b/src/mesa/tnl_dd/t_dd_dmatmp.h
>>> @@ -627,6 +627,9 @@ static void TAG(render_quads_verts)( struct gl_context
>> *ctx,
>>>LOCAL_VARS;
>>>GLuint j;
>>>
>>> +  if(count % 4 != 0)
>>^
>> ...because I'm quite sure Brian's code had a space here, per Mesa's
>> coding standards.
> 
>  So code style is:   if( count % 4 != 0 )? 
> I think Brian used:  if(count % 4 != 0)

I will copy and paste directly from Brian's e-mail.  I don't understand
why this is difficult.

if (count % 4 != 0)
   return

Space between "if" and the opening parenthesis.

Spaces around binary operators.

>> Also, this change is incorrect.  If an application does
>> glDrawArrays(GL_QUADS, 0, (n * 4) + 3), this change will cause zero
>> quads to be drawn when n quads should be drawn.
>>
>> 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."
>>
>> We probably don't have a piglit test for this scenario, so one should be
>> added.  You can CC me on that patch. :)
> 
> 
> Ok
> 
>>
>> I think the correct change is to trim count such that (count % 4) == 0.
>>  If the modified value of count is zero, bail out.  With that change,
>> the other hunk (below) is unnecessary.
>>
>>> +  return;
>>> +
>>>INIT(GL_TRIANGLES);
>>>
>>>for (j = start; j < count-3; j += 4) {
>>> @@ -1248,7 +1251,7 @@ static GLboolean TAG(validate_render)( struct
>> gl_context *ctx,
>>> ok = (GLint) count < GET_SUBSEQUENT_VB_MAX_ELTS();
>>>  }
>>>  else {
>>> -   ok = HAVE_TRIANGLES; /* flatshading is ok. */
>>> +   ok = HAVE_TRIANGLES && (count % 4 == 0); /* flatshading is ok. */
>>>  }
>>>  break;
>>>default:

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2015-09-09 Thread Ville Syrjälä 
On Wed, Sep 09, 2015 at 11:53:54AM -0700, Ian Romanick wrote:
> On 09/09/2015 11:16 AM, 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.
> 
> I don't think you really did...
> 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=38109
> > Signed-off-by: Marius Predut 
> > ---
> >  src/mesa/tnl_dd/t_dd_dmatmp.h | 5 -
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/mesa/tnl_dd/t_dd_dmatmp.h b/src/mesa/tnl_dd/t_dd_dmatmp.h
> > index 7be3954..79de224 100644
> > --- a/src/mesa/tnl_dd/t_dd_dmatmp.h
> > +++ b/src/mesa/tnl_dd/t_dd_dmatmp.h
> > @@ -627,6 +627,9 @@ static void TAG(render_quads_verts)( struct gl_context 
> > *ctx,
> >LOCAL_VARS;
> >GLuint j;
> >  
> > +  if(count % 4 != 0)
>^
> ...because I'm quite sure Brian's code had a space here, per Mesa's
> coding standards.
> 
> Also, this change is incorrect.  If an application does
> glDrawArrays(GL_QUADS, 0, (n * 4) + 3), this change will cause zero
> quads to be drawn when n quads should be drawn.

Due to the other check added to validate_render() we would never even
get here since we would have dropped off the fast path entrirely. Which
is not a nice thing to do simply due to an extra vertex hanging around
somewhere.

> 
> 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."
> 
> We probably don't have a piglit test for this scenario, so one should be
> added.  You can CC me on that patch. :)
> 
> I think the correct change is to trim count such that (count % 4) == 0.
>  If the modified value of count is zero, bail out.  With that change,
> the other hunk (below) is unnecessary.

I would suggest having one function that gets passed the primitive and
vertex count and have it return the trimmed vertex count. That could
then be called from both intel_run_render() and and TAG(validate_render())
to skip any extra vertices. That way none of the actual render functions
in t_dd_dmatmp.h need worry about any extra vertices.

> 
> > +  return;
> > +
> >INIT(GL_TRIANGLES);
> >  
> >for (j = start; j < count-3; j += 4) {
> > @@ -1248,7 +1251,7 @@ static GLboolean TAG(validate_render)( struct 
> > gl_context *ctx,
> > ok = (GLint) count < GET_SUBSEQUENT_VB_MAX_ELTS();
> >  }
> >  else {
> > -   ok = HAVE_TRIANGLES; /* flatshading is ok. */
> > +   ok = HAVE_TRIANGLES && (count % 4 == 0); /* flatshading is ok. */
> >  }
> >  break;
> >default:
> > 
> 
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev

-- 
Ville Syrjälä
Intel OTC
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2015-09-09 Thread Ian Romanick 
On 09/09/2015 11:53 AM, Ian Romanick wrote:
> On 09/09/2015 11:16 AM, 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.
> 
> I don't think you really did...
> 
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=38109
>> Signed-off-by: Marius Predut 
>> ---
>>  src/mesa/tnl_dd/t_dd_dmatmp.h | 5 -
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/mesa/tnl_dd/t_dd_dmatmp.h b/src/mesa/tnl_dd/t_dd_dmatmp.h
>> index 7be3954..79de224 100644
>> --- a/src/mesa/tnl_dd/t_dd_dmatmp.h
>> +++ b/src/mesa/tnl_dd/t_dd_dmatmp.h
>> @@ -627,6 +627,9 @@ static void TAG(render_quads_verts)( struct gl_context 
>> *ctx,
>>LOCAL_VARS;
>>GLuint j;
>>  
>> +  if(count % 4 != 0)
>^
> ...because I'm quite sure Brian's code had a space here, per Mesa's
> coding standards.
> 
> Also, this change is incorrect.  If an application does
> glDrawArrays(GL_QUADS, 0, (n * 4) + 3), this change will cause zero
> quads to be drawn when n quads should be drawn.
> 
> 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."
> 
> We probably don't have a piglit test for this scenario, so one should be
> added.  You can CC me on that patch. :)
> 
> I think the correct change is to trim count such that (count % 4) == 0.

Also... wherever you add this trim code, you should include the spec
quotation that I mentioned above.

>  If the modified value of count is zero, bail out.  With that change,
> the other hunk (below) is unnecessary.
> 
>> +  return;
>> +
>>INIT(GL_TRIANGLES);
>>  
>>for (j = start; j < count-3; j += 4) {
>> @@ -1248,7 +1251,7 @@ static GLboolean TAG(validate_render)( struct 
>> gl_context *ctx,
>>  ok = (GLint) count < GET_SUBSEQUENT_VB_MAX_ELTS();
>>   }
>>   else {
>> -ok = HAVE_TRIANGLES; /* flatshading is ok. */
>> +ok = HAVE_TRIANGLES && (count % 4 == 0); /* flatshading is ok. */
>>   }
>>   break;
>>default:
> 
> ___
> 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 v2] i915: fixing driver crashes if too few vertices are submitted

2015-09-09 Thread Ian Romanick 
On 09/09/2015 11:16 AM, 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.

I don't think you really did...

> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=38109
> Signed-off-by: Marius Predut 
> ---
>  src/mesa/tnl_dd/t_dd_dmatmp.h | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/mesa/tnl_dd/t_dd_dmatmp.h b/src/mesa/tnl_dd/t_dd_dmatmp.h
> index 7be3954..79de224 100644
> --- a/src/mesa/tnl_dd/t_dd_dmatmp.h
> +++ b/src/mesa/tnl_dd/t_dd_dmatmp.h
> @@ -627,6 +627,9 @@ static void TAG(render_quads_verts)( struct gl_context 
> *ctx,
>LOCAL_VARS;
>GLuint j;
>  
> +  if(count % 4 != 0)
   ^
...because I'm quite sure Brian's code had a space here, per Mesa's
coding standards.

Also, this change is incorrect.  If an application does
glDrawArrays(GL_QUADS, 0, (n * 4) + 3), this change will cause zero
quads to be drawn when n quads should be drawn.

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."

We probably don't have a piglit test for this scenario, so one should be
added.  You can CC me on that patch. :)

I think the correct change is to trim count such that (count % 4) == 0.
 If the modified value of count is zero, bail out.  With that change,
the other hunk (below) is unnecessary.

> +  return;
> +
>INIT(GL_TRIANGLES);
>  
>for (j = start; j < count-3; j += 4) {
> @@ -1248,7 +1251,7 @@ static GLboolean TAG(validate_render)( struct 
> gl_context *ctx,
>   ok = (GLint) count < GET_SUBSEQUENT_VB_MAX_ELTS();
>}
>else {
> - ok = HAVE_TRIANGLES; /* flatshading is ok. */
> + ok = HAVE_TRIANGLES && (count % 4 == 0); /* flatshading is ok. */
>}
>break;
>default:
> 

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2015-09-09 Thread Marius Predut 
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.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=38109
Signed-off-by: Marius Predut 
---
 src/mesa/tnl_dd/t_dd_dmatmp.h | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/mesa/tnl_dd/t_dd_dmatmp.h b/src/mesa/tnl_dd/t_dd_dmatmp.h
index 7be3954..79de224 100644
--- a/src/mesa/tnl_dd/t_dd_dmatmp.h
+++ b/src/mesa/tnl_dd/t_dd_dmatmp.h
@@ -627,6 +627,9 @@ static void TAG(render_quads_verts)( struct gl_context *ctx,
   LOCAL_VARS;
   GLuint j;
 
+  if(count % 4 != 0)
+  return;
+
   INIT(GL_TRIANGLES);
 
   for (j = start; j < count-3; j += 4) {
@@ -1248,7 +1251,7 @@ static GLboolean TAG(validate_render)( struct gl_context 
*ctx,
ok = (GLint) count < GET_SUBSEQUENT_VB_MAX_ELTS();
 }
 else {
-   ok = HAVE_TRIANGLES; /* flatshading is ok. */
+   ok = HAVE_TRIANGLES && (count % 4 == 0); /* flatshading is ok. */
 }
 break;
   default:
-- 
1.9.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev