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

[Chris Wilson]

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