[PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Zack Rusin]

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-kms-impl-device-atomic.c#L1188
https://invent.kde.org/plasma/kwin/-/blob/master/src/backends/drm/drm_gpu.cpp#L156

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/2aa61b50507a24f34d514fa65b7bcf07e910f459
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(-)

-- 
2.34.1

[PATCH 1/6] drm/atomic: Add support for mouse hotspots

[Zack Rusin]

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
-- 
2.34.1

[PATCH 2/6] drm/vmwgfx: Create mouse hotspot properties on cursor planes

[Zack Rusin]

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:
-- 
2.34.1

[PATCH 3/6] drm/qxl: Create mouse hotspot properties on cursor planes

[Zack Rusin]

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:
-- 
2.34.1

[PATCH 4/6] drm/vboxvideo: Create mouse hotspot properties on cursor planes

[Zack Rusin]

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");
 	}
-- 
2.34.1

[PATCH 5/6] drm/virtio: Create mouse hotspot properties on cursor planes

[Zack Rusin]

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);
-- 
2.34.1

[PATCH 6/6] drm: Remove legacy cursor hotspot code

[Zack Rusin]

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.
 	 */
-- 
2.34.1

Re: [PATCH 3/6] drm/qxl: Create mouse hotspot properties on cursor planes

[kernel test robot]

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-cursor-hotspot-support-to-atomic-KMS/20220602-234633
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@intel.com/config)
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/0bf2395ee17bd25ae6411c560de883496256195d
        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  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 3/6] drm/qxl: Create mouse hotspot properties on cursor planes

[kernel test robot]

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-cursor-hotspot-support-to-atomic-KMS/20220602-234633
base:   git://anongit.freedesktop.org/drm/drm drm-next
config: i386-randconfig-a013 (https://download.01.org/0day-ci/archive/20220603/202206030750.8hv8vdBA-lkp@intel.com/config)
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/0bf2395ee17bd25ae6411c560de883496256195d
        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  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Gerd Hoffmann]

  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

Re: [PATCH 2/6] drm/vmwgfx: Create mouse hotspot properties on cursor planes

[Martin Krastev (VMware)]

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]);
>   		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:


LGTM!

Reviewed-by: Martin Krastev <krastevm@vmware.com>


Regards,

Martin

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Simon Ser]

Hi,

Please, read this thread:
https://lists.freedesktop.org/archives/dri-devel/2020-March/thread.html#259615

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

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Zack Rusin]

> 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.freedesktop.org%2Farchives%2Fdri-devel%2F2020-March%2Fthread.html%23259615&amp;data=05%7C01%7Czackr%40vmware.com%7C05141cb812004e947f0408da456b76e2%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637898625140237028%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=%2BZUG0OFC5SXC8%2Bm3D93AliT0VNJHbc1AEwnhVAnw8WQ%3D&amp;reserved=0
> 
> 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

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Simon Ser]

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.

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Zack Rusin]

> 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

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Zack Rusin]

> 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

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Simon Ser]

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.

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Zack Rusin]

[-- Attachment #1: Type: text/plain, Size: 2936 bytes --]



On Jun 3, 2022, at 10:56 AM, Simon Ser <contact@emersion.fr<mailto:contact@emersion.fr>> wrote:

⚠ External Email

On Friday, June 3rd, 2022 at 16:38, Zack Rusin <zackr@vmware.com<mailto:zackr@vmware.com>> wrote:

On Jun 3, 2022, at 10:32 AM, Simon Ser <contact@emersion.fr<mailto:contact@emersion.fr>> wrote:

⚠ External Email

On Friday, June 3rd, 2022 at 16:27, Zack Rusin <zackr@vmware.com<mailto: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

[-- Attachment #2: Type: text/html, Size: 15479 bytes --]

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Simon Ser]

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.

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Zack Rusin]

[-- Attachment #1: Type: text/plain, Size: 3376 bytes --]



On Jun 3, 2022, at 11:22 AM, Simon Ser <contact@emersion.fr<mailto:contact@emersion.fr>> wrote:

⚠ External Email

On Friday, June 3rd, 2022 at 17:17, Zack Rusin <zackr@vmware.com<mailto:zackr@vmware.com>> wrote:


On Jun 3, 2022, at 10:56 AM, Simon Ser <contact@emersion.fr<mailto:contact@emersion.fr>> wrote:
⚠ External Email

On Friday, June 3rd, 2022 at 16:38, Zack Rusin <zackr@vmware.com<mailto:zackr@vmware.com>> wrote:

On Jun 3, 2022, at 10:32 AM, Simon Ser <contact@emersion.fr<mailto:contact@emersion.fr>> wrote:

⚠ External Email

On Friday, June 3rd, 2022 at 16:27, Zack Rusin <zackr@vmware.com<mailto: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

[-- Attachment #2: Type: text/html, Size: 12901 bytes --]

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Simon Ser]

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.

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Zack Rusin]

