[Mesa-dev] [PATCH v5] etnaviv: fix resource usage tracking across different pipe_context's
Boris Brezillon
boris.brezillon at collabora.com
Thu Feb 21 22:29:53 UTC 2019
Christian, Marek,
On Wed, 30 Jan 2019 05:28:14 +0100
Marek Vasut <marex at denx.de> wrote:
> 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.
We seem to be impacted by the problem you're fixing here, but, while
this patch definitely make things much better, the problem does not
seem to be entirely fixed (at least in our case).
A bit more context: we have Qt App using QtWebEngine to render html
content. When we call QtWebEngine::initialize(), which as for effect
to allow shared GL contexts, we sometimes notice that part of the web
page is mis-rendered. That does not happen when we omit the
QtWebEngine::initialize() call.
As said above, this patch make those rendering issues less likely to
happen, but we still have the problem from time to time. So I thought
I'd share my guesses about what could cause these issues before
debugging it further.
First thing I noticed: I couldn't reproduce the problem with [1]
applied (+ a tiny change forcing CPU-based tiling no matter the size of
the region to tile/untile). So, my guess is that it's related to how we
handle GPU-based tiling/untiling.
Also noticed that we're testing the rsc->status here [2] without the
screen->lock held, and there might be a race with another thread calling
resource_used(). We might also lack a resource_read(ctx, &src->base)
here [3]. But even after fixing those problems, the rendering issues
are not gone. So I'm wondering is the problem is not coming from the
GPU-based tiling/untiling logic we have in etnaviv_transfer.c.
Indeed, RS-based tiling require the surface we tile/untile to be
aligned on at least 16 pixels (and even more in case of multi/super
tiles). In order to handle those constraints when the region the user
wants to map/update is not properly aligned, etna_transfer_map() first
untiles an RS-aligned region into a temporary linear buffer which will
then be updated according to the caller's needs and tiled back to the
actual texture when etna_transfer_unmap() is called. But what happens
if, between the untile operation in etna_transfer_map() and the tile
operation in etna_transfer_unmap(), another rendering request is done
on region that overlaps the RS-aligned region we untiled in our
temporary buffer?
Can't we end up in a situation where some a the data we are about to
copy back to the actual texture are actually outdated?
I'm completely new to etnaviv and mesa, so I wouldn't be surprised if
my analysis of the problem was wrong, but I thought it was worth an
email before digging further.
Please let me know if you have other ideas or need more details about
our use case.
Thanks,
Boris
[1]https://lists.freedesktop.org/archives/mesa-dev/2019-February/215597.html
[2]https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/drivers/etnaviv/etnaviv_transfer.c#n316
[3]https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/drivers/etnaviv/etnaviv_rs.c#n731
>
> 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)
> Changes from v4 -> v5:
> - Fix coding style issues reported by Guido
> ---
> 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 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