[Mesa-dev] [PATCH] tgsi/nouveau: Add support for tesselation ctrl and tesselation eval
Marcos Souza
marcos.souza.org at gmail.com
Thu Aug 13 21:43:16 PDT 2015
HI Ilia
2015-08-14 1:31 GMT-03:00 Ilia Mirkin <imirkin at alum.mit.edu>:
> On Fri, Aug 14, 2015 at 12:25 AM, Marcos Souza
> <marcos.souza.org at gmail.com> wrote:
> > Hi Ilia,
> >
> > 2015-08-14 1:02 GMT-03:00 Ilia Mirkin <imirkin at alum.mit.edu>:
> >>
> >> On Thu, Aug 13, 2015 at 11:55 PM, Marcos Souza
> >> <marcos.souza.org at gmail.com> wrote:
> >> > Hi Ilia,
> >> >
> >> > So i found the point here it addrs that double brackets, and the
> >> > following
> >> > patch solves it, but this is a right solution? If someone could guide
> me
> >> > here, I could fix it :)
> >> >
> >> > diff --git a/src/gallium/auxiliary/tgsi/tgsi_dump.c
> >> > b/src/gallium/auxiliary/tgsi/tgsi_dump.c
> >> > index 8ceb5b4..046471e 100644
> >> > --- a/src/gallium/auxiliary/tgsi/tgsi_dump.c
> >> > +++ b/src/gallium/auxiliary/tgsi/tgsi_dump.c
> >> > @@ -302,10 +302,14 @@ iter_declaration(
> >> > TXT("[]");
> >> > }
> >> >
> >> > - if (decl->Declaration.Dimension) {
> >>
> >> The issue is that the declaration is getting a dimension set by the
> >> parser, which in turn is causing it to print funny. It shouldn't be
> >> getting a dimension in the first place for those.
> >
> >
> > The following patch fix the problem, is it the right place to put it?
>
> I don't think so. Just glanced at the code, look at
>
> parse_register_dcl
>
> /* for geometry shader we don't really care about
> * the first brackets it's always the size of the
> * input primitive. so we want to declare just
> * the index relevant to the semantics which is in
> * the second bracket */
> if (ctx->processor == TGSI_PROCESSOR_GEOMETRY && *file ==
> TGSI_FILE_INPUT) {
> brackets[0] = brackets[1];
> *num_brackets = 1;
> }
>
> Basically you need to extend this logic to similarly exclude
>
> (a) tess ctrl inputs and outputs
> (b) tess eval inputs
>
> Technically you need to exclude patch/tessinner/tessouter from that,
> but in practice they won't have an extra set of brackets either.
>
>
Sorry for flooding the list, but I'm relaly excited about it :)
So, this is the change you asked. It also solved the problem:
diff --git a/src/gallium/auxiliary/tgsi/tgsi_text.c
b/src/gallium/auxiliary/tgsi/tgsi_text.c
index 8647e4e..95c1daf 100644
--- a/src/gallium/auxiliary/tgsi/tgsi_text.c
+++ b/src/gallium/auxiliary/tgsi/tgsi_text.c
@@ -684,7 +684,12 @@ parse_register_dcl(
* input primitive. so we want to declare just
* the index relevant to the semantics which is in
* the second bracket */
- if (ctx->processor == TGSI_PROCESSOR_GEOMETRY && *file ==
TGSI_FILE_INPUT) {
+
+ /* similarly from tessalation */
+ int exclude = (ctx->processor == TGSI_PROCESSOR_TESS_EVAL && *file
== TGSI_FILE_INPUT) ||
+ (ctx->processor == TGSI_PROCESSOR_TESS_CTRL && (*file ==
TGSI_FILE_INPUT ||
+ *file == TGSI_FILE_OUTPUT));
+ if ((ctx->processor == TGSI_PROCESSOR_GEOMETRY && *file ==
TGSI_FILE_INPUT) || exclude) {
brackets[0] = brackets[1];
*num_brackets = 1;
} else {
What do you think Ilia?
Thanks!
> >
> > diff --git a/src/gallium/auxiliary/tgsi/tgsi_text.c
> > b/src/gallium/auxiliary/tgsi/tgsi_text.c
> > index 8647e4e..f734d58 100644
> > --- a/src/gallium/auxiliary/tgsi/tgsi_text.c
> > +++ b/src/gallium/auxiliary/tgsi/tgsi_text.c
> > @@ -1185,7 +1185,10 @@ static boolean parse_declaration( struct
> > translate_ctx *ctx )
> > decl.Range.First = brackets[1].first;
> > decl.Range.Last = brackets[1].last;
> >
> > - decl.Declaration.Dimension = 1;
> > + if (!(ctx->processor == TGSI_PROCESSOR_TESS_CTRL ||
> > + ctx->processor == TGSI_PROCESSOR_TESS_EVAL))
> > + decl.Declaration.Dimension = 1;
> > +
> > decl.Dim.Index2D = brackets[0].first;
> > }
> >
> >
> >
> >>
> >>
> >> > - CHR('[');
> >> > - SID(decl->Dim.Index2D);
> >> > - CHR(']');
> >> > + /* FIXME: patched version could have tree dimensions?? */
> >> > + if (patch && (iter->processor.Processor ==
> TGSI_PROCESSOR_TESS_CTRL
> >> > ||
> >> > + iter->processor.Processor == TGSI_PROCESSOR_TESS_EVAL)) {
> >> > + if (decl->Declaration.Dimension) {
> >> > + CHR('[');
> >> > + SID(decl->Dim.Index2D);
> >> > + CHR(']');
> >> > + }
> >> > }
> >> >
> >> > After this patch, tess_eval output is the same before and after, but
> >> > tess_ctrl is a little different:
> >> > [marcos at x mesa]$ diff tess_ctrl tess_ctrl_new
> >> > 29c29
> >> > < 15: LRP OUT[ADDR[1].x][3], TEMP[1].xxxx, TEMP[3], TEMP[2]
> >> > ---
> >> >> 15: LRP OUT[0][3], TEMP[1].xxxx, TEMP[3], TEMP[2]
> >> > 40c40
> >> > < 26: MOV OUT[ADDR[1].x][2], TEMP[0]
> >> > ---
> >> >> 26: MOV OUT[0][2], TEMP[0]
> >> >
> >> > I'll try to investigate and send a new patch in the weekend.
> >> >
> >> > Thanks for all help Ilia and others!
> >> >
> >> > 2015-08-13 18:43 GMT-03:00 Ilia Mirkin <imirkin at alum.mit.edu>:
> >> >>
> >> >> [mesa-dev readded, please don't drop CC's]
> >> >>
> >> >> I found it by feeding the shader to nouveau_compiler with
> >> >> NV50_PROG_DEBUG=1 set, which dumps the input tgsi. Those two should
> >> >> match up.
> >> >>
> >> >> On Thu, Aug 13, 2015 at 5:39 PM, Marcos Paulo de souza
> >> >> <marcos.souza.org at gmail.com> wrote:
> >> >> > Hi Ilia,
> >> >> >
> >> >> > So, how can I test it? Do I need to especify some patameter to
> verify
> >> >> > this
> >> >> > type of problem?
> >> >> >
> >> >> > Thanks for the quick revision!
> >> >> >
> >> >> >
> >> >> > Em 13-08-2015 16:03, Ilia Mirkin escreveu:
> >> >> >>
> >> >> >> Hi Macros,
> >> >> >>
> >> >> >> Looks like it's not parsed in exactly right. It will parse
> something
> >> >> >> like
> >> >> >>
> >> >> >> TESS_EVAL
> >> >> >> PROPERTY TES_PRIM_MODE 7
> >> >> >> PROPERTY TES_SPACING 2
> >> >> >> PROPERTY TES_VERTEX_ORDER_CW 0
> >> >> >> PROPERTY TES_POINT_MODE 0
> >> >> >> DCL IN[][0], GENERIC[0]
> >> >> >> DCL IN[][1], GENERIC[1]
> >> >> >>
> >> >> >> as
> >> >> >>
> >> >> >> TESS_EVAL
> >> >> >> PROPERTY TES_PRIM_MODE 7
> >> >> >> PROPERTY TES_SPACING 2
> >> >> >> PROPERTY TES_VERTEX_ORDER_CW 0
> >> >> >> PROPERTY TES_POINT_MODE 0
> >> >> >> DCL IN[][0][0], GENERIC[0]
> >> >> >> DCL IN[][0][1], GENERIC[1]
> >> >> >>
> >> >> >> Perhaps the same issue happens for geometry shaders, but that
> >> >> >> doesn't
> >> >> >> make it right :) You might have to look at the printing logic to
> get
> >> >> >> a
> >> >> >> better understanding of what's going wrong.
> >> >> >>
> >> >> >> Also you should send patches to nouveau separately from patches to
> >> >> >> the
> >> >> >> rest of the infra. Ideally this would have been 2 patches, e.g.
> >> >> >>
> >> >> >> tgsi: set implicit array size for tess stages
> >> >> >> nouveau: recognize tess stages in nouveau_compiler
> >> >> >>
> >> >> >> or something like that.
> >> >> >>
> >> >> >> On Wed, Aug 12, 2015 at 9:25 PM, Marcos Paulo de Souza
> >> >> >> <marcos.souza.org at gmail.com> wrote:
> >> >> >>>
> >> >> >>> From: Marcos Paulo de Souza <marcos.souza.org at gmail.com>
> >> >> >>>
> >> >> >>> Signed-off-by: Marcos Paulo de Souza <marcos.souza.org>
> >> >> >>> Suggested-by: Ilia Mirkin <imirkin at alum.mit.edu>
> >> >> >>> ---
> >> >> >>> src/gallium/auxiliary/tgsi/tgsi_text.c | 6 +++++-
> >> >> >>> src/gallium/drivers/nouveau/nouveau_compiler.c | 4 ++++
> >> >> >>> 2 files changed, 9 insertions(+), 1 deletion(-)
> >> >> >>>
> >> >> >>> diff --git a/src/gallium/auxiliary/tgsi/tgsi_text.c
> >> >> >>> b/src/gallium/auxiliary/tgsi/tgsi_text.c
> >> >> >>> index a6675c5..8647e4e 100644
> >> >> >>> --- a/src/gallium/auxiliary/tgsi/tgsi_text.c
> >> >> >>> +++ b/src/gallium/auxiliary/tgsi/tgsi_text.c
> >> >> >>> @@ -259,7 +259,7 @@ struct translate_ctx
> >> >> >>> struct tgsi_token *tokens_end;
> >> >> >>> struct tgsi_header *header;
> >> >> >>> unsigned processor : 4;
> >> >> >>> - int implied_array_size : 5;
> >> >> >>> + int implied_array_size : 6;
> >> >> >>> unsigned num_immediates;
> >> >> >>> };
> >> >> >>>
> >> >> >>> @@ -1623,6 +1623,10 @@ static boolean translate( struct
> >> >> >>> translate_ctx
> >> >> >>> *ctx )
> >> >> >>> if (!parse_header( ctx ))
> >> >> >>> return FALSE;
> >> >> >>>
> >> >> >>> + if (ctx->processor == TGSI_PROCESSOR_TESS_CTRL ||
> >> >> >>> + ctx->processor == TGSI_PROCESSOR_TESS_EVAL)
> >> >> >>> + ctx->implied_array_size = 32 ;
> >> >> >>> +
> >> >> >>> while (*ctx->cur != '\0') {
> >> >> >>> uint label_val = 0;
> >> >> >>> if (!eat_white( &ctx->cur )) {
> >> >> >>> diff --git a/src/gallium/drivers/nouveau/nouveau_compiler.c
> >> >> >>> b/src/gallium/drivers/nouveau/nouveau_compiler.c
> >> >> >>> index 8660498..495450b 100644
> >> >> >>> --- a/src/gallium/drivers/nouveau/nouveau_compiler.c
> >> >> >>> +++ b/src/gallium/drivers/nouveau/nouveau_compiler.c
> >> >> >>> @@ -190,6 +190,10 @@ main(int argc, char *argv[])
> >> >> >>> type = PIPE_SHADER_GEOMETRY;
> >> >> >>> else if (!strncmp(text, "COMP", 4))
> >> >> >>> type = PIPE_SHADER_COMPUTE;
> >> >> >>> + else if (!strncmp(text, "TESS_CTRL", 9))
> >> >> >>> + type = PIPE_SHADER_TESS_CTRL;
> >> >> >>> + else if (!strncmp(text, "TESS_EVAL", 9))
> >> >> >>> + type = PIPE_SHADER_TESS_EVAL;
> >> >> >>> else {
> >> >> >>> _debug_printf("Unrecognized TGSI header\n");
> >> >> >>> return 1;
> >> >> >>> --
> >> >> >>> 2.4.3
> >> >> >>>
> >> >> >>> _______________________________________________
> >> >> >>> mesa-dev mailing list
> >> >> >>> mesa-dev at lists.freedesktop.org
> >> >> >>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >> >> >
> >> >> >
> >> >
> >> >
> >> >
> >> >
> >> > --
> >> > Att,
> >> >
> >> > Marcos Paulo de Souza
> >> > Github: https://github.com/marcosps/
> >
> >
> >
> >
> > --
> > Att,
> >
> > Marcos Paulo de Souza
> > Github: https://github.com/marcosps/
>
--
Att,
Marcos Paulo de Souza
Github: https://github.com/marcosps/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150814/abd25e25/attachment-0001.html>
More information about the mesa-dev
mailing list