wl_surface_commit from different thread

[Pekka Paalanen]

On Fri, 14 Feb 2014 16:11:54 -0600
Prabhu S <prabhusundar at gmail.com> wrote:

> Now I understand calling wl_surface_commit from different thread
> is not the problem. wl_surface_commit has to be called inside the
> swapbuffer before return, otherwise keeps waiting in the event
> loop.

It's more than that. Applications will issue wl_surface and other
requests to queue up state updates that will be kicked in on the
next wl_surface.commit, and they rely on eglSwapBuffers doing the
wl_surface.commit before it returns. This gives us atomic window
state changes without flicker, and guarantees that the application's
idea of window state is what has been actually sent to the server.

> Is there any way to pump the event queue after return from
> swapbuffer may be from different thread.

Sorry, I just cannot understand this question.

> Modified the simple-shm.c, there are 2 cases when SEM_WAIT = 0,
> in the main thread redraw function will not wait for the commit
> and commit will be performed by the commitworker thread, this
> results in the issue currently facing.

That's obviously wrong, the commit has to happen before the
application continues.

> If SEM_WAIT = 0, wl_surface_commit is sequential, everything
> seems to be fine.

I assume you mean 1. Yes, that is the correct order of operations.

> Any idea what event need to be posted in the commitworker to pump
> the event queue.

Nothing as far as I can see. The main thread will continue in the
redraw function only after the commit has been done, and it then
flushes in the mail loop like it normally does. Not doing the
commit before returning from the redraw function is wrong.

You have to guarantee, that the eglSwapBuffers call will not return
before the wl_surface.commit has been issued.

I wonder if your threading case is clouding a very different
problem. If the application has a thread that repeatedly draws and
calls eglSwapBuffers without returning to its main loop, it would
be good if eglSwapBuffers did a wl_display_flush() to flush all
requests to the server.

Or is your problem perhaps about dispatching incoming Wayland
events? That would be a completely different kind of matter.

Finally, looking at the WAYLAND_DEBUG=client output with whatever
client you develop with should be enlightening. The requests are
sent to the server in the order the protocol functions are called,
and the order is very important. If you send requests from multiple
threads, you need to guarantee the correct ordering. If the
client is waiting for events that do not seem to arrive, using
WAYLAND_DEBUG=server with the compositor will show reliably if and
when the events are emitted.


Thanks,
pq


