Re: [Mesa-dev] [PATCH 7/7] broadcom/vc4: Native fence fd support

[Eric Anholt]

Stefan Schake  writes:

> With the syncobj support in place, lets use it to implement the
> native fence fd extension. This mostly follows previous implementations
> in freedreno and etnaviv.

Could we include the name of the actual extension being exposed, in the
commit message?

> Signed-off-by: Stefan Schake 
> ---
>  src/gallium/drivers/vc4/vc4_context.c | 47 
>  src/gallium/drivers/vc4/vc4_context.h |  5 +++
>  src/gallium/drivers/vc4/vc4_fence.c   | 68 
> ---
>  src/gallium/drivers/vc4/vc4_screen.c  |  6 ++--
>  src/gallium/drivers/vc4/vc4_screen.h  |  4 +--
>  5 files changed, 116 insertions(+), 14 deletions(-)
>
> diff --git a/src/gallium/drivers/vc4/vc4_context.c 
> b/src/gallium/drivers/vc4/vc4_context.c
> index 0deb3ef85e..d41f1b9a26 100644
> --- a/src/gallium/drivers/vc4/vc4_context.c
> +++ b/src/gallium/drivers/vc4/vc4_context.c
> @@ -37,30 +37,54 @@
>  #include "vc4_context.h"
>  #include "vc4_resource.h"
>  
> -void
> -vc4_flush(struct pipe_context *pctx)
> +static void
> +vc4_flush_sync(struct pipe_context *pctx, uint32_t *in_sync)
>  {
>  struct vc4_context *vc4 = vc4_context(pctx);
>  
>  struct hash_entry *entry;
>  hash_table_foreach(vc4->jobs, entry) {
>  struct vc4_job *job = entry->data;
> -vc4_job_submit(vc4, job);
> +vc4_job_submit_sync(vc4, job, in_sync);
>  }
>  }
>  
> +void
> +vc4_flush(struct pipe_context *pctx)
> +{
> +vc4_flush_sync(pctx, NULL);
> +}
> +
>  static void
>  vc4_pipe_flush(struct pipe_context *pctx, struct pipe_fence_handle **fence,
> unsigned flags)
>  {
>  struct vc4_context *vc4 = vc4_context(pctx);
>  
> -vc4_flush(pctx);
> +if (vc4->in_fence_fd >= 0) {
> +/* This replaces the fence in the syncobj. */
> +drmSyncobjImportSyncFile(vc4->fd, vc4->in_syncobj,
> + vc4->in_fence_fd);
> +close(vc4->in_fence_fd);
> +vc4->in_fence_fd = -1;
> +vc4_flush_sync(pctx, >in_syncobj);
> +} else {
> +vc4_flush(pctx);
> +}

I suspect this is the wrong place to be doing the in-fence handling.
Imagine the sequence:

- vc4_fence_server_sync()
- Render from some shared object to a texture
- Render from the texture to the screen
- eglSwapBuffers().

This pipe_flush will happen at the top of eglSwapBuffers().  However,
the render from texture to screen will have already flushed the first
render job through vc4_flush_jobs_writing_resource() on its source
texture, so that job didn't wait on the fence like it was supposed to.

I think the solution might be to move this in_fence_fd logic to
vc4_job_submit()?

>  
>  if (fence) {
>  struct pipe_screen *screen = pctx->screen;
> +int fd = -1;
> +
> +if (flags & PIPE_FLUSH_FENCE_FD) {
> +/* The vc4_fence takes ownership of the returned fd. 
> */
> +drmSyncobjExportSyncFile(vc4->fd, vc4->job_syncobj,
> + );
> +}
> +
>  struct vc4_fence *f = vc4_fence_create(vc4->screen,
> -   vc4->last_emit_seqno);
> +   vc4->last_emit_seqno,
> +   fd);
>  screen->fence_reference(screen, fence, NULL);
>  *fence = (struct pipe_fence_handle *)f;
>  }
> @@ -124,8 +148,12 @@ vc4_context_destroy(struct pipe_context *pctx)
>  
>  vc4_program_fini(pctx);
>  
> -if (vc4->screen->has_syncobj)
> +if (vc4->screen->has_syncobj) {
>  drmSyncobjDestroy(vc4->fd, vc4->job_syncobj);
> +drmSyncobjDestroy(vc4->fd, vc4->in_syncobj);
> +}
> +if (vc4->in_fence_fd >= 0)
> +close(vc4->in_fence_fd);
>  
>  ralloc_free(vc4);
>  }
> @@ -161,12 +189,19 @@ vc4_context_create(struct pipe_screen *pscreen, void 
> *priv, unsigned flags)
>  vc4_query_init(pctx);
>  vc4_resource_context_init(pctx);
>  
> +vc4_job_init(vc4);
> +vc4_fence_context_init(vc4);
> +
>  vc4->fd = screen->fd;
>  
>  err = vc4_job_init(vc4);
>  if (err)
>  goto fail;
>  
> +err = vc4_fence_context_init(vc4);
> +if (err)
> +goto fail;
> +

Two vc4_fence_context_init()?

