[Mesa-dev] [PATCH 05/13] i965: Fix the untyped surface opcodes to deal with indirect surface access.

[Pohjolainen, Topi]

On Thu, Mar 12, 2015 at 07:25:34PM +0200, Francisco Jerez wrote:
> "Pohjolainen, Topi" <topi.pohjolainen at intel.com> writes:
> 
> > On Fri, Feb 27, 2015 at 05:34:48PM +0200, Francisco Jerez wrote:
> >> Change brw_untyped_atomic() and brw_untyped_surface_read() to take the
> >> surface index as a register instead of a constant and to use
> >> brw_send_indirect_message() to emit the indirect variant of send with
> >> a dynamically calculated message descriptor.  This will be required to
> >> support variable indexing of image arrays for
> >> ARB_shader_image_load_store.
> >> ---
> >>  src/mesa/drivers/dri/i965/brw_eu.h               |  10 +-
> >>  src/mesa/drivers/dri/i965/brw_eu_emit.c          | 158 +++++++++++++----------
> >>  src/mesa/drivers/dri/i965/brw_fs_generator.cpp   |   4 +-
> >>  src/mesa/drivers/dri/i965/brw_vec4_generator.cpp |   4 +-
> >>  4 files changed, 96 insertions(+), 80 deletions(-)
> >> 
> >> diff --git a/src/mesa/drivers/dri/i965/brw_eu.h b/src/mesa/drivers/dri/i965/brw_eu.h
> >> index 87a9f3f..9cc9123 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_eu.h
> >> +++ b/src/mesa/drivers/dri/i965/brw_eu.h
> >> @@ -398,18 +398,18 @@ void brw_CMP(struct brw_compile *p,
> >>  
> >>  void
> >>  brw_untyped_atomic(struct brw_compile *p,
> >> -                   struct brw_reg dest,
> >> +                   struct brw_reg dst,
> >>                     struct brw_reg payload,
> >> +                   struct brw_reg surface,
> >>                     unsigned atomic_op,
> >> -                   unsigned bind_table_index,
> >>                     unsigned msg_length,
> >>                     bool response_expected);
> >>  
> >>  void
> >>  brw_untyped_surface_read(struct brw_compile *p,
> >> -                         struct brw_reg dest,
> >> -                         struct brw_reg mrf,
> >> -                         unsigned bind_table_index,
> >> +                         struct brw_reg dst,
> >> +                         struct brw_reg payload,
> >> +                         struct brw_reg surface,
> >>                           unsigned msg_length,
> >>                           unsigned num_channels);
> >>  
> >> diff --git a/src/mesa/drivers/dri/i965/brw_eu_emit.c b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> >> index 0b655d4..34695bf 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_eu_emit.c
> >> +++ b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> >> @@ -2518,6 +2518,48 @@ brw_send_indirect_message(struct brw_compile *p,
> >>     return setup;
> >>  }
> >>  
> >> +static struct brw_inst *
> >> +brw_send_indirect_surface_message(struct brw_compile *p,
> >> +                                  unsigned sfid,
> >> +                                  struct brw_reg dst,
> >> +                                  struct brw_reg payload,
> >> +                                  struct brw_reg surface,
> >> +                                  unsigned message_len,
> >> +                                  unsigned response_len,
> >> +                                  bool header_present)
> >> +{
> >> +   const struct brw_context *brw = p->brw;
> >> +   struct brw_inst *insn;
> >> +
> >> +   if (surface.file != BRW_IMMEDIATE_VALUE) {
> >> +      struct brw_reg addr = retype(brw_address_reg(0), BRW_REGISTER_TYPE_UD);
> >> +
> >> +      brw_push_insn_state(p);
> >> +      brw_set_default_access_mode(p, BRW_ALIGN_1);
> >> +      brw_set_default_mask_control(p, BRW_MASK_DISABLE);
> >> +      brw_set_default_predicate_control(p, BRW_PREDICATE_NONE);
> >> +
> >> +      /* Mask out invalid bits from the surface index to avoid hangs e.g. when
> >> +       * some surface array is accessed out of bounds.
> >> +       */
> >> +      insn = brw_AND(p, addr,
> >> +                     suboffset(vec1(retype(surface, BRW_REGISTER_TYPE_UD)),
> >> +                               BRW_GET_SWZ(surface.dw1.bits.swizzle, 0)),
> >> +                     brw_imm_ud(0xff));
> >> +
> >> +      brw_pop_insn_state(p);
> >> +
> >> +      surface = addr;
> >
> > Also this patch didn't really need this branch at all. Instead it could
> > have been its own patch and here just:
> >
> >       assert(surface.file == BRW_IMMEDIATE_VALUE);
> >
> The whole point of this patch is to fix untyped surface message sends to
> deal with non-immediate "surface" arguments.  How could it not be
> needed?

I mean that the patch could be split in two parts: one doing the
re-organization and another adding the non-immediate branch to
brw_send_indirect_surface_message().
If I'm reading the patch right, brw_untyped_surface_read() and
brw_untyped_atomic() are only called with surface indices of immediate type.
Hence the added branch is not yet hit in runtime at this point.