[Mesa-stable] [Mesa-dev] [PATCH 01/30] i965: Mark depth surfaces as needing a HiZ resolve after blitting

[Chad Versace]

On Fri 02 Jun 2017, Jason Ekstrand wrote:
> On Fri, Jun 2, 2017 at 1:01 PM, Chad Versace <[1]chadversary at 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.pohjolainen at gmail.com> wrote:
> 
>     > > > > On Fri, May 26, 2017 at 04:30:05PM -0700, Jason Ekstrand wrote:
>     > > > > > Cc: "17.0 17.1" <[3]mesa-stable at 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 <chadversary at chromium.org>