On Aug 19, 2015 6:45 AM, "Francisco Jerez" <curroje...@riseup.net> wrote:
>
> Jason Ekstrand <ja...@jlekstrand.net> writes:
>
> > On Aug 19, 2015 2:56 AM, "Francisco Jerez" <curroje...@riseup.net>
wrote:
> >>
> >> Jason Ekstrand <ja...@jlekstrand.net> 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 <curroje...@riseup.net>
> >> > Cc: Kenneth Graunke <kenn...@whitecape.org>
> >> > ---
> >> > 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 *)¶m->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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
