[Mesa-dev] [PATCH 16/22] anv/hiz: Perform HiZ resolves for all partial renders
Jason Ekstrand
jason at jlekstrand.net
Wed Oct 12 01:55:53 UTC 2016
On Tue, Oct 11, 2016 at 6:16 PM, Nanley Chery <nanleychery at gmail.com> wrote:
> 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.
>
By "correct" I mean "consistent with the depth buffer" or, more precicely,
"all well-defined pixels of the depth buffer are consistent with the HiZ
buffer". We *may* be able to avoid the depth resolve at the end if you
have STORE_OP_DONT_CARE. However, we would probably not do anything
interesting with LOAD_OP_DONT_CARE.
> Also, could you please explain where the danger comes into play?
>
We need to have a solid mental model of when HiZ and depth are consistent.
Otherwise, we'll make mistakes, things will get inconsistent, and we'll
have weird bugs. This bug is a good example of this. Our mental model (2)
works fine except that we were leaking garbage depth from DONT_CARE when we
have a partial areat. Just doing a HiZ resolve after a blorp clear "fixes"
the bug by making things always consistent (mental model 1). But then it
means that we have LOAD_OP_LOAD, we're doing two HiZ resolves which we
don't want either.
As I intended to say below, I don't mind moving to (1), but if that's what
we want to do we should commit to it and change rather than going with
half-and-half.
> > 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.
>
I meant "moving to (1)". Sorry for the typo.
>
> -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
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20161011/5280c720/attachment-0001.html>
More information about the mesa-dev
mailing list