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

[Pohjolainen, Topi]

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.

> 
>    > 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