[Mesa-dev] [PATCH 03/10] i965: Move tex buffer dispatch into hw independent setup

[Pohjolainen, Topi]

On Wed, Aug 19, 2015 at 05:34:14PM +0300, Francisco Jerez wrote:
> Topi Pohjolainen <topi.pohjolainen at intel.com> writes:
> 
> > All generations do the same exact dispatch and it could be
> > therefore done in the hardware independent stage.
> >
> > v2: Rebased where there are still duplicate calls in gen7 and gen8
> >     handlers. These will be dropped in subsequent patches.
> >
> > Reviewed-by: Matt Turner <mattst88 at gmail.com> (v1)
> > Reviewed-by: Kenneth Graunke <kenneth at whitecape.org> (v1)
> > Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> > ---
> >  src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 28 ++++++++++++++----------
> >  1 file changed, 16 insertions(+), 12 deletions(-)
> >
> > 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 73aa719..dca67e8 100644
> > --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > @@ -319,12 +319,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(brw, tObj, surf_offset);
> > -      return;
> > -   }
> > -
> >     surf = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE,
> >  			  6 * 4, 32, surf_offset);
> >  
> > @@ -819,14 +813,24 @@ update_stage_texture_surfaces(struct brw_context *brw,
> >     for (unsigned s = 0; s < num_samplers; s++) {
> >        surf_offset[s] = 0;
> >  
> > -      if (prog->SamplersUsed & (1 << s)) {
> > -         const unsigned unit = prog->SamplerUnits[s];
> > +      if (!(prog->SamplersUsed & (1 << s)))
> > +         continue;
> >  
> > -         /* _NEW_TEXTURE */
> > -         if (ctx->Texture.Unit[unit]._Current) {
> > -            brw->vtbl.update_texture_surface(ctx, unit, surf_offset + s, for_gather);
> > -         }
> > +      const unsigned unit = prog->SamplerUnits[s];
> > +      struct gl_texture_object *tex = ctx->Texture.Unit[unit]._Current;
> > +
> > +      if (!tex)
> > +         continue;
> > +
> > +      /* BRW_NEW_TEXTURE_BUFFER */
> > +      if (tex->Target == GL_TEXTURE_BUFFER) {
> > +         brw_update_buffer_texture_surface(brw, tex, surf_offset);
> 
> You probably didn't mean to always pass the first surface state entry
> (missing "+ s"?).

Oh, that's really bad. Many thanks for the careful review. I wonder how I
got clean piglit run, I need to recheck this.

> 
> > +         continue;
> >        }
> > +
> > +      /* _NEW_TEXTURE */
> > +      brw->vtbl.update_texture_surface(ctx, unit,
> > +                                       surf_offset + s, for_gather);
> 
> I'd keep the control flow structured here instead of adding a jump,
> like:
> 
> | if (tex->Target == GL_TEXTURE_BUFFER) {
> |  // Handle buffer textures.
> | } else {
> |  // Handle non-buffer textures.
> | }

I fully agree, thanks.

> 
> Anyway, just nitpicking.  With these fixed:
> 
> Reviewed-by: Francisco Jerez <currojerez at riseup.net>
> 
> >     }
> >  }
> >  
> > -- 
> > 1.9.3
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev