On Tue, 2022-06-14 at 10:36 +0300, Pekka Paalanen wrote: > On Mon, 13 Jun 2022 14:54:57 +0000 > Zack Rusin <za...@vmware.com> wrote: > > > On Mon, 2022-06-13 at 17:25 +0300, Pekka Paalanen wrote: > > > On Mon, 13 Jun 2022 13:14:48 +0000 > > > Zack Rusin <za...@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 <za...@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. > > I did read that from your replies to the claims that you are ignoring > CRTC_X/Y. It's the impression I got. > > Saying that you don't ignore CRTC_X/Y but also implying that you are > not always using them as the cursor image position did not make sense > to me. > > > 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. > > The reason I am saying that you need to fix other issues with > virtualized drivers at the same time is because those other issues are > the sole and only reason why the driver would ever need hotspot info. > > Hotspot info must not be necessary for correct KMS operation, yet you > seem to insist it is. You assume that cursor plane commandeering is ok > to do. But it is not ok without adding the KMS UAPI that would allow > it: a way for guest userspace to explicitly say that commandeering will > be ok. > > If and only if guest userspace says commandeering is ok, then you can > require hotspot info. On the other hand, you cannot retrofit hotspot > info by saying that if a driver exposes hotspot properties then all KMS > clients must set them. That would be a kernel regression by definition: > KMS clients that used to abide the KMS UAPI contract are suddenly > breaking the contract because the kernel changed the contract. > > Therefore I very much disagree that virtualized drivers need hotspot > info. They do not strictly need hotspot info for correct operation, > they need it only for making the user experience more smooth, which is > an optional optimization. That optimization may be very important in > practise, but it's still an optimization and is generally not expected > by KMS clients.
I strongly disagree with that (both the sentiment towards hotspots and the client handling of it). I don't think we have to agree on reasoning here at all to make progress though so I'm going to let it go (we can always continue on irc or email if you'd like to try to conclude this bit but we could all use a few days of break from this discussion probably). > > 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. > > I thought we already agreed on that. I think it's a fine plan from > userspace perspective, and it's along the lines of what Daniel Vetter > wrote which I agreed to. > > I don't care if there is cursor plane not being exposed or yes being > exposed. I only care that if a cursor plane is exposed, and it is used > by a KMS client, and the KMS client does not explicitly indicate that > commandeering is ok, then commandeering cannot happen. ok, sounds good, looks like we have a way forward. Let me respin the series with the above suggested patch plus add the new cap code to mutter to get gnome-shell working so that we can all see how it would look/work in practice and see if we're all happy enough with it (or at least less unhappy with it than the current situation). z
