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

[Hans de Goede]

Hi,

On 09-09-16 03:11, Michel Dänzer wrote:
> On 08/09/16 07:08 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 v7:
>> -Do not use xf86_crtc_rotate_coord_back, it is not suitable for our purposes
>> -Modify xf86_crtc_transform_cursor_position instead of adding a new
>>  xf86_crtc_transform_gpu_cursor_position function
>
> [...]
>
>> diff --git a/hw/xfree86/modes/xf86Cursors.c b/hw/xfree86/modes/xf86Cursors.c
>> index 8437000..f638452 100644
>> --- a/hw/xfree86/modes/xf86Cursors.c
>> +++ b/hw/xfree86/modes/xf86Cursors.c
>> @@ -384,16 +384,35 @@ xf86_crtc_transform_cursor_position(xf86CrtcPtr crtc, int *x, int *y)
>>      xf86CursorScreenPtr ScreenPriv =
>>          (xf86CursorScreenPtr) dixLookupPrivate(&screen->devPrivates,
>>                                                 xf86CursorScreenKey);
>> -    struct pict_f_vector v;
>> -    int dx, dy;
>> -
>> -    v.v[0] = (*x + ScreenPriv->HotX) + 0.5;
>> -    v.v[1] = (*y + ScreenPriv->HotY) + 0.5;
>> -    v.v[2] = 1;
>> -    pixman_f_transform_point(&crtc->f_framebuffer_to_crtc, &v);
>> -    /* cursor will have 0.5 added to it already so floor is sufficent */
>> -    *x = floor(v.v[0]);
>> -    *y = floor(v.v[1]);
>> +    int dx, dy, t;
>> +
>> +    *x = *x - crtc->x + ScreenPriv->HotX;
>> +    *y = *y - crtc->y + ScreenPriv->HotY;
>> +
>> +    if (crtc->rotation & RR_Reflect_X)
>> +        *x = crtc->mode.HDisplay - *x - 1;
>> +    if (crtc->rotation & RR_Reflect_Y)
>> +        *y = crtc->mode.VDisplay - *y - 1;
>
> I think reflection needs to be applied after rotation. Please test the
> combination of 90/270 degree rotation and reflection to make sure it
> works well.

Funny I was thinking about this patchset while doing my
"laps" in the swimingpool this morning and I was wondering if I
got reflection right too (note no I didn't will fix in v8).

>> @@ -416,7 +436,7 @@ xf86_crtc_set_cursor_position(xf86CrtcPtr crtc, int x, int y)
>>      /*
>>       * Transform position of cursor on screen
>>       */
>> -    if (crtc->transform_in_use)
>> +    if (crtc->transform_in_use || screen->isGPU)
>>          xf86_crtc_transform_cursor_position(crtc, &crtc_x, &crtc_y);
>
> Use something like
>
>     if (crtc->transform_in_use || crtc->rotation != RR_Rotate_0)
>
> instead. No need to call xf86_crtc_transform_cursor_position for a GPU
> screen with crtc->rotation == RR_Rotate_0.

Coincidence or not this actually was the other thing I was
thinking about this morning. Since we're no longer using
crtc->f_framebuffer_to_crtc still checking for crtc->transform_in_use
is a bit weird and also unnecessary, so I've replaced this
with just if (crtc->rotation != RR_Rotate_0) for v8.

> Patches 3 & 5 are
>
> Reviewed-by: Michel Dänzer <michel.daenzer at amd.com>

Thanks.

> P.S. Until we've figured why I'm not receiving your patches from the
> list, please Cc this e-mail address of mine on future revisions of this
> patch.

Will do, v8 is coming up, since this is the last patch in the set
which has not been reviewed, I'm only go to send out this one
and not the entire set again.

Regards,

Hans