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

[Michel Dänzer]

On 08/09/16 12:43 AM, Hans de Goede wrote:
> 
> /*
>  * 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.

I've come to agree with that, but the problem you describe is actually
in xf86_crtc_rotate_coord, not in xf86_crtc_rotate_coord_back. ASCII art
time!

First of all, I'm afraid xf86_crtc_rotate_coord(_back) talking about
"screen coordinate" is a little confusing. They really convert between
cursor space and CRTC space, not screen space (in the X screen sense).
Cursor space and screen space always have the same orientation.

+--------------------------- CRTC space x axis ------------------------+
|                                                                      |
| +- cursor space y axis -+                                            |
| |                       |                                            |
| c                       |
| u                       |                                            C
| r                       |                                            R
| s                       |                                            T
| o                       |                                            C
| r                       |
|                         |                                            s
| s                       |                                            p
| p                       |                                            a
| a                       |                                            c
| c                       |                                            e
| e                       |
|                         |                                            y
| x                       |
|                         |                                            a
| a                       |                                            x
| x                       |                                            i
| i                       |                                            s
| s                       |
| |                       |                                            |
| +-----------------------+                                            |
|                                                                      |
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|                                                                      |
+----------------------------------------------------------------------+

Keep in mind that the width and height passed to both functions are in
cursor space.

When transforming from cursor space to CRTC space by 90/270 degrees, it
becomes clear that the CRTC space x depends on the cursor space height,
and the CRTC space y depends on the cursor space width, just as it's
done in xf86_crtc_rotate_coord_back.

However, for the reverse transform, the cursor space x depends on the
cursor space width, and the cursor space y depends on the cursor space
height, so xf86_crtc_rotate_coord has it backwards.


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

No, transform_(gpu_)cursor_position[0] take screen space coordinates and
transform them to CRTC space.

Which means that neither xf86_crtc_rotate_coord_back nor
xf86_crtc_rotate_coord are a perfect fit for
xf86_crtc_transform_gpu_cursor_position. The fixed
xf86_crtc_rotate_coord might kind of work, but might require inverting
the rotation sense.


[0] BTW, do we really need two functions? The method used in
xf86_crtc_transform_gpu_cursor_position should work for non-GPU screens
as well, doesn't it?

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 163 bytes
Desc: OpenPGP digital signature
URL: <https://lists.x.org/archives/xorg-devel/attachments/20160908/fea0f1f6/attachment.sig>