[v2,25/37] panfrost: Add a batch fence

Submitted by Boris Brezillon on Sept. 16, 2019, 9:37 a.m.

Details

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

Not browsing as part of any series.

Commit Message

Boris Brezillon Sept. 16, 2019, 9:37 a.m.
So we can implement fine-grained dependency tracking between batches.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 src/gallium/drivers/panfrost/pan_job.c | 51 ++++++++++++++++++++++++++
 src/gallium/drivers/panfrost/pan_job.h | 39 ++++++++++++++++++++
 2 files changed, 90 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/gallium/drivers/panfrost/pan_job.c b/src/gallium/drivers/panfrost/pan_job.c
index 30720ab98bb9..8712e2ce598a 100644
--- a/src/gallium/drivers/panfrost/pan_job.c
+++ b/src/gallium/drivers/panfrost/pan_job.c
@@ -36,6 +36,47 @@ 
 #include "pan_util.h"
 #include "pandecode/decode.h"
 
+static struct panfrost_batch_fence *
+panfrost_create_batch_fence(struct panfrost_batch *batch)
+{
+        struct panfrost_batch_fence *fence;
+
+        fence = rzalloc(NULL, struct panfrost_batch_fence);
+        assert(fence);
+        pipe_reference_init(&fence->reference, 1);
+        fence->ctx = batch->ctx;
+        fence->batch = batch;
+
+        /* Start in a signaled state so that even non-submitted batches
+         * (those that have no draw/clear) can be waited upon.
+         */
+        drmSyncobjCreate(pan_screen(batch->ctx->base.screen)->fd,
+                         DRM_SYNCOBJ_CREATE_SIGNALED, &fence->syncobj);
+
+        return fence;
+}
+
+static void
+panfrost_free_batch_fence(struct panfrost_batch_fence *fence)
+{
+        drmSyncobjDestroy(pan_screen(fence->ctx->base.screen)->fd,
+                          fence->syncobj);
+        ralloc_free(fence);
+}
+
+void
+panfrost_batch_fence_unreference(struct panfrost_batch_fence *fence)
+{
+        if (pipe_reference(&fence->reference, NULL))
+                 panfrost_free_batch_fence(fence);
+}
+
+void
+panfrost_batch_fence_reference(struct panfrost_batch_fence *fence)
+{
+        pipe_reference(NULL, &fence->reference);
+}
+
 static struct panfrost_batch *
 panfrost_create_batch(struct panfrost_context *ctx,
                       const struct pipe_framebuffer_state *key)
@@ -53,6 +94,7 @@  panfrost_create_batch(struct panfrost_context *ctx,
 
         util_dynarray_init(&batch->headers, batch);
         util_dynarray_init(&batch->gpu_headers, batch);
+        batch->out_sync = panfrost_create_batch_fence(batch);
         util_copy_framebuffer_state(&batch->key, key);
 
         return batch;
@@ -74,6 +116,15 @@  panfrost_free_batch(struct panfrost_batch *batch)
         if (ctx->batch == batch)
                 ctx->batch = NULL;
 
+        /* The out_sync fence lifetime is different from the the batch one
+         * since other batches might want to wait on an fence of already
+         * submitted/signaled batch. All we need to do here is make sure the
+         * fence does not point to an invalid batch, which the core will
+         * interpret as 'batch is already submitted'.
+         */
+        batch->out_sync->batch = NULL;
+        panfrost_batch_fence_unreference(batch->out_sync);
+
         util_unreference_framebuffer_state(&batch->key);
         ralloc_free(batch);
 }
diff --git a/src/gallium/drivers/panfrost/pan_job.h b/src/gallium/drivers/panfrost/pan_job.h
index 3f2cf1a999f3..32bfc1fe3388 100644
--- a/src/gallium/drivers/panfrost/pan_job.h
+++ b/src/gallium/drivers/panfrost/pan_job.h
@@ -31,6 +31,36 @@ 
 #include "pan_allocate.h"
 #include "pan_resource.h"
 
