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

[Nanley Chery]

On Mon, Oct 10, 2016 at 06:00:49PM -0700, Jason Ekstrand wrote:
> 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.
> 

What is the definition of correct here? I'd assume you mean that the
data matches what's in the depth buffer, but that sometimes may not be
the case (STORE_OP_DONTCARE) yet the program behavior is correct
nonetheless.

Also, could you please explain where the danger comes into play?

> 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
                                         ^
					 I'm not sure which model you're
					 referring to here since you
					 said this was our initial
					 enabling model.

-Nanley

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