[Mesa-dev] [PATCH v2] i965: Refactor image uniform setup
Francisco Jerez
currojerez at riseup.net
Wed Aug 19 06:45:12 PDT 2015
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150819/48168498/attachment.sig>
More information about the mesa-dev
mailing list