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

[Emil Velikov]

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