On Jan 20, 2012 9:07 PM, "Jonathan Coome" <<a href="mailto:jcoome@fastmail.co.uk">jcoome@fastmail.co.uk</a>> 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: <a href="https://bugs.freedesktop.org/show_bug.cgi?id=44613">https://bugs.freedesktop.org/show_bug.cgi?id=44613</a> > > > > 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 <<a href="mailto:yuanhan.liu@linux.intel.com">yuanhan.liu@linux.intel.com</a>> > > --- > > 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 my phone, please forgive for the poor format) >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 > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a>