|Summary:||gl_BaseVertex should always be zero when the draw command has no <basevertex> parameter|
|Product:||Mesa||Reporter:||Antia Puentes <apuentes>|
|Component:||Drivers/DRI/i965||Assignee:||Antia Puentes <apuentes>|
|Status:||RESOLVED FIXED||QA Contact:||Intel 3D Bugs Mailing List <intel-3d-bugs>|
|Priority:||medium||CC:||apuentes, idr, kenneth|
|i915 platform:||i915 features:|
|Bug Depends on:|
Description Antia Puentes 2017-09-12 16:16:14 UTC
Hardware: Broadwell or Skylake Mesa commit: f940b1665a7f17ad2ae7ae2e951d90d151482875 vk-gl-cts commit: dfcb8e870438f6f2bfe71d4bb63d43120debb3a3 The following ARB_shader_draw_parameters tests fail: * KHR-GL45.shader_draw_parameters_tests.ShaderDrawArraysParameters * KHR-GL45.shader_draw_parameters_tests.ShaderDrawArraysInstancedParameters * KHR-GL45.shader_draw_parameters_tests.ShaderMultiDrawArraysParameters * KHR-GL45.shader_draw_parameters_tests.ShaderMultiDrawArraysIndirectParameters - Expected outcome: The tests expect gl_BaseVertex to be zero when the draw command does not accept a <basevertex> parameter. From the ARB_shader_draw_parameters specification: "<gl_BaseVertexARB> holds the integer value passed to the <baseVertex> parameter to the command that resulted in the current shader invocation. In the case where the command has no <baseVertex> parameter, the value of <gl_BaseVertexARB> is zero." - Actual outcome: Although non-indexed draw commands like gl*DrawArrays* do not accept <basevertex> parameters, Mesa sets the value of gl_BaseVertex to the value passed to the <first> parameter of the gl*DrawArrays* command instead of setting it to zero. Indexed draw calls like glDrawElements which do not accept a <basevertex> parameter work as expected, gl_BaseVertex is zero in that case.
Comment 1 Kenneth Graunke 2017-10-10 17:54:57 UTC
I believe Antia is working on a fix.
Comment 2 Antia Puentes 2017-10-12 11:12:57 UTC
(In reply to Kenneth Graunke from comment #1) > I believe Antia is working on a fix. I have a patch that fixes the tests. I am currently testing for regressions, if there isn't any I will send it shortly. The patch also fixes: KHR-GL45.shader_draw_parameters_tests.ShaderMultiDrawArraysIndirectCountParameters which was failing for the same reason. This failure was unnoticed when I filled the bug because at the time we were not supporting ARB_indirect_parameters.
Comment 3 Antia Puentes 2017-10-12 18:34:30 UTC
(In reply to Antia Puentes from comment #2) > (In reply to Kenneth Graunke from comment #1) > > I believe Antia is working on a fix. > > I have a patch that fixes the tests. I am currently testing for regressions, > if there isn't any I will send it shortly. The patch produced regressions related to gl_VertexID. Specifically in the tests which involve gl*DrawArrays*, pass a value different from zero as argument of the <first> parameter of the draw command, and check if gl_VertexID is correct. According to the spec, for non-indexed draw calls: "The vertex ID of the ith element transferred is <first> + i." Mesa's compiler lowers the gl_VertexID as gl_VertexID(base zero) + gl_BaseVertex, which is the way expected for indexed draw calls. For non-indexed draw calls, this currently works because we use the <first> argument as gl_BaseVertex. If we give gl_BaseVertex a zero value for gl*DrawArrays* as the spec says, the gl_VertexID will be incorrect. Not sure how to fix the gl_VertexID issue, it looks like the i965 does not have a way to add a base value during the vertex-id generation.
Comment 4 Kenneth Graunke 2017-10-13 08:00:38 UTC
Ian, any thoughts? This just feels wrong at this point...I thought the intention was that gl_VertexID - gl_BaseVertex would be the zero-based vertex ID. But if gl_VertexID is <first + i>, and gl_BaseVertex is required to be 0 for DrawArrays, then...there is no GLSL variable to get <first>, so there is no way to get the zero-based index. It seems like gl_BaseVertex really ought to be <first> for DrawArrays.
Comment 5 Ian Romanick 2017-10-14 00:26:03 UTC
glDrawArrays behaves as if you did unsigned indices[count]; for (i = 0; i < count; i++) indices[i] = first + count; glDrawElements(mode, count, GL_UNSIGNED_INT, indices); The language in the spec is a bit more complicated than that, IIRC, but it should be equivalent. I think gl_BaseVertex=0 and gl_VertexID=first+i is consistent with that.
Comment 7 Antia Puentes 2018-05-02 09:37:55 UTC
Fixed by: commit 3a1df14a7b5c1652aa72eb6cf43e69ab447c6273 Author: Antia Puentes <email@example.com> Date: Sat Apr 28 14:09:22 2018 +0200 intel: activate the gl_BaseVertex lowering Surplus code related to the basevertex is removed. The Vertex Elements contain now: * VE 1: <firstvertex, BaseInstance, VertexID, InstanceID> * VE 2: <DrawID, is_indexed_draw, 0, 0> Also fixes unreachable message. Fixes OpenGL CTS tests: * KHR-GL46.shader_draw_parameters_tests.ShaderDrawArraysInstancedParameters * KHR-GL46.shader_draw_parameters_tests.ShaderMultiDrawArraysParameters * KHR-GL46.shader_draw_parameters_tests.MultiDrawArraysIndirectCountParameters * KHR-GL46.shader_draw_parameters_tests.ShaderDrawArraysParameters * KHR-GL46.shader_draw_parameters_tests.ShaderMultiDrawArraysIndirectParameters Fixes Piglit tests: * arb_shader_draw_parameters-drawid-indirect baseinstance * arb_shader_draw_parameters-basevertex Reviewed-by: Jason Ekstrand <firstname.lastname@example.org> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102678