[Mesa-dev] [PATCH v6] etnaviv: fix resource usage tracking across different pipe_context's

Boris Brezillon boris.brezillon at collabora.com
Mon Feb 25 09:53:57 UTC 2019


On Sat, 23 Feb 2019 16:15:19 +0100
Christian Gmeiner <christian.gmeiner at gmail.com> wrote:

> A pipe_resource can be shared by all the pipe_context's hanging off the
> same pipe_screen.
> 
> Changes from v2 -> v3:
>  - add locking with mtx_*() to resource and screen (Marek)
> Changes from v3 -> v4:
>  - drop rsc->lock, just use screen->lock for the entire serialization (Marek)
>  - simplify etna_resource_used() flush condition, which also prevents
>    potentially flushing resources twice (Marek)
>  - don't remove resouces from screen->used_resources in
>    etna_cmd_stream_reset_notify(), they may still be used in other
>    contexts and may need flushing there later on (Marek)
> Changes from v4 -> v5:
>  - Fix coding style issues reported by Guido
> Changes from v5 -> v6:
>  - Add missing locking in etna_transfer_map(..) (Boris)
> 
> Signed-off-by: Christian Gmeiner <christian.gmeiner at gmail.com>
> Signed-off-by: Marek Vasut <marex at denx.de>
> Signed-off-by: Boris Brezillon <boris.brezillon at collabora.com>

Reviewed-by: Boris Brezillon <boris.brezillon at collabora.com>
Tested-by: Boris Brezillon <boris.brezillon at collabora.com>

This being said, I'm still unsure all races are fixed with this patch
(see the part about RS-based tiling in my reply to v5).

