[Mesa-dev] [PATCH] glsl: uniform sampler should occupy 1 location
Tapani Pälli
tapani.palli at intel.com
Wed Aug 27 23:40:11 PDT 2014
On 08/28/2014 01:50 AM, Micael wrote:
> I am getting another error regarding explicit locations, which I
> believe has a one-line fix. If I only have a single uniform in my
> shader, and that uniform is explicitely given location 1 (or anything
> above zero), my program will crash if I attempt to give the
> unnassigned locs a value with glProgramUniform1i().
> A quick view into the code appears to reveal
> reserve_explicit_locations() as the culprit by initializing the remap
> table entries to NULL instead of INACTIVE_UNIFORM_EXPLICIT_LOCATION,
> which will allow validate_uniform_parameters() to pass the test "if
> (shProg->UniformRemapTable[location]
> ==INACTIVE_UNIFORM_EXPLICIT_LOCATION)" later.
>
> Should I open another bug report?
>
Yes please, I'll write a Piglit test for this.
Thanks;
>
> On Wed, Aug 27, 2014 at 11:15 AM, Tapani Pälli <tapani.palli at intel.com
> <mailto:tapani.palli at intel.com>> wrote:
>
> 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>
> >> <mailto: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>
> >> <mailto: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
> <mailto:mesa-dev at lists.freedesktop.org>
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
>
>
> --
> Micael Dias
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140828/449e6e2f/attachment-0001.html>
More information about the mesa-dev
mailing list