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

[Predut, Marius]

> -Original Message-
> From: Eirik Byrkjeflot Anonsen [mailto:ei...@eirikba.org]
> Sent: Wednesday, September 09, 2015 9:18 PM
> To: Predut, Marius; mesa-dev@lists.freedesktop.org
> Subject: Re: [Mesa-dev] [PATCH v1] i915: fixing driver crashes if too few
> vertices are submitted
> 
> Marius Predut <marius.pre...@intel.com> writes:
> 
> > 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.
> >
> > 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 | 4 +++-
> >  1 file changed, 3 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..88ecc78 100644
> > --- a/src/mesa/tnl_dd/t_dd_dmatmp.h
> > +++ b/src/mesa/tnl_dd/t_dd_dmatmp.h
> > @@ -627,6 +627,8 @@ static void TAG(render_quads_verts)( struct gl_context
> *ctx,
> >LOCAL_VARS;
> >GLuint j;
> >
> > +  if(count%4 != 0) return;
> > +
> 
> Seems to me that does a bit more than just fixing the crash. I would think
> 
> if (count < 4)
> 
> would fix the crash only. (And then quite possibly you won't need the next
> hunk?) But I have no idea whether the side effect is desired.
> 
> (Actually, what if count is 0? Or is that impossible?)

No count can't be 0 because is check filtered before in validate_render

> 
> eirik
> 
> >INIT(GL_TRIANGLES);
> >
> >for (j = start; j < count-3; j += 4) { @@ -1248,7 +1250,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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

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

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=38109
Signed-off-by: Marius Predut 
---
 src/mesa/tnl_dd/t_dd_dmatmp.h | 4 +++-
 1 file changed, 3 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..88ecc78 100644
--- a/src/mesa/tnl_dd/t_dd_dmatmp.h
+++ b/src/mesa/tnl_dd/t_dd_dmatmp.h
@@ -627,6 +627,8 @@ 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 +1250,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

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

[Brian Paul]

On 09/09/2015 08: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.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=38109
Signed-off-by: Marius Predut 
---
  src/mesa/tnl_dd/t_dd_dmatmp.h | 4 +++-
  1 file changed, 3 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..88ecc78 100644
--- a/src/mesa/tnl_dd/t_dd_dmatmp.h
+++ b/src/mesa/tnl_dd/t_dd_dmatmp.h
@@ -627,6 +627,8 @@ static void TAG(render_quads_verts)( struct gl_context *ctx,
LOCAL_VARS;
GLuint j;

+  if(count%4 != 0) return;


Should be:

   if (count % 4 != 0)
  return

In addition to following our code style, this allows one to put a 
breakpoint on the return statement if one wanted to catch that 
particular condition when debugging.




+
INIT(GL_TRIANGLES);

for (j = start; j < count-3; j += 4) {
@@ -1248,7 +1250,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. */


We put spaces around operators, so "count % 4" please.



 }
 break;
default:



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

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

[Eirik Byrkjeflot Anonsen]

Marius Predut  writes:

> 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.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=38109
> Signed-off-by: Marius Predut 
> ---
>  src/mesa/tnl_dd/t_dd_dmatmp.h | 4 +++-
>  1 file changed, 3 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..88ecc78 100644
> --- a/src/mesa/tnl_dd/t_dd_dmatmp.h
> +++ b/src/mesa/tnl_dd/t_dd_dmatmp.h
> @@ -627,6 +627,8 @@ static void TAG(render_quads_verts)( struct gl_context 
> *ctx,
>LOCAL_VARS;
>GLuint j;
>  
> +  if(count%4 != 0) return;
> +

Seems to me that does a bit more than just fixing the crash. I would
think

if (count < 4)

would fix the crash only. (And then quite possibly you won't need the
next hunk?) But I have no idea whether the side effect is desired.

(Actually, what if count is 0? Or is that impossible?)

eirik

>INIT(GL_TRIANGLES);
>  
>for (j = start; j < count-3; j += 4) {
> @@ -1248,7 +1250,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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev