Re: [Mesa-dev] [PATCH] mesa: Clamp GetUniformuiv values to be >= 0.

2017-02-17 Thread Nicolai Hähnle 

On 16.02.2017 20:24, Antía Puentes wrote:

On lun, 2016-12-12 at 10:43 +0100, Nicolai Hähnle wrote:

On 12.12.2016 00:25, Kenneth Graunke wrote:


Section 2.2.2 (Data Conversions For State Query Commands) of the
OpenGL 4.5 October 24th 2016 specification says:

"If a command returning unsigned integer data is called, such as
 GetSamplerParameterIuiv, negative values are clamped to zero."

Fixes GL44-CTS.gpu_shader_fp64.state_query.

Signed-off-by: Kenneth Graunke 
---
 src/mesa/main/uniform_query.cpp | 48
+
 1 file changed, 39 insertions(+), 9 deletions(-)

Hey Nicolai,

I wrote a similar patch a while back, but never got around to
sending it,
since I realized that the gl45release branch expects our current
behavior,
and the change to make the CTS expect clamping is only on the
master branch.

Apparently I made some additional changes, compared to yours.  I
figured
I'd send this along and let you see if you think any of my extra
changes
are still necessary.  If so, feel free to fold them into your
patch.

I also think we need to fix several other glGet* commands...it's
just that
this is the only one currently tested.  A bunch work because the
values
returned can't be negative.

I think your patch is a strict superset of what mine does and should
be
used instead. I do have one comment below, with that fixed it has my
R-b.


This patch was never pushed, was it? and GL45-CTS.gpu_shader_fp64.state_query
fails in the new vk-gl-cts repository because it expects these negative
values to be clamped.


Fine with me. But again, take Ken's patch, not mine :)

Cheers,
Nicolai
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] mesa: Clamp GetUniformuiv values to be >= 0.

2017-02-16 Thread Antía Puentes 
On lun, 2016-12-12 at 10:43 +0100, Nicolai Hähnle wrote:
> On 12.12.2016 00:25, Kenneth Graunke wrote:
> > 
> > Section 2.2.2 (Data Conversions For State Query Commands) of the
> > OpenGL 4.5 October 24th 2016 specification says:
> > 
> > "If a command returning unsigned integer data is called, such as
> >  GetSamplerParameterIuiv, negative values are clamped to zero."
> > 
> > Fixes GL44-CTS.gpu_shader_fp64.state_query.
> > 
> > Signed-off-by: Kenneth Graunke 
> > ---
> >  src/mesa/main/uniform_query.cpp | 48
> > +
> >  1 file changed, 39 insertions(+), 9 deletions(-)
> > 
> > Hey Nicolai,
> > 
> > I wrote a similar patch a while back, but never got around to
> > sending it,
> > since I realized that the gl45release branch expects our current
> > behavior,
> > and the change to make the CTS expect clamping is only on the
> > master branch.
> > 
> > Apparently I made some additional changes, compared to yours.  I
> > figured
> > I'd send this along and let you see if you think any of my extra
> > changes
> > are still necessary.  If so, feel free to fold them into your
> > patch.
> > 
> > I also think we need to fix several other glGet* commands...it's
> > just that
> > this is the only one currently tested.  A bunch work because the
> > values
> > returned can't be negative.
> I think your patch is a strict superset of what mine does and should
> be 
> used instead. I do have one comment below, with that fixed it has my
> R-b.

This patch was never pushed, was it? and GL45-CTS.gpu_shader_fp64.state_query
fails in the new vk-gl-cts repository because it expects these negative
values to be clamped.

