[Mesa-dev] [PATCH v2] i965: Refactor image uniform setup
Jason Ekstrand
jason at jlekstrand.net
Wed Aug 19 17:58:10 PDT 2015
On Wed, Aug 19, 2015 at 6:48 AM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>
> On Aug 19, 2015 6:45 AM, "Francisco Jerez" <currojerez at riseup.net> wrote:
>>
>> Jason Ekstrand <jason at jlekstrand.net> writes:
>>
>> > On Aug 19, 2015 2:56 AM, "Francisco Jerez" <currojerez at riseup.net>
>> > wrote:
>> >>
>> >> Jason Ekstrand <jason at 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 <currojerez at riseup.net>
>> >> > Cc: Kenneth Graunke <kenneth at 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.
I've re-written it to use an offset. Hopefully, I'll get the patches
sent out tomorrow. Until then, you can view it here:
http://cgit.freedesktop.org/~jekstrand/mesa/commit/?h=wip/fs-uniform-indirect&id=fd908c80975992112e449ae19321a05d85ecbf2e
--Jason
More information about the mesa-dev
mailing list