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

[Ian Romanick]

On 01/19/2012 09:32 AM, Brian Paul wrote:
> 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.

I hadn't noticed that it was used earlier.  I'll submit a patch to fix 
those. :)

> But I think you want != in this case, not ==.

Of course.  *blush*

> -Brian