[Mesa-dev] [PATCH 5/5] radeonsi: use current context for DCC feedback-loop decompress, fixes Elemental
Nicolai Hähnle
nhaehnle at gmail.com
Wed Aug 17 09:41:59 UTC 2016
On 11.08.2016 18:16, Marek Olšák wrote:
> From: Marek Olšák <marek.olsak at amd.com>
>
> This is just a workaround. The problem is described in the code.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96541
> ---
> src/gallium/drivers/radeon/r600_pipe_common.h | 2 +-
> src/gallium/drivers/radeon/r600_texture.c | 35 ++++++++++++++++++++++-----
> src/gallium/drivers/radeonsi/si_blit.c | 12 +++------
> src/gallium/drivers/radeonsi/si_descriptors.c | 2 +-
> 4 files changed, 35 insertions(+), 16 deletions(-)
>
> diff --git a/src/gallium/drivers/radeon/r600_pipe_common.h b/src/gallium/drivers/radeon/r600_pipe_common.h
> index e4002f9..5375044 100644
> --- a/src/gallium/drivers/radeon/r600_pipe_common.h
> +++ b/src/gallium/drivers/radeon/r600_pipe_common.h
> @@ -764,21 +764,21 @@ void vi_separate_dcc_stop_query(struct pipe_context *ctx,
> void vi_separate_dcc_process_and_reset_stats(struct pipe_context *ctx,
> struct r600_texture *tex);
> void vi_dcc_clear_level(struct r600_common_context *rctx,
> struct r600_texture *rtex,
> unsigned level, unsigned clear_value);
> void evergreen_do_fast_color_clear(struct r600_common_context *rctx,
> struct pipe_framebuffer_state *fb,
> struct r600_atom *fb_state,
> unsigned *buffers, unsigned *dirty_cbufs,
> const union pipe_color_union *color);
> -bool r600_texture_disable_dcc(struct r600_common_screen *rscreen,
> +bool r600_texture_disable_dcc(struct r600_common_context *rctx,
> struct r600_texture *rtex);
> void r600_init_screen_texture_functions(struct r600_common_screen *rscreen);
> void r600_init_context_texture_functions(struct r600_common_context *rctx);
>
> /* r600_viewport.c */
> void evergreen_apply_scissor_bug_workaround(struct r600_common_context *rctx,
> struct pipe_scissor_state *scissor);
> void r600_set_scissor_enable(struct r600_common_context *rctx, bool enable);
> void r600_update_vs_writes_viewport_index(struct r600_common_context *rctx,
> struct tgsi_shader_info *info);
> diff --git a/src/gallium/drivers/radeon/r600_texture.c b/src/gallium/drivers/radeon/r600_texture.c
> index ddfa135..78dd177 100644
> --- a/src/gallium/drivers/radeon/r600_texture.c
> +++ b/src/gallium/drivers/radeon/r600_texture.c
> @@ -393,34 +393,55 @@ static bool r600_texture_discard_dcc(struct r600_common_screen *rscreen,
> assert(rtex->dcc_separate_buffer == NULL);
>
> /* Disable DCC. */
> rtex->dcc_offset = 0;
>
> /* Notify all contexts about the change. */
> r600_dirty_all_framebuffer_states(rscreen);
> return true;
> }
>
> -bool r600_texture_disable_dcc(struct r600_common_screen *rscreen,
> +/**
> + * Disable DCC for the texture. (first decompress, then discard metadata).
> + *
> + * Unresolved multi-context synchronization issue:
> + *
> + * If context 1 disables DCC and context 2 has queued commands that write
> + * to the texture via CB with DCC enabled, and the order of operations is
> + * as follows:
> + * context 2 queues draw calls rendering to the texture, but doesn't flush
> + * context 1 disables DCC and flushes
> + * context 1 & 2 reset descriptors and FB state
> + * context 2 flushes (new compressed tiles written by the draw calls)
> + * context 1 & 2 read garbage, because DCC is disabled, yet there are
> + * compressed tiled
Should this case lead to defined result according to the spec? The
reason for context 1 disabling DCC is that it in some sense "reads" from
the texture -- via a blit, get_handle, or whatever. But without any
synchronization, those reads should be undefined anyway, because context
1 doesn't know whether context 2 has completed.
Granted, the _type_ of undefined behavior is perhaps unusual, because
some people might expect either the results from before context 2's
draws or from after context 2's draws, not some random DCC-related
garbage, but should we care?
Or maybe I'm missing some other situation?
Nicolai
> + *
> + * \param rctx the current context if you have one, or rscreen->aux_context
> + * if you don't.
> + */
> +bool r600_texture_disable_dcc(struct r600_common_context *rctx,
> struct r600_texture *rtex)
> {
> - struct r600_common_context *rctx =
> - (struct r600_common_context *)rscreen->aux_context;
> + struct r600_common_screen *rscreen = rctx->screen;
>
> if (!r600_can_disable_dcc(rtex))
> return false;
>
> + if (&rctx->b == rscreen->aux_context)
> + pipe_mutex_lock(rscreen->aux_context_lock);
> +
> /* Decompress DCC. */
> - pipe_mutex_lock(rscreen->aux_context_lock);
> rctx->decompress_dcc(&rctx->b, rtex);
> rctx->b.flush(&rctx->b, NULL, 0);
> - pipe_mutex_unlock(rscreen->aux_context_lock);
> +
> + if (&rctx->b == rscreen->aux_context)
> + pipe_mutex_unlock(rscreen->aux_context_lock);
>
> return r600_texture_discard_dcc(rscreen, rtex);
> }
>
> static void r600_degrade_tile_mode_to_linear(struct r600_common_context *rctx,
> struct r600_texture *rtex,
> bool invalidate_storage)
> {
> struct pipe_screen *screen = rctx->b.screen;
> struct r600_texture *new_tex;
> @@ -485,39 +506,41 @@ static void r600_degrade_tile_mode_to_linear(struct r600_common_context *rctx,
> r600_dirty_all_framebuffer_states(rctx->screen);
> p_atomic_inc(&rctx->screen->dirty_tex_descriptor_counter);
> }
>
> static boolean r600_texture_get_handle(struct pipe_screen* screen,
> struct pipe_resource *resource,
> struct winsys_handle *whandle,
> unsigned usage)
> {
> struct r600_common_screen *rscreen = (struct r600_common_screen*)screen;
> + struct r600_common_context *aux_context =
> + (struct r600_common_context*)rscreen->aux_context;
> struct r600_resource *res = (struct r600_resource*)resource;
> struct r600_texture *rtex = (struct r600_texture*)resource;
> struct radeon_bo_metadata metadata;
> bool update_metadata = false;
>
> /* This is not supported now, but it might be required for OpenCL
> * interop in the future.
> */
> if (resource->target != PIPE_BUFFER &&
> (resource->nr_samples > 1 || rtex->is_depth))
> return false;
>
> if (resource->target != PIPE_BUFFER) {
> /* Since shader image stores don't support DCC on VI,
> * disable it for external clients that want write
> * access.
> */
> if (usage & PIPE_HANDLE_USAGE_WRITE && rtex->dcc_offset) {
> - if (r600_texture_disable_dcc(rscreen, rtex))
> + if (r600_texture_disable_dcc(aux_context, rtex))
> update_metadata = true;
> }
>
> if (!(usage & PIPE_HANDLE_USAGE_EXPLICIT_FLUSH) &&
> rtex->cmask.size) {
> /* Eliminate fast clear (both CMASK and DCC) */
> r600_eliminate_fast_color_clear(rscreen, rtex);
>
> /* Disable CMASK if flush_resource isn't going
> * to be called.
> diff --git a/src/gallium/drivers/radeonsi/si_blit.c b/src/gallium/drivers/radeonsi/si_blit.c
> index f4bff6b..3cfd011 100644
> --- a/src/gallium/drivers/radeonsi/si_blit.c
> +++ b/src/gallium/drivers/radeonsi/si_blit.c
> @@ -550,24 +550,22 @@ static void si_check_render_feedback_textures(struct si_context *sctx,
> surf = (struct r600_surface*)sctx->framebuffer.state.cbufs[j];
>
> if (tex == (struct r600_texture*)surf->base.texture &&
> surf->base.u.tex.level >= view->u.tex.first_level &&
> surf->base.u.tex.level <= view->u.tex.last_level &&
> surf->base.u.tex.first_layer <= view->u.tex.last_layer &&
> surf->base.u.tex.last_layer >= view->u.tex.first_layer)
> render_feedback = true;
> }
>
> - if (render_feedback) {
> - struct si_screen *screen = sctx->screen;
> - r600_texture_disable_dcc(&screen->b, tex);
> - }
> + if (render_feedback)
> + r600_texture_disable_dcc(&sctx->b, tex);
> }
> }
>
> static void si_check_render_feedback_images(struct si_context *sctx,
> struct si_images_info *images)
> {
> uint32_t mask = images->enabled_mask;
>
> while (mask) {
> const struct pipe_image_view *view;
> @@ -592,24 +590,22 @@ static void si_check_render_feedback_images(struct si_context *sctx,
>
> surf = (struct r600_surface*)sctx->framebuffer.state.cbufs[j];
>
> if (tex == (struct r600_texture*)surf->base.texture &&
> surf->base.u.tex.level == view->u.tex.level &&
> surf->base.u.tex.first_layer <= view->u.tex.last_layer &&
> surf->base.u.tex.last_layer >= view->u.tex.first_layer)
> render_feedback = true;
> }
>
> - if (render_feedback) {
> - struct si_screen *screen = sctx->screen;
> - r600_texture_disable_dcc(&screen->b, tex);
> - }
> + if (render_feedback)
> + r600_texture_disable_dcc(&sctx->b, tex);
> }
> }
>
> static void si_check_render_feedback(struct si_context *sctx)
> {
>
> if (!sctx->need_check_render_feedback)
> return;
>
> for (int i = 0; i < SI_NUM_SHADERS; ++i) {
> diff --git a/src/gallium/drivers/radeonsi/si_descriptors.c b/src/gallium/drivers/radeonsi/si_descriptors.c
> index 7600671..1d04a9c 100644
> --- a/src/gallium/drivers/radeonsi/si_descriptors.c
> +++ b/src/gallium/drivers/radeonsi/si_descriptors.c
> @@ -656,21 +656,21 @@ static void si_set_shader_image(struct si_context *ctx,
>
> assert(!tex->is_depth);
> assert(tex->fmask.size == 0);
>
> if (uses_dcc &&
> view->access & PIPE_IMAGE_ACCESS_WRITE) {
> /* If DCC can't be disabled, at least decompress it.
> * The decompression is relatively cheap if the surface
> * has been decompressed already.
> */
> - if (r600_texture_disable_dcc(&screen->b, tex))
> + if (r600_texture_disable_dcc(&ctx->b, tex))
> uses_dcc = false;
> else
> ctx->b.decompress_dcc(&ctx->b.b, tex);
> }
>
> if (is_compressed_colortex(tex)) {
> images->compressed_colortex_mask |= 1 << slot;
> } else {
> images->compressed_colortex_mask &= ~(1 << slot);
> }
>
More information about the mesa-dev
mailing list