[Mesa-dev] [PATCH V2 08/11] anv/cmd_buffer: Add code for performing HZ operations

[Chad Versace]

On Tue 27 Sep 2016, Nanley Chery wrote:
> On Tue, Sep 27, 2016 at 11:00:14AM -0700, Chad Versace wrote:
> > On Mon 26 Sep 2016, Nanley Chery wrote:
> > > Create a function that performs one of three HiZ operations -
> > > depth/stencil clears, HiZ resolve, and depth resolves.
> > > 
> > > Signed-off-by: Nanley Chery <nanley.g.chery at intel.com>
> > > 
> > > ---
> > > 
> > > v2. Add documentation
> > >     Fix the alignment check
> > >     Don't minify clear rectangle (Jason)
> > >     Use blorp enums (Jason)
> > >     Enable depth stalls and flushes
> > >     Use full RT rectangle for resolve ops
> > >     Add stencil clear todo
> > > 
> > >  src/intel/vulkan/anv_genX.h        |   3 +
> > >  src/intel/vulkan/gen7_cmd_buffer.c |   6 ++
> > >  src/intel/vulkan/gen8_cmd_buffer.c | 167 +++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 176 insertions(+)
> > 
> > 
> > 
> > > +/**
> > > + * Emit the HZ_OP packet in the sequence specified by the BDW PRM section
> > > + * entitled: "Optimized Depth Buffer Clear and/or Stencil Buffer Clear."
> > > + *
> > > + * \todo Enable Stencil Buffer-only clears
> > > + */
> > > +void
> > > +genX(cmd_buffer_do_hz_op)(struct anv_cmd_buffer *cmd_buffer,
> > > +                          enum blorp_hiz_op op)
> > > +{
> > 
> > All other "emission" functions in gen8_cmd_buffer.c are named
> > gen8_cmd_buffer_emit_foo(). I think this funtion should be named
> > gen8_cmd_buffer_emit_hz_op for consistency.
> > 
> 
> Sounds good. I'll fix that in the v3.

Ok.
> 
> > > +   struct anv_cmd_state *cmd_state = &cmd_buffer->state;
> > > +   const struct anv_image_view *iview =
> > > +      anv_cmd_buffer_get_depth_stencil_view(cmd_buffer);
> > > +
> > > +   if (iview == NULL || !anv_image_has_hiz(iview->image))
> > > +      return;
> > 
> > Shouldn't this check for subpass_count > 1, like the previous patches
> > do?
> > 
> 
> The following patch in the series adds this check. [...]

Ok.

> > > +      if (op != BLORP_HIZ_OP_DEPTH_CLEAR) {
> > > +         /* The Optimized HiZ resolve rectangle must be the size of the full RT
> > > +          * and aligned to 8x4. The non-optimized Depth resolve rectangle must
> > > +          * be the size of the full RT. The same alignment is assumed to be
> > > +          * required.
> > > +          *
> > > +          * TODO:
> > > +          * Consider changing halign of non-D16 depth formats to 8 as mip 2 may
> > > +          * get clobbered.
> > 
> > Jason and I did some experiments on BDW and SKL. The SKL hardware aligns
> > the hiz surface correctly for all miplevels, so the clobbered-miplevel-2
> > issue is a non-issue. If I recall correctly, BDW hardware also
> > eliminates the clobbered-miplevel-2 issue; but I'm not 100% sure, so ask
> > Jason. Pre-gen8 definitely suffers from the clobbered-miplevel-2 issue.
> > It would be very very good to list in the comment which hardware does
> > and does not suffer from the issue, as that's not documented anywhere.
> > 
> 
> I'd also like to have these findings documented as well. However, collecting
> that data would require creating new tests and testing on platforms that are
> outside the scope of this series (single-miplevel, gen8+ HiZ). I wouldn't
> mind doing this in a series where multiple mip-level support is added though.
> Would it be better to keep the TODO task locally and omit the comment?

I see no harm in keeping the TODO in the code. I also see no harm in
removing. It's up to you.

> 
> > > +          */
> > 
> > For readability, please explicity do
> > 
> >             hzp.ClearRectangleXMin = 0;
> >             hzp.ClearRectangleYMin = 0;
> > 
> 
> This will be present in the v3.

Ok.