Re: [Mesa-dev] [PATCH v2 3/5] i965: Use brw.vue_map_geom_out instead of VS output VUE map where appropriate.

2013-03-24 Thread Paul Berry 
On 23 March 2013 10:59, Kenneth Graunke kenn...@whitecape.org wrote:

 On 03/21/2013 05:40 PM, Paul Berry wrote:

 This patch modifies post-GS pipeline stages (transform feedback, clip,
 sf, fs) to refer to the VUE map through brw-vue_map_geom_out rather
 than brw-vs.prog_data-vue_map.  This ensures that when geometry
 shader support is added, these pipeline stages will consult the
 geometry shader output VUE map when appropriate, rather than the
 vertex shader output VUE map.
 ---
   src/mesa/drivers/dri/i965/brw_**clip.c   |  7 +++
   src/mesa/drivers/dri/i965/brw_**sf.c |  7 +++
   src/mesa/drivers/dri/i965/brw_**state.h  |  2 +-
   src/mesa/drivers/dri/i965/brw_**wm.c |  6 +++---
   src/mesa/drivers/dri/i965/**gen6_sf_state.c  | 10 +-
   src/mesa/drivers/dri/i965/**gen7_sf_state.c  |  8 
   src/mesa/drivers/dri/i965/**gen7_sol_state.c | 14 +++---
   7 files changed, 26 insertions(+), 28 deletions(-)


 I heartily approve of this patch :)  It really untangles the mess of VS
 dependencies.

 Some comments below...


  diff --git a/src/mesa/drivers/dri/i965/**brw_clip.c
 b/src/mesa/drivers/dri/i965/**brw_clip.c
 index e20f7c2..fa7e85d 100644
 --- a/src/mesa/drivers/dri/i965/**brw_clip.c
 +++ b/src/mesa/drivers/dri/i965/**brw_clip.c
 @@ -69,7 +69,7 @@ static void compile_clip_prog( struct brw_context *brw,
  c.func.single_program_flow = 1;

  c.key = *key;

  -   c.vue_map = brw-vs.prog_data-vue_map;

 +   c.vue_map = brw-vue_map_geom_out;

  /* nr_regs is the number of registers filled by reading data from
 the VUE.
   * This program accesses the entire VUE, so nr_regs needs to be the
 size of
 @@ -146,7 +146,7 @@ brw_upload_clip_prog(struct brw_context *brw)
  /* BRW_NEW_REDUCED_PRIMITIVE */
  key.primitive = brw-intel.reduced_primitive;
  /* CACHE_NEW_VS_PROG (also part of VUE map) */
 -   key.attrs = brw-vs.prog_data-vue_map.**slots_valid;
 +   key.attrs = brw-vue_map_geom_out.slots_**valid;


 This comment is now wrong.  It should be
 /* BRW_NEW_VUE_MAP_GEOM_OUT */ not /* CACHE_NEW_VS_PROG */.


Oops, you're right.  I'll fix before pushing.



   /* _NEW_LIGHT */
  key.do_flat_shading = (ctx-Light.ShadeModel == GL_FLAT);
  key.pv_first = (ctx-Light.ProvokingVertex ==
 GL_FIRST_VERTEX_CONVENTION);
 @@ -258,8 +258,7 @@ const struct brw_tracked_state brw_clip_prog = {
 _NEW_TRANSFORM |
 _NEW_POLYGON |
 _NEW_BUFFERS),
 -  .brw   = (BRW_NEW_REDUCED_PRIMITIVE),
 -  .cache = CACHE_NEW_VS_PROG


 ...and I think this was actually the last use of CACHE_NEW_VS_PROG, so you
 can probably eliminate that.


Actually I did (note the - at the beginning of the line above).




  +  .brw   = (BRW_NEW_REDUCED_PRIMITIVE | BRW_NEW_VUE_MAP_GEOM_OUT)
  },
  .emit = brw_upload_clip_prog
   };
 diff --git a/src/mesa/drivers/dri/i965/**brw_sf.c
 b/src/mesa/drivers/dri/i965/**brw_sf.c
 index c8b7033..fc36961 100644
 --- a/src/mesa/drivers/dri/i965/**brw_sf.c
 +++ b/src/mesa/drivers/dri/i965/**brw_sf.c
 @@ -65,7 +65,7 @@ static void compile_sf_prog( struct brw_context *brw,
  brw_init_compile(brw, c.func, mem_ctx);

  c.key = *key;
 -   c.vue_map = brw-vs.prog_data-vue_map;
 +   c.vue_map = brw-vue_map_geom_out;
  if (c.key.do_point_coord) {
 /*
  * gl_PointCoord is a FS instead of VS builtin variable, thus it's
 @@ -145,7 +145,7 @@ brw_upload_sf_prog(struct brw_context *brw)
  /* Populate the key, noting state dependencies:
   */
  /* CACHE_NEW_VS_PROG */
 -   key.attrs = brw-vs.prog_data-vue_map.**slots_valid;


 Ditto (wrong comment)