+/* panfrost_batch_fence is the out fence of batch that users or other batches
+ * might want to wait on. The batch fence lifetime is different from the batch
+ * one as want will certainly want to wait upon the fence after the batch has
+ * been submitted (which is when panfrost_batch objects are freed).
+ */
+struct panfrost_batch_fence {
+        /* Refcounting object for the fence. */
+        struct pipe_reference reference;
+
+        /* Batch that created this fence object. Will become NULL at batch
+         * submission time. This field is mainly here to know whether the
+         * batch has been flushed or not.
+         */
+        struct panfrost_batch *batch;
+
+        /* Context this fence is attached to. We need both ctx and batch, as
+         * the batch will go away after it's been submitted, but the fence
+         * will stay a bit longer.
+         */
+        struct panfrost_context *ctx;
+
+        /* Sync object backing this fence. */
+        uint32_t syncobj;
+
+        /* Cached value of the signaled state to avoid calling WAIC_SYNCOBJs
+         * when we know the fence has already been signaled.
+         */
+        bool signaled;
+};
+
 #define PAN_REQ_MSAA            (1 << 0)
 #define PAN_REQ_DEPTH_WRITE     (1 << 1)
 
@@ -120,10 +150,19 @@  struct panfrost_batch {
 
         /* Framebuffer descriptor. */
         mali_ptr framebuffer;
+
+        /* Output sync object. Only valid when submitted is true. */
+        struct panfrost_batch_fence *out_sync;
 };
 
 /* Functions for managing the above */
 
+void
+panfrost_batch_fence_unreference(struct panfrost_batch_fence *fence);
+
+void
+panfrost_batch_fence_reference(struct panfrost_batch_fence *batch);
+
 struct panfrost_batch *
 panfrost_get_batch_for_fbo(struct panfrost_context *ctx);
 

Comments

> +        /* Start in a signaled state so that even non-submitted batches
> +         * (those that have no draw/clear) can be waited upon.
> +         */

When would this happen? Presumably if a batch does nothing whatsoever,
it doesn't make sense to wait on it.
>  #include "pan_resource.h"
>  
> +        /* Batch that created this fence object. Will become NULL at batch
> +         * submission time. This field is mainly here to know whether the
> +         * batch has been flushed or not.
> +         */
> +        struct panfrost_batch *batch;

Could this be replaced by just a `bool flushed`, or do we actually use
the value in a later patch?
Also, some typos:

> +         * since other batches might want to wait on an fence of already

a fence.

> +/* panfrost_batch_fence is the out fence of batch that users or other batches

a batch

> +        /* Cached value of the signaled state to avoid calling WAIC_SYNCOBJs

WAIT_SYNCOBJs
On Mon, 16 Sep 2019 15:38:10 -0400
Alyssa Rosenzweig <alyssa@rosenzweig.io> wrote:

> > +        /* Start in a signaled state so that even non-submitted batches
> > +         * (those that have no draw/clear) can be waited upon.
> > +         */  
> 
> When would this happen? Presumably if a batch does nothing whatsoever,
> it doesn't make sense to wait on it.

Was just simpler to have all batch fences containing a syncobjs on
which we can call WAIT_SYNCOBJ even no real fence is attached to it.

The other option being to set fence->signaled to true when a batch with
no draw/clear is submitted, and then skip those entries when we build
the array passed to the waitsyncobj() func.

> >  #include "pan_resource.h"
> >  
> > +        /* Batch that created this fence object. Will become NULL at batch
> > +         * submission time. This field is mainly here to know whether the
> > +         * batch has been flushed or not.
> > +         */
> > +        struct panfrost_batch *batch;  
> 
> Could this be replaced by just a `bool flushed`, or do we actually use
> the value in a later patch?

I actually use the value to flush deps when they're not already flushed.
> > > +        /* Start in a signaled state so that even non-submitted batches
> > > +         * (those that have no draw/clear) can be waited upon.
> > > +         */  
> > 
> > When would this happen? Presumably if a batch does nothing whatsoever,
> > it doesn't make sense to wait on it.
> 
> Was just simpler to have all batch fences containing a syncobjs on
> which we can call WAIT_SYNCOBJ even no real fence is attached to it.
> 
> The other option being to set fence->signaled to true when a batch with
> no draw/clear is submitted, and then skip those entries when we build
> the array passed to the waitsyncobj() func.

I'm wondering the latter option would be cleaner, rather than having
extra DRM objects floating around?

> > >  #include "pan_resource.h"
> > >  
> > > +        /* Batch that created this fence object. Will become NULL at batch
> > > +         * submission time. This field is mainly here to know whether the
> > > +         * batch has been flushed or not.
> > > +         */
> > > +        struct panfrost_batch *batch;  
> > 
> > Could this be replaced by just a `bool flushed`, or do we actually use
> > the value in a later patch?
> 
> I actually use the value to flush deps when they're not already flushed.

Fair enough.