On 04/10/2014 11:42 AM, Ian Romanick wrote: > On 04/09/2014 08:02 AM, Kenneth Graunke wrote: >> On 04/04/2014 02:01 PM, Ian Romanick wrote: >>> From: Ian Romanick <ian.d.roman...@intel.com> >>> >>> In the next patch, we'll see that using >>> gl_shader_program::UniformStorage is not correct for uniform blocks. >>> That means we can't use ::UniformStorage to select between the sampler >>> path and the block path. Instead we want to just use the type of the >>> variable. That's never passed to set_uniform_binding, and it's easier >> >> Ehhhmm.....then.... > > "That" in this instance the variable, not the variable's type. I'll > update the commit message. More reply below. > >>> to just remove the function (especially for later patches in the series) >>> than to add another parameter. >>> >>> Signed-off-by: Ian Romanick <ian.d.roman...@intel.com> >>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76323 >>> Cc: "10.1" <mesa-sta...@lists.freedesktop.org> >>> Cc: git...@socker.lepus.uberspace.de >>> --- >>> src/glsl/link_uniform_initializers.cpp | 33 >>> ++++++++++++--------------------- >>> 1 file changed, 12 insertions(+), 21 deletions(-) >>> >>> diff --git a/src/glsl/link_uniform_initializers.cpp >>> b/src/glsl/link_uniform_initializers.cpp >>> index 6f15e69..bbdeec9 100644 >>> --- a/src/glsl/link_uniform_initializers.cpp >>> +++ b/src/glsl/link_uniform_initializers.cpp >>> @@ -151,25 +151,6 @@ set_block_binding(void *mem_ctx, gl_shader_program >>> *prog, >>> } >>> >>> void >>> -set_uniform_binding(void *mem_ctx, gl_shader_program *prog, >>> - const char *name, const glsl_type *type, int binding) >> >> ...what exactly is this? ^^^^^^^^^^^^^^^^^^^^^ >> >>> -{ >>> - struct gl_uniform_storage *const storage = >>> - get_storage(prog->UniformStorage, prog->NumUserUniformStorage, name); >>> - >>> - if (storage == NULL) { >>> - assert(storage != NULL); >>> - return; >>> - } >>> - >>> - if (storage->type->is_sampler()) { >>> - set_sampler_binding(mem_ctx, prog, name, type, binding); >>> - } else if (storage->block_index != -1) { >>> - set_block_binding(mem_ctx, prog, name, type, binding); >>> - } >>> -} >>> - >>> -void >>> set_uniform_initializer(void *mem_ctx, gl_shader_program *prog, >>> const char *name, const glsl_type *type, >>> ir_constant *val) >>> @@ -268,8 +249,18 @@ link_set_uniform_initializers(struct gl_shader_program >>> *prog) >>> mem_ctx = ralloc_context(NULL); >>> >>> if (var->data.explicit_binding) { >>> - linker::set_uniform_binding(mem_ctx, prog, var->name, >>> - var->type, var->data.binding); >>> + const glsl_type *const type = var->type; >> >> Here you're using type, which is var->type, which is exactly what we >> were already passing. >> >> AFAICT all you needed to do was change: >> >> if (storage->type->is_sampler()) >> >> to >> >> if (type->is_sampler() || (type->is_array() && >> type->fields.array->is_sampler())) >> >> in set_uniform_binding. > > That would resolve this issue, but patch 5 needs the interface type and > not the variable's type. The only way to get the interface type is from > the variable. Splitting it out here, then passing the interface type > instead of the variable's type in a later patch resulted in what I > believed to be better code. > > Sound reasonable?
Definitely. Sorry I wasn't clear - although I thought these changes were unnecessary, I wasn't opposed to them. They seem worth doing regardless. I believe you have my R-b for the whole series. Thanks Ian!
signature.asc
Description: OpenPGP digital signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
