Re: [Mesa-dev] [PATCH] i965: handle gl_PointCoord for Gen4 and Gen5 platforms

2012-03-06 Thread Eric Anholt 
On Mon, 27 Feb 2012 15:46:32 +0800, Yuanhan Liu yuanhan@linux.intel.com 
wrote:
 diff --git a/src/mesa/drivers/dri/i965/brw_sf.c 
 b/src/mesa/drivers/dri/i965/brw_sf.c
 index 6e63583..7950c47 100644
 --- a/src/mesa/drivers/dri/i965/brw_sf.c
 +++ b/src/mesa/drivers/dri/i965/brw_sf.c
 @@ -125,6 +135,7 @@ brw_upload_sf_prog(struct brw_context *brw)
  {
 struct gl_context *ctx = brw-intel.ctx;
 struct brw_sf_prog_key key;

There should be a /* _NEW_BUFFERS */ comment here to note the state flag
dependency.

 +   bool render_to_fbo = ctx-DrawBuffer-Name != 0;

Other than that,

Reviewed-by: Eric Anholt e...@anholt.net


pgpyenFytQOyB.pgp
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: handle gl_PointCoord for Gen4 and Gen5 platforms

2012-03-06 Thread Yuanhan Liu 
On Tue, Mar 06, 2012 at 08:25:10AM -0800, Eric Anholt wrote:
 On Mon, 27 Feb 2012 15:46:32 +0800, Yuanhan Liu yuanhan@linux.intel.com 
 wrote:
  diff --git a/src/mesa/drivers/dri/i965/brw_sf.c 
  b/src/mesa/drivers/dri/i965/brw_sf.c
  index 6e63583..7950c47 100644
  --- a/src/mesa/drivers/dri/i965/brw_sf.c
  +++ b/src/mesa/drivers/dri/i965/brw_sf.c
  @@ -125,6 +135,7 @@ brw_upload_sf_prog(struct brw_context *brw)
   {
  struct gl_context *ctx = brw-intel.ctx;
  struct brw_sf_prog_key key;
 
 There should be a /* _NEW_BUFFERS */ comment here to note the state flag
 dependency.

Ok.

 
  +   bool render_to_fbo = ctx-DrawBuffer-Name != 0;
 
 Other than that,
 
 Reviewed-by: Eric Anholt e...@anholt.net

Thanks for the Reviewed-by, which I have been waiting for a quite while ;) 

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

Re: [Mesa-dev] [PATCH] i965: handle gl_PointCoord for Gen4 and Gen5 platforms

2012-02-28 Thread Yuanhan Liu 
Ping..

Comments?

Thanks,
Yuanhan Liu

On Mon, Feb 27, 2012 at 03:46:32PM +0800, Yuanhan Liu wrote:
 This patch add the support of gl_PointCoord gl builtin variable for
 platform gen4 and gen5(ILK).
 
 Unlike gen6+, we don't have a hardware support of gl_PointCoord, means
 hardware will not calculate the interpolation coefficient for you.
 Instead, you should handle it yourself in sf shader stage.
 
 But badly, gl_PointCoord is a FS instead of VS builtin variable, thus
 it's not included in c.vue_map generated in VS stage. Thus the current
 code doesn't aware of this attribute. And to handle it correctly, we
 need add it to c.vue_map manually to let SF shader generate the needed
 interpolation coefficient for FS shader. SF stage has it's own copy of
 vue_map, thus I think it's safe to do it manually.
 
 Since handling gl_PointCoord for gen4 and gen5 platforms is somehow a
 little special, I added a lot of comments and hope I didn't overdo it ;)
 
 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=45975
 Piglit: glsl-fs-pointcoord and fbo-gl_pointcoord
 
 NOTE: This is a candidate for stable release branches.
 
 Signed-off-by: Yuanhan Liu yuanhan@linux.intel.com
 ---
  src/mesa/drivers/dri/i965/brw_context.h |6 ++
  src/mesa/drivers/dri/i965/brw_fs.cpp|9 +
  src/mesa/drivers/dri/i965/brw_sf.c  |   26 ++
  src/mesa/drivers/dri/i965/brw_sf.h  |1 +
  src/mesa/drivers/dri/i965/brw_sf_emit.c |4 
  5 files changed, 42 insertions(+), 4 deletions(-)
 
 diff --git a/src/mesa/drivers/dri/i965/brw_context.h 
 b/src/mesa/drivers/dri/i965/brw_context.h
 index 09d8373..7c794ad 100644
 --- a/src/mesa/drivers/dri/i965/brw_context.h
 +++ b/src/mesa/drivers/dri/i965/brw_context.h
 @@ -296,6 +296,12 @@ typedef enum
 BRW_VERT_RESULT_NDC = VERT_RESULT_MAX,
 BRW_VERT_RESULT_HPOS_DUPLICATE,
 BRW_VERT_RESULT_PAD,
 +   /*
 +* It's actually not a vert_result but just a _mark_ to let sf aware that
 +* he need do something special to handle gl_PointCoord builtin variable
 +* correctly. see compile_sf_prog() for more info.
 +*/
 +   BRW_VERT_RESULT_PNTC,
 BRW_VERT_RESULT_MAX
  } brw_vert_result;
  
 diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp 
 b/src/mesa/drivers/dri/i965/brw_fs.cpp
 index bf59da3..5f3d79d 100644
 --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
 +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
 @@ -710,6 +710,15 @@ fs_visitor::calculate_urb_setup()
  urb_setup[fp_index] = urb_next++;
}
}
 +
 +  /*
 +   * It's a FS only attribute, and we did interpolation for this 
 attribute
 +   * in SF thread. So, count it here, too.
 +   *
 +   * See compile_sf_prog() for more info.
 +   */
 +  if (brw-fragment_program-Base.InputsRead  
 BITFIELD64_BIT(FRAG_ATTRIB_PNTC))
 + urb_setup[FRAG_ATTRIB_PNTC] = urb_next++;
 }
  
 /* Each attribute is 4 setup channels, each of which is half a reg. */
 diff --git a/src/mesa/drivers/dri/i965/brw_sf.c 
 b/src/mesa/drivers/dri/i965/brw_sf.c
 index 6e63583..7950c47 100644
 --- a/src/mesa/drivers/dri/i965/brw_sf.c
 +++ b/src/mesa/drivers/dri/i965/brw_sf.c
 @@ -64,6 +64,16 @@ static void compile_sf_prog( struct brw_context *brw,
  
 c.key = *key;
 c.vue_map = brw-vs.prog_data-vue_map;
 +   if (c.key.do_point_coord) {
 +  /*
 +   * gl_PointCoord is a FS instead of VS builtin variable, thus it's
 +   * not included in c.vue_map generated in VS stage. Here we add
 +   * it manually to let SF shader generate the needed interpolation
 +   * coefficient for FS shader.
 +   */
 +  c.vue_map.vert_result_to_slot[BRW_VERT_RESULT_PNTC] = 
 c.vue_map.num_slots;
 +  c.vue_map.slot_to_vert_result[c.vue_map.num_slots++] = 
 BRW_VERT_RESULT_PNTC;
 +   }
 c.urb_entry_read_offset = brw_sf_compute_urb_entry_read_offset(intel);
 c.nr_attr_regs = (c.vue_map.num_slots + 1)/2 - c.urb_entry_read_offset;
 c.nr_setup_regs = c.nr_attr_regs;
 @@ -125,6 +135,7 @@ brw_upload_sf_prog(struct brw_context *brw)
  {
 struct gl_context *ctx = brw-intel.ctx;
 struct brw_sf_prog_key key;
 +   bool render_to_fbo = ctx-DrawBuffer-Name != 0;
  
 memset(key, 0, sizeof(key));
  
 @@ -167,7 +178,15 @@ brw_upload_sf_prog(struct brw_context *brw)
   key.point_sprite_coord_replace |= (1  i);
}
 }
 -   key.sprite_origin_lower_left = (ctx-Point.SpriteOrigin == GL_LOWER_LEFT);
 +   if (brw-fragment_program-Base.InputsRead  
 BITFIELD64_BIT(FRAG_ATTRIB_PNTC))
 +  key.do_point_coord = 1;
 +   /*
 +* Window coordinates in a FBO are inverted, which means point
 +* sprite origin must be inverted, too.
 +*/
 +   if ((ctx-Point.SpriteOrigin == GL_LOWER_LEFT) != render_to_fbo)
 +  key.sprite_origin_lower_left = true;
 +
 /* _NEW_LIGHT */
 key.do_flat_shading = (ctx-Light.ShadeModel == GL_FLAT);
 key.do_twoside_color = (ctx-Light.Enabled  ctx-Light.Model.TwoSide);

