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

[Chad Versace]

On 10/01/2013 04:48 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:

>>> 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).

As you pointed out yesterday, this is C++ code, so a designated struct initializer
can't be used. Oops.

We still need to avoid passing around uninitialized data, though. I think memset is
a good choice here.