Bug 102678

Summary: gl_BaseVertex should always be zero when the draw command has no <basevertex> parameter
Product: Mesa Reporter: Antia Puentes <apuentes>
Component: Drivers/DRI/i965Assignee: Antia Puentes <apuentes>
Status: RESOLVED FIXED QA Contact: Intel 3D Bugs Mailing List <intel-3d-bugs>
Severity: normal    
Priority: medium CC: apuentes, idr, kenneth
Version: git   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Bug Depends on:    
Bug Blocks: 102590    

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 6 Antia Puentes 2017-11-10 18:00:37 UTC
https://patchwork.freedesktop.org/series/33623/
Comment 7 Antia Puentes 2018-05-02 09:37:55 UTC
Fixed by:

commit 3a1df14a7b5c1652aa72eb6cf43e69ab447c6273

Author: Antia Puentes <apuentes@igalia.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 <jason@jlekstrand.net>
    
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102678

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.