[Mesa-dev] [PATCH 02/11] spirv: Add tessellation varying and built-in support.
Jason Ekstrand
jason at jlekstrand.net
Mon Jan 9 16:20:02 UTC 2017
I hate decorations... 1 and 2 are
Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>
On Mon, Jan 9, 2017 at 7:16 AM, Iago Toral <itoral at igalia.com> wrote:
> On Sun, 2017-01-08 at 21:26 -0800, Kenneth Graunke wrote:
> > We need to:
> > - handle the extra array level for per-vertex varyings
> > - handle the patch qualifier correctly
> > - assign varying locations
> >
> > Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> > ---
> > src/compiler/spirv/vtn_private.h | 1 +
> > src/compiler/spirv/vtn_variables.c | 56
> > ++++++++++++++++++++++++++++++++++----
> > 2 files changed, 52 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/compiler/spirv/vtn_private.h
> > b/src/compiler/spirv/vtn_private.h
> > index 9302611803f..8b450106543 100644
> > --- a/src/compiler/spirv/vtn_private.h
> > +++ b/src/compiler/spirv/vtn_private.h
> > @@ -280,6 +280,7 @@ struct vtn_variable {
> > unsigned descriptor_set;
> > unsigned binding;
> > unsigned input_attachment_index;
> > + bool patch;
> >
> > nir_variable *var;
> > nir_variable **members;
> > diff --git a/src/compiler/spirv/vtn_variables.c
> > b/src/compiler/spirv/vtn_variables.c
> > index e3845365bdd..dcd0609626b 100644
> > --- a/src/compiler/spirv/vtn_variables.c
> > +++ b/src/compiler/spirv/vtn_variables.c
> > @@ -935,10 +935,19 @@ vtn_get_builtin_location(struct vtn_builder *b,
> > unreachable("invalid stage for SpvBuiltInViewportIndex");
> > break;
> > case SpvBuiltInTessLevelOuter:
> > + *location = VARYING_SLOT_TESS_LEVEL_OUTER;
> > + break;
> > case SpvBuiltInTessLevelInner:
> > + *location = VARYING_SLOT_TESS_LEVEL_INNER;
> > + break;
> > case SpvBuiltInTessCoord:
> > + *location = SYSTEM_VALUE_TESS_COORD;
> > + set_mode_system_value(mode);
> > + break;
> > case SpvBuiltInPatchVertices:
> > - unreachable("no tessellation support");
> > + *location = SYSTEM_VALUE_VERTICES_IN;
> > + set_mode_system_value(mode);
> > + break;
> > case SpvBuiltInFragCoord:
> > *location = VARYING_SLOT_POS;
> > assert(*mode == nir_var_shader_in);
> > @@ -1052,6 +1061,11 @@ apply_var_decoration(struct vtn_builder *b,
> > nir_variable *nir_var,
> > vtn_get_builtin_location(b, builtin, &nir_var->data.location,
> > &mode);
> > nir_var->data.mode = mode;
> >
> > + if (builtin == SpvBuiltInTessLevelOuter ||
> > + builtin == SpvBuiltInTessLevelInner) {
> > + nir_var->data.compact = true;
> > + }
> > +
>
> This is a nitpick so feel free to ignore it: maybe we we want to put
> the two ifs below in a single 'else if' block after this hunk.
>
I think I actually like it better the way he wrote it since they're setting
unrelated things. :P
> > if (builtin == SpvBuiltInFragCoord || builtin ==
> > SpvBuiltInSamplePosition)
> > nir_var->data.origin_upper_left = b->origin_upper_left;
> >
> > @@ -1076,7 +1090,7 @@ apply_var_decoration(struct vtn_builder *b,
> > nir_variable *nir_var,
> > break; /* Do nothing with these here */
> >
> > case SpvDecorationPatch:
> > - vtn_warn("Tessellation not yet supported");
> > + nir_var->data.patch = true;
> > break;
> >
> > case SpvDecorationLocation:
> > @@ -1116,6 +1130,15 @@ apply_var_decoration(struct vtn_builder *b,
> > nir_variable *nir_var,
> > }
> >
> > static void
> > +var_is_patch_cb(struct vtn_builder *b, struct vtn_value *val, int
> > member,
> > + const struct vtn_decoration *dec, void
> > *out_is_patch)
> > +{
> > + if (dec->decoration == SpvDecorationPatch) {
> > + *((bool *) out_is_patch) = true;
> > + }
> > +}
> > +
> > +static void
> > var_decoration_cb(struct vtn_builder *b, struct vtn_value *val, int
> > member,
> > const struct vtn_decoration *dec, void *void_var)
> > {
> > @@ -1132,6 +1155,9 @@ var_decoration_cb(struct vtn_builder *b, struct
> > vtn_value *val, int member,
> > case SpvDecorationInputAttachmentIndex:
> > vtn_var->input_attachment_index = dec->literals[0];
> > return;
> > + case SpvDecorationPatch:
> > + vtn_var->patch = true;
> > + break;
> > default:
> > break;
> > }
> > @@ -1162,7 +1188,7 @@ var_decoration_cb(struct vtn_builder *b, struct
> > vtn_value *val, int member,
> > } else if (vtn_var->mode == vtn_variable_mode_input ||
> > vtn_var->mode == vtn_variable_mode_output) {
> > is_vertex_input = false;
> > - location += VARYING_SLOT_VAR0;
> > + location += vtn_var->patch ? VARYING_SLOT_PATCH0 :
> > VARYING_SLOT_VAR0;
> > } else {
> > unreachable("Location must be on input or output
> > variable");
> > }
> > @@ -1209,6 +1235,24 @@ var_decoration_cb(struct vtn_builder *b,
> > struct vtn_value *val, int member,
> > }
> > }
> >
> > +static bool
> > +is_per_vertex_inout(const struct vtn_variable *var, gl_shader_stage
> > stage)
> > +{
> > + if (var->patch || !glsl_type_is_array(var->type->type))
> > + return false;
> > +
> > + if (var->mode == vtn_variable_mode_input) {
> > + return stage == MESA_SHADER_TESS_CTRL ||
> > + stage == MESA_SHADER_TESS_EVAL ||
> > + stage == MESA_SHADER_GEOMETRY;
> > + }
> > +
> > + if (var->mode == vtn_variable_mode_output)
> > + return stage == MESA_SHADER_TESS_CTRL;
> > +
> > + return false;
> > +}
> > +
> > void
> > vtn_handle_variables(struct vtn_builder *b, SpvOp opcode,
> > const uint32_t *w, unsigned count)
> > @@ -1308,6 +1352,9 @@ vtn_handle_variables(struct vtn_builder *b,
> > SpvOp opcode,
> >
> > case vtn_variable_mode_input:
> > case vtn_variable_mode_output: {
> > + var->patch = false;
> > + vtn_foreach_decoration(b, val, var_is_patch_cb, &var-
> > >patch);
> > +
> > /* For inputs and outputs, we immediately split
> > structures. This
> > * is for a couple of reasons. For one, builtins may all
> > come in
> > * a struct and we really want those split out into
> > separate
> > @@ -1318,8 +1365,7 @@ vtn_handle_variables(struct vtn_builder *b,
> > SpvOp opcode,
> >
> > int array_length = -1;
> > struct vtn_type *interface_type = var->type;
> > - if (b->shader->stage == MESA_SHADER_GEOMETRY &&
> > - glsl_type_is_array(var->type->type)) {
> > + if (is_per_vertex_inout(var, b->shader->stage)) {
> > /* In Geometry shaders (and some tessellation), inputs
> > come
> > * in per-vertex arrays. However, some builtins come in
> > * non-per-vertex, hence the need for the is_array
> > check. In
> _______________________________________________
> 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/20170109/0666fc0b/attachment-0001.html>
More information about the mesa-dev
mailing list