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

Wed Aug 27 04:15:04 PDT 2014

On 08/26/2014 10:37 PM, Ian Romanick wrote:
> 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.

Yep, at the moment of writing this I have gotten confused of the
locations and actual storage space (resulting in vectors using too many
locs). As this is my mess, I've sent a patch that implements the count
as you describe below + adds a bit more documentation to the header.

> 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
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev