On Fri 02 Jun 2017, Jason Ekstrand wrote: > On Fri, Jun 2, 2017 at 1:01 PM, Chad Versace <[1]chadvers...@chromium.org> > wrote: > > On Wed 31 May 2017, Pohjolainen, Topi wrote: > > On Wed, May 31, 2017 at 10:22:09AM -0700, Jason Ekstrand wrote: > > > On Tue, May 30, 2017 at 7:29 AM, Pohjolainen, Topi < > > > [2]topi.pohjolai...@gmail.com> wrote: > > > > > > On Fri, May 26, 2017 at 04:30:05PM -0700, Jason Ekstrand wrote: > > > > > > Cc: "17.0 17.1" <[3]mesa-sta...@lists.freedesktop.org> > > > > > > --- > > > > > > src/mesa/drivers/dri/i965/intel_blit.c | 2 ++ > > > > > > 1 file changed, 2 insertions(+) > > > > > > > > > > > > diff --git a/src/mesa/drivers/dri/i965/intel_blit.c > > > > b/src/mesa/drivers/dri/i965/intel_blit.c > > > > > > index 2925fc2..b1e1eaa 100644 > > > > > > --- a/src/mesa/drivers/dri/i965/intel_blit.c > > > > > > +++ b/src/mesa/drivers/dri/i965/intel_blit.c > > > > > > @@ -329,6 +329,7 @@ intel_miptree_blit(struct brw_context *brw, > > > > > > intel_miptree_slice_resolve_depth(brw, dst_mt, dst_level, > > > > dst_slice); > > > > > > intel_miptree_resolve_color(brw, src_mt, src_level, > src_slice, 1, > > > > 0); > > > > > > intel_miptree_resolve_color(brw, dst_mt, dst_level, > dst_slice, 1, > > > > 0); > > > > > > + intel_miptree_slice_set_needs_hiz_resolve(dst_mt, dst_level, > > > > dst_slice); > > Something feels wrong. Suppose that, before the blit, the HiZ buffer > contains significant data. Here, we schedule an op to invalidate the HiZ > data. Also, suppose the the below call to emit_miptree_blit() rejects > the blit (maybe because the pitch is too big) and returns false. The > failed blit will likely emit a GL error or, in some cases, i965 will > fallback to to doing the blit with the 3D engine. In either case, there > is a likely bug waiting because the HiZ buffer is scheduled to have its > HiZ data invalidated before its next use, but the HiZ data is still > significant. > > > I agree that you are technically mostly correct. However, I don't think it's > actually going to matter in practice for 3 reasons: > > 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. > > 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. > > 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.
You convinced with me point 3. I failed to take into account the depth resolve a few lines above. Reviewed-by: Chad Versace <chadvers...@chromium.org> _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
