[Mesa-dev] [PATCH] i965: fix inverted point sprite origin when rendering to FBO
Liu Aleaxander
aleaxander at gmail.com
Fri Jan 20 09:35:19 PST 2012
On Jan 20, 2012 9:07 PM, "Jonathan Coome" <jcoome at 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.liu at 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 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
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120121/0bd4fac2/attachment.htm>
More information about the mesa-dev
mailing list