On Mon, May 07, 2018 at 09:49:01AM +0300, Tapani Pälli wrote:
>
> On 05/04/2018 05:59 PM, Nanley Chery wrote:
> > 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
>
> Problem is that the error propagation stops around here,
> intel_update_image_buffer (or intel_prepare_render) so there's no way
> currently for EGL to know.
>
> However, this patch makes sense to me and makes the overall state cleaner so
> that we could actually propagate error if wanted;
>
> Reviewed-by: Tapani Pälli <tapani.pa...@intel.com>
>
Thanks!
-Nanley
>
> > #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-sta...@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;
> > > > }
> > > >
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
