On 17-01-03 13:12:43, Chad Versace wrote:
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 <[email protected]>
> > Cc: Chad Versace <[email protected]>
> > Signed-off-by: Ben Widawsky <[email protected]>
> > ---
> > 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
Thanks Chad. I'll see if I can fix the bug in a way that allows me to keep what
I need in place.
_______________________________________________
mesa-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
