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

[Pohjolainen, Topi]

On Thu, Aug 20, 2015 at 11:20:31AM +0300, Pohjolainen, Topi wrote:
> 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.

I tried with jenkins and piglit doesn't get upset about it. I guess this is
a path that just doesn't get tickled.
Therefore I'm even more grateful for the careful review :)

> 
> > 
> > > +         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.

Well, actually I had a reason for the jump. The latter case will get augmented
quite a bit by the rest of the series and I wanted to keep the indentation
level (instead of growing the else-branch). I'd rather have it this way if
you don't mind.