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

[Jason Ekstrand]

On Fri, Jun 2, 2017 at 1:01 PM, Chad Versace <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 <
> > > topi.pohjolainen at gmail.com> wrote:
>
> > > > > On Fri, May 26, 2017 at 04:30:05PM -0700, Jason Ekstrand wrote:
> > > > > > Cc: "17.0 17.1" <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.

--Jason


> The intel_miptree_slice_set_needs_hiz_resolve() needs to happen *after*
> the successfull call to emit_miptree_blit().
>
> > > > > >
> > > > > >     if (src_flip)
> > > > > >        src_y = minify(src_mt->physical_height0, src_level -
> > > > src_mt->first_level) - src_y - height;
> > > > > > @@ -387,6 +388,7 @@ intel_miptree_copy(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);
>
> Same problem again.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-stable/attachments/20170602/7ed707ba/attachment.html>