[Mesa-dev] [PATCH] glsl: uniform sampler should occupy 1 location

[Ian Romanick]

On 08/23/2014 07:51 AM, Micael wrote:
> On second thought, the glsl_type::uniform_locations() method comment in
> the header file says:
> /**
>  * Calculate the number of unique values from glGetUniformLocation for the
>  * elements of the type.
>  */
> 
> Which makes me believe that maybe we should return at least 1 for every
> case because this is only called for explicit locations and therefore
> will not make shaders failing to link unless they've explicitely used
> more locations than they should.
> Maybe glsl_type::uniform_locations() should be renamed to something that
> makes it clear it's only used for explicit locations too, or make it
> clear that it reflects the size for the API, not GPU memory (since it's
> only used to fill prog->UniformRemapTable).

I spent some time this morning looking at this code a bit more closely,
and I think you're right.  The problem is that GL overloads "locations"
both to mean the number of "handles" a uniform gets and to mean amount
of storage space the uniform occupies.  For some things this is the same
(simple variables, arrays, structures), but for other things it is not
(matrices, samplers, other opaque types).

I think it's weird for uniform_locations to call component_slots at all.

Looking at the OpenGL 4.2 spec, it seems that everything except atomic
counters and subroutines (not yet supported by Mesa) should have at
least one location per array element.

Here's what I think would be better: have uniform_locations mirror the
structure of component_slots.  Eliminate the is_matrix() check at the
top, and just have all the basic types (e.g., GLSL_TYPE_FLOAT) return 1.

Keeping functions separate that count separate kinds of things seems
like a good idea.

> On Sat, Aug 23, 2014 at 2:07 PM, Micael <kam1kaz3 at gmail.com
> <mailto:kam1kaz3 at gmail.com>> wrote:
> 
>     On Fri, Aug 22, 2014 at 9:23 PM, Ian Romanick <idr at freedesktop.org
>     <mailto:idr at freedesktop.org>> wrote:
> 
>         I'm not sure this is correct, and I think we need a more complex
>         fix.
>         As Dave points out, bindless will make it even more complex.
> 
>         In a non-bindless, static-sampler-array-indexing world, applications
>         assume that samplers will use zero uniform locations.  The sampler
>         information is baked into the instructions, and it isn't represented
>         anywhere else in GPU memory.  As a result, I would not be
>         surprised to
>         see previously working applications fail to link (due to using
>         too many
>         uniforms) with this change.
> 
>         It seems that the only time a sampler needs non-zero space is
>         when it is
>         part of any array samplers (or array of structures containing
>         samplers,
>         etc.) or is bindless.  In the array cases, it is probably only
>         necessary
>         when the index is dynamic.  I think the array splitting and
>         structure
>         splitting passes may make that moot.
> 
> 
>     Did you miss the case of assigning an explicit location to a
>     sampler, or did you ommit on purpose?
>     I'm not very familiar with mesa source, but as far as I could
>     analyze it, only reserve_explicit_locations() and
>     validate_explicit_location() call glsl_type::uniform_locations(),
>     everything else uses glsl_type::component_slots().
>     Now I agree with you that simply returning 1 from
>     glsl_type::uniform_locations() for samplers can be misleading.
> 
>     How about if glsl_type::uniform_locations() takes a "bool
>     isExplicitLocation" and return at least 1 for every type? I am aware
>     that this won't solve the bindless textures issue, but it would fix
>     this bug (and the integer underflows) for now, I think.
>      
> 
> 
>         The 82921 bug seems to be an orthognal issue.  There is a difference
>         between the API visible locations assigned to a uniform and the GPU
>         visible locations where the uniform is stored.  My guess is that
>         we're
>         using the same accounting for both in places where we should not.
> 
>         On 08/21/2014 04:25 PM, Micael Dias wrote:
>         > ---
>         > If samplers occupy zero locations we can run into a lot of
>         issues. See #82921.
>         > I briefly tested this with my own code (which was previously
>         crashing and
>         > misbehaving) and also ran other apps and everything seems to
>         work fine.
>         > I'm not used to contributing code in this fashion, so please
>         forgive me if I'm
>         > making some mistake. Thanks.
>         >
>         >  src/glsl/glsl_types.cpp | 2 ++
>         >  1 file changed, 2 insertions(+)
>         >
>         > diff --git a/src/glsl/glsl_types.cpp b/src/glsl/glsl_types.cpp
>         > index 66e9b13..cc05193 100644
>         > --- a/src/glsl/glsl_types.cpp
>         > +++ b/src/glsl/glsl_types.cpp
>         > @@ -691,6 +691,8 @@ glsl_type::uniform_locations() const
>         >        return size;
>         >     case GLSL_TYPE_ARRAY:
>         >        return this->length *
>         this->fields.array->uniform_locations();
>         > +   case GLSL_TYPE_SAMPLER:
>         > +      return 1;
>         >     default:
>         >        break;
>         >     }
>         >
> 
> 
> 
> 
>     -- 
>     Micael Dias
> 
> 
> 
> 
> -- 
> Micael Dias