Re: [Mesa-dev] [PATCH] mesa: fix ARRAY_SIZE query for GetProgramResourceiv

2015-09-29 Thread Emil Velikov 
Hi guys,

On 29 September 2015 at 10:30, Martin Peres
 wrote:
> On 28/09/15 13:51, Tapani Pälli wrote:
>>
>> Patch also refactors name length queries which were using array size
>> in computation, this has to be done in same time to avoid regression in
>> arb_program_interface_query-resource-query Piglit test.
>>
>> Fixes rest of the failures with
>> ES31-CTS.program_interface_query.no-locations
>>
>> v2: make additional check only for GS inputs
>> v3: create helper function for resource name length
>>  so that it gets calculated only in one place
>>
>> Signed-off-by: Tapani Pälli 
>> Reviewed-by: Martin Peres  [v2]
>> ---
>>   src/mesa/main/program_resource.c |  8 ++--
>>   src/mesa/main/shader_query.cpp   | 94
>> 
>>   src/mesa/main/shaderapi.h|  3 ++
>>   3 files changed, 62 insertions(+), 43 deletions(-)
>>
>> diff --git a/src/mesa/main/program_resource.c
>> b/src/mesa/main/program_resource.c
>> index c609abe..eb71fdd 100644
>> --- a/src/mesa/main/program_resource.c
>> +++ b/src/mesa/main/program_resource.c
>> @@ -111,11 +111,9 @@ _mesa_GetProgramInterfaceiv(GLuint program, GLenum
>> programInterface,
>> for (i = 0, *params = 0; i < shProg->NumProgramResourceList; i++)
>> {
>>if (shProg->ProgramResourceList[i].Type != programInterface)
>>   continue;
>> - const char *name =
>> -_mesa_program_resource_name(>ProgramResourceList[i]);
>> - unsigned array_size =
>> -
>> _mesa_program_resource_array_size(>ProgramResourceList[i]);
>> - *params = MAX2(*params, strlen(name) + (array_size ? 3 : 0) +
>> 1);
>> + unsigned len =
>> +
>> _mesa_program_resource_name_len(>ProgramResourceList[i]);
>> + *params = MAX2(*params, len + 1);
>> }
>> break;
>>  case GL_MAX_NUM_ACTIVE_VARIABLES:
>> diff --git a/src/mesa/main/shader_query.cpp
>> b/src/mesa/main/shader_query.cpp
>> index 99d9e10..a5572ff 100644
>> --- a/src/mesa/main/shader_query.cpp
>> +++ b/src/mesa/main/shader_query.cpp
>> @@ -478,7 +478,7 @@ _mesa_program_resource_array_size(struct
>> gl_program_resource *res)
>>RESOURCE_XFB(res)->Size : 0;
>>  case GL_PROGRAM_INPUT:
>>  case GL_PROGRAM_OUTPUT:
>> -  return RESOURCE_VAR(res)->data.max_array_access;
>> +  return RESOURCE_VAR(res)->type->length;
>>  case GL_UNIFORM:
>>  case GL_VERTEX_SUBROUTINE_UNIFORM:
>>  case GL_GEOMETRY_SUBROUTINE_UNIFORM:
>> @@ -670,6 +670,57 @@ _mesa_program_resource_find_index(struct
>> gl_shader_program *shProg,
>>  return NULL;
>>   }
>>   +/* Function returns if resource name is expected to have index
>> + * appended into it.
>> + *
>> + *
>> + * Page 61 (page 73 of the PDF) in section 2.11 of the OpenGL ES 3.0
>> + * spec says:
>> + *
>> + * "If the active uniform is an array, the uniform name returned in
>> + * name will always be the name of the uniform array appended with
>> + * "[0]"."
>> + *
>> + * The same text also appears in the OpenGL 4.2 spec.  It does not,
>> + * however, appear in any previous spec.  Previous specifications are
>> + * ambiguous in this regard.  However, either name can later be passed
>> + * to glGetUniformLocation (and related APIs), so there shouldn't be any
>> + * harm in always appending "[0]" to uniform array names.
>> + *
>> + * Geometry shader stage has different naming convention where the
>> 'normal'
>> + * condition is an array, therefore for variables referenced in geometry
>> + * stage we do not add '[0]'.
>> + *
>> + * Note, that TCS outputs and TES inputs should not have index appended
>> + * either.
>> + */
>> +bool
>> +add_index_to_name(struct gl_program_resource *res)
>
> static please!
>
> With this, you get:
> Reviewed-by: Martin Peres 
>
> Thanks for the cleanup!
>
Would this be mesa-stable worthy material ?

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

Re: [Mesa-dev] [PATCH] mesa: fix ARRAY_SIZE query for GetProgramResourceiv

2015-09-29 Thread Tapani Pälli 



On 09/29/2015 01:42 PM, Emil Velikov wrote:

Hi guys,

On 29 September 2015 at 10:30, Martin Peres
 wrote:

On 28/09/15 13:51, Tapani Pälli wrote:


Patch also refactors name length queries which were using array size
in computation, this has to be done in same time to avoid regression in
arb_program_interface_query-resource-query Piglit test.

Fixes rest of the failures with
 ES31-CTS.program_interface_query.no-locations

v2: make additional check only for GS inputs
v3: create helper function for resource name length
  so that it gets calculated only in one place

Signed-off-by: Tapani Pälli 
Reviewed-by: Martin Peres  [v2]
---
   src/mesa/main/program_resource.c |  8 ++--
   src/mesa/main/shader_query.cpp   | 94

   src/mesa/main/shaderapi.h|  3 ++
   3 files changed, 62 insertions(+), 43 deletions(-)

diff --git a/src/mesa/main/program_resource.c
b/src/mesa/main/program_resource.c
index c609abe..eb71fdd 100644
--- a/src/mesa/main/program_resource.c
+++ b/src/mesa/main/program_resource.c
@@ -111,11 +111,9 @@ _mesa_GetProgramInterfaceiv(GLuint program, GLenum
programInterface,
 for (i = 0, *params = 0; i < shProg->NumProgramResourceList; i++)
{
if (shProg->ProgramResourceList[i].Type != programInterface)
   continue;
- const char *name =
-_mesa_program_resource_name(>ProgramResourceList[i]);
- unsigned array_size =
-
_mesa_program_resource_array_size(>ProgramResourceList[i]);
- *params = MAX2(*params, strlen(name) + (array_size ? 3 : 0) +
1);
+ unsigned len =
+
_mesa_program_resource_name_len(>ProgramResourceList[i]);
+ *params = MAX2(*params, len + 1);
 }
 break;
  case GL_MAX_NUM_ACTIVE_VARIABLES:
diff --git a/src/mesa/main/shader_query.cpp
b/src/mesa/main/shader_query.cpp
index 99d9e10..a5572ff 100644
--- a/src/mesa/main/shader_query.cpp
+++ b/src/mesa/main/shader_query.cpp
@@ -478,7 +478,7 @@ _mesa_program_resource_array_size(struct
gl_program_resource *res)
RESOURCE_XFB(res)->Size : 0;
  case GL_PROGRAM_INPUT:
  case GL_PROGRAM_OUTPUT:
-  return RESOURCE_VAR(res)->data.max_array_access;
+  return RESOURCE_VAR(res)->type->length;
  case GL_UNIFORM:
  case GL_VERTEX_SUBROUTINE_UNIFORM:
  case GL_GEOMETRY_SUBROUTINE_UNIFORM:
@@ -670,6 +670,57 @@ _mesa_program_resource_find_index(struct
gl_shader_program *shProg,
  return NULL;
   }
   +/* Function returns if resource name is expected to have index
+ * appended into it.
+ *
+ *
+ * Page 61 (page 73 of the PDF) in section 2.11 of the OpenGL ES 3.0
+ * spec says:
+ *
+ * "If the active uniform is an array, the uniform name returned in
+ * name will always be the name of the uniform array appended with
+ * "[0]"."
+ *
+ * The same text also appears in the OpenGL 4.2 spec.  It does not,
+ * however, appear in any previous spec.  Previous specifications are
+ * ambiguous in this regard.  However, either name can later be passed
+ * to glGetUniformLocation (and related APIs), so there shouldn't be any
+ * harm in always appending "[0]" to uniform array names.
+ *
+ * Geometry shader stage has different naming convention where the
'normal'
+ * condition is an array, therefore for variables referenced in geometry
+ * stage we do not add '[0]'.
+ *
+ * Note, that TCS outputs and TES inputs should not have index appended
+ * either.
+ */
+bool
+add_index_to_name(struct gl_program_resource *res)


static please!

With this, you get:
Reviewed-by: Martin Peres 

Thanks for the cleanup!


Would this be mesa-stable worthy material ?


We haven't had bugs filed for this but I guess it's just matter of time. 
Yes please!


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

Re: [Mesa-dev] [PATCH] mesa: fix ARRAY_SIZE query for GetProgramResourceiv

2015-09-29 Thread Martin Peres 



On 28/09/15 13:51, Tapani Pälli wrote:

Patch also refactors name length queries which were using array size
in computation, this has to be done in same time to avoid regression in
arb_program_interface_query-resource-query Piglit test.

Fixes rest of the failures with
ES31-CTS.program_interface_query.no-locations

v2: make additional check only for GS inputs
v3: create helper function for resource name length
 so that it gets calculated only in one place

Signed-off-by: Tapani Pälli 
Reviewed-by: Martin Peres  [v2]
---
  src/mesa/main/program_resource.c |  8 ++--
  src/mesa/main/shader_query.cpp   | 94 
  src/mesa/main/shaderapi.h|  3 ++
  3 files changed, 62 insertions(+), 43 deletions(-)

diff --git a/src/mesa/main/program_resource.c b/src/mesa/main/program_resource.c
index c609abe..eb71fdd 100644
--- a/src/mesa/main/program_resource.c
+++ b/src/mesa/main/program_resource.c
@@ -111,11 +111,9 @@ _mesa_GetProgramInterfaceiv(GLuint program, GLenum 
programInterface,
for (i = 0, *params = 0; i < shProg->NumProgramResourceList; i++) {
   if (shProg->ProgramResourceList[i].Type != programInterface)
  continue;
- const char *name =
-_mesa_program_resource_name(>ProgramResourceList[i]);
- unsigned array_size =
-_mesa_program_resource_array_size(>ProgramResourceList[i]);
- *params = MAX2(*params, strlen(name) + (array_size ? 3 : 0) + 1);
+ unsigned len =
+_mesa_program_resource_name_len(>ProgramResourceList[i]);
+ *params = MAX2(*params, len + 1);
}
break;
 case GL_MAX_NUM_ACTIVE_VARIABLES:
diff --git a/src/mesa/main/shader_query.cpp b/src/mesa/main/shader_query.cpp
index 99d9e10..a5572ff 100644
--- a/src/mesa/main/shader_query.cpp
+++ b/src/mesa/main/shader_query.cpp
@@ -478,7 +478,7 @@ _mesa_program_resource_array_size(struct 
gl_program_resource *res)
   RESOURCE_XFB(res)->Size : 0;
 case GL_PROGRAM_INPUT:
 case GL_PROGRAM_OUTPUT:
-  return RESOURCE_VAR(res)->data.max_array_access;
+  return RESOURCE_VAR(res)->type->length;
 case GL_UNIFORM:
 case GL_VERTEX_SUBROUTINE_UNIFORM:
 case GL_GEOMETRY_SUBROUTINE_UNIFORM:
@@ -670,6 +670,57 @@ _mesa_program_resource_find_index(struct gl_shader_program 
*shProg,
 return NULL;
  }
  
+/* Function returns if resource name is expected to have index

+ * appended into it.
+ *
+ *
+ * Page 61 (page 73 of the PDF) in section 2.11 of the OpenGL ES 3.0
+ * spec says:
+ *
+ * "If the active uniform is an array, the uniform name returned in
+ * name will always be the name of the uniform array appended with
+ * "[0]"."
+ *
+ * The same text also appears in the OpenGL 4.2 spec.  It does not,
+ * however, appear in any previous spec.  Previous specifications are
+ * ambiguous in this regard.  However, either name can later be passed
+ * to glGetUniformLocation (and related APIs), so there shouldn't be any
+ * harm in always appending "[0]" to uniform array names.
+ *
+ * Geometry shader stage has different naming convention where the 'normal'
+ * condition is an array, therefore for variables referenced in geometry
+ * stage we do not add '[0]'.
+ *
+ * Note, that TCS outputs and TES inputs should not have index appended
+ * either.
+ */
+bool
+add_index_to_name(struct gl_program_resource *res)

static please!

With this, you get:
Reviewed-by: Martin Peres 

Thanks for the cleanup!

+{
+   bool add_index = !(((res->Type == GL_PROGRAM_INPUT) &&
+   res->StageReferences & (1 << MESA_SHADER_GEOMETRY)));
+
+   /* Transform feedback varyings have array index already appended
+* in their names.
+*/
+   if (res->Type == GL_TRANSFORM_FEEDBACK_VARYING)
+  add_index = false;
+
+   return add_index;
+}
+
+/* Get name length of a program resource. This consists of
+ * base name + 3 for '[0]' if resource is an array.
+ */
+extern unsigned
+_mesa_program_resource_name_len(struct gl_program_resource *res)
+{
+   unsigned length = strlen(_mesa_program_resource_name(res));
+   if (_mesa_program_resource_array_size(res) && add_index_to_name(res))
+  length += 3;
+   return length;
+}
+
  /* Get full name of a program resource.
   */
  bool
@@ -705,36 +756,7 @@ _mesa_get_program_resource_name(struct gl_shader_program 
*shProg,
  
 _mesa_copy_string(name, bufSize, length, _mesa_program_resource_name(res));
  
-   /* Page 61 (page 73 of the PDF) in section 2.11 of the OpenGL ES 3.0

-* spec says:
-*
-* "If the active uniform is an array, the uniform name returned in
-* name will always be the name of the uniform array appended with
-* "[0]"."
-*
-* The same text also appears in the OpenGL 4.2 spec.  It does not,
-* however, appear in any previous spec.  Previous specifications are
-* ambiguous in

[Mesa-dev] [PATCH] mesa: fix ARRAY_SIZE query for GetProgramResourceiv

2015-09-28 Thread Tapani Pälli 
Patch also refactors name length queries which were using array size
in computation, this has to be done in same time to avoid regression in
arb_program_interface_query-resource-query Piglit test.

Fixes rest of the failures with
   ES31-CTS.program_interface_query.no-locations

v2: make additional check only for GS inputs
v3: create helper function for resource name length
so that it gets calculated only in one place

Signed-off-by: Tapani Pälli 
Reviewed-by: Martin Peres  [v2]
---
 src/mesa/main/program_resource.c |  8 ++--
 src/mesa/main/shader_query.cpp   | 94 
 src/mesa/main/shaderapi.h|  3 ++
 3 files changed, 62 insertions(+), 43 deletions(-)

diff --git a/src/mesa/main/program_resource.c b/src/mesa/main/program_resource.c
index c609abe..eb71fdd 100644
--- a/src/mesa/main/program_resource.c
+++ b/src/mesa/main/program_resource.c
@@ -111,11 +111,9 @@ _mesa_GetProgramInterfaceiv(GLuint program, GLenum 
programInterface,
   for (i = 0, *params = 0; i < shProg->NumProgramResourceList; i++) {
  if (shProg->ProgramResourceList[i].Type != programInterface)
 continue;
- const char *name =
-_mesa_program_resource_name(>ProgramResourceList[i]);
- unsigned array_size =
-_mesa_program_resource_array_size(>ProgramResourceList[i]);
- *params = MAX2(*params, strlen(name) + (array_size ? 3 : 0) + 1);
+ unsigned len =
+_mesa_program_resource_name_len(>ProgramResourceList[i]);
+ *params = MAX2(*params, len + 1);
   }
   break;
case GL_MAX_NUM_ACTIVE_VARIABLES:
diff --git a/src/mesa/main/shader_query.cpp b/src/mesa/main/shader_query.cpp
index 99d9e10..a5572ff 100644
--- a/src/mesa/main/shader_query.cpp
+++ b/src/mesa/main/shader_query.cpp
@@ -478,7 +478,7 @@ _mesa_program_resource_array_size(struct 
gl_program_resource *res)
  RESOURCE_XFB(res)->Size : 0;
case GL_PROGRAM_INPUT:
case GL_PROGRAM_OUTPUT:
-  return RESOURCE_VAR(res)->data.max_array_access;
+  return RESOURCE_VAR(res)->type->length;
case GL_UNIFORM:
case GL_VERTEX_SUBROUTINE_UNIFORM:
case GL_GEOMETRY_SUBROUTINE_UNIFORM:
@@ -670,6 +670,57 @@ _mesa_program_resource_find_index(struct gl_shader_program 
*shProg,
return NULL;
 }
 
+/* Function returns if resource name is expected to have index
+ * appended into it.
+ *
+ *
+ * Page 61 (page 73 of the PDF) in section 2.11 of the OpenGL ES 3.0
+ * spec says:
+ *
+ * "If the active uniform is an array, the uniform name returned in
+ * name will always be the name of the uniform array appended with
+ * "[0]"."
+ *
+ * The same text also appears in the OpenGL 4.2 spec.  It does not,
+ * however, appear in any previous spec.  Previous specifications are
+ * ambiguous in this regard.  However, either name can later be passed
+ * to glGetUniformLocation (and related APIs), so there shouldn't be any
+ * harm in always appending "[0]" to uniform array names.
+ *
+ * Geometry shader stage has different naming convention where the 'normal'
+ * condition is an array, therefore for variables referenced in geometry
+ * stage we do not add '[0]'.
+ *
+ * Note, that TCS outputs and TES inputs should not have index appended
+ * either.
+ */
+bool
+add_index_to_name(struct gl_program_resource *res)
+{
+   bool add_index = !(((res->Type == GL_PROGRAM_INPUT) &&
+   res->StageReferences & (1 << MESA_SHADER_GEOMETRY)));
+
+   /* Transform feedback varyings have array index already appended
+* in their names.
+*/
+   if (res->Type == GL_TRANSFORM_FEEDBACK_VARYING)
+  add_index = false;
+
+   return add_index;
+}
+
+/* Get name length of a program resource. This consists of
+ * base name + 3 for '[0]' if resource is an array.
+ */
+extern unsigned
+_mesa_program_resource_name_len(struct gl_program_resource *res)
+{
+   unsigned length = strlen(_mesa_program_resource_name(res));
+   if (_mesa_program_resource_array_size(res) && add_index_to_name(res))
+  length += 3;
+   return length;
+}
+
 /* Get full name of a program resource.
  */
 bool
@@ -705,36 +756,7 @@ _mesa_get_program_resource_name(struct gl_shader_program 
*shProg,
 
_mesa_copy_string(name, bufSize, length, _mesa_program_resource_name(res));
 
-   /* Page 61 (page 73 of the PDF) in section 2.11 of the OpenGL ES 3.0
-* spec says:
-*
-* "If the active uniform is an array, the uniform name returned in
-* name will always be the name of the uniform array appended with
-* "[0]"."
-*
-* The same text also appears in the OpenGL 4.2 spec.  It does not,
-* however, appear in any previous spec.  Previous specifications are
-* ambiguous in this regard.  However, either name can later be passed
-* to glGetUniformLocation (and related APIs), so there shouldn't be any
-* harm in always appending "[0]" to uniform array