Hey Ilia, Marek, Do you have an opinion about this? I've got a R-b from Eric Anholt and what sounds like an Ack from Roland, but I wanted to make sure everyone was OK with this before landing it.
--Ken
On Wednesday, March 6, 2019 12:32:23 AM PST Kenneth Graunke wrote:
> The glMemoryBarrier() function makes shader memory stores ordered with
> respect to things specified by the given bits. Until now, st/mesa has
> ignored GL_TEXTURE_UPDATE_BARRIER_BIT and GL_BUFFER_UPDATE_BARRIER_BIT,
> saying that drivers should implicitly perform the needed flushing.
>
> This seems like a pretty big assumption to make. Instead, this commit
> opts to translate them to new PIPE_BARRIER bits, and adjusts existing
> drivers to continue ignoring them (preserving the current behavior).
>
> The i965 driver performs actions on these memory barriers. Shader
> memory stores go through a "data cache" which is separate from the
> render cache and other read caches (like the texture cache). All
> memory barriers need to flush the data cache (to ensure shader memory
> stores are visible), and possibly invalidate read caches (to ensure
> stale data is no longer visible). The driver implicitly flushes for
> most caches, but not for data cache, since ARB_shader_image_load_store
> introduced MemoryBarrier() precisely to order these explicitly.
>
> I would like to follow i965's approach in iris, flushing the data cache
> on any MemoryBarrier() call, so I need st/mesa to actually call the
> pipe->memory_barrier() callback.
> ---
> .../drivers/freedreno/freedreno_context.c | 3 ++
> src/gallium/drivers/r600/r600_state_common.c | 4 +++
> src/gallium/drivers/radeonsi/si_state.c | 3 ++
> src/gallium/drivers/softpipe/sp_flush.c | 3 ++
> src/gallium/drivers/tegra/tegra_context.c | 3 ++
> src/gallium/drivers/v3d/v3d_context.c | 3 ++
> src/gallium/include/pipe/p_defines.h | 7 +++-
> src/mesa/state_tracker/st_cb_texturebarrier.c | 34 +++++++++++--------
> 8 files changed, 44 insertions(+), 16 deletions(-)
>
> I am curious to hear people's thoughts on this. It seems useful for the
> driver to receive a memory_barrier() call...and adding a few bits seemed
> to be the cleanest way to make that happen. But I'm open to ideas.
>
> There are no nouveau changes in this patch, but that's only because none
> appeared to be necessary. Most drivers performed some kind of flush on
> any memory_barrier() call, regardless of the bits - but nouveau's hooks
> don't. So the newly added case would already be a no-op.
>
> diff --git a/src/gallium/drivers/freedreno/freedreno_context.c
> b/src/gallium/drivers/freedreno/freedreno_context.c
> index 6c01c15bb32..4e86d099974 100644
> --- a/src/gallium/drivers/freedreno/freedreno_context.c
> +++ b/src/gallium/drivers/freedreno/freedreno_context.c
> @@ -99,6 +99,9 @@ fd_texture_barrier(struct pipe_context *pctx, unsigned
> flags)
> static void
> fd_memory_barrier(struct pipe_context *pctx, unsigned flags)
> {
> + if (!(flags & ~PIPE_BARRIER_UPDATE))
> + return;
> +
> fd_context_flush(pctx, NULL, 0);
> /* TODO do we need to check for persistently mapped buffers and
> fd_bo_cpu_prep()?? */
> }
> diff --git a/src/gallium/drivers/r600/r600_state_common.c
> b/src/gallium/drivers/r600/r600_state_common.c
> index f886a27170d..c7c606f131b 100644
> --- a/src/gallium/drivers/r600/r600_state_common.c
> +++ b/src/gallium/drivers/r600/r600_state_common.c
> @@ -94,6 +94,10 @@ void r600_emit_alphatest_state(struct r600_context *rctx,
> struct r600_atom *atom
> static void r600_memory_barrier(struct pipe_context *ctx, unsigned flags)
> {
> struct r600_context *rctx = (struct r600_context *)ctx;
> +
> + if (!(flags & ~PIPE_BARRIER_UPDATE))
> + return;
> +
> if (flags & PIPE_BARRIER_CONSTANT_BUFFER)
> rctx->b.flags |= R600_CONTEXT_INV_CONST_CACHE;
>
> diff --git a/src/gallium/drivers/radeonsi/si_state.c
> b/src/gallium/drivers/radeonsi/si_state.c
> index 458b108a7e3..3c29b4c92ed 100644
> --- a/src/gallium/drivers/radeonsi/si_state.c
> +++ b/src/gallium/drivers/radeonsi/si_state.c
> @@ -4710,6 +4710,9 @@ void si_memory_barrier(struct pipe_context *ctx,
> unsigned flags)
> {
> struct si_context *sctx = (struct si_context *)ctx;
>
> + if (!(flags & ~PIPE_BARRIER_UPDATE))
> + return;
> +
> /* Subsequent commands must wait for all shader invocations to
> * complete. */
> sctx->flags |= SI_CONTEXT_PS_PARTIAL_FLUSH |
> diff --git a/src/gallium/drivers/softpipe/sp_flush.c
> b/src/gallium/drivers/softpipe/sp_flush.c
> index 3bf8c499218..5eccbe34d46 100644
> --- a/src/gallium/drivers/softpipe/sp_flush.c
> +++ b/src/gallium/drivers/softpipe/sp_flush.c
> @@ -192,5 +192,8 @@ void softpipe_texture_barrier(struct pipe_context *pipe,
> unsigned flags)
>
> void softpipe_memory_barrier(struct pipe_context *pipe, unsigned flags)
> {
> + if (!(flags & ~PIPE_BARRIER_UPDATE))
> + return;
> +
> softpipe_texture_barrier(pipe, 0);
> }
> diff --git a/src/gallium/drivers/tegra/tegra_context.c
> b/src/gallium/drivers/tegra/tegra_context.c
> index bbc03628336..e9e51656921 100644
> --- a/src/gallium/drivers/tegra/tegra_context.c
> +++ b/src/gallium/drivers/tegra/tegra_context.c
> @@ -974,6 +974,9 @@ tegra_memory_barrier(struct pipe_context *pcontext,
> unsigned int flags)
> {
> struct tegra_context *context = to_tegra_context(pcontext);
>
> + if (!(flags & ~PIPE_BARRIER_UPDATE))
> + return;
> +
> context->gpu->memory_barrier(context->gpu, flags);
> }
>
> diff --git a/src/gallium/drivers/v3d/v3d_context.c
> b/src/gallium/drivers/v3d/v3d_context.c
> index d07ad403590..fcd2d5ec69b 100644
> --- a/src/gallium/drivers/v3d/v3d_context.c
> +++ b/src/gallium/drivers/v3d/v3d_context.c
> @@ -71,6 +71,9 @@ v3d_memory_barrier(struct pipe_context *pctx, unsigned int
> flags)
> {
> struct v3d_context *v3d = v3d_context(pctx);
>
> + if (!(flags & ~PIPE_BARRIER_UPDATE))
> + return;
> +
> /* We only need to flush jobs writing to SSBOs/images. */
> perf_debug("Flushing all jobs for glMemoryBarrier(), could do
> better");
> v3d_flush(pctx);
> diff --git a/src/gallium/include/pipe/p_defines.h
> b/src/gallium/include/pipe/p_defines.h
> index e2b0104ce43..2970c47c3c7 100644
> --- a/src/gallium/include/pipe/p_defines.h
> +++ b/src/gallium/include/pipe/p_defines.h
> @@ -425,7 +425,12 @@ enum pipe_flush_flags
> #define PIPE_BARRIER_FRAMEBUFFER (1 << 9)
> #define PIPE_BARRIER_STREAMOUT_BUFFER (1 << 10)
> #define PIPE_BARRIER_GLOBAL_BUFFER (1 << 11)
> -#define PIPE_BARRIER_ALL ((1 << 12) - 1)
> +#define PIPE_BARRIER_UPDATE_BUFFER (1 << 12)
> +#define PIPE_BARRIER_UPDATE_TEXTURE (1 << 13)
> +#define PIPE_BARRIER_ALL ((1 << 14) - 1)
> +
> +#define PIPE_BARRIER_UPDATE \
> + (PIPE_BARRIER_UPDATE_BUFFER | PIPE_BARRIER_UPDATE_TEXTURE)
>
> /**
> * Flags for pipe_context::texture_barrier.
> diff --git a/src/mesa/state_tracker/st_cb_texturebarrier.c
> b/src/mesa/state_tracker/st_cb_texturebarrier.c
> index 2bff03b484a..4a9c72f2c62 100644
> --- a/src/mesa/state_tracker/st_cb_texturebarrier.c
> +++ b/src/mesa/state_tracker/st_cb_texturebarrier.c
> @@ -95,21 +95,25 @@ st_MemoryBarrier(struct gl_context *ctx, GLbitfield
> barriers)
> */
> flags |= PIPE_BARRIER_TEXTURE;
> }
> - /* GL_TEXTURE_UPDATE_BARRIER_BIT:
> - * Texture updates translate to:
> - * (1) texture transfers to/from the CPU,
> - * (2) texture as blit destination, or
> - * (3) texture as framebuffer.
> - * In all cases, we assume the driver does the required flushing
> - * automatically.
> - */
> - /* GL_BUFFER_UPDATE_BARRIER_BIT:
> - * Buffer updates translate to
> - * (1) buffer transfers to/from the CPU,
> - * (2) resource copies and clears.
> - * In all cases, we assume the driver does the required flushing
> - * automatically.
> - */
> + if (barriers & GL_TEXTURE_UPDATE_BARRIER_BIT) {
> + /* GL_TEXTURE_UPDATE_BARRIER_BIT:
> + * Texture updates translate to:
> + * (1) texture transfers to/from the CPU,
> + * (2) texture as blit destination, or
> + * (3) texture as framebuffer.
> + * Some drivers may handle these automatically, and can ignore the bit.
> + */
> + flags |= PIPE_BARRIER_UPDATE_TEXTURE;
> + }
> + if (barriers & GL_BUFFER_UPDATE_BARRIER_BIT) {
> + /* GL_BUFFER_UPDATE_BARRIER_BIT:
> + * Buffer updates translate to
> + * (1) buffer transfers to/from the CPU,
> + * (2) resource copies and clears.
> + * Some drivers may handle these automatically, and can ignore the bit.
> + */
> + flags |= PIPE_BARRIER_UPDATE_BUFFER;
> + }
> if (barriers & GL_CLIENT_MAPPED_BUFFER_BARRIER_BIT)
> flags |= PIPE_BARRIER_MAPPED_BUFFER;
> if (barriers & GL_QUERY_BUFFER_BARRIER_BIT)
>
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
