<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">The SURFACE_STATE entries for textures and renderbuffers share almost all of the same fields. Only a couple are specific to one or the other. Thus, it makes sense to have a single shared function that takes care of all the bit-shifting required to assemble the SURFACE_STATE structure. This removes a lot of complicated cut and pasted code. One change is that we now specify cube face enables for render targets, but as far as I can tell this is harmless. 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 | 210 ++++++++++------------ 1 file changed, 99 insertions(+), 111 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 8413308..8f95abe 100644 --- a/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c +++ b/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c @@ -257,6 +257,70 @@ gen7_emit_buffer_surface_state(struct brw_context *brw, } static void +gen7_emit_image_surface_state(struct brw_context *brw, + uint32_t *out_offset, + const struct intel_mipmap_tree *mt, + unsigned bo_offset, + unsigned surface_type, + unsigned surface_format, + bool is_array, + unsigned depth, + unsigned min_array_element, + unsigned rt_view_extent, + unsigned mocs, + unsigned mip_count, + int swizzle, + bool is_render_target) +{ + uint32_t *surf = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE, + 8 * 4, 32, out_offset); + surf[0] = surface_type << BRW_SURFACE_TYPE_SHIFT | + surface_format << BRW_SURFACE_FORMAT_SHIFT | + gen7_surface_tiling_mode(mt->region->tiling) | + BRW_SURFACE_CUBEFACE_ENABLES | + (mt->align_h == 4 ? GEN7_SURFACE_VALIGN_4 : GEN7_SURFACE_VALIGN_2) | + (mt->align_w == 8 ? GEN7_SURFACE_HALIGN_8 : GEN7_SURFACE_HALIGN_4) | + (is_array ? GEN7_SURFACE_IS_ARRAY : 0) | + (mt->array_spacing_lod0 ? GEN7_SURFACE_ARYSPC_LOD0 + : GEN7_SURFACE_ARYSPC_FULL); + surf[1] = mt->region->bo->offset + bo_offset; /* reloc */ + surf[2] = SET_FIELD(mt->logical_width0 - 1, GEN7_SURFACE_WIDTH) | + SET_FIELD(mt->logical_height0 - 1, GEN7_SURFACE_HEIGHT); + surf[3] = SET_FIELD(depth - 1, BRW_SURFACE_DEPTH) | + (mt->region->pitch - 1); + surf[4] = min_array_element << GEN7_SURFACE_MIN_ARRAY_ELEMENT_SHIFT | + rt_view_extent << GEN7_SURFACE_RENDER_TARGET_VIEW_EXTENT_SHIFT | + gen7_surface_msaa_bits(mt->num_samples, mt->msaa_layout); + surf[5] = SET_FIELD(mocs, GEN7_SURFACE_MOCS) | mip_count; + + if (mt->mcs_mt) { + gen7_set_surface_mcs_info(brw, surf, *out_offset, mt->mcs_mt, true); + } else { + surf[6] = 0; + } + + surf[7] = mt->fast_clear_color_value; + + if (brw->is_haswell) { + surf[7] |= + SET_FIELD(swizzle_to_scs(GET_SWZ(swizzle, 0)), GEN7_SURFACE_SCS_R) | + SET_FIELD(swizzle_to_scs(GET_SWZ(swizzle, 1)), GEN7_SURFACE_SCS_G) | + SET_FIELD(swizzle_to_scs(GET_SWZ(swizzle, 2)), GEN7_SURFACE_SCS_B) | + SET_FIELD(swizzle_to_scs(GET_SWZ(swizzle, 3)), GEN7_SURFACE_SCS_A); + } + + uint32_t read_domain = + is_render_target ? I915_GEM_DOMAIN_RENDER : I915_GEM_DOMAIN_SAMPLER; + + /* Emit relocation to surface contents */ + drm_intel_bo_emit_reloc(brw-><a href="http://batch.bo" target="_blank">batch.bo</a>, *out_offset + 4, + mt->region->bo, bo_offset, + read_domain, 0); + + gen7_check_surface_setup(surf, is_render_target); +} + +static void gen7_update_buffer_texture_surface(struct gl_context *ctx, unsigned unit, uint32_t *surf_offset) @@ -305,43 +369,14 @@ gen7_update_texture_surface(struct gl_context *ctx, return; } - uint32_t *surf = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE, - 8 * 4, 32, surf_offset); - memset(surf, 0, 8 * 4); - - uint32_t tex_format = translate_tex_format(brw, + bool is_array = mt->logical_depth0 > 1 && tObj->Target != GL_TEXTURE_3D; + unsigned mip_count = intelObj->_MaxLevel - intel_image->mt->first_level; + uint32_t brw_format = translate_tex_format(brw, mt->format, tObj->DepthMode, sampler->sRGBDecode); - surf[0] = translate_tex_target(tObj->Target) << BRW_SURFACE_TYPE_SHIFT | - tex_format << BRW_SURFACE_FORMAT_SHIFT | - gen7_surface_tiling_mode(mt->region->tiling) | - BRW_SURFACE_CUBEFACE_ENABLES; - - if (mt->align_h == 4) - surf[0] |= GEN7_SURFACE_VALIGN_4; - if (mt->align_w == 8) - surf[0] |= GEN7_SURFACE_HALIGN_8; - - if (mt->logical_depth0 > 1 && tObj->Target != GL_TEXTURE_3D) - surf[0] |= GEN7_SURFACE_IS_ARRAY; - - if (mt->array_spacing_lod0) - surf[0] |= GEN7_SURFACE_ARYSPC_LOD0; - - surf[1] = mt->region->bo->offset + mt->offset; /* reloc */ - - surf[2] = SET_FIELD(mt->logical_width0 - 1, GEN7_SURFACE_WIDTH) | - SET_FIELD(mt->logical_height0 - 1, GEN7_SURFACE_HEIGHT); - surf[3] = SET_FIELD(mt->logical_depth0 - 1, BRW_SURFACE_DEPTH) | - ((intelObj->mt->region->pitch) - 1); - - surf[4] = gen7_surface_msaa_bits(mt->num_samples, mt->msaa_layout); - - surf[5] = (SET_FIELD(GEN7_MOCS_L3, GEN7_SURFACE_MOCS) | - /* mip count */ - (intelObj->_MaxLevel - intel_image->mt->first_level)); + int swizzle = SWIZZLE_XYZW; if (brw->is_haswell) { /* Handling GL_ALPHA as a surface format override breaks 1.30+ style @@ -352,24 +387,24 @@ gen7_update_texture_surface(struct gl_context *ctx, (firstImage->_BaseFormat == GL_DEPTH_COMPONENT || firstImage->_BaseFormat == GL_DEPTH_STENCIL); - const int swizzle = unlikely(alpha_depth) - ? SWIZZLE_XYZW : brw_get_texture_swizzle(ctx, tObj); - - surf[7] = - SET_FIELD(swizzle_to_scs(GET_SWZ(swizzle, 0)), GEN7_SURFACE_SCS_R) | - SET_FIELD(swizzle_to_scs(GET_SWZ(swizzle, 1)), GEN7_SURFACE_SCS_G) | - SET_FIELD(swizzle_to_scs(GET_SWZ(swizzle, 2)), GEN7_SURFACE_SCS_B) | - SET_FIELD(swizzle_to_scs(GET_SWZ(swizzle, 3)), GEN7_SURFACE_SCS_A); + if (likely(!alpha_depth)) + swizzle = brw_get_texture_swizzle(ctx, tObj); } - /* Emit relocation to surface contents */ - drm_intel_bo_emit_reloc(brw-><a href="http://batch.bo" target="_blank">batch.bo</a>, - *surf_offset + 4, - intelObj->mt->region->bo, - surf[1] - intelObj->mt->region->bo->offset, - I915_GEM_DOMAIN_SAMPLER, 0); - - gen7_check_surface_setup(surf, false /* is_render_target */); + gen7_emit_image_surface_state(brw, + surf_offset, + mt, + mt->offset, + translate_tex_target(tObj->Target), + brw_format, + is_array, + mt->logical_depth0, + 0, /* min_array_element */ + 0, /* render target view extent */ + GEN7_MOCS_L3, + mip_count, + swizzle, + false /* is_render_target */); } /** @@ -467,31 +502,24 @@ gen7_update_renderbuffer_surface(struct brw_context *brw, { struct gl_context *ctx = &brw->ctx; struct intel_renderbuffer *irb = intel_renderbuffer(rb); - struct intel_region *region = irb->mt->region; - uint32_t format; /* _NEW_BUFFERS */ gl_format rb_format = _mesa_get_render_format(ctx, intel_rb_format(irb)); uint32_t surftype; bool is_array = false; int depth = MAX2(rb->Depth, 1); int min_array_element; - const uint8_t mocs = GEN7_MOCS_L3; GLenum gl_target = rb->TexImage ? rb->TexImage->TexObject->Target : GL_TEXTURE_2D; uint32_t surf_index = SURF_INDEX_DRAW(unit); - uint32_t *surf = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE, 8 * 4, 32, - &brw->wm.base.surf_offset[surf_index]); - memset(surf, 0, 8 * 4); - intel_miptree_used_for_rendering(irb->mt); /* Render targets can't use IMS layout */ assert(irb->mt->msaa_layout != INTEL_MSAA_LAYOUT_IMS); assert(brw_render_target_supported(brw, rb)); - format = brw->render_target_format[rb_format]; + uint32_t brw_format = brw->render_target_format[rb_format]; if (unlikely(!brw->format_supported_as_render_target[rb_format])) { _mesa_problem(ctx, "%s: renderbuffer format %s unsupported\n", __FUNCTION__, _mesa_get_format_name(rb_format)); @@ -518,60 +546,20 @@ gen7_update_renderbuffer_surface(struct brw_context *brw, min_array_element = irb->mt_layer; } - surf[0] = surftype << BRW_SURFACE_TYPE_SHIFT | - format << BRW_SURFACE_FORMAT_SHIFT | - (irb->mt->array_spacing_lod0 ? GEN7_SURFACE_ARYSPC_LOD0 - : GEN7_SURFACE_ARYSPC_FULL) | - gen7_surface_tiling_mode(region->tiling); - - if (irb->mt->align_h == 4) - surf[0] |= GEN7_SURFACE_VALIGN_4; - if (irb->mt->align_w == 8) - surf[0] |= GEN7_SURFACE_HALIGN_8; - - if (is_array) { - surf[0] |= GEN7_SURFACE_IS_ARRAY; - } - - surf[1] = region->bo->offset; - - assert(brw->has_surface_tile_offset); - - surf[5] = SET_FIELD(mocs, GEN7_SURFACE_MOCS) | - (irb->mt_level - irb->mt->first_level); - - surf[2] = SET_FIELD(irb->mt->logical_width0 - 1, GEN7_SURFACE_WIDTH) | - SET_FIELD(irb->mt->logical_height0 - 1, GEN7_SURFACE_HEIGHT); - - surf[3] = ((depth - 1) << BRW_SURFACE_DEPTH_SHIFT) | - (region->pitch - 1); - - surf[4] = gen7_surface_msaa_bits(irb->mt->num_samples, irb->mt->msaa_layout) | - min_array_element << GEN7_SURFACE_MIN_ARRAY_ELEMENT_SHIFT | - (depth - 1) << GEN7_SURFACE_RENDER_TARGET_VIEW_EXTENT_SHIFT; - - if (irb->mt->mcs_mt) { - gen7_set_surface_mcs_info(brw, surf, brw->wm.base.surf_offset[surf_index], - irb->mt->mcs_mt, true /* is RT */); - } - - surf[7] = irb->mt->fast_clear_color_value; - - 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)); - } - - drm_intel_bo_emit_reloc(brw-><a href="http://batch.bo" target="_blank">batch.bo</a>, - brw->wm.base.surf_offset[surf_index] + 4, - region->bo, - surf[1] - region->bo->offset, - I915_GEM_DOMAIN_RENDER, - I915_GEM_DOMAIN_RENDER); </blockquote><div> </div><div>The new gen7_emit_image_surface_state() function always passes 0 for the last argument of drm_intel_bo_emit_reloc(), but the code you're removing passed I915_GEM_DOMAIN_RENDER. Is this 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"> - - gen7_check_surface_setup(surf, true /* is_render_target */); + gen7_emit_image_surface_state(brw, + &brw->wm.base.surf_offset[surf_index], + irb->mt, + 0, </blockquote><div> </div><div>I see that you're passing 0 for bo_offset in this case because the old code for gen7_update_renderbuffer_surface() didn't add any offset to the surface address. But I believe that was a latent bug that luckily no one stumbled upon (since the only way to get a miptree with a nonzero offset is via lookupEGLImage*, and people generally use that mechanism only for images that they texture from). (*technically there are a few other paths that create temporary miptrees with nonzero offsets, but those miptrees are only used for blitting.) </div><div>I'd rather see the bo_offset argument of gen7_emit_image_surface_state() dropped, and have the function just refer to mt->offset internally. </div><div>I really like this patch. I think the latent bug with mt->offset is a strong argument why doing this sort of refactoring is a good idea--if we had never duplicated this code in the first place, the latent bug probably would never have occurred. With the minor issues I've brought up here addressed, the patch is: </div><div>Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> </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"> + surftype, + brw_format, + is_array, + depth, + min_array_element, + depth - 1, /* render target view extent */ + GEN7_MOCS_L3, + irb->mt_level - irb->mt->first_level, + SWIZZLE_XYZW, + true); } 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>