[Mesa-dev] [PATCH] glsl: initialize samplers mapping with 0

[Ian Romanick]

On 04/29/2012 12:54 AM, Vadim Girlin wrote:
> On Sat, 2012-04-28 at 13:02 -0700, Kenneth Graunke wrote:
>> On 04/28/2012 12:24 PM, Vadim Girlin wrote:
>>> On Sat, 2012-04-28 at 11:42 -0700, Kenneth Graunke wrote:
>>>> On 04/28/2012 11:20 AM, Vadim Girlin wrote:
>>>>> According to GLSL 1.30 specification, initial value for all uniforms
>>>>> without initializer is 0.  Some applications rely on this behaviour,
>>>>> e.g. google's MapGL doesn't set all sampler uniforms.
>>>>>
>>>>> (see https://bugs.freedesktop.org/show_bug.cgi?id=49088 )
>>>>>
>>>>> Signed-off-by: Vadim Girlin<vadimgirlin at gmail.com>
>>>>> ---
>>>>>
>>>>> Tested with r600g only - no regressions.
>>>>
>>>> Awesome find!  I was at a complete loss here. :)
>>>>
>>>> It turns out this is in the 1.20 spec too; it looks like 1.10 doesn't
>>>> say (but that isn't too surprising).  I might add a comment:
>>>>
>>>> /* From the GLSL 1.20 specification, page 24, section 4.3.5 "Uniform":
>>>>     * "The link time initial value is either the value of the variable's
>>>>     *  initializer, or 0 if no initializer present.  Sampler types cannot
>>>>     *  have initializers."
>>>>     */
>>>>
>>>> Also, do you really need the memsets in ir_to_mesa and st_glsl_to_tgsi?
>>>>     Everything should go through the linker, so that seems somewhat
>>>> redundant.  I tested with just the link_uniforms change and that was
>>>> enough to fix MapsGL on i965/Sandybridge.
>>>
>>> Without the memset in the st_glsl_to_tgsi firefox crashed with MapGL.
>>> Probably some code in the state tracker relies on the synchronized
>>> values of the SamplerUnits arrays in the gl_program and
>>> gl_shader_program. Also, _mesa_uniform function compares these arrays to
>>> check for update, and some piglit test failed due to the following
>>> problem:
>>>
>>> First array is initialized: 0, 0, 0, ...
>>> Second (without memset)	  : 0, 1, 2, ...
>>>
>>> The app calls Uniform1i to set sampler[1] to 1, so we do first[1]=1, but
>>> then it's equal with second[1] and update is not detected.
>>>
>>> Vadim
>>
>> Ah, okay...I missed that one was operating on the gl_shader_program
>> while the other was setting the parallel structure in the gl_program.
>>
>> I can believe that you'd need that, then.
>>
>
> BTW, it seems there is a patch from Ian for this issue on the list:
>
> http://lists.freedesktop.org/archives/mesa-dev/2012-April/020767.html

Right.  Without the first patch in that series, the patch you reference 
probably isn't sufficient (due to ir_to_mesa and st_glsl_to_tgsi not 
being modified).  Eric had a couple concerns about the first patch in 
the series, and I haven't had a chance to put together a test case for 
those issues.

Maybe we could combine my patch with the ir_to_mesa and st_glsl_to_tgsi 
parts from yours?