[Mesa-dev] [PATCH 2/2] i965/miptree: Create a disable CCS flag

[Chad Versace]

On Tue 03 Jan 2017, Ben Widawsky wrote:
> On 17-01-03 08:21:06, Chad Versace wrote:
> > On Sun 01 Jan 2017, Ben Widawsky wrote:
> > > Cc: Topi Pohjolainen <topi.pohjolainen at intel.com>
> > > Cc: Chad Versace <chadversary at chromium.org>
> > > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_blorp.c         |  2 +-
> > >  src/mesa/drivers/dri/i965/brw_draw.c          |  3 ++-
> > >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 21 ++++++++++-----------
> > >  src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 10 +++-------
> > >  4 files changed, 16 insertions(+), 20 deletions(-)
> > 
> > 
> > > @@ -2334,19 +2334,18 @@ intel_miptree_make_shareable(struct brw_context *brw,
> > > 
> > >     if (mt->mcs_buf) {
> > >        intel_miptree_all_slices_resolve_color(brw, mt, 0);
> > > -      mt->no_ccs = true;
> > > +      mt->aux_disable |= (INTEL_AUX_DISABLE_CCS | INTEL_AUX_DISABLE_MCS);
> > >        drm_intel_bo_unreference(mt->mcs_buf->bo);
> > >        free(mt->mcs_buf);
> > >        mt->mcs_buf = NULL;
> > >     }
> > > 
> > >     if (mt->hiz_buf) {
> > > +      mt->aux_disable |= INTEL_AUX_DISABLE_HIZ;
> > >        intel_miptree_all_slices_resolve_depth(brw, mt);
> > >        intel_miptree_hiz_buffer_free(mt->hiz_buf);
> > >        mt->hiz_buf = NULL;
> > >     }
> > > -
> > > -   mt->aux_disable = INTEL_AUX_DISABLE_ALL;
> > >  }
> > 
> > All look goods to me except the above hunk. As written, in some
> > instances the driver will allocate (and use) the aux surface *after*
> > intel_miptree_make_shareable() unshares the miptree. To fix that, the
> > mt->aux_disable bits must be set unconditionally, outside the if's.
> > 
> 
> Getting unconditional disable is exactly what the patches are trying to get rid
> of (specifically for the CCS case). So I really hope there is some solution
> other than putting it outside of the if's.

I understand that is the eventual goal. And I see you recently posted
the RBC patch series.

> > The problem is that the driver sometimes allocates the aux surfaces
> > lazily. For evidence, grep for all instances of
> > intel_miptree_alloc_hiz() and intel_miptree_alloc_non_msrt_mcs() outside
> > of intel_mipmap_tree.c. If the code calls intel_miptree_make_shareable()
> > on a miptree for which the aux surface has not yet been allocated yet,
> > then intel_miptree_make_shareable() fails to set the disable bit.

> Hmm. I wasn't able to find a single regression with this patch, and everything
> seemed pretty okay to me with the code. Can you find a specific failing case,
> because I cannot?

I'm not surprised that the test suites found no regressions. The bug
depends on the user create the EGLImage before the miptree's first use.

So... I wrote a test and a bug :). And it seems that problem is worse
than I thought. The HiZ problem has causes crashes for a long time.
And your patch imporoves the situation for color buffers, changing
a crash to a fail.

    [Piglit] egl_khr_gl_image: Add test that clears a shared image
    https://lists.freedesktop.org/archives/piglit/2017-January/021626.html

    Bug 99265 - i965: Piglit egl_khr_gl_renderbuffer_image-clear-shared-image fails
    https://bugs.freedesktop.org/show_bug.cgi?id=99265