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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