[-- Attachment #1: Type: text/plain, Size: 5682 bytes --]



On Jun 3, 2022, at 11:49 AM, Simon Ser <contact@emersion.fr<mailto:contact@emersion.fr>> wrote:

⚠ External Email

On Friday, June 3rd, 2022 at 17:32, Zack Rusin <zackr@vmware.com<mailto:zackr@vmware.com>> wrote:

On Jun 3, 2022, at 11:22 AM, Simon Ser <contact@emersion.fr<mailto:contact@emersion.fr>> wrote:
⚠ External Email

On Friday, June 3rd, 2022 at 17:17, Zack Rusin <zackr@vmware.com<mailto:zackr@vmware.com>> wrote:




On Jun 3, 2022, at 10:56 AM, Simon Ser <contact@emersion.fr<mailto:contact@emersion.fr>> wrote:
⚠ External Email

On Friday, June 3rd, 2022 at 16:38, Zack Rusin <zackr@vmware.com<mailto:zackr@vmware.com>> wrote:


On Jun 3, 2022, at 10:32 AM, Simon Ser <contact@emersion.fr<mailto:contact@emersion.fr>> wrote:

⚠ External Email

On Friday, June 3rd, 2022 at 16:27, Zack Rusin <zackr@vmware.com<mailto: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-kms-impl-device-atomic.c#L1188
https://invent.kde.org/plasma/kwin/-/blob/master/src/backends/drm/drm_gpu.cpp#L156

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


[-- Attachment #2: Type: text/html, Size: 16514 bytes --]

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Hans de Goede]

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

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Simon Ser]

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-kms-impl-device-atomic.c#L1188
> https://invent.kde.org/plasma/kwin/-/blob/master/src/backends/drm/drm_gpu.cpp#L156

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.

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Simon Ser]

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:
>
> 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.

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.

> 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.

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.

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Zack Rusin]

> 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.gnome.org%2FGNOME%2Fmutter%2F-%2Fblob%2Fmain%2Fsrc%2Fbackends%2Fnative%2Fmeta-kms-impl-device-atomic.c%23L1188&amp;data=05%7C01%7Czackr%40vmware.com%7C2b0ab6c67e7d4bfb618a08da46c5394f%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637900110157712044%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=wggJaNScF0ziIG%2BfSdSUKBVZGoNjtm4Q8amS7SbJa%2FY%3D&amp;reserved=0
>> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Finvent.kde.org%2Fplasma%2Fkwin%2F-%2Fblob%2Fmaster%2Fsrc%2Fbackends%2Fdrm%2Fdrm_gpu.cpp%23L156&amp;data=05%7C01%7Czackr%40vmware.com%7C2b0ab6c67e7d4bfb618a08da46c5394f%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637900110157712044%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=%2BCTEJ9XtlI9kuKJJZVMNodtkZSkRIv8RN9jSRAM1pqM%3D&amp;reserved=0
> 
> 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

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Simon Ser]

------- 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.

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Zack Rusin]

> 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

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Simon Ser]

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.

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 3816 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 7346 bytes --]

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#259615
> 
> 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-properties
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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Gerd Hoffmann]

  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

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Gerd Hoffmann]

> 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

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Zack Rusin]

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#259615
> > 
> > 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

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 7540 bytes --]

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#259615
> > > 
> > > 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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 7171 bytes --]

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#gab86a05e7a220d6ccd0d45a79d85339dd
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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Gerd Hoffmann]

  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

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Zack Rusin]

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#259615
> > > > 
> > > > 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

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 9287 bytes --]

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#259615
> > > > > 
> > > > > 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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Jonas Ådahl]

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#259615
> > > > > > 
> > > > > > 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

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Daniel Vetter]

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#259615
> 
> 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

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Simon Ser]

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.

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 2857 bytes --]

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:
>   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?

Hi,

sounds like a good plan to me.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Daniel Vetter]

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?

> - 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#259615
> > 
> > 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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Simon Ser]

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.

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Gerd Hoffmann]

  Hi,

> >   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?

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

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Gerd Hoffmann]

  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

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Simon Ser]

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.home.kraxel.org/T/#m119cfbbf736e43831c3105f0c91bd790da2d58fb

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.

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Zack Rusin]

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.

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

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Hans de Goede]

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.home.kraxel.org/T/#m119cfbbf736e43831c3105f0c91bd790da2d58fb
> 
> 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

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 5862 bytes --]

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.

> 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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 4789 bytes --]

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.home.kraxel.org/T/#m119cfbbf736e43831c3105f0c91bd790da2d58fb
> > 
> > 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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Zack Rusin]

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.

