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.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>> wrote: > >> > >> On Fri, Aug 22, 2014 at 9:23 PM, Ian Romanick <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 > > 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
