[Mesa-dev] [PATCH 1/2] [v2] i965: Extract region use from hiz depth buffer
Chad Versace
chad.versace at linux.intel.com
Tue Oct 1 13:06:02 PDT 2013
On 09/30/2013 12:35 PM, Ben Widawsky wrote:
> Starting with Ivybridge, the hierarchical had relaxed requirements for
> its allocation. Following a "simple" formula in the bspec was all you
> needed to satisfy the requirement.
>
> To prepare the code for this, extract all places where the miptree was
> used, when we really only needed the region. This allows an upcoming
> patch to simply allocate the region, and not the whole miptree.
>
> v2: Don't use intel_region. Instead use bo + stride. We actually do
> store the stride in libdrm, but it is inaccessible in the current
> libdrm version.
>
> CC: Chad Versace <chad.versace at linux.intel.com>
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> ---
> src/mesa/drivers/dri/i965/brw_misc_state.c | 11 +++++---
> src/mesa/drivers/dri/i965/gen6_blorp.cpp | 20 +++++++++------
> src/mesa/drivers/dri/i965/gen7_blorp.cpp | 6 ++---
> src/mesa/drivers/dri/i965/gen7_misc_state.c | 5 ++--
> src/mesa/drivers/dri/i965/intel_fbo.c | 4 +--
> src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 36 +++++++++++++++------------
> src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 6 ++++-
> 7 files changed, 52 insertions(+), 36 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_misc_state.c b/src/mesa/drivers/dri/i965/brw_misc_state.c
> index 7f4cd6f..23ffeab 100644
> --- a/src/mesa/drivers/dri/i965/brw_misc_state.c
> +++ b/src/mesa/drivers/dri/i965/brw_misc_state.c
> @@ -210,8 +210,12 @@ brw_get_depthstencil_tile_masks(struct intel_mipmap_tree *depth_mt,
> &tile_mask_x, &tile_mask_y, false);
>
> if (intel_miptree_slice_has_hiz(depth_mt, depth_level, depth_layer)) {
> + uint32_t tmp;
> uint32_t hiz_tile_mask_x, hiz_tile_mask_y;
> - intel_region_get_tile_masks(depth_mt->hiz_mt->region,
> + struct intel_region region = { .cpp = depth_mt->cpp };
> +
> + drm_intel_bo_get_tiling(depth_mt->hiz_buffer.bo, ®ion.tiling, &tmp);
> + intel_region_get_tile_masks(®ion,
> &hiz_tile_mask_x, &hiz_tile_mask_y, false);
>
> /* Each HiZ row represents 2 rows of pixels */
> @@ -667,11 +671,10 @@ brw_emit_depth_stencil_hiz(struct brw_context *brw,
>
> /* Emit hiz buffer. */
> if (hiz) {
> - struct intel_mipmap_tree *hiz_mt = depth_mt->hiz_mt;
> BEGIN_BATCH(3);
> OUT_BATCH((_3DSTATE_HIER_DEPTH_BUFFER << 16) | (3 - 2));
> - OUT_BATCH(hiz_mt->region->pitch - 1);
> - OUT_RELOC(hiz_mt->region->bo,
> + OUT_BATCH(depth_mt->hiz_buffer.stride - 1);
> + OUT_RELOC(depth_mt->hiz_buffer.bo,
> I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER,
> brw->depthstencil.hiz_offset);
> ADVANCE_BATCH();
> diff --git a/src/mesa/drivers/dri/i965/gen6_blorp.cpp b/src/mesa/drivers/dri/i965/gen6_blorp.cpp
> index da523e5..fc3a331 100644
> --- a/src/mesa/drivers/dri/i965/gen6_blorp.cpp
> +++ b/src/mesa/drivers/dri/i965/gen6_blorp.cpp
> @@ -887,16 +887,22 @@ gen6_blorp_emit_depth_stencil_config(struct brw_context *brw,
>
> /* 3DSTATE_HIER_DEPTH_BUFFER */
> {
> - struct intel_region *hiz_region = params->depth.mt->hiz_mt->region;
> - uint32_t hiz_offset =
> - intel_region_get_aligned_offset(hiz_region,
> - draw_x & ~tile_mask_x,
> - (draw_y & ~tile_mask_y) / 2, false);
> + uint32_t hiz_offset, tmp;
> + struct intel_mipmap_tree *depth_mt = params->depth.mt;
> + struct intel_region hiz_region;
> +
> + hiz_region.cpp = depth_mt->cpp;
> + hiz_region.pitch = depth_mt->hiz_buffer.stride;
> + drm_intel_bo_get_tiling(depth_mt->hiz_buffer.bo, &hiz_region.tiling, &tmp);
This initialization of hiz_region subtly differs from the initialization in the previous
hunk that uses the designated initializer syntax. When using designated initializers,
all uninitialized fields are initialized to 0. Here, the uninitialized fields have
undefined values. Please use designated initializers here to prevent undefined behavior.
> +
> + hiz_offset = intel_region_get_aligned_offset(&hiz_region,
> + draw_x & ~tile_mask_x,
> + (draw_y & ~tile_mask_y) / 2, false);
> BEGIN_BATCH(3);
> OUT_BATCH((_3DSTATE_HIER_DEPTH_BUFFER << 16) | (3 - 2));
> - OUT_BATCH(hiz_region->pitch - 1);
> - OUT_RELOC(hiz_region->bo,
> + OUT_BATCH(hiz_region.pitch - 1);
> + OUT_RELOC(depth_mt->hiz_buffer.bo,
The 'hiz_region' is a temporary thing that will eventually die off as we clean up
the driver. So, replace OUT_BATCH(hiz_region.pitch - 1) with
OUT_BATCH(depth_mt->hiz_buffer.stride - 1). (As a nice little side-effect, the sequence of
OUT_BATCH's look more symmetric that way).
> I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER,
> hiz_offset);
> ADVANCE_BATCH();
> diff --git a/src/mesa/drivers/dri/i965/gen7_blorp.cpp b/src/mesa/drivers/dri/i965/gen7_blorp.cpp
> index 9df3d92..379e8ee 100644
> --- a/src/mesa/drivers/dri/i965/gen7_blorp.cpp
> +++ b/src/mesa/drivers/dri/i965/gen7_blorp.cpp
> @@ -737,13 +737,13 @@ gen7_blorp_emit_depth_stencil_config(struct brw_context *brw,
>
> /* 3DSTATE_HIER_DEPTH_BUFFER */
> {
> - struct intel_region *hiz_region = params->depth.mt->hiz_mt->region;
> + struct intel_mipmap_tree *depth_mt = params->depth.mt;
>
> BEGIN_BATCH(3);
> OUT_BATCH((GEN7_3DSTATE_HIER_DEPTH_BUFFER << 16) | (3 - 2));
> OUT_BATCH((mocs << 25) |
> - (hiz_region->pitch - 1));
> - OUT_RELOC(hiz_region->bo,
> + (depth_mt->hiz_buffer.stride - 1));
> + OUT_RELOC(depth_mt->hiz_buffer.bo,
> I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER,
> 0);
> ADVANCE_BATCH();
In this hunk, let's follow the dominant pattern in the rest of the function.
Kill the temporary 'depth_mt' and replace it with 'params->depth.mt' in each
OUT_BATCH/RELOC.
> diff --git a/src/mesa/drivers/dri/i965/gen7_misc_state.c b/src/mesa/drivers/dri/i965/gen7_misc_state.c
> index eb942cf..cb0594d 100644
> --- a/src/mesa/drivers/dri/i965/gen7_misc_state.c
> +++ b/src/mesa/drivers/dri/i965/gen7_misc_state.c
> @@ -143,12 +143,11 @@ gen7_emit_depth_stencil_hiz(struct brw_context *brw,
> OUT_BATCH(0);
> ADVANCE_BATCH();
> } else {
> - struct intel_mipmap_tree *hiz_mt = depth_mt->hiz_mt;
> BEGIN_BATCH(3);
> OUT_BATCH(GEN7_3DSTATE_HIER_DEPTH_BUFFER << 16 | (3 - 2));
> OUT_BATCH((mocs << 25) |
> - (hiz_mt->region->pitch - 1));
> - OUT_RELOC(hiz_mt->region->bo,
> + (depth_mt->hiz_buffer.stride - 1));
> + OUT_RELOC(depth_mt->hiz_buffer.bo,
> I915_GEM_DOMAIN_RENDER,
> I915_GEM_DOMAIN_RENDER,
> 0);
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index 2f5e04f..e1da9de 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -793,7 +793,8 @@ intel_miptree_release(struct intel_mipmap_tree **mt)
>
> intel_region_release(&((*mt)->region));
> intel_miptree_release(&(*mt)->stencil_mt);
> - intel_miptree_release(&(*mt)->hiz_mt);
> + intel_miptree_release(&(*mt)->hiz_buffer.mt);
> + (*mt)->hiz_buffer.bo = NULL;
> intel_miptree_release(&(*mt)->mcs_mt);
> intel_miptree_release(&(*mt)->singlesample_mt);
> intel_resolve_map_clear(&(*mt)->hiz_map);
After patch 2, a memory leak occurs here becuse the bo refcount
never drops to 0. Replace `bo = NULL` with `drm_intel_bo_unreference(&bo)`.
> @@ -1276,22 +1277,25 @@ bool
> intel_miptree_alloc_hiz(struct brw_context *brw,
> struct intel_mipmap_tree *mt)
> {
> - assert(mt->hiz_mt == NULL);
> - mt->hiz_mt = intel_miptree_create(brw,
> - mt->target,
> - mt->format,
> - mt->first_level,
> - mt->last_level,
> - mt->logical_width0,
> - mt->logical_height0,
> - mt->logical_depth0,
> - true,
> - mt->num_samples,
> - INTEL_MIPTREE_TILING_ANY);
> -
> - if (!mt->hiz_mt)
> + assert(mt->hiz_buffer.mt == NULL);
> + mt->hiz_buffer.mt = intel_miptree_create(brw,
> + mt->target,
> + mt->format,
> + mt->first_level,
> + mt->last_level,
> + mt->logical_width0,
> + mt->logical_height0,
> + mt->logical_depth0,
> + true,
> + mt->num_samples,
> + INTEL_MIPTREE_TILING_ANY);
> +
> + if (!mt->hiz_buffer.mt)
> return false;
>
For the memory-leak fix in the previous hunk, here we need to bump
the bo refcount with drm_intel_bo_reference().
And like Ian said, use spaces not tabs.
Other than the above fixes, the patch looks good to me. I'm waiting for v3.
More information about the mesa-dev
mailing list