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

[Eric Anholt]

Stefan Schake <stschake at gmail.com> 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 <stschake at gmail.com>
> ---
>  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, &vc4->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,
> +                                                 &fd);
> +                }
> +
>                  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);" instead of
"dup(fd)". Same comment for vc4_fence_create_fd().
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180423/5d68abbc/attachment.sig>