[v3,07/17] panfrost: Prepare panfrost_fence for batch pipelining

Submitted by Boris Brezillon on Sept. 18, 2019, 1:24 p.m.

Details

Message ID 20190918132439.27705-8-boris.brezillon@collabora.com
State New
Headers show
Series "panfrost: Support batch pipelining" ( rev: 2 ) in Mesa

Not browsing as part of any series.

Commit Message

Boris Brezillon Sept. 18, 2019, 1:24 p.m.
The panfrost_fence logic currently waits on the last submitted batch,
but the batch serialization that was enforced in
panfrost_batch_submit() is about to go away, allowing for several
batches to be pipelined, and the last submitted one is not necessarily
the one that will finish last.

We need to make sure the fence logic waits on all flushed batches, not
only the last one.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
---
Changes in v3:
* Fix a comment
* Adjust things to match the changes done in "panfrost: Add a batch fence"
---
 src/gallium/drivers/panfrost/pan_context.c | 18 +++++-
 src/gallium/drivers/panfrost/pan_context.h |  5 +-
 src/gallium/drivers/panfrost/pan_job.c     | 16 -----
 src/gallium/drivers/panfrost/pan_screen.c  | 71 +++++++++++-----------
 src/gallium/drivers/panfrost/pan_screen.h  |  3 +-
 5 files changed, 57 insertions(+), 56 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/gallium/drivers/panfrost/pan_context.c b/src/gallium/drivers/panfrost/pan_context.c
