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.pa...@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.
> > > > +
> > > > + 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.
Anyways its not a big deal I guess, if you fix up the issue above you
can have.
Reviewed-by: Timothy Arceri <timothy.arc...@collabora.com>
>
>
> >
> > >
> > >
> > > > delete uniform_map;
> > > > }
> > > >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
