Re: [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  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: 
> VE 2: 
> 
> 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



signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Neil Roberts]

Kenneth Graunke  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: 
VE 2: 

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. 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”.

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


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Kenneth Graunke]

On Friday, November 10, 2017 9:53:28 AM PST Antia Puentes wrote:
> Hi,
> 
> the series sets gl_BaseVertex to zero for gl*DrawArrays* in i965.
[snip]

Hi Antia, Neil,

The end result of this series looks reasonable, but it looks like it's
going to break a bunch of things at each step.  Either that or I'm a bit
lost, sorry.

Could you reorganize/split  the patches so that things keep working at
each step along the way?

>   compiler: Add new system value SYSTEM_VALUE_BASE_VERTEX_ID
>   nir: Add SYSTEM_VALUE_BASE_VERTEX_ID instrinsics
>   intel/compiler: Add a uses_basevertexid flag
>   i965: emit basevertexid as a vertex element component

It looks like this moves SYSTEM_VALUE_BASE_VERTEX from VE1 to VE2...but
it doesn't update the compiler to fetch gl_BaseVertex from the new
location.  Plus, we haven't switched to using BASE_VERTEX_ID lowering
for gl_VertexID yet...so prog_data->uses_basevertexid will never be set,
so we won't upload BASE_VERTEX_ID either.

It sure looks like this would break anything using gl_VertexID or
gl_BaseVertex.  Maybe I'm missing something.

>   i965: gl_BaseVertex must be zero for non-indexed draw calls

Again, at this point, we're still using SYSTEM_VALUE_BASE_VERTEX for
gl_VertexID lowering, so this would break gl_VertexID tests.

>   intel/compiler: implement the basevertex(id) load intrinsics
>   nir: Offset vertex_id by base_vertex_id instead of base_vertex
>   i965: Let nir lower gl_VertexID instead of the linker
>   spirv: Lower BaseVertex to BASE_VERTEX_ID instead of BASE_VERTEX

The order I'd expect to see is:

1. Introduce new SYSTEM_VALUE_FIRST_VERTEX enum and nir intrinsics.

2. Add vs_prog_data::uses_first_vertex or system_values_read field.

3. In Intel code, move system values around to new VE locations.

   Vertex Element 1: 
   Vertex Element 2: 

   This would involve changing intel/compiler and both brw and anv's
   state upload code, in the same patch.

4. In Intel code, start uploading SYSTEM_VALUE_FIRST_VERTEX as VE1.y.
   Update the state upload code and the compiler in the same patch.

   Now all the values are in place, but nothing uses the new one yet.

5. Switch to NIR instead of GLSL IR based Vertex ID lowering.
   (i965: Let nir lower gl_VertexID instead of the linker)

6. Make NIR VertexID lowering use SYSTEM_VALUE_FIRST_VERTEX.
   (nir: Offset vertex_id by base_vertex_id instead of base_vertex)

   Now i965 no longer uses SYSTEM_VALUE_BASE_VERTEX for VertexID,
   so you can safely change the meaning.

7. Fix gl_BaseVertex to be 0 for non-indexed draw calls.
   (i965: gl_BaseVertex must be zero for non-indexed draw calls)

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...


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Neil Roberts]

Just to note, I made some Piglit patches to verify this new behaviour
too:

https://patchwork.freedesktop.org/series/33626/

This might make testing a little easier if someone wants to implement
the behaviour for other drivers too.

- Neil

Antia Puentes  writes:

