[v3,04/17] panfrost: Use the per-batch fences to wait on the last submitted batch

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

Details

Message ID 20190918132439.27705-5-boris.brezillon@collabora.com
State Accepted
Commit 819738e4af1ab0625e17c6fa555e5e23d737c5a0
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.
We just replace the per-context out_sync object by a pointer to the
the fence of the last last submitted batch. Pipelining of batches will
come later.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
Alyssa, I dropped your R-b since the other changes you asked me to do
in "panfrost: Add a batch fence" had some impact on this patch.

Changes in v3:
* Make sure we don't try to wait on dummy batches (those with no
  vertex/tiler/fragment jobs)
---
 src/gallium/drivers/panfrost/pan_context.c |  6 ----
 src/gallium/drivers/panfrost/pan_context.h |  3 +-
 src/gallium/drivers/panfrost/pan_job.c     | 35 ++++++++++++++++++----
 src/gallium/drivers/panfrost/pan_screen.c  | 18 +++++++++--
 4 files changed, 47 insertions(+), 15 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 65a6c7f8c5ae..312a9e93e455 100644
--- a/src/gallium/drivers/panfrost/pan_context.c
+++ b/src/gallium/drivers/panfrost/pan_context.c
@@ -2702,12 +2702,6 @@  panfrost_create_context(struct pipe_screen *screen, void *priv, unsigned flags)
         panfrost_blend_context_init(gallium);
         panfrost_compute_context_init(gallium);
 
-        ASSERTED int ret;
-
-        ret = drmSyncobjCreate(pscreen->fd, DRM_SYNCOBJ_CREATE_SIGNALED,
-                               &ctx->out_sync);
-        assert(!ret);
-
         /* XXX: leaks */
         gallium->stream_uploader = u_upload_create_default(gallium);
         gallium->const_uploader = gallium->stream_uploader;
diff --git a/src/gallium/drivers/panfrost/pan_context.h b/src/gallium/drivers/panfrost/pan_context.h
index c145d589757e..ce3e0c899a4f 100644
--- a/src/gallium/drivers/panfrost/pan_context.h
+++ b/src/gallium/drivers/panfrost/pan_context.h
@@ -191,7 +191,8 @@  struct panfrost_context {
         /* True for t6XX, false for t8xx. */
         bool is_t6xx;
 
-        uint32_t out_sync;
+        /* 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 b6763da66a97..55780dd3d9d6 100644
--- a/src/gallium/drivers/panfrost/pan_job.c
+++ b/src/gallium/drivers/panfrost/pan_job.c
@@ -425,11 +425,13 @@  panfrost_batch_submit_ioctl(struct panfrost_batch *batch,
         uint32_t *bo_handles;
         int ret;
 
-        submit.in_syncs = (u64) (uintptr_t) &ctx->out_sync;
-        submit.in_sync_count = 1;
 
-        submit.out_sync = ctx->out_sync;
+        if (ctx->last_out_sync) {
+                submit.in_sync_count = 1;
+                submit.in_syncs = (uintptr_t)&ctx->last_out_sync->syncobj;
+        }
 
+        submit.out_sync = batch->out_sync->syncobj;
         submit.jc = first_job_desc;
         submit.requirements = reqs;
 
@@ -445,6 +447,14 @@  panfrost_batch_submit_ioctl(struct panfrost_batch *batch,
         submit.bo_handles = (u64) (uintptr_t) bo_handles;
         ret = drmIoctl(screen->fd, DRM_IOCTL_PANFROST_SUBMIT, &submit);
         free(bo_handles);
+
+        /* 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;
@@ -453,7 +463,8 @@  panfrost_batch_submit_ioctl(struct panfrost_batch *batch,
         /* Trace the job if we're doing that */
         if (pan_debug & PAN_DBG_TRACE) {
                 /* Wait so we can get errors reported back */
-                drmSyncobjWait(screen->fd, &ctx->out_sync, 1, INT64_MAX, 0, NULL);
+                drmSyncobjWait(screen->fd, &batch->out_sync->syncobj, 1,
+                               INT64_MAX, 0, NULL);
                 pandecode_jc(submit.jc, FALSE);
         }
 
@@ -495,6 +506,15 @@  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;
         }
 
@@ -527,8 +547,11 @@  out:
          * rendering is quite broken right now (to be fixed by the panfrost_job
          * refactor, just take the perf hit for correctness)
          */
