On Aug 19, 2015 6:45 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: > > > 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? > > The VEC4 back-end additionally keeps track of the uniform vector sizes > in the uniform_vector_size[] array. Right... Thanks for catching that. I'll have to think about it.