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

[Michel Dänzer]

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?

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.


> First lets call xf86_crtc_rotate_coord, with 270 degree rotation,

Where would that be? I can't see that happening anywhere, neither before
nor after this series.

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.


>>> 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. You're trying to use xf86_crtc_rotate_coord_back for
something else than what it's being used for now, and your changes break
xf86_crtc_transform_cursor_position if cursor_info->MaxWidth !=
cursor_info->MaxHeight .


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


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer