Hi all,
On 17 August 2015 at 19:06, Chad Versace <chad.vers...@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 <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);
> }
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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