Fixed.




  +   key.attrs = brw-vue_map_geom_out.slots_**valid;

  /* BRW_NEW_REDUCED_PRIMITIVE */
  switch (brw-intel.reduced_primitive) {
 @@ -216,8 +216,7 @@ const struct brw_tracked_state brw_sf_prog = {
  .dirty = {
 .mesa  = (_NEW_HINT | _NEW_LIGHT | _NEW_POLYGON | _NEW_POINT |
   _NEW_TRANSFORM | _NEW_BUFFERS | _NEW_PROGRAM),
 -  .brw   = (BRW_NEW_REDUCED_PRIMITIVE),
 -  .cache = CACHE_NEW_VS_PROG


 Ditto (CACHE_NEW_VS_PROG doesn't appear necessary)


Already deleted this one too.




  +  .brw   = (BRW_NEW_REDUCED_PRIMITIVE | BRW_NEW_VUE_MAP_GEOM_OUT)
  },
  .emit = brw_upload_sf_prog
   };
 diff --git a/src/mesa/drivers/dri/i965/**brw_state.h
 b/src/mesa/drivers/dri/i965/**brw_state.h
 index 02ce57b..1f5e18a 100644
 --- a/src/mesa/drivers/dri/i965/**brw_state.h
 +++ b/src/mesa/drivers/dri/i965/**brw_state.h
 @@ -227,7 +227,7 @@ void upload_default_color(struct brw_context *brw,

   /* gen6_sf_state.c */
   uint32_t
 -get_attr_override(struct brw_vue_map *vue_map, int urb_entry_read_offset,
 +get_attr_override(const struct brw_vue_map *vue_map, int
 urb_entry_read_offset,
 int fs_attr, bool two_side_color, uint32_t
 *max_source_attr);

   #ifdef __cplusplus
 diff --git a/src/mesa/drivers/dri/i965/**brw_wm.c

Re: [Mesa-dev] [PATCH v2 3/5] i965: Use brw.vue_map_geom_out instead of VS output VUE map where appropriate.

2013-03-24 Thread Kenneth Graunke 

On 03/24/2013 07:55 AM, Paul Berry wrote:

On 23 March 2013 10:59, Kenneth Graunke wrote:

[snip]

-  .brw   = (BRW_NEW_REDUCED_PRIMITIVE),
-  .cache = CACHE_NEW_VS_PROG


...and I think this was actually the last use of CACHE_NEW_VS_PROG,
so you can probably eliminate that.


Actually I did (note the - at the beginning of the line above).


Oops.  I can read :)

[snip]

I would like to see this series run through Piglit on pre-Gen6 just
to be sure we haven't broken anything.  I can do that, as I'm
setting up my Ironlake for other reasons anyway...


I wouldn't say no to additional testing, but FWIW I have tested this
series on Gen5, and there were no regressions :)


Oh!  Perfect, that's all I wanted.  I'd say go ahead and push then.

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

Re: [Mesa-dev] [PATCH v2 3/5] i965: Use brw.vue_map_geom_out instead of VS output VUE map where appropriate.

2013-03-23 Thread Kenneth Graunke 

On 03/21/2013 05:40 PM, Paul Berry wrote:

This patch modifies post-GS pipeline stages (transform feedback, clip,
sf, fs) to refer to the VUE map through brw-vue_map_geom_out rather
than brw-vs.prog_data-vue_map.  This ensures that when geometry
shader support is added, these pipeline stages will consult the
geometry shader output VUE map when appropriate, rather than the
vertex shader output VUE map.
---
  src/mesa/drivers/dri/i965/brw_clip.c   |  7 +++
  src/mesa/drivers/dri/i965/brw_sf.c |  7 +++
  src/mesa/drivers/dri/i965/brw_state.h  |  2 +-
  src/mesa/drivers/dri/i965/brw_wm.c |  6 +++---
  src/mesa/drivers/dri/i965/gen6_sf_state.c  | 10 +-
  src/mesa/drivers/dri/i965/gen7_sf_state.c  |  8 
  src/mesa/drivers/dri/i965/gen7_sol_state.c | 14 +++---
  7 files changed, 26 insertions(+), 28 deletions(-)


I heartily approve of this patch :)  It really untangles the mess of VS 
dependencies.


Some comments below...


diff --git a/src/mesa/drivers/dri/i965/brw_clip.c 
b/src/mesa/drivers/dri/i965/brw_clip.c
index e20f7c2..fa7e85d 100644
--- a/src/mesa/drivers/dri/i965/brw_clip.c
+++ b/src/mesa/drivers/dri/i965/brw_clip.c
@@ -69,7 +69,7 @@ static void compile_clip_prog( struct brw_context *brw,
 c.func.single_program_flow = 1;

 c.key = *key;

 -   c.vue_map = brw-vs.prog_data-vue_map;

+   c.vue_map = brw-vue_map_geom_out;

 /* nr_regs is the number of registers filled by reading data from the VUE.
  * This program accesses the entire VUE, so nr_regs needs to be the size of
@@ -146,7 +146,7 @@ brw_upload_clip_prog(struct brw_context *brw)
 /* BRW_NEW_REDUCED_PRIMITIVE */
 key.primitive = brw-intel.reduced_primitive;
 /* CACHE_NEW_VS_PROG (also part of VUE map) */
-   key.attrs = brw-vs.prog_data-vue_map.slots_valid;
+   key.attrs = brw-vue_map_geom_out.slots_valid;


This comment is now wrong.  It should be
/* BRW_NEW_VUE_MAP_GEOM_OUT */ not /* CACHE_NEW_VS_PROG */.


 /* _NEW_LIGHT */
 key.do_flat_shading = (ctx-Light.ShadeModel == GL_FLAT);
 key.pv_first = (ctx-Light.ProvokingVertex == GL_FIRST_VERTEX_CONVENTION);
@@ -258,8 +258,7 @@ const struct brw_tracked_state brw_clip_prog = {
_NEW_TRANSFORM |
_NEW_POLYGON |
_NEW_BUFFERS),
-  .brw   = (BRW_NEW_REDUCED_PRIMITIVE),
-  .cache = CACHE_NEW_VS_PROG


...and I think this was actually the last use of CACHE_NEW_VS_PROG, so 
you can probably eliminate that.



+  .brw   = (BRW_NEW_REDUCED_PRIMITIVE | BRW_NEW_VUE_MAP_GEOM_OUT)
 },
 .emit = brw_upload_clip_prog
  };
diff --git a/src/mesa/drivers/dri/i965/brw_sf.c 
b/src/mesa/drivers/dri/i965/brw_sf.c
index c8b7033..fc36961 100644
--- a/src/mesa/drivers/dri/i965/brw_sf.c
+++ b/src/mesa/drivers/dri/i965/brw_sf.c
@@ -65,7 +65,7 @@ static void compile_sf_prog( struct brw_context *brw,
 brw_init_compile(brw, c.func, mem_ctx);

 c.key = *key;
-   c.vue_map = brw-vs.prog_data-vue_map;
+   c.vue_map = brw-vue_map_geom_out;
 if (c.key.do_point_coord) {
/*
 * gl_PointCoord is a FS instead of VS builtin variable, thus it's
@@ -145,7 +145,7 @@ brw_upload_sf_prog(struct brw_context *brw)
 /* Populate the key, noting state dependencies:
  */
 /* CACHE_NEW_VS_PROG */
-   key.attrs = brw-vs.prog_data-vue_map.slots_valid;


Ditto (wrong comment)


+   key.attrs = brw-vue_map_geom_out.slots_valid;

 /* BRW_NEW_REDUCED_PRIMITIVE */
 switch (brw-intel.reduced_primitive) {
@@ -216,8 +216,7 @@ const struct brw_tracked_state brw_sf_prog = {
 .dirty = {
.mesa  = (_NEW_HINT | _NEW_LIGHT | _NEW_POLYGON | _NEW_POINT |
  _NEW_TRANSFORM | _NEW_BUFFERS | _NEW_PROGRAM),
-  .brw   = (BRW_NEW_REDUCED_PRIMITIVE),
-  .cache = CACHE_NEW_VS_PROG


Ditto (CACHE_NEW_VS_PROG doesn't appear necessary)


+  .brw   = (BRW_NEW_REDUCED_PRIMITIVE | BRW_NEW_VUE_MAP_GEOM_OUT)
 },
 .emit = brw_upload_sf_prog
  };
diff --git a/src/mesa/drivers/dri/i965/brw_state.h 
b/src/mesa/drivers/dri/i965/brw_state.h
index 02ce57b..1f5e18a 100644
--- a/src/mesa/drivers/dri/i965/brw_state.h
+++ b/src/mesa/drivers/dri/i965/brw_state.h
@@ -227,7 +227,7 @@ void upload_default_color(struct brw_context *brw,

  /* gen6_sf_state.c */
  uint32_t
-get_attr_override(struct brw_vue_map *vue_map, int urb_entry_read_offset,
+get_attr_override(const struct brw_vue_map *vue_map, int urb_entry_read_offset,
int fs_attr, bool two_side_color, uint32_t 
*max_source_attr);

  #ifdef __cplusplus
diff --git a/src/mesa/drivers/dri/i965/brw_wm.c 
b/src/mesa/drivers/dri/i965/brw_wm.c
index e7e9ddc..6053f94 100644
--- a/src/mesa/drivers/dri/i965/brw_wm.c
+++ b/src/mesa/drivers/dri/i965/brw_wm.c
@@ -481,7 +481,7 @@ static void brw_wm_populate_key( struct brw_context *brw,

 /* CACHE_NEW_VS_PROG */
 if (intel-gen  6)
-  key-vp_outputs_written = brw-vs.prog_data-vue_map.slots_valid;
+  key-vp_outputs_written =

[Mesa-dev] [PATCH v2 3/5] i965: Use brw.vue_map_geom_out instead of VS output VUE map where appropriate.

2013-03-21 Thread Paul Berry 
This patch modifies post-GS pipeline stages (transform feedback, clip,
sf, fs) to refer to the VUE map through brw-vue_map_geom_out rather
than brw-vs.prog_data-vue_map.  This ensures that when geometry
shader support is added, these pipeline stages will consult the
geometry shader output VUE map when appropriate, rather than the
vertex shader output VUE map.
---
 src/mesa/drivers/dri/i965/brw_clip.c   |  7 +++
 src/mesa/drivers/dri/i965/brw_sf.c |  7 +++
 src/mesa/drivers/dri/i965/brw_state.h  |  2 +-
 src/mesa/drivers/dri/i965/brw_wm.c |  6 +++---
 src/mesa/drivers/dri/i965/gen6_sf_state.c  | 10 +-
 src/mesa/drivers/dri/i965/gen7_sf_state.c  |  8 
 src/mesa/drivers/dri/i965/gen7_sol_state.c | 14 +++---
 7 files changed, 26 insertions(+), 28 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_clip.c 
b/src/mesa/drivers/dri/i965/brw_clip.c
index e20f7c2..fa7e85d 100644
--- a/src/mesa/drivers/dri/i965/brw_clip.c
+++ b/src/mesa/drivers/dri/i965/brw_clip.c
@@ -69,7 +69,7 @@ static void compile_clip_prog( struct brw_context *brw,
c.func.single_program_flow = 1;
 
c.key = *key;
-   c.vue_map = brw-vs.prog_data-vue_map;
+   c.vue_map = brw-vue_map_geom_out;
 
/* nr_regs is the number of registers filled by reading data from the VUE.
 * This program accesses the entire VUE, so nr_regs needs to be the size of
@@ -146,7 +146,7 @@ brw_upload_clip_prog(struct brw_context *brw)
/* BRW_NEW_REDUCED_PRIMITIVE */
key.primitive = brw-intel.reduced_primitive;
/* CACHE_NEW_VS_PROG (also part of VUE map) */
-   key.attrs = brw-vs.prog_data-vue_map.slots_valid;
+   key.attrs = brw-vue_map_geom_out.slots_valid;
/* _NEW_LIGHT */
key.do_flat_shading = (ctx-Light.ShadeModel == GL_FLAT);
key.pv_first = (ctx-Light.ProvokingVertex == GL_FIRST_VERTEX_CONVENTION);
@@ -258,8 +258,7 @@ const struct brw_tracked_state brw_clip_prog = {
_NEW_TRANSFORM |
_NEW_POLYGON | 
_NEW_BUFFERS),
-  .brw   = (BRW_NEW_REDUCED_PRIMITIVE),
-  .cache = CACHE_NEW_VS_PROG
+  .brw   = (BRW_NEW_REDUCED_PRIMITIVE | BRW_NEW_VUE_MAP_GEOM_OUT)
},
.emit = brw_upload_clip_prog
 };
diff --git a/src/mesa/drivers/dri/i965/brw_sf.c 
b/src/mesa/drivers/dri/i965/brw_sf.c
index c8b7033..fc36961 100644
--- a/src/mesa/drivers/dri/i965/brw_sf.c
+++ b/src/mesa/drivers/dri/i965/brw_sf.c
@@ -65,7 +65,7 @@ static void compile_sf_prog( struct brw_context *brw,
brw_init_compile(brw, c.func, mem_ctx);
 
c.key = *key;
-   c.vue_map = brw-vs.prog_data-vue_map;
+   c.vue_map = brw-vue_map_geom_out;
if (c.key.do_point_coord) {
   /*
* gl_PointCoord is a FS instead of VS builtin variable, thus it's
@@ -145,7 +145,7 @@ brw_upload_sf_prog(struct brw_context *brw)
/* Populate the key, noting state dependencies:
 */
/* CACHE_NEW_VS_PROG */
-   key.attrs = brw-vs.prog_data-vue_map.slots_valid;
+   key.attrs = brw-vue_map_geom_out.slots_valid;
 
/* BRW_NEW_REDUCED_PRIMITIVE */
switch (brw-intel.reduced_primitive) {
@@ -216,8 +216,7 @@ const struct brw_tracked_state brw_sf_prog = {
.dirty = {
   .mesa  = (_NEW_HINT | _NEW_LIGHT | _NEW_POLYGON | _NEW_POINT |
 _NEW_TRANSFORM | _NEW_BUFFERS | _NEW_PROGRAM),
-  .brw   = (BRW_NEW_REDUCED_PRIMITIVE),
-  .cache = CACHE_NEW_VS_PROG
+  .brw   = (BRW_NEW_REDUCED_PRIMITIVE | BRW_NEW_VUE_MAP_GEOM_OUT)
},
.emit = brw_upload_sf_prog
 };
diff --git a/src/mesa/drivers/dri/i965/brw_state.h 
b/src/mesa/drivers/dri/i965/brw_state.h
index 02ce57b..1f5e18a 100644
--- a/src/mesa/drivers/dri/i965/brw_state.h
+++ b/src/mesa/drivers/dri/i965/brw_state.h
@@ -227,7 +227,7 @@ void upload_default_color(struct brw_context *brw,
 
 /* gen6_sf_state.c */
 uint32_t
-get_attr_override(struct brw_vue_map *vue_map, int urb_entry_read_offset,
+get_attr_override(const struct brw_vue_map *vue_map, int urb_entry_read_offset,
   int fs_attr, bool two_side_color, uint32_t *max_source_attr);
 
 #ifdef __cplusplus
diff --git a/src/mesa/drivers/dri/i965/brw_wm.c 
b/src/mesa/drivers/dri/i965/brw_wm.c
index e7e9ddc..6053f94 100644
--- a/src/mesa/drivers/dri/i965/brw_wm.c
+++ b/src/mesa/drivers/dri/i965/brw_wm.c
@@ -481,7 +481,7 @@ static void brw_wm_populate_key( struct brw_context *brw,
 
/* CACHE_NEW_VS_PROG */
if (intel-gen  6)
-  key-vp_outputs_written = brw-vs.prog_data-vue_map.slots_valid;
+  key-vp_outputs_written = brw-vue_map_geom_out.slots_valid;
 
/* The unique fragment program ID */
key-program_string_id = fp-id;
@@ -524,8 +524,8 @@ const struct brw_tracked_state brw_wm_prog = {
_NEW_MULTISAMPLE),
   .brw   = (BRW_NEW_FRAGMENT_PROGRAM |
BRW_NEW_WM_INPUT_DIMENSIONS |
-   BRW_NEW_REDUCED_PRIMITIVE),
-  .cache = CACHE_NEW_VS_PROG,
+   BRW_NEW_REDUCED_PRIMITIVE |
+BRW_NEW_VUE_MAP_GEOM_OUT)
},
.emit =