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

Lukas Hartmann lukas at mntmn.com
Wed Jan 16 11:26:28 UTC 2019


FYI I tested this in production for a few days and encountered no problems.

Cheers
Lukas

> On 16. Jan 2019, at 12:14, Lucas Stach <l.stach at pengutronix.de> wrote:
> 
> Am Samstag, den 12.01.2019, 22:22 +0100 schrieb Marek Vasut:
>>> From: Christian Gmeiner <christian.gmeiner at gmail.com>
>> 
>> A pipe_resource can be shared by all the pipe_context's hanging off the
>> same pipe_screen.
>> 
>>> Signed-off-by: Christian Gmeiner <christian.gmeiner at gmail.com>
>>> Signed-off-by: Marek Vasut <marex at denx.de>
>> To: mesa-dev at lists.freedesktop.org
>> Cc: etnaviv at lists.freedesktop.org
> 
> I would appreciate if this can get a review from someone who didn't
> totally mess up the mental model of the locking involved. To me this
> change looks good. I'll push this through some internal testing and
> push upstream by the end of the week if there aren't any objections.
> 
> Regards,
> Lucas
> 
>> ---
>> Changes from v1 -> v2:
>>  - to remove the resource from the used_resources set when it is destroyed
>> 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)
>> ---
>>  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 +++
>>  6 files changed, 78 insertions(+), 29 deletions(-)
>> 
>> diff --git a/src/gallium/drivers/etnaviv/etnaviv_context.c b/src/gallium/drivers/etnaviv/etnaviv_context.c
>> index 3038d21..2f8cae8 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 584caa7..eff0a23 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 3808c29..46ab849 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"
>>  
>> @@ -275,7 +276,6 @@ etna_resource_alloc(struct pipe_screen *pscreen, unsigned layout,
>>     rsc->halign = halign;
>>  
>>     pipe_reference_init(&rsc->base.reference, 1);
>> -   list_inithead(&rsc->list);
>>  
>>     size = setup_miptree(rsc, paddingX, paddingY, msaa_xscale, msaa_yscale);
>>  
>> @@ -296,6 +296,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:
>> @@ -455,8 +460,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);
>>  
>> @@ -466,8 +477,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);
>>  
>> @@ -501,7 +510,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);
>> @@ -547,6 +555,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
>> @@ -621,20 +634,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 everything 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 11ccf8f..3827aff 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 {
>> @@ -83,10 +86,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 fb51aa5..3e93701 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);
>>  
>> @@ -1026,8 +1030,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 bffd4b6..9757985 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 *
> _______________________________________________
> etnaviv mailing list
> etnaviv at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/etnaviv



More information about the mesa-dev mailing list