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 <[email protected]> > >Signed-off-by: Ben Widawsky <[email protected]> > >--- > > 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().
I'll get to all your other requests shortly, can you please verify the leak is there. I believe it is not. I can't drm_bo_unreference at this point because intel_region still owns the bio. > > 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 _______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