> There is the more general question of how to cope with those cases
> where 
> the CTS requires non-standard behavior. I think we should insist on 
> doing the right thing in Mesa, and push for changes to the CTS.
> 
> Until quite recently, I've been occupied by radeonsi- and 
> Gallium-specific bugs, but that's changing and I'm looking into
> using 
> CTS master rather than back-porting fixes to the dead gl45release
> branch 
> (hence this patch).
> 
> > 
> > 
> >  --Ken
> > 
> > diff --git a/src/mesa/main/uniform_query.cpp
> > b/src/mesa/main/uniform_query.cpp
> > index db700df..f05a29f 100644
> > --- a/src/mesa/main/uniform_query.cpp
> > +++ b/src/mesa/main/uniform_query.cpp
> > @@ -347,14 +347,10 @@ _mesa_get_uniform(struct gl_context *ctx,
> > GLuint program, GLint location,
> > * just memcpy the data.  If the types are not compatible,
> > perform a
> > * slower convert-and-copy process.
> > */
> > -  if (returnType == uni->type->base_type
> > -     || ((returnType == GLSL_TYPE_INT
> > -      || returnType == GLSL_TYPE_UINT)
> > -     &&
> > -     (uni->type->base_type == GLSL_TYPE_INT
> > -      || uni->type->base_type == GLSL_TYPE_UINT
> > -   || uni->type->base_type == GLSL_TYPE_SAMPLER
> > -   || uni->type->base_type == GLSL_TYPE_IMAGE))) {
> > +  if (returnType == uni->type->base_type ||
> > +  ((returnType == GLSL_TYPE_INT || returnType ==
> > GLSL_TYPE_UINT) &&
> > +   (uni->type->base_type == GLSL_TYPE_SAMPLER ||
> > +uni->type->base_type == GLSL_TYPE_IMAGE))) {
> >      memcpy(paramsOut, src, bytes);
> >    } else {
> >      union gl_constant_value *const dst =
> > @@ -422,7 +418,6 @@ _mesa_get_uniform(struct gl_context *ctx,
> > GLuint program, GLint location,
> >        }
> >        break;
> >     case GLSL_TYPE_INT:
> > -   case GLSL_TYPE_UINT:
> >        switch (uni->type->base_type) {
> >        case GLSL_TYPE_FLOAT:
> >       /* While the GL 3.2 core spec doesn't explicitly
> > @@ -447,6 +442,9 @@ _mesa_get_uniform(struct gl_context *ctx,
> > GLuint program, GLint location,
> >        case GLSL_TYPE_BOOL:
> >       dst[didx].i = src[sidx].i ? 1 : 0;
> >       break;
> > +   case GLSL_TYPE_UINT:
> > +  dst[didx].i = src[sidx].i;
> I think this should be
> 
> dst[didx].i = MIN2(src[sidx].u, INT_MAX);
> 
> Cheers,
> Nicolai
> 
> > 
> > +  break;
> > case GLSL_TYPE_DOUBLE: {
> >    double tmp;
> >    memcpy(, [sidx].f, sizeof(tmp));
> > @@ -458,6 +456,38 @@ _mesa_get_uniform(struct gl_context *ctx,
> > GLuint program, GLint location,
> >       break;
> >        }
> >        break;
> > +case GLSL_TYPE_UINT:
> > +   switch (uni->type->base_type) {
> > +   case GLSL_TYPE_FLOAT:
> > +  /* The spec isn't terribly clear how to handle
> > negative
> > +   * values with an unsigned return type.
> > +   *
> > +   * GL 4.5 section 2.2.2 ("Data Conversions for
> > State
> > +   * Query Commands") says:
> > +   *
> > +   * "If a value is so

Re: [Mesa-dev] [PATCH] mesa: Clamp GetUniformuiv values to be >= 0.

2016-12-12 Thread Nicolai Hähnle 

On 12.12.2016 00:25, Kenneth Graunke wrote:

Section 2.2.2 (Data Conversions For State Query Commands) of the
OpenGL 4.5 October 24th 2016 specification says:

"If a command returning unsigned integer data is called, such as
 GetSamplerParameterIuiv, negative values are clamped to zero."

Fixes GL44-CTS.gpu_shader_fp64.state_query.

Signed-off-by: Kenneth Graunke 
---
 src/mesa/main/uniform_query.cpp | 48 +
 1 file changed, 39 insertions(+), 9 deletions(-)

Hey Nicolai,

I wrote a similar patch a while back, but never got around to sending it,
since I realized that the gl45release branch expects our current behavior,
and the change to make the CTS expect clamping is only on the master branch.

Apparently I made some additional changes, compared to yours.  I figured
I'd send this along and let you see if you think any of my extra changes
are still necessary.  If so, feel free to fold them into your patch.

I also think we need to fix several other glGet* commands...it's just that
this is the only one currently tested.  A bunch work because the values
returned can't be negative.


I think your patch is a strict superset of what mine does and should be 
used instead. I do have one comment below, with that fixed it has my R-b.


There is the more general question of how to cope with those cases where 
the CTS requires non-standard behavior. I think we should insist on 
doing the right thing in Mesa, and push for changes to the CTS.


Until quite recently, I've been occupied by radeonsi- and 
Gallium-specific bugs, but that's changing and I'm looking into using 
CTS master rather than back-porting fixes to the dead gl45release branch 
(hence this patch).




 --Ken

diff --git a/src/mesa/main/uniform_query.cpp b/src/mesa/main/uniform_query.cpp
index db700df..f05a29f 100644
--- a/src/mesa/main/uniform_query.cpp
+++ b/src/mesa/main/uniform_query.cpp
@@ -347,14 +347,10 @@ _mesa_get_uniform(struct gl_context *ctx, GLuint program, 
GLint location,
* just memcpy the data.  If the types are not compatible, perform a
* slower convert-and-copy process.
*/
-  if (returnType == uni->type->base_type
- || ((returnType == GLSL_TYPE_INT
-  || returnType == GLSL_TYPE_UINT)
- &&
- (uni->type->base_type == GLSL_TYPE_INT
-  || uni->type->base_type == GLSL_TYPE_UINT
-   || uni->type->base_type == GLSL_TYPE_SAMPLER
-   || uni->type->base_type == GLSL_TYPE_IMAGE))) {
+  if (returnType == uni->type->base_type ||
+  ((returnType == GLSL_TYPE_INT || returnType == GLSL_TYPE_UINT) &&
+   (uni->type->base_type == GLSL_TYPE_SAMPLER ||
+uni->type->base_type == GLSL_TYPE_IMAGE))) {
 memcpy(paramsOut, src, bytes);
   } else {
 union gl_constant_value *const dst =
@@ -422,7 +418,6 @@ _mesa_get_uniform(struct gl_context *ctx, GLuint program, 
GLint location,
   }
   break;
case GLSL_TYPE_INT:
-   case GLSL_TYPE_UINT:
   switch (uni->type->base_type) {
   case GLSL_TYPE_FLOAT:
  /* While the GL 3.2 core spec doesn't explicitly
@@ -447,6 +442,9 @@ _mesa_get_uniform(struct gl_context *ctx, GLuint program, 
GLint location,
   case GLSL_TYPE_BOOL:
  dst[didx].i = src[sidx].i ? 1 : 0;
  break;
+   case GLSL_TYPE_UINT:
+  dst[didx].i = src[sidx].i;


I think this should be

dst[didx].i = MIN2(src[sidx].u, INT_MAX);

Cheers,
Nicolai


+  break;
case GLSL_TYPE_DOUBLE: {
   double tmp;
   memcpy(, [sidx].f, sizeof(tmp));
@@ -458,6 +456,38 @@ _mesa_get_uniform(struct gl_context *ctx, GLuint program, 
GLint location,
  break;
   }
   break;
+case GLSL_TYPE_UINT:
+   switch (uni->type->base_type) {
+   case GLSL_TYPE_FLOAT:
+  /* The spec isn't terribly clear how to handle negative
+   * values with an unsigned return type.
+   *
+   * GL 4.5 section 2.2.2 ("Data Conversions for State
+   * Query Commands") says:
+   *
+   * "If a value is so large in magnitude that it cannot be
+   *  represented by the returned data type, then the nearest
+   *  value representable using the requested type is
+   *  returned."
+   */
+  dst[didx].i = src[sidx].f < 0.0f ? 0 : IROUND(src[sidx].f);
+  break;
+   case GLSL_TYPE_BOOL:
+  dst[didx].i = src[sidx].i ? 1 : 0;
+  break;
+   case GLSL_TYPE_INT:
+  dst[didx].i = MAX2(src[sidx].i, 0);
+  break;
+   case

[Mesa-dev] [PATCH] mesa: Clamp GetUniformuiv values to be >= 0.

2016-12-11 Thread Kenneth Graunke 
Section 2.2.2 (Data Conversions For State Query Commands) of the
OpenGL 4.5 October 24th 2016 specification says:

"If a command returning unsigned integer data is called, such as
 GetSamplerParameterIuiv, negative values are clamped to zero."

Fixes GL44-CTS.gpu_shader_fp64.state_query.

Signed-off-by: Kenneth Graunke 
---
 src/mesa/main/uniform_query.cpp | 48 +
 1 file changed, 39 insertions(+), 9 deletions(-)

Hey Nicolai,

I wrote a similar patch a while back, but never got around to sending it,
since I realized that the gl45release branch expects our current behavior,
and the change to make the CTS expect clamping is only on the master branch.

Apparently I made some additional changes, compared to yours.  I figured
I'd send this along and let you see if you think any of my extra changes
are still necessary.  If so, feel free to fold them into your patch.

I also think we need to fix several other glGet* commands...it's just that
this is the only one currently tested.  A bunch work because the values
returned can't be negative.

 --Ken

diff --git a/src/mesa/main/uniform_query.cpp b/src/mesa/main/uniform_query.cpp
index db700df..f05a29f 100644
--- a/src/mesa/main/uniform_query.cpp
+++ b/src/mesa/main/uniform_query.cpp
@@ -347,14 +347,10 @@ _mesa_get_uniform(struct gl_context *ctx, GLuint program, 
GLint location,
* just memcpy the data.  If the types are not compatible, perform a
* slower convert-and-copy process.
*/
-  if (returnType == uni->type->base_type
- || ((returnType == GLSL_TYPE_INT
-  || returnType == GLSL_TYPE_UINT)
- &&
- (uni->type->base_type == GLSL_TYPE_INT
-  || uni->type->base_type == GLSL_TYPE_UINT
-   || uni->type->base_type == GLSL_TYPE_SAMPLER
-   || uni->type->base_type == GLSL_TYPE_IMAGE))) {
+  if (returnType == uni->type->base_type ||
+  ((returnType == GLSL_TYPE_INT || returnType == GLSL_TYPE_UINT) &&
+   (uni->type->base_type == GLSL_TYPE_SAMPLER ||
+uni->type->base_type == GLSL_TYPE_IMAGE))) {
 memcpy(paramsOut, src, bytes);
   } else {
 union gl_constant_value *const dst =
@@ -422,7 +418,6 @@ _mesa_get_uniform(struct gl_context *ctx, GLuint program, 
GLint location,
   }
   break;
case GLSL_TYPE_INT:
-   case GLSL_TYPE_UINT:
   switch (uni->type->base_type) {
   case GLSL_TYPE_FLOAT:
  /* While the GL 3.2 core spec doesn't explicitly
@@ -447,6 +442,9 @@ _mesa_get_uniform(struct gl_context *ctx, GLuint program, 
GLint location,
   case GLSL_TYPE_BOOL:
  dst[didx].i = src[sidx].i ? 1 : 0;
  break;
+   case GLSL_TYPE_UINT:
+  dst[didx].i = src[sidx].i;
+  break;
case GLSL_TYPE_DOUBLE: {
   double tmp;
   memcpy(, [sidx].f, sizeof(tmp));
@@ -458,6 +456,38 @@ _mesa_get_uniform(struct gl_context *ctx, GLuint program, 
GLint location,
  break;
   }
   break;
+case GLSL_TYPE_UINT:
+   switch (uni->type->base_type) {
+   case GLSL_TYPE_FLOAT:
+  /* The spec isn't terribly clear how to handle negative
+   * values with an unsigned return type.
+   *
+   * GL 4.5 section 2.2.2 ("Data Conversions for State
+   * Query Commands") says:
+   *
+   * "If a value is so large in magnitude that it cannot be
+   *  represented by the returned data type, then the nearest
+   *  value representable using the requested type is
+   *  returned."
+   */
+  dst[didx].i = src[sidx].f < 0.0f ? 0 : IROUND(src[sidx].f);
+  break;
+   case GLSL_TYPE_BOOL:
+  dst[didx].i = src[sidx].i ? 1 : 0;
+  break;
+   case GLSL_TYPE_INT:
+  dst[didx].i = MAX2(src[sidx].i, 0);
+  break;
+   case GLSL_TYPE_DOUBLE: {
+  double tmp;
+  memcpy(, [sidx].f, sizeof(tmp));
+  dst[didx].i = tmp < 0.0 ? 0 : IROUNDD(tmp);
+  break;
+   }
+   default:
+  unreachable("invalid uniform type");
+   }
+   break;
 
default:
   assert(!"Should not get here.");
-- 
2.10.2

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