[Mesa-dev] [PATCH 17/33] i965: Fold prepare() and emit() of VS surface state setup together.

[Eric Anholt]

On Thu, 27 Oct 2011 10:27:40 -0700, Paul Berry <stereotype441 at gmail.com> wrote:
Non-text part: multipart/alternative
> On 24 October 2011 14:17, Eric Anholt <eric at anholt.net> wrote:
> 
> > This rearranges the code a bit, and makes the upload of the binding
> > table take only as many surfaces as there are in use.
> > ---
> >  src/mesa/drivers/dri/i965/brw_vs_surface_state.c |   62
> > +++++++++-------------
> >  1 files changed, 25 insertions(+), 37 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_vs_surface_state.c
> > b/src/mesa/drivers/dri/i965/brw_vs_surface_state.c
> > index bfc4f5d..0237b58 100644
> > --- a/src/mesa/drivers/dri/i965/brw_vs_surface_state.c
> > +++ b/src/mesa/drivers/dri/i965/brw_vs_surface_state.c
> > @@ -138,22 +138,6 @@ brw_update_vs_constant_surface( struct gl_context
> > *ctx,
> >    }
> >  }
> >
> > -
> > -static void
> > -prepare_vs_surfaces(struct brw_context *brw)
> > -{
> > -   int nr_surfaces = 0;
> > -
> > -   if (brw->vs.const_bo) {
> > -      nr_surfaces = 1;
> > -   }
> > -
> > -   if (brw->vs.nr_surfaces != nr_surfaces) {
> > -      brw->state.dirty.brw |= BRW_NEW_NR_VS_SURFACES;
> > -      brw->vs.nr_surfaces = nr_surfaces;
> > -   }
> > -}
> > -
> >  /**
> >  * Vertex shader surfaces (constant buffer).
> >  *
> > @@ -161,36 +145,41 @@ prepare_vs_surfaces(struct brw_context *brw)
> >  * to be updated, and produces BRW_NEW_NR_VS_SURFACES for the VS unit and
> >  * CACHE_NEW_SURF_BIND for the binding table upload.
> >  */
> > -static void upload_vs_surfaces(struct brw_context *brw)
> > +static void
> > +brw_upload_vs_surfaces(struct brw_context *brw)
> >  {
> >    struct gl_context *ctx = &brw->intel.ctx;
> >    uint32_t *bind;
> >    int i;
> > +   int nr_surfaces = 0;
> > +
> > +   /* BRW_NEW_VS_CONSTBUF */
> > +   if (brw->vs.const_bo) {
> > +      nr_surfaces = 1;
> > +      brw_update_vs_constant_surface(ctx, SURF_INDEX_VERT_CONST_BUFFER);
> > +   }
> > +
> > +   if (nr_surfaces != 0) {
> >
> 
> This reads a little strange to me.  The only way nr_surfaces can be nonzero
> is if the previous if test succeeded, so why not just merge the two if
> statements?
> 
> 
> > +      bind = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE,
> > +                            sizeof(uint32_t) * nr_surfaces,
> > +                            32, &brw->vs.bind_bo_offset);
> >
> > -   /* BRW_NEW_NR_VS_SURFACES */
> > -   if (brw->vs.nr_surfaces == 0) {
> > +      for (i = 0; i < nr_surfaces; i++) {
> > +        /* BRW_NEW_VS_CONSTBUF */
> > +        bind[i] = brw->vs.surf_offset[i];
> > +      }
> >
> 
> This for loop also seems weird, since at this point in the code nr_surfaces
> is known to be equal to 1.  Why not just do "bind[0] =
> brw->vs.surf_offset[0];"?

Basically, because this is only 1/2 of what this function should be
doing, and it's about to be get a new block increasing nr_surfaces for
vertex texturing.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20111029/435abe9c/attachment.pgp>