[Mesa-dev] [PATCH 08/70] i965: Remove early release of DRI2 miptree

[Martin Peres]

On 28/09/15 17:27, Emil Velikov wrote:
> Hi all,
>
> On 17 August 2015 at 19:06, Chad Versace <chad.versace at intel.com> wrote:
>> On Fri 14 Aug 2015, Chris Wilson wrote:
>>> On Thu, Aug 13, 2015 at 09:58:52PM -0700, Kenneth Graunke wrote:
>>>> On Thursday, August 13, 2015 02:57:20 PM Martin Peres wrote:
>>>>> On 07/08/15 23:13, Chris Wilson wrote:
>>>>>> intel_update_winsys_renderbuffer_miptree() will release the existing
>>>>>> miptree when wrapping a new DRI2 buffer, so we can remove the early
>>>>>> release and so prevent a NULL mt dereference should importing the new
>>>>>> DRI2 name fail for any reason. (Reusing the old DRI2 name will result
>>>>>> in the rendering going astray, to a stale buffer, and not shown on the
>>>>>> screen, but it allows us to issue a warning and not crash much later in
>>>>>> innocent code.)
>>>>>>
>>>>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>>>>> ---
>>>>>>    src/mesa/drivers/dri/i965/brw_context.c | 1 -
>>>>>>    1 file changed, 1 deletion(-)
>>>>>>
>>>>>> diff --git a/src/mesa/drivers/dri/i965/brw_context.c b/src/mesa/drivers/dri/i965/brw_context.c
>>>>>> index e8d1396..72f3897 100644
>>>>>> --- a/src/mesa/drivers/dri/i965/brw_context.c
>>>>>> +++ b/src/mesa/drivers/dri/i965/brw_context.c
>>>>>> @@ -1388,7 +1388,6 @@ intel_process_dri2_buffer(struct brw_context *brw,
>>>>>>                  buffer->cpp, buffer->pitch);
>>>>>>       }
>>>>>>
>>>>>> -   intel_miptree_release(&rb->mt);
>>>>>>       bo = drm_intel_bo_gem_create_from_name(brw->bufmgr, buffer_name,
>>>>>>                                              buffer->name);
>>>>>>       if (!bo) {
>>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=86281
>>>>> Reviewed-by: Martin Peres <martin.peres at linux.intel.com>
>>>>>
>>>>> If no-one has any objection, I will push the patch tomorrow to avoid the
>>>>> crashes experienced by a lot of users, myself included!
>>>> (resending with Chris's actual email...sorry!)
>>>>
>>>> This seems reasonable from a robustness point of view - try to limp
>>>> along a bit further and hope the import works next time, somehow.
>>>>
>>>> That said, I do have one concern: we might get into trouble with
>>>> multisample buffers.
>>>>
>>>> In the multisample case, irb->mt is the MSAA buffer, and
>>>> irb->singlesample_mt is the actual single-sampled DRI2 buffer.
>>>>
>>>> Previously, this call always released irb->mt, deleting the MSAA buffer
>>>> every time.  Then, intel_update_winsys_renderbuffer_miptree would hit
>>>> the !irb->mt case, creating a fresh new irb->mt, and setting
>>>> irb->need_downsample = false.  That may not happen now, which is a
>>>> subtle change.  (It might be OK...)
>>>>
>>>> If front buffer drawing, we call intel_renderbuffer_upsample(), which
>>>> asserts that irb->need_downsample == false.  So...if somehow we hit this
>>>> path with a dirty multisample buffer and front buffer rendering...then
>>>> I think we'd assert fail and die.  I'm not sure if that can happen; it
>>>> at least seems unlikely...
>>>>
>>>> Do you think the new multisampled behavior will work OK?  If so, perhaps
>>>> we can at least leave a note about it in the commit message.
>>> Hmm, yes we should be invalidating the multisample buffer if we swap out
>>> the "true" singlesample buffer beneath it. Either the contents are
>>> irrelevant (new undefined back buffer after swap) or the contents are
>>> well defined and different (front buffer, aged back buffer). In the
>>> latter case, we expect we flushed the multisample buffer before the
>>> swap, so the only likely case is intel_prepare_render() being handed a
>>> new front buffer which could have pending rendering, now invalid.
>>>
>>> Just disregarding the unresolved multisample buffer seems like an easy
>>> consistency fix:
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>>> index e85c3f0..1ed39f2 100644
>>> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>>> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>>> @@ -837,6 +837,7 @@ intel_update_winsys_renderbuffer_miptree(struct brw_context *intel,
>>>      } else {
>>>         intel_miptree_release(&irb->singlesample_mt);
>>>         irb->singlesample_mt = singlesample_mt;
>>> +      irb->need_downsample = false;
>>>
>>>         if (!irb->mt ||
>>>             irb->mt->logical_width0 != width ||
>>> @@ -849,7 +850,6 @@ intel_update_winsys_renderbuffer_miptree(struct brw_context *intel,
>>>            if (!multisample_mt)
>>>               goto fail;
>>>
>>> -         irb->need_downsample = false;
>>>            intel_miptree_release(&irb->mt);
>>>            irb->mt = multisample_mt;
>>>         }
>>>
>>> --
>>> Chris Wilson, Intel Open Source Technology Centre
>> Moving `irb->need_downsample = false` to eariler in
>> intel_update_winsys_renderbuffer_miptree() eliminates some problems, but a
>> crash still remains.  Explanation in annotated code below.
>>
>>     static void
>>     intel_process_dri2_buffer
>>     {
>>        ...
>>
>>        // Suppose this fails.
>>        intel_update_winsys_renderbuffer_miptree(brw, rb, bo,
>>                                                 drawable->w, drawable->h,
>>                                                 buffer->pitch);
>>
>>        // And suppose we are rendering to the front buffer.
>>        if (brw_is_front_buffer_drawing(fb) &&
>>            (buffer->attachment == __DRI_BUFFER_FRONT_LEFT ||
>>             buffer->attachment == __DRI_BUFFER_FAKE_FRONT_LEFT) &&
>>            rb->Base.Base.NumSamples > 1) {
>>
>>           // Then this crashes because, as the comment to
>>           // intel_renderbuffer_sample() states below, the "upsample is done
>>           // unconditionally", and it will access bogus miptrees.
>>           intel_renderbuffer_upsample(brw, rb);
>>        }
>>
>>        ...
>>     }
>>
>>     /**
>>      * \brief Upsample a winsys renderbuffer from singlesample_mt to mt.
>>      *
>>      * The upsample is done unconditionally.
>>      */
>>     void
>>     intel_renderbuffer_upsample(struct brw_context *brw,
>>                                 struct intel_renderbuffer *irb)
>>     {
>>        assert(!irb->need_downsample);
>>
>>        intel_miptree_updownsample(brw, irb->singlesample_mt, irb->mt);
>>     }
> As I haven't been following the discussion, can anyone point me to the
> solution (if any) on this topic ?
> Archlinux seems to be using the original "remove early release..."
> alongside 10.6 and 11.0 as it is still an issue [1]. On the other hand
> I've not seen any other distro picking it/any fixes on top of the
> upstream tarball.
>
> Thanks
> Emil
>
> [1] https://bugs.archlinux.org/task/45750
I would be in favour of merging this patch right now. It is partial and 
will require more work to handle more cases but this is already a huge 
improvement for kde users.

Martin