[Mesa-dev] [PATCH] glsl: uniform sampler should occupy 1 location

[Micael]

On Fri, Aug 22, 2014 at 9:23 PM, Ian Romanick <idr at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140823/a2878b95/attachment.html>