[Mesa-dev] [PATCH 6/8] i965: Implement HiZ resolves on Broadwell.

[Kenneth Graunke]

On 02/19/2014 11:12 AM, Eric Anholt wrote:
> Kenneth Graunke <kenneth at whitecape.org> writes:
> 
>> On 02/18/2014 01:38 PM, Eric Anholt wrote:
>>> Kenneth Graunke <kenneth at whitecape.org> writes:
>> [snip]
>>>> diff --git a/src/mesa/drivers/dri/i965/gen8_depth_state.c b/src/mesa/drivers/dri/i965/gen8_depth_state.c
>>>> index f30ff28..3fa20c8 100644
>>>> --- a/src/mesa/drivers/dri/i965/gen8_depth_state.c
>>>> +++ b/src/mesa/drivers/dri/i965/gen8_depth_state.c
>>>> @@ -203,3 +203,108 @@ gen8_emit_depth_stencil_hiz(struct brw_context *brw,
>>>>                        brw->depthstencil.stencil_offset,
>>>>                        hiz, width, height, depth, lod, min_array_element);
>>>>  }
>>>> +
>>>> +/**
>>>> + * Emit packets to perform a depth/HiZ resolve or fast depth/stencil clear.
>>>> + *
>>>> + * See the "Optimized Depth Buffer Clear and/or Stencil Buffer Clear" section
>>>> + * of the hardware documentation for details.
>>>> + */
>>>> +void
>>>> +gen8_hiz_exec(struct brw_context *brw, struct intel_mipmap_tree *mt,
>>>> +              unsigned int level, unsigned int layer, enum gen6_hiz_op op)
>>>> +{
>>>> +   if (op == GEN6_HIZ_OP_NONE)
>>>> +      return;
>>>> +
>>>> +   assert(mt->first_level == 0);
>>>> +
>>>> +   struct intel_mipmap_level *miplevel = &mt->level[level];
>>>> +
>>>> +   /* The basic algorithm is:
>>>> +    * - If needed, emit 3DSTATE_{DEPTH,HIER_DEPTH,STENCIL}_BUFFER and
>>>> +    *   3DSTATE_CLEAR_PARAMS packets to set up the relevant buffers.
>>>> +    * - If needed, emit 3DSTATE_DRAWING_RECTANGLE.
>>>> +    * - Emit 3DSTATE_WM_HZ_OP with a bit set for the particular operation.
>>>> +    * - Do a special PIPE_CONTROL to trigger an implicit rectangle primitive.
>>>> +    * - Emit 3DSTATE_WM_HZ_OP with no bits set to return to normal rendering.
>>>> +    */
>>>> +   emit_depth_packets(brw, mt,
>>>> +                      brw_depth_format(brw, mt->format),
>>>> +                      BRW_SURFACE_2D,
>>>> +                      true, /* depth writes */
>>>> +                      NULL, false, 0, /* no stencil for now */
>>>> +                      true, /* hiz */
>>>> +                      mt->logical_width0,
>>>> +                      mt->logical_height0,
>>>> +                      MAX2(mt->logical_depth0, 1),
>>>
>>> Is logical_depth0 ever 0?  That seems like a bug.
>>
>> No, I guess it isn't.  It looks like I copy and pasted this from BLORP,
>> or was being overly cautious for some reason.  Dropped.
>>
>>>> +                      level,
>>>> +                      layer); /* min_array_element */
>>>> +
>>>> +   BEGIN_BATCH(4);
>>>> +   OUT_BATCH(_3DSTATE_DRAWING_RECTANGLE << 16 | (4 - 2));
>>>> +   OUT_BATCH(0);
>>>> +   OUT_BATCH(((mt->logical_width0 - 1) & 0xffff) |
>>>> +             ((mt->logical_height0 - 1) << 16));
>>>> +   OUT_BATCH(0);
>>>> +   ADVANCE_BATCH();
>>>
>>> The drawing rectangle should be using the level's size, not the level 0
>>> size.
>>
>> Yes, this makes sense...we bind a specific miplevel of the depth buffer,
>> so presumably the (0, 0) origin is the start of that miplevel, not the
>> start of the whole tree.  I'll change that.
>>
>> Since the drawing rectangle is just the bounds of where you can draw,
>> and not actually the clear/resolve rectangle, I think specifying one
>> that's too large shouldn't be harmful.  But specifying the right value
>> is trivial, so I agree we should do it.
>>
>>>> +   uint32_t sample_mask = 0xFFFF;
>>>> +   if (mt->num_samples > 0) {
>>>> +      dw1 |= SET_FIELD(ffs(mt->num_samples) - 1, GEN8_WM_HZ_NUM_SAMPLES);
>>>> +      sample_mask = gen6_determine_sample_mask(brw);
>>>> +   }
>>>
>>> I don't think we want the user-set sample mask stuff to change the
>>> samples affected by our hiz/depth resolves.  I think you can just drop
>>> the if block.
>>
>> Good point, whatever the user specified is probably unrelated to our
>> values.  I've dropped the sample_mask variable and just stuffed 0xFFFF
>> in the packet.
>>
>> I kept the if-block for the "dw1 |= ...num_samples..." line.
>>
>>>> +
>>>> +   BEGIN_BATCH(5);
>>>> +   OUT_BATCH(_3DSTATE_WM_HZ_OP << 16 | (5 - 2));
>>>> +   OUT_BATCH(dw1);
>>>> +   OUT_BATCH(0);
>>>> +   OUT_BATCH(SET_FIELD(miplevel->width, GEN8_WM_HZ_CLEAR_RECTANGLE_X_MAX) |
>>>> +             SET_FIELD(miplevel->height, GEN8_WM_HZ_CLEAR_RECTANGLE_Y_MAX));
>>>> +   OUT_BATCH(SET_FIELD(sample_mask, GEN8_WM_HZ_SAMPLE_MASK));
>>>> +   ADVANCE_BATCH();
>>>
>>> I think now the miplevel->width should be minify(mt->logical_width0,
>>> level).  Hope that helped
>>
>> Yes, that's much nicer - and correct for MSAA buffers!  I'm unclear
>> whether we need to do:
>>
>>    ALIGN(minify(mt->logical_width0,  level), 8)
>>    ALIGN(minify(mt->logical_height0, level), 4)
>>
>> (both here and in the drawing rectangle)
>>
>> I've read seemingly contradictory information...it sounds like it might
>> be necessary for depth resolves, but not otherwise...but I could be
>> misinterpreting it.  It seems to be working...
> 
> Yeah, I'm not clear on how this ought to work for resolves.  For clears,
> the strategy explained for the previous gens made sense: Round down your
> coords to get 8x4 alignment, then do slow clears on the remaining strips
> (if any).  But for a resolve, what else do you do?

We only enable HiZ for miplevels that align to 8x4 boundaries.

--Ken