[Mesa-dev] [PATCH v4 4/6] spirv: Lower BaseVertex to FIRST_VERTEX instead of BASE_VERTEX
Jason Ekstrand
jason at jlekstrand.net
Sat Apr 7 18:19:00 UTC 2018
On Sat, Apr 7, 2018 at 10:49 AM, Ian Romanick <idr at freedesktop.org> wrote:
> On 04/06/2018 11:00 PM, Jason Ekstrand wrote:
> > On Fri, Apr 6, 2018 at 2:53 PM, Ian Romanick <idr at freedesktop.org
> > <mailto:idr at freedesktop.org>> wrote:
> >
> > From: Neil Roberts <nroberts at igalia.com <mailto:nroberts at igalia.com
> >>
> >
> > The base vertex in Vulkan is different from GL in that for
> non-indexed
> > primitives the value is taken from the firstVertex parameter instead
> > of being set to zero. This coincides with the new
> > SYSTEM_VALUE_FIRST_VERTEX
> > instead of BASE_VERTEX.
> >
> > Reviewed-by: Ian Romanick <ian.d.romanick at intel.com
> > <mailto:ian.d.romanick at intel.com>>
> > ---
> > src/compiler/spirv/vtn_variables.c | 2 +-
> > src/intel/vulkan/genX_cmd_buffer.c | 16 ++++++++++++----
> > src/intel/vulkan/genX_pipeline.c | 2 ++
> > 3 files changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/compiler/spirv/vtn_variables.c
> > b/src/compiler/spirv/vtn_variables.c
> > index b2897407fb1..9bb7d5a575e 100644
> > --- a/src/compiler/spirv/vtn_variables.c
> > +++ b/src/compiler/spirv/vtn_variables.c
> > @@ -1296,7 +1296,7 @@ vtn_get_builtin_location(struct vtn_builder
> *b,
> > set_mode_system_value(b, mode);
> > break;
> > case SpvBuiltInBaseVertex:
> > - *location = SYSTEM_VALUE_BASE_VERTEX;
> > + *location = SYSTEM_VALUE_FIRST_VERTEX;
> >
> >
> > Given that we are taking something called BaseVertex and using the
> > FIRST_VERTEX location when there is a location called BASE_VERTEX, I
> > think a comment is in order.
>
> That is fair, but I struggled a bit to come up with something
> non-circular. The best I could get is:
>
> /* OpenGL gl_BaseVertex (SYSTEM_VALUE_BASE_VERTEX) is not the
> * same semantic as SPIR-V BaseVertex
> (SYSTEM_VALUE_FIRST_VERTEX).
> */
>
That's fine with me.
> > set_mode_system_value(b, mode);
> > break;
> > case SpvBuiltInBaseInstance:
> > diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> > b/src/intel/vulkan/genX_cmd_buffer.c
> > index 3c703f6be44..7d190a4d5cf 100644
> > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > @@ -2673,7 +2673,9 @@ void genX(CmdDraw)(
> >
> > genX(cmd_buffer_flush_state)(cmd_buffer);
> >
> > - if (vs_prog_data->uses_basevertex ||
> > vs_prog_data->uses_baseinstance)
> > + if (vs_prog_data->uses_firstvertex ||
> > + vs_prog_data->uses_basevertex ||
> > + vs_prog_data->uses_baseinstance)
> >
> >
> > I was going to ask if this was needed. Then I saw patch 5.
> >
> > Reviewed-by: Jason Ekstrand <jason at jlekstrand.net
> > <mailto:jason at jlekstrand.net>>
> >
> >
> > emit_base_vertex_instance(cmd_buffer, firstVertex,
> > firstInstance);
> > if (vs_prog_data->uses_drawid)
> > emit_draw_index(cmd_buffer, 0);
> > @@ -2711,7 +2713,9 @@ void genX(CmdDrawIndexed)(
> >
> > genX(cmd_buffer_flush_state)(cmd_buffer);
> >
> > - if (vs_prog_data->uses_basevertex ||
> > vs_prog_data->uses_baseinstance)
> > + if (vs_prog_data->uses_firstvertex ||
> > + vs_prog_data->uses_basevertex ||
> > + vs_prog_data->uses_baseinstance)
> > emit_base_vertex_instance(cmd_buffer, vertexOffset,
> > firstInstance);
> > if (vs_prog_data->uses_drawid)
> > emit_draw_index(cmd_buffer, 0);
> > @@ -2850,7 +2854,9 @@ void genX(CmdDrawIndirect)(
> > struct anv_bo *bo = buffer->bo;
> > uint32_t bo_offset = buffer->offset + offset;
> >
> > - if (vs_prog_data->uses_basevertex ||
> > vs_prog_data->uses_baseinstance)
> > + if (vs_prog_data->uses_firstvertex ||
> > + vs_prog_data->uses_basevertex ||
> > + vs_prog_data->uses_baseinstance)
> > emit_base_vertex_instance_bo(cmd_buffer, bo, bo_offset +
> 8);
> > if (vs_prog_data->uses_drawid)
> > emit_draw_index(cmd_buffer, i);
> > @@ -2889,7 +2895,9 @@ void genX(CmdDrawIndexedIndirect)(
> > uint32_t bo_offset = buffer->offset + offset;
> >
> > /* TODO: We need to stomp base vertex to 0 somehow */
> > - if (vs_prog_data->uses_basevertex ||
> > vs_prog_data->uses_baseinstance)
> > + if (vs_prog_data->uses_firstvertex ||
> > + vs_prog_data->uses_basevertex ||
> > + vs_prog_data->uses_baseinstance)
> > emit_base_vertex_instance_bo(cmd_buffer, bo, bo_offset +
> 12);
> > if (vs_prog_data->uses_drawid)
> > emit_draw_index(cmd_buffer, i);
> > diff --git a/src/intel/vulkan/genX_pipeline.c
> > b/src/intel/vulkan/genX_pipeline.c
> > index eb2d4147357..a473f42c7e1 100644
> > --- a/src/intel/vulkan/genX_pipeline.c
> > +++ b/src/intel/vulkan/genX_pipeline.c
> > @@ -98,6 +98,7 @@ emit_vertex_input(struct anv_pipeline *pipeline,
> > const bool needs_svgs_elem = vs_prog_data->uses_vertexid ||
> > vs_prog_data->uses_instanceid ||
> > vs_prog_data->uses_basevertex ||
> > + vs_prog_data->uses_firstvertex ||
> > vs_prog_data->uses_baseinstance;
> >
> > uint32_t elem_count = __builtin_popcount(elements) -
> > @@ -178,6 +179,7 @@ emit_vertex_input(struct anv_pipeline *pipeline,
> > * well. Just do all or nothing.
> > */
> > uint32_t base_ctrl = (vs_prog_data->uses_basevertex ||
> > + vs_prog_data->uses_firstvertex ||
> > vs_prog_data->uses_baseinstance) ?
> > VFCOMP_STORE_SRC : VFCOMP_STORE_0;
> >
> > --
> > 2.14.3
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org <mailto:mesa-dev at lists.
> freedesktop.org>
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > <https://lists.freedesktop.org/mailman/listinfo/mesa-dev>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180407/e050257c/attachment-0001.html>
More information about the mesa-dev
mailing list