Bug 105351 - [Gen6+] piglit's arb_shader_image_load_store-host-mem-barrier fails with a glGetTexSubImage fallback path
Summary: [Gen6+] piglit's arb_shader_image_load_store-host-mem-barrier fails with a gl...
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/DRI/i965 (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: Intel 3D Bugs Mailing List
QA Contact: Intel 3D Bugs Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-03-05 21:10 UTC by Nanley Chery
Modified: 2018-06-04 20:55 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
i965-tbo-assorted-fixes.patch (6.36 KB, patch)
2018-03-17 00:50 UTC, Francisco Jerez
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nanley Chery 2018-03-05 21:10:57 UTC
Test case:
1. Modify intel_gettexsubimage_blorp to return false.
2. Execute ./bin/arb_shader_image_load_store-host-mem-barrier --quick -auto -fbo

Expected result: Pass
Actual result: 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
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
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
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
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
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
Comment 1 Nanley Chery 2018-03-08 21:05:49 UTC
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"}}
Comment 2 Andriy Khulap 2018-03-15 15:23:32 UTC
I can reproduce this issue with mesa-11.0.0-rc1 and mesa-18.0.0-rc4 on both Skylake and Haswell.
Comment 3 Francisco Jerez 2018-03-15 23:20:26 UTC
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...
Comment 4 Andriy Khulap 2018-03-16 11:28:18 UTC
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.
Comment 5 Francisco Jerez 2018-03-17 00:48:57 UTC
(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).
Comment 6 Francisco Jerez 2018-03-17 00:50:01 UTC
Created attachment 138163 [details] [review]
i965-tbo-assorted-fixes.patch
Comment 7 Andriy Khulap 2018-03-19 09:41:31 UTC
I've tested the patch and can confirm that it fixes this issue: all "Buffer update/WaW..." tests are now passing.
Comment 8 Juan A. Suarez 2018-06-03 10:17:43 UTC
The fix was released. Can we close this?
Comment 9 Nanley Chery 2018-06-04 20:55:22 UTC
(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>


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.