[Mesa-dev] [PATCH] i965: fix inverted point sprite origin when rendering to FBO

[Yuanhan Liu]

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 Liu<yuanhan.liu at 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