Re: [Mesa-dev] [PATCH 2/2] i965/tcs: support gl_PatchVerticesIn as a system value from SPIR-V

[Kenneth Graunke]

On Saturday, January 6, 2018 9:07:44 PM PST Jason Ekstrand wrote:
> On Sat, Jan 6, 2018 at 5:12 PM, Kenneth Graunke 
> wrote:
> 
> > On Wednesday, November 15, 2017 11:53:08 PM PST Iago Toral Quiroga wrote:
> > > We currently handle this by lowering it to a uniform for gen8+ but
> > > the SPIR-V path generates this as a system value, so handle that
> > > case as well.
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_tcs.c | 9 -
> > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/mesa/drivers/dri/i965/brw_tcs.c
> > b/src/mesa/drivers/dri/i965/brw_tcs.c
> > > index 4424efea4f0..b07b11f485d 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_tcs.c
> > > +++ b/src/mesa/drivers/dri/i965/brw_tcs.c
> > > @@ -296,7 +296,14 @@ brw_tcs_populate_key(struct brw_context *brw,
> > >per_patch_slots |= prog->info.patch_outputs_written;
> > > }
> > >
> > > -   if (devinfo->gen < 8 || !tcp)
> > > +   /* For GLSL, gen8+ lowers gl_PatchVerticesIn to a uniform, however
> > > +* the SPIR-V path always lowers it to a system value.
> > > +*/
> > > +   bool reads_patch_vertices_as_system_value =
> > > +  tcp && (tcp->program.nir->info.system_values_read &
> > > +  BITFIELD64_BIT(SYSTEM_VALUE_VERTICES_IN));
> > > +
> > > +   if (devinfo->gen < 8 || !tcp || reads_patch_vertices_as_
> > system_value)
> > >key->input_vertices = brw->ctx.TessCtrlProgram.patch_vertices;
> > > key->outputs_written = per_vertex_slots;
> > > key->patch_outputs_written = per_patch_slots;
> > >
> >
> > I guess this is okay, and it's better than nothing.  I'd really rather
> > see it converted to a uniform, like it is in the normal GLSL paths.  If
> > you're going to add recompiles based on the key like this, it might be
> > nice to at least update the brw_tcs_precompile function to guess, so we
> > at least attempt to avoid a recompile.
> >
> 
> Ugh... I'm happy to give a stronger "I don't like this".  In Vulkan, this
> is part of the pipeline state so we just pass it in through the shader
> key.  With GL, ugh... Personally, I think I'd be ok with just making it
> state based all the time but we already have the infrastructure to pass it
> through as a uniform so we may as well.  I think the better thing to do
> would be to add a quick little pass that moves VERTICES_IN to a uniform and
> call that on gen8+ brw_link.cpp.  Then we can delete
> LowerTESPatchVerticesIn as i965 is the only user.  The "pass" would be
> really easy:

LowerTCSPatchVerticesIn rather.  I like this plan.

> 
> void
> brw_nir_lower_tcs_vertices_in_to_uniform(nir_shader *nir, const struct
> gl_program *prog, brw_tcs_prog_data *prog_data)
> {
>int uniform = -1;
>nir_foreach_var_safe(var, >system_values) {
>   if (var->data.location != SYSTEM_VALUE_VERTICES_IN)
>  continue;
> 
>   if (uniform < -1) {
>  gl_state_index tokens[5] = {
> STATE_INTERNAL,
> STATE_TESS_PATCH_VERTICES_IN,
>  };
>  int index = _mesa_add_state_reference(prog->Parameters, tokens);
> 
>  uniform = prog_data->nr_params;
>  uint32_t *param =
> brw_stage_prog_data_add_params(_data->base->base, 1);
>  *param = BRW_PARAM_PARAMETER(index, SWIZZLE_);
>   }
> 
>   var->mode = nir_var_uniform;
>   var->data.location = uniform;
>   exec_node_remove(>node);
>   exec_list_push_tail(>uniforms, >node);
>}
> }
> 
> I may not have gotten my state referencing quite right there, but I think
> it's close.  I'd probably put the pas in brw_nir_uniforms.cpp if I was
> writing it.
> 
> --Jason
> 



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 2/2] i965/tcs: support gl_PatchVerticesIn as a system value from SPIR-V

[Jason Ekstrand]

On Sat, Jan 6, 2018 at 5:12 PM, Kenneth Graunke 
wrote:

