(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.brezil...@collabora.com>
> Reviewed-by: Alyssa Rosenzweig <alyssa.rosenzw...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
