<div dir="ltr">On 16 September 2013 09:38, Eric Anholt <<a href="mailto:eric@anholt.net" target="_blank">eric@anholt.net</a>> wrote: <div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div class=""><div class="h5">Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> 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 <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> > --- > 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-><a href="http://batch.bo" target="_blank">batch.bo</a>, *out_offset + 4, > + bo, buffer_offset, I915_GEM_DOMAIN_SAMPLER, 0); </div></div>I think this will segfault if BO is NULL <div><div class="h5"> > + > + 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-><a href="http://batch.bo" target="_blank">batch.bo</a>, > - *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 */); </div></div>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) </blockquote><div> </div><div>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. </div></div></div></div>