On Fri, Aug 22, 2014 at 9:23 PM, Ian Romanick <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
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