index 312a9e93e455..aad69e3f9991 100644
--- a/src/gallium/drivers/panfrost/pan_context.c
+++ b/src/gallium/drivers/panfrost/pan_context.c
@@ -1349,14 +1349,30 @@  panfrost_flush(
 {
         struct panfrost_context *ctx = pan_context(pipe);
         struct panfrost_batch *batch = panfrost_get_batch_for_fbo(ctx);
+        struct util_dynarray fences;
+
+        /* We must collect the fences before the flush is done, otherwise we'll
+         * lose track of them.
+         */
+        if (fence) {
+                util_dynarray_init(&fences, NULL);
+                panfrost_batch_fence_reference(batch->out_sync);
+                util_dynarray_append(&fences, struct panfrost_batch_fence *,
+                                     batch->out_sync);
+        }
 
         /* Submit the frame itself */
         panfrost_batch_submit(batch);
 
         if (fence) {
-                struct panfrost_fence *f = panfrost_fence_create(ctx);
+                struct panfrost_fence *f = panfrost_fence_create(ctx, &fences);
                 pipe->screen->fence_reference(pipe->screen, fence, NULL);
                 *fence = (struct pipe_fence_handle *)f;
+
+                util_dynarray_foreach(&fences, struct panfrost_batch_fence *, fence)
+                        panfrost_batch_fence_unreference(*fence);
+
+                util_dynarray_fini(&fences);
         }
 }
 
diff --git a/src/gallium/drivers/panfrost/pan_context.h b/src/gallium/drivers/panfrost/pan_context.h
index 3b09952345cf..d50ed57d5d8a 100644
--- a/src/gallium/drivers/panfrost/pan_context.h
+++ b/src/gallium/drivers/panfrost/pan_context.h
@@ -94,7 +94,7 @@  struct panfrost_query {
 
 struct panfrost_fence {
         struct pipe_reference reference;
-        int fd;
+        struct util_dynarray syncfds;
 };
 
 struct panfrost_streamout {
@@ -193,9 +193,6 @@  struct panfrost_context {
 
         /* True for t6XX, false for t8xx. */
         bool is_t6xx;
-
-        /* The out sync fence of the last submitted batch. */
-        struct panfrost_batch_fence *last_out_sync;
 };
 
 /* Corresponds to the CSO */
diff --git a/src/gallium/drivers/panfrost/pan_job.c b/src/gallium/drivers/panfrost/pan_job.c
index b0494af3482f..211e48bafd4e 100644
--- a/src/gallium/drivers/panfrost/pan_job.c
+++ b/src/gallium/drivers/panfrost/pan_job.c
@@ -819,13 +819,6 @@  panfrost_batch_submit_ioctl(struct panfrost_batch *batch,
         free(bo_handles);
         free(in_syncs);
 
-        /* Release the last batch fence if any, and retain the new one */
-        if (ctx->last_out_sync)
-                panfrost_batch_fence_unreference(ctx->last_out_sync);
-
-        panfrost_batch_fence_reference(batch->out_sync);
-        ctx->last_out_sync = batch->out_sync;
-
         if (ret) {
                 fprintf(stderr, "Error submitting: %m\n");
                 return errno;
@@ -884,15 +877,6 @@  panfrost_batch_submit(struct panfrost_batch *batch)
                  * to wait on it.
                  */
                 batch->out_sync->signaled = true;
-
-                /* Release the last batch fence if any, and set ->last_out_sync
-                 * to NULL
-                 */
-                if (ctx->last_out_sync) {
-                        panfrost_batch_fence_unreference(ctx->last_out_sync);
-                        ctx->last_out_sync = NULL;
-                }
-
                 goto out;
         }
 
diff --git a/src/gallium/drivers/panfrost/pan_screen.c b/src/gallium/drivers/panfrost/pan_screen.c
index e2c31f7f8213..55c66e0c9a79 100644
--- a/src/gallium/drivers/panfrost/pan_screen.c
+++ b/src/gallium/drivers/panfrost/pan_screen.c
@@ -575,7 +575,9 @@  panfrost_fence_reference(struct pipe_screen *pscreen,
         struct panfrost_fence *old = *p;
 
         if (pipe_reference(&(*p)->reference, &f->reference)) {
-                close(old->fd);
+                util_dynarray_foreach(&old->syncfds, int, fd)
+                        close(*fd);
+                util_dynarray_fini(&old->syncfds);
                 free(old);
         }
         *p = f;
@@ -589,66 +591,67 @@  panfrost_fence_finish(struct pipe_screen *pscreen,
 {
         struct panfrost_screen *screen = pan_screen(pscreen);
         struct panfrost_fence *f = (struct panfrost_fence *)fence;
+        struct util_dynarray syncobjs;
         int ret;
-        unsigned syncobj;
 
-        /* The fence was already signaled */
-        if (f->fd == -1)
+        /* All fences were already signaled */
+        if (!util_dynarray_num_elements(&f->syncfds, int))
                 return true;
 
-        ret = drmSyncobjCreate(screen->fd, 0, &syncobj);
-        if (ret) {
-                fprintf(stderr, "Failed to create syncobj to wait on: %m\n");
-                return false;
-        }
+        util_dynarray_init(&syncobjs, NULL);
+        util_dynarray_foreach(&f->syncfds, int, fd) {
+                uint32_t syncobj;
 
-        ret = drmSyncobjImportSyncFile(screen->fd, syncobj, f->fd);
-        if (ret) {
-                fprintf(stderr, "Failed to import fence to syncobj: %m\n");
-                return false;
+                ret = drmSyncobjCreate(screen->fd, 0, &syncobj);
+                assert(!ret);
+
+                ret = drmSyncobjImportSyncFile(screen->fd, syncobj, *fd);
+                assert(!ret);
+                util_dynarray_append(&syncobjs, uint32_t, syncobj);
         }
 
         uint64_t abs_timeout = os_time_get_absolute_timeout(timeout);
         if (abs_timeout == OS_TIMEOUT_INFINITE)
                 abs_timeout = INT64_MAX;
 
-        ret = drmSyncobjWait(screen->fd, &syncobj, 1, abs_timeout, 0, NULL);
+        ret = drmSyncobjWait(screen->fd, util_dynarray_begin(&syncobjs),
+                             util_dynarray_num_elements(&syncobjs, uint32_t),
+                             abs_timeout, DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL,
+                             NULL);
 
-        drmSyncobjDestroy(screen->fd, syncobj);
+        util_dynarray_foreach(&syncobjs, uint32_t, syncobj)
+                drmSyncobjDestroy(screen->fd, *syncobj);
 
         return ret >= 0;
 }
 
 struct panfrost_fence *
-panfrost_fence_create(struct panfrost_context *ctx)
+panfrost_fence_create(struct panfrost_context *ctx,
+                      struct util_dynarray *fences)
 {
         struct panfrost_screen *screen = pan_screen(ctx->base.screen);
         struct panfrost_fence *f = calloc(1, sizeof(*f));
         if (!f)
                 return NULL;
 
-        f->fd = -1;
+        util_dynarray_init(&f->syncfds, NULL);
 
-        /* There was no job flushed yet or the batch fence was already
-         * signaled, let's return a dummy fence object that returns true
-         * directly when ->fence_finish() is called.
-         */
-        if (!ctx->last_out_sync || ctx->last_out_sync->signaled)
-                goto out;
+        /* Export fences from all pending batches. */
+        util_dynarray_foreach(fences, struct panfrost_batch_fence *, fence) {
+                int fd = -1;
 
-        /* Snapshot the last Panfrost's rendering's out fence.  We'd rather have
-         * another syncobj instead of a sync file, but this is all we get.
-         * (HandleToFD/FDToHandle just gives you another syncobj ID for the
-         * same syncobj).
-         */
-        drmSyncobjExportSyncFile(screen->fd, ctx->last_out_sync->syncobj, &f->fd);
-        if (f->fd == -1) {
-                fprintf(stderr, "export failed: %m\n");
-                free(f);
-                return NULL;
+                /* The fence is already signaled, no need to export it. */
+                if ((*fence)->signaled)
+                        continue;
+
+                drmSyncobjExportSyncFile(screen->fd, (*fence)->syncobj, &fd);
+                if (fd == -1)
+                        fprintf(stderr, "export failed: %m\n");
+
+                assert(fd != -1);
+                util_dynarray_append(&f->syncfds, int, fd);
         }
 
-out:
         pipe_reference_init(&f->reference, 1);
 
         return f;
diff --git a/src/gallium/drivers/panfrost/pan_screen.h b/src/gallium/drivers/panfrost/pan_screen.h
index fdc47df00ea1..f09db8011c6a 100644
--- a/src/gallium/drivers/panfrost/pan_screen.h
+++ b/src/gallium/drivers/panfrost/pan_screen.h
@@ -101,6 +101,7 @@  pan_screen(struct pipe_screen *p)
 }
 
 struct panfrost_fence *
-panfrost_fence_create(struct panfrost_context *ctx);
+panfrost_fence_create(struct panfrost_context *ctx,
+                      struct util_dynarray *fences);
 
 #endif /* PAN_SCREEN_H */

Comments

(Still r-b)

On Wed, Sep 18, 2019 at 03:24:29PM +0200, Boris Brezillon wrote:
> The panfrost_fence logic currently waits on the last submitted batch,
> but the batch serialization that was enforced in
> panfrost_batch_submit() is about to go away, allowing for several
> batches to be pipelined, and the last submitted one is not necessarily
> the one that will finish last.
> 
> We need to make sure the fence logic waits on all flushed batches, not
> only the last one.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> Reviewed-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> ---
> Changes in v3:
> * Fix a comment
> * Adjust things to match the changes done in "panfrost: Add a batch fence"
> ---
>  src/gallium/drivers/panfrost/pan_context.c | 18 +++++-
>  src/gallium/drivers/panfrost/pan_context.h |  5 +-
>  src/gallium/drivers/panfrost/pan_job.c     | 16 -----
>  src/gallium/drivers/panfrost/pan_screen.c  | 71 +++++++++++-----------
>  src/gallium/drivers/panfrost/pan_screen.h  |  3 +-
>  5 files changed, 57 insertions(+), 56 deletions(-)
> 
> diff --git a/src/gallium/drivers/panfrost/pan_context.c b/src/gallium/drivers/panfrost/pan_context.c
> index 312a9e93e455..aad69e3f9991 100644
> --- a/src/gallium/drivers/panfrost/pan_context.c
> +++ b/src/gallium/drivers/panfrost/pan_context.c
> @@ -1349,14 +1349,30 @@ panfrost_flush(
>  {
>          struct panfrost_context *ctx = pan_context(pipe);
>          struct panfrost_batch *batch = panfrost_get_batch_for_fbo(ctx);
> +        struct util_dynarray fences;
> +
> +        /* We must collect the fences before the flush is done, otherwise we'll
> +         * lose track of them.
> +         */
> +        if (fence) {
> +                util_dynarray_init(&fences, NULL);
> +                panfrost_batch_fence_reference(batch->out_sync);
> +                util_dynarray_append(&fences, struct panfrost_batch_fence *,
> +                                     batch->out_sync);
> +        }
>  
>          /* Submit the frame itself */
>          panfrost_batch_submit(batch);
>  
>          if (fence) {
> -                struct panfrost_fence *f = panfrost_fence_create(ctx);
> +                struct panfrost_fence *f = panfrost_fence_create(ctx, &fences);
>                  pipe->screen->fence_reference(pipe->screen, fence, NULL);
>                  *fence = (struct pipe_fence_handle *)f;
> +
> +                util_dynarray_foreach(&fences, struct panfrost_batch_fence *, fence)
> +                        panfrost_batch_fence_unreference(*fence);
> +
> +                util_dynarray_fini(&fences);
>          }
>  }
>  
> diff --git a/src/gallium/drivers/panfrost/pan_context.h b/src/gallium/drivers/panfrost/pan_context.h
> index 3b09952345cf..d50ed57d5d8a 100644
> --- a/src/gallium/drivers/panfrost/pan_context.h
> +++ b/src/gallium/drivers/panfrost/pan_context.h
> @@ -94,7 +94,7 @@ struct panfrost_query {
>  
>  struct panfrost_fence {
>          struct pipe_reference reference;
> -        int fd;
> +        struct util_dynarray syncfds;
>  };
>  
>  struct panfrost_streamout {
> @@ -193,9 +193,6 @@ struct panfrost_context {
>  
>          /* True for t6XX, false for t8xx. */
>          bool is_t6xx;
> -
> -        /* The out sync fence of the last submitted batch. */
> -        struct panfrost_batch_fence *last_out_sync;
>  };
>  
>  /* Corresponds to the CSO */
> diff --git a/src/gallium/drivers/panfrost/pan_job.c b/src/gallium/drivers/panfrost/pan_job.c
> index b0494af3482f..211e48bafd4e 100644
> --- a/src/gallium/drivers/panfrost/pan_job.c
> +++ b/src/gallium/drivers/panfrost/pan_job.c
> @@ -819,13 +819,6 @@ panfrost_batch_submit_ioctl(struct panfrost_batch *batch,
>          free(bo_handles);
>          free(in_syncs);
>  
> -        /* Release the last batch fence if any, and retain the new one */
> -        if (ctx->last_out_sync)
> -                panfrost_batch_fence_unreference(ctx->last_out_sync);
> -
> -        panfrost_batch_fence_reference(batch->out_sync);
> -        ctx->last_out_sync = batch->out_sync;
> -
>          if (ret) {
>                  fprintf(stderr, "Error submitting: %m\n");
>                  return errno;
> @@ -884,15 +877,6 @@ panfrost_batch_submit(struct panfrost_batch *batch)
>                   * to wait on it.
>                   */
>                  batch->out_sync->signaled = true;
> -
> -                /* Release the last batch fence if any, and set ->last_out_sync
> -                 * to NULL
> -                 */
> -                if (ctx->last_out_sync) {
> -                        panfrost_batch_fence_unreference(ctx->last_out_sync);
> -                        ctx->last_out_sync = NULL;
> -                }
> -
>                  goto out;
>          }
>  
> diff --git a/src/gallium/drivers/panfrost/pan_screen.c b/src/gallium/drivers/panfrost/pan_screen.c
> index e2c31f7f8213..55c66e0c9a79 100644
> --- a/src/gallium/drivers/panfrost/pan_screen.c
> +++ b/src/gallium/drivers/panfrost/pan_screen.c
> @@ -575,7 +575,9 @@ panfrost_fence_reference(struct pipe_screen *pscreen,
>          struct panfrost_fence *old = *p;
>  
>          if (pipe_reference(&(*p)->reference, &f->reference)) {
> -                close(old->fd);
> +                util_dynarray_foreach(&old->syncfds, int, fd)
> +                        close(*fd);
> +                util_dynarray_fini(&old->syncfds);
>                  free(old);
>          }
>          *p = f;
> @@ -589,66 +591,67 @@ panfrost_fence_finish(struct pipe_screen *pscreen,
>  {
>          struct panfrost_screen *screen = pan_screen(pscreen);
>          struct panfrost_fence *f = (struct panfrost_fence *)fence;
> +        struct util_dynarray syncobjs;
>          int ret;
> -        unsigned syncobj;
>  
> -        /* The fence was already signaled */
> -        if (f->fd == -1)
> +        /* All fences were already signaled */
> +        if (!util_dynarray_num_elements(&f->syncfds, int))
>                  return true;
>  
> -        ret = drmSyncobjCreate(screen->fd, 0, &syncobj);
> -        if (ret) {
> -                fprintf(stderr, "Failed to create syncobj to wait on: %m\n");
> -                return false;
> -        }
> +        util_dynarray_init(&syncobjs, NULL);
> +        util_dynarray_foreach(&f->syncfds, int, fd) {
> +                uint32_t syncobj;
>  
> -        ret = drmSyncobjImportSyncFile(screen->fd, syncobj, f->fd);
> -        if (ret) {
> -                fprintf(stderr, "Failed to import fence to syncobj: %m\n");
> -                return false;
> +                ret = drmSyncobjCreate(screen->fd, 0, &syncobj);
> +                assert(!ret);
> +
> +                ret = drmSyncobjImportSyncFile(screen->fd, syncobj, *fd);
> +                assert(!ret);
> +                util_dynarray_append(&syncobjs, uint32_t, syncobj);
>          }
>  
>          uint64_t abs_timeout = os_time_get_absolute_timeout(timeout);
>          if (abs_timeout == OS_TIMEOUT_INFINITE)
>                  abs_timeout = INT64_MAX;
>  
> -        ret = drmSyncobjWait(screen->fd, &syncobj, 1, abs_timeout, 0, NULL);
> +        ret = drmSyncobjWait(screen->fd, util_dynarray_begin(&syncobjs),
> +                             util_dynarray_num_elements(&syncobjs, uint32_t),
> +                             abs_timeout, DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL,
> +                             NULL);
>  
> -        drmSyncobjDestroy(screen->fd, syncobj);
> +        util_dynarray_foreach(&syncobjs, uint32_t, syncobj)
> +                drmSyncobjDestroy(screen->fd, *syncobj);
>  
>          return ret >= 0;
>  }
>  
>  struct panfrost_fence *
> -panfrost_fence_create(struct panfrost_context *ctx)
> +panfrost_fence_create(struct panfrost_context *ctx,
> +                      struct util_dynarray *fences)
>  {
>          struct panfrost_screen *screen = pan_screen(ctx->base.screen);
>          struct panfrost_fence *f = calloc(1, sizeof(*f));
>          if (!f)
>                  return NULL;
>  
> -        f->fd = -1;
> +        util_dynarray_init(&f->syncfds, NULL);
>  
> -        /* There was no job flushed yet or the batch fence was already
> -         * signaled, let's return a dummy fence object that returns true
> -         * directly when ->fence_finish() is called.
> -         */
> -        if (!ctx->last_out_sync || ctx->last_out_sync->signaled)
> -                goto out;
> +        /* Export fences from all pending batches. */
> +        util_dynarray_foreach(fences, struct panfrost_batch_fence *, fence) {
> +                int fd = -1;
>  
> -        /* Snapshot the last Panfrost's rendering's out fence.  We'd rather have
> -         * another syncobj instead of a sync file, but this is all we get.
> -         * (HandleToFD/FDToHandle just gives you another syncobj ID for the
> -         * same syncobj).
> -         */
> -        drmSyncobjExportSyncFile(screen->fd, ctx->last_out_sync->syncobj, &f->fd);
> -        if (f->fd == -1) {
> -                fprintf(stderr, "export failed: %m\n");
> -                free(f);
> -                return NULL;
> +                /* The fence is already signaled, no need to export it. */
> +                if ((*fence)->signaled)
> +                        continue;
> +
> +                drmSyncobjExportSyncFile(screen->fd, (*fence)->syncobj, &fd);
> +                if (fd == -1)
> +                        fprintf(stderr, "export failed: %m\n");
> +
> +                assert(fd != -1);
> +                util_dynarray_append(&f->syncfds, int, fd);
>          }
>  
> -out:
>          pipe_reference_init(&f->reference, 1);
>  
>          return f;
> diff --git a/src/gallium/drivers/panfrost/pan_screen.h b/src/gallium/drivers/panfrost/pan_screen.h
> index fdc47df00ea1..f09db8011c6a 100644
> --- a/src/gallium/drivers/panfrost/pan_screen.h
> +++ b/src/gallium/drivers/panfrost/pan_screen.h
> @@ -101,6 +101,7 @@ pan_screen(struct pipe_screen *p)
>  }
>  
>  struct panfrost_fence *
> -panfrost_fence_create(struct panfrost_context *ctx);
> +panfrost_fence_create(struct panfrost_context *ctx,
> +                      struct util_dynarray *fences);
>  
>  #endif /* PAN_SCREEN_H */
> -- 
> 2.21.0