<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Jun 2, 2017 at 1:01 PM, Chad Versace <span dir="ltr"><<a href="mailto:chadversary@chromium.org" target="_blank">chadversary@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Wed 31 May 2017, Pohjolainen, Topi wrote:<br>
> On Wed, May 31, 2017 at 10:22:09AM -0700, Jason Ekstrand wrote:<br>
> > On Tue, May 30, 2017 at 7:29 AM, Pohjolainen, Topi <<br>
> > <a href="mailto:topi.pohjolainen@gmail.com">topi.pohjolainen@gmail.com</a>> wrote:<br>
<br>
</span><span class="">> > > > On Fri, May 26, 2017 at 04:30:05PM -0700, Jason Ekstrand wrote:<br>
> > > > > Cc: "17.0 17.1" <<a href="mailto:mesa-stable@lists.freedesktop.org">mesa-stable@lists.<wbr>freedesktop.org</a>><br>
> > > > > ---<br>
> > > > >  src/mesa/drivers/dri/i965/<wbr>intel_blit.c | 2 ++<br>
> > > > >  1 file changed, 2 insertions(+)<br>
> > > > ><br>
> > > > > diff --git a/src/mesa/drivers/dri/i965/<wbr>intel_blit.c<br>
> > > b/src/mesa/drivers/dri/i965/<wbr>intel_blit.c<br>
> > > > > index 2925fc2..b1e1eaa 100644<br>
> > > > > --- a/src/mesa/drivers/dri/i965/<wbr>intel_blit.c<br>
> > > > > +++ b/src/mesa/drivers/dri/i965/<wbr>intel_blit.c<br>
> > > > > @@ -329,6 +329,7 @@ intel_miptree_blit(struct brw_context *brw,<br>
> > > > >     intel_miptree_slice_resolve_<wbr>depth(brw, dst_mt, dst_level,<br>
> > > dst_slice);<br>
> > > > >     intel_miptree_resolve_color(<wbr>brw, src_mt, src_level, src_slice, 1,<br>
> > > 0);<br>
> > > > >     intel_miptree_resolve_color(<wbr>brw, dst_mt, dst_level, dst_slice, 1,<br>
> > > 0);<br>
> > > > > +   intel_miptree_slice_set_needs_<wbr>hiz_resolve(dst_mt, dst_level,<br>
> > > dst_slice);<br>
<br>
</span>Something feels wrong. Suppose that, before the blit, the HiZ buffer<br>
contains significant data. Here, we schedule an op to invalidate the HiZ<br>
data. Also, suppose the the below call to emit_miptree_blit() rejects<br>
the blit (maybe because the pitch is too big) and returns false. The<br>
failed blit will likely emit a GL error or, in some cases, i965 will<br>
fallback to to doing the blit with the 3D engine. In either case, there<br>
is a likely bug waiting because the HiZ buffer is scheduled to have its<br>
HiZ data invalidated before its next use, but the HiZ data is still<br>
significant.<br></blockquote><div><br></div><div>I agree that you are technically mostly correct.  However, I don't think it's actually going to matter in practice for 3 reasons:<br><br></div><div> 1) The GL error case is bogus.  We aren't allowed to start modifying things and then throw a GL error unless that error is OUT_OF_MEMORY.  All of the error-checking should have happened up-front and it is our job to succeed somehow.<br><br></div><div> 2) By the time we get to a blit path, we've given up on HiZ.  We never fall back from BLIT to render.  If BLIT fails, we fall back to software at which point we're going to write without HiZ anyway so setting needs_hiz_resolve is fine.<br><br></div><div> 3) There is no bug waiting.  Since we do a depth resolve first, the HiZ and depth will be consistent and anything that anything that would use it with HiZ will see the fact that it needs a HiZ resolve and do one.  You can't get into a case where we say that it needs a hiz resolve and then edit HiZ without resolving.  The worst that happens if I'm completely wrong about (1) and (2) is that you get more HiZ resolves than needed.<br><br></div><div>--Jason<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The intel_miptree_slice_set_needs_<wbr>hiz_resolve() needs to happen *after*<br>
the successfull call to emit_miptree_blit().<br>
<span class=""><br>
> > > > ><br>
> > > > >     if (src_flip)<br>
> > > > >        src_y = minify(src_mt->physical_<wbr>height0, src_level -<br>
> > > src_mt->first_level) - src_y - height;<br>
> > > > > @@ -387,6 +388,7 @@ intel_miptree_copy(struct brw_context *brw,<br>
> > > > >     intel_miptree_slice_resolve_<wbr>depth(brw, dst_mt, dst_level,<br>
> > > dst_slice);<br>
> > > > >     intel_miptree_resolve_color(<wbr>brw, src_mt, src_level, src_slice, 1,<br>
> > > 0);<br>
> > > > >     intel_miptree_resolve_color(<wbr>brw, dst_mt, dst_level, dst_slice, 1,<br>
> > > 0);<br>
> > > > > +   intel_miptree_slice_set_needs_<wbr>hiz_resolve(dst_mt, dst_level,<br>
> > > dst_slice);<br>
<br>
</span>Same problem again.<br>
</blockquote></div><br></div></div>