[Mesa-dev] [PATCH 0/9] Fix shader_draw_parameters CTS tests

[Kenneth Graunke]

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
changes them?

> 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.
> 
> Regards,
> - Neil

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171130/25fd4e5b/attachment.sig>