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

[Hans de Goede]

Hi,

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!

and we need the _back version because although you we're
right that reflection was broken in v7, moving:

     if (rotation & RR_Reflect_X)
         x_dst = width - x_dst - 1;
     if (rotation & RR_Reflect_Y)
         y_dst = height - y_dst - 1;

To after applying the rotation as the non _back version does,
does not fix things, the trick is to swap width <-> height
on 90 / 270 degrees rotation, that fixes reflection when
rotated (and yes I've also tested 0 degree rotation with
reflection).

At which point xf86_crtc_rotate_coord_back is an exact match
to what we need, since it:

1) Does the reflection before rotation
2) Does the right thing for 90 / 270 degree rotation if called
with swapped width/height (which is needed for reflection
to work in those cases anyways)

 > You can have
>
> Reviewed-by: Michel Dänzer <michel.daenzer at amd.com>
>
> for that or
>
> Acked-by: Michel Dänzer <michel.daenzer at amd.com>
>
> for either of the dirty options.

I'll take the Acked-by then, as said when first
swapping width/height on 90 / 270 degrees rotation
then xf86_crtc_rotate_coord_back is an exact match
to what we need, and we need to do that swap anyways
to get reflection to work. And since it is an
exact match essentially just copy and pasting its
entire contents to open-code things does not
seem useful to me.

Regards,

Hans