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

[Alejandro Piñeiro]

Forget the previous email. It was an error on the test. The texture
barrier works fine.

Sorry for the noise.

BR

On 08/07/16 15:58, Alejandro Piñeiro wrote:
> 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/4d1cbb79/attachment.sig>