[Mesa-dev] [PATCH 14/20] i965: Implement logic to set up and upload an image uniform.

[Pohjolainen, Topi]

On Wed, Aug 05, 2015 at 01:47:26PM +0300, Francisco Jerez wrote:
> "Pohjolainen, Topi" <topi.pohjolainen at intel.com> writes:
> 
> > On Wed, Aug 05, 2015 at 10:36:09AM +0300, Pohjolainen, Topi wrote:
> >> On Tue, Jul 21, 2015 at 07:38:49PM +0300, Francisco Jerez wrote:
> >> > v2: Move the image_params array back to brw_stage_prog_data.
> >> > ---
> >> >  src/mesa/drivers/dri/i965/brw_shader.cpp | 31 +++++++++++++++++++++++++++++++
> >> >  src/mesa/drivers/dri/i965/brw_shader.h   |  1 +
> >> >  2 files changed, 32 insertions(+)
> >> > 
> >> > diff --git a/src/mesa/drivers/dri/i965/brw_shader.cpp b/src/mesa/drivers/dri/i965/brw_shader.cpp
> >> > index 24bf42d..f7186a4 100644
> >> > --- a/src/mesa/drivers/dri/i965/brw_shader.cpp
> >> > +++ b/src/mesa/drivers/dri/i965/brw_shader.cpp
> >> > @@ -1402,3 +1402,34 @@ 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. */
> >> >  }
> >> > +
> >> > +void
> >> > +backend_shader::setup_image_uniform_values(const gl_uniform_storage *storage)
> >> > +{
> >> > +   const unsigned stage = _mesa_program_enum_to_shader_stage(prog->Target);
> >> > +
> >> > +   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];
> >> > +
> >> > +      /* Upload the brw_image_param structure.  The order is expected to match
> >> > +       * the BRW_IMAGE_PARAM_*_OFFSET defines.
> >> > +       */
> >> > +      setup_vector_uniform_values(
> >> > +         (const gl_constant_value *)&param->surface_idx, 1);
> >> > +      setup_vector_uniform_values(
> >> > +         (const gl_constant_value *)param->offset, 2);
> >> > +      setup_vector_uniform_values(
> >> > +         (const gl_constant_value *)param->size, 3);
> >> > +      setup_vector_uniform_values(
> >> > +         (const gl_constant_value *)param->stride, 4);
> >> > +      setup_vector_uniform_values(
> >> > +         (const gl_constant_value *)param->tiling, 3);
> >> > +      setup_vector_uniform_values(
> >> > +         (const gl_constant_value *)param->swizzling, 2);
> >> > +
> >> 
> >> I need to understand the concept of image index before I can tell how this
> >> works.
> >
> > The mechanism looks fine to me:
> >
> > Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> >
> >> 
> >> But I checked that the order and dimensions of the individual fields match the
> >> BRW_IMAGE_PARAM_* defines and the members of "struct brw_image_param". I
> >> noticed that in the structure the member "size" is the second while we give
> >> it as the third for the hw. Might be worth keep the order consistent between
> >> the two just for clarity. (Of course it works as is as the compiled program
> >> and the ubo-layout match).
> >> 
> 
> Hah, sure, I've reordered them locally so that the brw_image_param
> structure matches the order in which they are written to the push param
> array.  Do you want me to resend?

No, that's fine.