[PATCH v5 xserver 5/7] xf86Cursor: Fix xf86_crtc_rotate_coord_back using width/height wrongly

[Hans de Goede]

Hi,

On 07-09-16 17:19, Michel Dänzer wrote:
> On 07/09/16 08:43 PM, Hans de Goede wrote:
>> On 07-09-16 03:21, Michel Dänzer wrote:
>>> On 06/09/16 08:31 PM, Hans De Goede wrote:
>>>> xf86_crtc_rotate_coord_back should be the exact inverse operation of
>>>> xf86_crtc_rotate_coord, but when calculating x / y for 90 / 270 degrees
>>>> rotation it was using height to calculate x / width to calculate y,
>>>> instead of the otherway around.
>>>
>>> I think it's correct as is. It's certainly symmetrical with
>>> xf86_crtc_rotate_coord.
>>
>> Actually no, it is not symmetrical, it uses width to calculate y and
>> height to calculate x, where as xf86_crtc_rotate_coord uses
>> width when calculating x and height when calculating y as one would
>> expect.
>
> Are we looking at the same code?

Yes, the 270 case you quote is the same as the one I quoted.

> https://cgit.freedesktop.org/xorg/xserver/tree/hw/xfree86/modes/xf86Cursors.c#n75
>
>     case RR_Rotate_90:
>         t = x_dst;
>         x_dst = height - y_dst - 1;
>         y_dst = t;
>         break;
>
> https://cgit.freedesktop.org/xorg/xserver/tree/hw/xfree86/modes/xf86Cursors.c#n84
>
>     case RR_Rotate_270:
>         t = x_dst;
>         x_dst = y_dst;
>         y_dst = width - t - 1;
>         break;
>
>
> If you think about it, it makes sense that the x & y axes have to be
> crossed for 90/270 degree rotation.

Right. But there are *2* rotate functions:

xf86_crtc_rotate_coord      -> which you quoted above, which is fine
xf86_crtc_rotate_coord_back -> which I'm trying to fix.

And the comments above those 2 functions are:

/*
  * Given a screen coordinate, rotate back to a cursor source coordinate
  */
xf86_crtc_rotate_coord(...)

/*
  * Given a cursor source  coordinate, rotate to a screen coordinate
  */
xf86_crtc_rotate_coord_back(...)

Looking at the comments, these 2 are meant to be each-others inverse
operation, so if I call them both succession I should get back the same
end result as before, but as explained in detail in my previous mail
for 90 / 270 degree rotation they are not each-others inverse, showing
that something is wrong.

>> First lets call xf86_crtc_rotate_coord, with 270 degree rotation,
>
> Where would that be?

This is a hypotetical example to show that if calling both functions
in succession with the same rotation they are not each others
inverse as they should be.

> I can't see that happening anywhere, neither before
> nor after this series.

Both before and after xf86_crtc_rotate_coord[_back] get called with
whatever rotation is active on the crtc, as said the 270 degrees
is an example, it could have been 90 degrees too.

> Before your series, the only caller of xf86_crtc_rotate_coord_back is
> xf86_crtc_transform_cursor_position, which uses it to transform the
> position of the cursor hotspot inside the cursor image. The hotspot
> position isn't transformed with xf86_crtc_rotate_coord before that.
>
> Similarly, the only callers of xf86_crtc_rotate_coord just use it for
> transforming positions within the cursor image, not the position of the
> cursor relative to anything else.

So in one case we're rotating the image, and in the other case we are
rotating cursor coordinates to adjust for the hotspot of the rotated
image being in a different place then of the non rotated image, iow
one is used to cancel out the effects of the other.

Also just look at the comments above the 2 functions:
"screen coordinate, rotate to a cursor source coordinate" vs
"cursor source coordinate, rotate to a screen coordinate"

So we've screen->cursor and cursor->screen clearly these
should work as inverse functions, but they do not, hence
my patch to fix this.

>>>> This was likely not noticed before since xf86_crtc_rotate_coord_back
>>>> until now was only used with cursor_info->MaxWidth and
>>>> cursor_info->MaxHeight, which are usally the same.
>>>
>>> I'd say it's kind of the other way around:
>>> xf86_crtc_transform_cursor_position just happens to still work with your
>>> change because usually cursor_info->MaxWidth == cursor_info->MaxHeight .
>>
>> I could be wrong, but I don't think so, see above.
>
> My analysis stands.

And I still believe your analysis is wrong.

 > You're trying to use xf86_crtc_rotate_coord_back for
> something else than what it's being used for now,

xf86_crtc_rotate_coord_back() is described as "Given a cursor source
coordinate, rotate to a screen coordinate" which is exactly what we
need to properly position the cursor on a slave output, we start with
cursor coordinates and need to adjust those for the screen's rotation.

Yet it does not work and as shown in my previous mail the
2 functions are not each others inverse operation as they
should be, to me both point at a bug in xf86_crtc_rotate_coord_back.

 > and your changes break
> xf86_crtc_transform_cursor_position if cursor_info->MaxWidth !=
> cursor_info->MaxHeight .

And my proposition is that it has been broken for this case all
along and that I'm actually fixing it. Is there actually any
hardware which has cursor_info->MaxWidth != cursor_info->MaxHeight ?

Because if there is not that would explain why no one has noticed
that xf86_crtc_rotate_coord_back() is using width / height wrongly
in the 90 / 270 cases.

> That's a NAK for this patch (and patch 6 in the current form).

If you insist I can just copy the contents of the fixed
xf86_crtc_rotate_coord_back into patch 6, but that seems
counter productive.

Regards,

Hans