[Mesa-dev] [PATCH v4 4/6] spirv: Lower BaseVertex to FIRST_VERTEX instead of BASE_VERTEX

[Jason Ekstrand]

On Fri, Apr 6, 2018 at 2:53 PM, Ian Romanick <idr at freedesktop.org> wrote:

> From: Neil Roberts <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>
> ---
>  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.


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


>        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_baseinstanc
> e)
> +      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_baseinstanc
> e)
> +      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
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180406/99fe2dff/attachment.html>