[Mesa-dev] [PATCH 16/22] anv/hiz: Perform HiZ resolves for all partial renders

[Jason Ekstrand]

On Mon, Oct 10, 2016 at 2:23 PM, Nanley Chery <nanleychery at gmail.com> wrote:

> On Fri, Oct 07, 2016 at 09:41:14PM -0700, Jason Ekstrand wrote:
> > If we don't, we can end up with corruption in the portion of the depth
> > buffer that lies outside the render area when we do a HiZ resolve at the
> > end.  The only reason we weren't seeing this before was that all of the
> > meta-based clears such as VkCmdClearDepthStencilImage were internally
> using
> > HiZ so the HiZ buffer never truly got out-of-sync.  If the CTS ever
> tested
> > a depth upload (which doesn't care about HiZ) and then a partial render
> we
> > would have seen problems.  Soon, we will be using blorp to do depth
> clears
> > and it won't bother with HiZ so we would get CTS regressions without
> this.
> >
>
> I understand the problem, but I think this solution unnecessarily
> penalizes the user's renderpass.
>
> Since depth buffer updates via vkCopy*ToImage and
> vkCmdClearDepthStencilImage cause the HiZ buffer to become stale,
> calling
>
> genX(cmd_buffer_emit_hz_op)(cmd_buffer, BLORP_HIZ_OP_HIZ_RESOLVE);
>
> at the bottom of those commands should fix the issue without the extra
> penalty. I'd imagine that as a prequisite, blorp would have to learn to
> emit enough depth stencil state for this command.
>

I think that's dangerously mixing HiZ data validity models.  There are 3
basic aux data validity models that we've thrown around:

 1) AUX is always correct.
 2) AUX is correct within a render pass and invalid outside.
 3) Track whether or not AUX is valid and resolve only as needed.

So far, our initial enabling strategy has been (2).  We can move to (1) or
maybe even (3) with layout transitions, but that's not what we've done so
far.  Your suggestion is to mix in a little of (1) because there is a bug
in our implementation of (2).

I'm Ok, for gen8+ HiZ, with moving to (2) eventually since gen8 is capable
of sampling with HiZ.  (Maybe we can even move to (3).)  However, that's
not where we are right now and I don't really want to start mixing mental
models.

--Jason


> -Nanley
>
> > Signed-off-by: Jason Ekstrand <jason at jlekstrand.net>
> > ---
> >  src/intel/vulkan/gen8_cmd_buffer.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/intel/vulkan/gen8_cmd_buffer.c
> b/src/intel/vulkan/gen8_cmd_buffer.c
> > index e6a3c3d..44ffcbf 100644
> > --- a/src/intel/vulkan/gen8_cmd_buffer.c
> > +++ b/src/intel/vulkan/gen8_cmd_buffer.c
> > @@ -513,7 +513,17 @@ genX(cmd_buffer_emit_hz_op)(struct anv_cmd_buffer
> *cmd_buffer,
> >           return;
> >        break;
> >     case BLORP_HIZ_OP_HIZ_RESOLVE:
> > -      if (cmd_buffer->state.pass->attachments[ds].load_op !=
> > +      /* If the render area covers the entire surface *and* load_op is
> either
> > +       * CLEAR or DONT_CARE then the previous contents of the depth
> buffer
> > +       * will be entirely discarded.  In this case, we can skip the HiZ
> > +       * resolve.
> > +       *
> > +       * If the render area is not the full surface, we need to do
> > +       * the resolve because otherwise data outside the render area may
> get
> > +       * garbled by the resolve at the end of the render pass.
> > +       */
> > +      if (full_surface_op &&
> > +          cmd_buffer->state.pass->attachments[ds].load_op !=
> >            VK_ATTACHMENT_LOAD_OP_LOAD)
> >           return;
> >        break;
> > --
> > 2.5.0.400.gff86faf
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20161010/32d67eb0/attachment.html>