> Tested-by: Marek Vasut <marex at denx.de>
> ---
>  src/gallium/drivers/etnaviv/etnaviv_context.c | 26 +++++-----
>  src/gallium/drivers/etnaviv/etnaviv_context.h |  3 --
>  .../drivers/etnaviv/etnaviv_resource.c        | 52 +++++++++++++++----
>  .../drivers/etnaviv/etnaviv_resource.h        |  8 +--
>  src/gallium/drivers/etnaviv/etnaviv_screen.c  | 12 +++++
>  src/gallium/drivers/etnaviv/etnaviv_screen.h  |  6 +++
>  .../drivers/etnaviv/etnaviv_transfer.c        |  5 ++
>  7 files changed, 83 insertions(+), 29 deletions(-)
> 
> diff --git a/src/gallium/drivers/etnaviv/etnaviv_context.c b/src/gallium/drivers/etnaviv/etnaviv_context.c
> index 44b50925a4f..83a703f7cc2 100644
> --- a/src/gallium/drivers/etnaviv/etnaviv_context.c
> +++ b/src/gallium/drivers/etnaviv/etnaviv_context.c
> @@ -36,6 +36,7 @@
>  #include "etnaviv_query.h"
>  #include "etnaviv_query_hw.h"
>  #include "etnaviv_rasterizer.h"
> +#include "etnaviv_resource.h"
>  #include "etnaviv_screen.h"
>  #include "etnaviv_shader.h"
>  #include "etnaviv_state.h"
> @@ -329,7 +330,8 @@ static void
>  etna_cmd_stream_reset_notify(struct etna_cmd_stream *stream, void *priv)
>  {
>     struct etna_context *ctx = priv;
> -   struct etna_resource *rsc, *rsc_tmp;
> +   struct etna_screen *screen = ctx->screen;
> +   struct set_entry *entry;
>  
>     etna_set_state(stream, VIVS_GL_API_MODE, VIVS_GL_API_MODE_OPENGL);
>     etna_set_state(stream, VIVS_GL_VERTEX_ELEMENT_CONFIG, 0x00000001);
> @@ -384,16 +386,18 @@ etna_cmd_stream_reset_notify(struct etna_cmd_stream *stream, void *priv)
>     ctx->dirty = ~0L;
>     ctx->dirty_sampler_views = ~0L;
>  
> -   /* go through all the used resources and clear their status flag */
> -   LIST_FOR_EACH_ENTRY_SAFE(rsc, rsc_tmp, &ctx->used_resources, list)
> -   {
> -      debug_assert(rsc->status != 0);
> -      rsc->status = 0;
> -      rsc->pending_ctx = NULL;
> -      list_delinit(&rsc->list);
> -   }
> +   /*
> +    * Go through all _resources_ associated with this _screen_, pending
> +    * in this _context_ and mark them as not pending in this _context_
> +    * anymore, since they were just flushed.
> +    */
> +   mtx_lock(&screen->lock);
> +   set_foreach(screen->used_resources, entry) {
> +      struct etna_resource *rsc = (struct etna_resource *)entry->key;
>  
> -   assert(LIST_IS_EMPTY(&ctx->used_resources));
> +      _mesa_set_remove_key(rsc->pending_ctx, ctx);
> +   }
> +   mtx_unlock(&screen->lock);
>  }
>  
>  static void
> @@ -437,8 +441,6 @@ etna_context_create(struct pipe_screen *pscreen, void *priv, unsigned flags)
>     /* need some sane default in case state tracker doesn't set some state: */
>     ctx->sample_mask = 0xffff;
>  
> -   list_inithead(&ctx->used_resources);
> -
>     /*  Set sensible defaults for state */
>     etna_cmd_stream_reset_notify(ctx->stream, ctx);
>  
> diff --git a/src/gallium/drivers/etnaviv/etnaviv_context.h b/src/gallium/drivers/etnaviv/etnaviv_context.h
> index 6ad9f3431e1..50a2cdf3d07 100644
> --- a/src/gallium/drivers/etnaviv/etnaviv_context.h
> +++ b/src/gallium/drivers/etnaviv/etnaviv_context.h
> @@ -136,9 +136,6 @@ struct etna_context {
>     uint32_t prim_hwsupport;
>     struct primconvert_context *primconvert;
>  
> -   /* list of resources used by currently-unsubmitted renders */
> -   struct list_head used_resources;
> -
>     struct slab_child_pool transfer_pool;
>     struct blitter_context *blitter;
>  
> diff --git a/src/gallium/drivers/etnaviv/etnaviv_resource.c b/src/gallium/drivers/etnaviv/etnaviv_resource.c
> index db5ead4d0ba..1e8fa714060 100644
> --- a/src/gallium/drivers/etnaviv/etnaviv_resource.c
> +++ b/src/gallium/drivers/etnaviv/etnaviv_resource.c
> @@ -33,6 +33,7 @@
>  #include "etnaviv_screen.h"
>  #include "etnaviv_translate.h"
>  
> +#include "util/hash_table.h"
>  #include "util/u_inlines.h"
>  #include "util/u_memory.h"
>  
> @@ -280,7 +281,6 @@ etna_resource_alloc(struct pipe_screen *pscreen, unsigned layout,
>     rsc->addressing_mode = mode;
>  
>     pipe_reference_init(&rsc->base.reference, 1);
> -   list_inithead(&rsc->list);
>  
>     size = setup_miptree(rsc, paddingX, paddingY, msaa_xscale, msaa_yscale);
>  
> @@ -301,6 +301,11 @@ etna_resource_alloc(struct pipe_screen *pscreen, unsigned layout,
>        memset(map, 0, size);
>     }
>  
> +   rsc->pending_ctx = _mesa_set_create(NULL, _mesa_hash_pointer,
> +                                       _mesa_key_pointer_equal);
> +   if (!rsc->pending_ctx)
> +      goto free_rsc;
> +
>     return &rsc->base;
>  
>  free_rsc:
> @@ -462,8 +467,14 @@ etna_resource_changed(struct pipe_screen *pscreen, struct pipe_resource *prsc)
>  static void
>  etna_resource_destroy(struct pipe_screen *pscreen, struct pipe_resource *prsc)
>  {
> +   struct etna_screen *screen = etna_screen(pscreen);
>     struct etna_resource *rsc = etna_resource(prsc);
>  
> +   mtx_lock(&screen->lock);
> +   _mesa_set_remove_key(screen->used_resources, rsc);
> +   _mesa_set_destroy(rsc->pending_ctx, NULL);
> +   mtx_unlock(&screen->lock);
> +
>     if (rsc->bo)
>        etna_bo_del(rsc->bo);
>  
> @@ -473,8 +484,6 @@ etna_resource_destroy(struct pipe_screen *pscreen, struct pipe_resource *prsc)
>     if (rsc->scanout)
>        renderonly_scanout_destroy(rsc->scanout, etna_screen(pscreen)->ro);
>  
> -   list_delinit(&rsc->list);
> -
>     pipe_resource_reference(&rsc->texture, NULL);
>     pipe_resource_reference(&rsc->external, NULL);
>  
> @@ -508,7 +517,6 @@ etna_resource_from_handle(struct pipe_screen *pscreen,
>     *prsc = *tmpl;
>  
>     pipe_reference_init(&prsc->reference, 1);
> -   list_inithead(&rsc->list);
>     prsc->screen = pscreen;
>  
>     rsc->bo = etna_screen_bo_from_handle(pscreen, handle, &level->stride);
> @@ -555,6 +563,11 @@ etna_resource_from_handle(struct pipe_screen *pscreen,
>        goto fail;
>     }
>  
> +   rsc->pending_ctx = _mesa_set_create(NULL, _mesa_hash_pointer,
> +                                       _mesa_key_pointer_equal);
> +   if (!rsc->pending_ctx)
> +      goto fail;
> +
>     if (rsc->layout == ETNA_LAYOUT_LINEAR) {
>        /*
>         * Both sampler and pixel pipes can't handle linear, create a compatible
> @@ -629,20 +642,39 @@ void
>  etna_resource_used(struct etna_context *ctx, struct pipe_resource *prsc,
>                     enum etna_resource_status status)
>  {
> +   struct etna_screen *screen = ctx->screen;
> +   struct set_entry *entry;
>     struct etna_resource *rsc;
>  
>     if (!prsc)
>        return;
>  
>     rsc = etna_resource(prsc);
> +
> +   mtx_lock(&screen->lock);
> +
> +   /*
> +    * if we are pending read or write by any other context or
> +    * if reading a resource pending a write, then
> +    * flush all the contexts to maintain coherency
> +    */
> +   if (((status & ETNA_PENDING_WRITE) && rsc->status) ||
> +       ((status & ETNA_PENDING_READ) && (rsc->status & ETNA_PENDING_WRITE))) {
> +      set_foreach(rsc->pending_ctx, entry) {
> +         struct etna_context *extctx = (struct etna_context *)entry->key;
> +         struct pipe_context *pctx = &extctx->base;
> +
> +         pctx->flush(pctx, NULL, 0);
> +      }
> +      rsc->status = 0;
> +   }
> +
>     rsc->status |= status;
>  
> -   /* TODO resources can actually be shared across contexts,
> -    * so I'm not sure a single list-head will do the trick? */
> -   debug_assert((rsc->pending_ctx == ctx) || !rsc->pending_ctx);
> -   list_delinit(&rsc->list);
> -   list_addtail(&rsc->list, &ctx->used_resources);
> -   rsc->pending_ctx = ctx;
> +   _mesa_set_add(screen->used_resources, rsc);
> +   _mesa_set_add(rsc->pending_ctx, ctx);
> +
> +   mtx_unlock(&screen->lock);
>  }
>  
>  bool
> diff --git a/src/gallium/drivers/etnaviv/etnaviv_resource.h b/src/gallium/drivers/etnaviv/etnaviv_resource.h
> index 75aa80b3d7a..75414a2f949 100644
> --- a/src/gallium/drivers/etnaviv/etnaviv_resource.h
> +++ b/src/gallium/drivers/etnaviv/etnaviv_resource.h
> @@ -31,7 +31,10 @@
>  #include "etnaviv_tiling.h"
>  #include "pipe/p_state.h"
>  #include "util/list.h"
> +#include "util/set.h"
> +#include "util/u_helpers.h"
>  
> +struct etna_context;
>  struct pipe_screen;
>  
>  struct etna_resource_level {
> @@ -89,10 +92,7 @@ struct etna_resource {
>  
>     enum etna_resource_status status;
>  
> -   /* resources accessed by queued but not flushed draws are tracked
> -    * in the used_resources list. */
> -   struct list_head list;
> -   struct etna_context *pending_ctx;
> +   struct set *pending_ctx;
>  };
>  
>  /* returns TRUE if a is newer than b */
> diff --git a/src/gallium/drivers/etnaviv/etnaviv_screen.c b/src/gallium/drivers/etnaviv/etnaviv_screen.c
> index de822fc85ca..0a5477d6e84 100644
> --- a/src/gallium/drivers/etnaviv/etnaviv_screen.c
> +++ b/src/gallium/drivers/etnaviv/etnaviv_screen.c
> @@ -38,6 +38,7 @@
>  #include "etnaviv_resource.h"
>  #include "etnaviv_translate.h"
>  
> +#include "util/hash_table.h"
>  #include "util/os_time.h"
>  #include "util/u_math.h"
>  #include "util/u_memory.h"
> @@ -82,6 +83,9 @@ etna_screen_destroy(struct pipe_screen *pscreen)
>  {
>     struct etna_screen *screen = etna_screen(pscreen);
>  
> +   _mesa_set_destroy(screen->used_resources, NULL);
> +   mtx_destroy(&screen->lock);
> +
>     if (screen->perfmon)
>        etna_perfmon_del(screen->perfmon);
>  
> @@ -1029,8 +1033,16 @@ etna_screen_create(struct etna_device *dev, struct etna_gpu *gpu,
>     if (screen->drm_version >= ETNA_DRM_VERSION_PERFMON)
>        etna_pm_query_setup(screen);
>  
> +   mtx_init(&screen->lock, mtx_recursive);
> +   screen->used_resources = _mesa_set_create(NULL, _mesa_hash_pointer,
> +                                             _mesa_key_pointer_equal);
> +   if (!screen->used_resources)
> +      goto fail2;
> +
>     return pscreen;
>  
> +fail2:
> +   mtx_destroy(&screen->lock);
>  fail:
>     etna_screen_destroy(pscreen);
>     return NULL;
> diff --git a/src/gallium/drivers/etnaviv/etnaviv_screen.h b/src/gallium/drivers/etnaviv/etnaviv_screen.h
> index bffd4b6ef94..9757985526e 100644
> --- a/src/gallium/drivers/etnaviv/etnaviv_screen.h
> +++ b/src/gallium/drivers/etnaviv/etnaviv_screen.h
> @@ -34,8 +34,10 @@
>  #include "os/os_thread.h"
>  #include "pipe/p_screen.h"
>  #include "renderonly/renderonly.h"
> +#include "util/set.h"
>  #include "util/slab.h"
>  #include "util/u_dynarray.h"
> +#include "util/u_helpers.h"
>  
>  struct etna_bo;
>  
> @@ -80,6 +82,10 @@ struct etna_screen {
>     struct etna_specs specs;
>  
>     uint32_t drm_version;
> +
> +   /* set of resources used by currently-unsubmitted renders */
> +   mtx_t lock;
> +   struct set *used_resources;
>  };
>  
>  static inline struct etna_screen *
> diff --git a/src/gallium/drivers/etnaviv/etnaviv_transfer.c b/src/gallium/drivers/etnaviv/etnaviv_transfer.c
> index 0b7411b47ef..9bfd832bd48 100644
> --- a/src/gallium/drivers/etnaviv/etnaviv_transfer.c
> +++ b/src/gallium/drivers/etnaviv/etnaviv_transfer.c
> @@ -303,6 +303,7 @@ etna_transfer_map(struct pipe_context *pctx, struct pipe_resource *prsc,
>      * transfers without a temporary resource.
>      */
>     if (trans->rsc || !(usage & PIPE_TRANSFER_UNSYNCHRONIZED)) {
> +      struct etna_screen *screen = ctx->screen;
>        uint32_t prep_flags = 0;
>  
>        /*
> @@ -311,12 +312,16 @@ etna_transfer_map(struct pipe_context *pctx, struct pipe_resource *prsc,
>         * current GPU usage (reads must wait for GPU writes, writes must have
>         * exclusive access to the buffer).
>         */
> +      mtx_lock(&screen->lock);
> +
>        if ((trans->rsc && (etna_resource(trans->rsc)->status & ETNA_PENDING_WRITE)) ||
>            (!trans->rsc &&
>             (((usage & PIPE_TRANSFER_READ) && (rsc->status & ETNA_PENDING_WRITE)) ||
>             ((usage & PIPE_TRANSFER_WRITE) && rsc->status))))
>           pctx->flush(pctx, NULL, 0);
>  
> +      mtx_unlock(&screen->lock);
> +
>        if (usage & PIPE_TRANSFER_READ)
>           prep_flags |= DRM_ETNA_PREP_READ;
>        if (usage & PIPE_TRANSFER_WRITE)



More information about the mesa-dev mailing list