[Mesa-dev] [PATCH 1/2] [v2] i965: Extract region use from hiz depth buffer
Chad Versace
chad.versace at linux.intel.com
Thu Oct 3 15:41:20 PDT 2013
On 10/01/2013 04:36 PM, Ben Widawsky wrote:
> 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).
>>
>>> 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.
Ah, I see that you modify intel_miptree_release() in patch 2 to prevent
the leak. So, my complaint is moot.
More information about the mesa-dev
mailing list