From: Zack Rusin zackr@vmware.com
Support for setting mouse cursor hotspot never made the transition from the legacy kms to atomic. This left virtualized drivers, all which are atomic, in a weird spot because all userspace compositors put those drivers on deny-lists for atomic kms due to the fact that mouse clicks were incorrectly routed, e.g: https://gitlab.gnome.org/GNOME/mutter/-/blob/main/src/backends/native/meta-k... https://invent.kde.org/plasma/kwin/-/blob/master/src/backends/drm/drm_gpu.cp...
So even though all the virtualized drm drivers are atomic, none of them could be used with atomic kms because of the missing mouse cursor hotspot support.
The first change adds support for mouse cursor hotspots to drm core atomic via the HOTSPOT_X and HOTSPOT_Y properties and implements it in the legacy paths. The next few changes add support for the mouse hotspot properties to all the drivers which required hotspots. And the final change removes the legacy hotspot code because it's unused.
A sample mutter patch which makes gnome-shell work with all the virtualized drivers with atomic kms is here: https://gitlab.gnome.org/zackr/mutter/-/commit/2aa61b50507a24f34d514fa65b7bc... I'll have a similar patch for kwin.
This gets virtualized drivers working correctly with atomic kms, but the hotspot codepaths aren't fool proof, e.g.: - there's no way of currently validating that the drm drivers actually did anything with the information and no way of testing it via igt, - all userspace code needs to hardcore a list of drivers which require hotspots because there's no way to query from drm "does this driver require hotspot"
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: Gerd Hoffmann kraxel@redhat.com Cc: Gurchetan Singh gurchetansingh@chromium.org Cc: Chia-I Wu olvaffe@gmail.com Cc: Hans de Goede hdegoede@redhat.com
Zack Rusin (6): drm/atomic: Add support for mouse hotspots drm/vmwgfx: Create mouse hotspot properties on cursor planes drm/qxl: Create mouse hotspot properties on cursor planes drm/vboxvideo: Create mouse hotspot properties on cursor planes drm/virtio: Create mouse hotspot properties on cursor planes drm: Remove legacy cursor hotspot code
drivers/gpu/drm/drm_atomic_state_helper.c | 14 ++++++++ drivers/gpu/drm/drm_atomic_uapi.c | 20 +++++++++++ drivers/gpu/drm/drm_plane.c | 44 +++++++++++++++++++++-- drivers/gpu/drm/qxl/qxl_display.c | 13 +++---- drivers/gpu/drm/vboxvideo/vbox_mode.c | 5 +-- drivers/gpu/drm/virtio/virtgpu_display.c | 1 + drivers/gpu/drm/virtio/virtgpu_plane.c | 8 ++--- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 11 ++---- drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 2 ++ drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 1 + drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 2 ++ include/drm/drm_framebuffer.h | 12 ------- include/drm/drm_plane.h | 15 ++++++++ 13 files changed, 113 insertions(+), 35 deletions(-)
From: Zack Rusin zackr@vmware.com
Atomic modesetting code lacked support for specifying mouse cursor hotspots. The legacy kms DRM_IOCTL_MODE_CURSOR2 had support for setting the hotspot but the functionality was not implemented in the new atomic paths.
Due to the lack of hotspots in the atomic paths userspace compositors completely disable atomic modesetting for drivers that require it (i.e. all paravirtualized drivers).
This change adds hotspot properties to the atomic codepaths throughtout the DRM core and will allow enabling atomic modesetting for virtualized drivers in the userspace.
Signed-off-by: Zack Rusin zackr@vmware.com 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 --- drivers/gpu/drm/drm_atomic_state_helper.c | 14 ++++++++ drivers/gpu/drm/drm_atomic_uapi.c | 20 +++++++++++ drivers/gpu/drm/drm_plane.c | 43 +++++++++++++++++++++++ include/drm/drm_plane.h | 15 ++++++++ 4 files changed, 92 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c index 3b6d3bdbd099..bc8735998099 100644 --- a/drivers/gpu/drm/drm_atomic_state_helper.c +++ b/drivers/gpu/drm/drm_atomic_state_helper.c @@ -273,6 +273,20 @@ void __drm_atomic_helper_plane_state_reset(struct drm_plane_state *plane_state, plane_state->normalized_zpos = val; } } + + if (plane->hotspot_x_property) { + if (!drm_object_property_get_default_value(&plane->base, + plane->hotspot_x_property, + &val)) + plane_state->hotspot_x = val; + } + + if (plane->hotspot_y_property) { + if (!drm_object_property_get_default_value(&plane->base, + plane->hotspot_y_property, + &val)) + plane_state->hotspot_y = val; + } } EXPORT_SYMBOL(__drm_atomic_helper_plane_state_reset);
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 434f3d4cb8a2..76a5d5221442 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -574,6 +574,22 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, } else if (plane->funcs->atomic_set_property) { return plane->funcs->atomic_set_property(plane, state, property, val); + } else if (property == plane->hotspot_x_property) { + if (plane->type != DRM_PLANE_TYPE_CURSOR) { + drm_dbg_atomic(plane->dev, + "[PLANE:%d:%s] is not a cursor plane: 0x%llx\n", + plane->base.id, plane->name, val); + return -EINVAL; + } + state->hotspot_x = val; + } else if (property == plane->hotspot_y_property) { + if (plane->type != DRM_PLANE_TYPE_CURSOR) { + drm_dbg_atomic(plane->dev, + "[PLANE:%d:%s] is not a cursor plane: 0x%llx\n", + plane->base.id, plane->name, val); + return -EINVAL; + } + state->hotspot_y = val; } else { drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] unknown property [PROP:%d:%s]]\n", @@ -634,6 +650,10 @@ drm_atomic_plane_get_property(struct drm_plane *plane, *val = state->scaling_filter; } else if (plane->funcs->atomic_get_property) { return plane->funcs->atomic_get_property(plane, state, property, val); + } else if (property == plane->hotspot_x_property) { + *val = state->hotspot_x; + } else if (property == plane->hotspot_y_property) { + *val = state->hotspot_y; } else { return -EINVAL; } diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 726f2f163c26..bb28d1eaf985 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -1043,6 +1043,11 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc,
fb->hot_x = req->hot_x; fb->hot_y = req->hot_y; + + if (plane->hotspot_x_property && plane->state) + plane->state->hotspot_x = req->hot_x; + if (plane->hotspot_y_property && plane->state) + plane->state->hotspot_y = req->hot_y; } else { fb = NULL; } @@ -1571,3 +1576,41 @@ int drm_plane_create_scaling_filter_property(struct drm_plane *plane, return 0; } EXPORT_SYMBOL(drm_plane_create_scaling_filter_property); + +/** + * drm_plane_create_hotspot_properties - creates the mouse hotspot + * properties and attaches them to the given cursor plane + * + * @plane: drm cursor plane + * + * This function lets driver to enable the mouse hotspot property on a given + * cursor plane. + * + * RETURNS: + * Zero for success or -errno + */ +int drm_plane_create_hotspot_properties(struct drm_plane *plane) +{ + struct drm_property *prop_x; + struct drm_property *prop_y; + + prop_x = drm_property_create_signed_range(plane->dev, 0, "HOTSPOT_X", + INT_MIN, INT_MAX); + if (IS_ERR(prop_x)) + return PTR_ERR(prop_x); + + prop_y = drm_property_create_signed_range(plane->dev, 0, "HOTSPOT_Y", + INT_MIN, INT_MAX); + if (IS_ERR(prop_y)) { + drm_property_destroy(plane->dev, prop_x); + return PTR_ERR(prop_y); + } + + drm_object_attach_property(&plane->base, prop_x, 0); + drm_object_attach_property(&plane->base, prop_y, 0); + plane->hotspot_x_property = prop_x; + plane->hotspot_y_property = prop_y; + + return 0; +} +EXPORT_SYMBOL(drm_plane_create_hotspot_properties); diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 89ea54652e87..09d3e3791e67 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -116,6 +116,10 @@ struct drm_plane_state { /** @src_h: height of visible portion of plane (in 16.16) */ uint32_t src_h, src_w;
+ /** @hotspot_x: x offset to mouse cursor hotspot */ + /** @hotspot_y: y offset to mouse cursor hotspot */ + int32_t hotspot_x, hotspot_y; + /** * @alpha: * Opacity of the plane with 0 as completely transparent and 0xffff as @@ -748,6 +752,16 @@ struct drm_plane { * scaling. */ struct drm_property *scaling_filter_property; + + /** + * @hotspot_x_property: property to set mouse hotspot x offset. + */ + struct drm_property *hotspot_x_property; + + /** + * @hotspot_y_property: property to set mouse hotspot y offset. + */ + struct drm_property *hotspot_y_property; };
#define obj_to_plane(x) container_of(x, struct drm_plane, base) @@ -907,5 +921,6 @@ drm_plane_get_damage_clips(const struct drm_plane_state *state);
int drm_plane_create_scaling_filter_property(struct drm_plane *plane, unsigned int supported_filters); +int drm_plane_create_hotspot_properties(struct drm_plane *plane);
#endif
From: Zack Rusin zackr@vmware.com
Atomic modesetting got support for mouse hotspots via the hotspot properties. Drivers need to create those properties on cursor planes which require the mouse hotspot coordinates.
Add the code creating hotspot properties and port away from old legacy hotspot API. The legacy hotspot paths have an implementation that works with new atomic properties so there's no reason to keep them and it makes sense to unify both paths.
Signed-off-by: Zack Rusin zackr@vmware.com Cc: Martin Krastev krastevm@vmware.com Cc: Maaz Mombasawala mombasawalam@vmware.com --- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 11 ++--------- drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 2 ++ drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 1 + drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 2 ++ 4 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index 693028c31b6b..a4cd312fee46 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -652,13 +652,8 @@ vmw_du_cursor_plane_atomic_update(struct drm_plane *plane, struct vmw_plane_state *vps = vmw_plane_state_to_vps(new_state); s32 hotspot_x, hotspot_y;
- hotspot_x = du->hotspot_x; - hotspot_y = du->hotspot_y; - - if (new_state->fb) { - hotspot_x += new_state->fb->hot_x; - hotspot_y += new_state->fb->hot_y; - } + hotspot_x = du->hotspot_x + new_state->hotspot_x; + hotspot_y = du->hotspot_y + new_state->hotspot_y;
du->cursor_surface = vps->surf; du->cursor_bo = vps->bo; @@ -2270,8 +2265,6 @@ int vmw_du_crtc_gamma_set(struct drm_crtc *crtc, int i;
for (i = 0; i < size; i++) { - DRM_DEBUG("%d r/g/b = 0x%04x / 0x%04x / 0x%04x\n", i, - r[i], g[i], b[i]); vmw_write(dev_priv, SVGA_PALETTE_BASE + i * 3 + 0, r[i] >> 8); vmw_write(dev_priv, SVGA_PALETTE_BASE + i * 3 + 1, g[i] >> 8); vmw_write(dev_priv, SVGA_PALETTE_BASE + i * 3 + 2, b[i] >> 8); diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c index e4347faccee0..43e89c6755b2 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c @@ -474,6 +474,8 @@ static int vmw_ldu_init(struct vmw_private *dev_priv, unsigned unit) (&connector->base, dev_priv->implicit_placement_property, 1); + if (vmw_cmd_supported(dev_priv)) + drm_plane_create_hotspot_properties(&cursor->base);
return 0;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c index c89ad3a2d141..8d46b0cbe640 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c @@ -932,6 +932,7 @@ static int vmw_sou_init(struct vmw_private *dev_priv, unsigned unit) dev->mode_config.suggested_x_property, 0); drm_object_attach_property(&connector->base, dev->mode_config.suggested_y_property, 0); + drm_plane_create_hotspot_properties(&cursor->base); return 0;
err_free_unregister: diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c index eb014b97d156..d940b9a525e7 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c @@ -1822,6 +1822,8 @@ static int vmw_stdu_init(struct vmw_private *dev_priv, unsigned unit) dev->mode_config.suggested_x_property, 0); drm_object_attach_property(&connector->base, dev->mode_config.suggested_y_property, 0); + drm_plane_create_hotspot_properties(&cursor->base); + return 0;
err_free_unregister:
From: Martin Krastev krastevm@vmware.com
On 2.06.22 г. 18:42 ч., Zack Rusin wrote:
From: Zack Rusin zackr@vmware.com
Atomic modesetting got support for mouse hotspots via the hotspot properties. Drivers need to create those properties on cursor planes which require the mouse hotspot coordinates.
Add the code creating hotspot properties and port away from old legacy hotspot API. The legacy hotspot paths have an implementation that works with new atomic properties so there's no reason to keep them and it makes sense to unify both paths.
Signed-off-by: Zack Rusin zackr@vmware.com Cc: Martin Krastev krastevm@vmware.com Cc: Maaz Mombasawala mombasawalam@vmware.com
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 11 ++--------- drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 2 ++ drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 1 + drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 2 ++ 4 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index 693028c31b6b..a4cd312fee46 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -652,13 +652,8 @@ vmw_du_cursor_plane_atomic_update(struct drm_plane *plane, struct vmw_plane_state *vps = vmw_plane_state_to_vps(new_state); s32 hotspot_x, hotspot_y;
- hotspot_x = du->hotspot_x;
- hotspot_y = du->hotspot_y;
- if (new_state->fb) {
hotspot_x += new_state->fb->hot_x;hotspot_y += new_state->fb->hot_y;- }
hotspot_x = du->hotspot_x + new_state->hotspot_x;
hotspot_y = du->hotspot_y + new_state->hotspot_y;
du->cursor_surface = vps->surf; du->cursor_bo = vps->bo;
@@ -2270,8 +2265,6 @@ int vmw_du_crtc_gamma_set(struct drm_crtc *crtc, int i;
for (i = 0; i < size; i++) {
DRM_DEBUG("%d r/g/b = 0x%04x / 0x%04x / 0x%04x\n", i,r[i], g[i], b[i]);diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c index e4347faccee0..43e89c6755b2 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c @@ -474,6 +474,8 @@ static int vmw_ldu_init(struct vmw_private *dev_priv, unsigned unit) (&connector->base, dev_priv->implicit_placement_property, 1);
if (vmw_cmd_supported(dev_priv))
drm_plane_create_hotspot_properties(&cursor->base);return 0;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c index c89ad3a2d141..8d46b0cbe640 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c @@ -932,6 +932,7 @@ static int vmw_sou_init(struct vmw_private *dev_priv, unsigned unit) dev->mode_config.suggested_x_property, 0); drm_object_attach_property(&connector->base, dev->mode_config.suggested_y_property, 0);
drm_plane_create_hotspot_properties(&cursor->base); return 0;
err_free_unregister:
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c index eb014b97d156..d940b9a525e7 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c @@ -1822,6 +1822,8 @@ static int vmw_stdu_init(struct vmw_private *dev_priv, unsigned unit) dev->mode_config.suggested_x_property, 0); drm_object_attach_property(&connector->base, dev->mode_config.suggested_y_property, 0);
drm_plane_create_hotspot_properties(&cursor->base);
return 0;
err_free_unregister:
LGTM!
Reviewed-by: Martin Krastev krastevm@vmware.com
Regards,
Martin
From: Zack Rusin zackr@vmware.com
Atomic modesetting got support for mouse hotspots via the hotspot properties. Drivers need to create those properties on cursor planes which require the mouse hotspot coordinates.
Add the code creating hotspot properties and port away from old legacy hotspot API. The legacy hotspot paths have an implementation that works with new atomic properties so there's no reason to keep them and it makes sense to unify both paths.
Signed-off-by: Zack Rusin zackr@vmware.com Cc: Dave Airlie airlied@redhat.com Cc: Gerd Hoffmann kraxel@redhat.com Cc: Daniel Vetter daniel@ffwll.ch Cc: virtualization@lists.linux-foundation.org Cc: spice-devel@lists.freedesktop.org --- drivers/gpu/drm/qxl/qxl_display.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index 9a64fa4c7530..4b578f9960cd 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -508,8 +508,8 @@ static int qxl_primary_apply_cursor(struct qxl_device *qdev,
cmd = (struct qxl_cursor_cmd *)qxl_release_map(qdev, release); cmd->type = QXL_CURSOR_SET; - cmd->u.set.position.x = plane_state->crtc_x + fb->hot_x; - cmd->u.set.position.y = plane_state->crtc_y + fb->hot_y; + cmd->u.set.position.x = plane_state->crtc_x + plane_state->hotspot_x; + cmd->u.set.position.y = plane_state->crtc_y + plane_state->hotspot_y;
cmd->u.set.shape = qxl_bo_physical_address(qdev, qcrtc->cursor_bo, 0);
@@ -552,8 +552,8 @@ static int qxl_primary_move_cursor(struct qxl_device *qdev,
cmd = (struct qxl_cursor_cmd *)qxl_release_map(qdev, release); cmd->type = QXL_CURSOR_MOVE; - cmd->u.position.x = plane_state->crtc_x + fb->hot_x; - cmd->u.position.y = plane_state->crtc_y + fb->hot_y; + cmd->u.position.x = plane_state->crtc_x + plane_state->hotspot_x; + cmd->u.position.y = plane_state->crtc_y + plane_state->hotspot_y; qxl_release_unmap(qdev, release, &cmd->release_info);
qxl_release_fence_buffer_objects(release); @@ -849,8 +849,8 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane, struct qxl_bo *old_cursor_bo = qcrtc->cursor_bo;
qcrtc->cursor_bo = qxl_create_cursor(qdev, user_bo, - new_state->fb->hot_x, - new_state->fb->hot_y); + new_state->hotspot_x, + new_state->hotspot_y); qxl_free_cursor(old_cursor_bo); }
@@ -1002,6 +1002,7 @@ static int qdev_crtc_init(struct drm_device *dev, int crtc_id)
qxl_crtc->index = crtc_id; drm_crtc_helper_add(&qxl_crtc->base, &qxl_crtc_helper_funcs); + drm_plane_create_hotspot_properties(cursor); return 0;
clean_cursor:
Hi Zack,
I love your patch! Perhaps something to improve:
[auto build test WARNING on drm/drm-next] [also build test WARNING on drm-exynos/exynos-drm-next drm-intel/for-linux-next drm-tip/drm-tip v5.18 next-20220602] [cannot apply to airlied/drm-next tegra-drm/drm/tegra/for-next] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Zack-Rusin/drm-Add-mouse-curs... base: git://anongit.freedesktop.org/drm/drm drm-next config: x86_64-randconfig-a011 (https://download.01.org/0day-ci/archive/20220603/202206030624.TdMaRYS5-lkp@i...) compiler: gcc-11 (Debian 11.3.0-1) 11.3.0 reproduce (this is a W=1 build): # https://github.com/intel-lab-lkp/linux/commit/0bf2395ee17bd25ae6411c560de883... git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Zack-Rusin/drm-Add-mouse-cursor-hotspot-support-to-atomic-KMS/20220602-234633 git checkout 0bf2395ee17bd25ae6411c560de883496256195d # save the config file mkdir build_dir && cp config build_dir/.config make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/gpu/drm/qxl/
If you fix the issue, kindly add following tag where applicable Reported-by: kernel test robot lkp@intel.com
All warnings (new ones prefixed by >>):
drivers/gpu/drm/qxl/qxl_display.c: In function 'qxl_primary_apply_cursor':
drivers/gpu/drm/qxl/qxl_display.c:486:33: warning: unused variable 'fb' [-Wunused-variable]
486 | struct drm_framebuffer *fb = plane_state->fb; | ^~ drivers/gpu/drm/qxl/qxl_display.c: In function 'qxl_primary_move_cursor': drivers/gpu/drm/qxl/qxl_display.c:532:33: warning: unused variable 'fb' [-Wunused-variable] 532 | struct drm_framebuffer *fb = plane_state->fb; | ^~
vim +/fb +486 drivers/gpu/drm/qxl/qxl_display.c
c2ff663260fee3 Gabriel Krisman Bertazi 2017-02-27 482 b4b27f08f9f96d Gerd Hoffmann 2021-02-17 483 static int qxl_primary_apply_cursor(struct qxl_device *qdev, b4b27f08f9f96d Gerd Hoffmann 2021-02-17 484 struct drm_plane_state *plane_state) 9428088c90b6f7 Ray Strode 2017-11-27 485 { b4b27f08f9f96d Gerd Hoffmann 2021-02-17 @486 struct drm_framebuffer *fb = plane_state->fb; b4b27f08f9f96d Gerd Hoffmann 2021-02-17 487 struct qxl_crtc *qcrtc = to_qxl_crtc(plane_state->crtc); 9428088c90b6f7 Ray Strode 2017-11-27 488 struct qxl_cursor_cmd *cmd; 9428088c90b6f7 Ray Strode 2017-11-27 489 struct qxl_release *release; 9428088c90b6f7 Ray Strode 2017-11-27 490 int ret = 0; 9428088c90b6f7 Ray Strode 2017-11-27 491 9428088c90b6f7 Ray Strode 2017-11-27 492 if (!qcrtc->cursor_bo) 9428088c90b6f7 Ray Strode 2017-11-27 493 return 0; 9428088c90b6f7 Ray Strode 2017-11-27 494 9428088c90b6f7 Ray Strode 2017-11-27 495 ret = qxl_alloc_release_reserved(qdev, sizeof(*cmd), 9428088c90b6f7 Ray Strode 2017-11-27 496 QXL_RELEASE_CURSOR_CMD, 9428088c90b6f7 Ray Strode 2017-11-27 497 &release, NULL); 9428088c90b6f7 Ray Strode 2017-11-27 498 if (ret) 9428088c90b6f7 Ray Strode 2017-11-27 499 return ret; 9428088c90b6f7 Ray Strode 2017-11-27 500 9428088c90b6f7 Ray Strode 2017-11-27 501 ret = qxl_release_list_add(release, qcrtc->cursor_bo); 9428088c90b6f7 Ray Strode 2017-11-27 502 if (ret) 9428088c90b6f7 Ray Strode 2017-11-27 503 goto out_free_release; 9428088c90b6f7 Ray Strode 2017-11-27 504 9428088c90b6f7 Ray Strode 2017-11-27 505 ret = qxl_release_reserve_list(release, false); 9428088c90b6f7 Ray Strode 2017-11-27 506 if (ret) 9428088c90b6f7 Ray Strode 2017-11-27 507 goto out_free_release; 9428088c90b6f7 Ray Strode 2017-11-27 508 9428088c90b6f7 Ray Strode 2017-11-27 509 cmd = (struct qxl_cursor_cmd *)qxl_release_map(qdev, release); 9428088c90b6f7 Ray Strode 2017-11-27 510 cmd->type = QXL_CURSOR_SET; 0bf2395ee17bd2 Zack Rusin 2022-06-02 511 cmd->u.set.position.x = plane_state->crtc_x + plane_state->hotspot_x; 0bf2395ee17bd2 Zack Rusin 2022-06-02 512 cmd->u.set.position.y = plane_state->crtc_y + plane_state->hotspot_y; 9428088c90b6f7 Ray Strode 2017-11-27 513 9428088c90b6f7 Ray Strode 2017-11-27 514 cmd->u.set.shape = qxl_bo_physical_address(qdev, qcrtc->cursor_bo, 0); 9428088c90b6f7 Ray Strode 2017-11-27 515 9428088c90b6f7 Ray Strode 2017-11-27 516 cmd->u.set.visible = 1; 9428088c90b6f7 Ray Strode 2017-11-27 517 qxl_release_unmap(qdev, release, &cmd->release_info); 9428088c90b6f7 Ray Strode 2017-11-27 518 9428088c90b6f7 Ray Strode 2017-11-27 519 qxl_release_fence_buffer_objects(release); 933db73351d359 Vasily Averin 2020-04-29 520 qxl_push_cursor_ring_release(qdev, release, QXL_CMD_CURSOR, false); 9428088c90b6f7 Ray Strode 2017-11-27 521 9428088c90b6f7 Ray Strode 2017-11-27 522 return ret; 9428088c90b6f7 Ray Strode 2017-11-27 523 9428088c90b6f7 Ray Strode 2017-11-27 524 out_free_release: 9428088c90b6f7 Ray Strode 2017-11-27 525 qxl_release_free(qdev, release); 9428088c90b6f7 Ray Strode 2017-11-27 526 return ret; 9428088c90b6f7 Ray Strode 2017-11-27 527 } 9428088c90b6f7 Ray Strode 2017-11-27 528
Hi Zack,
I love your patch! Perhaps something to improve:
[auto build test WARNING on drm/drm-next] [also build test WARNING on drm-exynos/exynos-drm-next drm-intel/for-linux-next drm-tip/drm-tip v5.18 next-20220602] [cannot apply to airlied/drm-next tegra-drm/drm/tegra/for-next] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Zack-Rusin/drm-Add-mouse-curs... base: git://anongit.freedesktop.org/drm/drm drm-next config: i386-randconfig-a013 (https://download.01.org/0day-ci/archive/20220603/202206030750.8hv8vdBA-lkp@i...) compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project b364c76683f8ef241025a9556300778c07b590c2) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/intel-lab-lkp/linux/commit/0bf2395ee17bd25ae6411c560de883... git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Zack-Rusin/drm-Add-mouse-cursor-hotspot-support-to-atomic-KMS/20220602-234633 git checkout 0bf2395ee17bd25ae6411c560de883496256195d # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/gpu/drm/qxl/
If you fix the issue, kindly add following tag where applicable Reported-by: kernel test robot lkp@intel.com
All warnings (new ones prefixed by >>):
drivers/gpu/drm/qxl/qxl_display.c:486:26: warning: unused variable 'fb' [-Wunused-variable]
struct drm_framebuffer *fb = plane_state->fb; ^ drivers/gpu/drm/qxl/qxl_display.c:532:26: warning: unused variable 'fb' [-Wunused-variable] struct drm_framebuffer *fb = plane_state->fb; ^ 2 warnings generated.
vim +/fb +486 drivers/gpu/drm/qxl/qxl_display.c
c2ff663260fee3 Gabriel Krisman Bertazi 2017-02-27 482 b4b27f08f9f96d Gerd Hoffmann 2021-02-17 483 static int qxl_primary_apply_cursor(struct qxl_device *qdev, b4b27f08f9f96d Gerd Hoffmann 2021-02-17 484 struct drm_plane_state *plane_state) 9428088c90b6f7 Ray Strode 2017-11-27 485 { b4b27f08f9f96d Gerd Hoffmann 2021-02-17 @486 struct drm_framebuffer *fb = plane_state->fb; b4b27f08f9f96d Gerd Hoffmann 2021-02-17 487 struct qxl_crtc *qcrtc = to_qxl_crtc(plane_state->crtc); 9428088c90b6f7 Ray Strode 2017-11-27 488 struct qxl_cursor_cmd *cmd; 9428088c90b6f7 Ray Strode 2017-11-27 489 struct qxl_release *release; 9428088c90b6f7 Ray Strode 2017-11-27 490 int ret = 0; 9428088c90b6f7 Ray Strode 2017-11-27 491 9428088c90b6f7 Ray Strode 2017-11-27 492 if (!qcrtc->cursor_bo) 9428088c90b6f7 Ray Strode 2017-11-27 493 return 0; 9428088c90b6f7 Ray Strode 2017-11-27 494 9428088c90b6f7 Ray Strode 2017-11-27 495 ret = qxl_alloc_release_reserved(qdev, sizeof(*cmd), 9428088c90b6f7 Ray Strode 2017-11-27 496 QXL_RELEASE_CURSOR_CMD, 9428088c90b6f7 Ray Strode 2017-11-27 497 &release, NULL); 9428088c90b6f7 Ray Strode 2017-11-27 498 if (ret) 9428088c90b6f7 Ray Strode 2017-11-27 499 return ret; 9428088c90b6f7 Ray Strode 2017-11-27 500 9428088c90b6f7 Ray Strode 2017-11-27 501 ret = qxl_release_list_add(release, qcrtc->cursor_bo); 9428088c90b6f7 Ray Strode 2017-11-27 502 if (ret) 9428088c90b6f7 Ray Strode 2017-11-27 503 goto out_free_release; 9428088c90b6f7 Ray Strode 2017-11-27 504 9428088c90b6f7 Ray Strode 2017-11-27 505 ret = qxl_release_reserve_list(release, false); 9428088c90b6f7 Ray Strode 2017-11-27 506 if (ret) 9428088c90b6f7 Ray Strode 2017-11-27 507 goto out_free_release; 9428088c90b6f7 Ray Strode 2017-11-27 508 9428088c90b6f7 Ray Strode 2017-11-27 509 cmd = (struct qxl_cursor_cmd *)qxl_release_map(qdev, release); 9428088c90b6f7 Ray Strode 2017-11-27 510 cmd->type = QXL_CURSOR_SET; 0bf2395ee17bd2 Zack Rusin 2022-06-02 511 cmd->u.set.position.x = plane_state->crtc_x + plane_state->hotspot_x; 0bf2395ee17bd2 Zack Rusin 2022-06-02 512 cmd->u.set.position.y = plane_state->crtc_y + plane_state->hotspot_y; 9428088c90b6f7 Ray Strode 2017-11-27 513 9428088c90b6f7 Ray Strode 2017-11-27 514 cmd->u.set.shape = qxl_bo_physical_address(qdev, qcrtc->cursor_bo, 0); 9428088c90b6f7 Ray Strode 2017-11-27 515 9428088c90b6f7 Ray Strode 2017-11-27 516 cmd->u.set.visible = 1; 9428088c90b6f7 Ray Strode 2017-11-27 517 qxl_release_unmap(qdev, release, &cmd->release_info); 9428088c90b6f7 Ray Strode 2017-11-27 518 9428088c90b6f7 Ray Strode 2017-11-27 519 qxl_release_fence_buffer_objects(release); 933db73351d359 Vasily Averin 2020-04-29 520 qxl_push_cursor_ring_release(qdev, release, QXL_CMD_CURSOR, false); 9428088c90b6f7 Ray Strode 2017-11-27 521 9428088c90b6f7 Ray Strode 2017-11-27 522 return ret; 9428088c90b6f7 Ray Strode 2017-11-27 523 9428088c90b6f7 Ray Strode 2017-11-27 524 out_free_release: 9428088c90b6f7 Ray Strode 2017-11-27 525 qxl_release_free(qdev, release); 9428088c90b6f7 Ray Strode 2017-11-27 526 return ret; 9428088c90b6f7 Ray Strode 2017-11-27 527 } 9428088c90b6f7 Ray Strode 2017-11-27 528
From: Zack Rusin zackr@vmware.com
Atomic modesetting got support for mouse hotspots via the hotspot properties. Drivers need to create those properties on cursor planes which require the mouse hotspot coordinates.
Add the code creating hotspot properties and port away from old legacy hotspot API. The legacy hotspot paths have an implementation that works with new atomic properties so there's no reason to keep them and it makes sense to unify both paths.
Signed-off-by: Zack Rusin zackr@vmware.com Cc: Hans de Goede hdegoede@redhat.com Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch --- drivers/gpu/drm/vboxvideo/vbox_mode.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/vboxvideo/vbox_mode.c b/drivers/gpu/drm/vboxvideo/vbox_mode.c index 4017b0a621fc..4c0a01a18f5a 100644 --- a/drivers/gpu/drm/vboxvideo/vbox_mode.c +++ b/drivers/gpu/drm/vboxvideo/vbox_mode.c @@ -427,8 +427,8 @@ static void vbox_cursor_atomic_update(struct drm_plane *plane, flags = VBOX_MOUSE_POINTER_VISIBLE | VBOX_MOUSE_POINTER_SHAPE | VBOX_MOUSE_POINTER_ALPHA; hgsmi_update_pointer_shape(vbox->guest_pool, flags, - min_t(u32, max(fb->hot_x, 0), width), - min_t(u32, max(fb->hot_y, 0), height), + min_t(u32, max(new_state->hotspot_x, 0), width), + min_t(u32, max(new_state->hotspot_y, 0), height), width, height, vbox->cursor_data, data_size);
mutex_unlock(&vbox->hw_mutex); @@ -575,6 +575,7 @@ static struct vbox_crtc *vbox_crtc_init(struct drm_device *dev, unsigned int i) ret = PTR_ERR(cursor); goto clean_primary; } + drm_plane_create_hotspot_properties(cursor); } else { DRM_WARN("VirtualBox host is too old, no cursor support\n"); }
From: Zack Rusin zackr@vmware.com
Atomic modesetting got support for mouse hotspots via the hotspot properties. Drivers need to create those properties on cursor planes which require the mouse hotspot coordinates.
Add the code creating hotspot properties and port away from old legacy hotspot API. The legacy hotspot paths have an implementation that works with new atomic properties so there's no reason to keep them and it makes sense to unify both paths.
Signed-off-by: Zack Rusin zackr@vmware.com Cc: David Airlie airlied@linux.ie Cc: Gerd Hoffmann kraxel@redhat.com Cc: Gurchetan Singh gurchetansingh@chromium.org Cc: Chia-I Wu olvaffe@gmail.com Cc: Daniel Vetter daniel@ffwll.ch Cc: virtualization@lists.linux-foundation.org --- drivers/gpu/drm/virtio/virtgpu_display.c | 1 + drivers/gpu/drm/virtio/virtgpu_plane.c | 8 ++++---- 2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c index f73352e7b832..848ac2314399 100644 --- a/drivers/gpu/drm/virtio/virtgpu_display.c +++ b/drivers/gpu/drm/virtio/virtgpu_display.c @@ -288,6 +288,7 @@ static int vgdev_output_init(struct virtio_gpu_device *vgdev, int index)
drm_connector_attach_encoder(connector, encoder); drm_connector_register(connector); + drm_plane_create_hotspot_properties(cursor); return 0; }
diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c index 6d3cc9e238a4..21c8adf51c6c 100644 --- a/drivers/gpu/drm/virtio/virtgpu_plane.c +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c @@ -331,16 +331,16 @@ static void virtio_gpu_cursor_plane_update(struct drm_plane *plane, DRM_DEBUG("update, handle %d, pos +%d+%d, hot %d,%d\n", handle, plane->state->crtc_x, plane->state->crtc_y, - plane->state->fb ? plane->state->fb->hot_x : 0, - plane->state->fb ? plane->state->fb->hot_y : 0); + plane->state->hotspot_x, + plane->state->hotspot_y); output->cursor.hdr.type = cpu_to_le32(VIRTIO_GPU_CMD_UPDATE_CURSOR); output->cursor.resource_id = cpu_to_le32(handle); if (plane->state->fb) { output->cursor.hot_x = - cpu_to_le32(plane->state->fb->hot_x); + cpu_to_le32(plane->state->hotspot_x); output->cursor.hot_y = - cpu_to_le32(plane->state->fb->hot_y); + cpu_to_le32(plane->state->hotspot_y); } else { output->cursor.hot_x = cpu_to_le32(0); output->cursor.hot_y = cpu_to_le32(0);
From: Zack Rusin zackr@vmware.com
Atomic modesetting support mouse cursor offsets via the hotspot properties that are creates on cursor planes. All drivers which support hotspot are atomic and the legacy code has been implemented in terms of the atomic properties as well.
Due to the above the lagacy cursor hotspot code is no longer used or needed and can be removed.
Signed-off-by: Zack Rusin zackr@vmware.com 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 --- drivers/gpu/drm/drm_plane.c | 3 --- include/drm/drm_framebuffer.h | 12 ------------ 2 files changed, 15 deletions(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index bb28d1eaf985..d38b5301b5dd 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -1041,9 +1041,6 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, return PTR_ERR(fb); }
- fb->hot_x = req->hot_x; - fb->hot_y = req->hot_y; - if (plane->hotspot_x_property && plane->state) plane->state->hotspot_x = req->hot_x; if (plane->hotspot_y_property && plane->state) diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h index f67c5b7bcb68..c306ae2e2d47 100644 --- a/include/drm/drm_framebuffer.h +++ b/include/drm/drm_framebuffer.h @@ -188,18 +188,6 @@ struct drm_framebuffer { * DRM_MODE_FB_MODIFIERS. */ int flags; - /** - * @hot_x: X coordinate of the cursor hotspot. Used by the legacy cursor - * IOCTL when the driver supports cursor through a DRM_PLANE_TYPE_CURSOR - * universal plane. - */ - int hot_x; - /** - * @hot_y: Y coordinate of the cursor hotspot. Used by the legacy cursor - * IOCTL when the driver supports cursor through a DRM_PLANE_TYPE_CURSOR - * universal plane. - */ - int hot_y; /** * @filp_head: Placed on &drm_file.fbs, protected by &drm_file.fbs_lock. */
Hi,
the legacy kms to atomic. This left virtualized drivers, all which are atomic, in a weird spot because all userspace compositors put those drivers on deny-lists for atomic kms due to the fact that mouse clicks were incorrectly routed, e.g:
- all userspace code needs to hardcore a list of drivers which require
hotspots because there's no way to query from drm "does this driver require hotspot"
So drivers move from one list to another. Not ideal, but also not worse than it was before, so:
Acked-by: Gerd Hoffmann kraxel@redhat.com
for the qxl and virtio driver changes.
take care, Gerd
On Jun 3, 2022, at 6:28 AM, Gerd Hoffmann kraxel@redhat.com wrote:
⚠ External Email
Hi,
the legacy kms to atomic. This left virtualized drivers, all which are atomic, in a weird spot because all userspace compositors put those drivers on deny-lists for atomic kms due to the fact that mouse clicks were incorrectly routed, e.g:
- all userspace code needs to hardcore a list of drivers which require
hotspots because there's no way to query from drm "does this driver require hotspot"
So drivers move from one list to another. Not ideal, but also not worse than it was before, so:
Yea, that problem was always there because we never had a way of checking whether a particular driver requires hotspot info. Unfortunately I don’t think this can not be solved in the kernel anymore. Technically the hotspot properties solve it: does the driver expose hotspot properties on cursor planes? If yes, then it requires hotspot info. But we can’t differentiate between: - driver that doesn’t require hotspot info - driver that requires hotspot data but we’re running on a kernel before those properties were implemented We’d have to backport those properties to every kernel between now and when atomic kms was implemented. Same with any other check we could add to the kernel. Likely libdrm would be the place for this atm but I’m not sure in what form.
z
Hi,
Please, read this thread: https://lists.freedesktop.org/archives/dri-devel/2020-March/thread.html#2596...
It has a lot of information about the pitfalls of cursor hotspot and other things done by VM software.
In particular: since the driver will ignore the KMS cursor plane position set by user-space, I don't think it's okay to just expose without opt-in from user-space (e.g. with a DRM_CLIENT_CAP).
cc wayland-devel and Pekka for user-space feedback.
On Thursday, June 2nd, 2022 at 17:42, Zack Rusin zack@kde.org wrote:
- all userspace code needs to hardcore a list of drivers which require
hotspots because there's no way to query from drm "does this driver require hotspot"
Can you elaborate? I'm not sure I understand what you mean here.
Thanks,
Simon
On Jun 3, 2022, at 10:14 AM, Simon Ser contact@emersion.fr wrote:
Hi,
Please, read this thread: https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.free...
It has a lot of information about the pitfalls of cursor hotspot and other things done by VM software.
In particular: since the driver will ignore the KMS cursor plane position set by user-space, I don't think it's okay to just expose without opt-in from user-space (e.g. with a DRM_CLIENT_CAP).
cc wayland-devel and Pekka for user-space feedback.
I think Thomas expressed our concerns and reasons why those wouldn’t work for us back then. Is there something else you’d like to add?
On Thursday, June 2nd, 2022 at 17:42, Zack Rusin zack@kde.org wrote:
- all userspace code needs to hardcore a list of drivers which require
hotspots because there's no way to query from drm "does this driver require hotspot"
Can you elaborate? I'm not sure I understand what you mean here.
Only some drivers require informations about cursor hotspot, user space currently has no way of figuring out which ones are those, i.e. amdgpu doesn’t care about hotspots, qxl does so when running on qxl without properly set cursor hotspot atomic kms will result in a desktop where mouse clicks have incorrect coordinates. There’s no way to differentiate between drivers that do not care about cursor hotspots and ones that absolutely require it.
z
On Friday, June 3rd, 2022 at 16:27, Zack Rusin zackr@vmware.com wrote:
In particular: since the driver will ignore the KMS cursor plane position set by user-space, I don't think it's okay to just expose without opt-in from user-space (e.g. with a DRM_CLIENT_CAP).
cc wayland-devel and Pekka for user-space feedback.
I think Thomas expressed our concerns and reasons why those wouldn’t work for us back then. Is there something else you’d like to add?
I disagreed, and I don't understand Thomas' reasoning.
KMS clients will need an update to work correctly. Adding a new prop without a cap leaves existing KMS clients broken. Adding a cap allows both existing KMS clients and updated KMS clients to work correctly.
On Thursday, June 2nd, 2022 at 17:42, Zack Rusin zack@kde.org wrote:
- all userspace code needs to hardcore a list of drivers which require
hotspots because there's no way to query from drm "does this driver require hotspot"
Can you elaborate? I'm not sure I understand what you mean here.
Only some drivers require informations about cursor hotspot, user space currently has no way of figuring out which ones are those, i.e. amdgpu doesn’t care about hotspots, qxl does so when running on qxl without properly set cursor hotspot atomic kms will result in a desktop where mouse clicks have incorrect coordinates.
There’s no way to differentiate between drivers that do not care about cursor hotspots and ones that absolutely require it.
Only VM drivers should expose the hotspot properties. Real drivers like amdgpu must not expose it.
On Jun 3, 2022, at 10:32 AM, Simon Ser contact@emersion.fr wrote:
⚠ External Email
On Friday, June 3rd, 2022 at 16:27, Zack Rusin zackr@vmware.com wrote:
In particular: since the driver will ignore the KMS cursor plane position set by user-space, I don't think it's okay to just expose without opt-in from user-space (e.g. with a DRM_CLIENT_CAP).
cc wayland-devel and Pekka for user-space feedback.
I think Thomas expressed our concerns and reasons why those wouldn’t work for us back then. Is there something else you’d like to add?
I disagreed, and I don't understand Thomas' reasoning.
KMS clients will need an update to work correctly. Adding a new prop without a cap leaves existing KMS clients broken. Adding a cap allows both existing KMS clients and updated KMS clients to work correctly.
I’m not sure what you mean here. They are broken right now. That’s what we’re fixing. That’s the reason why the virtualized drivers are on deny-lists for all atomic kms. So nothing needs to be updated. If you have a kms client that was using virtualized drivers with atomic kms then mouse clicks never worked correctly. So, yes, clients need to set cursor hotspot if they want mouse to work correctly with virtualized drivers.
On Thursday, June 2nd, 2022 at 17:42, Zack Rusin zack@kde.org wrote:
- all userspace code needs to hardcore a list of drivers which require
hotspots because there's no way to query from drm "does this driver require hotspot"
Can you elaborate? I'm not sure I understand what you mean here.
Only some drivers require informations about cursor hotspot, user space currently has no way of figuring out which ones are those, i.e. amdgpu doesn’t care about hotspots, qxl does so when running on qxl without properly set cursor hotspot atomic kms will result in a desktop where mouse clicks have incorrect coordinates.
There’s no way to differentiate between drivers that do not care about cursor hotspots and ones that absolutely require it.
Only VM drivers should expose the hotspot properties. Real drivers like amdgpu must not expose it.
Yes, that’s what I said. There’s no way to differentiate between amdgpu that doesn’t have those properties and virtio_gpu driver from a kernel before hotspot properties were added.
z
On Friday, June 3rd, 2022 at 16:38, Zack Rusin zackr@vmware.com wrote:
On Jun 3, 2022, at 10:32 AM, Simon Ser contact@emersion.fr wrote:
⚠ External Email
On Friday, June 3rd, 2022 at 16:27, Zack Rusin zackr@vmware.com wrote:
In particular: since the driver will ignore the KMS cursor plane position set by user-space, I don't think it's okay to just expose without opt-in from user-space (e.g. with a DRM_CLIENT_CAP).
cc wayland-devel and Pekka for user-space feedback.
I think Thomas expressed our concerns and reasons why those wouldn’t work for us back then. Is there something else you’d like to add?
I disagreed, and I don't understand Thomas' reasoning.
KMS clients will need an update to work correctly. Adding a new prop without a cap leaves existing KMS clients broken. Adding a cap allows both existing KMS clients and updated KMS clients to work correctly.
I’m not sure what you mean here. They are broken right now. That’s what we’re fixing. That’s the reason why the virtualized drivers are on deny-lists for all atomic kms. So nothing needs to be updated. If you have a kms client that was using virtualized drivers with atomic kms then mouse clicks never worked correctly.
So, yes, clients need to set cursor hotspot if they want mouse to work correctly with virtualized drivers.
My proposal was:
- Introduce DRM_CLIENT_CAP_CURSOR_PLANE_NO_POSITION (or a better name). Only user-space which supports the hotspot props will enable it. - By default, don't expose a cursor plane, because current user-space doesn't support it (Weston might put a window in the cursor plane, and nobody can report hotspot). - If the KMS client enables the cap, advertise the cursor plane, and make it so the plane doesn't have the CRTC_X/CRTC_Y properties since the driver will pick the position.
That way both old and new user-space are fixed.
On Thursday, June 2nd, 2022 at 17:42, Zack Rusin zack@kde.org wrote:
- all userspace code needs to hardcore a list of drivers which require
hotspots because there's no way to query from drm "does this driver require hotspot"
Can you elaborate? I'm not sure I understand what you mean here.
Only some drivers require informations about cursor hotspot, user space currently has no way of figuring out which ones are those, i.e. amdgpu doesn’t care about hotspots, qxl does so when running on qxl without properly set cursor hotspot atomic kms will result in a desktop where mouse clicks have incorrect coordinates.
There’s no way to differentiate between drivers that do not care about cursor hotspots and ones that absolutely require it.
Only VM drivers should expose the hotspot properties. Real drivers like amdgpu must not expose it.
Yes, that’s what I said. There’s no way to differentiate between amdgpu that doesn’t have those properties and virtio_gpu driver from a kernel before hotspot properties were added.
See cap proposal above.
On Jun 3, 2022, at 10:56 AM, Simon Ser <contact@emersion.frmailto:contact@emersion.fr> wrote:
⚠ External Email
On Friday, June 3rd, 2022 at 16:38, Zack Rusin <zackr@vmware.commailto:zackr@vmware.com> wrote:
On Jun 3, 2022, at 10:32 AM, Simon Ser <contact@emersion.frmailto:contact@emersion.fr> wrote:
⚠ External Email
On Friday, June 3rd, 2022 at 16:27, Zack Rusin <zackr@vmware.commailto:zackr@vmware.com> wrote:
In particular: since the driver will ignore the KMS cursor plane position set by user-space, I don't think it's okay to just expose without opt-in from user-space (e.g. with a DRM_CLIENT_CAP).
cc wayland-devel and Pekka for user-space feedback.
I think Thomas expressed our concerns and reasons why those wouldn’t work for us back then. Is there something else you’d like to add?
I disagreed, and I don't understand Thomas' reasoning.
KMS clients will need an update to work correctly. Adding a new prop without a cap leaves existing KMS clients broken. Adding a cap allows both existing KMS clients and updated KMS clients to work correctly.
I’m not sure what you mean here. They are broken right now. That’s what we’re fixing. That’s the reason why the virtualized drivers are on deny-lists for all atomic kms. So nothing needs to be updated. If you have a kms client that was using virtualized drivers with atomic kms then mouse clicks never worked correctly.
So, yes, clients need to set cursor hotspot if they want mouse to work correctly with virtualized drivers.
My proposal was:
- Introduce DRM_CLIENT_CAP_CURSOR_PLANE_NO_POSITION (or a better name). Only user-space which supports the hotspot props will enable it. - By default, don't expose a cursor plane, because current user-space doesn't support it (Weston might put a window in the cursor plane, and nobody can report hotspot). - If the KMS client enables the cap, advertise the cursor plane, and make it so the plane doesn't have the CRTC_X/CRTC_Y properties since the driver will pick the position.
That way both old and new user-space are fixed.
I don’t think I see how that fixes anything. In particular I don’t see a way of fixing the old user space at all. We require hotspot info, old user space doesn’t set it because there’s no way of setting it on atomic kms. Whether we expose cursor plane or not doesn’t change the fact that we still require the hotspot info. I don’t know… Unless the small-print in your proposal is “rewrite mouse cursor support in all hypervisors” (which I don’t see how you could make work without hotspot info) then I don’t think this solves anything, old or new.
Or did you have some magical way of extracting the hotspot data without the kms clients providing it? e.g. in your proposal in virtgpu_plane.c how is output->cursor.hot_x and output->cursor.hot_y set without a cursor plane and with it?
z
On Friday, June 3rd, 2022 at 17:17, Zack Rusin zackr@vmware.com wrote:
On Jun 3, 2022, at 10:56 AM, Simon Ser contact@emersion.fr wrote: ⚠ External Email
On Friday, June 3rd, 2022 at 16:38, Zack Rusin zackr@vmware.com wrote:
On Jun 3, 2022, at 10:32 AM, Simon Ser contact@emersion.fr wrote:
⚠ External Email
On Friday, June 3rd, 2022 at 16:27, Zack Rusin zackr@vmware.com wrote:
In particular: since the driver will ignore the KMS cursor plane position set by user-space, I don't think it's okay to just expose without opt-in from user-space (e.g. with a DRM_CLIENT_CAP).
cc wayland-devel and Pekka for user-space feedback.
I think Thomas expressed our concerns and reasons why those wouldn’t work for us back then. Is there something else you’d like to add?
I disagreed, and I don't understand Thomas' reasoning.
KMS clients will need an update to work correctly. Adding a new prop without a cap leaves existing KMS clients broken. Adding a cap allows both existing KMS clients and updated KMS clients to work correctly.
I’m not sure what you mean here. They are broken right now. That’s what we’re fixing. That’s the reason why the virtualized drivers are on deny-lists for all atomic kms. So nothing needs to be updated. If you have a kms client that was using virtualized drivers with atomic kms then mouse clicks never worked correctly.
So, yes, clients need to set cursor hotspot if they want mouse to work correctly with virtualized drivers.
My proposal was:
- Introduce DRM_CLIENT_CAP_CURSOR_PLANE_NO_POSITION (or a better name). Only
user-space which supports the hotspot props will enable it.
- By default, don't expose a cursor plane, because current user-space doesn't
support it (Weston might put a window in the cursor plane, and nobody can report hotspot).
- If the KMS client enables the cap, advertise the cursor
plane, and make it so the plane doesn't have the CRTC_X/CRTC_Y properties since the driver will pick the position.
That way both old and new user-space are fixed.
I don’t think I see how that fixes anything. In particular I don’t see a way of fixing the old user space at all. We require hotspot info, old user space doesn’t set it because there’s no way of setting it on atomic kms.
Old atomic user-space is fixed by removing the cursor plane. Then old atomic user-space will fallback to drawing the cursor itself, e.g. via OpenGL.
New user-space supports setting the hotspot prop, and is aware that it can't set the cursor plane position, so the cursor plane can be exposed again when the cap is enabled.
On Jun 3, 2022, at 11:22 AM, Simon Ser <contact@emersion.frmailto:contact@emersion.fr> wrote:
⚠ External Email
On Friday, June 3rd, 2022 at 17:17, Zack Rusin <zackr@vmware.commailto:zackr@vmware.com> wrote:
On Jun 3, 2022, at 10:56 AM, Simon Ser <contact@emersion.frmailto:contact@emersion.fr> wrote: ⚠ External Email
On Friday, June 3rd, 2022 at 16:38, Zack Rusin <zackr@vmware.commailto:zackr@vmware.com> wrote:
On Jun 3, 2022, at 10:32 AM, Simon Ser <contact@emersion.frmailto:contact@emersion.fr> wrote:
⚠ External Email
On Friday, June 3rd, 2022 at 16:27, Zack Rusin <zackr@vmware.commailto:zackr@vmware.com> wrote:
In particular: since the driver will ignore the KMS cursor plane position set by user-space, I don't think it's okay to just expose without opt-in from user-space (e.g. with a DRM_CLIENT_CAP).
cc wayland-devel and Pekka for user-space feedback.
I think Thomas expressed our concerns and reasons why those wouldn’t work for us back then. Is there something else you’d like to add?
I disagreed, and I don't understand Thomas' reasoning.
KMS clients will need an update to work correctly. Adding a new prop without a cap leaves existing KMS clients broken. Adding a cap allows both existing KMS clients and updated KMS clients to work correctly.
I’m not sure what you mean here. They are broken right now. That’s what we’re fixing. That’s the reason why the virtualized drivers are on deny-lists for all atomic kms. So nothing needs to be updated. If you have a kms client that was using virtualized drivers with atomic kms then mouse clicks never worked correctly.
So, yes, clients need to set cursor hotspot if they want mouse to work correctly with virtualized drivers.
My proposal was:
- Introduce DRM_CLIENT_CAP_CURSOR_PLANE_NO_POSITION (or a better name). Only user-space which supports the hotspot props will enable it. - By default, don't expose a cursor plane, because current user-space doesn't support it (Weston might put a window in the cursor plane, and nobody can report hotspot). - If the KMS client enables the cap, advertise the cursor plane, and make it so the plane doesn't have the CRTC_X/CRTC_Y properties since the driver will pick the position.
That way both old and new user-space are fixed.
I don’t think I see how that fixes anything. In particular I don’t see a way of fixing the old user space at all. We require hotspot info, old user space doesn’t set it because there’s no way of setting it on atomic kms.
Old atomic user-space is fixed by removing the cursor plane. Then old atomic user-space will fallback to drawing the cursor itself, e.g. via OpenGL.
But it’s not fixed, because the driver is still on a deny-list and nothing implements this. You’re saying you could potentially “fix” by implementing client side cursor handling in all kms clients? That’s a hard sell if the user space can just put the virtualized driver on deny-lists and fallback to use old kms which supports hotspots.
New user-space supports setting the hotspot prop, and is aware that it can't set the cursor plane position, so the cursor plane can be exposed again when the cap is enabled.
But we still use cursor plane position. Hotspots are offsets from cursor plane positions. Both are required.
z
On Friday, June 3rd, 2022 at 17:32, Zack Rusin zackr@vmware.com wrote:
On Jun 3, 2022, at 11:22 AM, Simon Ser contact@emersion.fr wrote: ⚠ External Email
On Friday, June 3rd, 2022 at 17:17, Zack Rusin zackr@vmware.com wrote:
On Jun 3, 2022, at 10:56 AM, Simon Ser contact@emersion.fr wrote: ⚠ External Email
On Friday, June 3rd, 2022 at 16:38, Zack Rusin zackr@vmware.com wrote:
On Jun 3, 2022, at 10:32 AM, Simon Ser contact@emersion.fr wrote:
⚠ External Email
On Friday, June 3rd, 2022 at 16:27, Zack Rusin zackr@vmware.com wrote:
> > In particular: since the driver will ignore the KMS cursor plane > > position set by user-space, I don't think it's okay to just expose > > without opt-in from user-space (e.g. with a DRM_CLIENT_CAP). > > > > cc wayland-devel and Pekka for user-space feedback. > > > I think Thomas expressed our concerns and reasons why those wouldn’t > work for us back then. Is there something else you’d like to add?
I disagreed, and I don't understand Thomas' reasoning.
KMS clients will need an update to work correctly. Adding a new prop without a cap leaves existing KMS clients broken. Adding a cap allows both existing KMS clients and updated KMS clients to work correctly.
I’m not sure what you mean here. They are broken right now. That’s what we’re fixing. That’s the reason why the virtualized drivers are on deny-lists for all atomic kms. So nothing needs to be updated. If you have a kms client that was using virtualized drivers with atomic kms then mouse clicks never worked correctly.
So, yes, clients need to set cursor hotspot if they want mouse to work correctly with virtualized drivers.
My proposal was:
- Introduce DRM_CLIENT_CAP_CURSOR_PLANE_NO_POSITION (or a better name). Only
user-space which supports the hotspot props will enable it.
- By default, don't expose a cursor plane, because current user-space doesn't
support it (Weston might put a window in the cursor plane, and nobody can report hotspot).
- If the KMS client enables the cap, advertise the cursor
plane, and make it so the plane doesn't have the CRTC_X/CRTC_Y properties since the driver will pick the position.
That way both old and new user-space are fixed.
I don’t think I see how that fixes anything. In particular I don’t see a way of fixing the old user space at all. We require hotspot info, old user space doesn’t set it because there’s no way of setting it on atomic kms.
Old atomic user-space is fixed by removing the cursor plane. Then old atomic user-space will fallback to drawing the cursor itself, e.g. via OpenGL.
But it’s not fixed, because the driver is still on a deny-list and nothing implements this. You’re saying you could potentially “fix” by implementing client side cursor handling in all kms clients? That’s a hard sell if the user space can just put the virtualized driver on deny-lists and fallback to use old kms which supports hotspots.
What deny-list are you referring to?
All compositors I know of implement a fallback when no cursor plane is usable.
New user-space supports setting the hotspot prop, and is aware that it can't set the cursor plane position, so the cursor plane can be exposed again when the cap is enabled.
But we still use cursor plane position. Hotspots are offsets from cursor plane positions. Both are required.
No, VM drivers don't need and disregard the cursor position AFAIK. They replace it with the host cursor's position.
This is what breaks Weston, breaks uAPI expectations, and IMHO is unacceptable without KMS client opt-in.
On Jun 3, 2022, at 11:49 AM, Simon Ser <contact@emersion.frmailto:contact@emersion.fr> wrote:
⚠ External Email
On Friday, June 3rd, 2022 at 17:32, Zack Rusin <zackr@vmware.commailto:zackr@vmware.com> wrote:
On Jun 3, 2022, at 11:22 AM, Simon Ser <contact@emersion.frmailto:contact@emersion.fr> wrote: ⚠ External Email
On Friday, June 3rd, 2022 at 17:17, Zack Rusin <zackr@vmware.commailto:zackr@vmware.com> wrote:
On Jun 3, 2022, at 10:56 AM, Simon Ser <contact@emersion.frmailto:contact@emersion.fr> wrote: ⚠ External Email
On Friday, June 3rd, 2022 at 16:38, Zack Rusin <zackr@vmware.commailto:zackr@vmware.com> wrote:
On Jun 3, 2022, at 10:32 AM, Simon Ser <contact@emersion.frmailto:contact@emersion.fr> wrote:
⚠ External Email
On Friday, June 3rd, 2022 at 16:27, Zack Rusin <zackr@vmware.commailto:zackr@vmware.com> wrote:
In particular: since the driver will ignore the KMS cursor plane position set by user-space, I don't think it's okay to just expose without opt-in from user-space (e.g. with a DRM_CLIENT_CAP).
cc wayland-devel and Pekka for user-space feedback.
I think Thomas expressed our concerns and reasons why those wouldn’t work for us back then. Is there something else you’d like to add?
I disagreed, and I don't understand Thomas' reasoning.
KMS clients will need an update to work correctly. Adding a new prop without a cap leaves existing KMS clients broken. Adding a cap allows both existing KMS clients and updated KMS clients to work correctly.
I’m not sure what you mean here. They are broken right now. That’s what we’re fixing. That’s the reason why the virtualized drivers are on deny-lists for all atomic kms. So nothing needs to be updated. If you have a kms client that was using virtualized drivers with atomic kms then mouse clicks never worked correctly.
So, yes, clients need to set cursor hotspot if they want mouse to work correctly with virtualized drivers.
My proposal was:
- Introduce DRM_CLIENT_CAP_CURSOR_PLANE_NO_POSITION (or a better name). Only user-space which supports the hotspot props will enable it. - By default, don't expose a cursor plane, because current user-space doesn't support it (Weston might put a window in the cursor plane, and nobody can report hotspot). - If the KMS client enables the cap, advertise the cursor plane, and make it so the plane doesn't have the CRTC_X/CRTC_Y properties since the driver will pick the position.
That way both old and new user-space are fixed.
I don’t think I see how that fixes anything. In particular I don’t see a way of fixing the old user space at all. We require hotspot info, old user space doesn’t set it because there’s no way of setting it on atomic kms.
Old atomic user-space is fixed by removing the cursor plane. Then old atomic user-space will fallback to drawing the cursor itself, e.g. via OpenGL.
But it’s not fixed, because the driver is still on a deny-list and nothing implements this. You’re saying you could potentially “fix” by implementing client side cursor handling in all kms clients? That’s a hard sell if the user space can just put the virtualized driver on deny-lists and fallback to use old kms which supports hotspots.
What deny-list are you referring to?
All compositors I know of implement a fallback when no cursor plane is usable.
I put the links in the first email in the series: https://gitlab.gnome.org/GNOME/mutter/-/blob/main/src/backends/native/meta-k... https://invent.kde.org/plasma/kwin/-/blob/master/src/backends/drm/drm_gpu.cp...
Also, let me point this out because it also seems to be a fundamental misunderstanding, user space implementing software cursor doesn’t fix anything. Just leaves everything broken in different ways. The reason virtualized drivers went away from software cursors is because it makes it impossible to make it work with a bunch of interesting and desirable scenarios, e.g. unity mode where the guest might have a terminal and browser open and then the virtual machine software creates windows out of those, so you don’t have the entire desktop of the guest but instead native looking windows with the apps. In that case guest has no way of knowing when the cursor leaves the window, so to make seemless cursors work you’d need to implement irc or backdoors that send that information from the host to the guest, then have virtualized drivers create some sort of uevent, to send to the userspace to let it know to hide the cursor because it actually left the window. That’s a trivial scenario and there’s a lot more with mice that are remoted themselves, guests with singular or maybe many apps exported, possibly overlapping on the host but all within a desktop in the guest, etc.
New user-space supports setting the hotspot prop, and is aware that it can't set the cursor plane position, so the cursor plane can be exposed again when the cap is enabled.
But we still use cursor plane position. Hotspots are offsets from cursor plane positions. Both are required.
No, VM drivers don't need and disregard the cursor position AFAIK. They replace it with the host cursor's position.
Iirc they all use it. Unless we have some miscommunication about crtc_x|y vs src_x|y. The latter isn’t used but the former is. I guess we could repurpose src_x|y to mean mouse hotspots and write patches for userspace to use that instead of explicit properties, but I don’t think that’s in any way better or cleaner than having explicit hotspot properties at this point.
z
On Friday, June 3rd, 2022 at 20:31, Zack Rusin zackr@vmware.com wrote:
My proposal was:
- Introduce DRM_CLIENT_CAP_CURSOR_PLANE_NO_POSITION (or a better name). Only
user-space which supports the hotspot props will enable it.
- By default, don't expose a cursor plane, because current user-space doesn't
support it (Weston might put a window in the cursor plane, and nobody can report hotspot).
- If the KMS client enables the cap, advertise the cursor
plane, and make it so the plane doesn't have the CRTC_X/CRTC_Y properties since the driver will pick the position.
That way both old and new user-space are fixed.
I don’t think I see how that fixes anything. In particular I don’t see a way of fixing the old user space at all. We require hotspot info, old user space doesn’t set it because there’s no way of setting it on atomic kms.
Old atomic user-space is fixed by removing the cursor plane. Then old atomic user-space will fallback to drawing the cursor itself, e.g. via OpenGL.
But it’s not fixed, because the driver is still on a deny-list and nothing implements this. You’re saying you could potentially “fix” by implementing client side cursor handling in all kms clients? That’s a hard sell if the user space can just put the virtualized driver on deny-lists and fallback to use old kms which supports hotspots.
What deny-list are you referring to?
All compositors I know of implement a fallback when no cursor plane is usable.
I put the links in the first email in the series: https://gitlab.gnome.org/GNOME/mutter/-/blob/main/src/backends/native/meta-k... https://invent.kde.org/plasma/kwin/-/blob/master/src/backends/drm/drm_gpu.cp...
I was not aware of these lists. Having to work-around drivers in user-space is really not great.
But regardless, that doesn't really change anything. With my proposal:
- Old user-space with hacky deny-lists (Mutter, KWin) still goes through the legacy uAPI. - Old user-space without the hacky deny-lists (Weston, wlroots) uses software cursors and is not broken anymore. - New user-space can report hotspot and is not broken.
Also, let me point this out because it also seems to be a fundamental misunderstanding, user space implementing software cursor doesn’t fix anything. Just leaves everything broken in different ways. The reason virtualized drivers went away from software cursors is because it makes it impossible to make it work with a bunch of interesting and desirable scenarios, e.g. unity mode where the guest might have a terminal and browser open and then the virtual machine software creates windows out of those, so you don’t have the entire desktop of the guest but instead native looking windows with the apps. In that case guest has no way of knowing when the cursor leaves the window, so to make seemless cursors work you’d need to implement irc or backdoors that send that information from the host to the guest, then have virtualized drivers create some sort of uevent, to send to the userspace to let it know to hide the cursor because it actually left the window. That’s a trivial scenario and there’s a lot more with mice that are remoted themselves, guests with singular or maybe many apps exported, possibly overlapping on the host but all within a desktop in the guest, etc.
Are you saying that the current broken behavior is better than software cursors?
New user-space supports setting the hotspot prop, and is aware that it can't set the cursor plane position, so the cursor plane can be exposed again when the cap is enabled.
But we still use cursor plane position. Hotspots are offsets from cursor plane positions. Both are required.
No, VM drivers don't need and disregard the cursor position AFAIK. They replace it with the host cursor's position.
Iirc they all use it.
What do they use it for?
The whole point of this discussion is to be able to display the guest's cursor image on the host's cursor, so that latency is reduced? When the VM software does this, why does it need to use CRTC_X/CRTC_Y, since these are thrown away by the host?
I guess we could repurpose src_x|y to mean mouse hotspots and write patches for userspace to use that instead of explicit properties
That sounds like a terrible idea.
On Jun 5, 2022, at 3:30 AM, Simon Ser contact@emersion.fr wrote:
⚠ External Email
On Friday, June 3rd, 2022 at 20:31, Zack Rusin zackr@vmware.com wrote:
> My proposal was: > > - Introduce DRM_CLIENT_CAP_CURSOR_PLANE_NO_POSITION (or a better name). Only > user-space which supports the hotspot props will enable it. > - By default, don't expose a cursor plane, because current user-space doesn't > support it (Weston might put a window in the cursor plane, and nobody can > report hotspot). > - If the KMS client enables the cap, advertise the cursor > plane, and make it so the plane doesn't have the CRTC_X/CRTC_Y properties > since the driver will pick the position. > > That way both old and new user-space are fixed.
I don’t think I see how that fixes anything. In particular I don’t see a way of fixing the old user space at all. We require hotspot info, old user space doesn’t set it because there’s no way of setting it on atomic kms.
Old atomic user-space is fixed by removing the cursor plane. Then old atomic user-space will fallback to drawing the cursor itself, e.g. via OpenGL.
But it’s not fixed, because the driver is still on a deny-list and nothing implements this. You’re saying you could potentially “fix” by implementing client side cursor handling in all kms clients? That’s a hard sell if the user space can just put the virtualized driver on deny-lists and fallback to use old kms which supports hotspots.
What deny-list are you referring to?
All compositors I know of implement a fallback when no cursor plane is usable.
I put the links in the first email in the series: https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.gno... https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Finvent.kde...
I was not aware of these lists. Having to work-around drivers in user-space is really not great.
But regardless, that doesn't really change anything. With my proposal:
- Old user-space with hacky deny-lists (Mutter, KWin) still goes through the
legacy uAPI.
- Old user-space without the hacky deny-lists (Weston, wlroots) uses software
cursors and is not broken anymore.
- New user-space can report hotspot and is not broken.
Yea, that doesn’t work, but I think below I stopped where the issue is.
Also, let me point this out because it also seems to be a fundamental misunderstanding, user space implementing software cursor doesn’t fix anything. Just leaves everything broken in different ways. The reason virtualized drivers went away from software cursors is because it makes it impossible to make it work with a bunch of interesting and desirable scenarios, e.g. unity mode where the guest might have a terminal and browser open and then the virtual machine software creates windows out of those, so you don’t have the entire desktop of the guest but instead native looking windows with the apps. In that case guest has no way of knowing when the cursor leaves the window, so to make seemless cursors work you’d need to implement irc or backdoors that send that information from the host to the guest, then have virtualized drivers create some sort of uevent, to send to the userspace to let it know to hide the cursor because it actually left the window. That’s a trivial scenario and there’s a lot more with mice that are remoted themselves, guests with singular or maybe many apps exported, possibly overlapping on the host but all within a desktop in the guest, etc.
Are you saying that the current broken behavior is better than software cursors?
They’re broken in very different ways. You’re asking me which bugs do I prefer. Ultimately the current lack of hotspots leaves mouse unusable but I don’t see any reason to trade that for another set of bugs instead of just fixing it (either via fallback to legacy or using the new hotspot api).
New user-space supports setting the hotspot prop, and is aware that it can't set the cursor plane position, so the cursor plane can be exposed again when the cap is enabled.
But we still use cursor plane position. Hotspots are offsets from cursor plane positions. Both are required.
No, VM drivers don't need and disregard the cursor position AFAIK. They replace it with the host cursor's position.
Iirc they all use it.
What do they use it for?
The whole point of this discussion is to be able to display the guest's cursor image on the host's cursor, so that latency is reduced?
Ah, I think I see now where the confusion is coming from. No, it’s definitely not. This has nothing to do with latency. By default position of a mouse cursor doesn’t tell us where the point that is actually used as an activation of a click is, e.g. a mouse cursor image with an arrow pointing to the top-left and a mouse cursor image pointing to the bottom right - both at the same position, as you can imagine it will become impossible to use the desktop if the click defaults to the top left, especially as the number of cursor images increases, you need information about which point within the cursor image activates the click, that’s the hotspot. You need to know the position of the image and you need to know which point within that image is responsible for actually activating the events.
So this has nothing to do with latency, this is about mouse cursor clicks/drags simply having wrong coordinates and mouse being effectively impossible to use on anything that doesn’t have the same workarounds as gnome-shell/kwin. So if you have user space code which hasn’t implemented the same workarounds gnome-shell/kwin that means that is has never been used with virtualized drivers because people tend to notice pretty quickly that when they click on a button/link a completely different button/link in a different part of the screen is activated...
z
------- Original Message ------- On Sunday, June 5th, 2022 at 17:47, Zack Rusin zackr@vmware.com wrote:
Also, let me point this out because it also seems to be a fundamental misunderstanding, user space implementing software cursor doesn’t fix anything. Just leaves everything broken in different ways. The reason virtualized drivers went away from software cursors is because it makes it impossible to make it work with a bunch of interesting and desirable scenarios, e.g. unity mode where the guest might have a terminal and browser open and then the virtual machine software creates windows out of those, so you don’t have the entire desktop of the guest but instead native looking windows with the apps. In that case guest has no way of knowing when the cursor leaves the window, so to make seemless cursors work you’d need to implement irc or backdoors that send that information from the host to the guest, then have virtualized drivers create some sort of uevent, to send to the userspace to let it know to hide the cursor because it actually left the window. That’s a trivial scenario and there’s a lot more with mice that are remoted themselves, guests with singular or maybe many apps exported, possibly overlapping on the host but all within a desktop in the guest, etc.
Are you saying that the current broken behavior is better than software cursors?
They’re broken in very different ways. You’re asking me which bugs do I prefer. Ultimately the current lack of hotspots leaves mouse unusable but I don’t see any reason to trade that for another set of bugs instead of just fixing it (either via fallback to legacy or using the new hotspot api).
Software cursors aren't a bug. They're a fallback.
New user-space supports setting the hotspot prop, and is aware that it can't set the cursor plane position, so the cursor plane can be exposed again when the cap is enabled.
But we still use cursor plane position. Hotspots are offsets from cursor plane positions. Both are required.
No, VM drivers don't need and disregard the cursor position AFAIK. They replace it with the host cursor's position.
Iirc they all use it.
What do they use it for?
The whole point of this discussion is to be able to display the guest's cursor image on the host's cursor, so that latency is reduced?
Ah, I think I see now where the confusion is coming from. No, it’s definitely not. This has nothing to do with latency. By default position of a mouse cursor doesn’t tell us where the point that is actually used as an activation of a click is, e.g. a mouse cursor image with an arrow pointing to the top-left and a mouse cursor image pointing to the bottom right - both at the same position, as you can imagine it will become impossible to use the desktop if the click defaults to the top left, especially as the number of cursor images increases, you need information about which point within the cursor image activates the click, that’s the hotspot. You need to know the position of the image and you need to know which point within that image is responsible for actually activating the events.
Yeah, that's what a hotspot is.
By "the whole point of this discussion", I meant that the whole point for VM drivers to expose a hardware cursor is to improve latency (and provide other related features).
At any rate, I consider broken any driver which exposes a cursor plane, then doesn't display it exactly at the CRTC_X/CRTC_Y or misbehaves if it's missing hotspot info.
On Jun 5, 2022, at 12:26 PM, Simon Ser contact@emersion.fr wrote:
------- Original Message ------- On Sunday, June 5th, 2022 at 17:47, Zack Rusin zackr@vmware.com wrote:
Also, let me point this out because it also seems to be a fundamental misunderstanding, user space implementing software cursor doesn’t fix anything. Just leaves everything broken in different ways. The reason virtualized drivers went away from software cursors is because it makes it impossible to make it work with a bunch of interesting and desirable scenarios, e.g. unity mode where the guest might have a terminal and browser open and then the virtual machine software creates windows out of those, so you don’t have the entire desktop of the guest but instead native looking windows with the apps. In that case guest has no way of knowing when the cursor leaves the window, so to make seemless cursors work you’d need to implement irc or backdoors that send that information from the host to the guest, then have virtualized drivers create some sort of uevent, to send to the userspace to let it know to hide the cursor because it actually left the window. That’s a trivial scenario and there’s a lot more with mice that are remoted themselves, guests with singular or maybe many apps exported, possibly overlapping on the host but all within a desktop in the guest, etc.
Are you saying that the current broken behavior is better than software cursors?
They’re broken in very different ways. You’re asking me which bugs do I prefer. Ultimately the current lack of hotspots leaves mouse unusable but I don’t see any reason to trade that for another set of bugs instead of just fixing it (either via fallback to legacy or using the new hotspot api).
Software cursors aren't a bug. They're a fallback.
They work well, or usually well enough, on native but not with virtual machines. They break a lot of features. The nature of virtualization is that the guest doesn’t explicitly get access to a lot of host side info. If you move to software cursor you automatically lose all that info (unless, as I mentioned before, you rewrite hypervisors to push all that info to guests again, either via backdoors, irq or register accesses). Software cursor just doesn’t work with modern hypervisors well enough to be a reasonable replacement.
> New user-space supports setting the hotspot prop, and is aware that it can't > set the cursor plane position, so the cursor plane can be exposed again when > the cap is enabled.
But we still use cursor plane position. Hotspots are offsets from cursor plane positions. Both are required.
No, VM drivers don't need and disregard the cursor position AFAIK. They replace it with the host cursor's position.
Iirc they all use it.
What do they use it for?
The whole point of this discussion is to be able to display the guest's cursor image on the host's cursor, so that latency is reduced?
Ah, I think I see now where the confusion is coming from. No, it’s definitely not. This has nothing to do with latency. By default position of a mouse cursor doesn’t tell us where the point that is actually used as an activation of a click is, e.g. a mouse cursor image with an arrow pointing to the top-left and a mouse cursor image pointing to the bottom right - both at the same position, as you can imagine it will become impossible to use the desktop if the click defaults to the top left, especially as the number of cursor images increases, you need information about which point within the cursor image activates the click, that’s the hotspot. You need to know the position of the image and you need to know which point within that image is responsible for actually activating the events.
Yeah, that's what a hotspot is.
By "the whole point of this discussion", I meant that the whole point for VM drivers to expose a hardware cursor is to improve latency (and provide other related features).
At any rate, I consider broken any driver which exposes a cursor plane, then doesn't display it exactly at the CRTC_X/CRTC_Y
But we do… The cursor is at crtc_x, crtc_y.
or misbehaves if it's missing hotspot info.
That doesn’t follow at all. How can they not behave incorrectly if the information is missing?
Maybe we can refocus the discussion because it seems like we’re moving in circles. Your proposal for a feature cap and removal of the cursor plane doesn’t fix anything: - current user space which doesn’t fallback to legacy kms with virtualized driver is completely broken and obviously will always remain broken on all already released kernels - making the same software fallback to software cursor breaks a massive amount of interactions in VMs so what are you proposing?
z
On Sunday, June 5th, 2022 at 20:16, Zack Rusin zackr@vmware.com wrote:
At any rate, I consider broken any driver which exposes a cursor plane, then doesn't display it exactly at the CRTC_X/CRTC_Y
But we do… The cursor is at crtc_x, crtc_y.
How do you show the cursor on the host side then? Pretty sure you use the host X11/Wayland cursor? In which case nothing guarantees that the host cursor position matches CRTC_X/CRTC_Y.
Hi,
On 6/3/22 17:22, Simon Ser wrote:
On Friday, June 3rd, 2022 at 17:17, Zack Rusin zackr@vmware.com wrote:
On Jun 3, 2022, at 10:56 AM, Simon Ser contact@emersion.fr wrote: ⚠ External Email
On Friday, June 3rd, 2022 at 16:38, Zack Rusin zackr@vmware.com wrote:
On Jun 3, 2022, at 10:32 AM, Simon Ser contact@emersion.fr wrote:
⚠ External Email
On Friday, June 3rd, 2022 at 16:27, Zack Rusin zackr@vmware.com wrote:
> In particular: since the driver will ignore the KMS cursor plane > position set by user-space, I don't think it's okay to just expose > without opt-in from user-space (e.g. with a DRM_CLIENT_CAP). > > cc wayland-devel and Pekka for user-space feedback.
I think Thomas expressed our concerns and reasons why those wouldn’t work for us back then. Is there something else you’d like to add?
I disagreed, and I don't understand Thomas' reasoning.
KMS clients will need an update to work correctly. Adding a new prop without a cap leaves existing KMS clients broken. Adding a cap allows both existing KMS clients and updated KMS clients to work correctly.
I’m not sure what you mean here. They are broken right now. That’s what we’re fixing. That’s the reason why the virtualized drivers are on deny-lists for all atomic kms. So nothing needs to be updated. If you have a kms client that was using virtualized drivers with atomic kms then mouse clicks never worked correctly.
So, yes, clients need to set cursor hotspot if they want mouse to work correctly with virtualized drivers.
My proposal was:
- Introduce DRM_CLIENT_CAP_CURSOR_PLANE_NO_POSITION (or a better name). Only
user-space which supports the hotspot props will enable it.
- By default, don't expose a cursor plane, because current user-space doesn't
support it (Weston might put a window in the cursor plane, and nobody can report hotspot).
- If the KMS client enables the cap, advertise the cursor
plane, and make it so the plane doesn't have the CRTC_X/CRTC_Y properties since the driver will pick the position.
That way both old and new user-space are fixed.
I don’t think I see how that fixes anything. In particular I don’t see a way of fixing the old user space at all. We require hotspot info, old user space doesn’t set it because there’s no way of setting it on atomic kms.
Old atomic user-space is fixed by removing the cursor plane. Then old atomic user-space will fallback to drawing the cursor itself, e.g. via OpenGL.
That is just a terrible idea, the current situation is that userspace has a hardcoded list of drivers which need hotspots, so it uses the old non-atomic APIs on that "hw" since the atomic APIs don't support hotspots.
The downsides I see to your proposal are:
1. Falling back to sw cursor rendering instead of using the old APIs would be a pretty significant regression in user experience. I know that in theory sw rendering can be made to not flicker, but in practice it tends to be a flickery mess.
2. It does not help anything since userspace is still hardcoded to use the old, hotspot aware non-atomic API anyways. So it would only make things worse since hiding the cursorplane for userspace which does not set the CAP would mean the the old non-atomic API also stops working. Or this would add extra complications in the kernel to still keep the old APIs working.
I do think that a CAP might be a good idea, but the disabling of the cursor plane based on the CAP does not sound like a good idea to me.
Regards,
Hans
On Saturday, June 4th, 2022 at 23:19, Hans de Goede hdegoede@redhat.com wrote:
My proposal was:
- Introduce DRM_CLIENT_CAP_CURSOR_PLANE_NO_POSITION (or a better name). Only
user-space which supports the hotspot props will enable it.
- By default, don't expose a cursor plane, because current user-space doesn't
support it (Weston might put a window in the cursor plane, and nobody can report hotspot).
- If the KMS client enables the cap, advertise the cursor
plane, and make it so the plane doesn't have the CRTC_X/CRTC_Y properties since the driver will pick the position.
That way both old and new user-space are fixed.
I don’t think I see how that fixes anything. In particular I don’t see a way of fixing the old user space at all. We require hotspot info, old user space doesn’t set it because there’s no way of setting it on atomic kms.
Old atomic user-space is fixed by removing the cursor plane. Then old atomic user-space will fallback to drawing the cursor itself, e.g. via OpenGL.
That is just a terrible idea, the current situation is that userspace has a hardcoded list of drivers which need hotspots, so it uses the old non-atomic APIs on that "hw" since the atomic APIs don't support hotspots.
*Some* user-space does this (Mutter, KWin). There is also user-space which doesn't (Weston, wlroots).
The downsides I see to your proposal are:
- Falling back to sw cursor rendering instead of using the old APIs would
be a pretty significant regression in user experience. I know that in theory sw rendering can be made to not flicker, but in practice it tends to be a flickery mess.
Old Mutter/KWin with the deny-lists cannot be updated and will still use the legacy uAPI. New Muter/KWin with support for atomic hotspot will not need a deny-list anymore. No breakage here.
- It does not help anything since userspace is still hardcoded to use
the old, hotspot aware non-atomic API anyways. So it would only make things worse since hiding the cursorplane for userspace which does not set the CAP would mean the the old non-atomic API also stops working. Or this would add extra complications in the kernel to still keep the old APIs working.
The cursor plane would only be hidden when user-space has enabled DRM_CAP_UNIVERSAL_PLANES. IOW, user-space only supporting the legacy uAPI will still work as it does today.
Hi,
I don’t think I see how that fixes anything. In particular I don’t see a way of fixing the old user space at all. We require hotspot info, old user space doesn’t set it because there’s no way of setting it on atomic kms. Whether we expose cursor plane or not doesn’t change the fact that we still require the hotspot info.
Not exposing a cursor plane at all forces swcursor, which sidesteps the hotspot issue at the expense of a rather sluggish pointer updates because those suddenly require a round-trip to the guest.
take care, Gerd
On Fri, 3 Jun 2022 14:38:54 +0000 Zack Rusin zackr@vmware.com wrote:
On Jun 3, 2022, at 10:32 AM, Simon Ser contact@emersion.fr wrote:
⚠ External Email
On Friday, June 3rd, 2022 at 16:27, Zack Rusin zackr@vmware.com wrote:
In particular: since the driver will ignore the KMS cursor plane position set by user-space, I don't think it's okay to just expose without opt-in from user-space (e.g. with a DRM_CLIENT_CAP).
cc wayland-devel and Pekka for user-space feedback.
I think Thomas expressed our concerns and reasons why those wouldn’t work for us back then. Is there something else you’d like to add?
I disagreed, and I don't understand Thomas' reasoning.
KMS clients will need an update to work correctly. Adding a new prop without a cap leaves existing KMS clients broken. Adding a cap allows both existing KMS clients and updated KMS clients to work correctly.
I’m not sure what you mean here. They are broken right now. That’s what we’re fixing. That’s the reason why the virtualized drivers are on deny-lists for all atomic kms. So nothing needs to be updated. If
Hi Zack,
please, stop removing all email quoting level indicators, you have been doing that a lot in your more recent replies. It makes reading the emails really difficult, and I had to stop trying to make sense of the latest emails.
It is really unfortunate that display servers have driver deny-lists indeed. All the bug reports they got from being broken should have been denied and forwarded to the respective drivers instead for breaking the KMS UAPI. OTOH, I understand that some userspace projects do not want to stop because of few broken but popular drivers.
you have a kms client that was using virtualized drivers with atomic kms then mouse clicks never worked correctly. So, yes, clients need to set cursor hotspot if they want mouse to work correctly with virtualized drivers.
I'm very much agreeing with Simon. He has made similar questions and comments that occurred to me.
Reading as much of the discussion between Simon and Zack as I could, it seems every time Simon gets to the point, Zack hops to a completely different use case to make Simon's argument no longer apply.
Like, root-window-less per-window remoting through KMS? How is that even possible?
On Thursday, June 2nd, 2022 at 17:42, Zack Rusin zack@kde.org wrote:
- all userspace code needs to hardcore a list of drivers which require
hotspots because there's no way to query from drm "does this driver require hotspot"
Can you elaborate? I'm not sure I understand what you mean here.
Only some drivers require informations about cursor hotspot, user space currently has no way of figuring out which ones are those, i.e. amdgpu doesn’t care about hotspots, qxl does so when running on qxl without properly set cursor hotspot atomic kms will result in a desktop where mouse clicks have incorrect coordinates.
There’s no way to differentiate between drivers that do not care about cursor hotspots and ones that absolutely require it.
Only VM drivers should expose the hotspot properties. Real drivers like amdgpu must not expose it.
Yes, that’s what I said. There’s no way to differentiate between amdgpu that doesn’t have those properties and virtio_gpu driver from a kernel before hotspot properties were added.
Why would userspace want to tell the difference between a driver that truly does not need those properties, and a driver that did not add those properties *yet*?
Thanks, pq
On Fri, 03 Jun 2022 14:14:59 +0000 Simon Ser contact@emersion.fr wrote:
Hi,
Please, read this thread: https://lists.freedesktop.org/archives/dri-devel/2020-March/thread.html#2596...
It has a lot of information about the pitfalls of cursor hotspot and other things done by VM software.
In particular: since the driver will ignore the KMS cursor plane position set by user-space, I don't think it's okay to just expose without opt-in from user-space (e.g. with a DRM_CLIENT_CAP).
cc wayland-devel and Pekka for user-space feedback.
On Thursday, June 2nd, 2022 at 17:42, Zack Rusin zack@kde.org wrote:
- all userspace code needs to hardcore a list of drivers which require
hotspots because there's no way to query from drm "does this driver require hotspot"
Can you elaborate? I'm not sure I understand what you mean here.
Hi Zack and everyone,
I would like to try to reboot this discussion and explain where I come from. Maybe I have misunderstood something.
I have no fundamental objection to adding KMS properties for cursor hotspot, but I do want to make sure the design is fully thought out and not simply copied from legacy KMS, because atomic KMS with universal planes is not like legacy KMS.
To my understanding from the past discussions, the fundamental reason why you are proposing hotspot properties is that when one is running a desktop inside a VM and looking at it through a VM viewer application, the pointer cursor is misplaced: the hotspot is not where the end user sees it. This you never mentioned in any of the patches nor in the cover letter. I can only guess that this misplacement could be the reason why some display servers have deny-listed paravirtualized drivers. While your goal is to get paravirtualized drivers out of the deny-lists, we must first understand why they got there in the first place.
Why are pointer cursors misplaced on paravirtualized drivers?
It is because the paravirtualized drivers or VM viewers do *not* place the cursor plane at the CRTC_X, CRTC_Y position in the guest CRTC area. This is obvious: if CRTC_X, CRTC_Y were honoured, there would be no misplacement.
Instead, the VM stack plays clever tricks with cursor planes. I have understood only one of those tricks, and it goes something like this. To improve hand-eye coordination, that is to reduce the hand-to-eye response time a.k.a latency, the VM guest KMS driver relays the cursor plane separately to the VM viewer application. The VM viewer application presents the cursor plane content by pushing them to the host window system as the pointer cursor. This means the host window system will be autonomously moving the cursor plane image around, completely disregarding what the guest KMS client programmed into CRTC_X, CRTC_Y. When this works, it is a huge improvement in user experience. I believe this is called "seamless pointer" or such.
When it doesn't work, the cursor is misplaced or even completely arbitrary guest windows start flying around as if they were the pointer cursor.
In legacy KMS, cursors have always been very special and had their own special UAPI with even hotspot information. There paravirtualized drivers got away with these clever tricks because no-one bothered putting anything but actual cursor images on the (one) cursor plane.
Then comes KMS universal planes concept. All planes are assumed roughly equal, apart from what KMS properties tell userspace about them. The plane type primary/overlay/cursor is still kept, but only because it helps userspace find a KMS configuration that the driver accepts at all. Hardware may not be able to light up a CRTC without at least one primary plane, for example. Atomic KMS requires universal planes.
The atomic KMS UAPI contract says, that a plane is positioned at CRTC_X, CRTC_Y inside the respective CRTC. I do not know of any documented exceptions to this. Therefore, an atomic driver that does not show a cursor plane at the programmed CRTC_X, CRTC_Y is violating the UAPI contract.
See https://www.kernel.org/doc/html/latest/gpu/drm-kms.html#standard-plane-prope... and how "cursor" plane type makes no exceptions, and how CRTC_X and CRTC_Y are defined. Also note that all hardware drivers and VKMS apparently follow the contract.
Given this UAPI contract, it is very easy for userspace to make the conclusion that a cursor plane is just another plane it can use for whatever it wants. Weston and wlroots indeed leverage this, putting also normal windows and other stuff to the cursor plane when they happen to fit.
If a paravirtualized driver commandeers the cursor plane display position, a possible result is that arbitrary windows start flying around unexpectedly.
Therefore we have two problems:
- paravirtualized drivers commandeering the cursor plane - VM software not able to implement "seamless pointer" correctly
To my understanding, this patch series concerns only the latter problem but not the former problem. I believe the solutions to both are related and need to be considered together.
How do we stop paravirtualized drivers from commandeering their cursor plane when guest userspace does not expect it?
How do we still make "seamless pointer" possible when the guest userspace is prepared for cursor plane commandeering?
These are the questions that need answers. To me, getting paravirtualized drivers off display server deny-lists is a consequence, a secondary goal. The primary goal must be to fix why the drivers ended up in deny-lists in the first place (and I am only assuming that this is he reason, so maybe there are other reasons?).
I believe the solution has two parts:
- The guest KMS driver needs to know whether the guest userspace is prepared for the cursor plane being commandeered. If userspace does not indicate it is prepared for it, commandeering must not happen.
- Cursor hotspot needs new KMS properties, and a KMS client must set them if the KMS client indicates it is prepared for cursor plane commandeering.
How to exactly do those can be discussed after we can agree on what problems we are solving.
There are further problems with cursor plane commandeering. The 2020 email thread Simon linked to discusses the problem of pointer devices: if VM guest userspace takes pointer input from multiple sources, how will the VM stack know which virtual input device, if any, should drive the cursor plane position?
To me the answer to this question seems it could be intimately tied to the first problem: commandeering the cursor plane is allowed only if guest userspace tells the guest KMS driver which input device the cursor plane shall be related to. If no input device is indicated, then commandeering must not happen. I can understand if people do not want to tackle this question, because it probably has not been a problem yet. Ignoring it would be unfortunate though, because that would seem to imply that VM software still needs to keep some heuristics for when commandeering the cursor plane is desired.
I could also talk about multiple cursors, but I guess that goes really too far.
Which root problems do you want to solve exactly?
Thanks, pq
Why are pointer cursors misplaced on paravirtualized drivers?
It is because the paravirtualized drivers or VM viewers do *not* place the cursor plane at the CRTC_X, CRTC_Y position in the guest CRTC area. This is obvious: if CRTC_X, CRTC_Y were honoured, there would be no misplacement.
Instead, the VM stack plays clever tricks with cursor planes. I have understood only one of those tricks, and it goes something like this. To improve hand-eye coordination, that is to reduce the hand-to-eye response time a.k.a latency, the VM guest KMS driver relays the cursor plane separately to the VM viewer application.
Yes, the cursor is sent separately.
The VM viewer application presents the cursor plane content by pushing them to the host window system as the pointer cursor.
Yes (i.e. gdk_window_set_cursor() on the host).
This means the host window system will be autonomously moving the cursor plane image around, completely disregarding what the guest KMS client programmed into CRTC_X, CRTC_Y.
Yes.
That is combined with a virtual input device sending absolute coordinates (i.e. tablet), so mouse clicks land at the correct place. And that is the point where having the hotspot information is essential on the host side.
Given this UAPI contract, it is very easy for userspace to make the conclusion that a cursor plane is just another plane it can use for whatever it wants. Weston and wlroots indeed leverage this, putting also normal windows and other stuff to the cursor plane when they happen to fit.
virtual machine display devices typically offer small (64x64) cursor planes, so unlike the 512x512 planes I've seen offered by i915 they are hardly usable for anything but cursors. Likewise additional overlay planes typically not offered, so the classic primary+cursor setup is pretty much the only reasonable option userspace has.
I believe the solution has two parts:
- The guest KMS driver needs to know whether the guest userspace is prepared for the cursor plane being commandeered. If userspace does not indicate it is prepared for it, commandeering must not happen.
Yes. That isn't much of a problem in practice though due to the limited driver/device offerings outlined above.
- Cursor hotspot needs new KMS properties, and a KMS client must set them if the KMS client indicates it is prepared for cursor plane commandeering.
Yes, and that is what hurts in practice and thus caused the blacklists being created.
There are further problems with cursor plane commandeering. The 2020 email thread Simon linked to discusses the problem of pointer devices: if VM guest userspace takes pointer input from multiple sources, how will the VM stack know which virtual input device, if any, should drive the cursor plane position?
Typically there is a communication path from guest to host for pointer movements (i.e. crtc_x + crtc_y updates), so the host knows where the guest wants the cursor plane being placed. So in case the pointer is moved by other means (different input device, some application warping the pointer, ...) the host can adapt.
Nevertheless behavior is not consistent here because in some cases the feedback loop is not wired up end-to-end. The spice protocol has a message type for that, so pointer warps work. The vnc protocol has not, so they don't.
To me the answer to this question seems it could be intimately tied to the first problem: commandeering the cursor plane is allowed only if guest userspace tells the guest KMS driver which input device the cursor plane shall be related to. If no input device is indicated, then commandeering must not happen.
Why require an input device? I just don't see how that would help.
For allowing the host freely move around the cursor place ("commandeering") I do see the point in enforcing that from a design point of view, although I doubt it'll buy us much in practice given we have broken drivers in the wild so userspace will continue to work with blacklists.
Having some capability to negotiate "commandeering" between kernel and userspace certainly makes sense, so we can get of the black lists long-term (although it'll probably takes a few years ...).
I can understand if people do not want to tackle this question, because it probably has not been a problem yet.
On a standard guest this isn't a problem indeed because there is only one input device and only one crtc.
It actually is a problem for multihead configurations though. Having some way to map input devices to scanouts would actually be helpful. Years ago I checked how this works for touchscreens to see whenever it is possible to leverage that for VMs somehow. There wasn't some obvious way, and I forgot the details meanwhile ...
take care, Gerd
On Tue, 7 Jun 2022 16:30:23 +0200 Gerd Hoffmann kraxel@redhat.com wrote:
Why are pointer cursors misplaced on paravirtualized drivers?
It is because the paravirtualized drivers or VM viewers do *not* place the cursor plane at the CRTC_X, CRTC_Y position in the guest CRTC area. This is obvious: if CRTC_X, CRTC_Y were honoured, there would be no misplacement.
Instead, the VM stack plays clever tricks with cursor planes. I have understood only one of those tricks, and it goes something like this. To improve hand-eye coordination, that is to reduce the hand-to-eye response time a.k.a latency, the VM guest KMS driver relays the cursor plane separately to the VM viewer application.
Yes, the cursor is sent separately.
The VM viewer application presents the cursor plane content by pushing them to the host window system as the pointer cursor.
Yes (i.e. gdk_window_set_cursor() on the host).
This means the host window system will be autonomously moving the cursor plane image around, completely disregarding what the guest KMS client programmed into CRTC_X, CRTC_Y.
Yes.
That is combined with a virtual input device sending absolute coordinates (i.e. tablet), so mouse clicks land at the correct place. And that is the point where having the hotspot information is essential on the host side.
Hi Gerd,
thanks for confirming.
Given this UAPI contract, it is very easy for userspace to make the conclusion that a cursor plane is just another plane it can use for whatever it wants. Weston and wlroots indeed leverage this, putting also normal windows and other stuff to the cursor plane when they happen to fit.
virtual machine display devices typically offer small (64x64) cursor planes, so unlike the 512x512 planes I've seen offered by i915 they are hardly usable for anything but cursors. Likewise additional overlay planes typically not offered, so the classic primary+cursor setup is pretty much the only reasonable option userspace has.
weston-simple-shm is 256x256, and has been demonstrated to go flying in e.g. vmware environments: https://oftc.irclog.whitequark.org/dri-devel/2022-06-06#30987017;
If KMS exposes planes, then userspace will try hard to make use of them as much as possible. It's not unimaginable that there could also be some small icon generated by the window system overlaying an application window. That might fit a tiny cursor plane perfectly.
I believe the solution has two parts:
- The guest KMS driver needs to know whether the guest userspace is prepared for the cursor plane being commandeered. If userspace does not indicate it is prepared for it, commandeering must not happen.
Yes. That isn't much of a problem in practice though due to the limited driver/device offerings outlined above.
- Cursor hotspot needs new KMS properties, and a KMS client must set them if the KMS client indicates it is prepared for cursor plane commandeering.
Yes, and that is what hurts in practice and thus caused the blacklists being created.
There are further problems with cursor plane commandeering. The 2020 email thread Simon linked to discusses the problem of pointer devices: if VM guest userspace takes pointer input from multiple sources, how will the VM stack know which virtual input device, if any, should drive the cursor plane position?
Typically there is a communication path from guest to host for pointer movements (i.e. crtc_x + crtc_y updates), so the host knows where the guest wants the cursor plane being placed. So in case the pointer is moved by other means (different input device, some application warping the pointer, ...) the host can adapt.
Would it not be better to be explicit about it? To avoid fragile heuristics.
Nevertheless behavior is not consistent here because in some cases the feedback loop is not wired up end-to-end. The spice protocol has a message type for that, so pointer warps work. The vnc protocol has not, so they don't.
To me the answer to this question seems it could be intimately tied to the first problem: commandeering the cursor plane is allowed only if guest userspace tells the guest KMS driver which input device the cursor plane shall be related to. If no input device is indicated, then commandeering must not happen.
Why require an input device? I just don't see how that would help.
For allowing the host freely move around the cursor place ("commandeering") I do see the point in enforcing that from a design point of view, although I doubt it'll buy us much in practice given we have broken drivers in the wild so userspace will continue to work with blacklists.
Having some capability to negotiate "commandeering" between kernel and userspace certainly makes sense, so we can get of the black lists long-term (although it'll probably takes a few years ...).
Yes, there is no quick solution that I can imagine. Propagating the fixes takes time. I don't think the deny-lists will ever be completely removed, because people may run old or LTS kernels which won't be getting new UAPI I presume. The only thing I can imagine happening is that the deny-lists get overridden if the userspace software detects kernel support for the new negotiation UAPI. Then the negotiation UAPI takes precedence and the deny-list becomes ineffective.
I can understand if people do not want to tackle this question, because it probably has not been a problem yet.
On a standard guest this isn't a problem indeed because there is only one input device and only one crtc.
It actually is a problem for multihead configurations though. Having some way to map input devices to scanouts would actually be helpful. Years ago I checked how this works for touchscreens to see whenever it is possible to leverage that for VMs somehow. There wasn't some obvious way, and I forgot the details meanwhile ...
Ah, that's the other way around, right? To tell guest OS which output an absolute input device is relative to?
For bare hardware touchscreens we have some vague convention of using udev device properties to tag an input device with an output name.
The first attempt at it was libinput_device_get_output_name(): https://wayland.freedesktop.org/libinput/doc/latest/api/group__device.html#g... But using it is discouraged because of being too vaguely defined what the value is. Weston uses the discouraged API still, and I'm not aware of any better standard having been developed.
Having a standard for naming outputs is hard it seems, and there is also the connector vs. monitor dilemma. I guess absolute input devices would usually want to be associated with the (real or virtual) monitor regardless of which (real or virtual) connector it is connected to.
Thanks, pq
Hi,
Typically there is a communication path from guest to host for pointer movements (i.e. crtc_x + crtc_y updates), so the host knows where the guest wants the cursor plane being placed. So in case the pointer is moved by other means (different input device, some application warping the pointer, ...) the host can adapt.
Would it not be better to be explicit about it? To avoid fragile heuristics.
Explicit about what exactly? Whenever pointer warps via crtc_x + crtc_y update work or not? Not so easy ...
Nevertheless behavior is not consistent here because in some cases the feedback loop is not wired up end-to-end. The spice protocol has a message type for that, so pointer warps work. The vnc protocol has not, so they don't.
... for example qemu allows to enable both spice and vnc protocols.
It actually is a problem for multihead configurations though. Having some way to map input devices to scanouts would actually be helpful. Years ago I checked how this works for touchscreens to see whenever it is possible to leverage that for VMs somehow. There wasn't some obvious way, and I forgot the details meanwhile ...
Ah, that's the other way around, right? To tell guest OS which output an absolute input device is relative to?
Yes.
Having a standard for naming outputs is hard it seems, and there is also the connector vs. monitor dilemma. I guess absolute input devices would usually want to be associated with the (real or virtual) monitor regardless of which (real or virtual) connector it is connected to.
Yes, this should be linked to the monitor not the connector. qemu learned to generate edid blobs a while back, so we actually have virtual monitors and can create virtual monitor properties for them.
take care, Gerd
On Tue, 2022-06-07 at 11:07 +0300, Pekka Paalanen wrote:
On Fri, 03 Jun 2022 14:14:59 +0000 Simon Ser contact@emersion.fr wrote:
Hi,
Please, read this thread: https://lists.freedesktop.org/archives/dri-devel/2020-March/thread.html#2596...
It has a lot of information about the pitfalls of cursor hotspot and other things done by VM software.
In particular: since the driver will ignore the KMS cursor plane position set by user-space, I don't think it's okay to just expose without opt-in from user-space (e.g. with a DRM_CLIENT_CAP).
cc wayland-devel and Pekka for user-space feedback.
On Thursday, June 2nd, 2022 at 17:42, Zack Rusin zack@kde.org wrote:
- all userspace code needs to hardcore a list of drivers which require
hotspots because there's no way to query from drm "does this driver require hotspot"
Can you elaborate? I'm not sure I understand what you mean here.
Hi Zack and everyone,
I would like to try to reboot this discussion and explain where I come from. Maybe I have misunderstood something.
<snip> First of all thanks for restarting the discussions. I think Gerd did a good job responding to individual points, so let me take a step back and explain the big picture here so we can reboot.
Which root problems do you want to solve exactly?
The problem that this patch set is solving is the relatively trivial problem of not having a way of setting the hotspot in the atomic kms interface. When we (virtualized drivers) are using the native cursor we need to know not only the image and position of the cursor, we need to know which point within that cursor activates the click (going back to analogy from the previous email: cursor image with arrow point top-left and cursor image image with arrow pointing bottom-right will have different hotspots, likely [0, 0] in the first case and [cursor_width, cursor_height] in the latter). To correctly route the mouse clicks we need to be aware of the hotspot coordinates. Currently even though all virtualized drivers are atomic userspace has to explicitly disable atomic kms for those drivers because mouse clicks are offset incorrectly as soon as the cursor image changes from the top-left pointing arrow, i.e. the hotspot isn't at [0,0]).
So we would like to be able to enable atomic kms with gnome and kde on virtualized drivers and to do that we need a way to pass hotspot coordinates from userspace to virtualized driver.
That seems to be pretty self-explanatory and, I think, we all agree that will go through properties like in the attached patch set.
Now, where the disagreements come from is from the fact that all virtualized drivers do not implement the atomic KMS cursor plane contract exactly as specified. In atomic kms with universal planes the "cursor" plane can be anything so asking for hotspot's for something that's not a cursor is a bit silly (but arguably so is calling it a cursor plane and then complaining that people expect cursor in it).
So the argument is that we can't put hotspot data into atomic kms without first rewriting all of the virtualized drivers cursor code to fix the underlying contract violation that they all commit. That would likely be a lot easier sell if not that gnome/kde don't put none cursor stuff in the cursor plane, so all this work is to fix breakages that seem to affect 0 of our users (and I completely understand that we'd still like all the drivers to be correct and unified in terms of their behaviour, I'm just saying it's a hard sell without something that we can point to and say "this fixes/improves things for our customers")
Finally there's a discussion on how to fix it and whether virtualized drivers need to do funky stuff with the cursor. I'd like to first make sure we're on the same page and then discuss how to fix it next, so let me finish by saying why not "software cursor".
The easiest way to understand why we do what we do with the cursor, avoiding any complex and weird cases lets go back to the latency issue: the round trips to the guest to move the cursor are certainly noticeable when running locally, but with my VMware hat on, "local" is not the case that interest us, e.g. on ESXi every VM is accessed over a network so now we're dealing with remote round trips. When multiplied by slow connections and scale at which we're operating the "software cursors" go from "some latency" to "completely unusable". That's what I was alluding to in the earlier email when I said they're broken in different ways because if asked what would you prefer: cursor that when clicked is always incorrectly offset, or choppy/unusably slow cursor - you'll get different answers, depending on who you ask. Virtualization vendors tend to invest pretty heavily into protocols that improve on vnc/rdp for remote machine access and we'd like to not lose that.
All in all, what we'd like: - is to be able to run at least the dominant desktops with atomic kms what we need: - we need the hotspot data, what we want to avoid: - fallbacks to software cursors - large additions to user-space
I hope this clarifies things a bit.
z
On Tue, 7 Jun 2022 17:50:24 +0000 Zack Rusin zackr@vmware.com wrote:
On Tue, 2022-06-07 at 11:07 +0300, Pekka Paalanen wrote:
On Fri, 03 Jun 2022 14:14:59 +0000 Simon Ser contact@emersion.fr wrote:
Hi,
Please, read this thread: https://lists.freedesktop.org/archives/dri-devel/2020-March/thread.html#2596...
It has a lot of information about the pitfalls of cursor hotspot and other things done by VM software.
In particular: since the driver will ignore the KMS cursor plane position set by user-space, I don't think it's okay to just expose without opt-in from user-space (e.g. with a DRM_CLIENT_CAP).
cc wayland-devel and Pekka for user-space feedback.
On Thursday, June 2nd, 2022 at 17:42, Zack Rusin zack@kde.org wrote:
- all userspace code needs to hardcore a list of drivers which require
hotspots because there's no way to query from drm "does this driver require hotspot"
Can you elaborate? I'm not sure I understand what you mean here.
Hi Zack and everyone,
I would like to try to reboot this discussion and explain where I come from. Maybe I have misunderstood something.
<snip> First of all thanks for restarting the discussions. I think Gerd did a good job responding to individual points, so let me take a step back and explain the big picture here so we can reboot.
Which root problems do you want to solve exactly?
The problem that this patch set is solving is the relatively trivial problem of not having a way of setting the hotspot in the atomic kms interface. When we (virtualized drivers) are using the native cursor we need to know not only the image
Could you clarify what is "native cursor" here? I guess it is the host window system pointer's cursor?
and position of the cursor, we need to know which point within that cursor activates the click (going back to analogy from the previous email: cursor image with arrow point top-left and cursor image image with arrow pointing bottom-right will have different hotspots, likely [0, 0] in the first case and [cursor_width, cursor_height] in the latter). To correctly route the mouse clicks we need to be aware of the hotspot coordinates. Currently even though all virtualized drivers are atomic userspace has to explicitly disable atomic kms for those drivers because mouse clicks are offset incorrectly as soon as the cursor image changes from the top-left pointing arrow, i.e. the hotspot isn't at [0,0]).
So we would like to be able to enable atomic kms with gnome and kde on virtualized drivers and to do that we need a way to pass hotspot coordinates from userspace to virtualized driver.
That seems to be pretty self-explanatory and, I think, we all agree that will go through properties like in the attached patch set.
Right.
Now, where the disagreements come from is from the fact that all virtualized drivers do not implement the atomic KMS cursor plane contract exactly as specified. In atomic kms with universal planes the "cursor" plane can be anything so asking for hotspot's for something that's not a cursor is a bit silly (but arguably so is calling it a cursor plane and then complaining that people expect cursor in it).
So the argument is that we can't put hotspot data into atomic kms without first rewriting all of the virtualized drivers cursor code to fix the underlying contract violation that they all commit. That would likely be a lot easier sell if not that gnome/kde don't put none cursor stuff in the cursor plane, so all this work is to fix breakages that seem to affect 0 of our users (and I completely understand that we'd still like all the drivers to be correct and unified in terms of their behaviour, I'm just saying it's a hard sell without something that we can point to and say "this fixes/improves things for our customers")
What's the cost of making paravirtualized drivers honour the UAPI contract? Can't you do that on the side of implementing these new hotspot properties, with the little addition to allowing guest userspace to be explicit about whether it supports commandeering or not?
The deny-lists exist in guest userspace already, and they are not going anywhere any time soon that I can see. So the delay in getting those deny-lists side-stepped is the same delay it would take to have the guest userspace use the new UAPI to properly say they do support commandeering.
In your mind, how do you expect guest userspace like Mutter to drop the deny-lists? What would make GNOME developers be willing to do that? And why is that somehow easier or faster than a new proper UAPI to be explicit about commandeering?
You already need patches to guest userspace to fill in the hotspot properties, so I don't get the resistance to adding another flag to be explicit about commandeering as well. Especially when, as you say, the big desktops do not even try to put non-cursor images on cursor planes, it should be trivial for them to set the flag, easier than filling in the hotspot properties.
Then projects like Weston who would indeed need larger surgery to support commandeering will simply not set the flag, nor program hotspot properties, and will get correct (but with high cursor latency) behaviour from guest KMS.
Finally there's a discussion on how to fix it and whether virtualized drivers need to do funky stuff with the cursor. I'd like to first make sure we're on the same page and then discuss how to fix it next, so let me finish by saying why not "software cursor".
The easiest way to understand why we do what we do with the cursor, avoiding any complex and weird cases lets go back to the latency issue: the round trips to the guest to move the cursor are certainly noticeable when running locally, but with my VMware hat on, "local" is not the case that interest us, e.g. on ESXi every VM is accessed over a network so now we're dealing with remote round trips. When multiplied by slow connections and scale at which we're operating the "software cursors" go from "some latency" to "completely unusable". That's what I was alluding to in the earlier email when I said they're broken in different ways because if asked what would you prefer: cursor that when clicked is always incorrectly offset, or choppy/unusably slow cursor - you'll get different answers, depending on who you ask. Virtualization vendors tend to invest pretty heavily into protocols that improve on vnc/rdp for remote machine access and we'd like to not lose that.
Very good.
It is important to be clear about the different ways of being broken, so that we can discuss which faults are being fixed by a proposal rather than arguing whether the proposal fixes "the fault" or not. If one side understands "the fault" to be one thing and the other side thinks it's two separate things, there can never be agreement.
All in all, what we'd like:
- is to be able to run at least the dominant desktops with atomic kms
what we need:
- we need the hotspot data,
what we want to avoid:
- fallbacks to software cursors
- large additions to user-space
I hope this clarifies things a bit.
Yes, it does, to me at least.
Thanks, pq
On Wed, 2022-06-08 at 10:53 +0300, Pekka Paalanen wrote:
On Tue, 7 Jun 2022 17:50:24 +0000 Zack Rusin zackr@vmware.com wrote:
On Tue, 2022-06-07 at 11:07 +0300, Pekka Paalanen wrote:
On Fri, 03 Jun 2022 14:14:59 +0000 Simon Ser contact@emersion.fr wrote:
Hi,
Please, read this thread: https://lists.freedesktop.org/archives/dri-devel/2020-March/thread.html#2596...
It has a lot of information about the pitfalls of cursor hotspot and other things done by VM software.
In particular: since the driver will ignore the KMS cursor plane position set by user-space, I don't think it's okay to just expose without opt-in from user-space (e.g. with a DRM_CLIENT_CAP).
cc wayland-devel and Pekka for user-space feedback.
On Thursday, June 2nd, 2022 at 17:42, Zack Rusin zack@kde.org wrote:
- all userspace code needs to hardcore a list of drivers which require
hotspots because there's no way to query from drm "does this driver require hotspot"
Can you elaborate? I'm not sure I understand what you mean here.
Hi Zack and everyone,
I would like to try to reboot this discussion and explain where I come from. Maybe I have misunderstood something.
<snip> First of all thanks for restarting the discussions. I think Gerd did a good job responding to individual points, so let me take a step back and explain the big picture here so we can reboot.
Which root problems do you want to solve exactly?
The problem that this patch set is solving is the relatively trivial problem of not having a way of setting the hotspot in the atomic kms interface. When we (virtualized drivers) are using the native cursor we need to know not only the image
Could you clarify what is "native cursor" here? I guess it is the host window system pointer's cursor?
Right, exactly. I'm a little hesitant to call it "host" because it gets tricky in remote scenarios, where the host is some ESXi server but the local machine is the one that's actually interacting with the guest. So it's the cursor of the machine where the guest screen is displayed.
Now, where the disagreements come from is from the fact that all virtualized drivers do not implement the atomic KMS cursor plane contract exactly as specified. In atomic kms with universal planes the "cursor" plane can be anything so asking for hotspot's for something that's not a cursor is a bit silly (but arguably so is calling it a cursor plane and then complaining that people expect cursor in it).
So the argument is that we can't put hotspot data into atomic kms without first rewriting all of the virtualized drivers cursor code to fix the underlying contract violation that they all commit. That would likely be a lot easier sell if not that gnome/kde don't put none cursor stuff in the cursor plane, so all this work is to fix breakages that seem to affect 0 of our users (and I completely understand that we'd still like all the drivers to be correct and unified in terms of their behaviour, I'm just saying it's a hard sell without something that we can point to and say "this fixes/improves things for our customers")
What's the cost of making paravirtualized drivers honour the UAPI contract? Can't you do that on the side of implementing these new hotspot properties, with the little addition to allowing guest userspace to be explicit about whether it supports commandeering or not?
I'm reluctant here because "fixing" here seems to be a bit ephemeral as we move from one solution to the next. I'm happy to write a patch that's adding a DRM_CLIENT_CAP_VIRTUAL_CURSOR_AWARE flag and hiding the cursor plane in virtualized drivers for clients that advertise DRM_CLIENT_CAP_ATOMIC but not DRM_CLIENT_CAP_VIRTUAL_CURSOR_AWARE, but that doesn't solve Weston on virtualized drivers.
I feel like that's a larger discussion. One that we need to have in general - it's about standardising on behaviour of userspace with virtualized drivers, e.g. on Weston even the most basic functionality of virtualized drivers which is resizing a window doesn't work correctly (virtualized drivers send drm_kms_helper_hotplug_event which sends a HOTPLUG=1 event with a changed preferred width/height but Weston doesn't seem to resize the fb on them which results in Weston not resizing to the new size of the window) or even considering the suggested_x and suggested_y properties. It seems like we might need to have a wider discussion on standardising those common issues on virtualized drivers because currently, I'm guessing, that apart from Gnome and KDE most compositors are completely broken on virtualized drivers.
I'd prefer not blocking fixing hotspots until all those issues are resolved so if we can agree on what we'd like to fix before hotspots go in, that'd be great.
z
On Thu, 9 Jun 2022 19:39:39 +0000 Zack Rusin zackr@vmware.com wrote:
On Wed, 2022-06-08 at 10:53 +0300, Pekka Paalanen wrote:
On Tue, 7 Jun 2022 17:50:24 +0000 Zack Rusin zackr@vmware.com wrote:
On Tue, 2022-06-07 at 11:07 +0300, Pekka Paalanen wrote:
On Fri, 03 Jun 2022 14:14:59 +0000 Simon Ser contact@emersion.fr wrote:
Hi,
Please, read this thread: https://lists.freedesktop.org/archives/dri-devel/2020-March/thread.html#2596...
It has a lot of information about the pitfalls of cursor hotspot and other things done by VM software.
In particular: since the driver will ignore the KMS cursor plane position set by user-space, I don't think it's okay to just expose without opt-in from user-space (e.g. with a DRM_CLIENT_CAP).
cc wayland-devel and Pekka for user-space feedback.
On Thursday, June 2nd, 2022 at 17:42, Zack Rusin zack@kde.org wrote:
- all userspace code needs to hardcore a list of drivers which require
hotspots because there's no way to query from drm "does this driver require hotspot"
Can you elaborate? I'm not sure I understand what you mean here.
Hi Zack and everyone,
I would like to try to reboot this discussion and explain where I come from. Maybe I have misunderstood something.
<snip> First of all thanks for restarting the discussions. I think Gerd did a good job responding to individual points, so let me take a step back and explain the big picture here so we can reboot.
Which root problems do you want to solve exactly?
The problem that this patch set is solving is the relatively trivial problem of not having a way of setting the hotspot in the atomic kms interface. When we (virtualized drivers) are using the native cursor we need to know not only the image
Could you clarify what is "native cursor" here? I guess it is the host window system pointer's cursor?
Right, exactly. I'm a little hesitant to call it "host" because it gets tricky in remote scenarios, where the host is some ESXi server but the local machine is the one that's actually interacting with the guest. So it's the cursor of the machine where the guest screen is displayed.
Now, where the disagreements come from is from the fact that all virtualized drivers do not implement the atomic KMS cursor plane contract exactly as specified. In atomic kms with universal planes the "cursor" plane can be anything so asking for hotspot's for something that's not a cursor is a bit silly (but arguably so is calling it a cursor plane and then complaining that people expect cursor in it).
So the argument is that we can't put hotspot data into atomic kms without first rewriting all of the virtualized drivers cursor code to fix the underlying contract violation that they all commit. That would likely be a lot easier sell if not that gnome/kde don't put none cursor stuff in the cursor plane, so all this work is to fix breakages that seem to affect 0 of our users (and I completely understand that we'd still like all the drivers to be correct and unified in terms of their behaviour, I'm just saying it's a hard sell without something that we can point to and say "this fixes/improves things for our customers")
What's the cost of making paravirtualized drivers honour the UAPI contract? Can't you do that on the side of implementing these new hotspot properties, with the little addition to allowing guest userspace to be explicit about whether it supports commandeering or not?
I'm reluctant here because "fixing" here seems to be a bit ephemeral as we move from one solution to the next. I'm happy to write a patch that's adding a DRM_CLIENT_CAP_VIRTUAL_CURSOR_AWARE flag and hiding the cursor plane in virtualized drivers for clients that advertise DRM_CLIENT_CAP_ATOMIC but not DRM_CLIENT_CAP_VIRTUAL_CURSOR_AWARE, but that doesn't solve Weston on virtualized drivers.
Mind, I have not talked about hiding cursor planes. I have talked *only* about stopping commandeering cursor planes if guest userspace does not indicate it is prepared for commandeering.
I don't understand how it does not "solve on Weston on virtualized drivers". Can you explain what is not solved?
As far as I can see, it does solve Weston: it makes cursor plane behaviour correct from KMS UAPI contract point of view. Anything that is not quite optimal after that with cursor planes you can blame on Weston not making use of additional optional features.
Cursor plane itself is an additional optional feature. Userspace has no obligation to use one at all, but if it does, it better behave by the UAPI contract.
I feel like that's a larger discussion. One that we need to have in general - it's about standardising on behaviour of userspace with virtualized drivers, e.g. on Weston even the most basic functionality of virtualized drivers which is resizing a window doesn't work correctly (virtualized drivers send drm_kms_helper_hotplug_event which sends a HOTPLUG=1 event with a changed preferred width/height but Weston doesn't seem to resize the fb on them which results in Weston not resizing to the new size of the window) or even considering the suggested_x and suggested_y properties. It seems like we might need to have a wider discussion on standardising those common issues on virtualized drivers because currently, I'm guessing, that apart from Gnome and KDE most compositors are completely broken on virtualized drivers.
You say "broken", I say "not implemented yet". *Those* problems are Weston's own problems. They are new features that require explicit support in Weston. No driver should try to implement those behind guest userspace back.
This "not resizing" is not at all the same as the cursor plane commandeering. Weston not supporting KMS-induced resizing does not silently result in fundamentally incorrect behaviour. The CRTC size remains the same, and nothing is actually broken. Only the end user cannot seem to resize the viewer window, but everything works fine otherwise. OTOH, when the VM stack commandeers the cursor plane without permission, it breaks things so bad that user interaction is near impossible. And it's a violation of the KMS UAPI contract.
If we are looking at things virtual drivers make strange, another thing is that Weston is not expecting KMS pageflips to complete always immediately regardless of the programmed refresh rate and "hardware" refresh cycle phase. It is only luck that Weston does not end up in a busy-loop updating the screen on virtual drivers, not intentional. We can have a similar discussion there, are KMS drivers in general allowed to complete atomic flips always immediately even if userspace asks for vsync'd flips, or does it require explicit userspace opt-in.
I'd prefer not blocking fixing hotspots until all those issues are resolved so if we can agree on what we'd like to fix before hotspots go in, that'd be great.
I think you are confusing things here. In my mind there is no doubt about the KMS UAPI contract on cursor planes: commandeering is not allowed. You have to add new UAPI to allow the VM stack to commandeer cursor planes, and guest userspace must opt-in for that.
How you design that is up to you. Maybe a new client cap, or maybe you inspect every atomic commit did the userspace set the hotspot properties this time or not. The main thing is that this has been thought about and documented.
I really do not see why adding that detail is so big deal, while not adding that will leave virtual drivers fundamentally broken (incorrect behaviour resulting from violating the KMS UAPI contract) for cursor planes.
-----
Maybe we need to take another step back. Why are virtual drivers specifically DRM KMS drivers? Was the idea not that if virtual drivers pretend to be KMS drivers, we would not need to patch userspace? But now we need to patch userspace anyway, so why bother with KMS and its design limitations that are well appropriate for hardware drivers but not for virtual drivers? If you had your own winsys virtualization protocol, you could do so much more than KMS is ever going to allow you, and so much better.
Or just, you know, use RDP, VNC, and whatnot there already exists.
Why KMS?
That's probably obvious to you, but not me.
I would also like to point out that I am not a kernel developer and I have no NAK/veto rights on any kernel patches. I can only explain how things look like from userspace perspective.
I suspect there is nothing more I can say. Those were my opinions, but the decisions are up to kernel maintainers. Hence, I can agree to disagree with you.
Thanks, pq
On Fri, Jun 10, 2022 at 10:49:31AM +0300, Pekka Paalanen wrote:
On Thu, 9 Jun 2022 19:39:39 +0000 Zack Rusin zackr@vmware.com wrote:
On Wed, 2022-06-08 at 10:53 +0300, Pekka Paalanen wrote:
On Tue, 7 Jun 2022 17:50:24 +0000 Zack Rusin zackr@vmware.com wrote:
On Tue, 2022-06-07 at 11:07 +0300, Pekka Paalanen wrote:
On Fri, 03 Jun 2022 14:14:59 +0000 Simon Ser contact@emersion.fr wrote:
Hi,
Please, read this thread: https://lists.freedesktop.org/archives/dri-devel/2020-March/thread.html#2596...
It has a lot of information about the pitfalls of cursor hotspot and other things done by VM software.
In particular: since the driver will ignore the KMS cursor plane position set by user-space, I don't think it's okay to just expose without opt-in from user-space (e.g. with a DRM_CLIENT_CAP).
cc wayland-devel and Pekka for user-space feedback.
On Thursday, June 2nd, 2022 at 17:42, Zack Rusin zack@kde.org wrote:
> - all userspace code needs to hardcore a list of drivers which require > hotspots because there's no way to query from drm "does this driver > require hotspot"
Can you elaborate? I'm not sure I understand what you mean here.
Hi Zack and everyone,
I would like to try to reboot this discussion and explain where I come from. Maybe I have misunderstood something.
<snip> First of all thanks for restarting the discussions. I think Gerd did a good job responding to individual points, so let me take a step back and explain the big picture here so we can reboot.
Which root problems do you want to solve exactly?
The problem that this patch set is solving is the relatively trivial problem of not having a way of setting the hotspot in the atomic kms interface. When we (virtualized drivers) are using the native cursor we need to know not only the image
Could you clarify what is "native cursor" here? I guess it is the host window system pointer's cursor?
Right, exactly. I'm a little hesitant to call it "host" because it gets tricky in remote scenarios, where the host is some ESXi server but the local machine is the one that's actually interacting with the guest. So it's the cursor of the machine where the guest screen is displayed.
Now, where the disagreements come from is from the fact that all virtualized drivers do not implement the atomic KMS cursor plane contract exactly as specified. In atomic kms with universal planes the "cursor" plane can be anything so asking for hotspot's for something that's not a cursor is a bit silly (but arguably so is calling it a cursor plane and then complaining that people expect cursor in it).
So the argument is that we can't put hotspot data into atomic kms without first rewriting all of the virtualized drivers cursor code to fix the underlying contract violation that they all commit. That would likely be a lot easier sell if not that gnome/kde don't put none cursor stuff in the cursor plane, so all this work is to fix breakages that seem to affect 0 of our users (and I completely understand that we'd still like all the drivers to be correct and unified in terms of their behaviour, I'm just saying it's a hard sell without something that we can point to and say "this fixes/improves things for our customers")
What's the cost of making paravirtualized drivers honour the UAPI contract? Can't you do that on the side of implementing these new hotspot properties, with the little addition to allowing guest userspace to be explicit about whether it supports commandeering or not?
I'm reluctant here because "fixing" here seems to be a bit ephemeral as we move from one solution to the next. I'm happy to write a patch that's adding a DRM_CLIENT_CAP_VIRTUAL_CURSOR_AWARE flag and hiding the cursor plane in virtualized drivers for clients that advertise DRM_CLIENT_CAP_ATOMIC but not DRM_CLIENT_CAP_VIRTUAL_CURSOR_AWARE, but that doesn't solve Weston on virtualized drivers.
Mind, I have not talked about hiding cursor planes. I have talked *only* about stopping commandeering cursor planes if guest userspace does not indicate it is prepared for commandeering.
I don't understand how it does not "solve on Weston on virtualized drivers". Can you explain what is not solved?
As far as I can see, it does solve Weston: it makes cursor plane behaviour correct from KMS UAPI contract point of view. Anything that is not quite optimal after that with cursor planes you can blame on Weston not making use of additional optional features.
Cursor plane itself is an additional optional feature. Userspace has no obligation to use one at all, but if it does, it better behave by the UAPI contract.
I feel like that's a larger discussion. One that we need to have in general - it's about standardising on behaviour of userspace with virtualized drivers, e.g. on Weston even the most basic functionality of virtualized drivers which is resizing a window doesn't work correctly (virtualized drivers send drm_kms_helper_hotplug_event which sends a HOTPLUG=1 event with a changed preferred width/height but Weston doesn't seem to resize the fb on them which results in Weston not resizing to the new size of the window) or even considering the suggested_x and suggested_y properties. It seems like we might need to have a wider discussion on standardising those common issues on virtualized drivers because currently, I'm guessing, that apart from Gnome and KDE most compositors are completely broken on virtualized drivers.
You say "broken", I say "not implemented yet". *Those* problems are Weston's own problems. They are new features that require explicit support in Weston. No driver should try to implement those behind guest userspace back.
This "not resizing" is not at all the same as the cursor plane commandeering. Weston not supporting KMS-induced resizing does not silently result in fundamentally incorrect behaviour. The CRTC size remains the same, and nothing is actually broken. Only the end user cannot seem to resize the viewer window, but everything works fine otherwise. OTOH, when the VM stack commandeers the cursor plane without permission, it breaks things so bad that user interaction is near impossible. And it's a violation of the KMS UAPI contract.
If we are looking at things virtual drivers make strange, another thing is that Weston is not expecting KMS pageflips to complete always immediately regardless of the programmed refresh rate and "hardware" refresh cycle phase. It is only luck that Weston does not end up in a busy-loop updating the screen on virtual drivers, not intentional. We can have a similar discussion there, are KMS drivers in general allowed to complete atomic flips always immediately even if userspace asks for vsync'd flips, or does it require explicit userspace opt-in.
I'd prefer not blocking fixing hotspots until all those issues are resolved so if we can agree on what we'd like to fix before hotspots go in, that'd be great.
I think you are confusing things here. In my mind there is no doubt about the KMS UAPI contract on cursor planes: commandeering is not allowed. You have to add new UAPI to allow the VM stack to commandeer cursor planes, and guest userspace must opt-in for that.
How you design that is up to you. Maybe a new client cap, or maybe you inspect every atomic commit did the userspace set the hotspot properties this time or not. The main thing is that this has been thought about and documented.
I really do not see why adding that detail is so big deal, while not adding that will leave virtual drivers fundamentally broken (incorrect behaviour resulting from violating the KMS UAPI contract) for cursor planes.
Maybe we need to take another step back. Why are virtual drivers specifically DRM KMS drivers? Was the idea not that if virtual drivers pretend to be KMS drivers, we would not need to patch userspace? But now we need to patch userspace anyway, so why bother with KMS and its design limitations that are well appropriate for hardware drivers but not for virtual drivers? If you had your own winsys virtualization protocol, you could do so much more than KMS is ever going to allow you, and so much better.
From a user space perspective and distribution developer perspective,
having virtual machines virtualize not only disks, memory and network cards but also GPUs and input devices is very helpful. Naturally there are alternative methods to getting content of a graphical session from a virtualized environment, e.g. RDP, without involving KMS, but when a virtual machine is used for running arbitrary distributions, e.g. for testing purposes, one wouldn't test the actual distribution if the graphical session didn't use the APIs otherwise used, so discussing whether we need this or not seems quite orthogonal to the issue at hand.
I get that using the cursor plane the way virtual machines use them is problematic right now and method to get the expected behavior from both sides is needed, but the fact that there is minor differences in how hardware is virtualized and how it behaves in the real world (the same applies to e.g. pointer devices) is not a reason to say virtualization via KMS and evdev isn't needed, and it should be unblocked to allow using atomic mode setting to get the expected behavior.
No matter what, to use atomic mode setting together with virtual machines, user space needs patching, but patching by enabling a client cap or equivalent is not comparable to introducing a whole new subsystem using something other than KMS.
From mutter's perspective, all we need is a way to set hotspots on
cursor planes, and we can use atomic mode setting instead of the non-atomic paths that we currently fall back to for virtual machine drivers.
Simon's suggestion of using a client capability for exposing the hotspot property, and not exposing the x,y coordinate of the cursor plane would be a completely acceptable, as it'd allow us to eventually migrate away from the deny list we have in place.
Jonas
Or just, you know, use RDP, VNC, and whatnot there already exists.
Why KMS?
That's probably obvious to you, but not me.
I would also like to point out that I am not a kernel developer and I have no NAK/veto rights on any kernel patches. I can only explain how things look like from userspace perspective.
I suspect there is nothing more I can say. Those were my opinions, but the decisions are up to kernel maintainers. Hence, I can agree to disagree with you.
Thanks, pq
I agree with what others have replied, just adding a few more details.
On Thursday, June 9th, 2022 at 21:39, Zack Rusin zackr@vmware.com wrote:
virtualized drivers send drm_kms_helper_hotplug_event which sends a HOTPLUG=1 event with a changed preferred width/height
(Note: and the "hotplug_mode_update" property is set to 1.)
suggested_x and suggested_y properties
These come with their own set of issues. They are poorly defined, but it seems like they describe a position in physical pixel coordinates. Compositors don't use physical pixel coordinates to organize their outputs, instead they use logical coordinates. For instance, a HiDPI 4k screen with a scale of 2 will take up 1920x1080 logical pixels. There is no way to convert physical pixel coordinates to logical pixel coordinates in the general case, because there's no "global scale factor". So suggested_x/y are incompatible with the way compositors work.
On Fri, Jun 10, 2022 at 08:54:03AM +0000, Simon Ser wrote:
I agree with what others have replied, just adding a few more details.
On Thursday, June 9th, 2022 at 21:39, Zack Rusin zackr@vmware.com wrote:
virtualized drivers send drm_kms_helper_hotplug_event which sends a HOTPLUG=1 event with a changed preferred width/height
(Note: and the "hotplug_mode_update" property is set to 1.)
suggested_x and suggested_y properties
These come with their own set of issues. They are poorly defined, but it seems like they describe a position in physical pixel coordinates. Compositors don't use physical pixel coordinates to organize their outputs, instead they use logical coordinates. For instance, a HiDPI 4k screen with a scale of 2 will take up 1920x1080 logical pixels. There is no way to convert physical pixel coordinates to logical pixel coordinates in the general case, because there's no "global scale factor". So suggested_x/y are incompatible with the way compositors work.
I dropped a request for a proper doc section that explains all the virtualized kms driver stuff. I think we should also put in a "limitations" part there and just spec that any kind of scaling is a no-go on these (and that drivers better validate this is the case). -Daniel
Hi all,
Kinda top post because the thread is sprawling and I think we need a summary/restart. I think there's at least 3 issues here:
- lack of hotspot property support, which means compositors can't really support hotspot with atomic. Which isn't entirely true, because you totally can use atomic for the primary planes/crtcs and the legacy cursor ioctls, but I understand that people might find that a bit silly :-)
Anyway this problme is solved by the patch set here, and I think results in some nice cleanups to boot.
- the fact that cursors for virtual drivers are not planes, but really special things. Which just breaks the universal plane kms uapi. That part isn't solved, and I do agree with Simon and Pekka that we really should solve this before we unleash even more compositors onto the atomic paths of virtual drivers.
I think the simplest solution for this is: 1. add a new DRM_PLANE_TYPE_VIRTUAL_CURSOR, and set that for these special cursor planes on all virtual drivers 2. add the new "I understand virtual cursors planes" setparam, filter virtual cursor planes for userspace which doesn't set this (like we do right now if userspace doesn't set the universal plane mode) 3. backport the above patches to all stable kernels 4. make sure the hotspot property is only set on VIRTUAL_CURSOR planes and nothing else in the rebased patch series
- third issue: These special virtual display properties arent documented. Aside from hotspot there's also suggested X/Y and maybe other stuff. I have no idea what suggested X/Y does and what userspace should do with it. I think we need a new section for virtualized drivers which: - documents all the properties involved - documents the new cap for enabling virtual cursor planes - documents some of the key flows that compositors should implement for best experience - documents how exactly the user experience will degrade if compositors pretend it's just a normal kms driver (maybe put that into each of the special flows that a compositor ideally supports) - whatever other comments and gaps I've missed, I'm sure Simon/Pekka/others will chime in once the patch exists.
There's a bit of fixing oopsies (virtualized drivers really shouldn't have enabled universal planes for their cursors) and debt (all these properties predate the push to document stuff so we need to fix that), but I don't think it's too much. And I think, from reading the threads, that this should cover everything?
Anything I've missed? Or got completely wrong?
Cheers, Daniel
On Fri, Jun 03, 2022 at 02:14:59PM +0000, Simon Ser wrote:
Hi,
Please, read this thread: https://lists.freedesktop.org/archives/dri-devel/2020-March/thread.html#2596...
It has a lot of information about the pitfalls of cursor hotspot and other things done by VM software.
In particular: since the driver will ignore the KMS cursor plane position set by user-space, I don't think it's okay to just expose without opt-in from user-space (e.g. with a DRM_CLIENT_CAP).
cc wayland-devel and Pekka for user-space feedback.
On Thursday, June 2nd, 2022 at 17:42, Zack Rusin zack@kde.org wrote:
- all userspace code needs to hardcore a list of drivers which require
hotspots because there's no way to query from drm "does this driver require hotspot"
Can you elaborate? I'm not sure I understand what you mean here.
Thanks,
Simon
On Fri, 10 Jun 2022 10:41:05 +0200 Daniel Vetter daniel@ffwll.ch wrote:
Hi all,
Kinda top post because the thread is sprawling and I think we need a summary/restart. I think there's at least 3 issues here:
lack of hotspot property support, which means compositors can't really support hotspot with atomic. Which isn't entirely true, because you totally can use atomic for the primary planes/crtcs and the legacy cursor ioctls, but I understand that people might find that a bit silly :-)
Anyway this problme is solved by the patch set here, and I think results in some nice cleanups to boot.
the fact that cursors for virtual drivers are not planes, but really special things. Which just breaks the universal plane kms uapi. That part isn't solved, and I do agree with Simon and Pekka that we really should solve this before we unleash even more compositors onto the atomic paths of virtual drivers.
I think the simplest solution for this is:
- add a new DRM_PLANE_TYPE_VIRTUAL_CURSOR, and set that for these
special cursor planes on all virtual drivers 2. add the new "I understand virtual cursors planes" setparam, filter virtual cursor planes for userspace which doesn't set this (like we do right now if userspace doesn't set the universal plane mode) 3. backport the above patches to all stable kernels 4. make sure the hotspot property is only set on VIRTUAL_CURSOR planes and nothing else in the rebased patch series
third issue: These special virtual display properties arent documented. Aside from hotspot there's also suggested X/Y and maybe other stuff. I have no idea what suggested X/Y does and what userspace should do with it. I think we need a new section for virtualized drivers which:
- documents all the properties involved
- documents the new cap for enabling virtual cursor planes
- documents some of the key flows that compositors should implement for best experience
- documents how exactly the user experience will degrade if compositors pretend it's just a normal kms driver (maybe put that into each of the special flows that a compositor ideally supports)
- whatever other comments and gaps I've missed, I'm sure Simon/Pekka/others will chime in once the patch exists.
There's a bit of fixing oopsies (virtualized drivers really shouldn't have enabled universal planes for their cursors) and debt (all these properties predate the push to document stuff so we need to fix that), but I don't think it's too much. And I think, from reading the threads, that this should cover everything?
Anything I've missed? Or got completely wrong?
Hi,
sounds like a good plan to me.
Thanks, pq
On Fri, Jun 10, 2022 at 10:41:05AM +0200, Daniel Vetter wrote:
Hi all,
Kinda top post because the thread is sprawling and I think we need a summary/restart. I think there's at least 3 issues here:
lack of hotspot property support, which means compositors can't really support hotspot with atomic. Which isn't entirely true, because you totally can use atomic for the primary planes/crtcs and the legacy cursor ioctls, but I understand that people might find that a bit silly :-)
Anyway this problme is solved by the patch set here, and I think results in some nice cleanups to boot.
the fact that cursors for virtual drivers are not planes, but really special things. Which just breaks the universal plane kms uapi. That part isn't solved, and I do agree with Simon and Pekka that we really should solve this before we unleash even more compositors onto the atomic paths of virtual drivers.
I think the simplest solution for this is:
- add a new DRM_PLANE_TYPE_VIRTUAL_CURSOR, and set that for these
special cursor planes on all virtual drivers 2. add the new "I understand virtual cursors planes" setparam, filter virtual cursor planes for userspace which doesn't set this (like we do right now if userspace doesn't set the universal plane mode) 3. backport the above patches to all stable kernels 4. make sure the hotspot property is only set on VIRTUAL_CURSOR planes and nothing else in the rebased patch series
Simon also mentioned on irc that these special planes must not expose the CRTC_X/Y property, since that doesn't really do much at all. Or is our understanding of how this all works for commandeered cursors wrong?
- third issue: These special virtual display properties arent documented. Aside from hotspot there's also suggested X/Y and maybe other stuff. I have no idea what suggested X/Y does and what userspace should do with it. I think we need a new section for virtualized drivers which:
- documents all the properties involved
- documents the new cap for enabling virtual cursor planes
- documents some of the key flows that compositors should implement for best experience
- documents how exactly the user experience will degrade if compositors pretend it's just a normal kms driver (maybe put that into each of the special flows that a compositor ideally supports)
- whatever other comments and gaps I've missed, I'm sure Simon/Pekka/others will chime in once the patch exists.
Great bonus would be an igt which demonstrates these flows. With the interactive debug breakpoints to wait for resizing or whatever this should be all neatly possible. -Daniel
There's a bit of fixing oopsies (virtualized drivers really shouldn't have enabled universal planes for their cursors) and debt (all these properties predate the push to document stuff so we need to fix that), but I don't think it's too much. And I think, from reading the threads, that this should cover everything?
Anything I've missed? Or got completely wrong?
Cheers, Daniel
On Fri, Jun 03, 2022 at 02:14:59PM +0000, Simon Ser wrote:
Hi,
Please, read this thread: https://lists.freedesktop.org/archives/dri-devel/2020-March/thread.html#2596...
It has a lot of information about the pitfalls of cursor hotspot and other things done by VM software.
In particular: since the driver will ignore the KMS cursor plane position set by user-space, I don't think it's okay to just expose without opt-in from user-space (e.g. with a DRM_CLIENT_CAP).
cc wayland-devel and Pekka for user-space feedback.
On Thursday, June 2nd, 2022 at 17:42, Zack Rusin zack@kde.org wrote:
- all userspace code needs to hardcore a list of drivers which require
hotspots because there's no way to query from drm "does this driver require hotspot"
Can you elaborate? I'm not sure I understand what you mean here.
Thanks,
Simon
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Hi,
- make sure the hotspot property is only set on VIRTUAL_CURSOR planes
and nothing else in the rebased patch series
Simon also mentioned on irc that these special planes must not expose the CRTC_X/Y property, since that doesn't really do much at all. Or is our understanding of how this all works for commandeered cursors wrong?
Depends. In some cases the pointer position is a one-way host->guest ticket (via tablet device). In some cases the other direction works too and the guest can warp the mouse pointer to another place on the host display. The guest can't easily figure whenever warp works or not because that depends on host-side configuration the guest has no insight to.
take care, Gerd
On Fri, 2022-06-10 at 10:59 +0200, Daniel Vetter wrote:
⚠ External Email
On Fri, Jun 10, 2022 at 10:41:05AM +0200, Daniel Vetter wrote:
Hi all,
Kinda top post because the thread is sprawling and I think we need a summary/restart. I think there's at least 3 issues here:
lack of hotspot property support, which means compositors can't really support hotspot with atomic. Which isn't entirely true, because you totally can use atomic for the primary planes/crtcs and the legacy cursor ioctls, but I understand that people might find that a bit silly :-)
Anyway this problme is solved by the patch set here, and I think results in some nice cleanups to boot.
the fact that cursors for virtual drivers are not planes, but really special things. Which just breaks the universal plane kms uapi. That part isn't solved, and I do agree with Simon and Pekka that we really should solve this before we unleash even more compositors onto the atomic paths of virtual drivers.
I think the simplest solution for this is:
- add a new DRM_PLANE_TYPE_VIRTUAL_CURSOR, and set that for these
special cursor planes on all virtual drivers 2. add the new "I understand virtual cursors planes" setparam, filter virtual cursor planes for userspace which doesn't set this (like we do right now if userspace doesn't set the universal plane mode) 3. backport the above patches to all stable kernels 4. make sure the hotspot property is only set on VIRTUAL_CURSOR planes and nothing else in the rebased patch series
Simon also mentioned on irc that these special planes must not expose the CRTC_X/Y property, since that doesn't really do much at all. Or is our understanding of how this all works for commandeered cursors wrong?
Yes, that's the part I don't understand. I don't think I see how the CRTC_X|Y properties aren't used.
I think the confusion might stem from the fact that it appears that the hypervisors/hosts are moving that plane, which is not the case. We never move the plane itself, we redirect the mouse focus/movement, that's what's reducing the latency but don't touch drm internals. I can't speak for every virtualized stack, but what is happening on ours is that we set the image that's on the cursor plane as the cursor on the machine that's running the guest. So when you move the mouse across the screen as you enter the VM window the cursor plane isn't touched, but the local machines cursor changes to what's inside the cursor plane. When the mouse is clicked the mouse device in the guest generates the event with the proper coordinates (hence we need the hotspot to route that event correctly). That's when the guest reacts just like it would normally on native if a mouse event was received.
The actual cursor plane might not be visible while this is happening but even when it's not visible it's still at whatever crtc_x|y the guest thinks it is. crtc_x|y are still only driven by the guests mouse device.
We control the mouse device and visibility of the cursor plane itself based on what's happening in the system but I don't think that's that different from a native system.
This is easy to see by running the "weston-simple-damage --width=64 --height=64" if you click on that window and move the cursor to the very edge of the virtualized window you'll notice that it's coming out outside of the window, that's because it's the local machines cursor, but if you stop the press the window will jump to wherever it was because the real guest plane is still at crtc_x|y (and because in weston that window doesn't react to mouse move events like a cursor would it never sets crtc_x|y to the mouse directed coordinates).
The problem stems from the fact that the cursor plane has something that's not a cursor and doesn't react to mouse events like a cursor would. So the flag (or new plane) that we'd be adding is simply making user-space give the following promise:
"I understand that what's inside the cursor plane needs to react and deal with mouse events like a mouse cursor would, i.e. it should be a mouse cursor"
z
On Fri, 10 Jun 2022 14:24:01 +0000 Zack Rusin zackr@vmware.com wrote:
On Fri, 2022-06-10 at 10:59 +0200, Daniel Vetter wrote:
⚠ External Email
On Fri, Jun 10, 2022 at 10:41:05AM +0200, Daniel Vetter wrote:
Hi all,
Kinda top post because the thread is sprawling and I think we need a summary/restart. I think there's at least 3 issues here:
lack of hotspot property support, which means compositors can't really support hotspot with atomic. Which isn't entirely true, because you totally can use atomic for the primary planes/crtcs and the legacy cursor ioctls, but I understand that people might find that a bit silly :-)
Anyway this problme is solved by the patch set here, and I think results in some nice cleanups to boot.
the fact that cursors for virtual drivers are not planes, but really special things. Which just breaks the universal plane kms uapi. That part isn't solved, and I do agree with Simon and Pekka that we really should solve this before we unleash even more compositors onto the atomic paths of virtual drivers.
I think the simplest solution for this is:
- add a new DRM_PLANE_TYPE_VIRTUAL_CURSOR, and set that for these
special cursor planes on all virtual drivers 2. add the new "I understand virtual cursors planes" setparam, filter virtual cursor planes for userspace which doesn't set this (like we do right now if userspace doesn't set the universal plane mode) 3. backport the above patches to all stable kernels 4. make sure the hotspot property is only set on VIRTUAL_CURSOR planes and nothing else in the rebased patch series
Simon also mentioned on irc that these special planes must not expose the CRTC_X/Y property, since that doesn't really do much at all. Or is our understanding of how this all works for commandeered cursors wrong?
Yes, that's the part I don't understand. I don't think I see how the CRTC_X|Y properties aren't used.
I think the confusion might stem from the fact that it appears that the hypervisors/hosts are moving that plane, which is not the case. We never move the plane itself, we redirect the mouse focus/movement, that's what's reducing the latency but don't touch drm internals. I can't speak for every virtualized stack, but what is happening on ours is that we set the image that's on the cursor plane as the cursor on the machine that's running the guest. So when you move the mouse across the screen as you enter the VM window the cursor plane isn't touched, but the local machines cursor changes to what's inside the cursor plane. When the mouse is clicked the mouse device in the guest generates the event with the proper coordinates (hence we need the hotspot to route that event correctly). That's when the guest reacts just like it would normally on native if a mouse event was received.
The actual cursor plane might not be visible while this is happening but even when it's not visible it's still at whatever crtc_x|y the guest thinks it is. crtc_x|y are still only driven by the guests mouse device.
We control the mouse device and visibility of the cursor plane itself based on what's happening in the system but I don't think that's that different from a native system.
Sorry Zack, while I'm sure that is technically correct and very detaily accurate, it's totally not any different to what we have been talking about all along.
We care about how things look like to the end user, and not what property values the guest KMS driver might have for each plane. The KMS UAPI contract is about how things look to the end user, not just what values might be stored in a KMS driver (and then ignored).
It doesn't matter if the CRTC_X/Y properties remain at what the guest userspace set them to, if you are going to hide the "real" virtual cursor plane and instead upload the cursor image to the host window system to be composited independently. You are breaking the UAPI contract. What the end user sees is *not* what the guest OS programmed. That's the whole point.
What you described is the very definition of cursor plane commandeering as I defined it: showing the cursor image not where the guest OS put it.
I never assumed that you would actually reflect host cursor position in the guest KMS cursor plane CRTC_X/Y. You just ignore those values completely in the VM stack levels closer to the end user's eyes in seamless mouse mode. The end effect is the same.
This is easy to see by running the "weston-simple-damage --width=64 --height=64" if you click on that window and move the cursor to the very edge of the virtualized window you'll notice that it's coming out outside of the window, that's because it's the local machines cursor, but if you stop the press the window will jump to wherever it was because the real guest plane is still at crtc_x|y (and because in weston that window doesn't react to mouse move events like a cursor would it never sets crtc_x|y to the mouse directed coordinates).
The problem stems from the fact that the cursor plane has something that's not a cursor and doesn't react to mouse events like a cursor would. So the flag (or new plane) that we'd be adding is simply making user-space give the following promise:
"I understand that what's inside the cursor plane needs to react and deal with mouse events like a mouse cursor would, i.e. it should be a mouse cursor"
Correct.
Without such explicit promise from guest userspace you cannot activate seamless mouse mode at all.
Thanks, pq
On Mon, 2022-06-13 at 10:33 +0300, Pekka Paalanen wrote:
On Fri, 10 Jun 2022 14:24:01 +0000 Zack Rusin zackr@vmware.com wrote:
On Fri, 2022-06-10 at 10:59 +0200, Daniel Vetter wrote:
⚠ External Email
On Fri, Jun 10, 2022 at 10:41:05AM +0200, Daniel Vetter wrote:
Hi all,
Kinda top post because the thread is sprawling and I think we need a summary/restart. I think there's at least 3 issues here:
lack of hotspot property support, which means compositors can't really support hotspot with atomic. Which isn't entirely true, because you totally can use atomic for the primary planes/crtcs and the legacy cursor ioctls, but I understand that people might find that a bit silly :-)
Anyway this problme is solved by the patch set here, and I think results in some nice cleanups to boot.
the fact that cursors for virtual drivers are not planes, but really special things. Which just breaks the universal plane kms uapi. That part isn't solved, and I do agree with Simon and Pekka that we really should solve this before we unleash even more compositors onto the atomic paths of virtual drivers.
I think the simplest solution for this is:
- add a new DRM_PLANE_TYPE_VIRTUAL_CURSOR, and set that for these
special cursor planes on all virtual drivers 2. add the new "I understand virtual cursors planes" setparam, filter virtual cursor planes for userspace which doesn't set this (like we do right now if userspace doesn't set the universal plane mode) 3. backport the above patches to all stable kernels 4. make sure the hotspot property is only set on VIRTUAL_CURSOR planes and nothing else in the rebased patch series
Simon also mentioned on irc that these special planes must not expose the CRTC_X/Y property, since that doesn't really do much at all. Or is our understanding of how this all works for commandeered cursors wrong?
Yes, that's the part I don't understand. I don't think I see how the CRTC_X|Y properties aren't used.
I think the confusion might stem from the fact that it appears that the hypervisors/hosts are moving that plane, which is not the case. We never move the plane itself, we redirect the mouse focus/movement, that's what's reducing the latency but don't touch drm internals. I can't speak for every virtualized stack, but what is happening on ours is that we set the image that's on the cursor plane as the cursor on the machine that's running the guest. So when you move the mouse across the screen as you enter the VM window the cursor plane isn't touched, but the local machines cursor changes to what's inside the cursor plane. When the mouse is clicked the mouse device in the guest generates the event with the proper coordinates (hence we need the hotspot to route that event correctly). That's when the guest reacts just like it would normally on native if a mouse event was received.
The actual cursor plane might not be visible while this is happening but even when it's not visible it's still at whatever crtc_x|y the guest thinks it is. crtc_x|y are still only driven by the guests mouse device.
We control the mouse device and visibility of the cursor plane itself based on what's happening in the system but I don't think that's that different from a native system.
Sorry Zack, while I'm sure that is technically correct and very detaily accurate, it's totally not any different to what we have been talking about all along.
We care about how things look like to the end user, and not what property values the guest KMS driver might have for each plane. The KMS UAPI contract is about how things look to the end user, not just what values might be stored in a KMS driver (and then ignored).
It doesn't matter if the CRTC_X/Y properties remain at what the guest userspace set them to, if you are going to hide the "real" virtual cursor plane and instead upload the cursor image to the host window system to be composited independently. You are breaking the UAPI contract. What the end user sees is *not* what the guest OS programmed. That's the whole point.
What you described is the very definition of cursor plane commandeering as I defined it: showing the cursor image not where the guest OS put it.
I never assumed that you would actually reflect host cursor position in the guest KMS cursor plane CRTC_X/Y. You just ignore those values completely in the VM stack levels closer to the end user's eyes in seamless mouse mode. The end effect is the same.
But we don't ignore them, we absolutely need them to set the mouse cursor. This is a two way process, I think Hans mentioned that, mouse might be "seamless", i.e. it's the host routing clicks to the guest, or it might be "guest locked", also known as "gaming mouse", in which case it's fully guest routed/controlled. To have any idea where the cursor is we absolutely require the crtc_x|y.
z
On Mon, 13 Jun 2022 13:14:48 +0000 Zack Rusin zackr@vmware.com wrote:
On Mon, 2022-06-13 at 10:33 +0300, Pekka Paalanen wrote:
On Fri, 10 Jun 2022 14:24:01 +0000 Zack Rusin zackr@vmware.com wrote:
On Fri, 2022-06-10 at 10:59 +0200, Daniel Vetter wrote:
⚠ External Email
On Fri, Jun 10, 2022 at 10:41:05AM +0200, Daniel Vetter wrote:
Hi all,
Kinda top post because the thread is sprawling and I think we need a summary/restart. I think there's at least 3 issues here:
lack of hotspot property support, which means compositors can't really support hotspot with atomic. Which isn't entirely true, because you totally can use atomic for the primary planes/crtcs and the legacy cursor ioctls, but I understand that people might find that a bit silly :-)
Anyway this problme is solved by the patch set here, and I think results in some nice cleanups to boot.
the fact that cursors for virtual drivers are not planes, but really special things. Which just breaks the universal plane kms uapi. That part isn't solved, and I do agree with Simon and Pekka that we really should solve this before we unleash even more compositors onto the atomic paths of virtual drivers.
I think the simplest solution for this is:
- add a new DRM_PLANE_TYPE_VIRTUAL_CURSOR, and set that for these
special cursor planes on all virtual drivers 2. add the new "I understand virtual cursors planes" setparam, filter virtual cursor planes for userspace which doesn't set this (like we do right now if userspace doesn't set the universal plane mode) 3. backport the above patches to all stable kernels 4. make sure the hotspot property is only set on VIRTUAL_CURSOR planes and nothing else in the rebased patch series
Simon also mentioned on irc that these special planes must not expose the CRTC_X/Y property, since that doesn't really do much at all. Or is our understanding of how this all works for commandeered cursors wrong?
Yes, that's the part I don't understand. I don't think I see how the CRTC_X|Y properties aren't used.
I think the confusion might stem from the fact that it appears that the hypervisors/hosts are moving that plane, which is not the case. We never move the plane itself, we redirect the mouse focus/movement, that's what's reducing the latency but don't touch drm internals. I can't speak for every virtualized stack, but what is happening on ours is that we set the image that's on the cursor plane as the cursor on the machine that's running the guest. So when you move the mouse across the screen as you enter the VM window the cursor plane isn't touched, but the local machines cursor changes to what's inside the cursor plane. When the mouse is clicked the mouse device in the guest generates the event with the proper coordinates (hence we need the hotspot to route that event correctly). That's when the guest reacts just like it would normally on native if a mouse event was received.
The actual cursor plane might not be visible while this is happening but even when it's not visible it's still at whatever crtc_x|y the guest thinks it is. crtc_x|y are still only driven by the guests mouse device.
We control the mouse device and visibility of the cursor plane itself based on what's happening in the system but I don't think that's that different from a native system.
Sorry Zack, while I'm sure that is technically correct and very detaily accurate, it's totally not any different to what we have been talking about all along.
We care about how things look like to the end user, and not what property values the guest KMS driver might have for each plane. The KMS UAPI contract is about how things look to the end user, not just what values might be stored in a KMS driver (and then ignored).
It doesn't matter if the CRTC_X/Y properties remain at what the guest userspace set them to, if you are going to hide the "real" virtual cursor plane and instead upload the cursor image to the host window system to be composited independently. You are breaking the UAPI contract. What the end user sees is *not* what the guest OS programmed. That's the whole point.
What you described is the very definition of cursor plane commandeering as I defined it: showing the cursor image not where the guest OS put it.
I never assumed that you would actually reflect host cursor position in the guest KMS cursor plane CRTC_X/Y. You just ignore those values completely in the VM stack levels closer to the end user's eyes in seamless mouse mode. The end effect is the same.
But we don't ignore them, we absolutely need them to set the mouse cursor. This is a two way process, I think Hans mentioned that, mouse might be "seamless", i.e. it's the host routing clicks to the guest, or it might be "guest locked", also known as "gaming mouse", in which case it's fully guest routed/controlled. To have any idea where the cursor is we absolutely require the crtc_x|y.
Ok, so seamless mouse mode ignores CRTC_X/Y. This is the commandeered mode.
The normal non-commandeered mode, or what you seem to call "guest locked" which I guess also includes grabbing the mouse into the VM viewer window in the host/viewer system, requires CRTC_X/Y. That's clear.
In other words, the VM stack is switching between seamless pointer, normal pointer, and normal pointer with a grab on the VM viewer winsys, right?
This only means that virtual cursor planes do need CRTC_X/Y properties. That's fine.
The VM stack is still breaking the KMS UAPI contract if the VM stack enters seamless pointer mode without an explicit approval from the guest userspace. You can't say it's ok to do seamless pointer if you *sometimes* also do normal pointer instead, that's not enough.
Thanks, pq
On Mon, 2022-06-13 at 17:25 +0300, Pekka Paalanen wrote:
On Mon, 13 Jun 2022 13:14:48 +0000 Zack Rusin zackr@vmware.com wrote:
On Mon, 2022-06-13 at 10:33 +0300, Pekka Paalanen wrote:
On Fri, 10 Jun 2022 14:24:01 +0000 Zack Rusin zackr@vmware.com wrote:
On Fri, 2022-06-10 at 10:59 +0200, Daniel Vetter wrote:
⚠ External Email
On Fri, Jun 10, 2022 at 10:41:05AM +0200, Daniel Vetter wrote:
Hi all,
Kinda top post because the thread is sprawling and I think we need a summary/restart. I think there's at least 3 issues here:
lack of hotspot property support, which means compositors can't really support hotspot with atomic. Which isn't entirely true, because you totally can use atomic for the primary planes/crtcs and the legacy cursor ioctls, but I understand that people might find that a bit silly :-)
Anyway this problme is solved by the patch set here, and I think results in some nice cleanups to boot.
the fact that cursors for virtual drivers are not planes, but really special things. Which just breaks the universal plane kms uapi. That part isn't solved, and I do agree with Simon and Pekka that we really should solve this before we unleash even more compositors onto the atomic paths of virtual drivers.
I think the simplest solution for this is:
- add a new DRM_PLANE_TYPE_VIRTUAL_CURSOR, and set that for these
special cursor planes on all virtual drivers 2. add the new "I understand virtual cursors planes" setparam, filter virtual cursor planes for userspace which doesn't set this (like we do right now if userspace doesn't set the universal plane mode) 3. backport the above patches to all stable kernels 4. make sure the hotspot property is only set on VIRTUAL_CURSOR planes and nothing else in the rebased patch series
Simon also mentioned on irc that these special planes must not expose the CRTC_X/Y property, since that doesn't really do much at all. Or is our understanding of how this all works for commandeered cursors wrong?
Yes, that's the part I don't understand. I don't think I see how the CRTC_X|Y properties aren't used.
I think the confusion might stem from the fact that it appears that the hypervisors/hosts are moving that plane, which is not the case. We never move the plane itself, we redirect the mouse focus/movement, that's what's reducing the latency but don't touch drm internals. I can't speak for every virtualized stack, but what is happening on ours is that we set the image that's on the cursor plane as the cursor on the machine that's running the guest. So when you move the mouse across the screen as you enter the VM window the cursor plane isn't touched, but the local machines cursor changes to what's inside the cursor plane. When the mouse is clicked the mouse device in the guest generates the event with the proper coordinates (hence we need the hotspot to route that event correctly). That's when the guest reacts just like it would normally on native if a mouse event was received.
The actual cursor plane might not be visible while this is happening but even when it's not visible it's still at whatever crtc_x|y the guest thinks it is. crtc_x|y are still only driven by the guests mouse device.
We control the mouse device and visibility of the cursor plane itself based on what's happening in the system but I don't think that's that different from a native system.
Sorry Zack, while I'm sure that is technically correct and very detaily accurate, it's totally not any different to what we have been talking about all along.
We care about how things look like to the end user, and not what property values the guest KMS driver might have for each plane. The KMS UAPI contract is about how things look to the end user, not just what values might be stored in a KMS driver (and then ignored).
It doesn't matter if the CRTC_X/Y properties remain at what the guest userspace set them to, if you are going to hide the "real" virtual cursor plane and instead upload the cursor image to the host window system to be composited independently. You are breaking the UAPI contract. What the end user sees is *not* what the guest OS programmed. That's the whole point.
What you described is the very definition of cursor plane commandeering as I defined it: showing the cursor image not where the guest OS put it.
I never assumed that you would actually reflect host cursor position in the guest KMS cursor plane CRTC_X/Y. You just ignore those values completely in the VM stack levels closer to the end user's eyes in seamless mouse mode. The end effect is the same.
But we don't ignore them, we absolutely need them to set the mouse cursor. This is a two way process, I think Hans mentioned that, mouse might be "seamless", i.e. it's the host routing clicks to the guest, or it might be "guest locked", also known as "gaming mouse", in which case it's fully guest routed/controlled. To have any idea where the cursor is we absolutely require the crtc_x|y.
Ok, so seamless mouse mode ignores CRTC_X/Y. This is the commandeered mode.
The normal non-commandeered mode, or what you seem to call "guest locked" which I guess also includes grabbing the mouse into the VM viewer window in the host/viewer system, requires CRTC_X/Y. That's clear.
In other words, the VM stack is switching between seamless pointer, normal pointer, and normal pointer with a grab on the VM viewer winsys, right?
This only means that virtual cursor planes do need CRTC_X/Y properties. That's fine.
The VM stack is still breaking the KMS UAPI contract if the VM stack enters seamless pointer mode without an explicit approval from the guest userspace. You can't say it's ok to do seamless pointer if you *sometimes* also do normal pointer instead, that's not enough.
I don't think I ever said that. In general I'm not making any philosophical argument, all I'm saying is that "virtualized drivers require hotspot info". Simon and you are saying that we can't get it until we fix other issues with virtualized drivers and I'm simply pointing out that your solutions do not work. When we started I did mention that this is a lot bigger issue, that's been present for years and will be hard to solve, which is why we're off to crazy town right now talking about essentially forking kms for virtualized drivers.
I thought the solution consisting of an addition of a DRM_CLIENT_CAP_VIRTUAL_CURSOR_AWARE flag and hiding the cursor plane in virtualized drivers for clients that advertise DRM_CLIENT_CAP_ATOMIC but not DRM_CLIENT_CAP_VIRTUAL_CURSOR_AWARE was simple enough and enough to get weston using software cursors while having a clear path for gnome-shell and kwin to use atomic with virtualized drivers without rewriting them. If we can't agree on that then, at this point, it might be better if we just schedule an irc and/or video conference with all interested parties to figure this out.
z
On Mon, 13 Jun 2022 14:54:57 +0000 Zack Rusin zackr@vmware.com wrote:
On Mon, 2022-06-13 at 17:25 +0300, Pekka Paalanen wrote:
On Mon, 13 Jun 2022 13:14:48 +0000 Zack Rusin zackr@vmware.com wrote:
On Mon, 2022-06-13 at 10:33 +0300, Pekka Paalanen wrote:
On Fri, 10 Jun 2022 14:24:01 +0000 Zack Rusin zackr@vmware.com wrote:
On Fri, 2022-06-10 at 10:59 +0200, Daniel Vetter wrote:
⚠ External Email
On Fri, Jun 10, 2022 at 10:41:05AM +0200, Daniel Vetter wrote: > Hi all, > > Kinda top post because the thread is sprawling and I think we need a > summary/restart. I think there's at least 3 issues here: > > - lack of hotspot property support, which means compositors can't really > support hotspot with atomic. Which isn't entirely true, because you > totally can use atomic for the primary planes/crtcs and the legacy > cursor ioctls, but I understand that people might find that a bit silly :-) > > Anyway this problme is solved by the patch set here, and I think results > in some nice cleanups to boot. > > - the fact that cursors for virtual drivers are not planes, but really > special things. Which just breaks the universal plane kms uapi. That > part isn't solved, and I do agree with Simon and Pekka that we really > should solve this before we unleash even more compositors onto the > atomic paths of virtual drivers. > > I think the simplest solution for this is: > 1. add a new DRM_PLANE_TYPE_VIRTUAL_CURSOR, and set that for these > special cursor planes on all virtual drivers > 2. add the new "I understand virtual cursors planes" setparam, filter > virtual cursor planes for userspace which doesn't set this (like we do > right now if userspace doesn't set the universal plane mode) > 3. backport the above patches to all stable kernels > 4. make sure the hotspot property is only set on VIRTUAL_CURSOR planes > and nothing else in the rebased patch series
Simon also mentioned on irc that these special planes must not expose the CRTC_X/Y property, since that doesn't really do much at all. Or is our understanding of how this all works for commandeered cursors wrong?
Yes, that's the part I don't understand. I don't think I see how the CRTC_X|Y properties aren't used.
I think the confusion might stem from the fact that it appears that the hypervisors/hosts are moving that plane, which is not the case. We never move the plane itself, we redirect the mouse focus/movement, that's what's reducing the latency but don't touch drm internals. I can't speak for every virtualized stack, but what is happening on ours is that we set the image that's on the cursor plane as the cursor on the machine that's running the guest. So when you move the mouse across the screen as you enter the VM window the cursor plane isn't touched, but the local machines cursor changes to what's inside the cursor plane. When the mouse is clicked the mouse device in the guest generates the event with the proper coordinates (hence we need the hotspot to route that event correctly). That's when the guest reacts just like it would normally on native if a mouse event was received.
The actual cursor plane might not be visible while this is happening but even when it's not visible it's still at whatever crtc_x|y the guest thinks it is. crtc_x|y are still only driven by the guests mouse device.
We control the mouse device and visibility of the cursor plane itself based on what's happening in the system but I don't think that's that different from a native system.
Sorry Zack, while I'm sure that is technically correct and very detaily accurate, it's totally not any different to what we have been talking about all along.
We care about how things look like to the end user, and not what property values the guest KMS driver might have for each plane. The KMS UAPI contract is about how things look to the end user, not just what values might be stored in a KMS driver (and then ignored).
It doesn't matter if the CRTC_X/Y properties remain at what the guest userspace set them to, if you are going to hide the "real" virtual cursor plane and instead upload the cursor image to the host window system to be composited independently. You are breaking the UAPI contract. What the end user sees is *not* what the guest OS programmed. That's the whole point.
What you described is the very definition of cursor plane commandeering as I defined it: showing the cursor image not where the guest OS put it.
I never assumed that you would actually reflect host cursor position in the guest KMS cursor plane CRTC_X/Y. You just ignore those values completely in the VM stack levels closer to the end user's eyes in seamless mouse mode. The end effect is the same.
But we don't ignore them, we absolutely need them to set the mouse cursor. This is a two way process, I think Hans mentioned that, mouse might be "seamless", i.e. it's the host routing clicks to the guest, or it might be "guest locked", also known as "gaming mouse", in which case it's fully guest routed/controlled. To have any idea where the cursor is we absolutely require the crtc_x|y.
Ok, so seamless mouse mode ignores CRTC_X/Y. This is the commandeered mode.
The normal non-commandeered mode, or what you seem to call "guest locked" which I guess also includes grabbing the mouse into the VM viewer window in the host/viewer system, requires CRTC_X/Y. That's clear.
In other words, the VM stack is switching between seamless pointer, normal pointer, and normal pointer with a grab on the VM viewer winsys, right?
This only means that virtual cursor planes do need CRTC_X/Y properties. That's fine.
The VM stack is still breaking the KMS UAPI contract if the VM stack enters seamless pointer mode without an explicit approval from the guest userspace. You can't say it's ok to do seamless pointer if you *sometimes* also do normal pointer instead, that's not enough.
I don't think I ever said that.
I did read that from your replies to the claims that you are ignoring CRTC_X/Y. It's the impression I got.
Saying that you don't ignore CRTC_X/Y but also implying that you are not always using them as the cursor image position did not make sense to me.
In general I'm not making any philosophical argument, all I'm saying is that "virtualized drivers require hotspot info". Simon and you are saying that we can't get it until we fix other issues with virtualized drivers and I'm simply pointing out that your solutions do not work. When we started I did mention that this is a lot bigger issue, that's been present for years and will be hard to solve, which is why we're off to crazy town right now talking about essentially forking kms for virtualized drivers.
The reason I am saying that you need to fix other issues with virtualized drivers at the same time is because those other issues are the sole and only reason why the driver would ever need hotspot info.
Hotspot info must not be necessary for correct KMS operation, yet you seem to insist it is. You assume that cursor plane commandeering is ok to do. But it is not ok without adding the KMS UAPI that would allow it: a way for guest userspace to explicitly say that commandeering will be ok.
If and only if guest userspace says commandeering is ok, then you can require hotspot info. On the other hand, you cannot retrofit hotspot info by saying that if a driver exposes hotspot properties then all KMS clients must set them. That would be a kernel regression by definition: KMS clients that used to abide the KMS UAPI contract are suddenly breaking the contract because the kernel changed the contract.
Therefore I very much disagree that virtualized drivers need hotspot info. They do not strictly need hotspot info for correct operation, they need it only for making the user experience more smooth, which is an optional optimization. That optimization may be very important in practise, but it's still an optimization and is generally not expected by KMS clients.
I have not understood yet why our proposed solutions do not work. You keep saying they don't work, but how do they not work?
It is exactly that forking of KMS that I do not want. Hence we need explicit approval from KMS clients that doing those tricks you want to do will be ok. If you do those tricks without explicit KMS client approval, then you have forked KMS: we have two kinds of drivers, they behave differently, and there is no way for userspace to know which way they behave, aside for special-casing by driver name, which would be a maintenance nightmare.
I would not want virtualized drivers to do any of those tricks to begin with, at all, because that's not what KMS is designed for. But I have already given in to the idea of the use case (seamless pointer mode) that requires hotspot info, so I am not fighting against that.
I thought the solution consisting of an addition of a DRM_CLIENT_CAP_VIRTUAL_CURSOR_AWARE flag and hiding the cursor plane in virtualized drivers for clients that advertise DRM_CLIENT_CAP_ATOMIC but not DRM_CLIENT_CAP_VIRTUAL_CURSOR_AWARE was simple enough and enough to get weston using software cursors while having a clear path for gnome-shell and kwin to use atomic with virtualized drivers without rewriting them. If we can't agree on that then, at this point, it might be better if we just schedule an irc and/or video conference with all interested parties to figure this out.
I thought we already agreed on that. I think it's a fine plan from userspace perspective, and it's along the lines of what Daniel Vetter wrote which I agreed to.
I don't care if there is cursor plane not being exposed or yes being exposed. I only care that if a cursor plane is exposed, and it is used by a KMS client, and the KMS client does not explicitly indicate that commandeering is ok, then commandeering cannot happen.
Thanks, pq
On Tue, 2022-06-14 at 10:36 +0300, Pekka Paalanen wrote:
On Mon, 13 Jun 2022 14:54:57 +0000 Zack Rusin zackr@vmware.com wrote:
On Mon, 2022-06-13 at 17:25 +0300, Pekka Paalanen wrote:
On Mon, 13 Jun 2022 13:14:48 +0000 Zack Rusin zackr@vmware.com wrote:
On Mon, 2022-06-13 at 10:33 +0300, Pekka Paalanen wrote:
On Fri, 10 Jun 2022 14:24:01 +0000 Zack Rusin zackr@vmware.com wrote:
On Fri, 2022-06-10 at 10:59 +0200, Daniel Vetter wrote: > ⚠ External Email > > On Fri, Jun 10, 2022 at 10:41:05AM +0200, Daniel Vetter wrote: > > Hi all, > > > > Kinda top post because the thread is sprawling and I think we need a > > summary/restart. I think there's at least 3 issues here: > > > > - lack of hotspot property support, which means compositors can't really > > support hotspot with atomic. Which isn't entirely true, because you > > totally can use atomic for the primary planes/crtcs and the legacy > > cursor ioctls, but I understand that people might find that a bit silly :-) > > > > Anyway this problme is solved by the patch set here, and I think results > > in some nice cleanups to boot. > > > > - the fact that cursors for virtual drivers are not planes, but really > > special things. Which just breaks the universal plane kms uapi. That > > part isn't solved, and I do agree with Simon and Pekka that we really > > should solve this before we unleash even more compositors onto the > > atomic paths of virtual drivers. > > > > I think the simplest solution for this is: > > 1. add a new DRM_PLANE_TYPE_VIRTUAL_CURSOR, and set that for these > > special cursor planes on all virtual drivers > > 2. add the new "I understand virtual cursors planes" setparam, filter > > virtual cursor planes for userspace which doesn't set this (like we do > > right now if userspace doesn't set the universal plane mode) > > 3. backport the above patches to all stable kernels > > 4. make sure the hotspot property is only set on VIRTUAL_CURSOR planes > > and nothing else in the rebased patch series > > Simon also mentioned on irc that these special planes must not expose the > CRTC_X/Y property, since that doesn't really do much at all. Or is our > understanding of how this all works for commandeered cursors wrong?
Yes, that's the part I don't understand. I don't think I see how the CRTC_X|Y properties aren't used.
I think the confusion might stem from the fact that it appears that the hypervisors/hosts are moving that plane, which is not the case. We never move the plane itself, we redirect the mouse focus/movement, that's what's reducing the latency but don't touch drm internals. I can't speak for every virtualized stack, but what is happening on ours is that we set the image that's on the cursor plane as the cursor on the machine that's running the guest. So when you move the mouse across the screen as you enter the VM window the cursor plane isn't touched, but the local machines cursor changes to what's inside the cursor plane. When the mouse is clicked the mouse device in the guest generates the event with the proper coordinates (hence we need the hotspot to route that event correctly). That's when the guest reacts just like it would normally on native if a mouse event was received.
The actual cursor plane might not be visible while this is happening but even when it's not visible it's still at whatever crtc_x|y the guest thinks it is. crtc_x|y are still only driven by the guests mouse device.
We control the mouse device and visibility of the cursor plane itself based on what's happening in the system but I don't think that's that different from a native system.
Sorry Zack, while I'm sure that is technically correct and very detaily accurate, it's totally not any different to what we have been talking about all along.
We care about how things look like to the end user, and not what property values the guest KMS driver might have for each plane. The KMS UAPI contract is about how things look to the end user, not just what values might be stored in a KMS driver (and then ignored).
It doesn't matter if the CRTC_X/Y properties remain at what the guest userspace set them to, if you are going to hide the "real" virtual cursor plane and instead upload the cursor image to the host window system to be composited independently. You are breaking the UAPI contract. What the end user sees is *not* what the guest OS programmed. That's the whole point.
What you described is the very definition of cursor plane commandeering as I defined it: showing the cursor image not where the guest OS put it.
I never assumed that you would actually reflect host cursor position in the guest KMS cursor plane CRTC_X/Y. You just ignore those values completely in the VM stack levels closer to the end user's eyes in seamless mouse mode. The end effect is the same.
But we don't ignore them, we absolutely need them to set the mouse cursor. This is a two way process, I think Hans mentioned that, mouse might be "seamless", i.e. it's the host routing clicks to the guest, or it might be "guest locked", also known as "gaming mouse", in which case it's fully guest routed/controlled. To have any idea where the cursor is we absolutely require the crtc_x|y.
Ok, so seamless mouse mode ignores CRTC_X/Y. This is the commandeered mode.
The normal non-commandeered mode, or what you seem to call "guest locked" which I guess also includes grabbing the mouse into the VM viewer window in the host/viewer system, requires CRTC_X/Y. That's clear.
In other words, the VM stack is switching between seamless pointer, normal pointer, and normal pointer with a grab on the VM viewer winsys, right?
This only means that virtual cursor planes do need CRTC_X/Y properties. That's fine.
The VM stack is still breaking the KMS UAPI contract if the VM stack enters seamless pointer mode without an explicit approval from the guest userspace. You can't say it's ok to do seamless pointer if you *sometimes* also do normal pointer instead, that's not enough.
I don't think I ever said that.
I did read that from your replies to the claims that you are ignoring CRTC_X/Y. It's the impression I got.
Saying that you don't ignore CRTC_X/Y but also implying that you are not always using them as the cursor image position did not make sense to me.
In general I'm not making any philosophical argument, all I'm saying is that "virtualized drivers require hotspot info". Simon and you are saying that we can't get it until we fix other issues with virtualized drivers and I'm simply pointing out that your solutions do not work. When we started I did mention that this is a lot bigger issue, that's been present for years and will be hard to solve, which is why we're off to crazy town right now talking about essentially forking kms for virtualized drivers.
The reason I am saying that you need to fix other issues with virtualized drivers at the same time is because those other issues are the sole and only reason why the driver would ever need hotspot info.
Hotspot info must not be necessary for correct KMS operation, yet you seem to insist it is. You assume that cursor plane commandeering is ok to do. But it is not ok without adding the KMS UAPI that would allow it: a way for guest userspace to explicitly say that commandeering will be ok.
If and only if guest userspace says commandeering is ok, then you can require hotspot info. On the other hand, you cannot retrofit hotspot info by saying that if a driver exposes hotspot properties then all KMS clients must set them. That would be a kernel regression by definition: KMS clients that used to abide the KMS UAPI contract are suddenly breaking the contract because the kernel changed the contract.
Therefore I very much disagree that virtualized drivers need hotspot info. They do not strictly need hotspot info for correct operation, they need it only for making the user experience more smooth, which is an optional optimization. That optimization may be very important in practise, but it's still an optimization and is generally not expected by KMS clients.
I strongly disagree with that (both the sentiment towards hotspots and the client handling of it). I don't think we have to agree on reasoning here at all to make progress though so I'm going to let it go (we can always continue on irc or email if you'd like to try to conclude this bit but we could all use a few days of break from this discussion probably).
I thought the solution consisting of an addition of a DRM_CLIENT_CAP_VIRTUAL_CURSOR_AWARE flag and hiding the cursor plane in virtualized drivers for clients that advertise DRM_CLIENT_CAP_ATOMIC but not DRM_CLIENT_CAP_VIRTUAL_CURSOR_AWARE was simple enough and enough to get weston using software cursors while having a clear path for gnome-shell and kwin to use atomic with virtualized drivers without rewriting them. If we can't agree on that then, at this point, it might be better if we just schedule an irc and/or video conference with all interested parties to figure this out.
I thought we already agreed on that. I think it's a fine plan from userspace perspective, and it's along the lines of what Daniel Vetter wrote which I agreed to.
I don't care if there is cursor plane not being exposed or yes being exposed. I only care that if a cursor plane is exposed, and it is used by a KMS client, and the KMS client does not explicitly indicate that commandeering is ok, then commandeering cannot happen.
ok, sounds good, looks like we have a way forward. Let me respin the series with the above suggested patch plus add the new cap code to mutter to get gnome-shell working so that we can all see how it would look/work in practice and see if we're all happy enough with it (or at least less unhappy with it than the current situation).
z
Hi,
On Tue, 14 Jun 2022 at 15:40, Zack Rusin zackr@vmware.com wrote:
On Tue, 2022-06-14 at 10:36 +0300, Pekka Paalanen wrote:
The reason I am saying that you need to fix other issues with virtualized drivers at the same time is because those other issues are the sole and only reason why the driver would ever need hotspot info.
Hotspot info must not be necessary for correct KMS operation, yet you seem to insist it is. You assume that cursor plane commandeering is ok to do. But it is not ok without adding the KMS UAPI that would allow it: a way for guest userspace to explicitly say that commandeering will be ok.
If and only if guest userspace says commandeering is ok, then you can require hotspot info. On the other hand, you cannot retrofit hotspot info by saying that if a driver exposes hotspot properties then all KMS clients must set them. That would be a kernel regression by definition: KMS clients that used to abide the KMS UAPI contract are suddenly breaking the contract because the kernel changed the contract.
Therefore I very much disagree that virtualized drivers need hotspot info. They do not strictly need hotspot info for correct operation, they need it only for making the user experience more smooth, which is an optional optimization. That optimization may be very important in practise, but it's still an optimization and is generally not expected by KMS clients.
I strongly disagree with that (both the sentiment towards hotspots and the client handling of it). I don't think we have to agree on reasoning here at all to make progress though so I'm going to let it go (we can always continue on irc or email if you'd like to try to conclude this bit but we could all use a few days of break from this discussion probably).
Well, it's just coming from two different directions: * many current KMS clients want the cursor plane to be displayed as the client-programmed plane properties indicate, and the output can be nonsensical if they aren't * VMware optimises the cursor by displaying the cursor plane not as the client-programmed plane properties indicate, and the output is sometimes good (faster response!) but sometimes bad (nonsensical display!)
The client cap sounds good. As a further suggestion, given that universal planes are supposed to make planes, er, universal, rather than imbued with magical special behaviour, how about _also_ adding an 'is cursor' plane prop which userspace has to set to 1 to indicate that the output is a cursor and has the hotspot correctly set and the 'display hardware' is free to make the cursor fly around the screen in accordance with the input events it sends? That way it's really really clear what's happening and no-one's getting surprised when 'the right thing' doesn't happen, not least because it's really clear what 'the right thing' is.
Cheers, Daniel
On Friday, June 10th, 2022 at 10:41, Daniel Vetter daniel@ffwll.ch wrote:
Anything I've missed? Or got completely wrong?
This plan looks good to me.
As Pekka mentionned, I'd also like to have a conversation of how far we want to push virtualized driver features. I think KMS support is a good feature to have to spin up a VM and have all of the basics working. However I don't think it's a good idea to try to plumb an ever-growing list of fancy features (seamless integration of guest windows into the host, HiDPI, multi-monitor, etc) into KMS. You'd just end up re-inventing Wayland or RDP on top of KMS. Instead of re-inventing these, just use RDP or waypipe or X11 forwarding directly.
So I think we need to draw a line somewhere, and decide e.g. that virtualized cursors are fine to add in KMS, but HiDPI is not.
On Fri, Jun 10, 2022 at 09:15:35AM +0000, Simon Ser wrote:
On Friday, June 10th, 2022 at 10:41, Daniel Vetter daniel@ffwll.ch wrote:
Anything I've missed? Or got completely wrong?
This plan looks good to me.
As Pekka mentionned, I'd also like to have a conversation of how far we want to push virtualized driver features. I think KMS support is a good feature to have to spin up a VM and have all of the basics working. However I don't think it's a good idea to try to plumb an ever-growing list of fancy features (seamless integration of guest windows into the host, HiDPI, multi-monitor, etc) into KMS. You'd just end up re-inventing Wayland or RDP on top of KMS. Instead of re-inventing these, just use RDP or waypipe or X11 forwarding directly.
So I think we need to draw a line somewhere, and decide e.g. that virtualized cursors are fine to add in KMS, but HiDPI is not.
It's getting a bit far off-topic, but google cros team has an out-of-tree (at least I think it's not merged yet) wayland-virtio driver for exactly this use-case. Trying to move towards something like that for fancy virtualized setups sounds like the better approach indeed, with kms just as the bare-bones fallback option. -Daniel
Hi,
As Pekka mentionned, I'd also like to have a conversation of how far we want to push virtualized driver features. I think KMS support is a good feature to have to spin up a VM and have all of the basics working. However I don't think it's a good idea to try to plumb an ever-growing list of fancy features (seamless integration of guest windows into the host, HiDPI, multi-monitor, etc) into KMS. You'd just end up re-inventing Wayland or RDP on top of KMS. Instead of re-inventing these, just use RDP or waypipe or X11 forwarding directly.
So I think we need to draw a line somewhere, and decide e.g. that virtualized cursors are fine to add in KMS, but HiDPI is not.
What is the problem with HiDPI? qemu generates standard edid blobs, there should be no need to special-case virtualized drivers in any way.
What is the problem with multi-monitor? That isn't much different than physical multi-monitor either.
One little thing though: On physical hardware you just don't know which monitor is left and which is right until the user tells you. In case of a virtual multi-monitor setup we know how the two windows for the two virtual monitors are arranged on the host and can pass that as hint to the guest (not sure whenever *that* is the purpose of the suggested_{x,y} properties).
It's getting a bit far off-topic, but google cros team has an out-of-tree (at least I think it's not merged yet) wayland-virtio driver for exactly this use-case. Trying to move towards something like that for fancy virtualized setups sounds like the better approach indeed, with kms just as the bare-bones fallback option.
virtio-gpu got the ability to attach uuids to objects, to allow them being identified on the host side. So it could be that wayland-virtio still uses kms for framebuffers (disclaimer: don't know how wayland-virtio works in detail). But, yes, all the scanout + cursor handling would be out of the way, virtio-gpu would "only" handle fast buffer sharing.
take care, Gerd
On Friday, June 10th, 2022 at 14:36, Gerd Hoffmann kraxel@redhat.com wrote:
Hi,
As Pekka mentionned, I'd also like to have a conversation of how far we want to push virtualized driver features. I think KMS support is a good feature to have to spin up a VM and have all of the basics working. However I don't think it's a good idea to try to plumb an ever-growing list of fancy features (seamless integration of guest windows into the host, HiDPI, multi-monitor, etc) into KMS. You'd just end up re-inventing Wayland or RDP on top of KMS. Instead of re-inventing these, just use RDP or waypipe or X11 forwarding directly.
So I think we need to draw a line somewhere, and decide e.g. that virtualized cursors are fine to add in KMS, but HiDPI is not.
What is the problem with HiDPI? qemu generates standard edid blobs, there should be no need to special-case virtualized drivers in any way.
What is the problem with multi-monitor? That isn't much different than physical multi-monitor either.
One little thing though: On physical hardware you just don't know which monitor is left and which is right until the user tells you. In case of a virtual multi-monitor setup we know how the two windows for the two virtual monitors are arranged on the host and can pass that as hint to the guest (not sure whenever that is the purpose of the suggested_{x,y} properties).
The problem with suggested_x/y is described here: https://lore.kernel.org/dri-devel/20220610123629.fgu2em3fto53fpfy@sirius.hom...
HiDPI would need a way to propagate the scale factor back-and-forth: the VM viewer needs to advertise the preferred scale to the guest compositor, and the guest compositor needs to indicate the scale it renders with to the VM viewer.
Sounds familiar? Yup, that's exactly the Wayland protocol. Do we really want to replicate the Wayland protocol in KMS? I'm not so sure.
It's getting a bit far off-topic, but google cros team has an out-of-tree (at least I think it's not merged yet) wayland-virtio driver for exactly this use-case. Trying to move towards something like that for fancy virtualized setups sounds like the better approach indeed, with kms just as the bare-bones fallback option.
virtio-gpu got the ability to attach uuids to objects, to allow them being identified on the host side. So it could be that wayland-virtio still uses kms for framebuffers (disclaimer: don't know how wayland-virtio works in detail). But, yes, all the scanout + cursor handling would be out of the way, virtio-gpu would "only" handle fast buffer sharing.
wayland-virtio is not used with KMS. wayland-virtio proxies the Wayland protocol between the host and the guest, so the guest doesn't use KMS in that case.
Hi,
On 6/10/22 14:53, Simon Ser wrote:
On Friday, June 10th, 2022 at 14:36, Gerd Hoffmann kraxel@redhat.com wrote:
Hi,
As Pekka mentionned, I'd also like to have a conversation of how far we want to push virtualized driver features. I think KMS support is a good feature to have to spin up a VM and have all of the basics working. However I don't think it's a good idea to try to plumb an ever-growing list of fancy features (seamless integration of guest windows into the host, HiDPI, multi-monitor, etc) into KMS. You'd just end up re-inventing Wayland or RDP on top of KMS. Instead of re-inventing these, just use RDP or waypipe or X11 forwarding directly.
So I think we need to draw a line somewhere, and decide e.g. that virtualized cursors are fine to add in KMS, but HiDPI is not.
What is the problem with HiDPI? qemu generates standard edid blobs, there should be no need to special-case virtualized drivers in any way.
What is the problem with multi-monitor? That isn't much different than physical multi-monitor either.
One little thing though: On physical hardware you just don't know which monitor is left and which is right until the user tells you. In case of a virtual multi-monitor setup we know how the two windows for the two virtual monitors are arranged on the host and can pass that as hint to the guest (not sure whenever that is the purpose of the suggested_{x,y} properties).
The problem with suggested_x/y is described here: https://lore.kernel.org/dri-devel/20220610123629.fgu2em3fto53fpfy@sirius.hom...
HiDPI would need a way to propagate the scale factor back-and-forth: the VM viewer needs to advertise the preferred scale to the guest compositor, and the guest compositor needs to indicate the scale it renders with to the VM viewer.
Sounds familiar? Yup, that's exactly the Wayland protocol. Do we really want to replicate the Wayland protocol in KMS? I'm not so sure.
It's getting a bit far off-topic, but google cros team has an out-of-tree (at least I think it's not merged yet) wayland-virtio driver for exactly this use-case. Trying to move towards something like that for fancy virtualized setups sounds like the better approach indeed, with kms just as the bare-bones fallback option.
virtio-gpu got the ability to attach uuids to objects, to allow them being identified on the host side. So it could be that wayland-virtio still uses kms for framebuffers (disclaimer: don't know how wayland-virtio works in detail). But, yes, all the scanout + cursor handling would be out of the way, virtio-gpu would "only" handle fast buffer sharing.
wayland-virtio is not used with KMS. wayland-virtio proxies the Wayland protocol between the host and the guest, so the guest doesn't use KMS in that case.
It would be more correct to say: wayland clients inside the guest don't talk to a compositor inside the guest (but rather one outside the guest) and thus also don't depend (indirectly) on\ having kms inside the guest.
But the guest likely still needs kms for e.g. the kernel console to e.g. debug boot failures. Note this could be done over a serial console too, so in some cases whatever "video-card" emulation the guest has could theoretically go away. But it is also completely possible for a guest to have both some emulated video-card which offers a kms API to userspace as well as wayland-virtio.
Regards,
Hans
On Sat, 11 Jun 2022 17:34:50 +0200 Hans de Goede hdegoede@redhat.com wrote:
Hi,
On 6/10/22 14:53, Simon Ser wrote:
On Friday, June 10th, 2022 at 14:36, Gerd Hoffmann kraxel@redhat.com wrote:
Hi,
As Pekka mentionned, I'd also like to have a conversation of how far we want to push virtualized driver features. I think KMS support is a good feature to have to spin up a VM and have all of the basics working. However I don't think it's a good idea to try to plumb an ever-growing list of fancy features (seamless integration of guest windows into the host, HiDPI, multi-monitor, etc) into KMS. You'd just end up re-inventing Wayland or RDP on top of KMS. Instead of re-inventing these, just use RDP or waypipe or X11 forwarding directly.
So I think we need to draw a line somewhere, and decide e.g. that virtualized cursors are fine to add in KMS, but HiDPI is not.
What is the problem with HiDPI? qemu generates standard edid blobs, there should be no need to special-case virtualized drivers in any way.
What is the problem with multi-monitor? That isn't much different than physical multi-monitor either.
One little thing though: On physical hardware you just don't know which monitor is left and which is right until the user tells you. In case of a virtual multi-monitor setup we know how the two windows for the two virtual monitors are arranged on the host and can pass that as hint to the guest (not sure whenever that is the purpose of the suggested_{x,y} properties).
The problem with suggested_x/y is described here: https://lore.kernel.org/dri-devel/20220610123629.fgu2em3fto53fpfy@sirius.hom...
HiDPI would need a way to propagate the scale factor back-and-forth: the VM viewer needs to advertise the preferred scale to the guest compositor, and the guest compositor needs to indicate the scale it renders with to the VM viewer.
Sounds familiar? Yup, that's exactly the Wayland protocol. Do we really want to replicate the Wayland protocol in KMS? I'm not so sure.
It's getting a bit far off-topic, but google cros team has an out-of-tree (at least I think it's not merged yet) wayland-virtio driver for exactly this use-case. Trying to move towards something like that for fancy virtualized setups sounds like the better approach indeed, with kms just as the bare-bones fallback option.
virtio-gpu got the ability to attach uuids to objects, to allow them being identified on the host side. So it could be that wayland-virtio still uses kms for framebuffers (disclaimer: don't know how wayland-virtio works in detail). But, yes, all the scanout + cursor handling would be out of the way, virtio-gpu would "only" handle fast buffer sharing.
wayland-virtio is not used with KMS. wayland-virtio proxies the Wayland protocol between the host and the guest, so the guest doesn't use KMS in that case.
It would be more correct to say: wayland clients inside the guest don't talk to a compositor inside the guest (but rather one outside the guest) and thus also don't depend (indirectly) on\ having kms inside the guest.
Both ways may work. There are many Wayland compositors that can present to another Wayland display.
In my mind which architecture you use depends on whether you want a root window for the VM (with the ability to run any desktop in the guest as-is) or whether you want root-window-less integration of VM/guest applications into the host desktop.
But the guest likely still needs kms for e.g. the kernel console to e.g. debug boot failures.
Sure. That doesn't need cursor planes at all, or anything else that virtualised guest KMS drivers are specially adding to make specifically a desktop experience more smooth.
Note this could be done over a serial console too, so in some cases whatever "video-card" emulation the guest has could theoretically go away. But it is also completely possible for a guest to have both some emulated video-card which offers a kms API to userspace as well as wayland-virtio.
Of course. However the question here is, how far are we willing to complicate, bend and even break KMS UAPI contract to make the KMS path work in more fancy ways (likely with sub-optimal performance due to its fundamental design) when something like wayland-virtio already exists which allows for much more and in much better ways.
Thanks, pq
dri-devel@lists.freedesktop.org
-
Daniel Stone -
Daniel Vetter -
Gerd Hoffmann -
Hans de Goede -
Jonas Ådahl -
kernel test robot -
Martin Krastev (VMware) -
Pekka Paalanen -
Simon Ser -
Zack Rusin -
Zack Rusin