[PATCH v8 xserver 6/7] xf86Cursor: Deal with rotation on GPU screens using a hw-cursor

[Hans de Goede]

Hi,

On 12-09-16 10:39, Michel Dänzer wrote:
> On 12/09/16 05:24 PM, Hans de Goede wrote:
>> On 12-09-16 09:47, Michel Dänzer wrote:
>>> On 09/09/16 09:50 PM, Hans De Goede wrote:
>>>> When a slave-output is rotated the transformation is done on the blit
>>>> from master to slave GPU, so crtc->transform_in_use is not set, but we
>>>> still need to adjust the mouse position for things to work.
>>>>
>>>> This commit modifies xf86_crtc_transform_cursor_position to not rely
>>>> on crtc->f_framebuffer_to_crtc, so that it can be used with GPU screens
>>>> to and always calls it for cursors on GPU screens.
>>>>
>>>> Note not using crtc->f_framebuffer_to_crtc means that crtc->transform
>>>> will not be taken into account, that is ok, because when we've a
>>>> transform
>>>> active hw-cursors are not used and xf86_crtc_transform_cursor_position
>>>> will never get called.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>>>> ---
>>>> Changes in v8:
>>>> -Fix reflection on rotated outputs (use xf86_crtc_rotate_coord_back
>>>> again)
>>>> -Only call xf86_crtc_transform_cursor_position when rotating or
>>>> reflecting
>>>>  (instead of always calling it for GPU screens)
>>>
>>> [...]
>>>
>>>> +    rotation = crtc->rotation & 0xf;
>>>> +    if (rotation == RR_Rotate_90 || rotation == RR_Rotate_270) {
>>>> +        width = crtc->mode.VDisplay;
>>>> +        height = crtc->mode.HDisplay;
>>>> +    } else {
>>>> +        width = crtc->mode.HDisplay;
>>>> +        height = crtc->mode.VDisplay;
>>>> +    }
>>>
>>> [...]
>>>
>>>> +    xf86_crtc_rotate_coord_back(crtc->rotation, width, height, *x,
>>>> *y, x, y);
>>>
>>> Instead of this, how about something like:
>>>
>>>     xf86_crtc_rotate_coord(RR_Reflect_X - (crtc->rotation & 0xf),
>>>                width, height, *x, *y, x, y);
>>>
>>> Though frankly, either of those seem a little dirty, shoehorning
>>> xf86_crtc_rotate_coord(_back) into doing something they're not intended
>>> for. But between two evils, I'd choose the simpler one. :)
>>>
>>> The cleaner alternative would be not using xf86_crtc_rotate_coord(_back)
>>> but open-coding the correct logic, basically like in v7 but with the
>>> reflection handling moved after the rotation handling.
>>
>> Actually we need to swap width/height on rotation before doing:
>>
>>     if (rotation & RR_Reflect_X)
>>         x_dst = width - x_dst - 1;
>>     if (rotation & RR_Reflect_Y)
>>         y_dst = height - y_dst - 1;
>>
>> To get reflection working on rotated outputs!
>
> I don't think so. Reflection is a transform in CRTC space, so it should
> work with the rotated coordinates and crtc->mode.HDisplay for width and
> crtc->mode.VDisplay for height. If you tried that and it didn't seem to
> work, please let me see that patch.

Ok, so I've been doing some more investigation of this, doing the reflection
after rotation does work, using crtc->mode.HDisplay to reflect x and
crtc->mode.VDisplay to reflect y, but since we're doing this after rotation
we must treat RR_Reflect_X as RR_Reflect_Y (and the other way around)
when doing 90 or 270 rotation, with that fixed v7 of this patch
(which you seem to like better) works.

v8 just was a tricky way to get the same end result really.

Functionally the 2 are identical. I'm going to send out v9 now, which
is a fixed v7 really. Let me know which one you prefer (v8 or v9),
then I'll go and prepare a pull-req for 1.19 with your prefered version
in there.

Regards,

Hans