On 13 September 2013 23:10, Kenneth Graunke <kenn...@whitecape.org> wrote:
> 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 <kenn...@whitecape.org>
> ---
> 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->batch.bo, *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->batch.bo,
> - *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->batch.bo,
> - brw->wm.base.surf_offset[surf_index] + 4,
> - region->bo,
> - surf[1] - region->bo->offset,
> - I915_GEM_DOMAIN_RENDER,
> - I915_GEM_DOMAIN_RENDER);
>
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?
> -
> - 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,
>
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.)
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.
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:
Reviewed-by: Paul Berry <stereotype...@gmail.com>
> + 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
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
