<div dir="ltr">On 13 September 2013 23:10, Kenneth Graunke <<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</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">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>> </blockquote><div> </div><div>I really like what you're doing here. A few minor comments: </div><div> </div> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> --- 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) | </blockquote><div> </div><div>These three instances of buffer_size should be (buffer_size - 1). I think that there was a pre-existing bug in gen7_update_buffer_texture_surface() where it wasn't subtracting 1 where it should. Probably we should fix the bug as a pre-patch. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + (pitch - 1); + surf[4] = 0; </blockquote><div> </div><div>Technically this line is unnecessary given the memset() above. I'm quibbling, though--it's hard to imagine this making a significant performance difference :) </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + 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); + + 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 */); } static void @@ -371,38 +386,15 @@ gen7_create_constant_surface(struct brw_context *brw, { uint32_t stride = dword_pitch ? 4 : 16; uint32_t elements = ALIGN(size, stride) / stride; - const GLint w = elements - 1; - 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 | - BRW_SURFACEFORMAT_R32G32B32A32_FLOAT << BRW_SURFACE_FORMAT_SHIFT | - BRW_SURFACE_RC_READ_WRITE; - - assert(bo); - surf[1] = bo->offset + offset; /* reloc */ - - /* note that these differ from GEN6 */ - surf[2] = SET_FIELD(w & 0x7f, GEN7_SURFACE_WIDTH) | - SET_FIELD((w >> 7) & 0x3fff, GEN7_SURFACE_HEIGHT); - surf[3] = SET_FIELD((w >> 21) & 0x3f, BRW_SURFACE_DEPTH) | - (stride - 1); - - if (brw->is_haswell) { - surf[7] = SET_FIELD(HSW_SCS_RED, GEN7_SURFACE_SCS_R) | - SET_FIELD(HSW_SCS_GREEN, GEN7_SURFACE_SCS_G) | - SET_FIELD(HSW_SCS_BLUE, GEN7_SURFACE_SCS_B) | - SET_FIELD(HSW_SCS_ALPHA, GEN7_SURFACE_SCS_A); - } </blockquote><div> </div><div>I don't see this Haswell-specific code in gen7_emit_buffer_surface_state(). Is that a problem? </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> - - drm_intel_bo_emit_reloc(brw-><a href="http://batch.bo" target="_blank">batch.bo</a>, - *out_offset + 4, - bo, offset, - I915_GEM_DOMAIN_SAMPLER, 0); - - gen7_check_surface_setup(surf, false /* is_render_target */); + gen7_emit_buffer_surface_state(brw, + out_offset, + bo, + offset, + BRW_SURFACEFORMAT_R32G32B32A32_FLOAT, + elements - 1, + stride, + 0 /* mocs */); } /** @@ -411,34 +403,14 @@ gen7_create_constant_surface(struct brw_context *brw, void gen7_create_shader_time_surface(struct brw_context *brw, uint32_t *out_offset) { - const int w = brw->shader_time.bo->size - 1; - - 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 | - BRW_SURFACEFORMAT_RAW << BRW_SURFACE_FORMAT_SHIFT | - BRW_SURFACE_RC_READ_WRITE; - - surf[1] = brw->shader_time.bo->offset; /* reloc */ - - /* note that these differ from GEN6 */ - surf[2] = SET_FIELD(w & 0x7f, GEN7_SURFACE_WIDTH) | - SET_FIELD((w >> 7) & 0x3fff, GEN7_SURFACE_HEIGHT); - surf[3] = SET_FIELD((w >> 21) & 0x3f, BRW_SURFACE_DEPTH); - - /* Unlike texture or renderbuffer surfaces, we only do untyped operations - * on the shader_time surface, so there's no need to set HSW channel - * overrides. - */ - - drm_intel_bo_emit_reloc(brw-><a href="http://batch.bo" target="_blank">batch.bo</a>, - *out_offset + 4, - brw-><a href="http://shader_time.bo" target="_blank">shader_time.bo</a>, 0, - I915_GEM_DOMAIN_SAMPLER, 0); - - gen7_check_surface_setup(surf, false /* is_render_target */); + gen7_emit_buffer_surface_state(brw, + out_offset, + brw-><a href="http://shader_time.bo" target="_blank">shader_time.bo</a>, + 0, + BRW_SURFACEFORMAT_RAW, + brw->shader_time.bo->size - 1, + 1, + 0 /* mocs */); } static void -- 1.8.3.4 _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div></div>