[Mesa-dev] [PATCH 1/2] [v2] i965: Extract region use from hiz depth buffer
Ben Widawsky
ben at bwidawsk.net
Tue Oct 1 16:48:53 PDT 2013
On Tue, Oct 01, 2013 at 01:06:02PM -0700, Chad Versace wrote:
> 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).
Are you referring to memset? The only initializer is
intel_region_alloc_internal() which I do not have access to (and indeed
it seems like the wrong thing to make it extern).
>
> > 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.
>
--
Ben Widawsky, Intel Open Source Technology Center
More information about the mesa-dev
mailing list