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

[Chad Versace]

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