> +static int
> +vc4_fence_get_fd(struct pipe_screen *screen, struct pipe_fence_handle 
> *pfence)
> +{
> +struct vc4_fence *fence = vc4_fence(pfence);
> +
> +return dup(fence->fd);

There was a patch series recently for other drivers saying that we
should be doing "fcntl(fd, F_DUPFD_CLOEXEC, 3);" 

[Mesa-dev] [PATCH 7/7] broadcom/vc4: Native fence fd support

[Stefan Schake]

With the syncobj support in place, lets use it to implement the
native fence fd extension. This mostly follows previous implementations
in freedreno and etnaviv.

Signed-off-by: Stefan Schake 
---
 src/gallium/drivers/vc4/vc4_context.c | 47 
 src/gallium/drivers/vc4/vc4_context.h |  5 +++
 src/gallium/drivers/vc4/vc4_fence.c   | 68 ---
 src/gallium/drivers/vc4/vc4_screen.c  |  6 ++--
 src/gallium/drivers/vc4/vc4_screen.h  |  4 +--
 5 files changed, 116 insertions(+), 14 deletions(-)

diff --git a/src/gallium/drivers/vc4/vc4_context.c 
b/src/gallium/drivers/vc4/vc4_context.c
index 0deb3ef85e..d41f1b9a26 100644
--- a/src/gallium/drivers/vc4/vc4_context.c
+++ b/src/gallium/drivers/vc4/vc4_context.c
@@ -37,30 +37,54 @@
 #include "vc4_context.h"
 #include "vc4_resource.h"
 
-void
-vc4_flush(struct pipe_context *pctx)
+static void
+vc4_flush_sync(struct pipe_context *pctx, uint32_t *in_sync)
 {
 struct vc4_context *vc4 = vc4_context(pctx);
 
 struct hash_entry *entry;
 hash_table_foreach(vc4->jobs, entry) {
 struct vc4_job *job = entry->data;
-vc4_job_submit(vc4, job);
+vc4_job_submit_sync(vc4, job, in_sync);
 }
 }
 
+void
+vc4_flush(struct pipe_context *pctx)
+{
+vc4_flush_sync(pctx, NULL);
+}
+
 static void
 vc4_pipe_flush(struct pipe_context *pctx, struct pipe_fence_handle **fence,
unsigned flags)
 {
 struct vc4_context *vc4 = vc4_context(pctx);
 
-vc4_flush(pctx);
+if (vc4->in_fence_fd >= 0) {
+/* This replaces the fence in the syncobj. */
+drmSyncobjImportSyncFile(vc4->fd, vc4->in_syncobj,
+ vc4->in_fence_fd);
+close(vc4->in_fence_fd);
+vc4->in_fence_fd = -1;
+vc4_flush_sync(pctx, >in_syncobj);
+} else {
+vc4_flush(pctx);
+}
 
 if (fence) {
 struct pipe_screen *screen = pctx->screen;
+int fd = -1;
+
+if (flags & PIPE_FLUSH_FENCE_FD) {
+/* The vc4_fence takes ownership of the returned fd. */
+drmSyncobjExportSyncFile(vc4->fd, vc4->job_syncobj,
+ );
+}
+
 struct vc4_fence *f = vc4_fence_create(vc4->screen,
-   vc4->last_emit_seqno);
+   vc4->last_emit_seqno,
+   fd);
 screen->fence_reference(screen, fence, NULL);
 *fence = (struct pipe_fence_handle *)f;
 }
@@ -124,8 +148,12 @@ vc4_context_destroy(struct pipe_context *pctx)
 
 vc4_program_fini(pctx);
 
-if (vc4->screen->has_syncobj)
+if (vc4->screen->has_syncobj) {
 drmSyncobjDestroy(vc4->fd, vc4->job_syncobj);
+drmSyncobjDestroy(vc4->fd, vc4->in_syncobj);
+}
+if (vc4->in_fence_fd >= 0)
+close(vc4->in_fence_fd);
 
 ralloc_free(vc4);
 }
@@ -161,12 +189,19 @@ vc4_context_create(struct pipe_screen *pscreen, void 
*priv, unsigned flags)
 vc4_query_init(pctx);
 vc4_resource_context_init(pctx);
 
+vc4_job_init(vc4);
+vc4_fence_context_init(vc4);
+
 vc4->fd = screen->fd;
 
 err = vc4_job_init(vc4);
 if (err)
 goto fail;
 
+err = vc4_fence_context_init(vc4);
+if (err)
+goto fail;
+
 slab_create_child(>transfer_pool, >transfer_pool);
 
vc4->uploader = u_upload_create_default(>base);
diff --git a/src/gallium/drivers/vc4/vc4_context.h 
b/src/gallium/drivers/vc4/vc4_context.h
index ac72a14008..762b82035d 100644
--- a/src/gallium/drivers/vc4/vc4_context.h
+++ b/src/gallium/drivers/vc4/vc4_context.h
@@ -411,6 +411,10 @@ struct vc4_context {
 
 /** Handle of syncobj containing the last submitted job fence. */
 uint32_t job_syncobj;
+
+int in_fence_fd;
+/** Handle of the syncobj that holds in_fence_fd for submission. */
+uint32_t in_syncobj;
 };
 
 struct vc4_rasterizer_state {
@@ -506,6 +510,7 @@ void vc4_write_uniforms(struct vc4_context *vc4,
 
 void vc4_flush(struct pipe_context *pctx);
 int vc4_job_init(struct vc4_context *vc4);
+int vc4_fence_context_init(struct vc4_context *vc4);
 struct vc4_job *vc4_get_job(struct vc4_context *vc4,
 struct pipe_surface *cbuf,
 struct pipe_surface *zsbuf);
diff --git a/src/gallium/drivers/vc4/vc4_fence.c 
b/src/gallium/drivers/vc4/vc4_fence.c
index f61e7c6a5e..018cd30d86 100644
--- a/src/gallium/drivers/vc4/vc4_fence.c
+++ b/src/gallium/drivers/vc4/vc4_fence.c
@@ -34,26 +34,38 @@
  * fired off as our fence