[Mesa-stable] [Mesa-dev] [PATCH] glx: Handle out-of-sequence swap completion events correctly. (v2)

[Michel Dänzer]

On 13.03.2015 04:34, Mario Kleiner wrote:
> The code for emitting INTEL_swap_events swap completion
> events needs to translate from 32-Bit sbc on the wire to
> 64-Bit sbc for the events and handle wraparound accordingly.
> 
> It assumed that events would be sent by the server in the
> order their corresponding swap requests were emitted from
> the client, iow. sbc count should be always increasing. This
> was correct for DRI2.
> 
> This is not always the case under the DRI3/Present backend,
> where the Present extension can execute presents and send out
> completion events in a different order than the submission
> order of the present requests, due to client code specifying
> targetMSC target vblank counts which are not strictly
> monotonically increasing. This confused the wraparound
> handling. This patch fixes the problem by handling 32-Bit
> wraparound in both directions. As long as successive swap
> completion events real 64-Bit sbc's don't differ by more
> than 2^30, this should be able to do the right thing.
> 
> How this is supposed to work:
> 
> awire->sbc contains the low 32-Bits of the true 64-Bit sbc
> of the current swap event, transmitted over the wire.
> 
> glxDraw->lastEventSbc contains the low 32-Bits of the 64-Bit
> sbc of the most recently processed swap event.
> 
> glxDraw->eventSbcWrap is a 64-Bit offset which tracks the upper
> 32-Bits of the current sbc. The final 64-Bit output sbc
> aevent->sbc is computed from the sum of awire->sbc and
> glxDraw->eventSbcWrap.
> 
> Under DRI3/Present, swap completion events can be received
> slightly out of order due to non-monotic targetMsc specified
> by client code, e.g., present request submission:
> 
> Submission sbc:   1   2   3
> targetMsc:        10  11  9
> 
> Reception of completion events:
> Completion sbc:   3   1   2
> 
> The completion sequence 3, 1, 2 would confuse the old wraparound
> handling made for DRI2 as 1 < 3 --> Assumes a 32-Bit wraparound
> has happened when it hasn't.
> 
> The client can queue multiple present requests, in the case of
> Mesa up to n requests for n-buffered rendering, e.g., n =  2-4 in
> the current Mesa GLX DRI3/Present implementation. In the case of
> direct Pixmap presents via xcb_present_pixmap() the number n is
> limited by the amount of memory available.
> 
> We reasonably assume that the number of outstanding requests n is
> much less than 2 billion due to memory contraints and common sense.
> Therefore while the order of received sbc's can be a bit scrambled,
> successive 64-Bit sbc's won't deviate by much, a given sbc may be
> a few counts lower or higher than the previous received sbc.
> 
> Therefore any large difference between the incoming awire->sbc and
> the last recorded glxDraw->lastEventSbc will be due to 32-Bit
> wraparound and we need to adapt glxDraw->eventSbcWrap accordingly
> to adjust the upper 32-Bits of the sbc.
> 
> Two cases, correponding to the two if-statements in the patch:
> 
> a) Previous sbc event was below the last 2^32 boundary, in the previous
> glxDraw->eventSbcWrap epoch, the new sbc event is in the next 2^32
> epoch, therefore the low 32-Bit awire->sbc wrapped around to zero,
> or close to zero --> awire->sbc is apparently much lower than the
> glxDraw->lastEventSbc recorded for the previous epoch
> 
> --> We need to increment glxDraw->eventSbcWrap by 2^32 to adjust
> the current epoch to be one higher than the previous one.
> 
> --> Case a) also handles the old DRI2 behaviour.
> 
> b) Previous sbc event was above closest 2^32 boundary, but now a
> late event from the previous 2^32 epoch arrives, with a true sbc
> that belongs to the previous 2^32 segment, so the awire->sbc of
> this late event has a high count close to 2^32, whereas
> glxDraw->lastEventSbc is closer to zero --> awire->sbc is much
> greater than glXDraw->lastEventSbc.
> 
> --> We need to decrement glxDraw->eventSbcWrap by 2^32 to adjust
> the current epoch back to the previous lower epoch of this late
> completion event.
> 
> We assume such a wraparound to a higher (a) epoch or lower (b)
> epoch has happened if awire->sbc and glxDraw->lastEventSbc differ
> by more than 2^30 counts, as such a difference can only happen
> on wraparound, or if somehow 2^30 present requests would be pending
> for a given drawable inside the server, which is rather unlikely.
> 
> v2: Explain the reason for this patch and the new wraparound handling
>     much more extensive in commit message, no code change wrt. initial
>     version.
> 
> Cc: "10.3 10.4 10.5" <mesa-stable at lists.freedesktop.org>
> Signed-off-by: Mario Kleiner <mario.kleiner.de at gmail.com>
> ---
>  src/glx/glxext.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/src/glx/glxext.c b/src/glx/glxext.c
> index 68c359e..fdc24d4 100644
> --- a/src/glx/glxext.c
> +++ b/src/glx/glxext.c
> @@ -143,8 +143,13 @@ __glXWireToEvent(Display *dpy, XEvent *event, xEvent *wire)
>        aevent->ust = ((CARD64)awire->ust_hi << 32) | awire->ust_lo;
>        aevent->msc = ((CARD64)awire->msc_hi << 32) | awire->msc_lo;
>  
> -      if (awire->sbc < glxDraw->lastEventSbc)
> -	 glxDraw->eventSbcWrap += 0x100000000;
> +      /* Handle 32-Bit wire sbc wraparound in both directions to cope with out
> +       * of sequence 64-Bit sbc's
> +       */
> +      if ((int64_t) awire->sbc < ((int64_t) glxDraw->lastEventSbc - 0x40000000))
> +         glxDraw->eventSbcWrap += 0x100000000;
> +      if ((int64_t) awire->sbc > ((int64_t) glxDraw->lastEventSbc + 0x40000000))
> +         glxDraw->eventSbcWrap -= 0x100000000;
>        glxDraw->lastEventSbc = awire->sbc;
>        aevent->sbc = awire->sbc + glxDraw->eventSbcWrap;
>        return True;
> 

Reviewed-by: Michel Dänzer <michel.daenzer at amd.com>


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer