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.ch...@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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
