from:"Marcos Souza"

Re: [Mesa-dev] [PATCH] tgsi/nouveau: Add support for tesselation ctrl and tesselation eval

2015-08-16 Thread Marcos Souza

Hi Ilia,

2015-08-14 1:02 GMT-03:00 Ilia Mirkin :

> On Thu, Aug 13, 2015 at 11:55 PM, Marcos Souza
>  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?

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@x mesa]$ diff tess_ctrl tess_ctrl_new
> > 29c29
> > <  15: LRP OUT[ADDR[1].x][3], TEMP[1]., TEMP[3], TEMP[2]
> > ---
> >>  15: LRP OUT[0][3], TEMP[1]., 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 :
> >>
> >> [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
> >>  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.
>

Re: [Mesa-dev] [PATCH] tgsi/nouveau: Add support for tesselation ctrl and tesselation eval

2015-08-16 Thread Marcos Souza

HI Ilia

2015-08-14 1:31 GMT-03:00 Ilia Mirkin :

> On Fri, Aug 14, 2015 at 12:25 AM, Marcos Souza
>  wrote:
> > Hi Ilia,
> >
> > 2015-08-14 1:02 GMT-03:00 Ilia Mirkin :
> >>
> >> On Thu, Aug 13, 2015 at 11:55 PM, Marcos Souza
> >>  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.

Re: [Mesa-dev] [PATCH] tgsi/nouveau: Add support for tesselation ctrl and tesselation eval

2015-08-16 Thread Marcos Souza

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) {
-  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@x mesa]$ diff tess_ctrl tess_ctrl_new
29c29
<  15: LRP OUT[ADDR[1].x][3], TEMP[1]., TEMP[3], TEMP[2]
---
>  15: LRP OUT[0][3], TEMP[1]., 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 :

> [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
>  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
> >>  wrote:
> >>>
> >>> From: Marcos Paulo de Souza 
> >>>
> >>> Signed-off-by: Marcos Paulo de Souza 
> >>> Suggested-by: Ilia Mirkin 
> >>> ---
> >>>   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

Re: [Mesa-dev] [PATCH] tgsi/nouveau: Add support for tesselation ctrl and tesselation eval

2015-08-16 Thread Marcos Souza

2015-08-14 1:55 GMT-03:00 Ilia Mirkin :

> On Fri, Aug 14, 2015 at 12:52 AM, Marcos Paulo de souza
>  wrote:
> > Now I'll take a look about the last problem of LRP and MOV.
>
> That should ideally have solved itself too... if not, do you have the
> full shader that demonstrates the problem?
>

Yes, there it is:

TESS_CTRL
PROPERTY TCS_VERTICES_OUT 9
DCL IN[][0], POSITION
DCL SV[0], INVOCATIONID
DCL SV[1], VERTICESIN
DCL OUT[0], TESSOUTER
DCL OUT[1], TESSINNER
DCL OUT[][2], GENERIC[0]
DCL OUT[][3], GENERIC[1]
DCL TEMP[0..3], LOCAL
DCL ADDR[0..1]
IMM[0] FLT32 {   21., 0.5000, 0., 1.}
IMM[1] INT32 {3, 4, 0, 0}
  0: MOV OUT[1].x, IMM[0].
  1: MOV OUT[1].y, IMM[0].
  2: MOV OUT[0].x, IMM[0].
  3: MOV OUT[0].y, IMM[0].
  4: MOV OUT[0].z, IMM[0].
  5: MOV OUT[0].w, IMM[0].
  6: MOD TEMP[0].x, SV[0]., IMM[1].
  7: I2F TEMP[0].x, TEMP[0].
  8: MUL TEMP[0].x, TEMP[0]., IMM[0].
  9: IDIV TEMP[1].x, SV[0]., IMM[1].
 10: I2F TEMP[1].x, TEMP[1].
 11: MUL TEMP[1].x, TEMP[1]., IMM[0].
 12: LRP TEMP[2], TEMP[0]., IN[1][0], IN[0][0]
 13: LRP TEMP[3], TEMP[0]., IN[3][0], IN[2][0]
 14: UARL ADDR[1].x, SV[0].
 15: LRP OUT[ADDR[1].x][3], TEMP[1]., TEMP[3], TEMP[2]
 16: USEQ TEMP[2].x, SV[1]., IMM[1].
 17: UIF TEMP[2]. :0
 18:   MOV TEMP[2].zw, IMM[0].wwzw
 19:   MOV TEMP[2].x, TEMP[0].
 20:   MOV TEMP[2].y, TEMP[1].
 21:   MOV TEMP[0], TEMP[2]
 22: ELSE :0
 23:   MOV TEMP[0], IMM[0].
 24: ENDIF
 25: UARL ADDR[1].x, SV[0].
 26: MOV OUT[ADDR[1].x][2], TEMP[0]
 27: END

Can you give me some tip to fix it, or do you think this can be sent in
another patch rather than the tgsi and the nouai part?

Thanks



-- 
Att,

Marcos Paulo de Souza
Github: https://github.com/marcosps/
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] tgsi/nouveau: Add support for tesselation ctrl and tesselation eval

Re: [Mesa-dev] [PATCH] tgsi/nouveau: Add support for tesselation ctrl and tesselation eval

Re: [Mesa-dev] [PATCH] tgsi/nouveau: Add support for tesselation ctrl and tesselation eval

Re: [Mesa-dev] [PATCH] tgsi/nouveau: Add support for tesselation ctrl and tesselation eval

4 matches

Site Navigation

Mail list logo

Footer information