[Mesa-dev] [v2 1/4] i965/blorp: Use the render cache mechanism instead of explicit flushing

[Pohjolainen, Topi]

On Wed, Jan 18, 2017 at 01:29:41PM -0800, Francisco Jerez wrote:
> Topi Pohjolainen <topi.pohjolainen at gmail.com> 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 <topi.pohjolainen at intel.com>
> > ---
> >  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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev