[Mesa-dev] [PATCH 06/11] i965/shader: Pull setup_image_uniform_values out of backend_shader

[Iago Toral]

On Wed, 2015-09-30 at 18:41 -0700, Jason Ekstrand wrote:
> I tried to do this once before but Curro pointed out that having it in
> backend_shader meant it could use the setup_vec4_uniform_values helper
> which did different things in vec4 and fs.  Now the setup_uniform_values
> function differs only by an assert in the two backends so there's no real
> good reason to be using it anymore.
> ---
>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp |  3 +-
>  src/mesa/drivers/dri/i965/brw_shader.cpp | 52 +++++++++++++++++++++-----------
>  src/mesa/drivers/dri/i965/brw_shader.h   |  7 +++--
>  3 files changed, 42 insertions(+), 20 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> index 7a965cd..d33e452 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> @@ -236,7 +236,8 @@ fs_visitor::nir_setup_uniform(nir_variable *var)
>        }
>  
>        if (storage->type->is_image()) {
> -         setup_image_uniform_values(index, storage);
> +         brw_setup_image_uniform_values(stage, stage_prog_data,
> +                                        index, storage);
>        } 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 72388ce..1d184a7 100644
> --- a/src/mesa/drivers/dri/i965/brw_shader.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_shader.cpp
> @@ -1419,32 +1419,50 @@ 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_vec4_uniform_value(const gl_constant_value **params,
> +                         const gl_constant_value *values,
> +                         unsigned n)
> +{
> +   static const gl_constant_value zero = { 0 };
> +
> +   for (unsigned i = 0; i < n; ++i)
> +      params[i] = &values[i];
> +
> +   for (unsigned i = n; i < 4; ++i)
> +      params[i] = &zero;
> +}

I actually liked the version that received an offset into params better,
since that allows us to assert that we are not messing our writes to
params. Not a big deal though, so either way:

Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>

>  void
> -backend_shader::setup_image_uniform_values(unsigned param_offset,
> -                                           const gl_uniform_storage *storage)
> +brw_setup_image_uniform_values(gl_shader_stage stage,
> +                               struct brw_stage_prog_data *stage_prog_data,
> +                               unsigned param_start_index,
> +                               const gl_uniform_storage *storage)
>  {
> -   const unsigned stage = _mesa_program_enum_to_shader_stage(prog->Target);
> +   const gl_constant_value **param =
> +      &stage_prog_data->param[param_start_index];
>  
>     for (unsigned i = 0; i < MAX2(storage->array_elements, 1); i++) {
>        const unsigned image_idx = storage->image[stage].index + i;
> -      const brw_image_param *param = &stage_prog_data->image_param[image_idx];
> +      const brw_image_param *image_param =
> +         &stage_prog_data->image_param[image_idx];
>  
>        /* Upload the brw_image_param structure.  The order is expected to match
>         * the BRW_IMAGE_PARAM_*_OFFSET defines.
>         */
> -      setup_vec4_uniform_value(param_offset + BRW_IMAGE_PARAM_SURFACE_IDX_OFFSET,
> -         (const gl_constant_value *)&param->surface_idx, 1);
> -      setup_vec4_uniform_value(param_offset + BRW_IMAGE_PARAM_OFFSET_OFFSET,
> -         (const gl_constant_value *)param->offset, 2);
> -      setup_vec4_uniform_value(param_offset + BRW_IMAGE_PARAM_SIZE_OFFSET,
> -         (const gl_constant_value *)param->size, 3);
> -      setup_vec4_uniform_value(param_offset + BRW_IMAGE_PARAM_STRIDE_OFFSET,
> -         (const gl_constant_value *)param->stride, 4);
> -      setup_vec4_uniform_value(param_offset + BRW_IMAGE_PARAM_TILING_OFFSET,
> -         (const gl_constant_value *)param->tiling, 3);
> -      setup_vec4_uniform_value(param_offset + BRW_IMAGE_PARAM_SWIZZLING_OFFSET,
> -         (const gl_constant_value *)param->swizzling, 2);
> -      param_offset += BRW_IMAGE_PARAM_SIZE;
> +      setup_vec4_uniform_value(param + BRW_IMAGE_PARAM_SURFACE_IDX_OFFSET,
> +         (const gl_constant_value *)&image_param->surface_idx, 1);
> +      setup_vec4_uniform_value(param + BRW_IMAGE_PARAM_OFFSET_OFFSET,
> +         (const gl_constant_value *)image_param->offset, 2);
> +      setup_vec4_uniform_value(param + BRW_IMAGE_PARAM_SIZE_OFFSET,
> +         (const gl_constant_value *)image_param->size, 3);
> +      setup_vec4_uniform_value(param + BRW_IMAGE_PARAM_STRIDE_OFFSET,
> +         (const gl_constant_value *)image_param->stride, 4);
> +      setup_vec4_uniform_value(param + BRW_IMAGE_PARAM_TILING_OFFSET,
> +         (const gl_constant_value *)image_param->tiling, 3);
> +      setup_vec4_uniform_value(param + BRW_IMAGE_PARAM_SWIZZLING_OFFSET,
> +         (const gl_constant_value *)image_param->swizzling, 2);
> +      param += BRW_IMAGE_PARAM_SIZE;
>  
>        brw_mark_surface_used(
>           stage_prog_data,
> diff --git a/src/mesa/drivers/dri/i965/brw_shader.h b/src/mesa/drivers/dri/i965/brw_shader.h
> index ccccf4d..ee6afce 100644
> --- a/src/mesa/drivers/dri/i965/brw_shader.h
> +++ b/src/mesa/drivers/dri/i965/brw_shader.h
> @@ -273,12 +273,15 @@ public:
>     virtual void setup_vec4_uniform_value(unsigned param_offset,
>                                           const gl_constant_value *values,
>                                           unsigned n) = 0;
> -   void setup_image_uniform_values(unsigned param_offset,
> -                                   const gl_uniform_storage *storage);
>  };
>  
>  uint32_t brw_texture_offset(int *offsets, unsigned num_components);
>  
> +void brw_setup_image_uniform_values(gl_shader_stage stage,
> +                                    struct brw_stage_prog_data *stage_prog_data,
> +                                    unsigned param_start_index,
> +                                    const gl_uniform_storage *storage);
> +
>  #endif /* __cplusplus */
>  
>  enum brw_reg_type brw_type_for_base_type(const struct glsl_type *type);