[PATCH v2 1/8] drm: Disable the cursor plane on atomic contexts with virtualized drivers
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)") Cc: stable@vger.kernel.org # v5.4+ Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: Dave Airlie airlied@redhat.com Cc: Gerd Hoffmann kraxel@redhat.com Cc: Hans de Goede hdegoede@redhat.com Cc: Gurchetan Singh gurchetansingh@chromium.org Cc: Chia-I Wu olvaffe@gmail.com Cc: dri-devel@lists.freedesktop.org Cc: virtualization@lists.linux-foundation.org Cc: spice-devel@lists.freedesktop.org --- drivers/gpu/drm/drm_plane.c | 11 +++++++++++ drivers/gpu/drm/qxl/qxl_drv.c | 2 +- drivers/gpu/drm/vboxvideo/vbox_drv.c | 2 +- drivers/gpu/drm/virtio/virtgpu_drv.c | 3 ++- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 2 +- include/drm/drm_drv.h | 10 ++++++++++ include/drm/drm_file.h | 12 ++++++++++++ 7 files changed, 38 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 726f2f163c26..e1e2a65c7119 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -667,6 +667,17 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data, !file_priv->universal_planes) continue;
+ /* + * Unless userspace supports virtual cursor plane + * then if we're running on virtual driver do not + * advertise cursor planes because they'll be broken + */ + if (plane->type == DRM_PLANE_TYPE_CURSOR && + drm_core_check_feature(dev, DRIVER_VIRTUAL) && + file_priv->atomic && + !file_priv->supports_virtual_cursor_plane) + continue; + if (drm_lease_held(file_priv, plane->base.id)) { if (count < plane_resp->count_planes && put_user(plane->base.id, plane_ptr + count)) diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c index 1cb6f0c224bb..0e4212e05caa 100644 --- a/drivers/gpu/drm/qxl/qxl_drv.c +++ b/drivers/gpu/drm/qxl/qxl_drv.c @@ -281,7 +281,7 @@ static const struct drm_ioctl_desc qxl_ioctls[] = { };
static struct drm_driver qxl_driver = { - .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, + .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_VIRTUAL,
.dumb_create = qxl_mode_dumb_create, .dumb_map_offset = drm_gem_ttm_dumb_map_offset, diff --git a/drivers/gpu/drm/vboxvideo/vbox_drv.c b/drivers/gpu/drm/vboxvideo/vbox_drv.c index f4f2bd79a7cb..84e75bcc3384 100644 --- a/drivers/gpu/drm/vboxvideo/vbox_drv.c +++ b/drivers/gpu/drm/vboxvideo/vbox_drv.c @@ -176,7 +176,7 @@ DEFINE_DRM_GEM_FOPS(vbox_fops);
static const struct drm_driver driver = { .driver_features = - DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC, + DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC | DRIVER_VIRTUAL,
.lastclose = drm_fb_helper_lastclose,
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c index 5f25a8d15464..3c5bb006159a 100644 --- a/drivers/gpu/drm/virtio/virtgpu_drv.c +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c @@ -198,7 +198,8 @@ MODULE_AUTHOR("Alon Levy"); DEFINE_DRM_GEM_FOPS(virtio_gpu_driver_fops);
static const struct drm_driver driver = { - .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER | DRIVER_ATOMIC, + .driver_features = + DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER | DRIVER_ATOMIC | DRIVER_VIRTUAL, .open = virtio_gpu_driver_open, .postclose = virtio_gpu_driver_postclose,
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c index 01a5b47e95f9..712f6ad0b014 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c @@ -1581,7 +1581,7 @@ static const struct file_operations vmwgfx_driver_fops = {
static const struct drm_driver driver = { .driver_features = - DRIVER_MODESET | DRIVER_RENDER | DRIVER_ATOMIC | DRIVER_GEM, + DRIVER_MODESET | DRIVER_RENDER | DRIVER_ATOMIC | DRIVER_GEM | DRIVER_VIRTUAL, .ioctls = vmw_ioctls, .num_ioctls = ARRAY_SIZE(vmw_ioctls), .master_set = vmw_master_set, 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),
/* IMPORTANT: Below are all the legacy flags, add new ones above. */
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h index e0a73a1e2df7..3e5c36891161 100644 --- a/include/drm/drm_file.h +++ b/include/drm/drm_file.h @@ -223,6 +223,18 @@ struct drm_file { */ bool is_master;
+ /** + * @supports_virtual_cursor_plane: + * + * This client is capable of handling the cursor plane with the + * restrictions imposed on it by the virtualized drivers. + * + * The implies that the cursor plane has to behave like a cursor + * i.e. track cursor movement. It also requires setting of the + * hotspot properties by the client on the cursor plane. + */ + bool supports_virtual_cursor_plane; + /** * @master: *
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)") Cc: stable@vger.kernel.org # v5.4+ Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: Dave Airlie airlied@redhat.com Cc: Gerd Hoffmann kraxel@redhat.com Cc: Hans de Goede hdegoede@redhat.com Cc: Gurchetan Singh gurchetansingh@chromium.org Cc: Chia-I Wu olvaffe@gmail.com Cc: dri-devel@lists.freedesktop.org Cc: virtualization@lists.linux-foundation.org Cc: spice-devel@lists.freedesktop.org
drivers/gpu/drm/drm_plane.c | 11 +++++++++++ drivers/gpu/drm/qxl/qxl_drv.c | 2 +- drivers/gpu/drm/vboxvideo/vbox_drv.c | 2 +- drivers/gpu/drm/virtio/virtgpu_drv.c | 3 ++- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 2 +- include/drm/drm_drv.h | 10 ++++++++++ include/drm/drm_file.h | 12 ++++++++++++ 7 files changed, 38 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 726f2f163c26..e1e2a65c7119 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -667,6 +667,17 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data, !file_priv->universal_planes) continue;
/** Unless userspace supports virtual cursor plane* then if we're running on virtual driver do not* advertise cursor planes because they'll be broken*/if (plane->type == DRM_PLANE_TYPE_CURSOR &&drm_core_check_feature(dev, DRIVER_VIRTUAL) &&file_priv->atomic &&!file_priv->supports_virtual_cursor_plane)continue;- if (drm_lease_held(file_priv, plane->base.id)) { if (count < plane_resp->count_planes && put_user(plane->base.id, plane_ptr + count))
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c index 1cb6f0c224bb..0e4212e05caa 100644 --- a/drivers/gpu/drm/qxl/qxl_drv.c +++ b/drivers/gpu/drm/qxl/qxl_drv.c @@ -281,7 +281,7 @@ static const struct drm_ioctl_desc qxl_ioctls[] = { }; static struct drm_driver qxl_driver = {
- .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
- .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_VIRTUAL,
.dumb_create = qxl_mode_dumb_create, .dumb_map_offset = drm_gem_ttm_dumb_map_offset, diff --git a/drivers/gpu/drm/vboxvideo/vbox_drv.c b/drivers/gpu/drm/vboxvideo/vbox_drv.c index f4f2bd79a7cb..84e75bcc3384 100644 --- a/drivers/gpu/drm/vboxvideo/vbox_drv.c +++ b/drivers/gpu/drm/vboxvideo/vbox_drv.c @@ -176,7 +176,7 @@ DEFINE_DRM_GEM_FOPS(vbox_fops); static const struct drm_driver driver = { .driver_features =
DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC | DRIVER_VIRTUAL,.lastclose = drm_fb_helper_lastclose, diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c index 5f25a8d15464..3c5bb006159a 100644 --- a/drivers/gpu/drm/virtio/virtgpu_drv.c +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c @@ -198,7 +198,8 @@ MODULE_AUTHOR("Alon Levy"); DEFINE_DRM_GEM_FOPS(virtio_gpu_driver_fops); static const struct drm_driver driver = {
- .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER | DRIVER_ATOMIC,
- .driver_features =
DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER | DRIVER_ATOMIC | DRIVER_VIRTUAL,diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c index 01a5b47e95f9..712f6ad0b014 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c @@ -1581,7 +1581,7 @@ static const struct file_operations vmwgfx_driver_fops = { static const struct drm_driver driver = { .driver_features =
- DRIVER_MODESET | DRIVER_RENDER | DRIVER_ATOMIC | DRIVER_GEM,
- DRIVER_MODESET | DRIVER_RENDER | DRIVER_ATOMIC | DRIVER_GEM | DRIVER_VIRTUAL, .ioctls = vmw_ioctls, .num_ioctls = ARRAY_SIZE(vmw_ioctls), .master_set = vmw_master_set,
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
/* IMPORTANT: Below are all the legacy flags, add new ones above. */ diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h index e0a73a1e2df7..3e5c36891161 100644 --- a/include/drm/drm_file.h +++ b/include/drm/drm_file.h @@ -223,6 +223,18 @@ struct drm_file { */ bool is_master;
- /**
* @supports_virtual_cursor_plane:** This client is capable of handling the cursor plane with the* restrictions imposed on it by the virtualized drivers.** The implies that the cursor plane has to behave like a cursor* i.e. track cursor movement. It also requires setting of the* hotspot properties by the client on the cursor plane.*/- bool supports_virtual_cursor_plane;
- /**
- @master:
-- 2.34.1
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 ?
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.
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
Zack Rusin zackr@vmware.com writes:
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.
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 ?
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.
I understand that every new feature needs tests but I fail to see why the bar is higher for this feature than others? I would prefer if this series are not blocked due some potential issues on hypothetical corner cases that might not happen in practice. Or do people really run two or more compositors on different console and switch between them ?
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 ;)
Agreed. And I believe that other than the driver cap name, everyone agrees with the color of your bikeshed :)
On Wed, 03 May 2023 09:48:54 +0200 Javier Martinez Canillas javierm@redhat.com wrote:
Zack Rusin zackr@vmware.com writes:
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.
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 ?
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.
I understand that every new feature needs tests but I fail to see why the bar is higher for this feature than others? I would prefer if this series are not blocked due some potential issues on hypothetical corner cases that might not happen in practice. Or do people really run two or more compositors on different console and switch between them ?
I have no recollection at all about what was talked about this, but in certain virtualized cases you will always have two display systems simultaneously: - the guest display system which uses the nested KMS driver in the guest VM, which presents to - a VM viewer application on the host, which presents via Wayland to - the host display system which uses a hardware KMS driver.
Maybe it was more like that to test?
Thanks, pq
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 ;)
Agreed. And I believe that other than the driver cap name, everyone agrees with the color of your bikeshed :)
On Wed, 2023-05-03 at 09:48 +0200, Javier Martinez Canillas wrote:
Zack Rusin zackr@vmware.com writes:
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.
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
On Thu, 4 May 2023 01:50:25 +0000 Zack Rusin zackr@vmware.com wrote:
On Wed, 2023-05-03 at 09:48 +0200, Javier Martinez Canillas wrote:
Zack Rusin zackr@vmware.com writes:
On Tue, 2023-05-02 at 11:32 +0200, Javier Martinez Canillas wrote:
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.
It seems I have not been clear enough, apologies. Once more, in short:
Zack, I'm worried about this statement from you (copied from above):
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 feels like you are intentionally limiting your own design options for the fear of "no one will ever support it". I'm worried that over the coming years, that will lead to a hard to use, hard to maintain patchwork of vague or undocumented or just too many little UAPI details.
Please, don't limit your designs. There are good reasons why nested KMS drivers behave fundamentally differently to most KMS hardware drivers. Userspace that does not or cannot take that into account is unavoidably crippled.
Thanks, pq
On Thu, May 04, 2023 at 01:39:04PM +0300, Pekka Paalanen wrote:
On Thu, 4 May 2023 01:50:25 +0000 Zack Rusin zackr@vmware.com wrote:
On Wed, 2023-05-03 at 09:48 +0200, Javier Martinez Canillas wrote:
Zack Rusin zackr@vmware.com writes:
On Tue, 2023-05-02 at 11:32 +0200, Javier Martinez Canillas wrote:
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.
It seems I have not been clear enough, apologies. Once more, in short:
Zack, I'm worried about this statement from you (copied from above):
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 feels like you are intentionally limiting your own design options for the fear of "no one will ever support it". I'm worried that over the coming years, that will lead to a hard to use, hard to maintain patchwork of vague or undocumented or just too many little UAPI details.
Please, don't limit your designs. There are good reasons why nested KMS drivers behave fundamentally differently to most KMS hardware drivers. Userspace that does not or cannot take that into account is unavoidably crippled.
From a compositor side, there is a valid reason to minimize the uAPI difference between "nested virtual machine" code paths and "running on actual hardware" code paths, which is to let virtual machines with a viewer connected to KMS act as a testing environment, rather than a production environment. Running a production environment in a virtual machine doesn't really need to use KMS at all.
When using virtual machines for testing, I want to minimize the amount of differentation between running on hardware and running in the VM because otherwise the parts that are tested are not the same.
I realize that hotpspots and the cursor moving viewer side contradicts that to some degree, but still, from a graphical testing witha VM point of view, one has to compromise, as testing isn't just for the KMS layer, but for the DE and distribution as a whole.
Jonas
Thanks, pq
On Thu, 4 May 2023 13:27:22 +0200 Jonas Ådahl jadahl@gmail.com wrote:
On Thu, May 04, 2023 at 01:39:04PM +0300, Pekka Paalanen wrote:
On Thu, 4 May 2023 01:50:25 +0000 Zack Rusin zackr@vmware.com wrote:
On Wed, 2023-05-03 at 09:48 +0200, Javier Martinez Canillas wrote:
Zack Rusin zackr@vmware.com writes:
On Tue, 2023-05-02 at 11:32 +0200, Javier Martinez Canillas wrote:
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.
It seems I have not been clear enough, apologies. Once more, in short:
Zack, I'm worried about this statement from you (copied from above):
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 feels like you are intentionally limiting your own design options for the fear of "no one will ever support it". I'm worried that over the coming years, that will lead to a hard to use, hard to maintain patchwork of vague or undocumented or just too many little UAPI details.
Please, don't limit your designs. There are good reasons why nested KMS drivers behave fundamentally differently to most KMS hardware drivers. Userspace that does not or cannot take that into account is unavoidably crippled.
From a compositor side, there is a valid reason to minimize the uAPI difference between "nested virtual machine" code paths and "running on actual hardware" code paths, which is to let virtual machines with a viewer connected to KMS act as a testing environment, rather than a production environment. Running a production environment in a virtual machine doesn't really need to use KMS at all.
When using virtual machines for testing, I want to minimize the amount of differentation between running on hardware and running in the VM because otherwise the parts that are tested are not the same.
I realize that hotpspots and the cursor moving viewer side contradicts that to some degree, but still, from a graphical testing witha VM point of view, one has to compromise, as testing isn't just for the KMS layer, but for the DE and distribution as a whole.
Right, I'm looking at this from the production use only point of view, and not as any kind of testing environment, not for compositor KMS driving bits at least. Using a virtualized driver for KMS testing seems so very... manual to me, and like you said, it's not representative of "real" behaviour.
As for the best choice for production use, KMS in guest OS is attractive because it offers zero-copy direct scanout to the host hardware, with the right stack. OTOH, I think RDP has extensions that could enable that too, and if the end point is not host hardware display then KMS use in guest is not the best idea indeed.
I don't recall any mention of actual use cases here recently. I agree the intended use makes a huge difference. Testing KMS userspace and production use are almost the opposite goals for virtualized drivers.
Thanks, pq
On Wed, 3 May 2023 03:35:29 +0000 Zack Rusin zackr@vmware.com wrote:
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.
Hi Zack,
you'd like to avoid that, but fundamentally that really is what has to happen in userspace for *nested* KMS drivers (VKMS is a virtual driver but not part of the interest group here) to reach optimality.
It really is a different path. I see no way around that. But if you accept that fact, then you could possibly gain a lot more benefits by asking userspace to handle nested KMS drivers differently. What those benefits are exactly I'm not sure, but I have a feeling there should be some, where the knowledge of running on a nested KMS driver allows for better decisions that are not possible if the nested KMS driver just pretends to be like any other KMS hardware driver.
You can get up to some level of interoperability by pretending to be just like any other KMS driver, but if you want to optimize things, I feel that's a whole different story. It's a trade-off.
I think frame timing is one thing. A nested KMS driver increases the depth of the "swapchain" between the guest KMS app and the actual hardware. This is unexpected if userspace does not know it is running on a nested KMS driver.
The existing KMS uAPI, both legacy and atomic, have been written for classic hardware. One fundamental result of that is the page flip completion event, it signals two things simultaneously: the new framebuffer is in use, and a new flip can be programmed. On a nested driver, these two are not the same thing: the nested driver can take another flip before the new framebuffer is actually being used in the host. More importantly, the nested driver can take a new flip before the old replaced framebuffer has actually been retired.
(The above can tie into the question of making the KMS swapchain deeper in general, also for classic scanout design, in connection to present-not-before-timestamp queueing at KMS level.)
However, as long as these two are the same event, it can decimate the framerate on a nested driver, because userspace is not prepared for a swapchain depth of greater than one. Or, the nested KMS driver gives up on zero-copy. Or, you need a fragile timing arrangement that essentially needs to be hand-configured in at least one display system, guest or host.
Somewhat related, there is also the matter of KMS drivers (hardware, nested, and virtual) that do not lock their page flip events to a hardware scanout cycle (because there is none) but "complete" any flip immediately. That too requires explicit handling in userspace, because you simply do not have a scanout cycle to lock on to.
We already have an example where userspace is explicitly helping "unusual" KMS drivers: FB_DAMAGE_CLIPS. While educating userspace does take considerable effort, I'd like to believe it is doable, and it is also necessary for optimality. Excellent KMS documentation is key, naturally.
Of course, it is up to you and other people to decide to want to do the work or not. I just feel you could potentially gain a lot if you decide to take on that fight.
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.
That's not just userspace, it's built into the kernel UAPI design that you cannot have atomic without universal planes.
Thanks, pq
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
On Wed, 2023-05-03 at 10:54 +0300, Pekka Paalanen wrote:
On Wed, 3 May 2023 03:35:29 +0000 Zack Rusin zackr@vmware.com wrote:
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.
Hi Zack,
you'd like to avoid that, but fundamentally that really is what has to happen in userspace for *nested* KMS drivers (VKMS is a virtual driver but not part of the interest group here) to reach optimality.
It really is a different path. I see no way around that. But if you accept that fact, then you could possibly gain a lot more benefits by asking userspace to handle nested KMS drivers differently. What those benefits are exactly I'm not sure, but I have a feeling there should be some, where the knowledge of running on a nested KMS driver allows for better decisions that are not possible if the nested KMS driver just pretends to be like any other KMS hardware driver.
I'm not quite sure I buy the "virtualized drivers return immediately from a flip and require two extra integers on the cursor plane, so there's no possible way drm api could handle that" argument because it seems rather flimsy. If the premise is that the paravirtualized drivers are so different that drm uapi can not handle them then why would they stay in drm? What's the benefit if they'll have their own uapi and their own interfaces?
I'd flip the argument and say you'd be a lot happier if you accepted that universal planes aren't really universal no matter what. If weston puts a spreadsheet app in a cursor plane presumably users would be concerned that their mouse cursor disappears when they enter the spreadsheet app and responding to their concerns with "it's cool, the planes are universal" wouldn't quite work. Something needs to special case cursor plane no matter what. Anyway, I think we went through this argument of what exactly "universal" refers to and whatever the definition of it is why I don't see why two extra integers on cursor planes violates any part of it so I'll let it go.
I have nothing against any of those solutions - from creating whole new kms uapi for paravirtualized drivers, through creating a new subsystem for paravirtualized drivers. I just have no interest in implementing those myself right now but if someone implemented mutter/kwin code on top of either a new subsystem for paravirtualized drivers or implemented atomic kms on top of some new api's for them, we'll be sure to port vmwgfx to that.
z
On Thu, 4 May 2023 01:43:51 +0000 Zack Rusin zackr@vmware.com wrote:
On Wed, 2023-05-03 at 10:54 +0300, Pekka Paalanen wrote:
On Wed, 3 May 2023 03:35:29 +0000 Zack Rusin zackr@vmware.com wrote:
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.
Hi Zack,
you'd like to avoid that, but fundamentally that really is what has to happen in userspace for *nested* KMS drivers (VKMS is a virtual driver but not part of the interest group here) to reach optimality.
It really is a different path. I see no way around that. But if you accept that fact, then you could possibly gain a lot more benefits by asking userspace to handle nested KMS drivers differently. What those benefits are exactly I'm not sure, but I have a feeling there should be some, where the knowledge of running on a nested KMS driver allows for better decisions that are not possible if the nested KMS driver just pretends to be like any other KMS hardware driver.
I'm not quite sure I buy the "virtualized drivers return immediately from a flip and require two extra integers on the cursor plane, so there's no possible way drm api could handle that" argument because it seems rather flimsy. If the premise is that the paravirtualized drivers are so different that drm uapi can not handle them then why would they stay in drm? What's the benefit if they'll have their own uapi and their own interfaces?
Hi Zack,
I never implied to go that far as you make it sound here.
I only pointed out that there definitely are behavioral differences that userspace MUST acknowledge and handle. The cursor plane being special is just the start.
It does not invalidate the whole existing KMS UAPI, but *if* you aim for optimal performance, you cannot ignore the differences or paper over them either.
This NOT a NAK to this patch series in any way!
I'd flip the argument and say you'd be a lot happier if you accepted that universal planes aren't really universal no matter what. If weston puts a spreadsheet app in a cursor plane presumably users would be concerned that their mouse cursor disappears when they enter the spreadsheet app and responding to their concerns with "it's cool, the planes are universal" wouldn't quite work. Something needs to special case cursor plane no matter what. Anyway, I think we went through this argument of what exactly "universal" refers to and whatever the definition of it is why I don't see why two extra integers on cursor planes violates any part of it so I'll let it go.
If you say so.
But then you need userspace to set those two integers. The concept of those two integers does not even exist without explicit care in userspace. You are already letting go of your goal to not need special case code or changes in userspace like you said in (copied from above):
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.
You want to make cursor planes special and not universal, which means userspace needs special code to use them at all. That's a separate code path. That is good! That is the way to get more performance through better userspace decisions.
I have nothing against any of those solutions - from creating whole new kms uapi for paravirtualized drivers, through creating a new subsystem for paravirtualized drivers. I just have no interest in implementing those myself right now but if someone implemented mutter/kwin code on top of either a new subsystem for paravirtualized drivers or implemented atomic kms on top of some new api's for them, we'll be sure to port vmwgfx to that.
Again, I said half a meter, but you are jumping a mile.
I think you are seeing my comments as NAKs towards everything you try to do. That's not my intention, and I cannot NAK anything anyway. I am only pointing out that you seem too fixed to these details to see the rest of the problems that hinder performance of nested KMS drivers.
See for example https://gitlab.freedesktop.org/wayland/weston/-/issues/514
It's a nested KMS use case with qemu (would vmwgfx be any different?), where the goal is to have zero-copy direct scanout from guest application through guest KMS planes, qemu, host Weston onto host KMS planes. The direct scanout works, but due to the UAPI design I explained in my previous email, the only way to make that actually run at ~full framerate is to very carefully manually tune the timings of both guest and host Weston instance.
If you deem the cost of gaining that performance too high, that's fine.
However, I do absolutely believe, that if you wanted it, any Wayland compositor project that aims to be in any way a performant general purpose compositor for desktops or more would be willing to follow you in using new UAPI that would make life better with nested KMS drivers. You just have to ask, instead of assume that userspace won't hear you. After all, working better is in their interest as well.
I *am* trying to push you forward, not stop you.
Sorry, I'll try to keep quiet from now on since I don't have any stakes here.
Thanks, pq
linux-stable-mirror@lists.linaro.org
-
Daniel Vetter -
Javier Martinez Canillas -
Jonas Ådahl -
Pekka Paalanen -
Zack Rusin -
Zack Rusin