z

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 6473 bytes --]

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.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Zack Rusin]

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. 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

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 11139 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Zack Rusin]

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

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Daniel Stone]

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

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Albert Esteve]

On 6/10/22 10:59, Daniel Vetter wrote:
> 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?
>
>> - 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

Hi all,

We have been testing the v2 of this patch and it works correctly for us.

First we tested with a patched Mutter, the mouse clicks were correct, 
and behavior was as expected.

But I've also added an IGT test as suggested above: 
https://gitlab.freedesktop.org/aesteve/igt-gpu-tools/-/compare/master...modeset-cursor-hotspot-test?from_project_id=1274

I could post the IGT patch upstream once Zack's patches land.

Hope that helps!

-Albert

>
>> 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#259615
>>>
>>> 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

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Javier Martinez Canillas]

[adding Zack Rusin again who seems to have fallen from the Cc list]

Albert Esteve <aesteve@redhat.com> writes:
> On 6/10/22 10:59, Daniel Vetter wrote:
>> On Fri, Jun 10, 2022 at 10:41:05AM +0200, Daniel Vetter wrote:

[...]

>>> - 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.

What is missing for these patches to land? If I understood correctly is
just these properties documentation that Sima asked above ?

>> 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
>
> Hi all,
>
> We have been testing the v2 of this patch and it works correctly for us.
>
> First we tested with a patched Mutter, the mouse clicks were correct, 
> and behavior was as expected.
>
> But I've also added an IGT test as suggested above: 
> https://gitlab.freedesktop.org/aesteve/igt-gpu-tools/-/compare/master...modeset-cursor-hotspot-test?from_project_id=1274
>
> I could post the IGT patch upstream once Zack's patches land.
>

Zack, are you planning to re-spin the series soon? Otherwise Albert could
continue working on that if you prefer.

> Hope that helps!
>
> -Albert
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Zack Rusin]

On Wed, 2023-06-21 at 09:10 +0200, Javier Martinez Canillas wrote:
> !! External Email
> 
> [adding Zack Rusin again who seems to have fallen from the Cc list]
> 
> Albert Esteve <aesteve@redhat.com> writes:
> > On 6/10/22 10:59, Daniel Vetter wrote:
> > > On Fri, Jun 10, 2022 at 10:41:05AM +0200, Daniel Vetter wrote:
> 
> [...]
> 
> > > > - 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.
> 
> What is missing for these patches to land? If I understood correctly is
> just these properties documentation that Sima asked above ?
> 
> > > 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
> > 
> > Hi all,
> > 
> > We have been testing the v2 of this patch and it works correctly for us.
> > 
> > First we tested with a patched Mutter, the mouse clicks were correct,
> > and behavior was as expected.
> > 
> > But I've also added an IGT test as suggested above:
> > https://gitlab.freedesktop.org/aesteve/igt-gpu-tools/-/compare/master...modeset-cursor-hotspot-test?from_project_id=1274
> > 
> > I could post the IGT patch upstream once Zack's patches land.
> > 
> 
> Zack, are you planning to re-spin the series soon? Otherwise Albert could
> continue working on that if you prefer.

Besides mutter I wanted to patch kwin as well, but I haven't been able to find the
time to patch it as well. I can respin with discussed changes over the weekend if
we're ok with getting this in without kde support from the start.

z

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

[Javier Martinez Canillas]

Zack Rusin <zackr@vmware.com> writes:

[adding Aleix Pol from KDE/kwin to Cc list]

Hello Zack,

> On Wed, 2023-06-21 at 09:10 +0200, Javier Martinez Canillas wrote:

[...]

>> > 
>> > Hi all,
>> > 
>> > We have been testing the v2 of this patch and it works correctly for us.
>> > 
>> > First we tested with a patched Mutter, the mouse clicks were correct,
>> > and behavior was as expected.
>> > 
>> > But I've also added an IGT test as suggested above:
>> > https://gitlab.freedesktop.org/aesteve/igt-gpu-tools/-/compare/master...modeset-cursor-hotspot-test?from_project_id=1274
>> > 
>> > I could post the IGT patch upstream once Zack's patches land.
>> > 
>> 
>> Zack, are you planning to re-spin the series soon? Otherwise Albert could
>> continue working on that if you prefer.
>
> Besides mutter I wanted to patch kwin as well, but I haven't been able to find the
> time to patch it as well. I can respin with discussed changes over the weekend if

That would be great!

> we're ok with getting this in without kde support from the start.
>

In my opinion that is OK, specially now that Albert also wrote an IGT
test, so there are two different users of your new KMS properties.

Support for kwin can be added as a follow-up once your patches land.
But I don't know what others thing.

> z
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat