[Mesa-dev] [PATCH v4] etnaviv: fix resource usage tracking across different pipe_context's
Lucas Stach
l.stach at pengutronix.de
Mon Jan 14 14:02:12 UTC 2019
Am Montag, den 14.01.2019, 14:54 +0100 schrieb Marek Vasut:
> On 1/14/19 12:16 PM, Lucas Stach wrote:
> > Hi Marek,
> >
> > 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
> > > ---
> > > 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)
> >
> > The patch mostly makes sense to me now, but don't we need to take the
> > screen->lock on all call sites where we do a ctx->flush? Otherwise we
> > may enter etna_cmd_stream_reset_notify unlocked, changing the
> > used_resources set while other threads might use it at the same time,
> > right?
>
> etna_cmd_stream_reset_notify() takes the lock when accessing the
> used_resources set , see below.
Uh, sorry seems I mixed this up. But then I don't see how this isn't
deadlocking, as AFAICS mtx_lock isn't recursive.
In etna_resource_used() you already lock the screen mutex, then when
you find a context that needs flushing you call the context flush,
which flushes the cmd stream and calls into
etna_cmd_stream_reset_notify() where the mutex is locked again -> self
deadlock.
Regards,
Lucas
> > Regards,
> > Lucas
> >
> > > ---
> > > 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 *
>
>
More information about the mesa-dev
mailing list