[Mesa-dev] [PATCH 20/26] radeonsi: disable DCC on handle export if expecting write access

[Nicolai Hähnle]

On 02.03.2016 11:36, Marek Olšák wrote:
> From: Marek Olšák <marek.olsak at amd.com>
>
> This should be okay except that sampler views and images are not re-set.
> ---
>   src/gallium/drivers/radeon/r600_pipe_common.h |  3 +++
>   src/gallium/drivers/radeon/r600_texture.c     | 33 +++++++++++++++++++++++++++
>   src/gallium/drivers/radeonsi/si_blit.c        | 12 ++++++++++
>   3 files changed, 48 insertions(+)
>
> diff --git a/src/gallium/drivers/radeon/r600_pipe_common.h b/src/gallium/drivers/radeon/r600_pipe_common.h
> index 6e65742..43218f1 100644
> --- a/src/gallium/drivers/radeon/r600_pipe_common.h
> +++ b/src/gallium/drivers/radeon/r600_pipe_common.h
> @@ -481,6 +481,9 @@ struct r600_common_context {
>   				      unsigned first_layer, unsigned last_layer,
>   				      unsigned first_sample, unsigned last_sample);
>
> +	void (*decompress_dcc)(struct pipe_context *ctx,
> +			       struct r600_texture *rtex);
> +
>   	/* Reallocate the buffer and update all resource bindings where
>   	 * the buffer is bound, including all resource descriptors. */
>   	void (*invalidate_buffer)(struct pipe_context *ctx, struct pipe_resource *buf);
> diff --git a/src/gallium/drivers/radeon/r600_texture.c b/src/gallium/drivers/radeon/r600_texture.c
> index 4424ca3..d42d807 100644
> --- a/src/gallium/drivers/radeon/r600_texture.c
> +++ b/src/gallium/drivers/radeon/r600_texture.c
> @@ -296,6 +296,31 @@ static void r600_texture_disable_cmask(struct r600_common_screen *rscreen,
>   	r600_dirty_all_framebuffer_states(rscreen);
>   }
>
> +static void r600_texture_disable_dcc(struct r600_common_screen *rscreen,
> +				     struct r600_texture *rtex)
> +{
> +	struct r600_common_context *rctx =
> +		(struct r600_common_context *)rscreen->aux_context;
> +
> +	if (!rtex->dcc_offset)
> +		return;
> +
> +	/* 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);
> +
> +	/* Disable DCC. */
> +	rtex->dcc_offset = 0;
> +	rtex->cb_color_info &= ~VI_S_028C70_DCC_ENABLE(1);
> +
> +	/* Notify all contexts about the change. */
> +	r600_dirty_all_framebuffer_states(rscreen);
> +
> +	/* TODO: re-set all sampler views and images, but how? */
> +}
> +
>   static boolean r600_texture_get_handle(struct pipe_screen* screen,
>   				       struct pipe_resource *resource,
>   				       struct winsys_handle *whandle,
> @@ -318,6 +343,13 @@ static boolean r600_texture_get_handle(struct pipe_screen* screen,
>   		res->external_usage = usage;
>
>   		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)
> +				r600_texture_disable_dcc(rscreen, rtex);

Have you considered keeping DCC enabled when the user sets the explicit 
flush flag and having flush_resource decompress for writably-shared 
resources?

Thinking about this brings up a more general question about the intended 
semantics of the explicit flush flag. If process A creates and exports a 
texture with explicit flush and process B imports it, is process B 
expected to explicitly flush the texture for *its* changes to the 
texture to become visible in program A?

If I understand your current implementation correctly, changes in 
process B do *not* currently need explicit flush (because DCC is 
disabled and process B will never allocate a CMASK). The question is 
whether this is just a happy coincidence of the current implementation 
or whether it is actually something we want to promise for the future.

I'd prefer the first interpretation, i.e. that we expect program B to 
also explicitly flush, because that keeps the door more open for sharing 
a writable texture with DCC enabled, but I'm not sure what the 
user-facing APIs say about this topic (or whether there is even a plan 
to combine explicit flush with write usage).

Anyway, I don't think any of these questions block this change.

Cheers,
Nicolai

> +
>   			if (!(usage & PIPE_HANDLE_USAGE_EXPLICIT_FLUSH)) {
>   				/* Eliminate fast clear (both CMASK and DCC) */
>   				r600_eliminate_fast_color_clear(rscreen, rtex);
> @@ -328,6 +360,7 @@ static boolean r600_texture_get_handle(struct pipe_screen* screen,
>   				r600_texture_disable_cmask(rscreen, rtex);
>   			}
>
> +			/* Set metadata. */
>   			r600_texture_init_metadata(rtex, &metadata);
>   			rscreen->ws->buffer_set_metadata(res->buf, &metadata);
>   		}
> diff --git a/src/gallium/drivers/radeonsi/si_blit.c b/src/gallium/drivers/radeonsi/si_blit.c
> index 198f57d..d9c4f8f 100644
> --- a/src/gallium/drivers/radeonsi/si_blit.c
> +++ b/src/gallium/drivers/radeonsi/si_blit.c
> @@ -777,6 +777,17 @@ static void si_flush_resource(struct pipe_context *ctx,
>   	}
>   }
>
> +static void si_decompress_dcc(struct pipe_context *ctx,
> +			      struct r600_texture *rtex)
> +{
> +	if (!rtex->dcc_offset)
> +		return;
> +
> +	si_blit_decompress_color(ctx, rtex, 0, rtex->resource.b.b.last_level,
> +				 0, util_max_layer(&rtex->resource.b.b, 0),
> +				 true);
> +}
> +
>   static void si_pipe_clear_buffer(struct pipe_context *ctx,
>   				 struct pipe_resource *dst,
>   				 unsigned offset, unsigned size,
> @@ -846,4 +857,5 @@ void si_init_blit_functions(struct si_context *sctx)
>   	sctx->b.b.blit = si_blit;
>   	sctx->b.b.flush_resource = si_flush_resource;
>   	sctx->b.blit_decompress_depth = si_blit_decompress_depth;
> +	sctx->b.decompress_dcc = si_decompress_dcc;
>   }
>