[Mesa-stable] [Mesa-dev] [PATCH 01/17] i965/miptree: Fix handling of uninitialized MCS buffers

[Nanley Chery]

On Fri, May 04, 2018 at 09:03:32AM +0300, Tapani Pälli wrote:
> Hi Nanley;
> 

Hey Tapani,

> On 05/03/2018 10:03 PM, Nanley Chery wrote:
> > Before this patch, if we failed to initialize an MCS buffer, we'd
> > end up in a state in which the miptree thinks it has an MCS buffer,
> > but doesn't. We also leaked the clear_color_bo if it existed.
> > 
> > With this patch, we now free the miptree aux buffer resources and let
> > intel_miptree_alloc_mcs() know that the MCS buffer no longer exists.
> 
> I triggered this by forcing the failure (by inserting 1 instead unlikely(map
> ==NULL) to mcs buffer map condition) and following happens currently:

Thanks for testing this! It'd be great if we could have a piglit test
that would trigger this case for us, but it's not clear to me what that
would look like.

> 
> --- 8< ---
> Failed to map mcs buffer into GTT
> deqp-egl: ../src/intel/blorp/blorp_clear.c:313: blorp_fast_clear: Assertion
> `start_layer + num_layers <= MAX2(surf->aux_surf->logical_level0_px.depth >>
> level, surf->aux_surf->logical_level0_px.array_len)' failed.
> Aborted (core dumped)

I can confirm a similar assertion with piglit's
arb_clear_texture-multisample. What's happening here is that
blorp_fast_clear is receiving garbage aux surfaces, which causes it to
randomly trigger different assertions.

> --- 8< ---
> 
> However even with this fix it seems we will end up in same or similar case
> (segfault happening in do_single_blorp_clear):
> 
> --- 8< ---
> Failed to map mcs buffer into GTT
> Segmentation fault (core dumped)
> --- 8< ---
> 
> Test case used was:
>    dEQP-EGL.functional.color_clears.single_context.gles2.rgba8888_window

This is interesting. What happens here is that we enter
do_single_blorp_clear() with irb->mt == NULL and we segfault at:

   if (irb->Base.Base.Format != irb->mt->format)

It looks like some callers of intel_miptree_create() don't handle
failure well. Maybe they should throw an EGL error? Here's the backtrace
from the failed init:

#0  intel_miptree_init_mcs
#1  intel_miptree_alloc_mcs
#2  intel_miptree_alloc_aux
#3  intel_miptree_create
#4  intel_miptree_create_for_renderbuffer
#5  intel_update_winsys_renderbuffer_miptree
#6  intel_update_image_buffer
#7  intel_update_image_buffers
#8  intel_update_renderbuffers
#9  intel_prepare_render
#10 intelMakeCurrent
#11 driBindContext
#12 ?? () from /usr/lib/libEGL_mesa.so.0
#13 eglMakeCurrent
#14 ?? () from /usr/lib/libEGL.so
#15 ?? () from /usr/lib/libEGL.so
#16 deqp::egl::SingleThreadColorClearCase::executeForContexts
#17 deqp::egl::MultiContextRenderCase::executeForSurface
#18 deqp::egl::RenderCase::executeForConfig
#19 deqp::egl::SimpleConfigCase::iterate
#20 tcu::TestSessionExecutor::iterateTestCase
#21 tcu::TestSessionExecutor::iterate 
#22 tcu::App::iterate
#23 main (argc=<optimized out>, argv=<optimized out>)

> 
> 
> Are we expected to survive without mcs?

I think it depends. Once we determine that we can enable MCS for a
miptree, we require that the aux buffer exist in order to successfully
create the miptree. In GL, if we can't successfully create a miptree, we
can throw a GL_OUT_OF_MEMORY error, and have mostly undefined behavior
for the rest of our existence. In EGL however, I think we can throw an
error that's not EGL_BAD_CONTEXT and carry on.

-Nanley

> 
> 
> > Cc: <mesa-stable at lists.freedesktop.org>
> > ---
> >   src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 14 +++++++-------
> >   1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > index b9a564552df..377efae32c9 100644
> > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > @@ -1658,7 +1658,7 @@ intel_miptree_copy_teximage(struct brw_context *brw,
> >      intel_obj->needs_validate = true;
> >   }
> > -static void
> > +static bool
> >   intel_miptree_init_mcs(struct brw_context *brw,
> >                          struct intel_mipmap_tree *mt,
> >                          int init_value)
> > @@ -1678,13 +1678,14 @@ intel_miptree_init_mcs(struct brw_context *brw,
> >      void *map = brw_bo_map(brw, mt->aux_buf->bo, MAP_WRITE | MAP_RAW);
> >      if (unlikely(map == NULL)) {
> >         fprintf(stderr, "Failed to map mcs buffer into GTT\n");
> > -      brw_bo_unreference(mt->aux_buf->bo);
> > -      free(mt->aux_buf);
> > -      return;
> > +      intel_miptree_aux_buffer_free(mt->aux_buf);
> > +      mt->aux_buf = NULL;
> > +      return false;
> >      }
> >      void *data = map;
> >      memset(data, init_value, mt->aux_buf->size);
> >      brw_bo_unmap(mt->aux_buf->bo);
> > +   return true;
> >   }
> >   static struct intel_miptree_aux_buffer *
> > @@ -1764,15 +1765,14 @@ intel_miptree_alloc_mcs(struct brw_context *brw,
> >      const uint32_t alloc_flags = 0;
> >      mt->aux_buf = intel_alloc_aux_buffer(brw, "mcs-miptree",
> >                                           &temp_mcs_surf, alloc_flags, mt);
> > -   if (!mt->aux_buf) {
> > +   if (!mt->aux_buf ||
> > +       !intel_miptree_init_mcs(brw, mt, 0xFF)) {
> >         free(aux_state);
> >         return false;
> >      }
> >      mt->aux_state = aux_state;
> > -   intel_miptree_init_mcs(brw, mt, 0xFF);
> > -
> >      return true;
> >   }
> >