[Mesa-dev] [PATCH] glsl: uniform sampler should occupy 1 location
Micael
kam1kaz3 at gmail.com
Wed Aug 27 15:50:00 PDT 2014
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?
On Wed, Aug 27, 2014 at 11:15 AM, Tapani Pälli <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>> 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
>
>
--
Micael Dias
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140827/1c45094e/attachment.html>
More information about the mesa-dev
mailing list