Re: [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 
---
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
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [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 
---
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




___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Michel Dänzer]

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


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [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 
---
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 

for that or

Acked-by: Michel Dänzer 

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
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Michel Dänzer]

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 
> ---
> 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. You can have

Reviewed-by: Michel Dänzer 

for that or

Acked-by: Michel Dänzer 

for either of the dirty options.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel