[Mesa-dev] [Mesa-stable] [PATCH] i965: Fix brw_render_cache_set_check_flush's PIPE_CONTROLs.

[Francisco Jerez]

Kenneth Graunke <kenneth at whitecape.org> writes:

> Our driver uses the brw_render_cache mechanism to track buffers we've
> rendered to and are about to sample from.
>
> Previously, we did a single PIPE_CONTROL with the following bits set:
> - Render Target Flush
> - Depth Cache Flush
> - Texture Cache Invalidate
> - VF Cache Invalidate
> - Instruction Cache Invalidate
> - CS Stall
>
> This combined both "top of pipe" invalidations and "bottom of pipe"
> flushes, which isn't how the hardware is intended to be programmed.
>
> The "top of pipe" invalidations may happen right away, without any
> guarantees that rendering using those caches has completed.  That
> rendering may continue altering the caches.  The "bottom of pipe"
> flushes do wait for the rendering to complete.  The CS stall also
> prevents further work from happening until data is flushed out.
>
> What we wanted to do was wait for rendering complete, flush the new
> data out of the render and depth caches, wait, then invalidate any
> stale data in read-only caches.  We can accomplish this by doing the
> "bottom of pipe" flushes with a CS stall, then the "top of pipe"
> flushes as a second PIPE_CONTROL.  The flushes will wait until the
> rendering is complete, and the CS stall will prevent the second
> PIPE_CONTROL with the invalidations from executing until the first
> is done.
>
> Fixes dEQP-GLES3.functional.texture.specification.teximage2d_pbo
> subtests on Braswell and Skylake.  These tests hit the meta PBO
> texture upload path, which binds the PBO as a texture and samples
> from it, while rendering to the destination texture.  The tests
> then sample from the texture.
>
> For now, we leave Gen4-5 alone.  It probably needs work too, but
> apparently it hasn't even been setting the (G45+) TC invalidation
> bit at all...
>

I think Gen4-5 should be fine, according to the hardware docs a write
cache flush causes an implicit read cache invalidation.  Apparently the
invalidation happens after the completion of the flush but the hardware
docs are not particularly clear about this.  According to the
documentation the Gen5 TC flush bit is ignored if the WC flush bit is
set, the hardware will perform an implicit Sampler/Constant/Vertex cache
flush want it or not -- Luckily it is what we want in this case.

> Cc: mesa-stable at lists.freedesktop.org
> Suggested-by: Francisco Jerez <currojerez at riseup.net>
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/mesa/drivers/dri/i965/brw_pipe_control.c |  2 --
>  src/mesa/drivers/dri/i965/intel_fbo.c        | 15 ++++++++++++++-
>  2 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_pipe_control.c b/src/mesa/drivers/dri/i965/brw_pipe_control.c
> index b41e28e..4672efd 100644
> --- a/src/mesa/drivers/dri/i965/brw_pipe_control.c
> +++ b/src/mesa/drivers/dri/i965/brw_pipe_control.c
> @@ -338,8 +338,6 @@ brw_emit_mi_flush(struct brw_context *brw)
>        }
>        brw_emit_pipe_control_flush(brw, flags);
>     }
> -
> -   brw_render_cache_set_clear(brw);
>  }
>  
>  int
> diff --git a/src/mesa/drivers/dri/i965/intel_fbo.c b/src/mesa/drivers/dri/i965/intel_fbo.c
> index b7b6796..707041a 100644
> --- a/src/mesa/drivers/dri/i965/intel_fbo.c
> +++ b/src/mesa/drivers/dri/i965/intel_fbo.c
> @@ -1065,7 +1065,20 @@ brw_render_cache_set_check_flush(struct brw_context *brw, drm_intel_bo *bo)
>     if (!_mesa_set_search(brw->render_cache, bo))
>        return;
>  
> -   brw_emit_mi_flush(brw);
> +   if (brw->gen >= 6) {

In theory you need to implement a hardware workaround here on SNB:

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

I know you're now doing a non-zero post-sync op unconditionally on SNB
at draw time, but I don't think it has already happened by the time this
is called.  With that sorted out:

Reviewed-by: Francisco Jerez <currojerez at riseup.net>

> +      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);
> +   } else {
> +      brw_emit_mi_flush(brw);
> +   }
> +
> +   brw_render_cache_set_clear(brw);
>  }
>  
>  /**
> -- 
> 2.7.3
>
> _______________________________________________
> mesa-stable mailing list
> mesa-stable at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-stable
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160326/85b8fac6/attachment.sig>