On Wed, 2023-05-03 at 09:48 +0200, Javier Martinez Canillas wrote: > Zack Rusin <zackr@xxxxxxxxxx> writes: > > > On Tue, 2023-05-02 at 11:32 +0200, Javier Martinez Canillas wrote: > > > !! External Email > > > > > > Daniel Vetter <daniel@xxxxxxxx> writes: > > > > > > > On Mon, Jul 11, 2022 at 11:32:39PM -0400, Zack Rusin wrote: > > > > > From: Zack Rusin <zackr@xxxxxxxxxx> > > > > > > > > > > Cursor planes on virtualized drivers have special meaning and require > > > > > that the clients handle them in specific ways, e.g. the cursor plane > > > > > should react to the mouse movement the way a mouse cursor would be > > > > > expected to and the client is required to set hotspot properties on it > > > > > in order for the mouse events to be routed correctly. > > > > > > > > > > This breaks the contract as specified by the "universal planes". Fix it > > > > > by disabling the cursor planes on virtualized drivers while adding > > > > > a foundation on top of which it's possible to special case mouse cursor > > > > > planes for clients that want it. > > > > > > > > > > Disabling the cursor planes makes some kms compositors which were broken, > > > > > e.g. Weston, fallback to software cursor which works fine or at least > > > > > better than currently while having no effect on others, e.g. gnome-shell > > > > > or kwin, which put virtualized drivers on a deny-list when running in > > > > > atomic context to make them fallback to legacy kms and avoid this issue. > > > > > > > > > > Signed-off-by: Zack Rusin <zackr@xxxxxxxxxx> > > > > > Fixes: 681e7ec73044 ("drm: Allow userspace to ask for universal plane list > > > > > (v2)") > > > > > > [...] > > > > > > > > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h > > > > > index f6159acb8856..c4cd7fc350d9 100644 > > > > > --- a/include/drm/drm_drv.h > > > > > +++ b/include/drm/drm_drv.h > > > > > @@ -94,6 +94,16 @@ enum drm_driver_feature { > > > > > * synchronization of command submission. > > > > > */ > > > > > DRIVER_SYNCOBJ_TIMELINE = BIT(6), > > > > > + /** > > > > > + * @DRIVER_VIRTUAL: > > > > > + * > > > > > + * Driver is running on top of virtual hardware. The most significant > > > > > + * implication of this is a requirement of special handling of the > > > > > + * cursor plane (e.g. cursor plane has to actually track the mouse > > > > > + * cursor and the clients are required to set hotspot in order for > > > > > + * the cursor planes to work correctly). > > > > > + */ > > > > > + DRIVER_VIRTUAL = BIT(7), > > > > > > > > I think the naming here is unfortunate, because people will vonder why > > > > e.g. vkms doesn't set this, and then add it, and confuse stuff completely. > > > > > > > > Also it feels a bit wrong to put this onto the driver, when really it's a > > > > cursor flag. I guess you can make it some kind of flag in the drm_plane > > > > structure, or a new plane type, but putting it there instead of into the > > > > "random pile of midlayer-mistake driver flags" would be a lot better. > > > > > > > > Otherwise I think the series looks roughly how I'd expect it to look. > > > > -Daniel > > > > > > > > > > AFAICT this is the only remaining thing to be addressed for this series ? > > > > No, there was more. tbh I haven't had the time to think about whether the above > > makes sense to me, e.g. I'm not sure if having virtualized drivers expose > > "support > > universal planes" and adding another plane which is not universal (the only > > "universal" plane on them being the default one) makes more sense than a flag > > that > > says "this driver requires a cursor in the cursor plane". There's certainly a > > huge > > difference in how userspace would be required to handle it and it's way uglier > > with > > two different cursor planes. i.e. there's a lot of ways in which this could be > > cleaner in the kernel but they all require significant changes to userspace, > > that go > > way beyond "attach hotspot info to this plane". I'd like to avoid approaches > > that > > mean running with atomic kms requires completely separate paths for virtualized > > drivers because no one will ever support and maintain it. > > > > It's not a trivial thing because it's fundamentally hard to untangle the fact > > the > > virtualized drivers have been advertising universal plane support without ever > > supporting universal planes. Especially because most new userspace in general > > checks > > for "universal planes" to expose atomic kms paths. > > > > After some discussion on the #dri-devel, your approach makes sense and the > only contention point is the name of the driver feature flag name. The one > you are using (DRIVER_VIRTUAL) seems to be too broad and generic (the fact > that vkms won't set and is a virtual driver as well, is a good example). > > Maybe something like DRIVER_CURSOR_HOTSPOT or DRIVER_CURSOR_COMMANDEERING > would be more accurate and self explanatory ? Sure, or even more verbose DRIVER_NEEDS_CURSOR_PLANE_HOTSPOT, but it sounds like Pekka doesn't agree with this approach. As I mentioned in my response to him, I'd be happy with any approach that gets paravirtualized drivers working with atomic kms, but atm I don't have enough time to be creating a new kernel subsystem or a new set of uapi's for paravirtualized drivers and then porting mutter/kwin to those. z