<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Oct 11, 2016 at 6:16 PM, Nanley Chery <span dir="ltr"><<a href="mailto:nanleychery@gmail.com" target="_blank">nanleychery@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Mon, Oct 10, 2016 at 06:00:49PM -0700, Jason Ekstrand wrote:<br>
> On Mon, Oct 10, 2016 at 2:23 PM, Nanley Chery <<a href="mailto:nanleychery@gmail.com">nanleychery@gmail.com</a>> wrote:<br>
><br>
> > On Fri, Oct 07, 2016 at 09:41:14PM -0700, Jason Ekstrand wrote:<br>
> > > If we don't, we can end up with corruption in the portion of the depth<br>
> > > buffer that lies outside the render area when we do a HiZ resolve at the<br>
> > > end. The only reason we weren't seeing this before was that all of the<br>
> > > meta-based clears such as VkCmdClearDepthStencilImage were internally<br>
> > using<br>
> > > HiZ so the HiZ buffer never truly got out-of-sync. If the CTS ever<br>
> > tested<br>
> > > a depth upload (which doesn't care about HiZ) and then a partial render<br>
> > we<br>
> > > would have seen problems. Soon, we will be using blorp to do depth<br>
> > clears<br>
> > > and it won't bother with HiZ so we would get CTS regressions without<br>
> > this.<br>
> > ><br>
> ><br>
> > I understand the problem, but I think this solution unnecessarily<br>
> > penalizes the user's renderpass.<br>
> ><br>
> > Since depth buffer updates via vkCopy*ToImage and<br>
> > vkCmdClearDepthStencilImage cause the HiZ buffer to become stale,<br>
> > calling<br>
> ><br>
> > genX(cmd_buffer_emit_hz_op)(<wbr>cmd_buffer, BLORP_HIZ_OP_HIZ_RESOLVE);<br>
> ><br>
> > at the bottom of those commands should fix the issue without the extra<br>
> > penalty. I'd imagine that as a prequisite, blorp would have to learn to<br>
> > emit enough depth stencil state for this command.<br>
> ><br>
><br>
> I think that's dangerously mixing HiZ data validity models. There are 3<br>
> basic aux data validity models that we've thrown around:<br>
><br>
> 1) AUX is always correct.<br>
> 2) AUX is correct within a render pass and invalid outside.<br>
> 3) Track whether or not AUX is valid and resolve only as needed.<br>
><br>
<br>
</div></div>What is the definition of correct here? I'd assume you mean that the<br>
data matches what's in the depth buffer, but that sometimes may not be<br>
the case (STORE_OP_DONTCARE) yet the program behavior is correct<br>
nonetheless.<br></blockquote><div><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Also, could you please explain where the danger comes into play?<span class=""><br></span></blockquote><div><br>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.<br><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> So far, our initial enabling strategy has been (2). We can move to (1) or<br>
> maybe even (3) with layout transitions, but that's not what we've done so<br>
> far. Your suggestion is to mix in a little of (1) because there is a bug<br>
> in our implementation of (2).<br>
><br>
> I'm Ok, for gen8+ HiZ, with moving to (2) eventually since gen8 is capable<br>
</span> ^<br>
I'm not sure which model you're<br>
referring to here since you<br>
said this was our initial<br>
enabling model.<br></blockquote><div><br></div><div>I meant "moving to (1)". Sorry for the typo.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
-Nanley<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
> of sampling with HiZ. (Maybe we can even move to (3).) However, that's<br>
> not where we are right now and I don't really want to start mixing mental<br>
> models.<br>
><br>
> --Jason<br>
><br>
><br>
> > -Nanley<br>
> ><br>
> > > Signed-off-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>><br>
> > > ---<br>
> > > src/intel/vulkan/gen8_cmd_<wbr>buffer.c | 12 +++++++++++-<br>
> > > 1 file changed, 11 insertions(+), 1 deletion(-)<br>
> > ><br>
> > > diff --git a/src/intel/vulkan/gen8_cmd_<wbr>buffer.c<br>
> > b/src/intel/vulkan/gen8_cmd_<wbr>buffer.c<br>
> > > index e6a3c3d..44ffcbf 100644<br>
> > > --- a/src/intel/vulkan/gen8_cmd_<wbr>buffer.c<br>
> > > +++ b/src/intel/vulkan/gen8_cmd_<wbr>buffer.c<br>
> > > @@ -513,7 +513,17 @@ genX(cmd_buffer_emit_hz_op)(<wbr>struct anv_cmd_buffer<br>
> > *cmd_buffer,<br>
> > > return;<br>
> > > break;<br>
> > > case BLORP_HIZ_OP_HIZ_RESOLVE:<br>
> > > - if (cmd_buffer->state.pass-><wbr>attachments[ds].load_op !=<br>
> > > + /* If the render area covers the entire surface *and* load_op is<br>
> > either<br>
> > > + * CLEAR or DONT_CARE then the previous contents of the depth<br>
> > buffer<br>
> > > + * will be entirely discarded. In this case, we can skip the HiZ<br>
> > > + * resolve.<br>
> > > + *<br>
> > > + * If the render area is not the full surface, we need to do<br>
> > > + * the resolve because otherwise data outside the render area may<br>
> > get<br>
> > > + * garbled by the resolve at the end of the render pass.<br>
> > > + */<br>
> > > + if (full_surface_op &&<br>
> > > + cmd_buffer->state.pass-><wbr>attachments[ds].load_op !=<br>
> > > VK_ATTACHMENT_LOAD_OP_LOAD)<br>
> > > return;<br>
> > > break;<br>
> > > --<br>
> > > 2.5.0.400.gff86faf<br>
> > ><br>
> > > ______________________________<wbr>_________________<br>
> > > mesa-dev mailing list<br>
> > > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
> > > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/mesa-dev</a><br>
> ><br>
</div></div></blockquote></div><br></div></div>