[Mesa-dev] [PATCH] i965: handle gl_PointCoord for Gen4 and Gen5 platforms

2012-02-26 Thread Yuanhan Liu 
This patch add the support of gl_PointCoord gl builtin variable for
platform gen4 and gen5(ILK).

Unlike gen6+, we don't have a hardware support of gl_PointCoord, means
hardware will not calculate the interpolation coefficient for you.
Instead, you should handle it yourself in sf shader stage.

But badly, gl_PointCoord is a FS instead of VS builtin variable, thus
it's not included in c.vue_map generated in VS stage. Thus the current
code doesn't aware of this attribute. And to handle it correctly, we
need add it to c.vue_map manually to let SF shader generate the needed
interpolation coefficient for FS shader. SF stage has it's own copy of
vue_map, thus I think it's safe to do it manually.

Since handling gl_PointCoord for gen4 and gen5 platforms is somehow a
little special, I added a lot of comments and hope I didn't overdo it ;)

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=45975
Piglit: glsl-fs-pointcoord and fbo-gl_pointcoord

NOTE: This is a candidate for stable release branches.

Signed-off-by: Yuanhan Liu yuanhan@linux.intel.com
---
 src/mesa/drivers/dri/i965/brw_context.h |6 ++
 src/mesa/drivers/dri/i965/brw_fs.cpp|9 +
 src/mesa/drivers/dri/i965/brw_sf.c  |   26 ++
 src/mesa/drivers/dri/i965/brw_sf.h  |1 +
 src/mesa/drivers/dri/i965/brw_sf_emit.c |4 
 5 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_context.h 
