[Mesa-dev] [PATCH 0/9] Fix shader_draw_parameters CTS tests
kenneth at whitecape.org
Fri Dec 1 00:14:29 UTC 2017
On Thursday, November 30, 2017 1:11:28 PM PST Neil Roberts wrote:
> Kenneth Graunke <kenneth at whitecape.org> writes:
> > Neil, in case you were wondering, I suggested the above organization
> > of vertex elements because it would let us only upload 1 in the common
> > case. Looking in shader-db, there are 3579 shaders that use
> > gl_InstanceID, 186 shaders that use gl_VertexID, and 0 shaders that
> > use gl_BaseVertex, gl_BaseInstance, or gl_DrawID.
> > It looks like your patches kicked gl_BaseVertex off to VE2 instead of
> > gl_BaseInstance. I suppose that works too, I just figured that keeping
> > VertexID/FirstVertex/BaseVertex together would make sense. If it's
> > more convenient to move gl_BaseVertex, I suppose I'm fine with that
> > too...
> Yes, Antia forwarded me your emails with the previous suggestion. The
> order we came up with for this patch series is:
> VE 1: <offset for vertex ID, BaseInstance, VertexID, InstanceID>
> VE 2: <Draw ID, BaseVertex, 0, 0>
> The “offset for vertex ID” corresponds to what we were previously
> (incorrectly) sending for gl_BaseVertex, so effectively the values in
> VE1 have not changed at all.
Right...that's what I thought was going to save us from intermediate
breakage. But it looked like you were uploading the new "offset for
vertex ID" field in VE1.x only when (system_values_read &
SYSTEM_VALUE_BASE_VERTEX_ID) != 0...which would be false...so we'd
effectively stop uploading it.
If we kept uploading it, it'd probably be fine...
> This also means we can continue to directly
> take these two values from the indirect draw params buffer. I believe
> your previous suggestion ended up needing a register store to put the
> values in the right place so with this ordering we get to avoid that.
> VE1 now contains everything needed for the most common values VertexID
> and InstanceID. VE2 is only needed for the rare cases that use gl_DrawID
> or gl_BaseVertex. On Vulkan VE2 won’t even be needed for the BaseVertex
> builtin because the semantics are different and in that case it
> corresponds to “offset for vertex ID”.
Cool, that sounds reasonable. Thanks for explaining. Perhaps you could
put the new order of VE's in the commit message for the patch that
> I haven’t looked into too much detail about reordering the patches yet,
> but it does sound reasonable to try and avoid intermediate breakages.
> Thanks for looking at the series.
> - Neil
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 833 bytes
Desc: This is a digitally signed message part.
More information about the mesa-dev