[Mesa-dev] [PATCH 02/12] spirv: Flip the tessellation winding order

[Kenneth Graunke]

On Friday, September 15, 2017 9:01:41 AM PDT Jason Ekstrand wrote:
> It's not SPIR-V that's backwards from GLSL, it's Vulkan that's backwards
> from GL.  Let's make NIR consistent with the source language and do the
> flipping inside the Vulkan driver instead.
> 
> Cc: Kenneth Graunke <kenneth at whitecape.org>
> Cc: Dave Airlie <airlied at redhat.com>
> ---
>  src/compiler/spirv/spirv_to_nir.c |  9 ++-------
>  src/intel/vulkan/genX_pipeline.c  | 13 ++++++++++++-
>  2 files changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/src/compiler/spirv/spirv_to_nir.c b/src/compiler/spirv/spirv_to_nir.c
> index 8653685..6ce9d1a 100644
> --- a/src/compiler/spirv/spirv_to_nir.c
> +++ b/src/compiler/spirv/spirv_to_nir.c
> @@ -2951,17 +2951,12 @@ vtn_handle_execution_mode(struct vtn_builder *b, struct vtn_value *entry_point,
>     case SpvExecutionModeVertexOrderCw:
>        assert(b->shader->stage == MESA_SHADER_TESS_CTRL ||
>               b->shader->stage == MESA_SHADER_TESS_EVAL);
> -      /* Vulkan's notion of CCW seems to match the hardware backends,
> -       * but be the opposite of OpenGL.  Currently NIR follows GL semantics,
> -       * so we set it backwards here.
> -       */
> -      b->shader->info.tess.ccw = true;
> +      b->shader->info.tess.ccw = false;
>        break;
>     case SpvExecutionModeVertexOrderCcw:
>        assert(b->shader->stage == MESA_SHADER_TESS_CTRL ||
>               b->shader->stage == MESA_SHADER_TESS_EVAL);
> -      /* Backwards; see above */
> -      b->shader->info.tess.ccw = false;
> +      b->shader->info.tess.ccw = true;
>        break;
>     case SpvExecutionModePointMode:
>        assert(b->shader->stage == MESA_SHADER_TESS_CTRL ||
> diff --git a/src/intel/vulkan/genX_pipeline.c b/src/intel/vulkan/genX_pipeline.c
> index 6dfa49b..844c118 100644
> --- a/src/intel/vulkan/genX_pipeline.c
> +++ b/src/intel/vulkan/genX_pipeline.c
> @@ -1217,7 +1217,18 @@ emit_3dstate_hs_te_ds(struct anv_pipeline *pipeline)
>  
>     anv_batch_emit(&pipeline->batch, GENX(3DSTATE_TE), te) {
>        te.Partitioning = tes_prog_data->partitioning;
> -      te.OutputTopology = tes_prog_data->output_topology;
> +
> +      /* Vulkan has its winding order backwards from GL so TRI_CCW becomes
> +       * TRI_CW and vice versa.
> +       */
> +      if (tes_prog_data->output_topology == OUTPUT_TRI_CCW) {
> +         te.OutputTopology = OUTPUT_TRI_CW;
> +      } else if (tes_prog_data->output_topology == OUTPUT_TRI_CW) {
> +         te.OutputTopology = OUTPUT_TRI_CCW;
> +      } else {
> +         te.OutputTopology = tes_prog_data->output_topology;
> +      }
> +
>        te.TEDomain = tes_prog_data->domain;
>        te.TEEnable = true;
>        te.MaximumTessellationFactorOdd = 63.0;
> 

This version is missing the radv hunk, which you have in your
wip/VK_KHR_maintenance2 branch:

+       tes_nir->info.tess.ccw = !tes_nir->info.tess.ccw;

assuming you have that, it looks fine, and both patches 2-3 are
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

I do think it would be less code to just whack the ccw boolean in anv
like Bas did for radv.  But I don't care too much.
-------------- 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/20170920/92aafd69/attachment.sig>