[Mesa-dev] [Bug 104553] mat4: m[i][j] incorrect result with row_major UBO

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=104553

Alejandro Piñeiro (freenode IRC: apinheiro) <apinheiro at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |apinheiro at igalia.com

--- Comment #9 from Alejandro Piñeiro (freenode IRC: apinheiro) <apinheiro at igalia.com> ---
Found this bug when I was about to report the same problem. Some comments
below:

(In reply to florian.will from comment #0)
> Created attachment 136630 [details]
> Failing piglit test (when test_variant = 2) for this bug.
> 
> I hit another bug while trying to get Banshee 3D
> <https://github.com/BearishSun/BansheeEngine> to work correctly on Mesa
> radeonsi (HD 7870) / amdgpu kernel module. I use git commit 2719467eb6 right
> now, which is a few days old.
> 
> Accessing a "mat4 m" component, e.g. m[1][2], returns unexpected results if
> m is declared inside a UBO block and uses row_major matrix format. col_major
> works as expected. m[1].z also works. Some experiments indicate that for
> m[i][j], the UBO buffer is accessed at offset (i+j)*4 instead of (i+j*4)*4.
> 
> The invalid offsets for loading floats from the UBO are visible in the Mesa
> IR when linker.cpp is done (probably introduced by lower_ubo_reference()).

As you imply below, this problem also affects ssbo. So perhaps it would be a
good idea to update the bug description?

> While exploring this issue, I prepared a piglit test case that tests for
> this. It fails on my setup (when test_variant = 2, the other tests succeed).
> I will attach it to this bug report and send it to the piglit list for
> review.
> 
> 
> 
> Possible Fix:
> 
> I doubt this is enough/correct, because I haven't fully grasped the IR
> processing in the glsl compiler and which fields/data types can/can't be
> row_major and the implications, but this simple change in
> lower_buffer_access.cpp 

FWIW, while I was doing a skimming, I also got to lower_buffer_access and
lower_ubo_reference (this one touches both ssbo and ubo), so I agree that the
issue is likely at that code. Not sure if your fix is correct or in the good
path sorry.

(In reply to florian.will from comment #7)
> Created attachment 136878 [details] [review]
> Changes to piglit UBO test generator
> 
> I have now extended the random UBO piglit test generator python script (in a
> hackish way) to generate SSBO tests as well, and added std430 packing rules
> to generate std430 SSBO tests. My changes are in the attached patch file,
> but I'd say it's not suitable for piglit git (too ugly).
> 
> It was helpful to validate the mesa patch I've attached to this bug report
> earlier. Using mesa git master, 391 out of the 540 generated UBO&SSBO tests
> fail. After applying my patch, only a few tests (3-7) fail. The failing
> tests are always very huge test files (some have more than 10k lines and
> sometimes up to 5MB shader_test files). Apparently they hit something like
> an internal size limit for vertex shaders, because the tests pass when
> commenting out one half of the test conditions in the vertex shader, and
> they still pass when commenting out the other half of the vertex shader.

Somewhat off-topic: Timothy mentioned that in the past it was not included due
all the amount of tests added. So perhaps a compromise would be added some
(~10?) barebone tests, to at least cover the most basic cases. Something like
this test I wrote while debugging this:
https://github.com/Igalia/piglit/blob/apinheiro/matrix-row-major-failure/tests/spec/arb_fake/execution/ubo/matrix-column-vs-row.shader_test

or in a ideal world, get the script to be configurable on how many tests to
create, and the default being a reasonable amount of tests (fwiw, 540 generated
tests seems somewhat too much).

> So I'm now fairly confident that my patch improves the SSBO / UBO buffer
> access behaviour when reading from SSBOs and UBOs.
> 
> Is there anything else that should be tested? Or any comments about the
> patch by someone who knows the lower_buffer_access code better than I do?

Unfourtunately although I would be interested on working on this, I don't have
the time right now.


And now totally off-topic, but probably it is worth to mention here to not
forget: VK-GL-CTS doesn't catch this problem either. And they have tons of
row_major tests, for example:
KHR-GL45.shaders.uniform_block.single_basic_type.std140.row_major_mediump_mat4

is passing properly. So or the test is wrong or it is incomplete. I tried to
take a look to the test, but it is somewhat hard to understand.
https://github.com/KhronosGroup/VK-GL-CTS/blob/master/external/openglcts/modules/common/glcUniformBlockCase.cpp#L1346

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180222/15233e7e/attachment-0001.html>