<div dir="ltr"><div><div>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(). </div>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. </div>Should I open another bug report? </div><div class="gmail_extra"> <div class="gmail_quote">On Wed, Aug 27, 2014 at 11:15 AM, Tapani Pälli <<a href="mailto:tapani.palli@intel.com" target="_blank">tapani.palli@intel.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">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. </div>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. <div class="HOEnZb"><div class="h5"> > 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 <<a href="mailto:kam1kaz3@gmail.com">kam1kaz3@gmail.com</a> >> <mailto:<a href="mailto:kam1kaz3@gmail.com">kam1kaz3@gmail.com</a>>> wrote: >> >> On Fri, Aug 22, 2014 at 9:23 PM, Ian Romanick <<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a> >> <mailto:<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a>>> 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 </div></div><div class="HOEnZb"><div class="h5">> _______________________________________________ > mesa-dev mailing list > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </div></div></blockquote></div> -- Micael Dias </div>