[Mesa-dev] [PATCH 1/4] i965: Refactor Gen7+ SURFACE_STATE setup for buffer surfaces.

[Paul Berry]

On 16 September 2013 09:38, Eric Anholt <eric at anholt.net> wrote:

> Kenneth Graunke <kenneth at whitecape.org> writes:
>
> > This was an embarassingly large amount of copy and pasted code,
> > and it wasn't particularly simple code either.  By factoring it out
> > into a helper function, we consolidate the complexity.
> >
> > Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> > ---
> >  src/mesa/drivers/dri/i965/gen7_wm_surface_state.c | 144
> +++++++++-------------
> >  1 file changed, 58 insertions(+), 86 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
> b/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
> > index 37e3174..8413308 100644
> > --- a/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
> > +++ b/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
> > @@ -224,6 +224,37 @@ gen7_check_surface_setup(uint32_t *surf, bool
> is_render_target)
> >     }
> >  }
> >
> > +static void
> > +gen7_emit_buffer_surface_state(struct brw_context *brw,
> > +                               uint32_t *out_offset,
> > +                               drm_intel_bo *bo,
> > +                               unsigned buffer_offset,
> > +                               unsigned surface_format,
> > +                               unsigned buffer_size,
> > +                               unsigned pitch,
> > +                               unsigned mocs)
> > +{
> > +   uint32_t *surf = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE,
> > +                                    8 * 4, 32, out_offset);
> > +   memset(surf, 0, 8 * 4);
> > +
> > +   surf[0] = BRW_SURFACE_BUFFER << BRW_SURFACE_TYPE_SHIFT |
> > +             surface_format << BRW_SURFACE_FORMAT_SHIFT |
> > +             BRW_SURFACE_RC_READ_WRITE;
> > +   surf[1] = bo->offset + buffer_offset; /* reloc */
> > +   surf[2] = SET_FIELD(buffer_size & 0x7f, GEN7_SURFACE_WIDTH) |
> > +             SET_FIELD((buffer_size >> 7) & 0x3fff,
> GEN7_SURFACE_HEIGHT);
> > +   surf[3] = SET_FIELD((buffer_size >> 21) & 0x3f, BRW_SURFACE_DEPTH) |
> > +             (pitch - 1);
> > +   surf[4] = 0;
> > +   surf[5] = SET_FIELD(mocs, GEN7_SURFACE_MOCS);
> > +
> > +   /* Emit relocation to surface contents */
> > +   drm_intel_bo_emit_reloc(brw->batch.bo, *out_offset + 4,
> > +                           bo, buffer_offset, I915_GEM_DOMAIN_SAMPLER,
> 0);
>
> I think this will segfault if BO is NULL
>
> > +
> > +   gen7_check_surface_setup(surf, false /* is_render_target */);
> > +}
> >
> >  static void
> >  gen7_update_buffer_texture_surface(struct gl_context *ctx,
> > @@ -237,39 +268,23 @@ gen7_update_buffer_texture_surface(struct
> gl_context *ctx,
> >     drm_intel_bo *bo = intel_obj ? intel_obj->buffer : NULL;
> >     gl_format format = tObj->_BufferObjectFormat;
> >
> > -   uint32_t *surf = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE,
> > -                                    8 * 4, 32, surf_offset);
> > -   memset(surf, 0, 8 * 4);
> > -
> >     uint32_t surface_format = brw_format_for_mesa_format(format);
> >     if (surface_format == 0 && format != MESA_FORMAT_RGBA_FLOAT32) {
> >        _mesa_problem(NULL, "bad format %s for texture buffer\n",
> >                      _mesa_get_format_name(format));
> >     }
> >
> > -   surf[0] = BRW_SURFACE_BUFFER << BRW_SURFACE_TYPE_SHIFT |
> > -             surface_format << BRW_SURFACE_FORMAT_SHIFT |
> > -             BRW_SURFACE_RC_READ_WRITE;
> > -
> > -   if (bo) {
> > -      surf[1] = bo->offset; /* reloc */
> > -
> > -      drm_intel_bo_emit_reloc(brw->batch.bo,
> > -                           *surf_offset + 4,
> > -                           bo, 0,
> > -                           I915_GEM_DOMAIN_SAMPLER, 0);
> > -
> > -      int texel_size = _mesa_get_format_bytes(format);
> > -      int w = intel_obj->Base.Size / texel_size;
> > -
> > -      /* note that these differ from GEN6 */
> > -      surf[2] = SET_FIELD(w & 0x7f, GEN7_SURFACE_WIDTH) | /* bits 6:0
> of size */
> > -                SET_FIELD((w >> 7) & 0x3fff, GEN7_SURFACE_HEIGHT); /*
> 20:7 */
> > -      surf[3] = SET_FIELD((w >> 21) & 0x3f, BRW_SURFACE_DEPTH) | /*
> bits 26:21 */
> > -                (texel_size - 1);
> > -   }
> > -
> > -   gen7_check_surface_setup(surf, false /* is_render_target */);
> > +   int texel_size = _mesa_get_format_bytes(format);
> > +   int w = intel_obj ? intel_obj->Base.Size / texel_size : 0;
> > +
> > +   gen7_emit_buffer_surface_state(brw,
> > +                                  surf_offset,
> > +                                  bo,
> > +                                  0,
> > +                                  surface_format,
> > +                                  w,
> > +                                  texel_size,
> > +                                  0 /* mocs */);
>
> But here you might pass in a NULL bo.  Same for patch 4.  I'm confused
> that this didn't produce a piglit regression.
>
> >  }
>
> I think the savings is well worth it in patches 1, 3, and 4, but I'm not
> a fan of patch 2.  I've got a lot of branches outstanding where I'm
> trying to finally finish off mt->first_level and the tile offset usage,
> and sharing code between renderbuffers and textures is just going to
> make that harder to do.  (I'm already really tempted to split gen4/5
> From gen6 RB setup, since I think I could land gen4/5 without
> regressing, while gen6 is the unstable hw)
>

I really like patch 2 and would like to see it land.  Can you expand on why
it would complicate your work with mt->first_level and tile offset usage?
AFAICT, the first_level stuff doesn't really seem to be affected by patch 2
(it's just moved from a subexpression in surf[5] to a parameter on
gen7_emit_image_surface_state()), and the tile offset logic has already
been removed from gen7, so it should be unrelated to patch 2.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130916/a667b5c3/attachment.html>