[Mesa-dev] [PATCH 09/17] mesa/st: add support for waiting for semaphore objects

[Nicolai Hähnle]

On 02.11.2017 04:57, Andres Rodriguez wrote:
> Bits to implement ServerWaitSemaphoreObject/ServerSignalSemaphoreObject
> 
> Signed-off-by: Andres Rodriguez <andresx7 at gmail.com>
> ---
>   src/mesa/state_tracker/st_cb_semaphoreobjects.c | 28 +++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/src/mesa/state_tracker/st_cb_semaphoreobjects.c b/src/mesa/state_tracker/st_cb_semaphoreobjects.c
> index 47ece47..4cff3fd 100644
> --- a/src/mesa/state_tracker/st_cb_semaphoreobjects.c
> +++ b/src/mesa/state_tracker/st_cb_semaphoreobjects.c
> @@ -1,5 +1,7 @@
> +
>   #include "main/imports.h"
>   #include "main/mtypes.h"
> +#include "main/context.h"
>   
>   #include "main/externalobjects.h"
>   
> @@ -47,10 +49,36 @@ st_import_semaphoreobj_fd(struct gl_context *ctx,
>   #endif
>   }
>   
> +static void
> +st_server_wait_semaphore(struct gl_context *ctx,
> +                         struct gl_semaphore_object *semObj)
> +{
> +   struct st_semaphore_object *st_obj = st_semaphore_object(semObj);
> +   struct st_context *st = st_context(ctx);
> +   struct pipe_context *pipe = st->pipe;
> +
> +   _mesa_flush(ctx);

What's the justification of this flush?


> +   pipe->semobj_wait(pipe, st_obj->semaphore);
> +}
> +
> +static void
> +st_server_signal_semaphore(struct gl_context *ctx,
> +                           struct gl_semaphore_object *semObj)
> +{
> +   struct st_semaphore_object *st_obj = st_semaphore_object(semObj);
> +   struct st_context *st = st_context(ctx);
> +   struct pipe_context *pipe = st->pipe;
> +

You have to flush state-tracker internal state here. This means 
FLUSH_VERTICES, st_flush_bitmap, possibly others. I don't think we have 
this factored out well yet, but see below.


> +   pipe->semobj_signal(pipe, st_obj->semaphore);
> +   _mesa_flush(ctx);

What's the justification of this flush? Does EXT_external_objects 
contain any language to guarantee that SignalSemaphoreEXT will "finish 
in finite time"? If no, the flush should probably not be there. If yes, 
please add a spec quote.

Furthermore, this whole code section is a strong hint that merging 
fences and semaphores at the Gallium level is indeed a good idea. 
Basically, the idea is that you'd end up calling

     pipe->flush(pipe, &st_obj->semaphore, flags);

where flags is PIPE_FLUSH_DEFERRED | PIPE_FLUSH_REUSE_FENCE or 
PIPE_FLUSH_ASYNC | PIPE_FLUSH_REUSE_FENCE, depending on how the previous 
paragraph is resolved. In one of my recent series that includes deferred 
fences for threaded contexts, I'm already doing the equivalent of adding 
a "PIPE_FLUSH_REUSE_FENCE". We could formalize this as a more broadly 
applicable feature in the Gallium interface.

Cheers,
Nicolai


> +}
> +
>   void
>   st_init_semaphoreobject_functions(struct dd_function_table *functions)
>   {
>      functions->NewSemaphoreObject = st_semaphoreobj_alloc;
>      functions->DeleteSemaphoreObject = st_semaphoreobj_free;
>      functions->ImportSemaphoreFd = st_import_semaphoreobj_fd;
> +   functions->ServerWaitSemaphoreObject = st_server_wait_semaphore;
> +   functions->ServerSignalSemaphoreObject = st_server_signal_semaphore;
>   }
> 


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.