On Tue, Oct 11, 2016 at 6:16 PM, Nanley Chery <nanleych...@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 <nanleych...@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 <ja...@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@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > > >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
