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

[Hans de Goede]

Hi,

On 08-09-16 03:10, Michel Dänzer wrote:
> 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.

Ah, ok. I'm not all that familiar with this code, so I'm going to take your
word for this. I'll do a new patch fixing xf86_crtc_rotate_coord instead of
xf86_crtc_rotate_coord_back, and also fix patch 6/7 to do things differently.

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

That uses crtc->f_framebuffer_to_crtc which gets setup by xf86CrtcRotate(),
which at the very top has:

     if (pScreen->isGPU)
         return TRUE;

Which is why I went the route I went. I'll see if instead I can safely modify
xf86CrtcRotate() to still setup crtc->f_framebuffer_to_crtc, without
it having any side-effects.

Regards,

Hans