On 28/09/15 17:27, Emil Velikov wrote:
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
I would be in favour of merging this patch right now. It is partial and
will require more work to handle more cases but this is already a huge
improvement for kde users.
Martin
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
