[Mesa-dev] [PATCH] i965: adds gen7_emit_cs_stall_flush on intel_texture_barrier

[Ilia Mirkin]

On Tue, Jun 28, 2016 at 11:46 AM, Alejandro Piñeiro
<apinheiro at igalia.com> wrote:
> Fixes:
> GL44-CTS.texture_barrier_ARB.same-texel-rw-multipass
>
> On Haswell, Broadwell and Skylake (note that in order to execute
> that test, it is needed to override GL and GLSL versions).
>
> I was not able to find a documentation reference that justifies it.
> ---
>
> Having said, I didn't find a documentation reference explicitly
> mention that this is needed.
>
> Initially I thought that a flag was missing when calling
> emit_pipe_control_flush at brw_emit_mi_flush, but it was not the case
> as far as I saw.  Then I noted that there is a gen6 workaround on that
> code:
>
>          if (brw->gen == 6) {
>             /* Hardware workaround: SNB B-Spec says:
>              *
>              * [Dev-SNB{W/A}]: Before a PIPE_CONTROL with Write Cache
>              * Flush Enable =1, a PIPE_CONTROL with any non-zero
>              * post-sync-op is required.
>              */
>             brw_emit_post_sync_nonzero_flush(brw);
>          }
>
> I tested calling that method for any gen, guessing if the workaround
> was needed also for other gens, and the test got fixed. But looking at
> the documentation of other gens, I didn't find the need for this
> workaround. For that reason I moved to use gen7_emit_cs_stall, that is
> less agressive and get the test fixed too. It seems that in order to
> get a complete flush you need a cs stall flush with a
> pipe_control_write. But again, I didn't find any reference at the PRMs
> confirming it.
>
> Intuitively, this would be needed on brw_emit_mi_flush or even at
> brw_emit_pipe_control_flush (this one already include some
> gen-specific workarounds), but I prefered to keep it on the only place
> that seems to need it for now.
>
> In addition to solve that CTS test, it also gets it passing for the
> test I recently sent to the piglit list, and not included on master
> yet (acked for now):
> https://lists.freedesktop.org/archives/piglit/2016-June/020055.html
>
> That piglit patch adds 48 parameter combination for the basic
> test. Without this mesa patch 5-6 subtests fails. With this patch all
> of them passes. Tested on Haswell, Broadwell and Skylake too.
>
>  src/mesa/drivers/dri/i965/intel_tex.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/src/mesa/drivers/dri/i965/intel_tex.c b/src/mesa/drivers/dri/i965/intel_tex.c
> index cac33ac..e7459cd 100644
> --- a/src/mesa/drivers/dri/i965/intel_tex.c
> +++ b/src/mesa/drivers/dri/i965/intel_tex.c
> @@ -362,6 +362,7 @@ intel_texture_barrier(struct gl_context *ctx)
>  {
>     struct brw_context *brw = brw_context(ctx);
>
> +   gen7_emit_cs_stall_flush(brw);
>     brw_emit_mi_flush(brw);

Without commenting on exactly what these do, what texture barrier *should* do is

(1) wait for all previous draws to complete (since they may be in the
process of filling caches with "old" data)
(2) flush texture caches

If you flush caches without waiting first, then a draw currently in
progress may continue dirtying them with the "bad" data.

As I said, however, I have no idea what either of the above functions
*really* do, or what forms of parallelism are possible on intel hw.
Hopefully the above comments will help someone with the proper
knowledge evaluate whether this or a different change is necessary.

  -ilia