[Mesa-dev] [PATCH v2 36/37] panfrost: Take draw call order into account
Boris Brezillon
boris.brezillon at collabora.com
Tue Sep 17 13:15:32 UTC 2019
On Tue, 17 Sep 2019 08:32:35 -0400
Alyssa Rosenzweig <alyssa at rosenzweig.io> wrote:
> Hmm, could you explain a bit why this is necessary?
>
> I would think if there's no dependency, there's no dependency, and if
> this fixes bugs, that's a dependency tracking issue that we're just
> papering over.
Indeed, there's no explicit dependency, and the logic works just fine
without this patch. It's just that Steven mentioned in his previous
review that you sometimes have weak (or non-explicit) dependencies. Say
you want to queue draws for frame N and N+1 before you call glFlush()
(is that what happens when we do triple buffering?), sometimes frame N
and N+1 have no inter-dependencies (they access different resources and
frame N+1 doesn't care about the state of frame N). Even if things
would be correct if frame N+1 is rendered before frame N, the user
probably expects the opposite, such that it can output frame N as soon
as possible.
Anyway, I'm fine dropping patch 35 and 36 (see the note in my cover
letter ;-)).
>
> (Also, I guess r-b on previous patch retracted temporarily since it was a setup for
> this.)
>
> On Mon, Sep 16, 2019 at 11:37:14AM +0200, Boris Brezillon wrote:
> > This is not strictly required, but let's try to match the draw call
> > orders, just in case the app had a reason to do it in this order.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon at collabora.com>
> > ---
> > src/gallium/drivers/panfrost/pan_context.h | 6 ++++++
> > src/gallium/drivers/panfrost/pan_job.c | 23 +++++++++++++++++++---
> > src/gallium/drivers/panfrost/pan_job.h | 3 +++
> > 3 files changed, 29 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/gallium/drivers/panfrost/pan_context.h b/src/gallium/drivers/panfrost/pan_context.h
> > index f13967f51b46..c6b53685b285 100644
> > --- a/src/gallium/drivers/panfrost/pan_context.h
> > +++ b/src/gallium/drivers/panfrost/pan_context.h
> > @@ -114,6 +114,12 @@ struct panfrost_context {
> > struct panfrost_batch *batch;
> > struct hash_table *fbo_to_batch;
> >
> > + /* A list containing all non-submitted batches since the last flush.
> > + * This list is used to keep track of clear/draw order on batches that
> > + * don't have explicit dependencies between them.
> > + */
> > + struct list_head batch_queue;
> > +
> > /* panfrost_bo -> panfrost_bo_access */
> > struct hash_table *accessed_bos;
> >
> > diff --git a/src/gallium/drivers/panfrost/pan_job.c b/src/gallium/drivers/panfrost/pan_job.c
> > index 13d7e8086e62..e030f8e98dad 100644
> > --- a/src/gallium/drivers/panfrost/pan_job.c
> > +++ b/src/gallium/drivers/panfrost/pan_job.c
> > @@ -118,6 +118,7 @@ panfrost_create_batch(struct panfrost_context *ctx,
> > util_dynarray_init(&batch->headers, batch);
> > util_dynarray_init(&batch->gpu_headers, batch);
> > util_dynarray_init(&batch->dependencies, batch);
> > + list_inithead(&batch->queue_node);
> > batch->out_sync = panfrost_create_batch_fence(batch);
> > util_copy_framebuffer_state(&batch->key, key);
> >
> > @@ -181,6 +182,9 @@ panfrost_free_batch(struct panfrost_batch *batch)
> > struct panfrost_batch_fence *, dep)
> > panfrost_batch_fence_unreference(*dep);
> >
> > + /* Remove the batch from the batch queue. */
> > + list_del(&batch->queue_node);
> > +
> > /* 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
> > @@ -543,6 +547,13 @@ void panfrost_batch_add_fbo_bos(struct panfrost_batch *batch)
> > struct panfrost_resource *rsrc = pan_resource(batch->key.zsbuf->texture);
> > panfrost_batch_add_bo(batch, rsrc->bo, flags);
> > }
> > +
> > + /* We the batch was not already present in the queue, add it know.
> > + * Should we move the batch at end of the queue when a new draw
> > + * happens?
> > + */
> > + if (list_empty(&batch->queue_node))
> > + list_addtail(&batch->queue_node, &batch->ctx->batch_queue);
> > }
> >
> > struct panfrost_bo *
> > @@ -878,10 +889,15 @@ panfrost_flush_all_batches(struct panfrost_context *ctx, bool wait)
> > util_dynarray_init(&syncobjs, NULL);
> > }
> >
> > - hash_table_foreach(ctx->fbo_to_batch, hentry) {
> > - struct panfrost_batch *batch = hentry->data;
> > + /* We can use the for_each_entry_safe() iterator here because the
> > + * next element might be removed from the list when flushing the
> > + * dependencies in panfrost_batch_submit().
> > + */
> > + while (!list_empty(&ctx->batch_queue)) {
> > + struct panfrost_batch *batch;
> >
> > - assert(batch);
> > + batch = list_first_entry(&ctx->batch_queue,
> > + struct panfrost_batch, queue_node);
> >
> > if (wait) {
> > panfrost_batch_fence_reference(batch->out_sync);
> > @@ -1150,4 +1166,5 @@ panfrost_batch_init(struct panfrost_context *ctx)
> > panfrost_batch_compare);
> > ctx->accessed_bos = _mesa_hash_table_create(ctx, _mesa_hash_pointer,
> > _mesa_key_pointer_equal);
> > + list_inithead(&ctx->batch_queue);
> > }
> > diff --git a/src/gallium/drivers/panfrost/pan_job.h b/src/gallium/drivers/panfrost/pan_job.h
> > index d198864ce4f7..34926e30cdde 100644
> > --- a/src/gallium/drivers/panfrost/pan_job.h
> > +++ b/src/gallium/drivers/panfrost/pan_job.h
> > @@ -71,6 +71,9 @@ struct panfrost_batch {
> > struct panfrost_context *ctx;
> > struct pipe_framebuffer_state key;
> >
> > + /* Used to insert the batch in the batch queue */
> > + struct list_head queue_node;
> > +
> > /* Buffers cleared (PIPE_CLEAR_* bitmask) */
> > unsigned clear;
> >
> > --
> > 2.21.0
More information about the mesa-dev
mailing list