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

[Ian Romanick]

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

   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 |= point_sprite_origin;
> +
>
>      dw10 = 0;
>      dw11 = 0;