> On Wednesday, November 15, 2017 11:53:08 PM PST Iago Toral Quiroga wrote:
> > We currently handle this by lowering it to a uniform for gen8+ but
> > the SPIR-V path generates this as a system value, so handle that
> > case as well.
> > ---
> >  src/mesa/drivers/dri/i965/brw_tcs.c | 9 -
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_tcs.c
> b/src/mesa/drivers/dri/i965/brw_tcs.c
> > index 4424efea4f0..b07b11f485d 100644
> > --- a/src/mesa/drivers/dri/i965/brw_tcs.c
> > +++ b/src/mesa/drivers/dri/i965/brw_tcs.c
> > @@ -296,7 +296,14 @@ brw_tcs_populate_key(struct brw_context *brw,
> >per_patch_slots |= prog->info.patch_outputs_written;
> > }
> >
> > -   if (devinfo->gen < 8 || !tcp)
> > +   /* For GLSL, gen8+ lowers gl_PatchVerticesIn to a uniform, however
> > +* the SPIR-V path always lowers it to a system value.
> > +*/
> > +   bool reads_patch_vertices_as_system_value =
> > +  tcp && (tcp->program.nir->info.system_values_read &
> > +  BITFIELD64_BIT(SYSTEM_VALUE_VERTICES_IN));
> > +
> > +   if (devinfo->gen < 8 || !tcp || reads_patch_vertices_as_
> system_value)
> >key->input_vertices = brw->ctx.TessCtrlProgram.patch_vertices;
> > key->outputs_written = per_vertex_slots;
> > key->patch_outputs_written = per_patch_slots;
> >
>
> I guess this is okay, and it's better than nothing.  I'd really rather
> see it converted to a uniform, like it is in the normal GLSL paths.  If
> you're going to add recompiles based on the key like this, it might be
> nice to at least update the brw_tcs_precompile function to guess, so we
> at least attempt to avoid a recompile.
>

Ugh... I'm happy to give a stronger "I don't like this".  In Vulkan, this
is part of the pipeline state so we just pass it in through the shader
key.  With GL, ugh... Personally, I think I'd be ok with just making it
state based all the time but we already have the infrastructure to pass it
through as a uniform so we may as well.  I think the better thing to do
would be to add a quick little pass that moves VERTICES_IN to a uniform and
call that on gen8+ brw_link.cpp.  Then we can delete
LowerTESPatchVerticesIn as i965 is the only user.  The "pass" would be
really easy:

void
brw_nir_lower_tcs_vertices_in_to_uniform(nir_shader *nir, const struct
gl_program *prog, brw_tcs_prog_data *prog_data)
{
   int uniform = -1;
   nir_foreach_var_safe(var, >system_values) {
  if (var->data.location != SYSTEM_VALUE_VERTICES_IN)
 continue;

  if (uniform < -1) {
 gl_state_index tokens[5] = {
STATE_INTERNAL,
STATE_TESS_PATCH_VERTICES_IN,
 };
 int index = _mesa_add_state_reference(prog->Parameters, tokens);

 uniform = prog_data->nr_params;
 uint32_t *param =
brw_stage_prog_data_add_params(_data->base->base, 1);
 *param = BRW_PARAM_PARAMETER(index, SWIZZLE_);
  }

  var->mode = nir_var_uniform;
  var->data.location = uniform;
  exec_node_remove(>node);
  exec_list_push_tail(>uniforms, >node);
   }
}

I may not have gotten my state referencing quite right there, but I think
it's close.  I'd probably put the pas in brw_nir_uniforms.cpp if I was
writing it.

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

Re: [Mesa-dev] [PATCH 2/2] i965/tcs: support gl_PatchVerticesIn as a system value from SPIR-V

[Kenneth Graunke]

