Re: [Mesa-dev] [PATCH] i965: handle gl_PointCoord for Gen4 and Gen5 platform
On Sat, Feb 18, 2012 at 3:45 AM, Eric Anholt e...@anholt.net wrote:
On Fri, 17 Feb 2012 16:09:37 +0800, Yuanhan Liu yuanhan@linux.intel.com
wrote:
This patch add the support of gl_PointCoord gl builtin var for platform
gen4 and gen5(ILK).
We can get the point start coord and the current pixel coord, and the
only left element needed is the point size. Thus I wrote another simple
SF routine for that.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=45975
You don't mention the piglit tests fixed. Have you run piglit on this?
Does it fix glsl-fs-pointcoord, for example?
Sorry, forgot to test piglit. But, yes, it does fix glsl-fs-pointcoord
and fbo-gl_pointcoord.
---
I am somehow not quite sure that this is the right way to implement
gl_PointCoord for gen4 and gen5 platform. But it does work on G45 and
ILK(the two platform I've tested, Zhao jian will help to test more
gen4 platforms).
diff --git a/src/mesa/drivers/dri/i965/brw_sf.c
b/src/mesa/drivers/dri/i965/brw_sf.c
index 54c27f9..69c507a 100644
--- a/src/mesa/drivers/dri/i965/brw_sf.c
+++ b/src/mesa/drivers/dri/i965/brw_sf.c
@@ -84,7 +84,9 @@ static void compile_sf_prog( struct brw_context *brw,
break;
case SF_POINTS:
c.nr_verts = 1;
- if (key-do_point_sprite)
+ if (key-do_point_sprite key-do_point_coord)
+ brw_emit_point_coord_setup( c, true );
+ else if (key-do_point_sprite)
brw_emit_point_sprite_setup( c, true );
If you have gl_PointCoord usage, is that suppposed to prevent point
sprite texture coordinate setup from working? I wouldn't think so.
Yeah, you are right. Thanks.
+/*
+ * A special routine just for handling gl_PointCoord builtin var
+ *
+ * We don't handle the point coord origin here but instead at the
+ * fragment shader code generation time.
+ */
+void brw_emit_point_coord_setup(struct brw_sf_compile *c, bool allocate)
+{
+ struct brw_compile *p = c-func;
+ GLuint i;
+
+ c-nr_verts = 1;
+
+ if (allocate)
+ alloc_regs(c);
+
+ for (i = 0; i c-nr_setup_regs; i++) {
+ bool last = (i == c-nr_setup_regs - 1);
+ /* Caculate 1.0/PointWidth */
+ brw_math(c-func,
+ c-tmp,
+ BRW_MATH_FUNCTION_INV,
+ BRW_MATH_SATURATE_NONE,
+ 0,
+ c-dx0,
+ BRW_MATH_DATA_SCALAR,
+ BRW_MATH_PRECISION_FULL);
You didn't set you predicate control, so I think you're doing extra math
here, which is very expensive on this platform.
Thanks for that info.
+
+ brw_set_access_mode(p, BRW_ALIGN_16);
+ /* dA/dx, dA/dy */
+ brw_MOV(p, brw_writemask(c-m1Cx, WRITEMASK_X), c-tmp);
+ brw_MOV(p, brw_writemask(c-m2Cy, WRITEMASK_Y), c-tmp);
+
+ /* attribute constant offset */
+ brw_MOV(p, brw_writemask(c-m3C0, WRITEMASK_W), brw_imm_f(1.0));
+ brw_set_access_mode(p, BRW_ALIGN_1);
+
+ /* Copy m0..m3 to URB. */
+ brw_urb_WRITE(p,
+ brw_null_reg(),
+ 0,
+ brw_vec8_grf(0, 0),
+ 0, /* allocate */
+ 1, /* used */
+ 4, /* msg len */
+ 0, /* response len */
+ last, /* eot */
+ last, /* writes complete */
+ i*4, /* urb destination offset */
+ BRW_URB_SWIZZLE_TRANSPOSE);
+ }
+}
This whole function looks to me like it should really just be a
modification to brw_emit_point_sprite_setup that doesn't care about the
other two components when setting up FRAG_ATTRIB_POINTCOORD.
Indeed.
There's a more major problem here, that the new function is setting up
interpolation for every VUE element, and your FS code appears to just be
picking one varying from the SF's output urb entry and using that for
the source. But all the varyings aren't supposed to interpolate like
that, they're supposed to interpolate like brw_emit_point_setup(). So
you need to actually allocate space for another attribute in the SF
output and put your pointcoord interpolation there, and then use that
From the FS.
Yeah, you are right. I made 2 new patches based on your comments.
Please help to review it. Appreciate your comments very much!!!
Thanks,
Yuanhan Liu
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] i965: handle gl_PointCoord for Gen4 and Gen5 platform
This patch add the support of gl_PointCoord gl builtin var for platform
gen4 and gen5(ILK).
We can get the point start coord and the current pixel coord, and the
only left element needed is the point size. Thus I wrote another simple
SF routine for that.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=45975
---
I am somehow not quite sure that this is the right way to implement
gl_PointCoord for gen4 and gen5 platform. But it does work on G45 and
ILK(the two platform I've tested, Zhao jian will help to test more
gen4 platforms).
Thus comments are hugely welcome!!!
NOTE: This is a candidate for stable release branches.
Signed-off-by: Yuanhan Liu yuanhan@linux.intel.com
---
src/mesa/drivers/dri/i965/brw_fs.cpp | 27 +
src/mesa/drivers/dri/i965/brw_fs.h |1 +
src/mesa/drivers/dri/i965/brw_fs_visitor.cpp |2 +
src/mesa/drivers/dri/i965/brw_sf.c |6 ++-
src/mesa/drivers/dri/i965/brw_sf.h |2 +
src/mesa/drivers/dri/i965/brw_sf_emit.c | 53 ++
6 files changed, 90 insertions(+), 1 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
b/src/mesa/drivers/dri/i965/brw_fs.cpp
index 6ecaa6c..178b9c9c 100644
--- a/src/mesa/drivers/dri/i965/brw_fs.cpp
+++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
@@ -421,6 +421,33 @@ fs_visitor::emit_fragcoord_interpolation(ir_variable *ir)
}
fs_reg *
+fs_visitor::emit_pointcoord_interpolation_gen4(ir_variable *ir)
+{
+ fs_reg *reg = new(this-mem_ctx) fs_reg(this, ir-type);
+ fs_reg pntcoord = *reg;
+ int urb_start = c-nr_payload_regs + c-prog_data.curb_read_length;
+ fs_reg pnt_size_rcp = fs_reg(brw_vec1_grf(urb_start, 0));
+ bool render_to_fbo = ctx-DrawBuffer-Name != 0;
+
+ /* gl_PointCoord.x */
+ emit(BRW_OPCODE_MUL, pntcoord,
this-delta_x[BRW_WM_PERSPECTIVE_PIXEL_BARYCENTRIC], pnt_size_rcp);
+ pntcoord.reg_offset++;
+
+ /* gl_PointCoord.y */
+ emit(BRW_OPCODE_MUL, pntcoord,
this-delta_y[BRW_WM_PERSPECTIVE_PIXEL_BARYCENTRIC], pnt_size_rcp);
+ /*
+* Window coordinates in an FBO are inverted, which means coord in Y
+* must be inverted, too.
+*/
+ if ((ctx-Point.SpriteOrigin == GL_LOWER_LEFT) != render_to_fbo) {
+ pntcoord.negate = true;
+ emit(BRW_OPCODE_ADD, pntcoord, pntcoord, fs_reg(1.0f));
+ }
+
+ return reg;
+}
+
+fs_reg *
fs_visitor::emit_general_interpolation(ir_variable *ir)
{
fs_reg *reg = new(this-mem_ctx) fs_reg(this, ir-type);
diff --git a/src/mesa/drivers/dri/i965/brw_fs.h
b/src/mesa/drivers/dri/i965/brw_fs.h
index 5fdc055..782c695 100644
--- a/src/mesa/drivers/dri/i965/brw_fs.h
+++ b/src/mesa/drivers/dri/i965/brw_fs.h
@@ -517,6 +517,7 @@ public:
void emit_dummy_fs();
fs_reg *emit_fragcoord_interpolation(ir_variable *ir);
fs_reg *emit_frontfacing_interpolation(ir_variable *ir);
+ fs_reg *emit_pointcoord_interpolation_gen4(ir_variable *ir);
fs_reg *emit_general_interpolation(ir_variable *ir);
void emit_interpolation_setup_gen4();
void emit_interpolation_setup_gen6();
diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
index ea8cd37..ed53ad5 100644
--- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
+++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
@@ -63,6 +63,8 @@ fs_visitor::visit(ir_variable *ir)
reg = emit_fragcoord_interpolation(ir);
} else if (!strcmp(ir-name, gl_FrontFacing)) {
reg = emit_frontfacing_interpolation(ir);
+ } else if (!strcmp(ir-name, gl_PointCoord) intel-gen 6) {
+ reg = emit_pointcoord_interpolation_gen4(ir);
} else {
reg = emit_general_interpolation(ir);
}
diff --git a/src/mesa/drivers/dri/i965/brw_sf.c
b/src/mesa/drivers/dri/i965/brw_sf.c
index 54c27f9..69c507a 100644
--- a/src/mesa/drivers/dri/i965/brw_sf.c
+++ b/src/mesa/drivers/dri/i965/brw_sf.c
@@ -84,7 +84,9 @@ static void compile_sf_prog( struct brw_context *brw,
break;
case SF_POINTS:
c.nr_verts = 1;
- if (key-do_point_sprite)
+ if (key-do_point_sprite key-do_point_coord)
+ brw_emit_point_coord_setup( c, true );
+ else if (key-do_point_sprite)
brw_emit_point_sprite_setup( c, true );
else
brw_emit_point_setup( c, true );
@@ -167,6 +169,8 @@ brw_upload_sf_prog(struct brw_context *brw)
key.point_sprite_coord_replace |= (1 i);
}
}
+ if (brw-fragment_program-Base.InputsRead
BITFIELD64_BIT(FRAG_ATTRIB_PNTC))
+ key.do_point_coord = 1;
key.sprite_origin_lower_left = (ctx-Point.SpriteOrigin == GL_LOWER_LEFT);
/* _NEW_LIGHT */
key.do_flat_shading = (ctx-Light.ShadeModel == GL_FLAT);
diff --git a/src/mesa/drivers/dri/i965/brw_sf.h
b/src/mesa/drivers/dri/i965/brw_sf.h
index 4ef0240..d4c70e7 100644
--- a/src/mesa/drivers/dri/i965/brw_sf.h
+++ b/src/mesa/drivers/dri/i965/brw_sf.h
@@ -52,6 +52,7 @@ struct brw_sf_prog_key {
GLuint do_flat_shading:1;
Re: [Mesa-dev] [PATCH] i965: handle gl_PointCoord for Gen4 and Gen5 platform
On Fri, 17 Feb 2012 16:09:37 +0800, Yuanhan Liu yuanhan@linux.intel.com
wrote:
This patch add the support of gl_PointCoord gl builtin var for platform
gen4 and gen5(ILK).
We can get the point start coord and the current pixel coord, and the
only left element needed is the point size. Thus I wrote another simple
SF routine for that.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=45975
You don't mention the piglit tests fixed. Have you run piglit on this?
Does it fix glsl-fs-pointcoord, for example?
---
I am somehow not quite sure that this is the right way to implement
gl_PointCoord for gen4 and gen5 platform. But it does work on G45 and
ILK(the two platform I've tested, Zhao jian will help to test more
gen4 platforms).
diff --git a/src/mesa/drivers/dri/i965/brw_sf.c
b/src/mesa/drivers/dri/i965/brw_sf.c
index 54c27f9..69c507a 100644
--- a/src/mesa/drivers/dri/i965/brw_sf.c
+++ b/src/mesa/drivers/dri/i965/brw_sf.c
@@ -84,7 +84,9 @@ static void compile_sf_prog( struct brw_context *brw,
break;
case SF_POINTS:
c.nr_verts = 1;
- if (key-do_point_sprite)
+ if (key-do_point_sprite key-do_point_coord)
+ brw_emit_point_coord_setup( c, true );
+ else if (key-do_point_sprite)
brw_emit_point_sprite_setup( c, true );
If you have gl_PointCoord usage, is that suppposed to prevent point
sprite texture coordinate setup from working? I wouldn't think so.
+/*
+ * A special routine just for handling gl_PointCoord builtin var
+ *
+ * We don't handle the point coord origin here but instead at the
+ * fragment shader code generation time.
+ */
+void brw_emit_point_coord_setup(struct brw_sf_compile *c, bool allocate)
+{
+ struct brw_compile *p = c-func;
+ GLuint i;
+
+ c-nr_verts = 1;
+
+ if (allocate)
+ alloc_regs(c);
+
+ for (i = 0; i c-nr_setup_regs; i++) {
+ bool last = (i == c-nr_setup_regs - 1);
+ /* Caculate 1.0/PointWidth */
+ brw_math(c-func,
+ c-tmp,
+ BRW_MATH_FUNCTION_INV,
+ BRW_MATH_SATURATE_NONE,
+ 0,
+ c-dx0,
+ BRW_MATH_DATA_SCALAR,
+ BRW_MATH_PRECISION_FULL);
You didn't set you predicate control, so I think you're doing extra math
here, which is very expensive on this platform.
+
+ brw_set_access_mode(p, BRW_ALIGN_16);
+ /* dA/dx, dA/dy */
+ brw_MOV(p, brw_writemask(c-m1Cx, WRITEMASK_X), c-tmp);
+ brw_MOV(p, brw_writemask(c-m2Cy, WRITEMASK_Y), c-tmp);
+
+ /* attribute constant offset */
+ brw_MOV(p, brw_writemask(c-m3C0, WRITEMASK_W), brw_imm_f(1.0));
+ brw_set_access_mode(p, BRW_ALIGN_1);
+
+ /* Copy m0..m3 to URB. */
+ brw_urb_WRITE(p,
+brw_null_reg(),
+0,
+brw_vec8_grf(0, 0),
+0, /* allocate */
+1, /* used */
+4, /* msg len */
+0, /* response len */
+last,/* eot */
+last,/* writes complete */
+i*4, /* urb destination offset */
+BRW_URB_SWIZZLE_TRANSPOSE);
+ }
+}
This whole function looks to me like it should really just be a
modification to brw_emit_point_sprite_setup that doesn't care about the
other two components when setting up FRAG_ATTRIB_POINTCOORD.
There's a more major problem here, that the new function is setting up
interpolation for every VUE element, and your FS code appears to just be
picking one varying from the SF's output urb entry and using that for
the source. But all the varyings aren't supposed to interpolate like
that, they're supposed to interpolate like brw_emit_point_setup(). So
you need to actually allocate space for another attribute in the SF
output and put your pointcoord interpolation there, and then use that
From the FS.
pgpRz5xkCftvG.pgp
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
