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.pa...@intel.com > <mailto:tapani.pa...@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 <kam1k...@gmail.com > <mailto:kam1k...@gmail.com> > >> <mailto:kam1k...@gmail.com <mailto:kam1k...@gmail.com>>> wrote: > >> > >> On Fri, Aug 22, 2014 at 9:23 PM, Ian Romanick > <i...@freedesktop.org <mailto:i...@freedesktop.org> > >> <mailto:i...@freedesktop.org <mailto:i...@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@lists.freedesktop.org > <mailto:mesa-dev@lists.freedesktop.org> > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev > > > > > -- > Micael Dias
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
