[Mesa-dev] [PATCH] glsl: set vector_elements to 1 for samplers

[Samuel Pitoiset]

On 04/28/2017 08:40 PM, Mark Janes wrote:
> Samuel Pitoiset <samuel.pitoiset at gmail.com> writes:
> 
>> On 04/28/2017 06:58 PM, Mark Janes wrote:
>>> With this commit, a wide range of intel hardware began hanging during
>>> the GLES CTS, with dmesg errors like:
>>>
>>> [25488.739167] traps: glcts[15106] general protection ip:7fdac6484ba5 sp:7ffdcda85a20 error:0
>>>
>>> Machines that did complete the cts, reported hundreds of errors like:
>>>
>>> *** Error in `/tmp/build_root/m64/bin/es/cts/glcts': malloc(): memory
>>>       corruption: 0x0000562c7503b270 ***
>>
>> That's unfortunate. I don't have any Intel hw.
> 
> I can set up a CI build that will test your patches on Intel hardware.
> Many non-Intel developers find that the exhaustive testing in our CI
> alerts them to bugs in their patches.  It takes about 30 minutes to run.
> 
> Please give me a branch that you want to use for testing.  When you
> force-push to that branch, a build will trigger.

Thanks. That will be very helpful to prevent such a thing to happen.

> 
>> Kenneth is going to revert the patch for now.
>>
>> Would be very nice if someone with the hardware could have a look.
>>
>> Which tests are failing? Can you list some?
> 
> I put a list of failing piglit tests in
> https://bugs.freedesktop.org/show_bug.cgi?id=100871
> 
>>>
>>>
>>>
>>> Samuel Pitoiset <samuel.pitoiset at gmail.com> writes:
>>>
>>>> I don't see any reasons why vector_elements is 1 for images and
>>>> 0 for samplers. This increases consistency and allows to clean
>>>> up some code a bit.
>>>>
>>>> This will also help for ARB_bindless_texture.
>>>>
>>>> No piglit regressions with RadeonSI.
>>>>
>>>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>>>> ---
>>>>    src/compiler/glsl_types.cpp     |  7 +------
>>>>    src/mesa/main/uniform_query.cpp | 15 +++++----------
>>>>    2 files changed, 6 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/src/compiler/glsl_types.cpp b/src/compiler/glsl_types.cpp
>>>> index 0480bef80e..bf078ad614 100644
>>>> --- a/src/compiler/glsl_types.cpp
>>>> +++ b/src/compiler/glsl_types.cpp
>>>> @@ -95,12 +95,7 @@ glsl_type::glsl_type(GLenum gl_type, glsl_base_type base_type,
>>>>    
>>>>       memset(& fields, 0, sizeof(fields));
>>>>    
>>>> -   if (is_sampler()) {
>>>> -      /* Samplers take no storage whatsoever. */
>>>> -      matrix_columns = vector_elements = 0;
>>>> -   } else {
>>>> -      matrix_columns = vector_elements = 1;
>>>> -   }
>>>> +   matrix_columns = vector_elements = 1;
>>>>    }
>>>>    
>>>>    glsl_type::glsl_type(const glsl_struct_field *fields, unsigned num_fields,
>>>> diff --git a/src/mesa/main/uniform_query.cpp b/src/mesa/main/uniform_query.cpp
>>>> index e400d0eb00..114f6fb5be 100644
>>>> --- a/src/mesa/main/uniform_query.cpp
>>>> +++ b/src/mesa/main/uniform_query.cpp
>>>> @@ -321,8 +321,7 @@ _mesa_get_uniform(struct gl_context *ctx, GLuint program, GLint location,
>>>>       }
>>>>    
>>>>       {
>>>> -      unsigned elements = (uni->type->is_sampler())
>>>> -	 ? 1 : uni->type->components();
>>>> +      unsigned elements = uni->type->components();
>>>>          const int dmul = uni->type->is_64bit() ? 2 : 1;
>>>>          const int rmul = glsl_base_type_is_64bit(returnType) ? 2 : 1;
>>>>    
>>>> @@ -648,10 +647,8 @@ _mesa_propagate_uniforms_to_driver_storage(struct gl_uniform_storage *uni,
>>>>    {
>>>>       unsigned i;
>>>>    
>>>> -   /* vector_elements and matrix_columns can be 0 for samplers.
>>>> -    */
>>>> -   const unsigned components = MAX2(1, uni->type->vector_elements);
>>>> -   const unsigned vectors = MAX2(1, uni->type->matrix_columns);
>>>> +   const unsigned components = uni->type->vector_elements;
>>>> +   const unsigned vectors = uni->type->matrix_columns;
>>>>       const int dmul = uni->type->is_64bit() ? 2 : 1;
>>>>    
>>>>       /* Store the data in the driver's requested type in the driver's storage
>>>> @@ -803,8 +800,7 @@ validate_uniform(GLint location, GLsizei count, const GLvoid *values,
>>>>       }
>>>>    
>>>>       /* Verify that the types are compatible. */
>>>> -   const unsigned components = uni->type->is_sampler()
>>>> -      ? 1 : uni->type->vector_elements;
>>>> +   const unsigned components = uni->type->vector_elements;
>>>>    
>>>>       if (components != src_components) {
>>>>          /* glUniformN() must match float/vecN type */
>>>> @@ -925,8 +921,7 @@ _mesa_uniform(GLint location, GLsizei count, const GLvoid *values,
>>>>             return;
>>>>       }
>>>>    
>>>> -   const unsigned components = uni->type->is_sampler()
>>>> -      ? 1 : uni->type->vector_elements;
>>>> +   const unsigned components = uni->type->vector_elements;
>>>>    
>>>>       /* Page 82 (page 96 of the PDF) of the OpenGL 2.1 spec says:
>>>>        *
>>>> -- 
>>>> 2.12.2
>>>>
>>>> _______________________________________________
>>>> mesa-dev mailing list
>>>> mesa-dev at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev