[Mesa-dev] [PATCH 1/2] [v2] i965: Extract region use from hiz depth buffer

Ben Widawsky ben at bwidawsk.net
Tue Oct 1 16:36:51 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, &region.tiling, &tmp);
> >+         intel_region_get_tile_masks(&region,
> >                                       &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


More information about the mesa-dev mailing list