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

[Vadim Girlin]

On Mon, 2012-05-07 at 11:37 -0700, Ian Romanick wrote:
> 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?

Of course, feel free to use any parts of this patch if you need.

Vadim