On Wednesday, November 15, 2017 11:53:08 PM PST Iago Toral Quiroga wrote:
> We currently handle this by lowering it to a uniform for gen8+ but
> the SPIR-V path generates this as a system value, so handle that
> case as well.
> ---
>  src/mesa/drivers/dri/i965/brw_tcs.c | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_tcs.c 
> b/src/mesa/drivers/dri/i965/brw_tcs.c
> index 4424efea4f0..b07b11f485d 100644
> --- a/src/mesa/drivers/dri/i965/brw_tcs.c
> +++ b/src/mesa/drivers/dri/i965/brw_tcs.c
> @@ -296,7 +296,14 @@ brw_tcs_populate_key(struct brw_context *brw,
>per_patch_slots |= prog->info.patch_outputs_written;
> }
>  
> -   if (devinfo->gen < 8 || !tcp)
> +   /* For GLSL, gen8+ lowers gl_PatchVerticesIn to a uniform, however
> +* the SPIR-V path always lowers it to a system value.
> +*/
> +   bool reads_patch_vertices_as_system_value =
> +  tcp && (tcp->program.nir->info.system_values_read &
> +  BITFIELD64_BIT(SYSTEM_VALUE_VERTICES_IN));
> +
> +   if (devinfo->gen < 8 || !tcp || reads_patch_vertices_as_system_value)
>key->input_vertices = brw->ctx.TessCtrlProgram.patch_vertices;
> key->outputs_written = per_vertex_slots;
> key->patch_outputs_written = per_patch_slots;
> 

I guess this is okay, and it's better than nothing.  I'd really rather
see it converted to a uniform, like it is in the normal GLSL paths.  If
you're going to add recompiles based on the key like this, it might be
nice to at least update the brw_tcs_precompile function to guess, so we
at least attempt to avoid a recompile.

As a stop-gap measure,
Reviewed-by: Kenneth Graunke 


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 2/2] i965/tcs: support gl_PatchVerticesIn as a system value from SPIR-V

[Iago Toral]

This patch is still missing a review, any takers? This is required to
pass one of the SPIR-V CTS tests.

Iago

On Thu, 2017-11-16 at 08:53 +0100, Iago Toral Quiroga wrote:
> We currently handle this by lowering it to a uniform for gen8+ but
> the SPIR-V path generates this as a system value, so handle that
> case as well.
> ---
>  src/mesa/drivers/dri/i965/brw_tcs.c | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_tcs.c
> b/src/mesa/drivers/dri/i965/brw_tcs.c
> index 4424efea4f0..b07b11f485d 100644
> --- a/src/mesa/drivers/dri/i965/brw_tcs.c
> +++ b/src/mesa/drivers/dri/i965/brw_tcs.c
> @@ -296,7 +296,14 @@ brw_tcs_populate_key(struct brw_context *brw,
>    per_patch_slots |= prog->info.patch_outputs_written;
> }
>  
> -   if (devinfo->gen < 8 || !tcp)
> +   /* For GLSL, gen8+ lowers gl_PatchVerticesIn to a uniform,
> however
> +* the SPIR-V path always lowers it to a system value.
> +*/
> +   bool reads_patch_vertices_as_system_value =
> +  tcp && (tcp->program.nir->info.system_values_read &
> +  BITFIELD64_BIT(SYSTEM_VALUE_VERTICES_IN));
> +
> +   if (devinfo->gen < 8 || !tcp ||
> reads_patch_vertices_as_system_value)
>    key->input_vertices = brw->ctx.TessCtrlProgram.patch_vertices;
> key->outputs_written = per_vertex_slots;
> key->patch_outputs_written = per_patch_slots;
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 2/2] i965/tcs: support gl_PatchVerticesIn as a system value from SPIR-V

[Iago Toral Quiroga]

We currently handle this by lowering it to a uniform for gen8+ but
the SPIR-V path generates this as a system value, so handle that
case as well.
---
 src/mesa/drivers/dri/i965/brw_tcs.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/mesa/drivers/dri/i965/brw_tcs.c 
b/src/mesa/drivers/dri/i965/brw_tcs.c
index 4424efea4f0..b07b11f485d 100644
--- a/src/mesa/drivers/dri/i965/brw_tcs.c
+++ b/src/mesa/drivers/dri/i965/brw_tcs.c
@@ -296,7 +296,14 @@ brw_tcs_populate_key(struct brw_context *brw,
   per_patch_slots |= prog->info.patch_outputs_written;
}
 
-   if (devinfo->gen < 8 || !tcp)
+   /* For GLSL, gen8+ lowers gl_PatchVerticesIn to a uniform, however
+* the SPIR-V path always lowers it to a system value.
+*/
+   bool reads_patch_vertices_as_system_value =
+  tcp && (tcp->program.nir->info.system_values_read &
+  BITFIELD64_BIT(SYSTEM_VALUE_VERTICES_IN));
+
+   if (devinfo->gen < 8 || !tcp || reads_patch_vertices_as_system_value)
   key->input_vertices = brw->ctx.TessCtrlProgram.patch_vertices;
key->outputs_written = per_vertex_slots;
key->patch_outputs_written = per_patch_slots;
-- 
2.11.0

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