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

[Kenneth Graunke]

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!

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.

Thanks!
--Ken
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150813/b5814011/attachment.sig>