> Hi,
>
> the series sets gl_BaseVertex to zero for gl*DrawArrays* in i965.
> Previously, its value was the value passed as  in the non-indexed
> draw call. This was convinient because the gl_VertexID was calculated as
> gl_VertexIDBaseZero + gl_BaseVertex.
>
> However, as gl_BaseVertex must be zero for non-indexed draw calls, this
> calculation is broken.
>
> Apart from setting the basevertex to zero in i965, the following patches add
> a new VS system value which will contain  or  as needed
> to make the gl_VertexID calculation right.
>
>
> The relevant bits of the OpenGL 4.6 specification involved here are:
>
> * 11.1.3.9 Shader Inputs
>
> "gl_BaseVertex 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_BaseVertex is zero."
>
> "gl_VertexID holds the integer index i implicitly passed by DrawArrays or
> one of the other drawing commands defined in section 10.4."
>
> * 10.4. Drawing Commands Using Vertex Arrays:
>
>   - Page 352:
> "The index of any element transferred to the GL by DrawArraysOneInstance is
> referred to as its vertex ID, and may be read by a vertex shader as 
> gl_VertexID.
> The vertex ID of the ith element transferred is first + i."
>
>   - Page 355:
> "The index of any element transferred to the GL by DrawElementsOneInstance is
> referred to as its vertex ID, and may be read by a vertex shader as 
> gl_VertexID.
> The vertex ID of the ith element transferred is the sum of basevertex and the
> value stored in the currently bound element array buffer at offset indices + 
> i."
>
> Regards,
> Antia.
>
> Antia Puentes (5):
>   compiler: Add new system value SYSTEM_VALUE_BASE_VERTEX_ID
>   nir: Add SYSTEM_VALUE_BASE_VERTEX_ID instrinsics
>   i965: emit basevertexid as a vertex element component
>   i965: gl_BaseVertex must be zero for non-indexed draw calls
>   intel/compiler: implement the basevertex(id) load intrinsics
>
> Neil Roberts (4):
>   intel/compiler: Add a uses_basevertexid flag
>   nir: Offset vertex_id by base_vertex_id instead of base_vertex
>   i965: Let nir lower gl_VertexID instead of the linker
>   spirv: Lower BaseVertex to BASE_VERTEX_ID instead of BASE_VERTEX
>
>  src/compiler/nir/nir.c|  4 +++
>  src/compiler/nir/nir_gather_info.c|  1 +
>  src/compiler/nir/nir_intrinsics.h |  1 +
>  src/compiler/nir/nir_lower_system_values.c|  2 +-
>  src/compiler/shader_enums.c   |  1 +
>  src/compiler/shader_enums.h   | 15 +++
>  src/compiler/spirv/vtn_variables.c|  2 +-
>  src/intel/compiler/brw_compiler.h |  1 +
>  src/intel/compiler/brw_nir.c  | 12 ++---
>  src/intel/compiler/brw_vec4.cpp   | 18 -
>  src/intel/vulkan/genX_cmd_buffer.c|  8 +++---
>  src/intel/vulkan/genX_pipeline.c  |  4 +--
>  src/mesa/drivers/dri/i965/brw_context.c   |  3 ++-
>  src/mesa/drivers/dri/i965/brw_context.h   | 36 ++---
>  src/mesa/drivers/dri/i965/brw_draw.c  | 33 ---
>  src/mesa/drivers/dri/i965/brw_draw_upload.c   | 13 -
>  src/mesa/drivers/dri/i965/genX_state_upload.c | 39 
> +++
>  17 files changed, 132 insertions(+), 61 deletions(-)
>
> -- 
> 2.14.1
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Antia Puentes]

Hi,

the series sets gl_BaseVertex to zero for gl*DrawArrays* in i965.
Previously, its value was the value passed as  in the non-indexed
draw call. This was convinient because the gl_VertexID was calculated as
gl_VertexIDBaseZero + gl_BaseVertex.

However, as gl_BaseVertex must be zero for non-indexed draw calls, this
calculation is broken.

Apart from setting the basevertex to zero in i965, the following patches add
a new VS system value which will contain  or  as needed
to make the gl_VertexID calculation right.


The relevant bits of the OpenGL 4.6 specification involved here are:

* 11.1.3.9 Shader Inputs

"gl_BaseVertex 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_BaseVertex is zero."

"gl_VertexID holds the integer index i implicitly passed by DrawArrays or
one of the other drawing commands defined in section 10.4."

* 10.4. Drawing Commands Using Vertex Arrays:

  - Page 352:
"The index of any element transferred to the GL by DrawArraysOneInstance is
referred to as its vertex ID, and may be read by a vertex shader as gl_VertexID.
The vertex ID of the ith element transferred is first + i."

  - Page 355:
"The index of any element transferred to the GL by DrawElementsOneInstance is
referred to as its vertex ID, and may be read by a vertex shader as gl_VertexID.
The vertex ID of the ith element transferred is the sum of basevertex and the
value stored in the currently bound element array buffer at offset indices + i."

Regards,
Antia.

Antia Puentes (5):
  compiler: Add new system value SYSTEM_VALUE_BASE_VERTEX_ID
  nir: Add SYSTEM_VALUE_BASE_VERTEX_ID instrinsics
  i965: emit basevertexid as a vertex element component
  i965: gl_BaseVertex must be zero for non-indexed draw calls
  intel/compiler: implement the basevertex(id) load intrinsics

Neil Roberts (4):
  intel/compiler: Add a uses_basevertexid flag
  nir: Offset vertex_id by base_vertex_id instead of base_vertex
  i965: Let nir lower gl_VertexID instead of the linker
  spirv: Lower BaseVertex to BASE_VERTEX_ID instead of BASE_VERTEX

 src/compiler/nir/nir.c|  4 +++
 src/compiler/nir/nir_gather_info.c|  1 +
 src/compiler/nir/nir_intrinsics.h |  1 +
 src/compiler/nir/nir_lower_system_values.c|  2 +-
 src/compiler/shader_enums.c   |  1 +
 src/compiler/shader_enums.h   | 15 +++
 src/compiler/spirv/vtn_variables.c|  2 +-
 src/intel/compiler/brw_compiler.h |  1 +
 src/intel/compiler/brw_nir.c  | 12 ++---
 src/intel/compiler/brw_vec4.cpp   | 18 -
 src/intel/vulkan/genX_cmd_buffer.c|  8 +++---
 src/intel/vulkan/genX_pipeline.c  |  4 +--
 src/mesa/drivers/dri/i965/brw_context.c   |  3 ++-
 src/mesa/drivers/dri/i965/brw_context.h   | 36 ++---
 src/mesa/drivers/dri/i965/brw_draw.c  | 33 ---
 src/mesa/drivers/dri/i965/brw_draw_upload.c   | 13 -
 src/mesa/drivers/dri/i965/genX_state_upload.c | 39 +++
 17 files changed, 132 insertions(+), 61 deletions(-)

-- 
2.14.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev