Re: [Mesa-dev] [PATCH] i965: fix inverted point sprite origin when rendering to FBO
On 01/19/2012 03:48 PM, Yuanhan Liu wrote: When rendering to FBO, rendering is inverted. At the same time, we would also make sure the point sprite origin is inverted. Or, we will get an inverted result correspoinding to rendering to the default winsys FBO. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=44613 NOTE: This is a candidate for stable release branches. v2: add the simliar logic to ivb, too (comments from Ian) simplify the logic operation (comments from Brian) v3: pick a better comment from Eric use != for the logic instead of ^ (comments from Ian) Signed-off-by: Yuanhan Liuyuanhan@linux.intel.com --- src/mesa/drivers/dri/i965/brw_defines.h |1 + src/mesa/drivers/dri/i965/gen6_sf_state.c | 13 +++-- src/mesa/drivers/dri/i965/gen7_sf_state.c | 18 +++--- 3 files changed, 27 insertions(+), 5 deletions(-) v3 looks great! Thanks for finding this. Reviewed-by: Kenneth Graunke kenn...@whitecape.org ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965: fix inverted point sprite origin when rendering to FBO
On 19/01/2012 23:48, Yuanhan Liu wrote:
When rendering to FBO, rendering is inverted. At the same time, we would
also make sure the point sprite origin is inverted. Or, we will get an
inverted result correspoinding to rendering to the default winsys FBO.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=44613
NOTE: This is a candidate for stable release branches.
v2: add the simliar logic to ivb, too (comments from Ian)
simplify the logic operation (comments from Brian)
v3: pick a better comment from Eric
use != for the logic instead of ^ (comments from Ian)
Signed-off-by: Yuanhan Liu yuanhan@linux.intel.com
---
src/mesa/drivers/dri/i965/brw_defines.h |1 +
src/mesa/drivers/dri/i965/gen6_sf_state.c | 13 +++--
src/mesa/drivers/dri/i965/gen7_sf_state.c | 18 +++---
3 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_defines.h
b/src/mesa/drivers/dri/i965/brw_defines.h
index 4d90a99..029be87 100644
--- a/src/mesa/drivers/dri/i965/brw_defines.h
+++ b/src/mesa/drivers/dri/i965/brw_defines.h
@@ -1128,6 +1128,7 @@ enum brw_message_target {
/* DW1 (for gen6) */
# define GEN6_SF_NUM_OUTPUTS_SHIFT 22
# define GEN6_SF_SWIZZLE_ENABLE (1 21)
+# define GEN6_SF_POINT_SPRITE_UPPERLEFT (0 20)
# define GEN6_SF_POINT_SPRITE_LOWERLEFT (1 20)
# define GEN6_SF_URB_ENTRY_READ_LENGTH_SHIFT 11
# define GEN6_SF_URB_ENTRY_READ_OFFSET_SHIFT 4
diff --git a/src/mesa/drivers/dri/i965/gen6_sf_state.c
b/src/mesa/drivers/dri/i965/gen6_sf_state.c
index 548c5a3..163b54c 100644
--- a/src/mesa/drivers/dri/i965/gen6_sf_state.c
+++ b/src/mesa/drivers/dri/i965/gen6_sf_state.c
@@ -129,6 +129,7 @@ upload_sf_state(struct brw_context *brw)
float point_size;
uint16_t attr_overrides[FRAG_ATTRIB_MAX];
bool userclip_active;
+ uint32_t point_sprite_origin;
/* _NEW_TRANSFORM */
userclip_active = (ctx-Transform.ClipPlanesEnabled != 0);
@@ -258,8 +259,16 @@ upload_sf_state(struct brw_context *brw)
/* Clamp to the hardware limits and convert to fixed point */
dw4 |= U_FIXED(CLAMP(point_size, 0.125, 255.875), 3);
- if (ctx-Point.SpriteOrigin == GL_LOWER_LEFT)
- dw1 |= GEN6_SF_POINT_SPRITE_LOWERLEFT;
+ /*
+* Window coordinates in an FBO are inverted, which means point
+* sprite origin must be inverted, too.
+*/
+ if ((ctx-Point.SpriteOrigin == GL_LOWER_LEFT) != render_to_fbo) {
+ point_sprite_origin = GEN6_SF_POINT_SPRITE_LOWERLEFT;
+ } else {
+ point_sprite_origin = GEN6_SF_POINT_SPRITE_UPPERLEFT;
+ }
+ dw1 |= point_sprite_origin;
/* _NEW_LIGHT */
if (ctx-Light.ProvokingVertex != GL_FIRST_VERTEX_CONVENTION) {
diff --git a/src/mesa/drivers/dri/i965/gen7_sf_state.c
b/src/mesa/drivers/dri/i965/gen7_sf_state.c
index 7691cb2..da7ef81 100644
--- a/src/mesa/drivers/dri/i965/gen7_sf_state.c
+++ b/src/mesa/drivers/dri/i965/gen7_sf_state.c
@@ -48,6 +48,9 @@ upload_sbe_state(struct brw_context *brw)
int urb_entry_read_offset = 1;
bool userclip_active = (ctx-Transform.ClipPlanesEnabled != 0);
uint16_t attr_overrides[FRAG_ATTRIB_MAX];
+ /* _NEW_BUFFERS */
+ bool render_to_fbo = ctx-DrawBuffer-Name != 0;
+ uint32_t point_sprite_origin;
brw_compute_vue_map(vue_map, intel, userclip_active, vs_outputs_written);
urb_entry_read_length = (vue_map.num_slots + 1)/2 - urb_entry_read_offset;
@@ -65,9 +68,18 @@ upload_sbe_state(struct brw_context *brw)
urb_entry_read_length GEN7_SBE_URB_ENTRY_READ_LENGTH_SHIFT |
urb_entry_read_offset GEN7_SBE_URB_ENTRY_READ_OFFSET_SHIFT;
- /* _NEW_POINT */
- if (ctx-Point.SpriteOrigin == GL_LOWER_LEFT)
- dw1 |= GEN6_SF_POINT_SPRITE_LOWERLEFT;
+ /* _NEW_POINT
+*
+* Window coordinates in an FBO are inverted, which means point
+* sprite origin must be inverted.
+*/
+ if ((ctx-Point.SpriteOrigin == GL_LOWER_LEFT) != render_to_fbo) {
+ point_sprite_origin = GEN6_SF_POINT_SPRITE_LOWERLEFT;
+ } else {
+ point_sprite_origin = GEN6_SF_POINT_SPRITE_UPPERLEFT;
+ }
+ dw1 |= point_sprite_origin;
+
dw10 = 0;
dw11 = 0;
Is there any need for GEN6_SF_POINT_SPRITE_UPPERLEFT? It's basically 0,
so the | operation with dw1 won't have any effect, and you could remove
the else clauses. And if you wanted to clear the bit, I think you'd need
to do something like this instead:
if ((ctx-Point.SpriteOrigin == GL_LOWER_LEFT) != render_to_fbo) {
dw1 |= GEN6_SF_POINT_SPRITE_LOWERLEFT;
} else {
dw1 = ~GEN6_SF_POINT_SPRITE_LOWERLEFT;
}
-Jonathan
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965: fix inverted point sprite origin when rendering to FBO
On Jan 20, 2012 9:07 PM, Jonathan Coome jco...@fastmail.co.uk wrote:
On 19/01/2012 23:48, Yuanhan Liu wrote:
When rendering to FBO, rendering is inverted. At the same time, we would
also make sure the point sprite origin is inverted. Or, we will get an
inverted result correspoinding to rendering to the default winsys FBO.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=44613
NOTE: This is a candidate for stable release branches.
v2: add the simliar logic to ivb, too (comments from Ian)
simplify the logic operation (comments from Brian)
v3: pick a better comment from Eric
use != for the logic instead of ^ (comments from Ian)
Signed-off-by: Yuanhan Liu yuanhan@linux.intel.com
---
src/mesa/drivers/dri/i965/brw_defines.h |1 +
src/mesa/drivers/dri/i965/gen6_sf_state.c | 13 +++--
src/mesa/drivers/dri/i965/gen7_sf_state.c | 18 +++---
3 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_defines.h
b/src/mesa/drivers/dri/i965/brw_defines.h
index 4d90a99..029be87 100644
--- a/src/mesa/drivers/dri/i965/brw_defines.h
+++ b/src/mesa/drivers/dri/i965/brw_defines.h
@@ -1128,6 +1128,7 @@ enum brw_message_target {
/* DW1 (for gen6) */
# define GEN6_SF_NUM_OUTPUTS_SHIFT 22
# define GEN6_SF_SWIZZLE_ENABLE (1 21)
+# define GEN6_SF_POINT_SPRITE_UPPERLEFT (0 20)
# define GEN6_SF_POINT_SPRITE_LOWERLEFT (1 20)
# define GEN6_SF_URB_ENTRY_READ_LENGTH_SHIFT 11
# define GEN6_SF_URB_ENTRY_READ_OFFSET_SHIFT 4
diff --git a/src/mesa/drivers/dri/i965/gen6_sf_state.c
b/src/mesa/drivers/dri/i965/gen6_sf_state.c
index 548c5a3..163b54c 100644
--- a/src/mesa/drivers/dri/i965/gen6_sf_state.c
+++ b/src/mesa/drivers/dri/i965/gen6_sf_state.c
@@ -129,6 +129,7 @@ upload_sf_state(struct brw_context *brw)
float point_size;
uint16_t attr_overrides[FRAG_ATTRIB_MAX];
bool userclip_active;
+ uint32_t point_sprite_origin;
/* _NEW_TRANSFORM */
userclip_active = (ctx-Transform.ClipPlanesEnabled != 0);
@@ -258,8 +259,16 @@ upload_sf_state(struct brw_context *brw)
/* Clamp to the hardware limits and convert to fixed point */
dw4 |= U_FIXED(CLAMP(point_size, 0.125, 255.875), 3);
- if (ctx-Point.SpriteOrigin == GL_LOWER_LEFT)
- dw1 |= GEN6_SF_POINT_SPRITE_LOWERLEFT;
+ /*
+* Window coordinates in an FBO are inverted, which means point
+* sprite origin must be inverted, too.
+*/
+ if ((ctx-Point.SpriteOrigin == GL_LOWER_LEFT) != render_to_fbo) {
+ point_sprite_origin = GEN6_SF_POINT_SPRITE_LOWERLEFT;
+ } else {
+ point_sprite_origin = GEN6_SF_POINT_SPRITE_UPPERLEFT;
+ }
+ dw1 |= point_sprite_origin;
/* _NEW_LIGHT */
if (ctx-Light.ProvokingVertex != GL_FIRST_VERTEX_CONVENTION) {
diff --git a/src/mesa/drivers/dri/i965/gen7_sf_state.c
b/src/mesa/drivers/dri/i965/gen7_sf_state.c
index 7691cb2..da7ef81 100644
--- a/src/mesa/drivers/dri/i965/gen7_sf_state.c
+++ b/src/mesa/drivers/dri/i965/gen7_sf_state.c
@@ -48,6 +48,9 @@ upload_sbe_state(struct brw_context *brw)
int urb_entry_read_offset = 1;
bool userclip_active = (ctx-Transform.ClipPlanesEnabled != 0);
uint16_t attr_overrides[FRAG_ATTRIB_MAX];
+ /* _NEW_BUFFERS */
+ bool render_to_fbo = ctx-DrawBuffer-Name != 0;
+ uint32_t point_sprite_origin;
brw_compute_vue_map(vue_map, intel, userclip_active,
vs_outputs_written);
urb_entry_read_length = (vue_map.num_slots + 1)/2 -
urb_entry_read_offset;
@@ -65,9 +68,18 @@ upload_sbe_state(struct brw_context *brw)
urb_entry_read_length GEN7_SBE_URB_ENTRY_READ_LENGTH_SHIFT |
urb_entry_read_offset GEN7_SBE_URB_ENTRY_READ_OFFSET_SHIFT;
- /* _NEW_POINT */
- if (ctx-Point.SpriteOrigin == GL_LOWER_LEFT)
- dw1 |= GEN6_SF_POINT_SPRITE_LOWERLEFT;
+ /* _NEW_POINT
+*
+* Window coordinates in an FBO are inverted, which means point
+* sprite origin must be inverted.
+*/
+ if ((ctx-Point.SpriteOrigin == GL_LOWER_LEFT) != render_to_fbo) {
+ point_sprite_origin = GEN6_SF_POINT_SPRITE_LOWERLEFT;
+ } else {
+ point_sprite_origin = GEN6_SF_POINT_SPRITE_UPPERLEFT;
+ }
+ dw1 |= point_sprite_origin;
+
dw10 = 0;
dw11 = 0;
Is there any need for GEN6_SF_POINT_SPRITE_UPPERLEFT?
For making the code clear and easy reading, I'd like to add it.
It's basically 0,
so the | operation with dw1 won't have any effect, and you could remove
the else clauses.
And if you wanted to clear the bit,
No, its not supposed to be a clear operation, but an _or_ operation. And
the operand happens to be zero. Just image we want to set a bit, but not
set 0. So, the logic of your following code doesn't make sense to me.
Thanks.
--
Yuanhan Liu
(Sent by
Re: [Mesa-dev] [PATCH] i965: fix inverted point sprite origin when rendering to FBO
On Fri, 20 Jan 2012 13:00:10 +, Jonathan Coome jco...@fastmail.co.uk
wrote:
On 19/01/2012 23:48, Yuanhan Liu wrote:
Is there any need for GEN6_SF_POINT_SPRITE_UPPERLEFT? It's basically 0,
so the | operation with dw1 won't have any effect, and you could remove
the else clauses. And if you wanted to clear the bit, I think you'd need
to do something like this instead:
if ((ctx-Point.SpriteOrigin == GL_LOWER_LEFT) != render_to_fbo) {
dw1 |= GEN6_SF_POINT_SPRITE_LOWERLEFT;
} else {
dw1 = ~GEN6_SF_POINT_SPRITE_LOWERLEFT;
}
Clearing wasn't the intention, it was just to make it clear what the two
values of that bitfield mean. It's a relatively common pattern in
driver coding, and you rely on the compiler to not do something silly
for dw1 |= 0.
pgpleEMgXLrv1.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: fix inverted point sprite origin when rendering to FBO
On 01/18/2012 06:30 PM, Yuanhan Liu wrote:
When rendering to FBO, rendering is inverted. At the same time, we would
also make sure the point sprite origin is inverted. Or, we will get an
inverted result correspoinding to rendering to the default winsys FBO.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=44613
NOTE: This is a candidate for stable release branches.
v2: add the simliar logic to ivb, too (comments from Ian)
simplify the logic operation (comments from Brian)
Signed-off-by: Yuanhan Liuyuanhan@linux.intel.com
---
src/mesa/drivers/dri/i965/brw_defines.h |1 +
src/mesa/drivers/dri/i965/gen6_sf_state.c | 15 +--
src/mesa/drivers/dri/i965/gen7_sf_state.c | 20 +---
3 files changed, 31 insertions(+), 5 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_defines.h
b/src/mesa/drivers/dri/i965/brw_defines.h
index 4d90a99..029be87 100644
--- a/src/mesa/drivers/dri/i965/brw_defines.h
+++ b/src/mesa/drivers/dri/i965/brw_defines.h
@@ -1128,6 +1128,7 @@ enum brw_message_target {
/* DW1 (for gen6) */
# define GEN6_SF_NUM_OUTPUTS_SHIFT22
# define GEN6_SF_SWIZZLE_ENABLE (1 21)
+# define GEN6_SF_POINT_SPRITE_UPPERLEFT(0 20)
# define GEN6_SF_POINT_SPRITE_LOWERLEFT (1 20)
# define GEN6_SF_URB_ENTRY_READ_LENGTH_SHIFT 11
# define GEN6_SF_URB_ENTRY_READ_OFFSET_SHIFT 4
diff --git a/src/mesa/drivers/dri/i965/gen6_sf_state.c
b/src/mesa/drivers/dri/i965/gen6_sf_state.c
index 548c5a3..67c208b 100644
--- a/src/mesa/drivers/dri/i965/gen6_sf_state.c
+++ b/src/mesa/drivers/dri/i965/gen6_sf_state.c
@@ -129,6 +129,7 @@ upload_sf_state(struct brw_context *brw)
float point_size;
uint16_t attr_overrides[FRAG_ATTRIB_MAX];
bool userclip_active;
+ uint32_t point_sprite_origin;
/* _NEW_TRANSFORM */
userclip_active = (ctx-Transform.ClipPlanesEnabled != 0);
@@ -258,8 +259,18 @@ upload_sf_state(struct brw_context *brw)
/* Clamp to the hardware limits and convert to fixed point */
dw4 |= U_FIXED(CLAMP(point_size, 0.125, 255.875), 3);
- if (ctx-Point.SpriteOrigin == GL_LOWER_LEFT)
- dw1 |= GEN6_SF_POINT_SPRITE_LOWERLEFT;
+ /*
+* When rendering to FBO, rendering is inverted. At the same time,
+* we would also make sure the point sprite origin is inverted.
+* Or, we will get an inverted result corresponding to rendering
+* to the default/window FBO.
+*/
+ if ((ctx-Point.SpriteOrigin == GL_LOWER_LEFT) ^ render_to_fbo) {
I (mostly) like that. I was trying to think of a way to simplify the
if-statements in the original patch, but I couldn't think of a good way.
However, using the bit-wise xor is not correct here. The compiler
accepts it because everything is an integer in C. Some tools, like
Coverty, will probably complain about this. You really want a ^^
(logical xor), but C doesn't have that. What C does have, that does
exactly the same thing, is ==. I suggest changing this to
if ((ctx-Point.SpriteOrigin == GL_LOWER_LEFT) == render_to_fbo) {
It looks a bit weird, but it is correct.
+ point_sprite_origin = GEN6_SF_POINT_SPRITE_LOWERLEFT;
+ } else {
+ point_sprite_origin = GEN6_SF_POINT_SPRITE_UPPERLEFT;
+ }
+ dw1 |= point_sprite_origin;
/* _NEW_LIGHT */
if (ctx-Light.ProvokingVertex != GL_FIRST_VERTEX_CONVENTION) {
diff --git a/src/mesa/drivers/dri/i965/gen7_sf_state.c
b/src/mesa/drivers/dri/i965/gen7_sf_state.c
index 7691cb2..75a1cb0 100644
--- a/src/mesa/drivers/dri/i965/gen7_sf_state.c
+++ b/src/mesa/drivers/dri/i965/gen7_sf_state.c
@@ -48,6 +48,9 @@ upload_sbe_state(struct brw_context *brw)
int urb_entry_read_offset = 1;
bool userclip_active = (ctx-Transform.ClipPlanesEnabled != 0);
uint16_t attr_overrides[FRAG_ATTRIB_MAX];
+ /* _NEW_BUFFERS */
+ bool render_to_fbo = brw-intel.ctx.DrawBuffer-Name != 0;
+ uint32_t point_sprite_origin;
brw_compute_vue_map(vue_map, intel, userclip_active, vs_outputs_written);
urb_entry_read_length = (vue_map.num_slots + 1)/2 - urb_entry_read_offset;
@@ -65,9 +68,20 @@ upload_sbe_state(struct brw_context *brw)
urb_entry_read_length GEN7_SBE_URB_ENTRY_READ_LENGTH_SHIFT |
urb_entry_read_offset GEN7_SBE_URB_ENTRY_READ_OFFSET_SHIFT;
- /* _NEW_POINT */
- if (ctx-Point.SpriteOrigin == GL_LOWER_LEFT)
- dw1 |= GEN6_SF_POINT_SPRITE_LOWERLEFT;
+ /* _NEW_POINT
+*
+* When rendering to FBO, rendering is inverted. At the same time,
+* we would also make sure the point sprite origin is inverted.
+* Or, we will get an inverted result corresponding to rendering
+* to the default/window FBO.
+*/
+ if ((ctx-Point.SpriteOrigin == GL_LOWER_LEFT) ^ render_to_fbo) {
+ point_sprite_origin = GEN6_SF_POINT_SPRITE_LOWERLEFT;
+ } else {
+ point_sprite_origin = GEN6_SF_POINT_SPRITE_UPPERLEFT;
+ }
+ dw1 |=
Re: [Mesa-dev] [PATCH] i965: fix inverted point sprite origin when rendering to FBO
On 01/19/2012 10:17 AM, Ian Romanick wrote:
On 01/18/2012 06:30 PM, Yuanhan Liu wrote:
When rendering to FBO, rendering is inverted. At the same time, we
would
also make sure the point sprite origin is inverted. Or, we will get an
inverted result correspoinding to rendering to the default winsys FBO.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=44613
NOTE: This is a candidate for stable release branches.
v2: add the simliar logic to ivb, too (comments from Ian)
simplify the logic operation (comments from Brian)
Signed-off-by: Yuanhan Liuyuanhan@linux.intel.com
---
src/mesa/drivers/dri/i965/brw_defines.h | 1 +
src/mesa/drivers/dri/i965/gen6_sf_state.c | 15 +--
src/mesa/drivers/dri/i965/gen7_sf_state.c | 20 +---
3 files changed, 31 insertions(+), 5 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_defines.h
b/src/mesa/drivers/dri/i965/brw_defines.h
index 4d90a99..029be87 100644
--- a/src/mesa/drivers/dri/i965/brw_defines.h
+++ b/src/mesa/drivers/dri/i965/brw_defines.h
@@ -1128,6 +1128,7 @@ enum brw_message_target {
/* DW1 (for gen6) */
# define GEN6_SF_NUM_OUTPUTS_SHIFT 22
# define GEN6_SF_SWIZZLE_ENABLE (1 21)
+# define GEN6_SF_POINT_SPRITE_UPPERLEFT (0 20)
# define GEN6_SF_POINT_SPRITE_LOWERLEFT (1 20)
# define GEN6_SF_URB_ENTRY_READ_LENGTH_SHIFT 11
# define GEN6_SF_URB_ENTRY_READ_OFFSET_SHIFT 4
diff --git a/src/mesa/drivers/dri/i965/gen6_sf_state.c
b/src/mesa/drivers/dri/i965/gen6_sf_state.c
index 548c5a3..67c208b 100644
--- a/src/mesa/drivers/dri/i965/gen6_sf_state.c
+++ b/src/mesa/drivers/dri/i965/gen6_sf_state.c
@@ -129,6 +129,7 @@ upload_sf_state(struct brw_context *brw)
float point_size;
uint16_t attr_overrides[FRAG_ATTRIB_MAX];
bool userclip_active;
+ uint32_t point_sprite_origin;
/* _NEW_TRANSFORM */
userclip_active = (ctx-Transform.ClipPlanesEnabled != 0);
@@ -258,8 +259,18 @@ upload_sf_state(struct brw_context *brw)
/* Clamp to the hardware limits and convert to fixed point */
dw4 |= U_FIXED(CLAMP(point_size, 0.125, 255.875), 3);
- if (ctx-Point.SpriteOrigin == GL_LOWER_LEFT)
- dw1 |= GEN6_SF_POINT_SPRITE_LOWERLEFT;
+ /*
+ * When rendering to FBO, rendering is inverted. At the same time,
+ * we would also make sure the point sprite origin is inverted.
+ * Or, we will get an inverted result corresponding to rendering
+ * to the default/window FBO.
+ */
+ if ((ctx-Point.SpriteOrigin == GL_LOWER_LEFT) ^ render_to_fbo) {
I (mostly) like that. I was trying to think of a way to simplify the
if-statements in the original patch, but I couldn't think of a good way.
However, using the bit-wise xor is not correct here. The compiler
accepts it because everything is an integer in C. Some tools, like
Coverty, will probably complain about this. You really want a ^^
(logical xor), but C doesn't have that. What C does have, that does
exactly the same thing, is ==. I suggest changing this to
if ((ctx-Point.SpriteOrigin == GL_LOWER_LEFT) == render_to_fbo) {
It looks a bit weird, but it is correct.
I suggested ^ to Yuanhan. Yes you have to be careful with it. Note
that X ^ render_to_fbo is already used earlier in the function.
But I think you want != in this case, not ==.
-Brian
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965: fix inverted point sprite origin when rendering to FBO
On Thu, 19 Jan 2012 10:30:53 +0800, Yuanhan Liu yuanhan@linux.intel.com
wrote:
When rendering to FBO, rendering is inverted. At the same time, we would
also make sure the point sprite origin is inverted. Or, we will get an
inverted result correspoinding to rendering to the default winsys FBO.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=44613
NOTE: This is a candidate for stable release branches.
v2: add the simliar logic to ivb, too (comments from Ian)
simplify the logic operation (comments from Brian)
Signed-off-by: Yuanhan Liu yuanhan@linux.intel.com
---
src/mesa/drivers/dri/i965/brw_defines.h |1 +
src/mesa/drivers/dri/i965/gen6_sf_state.c | 15 +--
src/mesa/drivers/dri/i965/gen7_sf_state.c | 20 +---
3 files changed, 31 insertions(+), 5 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_defines.h
b/src/mesa/drivers/dri/i965/brw_defines.h
index 4d90a99..029be87 100644
--- a/src/mesa/drivers/dri/i965/brw_defines.h
+++ b/src/mesa/drivers/dri/i965/brw_defines.h
@@ -1128,6 +1128,7 @@ enum brw_message_target {
/* DW1 (for gen6) */
# define GEN6_SF_NUM_OUTPUTS_SHIFT 22
# define GEN6_SF_SWIZZLE_ENABLE (1 21)
+# define GEN6_SF_POINT_SPRITE_UPPERLEFT (0 20)
# define GEN6_SF_POINT_SPRITE_LOWERLEFT (1 20)
# define GEN6_SF_URB_ENTRY_READ_LENGTH_SHIFT 11
# define GEN6_SF_URB_ENTRY_READ_OFFSET_SHIFT 4
diff --git a/src/mesa/drivers/dri/i965/gen6_sf_state.c
b/src/mesa/drivers/dri/i965/gen6_sf_state.c
index 548c5a3..67c208b 100644
--- a/src/mesa/drivers/dri/i965/gen6_sf_state.c
+++ b/src/mesa/drivers/dri/i965/gen6_sf_state.c
@@ -129,6 +129,7 @@ upload_sf_state(struct brw_context *brw)
float point_size;
uint16_t attr_overrides[FRAG_ATTRIB_MAX];
bool userclip_active;
+ uint32_t point_sprite_origin;
/* _NEW_TRANSFORM */
userclip_active = (ctx-Transform.ClipPlanesEnabled != 0);
@@ -258,8 +259,18 @@ upload_sf_state(struct brw_context *brw)
/* Clamp to the hardware limits and convert to fixed point */
dw4 |= U_FIXED(CLAMP(point_size, 0.125, 255.875), 3);
- if (ctx-Point.SpriteOrigin == GL_LOWER_LEFT)
- dw1 |= GEN6_SF_POINT_SPRITE_LOWERLEFT;
+ /*
+* When rendering to FBO, rendering is inverted. At the same time,
+* we would also make sure the point sprite origin is inverted.
+* Or, we will get an inverted result corresponding to rendering
+* to the default/window FBO.
+*/
I think this comment could be simplified to Window coordinates in an
FBO are inverted, which means point sprite origin must be inverted.
+ if ((ctx-Point.SpriteOrigin == GL_LOWER_LEFT) ^ render_to_fbo) {
+ point_sprite_origin = GEN6_SF_POINT_SPRITE_LOWERLEFT;
+ } else {
+ point_sprite_origin = GEN6_SF_POINT_SPRITE_UPPERLEFT;
+ }
Much better. That logic was hideous before.
pgp0PVx1V3VSe.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: fix inverted point sprite origin when rendering to FBO
On 01/19/2012 09:32 AM, Brian Paul wrote:
On 01/19/2012 10:17 AM, Ian Romanick wrote:
On 01/18/2012 06:30 PM, Yuanhan Liu wrote:
When rendering to FBO, rendering is inverted. At the same time, we
would
also make sure the point sprite origin is inverted. Or, we will get an
inverted result correspoinding to rendering to the default winsys FBO.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=44613
NOTE: This is a candidate for stable release branches.
v2: add the simliar logic to ivb, too (comments from Ian)
simplify the logic operation (comments from Brian)
Signed-off-by: Yuanhan Liuyuanhan@linux.intel.com
---
src/mesa/drivers/dri/i965/brw_defines.h | 1 +
src/mesa/drivers/dri/i965/gen6_sf_state.c | 15 +--
src/mesa/drivers/dri/i965/gen7_sf_state.c | 20 +---
3 files changed, 31 insertions(+), 5 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_defines.h
b/src/mesa/drivers/dri/i965/brw_defines.h
index 4d90a99..029be87 100644
--- a/src/mesa/drivers/dri/i965/brw_defines.h
+++ b/src/mesa/drivers/dri/i965/brw_defines.h
@@ -1128,6 +1128,7 @@ enum brw_message_target {
/* DW1 (for gen6) */
# define GEN6_SF_NUM_OUTPUTS_SHIFT 22
# define GEN6_SF_SWIZZLE_ENABLE (1 21)
+# define GEN6_SF_POINT_SPRITE_UPPERLEFT (0 20)
# define GEN6_SF_POINT_SPRITE_LOWERLEFT (1 20)
# define GEN6_SF_URB_ENTRY_READ_LENGTH_SHIFT 11
# define GEN6_SF_URB_ENTRY_READ_OFFSET_SHIFT 4
diff --git a/src/mesa/drivers/dri/i965/gen6_sf_state.c
b/src/mesa/drivers/dri/i965/gen6_sf_state.c
index 548c5a3..67c208b 100644
--- a/src/mesa/drivers/dri/i965/gen6_sf_state.c
+++ b/src/mesa/drivers/dri/i965/gen6_sf_state.c
@@ -129,6 +129,7 @@ upload_sf_state(struct brw_context *brw)
float point_size;
uint16_t attr_overrides[FRAG_ATTRIB_MAX];
bool userclip_active;
+ uint32_t point_sprite_origin;
/* _NEW_TRANSFORM */
userclip_active = (ctx-Transform.ClipPlanesEnabled != 0);
@@ -258,8 +259,18 @@ upload_sf_state(struct brw_context *brw)
/* Clamp to the hardware limits and convert to fixed point */
dw4 |= U_FIXED(CLAMP(point_size, 0.125, 255.875), 3);
- if (ctx-Point.SpriteOrigin == GL_LOWER_LEFT)
- dw1 |= GEN6_SF_POINT_SPRITE_LOWERLEFT;
+ /*
+ * When rendering to FBO, rendering is inverted. At the same time,
+ * we would also make sure the point sprite origin is inverted.
+ * Or, we will get an inverted result corresponding to rendering
+ * to the default/window FBO.
+ */
+ if ((ctx-Point.SpriteOrigin == GL_LOWER_LEFT) ^ render_to_fbo) {
I (mostly) like that. I was trying to think of a way to simplify the
if-statements in the original patch, but I couldn't think of a good way.
However, using the bit-wise xor is not correct here. The compiler
accepts it because everything is an integer in C. Some tools, like
Coverty, will probably complain about this. You really want a ^^
(logical xor), but C doesn't have that. What C does have, that does
exactly the same thing, is ==. I suggest changing this to
if ((ctx-Point.SpriteOrigin == GL_LOWER_LEFT) == render_to_fbo) {
It looks a bit weird, but it is correct.
I suggested ^ to Yuanhan. Yes you have to be careful with it. Note that
X ^ render_to_fbo is already used earlier in the function.
I hadn't noticed that it was used earlier. I'll submit a patch to fix
those. :)
But I think you want != in this case, not ==.
Of course. *blush*
-Brian
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965: fix inverted point sprite origin when rendering to FBO
On Thu, Jan 19, 2012 at 09:51:32AM -0800, Eric Anholt wrote:
On Thu, 19 Jan 2012 10:30:53 +0800, Yuanhan Liu yuanhan@linux.intel.com
wrote:
When rendering to FBO, rendering is inverted. At the same time, we would
also make sure the point sprite origin is inverted. Or, we will get an
inverted result correspoinding to rendering to the default winsys FBO.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=44613
NOTE: This is a candidate for stable release branches.
v2: add the simliar logic to ivb, too (comments from Ian)
simplify the logic operation (comments from Brian)
Signed-off-by: Yuanhan Liu yuanhan@linux.intel.com
---
src/mesa/drivers/dri/i965/brw_defines.h |1 +
src/mesa/drivers/dri/i965/gen6_sf_state.c | 15 +--
src/mesa/drivers/dri/i965/gen7_sf_state.c | 20 +---
3 files changed, 31 insertions(+), 5 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_defines.h
b/src/mesa/drivers/dri/i965/brw_defines.h
index 4d90a99..029be87 100644
--- a/src/mesa/drivers/dri/i965/brw_defines.h
+++ b/src/mesa/drivers/dri/i965/brw_defines.h
@@ -1128,6 +1128,7 @@ enum brw_message_target {
/* DW1 (for gen6) */
# define GEN6_SF_NUM_OUTPUTS_SHIFT 22
# define GEN6_SF_SWIZZLE_ENABLE(1 21)
+# define GEN6_SF_POINT_SPRITE_UPPERLEFT(0 20)
# define GEN6_SF_POINT_SPRITE_LOWERLEFT(1 20)
# define GEN6_SF_URB_ENTRY_READ_LENGTH_SHIFT 11
# define GEN6_SF_URB_ENTRY_READ_OFFSET_SHIFT 4
diff --git a/src/mesa/drivers/dri/i965/gen6_sf_state.c
b/src/mesa/drivers/dri/i965/gen6_sf_state.c
index 548c5a3..67c208b 100644
--- a/src/mesa/drivers/dri/i965/gen6_sf_state.c
+++ b/src/mesa/drivers/dri/i965/gen6_sf_state.c
@@ -129,6 +129,7 @@ upload_sf_state(struct brw_context *brw)
float point_size;
uint16_t attr_overrides[FRAG_ATTRIB_MAX];
bool userclip_active;
+ uint32_t point_sprite_origin;
/* _NEW_TRANSFORM */
userclip_active = (ctx-Transform.ClipPlanesEnabled != 0);
@@ -258,8 +259,18 @@ upload_sf_state(struct brw_context *brw)
/* Clamp to the hardware limits and convert to fixed point */
dw4 |= U_FIXED(CLAMP(point_size, 0.125, 255.875), 3);
- if (ctx-Point.SpriteOrigin == GL_LOWER_LEFT)
- dw1 |= GEN6_SF_POINT_SPRITE_LOWERLEFT;
+ /*
+* When rendering to FBO, rendering is inverted. At the same time,
+* we would also make sure the point sprite origin is inverted.
+* Or, we will get an inverted result corresponding to rendering
+* to the default/window FBO.
+*/
I think this comment could be simplified to Window coordinates in an
FBO are inverted, which means point sprite origin must be inverted.
looks better, will change that. Thanks!
+ if ((ctx-Point.SpriteOrigin == GL_LOWER_LEFT) ^ render_to_fbo) {
+ point_sprite_origin = GEN6_SF_POINT_SPRITE_LOWERLEFT;
+ } else {
+ point_sprite_origin = GEN6_SF_POINT_SPRITE_UPPERLEFT;
+ }
Much better. That logic was hideous before.
Yeah, agreed. Well, the logic before was somehow clear ;)
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: fix inverted point sprite origin when rendering to FBO
On Thu, Jan 19, 2012 at 10:32:30AM -0700, Brian Paul wrote:
On 01/19/2012 10:17 AM, Ian Romanick wrote:
On 01/18/2012 06:30 PM, Yuanhan Liu wrote:
When rendering to FBO, rendering is inverted. At the same time, we
would
also make sure the point sprite origin is inverted. Or, we will get an
inverted result correspoinding to rendering to the default winsys FBO.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=44613
NOTE: This is a candidate for stable release branches.
v2: add the simliar logic to ivb, too (comments from Ian)
simplify the logic operation (comments from Brian)
Signed-off-by: Yuanhan Liuyuanhan@linux.intel.com
---
src/mesa/drivers/dri/i965/brw_defines.h | 1 +
src/mesa/drivers/dri/i965/gen6_sf_state.c | 15 +--
src/mesa/drivers/dri/i965/gen7_sf_state.c | 20 +---
3 files changed, 31 insertions(+), 5 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_defines.h
b/src/mesa/drivers/dri/i965/brw_defines.h
index 4d90a99..029be87 100644
--- a/src/mesa/drivers/dri/i965/brw_defines.h
+++ b/src/mesa/drivers/dri/i965/brw_defines.h
@@ -1128,6 +1128,7 @@ enum brw_message_target {
/* DW1 (for gen6) */
# define GEN6_SF_NUM_OUTPUTS_SHIFT 22
# define GEN6_SF_SWIZZLE_ENABLE (1 21)
+# define GEN6_SF_POINT_SPRITE_UPPERLEFT (0 20)
# define GEN6_SF_POINT_SPRITE_LOWERLEFT (1 20)
# define GEN6_SF_URB_ENTRY_READ_LENGTH_SHIFT 11
# define GEN6_SF_URB_ENTRY_READ_OFFSET_SHIFT 4
diff --git a/src/mesa/drivers/dri/i965/gen6_sf_state.c
b/src/mesa/drivers/dri/i965/gen6_sf_state.c
index 548c5a3..67c208b 100644
--- a/src/mesa/drivers/dri/i965/gen6_sf_state.c
+++ b/src/mesa/drivers/dri/i965/gen6_sf_state.c
@@ -129,6 +129,7 @@ upload_sf_state(struct brw_context *brw)
float point_size;
uint16_t attr_overrides[FRAG_ATTRIB_MAX];
bool userclip_active;
+ uint32_t point_sprite_origin;
/* _NEW_TRANSFORM */
userclip_active = (ctx-Transform.ClipPlanesEnabled != 0);
@@ -258,8 +259,18 @@ upload_sf_state(struct brw_context *brw)
/* Clamp to the hardware limits and convert to fixed point */
dw4 |= U_FIXED(CLAMP(point_size, 0.125, 255.875), 3);
- if (ctx-Point.SpriteOrigin == GL_LOWER_LEFT)
- dw1 |= GEN6_SF_POINT_SPRITE_LOWERLEFT;
+ /*
+ * When rendering to FBO, rendering is inverted. At the same time,
+ * we would also make sure the point sprite origin is inverted.
+ * Or, we will get an inverted result corresponding to rendering
+ * to the default/window FBO.
+ */
+ if ((ctx-Point.SpriteOrigin == GL_LOWER_LEFT) ^ render_to_fbo) {
I (mostly) like that. I was trying to think of a way to simplify the
if-statements in the original patch, but I couldn't think of a good way.
However, using the bit-wise xor is not correct here. The compiler
accepts it because everything is an integer in C. Some tools, like
Coverty, will probably complain about this. You really want a ^^
(logical xor), but C doesn't have that. What C does have, that does
exactly the same thing, is ==. I suggest changing this to
Thanks for the information.
if ((ctx-Point.SpriteOrigin == GL_LOWER_LEFT) == render_to_fbo) {
It looks a bit weird, but it is correct.
I suggested ^ to Yuanhan. Yes you have to be careful with it. Note
that X ^ render_to_fbo is already used earlier in the function.
But I think you want != in this case, not ==.
Using '!=' is ok to me. Will change that.
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: fix inverted point sprite origin when rendering to FBO
On 01/19/2012 03:48 PM, Yuanhan Liu wrote:
When rendering to FBO, rendering is inverted. At the same time, we would
also make sure the point sprite origin is inverted. Or, we will get an
inverted result correspoinding to rendering to the default winsys FBO.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=44613
NOTE: This is a candidate for stable release branches.
v2: add the simliar logic to ivb, too (comments from Ian)
simplify the logic operation (comments from Brian)
v3: pick a better comment from Eric
use != for the logic instead of ^ (comments from Ian)
Signed-off-by: Yuanhan Liuyuanhan@linux.intel.com
Reviewed-by: Ian Romanick ian.d.roman...@intel.com
---
src/mesa/drivers/dri/i965/brw_defines.h |1 +
src/mesa/drivers/dri/i965/gen6_sf_state.c | 13 +++--
src/mesa/drivers/dri/i965/gen7_sf_state.c | 18 +++---
3 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_defines.h
b/src/mesa/drivers/dri/i965/brw_defines.h
index 4d90a99..029be87 100644
--- a/src/mesa/drivers/dri/i965/brw_defines.h
+++ b/src/mesa/drivers/dri/i965/brw_defines.h
@@ -1128,6 +1128,7 @@ enum brw_message_target {
/* DW1 (for gen6) */
# define GEN6_SF_NUM_OUTPUTS_SHIFT22
# define GEN6_SF_SWIZZLE_ENABLE (1 21)
+# define GEN6_SF_POINT_SPRITE_UPPERLEFT(0 20)
# define GEN6_SF_POINT_SPRITE_LOWERLEFT (1 20)
# define GEN6_SF_URB_ENTRY_READ_LENGTH_SHIFT 11
# define GEN6_SF_URB_ENTRY_READ_OFFSET_SHIFT 4
diff --git a/src/mesa/drivers/dri/i965/gen6_sf_state.c
b/src/mesa/drivers/dri/i965/gen6_sf_state.c
index 548c5a3..163b54c 100644
--- a/src/mesa/drivers/dri/i965/gen6_sf_state.c
+++ b/src/mesa/drivers/dri/i965/gen6_sf_state.c
@@ -129,6 +129,7 @@ upload_sf_state(struct brw_context *brw)
float point_size;
uint16_t attr_overrides[FRAG_ATTRIB_MAX];
bool userclip_active;
+ uint32_t point_sprite_origin;
/* _NEW_TRANSFORM */
userclip_active = (ctx-Transform.ClipPlanesEnabled != 0);
@@ -258,8 +259,16 @@ upload_sf_state(struct brw_context *brw)
/* Clamp to the hardware limits and convert to fixed point */
dw4 |= U_FIXED(CLAMP(point_size, 0.125, 255.875), 3);
- if (ctx-Point.SpriteOrigin == GL_LOWER_LEFT)
- dw1 |= GEN6_SF_POINT_SPRITE_LOWERLEFT;
+ /*
+* Window coordinates in an FBO are inverted, which means point
+* sprite origin must be inverted, too.
+*/
+ if ((ctx-Point.SpriteOrigin == GL_LOWER_LEFT) != render_to_fbo) {
+ point_sprite_origin = GEN6_SF_POINT_SPRITE_LOWERLEFT;
+ } else {
+ point_sprite_origin = GEN6_SF_POINT_SPRITE_UPPERLEFT;
+ }
+ dw1 |= point_sprite_origin;
/* _NEW_LIGHT */
if (ctx-Light.ProvokingVertex != GL_FIRST_VERTEX_CONVENTION) {
diff --git a/src/mesa/drivers/dri/i965/gen7_sf_state.c
b/src/mesa/drivers/dri/i965/gen7_sf_state.c
index 7691cb2..da7ef81 100644
--- a/src/mesa/drivers/dri/i965/gen7_sf_state.c
+++ b/src/mesa/drivers/dri/i965/gen7_sf_state.c
@@ -48,6 +48,9 @@ upload_sbe_state(struct brw_context *brw)
int urb_entry_read_offset = 1;
bool userclip_active = (ctx-Transform.ClipPlanesEnabled != 0);
uint16_t attr_overrides[FRAG_ATTRIB_MAX];
+ /* _NEW_BUFFERS */
+ bool render_to_fbo = ctx-DrawBuffer-Name != 0;
+ uint32_t point_sprite_origin;
brw_compute_vue_map(vue_map, intel, userclip_active, vs_outputs_written);
urb_entry_read_length = (vue_map.num_slots + 1)/2 - urb_entry_read_offset;
@@ -65,9 +68,18 @@ upload_sbe_state(struct brw_context *brw)
urb_entry_read_length GEN7_SBE_URB_ENTRY_READ_LENGTH_SHIFT |
urb_entry_read_offset GEN7_SBE_URB_ENTRY_READ_OFFSET_SHIFT;
- /* _NEW_POINT */
- if (ctx-Point.SpriteOrigin == GL_LOWER_LEFT)
- dw1 |= GEN6_SF_POINT_SPRITE_LOWERLEFT;
+ /* _NEW_POINT
+*
+* Window coordinates in an FBO are inverted, which means point
+* sprite origin must be inverted.
+*/
+ if ((ctx-Point.SpriteOrigin == GL_LOWER_LEFT) != render_to_fbo) {
+ point_sprite_origin = GEN6_SF_POINT_SPRITE_LOWERLEFT;
+ } else {
+ point_sprite_origin = GEN6_SF_POINT_SPRITE_UPPERLEFT;
+ }
+ dw1 |= point_sprite_origin;
+
dw10 = 0;
dw11 = 0;
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965: fix inverted point sprite origin when rendering to FBO
On Wed, Jan 18, 2012 at 06:23:52PM +0800, Yuanhan Liu wrote: When rendering to FBO, rendering is inverted. At the same time, we would also make sure the point sprite origin is inverted. Or, we will get an inverted result correspoinding to rendering to the default winsys FBO. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=44613 Actually, I'm not sure about this patch, and I would like to treat this patch as a _workaround_. So, any comments are welcome. 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: fix inverted point sprite origin when rendering to FBO
On 01/18/2012 02:23 AM, Yuanhan Liu wrote:
When rendering to FBO, rendering is inverted. At the same time, we would
also make sure the point sprite origin is inverted. Or, we will get an
inverted result correspoinding to rendering to the default winsys FBO.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=44613
Signed-off-by: Yuanhan Liuyuanhan@linux.intel.com
---
src/mesa/drivers/dri/i965/brw_defines.h |1 +
src/mesa/drivers/dri/i965/gen6_sf_state.c | 19 +--
2 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_defines.h
b/src/mesa/drivers/dri/i965/brw_defines.h
index 4d90a99..029be87 100644
--- a/src/mesa/drivers/dri/i965/brw_defines.h
+++ b/src/mesa/drivers/dri/i965/brw_defines.h
@@ -1128,6 +1128,7 @@ enum brw_message_target {
/* DW1 (for gen6) */
# define GEN6_SF_NUM_OUTPUTS_SHIFT22
# define GEN6_SF_SWIZZLE_ENABLE (1 21)
+# define GEN6_SF_POINT_SPRITE_UPPERLEFT(0 20)
# define GEN6_SF_POINT_SPRITE_LOWERLEFT (1 20)
# define GEN6_SF_URB_ENTRY_READ_LENGTH_SHIFT 11
# define GEN6_SF_URB_ENTRY_READ_OFFSET_SHIFT 4
diff --git a/src/mesa/drivers/dri/i965/gen6_sf_state.c
b/src/mesa/drivers/dri/i965/gen6_sf_state.c
index 548c5a3..d354a2b 100644
--- a/src/mesa/drivers/dri/i965/gen6_sf_state.c
+++ b/src/mesa/drivers/dri/i965/gen6_sf_state.c
I think gen7_sf_state.c needs similar changes.
@@ -129,6 +129,7 @@ upload_sf_state(struct brw_context *brw)
float point_size;
uint16_t attr_overrides[FRAG_ATTRIB_MAX];
bool userclip_active;
+ int point_sprite_origin;
dw1 is a uint32_t, so this should be too.
/* _NEW_TRANSFORM */
userclip_active = (ctx-Transform.ClipPlanesEnabled != 0);
@@ -258,8 +259,22 @@ upload_sf_state(struct brw_context *brw)
/* Clamp to the hardware limits and convert to fixed point */
dw4 |= U_FIXED(CLAMP(point_size, 0.125, 255.875), 3);
- if (ctx-Point.SpriteOrigin == GL_LOWER_LEFT)
- dw1 |= GEN6_SF_POINT_SPRITE_LOWERLEFT;
+ /*
+* When rendering to FBO, rendering is inverted. At the same time,
+* we would also make sure the point sprite origin is inverted.
+* Or, we will get an inverted result corresponding to rendering
+* to the default/window FBO.
+*/
+ if (ctx-Point.SpriteOrigin == GL_LOWER_LEFT) {
+ point_sprite_origin = GEN6_SF_POINT_SPRITE_LOWERLEFT;
+ if (render_to_fbo)
+ point_sprite_origin = GEN6_SF_POINT_SPRITE_UPPERLEFT;
+ } else {
+ point_sprite_origin = GEN6_SF_POINT_SPRITE_UPPERLEFT;
+ if (render_to_fbo)
+ point_sprite_origin = GEN6_SF_POINT_SPRITE_LOWERLEFT;
+ }
+ dw1 |= point_sprite_origin;
/* _NEW_LIGHT */
if (ctx-Light.ProvokingVertex != GL_FIRST_VERTEX_CONVENTION) {
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965: fix inverted point sprite origin when rendering to FBO
On Wed, Jan 18, 2012 at 12:13:28PM -0800, Ian Romanick wrote: On 01/18/2012 02:21 AM, Yuanhan Liu wrote: On Wed, Jan 18, 2012 at 06:23:52PM +0800, Yuanhan Liu wrote: When rendering to FBO, rendering is inverted. At the same time, we would also make sure the point sprite origin is inverted. Or, we will get an inverted result correspoinding to rendering to the default winsys FBO. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=44613 Actually, I'm not sure about this patch, and I would like to treat this patch as a _workaround_. So, any comments are welcome. Why? Do you have a different solution in mind? Since I tried to 'fix' this issue. Acutally, this is the first workaround fix I thought when met this issue. I just don't know why then. Well, Nanhai's suggestion told me that this _might_ be the right fix for this issue. But I'm still a little unsure, so that's why. This patch fixes this problem the way I would have expected it to be fixed. Yes, it does. 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: fix inverted point sprite origin when rendering to FBO
On Wed, Jan 18, 2012 at 11:53:20AM -0800, Ian Romanick wrote:
On 01/18/2012 02:23 AM, Yuanhan Liu wrote:
When rendering to FBO, rendering is inverted. At the same time, we would
also make sure the point sprite origin is inverted. Or, we will get an
inverted result correspoinding to rendering to the default winsys FBO.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=44613
Signed-off-by: Yuanhan Liuyuanhan@linux.intel.com
---
src/mesa/drivers/dri/i965/brw_defines.h |1 +
src/mesa/drivers/dri/i965/gen6_sf_state.c | 19 +--
2 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_defines.h
b/src/mesa/drivers/dri/i965/brw_defines.h
index 4d90a99..029be87 100644
--- a/src/mesa/drivers/dri/i965/brw_defines.h
+++ b/src/mesa/drivers/dri/i965/brw_defines.h
@@ -1128,6 +1128,7 @@ enum brw_message_target {
/* DW1 (for gen6) */
# define GEN6_SF_NUM_OUTPUTS_SHIFT 22
# define GEN6_SF_SWIZZLE_ENABLE(1 21)
+# define GEN6_SF_POINT_SPRITE_UPPERLEFT (0 20)
# define GEN6_SF_POINT_SPRITE_LOWERLEFT(1 20)
# define GEN6_SF_URB_ENTRY_READ_LENGTH_SHIFT 11
# define GEN6_SF_URB_ENTRY_READ_OFFSET_SHIFT 4
diff --git a/src/mesa/drivers/dri/i965/gen6_sf_state.c
b/src/mesa/drivers/dri/i965/gen6_sf_state.c
index 548c5a3..d354a2b 100644
--- a/src/mesa/drivers/dri/i965/gen6_sf_state.c
+++ b/src/mesa/drivers/dri/i965/gen6_sf_state.c
I think gen7_sf_state.c needs similar changes.
Yeah, you are right.
@@ -129,6 +129,7 @@ upload_sf_state(struct brw_context *brw)
float point_size;
uint16_t attr_overrides[FRAG_ATTRIB_MAX];
bool userclip_active;
+ int point_sprite_origin;
dw1 is a uint32_t, so this should be too.
Yes.
Will sent an updated patch soon.
/* _NEW_TRANSFORM */
userclip_active = (ctx-Transform.ClipPlanesEnabled != 0);
@@ -258,8 +259,22 @@ upload_sf_state(struct brw_context *brw)
/* Clamp to the hardware limits and convert to fixed point */
dw4 |= U_FIXED(CLAMP(point_size, 0.125, 255.875), 3);
- if (ctx-Point.SpriteOrigin == GL_LOWER_LEFT)
- dw1 |= GEN6_SF_POINT_SPRITE_LOWERLEFT;
+ /*
+* When rendering to FBO, rendering is inverted. At the same time,
+* we would also make sure the point sprite origin is inverted.
+* Or, we will get an inverted result corresponding to rendering
+* to the default/window FBO.
+*/
+ if (ctx-Point.SpriteOrigin == GL_LOWER_LEFT) {
+ point_sprite_origin = GEN6_SF_POINT_SPRITE_LOWERLEFT;
+ if (render_to_fbo)
+ point_sprite_origin = GEN6_SF_POINT_SPRITE_UPPERLEFT;
+ } else {
+ point_sprite_origin = GEN6_SF_POINT_SPRITE_UPPERLEFT;
+ if (render_to_fbo)
+ point_sprite_origin = GEN6_SF_POINT_SPRITE_LOWERLEFT;
+ }
+ dw1 |= point_sprite_origin;
/* _NEW_LIGHT */
if (ctx-Light.ProvokingVertex != GL_FIRST_VERTEX_CONVENTION) {
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