> 
> diff --git a/Makefile.am b/Makefile.am
> index dc50da3..02d7869
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -387,7 +387,7 @@ weston_simple_shm_SOURCES =            \
>      shared/os-compatibility.c        \
>      shared/os-compatibility.h
>  weston_simple_shm_CFLAGS = $(AM_CFLAGS) $(SIMPLE_CLIENT_CFLAGS)
> -weston_simple_shm_LDADD = $(SIMPLE_CLIENT_LIBS)
> +weston_simple_shm_LDADD = $(SIMPLE_CLIENT_LIBS) -lpthread
> 
>  weston_simple_touch_SOURCES =            \
>      clients/simple-touch.c            \
> diff --git a/clients/simple-shm.c b/clients/simple-shm.c
> old mode 100644
> new mode 100755
> index 40f1633..4078b2f
> --- a/clients/simple-shm.c
> +++ b/clients/simple-shm.c
> @@ -31,11 +31,14 @@
>  #include <unistd.h>
>  #include <sys/mman.h>
>  #include <signal.h>
> +#include <pthread.h>
> +#include <semaphore.h>
> 
>  #include <wayland-client.h>
>  #include "../shared/os-compatibility.h"
>  #include "xdg-shell-client-protocol.h"
> -
> +const int SEM_WAIT = 0;
> +#define SHARED 1
>  struct display {
>      struct wl_display *display;
>      struct wl_registry *registry;
> @@ -45,6 +48,9 @@ struct display {
>      uint32_t formats;
>  };
> 
> +sem_t empty, full;    /* the global semaphores */
> +pthread_t commitThread;
> +
>  struct buffer {
>      struct wl_buffer *buffer;
>      void *shm_data;
> @@ -308,7 +314,12 @@ redraw(void *data, struct wl_callback
> *callback, uint32_t time)
> 
>      window->callback = wl_surface_frame(window->surface);
>      wl_callback_add_listener(window->callback, &frame_listener,
> window);
> -    wl_surface_commit(window->surface);
> +
> +    sem_post(&full);
> +    if(SEM_WAIT == 1)
> +    {
> +        sem_wait(&empty);
> +    }
>      buffer->busy = 1;
>  }
> 
> @@ -421,6 +432,17 @@ signal_int(int signum)
>      running = 0;
>  }
> 
> +void* commitWorker(void* arg)
> +{
> +    struct window *window = arg;
> +    while (1)
> +    {
> +        sem_wait(&full);
> +        wl_surface_commit(window->surface);
> +        sem_post(&empty);
> +    }
> +    return NULL;
> +}
>  int
>  main(int argc, char **argv)
>  {
> @@ -430,6 +452,7 @@ main(int argc, char **argv)
>      int ret = 0;
> 
>      display = create_display();
> +
>      window = create_window(display, 250, 250);
>      if (!window)
>          return 1;
> @@ -439,6 +462,10 @@ main(int argc, char **argv)
>      sigint.sa_flags = SA_RESETHAND;
>      sigaction(SIGINT, &sigint, NULL);
> 
> +    sem_init(&empty, SHARED, 0);  /* sem empty = 1 */
> +    sem_init(&full, SHARED, 0);   /* sem full = 0  */
> +    pthread_create(&commitThread, NULL, &commitWorker, window);
> +
>      /* Initialise damage to full surface, so the padding gets
> painted */ wl_surface_damage(window->surface, 0, 0,
>                window->width, window->height);
> 
> 
> 
> On Fri, Feb 7, 2014 at 8:18 AM, Jason Ekstrand
> <jason at jlekstrand.net> wrote:
> 
> > Hi Prabhu,
> > Could you be a little more specific as to what you are doing.
> > It sounds like you are either writing a client or trying to
> > write the client-side wayland bits for a driver stack.
> > However, it's kind of hard from you description to tell exactly
> > what you're working on.  Nevertheless, I will try to answer
> > your question as best I can.
> >
> > While Weston itself is single-threaded, the libwayland client
> > library can handle multiple threads rather well.  Look into
> > wl_event_queue which allows you to manage what events get
> > called on what thread.  Also, any request can be called from
> > any thread.  The only issue is in the synchronization that your
> > app may need to do (know when wl_surface.commit occurs relative
> > to wl_shell_surface.set_maximized for instance).  If you want
> > to get a frame completion event in another thread, simply call
> > wl_surface.frame, wl_surface.commit and then add the callback
> > you got from wl_surface.frame to the wl_event_queue for that
> > thread.
> >
> > Also, I'm confused by the relationship between your
> > eglSwapBuffers calls and your wl_surface.commit calls.
> > eglSwapBuffers should be calling wl_surface.commit before it
> > returns.  Sometimes this means the underlying graphics drivers
> > pass sync fences or do other things to keep from doing a full
> > glFinish and stalling the GPU. Therefore, in most cases, you
> > shouldn't need to be calling wl_surface.commit manually.
> > Perhaps I'm just misunderstanding something?
> >
> > I hope that helps,
> > --Jason Ekstrand
> >
> >
> > On Fri, Feb 7, 2014 at 8:03 AM, Prabhu S
> > <prabhusundar at gmail.com> wrote:
> >
> >> Hello,
> >> eglSwapBuffers is a non blocking call. Normally, 3D GPU will
> >> be used both for content creation and composition, in that
> >> case everything will end up the GPU unit. So the end frame
> >> will be will be synchronized. (assuming 1 GPU unit).
> >>
> >> I have a scenario, where 3D GPU being used for creating
> >> content and blit engine used for Weston composition. I'm
> >> getting frame completion from 3D engine in different thread
> >> (not in the same thread eglSwapBuffers calling thread).
> >> Currently eglSwapBuffers calling thread need to wait for the
> >> frame completion and then call wl_surface_commit to notify
> >> Wayland compositor. This takes out most of the performance
> >> since GPUs are not active all the time.
> >>
> >> wl_surface_commit cannot be called from different thread
> >> AFAIK. Some advise, suggestions would be appreciated.
> >>
> >> Thanks
> >> Prabhu