[PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Zack Rusin]

On Mon, 2022-06-13 at 17:25 +0300, Pekka Paalanen wrote:
> On Mon, 13 Jun 2022 13:14:48 +0000
> Zack Rusin <zackr at vmware.com> wrote:
> 
> > On Mon, 2022-06-13 at 10:33 +0300, Pekka Paalanen wrote:
> > > On Fri, 10 Jun 2022 14:24:01 +0000
> > > Zack Rusin <zackr at vmware.com> 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 doesn't matter if the CRTC_X/Y properties remain at what the guest
> > > userspace set them to, if you are going to hide the "real" virtual
> > > cursor plane and instead upload the cursor image to the host window
> > > system to be composited independently. You are breaking the UAPI
> > > contract. What the end user sees is *not* what the guest OS programmed.
> > > That's the whole point.
> > > 
> > > What you described is the very definition of cursor plane commandeering
> > > as I defined it: showing the cursor image not where the guest OS put it.
> > > 
> > > I never assumed that you would actually reflect host cursor position in
> > > the guest KMS cursor plane CRTC_X/Y. You just ignore those values
> > > completely in the VM stack levels closer to the end user's eyes in
> > > seamless mouse mode. The end effect is the same.  
> > 
> > But we don't ignore them, we absolutely need them to set the mouse cursor. This is a
> > two way process, I think Hans mentioned that, mouse might be "seamless", i.e. it's
> > the host routing clicks to the guest, or it might be "guest locked", also known as
> > "gaming mouse", in which case it's fully guest routed/controlled. To have any idea
> > where the cursor is we absolutely require the crtc_x|y.
> 
> Ok, so seamless mouse mode ignores CRTC_X/Y. This is the commandeered
> mode.
> 
> The normal non-commandeered mode, or what you seem to call "guest
> locked" which I guess also includes grabbing the mouse into the VM
> viewer window in the host/viewer system, requires CRTC_X/Y. That's
> clear.
> 
> In other words, the VM stack is switching between seamless pointer,
> normal pointer, and normal pointer with a grab on the VM viewer winsys,
> right?
> 
> This only means that virtual cursor planes do need CRTC_X/Y properties.
> That's fine.
> 
> The VM stack is still breaking the KMS UAPI contract if the VM stack
> enters seamless pointer mode without an explicit approval from the guest
> userspace. You can't say it's ok to do seamless pointer if you
> *sometimes* also do normal pointer instead, that's not enough.

I don't think I ever said that. In general I'm not making any philosophical
argument, all I'm saying is that "virtualized drivers require hotspot info". Simon
and you are saying that we can't get it until we fix other issues with virtualized
drivers and I'm simply pointing out that your solutions do not work. When we started
I did mention that this is a lot bigger issue, that's been present for years and
will be hard to solve, which is why we're off to crazy town right now talking about
essentially forking kms for virtualized drivers.

I thought the solution consisting of an addition of 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 was simple enough and enough to get weston using
software cursors while having a clear path for gnome-shell and kwin to use atomic
with virtualized drivers without rewriting them. If we can't agree on that then, at
this point, it might be better if we just schedule an irc and/or video conference
with all interested parties to figure this out.

z