[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, I've just retraced my steps, planning to go to v7 as posted first,
while testing that (cursor position clearly wrong when using 90/270
+ reflect, hard to describe how wrong exactly) I noticed that not only
was the cursor position off, but if I was drawing a desktop icon selection
rectangle on the background (nautilus does this), it would not
go lower on the rotated screen then the height of monitor, iow
I could only reach a square area of the monitor.

Then went back to v8, corsur now properly lines up with where X apps
think it is (corner of the selection rectangle), but I could still
only reach a square area of the monitor. Dropped the patch going back
to the original X server behavior -> same problem.

So it seems that reflection + rotation has a bug regardless of
this patch. I'll go look into fixing that first. TBC ...

Regards,

Hans