b/src/mesa/drivers/dri/i965/brw_context.h
index 09d8373..7c794ad 100644
--- a/src/mesa/drivers/dri/i965/brw_context.h
+++ b/src/mesa/drivers/dri/i965/brw_context.h
@@ -296,6 +296,12 @@ typedef enum
BRW_VERT_RESULT_NDC = VERT_RESULT_MAX,
BRW_VERT_RESULT_HPOS_DUPLICATE,
BRW_VERT_RESULT_PAD,
+   /*
+* It's actually not a vert_result but just a _mark_ to let sf aware that
+* he need do something special to handle gl_PointCoord builtin variable
+* correctly. see compile_sf_prog() for more info.
+*/
+   BRW_VERT_RESULT_PNTC,
BRW_VERT_RESULT_MAX
 } brw_vert_result;
 
diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp 
b/src/mesa/drivers/dri/i965/brw_fs.cpp
index bf59da3..5f3d79d 100644
--- a/src/mesa/drivers/dri/i965/brw_fs.cpp
+++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
@@ -710,6 +710,15 @@ fs_visitor::calculate_urb_setup()
   urb_setup[fp_index] = urb_next++;
 }
   }
+
+  /*
+   * It's a FS only attribute, and we did interpolation for this attribute
+   * in SF thread. So, count it here, too.
+   *
+   * See compile_sf_prog() for more info.
+   */
+  if (brw-fragment_program-Base.InputsRead  
BITFIELD64_BIT(FRAG_ATTRIB_PNTC))
+ urb_setup[FRAG_ATTRIB_PNTC] = urb_next++;
}
 
/* Each attribute is 4 setup channels, each of which is half a reg. */
diff --git a/src/mesa/drivers/dri/i965/brw_sf.c 
b/src/mesa/drivers/dri/i965/brw_sf.c
index 6e63583..7950c47 100644
--- a/src/mesa/drivers/dri/i965/brw_sf.c
+++ b/src/mesa/drivers/dri/i965/brw_sf.c
@@ -64,6 +64,16 @@ static void compile_sf_prog( struct brw_context *brw,
 
c.key = *key;
c.vue_map = brw-vs.prog_data-vue_map;
+   if (c.key.do_point_coord) {
+  /*
+   * gl_PointCoord is a FS instead of VS builtin variable, thus it's
+   * not included in c.vue_map generated in VS stage. Here we add
+   * it manually to let SF shader generate the needed interpolation
+   * coefficient for FS shader.
+   */
+  c.vue_map.vert_result_to_slot[BRW_VERT_RESULT_PNTC] = 
c.vue_map.num_slots;
+  c.vue_map.slot_to_vert_result[c.vue_map.num_slots++] = 
BRW_VERT_RESULT_PNTC;
+   }
c.urb_entry_read_offset = brw_sf_compute_urb_entry_read_offset(intel);
c.nr_attr_regs = (c.vue_map.num_slots + 1)/2 - c.urb_entry_read_offset;
c.nr_setup_regs = c.nr_attr_regs;
@@ -125,6 +135,7 @@ brw_upload_sf_prog(struct brw_context *brw)
 {
struct gl_context *ctx = brw-intel.ctx;
struct brw_sf_prog_key key;
+   bool render_to_fbo = ctx-DrawBuffer-Name != 0;
 
memset(key, 0, sizeof(key));
 
@@ -167,7 +178,15 @@ brw_upload_sf_prog(struct brw_context *brw)
key.point_sprite_coord_replace |= (1  i);
   }
}
-   key.sprite_origin_lower_left = (ctx-Point.SpriteOrigin == GL_LOWER_LEFT);
+   if (brw-fragment_program-Base.InputsRead  
BITFIELD64_BIT(FRAG_ATTRIB_PNTC))
+  key.do_point_coord = 1;
+   /*
+* Window coordinates in a FBO are inverted, which means point
+* sprite origin must be inverted, too.
+*/
+   if ((ctx-Point.SpriteOrigin == GL_LOWER_LEFT) != render_to_fbo)
+  key.sprite_origin_lower_left = true;
+
/* _NEW_LIGHT */
key.do_flat_shading = (ctx-Light.ShadeModel == GL_FLAT);
key.do_twoside_color = (ctx-Light.Enabled  ctx-Light.Model.TwoSide);
@@ -176,10 +195,9 @@ brw_upload_sf_prog(struct brw_context *brw)
if (key.do_twoside_color) {
   /* If we're rendering to a FBO, we have to invert the polygon
* face orientation, just as we invert the