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

[Brian Paul]

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
>
> 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 ==.

-Brian