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

[Tapani Pälli]

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.palli at intel.com>


> #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;
>>>    }
>>>