[Mesa-dev] [PATCH 4/4] i965/blorp: Make post draw flush more explicit

[Pohjolainen, Topi]

On Thu, Jan 19, 2017 at 09:25:19AM +0200, Pohjolainen, Topi wrote:
> On Wed, Jan 18, 2017 at 01:54:43PM -0800, Jason Ekstrand wrote:
> >    On Jan 18, 2017 1:47 PM, "Francisco Jerez" <[1]currojerez at riseup.net>
> >    wrote:
> > 
> >    Topi Pohjolainen <[2]topi.pohjolainen at gmail.com> writes:
> >    > Blits do not need any special treatment as the target buffer
> >    > object is added to render cache just as one does for normal draw.
> >    > Color clears and resolves in turn require explicit "end of pipe
> >    > synchronization". It is not clear what this means exactly but the
> >    > assumption is that render cache flush with command stream stall
> >    > should be sufficient.
> >    >
> > 
> >      Don't the clear and resolve paths end up calling genX(blorp_exec),
> >      where
> >      you used the render cache mechanism to flush a superset of the
> >      caches
> >      flushed below?  Why do you need to synchronize and flush twice?
> > 
> >    Thanks.  I recall having that same thought but apparently forgot it.  I
> >    think we may still bed the resolve flush (there are extra flushes
> >    required around resolves) but not the color clear flush.
> 
> I was hesitant to drop them as the spec specifically chooses to point them
> out (see below). Otherwise they wouldn't be any different than normal renders
> which require render target flush before using the buffers again, right?
> 
> 
> Anyway, I tried dropping both flushes, the one after clear and the one
> after resolve. Jenkins gives me some 16k regressions. Jason, you say that
> resolves need the extra flushes but not clears. Is there something more in the
> specs than the IVB quote I have below? I'm going to check what jenkins says
> about dropping the flush after clear alone.

Dropping the flush only in case of clears but leaving it in place for resolves
gives me 15k regressions.

> 
> > 
> >    > Signed-off-by: Topi Pohjolainen <[3]topi.pohjolainen at intel.com>
> >    > ---
> >    >  src/mesa/drivers/dri/i965/brw_blorp.c       | 22
> >    ++++++++++++++++++++++
> >    >  src/mesa/drivers/dri/i965/genX_blorp_exec.c |  5 -----
> >    >  2 files changed, 22 insertions(+), 5 deletions(-)
> >    >
> >    > diff --git a/src/mesa/drivers/dri/i965/brw_blorp.c
> >    b/src/mesa/drivers/dri/i965/brw_blorp.c
> >    > index 8d58616..845abe3 100644
> >    > --- a/src/mesa/drivers/dri/i965/brw_blorp.c
> >    > +++ b/src/mesa/drivers/dri/i965/brw_blorp.c
> >    > @@ -908,6 +908,17 @@ do_single_blorp_clear(struct brw_context *brw,
> >    struct gl_framebuffer *fb,
> >    >        blorp_batch_finish(&batch);
> >    >     }
> >    >
> >    > +   /*
> >    > +    * IvyBrigde PRM Vol 2, Part 1, "11.7 MCS Buffer for Render
> >    Target(s)":
> >    > +    *
> >    > +    *  Any transition from any value in {Clear, Render, Resolve} to
> >    a
> >    > +    *  different value in {Clear, Render, Resolve} requires end of
> >    pipe
> >    > +    *  synchronization.
> >    > +    */
> >    > +   brw_emit_pipe_control_flush(brw,
> >    > +                               PIPE_CONTROL_RENDER_TARGET_FLUSH |
> >    > +                               PIPE_CONTROL_CS_STALL);
> >    > +
> >    >     return true;
> >    >  }
> >    >
> >    > @@ -975,6 +986,17 @@ brw_blorp_resolve_color(struct brw_context *brw,
> >    struct intel_mipmap_tree *mt,
> >    >                       brw_blorp_to_isl_format(brw, format, true),
> >    >                       resolve_op);
> >    >     blorp_batch_finish(&batch);
> >    > +
> >    > +   /*
> >    > +    * IvyBrigde PRM Vol 2, Part 1, "11.7 MCS Buffer for Render
> >    Target(s)":
> >    > +    *
> >    > +    *  Any transition from any value in {Clear, Render, Resolve} to
> >    a
> >    > +    *  different value in {Clear, Render, Resolve} requires end of
> >    pipe
> >    > +    *  synchronization.
> >    > +    */
> >    > +   brw_emit_pipe_control_flush(brw,
> >    > +                               PIPE_CONTROL_RENDER_TARGET_FLUSH |
> >    > +                               PIPE_CONTROL_CS_STALL);
> >    >  }
> >    >
> >    >  static void
> >    > diff --git a/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> >    b/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> >    > index c0cbde5..2c53444 100644
> >    > --- a/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> >    > +++ b/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> >    > @@ -260,9 +260,4 @@ retry:
> >    >     brw->ib.type = -1;
> >    >
> >    >     brw_render_cache_set_add_bo(brw, params->dst.addr.buffer);
> >    > -
> >    > -   /* Flush the sampler cache so any texturing from the destination
> >    is
> >    > -    * coherent.
> >    > -    */
> >    > -   brw_emit_mi_flush(brw);
> >    >  }
> >    > --
> >    > 2.5.5
> >    >
> >    > _______________________________________________
> >    > mesa-dev mailing list
> >    > [4]mesa-dev at lists.freedesktop.org
> >    > [5]https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > 
> >      _______________________________________________
> >      mesa-dev mailing list
> >      [6]mesa-dev at lists.freedesktop.org
> >      [7]https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > 
> > References
> > 
> >    1. mailto:currojerez at riseup.net
> >    2. mailto:topi.pohjolainen at gmail.com
> >    3. mailto:topi.pohjolainen at intel.com
> >    4. mailto:mesa-dev at lists.freedesktop.org
> >    5. https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >    6. mailto:mesa-dev at lists.freedesktop.org
> >    7. https://lists.freedesktop.org/mailman/listinfo/mesa-dev