-        drmSyncobjWait(pan_screen(ctx->base.screen)->fd, &ctx->out_sync, 1,
-                       INT64_MAX, 0, NULL);
+        if (!batch->out_sync->signaled)
+                drmSyncobjWait(pan_screen(ctx->base.screen)->fd,
+                               &batch->out_sync->syncobj, 1, INT64_MAX, 0,
+                               NULL);
+
         panfrost_free_batch(batch);
 }
 
diff --git a/src/gallium/drivers/panfrost/pan_screen.c b/src/gallium/drivers/panfrost/pan_screen.c
index dae8b941f1ea..e2c31f7f8213 100644
--- a/src/gallium/drivers/panfrost/pan_screen.c
+++ b/src/gallium/drivers/panfrost/pan_screen.c
@@ -590,8 +590,12 @@  panfrost_fence_finish(struct pipe_screen *pscreen,
         struct panfrost_screen *screen = pan_screen(pscreen);
         struct panfrost_fence *f = (struct panfrost_fence *)fence;
         int ret;
-
         unsigned syncobj;
+
+        /* The fence was already signaled */
+        if (f->fd == -1)
+                return true;
+
         ret = drmSyncobjCreate(screen->fd, 0, &syncobj);
         if (ret) {
                 fprintf(stderr, "Failed to create syncobj to wait on: %m\n");
@@ -623,18 +627,28 @@  panfrost_fence_create(struct panfrost_context *ctx)
         if (!f)
                 return NULL;
 
+        f->fd = -1;
+
+        /* 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;
+
         /* 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->out_sync, &f->fd);
+        drmSyncobjExportSyncFile(screen->fd, ctx->last_out_sync->syncobj, &f->fd);
         if (f->fd == -1) {
                 fprintf(stderr, "export failed: %m\n");
                 free(f);
                 return NULL;
         }
 
+out:
         pipe_reference_init(&f->reference, 1);
 
         return f;

Comments

R-b
On Wed, Sep 18, 2019 at 03:24:26PM +0200, Boris Brezillon wrote:
> We just replace the per-context out_sync object by a pointer to the
> the fence of the last last submitted batch. Pipelining of batches will
> come later.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
> Alyssa, I dropped your R-b since the other changes you asked me to do
> in "panfrost: Add a batch fence" had some impact on this patch.
> 
> Changes in v3:
> * Make sure we don't try to wait on dummy batches (those with no
>   vertex/tiler/fragment jobs)
> ---
>  src/gallium/drivers/panfrost/pan_context.c |  6 ----
>  src/gallium/drivers/panfrost/pan_context.h |  3 +-
>  src/gallium/drivers/panfrost/pan_job.c     | 35 ++++++++++++++++++----
>  src/gallium/drivers/panfrost/pan_screen.c  | 18 +++++++++--
>  4 files changed, 47 insertions(+), 15 deletions(-)
> 
> diff --git a/src/gallium/drivers/panfrost/pan_context.c b/src/gallium/drivers/panfrost/pan_context.c
> index 65a6c7f8c5ae..312a9e93e455 100644
> --- a/src/gallium/drivers/panfrost/pan_context.c
> +++ b/src/gallium/drivers/panfrost/pan_context.c
> @@ -2702,12 +2702,6 @@ panfrost_create_context(struct pipe_screen *screen, void *priv, unsigned flags)
>          panfrost_blend_context_init(gallium);
>          panfrost_compute_context_init(gallium);
>  
> -        ASSERTED int ret;
> -
> -        ret = drmSyncobjCreate(pscreen->fd, DRM_SYNCOBJ_CREATE_SIGNALED,
> -                               &ctx->out_sync);
> -        assert(!ret);
> -
>          /* XXX: leaks */
>          gallium->stream_uploader = u_upload_create_default(gallium);
>          gallium->const_uploader = gallium->stream_uploader;
> diff --git a/src/gallium/drivers/panfrost/pan_context.h b/src/gallium/drivers/panfrost/pan_context.h
> index c145d589757e..ce3e0c899a4f 100644
> --- a/src/gallium/drivers/panfrost/pan_context.h
> +++ b/src/gallium/drivers/panfrost/pan_context.h
> @@ -191,7 +191,8 @@ struct panfrost_context {
>          /* True for t6XX, false for t8xx. */
>          bool is_t6xx;
>  
> -        uint32_t out_sync;
> +        /* 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 b6763da66a97..55780dd3d9d6 100644
> --- a/src/gallium/drivers/panfrost/pan_job.c
> +++ b/src/gallium/drivers/panfrost/pan_job.c
> @@ -425,11 +425,13 @@ panfrost_batch_submit_ioctl(struct panfrost_batch *batch,
>          uint32_t *bo_handles;
>          int ret;
>  
> -        submit.in_syncs = (u64) (uintptr_t) &ctx->out_sync;
> -        submit.in_sync_count = 1;
>  
> -        submit.out_sync = ctx->out_sync;
> +        if (ctx->last_out_sync) {
> +                submit.in_sync_count = 1;
> +                submit.in_syncs = (uintptr_t)&ctx->last_out_sync->syncobj;
> +        }
>  
> +        submit.out_sync = batch->out_sync->syncobj;
>          submit.jc = first_job_desc;
>          submit.requirements = reqs;
>  
> @@ -445,6 +447,14 @@ panfrost_batch_submit_ioctl(struct panfrost_batch *batch,
>          submit.bo_handles = (u64) (uintptr_t) bo_handles;
>          ret = drmIoctl(screen->fd, DRM_IOCTL_PANFROST_SUBMIT, &submit);
>          free(bo_handles);
> +
> +        /* 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;
> @@ -453,7 +463,8 @@ panfrost_batch_submit_ioctl(struct panfrost_batch *batch,
>          /* Trace the job if we're doing that */
>          if (pan_debug & PAN_DBG_TRACE) {
>                  /* Wait so we can get errors reported back */
> -                drmSyncobjWait(screen->fd, &ctx->out_sync, 1, INT64_MAX, 0, NULL);
> +                drmSyncobjWait(screen->fd, &batch->out_sync->syncobj, 1,
> +                               INT64_MAX, 0, NULL);
>                  pandecode_jc(submit.jc, FALSE);
>          }
>  
> @@ -495,6 +506,15 @@ 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;
>          }
>  
> @@ -527,8 +547,11 @@ out:
>           * rendering is quite broken right now (to be fixed by the panfrost_job
>           * refactor, just take the perf hit for correctness)
>           */
> -        drmSyncobjWait(pan_screen(ctx->base.screen)->fd, &ctx->out_sync, 1,
> -                       INT64_MAX, 0, NULL);
> +        if (!batch->out_sync->signaled)
> +                drmSyncobjWait(pan_screen(ctx->base.screen)->fd,
> +                               &batch->out_sync->syncobj, 1, INT64_MAX, 0,
> +                               NULL);
> +
>          panfrost_free_batch(batch);
>  }
>  
> diff --git a/src/gallium/drivers/panfrost/pan_screen.c b/src/gallium/drivers/panfrost/pan_screen.c
> index dae8b941f1ea..e2c31f7f8213 100644
> --- a/src/gallium/drivers/panfrost/pan_screen.c
> +++ b/src/gallium/drivers/panfrost/pan_screen.c
> @@ -590,8 +590,12 @@ panfrost_fence_finish(struct pipe_screen *pscreen,
>          struct panfrost_screen *screen = pan_screen(pscreen);
>          struct panfrost_fence *f = (struct panfrost_fence *)fence;
>          int ret;
> -
>          unsigned syncobj;
> +
> +        /* The fence was already signaled */
> +        if (f->fd == -1)
> +                return true;
> +
>          ret = drmSyncobjCreate(screen->fd, 0, &syncobj);
>          if (ret) {
>                  fprintf(stderr, "Failed to create syncobj to wait on: %m\n");
> @@ -623,18 +627,28 @@ panfrost_fence_create(struct panfrost_context *ctx)
>          if (!f)
>                  return NULL;
>  
> +        f->fd = -1;
> +
> +        /* 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;
> +
>          /* 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->out_sync, &f->fd);
> +        drmSyncobjExportSyncFile(screen->fd, ctx->last_out_sync->syncobj, &f->fd);
>          if (f->fd == -1) {
>                  fprintf(stderr, "export failed: %m\n");
>                  free(f);
>                  return NULL;
>          }
>  
> +out:
>          pipe_reference_init(&f->reference, 1);
>  
>          return f;
> -- 
> 2.21.0