Re: [Mesa-dev] [v2 1/4] i965/blorp: Use the render cache mechanism instead of explicit flushing
On Wed, Jan 18, 2017 at 01:29:41PM -0800, Francisco Jerez wrote:
> Topi Pohjolainen writes:
>
> > by replacing brw_emit_mi_flush() with brw_render_cache_set_check_flush().
> > The latter splits the flush in two:
> >
> >brw_emit_pipe_control_flush(brw,
> >PIPE_CONTROL_DEPTH_CACHE_FLUSH |
> >PIPE_CONTROL_RENDER_TARGET_FLUSH |
> >PIPE_CONTROL_CS_STALL);
> >
> >brw_emit_pipe_control_flush(brw,
> >PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE |
> >PIPE_CONTROL_CONST_CACHE_INVALIDATE);
> >
> > instead of
> >
> >int flags = PIPE_CONTROL_NO_WRITE | PIPE_CONTROL_RENDER_TARGET_FLUSH;
> >if (brw->gen >= 6) {
> > flags |= PIPE_CONTROL_INSTRUCTION_INVALIDATE |
> >PIPE_CONTROL_CONST_CACHE_INVALIDATE |
> >PIPE_CONTROL_DEPTH_CACHE_FLUSH |
> >PIPE_CONTROL_VF_CACHE_INVALIDATE |
> >PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE |
> >PIPE_CONTROL_CS_STALL;
> >}
> >brw_emit_pipe_control_flush(brw, flags);
> >
>
> The commit message seems somewhat misleading,
> brw_emit_pipe_control_flush (and therefore also brw_emit_mi_flush) will
> split the flush into two pipe controls in exactly the same way as
> brw_render_cache_set_check_flush, in order to avoid a hardware race
> condition -- The main functional differences here are that you get rid
> of a bunch of RO invalidation bits which are unlikely to be useful, and
> that the flushes happen on-demand before draw time.
I managed to push before seeing your feedback, sorry.
>
> > v2 (Jason): Check that destination exists before trying to add to
> > render cache. Depth clears and resolves don't have it.
> >
> > Signed-off-by: Topi Pohjolainen
> > ---
> > src/mesa/drivers/dri/i965/genX_blorp_exec.c | 8 +++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> > b/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> > index bb1dfa9..b72ecb6 100644
> > --- a/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> > +++ b/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> > @@ -25,6 +25,7 @@
> >
> > #include "intel_batchbuffer.h"
> > #include "intel_mipmap_tree.h"
> > +#include "intel_fbo.h"
> >
> > #include "brw_context.h"
> > #include "brw_state.h"
> > @@ -179,7 +180,9 @@ genX(blorp_exec)(struct blorp_batch *batch,
> > * data with different formats, which blorp does for stencil and depth
> > * data.
> > */
> > - brw_emit_mi_flush(brw);
> > + if (params->src.enabled)
> > + brw_render_cache_set_check_flush(brw, params->src.addr.buffer);
> > + brw_render_cache_set_check_flush(brw, params->dst.addr.buffer);
> >
> > brw_select_pipeline(brw, BRW_RENDER_PIPELINE);
> >
> > @@ -256,6 +259,9 @@ retry:
> > brw->no_depth_or_stencil = false;
> > brw->ib.type = -1;
> >
> > + if (params->dst.enabled)
> > + brw_render_cache_set_add_bo(brw, params->dst.addr.buffer);
> > +
>
> Don't you also need to mark the depth and stencil buffers as pending
> flush here, if either depth or stencil writes were enabled?
You are correct. I'll write a follow-up patch.
>
> > /* Flush the sampler cache so any texturing from the destination is
> > * coherent.
> > */
> > --
> > 2.5.5
> >
> > ___
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [v2 1/4] i965/blorp: Use the render cache mechanism instead of explicit flushing
Topi Pohjolainen writes:
> by replacing brw_emit_mi_flush() with brw_render_cache_set_check_flush().
> The latter splits the flush in two:
>
>brw_emit_pipe_control_flush(brw,
>PIPE_CONTROL_DEPTH_CACHE_FLUSH |
>PIPE_CONTROL_RENDER_TARGET_FLUSH |
>PIPE_CONTROL_CS_STALL);
>
>brw_emit_pipe_control_flush(brw,
>PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE |
>PIPE_CONTROL_CONST_CACHE_INVALIDATE);
>
> instead of
>
>int flags = PIPE_CONTROL_NO_WRITE | PIPE_CONTROL_RENDER_TARGET_FLUSH;
>if (brw->gen >= 6) {
> flags |= PIPE_CONTROL_INSTRUCTION_INVALIDATE |
>PIPE_CONTROL_CONST_CACHE_INVALIDATE |
>PIPE_CONTROL_DEPTH_CACHE_FLUSH |
>PIPE_CONTROL_VF_CACHE_INVALIDATE |
>PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE |
>PIPE_CONTROL_CS_STALL;
>}
>brw_emit_pipe_control_flush(brw, flags);
>
The commit message seems somewhat misleading,
brw_emit_pipe_control_flush (and therefore also brw_emit_mi_flush) will
split the flush into two pipe controls in exactly the same way as
brw_render_cache_set_check_flush, in order to avoid a hardware race
condition -- The main functional differences here are that you get rid
of a bunch of RO invalidation bits which are unlikely to be useful, and
that the flushes happen on-demand before draw time.
> v2 (Jason): Check that destination exists before trying to add to
> render cache. Depth clears and resolves don't have it.
>
> Signed-off-by: Topi Pohjolainen
> ---
> src/mesa/drivers/dri/i965/genX_blorp_exec.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> b/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> index bb1dfa9..b72ecb6 100644
> --- a/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> +++ b/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> @@ -25,6 +25,7 @@
>
> #include "intel_batchbuffer.h"
> #include "intel_mipmap_tree.h"
> +#include "intel_fbo.h"
>
> #include "brw_context.h"
> #include "brw_state.h"
> @@ -179,7 +180,9 @@ genX(blorp_exec)(struct blorp_batch *batch,
> * data with different formats, which blorp does for stencil and depth
> * data.
> */
> - brw_emit_mi_flush(brw);
> + if (params->src.enabled)
> + brw_render_cache_set_check_flush(brw, params->src.addr.buffer);
> + brw_render_cache_set_check_flush(brw, params->dst.addr.buffer);
>
> brw_select_pipeline(brw, BRW_RENDER_PIPELINE);
>
> @@ -256,6 +259,9 @@ retry:
> brw->no_depth_or_stencil = false;
> brw->ib.type = -1;
>
> + if (params->dst.enabled)
> + brw_render_cache_set_add_bo(brw, params->dst.addr.buffer);
> +
Don't you also need to mark the depth and stencil buffers as pending
flush here, if either depth or stencil writes were enabled?
> /* Flush the sampler cache so any texturing from the destination is
> * coherent.
> */
> --
> 2.5.5
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [v2 1/4] i965/blorp: Use the render cache mechanism instead of explicit flushing
Reviewed-by: Jason Ekstrand
On Jan 18, 2017 2:22 AM, "Topi Pohjolainen"
wrote:
> by replacing brw_emit_mi_flush() with brw_render_cache_set_check_flush().
> The latter splits the flush in two:
>
>brw_emit_pipe_control_flush(brw,
>PIPE_CONTROL_DEPTH_CACHE_FLUSH |
>PIPE_CONTROL_RENDER_TARGET_FLUSH |
>PIPE_CONTROL_CS_STALL);
>
>brw_emit_pipe_control_flush(brw,
>PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE |
>PIPE_CONTROL_CONST_CACHE_INVALIDATE);
>
> instead of
>
>int flags = PIPE_CONTROL_NO_WRITE | PIPE_CONTROL_RENDER_TARGET_FLUSH;
>if (brw->gen >= 6) {
> flags |= PIPE_CONTROL_INSTRUCTION_INVALIDATE |
>PIPE_CONTROL_CONST_CACHE_INVALIDATE |
>PIPE_CONTROL_DEPTH_CACHE_FLUSH |
>PIPE_CONTROL_VF_CACHE_INVALIDATE |
>PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE |
>PIPE_CONTROL_CS_STALL;
>}
>brw_emit_pipe_control_flush(brw, flags);
>
> v2 (Jason): Check that destination exists before trying to add to
> render cache. Depth clears and resolves don't have it.
>
> Signed-off-by: Topi Pohjolainen
> ---
> src/mesa/drivers/dri/i965/genX_blorp_exec.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> b/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> index bb1dfa9..b72ecb6 100644
> --- a/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> +++ b/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> @@ -25,6 +25,7 @@
>
> #include "intel_batchbuffer.h"
> #include "intel_mipmap_tree.h"
> +#include "intel_fbo.h"
>
> #include "brw_context.h"
> #include "brw_state.h"
> @@ -179,7 +180,9 @@ genX(blorp_exec)(struct blorp_batch *batch,
> * data with different formats, which blorp does for stencil and depth
> * data.
> */
> - brw_emit_mi_flush(brw);
> + if (params->src.enabled)
> + brw_render_cache_set_check_flush(brw, params->src.addr.buffer);
> + brw_render_cache_set_check_flush(brw, params->dst.addr.buffer);
>
> brw_select_pipeline(brw, BRW_RENDER_PIPELINE);
>
> @@ -256,6 +259,9 @@ retry:
> brw->no_depth_or_stencil = false;
> brw->ib.type = -1;
>
> + if (params->dst.enabled)
> + brw_render_cache_set_add_bo(brw, params->dst.addr.buffer);
> +
> /* Flush the sampler cache so any texturing from the destination is
> * coherent.
> */
> --
> 2.5.5
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
