[Mesa-dev] [PATCH] loader/dri3: Try to make sure we only process our own NotifyMSC events

[Michel Dänzer]

Ping (sorry I forgot to mark this as v2 in the subject)


On 2017-11-23 10:26 AM, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daenzer at amd.com>
> 
> We were using a sequence counter value to wait for a specific NotifyMSC
> event. However, we can receive events from other clients as well, which
> may already be using higher sequence numbers than us. In that case, we
> could stop processing after an event from another client, which could
> have been received significantly earlier. This would have multiple
> undesirable effects:
> 
> * The computed MSC and UST values would be lower than they should be
> * We could leave a growing number of NotifyMSC events from ourselves and
>   other clients in XCB's special event queue
> 
> I ran into this with Firefox and Thunderbird, whose VSync threads both
> seem to use the same window. The result was sluggish screen updates and
> growing memory consumption in one of them.
> 
> Fix this by checking the XCB sequence number and MSC value of NotifyMSC
> events, instead of using our own sequence number.
> 
> v2:
> * Use the Present event ID for the sequence parameter of the
>   PresentNotifyMSC request, as another safeguard against processing
>   events from other clients
> * Rebase on drawable mutex changes
> 
> Cc: mesa-stable at lists.freedesktop.org
> Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com> # v1
> Signed-off-by: Michel Dänzer <michel.daenzer at amd.com>
> ---
>  src/loader/loader_dri3_helper.c | 36 ++++++++++++++++++------------------
>  src/loader/loader_dri3_helper.h |  4 ----
>  2 files changed, 18 insertions(+), 22 deletions(-)
> 
> diff --git a/src/loader/loader_dri3_helper.c b/src/loader/loader_dri3_helper.c
> index 7e6b8b2e056..d4cd9737ab2 100644
> --- a/src/loader/loader_dri3_helper.c
> +++ b/src/loader/loader_dri3_helper.c
> @@ -384,8 +384,7 @@ dri3_handle_present_event(struct loader_dri3_drawable *draw,
>  
>           draw->ust = ce->ust;
>           draw->msc = ce->msc;
> -      } else {
> -         draw->recv_msc_serial = ce->serial;
> +      } else if (ce->serial == draw->eid) {
>           draw->notify_ust = ce->ust;
>           draw->notify_msc = ce->msc;
>        }
> @@ -453,28 +452,29 @@ loader_dri3_wait_for_msc(struct loader_dri3_drawable *draw,
>                           int64_t divisor, int64_t remainder,
>                           int64_t *ust, int64_t *msc, int64_t *sbc)
>  {
> -   uint32_t msc_serial;
> -
> -   msc_serial = ++draw->send_msc_serial;
> -   xcb_present_notify_msc(draw->conn,
> -                          draw->drawable,
> -                          msc_serial,
> -                          target_msc,
> -                          divisor,
> -                          remainder);
> +   xcb_void_cookie_t cookie = xcb_present_notify_msc(draw->conn,
> +                                                     draw->drawable,
> +                                                     draw->eid,
> +                                                     target_msc,
> +                                                     divisor,
> +                                                     remainder);
> +   xcb_generic_event_t *ev;
> +   unsigned full_sequence;
>  
>     mtx_lock(&draw->mtx);
>     xcb_flush(draw->conn);
>  
>     /* Wait for the event */
> -   if (draw->special_event) {
> -      while ((int32_t) (msc_serial - draw->recv_msc_serial) > 0) {
> -         if (!dri3_wait_for_event_locked(draw)) {
> -            mtx_unlock(&draw->mtx);
> -            return false;
> -         }
> +   do {
> +      ev = xcb_wait_for_special_event(draw->conn, draw->special_event);
> +      if (!ev) {
> +         mtx_unlock(&draw->mtx);
> +         return false;
>        }
> -   }
> +
> +      full_sequence = ev->full_sequence;
> +      dri3_handle_present_event(draw, (void *) ev);
> +   } while (full_sequence != cookie.sequence || draw->notify_msc < target_msc);
>  
>     *ust = draw->notify_ust;
>     *msc = draw->notify_msc;
> diff --git a/src/loader/loader_dri3_helper.h b/src/loader/loader_dri3_helper.h
> index 0dd37e91717..4ce98b8c59f 100644
> --- a/src/loader/loader_dri3_helper.h
> +++ b/src/loader/loader_dri3_helper.h
> @@ -137,10 +137,6 @@ struct loader_dri3_drawable {
>     /* Last received UST/MSC values from present notify msc event */
>     uint64_t notify_ust, notify_msc;
>  
> -   /* Serial numbers for tracking wait_for_msc events */
> -   uint32_t send_msc_serial;
> -   uint32_t recv_msc_serial;
> -
>     struct loader_dri3_buffer *buffers[LOADER_DRI3_NUM_BUFFERS];
>     int cur_back;
>     int num_back;
> 


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