On Aug 19, 2015 2:56 AM, "Francisco Jerez" <<a href="mailto:currojerez@riseup.net">currojerez@riseup.net</a>> wrote: > > Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> writes: > > > Previously, setting up image uniforms relied on being called after > > fs_visitor::uniforms was set and with fs_visitor::uniforms not allocating > > space for it. This made sense in an ir_visitor world because the visitor > > assigns locations and uploads data as it walks through the variables. In > > NIR it also happened to work because nir_lower_io assumed zero space for > > images. In the near future, we will be able to reserve space using > > nir_lower_io and these invariants will be broken. > > > > This commit makes setup_image_uniforms take a pointer to the location in > > the prog_data params array that indicates where the image uniforms should > > be stored. This way it can be called from anywhere in the shader setup. > > > > v2: Fix nir_setup_uniform to correctly correspond with this change > > (rebase fail) > > > > Cc: Francisco Jerez <<a href="mailto:currojerez@riseup.net">currojerez@riseup.net</a>> > > Cc: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> > > --- > > src/mesa/drivers/dri/i965/brw_fs.cpp | 12 ------------ > > src/mesa/drivers/dri/i965/brw_fs.h | 3 --- > > src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 6 ++++-- > > src/mesa/drivers/dri/i965/brw_shader.cpp | 28 +++++++++++++++++++++------- > > src/mesa/drivers/dri/i965/brw_shader.h | 5 ++--- > > 5 files changed, 27 insertions(+), 27 deletions(-) > > > > diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp > > index 82cb499..d1550e6 100644 > > --- a/src/mesa/drivers/dri/i965/brw_fs.cpp > > +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp > > @@ -942,18 +942,6 @@ fs_visitor::import_uniforms(fs_visitor *v) > > this->param_size = v->param_size; > > } > > > > -void > > -fs_visitor::setup_vector_uniform_values(const gl_constant_value *values, unsigned n) > > -{ > > - static const gl_constant_value zero = { 0 }; > > - > > - for (unsigned i = 0; i < n; ++i) > > - stage_prog_data->param[uniforms++] = &values[i]; > > - > > - for (unsigned i = n; i < 4; ++i) > > - stage_prog_data->param[uniforms++] = &zero; > > -} > > - > > fs_reg * > > fs_visitor::emit_fragcoord_interpolation(bool pixel_center_integer, > > bool origin_upper_left) > > diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h > > index 975183e..28fcfa3 100644 > > --- a/src/mesa/drivers/dri/i965/brw_fs.h > > +++ b/src/mesa/drivers/dri/i965/brw_fs.h > > @@ -291,9 +291,6 @@ public: > > > > struct brw_reg interp_reg(int location, int channel); > > > > - virtual void setup_vector_uniform_values(const gl_constant_value *values, > > - unsigned n); > > - > > int implied_mrf_writes(fs_inst *inst); > > > > virtual void dump_instructions(); > > diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp > > index ce4153d..c8ea649 100644 > > --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp > > +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp > > @@ -244,10 +244,12 @@ fs_visitor::nir_setup_uniform(nir_variable *var) > > * space for them here at the end of the parameter array. > > */ > > var->data.driver_location = uniforms; > > - param_size[uniforms] = > > + unsigned size = > > BRW_IMAGE_PARAM_SIZE * MAX2(storage->array_elements, 1); > > > > - setup_image_uniform_values(storage); > > + setup_image_uniform_values(stage_prog_data->param + uniforms, storage); > > + param_size[uniforms] = size; > > + uniforms += size; > > } else { > > unsigned slots = storage->type->component_slots(); > > if (storage->array_elements) > > diff --git a/src/mesa/drivers/dri/i965/brw_shader.cpp b/src/mesa/drivers/dri/i965/brw_shader.cpp > > index 6b92806..b31ae9b 100644 > > --- a/src/mesa/drivers/dri/i965/brw_shader.cpp > > +++ b/src/mesa/drivers/dri/i965/brw_shader.cpp > > @@ -1419,8 +1419,22 @@ backend_shader::assign_common_binding_table_offsets(uint32_t next_binding_table_ > > /* prog_data->base.binding_table.size will be set by brw_mark_surface_used. */ > > } > > > > +static void > > +setup_vector_uniform_values(const gl_constant_value ***dst, > > + const gl_constant_value *values, unsigned n) > > +{ > > + static const gl_constant_value zero = { 0 }; > > + > > + for (unsigned i = 0; i < n; ++i) > > + *((*dst)++) = &values[i]; > > + > > + for (unsigned i = n; i < 4; ++i) > > + *((*dst)++) = &zero; > > +} > > + > > void > > -backend_shader::setup_image_uniform_values(const gl_uniform_storage *storage) > > +backend_shader::setup_image_uniform_values(const gl_constant_value **prog_param, > > + const gl_uniform_storage *storage) > > { > > const unsigned stage = _mesa_program_enum_to_shader_stage(prog->Target); > > > > @@ -1431,17 +1445,17 @@ backend_shader::setup_image_uniform_values(const gl_uniform_storage *storage) > > /* Upload the brw_image_param structure. The order is expected to match > > * the BRW_IMAGE_PARAM_*_OFFSET defines. > > */ > > - setup_vector_uniform_values( > > + setup_vector_uniform_values(&prog_param, > > (const gl_constant_value *)&param->surface_idx, 1); > > - setup_vector_uniform_values( > > + setup_vector_uniform_values(&prog_param, > > (const gl_constant_value *)param->offset, 2); > > - setup_vector_uniform_values( > > + setup_vector_uniform_values(&prog_param, > > (const gl_constant_value *)param->size, 3); > > - setup_vector_uniform_values( > > + setup_vector_uniform_values(&prog_param, > > (const gl_constant_value *)param->stride, 4); > > - setup_vector_uniform_values( > > + setup_vector_uniform_values(&prog_param, > > (const gl_constant_value *)param->tiling, 3); > > - setup_vector_uniform_values( > > + setup_vector_uniform_values(&prog_param, > > (const gl_constant_value *)param->swizzling, 2); > > > > brw_mark_surface_used( > > diff --git a/src/mesa/drivers/dri/i965/brw_shader.h b/src/mesa/drivers/dri/i965/brw_shader.h > > index 2cc97f2..2cd0a5a 100644 > > --- a/src/mesa/drivers/dri/i965/brw_shader.h > > +++ b/src/mesa/drivers/dri/i965/brw_shader.h > > @@ -270,9 +270,8 @@ public: > > > > virtual void invalidate_live_intervals() = 0; > > > > - virtual void setup_vector_uniform_values(const gl_constant_value *values, > > - unsigned n) = 0; > > - void setup_image_uniform_values(const gl_uniform_storage *storage); > > + void setup_image_uniform_values(const gl_constant_value **prog_param, > > + const gl_uniform_storage *storage); > > }; > > > > uint32_t brw_texture_offset(int *offsets, unsigned num_components); > > -- > > 2.4.3 > > Did you notice that this makes > backend_shader::setup_image_uniform_values() FS-specific? I think > passing the base uniform index where it should be set up would do what > you want and at the same time could be implemented in a backend-agnostic > way. How is this FS-specific? setup_vector_uniform_values was exactly the same code in both backends and the same as this static helper. They both have an array of params. Am I missing something?