[Mesa-dev] [PATCH v3] i965: intel_texture_barrier reimplemented

[Alejandro Piñeiro]

Hi, today I was updating the piglit test, as I got a detailed review.
While testing the changes, I found that there are some parameters
combinations that are still not passing. They are related to the number
of draw passes (or in other words, the number of glDrawElements calls
for the same vertices). So I added more combinations on the piglit
patch. Before it was testing with 1 and 8 drawing passes. But I added
the cases of 2, 3 and 4 drawing passes.

So:
  * v3 version of the piglit patch includes 48 parameter combinations of
the same test. All passing with current master.
  * v5 version (the one committed [1]) includes 144 parameter
combinations. 40 are failing.

Just as a quick test, I tried the old version of the mesa fix,
consisting on a combination of gen7_emit_cs_stall_flush +
brw_emit_mi_flush (that was called as a "big hammer"), and still have 40
subtests not passing.

I will start to look to this problem, but any idea/feedback to get it
solved would be welcome.

BR

[1]
https://cgit.freedesktop.org/piglit/commit/?id=8cb75a1bc1cbdec0e0c8f9a9b10631a369df2f5b

On 30/06/16 23:16, Francisco Jerez wrote:
> Alejandro Piñeiro <apinheiro at igalia.com> writes:
>
>> 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).
>>
>> On gen6 this test was already working without this change. It keeps
>> working after it.
>>
>> This commit replaces the call to brw_emit_mi_flush for gen6+ with two
>> calls to brw_emit_pipe_control_flush:
>>
>>  * The first one with RENDER_TARGET_FLUSH and CS_STALL set to initiate
>>    a render cache flush after any concurrent rendering completes and
>>    cause the CS to stop parsing commands until the render cache
>>    becomes coherent with memory.
>>
>>  * The second one have TEXTURE_CACHE_INVALIDATE set (and no CS stall)
>>    to clean up any stale data from the sampler caches before rendering
>>    continues.
>>
>> Didn't touch gen4-5, basically because I don't have a way to test
>> them.
>>
>> More info on commits:
>> 0aa4f99f562a05880a779707cbcd46be459863bf
>> 72473658c51d5e074ce219c1e6385a4cce29f467
>>
>> Thanks to Curro to help to tracking this down, as the root case was a
>> hw race condition.
>>
>> v2: use two calls to pipe_control_flush instead of a combination of
>>     gen7_emit_cs_stall_flush and brw_emit_mi_flush calls (Curro)
>> v3: no need to const cache invalidation (Curro)
>> ---
>>
>> FWIW: checked with the CTS tests, and the piglit series, and confirmed
>> that the const cache invalidation is not needed.
>>
>>  src/mesa/drivers/dri/i965/intel_tex.c | 21 ++++++++++++++++++++-
>>  1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/intel_tex.c b/src/mesa/drivers/dri/i965/intel_tex.c
>> index cac33ac..a802d5a 100644
>> --- a/src/mesa/drivers/dri/i965/intel_tex.c
>> +++ b/src/mesa/drivers/dri/i965/intel_tex.c
>> @@ -9,6 +9,7 @@
>>  #include "intel_mipmap_tree.h"
>>  #include "intel_tex.h"
>>  #include "intel_fbo.h"
>> +#include "intel_reg.h"
>>  
>>  #define FILE_DEBUG_FLAG DEBUG_TEXTURE
>>  
>> @@ -362,7 +363,25 @@ intel_texture_barrier(struct gl_context *ctx)
>>  {
>>     struct brw_context *brw = brw_context(ctx);
>>  
>> -   brw_emit_mi_flush(brw);
>> +   if (brw->gen >= 6) {
>> +      if (brw->gen == 6) {
>> +         /* [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);
>> +      }
>> +
>> +      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);
>> +   } else {
>> +      brw_emit_mi_flush(brw);
>> +   }
> Looks reasonable to me, let's get this bug fixed on master, things can
> be refactored later on:
>
> Reviewed-by: Francisco Jerez <currojerez at riseup.net>
>
>>  }
>>  
>>  void
>> -- 
>> 2.7.4


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160708/0b22fa39/attachment.sig>