[Mesa-dev] [PATCH 1/7] i965: Move texture buffer dispatch into single location

[Pohjolainen, Topi]

On Thu, May 07, 2015 at 05:55:48PM +0300, Francisco Jerez wrote:
> "Pohjolainen, Topi" <topi.pohjolainen at intel.com> writes:
> 
> > On Thu, May 07, 2015 at 05:15:35PM +0300, Francisco Jerez wrote:
> >> From: Topi Pohjolainen <topi.pohjolainen at intel.com>
> >> 
> >> All generations do the same exact dispatch and it could be
> >> therefore done in the hardware independent stage.
> >> 
> >> Reviewed-by: Matt Turner <mattst88 at gmail.com>
> >> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
> >> Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> >> [ Francisco Jerez: Non-trivial rebase. ]
> >> Reviewed-by: Francisco Jerez <currojerez at riseup.net>
> >> ---
> >>  src/mesa/drivers/dri/i965/brw_context.h           |  3 -
> >>  src/mesa/drivers/dri/i965/brw_wm_surface_state.c  | 31 ++++++----
> >>  src/mesa/drivers/dri/i965/gen7_wm_surface_state.c | 69 +++++++++++------------
> >>  src/mesa/drivers/dri/i965/gen8_surface_state.c    | 67 ++++++++++------------
> >>  4 files changed, 83 insertions(+), 87 deletions(-)
> >> 
> >> diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h
> >> index 2fcdcfa..a6282f4 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_context.h
> >> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> >> @@ -1698,9 +1698,6 @@ void brw_create_constant_surface(struct brw_context *brw,
> >>                                   uint32_t size,
> >>                                   uint32_t *out_offset,
> >>                                   bool dword_pitch);
> >> -void brw_update_buffer_texture_surface(struct gl_context *ctx,
> >> -                                       unsigned unit,
> >> -                                       uint32_t *surf_offset);
> >>  void
> >>  brw_update_sol_surface(struct brw_context *brw,
> >>                         struct gl_buffer_object *buffer_obj,
> >> diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> >> index 160dd2f..2b8040c 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> >> +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> >> @@ -274,10 +274,10 @@ gen4_emit_buffer_surface_state(struct brw_context *brw,
> >>     }
> >>  }
> >>  
> >> -void
> >> -brw_update_buffer_texture_surface(struct gl_context *ctx,
> >> -                                  unsigned unit,
> >> -                                  uint32_t *surf_offset)
> >> +static void
> >> +update_buffer_texture_surface(struct gl_context *ctx,
> >> +                              unsigned unit,
> >> +                              uint32_t *surf_offset)
> >>  {
> >>     struct brw_context *brw = brw_context(ctx);
> >>     struct gl_texture_object *tObj = ctx->Texture.Unit[unit]._Current;
> >> @@ -320,12 +320,6 @@ brw_update_texture_surface(struct gl_context *ctx,
> >>     struct gl_sampler_object *sampler = _mesa_get_samplerobj(ctx, unit);
> >>     uint32_t *surf;
> >>  
> >> -   /* BRW_NEW_TEXTURE_BUFFER */
> >> -   if (tObj->Target == GL_TEXTURE_BUFFER) {
> >> -      brw_update_buffer_texture_surface(ctx, unit, surf_offset);
> >> -      return;
> >> -   }
> >> -
> >>     surf = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE,
> >>  			  6 * 4, 32, surf_offset);
> >>  
> >> @@ -795,6 +789,21 @@ const struct brw_tracked_state gen6_renderbuffer_surfaces = {
> >>     .emit = update_renderbuffer_surfaces,
> >>  };
> >>  
> >> +static void
> >> +update_texture_surface(struct gl_context *ctx,
> >> +                       unsigned unit,
> >> +                       uint32_t *surf_offset,
> >> +                       bool for_gather)
> >> +{
> >> +   struct brw_context *brw = brw_context(ctx);
> >> +   struct gl_texture_object *obj = ctx->Texture.Unit[unit]._Current;
> >> +
> >> +   if (obj->Target == GL_TEXTURE_BUFFER) {
> >> +      update_buffer_texture_surface(ctx, unit, surf_offset);
> >
> > In order to avoid extra level of indentation I used the following. I would
> > have preferred it here also.
> >
> >       if (obj->Target == GL_TEXTURE_BUFFER) {
> >          update_buffer_texture_surface(ctx, unit, surf_offset);
> >          return;
> >       }
> >
> I kept this as an indented block because it's harmless IMHO and it
> seemed a somewhat lesser evil than:
> 1/ Define all texture-specific variables (i.e. things that are not
>    applicable to buffer textures, including some pointer dereferences)
>    at the top level, which is what you did, but it seemed a bit dodgy.
> 2/ Mix statements and declarations. (Granted, this file is likely
>    already relying on other C99 features, so it wouldn't matter in
>    practice, it's just a codestyle itch)
> 3/ Declare stuff and leave it uninitialized until later.
> 
> That said, the reason was largely subjective, and I don't really have a
> strong preference.  As you are still the author of this commit you're
> free to format it as you wish, you can keep my R-b if you simply
> reindent this function.

If Ken and Matt are happy with this series, so am I. I'm just glad if we
can land it.