Hi,
On 08-09-16 03:10, Michel Dänzer wrote:
On 08/09/16 12:43 AM, Hans de Goede wrote:
/*
* Given a screen coordinate, rotate back to a cursor source coordinate
*/
xf86_crtc_rotate_coord(...)
/*
* Given a cursor source coordinate, rotate to a screen coordinate
*/
xf86_crtc_rotate_coord_back(...)
Looking at the comments, these 2 are meant to be each-others inverse
operation, so if I call them both succession I should get back the same
end result as before, but as explained in detail in my previous mail
for 90 / 270 degree rotation they are not each-others inverse, showing
that something is wrong.
I've come to agree with that, but the problem you describe is actually
in xf86_crtc_rotate_coord, not in xf86_crtc_rotate_coord_back. ASCII art
time!
First of all, I'm afraid xf86_crtc_rotate_coord(_back) talking about
"screen coordinate" is a little confusing. They really convert between
cursor space and CRTC space, not screen space (in the X screen sense).
Cursor space and screen space always have the same orientation.
+--------------------------- CRTC space x axis ------------------------+
| |
| +- cursor space y axis -+ |
| | | |
| c |
| u | C
| r | R
| s | T
| o | C
| r |
| | s
| s | p
| p | a
| a | c
| c | e
| e |
| | y
| x |
| | a
| a | x
| x | i
| i | s
| s |
| | | |
| +-----------------------+ |
| |
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
+----------------------------------------------------------------------+
Keep in mind that the width and height passed to both functions are in
cursor space.
When transforming from cursor space to CRTC space by 90/270 degrees, it
becomes clear that the CRTC space x depends on the cursor space height,
and the CRTC space y depends on the cursor space width, just as it's
done in xf86_crtc_rotate_coord_back.
However, for the reverse transform, the cursor space x depends on the
cursor space width, and the cursor space y depends on the cursor space
height, so xf86_crtc_rotate_coord has it backwards.
This was likely not noticed before since xf86_crtc_rotate_coord_back
until now was only used with cursor_info->MaxWidth and
cursor_info->MaxHeight, which are usally the same.
I'd say it's kind of the other way around:
xf86_crtc_transform_cursor_position just happens to still work with
your
change because usually cursor_info->MaxWidth ==
cursor_info->MaxHeight .
I could be wrong, but I don't think so, see above.
My analysis stands.
And I still believe your analysis is wrong.
You're trying to use xf86_crtc_rotate_coord_back for
something else than what it's being used for now,
xf86_crtc_rotate_coord_back() is described as "Given a cursor source
coordinate, rotate to a screen coordinate" which is exactly what we
need to properly position the cursor on a slave output, we start with
cursor coordinates and need to adjust those for the screen's rotation.
No, transform_(gpu_)cursor_position[0] take screen space coordinates and
transform them to CRTC space.
Ah, ok. I'm not all that familiar with this code, so I'm going to take your
word for this. I'll do a new patch fixing xf86_crtc_rotate_coord instead of
xf86_crtc_rotate_coord_back, and also fix patch 6/7 to do things differently.
Which means that neither xf86_crtc_rotate_coord_back nor
xf86_crtc_rotate_coord are a perfect fit for
xf86_crtc_transform_gpu_cursor_position. The fixed
xf86_crtc_rotate_coord might kind of work, but might require inverting
the rotation sense.
[0] BTW, do we really need two functions? The method used in
xf86_crtc_transform_gpu_cursor_position should work for non-GPU screens
as well, doesn't it?
That uses crtc->f_framebuffer_to_crtc which gets setup by xf86CrtcRotate(),
which at the very top has:
if (pScreen->isGPU)
return TRUE;
Which is why I went the route I went. I'll see if instead I can safely modify
xf86CrtcRotate() to still setup crtc->f_framebuffer_to_crtc, without
it having any side-effects.
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
