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

[Yuanhan Liu]

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.liu at 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.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.
> > +    */
> 
> 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