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 <ch...@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.pe...@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);
}
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
