[2/2] loader_dri3: Variant 2: Wait for pending swaps to complete before drawable_fini.

Submitted by Michel Dänzer on May 8, 2018, 9:42 a.m.

Details

Message ID ad7564c6-1e1b-8322-3e5c-d8045e181e10@daenzer.net
State Superseded
Headers show
Series "Series without cover letter" ( rev: 3 ) in Mesa

Not browsing as part of any series.

Commit Message

Michel Dänzer May 8, 2018, 9:42 a.m.
On 2018-05-05 06:25 AM, Mario Kleiner wrote:
> On Sat, May 5, 2018 at 4:08 AM, Mike Lothian <mike@fireburn.co.uk> wrote:
>> I definately saw the steam bug with patch 1 but not with plasmashell,
>> I started seeing it with patch 2 but it seemed to fix itself
> 
> I had two hangs of kwin_x11 within the last 6 hours when alt-tabbing
> between windows, where it got stuck in the
> loader_dri3_swapbuffer_barrier() from patch 1/2. Not sure how that is
> possible, or if the stacktrace was misleading, because i had to VT
> switch to a text console to attach the debugger and this might be just
> a side effect of that. But if it is true, then patch 1/2 would not be
> it. Also 1/2 has a potential performance impact, whereas 2/2 doesn't.
> However 2/2 would also need more work, as i can think of more complex
> scenarios where it would filter the wrong events, although not in the
> case of plasmashell or steam. Probably we'd need to sacrifice a few
> sbc bits in the Present events serial field to transport a unique tag
> for each incarnation of the loader_dri3_drawable, like a mini-hash of
> the draw->eid. Ugly ugly...

How about the below?

Idle notify events shouldn't need special treatment, since the pixmap
XIDs of the buffers will be different between loader_dri3_drawable
incarnations, aren't they?


This still leaves the issue that the SBC moves backwards, which could
theoretically result in hangs with apps using glXWaitForSbcOML. Fixing
that would probably require changing the loader_dri3_drawable lifetime
cycle, which would probably be very invasive, if feasible at all. Maybe
we don't need to care about that for the time being, until there's a
real world app running into it.

Patch hide | download patch | download mbox

diff --git a/src/loader/loader_dri3_helper.c b/src/loader/loader_dri3_helper.c
index 6db8303d26d..f0ff2f07bde 100644
--- a/src/loader/loader_dri3_helper.c
+++ b/src/loader/loader_dri3_helper.c
@@ -370,9 +370,17 @@  dri3_handle_present_event(struct loader_dri3_drawable *draw,
        * checking for wrap.
        */
       if (ce->kind == XCB_PRESENT_COMPLETE_KIND_PIXMAP) {
-         draw->recv_sbc = (draw->send_sbc & 0xffffffff00000000LL) | ce->serial;
-         if (draw->recv_sbc > draw->send_sbc)
-            draw->recv_sbc -= 0x100000000;
+         uint64_t recv_sbc = (draw->send_sbc & 0xffffffff00000000LL) | ce->serial;
+
+         /* Only assume wraparound if that results in exactly the previous
+          * SBC + 1, otherwise ignore received SBC > sent SBC (those are
+          * probably from a previous loader_dri3_drawable instance) to avoid
+          * calculating bogus target MSC values in loader_dri3_swap_buffers_msc
+          */
+         if (recv_sbc <= draw->send_sbc)
+            draw->recv_sbc = recv_sbc;
+         else if (recv_sbc == (draw->recv_sbc + 0x100000001ULL))
+            draw->recv_sbc = recv_sbc - 0x100000000ULL;

          /* When moving from flip to copy, we assume that we can allocate in
           * a more optimal way if we don't need to cater for the display

Comments

On Tue, 2018-05-08 at 11:42 +0200, Michel Dänzer wrote:

> Idle notify events shouldn't need special treatment, since the pixmap
> XIDs of the buffers will be different between loader_dri3_drawable
> incarnations, aren't they?

We're destroying loader_dri3_drawable structs at MakeCurrent time, but
not destroying actual drawables, so I don't think the XIDs will change.

- ajax
On 2018-05-09 07:32 PM, Adam Jackson wrote:
> On Tue, 2018-05-08 at 11:42 +0200, Michel Dänzer wrote:
> 
>> Idle notify events shouldn't need special treatment, since the pixmap
>> XIDs of the buffers will be different between loader_dri3_drawable
>> incarnations, aren't they?
> 
> We're destroying loader_dri3_drawable structs at MakeCurrent time, but
> not destroying actual drawables, so I don't think the XIDs will change.

I'm talking about loader_dri3_buffer::pixmap, which are destroyed in
loader_dri3_drawable_fini -> dri3_free_render_buffer.