Bug 105351

Here's a more detailed output of the failing subtests:

Probe value at (0, 0, 0, 0)
  Expected: 0.000000 1.000000 0.000000 1.000000
  Observed: 1.000000 0.000000 0.000000 1.000000
PIGLIT: {"subtest": {"Buffer update/WaW/one bit barrier test/4x4" : "fail"}}
--
Probe value at (0, 0, 0, 0)
  Expected: 0.000000 1.000000 0.000000 1.000000
  Observed: 1.000000 0.000000 0.000000 1.000000
PIGLIT: {"subtest": {"Buffer update/WaW/full barrier test/4x4" : "fail"}}
--
Probe value at (0, 0, 0, 0)
  Expected: 0.000000 1.000000 0.000000 1.000000
  Observed: 1.000000 0.000000 0.000000 1.000000
PIGLIT: {"subtest": {"Buffer update/WaW/one bit barrier test/16x16" : "fail"}}
--
Probe value at (0, 0, 0, 0)
  Expected: 0.000000 1.000000 0.000000 1.000000
  Observed: 1.000000 0.000000 0.000000 1.000000
PIGLIT: {"subtest": {"Buffer update/WaW/full barrier test/16x16" : "fail"}}
--
Probe value at (0, 0, 0, 0)
  Expected: 0.000000 1.000000 0.000000 1.000000
  Observed: 1.000000 0.000000 0.000000 1.000000
PIGLIT: {"subtest": {"Buffer update/WaW/one bit barrier test/64x64" : "fail"}}
--
Probe value at (0, 0, 0, 0)
  Expected: 0.000000 1.000000 0.000000 1.000000
  Observed: 1.000000 0.000000 0.000000 1.000000
PIGLIT: {"subtest": {"Buffer update/WaW/full barrier test/64x64" : "fail"}}

I can reproduce this issue with mesa-11.0.0-rc1 and mesa-18.0.0-rc4 on both Skylake and Haswell.

I doubt this is an image load/store bug, the failing subtest isn't calling glGetTexImage on any surface which is ever accessed using image load/store.  It's doing the following:

 - Initialize a buffer object using imageStore() from the fragment shader.
 - Overwrite half of the same buffer with green using glBufferSubData().
 - Copy the buffer into a 2D texture by binding it as a PBO and calling
   glTexImage2D().
 - Download the texture using glGetTexImage() [which with your hack is somehow
   getting stale contents from the above].

Kind of smells like a coherency issue of glGetTexImage() with respect to the PBO upload path...

Actually intel_get_tex_sub_image() is called 3 times: 2 times during test initialization (source datas are float 0 1 0 1 and all floats 66) and once when checking the results (source data is half red - half green).
Destination addresses for these 3 calls all different.

Modifying test to do Write-Read-Write (instead of Write-Write) solving the issue.
Write-Write-Write still fails.

P.S. these results are for "fallback" path with intel_gettexsubimage_blorp returning false.

(In reply to Andriy Khulap from comment #4)
> Actually intel_get_tex_sub_image() is called 3 times: 2 times during test
> initialization (source datas are float 0 1 0 1 and all floats 66) and once
> when checking the results (source data is half red - half green).
> Destination addresses for these 3 calls all different.
> 
Yeah, but the textures downloaded by those are temporaries which aren't accessed by image load/store either.

> Modifying test to do Write-Read-Write (instead of Write-Write) solving the
> issue.
> Write-Write-Write still fails.
> 
> P.S. these results are for "fallback" path with intel_gettexsubimage_blorp
> returning false.

From playing around with the test a bit I believe the corruption is caused by glBufferSubData() unexpectedly taking the unsynchronized path when intel_gettexsubimage_blorp() hasn't been used earlier -- Other than that the glGetTexSubImage path taken doesn't seem to have any influence on the result.  The reason why glBufferSubData() is taking the unsynchronized path as a side effect even though the BO is busy is because image surface setup isn't using the intel_bufferobj_buffer() wrapper to get the BO pointer, which updates intel_buffer_object::gpu_active_start/end used by glBufferSubData() to decide whether to take the unsynchronized path.  The attached patch should address that (along with a few more issues I found while looking into this).

Created attachment 138163 [details] [review]
i965-tbo-assorted-fixes.patch

I've tested the patch and can confirm that it fixes this issue: all "Buffer update/WaW..." tests are now passing.

The fix was released. Can we close this?

(In reply to Juan A. Suarez from comment #8)
> The fix was released. Can we close this?

Yes.

The bug has been fixed as of

commit 936cd3c87a212c28fe89a5c059fc4febd8b52ab7
Author: Francisco Jerez <currojerez@riseup.net>
Date:   Fri Mar 16 14:28:59 2018 -0700

    i965: Use intel_bufferobj_buffer() wrapper in image surface state setup.
    
    Instead of directly using intel_obj->buffer.  Among other things
    intel_bufferobj_buffer() will update intel_buffer_object::
    gpu_active_start/end, which are used by glBufferSubData() to decide
    which path to take.  Fixes a failure in the Piglit
    ARB_shader_image_load_store-host-mem-barrier Buffer Update/WaW tests,
    which could be reproduced with a non-standard glGetTexSubImage
    implementation (see bug report).
    
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105351
    Reported-by: Nanley Chery <nanleychery@gmail.com>
    Cc: mesa-stable@lists.freedesktop.org
    Reviewed-by: Nanley Chery <nanley.g.chery@intel.com>