[Mesa-dev] [PATCH 14/33] i965/gen4: Fold WM surface state prepare()/emit() together.

[Paul Berry]

On 26 October 2011 11:45, Eric Anholt <eric at anholt.net> wrote:

> On Wed, 26 Oct 2011 10:11:15 -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:
> >
> > > These produce BRW_NEW_SURFACES (used by binding table emit()) and
> > > BRW_NEW_NR_WM_SURFACES (used by WM unit emit()).  Fixes a bug where
> > > with no texturing and no color buffer, we wouldn't consider the null
> > > renderbuffer in nr_surfaces.  This was harmless because nr_surfaces is
> > > only used for the prefetch info in the unit state.
>
> > > 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 bb7fd2e..782efd5 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > > +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > > @@ -541,47 +541,16 @@ brw_update_renderbuffer_surface(struct
> brw_context
> > > *brw,
> > >                           I915_GEM_DOMAIN_RENDER);
> > >  }
> > >
> > > -static void
> > > -prepare_wm_surfaces(struct brw_context *brw)
> > > -{
> > > -   struct gl_context *ctx = &brw->intel.ctx;
> > > -   int i;
> > > -   int nr_surfaces = 0;
> > > -
> > > -   for (i = 0; i < ctx->DrawBuffer->_NumColorDrawBuffers; i++) {
> > > -      nr_surfaces = SURF_INDEX_DRAW(i) + 1;
> > > -   }
> > > -
> > > -   if (brw->wm.const_bo) {
> > > -      nr_surfaces = SURF_INDEX_FRAG_CONST_BUFFER + 1;
> > > -   }
> > > -
> > > -   for (i = 0; i < BRW_MAX_TEX_UNIT; i++) {
> > > -      const struct gl_texture_unit *texUnit = &ctx->Texture.Unit[i];
> > > -
> > > -      if (texUnit->_ReallyEnabled) {
> > > -        nr_surfaces = SURF_INDEX_TEXTURE(i) + 1;
> > > -      }
> > > -   }
> > > -
> > > -   /* Have to update this in our prepare, since the unit's prepare
> > > -    * relies on it.
> > > -    */
> > > -   if (brw->wm.nr_surfaces != nr_surfaces) {
> > > -      brw->wm.nr_surfaces = nr_surfaces;
> > > -      brw->state.dirty.brw |= BRW_NEW_NR_WM_SURFACES;
> > > -   }
> > > -}
> > > -
> > >  /**
> > >  * Constructs the set of surface state objects pointed to by the
> > >  * binding table.
> > >  */
> > >  static void
> > > -upload_wm_surfaces(struct brw_context *brw)
> > > +brw_upload_wm_surfaces(struct brw_context *brw)
> > >  {
> > >    struct gl_context *ctx = &brw->intel.ctx;
> > >    GLuint i;
> > > +   int nr_surfaces = 0;
> > >
> > >    /* _NEW_BUFFERS | _NEW_COLOR */
> > >    /* Update surfaces for drawing buffers */
> > > @@ -595,8 +564,15 @@ upload_wm_surfaces(struct brw_context *brw)
> > >            brw_update_null_renderbuffer_surface(brw, i);
> > >         }
> > >       }
> > > +      nr_surfaces =
> > > SURF_INDEX_DRAW(ctx->DrawBuffer->_NumColorDrawBuffers);
> > >    } else {
> > >       brw_update_null_renderbuffer_surface(brw, 0);
> > > +      nr_surfaces = SURF_INDEX_DRAW(0) + 1;
> > >
> >
> > This looks like a behavioral change to me.  The old prepare_wm_surfaces()
> > code would have left nr_surfaces = 0 in this case.  Was this an
> intentional
> > change?  Was the old code buggy?
> >
> > I have similar questions about patch 15/33.
>
> I mentioned it in the commit message :)
>

Oops, so you did.  Would you mind mentioning it in the commit message for
patch 15/33 also?  I failed to notice because I reviewed patch 15/33 first
:)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20111026/871525cb/attachment.htm>