Re: [Mesa-dev] [PATCH 1/2] spirv: Add SpvCapabilityShaderViewportIndexLayerEXT

[Iago Toral]

On Fri, 2018-02-09 at 09:08 +0100, Iago Toral wrote:
> On Sat, 2018-01-27 at 18:58 -0800, Caio Marcelo de Oliveira Filho
> wrote:
> > ---
> > 
> > In the last chunk of the diff, should accepting stage of vertex
> > shader
> > should be conditioned on shader_viewport_index_layer?
> 
> I was going to say that we should assert that
> shader_viewport_index_layer is supported, but looking at the code, if
> we make it conditional on shader_viewport_index_layer it should
> suffice, since in that case if the implementation doesn't support it
> we
> will hit the vtn_fail fallback and report the error.
> 
> With that change, this patch is:
> Reviewed-by: Iago Toral Quiroga 

Sorry, but I have to withdraw the Rb :), I looked at the specs more
carefully now and realized there are things that we are missing here:
this extension also permits gl_Layer to be exported by Vertex and
Tessellation shaders (which is missing in this patch), and
ViewportIndex is also allowed in the Tessellation stage besides the
Vertex stage.

> 
> Iago
> 
> >  src/compiler/shader_info.h | 1 +
> >  src/compiler/spirv/spirv_to_nir.c  | 4 
> >  src/compiler/spirv/vtn_variables.c | 2 +-
> >  3 files changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/compiler/shader_info.h
> > b/src/compiler/shader_info.h
> > index 4492cad0e8..9cc68740d4 100644
> > --- a/src/compiler/shader_info.h
> > +++ b/src/compiler/shader_info.h
> > @@ -43,6 +43,7 @@ struct spirv_supported_capabilities {
> > bool multiview;
> > bool variable_pointers;
> > bool storage_16bit;
> > +   bool shader_viewport_index_layer;
> >  };
> >  
> >  typedef struct shader_info {
> > diff --git a/src/compiler/spirv/spirv_to_nir.c
> > b/src/compiler/spirv/spirv_to_nir.c
> > index c6df764682..fdb2993db5 100644
> > --- a/src/compiler/spirv/spirv_to_nir.c
> > +++ b/src/compiler/spirv/spirv_to_nir.c
> > @@ -3203,6 +3203,10 @@ vtn_handle_preamble_instruction(struct
> > vtn_builder *b, SpvOp opcode,
> >   spv_check_supported(storage_16bit, cap);
> >   break;
> >  
> > +  case SpvCapabilityShaderViewportIndexLayerEXT:
> > + spv_check_supported(shader_viewport_index_layer, cap);
> > + break;
> > +
> >default:
> >   vtn_fail("Unhandled capability");
> >}
> > diff --git a/src/compiler/spirv/vtn_variables.c
> > b/src/compiler/spirv/vtn_variables.c
> > index eb306d0c4a..889a9b3c10 100644
> > --- a/src/compiler/spirv/vtn_variables.c
> > +++ b/src/compiler/spirv/vtn_variables.c
> > @@ -1197,7 +1197,7 @@ vtn_get_builtin_location(struct vtn_builder
> > *b,
> >break;
> > case SpvBuiltInViewportIndex:
> >*location = VARYING_SLOT_VIEWPORT;
> > -  if (b->shader->info.stage == MESA_SHADER_GEOMETRY)
> > +  if (b->shader->info.stage == MESA_SHADER_GEOMETRY || b-
> > > shader->info.stage == MESA_SHADER_VERTEX)
> > 
> >   *mode = nir_var_shader_out;
> >else if (b->shader->info.stage == MESA_SHADER_FRAGMENT)
> >   *mode = nir_var_shader_in;
> 
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] spirv: Add SpvCapabilityShaderViewportIndexLayerEXT

[Iago Toral]

On Sat, 2018-01-27 at 18:58 -0800, Caio Marcelo de Oliveira Filho
wrote:
> ---
> 
> In the last chunk of the diff, should accepting stage of vertex
> shader
> should be conditioned on shader_viewport_index_layer?

I was going to say that we should assert that
shader_viewport_index_layer is supported, but looking at the code, if
we make it conditional on shader_viewport_index_layer it should
suffice, since in that case if the implementation doesn't support it we
will hit the vtn_fail fallback and report the error.

With that change, this patch is:
Reviewed-by: Iago Toral Quiroga 


Iago

>  src/compiler/shader_info.h | 1 +
>  src/compiler/spirv/spirv_to_nir.c  | 4 
>  src/compiler/spirv/vtn_variables.c | 2 +-
>  3 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/src/compiler/shader_info.h b/src/compiler/shader_info.h
> index 4492cad0e8..9cc68740d4 100644
> --- a/src/compiler/shader_info.h
> +++ b/src/compiler/shader_info.h
> @@ -43,6 +43,7 @@ struct spirv_supported_capabilities {
> bool multiview;
> bool variable_pointers;
> bool storage_16bit;
> +   bool shader_viewport_index_layer;
>  };
>  
>  typedef struct shader_info {
> diff --git a/src/compiler/spirv/spirv_to_nir.c
> b/src/compiler/spirv/spirv_to_nir.c
> index c6df764682..fdb2993db5 100644
> --- a/src/compiler/spirv/spirv_to_nir.c
> +++ b/src/compiler/spirv/spirv_to_nir.c
> @@ -3203,6 +3203,10 @@ vtn_handle_preamble_instruction(struct
> vtn_builder *b, SpvOp opcode,
>   spv_check_supported(storage_16bit, cap);
>   break;
>  
> +  case SpvCapabilityShaderViewportIndexLayerEXT:
> + spv_check_supported(shader_viewport_index_layer, cap);
> + break;
> +
>default:
>   vtn_fail("Unhandled capability");
>}
> diff --git a/src/compiler/spirv/vtn_variables.c
> b/src/compiler/spirv/vtn_variables.c
> index eb306d0c4a..889a9b3c10 100644
> --- a/src/compiler/spirv/vtn_variables.c
> +++ b/src/compiler/spirv/vtn_variables.c
> @@ -1197,7 +1197,7 @@ vtn_get_builtin_location(struct vtn_builder *b,
>break;
> case SpvBuiltInViewportIndex:
>*location = VARYING_SLOT_VIEWPORT;
> -  if (b->shader->info.stage == MESA_SHADER_GEOMETRY)
> +  if (b->shader->info.stage == MESA_SHADER_GEOMETRY || b-
> >shader->info.stage == MESA_SHADER_VERTEX)
>   *mode = nir_var_shader_out;
>else if (b->shader->info.stage == MESA_SHADER_FRAGMENT)
>   *mode = nir_var_shader_in;
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev