From: Zack Rusin <zackr@vmware.com> To: "daniel@ffwll.ch" <daniel@ffwll.ch>, "javierm@redhat.com" <javierm@redhat.com> Cc: Maaz Mombasawala <mombasawalam@vmware.com>, "airlied@redhat.com" <airlied@redhat.com>, "airlied@linux.ie" <airlied@linux.ie>, "tzimmermann@suse.de" <tzimmermann@suse.de>, "gurchetansingh@chromium.org" <gurchetansingh@chromium.org>, "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>, "kraxel@redhat.com" <kraxel@redhat.com>, Martin Krastev <krastevm@vmware.com>, "hdegoede@redhat.com" <hdegoede@redhat.com>, "belmouss@redhat.com" <belmouss@redhat.com>, "spice-devel@lists.freedesktop.org" <spice-devel@lists.freedesktop.org>, "stable@vger.kernel.org" <stable@vger.kernel.org>, "ppaalanen@gmail.com" <ppaalanen@gmail.com>, "virtualization@lists.linux-foundation.org" <virtualization@lists.linux-foundation.org>, Linux-graphics-maintainer <Linux-graphics-maintainer@vmware.com> Subject: Re: [PATCH v2 1/8] drm: Disable the cursor plane on atomic contexts with virtualized drivers Date: Wed, 3 May 2023 03:35:29 +0000 [thread overview] Message-ID: <0dd2fa763aa0e659c8cbae94f283d8101450082a.camel@vmware.com> (raw) In-Reply-To: <87lei7xemy.fsf@minerva.mail-host-address-is-not-set> On Tue, 2023-05-02 at 11:32 +0200, Javier Martinez Canillas wrote: > !! External Email > > Daniel Vetter <daniel@ffwll.ch> writes: > > > On Mon, Jul 11, 2022 at 11:32:39PM -0400, Zack Rusin wrote: > > > From: Zack Rusin <zackr@vmware.com> > > > > > > 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@vmware.com> > > > 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. The other thing blocking this series was the testing of all the edge cases, I think Simon and Pekka had some ideas for things to test (e.g. run mutter with support for this and wayland without support for this in at the same time in different consoles and see what happens). I never had the time to do that either. > Zack, are you planning to re-spin a v3 of this patch-set? Asking because > we want to take virtio-gpu out of the atomic KMS deny list in mutter, but > first need this to land. > > If you think that won't be able to do it in the short term, Bilal (Cc'ed) > or me would be glad to help with that. This has been on my todo for a while I just never had the time to go through all the remaining issues. Fundamentally it's not so much a technical issue anymore, it's about picking the least broken solution and trying to make the best out of a pretty bad situation. In general it's hard to paint a bikeshed if all you have is a million shades of gray ;) z
WARNING: multiple messages have this Message-ID (diff)
From: Zack Rusin <zackr@vmware.com> To: "daniel@ffwll.ch" <daniel@ffwll.ch>, "javierm@redhat.com" <javierm@redhat.com> Cc: "hdegoede@redhat.com" <hdegoede@redhat.com>, "airlied@linux.ie" <airlied@linux.ie>, "belmouss@redhat.com" <belmouss@redhat.com>, "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>, "gurchetansingh@chromium.org" <gurchetansingh@chromium.org>, Martin Krastev <krastevm@vmware.com>, "ppaalanen@gmail.com" <ppaalanen@gmail.com>, Linux-graphics-maintainer <Linux-graphics-maintainer@vmware.com>, "kraxel@redhat.com" <kraxel@redhat.com>, "tzimmermann@suse.de" <tzimmermann@suse.de>, "spice-devel@lists.freedesktop.org" <spice-devel@lists.freedesktop.org>, "airlied@redhat.com" <airlied@redhat.com>, "stable@vger.kernel.org" <stable@vger.kernel.org>, "virtualization@lists.linux-foundation.org" <virtualization@lists.linux-foundation.org>, Maaz Mombasawala <mombasawalam@vmware.com> Subject: Re: [PATCH v2 1/8] drm: Disable the cursor plane on atomic contexts with virtualized drivers Date: Wed, 3 May 2023 03:35:29 +0000 [thread overview] Message-ID: <0dd2fa763aa0e659c8cbae94f283d8101450082a.camel@vmware.com> (raw) In-Reply-To: <87lei7xemy.fsf@minerva.mail-host-address-is-not-set> On Tue, 2023-05-02 at 11:32 +0200, Javier Martinez Canillas wrote: > !! External Email > > Daniel Vetter <daniel@ffwll.ch> writes: > > > On Mon, Jul 11, 2022 at 11:32:39PM -0400, Zack Rusin wrote: > > > From: Zack Rusin <zackr@vmware.com> > > > > > > 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@vmware.com> > > > 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. The other thing blocking this series was the testing of all the edge cases, I think Simon and Pekka had some ideas for things to test (e.g. run mutter with support for this and wayland without support for this in at the same time in different consoles and see what happens). I never had the time to do that either. > Zack, are you planning to re-spin a v3 of this patch-set? Asking because > we want to take virtio-gpu out of the atomic KMS deny list in mutter, but > first need this to land. > > If you think that won't be able to do it in the short term, Bilal (Cc'ed) > or me would be glad to help with that. This has been on my todo for a while I just never had the time to go through all the remaining issues. Fundamentally it's not so much a technical issue anymore, it's about picking the least broken solution and trying to make the best out of a pretty bad situation. In general it's hard to paint a bikeshed if all you have is a million shades of gray ;) z
next prev parent reply other threads:[~2023-05-03 3:35 UTC|newest] Thread overview: 55+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-07-12 3:32 [PATCH v2 0/8] Fix cursor planes with virtualized drivers Zack Rusin 2022-07-12 3:32 ` [PATCH v2 1/8] drm: Disable the cursor plane on atomic contexts " Zack Rusin 2022-07-12 3:32 ` Zack Rusin 2022-08-10 16:40 ` Daniel Vetter 2022-08-10 16:40 ` Daniel Vetter 2022-08-10 16:40 ` Daniel Vetter 2023-05-02 9:32 ` Javier Martinez Canillas 2023-05-02 9:32 ` Javier Martinez Canillas 2023-05-03 3:35 ` Zack Rusin [this message] 2023-05-03 3:35 ` Zack Rusin 2023-05-03 7:48 ` Javier Martinez Canillas 2023-05-03 7:48 ` Javier Martinez Canillas 2023-05-03 8:01 ` Pekka Paalanen 2023-05-03 8:01 ` Pekka Paalanen 2023-05-03 8:01 ` Pekka Paalanen 2023-05-04 1:50 ` Zack Rusin 2023-05-04 1:50 ` Zack Rusin 2023-05-04 10:39 ` Pekka Paalanen 2023-05-04 10:39 ` Pekka Paalanen 2023-05-04 10:39 ` Pekka Paalanen 2023-05-04 11:27 ` Jonas Ådahl 2023-05-04 11:27 ` Jonas Ådahl 2023-05-04 12:13 ` Pekka Paalanen 2023-05-04 12:13 ` Pekka Paalanen 2023-05-04 12:13 ` Pekka Paalanen 2023-05-03 7:54 ` Pekka Paalanen 2023-05-03 7:54 ` Pekka Paalanen 2023-05-03 7:54 ` Pekka Paalanen 2023-05-04 1:43 ` Zack Rusin 2023-05-04 1:43 ` Zack Rusin 2023-05-04 8:21 ` Pekka Paalanen 2023-05-04 8:21 ` Pekka Paalanen 2023-05-04 8:21 ` Pekka Paalanen 2022-07-12 3:32 ` [PATCH v2 2/8] drm/atomic: Add support for mouse hotspots Zack Rusin 2023-05-03 7:59 ` Daniel Vetter 2022-07-12 3:32 ` [PATCH v2 3/8] drm/vmwgfx: Use the hotspot properties from cursor planes Zack Rusin 2022-07-25 13:35 ` Martin Krastev (VMware) 2022-07-12 3:32 ` [PATCH v2 4/8] drm/qxl: " Zack Rusin 2022-07-12 7:58 ` Gerd Hoffmann 2022-07-12 7:58 ` Gerd Hoffmann 2022-07-12 3:32 ` [PATCH v2 5/8] drm/vboxvideo: " Zack Rusin 2022-07-12 7:56 ` Pekka Paalanen 2022-07-13 3:35 ` Zack Rusin 2022-07-13 7:20 ` Pekka Paalanen 2022-07-13 13:35 ` Zack Rusin 2022-07-14 7:38 ` Pekka Paalanen 2022-07-12 3:32 ` [PATCH v2 6/8] drm/virtio: " Zack Rusin 2022-07-12 7:58 ` Gerd Hoffmann 2022-07-12 7:58 ` Gerd Hoffmann 2022-07-12 3:32 ` [PATCH v2 7/8] drm: Remove legacy cursor hotspot code Zack Rusin 2022-07-12 3:32 ` [PATCH v2 8/8] drm: Introduce DRM_CLIENT_CAP_SUPPORTS_VIRTUAL_CURSOR_PLANE Zack Rusin 2022-07-12 7:57 ` Simon Ser 2022-07-12 8:01 ` Pekka Paalanen 2022-07-12 7:54 ` [PATCH v2 0/8] Fix cursor planes with virtualized drivers Pekka Paalanen 2022-07-12 8:00 ` Simon Ser
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=0dd2fa763aa0e659c8cbae94f283d8101450082a.camel@vmware.com \ --to=zackr@vmware.com \ --cc=Linux-graphics-maintainer@vmware.com \ --cc=airlied@linux.ie \ --cc=airlied@redhat.com \ --cc=belmouss@redhat.com \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=gurchetansingh@chromium.org \ --cc=hdegoede@redhat.com \ --cc=javierm@redhat.com \ --cc=krastevm@vmware.com \ --cc=kraxel@redhat.com \ --cc=mombasawalam@vmware.com \ --cc=ppaalanen@gmail.com \ --cc=spice-devel@lists.freedesktop.org \ --cc=stable@vger.kernel.org \ --cc=tzimmermann@suse.de \ --cc=virtualization@lists.linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.