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

[Ben Widawsky]

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 <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

Thanks Chad. I'll see if I can fix the bug in a way that allows me to keep what
I need in place.