Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS
> On Jun 3, 2022, at 10:14 AM, Simon Ser wrote: > > Hi, > > Please, read this thread: > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2020-March%2Fthread.html%23259615&data=05%7C01%7Czackr%40vmware.com%7C05141cb812004e947f0408da456b76e2%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637898625140237028%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=%2BZUG0OFC5SXC8%2Bm3D93AliT0VNJHbc1AEwnhVAnw8WQ%3D&reserved=0 > > It has a lot of information about the pitfalls of cursor hotspot and > other things done by VM software. > > In particular: since the driver will ignore the KMS cursor plane > position set by user-space, I don't think it's okay to just expose > without opt-in from user-space (e.g. with a DRM_CLIENT_CAP). > > cc wayland-devel and Pekka for user-space feedback. I think Thomas expressed our concerns and reasons why those wouldn’t work for us back then. Is there something else you’d like to add? > On Thursday, June 2nd, 2022 at 17:42, Zack Rusin wrote: > >> - all userspace code needs to hardcore a list of drivers which require >> hotspots because there's no way to query from drm "does this driver >> require hotspot" > > Can you elaborate? I'm not sure I understand what you mean here. Only some drivers require informations about cursor hotspot, user space currently has no way of figuring out which ones are those, i.e. amdgpu doesn’t care about hotspots, qxl does so when running on qxl without properly set cursor hotspot atomic kms will result in a desktop where mouse clicks have incorrect coordinates. There’s no way to differentiate between drivers that do not care about cursor hotspots and ones that absolutely require it. z
Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS
On Jun 3, 2022, at 10:56 AM, Simon Ser mailto:cont...@emersion.fr>> wrote: ⚠ External Email On Friday, June 3rd, 2022 at 16:38, Zack Rusin mailto:za...@vmware.com>> wrote: On Jun 3, 2022, at 10:32 AM, Simon Ser mailto:cont...@emersion.fr>> wrote: ⚠ External Email On Friday, June 3rd, 2022 at 16:27, Zack Rusin mailto:za...@vmware.com>> wrote: In particular: since the driver will ignore the KMS cursor plane position set by user-space, I don't think it's okay to just expose without opt-in from user-space (e.g. with a DRM_CLIENT_CAP). cc wayland-devel and Pekka for user-space feedback. I think Thomas expressed our concerns and reasons why those wouldn’t work for us back then. Is there something else you’d like to add? I disagreed, and I don't understand Thomas' reasoning. KMS clients will need an update to work correctly. Adding a new prop without a cap leaves existing KMS clients broken. Adding a cap allows both existing KMS clients and updated KMS clients to work correctly. I’m not sure what you mean here. They are broken right now. That’s what we’re fixing. That’s the reason why the virtualized drivers are on deny-lists for all atomic kms. So nothing needs to be updated. If you have a kms client that was using virtualized drivers with atomic kms then mouse clicks never worked correctly. So, yes, clients need to set cursor hotspot if they want mouse to work correctly with virtualized drivers. My proposal was: - Introduce DRM_CLIENT_CAP_CURSOR_PLANE_NO_POSITION (or a better name). Only user-space which supports the hotspot props will enable it. - By default, don't expose a cursor plane, because current user-space doesn't support it (Weston might put a window in the cursor plane, and nobody can report hotspot). - If the KMS client enables the cap, advertise the cursor plane, and make it so the plane doesn't have the CRTC_X/CRTC_Y properties since the driver will pick the position. That way both old and new user-space are fixed. I don’t think I see how that fixes anything. In particular I don’t see a way of fixing the old user space at all. We require hotspot info, old user space doesn’t set it because there’s no way of setting it on atomic kms. Whether we expose cursor plane or not doesn’t change the fact that we still require the hotspot info. I don’t know… Unless the small-print in your proposal is “rewrite mouse cursor support in all hypervisors” (which I don’t see how you could make work without hotspot info) then I don’t think this solves anything, old or new. Or did you have some magical way of extracting the hotspot data without the kms clients providing it? e.g. in your proposal in virtgpu_plane.c how is output->cursor.hot_x and output->cursor.hot_y set without a cursor plane and with it? z
Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS
On Jun 3, 2022, at 11:22 AM, Simon Ser mailto:cont...@emersion.fr>> wrote: ⚠ External Email On Friday, June 3rd, 2022 at 17:17, Zack Rusin mailto:za...@vmware.com>> wrote: On Jun 3, 2022, at 10:56 AM, Simon Ser mailto:cont...@emersion.fr>> wrote: ⚠ External Email On Friday, June 3rd, 2022 at 16:38, Zack Rusin mailto:za...@vmware.com>> wrote: On Jun 3, 2022, at 10:32 AM, Simon Ser mailto:cont...@emersion.fr>> wrote: ⚠ External Email On Friday, June 3rd, 2022 at 16:27, Zack Rusin mailto:za...@vmware.com>> wrote: In particular: since the driver will ignore the KMS cursor plane position set by user-space, I don't think it's okay to just expose without opt-in from user-space (e.g. with a DRM_CLIENT_CAP). cc wayland-devel and Pekka for user-space feedback. I think Thomas expressed our concerns and reasons why those wouldn’t work for us back then. Is there something else you’d like to add? I disagreed, and I don't understand Thomas' reasoning. KMS clients will need an update to work correctly. Adding a new prop without a cap leaves existing KMS clients broken. Adding a cap allows both existing KMS clients and updated KMS clients to work correctly. I’m not sure what you mean here. They are broken right now. That’s what we’re fixing. That’s the reason why the virtualized drivers are on deny-lists for all atomic kms. So nothing needs to be updated. If you have a kms client that was using virtualized drivers with atomic kms then mouse clicks never worked correctly. So, yes, clients need to set cursor hotspot if they want mouse to work correctly with virtualized drivers. My proposal was: - Introduce DRM_CLIENT_CAP_CURSOR_PLANE_NO_POSITION (or a better name). Only user-space which supports the hotspot props will enable it. - By default, don't expose a cursor plane, because current user-space doesn't support it (Weston might put a window in the cursor plane, and nobody can report hotspot). - If the KMS client enables the cap, advertise the cursor plane, and make it so the plane doesn't have the CRTC_X/CRTC_Y properties since the driver will pick the position. That way both old and new user-space are fixed. I don’t think I see how that fixes anything. In particular I don’t see a way of fixing the old user space at all. We require hotspot info, old user space doesn’t set it because there’s no way of setting it on atomic kms. Old atomic user-space is fixed by removing the cursor plane. Then old atomic user-space will fallback to drawing the cursor itself, e.g. via OpenGL. But it’s not fixed, because the driver is still on a deny-list and nothing implements this. You’re saying you could potentially “fix” by implementing client side cursor handling in all kms clients? That’s a hard sell if the user space can just put the virtualized driver on deny-lists and fallback to use old kms which supports hotspots. New user-space supports setting the hotspot prop, and is aware that it can't set the cursor plane position, so the cursor plane can be exposed again when the cap is enabled. But we still use cursor plane position. Hotspots are offsets from cursor plane positions. Both are required. z
Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS
> On Jun 3, 2022, at 10:32 AM, Simon Ser wrote: > > ⚠ External Email > > On Friday, June 3rd, 2022 at 16:27, Zack Rusin wrote: > >>> In particular: since the driver will ignore the KMS cursor plane >>> position set by user-space, I don't think it's okay to just expose >>> without opt-in from user-space (e.g. with a DRM_CLIENT_CAP). >>> >>> cc wayland-devel and Pekka for user-space feedback. >> >> I think Thomas expressed our concerns and reasons why those wouldn’t >> work for us back then. Is there something else you’d like to add? > > I disagreed, and I don't understand Thomas' reasoning. > > KMS clients will need an update to work correctly. Adding a new prop > without a cap leaves existing KMS clients broken. Adding a cap allows > both existing KMS clients and updated KMS clients to work correctly. I’m not sure what you mean here. They are broken right now. That’s what we’re fixing. That’s the reason why the virtualized drivers are on deny-lists for all atomic kms. So nothing needs to be updated. If you have a kms client that was using virtualized drivers with atomic kms then mouse clicks never worked correctly. So, yes, clients need to set cursor hotspot if they want mouse to work correctly with virtualized drivers. >>> On Thursday, June 2nd, 2022 at 17:42, Zack Rusin z...@kde.org wrote: >>> >>>> - all userspace code needs to hardcore a list of drivers which require >>>> hotspots because there's no way to query from drm "does this driver >>>> require hotspot" >>> >>> Can you elaborate? I'm not sure I understand what you mean here. >> >> Only some drivers require informations about cursor hotspot, user space >> currently has no way of figuring out which ones are those, i.e. amdgpu >> doesn’t care about hotspots, qxl does so when running on qxl without >> properly set cursor hotspot atomic kms will result in a desktop where >> mouse clicks have incorrect coordinates. >> >> There’s no way to differentiate between drivers that do not care about >> cursor hotspots and ones that absolutely require it. > > Only VM drivers should expose the hotspot properties. Real drivers like > amdgpu must not expose it. Yes, that’s what I said. There’s no way to differentiate between amdgpu that doesn’t have those properties and virtio_gpu driver from a kernel before hotspot properties were added. z
Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS
On Jun 3, 2022, at 11:49 AM, Simon Ser mailto:cont...@emersion.fr>> wrote: ⚠ External Email On Friday, June 3rd, 2022 at 17:32, Zack Rusin mailto:za...@vmware.com>> wrote: On Jun 3, 2022, at 11:22 AM, Simon Ser mailto:cont...@emersion.fr>> wrote: ⚠ External Email On Friday, June 3rd, 2022 at 17:17, Zack Rusin mailto:za...@vmware.com>> wrote: On Jun 3, 2022, at 10:56 AM, Simon Ser mailto:cont...@emersion.fr>> wrote: ⚠ External Email On Friday, June 3rd, 2022 at 16:38, Zack Rusin mailto:za...@vmware.com>> wrote: On Jun 3, 2022, at 10:32 AM, Simon Ser mailto:cont...@emersion.fr>> wrote: ⚠ External Email On Friday, June 3rd, 2022 at 16:27, Zack Rusin mailto:za...@vmware.com>> wrote: In particular: since the driver will ignore the KMS cursor plane position set by user-space, I don't think it's okay to just expose without opt-in from user-space (e.g. with a DRM_CLIENT_CAP). cc wayland-devel and Pekka for user-space feedback. I think Thomas expressed our concerns and reasons why those wouldn’t work for us back then. Is there something else you’d like to add? I disagreed, and I don't understand Thomas' reasoning. KMS clients will need an update to work correctly. Adding a new prop without a cap leaves existing KMS clients broken. Adding a cap allows both existing KMS clients and updated KMS clients to work correctly. I’m not sure what you mean here. They are broken right now. That’s what we’re fixing. That’s the reason why the virtualized drivers are on deny-lists for all atomic kms. So nothing needs to be updated. If you have a kms client that was using virtualized drivers with atomic kms then mouse clicks never worked correctly. So, yes, clients need to set cursor hotspot if they want mouse to work correctly with virtualized drivers. My proposal was: - Introduce DRM_CLIENT_CAP_CURSOR_PLANE_NO_POSITION (or a better name). Only user-space which supports the hotspot props will enable it. - By default, don't expose a cursor plane, because current user-space doesn't support it (Weston might put a window in the cursor plane, and nobody can report hotspot). - If the KMS client enables the cap, advertise the cursor plane, and make it so the plane doesn't have the CRTC_X/CRTC_Y properties since the driver will pick the position. That way both old and new user-space are fixed. I don’t think I see how that fixes anything. In particular I don’t see a way of fixing the old user space at all. We require hotspot info, old user space doesn’t set it because there’s no way of setting it on atomic kms. Old atomic user-space is fixed by removing the cursor plane. Then old atomic user-space will fallback to drawing the cursor itself, e.g. via OpenGL. But it’s not fixed, because the driver is still on a deny-list and nothing implements this. You’re saying you could potentially “fix” by implementing client side cursor handling in all kms clients? That’s a hard sell if the user space can just put the virtualized driver on deny-lists and fallback to use old kms which supports hotspots. What deny-list are you referring to? All compositors I know of implement a fallback when no cursor plane is usable. I put the links in the first email in the series: https://gitlab.gnome.org/GNOME/mutter/-/blob/main/src/backends/native/meta-kms-impl-device-atomic.c#L1188 https://invent.kde.org/plasma/kwin/-/blob/master/src/backends/drm/drm_gpu.cpp#L156 Also, let me point this out because it also seems to be a fundamental misunderstanding, user space implementing software cursor doesn’t fix anything. Just leaves everything broken in different ways. The reason virtualized drivers went away from software cursors is because it makes it impossible to make it work with a bunch of interesting and desirable scenarios, e.g. unity mode where the guest might have a terminal and browser open and then the virtual machine software creates windows out of those, so you don’t have the entire desktop of the guest but instead native looking windows with the apps. In that case guest has no way of knowing when the cursor leaves the window, so to make seemless cursors work you’d need to implement irc or backdoors that send that information from the host to the guest, then have virtualized drivers create some sort of uevent, to send to the userspace to let it know to hide the cursor because it actually left the window. That’s a trivial scenario and there’s a lot more with mice that are remoted themselves, guests with singular or maybe many apps exported, possibly overlapping on the host but all within a desktop in the guest, etc. New user-space supports setting the hotspot prop, and is aware that it can't set the cursor plane position, so the cursor plane can be exposed again when the cap is enabled. But we still use cursor plane position. Hotspots are offsets from cursor plane positions. Both are required. No, VM drivers do
Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS
> On Jun 5, 2022, at 3:30 AM, Simon Ser wrote: > > ⚠ External Email > > On Friday, June 3rd, 2022 at 20:31, Zack Rusin wrote: > >>>>>>> My proposal was: >>>>>>> >>>>>>> - Introduce DRM_CLIENT_CAP_CURSOR_PLANE_NO_POSITION (or a better name). >>>>>>> Only >>>>>>> user-space which supports the hotspot props will enable it. >>>>>>> - By default, don't expose a cursor plane, because current user-space >>>>>>> doesn't >>>>>>> support it (Weston might put a window in the cursor plane, and nobody >>>>>>> can >>>>>>> report hotspot). >>>>>>> - If the KMS client enables the cap, advertise the cursor >>>>>>> plane, and make it so the plane doesn't have the CRTC_X/CRTC_Y >>>>>>> properties >>>>>>> since the driver will pick the position. >>>>>>> >>>>>>> That way both old and new user-space are fixed. >>>>>> >>>>>> I don’t think I see how that fixes anything. In particular I don’t see a >>>>>> way >>>>>> of fixing the old user space at all. We require hotspot info, old user >>>>>> space >>>>>> doesn’t set it because there’s no way of setting it on atomic kms. >>>>> >>>>> Old atomic user-space is fixed by removing the cursor plane. Then old >>>>> atomic user-space will fallback to drawing the cursor itself, e.g. via >>>>> OpenGL. >>>> >>>> But it’s not fixed, because the driver is still on a deny-list and >>>> nothing implements this. You’re saying you could potentially “fix” by >>>> implementing client side cursor handling in all kms clients? That’s a >>>> hard sell if the user space can just put the virtualized driver on >>>> deny-lists and fallback to use old kms which supports hotspots. >>> >>> What deny-list are you referring to? >>> >>> All compositors I know of implement a fallback when no cursor plane is >>> usable. >> >> I put the links in the first email in the >> series: >> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.gnome.org%2FGNOME%2Fmutter%2F-%2Fblob%2Fmain%2Fsrc%2Fbackends%2Fnative%2Fmeta-kms-impl-device-atomic.c%23L1188&data=05%7C01%7Czackr%40vmware.com%7C2b0ab6c67e7d4bfb618a08da46c5394f%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637900110157712044%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=wggJaNScF0ziIG%2BfSdSUKBVZGoNjtm4Q8amS7SbJa%2FY%3D&reserved=0 >> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Finvent.kde.org%2Fplasma%2Fkwin%2F-%2Fblob%2Fmaster%2Fsrc%2Fbackends%2Fdrm%2Fdrm_gpu.cpp%23L156&data=05%7C01%7Czackr%40vmware.com%7C2b0ab6c67e7d4bfb618a08da46c5394f%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637900110157712044%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=%2BCTEJ9XtlI9kuKJJZVMNodtkZSkRIv8RN9jSRAM1pqM%3D&reserved=0 > > I was not aware of these lists. Having to work-around drivers in user-space is > really not great. > > But regardless, that doesn't really change anything. With my proposal: > > - Old user-space with hacky deny-lists (Mutter, KWin) still goes through the > legacy uAPI. > - Old user-space without the hacky deny-lists (Weston, wlroots) uses software > cursors and is not broken anymore. > - New user-space can report hotspot and is not broken. Yea, that doesn’t work, but I think below I stopped where the issue is. >> Also, let me point this out because it also seems to be a fundamental >> misunderstanding, user space implementing software cursor doesn’t fix >> anything. Just leaves everything broken in different ways. The reason >> virtualized drivers went away from software cursors is because it makes it >> impossible to make it work with a bunch of interesting and desirable >> scenarios, e.g. unity mode where the guest might have a terminal and browser >> open and then the virtual machine software creates windows out of those, so >> you don’t have the entire desktop of the guest but instead native looking >> windows with the apps. In that case guest has no way of knowing when the >> cursor leaves the window, so to make seemless cursors work you’d need to >> implement irc or backdoors that send that information from the host to the >> guest, then have virtualiz
Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS
> On Jun 5, 2022, at 12:26 PM, Simon Ser wrote: > > --- Original Message --- > On Sunday, June 5th, 2022 at 17:47, Zack Rusin wrote: > >>>> Also, let me point this out because it also seems to be a fundamental >>>> misunderstanding, user space implementing software cursor doesn’t fix >>>> anything. Just leaves everything broken in different ways. The reason >>>> virtualized drivers went away from software cursors is because it makes it >>>> impossible to make it work with a bunch of interesting and desirable >>>> scenarios, e.g. unity mode where the guest might have a terminal and >>>> browser >>>> open and then the virtual machine software creates windows out of those, so >>>> you don’t have the entire desktop of the guest but instead native looking >>>> windows with the apps. In that case guest has no way of knowing when the >>>> cursor leaves the window, so to make seemless cursors work you’d need to >>>> implement irc or backdoors that send that information from the host to the >>>> guest, then have virtualized drivers create some sort of uevent, to send to >>>> the userspace to let it know to hide the cursor because it actually left >>>> the >>>> window. That’s a trivial scenario and there’s a lot more with mice that are >>>> remoted themselves, guests with singular or maybe many apps exported, >>>> possibly overlapping on the host but all within a desktop in the guest, >>>> etc. >>> >>> Are you saying that the current broken behavior is better than software >>> cursors? >> >> They’re broken in very different ways. You’re asking me which bugs do >> I prefer. Ultimately the current lack of hotspots leaves mouse unusable >> but I don’t see any reason to trade that for another set of bugs instead >> of just fixing it (either via fallback to legacy or using the new hotspot >> api). > > Software cursors aren't a bug. They're a fallback. They work well, or usually well enough, on native but not with virtual machines. They break a lot of features. The nature of virtualization is that the guest doesn’t explicitly get access to a lot of host side info. If you move to software cursor you automatically lose all that info (unless, as I mentioned before, you rewrite hypervisors to push all that info to guests again, either via backdoors, irq or register accesses). Software cursor just doesn’t work with modern hypervisors well enough to be a reasonable replacement. >>>>>>> New user-space supports setting the hotspot prop, and is aware that it >>>>>>> can't >>>>>>> set the cursor plane position, so the cursor plane can be exposed again >>>>>>> when >>>>>>> the cap is enabled. >>>>>> >>>>>> But we still use cursor plane position. Hotspots are offsets from >>>>>> cursor plane positions. Both are required. >>>>> >>>>> No, VM drivers don't need and disregard the cursor position AFAIK. They >>>>> replace it with the host cursor's position. >>>> >>>> Iirc they all use it. >>> >>> What do they use it for? >>> >>> The whole point of this discussion is to be able to display the guest's >>> cursor >>> image on the host's cursor, so that latency is reduced? >> >> >> Ah, I think I see now where the confusion is coming from. No, it’s >> definitely not. This has nothing to do with latency. By default >> position of a mouse cursor doesn’t tell us where the point that is >> actually used as an activation of a click is, e.g. a mouse cursor image >> with an arrow pointing to the top-left and a mouse cursor image pointing >> to the bottom right - both at the same position, as you can imagine it will >> become impossible to use the desktop if the click defaults to the top left, >> especially as the number of cursor images increases, you need information >> about which point within the cursor image activates the click, that’s the >> hotspot. You need to know the position of the image and you need to know >> which point within that image is responsible for actually activating the >> events. > > Yeah, that's what a hotspot is. > > By "the whole point of this discussion", I meant that the whole point > for VM drivers to expose a hardware cursor is to improve latency (and > provide other related features). > > At any rate, I consider broken any driver which exposes a cursor plane, > then doesn't display it exactly at the CRTC_X/CRTC_Y But we do… The cursor is at crtc_x, crtc_y. > or misbehaves if it's missing hotspot info. That doesn’t follow at all. How can they not behave incorrectly if the information is missing? Maybe we can refocus the discussion because it seems like we’re moving in circles. Your proposal for a feature cap and removal of the cursor plane doesn’t fix anything: - current user space which doesn’t fallback to legacy kms with virtualized driver is completely broken and obviously will always remain broken on all already released kernels - making the same software fallback to software cursor breaks a massive amount of interactions in VMs so what are you proposing? z
Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS
On Tue, 2022-06-07 at 11:07 +0300, Pekka Paalanen wrote: > On Fri, 03 Jun 2022 14:14:59 + > Simon Ser wrote: > > > Hi, > > > > Please, read this thread: > > https://lists.freedesktop.org/archives/dri-devel/2020-March/thread.html#259615 > > > > It has a lot of information about the pitfalls of cursor hotspot and > > other things done by VM software. > > > > In particular: since the driver will ignore the KMS cursor plane > > position set by user-space, I don't think it's okay to just expose > > without opt-in from user-space (e.g. with a DRM_CLIENT_CAP). > > > > cc wayland-devel and Pekka for user-space feedback. > > > > On Thursday, June 2nd, 2022 at 17:42, Zack Rusin wrote: > > > > > - all userspace code needs to hardcore a list of drivers which require > > > hotspots because there's no way to query from drm "does this driver > > > require hotspot" > > > > Can you elaborate? I'm not sure I understand what you mean here. > > > > Hi Zack and everyone, > > I would like to try to reboot this discussion and explain where I come > from. Maybe I have misunderstood something. First of all thanks for restarting the discussions. I think Gerd did a good job responding to individual points, so let me take a step back and explain the big picture here so we can reboot. > Which root problems do you want to solve exactly? The problem that this patch set is solving is the relatively trivial problem of not having a way of setting the hotspot in the atomic kms interface. When we (virtualized drivers) are using the native cursor we need to know not only the image and position of the cursor, we need to know which point within that cursor activates the click (going back to analogy from the previous email: cursor image with arrow point top-left and cursor image image with arrow pointing bottom-right will have different hotspots, likely [0, 0] in the first case and [cursor_width, cursor_height] in the latter). To correctly route the mouse clicks we need to be aware of the hotspot coordinates. Currently even though all virtualized drivers are atomic userspace has to explicitly disable atomic kms for those drivers because mouse clicks are offset incorrectly as soon as the cursor image changes from the top-left pointing arrow, i.e. the hotspot isn't at [0,0]). So we would like to be able to enable atomic kms with gnome and kde on virtualized drivers and to do that we need a way to pass hotspot coordinates from userspace to virtualized driver. That seems to be pretty self-explanatory and, I think, we all agree that will go through properties like in the attached patch set. Now, where the disagreements come from is from the fact that all virtualized drivers do not implement the atomic KMS cursor plane contract exactly as specified. In atomic kms with universal planes the "cursor" plane can be anything so asking for hotspot's for something that's not a cursor is a bit silly (but arguably so is calling it a cursor plane and then complaining that people expect cursor in it). So the argument is that we can't put hotspot data into atomic kms without first rewriting all of the virtualized drivers cursor code to fix the underlying contract violation that they all commit. That would likely be a lot easier sell if not that gnome/kde don't put none cursor stuff in the cursor plane, so all this work is to fix breakages that seem to affect 0 of our users (and I completely understand that we'd still like all the drivers to be correct and unified in terms of their behaviour, I'm just saying it's a hard sell without something that we can point to and say "this fixes/improves things for our customers") Finally there's a discussion on how to fix it and whether virtualized drivers need to do funky stuff with the cursor. I'd like to first make sure we're on the same page and then discuss how to fix it next, so let me finish by saying why not "software cursor". The easiest way to understand why we do what we do with the cursor, avoiding any complex and weird cases lets go back to the latency issue: the round trips to the guest to move the cursor are certainly noticeable when running locally, but with my VMware hat on, "local" is not the case that interest us, e.g. on ESXi every VM is accessed over a network so now we're dealing with remote round trips. When multiplied by slow connections and scale at which we're operating the "software cursors" go from "some latency" to "completely unusable". That's what I was alluding to in the earlier email when I said they're broken in different ways because if asked what would you prefer: cursor that when clicked is always i
Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS
On Wed, 2022-06-08 at 10:53 +0300, Pekka Paalanen wrote: > On Tue, 7 Jun 2022 17:50:24 + > Zack Rusin wrote: > > > On Tue, 2022-06-07 at 11:07 +0300, Pekka Paalanen wrote: > > > On Fri, 03 Jun 2022 14:14:59 + > > > Simon Ser wrote: > > > > > > > Hi, > > > > > > > > Please, read this thread: > > > > https://lists.freedesktop.org/archives/dri-devel/2020-March/thread.html#259615 > > > > > > > > It has a lot of information about the pitfalls of cursor hotspot and > > > > other things done by VM software. > > > > > > > > In particular: since the driver will ignore the KMS cursor plane > > > > position set by user-space, I don't think it's okay to just expose > > > > without opt-in from user-space (e.g. with a DRM_CLIENT_CAP). > > > > > > > > cc wayland-devel and Pekka for user-space feedback. > > > > > > > > On Thursday, June 2nd, 2022 at 17:42, Zack Rusin wrote: > > > > > > > > > - all userspace code needs to hardcore a list of drivers which require > > > > > hotspots because there's no way to query from drm "does this driver > > > > > require hotspot" > > > > > > > > Can you elaborate? I'm not sure I understand what you mean here. > > > > > > > > > > Hi Zack and everyone, > > > > > > I would like to try to reboot this discussion and explain where I come > > > from. Maybe I have misunderstood something. > > > > First of all thanks for restarting the discussions. I think Gerd did > > a good > > job responding to individual points, so let me take a step back and explain > > the big > > picture here so we can reboot. > > > > > Which root problems do you want to solve exactly? > > > > The problem that this patch set is solving is the relatively trivial > > problem of not > > having a way of setting the hotspot in the atomic kms interface. When we > > (virtualized drivers) are using the native cursor we need to know not only > > the image > > Could you clarify what is "native cursor" here? > I guess it is the host window system pointer's cursor? Right, exactly. I'm a little hesitant to call it "host" because it gets tricky in remote scenarios, where the host is some ESXi server but the local machine is the one that's actually interacting with the guest. So it's the cursor of the machine where the guest screen is displayed. > > Now, where the disagreements come from is from the fact that all > > virtualized drivers > > do not implement the atomic KMS cursor plane contract exactly as specified. > > In > > atomic kms with universal planes the "cursor" plane can be anything so > > asking for > > hotspot's for something that's not a cursor is a bit silly (but arguably so > > is > > calling it a cursor plane and then complaining that people expect cursor in > > it). > > > > So the argument is that we can't put hotspot data into atomic kms without > > first > > rewriting all of the virtualized drivers cursor code to fix the underlying > > contract > > violation that they all commit. That would likely be a lot easier sell if > > not that > > gnome/kde don't put none cursor stuff in the cursor plane, so all this work > > is to > > fix breakages that seem to affect 0 of our users (and I completely > > understand that > > we'd still like all the drivers to be correct and unified in terms of their > > behaviour, I'm just saying it's a hard sell without something that we can > > point to > > and say "this fixes/improves things for our customers") > > What's the cost of making paravirtualized drivers honour the UAPI contract? > Can't you do that on the side of implementing these new hotspot > properties, with the little addition to allowing guest userspace to be > explicit about whether it supports commandeering or not? I'm reluctant here because "fixing" here seems to be a bit ephemeral as we move from one solution to the next. I'm happy to write a patch that's adding a DRM_CLIENT_CAP_VIRTUAL_CURSOR_AWARE flag and hiding the cursor plane in virtualized drivers for clients that advertise DRM_CLIENT_CAP_ATOMIC but not DRM_CLIENT_CAP_VIRTUAL_CURSOR_AWARE, but that doesn't solve Weston on virtualized drivers. I feel like that's a larger discussion. One that we need to have in general
Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS
On Fri, 2022-06-10 at 10:59 +0200, Daniel Vetter wrote: > ⚠ External Email > > On Fri, Jun 10, 2022 at 10:41:05AM +0200, Daniel Vetter wrote: > > Hi all, > > > > Kinda top post because the thread is sprawling and I think we need a > > summary/restart. I think there's at least 3 issues here: > > > > - lack of hotspot property support, which means compositors can't really > > support hotspot with atomic. Which isn't entirely true, because you > > totally can use atomic for the primary planes/crtcs and the legacy > > cursor ioctls, but I understand that people might find that a bit silly > > :-) > > > > Anyway this problme is solved by the patch set here, and I think results > > in some nice cleanups to boot. > > > > - the fact that cursors for virtual drivers are not planes, but really > > special things. Which just breaks the universal plane kms uapi. That > > part isn't solved, and I do agree with Simon and Pekka that we really > > should solve this before we unleash even more compositors onto the > > atomic paths of virtual drivers. > > > > I think the simplest solution for this is: > > 1. add a new DRM_PLANE_TYPE_VIRTUAL_CURSOR, and set that for these > > special cursor planes on all virtual drivers > > 2. add the new "I understand virtual cursors planes" setparam, filter > > virtual cursor planes for userspace which doesn't set this (like we do > > right now if userspace doesn't set the universal plane mode) > > 3. backport the above patches to all stable kernels > > 4. make sure the hotspot property is only set on VIRTUAL_CURSOR planes > > and nothing else in the rebased patch series > > Simon also mentioned on irc that these special planes must not expose the > CRTC_X/Y property, since that doesn't really do much at all. Or is our > understanding of how this all works for commandeered cursors wrong? Yes, that's the part I don't understand. I don't think I see how the CRTC_X|Y properties aren't used. I think the confusion might stem from the fact that it appears that the hypervisors/hosts are moving that plane, which is not the case. We never move the plane itself, we redirect the mouse focus/movement, that's what's reducing the latency but don't touch drm internals. I can't speak for every virtualized stack, but what is happening on ours is that we set the image that's on the cursor plane as the cursor on the machine that's running the guest. So when you move the mouse across the screen as you enter the VM window the cursor plane isn't touched, but the local machines cursor changes to what's inside the cursor plane. When the mouse is clicked the mouse device in the guest generates the event with the proper coordinates (hence we need the hotspot to route that event correctly). That's when the guest reacts just like it would normally on native if a mouse event was received. The actual cursor plane might not be visible while this is happening but even when it's not visible it's still at whatever crtc_x|y the guest thinks it is. crtc_x|y are still only driven by the guests mouse device. We control the mouse device and visibility of the cursor plane itself based on what's happening in the system but I don't think that's that different from a native system. This is easy to see by running the "weston-simple-damage --width=64 --height=64" if you click on that window and move the cursor to the very edge of the virtualized window you'll notice that it's coming out outside of the window, that's because it's the local machines cursor, but if you stop the press the window will jump to wherever it was because the real guest plane is still at crtc_x|y (and because in weston that window doesn't react to mouse move events like a cursor would it never sets crtc_x|y to the mouse directed coordinates). The problem stems from the fact that the cursor plane has something that's not a cursor and doesn't react to mouse events like a cursor would. So the flag (or new plane) that we'd be adding is simply making user-space give the following promise: "I understand that what's inside the cursor plane needs to react and deal with mouse events like a mouse cursor would, i.e. it should be a mouse cursor" z
Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS
On Mon, 2022-06-13 at 10:33 +0300, Pekka Paalanen wrote: > On Fri, 10 Jun 2022 14:24:01 + > Zack Rusin wrote: > > > On Fri, 2022-06-10 at 10:59 +0200, Daniel Vetter wrote: > > > ⚠ External Email > > > > > > On Fri, Jun 10, 2022 at 10:41:05AM +0200, Daniel Vetter wrote: > > > > Hi all, > > > > > > > > Kinda top post because the thread is sprawling and I think we need a > > > > summary/restart. I think there's at least 3 issues here: > > > > > > > > - lack of hotspot property support, which means compositors can't really > > > > support hotspot with atomic. Which isn't entirely true, because you > > > > totally can use atomic for the primary planes/crtcs and the legacy > > > > cursor ioctls, but I understand that people might find that a bit > > > > silly :-) > > > > > > > > Anyway this problme is solved by the patch set here, and I think > > > > results > > > > in some nice cleanups to boot. > > > > > > > > - the fact that cursors for virtual drivers are not planes, but really > > > > special things. Which just breaks the universal plane kms uapi. That > > > > part isn't solved, and I do agree with Simon and Pekka that we really > > > > should solve this before we unleash even more compositors onto the > > > > atomic paths of virtual drivers. > > > > > > > > I think the simplest solution for this is: > > > > 1. add a new DRM_PLANE_TYPE_VIRTUAL_CURSOR, and set that for these > > > > special cursor planes on all virtual drivers > > > > 2. add the new "I understand virtual cursors planes" setparam, filter > > > > virtual cursor planes for userspace which doesn't set this (like we do > > > > right now if userspace doesn't set the universal plane mode) > > > > 3. backport the above patches to all stable kernels > > > > 4. make sure the hotspot property is only set on VIRTUAL_CURSOR planes > > > > and nothing else in the rebased patch series > > > > > > Simon also mentioned on irc that these special planes must not expose the > > > CRTC_X/Y property, since that doesn't really do much at all. Or is our > > > understanding of how this all works for commandeered cursors wrong? > > > > Yes, that's the part I don't understand. I don't think I see how the > > CRTC_X|Y > > properties aren't used. > > > > I think the confusion might stem from the fact that it appears that the > > hypervisors/hosts are moving that plane, which is not the case. We never > > move the > > plane itself, we redirect the mouse focus/movement, that's what's reducing > > the > > latency but don't touch drm internals. I can't speak for every virtualized > > stack, > > but what is happening on ours is that we set the image that's on the cursor > > plane as > > the cursor on the machine that's running the guest. So when you move the > > mouse > > across the screen as you enter the VM window the cursor plane isn't > > touched, but the > > local machines cursor changes to what's inside the cursor plane. When the > > mouse is > > clicked the mouse device in the guest generates the event with the proper > > coordinates (hence we need the hotspot to route that event correctly). > > That's when > > the guest reacts just like it would normally on native if a mouse event was > > received. > > > > The actual cursor plane might not be visible while this is happening but > > even when > > it's not visible it's still at whatever crtc_x|y the guest thinks it is. > > crtc_x|y > > are still only driven by the guests mouse device. > > > > We control the mouse device and visibility of the cursor plane itself based > > on > > what's happening in the system but I don't think that's that different from > > a native > > system. > > Sorry Zack, while I'm sure that is technically correct and very detaily > accurate, it's totally not any different to what we have been talking > about all along. > > We care about how things look like to the end user, and not what > property values the guest KMS driver might have for each plane. The KMS > UAPI contract is about how things look to the end user, not just what > values might be stored in a KMS driver (and then ignored). > > It do
Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS
On Mon, 2022-06-13 at 17:25 +0300, Pekka Paalanen wrote: > On Mon, 13 Jun 2022 13:14:48 + > Zack Rusin wrote: > > > On Mon, 2022-06-13 at 10:33 +0300, Pekka Paalanen wrote: > > > On Fri, 10 Jun 2022 14:24:01 + > > > Zack Rusin wrote: > > > > > > > On Fri, 2022-06-10 at 10:59 +0200, Daniel Vetter wrote: > > > > > ⚠ External Email > > > > > > > > > > On Fri, Jun 10, 2022 at 10:41:05AM +0200, Daniel Vetter wrote: > > > > > > Hi all, > > > > > > > > > > > > Kinda top post because the thread is sprawling and I think we need a > > > > > > summary/restart. I think there's at least 3 issues here: > > > > > > > > > > > > - lack of hotspot property support, which means compositors can't > > > > > > really > > > > > > support hotspot with atomic. Which isn't entirely true, because > > > > > > you > > > > > > totally can use atomic for the primary planes/crtcs and the legacy > > > > > > cursor ioctls, but I understand that people might find that a bit > > > > > > silly :-) > > > > > > > > > > > > Anyway this problme is solved by the patch set here, and I think > > > > > > results > > > > > > in some nice cleanups to boot. > > > > > > > > > > > > - the fact that cursors for virtual drivers are not planes, but > > > > > > really > > > > > > special things. Which just breaks the universal plane kms uapi. > > > > > > That > > > > > > part isn't solved, and I do agree with Simon and Pekka that we > > > > > > really > > > > > > should solve this before we unleash even more compositors onto the > > > > > > atomic paths of virtual drivers. > > > > > > > > > > > > I think the simplest solution for this is: > > > > > > 1. add a new DRM_PLANE_TYPE_VIRTUAL_CURSOR, and set that for these > > > > > > special cursor planes on all virtual drivers > > > > > > 2. add the new "I understand virtual cursors planes" setparam, > > > > > > filter > > > > > > virtual cursor planes for userspace which doesn't set this (like > > > > > > we do > > > > > > right now if userspace doesn't set the universal plane mode) > > > > > > 3. backport the above patches to all stable kernels > > > > > > 4. make sure the hotspot property is only set on VIRTUAL_CURSOR > > > > > > planes > > > > > > and nothing else in the rebased patch series > > > > > > > > > > Simon also mentioned on irc that these special planes must not expose > > > > > the > > > > > CRTC_X/Y property, since that doesn't really do much at all. Or is our > > > > > understanding of how this all works for commandeered cursors wrong? > > > > > > > > > > > > > Yes, that's the part I don't understand. I don't think I see how the > > > > CRTC_X|Y > > > > properties aren't used. > > > > > > > > I think the confusion might stem from the fact that it appears that the > > > > hypervisors/hosts are moving that plane, which is not the case. We > > > > never move the > > > > plane itself, we redirect the mouse focus/movement, that's what's > > > > reducing the > > > > latency but don't touch drm internals. I can't speak for every > > > > virtualized stack, > > > > but what is happening on ours is that we set the image that's on the > > > > cursor plane as > > > > the cursor on the machine that's running the guest. So when you move > > > > the mouse > > > > across the screen as you enter the VM window the cursor plane isn't > > > > touched, but the > > > > local machines cursor changes to what's inside the cursor plane. When > > > > the mouse is > > > > clicked the mouse device in the guest generates the event with the > > > > proper > > > > coordinates (hence we need the hotspot to route that event correctly). > > > > That's when > > > > the guest reac
Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS
On Tue, 2022-06-14 at 10:36 +0300, Pekka Paalanen wrote: > On Mon, 13 Jun 2022 14:54:57 + > Zack Rusin wrote: > > > On Mon, 2022-06-13 at 17:25 +0300, Pekka Paalanen wrote: > > > On Mon, 13 Jun 2022 13:14:48 + > > > Zack Rusin wrote: > > > > > > > On Mon, 2022-06-13 at 10:33 +0300, Pekka Paalanen wrote: > > > > > On Fri, 10 Jun 2022 14:24:01 + > > > > > Zack Rusin wrote: > > > > > > > > > > > On Fri, 2022-06-10 at 10:59 +0200, Daniel Vetter wrote: > > > > > > > ⚠ External Email > > > > > > > > > > > > > > On Fri, Jun 10, 2022 at 10:41:05AM +0200, Daniel Vetter wrote: > > > > > > > > > > > > > > > Hi all, > > > > > > > > > > > > > > > > Kinda top post because the thread is sprawling and I think we > > > > > > > > need a > > > > > > > > summary/restart. I think there's at least 3 issues here: > > > > > > > > > > > > > > > > - lack of hotspot property support, which means compositors > > > > > > > > can't really > > > > > > > > support hotspot with atomic. Which isn't entirely true, > > > > > > > > because you > > > > > > > > totally can use atomic for the primary planes/crtcs and the > > > > > > > > legacy > > > > > > > > cursor ioctls, but I understand that people might find that a > > > > > > > > bit silly :-) > > > > > > > > > > > > > > > > Anyway this problme is solved by the patch set here, and I > > > > > > > > think results > > > > > > > > in some nice cleanups to boot. > > > > > > > > > > > > > > > > - the fact that cursors for virtual drivers are not planes, but > > > > > > > > really > > > > > > > > special things. Which just breaks the universal plane kms > > > > > > > > uapi. That > > > > > > > > part isn't solved, and I do agree with Simon and Pekka that > > > > > > > > we really > > > > > > > > should solve this before we unleash even more compositors > > > > > > > > onto the > > > > > > > > atomic paths of virtual drivers. > > > > > > > > > > > > > > > > I think the simplest solution for this is: > > > > > > > > 1. add a new DRM_PLANE_TYPE_VIRTUAL_CURSOR, and set that for > > > > > > > > these > > > > > > > > special cursor planes on all virtual drivers > > > > > > > > 2. add the new "I understand virtual cursors planes" > > > > > > > > setparam, filter > > > > > > > > virtual cursor planes for userspace which doesn't set this > > > > > > > > (like we do > > > > > > > > right now if userspace doesn't set the universal plane mode) > > > > > > > > 3. backport the above patches to all stable kernels > > > > > > > > 4. make sure the hotspot property is only set on > > > > > > > > VIRTUAL_CURSOR planes > > > > > > > > and nothing else in the rebased patch series > > > > > > > > > > > > > > Simon also mentioned on irc that these special planes must not > > > > > > > expose the > > > > > > > CRTC_X/Y property, since that doesn't really do much at all. Or > > > > > > > is our > > > > > > > understanding of how this all works for commandeered cursors > > > > > > > wrong? > > > > > > > > > > > > Yes, that's the part I don't understand. I don't think I see how > > > > > > the CRTC_X|Y > > > > > > properties aren't used. > > > > > > > > > > > > I think the confusion might stem from the fact that it appears that > > > > > > the > > > > > > hypervisors/hosts are moving that plane, which is not the case. We > > > > > > never move the > &
Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS
On Wed, 2023-06-21 at 09:10 +0200, Javier Martinez Canillas wrote: > !! External Email > > [adding Zack Rusin again who seems to have fallen from the Cc list] > > Albert Esteve writes: > > On 6/10/22 10:59, Daniel Vetter wrote: > > > On Fri, Jun 10, 2022 at 10:41:05AM +0200, Daniel Vetter wrote: > > [...] > > > > > - third issue: These special virtual display properties arent > > > > documented. > > > > Aside from hotspot there's also suggested X/Y and maybe other stuff. > > > > I > > > > have no idea what suggested X/Y does and what userspace should do > > > > with > > > > it. I think we need a new section for virtualized drivers which: > > > > - documents all the properties involved > > > > - documents the new cap for enabling virtual cursor planes > > > > - documents some of the key flows that compositors should implement > > > > for > > > > best experience > > > > - documents how exactly the user experience will degrade if > > > > compositors > > > > pretend it's just a normal kms driver (maybe put that into each of > > > > the > > > > special flows that a compositor ideally supports) > > > > - whatever other comments and gaps I've missed, I'm sure > > > > Simon/Pekka/others will chime in once the patch exists. > > What is missing for these patches to land? If I understood correctly is > just these properties documentation that Sima asked above ? > > > > Great bonus would be an igt which demonstrates these flows. With the > > > interactive debug breakpoints to wait for resizing or whatever this should > > > be all neatly possible. > > > -Daniel > > > > Hi all, > > > > We have been testing the v2 of this patch and it works correctly for us. > > > > First we tested with a patched Mutter, the mouse clicks were correct, > > and behavior was as expected. > > > > But I've also added an IGT test as suggested above: > > https://gitlab.freedesktop.org/aesteve/igt-gpu-tools/-/compare/master...modeset-cursor-hotspot-test?from_project_id=1274 > > > > I could post the IGT patch upstream once Zack's patches land. > > > > Zack, are you planning to re-spin the series soon? Otherwise Albert could > continue working on that if you prefer. Besides mutter I wanted to patch kwin as well, but I haven't been able to find the time to patch it as well. I can respin with discussed changes over the weekend if we're ok with getting this in without kde support from the start. z
