[Mesa-dev] [PATCH v2 36/37] panfrost: Take draw call order into account

[Alyssa Rosenzweig]

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.

(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