[Mesa-dev] [PATCH] glsl: track total amount of uniform locations used

[Tapani Pälli]

On 01/12/2016 12:30 AM, Timothy Arceri wrote:
> On Mon, 2016-01-11 at 08:24 +0200, Tapani Pälli wrote:
>>
>> On 01/08/2016 11:32 AM, Tapani Pälli wrote:
>>>
>>>
>>> On 01/08/2016 11:17 AM, Timothy Arceri wrote:
>>>> On Fri, 2016-01-08 at 08:20 +0200, Tapani Pälli wrote:
>>>>> Linker missed a check for situation where we exceed max amount
>>>>> of
>>>>> uniform locations with explicit + implicit locations. Patch
>>>>> adds this
>>>>> check to already existing iteration over uniforms in linker.
>>>>>
>>>>> Fixes following CTS test:
>>>>>      ES31-CTS.explicit_uniform_location.uniform-loc-negative
>>>>> -link-max
>>>>> -num-of-locations
>>>>>
>>>>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>>>>> ---
>>>>>    src/glsl/linker.cpp | 17 +++++++++++++++--
>>>>>    1 file changed, 15 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
>>>>> index 7a18523..ef23b36 100644
>>>>> --- a/src/glsl/linker.cpp
>>>>> +++ b/src/glsl/linker.cpp
>>>>> @@ -3130,6 +3130,7 @@ check_explicit_uniform_locations(struct
>>>>> gl_context *ctx,
>>>>>          return;
>>>>>       }
>>>>>
>>>>> +   unsigned entries_total = 0;
>>>>>       for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) {
>>>>>          struct gl_shader *sh = prog->_LinkedShaders[i];
>>>>>
>>>>> @@ -3138,8 +3139,12 @@ check_explicit_uniform_locations(struct
>>>>> gl_context *ctx,
>>>>>
>>>>>          foreach_in_list(ir_instruction, node, sh->ir) {
>>>>>             ir_variable *var = node->as_variable();
>>>>> -         if (var && (var->data.mode == ir_var_uniform &&
>>>>> -                     var->data.explicit_location)) {
>>>>> +         if (!var || var->data.mode != ir_var_uniform)
>>>>> +            continue;
>>>>> +
>>>>> +         entries_total += var->type->is_array() ? var->type
>>>>> ->length
>>>>> : 1;
>
> This should be:
>
> entries_total += var->type->uniform_locations();
>
> Otherwise AoA and structs will not be counted correctly.

ah I had forgotten about uniform_locations(), thanks!

>
>>>>> +
>>>>> +         if (var->data.explicit_location) {
>>>>>                bool ret;
>>>>>                if (var->type->is_subroutine())
>>>>>                   ret =
>>>>> reserve_subroutine_explicit_locations(prog, sh,
>>>>> var);
>>>>> @@ -3153,6 +3158,14 @@ check_explicit_uniform_locations(struct
>>>>> gl_context *ctx,
>>>>>          }
>>>>>       }
>>>>>
>>>>> +   /* Verify that total amount of entries for explicit and
>>>>> implicit
>>>>> locations
>>>>> +    * is less than MAX_UNIFORM_LOCATIONS.
>>>>> +    */
>>>>> +   if (entries_total >= ctx
>>>>> ->Const.MaxUserAssignableUniformLocations) {
>>>>> +      linker_error(prog, "count of uniform locations >=
>>>>> MAX_UNIFORM_LOCATIONS"
>>>>> +                   "(%u >= %u)", entries_total,
>>>>> +                   ctx
>>>>> ->Const.MaxUserAssignableUniformLocations);
>>>>> +   }
>>>>
>>>> I think this check would be better in
>>>> link_assign_uniform_locations()
>>>> because check_explicit_uniform_locations() is called before
>>>> arrays are
>>>> resized via update_array_sizes()
>>>>
>>>> Also in link_assign_uniform_locations() there is already a count
>>>> of
>>>> uniform locations.
>>>>
>>>> See: const unsigned num_uniforms =
>>>> uniform_size.num_active_uniforms;
>>>
>>> That function can't currently fail and it does not know the max no
>>> of
>>> uniforms so it was easier to plug here. I can move it there but it
>>> requires changing the function signature a bit, I guess not that
>>> big
>>> deal though.
>>
>> I've made a version that counts used locations only at
>> link_assign_uniform_locations(). Problem with that is that it happens
>> too late. With the test case an uniform array gets optimized away
>> (since
>> it's not used in the shader, however as it is using explicit
>> location,
>> those locations must be reserved), therefore this happens too late.
>> IMO
>> we must do this calculation as the my first proposal did it.
>
> Hmm, sure they must be reserved but if they are inactive I don't think
> there is anything in the spec that disallows using the location for
> varyings that don't have an explicit location.

I don't think overlap between these matter as these are distinct 
resources, used for different things.

> Anyways its not a big deal I guess, if you fix up the issue above you
> can have.
>
> Reviewed-by: Timothy Arceri <timothy.arceri at collabora.com>

Thanks for the review!

// Tapani