[PATCH v4 0/8] Fix cursor planes with virtualized drivers

[PATCH v4 0/8] Fix cursor planes with virtualized drivers

[Zack Rusin]

From: Zack Rusin <zackr@vmware.com>

v4: Make drm_plane_create_hotspot_properties static, rename
DRM_CLIENT_CAP_VIRTUALIZED_CURSOR_PLANE to DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT
and some minor stylistic fixes for things found by Javier and Pekka
in v3.

v3: Renames, fixes and cleanups suggested by Daniel, Simon and Pekka
after v2. There's no major changes in functionality. Please let me know
if I missed anything, it's been a while since v2.

Virtualized drivers have had a lot of issues with cursor support on top
of atomic modesetting. This set both fixes the long standing problems
with atomic kms and virtualized drivers and adds code to let userspace
use atomic kms on virtualized drivers while preserving functioning
seamless cursors between the host and guest.

The first change in the set is one that should be backported as far as
possible, likely 5.4 stable, because earlier stable kernels do not have
virtualbox driver. The change makes virtualized drivers stop exposing
a cursor plane for atomic clients, this fixes mouse cursor on all well
formed compositors which will automatically fallback to software cursor.

The rest of the changes until the last one ports the legacy hotspot code
to atomic plane properties.

Finally the last change introduces userspace API to let userspace
clients advertise the fact that they are aware of additional restrictions
placed upon the cursor plane by virtualized drivers and lets them use
atomic kms with virtualized drivers (the clients are expected to set
hotspots correctly when advertising support for virtual cursor plane).

Zack Rusin (8):
  drm: Disable the cursor plane on atomic contexts with virtualized
    drivers
  drm/atomic: Add support for mouse hotspots
  drm/vmwgfx: Use the hotspot properties from cursor planes
  drm/qxl: Use the hotspot properties from cursor planes
  drm/vboxvideo: Use the hotspot properties from cursor planes
  drm/virtio: Use the hotspot properties from cursor planes
  drm: Remove legacy cursor hotspot code
  drm: Introduce DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT

 drivers/gpu/drm/drm_atomic_state_helper.c | 14 +++++
 drivers/gpu/drm/drm_atomic_uapi.c         | 20 +++++++
 drivers/gpu/drm/drm_ioctl.c               |  9 ++++
 drivers/gpu/drm/drm_plane.c               | 64 ++++++++++++++++++++++-
 drivers/gpu/drm/qxl/qxl_display.c         | 14 +++--
 drivers/gpu/drm/qxl/qxl_drv.c             |  2 +-
 drivers/gpu/drm/vboxvideo/vbox_drv.c      |  2 +-
 drivers/gpu/drm/vboxvideo/vbox_mode.c     |  4 +-
 drivers/gpu/drm/virtio/virtgpu_drv.c      |  3 +-
 drivers/gpu/drm/virtio/virtgpu_plane.c    |  8 +--
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c       |  2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c       |  9 +---
 include/drm/drm_drv.h                     |  9 ++++
 include/drm/drm_file.h                    | 12 +++++
 include/drm/drm_framebuffer.h             | 12 -----
 include/drm/drm_plane.h                   | 14 +++++
 include/uapi/drm/drm.h                    | 25 +++++++++
 17 files changed, 184 insertions(+), 39 deletions(-)

-- 
2.39.2

[PATCH v4 1/8] drm: Disable the cursor plane on atomic contexts with virtualized drivers

[Zack Rusin]

From: Zack Rusin <zackr@vmware.com>

Cursor planes on virtualized drivers have special meaning and require
that the clients handle them in specific ways, e.g. the cursor plane
should react to the mouse movement the way a mouse cursor would be
expected to and the client is required to set hotspot properties on it
in order for the mouse events to be routed correctly.

This breaks the contract as specified by the "universal planes". Fix it
by disabling the cursor planes on virtualized drivers while adding
a foundation on top of which it's possible to special case mouse cursor
planes for clients that want it.

Disabling the cursor planes makes some kms compositors which were broken,
e.g. Weston, fallback to software cursor which works fine or at least
better than currently while having no effect on others, e.g. gnome-shell
or kwin, which put virtualized drivers on a deny-list when running in
atomic context to make them fallback to legacy kms and avoid this issue.

Signed-off-by: Zack Rusin <zackr@vmware.com>
Fixes: 681e7ec73044 ("drm: Allow userspace to ask for universal plane list (v2)")
Cc: <stable@vger.kernel.org> # v5.4+
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Gurchetan Singh <gurchetansingh@chromium.org>
Cc: Chia-I Wu <olvaffe@gmail.com>
Cc: dri-devel@lists.freedesktop.org
Cc: virtualization@lists.linux-foundation.org
Cc: spice-devel@lists.freedesktop.org
Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/gpu/drm/drm_plane.c          | 13 +++++++++++++
 drivers/gpu/drm/qxl/qxl_drv.c        |  2 +-
 drivers/gpu/drm/vboxvideo/vbox_drv.c |  2 +-
 drivers/gpu/drm/virtio/virtgpu_drv.c |  3 ++-
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c  |  2 +-
 include/drm/drm_drv.h                |  9 +++++++++
 include/drm/drm_file.h               | 12 ++++++++++++
 7 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 24e7998d1731..c6bbb0c209f4 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -678,6 +678,19 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data,
 		    !file_priv->universal_planes)
 			continue;
 
+		/*
+		 * If we're running on a virtualized driver then,
+		 * unless userspace advertizes support for the
+		 * virtualized cursor plane, disable cursor planes
+		 * because they'll be broken due to missing cursor
+		 * hotspot info.
+		 */
+		if (plane->type == DRM_PLANE_TYPE_CURSOR &&
+		    drm_core_check_feature(dev, DRIVER_CURSOR_HOTSPOT) &&
+		    file_priv->atomic &&
+		    !file_priv->supports_virtualized_cursor_plane)
+			continue;
+
 		if (drm_lease_held(file_priv, plane->base.id)) {
 			if (count < plane_resp->count_planes &&
 			    put_user(plane->base.id, plane_ptr + count))
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index b30ede1cf62d..91930e84a9cd 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.c
+++ b/drivers/gpu/drm/qxl/qxl_drv.c
@@ -283,7 +283,7 @@ static const struct drm_ioctl_desc qxl_ioctls[] = {
 };
 
 static struct drm_driver qxl_driver = {
-	.driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
+	.driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_CURSOR_HOTSPOT,
 
 	.dumb_create = qxl_mode_dumb_create,
 	.dumb_map_offset = drm_gem_ttm_dumb_map_offset,
diff --git a/drivers/gpu/drm/vboxvideo/vbox_drv.c b/drivers/gpu/drm/vboxvideo/vbox_drv.c
index 4fee15c97c34..8ecd0863fad7 100644
--- a/drivers/gpu/drm/vboxvideo/vbox_drv.c
+++ b/drivers/gpu/drm/vboxvideo/vbox_drv.c
@@ -172,7 +172,7 @@ DEFINE_DRM_GEM_FOPS(vbox_fops);
 
 static const struct drm_driver driver = {
 	.driver_features =
-	    DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
+	    DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC | DRIVER_CURSOR_HOTSPOT,
 
 	.fops = &vbox_fops,
 	.name = DRIVER_NAME,
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
index a7ec5a3770da..60b1fd23229c 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -176,7 +176,8 @@ static const struct drm_driver driver = {
 	 * If KMS is disabled DRIVER_MODESET and DRIVER_ATOMIC are masked
 	 * out via drm_device::driver_features:
 	 */
-	.driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER | DRIVER_ATOMIC,
+	.driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER | DRIVER_ATOMIC |
+			   DRIVER_CURSOR_HOTSPOT,
 	.open = virtio_gpu_driver_open,
 	.postclose = virtio_gpu_driver_postclose,
 
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index 8b24ecf60e3e..d3e308fdfd5b 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -1611,7 +1611,7 @@ static const struct file_operations vmwgfx_driver_fops = {
 
 static const struct drm_driver driver = {
 	.driver_features =
-	DRIVER_MODESET | DRIVER_RENDER | DRIVER_ATOMIC | DRIVER_GEM,
+	DRIVER_MODESET | DRIVER_RENDER | DRIVER_ATOMIC | DRIVER_GEM | DRIVER_CURSOR_HOTSPOT,
 	.ioctls = vmw_ioctls,
 	.num_ioctls = ARRAY_SIZE(vmw_ioctls),
 	.master_set = vmw_master_set,
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index b77f2c7275b7..8303016665dd 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -104,6 +104,15 @@ enum drm_driver_feature {
 	 * acceleration should be handled by two drivers that are connected using auxiliary bus.
 	 */
 	DRIVER_COMPUTE_ACCEL            = BIT(7),
+	/**
+	 * @DRIVER_CURSOR_HOTSPOT:
+	 *
+	 * Driver supports and requires cursor hotspot information in the
+	 * cursor plane (e.g. cursor plane has to actually track the mouse
+	 * cursor and the clients are required to set hotspot in order for
+	 * the cursor planes to work correctly).
+	 */
+	DRIVER_CURSOR_HOTSPOT           = BIT(8),
 
 	/* IMPORTANT: Below are all the legacy flags, add new ones above. */
 
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index 966912053cb0..91cf7f452f86 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -228,6 +228,18 @@ struct drm_file {
 	 */
 	bool is_master;
 
+	/**
+	 * @supports_virtualized_cursor_plane:
+	 *
+	 * This client is capable of handling the cursor plane with the
+	 * restrictions imposed on it by the virtualized drivers.
+	 *
+	 * This implies that the cursor plane has to behave like a cursor
+	 * i.e. track cursor movement. It also requires setting of the
+	 * hotspot properties by the client on the cursor plane.
+	 */
+	bool supports_virtualized_cursor_plane;
+
 	/**
 	 * @master:
 	 *
-- 
2.39.2

[PATCH v4 1/8] drm: Disable the cursor plane on atomic contexts with virtualized drivers

[Zack Rusin]

From: Zack Rusin <zackr@vmware.com>

Cursor planes on virtualized drivers have special meaning and require
that the clients handle them in specific ways, e.g. the cursor plane
should react to the mouse movement the way a mouse cursor would be
expected to and the client is required to set hotspot properties on it
in order for the mouse events to be routed correctly.

This breaks the contract as specified by the "universal planes". Fix it
by disabling the cursor planes on virtualized drivers while adding
a foundation on top of which it's possible to special case mouse cursor
planes for clients that want it.

Disabling the cursor planes makes some kms compositors which were broken,
e.g. Weston, fallback to software cursor which works fine or at least
better than currently while having no effect on others, e.g. gnome-shell
or kwin, which put virtualized drivers on a deny-list when running in
atomic context to make them fallback to legacy kms and avoid this issue.

Signed-off-by: Zack Rusin <zackr@vmware.com>
Fixes: 681e7ec73044 ("drm: Allow userspace to ask for universal plane list (v2)")
Cc: <stable@vger.kernel.org> # v5.4+
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Gurchetan Singh <gurchetansingh@chromium.org>
Cc: Chia-I Wu <olvaffe@gmail.com>
Cc: dri-devel@lists.freedesktop.org
Cc: virtualization@lists.linux-foundation.org
Cc: spice-devel@lists.freedesktop.org
Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/gpu/drm/drm_plane.c          | 13 +++++++++++++
 drivers/gpu/drm/qxl/qxl_drv.c        |  2 +-
 drivers/gpu/drm/vboxvideo/vbox_drv.c |  2 +-
 drivers/gpu/drm/virtio/virtgpu_drv.c |  3 ++-
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c  |  2 +-
 include/drm/drm_drv.h                |  9 +++++++++
 include/drm/drm_file.h               | 12 ++++++++++++
 7 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 24e7998d1731..c6bbb0c209f4 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -678,6 +678,19 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data,
 		    !file_priv->universal_planes)
 			continue;
 
+		/*
+		 * If we're running on a virtualized driver then,
+		 * unless userspace advertizes support for the
+		 * virtualized cursor plane, disable cursor planes
+		 * because they'll be broken due to missing cursor
+		 * hotspot info.
+		 */
+		if (plane->type == DRM_PLANE_TYPE_CURSOR &&
+		    drm_core_check_feature(dev, DRIVER_CURSOR_HOTSPOT) &&
+		    file_priv->atomic &&
+		    !file_priv->supports_virtualized_cursor_plane)
+			continue;
+
 		if (drm_lease_held(file_priv, plane->base.id)) {
 			if (count < plane_resp->count_planes &&
 			    put_user(plane->base.id, plane_ptr + count))
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index b30ede1cf62d..91930e84a9cd 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.c
+++ b/drivers/gpu/drm/qxl/qxl_drv.c
@@ -283,7 +283,7 @@ static const struct drm_ioctl_desc qxl_ioctls[] = {
 };
 
 static struct drm_driver qxl_driver = {
-	.driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
+	.driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_CURSOR_HOTSPOT,
 
 	.dumb_create = qxl_mode_dumb_create,
 	.dumb_map_offset = drm_gem_ttm_dumb_map_offset,
diff --git a/drivers/gpu/drm/vboxvideo/vbox_drv.c b/drivers/gpu/drm/vboxvideo/vbox_drv.c
index 4fee15c97c34..8ecd0863fad7 100644
--- a/drivers/gpu/drm/vboxvideo/vbox_drv.c
+++ b/drivers/gpu/drm/vboxvideo/vbox_drv.c
@@ -172,7 +172,7 @@ DEFINE_DRM_GEM_FOPS(vbox_fops);
 
 static const struct drm_driver driver = {
 	.driver_features =
-	    DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
+	    DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC | DRIVER_CURSOR_HOTSPOT,
 
 	.fops = &vbox_fops,
 	.name = DRIVER_NAME,
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
index a7ec5a3770da..60b1fd23229c 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -176,7 +176,8 @@ static const struct drm_driver driver = {
 	 * If KMS is disabled DRIVER_MODESET and DRIVER_ATOMIC are masked
 	 * out via drm_device::driver_features:
 	 */
-	.driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER | DRIVER_ATOMIC,
+	.driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER | DRIVER_ATOMIC |
+			   DRIVER_CURSOR_HOTSPOT,
 	.open = virtio_gpu_driver_open,
 	.postclose = virtio_gpu_driver_postclose,
 
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index 8b24ecf60e3e..d3e308fdfd5b 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -1611,7 +1611,7 @@ static const struct file_operations vmwgfx_driver_fops = {
 
 static const struct drm_driver driver = {
 	.driver_features =
-	DRIVER_MODESET | DRIVER_RENDER | DRIVER_ATOMIC | DRIVER_GEM,
+	DRIVER_MODESET | DRIVER_RENDER | DRIVER_ATOMIC | DRIVER_GEM | DRIVER_CURSOR_HOTSPOT,
 	.ioctls = vmw_ioctls,
 	.num_ioctls = ARRAY_SIZE(vmw_ioctls),
 	.master_set = vmw_master_set,
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index b77f2c7275b7..8303016665dd 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -104,6 +104,15 @@ enum drm_driver_feature {
 	 * acceleration should be handled by two drivers that are connected using auxiliary bus.
 	 */
 	DRIVER_COMPUTE_ACCEL            = BIT(7),
+	/**
+	 * @DRIVER_CURSOR_HOTSPOT:
+	 *
+	 * Driver supports and requires cursor hotspot information in the
+	 * cursor plane (e.g. cursor plane has to actually track the mouse
+	 * cursor and the clients are required to set hotspot in order for
+	 * the cursor planes to work correctly).
+	 */
+	DRIVER_CURSOR_HOTSPOT           = BIT(8),
 
 	/* IMPORTANT: Below are all the legacy flags, add new ones above. */
 
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index 966912053cb0..91cf7f452f86 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -228,6 +228,18 @@ struct drm_file {
 	 */
 	bool is_master;
 
+	/**
+	 * @supports_virtualized_cursor_plane:
+	 *
+	 * This client is capable of handling the cursor plane with the
+	 * restrictions imposed on it by the virtualized drivers.
+	 *
+	 * This implies that the cursor plane has to behave like a cursor
+	 * i.e. track cursor movement. It also requires setting of the
+	 * hotspot properties by the client on the cursor plane.
+	 */
+	bool supports_virtualized_cursor_plane;
+
 	/**
 	 * @master:
 	 *
-- 
2.39.2

[PATCH v4 2/8] 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>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/gpu/drm/drm_atomic_state_helper.c | 14 +++++++
 drivers/gpu/drm/drm_atomic_uapi.c         | 20 +++++++++
 drivers/gpu/drm/drm_plane.c               | 50 +++++++++++++++++++++++
 include/drm/drm_plane.h                   | 14 +++++++
 4 files changed, 98 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index 784e63d70a42..54975de44a0e 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -275,6 +275,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 98d3b10c08ae..07a7b3f18df2 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -593,6 +593,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",
@@ -653,6 +669,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 {
 		drm_dbg_atomic(dev,
 			       "[PLANE:%d:%s] unknown property [PROP:%d:%s]\n",
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index c6bbb0c209f4..eaca367bdc7e 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -230,6 +230,47 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
 	return 0;
 }
 
+/**
+ * drm_plane_create_hotspot_properties - creates the mouse hotspot
+ * properties and attaches them to the given cursor plane
+ *
+ * @plane: drm cursor plane
+ *
+ * This function enables the mouse hotspot property on a given
+ * cursor plane.
+ *
+ * RETURNS:
+ * Zero for success or -errno
+ */
+static int drm_plane_create_hotspot_properties(struct drm_plane *plane)
+{
+	struct drm_property *prop_x;
+	struct drm_property *prop_y;
+
+	drm_WARN_ON(plane->dev,
+		    !drm_core_check_feature(plane->dev,
+					    DRIVER_CURSOR_HOTSPOT));
+
+	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;
+}
+
 __printf(9, 0)
 static int __drm_universal_plane_init(struct drm_device *dev,
 				      struct drm_plane *plane,
@@ -348,6 +389,10 @@ static int __drm_universal_plane_init(struct drm_device *dev,
 		drm_object_attach_property(&plane->base, config->prop_src_w, 0);
 		drm_object_attach_property(&plane->base, config->prop_src_h, 0);
 	}
+	if (drm_core_check_feature(dev, DRIVER_CURSOR_HOTSPOT) &&
+	    type == DRM_PLANE_TYPE_CURSOR) {
+		drm_plane_create_hotspot_properties(plane);
+	}
 
 	if (format_modifier_count)
 		create_in_format_blob(dev, plane);
@@ -1067,6 +1112,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;
 		}
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 51291983ea44..74e62f90a1ad 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)
-- 
2.39.2

[PATCH v4 3/8] drm/vmwgfx: Use the hotspot properties from cursor planes

[Zack Rusin]

From: Zack Rusin <zackr@vmware.com>

Atomic modesetting got support for mouse hotspots via the hotspot
properties. Port the legacy kms hotspot handling to the new properties
on cursor planes.

Signed-off-by: Zack Rusin <zackr@vmware.com>
Cc: Maaz Mombasawala <mombasawalam@vmware.com>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Martin Krastev <krastevm@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index b62207be3363..de294dfe05d0 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -768,13 +768,8 @@ vmw_du_cursor_plane_atomic_update(struct drm_plane *plane,
 	struct vmw_plane_state *old_vps = vmw_plane_state_to_vps(old_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;
-- 
2.39.2

[PATCH v4 4/8] drm/qxl: Use the hotspot properties from cursor planes

[Zack Rusin]

From: Zack Rusin <zackr@vmware.com>

Atomic modesetting got support for mouse hotspots via the hotspot
properties. Port the legacy kms hotspot handling to the new properties
on cursor planes.

Signed-off-by: Zack Rusin <zackr@vmware.com>
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: virtualization@lists.linux-foundation.org
Cc: spice-devel@lists.freedesktop.org
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_display.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index 6492a70e3c39..5d689e0d3586 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -485,7 +485,6 @@ static int qxl_primary_atomic_check(struct drm_plane *plane,
 static int qxl_primary_apply_cursor(struct qxl_device *qdev,
 				    struct drm_plane_state *plane_state)
 {
-	struct drm_framebuffer *fb = plane_state->fb;
 	struct qxl_crtc *qcrtc = to_qxl_crtc(plane_state->crtc);
 	struct qxl_cursor_cmd *cmd;
 	struct qxl_release *release;
@@ -510,8 +509,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);
 
@@ -531,7 +530,6 @@ static int qxl_primary_apply_cursor(struct qxl_device *qdev,
 static int qxl_primary_move_cursor(struct qxl_device *qdev,
 				   struct drm_plane_state *plane_state)
 {
-	struct drm_framebuffer *fb = plane_state->fb;
 	struct qxl_crtc *qcrtc = to_qxl_crtc(plane_state->crtc);
 	struct qxl_cursor_cmd *cmd;
 	struct qxl_release *release;
@@ -554,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);
@@ -851,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);
 	}
 
-- 
2.39.2

[PATCH v4 5/8] drm/vboxvideo: Use the hotspot properties from cursor planes

[Zack Rusin]

From: Zack Rusin <zackr@vmware.com>

Atomic modesetting got support for mouse hotspots via the hotspot
properties. Port the legacy kms hotspot handling to the new properties
on cursor planes.

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>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/gpu/drm/vboxvideo/vbox_mode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vboxvideo/vbox_mode.c b/drivers/gpu/drm/vboxvideo/vbox_mode.c
index 341edd982cb3..9ff3bade9795 100644
--- a/drivers/gpu/drm/vboxvideo/vbox_mode.c
+++ b/drivers/gpu/drm/vboxvideo/vbox_mode.c
@@ -429,8 +429,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);
-- 
2.39.2

[PATCH v4 6/8] drm/virtio: Use the hotspot properties from cursor planes

[Zack Rusin]

From: Zack Rusin <zackr@vmware.com>

Atomic modesetting got support for mouse hotspots via the hotspot
properties. Port the legacy kms hotspot handling to the new properties
on cursor planes.

Signed-off-by: Zack Rusin <zackr@vmware.com>
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
Cc: David Airlie <airlied@linux.ie>
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
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/gpu/drm/virtio/virtgpu_plane.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
index a2e045f3a000..20de599658c1 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -323,16 +323,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.39.2

[PATCH v4 7/8] drm: Remove legacy cursor hotspot code

[Zack Rusin]

From: Zack Rusin <zackr@vmware.com>

Atomic modesetting supports mouse cursor offsets via the hotspot
properties that are created on cursor planes. All drivers which
support hotspots 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>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 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 eaca367bdc7e..1dc00ad4c33c 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1110,9 +1110,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 0dcc07b68654..1e108c1789b1 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.39.2

[PATCH v4 8/8] drm: Introduce DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT

[Zack Rusin]

From: Zack Rusin <zackr@vmware.com>

Virtualized drivers place additional restrictions on the cursor plane
which breaks the contract of universal planes. To allow atomic
modesettings with virtualized drivers the clients need to advertise
that they're capable of dealing with those extra restrictions.

To do that introduce DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT which
lets DRM know that the client is aware of and capable of dealing with
the extra restrictions on the virtual cursor plane.

Setting this option to true makes DRM expose the cursor plane on
virtualized drivers. The userspace is expected to set the hotspots
and handle mouse events on that plane.

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>
Cc: dri-devel@lists.freedesktop.org
Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/gpu/drm/drm_ioctl.c |  9 +++++++++
 include/uapi/drm/drm.h      | 25 +++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 8e9afe7af19c..06c6497fb4d0 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -361,6 +361,15 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
 			return -EINVAL;
 		file_priv->writeback_connectors = req->value;
 		break;
+	case DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT:
+		if (!drm_core_check_feature(dev, DRIVER_CURSOR_HOTSPOT))
+			return -EOPNOTSUPP;
+		if (!file_priv->atomic)
+			return -EINVAL;
+		if (req->value > 1)
+			return -EINVAL;
+		file_priv->supports_virtualized_cursor_plane = req->value;
+		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index a87bbbbca2d4..594491446c52 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -836,6 +836,31 @@ struct drm_get_cap {
  */
 #define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS	5
 
+/**
+ * DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT
+ *
+ * Drivers for para-virtualized hardware (e.g. vmwgfx, qxl, virtio and
+ * virtualbox) have additional restrictions for cursor planes (thus
+ * making cursor planes on those drivers not truly universal,) e.g.
+ * they need cursor planes to act like one would expect from a mouse
+ * cursor and have correctly set hotspot properties.
+ * If this client cap is not set the DRM core will hide cursor plane on
+ * those virtualized drivers because not setting it implies that the
+ * client is not capable of dealing with those extra restictions.
+ * Clients which do set cursor hotspot and treat the cursor plane
+ * like a mouse cursor should set this property.
+ * The client must enable &DRM_CLIENT_CAP_ATOMIC first.
+ *
+ * Setting this property on drivers which do not special case
+ * cursor planes (i.e. non-virtualized drivers) will return
+ * EOPNOTSUPP, which can be used by userspace to gauge
+ * requirements of the hardware/drivers they're running on.
+ *
+ * This capability is always supported for atomic-capable virtualized
+ * drivers starting from kernel version 6.5.
+ */
+#define DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT	6
+
 /* DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
 struct drm_set_client_cap {
 	__u64 capability;
-- 
2.39.2

Re: [PATCH v4 2/8] drm/atomic: Add support for mouse hotspots

[Pekka Paalanen]

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

On Wed, 28 Jun 2023 01:21:27 -0400
Zack Rusin <zack@kde.org> wrote:

> 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>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Hi Zack,

I still do not see any UAPI docs for the new properties in this patch?


Thanks,
pq


> ---
>  drivers/gpu/drm/drm_atomic_state_helper.c | 14 +++++++
>  drivers/gpu/drm/drm_atomic_uapi.c         | 20 +++++++++
>  drivers/gpu/drm/drm_plane.c               | 50 +++++++++++++++++++++++
>  include/drm/drm_plane.h                   | 14 +++++++
>  4 files changed, 98 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> index 784e63d70a42..54975de44a0e 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -275,6 +275,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 98d3b10c08ae..07a7b3f18df2 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -593,6 +593,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",
> @@ -653,6 +669,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 {
>  		drm_dbg_atomic(dev,
>  			       "[PLANE:%d:%s] unknown property [PROP:%d:%s]\n",
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index c6bbb0c209f4..eaca367bdc7e 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -230,6 +230,47 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
>  	return 0;
>  }
>  
> +/**
> + * drm_plane_create_hotspot_properties - creates the mouse hotspot
> + * properties and attaches them to the given cursor plane
> + *
> + * @plane: drm cursor plane
> + *
> + * This function enables the mouse hotspot property on a given
> + * cursor plane.
> + *
> + * RETURNS:
> + * Zero for success or -errno
> + */
> +static int drm_plane_create_hotspot_properties(struct drm_plane *plane)
> +{
> +	struct drm_property *prop_x;
> +	struct drm_property *prop_y;
> +
> +	drm_WARN_ON(plane->dev,
> +		    !drm_core_check_feature(plane->dev,
> +					    DRIVER_CURSOR_HOTSPOT));
> +
> +	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;
> +}
> +
>  __printf(9, 0)
>  static int __drm_universal_plane_init(struct drm_device *dev,
>  				      struct drm_plane *plane,
> @@ -348,6 +389,10 @@ static int __drm_universal_plane_init(struct drm_device *dev,
>  		drm_object_attach_property(&plane->base, config->prop_src_w, 0);
>  		drm_object_attach_property(&plane->base, config->prop_src_h, 0);
>  	}
> +	if (drm_core_check_feature(dev, DRIVER_CURSOR_HOTSPOT) &&
> +	    type == DRM_PLANE_TYPE_CURSOR) {
> +		drm_plane_create_hotspot_properties(plane);
> +	}
>  
>  	if (format_modifier_count)
>  		create_in_format_blob(dev, plane);
> @@ -1067,6 +1112,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;
>  		}
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 51291983ea44..74e62f90a1ad 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)


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

Re: [PATCH v4 2/8] drm/atomic: Add support for mouse hotspots

[Pekka Paalanen]

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

On Wed, 28 Jun 2023 10:41:06 +0300
Pekka Paalanen <ppaalanen@gmail.com> wrote:

> On Wed, 28 Jun 2023 01:21:27 -0400
> Zack Rusin <zack@kde.org> wrote:
> 
> > 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>
> > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>  
> 
> Hi Zack,
> 
> I still do not see any UAPI docs for the new properties in this patch?

If you are wondering what there could be to write about, maybe this can
give a good mindset:

Let's assume that I am a Wayland compositor developer who has never used
"hotspots" with KMS UAPI before. As I have never tested anything in a
VM, I have no idea why the kernel would ever want to know about cursor
hotspots. Display hardware never does anything with that, it just puts
the cursor plane where I tell it to and composes a single image to be
sent to the sink. The virtual driver VKMS does the same. To me, a
cursor plane is just another universal plane that may have weird
stacking order, pixel format, and size limitations.

Ideally the doc for HOTSPOT_X and HOTSPOT_Y documents not only their
possible existence and allowed/expected values, but also the reasons
to have them and what they are used for, and that if the properties
are exposed they are mandatory to program in order to use the plane.


Thanks,
pq

> > ---
> >  drivers/gpu/drm/drm_atomic_state_helper.c | 14 +++++++
> >  drivers/gpu/drm/drm_atomic_uapi.c         | 20 +++++++++
> >  drivers/gpu/drm/drm_plane.c               | 50 +++++++++++++++++++++++
> >  include/drm/drm_plane.h                   | 14 +++++++
> >  4 files changed, 98 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> > index 784e63d70a42..54975de44a0e 100644
> > --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> > @@ -275,6 +275,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 98d3b10c08ae..07a7b3f18df2 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -593,6 +593,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",
> > @@ -653,6 +669,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 {
> >  		drm_dbg_atomic(dev,
> >  			       "[PLANE:%d:%s] unknown property [PROP:%d:%s]\n",
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index c6bbb0c209f4..eaca367bdc7e 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -230,6 +230,47 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
> >  	return 0;
> >  }
> >  
> > +/**
> > + * drm_plane_create_hotspot_properties - creates the mouse hotspot
> > + * properties and attaches them to the given cursor plane
> > + *
> > + * @plane: drm cursor plane
> > + *
> > + * This function enables the mouse hotspot property on a given
> > + * cursor plane.
> > + *
> > + * RETURNS:
> > + * Zero for success or -errno
> > + */
> > +static int drm_plane_create_hotspot_properties(struct drm_plane *plane)
> > +{
> > +	struct drm_property *prop_x;
> > +	struct drm_property *prop_y;
> > +
> > +	drm_WARN_ON(plane->dev,
> > +		    !drm_core_check_feature(plane->dev,
> > +					    DRIVER_CURSOR_HOTSPOT));
> > +
> > +	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;
> > +}
> > +
> >  __printf(9, 0)
> >  static int __drm_universal_plane_init(struct drm_device *dev,
> >  				      struct drm_plane *plane,
> > @@ -348,6 +389,10 @@ static int __drm_universal_plane_init(struct drm_device *dev,
> >  		drm_object_attach_property(&plane->base, config->prop_src_w, 0);
> >  		drm_object_attach_property(&plane->base, config->prop_src_h, 0);
> >  	}
> > +	if (drm_core_check_feature(dev, DRIVER_CURSOR_HOTSPOT) &&
> > +	    type == DRM_PLANE_TYPE_CURSOR) {
> > +		drm_plane_create_hotspot_properties(plane);
> > +	}
> >  
> >  	if (format_modifier_count)
> >  		create_in_format_blob(dev, plane);
> > @@ -1067,6 +1112,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;
> >  		}
> > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > index 51291983ea44..74e62f90a1ad 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)  
> 


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

Re: [PATCH v4 2/8] drm/atomic: Add support for mouse hotspots

[Javier Martinez Canillas]

Pekka Paalanen <ppaalanen@gmail.com> writes:

Hello Pekka,

> On Wed, 28 Jun 2023 10:41:06 +0300
> Pekka Paalanen <ppaalanen@gmail.com> wrote:
>
>> On Wed, 28 Jun 2023 01:21:27 -0400
>> Zack Rusin <zack@kde.org> wrote:
>> 
>> > 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>
>> > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>  
>> 
>> Hi Zack,
>> 
>> I still do not see any UAPI docs for the new properties in this patch?
>
> If you are wondering what there could be to write about, maybe this can
> give a good mindset:
>
> Let's assume that I am a Wayland compositor developer who has never used
> "hotspots" with KMS UAPI before. As I have never tested anything in a
> VM, I have no idea why the kernel would ever want to know about cursor
> hotspots. Display hardware never does anything with that, it just puts
> the cursor plane where I tell it to and composes a single image to be
> sent to the sink. The virtual driver VKMS does the same. To me, a
> cursor plane is just another universal plane that may have weird
> stacking order, pixel format, and size limitations.
>
> Ideally the doc for HOTSPOT_X and HOTSPOT_Y documents not only their
> possible existence and allowed/expected values, but also the reasons
> to have them and what they are used for, and that if the properties
> are exposed they are mandatory to program in order to use the plane.
>

Agreed with you that this documentation would be very useful. When I first
noticed that the virtio-gpu was in a deny list for atomic KMS, it took me
some time to understand that this was related to the lack of support for
cursor hotspots in the atomic API.

Another thing that could include this document is how user-space usually
deals with the lack of cursor hotspot properties in drivers that need it
(i.e: falling back to software cursor rendering as Weston does). And that
for this reason the cursor plane is disabled on these drivers, unless the
client advertises support for it with DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT.

I think though that this could be added as follow-up and not block this
series, which IMO are already in a good shape to be merged.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v4 2/8] drm/atomic: Add support for mouse hotspots

[Simon Ser]

I think we should drop the CRTC_X/CRTC_Y properties for hotspot-aware cursor planes.
The drivers aren't going to do anything with these, and exposing them to user-space
makes it sound like user-space controls the position of the plane, but it really
doesn't.

Re: [PATCH v4 2/8] drm/atomic: Add support for mouse hotspots

[Zack Rusin]

On Wed, 2023-06-28 at 14:15 +0000, Simon Ser wrote:
> I think we should drop the CRTC_X/CRTC_Y properties for hotspot-aware cursor
> planes.
> The drivers aren't going to do anything with these, and exposing them to user-
> space
> makes it sound like user-space controls the position of the plane, but it really
> doesn't.

I think we talked about this before. The CRTC_X/CRTC_Y properties are absolutely
being used and they're respected when the rendering is done guest-side - the system
will be pretty broken if the client sets the crtc x/y properties to somewhere where
the mouse isn't though.

An argument could be made that crtc x/y properties should be removed on the cursor
plane in drivers for para-virtualized hardware and instead replaced with
mouse_position x/y properties that do exactly what crtc x/y properties do but make
it explicit what they really are on a cursor plane.

z

Re: [PATCH v4 2/8] drm/atomic: Add support for mouse hotspots

[Zack Rusin]

On Wed, 2023-06-28 at 10:54 +0300, Pekka Paalanen wrote:
> On Wed, 28 Jun 2023 10:41:06 +0300
> Pekka Paalanen <ppaalanen@gmail.com> wrote:
> 
> > On Wed, 28 Jun 2023 01:21:27 -0400
> > Zack Rusin <zack@kde.org> wrote:
> > 
> > > 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>
> > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>  
> > 
> > Hi Zack,
> > 
> > I still do not see any UAPI docs for the new properties in this patch?
> 
> If you are wondering what there could be to write about, maybe this can
> give a good mindset:
> 
> Let's assume that I am a Wayland compositor developer who has never used
> "hotspots" with KMS UAPI before. As I have never tested anything in a
> VM, I have no idea why the kernel would ever want to know about cursor
> hotspots. Display hardware never does anything with that, it just puts
> the cursor plane where I tell it to and composes a single image to be
> sent to the sink. The virtual driver VKMS does the same. To me, a
> cursor plane is just another universal plane that may have weird
> stacking order, pixel format, and size limitations.
> 
> Ideally the doc for HOTSPOT_X and HOTSPOT_Y documents not only their
> possible existence and allowed/expected values, but also the reasons
> to have them and what they are used for, and that if the properties
> are exposed they are mandatory to program in order to use the plane.

Instead of resending the entire series maybe I can draft a possible doc below and
see if we like it (once we're ok with I'll send out v5 which hopefully will be
good). How about:

/**
 * @hotspot_x_property: property to set mouse hotspot x offset.
 *
 * Hotspot is the point within the cursor image that's activating
 * the click e.g. imagine an arrow cursor pointing to bottom right -
 * the origin coordinate for that image would be top left
 * but the point which would be propagating the click would be
 * the bottom right cursor position (crtc_x, crtc_y) + hotspot
 * coordinates which for bottom right facing arrow would probably
 * be (cursor_width, cursor_height).
 *
 * This information is only relevant for drivers working on top
 * of para-virtualized hardware. The reason for that is that
 * the hotspot is fairly encapsulated in the system but imagine having
 * multiple windows with virtual machines running on servers
 * across the globe, as you move the mouse across the screen
 * and the cursor moves over those multiple windows you wouldn't
 * want to be sending those mouse events to those machines, so virtual
 * machine monitors implement an optimization where unless the mouse
 * is locked to the VM window (e.g. via a click) instead of propagating
 * those mouse events to those VM's they change the image of the native
 * cursor to what's inside the mouse cursor plane and do not interact
 * with the VM window until mouse is clicked in it.
 *
 * In order for that click to correctly and seamlessly propagate
 * between the native and virtual machine, not only the cursor image
 * but also the hotspot information has to match between them.
 *
 * Make sure to set this property on the cursor plane if you'd like
 * your application to behave correctly when running on
 * para-virtualized drivers (qxl, vbox, virtio or vmwgfx).
 * /

z

Re: [PATCH v4 2/8] drm/atomic: Add support for mouse hotspots

[Pekka Paalanen]

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

On Wed, 28 Jun 2023 19:54:49 +0000
Zack Rusin <zackr@vmware.com> wrote:

> On Wed, 2023-06-28 at 10:54 +0300, Pekka Paalanen wrote:
> > On Wed, 28 Jun 2023 10:41:06 +0300
> > Pekka Paalanen <ppaalanen@gmail.com> wrote:
> >   
> > > On Wed, 28 Jun 2023 01:21:27 -0400
> > > Zack Rusin <zack@kde.org> wrote:
> > >   
> > > > 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>
> > > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>    
> > > 
> > > Hi Zack,
> > > 
> > > I still do not see any UAPI docs for the new properties in this patch?  
> > 
> > If you are wondering what there could be to write about, maybe this can
> > give a good mindset:
> > 
> > Let's assume that I am a Wayland compositor developer who has never used
> > "hotspots" with KMS UAPI before. As I have never tested anything in a
> > VM, I have no idea why the kernel would ever want to know about cursor
> > hotspots. Display hardware never does anything with that, it just puts
> > the cursor plane where I tell it to and composes a single image to be
> > sent to the sink. The virtual driver VKMS does the same. To me, a
> > cursor plane is just another universal plane that may have weird
> > stacking order, pixel format, and size limitations.
> > 
> > Ideally the doc for HOTSPOT_X and HOTSPOT_Y documents not only their
> > possible existence and allowed/expected values, but also the reasons
> > to have them and what they are used for, and that if the properties
> > are exposed they are mandatory to program in order to use the plane.  
> 
> Instead of resending the entire series maybe I can draft a possible doc below and
> see if we like it (once we're ok with I'll send out v5 which hopefully will be
> good). How about:

Hi Zack,

cool!

> 
> /**
>  * @hotspot_x_property: property to set mouse hotspot x offset.

Hmm, this does not look like the style of
https://www.kernel.org/doc/html/latest/gpu/drm-kms.html#plane-composition-properties

I suspect it's a .rst file somewhere.

It is important to use the userspace visible concepts and names, like
the property name being "HOTSPOT_X", not hotspot_x_property. After all,
"HOTSPOT_X" is the string that userspace must use to find this
property. That's the UAPI.

>  *
>  * Hotspot is the point within the cursor image that's activating
>  * the click e.g. imagine an arrow cursor pointing to bottom right -
>  * the origin coordinate for that image would be top left
>  * but the point which would be propagating the click would be
>  * the bottom right cursor position (crtc_x, crtc_y) + hotspot
>  * coordinates which for bottom right facing arrow would probably
>  * be (cursor_width, cursor_height).

Is it really required that the hotspot coordinates fall inside the
cursor plane? Will the atomic commit be rejected otherwise?

Are they given with respect to the cursor plane top-left corner,
positive directions being right/down? Is the unit in CRTC pixels or FB
pixels? The example does give an indirect answer, but my personal taste
would like it to be more explicit.

>  *
>  * This information is only relevant for drivers working on top
>  * of para-virtualized hardware. The reason for that is that
>  * the hotspot is fairly encapsulated in the system but imagine having
>  * multiple windows with virtual machines running on servers
>  * across the globe, as you move the mouse across the screen
>  * and the cursor moves over those multiple windows you wouldn't
>  * want to be sending those mouse events to those machines, so virtual
>  * machine monitors implement an optimization where unless the mouse
>  * is locked to the VM window (e.g. via a click) instead of propagating
>  * those mouse events to those VM's they change the image of the native
>  * cursor to what's inside the mouse cursor plane and do not interact
>  * with the VM window until mouse is clicked in it.

Surely the mouse events are sent to those machines across the globe
regardless?

The point I believe you want to make is that in addition that, a
virtual machine viewer application independently moves the cursor image
on the viewer window to avoid the roundtrip latency across the globe
between mouse movement and cursor movement.

Why is the locking you mention relevant? Wouldn't you do this
optimization always if there is any cursor plane image set?

Or if you literally do mean that no input is sent to the VM at all
until the pointer is locked to that window, then why bother using the
guest cursor image without locking?

I suppose different viewers could do different things, so maybe it's
not necessary to go into those details. Just the idea of the viewer
independently moving the cursor image to reduce hand-eye latency is
important, right?

>  *
>  * In order for that click to correctly and seamlessly propagate
>  * between the native and virtual machine, not only the cursor image
>  * but also the hotspot information has to match between them.
>  *
>  * Make sure to set this property on the cursor plane if you'd like
>  * your application to behave correctly when running on
>  * para-virtualized drivers (qxl, vbox, virtio or vmwgfx).
>  * /

I think you could be more strict here. If these properties exist, then
userspace must set them appropriately and use the cursor plane only for
an actual input controlled cursor image. I might even leave the driver
list out here, because they are mentioned at
DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT doc, and userspace should not base
anything on "if driver is X or Y".

This doc should also link to DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT.

The question of which input device corresponds to which cursor plane
might be good to answer too. I presume the VM runner is configured to
expose exactly one of each, so there can be only one association?

Btw. for my own curiosity, what happens if there are two simultaneous
viewer instances open to the same VM/guest instance? Will they fight
over controlling the same pointer?


Thanks,
pq

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

Re: [PATCH v4 2/8] drm/atomic: Add support for mouse hotspots

[Pekka Paalanen]

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

On Wed, 28 Jun 2023 16:26:37 +0000
Zack Rusin <zackr@vmware.com> wrote:

> On Wed, 2023-06-28 at 14:15 +0000, Simon Ser wrote:
> > I think we should drop the CRTC_X/CRTC_Y properties for hotspot-aware cursor
> > planes.
> > The drivers aren't going to do anything with these, and exposing them to user-
> > space
> > makes it sound like user-space controls the position of the plane, but it really
> > doesn't.  
> 
> I think we talked about this before. The CRTC_X/CRTC_Y properties are absolutely
> being used and they're respected when the rendering is done guest-side - the system
> will be pretty broken if the client sets the crtc x/y properties to somewhere where
> the mouse isn't though.

Right, but it would be useful to hear more about the "why".

> An argument could be made that crtc x/y properties should be removed on the cursor
> plane in drivers for para-virtualized hardware and instead replaced with
> mouse_position x/y properties that do exactly what crtc x/y properties do but make
> it explicit what they really are on a cursor plane.

I suppose this is needed to support the guest OS warping the cursor position
while the viewer has a relative-motion pointer locked to it?

When the pointer is not locked to the VM viewer window, the pointer
sends absolute motion events? Which is necessary for the roundtrip
elision that the hotspot is needed for in the first place.

Btw. this is somewhat conflicting with what you wrote as the first UAPI
doc draft. I don't see how the viewer/host could independently position
the cursor image if the related pointer device is not also delivering
absolute motion events in the guest. Delivering relative motion events
would cause the guest and host opinion of pointer position to drift
apart primarily due to different acceleration curves.


Thanks,
pq

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

Re: [PATCH v4 2/8] drm/atomic: Add support for mouse hotspots

[Michael Banack]

Hi, I can speak to the virtual mouse/console half of this from the 
VMware-side.

I believe Zack's preparing a new set of comments here that can speak to 
most of your concerns, but I'll answer some of the other questions directly.

On 6/29/23 01:03, Pekka Paalanen wrote:
>
> Is it really required that the hotspot coordinates fall inside the
> cursor plane? Will the atomic commit be rejected otherwise?
Most console systems require the hotspot to get within the cursor image, 
but in theory it's semantically meaningful to have it extend outside the 
image.

VMware's clients in particular will clamp the hotspot to the dimension 
of the cursor image if we receive one that's out of bounds.

So I would assume the right thing to do here would be to allow it and 
let the clients figure out how to best handle it.

>
>>   *
>>   * This information is only relevant for drivers working on top
>>   * of para-virtualized hardware. The reason for that is that
>>   * the hotspot is fairly encapsulated in the system but imagine having
>>   * multiple windows with virtual machines running on servers
>>   * across the globe, as you move the mouse across the screen
>>   * and the cursor moves over those multiple windows you wouldn't
>>   * want to be sending those mouse events to those machines, so virtual
>>   * machine monitors implement an optimization where unless the mouse
>>   * is locked to the VM window (e.g. via a click) instead of propagating
>>   * those mouse events to those VM's they change the image of the native
>>   * cursor to what's inside the mouse cursor plane and do not interact
>>   * with the VM window until mouse is clicked in it.
> Surely the mouse events are sent to those machines across the globe
> regardless?
>
> The point I believe you want to make is that in addition that, a
> virtual machine viewer application independently moves the cursor image
> on the viewer window to avoid the roundtrip latency across the globe
> between mouse movement and cursor movement.
>
> Why is the locking you mention relevant? Wouldn't you do this
> optimization always if there is any cursor plane image set?
>
> Or if you literally do mean that no input is sent to the VM at all
> until the pointer is locked to that window, then why bother using the
> guest cursor image without locking?
>
> I suppose different viewers could do different things, so maybe it's
> not necessary to go into those details. Just the idea of the viewer
> independently moving the cursor image to reduce hand-eye latency is
> important, right?

Yeah, the discussions of "locking" here are more implementation details 
of how VMware's console works, and while  there are other consoles that 
work this way, many of them don't.

So it's probably more confusing than helpful without some more 
background that the other layers here don't need to worry about.

The main idea is that the client needs to know where the VM thinks the 
mouse is to determine whether it /can/ safely accelerate it all the way 
through to the client's cursor stack.  If something else in the VM is 
moving the mouse around, like an additional remote connection, then the 
client needs to fallback to an emulated cursor on the client side for a 
while.

> The question of which input device corresponds to which cursor plane
> might be good to answer too. I presume the VM runner is configured to
> expose exactly one of each, so there can be only one association?
As far as I know, all of the VM consoles are written as though they 
taking the place of what would the the physical monitors and input 
devices on a native machine.  So they assume that there is one user, 
sitting in front of one console, and all monitors/input devices are 
being used by that user.

Any more complicated multi-user/multi-cursor setup would have to be 
coordinated through a lot of layers (ie from the VM's userspace/kernel 
and then through hypervisor/client-consoles), and as far as I know 
nobody has tried to plumb that all the way through.  Even physical 
multi-user/multi-console configurations like that are rare.

>
> Btw. for my own curiosity, what happens if there are two simultaneous
> viewer instances open to the same VM/guest instance? Will they fight
> over controlling the same pointer?

I can't speak for all consoles, but VMware at least uses a bunch of 
heuristics that typically boil down to which mouse input source has been 
moving the cursor most recently.

--Michael Banack

Re: [PATCH v4 2/8] drm/atomic: Add support for mouse hotspots

[Michael Banack]

On 6/29/23 01:16, Pekka Paalanen wrote:
> On Wed, 28 Jun 2023 16:26:37 +0000
> Zack Rusin <zackr@vmware.com> wrote:
>
>> An argument could be made that crtc x/y properties should be removed on the cursor
>> plane in drivers for para-virtualized hardware and instead replaced with
>> mouse_position x/y properties that do exactly what crtc x/y properties do but make
>> it explicit what they really are on a cursor plane.
> I suppose this is needed to support the guest OS warping the cursor position
> while the viewer has a relative-motion pointer locked to it?
>
> When the pointer is not locked to the VM viewer window, the pointer
> sends absolute motion events? Which is necessary for the roundtrip
> elision that the hotspot is needed for in the first place.
>
> Btw. this is somewhat conflicting with what you wrote as the first UAPI
> doc draft. I don't see how the viewer/host could independently position
> the cursor image if the related pointer device is not also delivering
> absolute motion events in the guest. Delivering relative motion events
> would cause the guest and host opinion of pointer position to drift
> apart primarily due to different acceleration curves.

Again, I can't speak for all clients, but VMware will heuristically 
determine when to send absolute vs relative mouse events, and when to 
accelerate the cursor into the client's cursor, and when to emulate the 
cursor image on the client.

So yes, if a userspace application is moving the mouse on it's own (like 
a VNC server warping the mouse), that will today get forwarded to the 
display driver and up to the hypervisor and the client console, which 
would normally then choose to correctly draw the cursor image where the 
guest positioned it.  Only when the client believes that it is currently 
in control of the mouse will it accelerate it all the way through the 
client and send absolute input events down.

--Michael Banack

Re: [PATCH v4 2/8] drm/atomic: Add support for mouse hotspots

[Pekka Paalanen]

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

On Mon, 3 Jul 2023 14:06:56 -0700
Michael Banack <banackm@vmware.com> wrote:

> Hi, I can speak to the virtual mouse/console half of this from the 
> VMware-side.
> 
> I believe Zack's preparing a new set of comments here that can speak to 
> most of your concerns, but I'll answer some of the other questions directly.
> 
> On 6/29/23 01:03, Pekka Paalanen wrote:
> >
> > Is it really required that the hotspot coordinates fall inside the
> > cursor plane? Will the atomic commit be rejected otherwise?  
> Most console systems require the hotspot to get within the cursor image, 
> but in theory it's semantically meaningful to have it extend outside the 
> image.
> 
> VMware's clients in particular will clamp the hotspot to the dimension 
> of the cursor image if we receive one that's out of bounds.
> 
> So I would assume the right thing to do here would be to allow it and 
> let the clients figure out how to best handle it.

Hi,

if it is normal that clients clamp the hotspot to inside the cursor
image, then I would come to the opposite conclusion: KMS UAPI needs to
require the hotspot to be within the cursor image. Otherwise the
results would be unpredictable, if clients still continue to clamp it
anyway. I would assume that clients in use today are not prepared to
handle hotspot outside the cursor image.

It is also not a big deal to require that. I think it would be very rare
to not have hotspot inside the cursor image, and even if it happened,
the only consequence would be that the guest display server falls back
to rendered cursor instead of a cursor plane. That may happen any time
anyway, if an application sets e.g. a huge cursor that exceeds cursor
plane size limits.

What I'm after with the question is that the requirement must be spelled
out clearly if it is there, or not even hinted at if it's not there.

> >  
> >>   *
> >>   * This information is only relevant for drivers working on top
> >>   * of para-virtualized hardware. The reason for that is that
> >>   * the hotspot is fairly encapsulated in the system but imagine having
> >>   * multiple windows with virtual machines running on servers
> >>   * across the globe, as you move the mouse across the screen
> >>   * and the cursor moves over those multiple windows you wouldn't
> >>   * want to be sending those mouse events to those machines, so virtual
> >>   * machine monitors implement an optimization where unless the mouse
> >>   * is locked to the VM window (e.g. via a click) instead of propagating
> >>   * those mouse events to those VM's they change the image of the native
> >>   * cursor to what's inside the mouse cursor plane and do not interact
> >>   * with the VM window until mouse is clicked in it.  
> > Surely the mouse events are sent to those machines across the globe
> > regardless?
> >
> > The point I believe you want to make is that in addition that, a
> > virtual machine viewer application independently moves the cursor image
> > on the viewer window to avoid the roundtrip latency across the globe
> > between mouse movement and cursor movement.
> >
> > Why is the locking you mention relevant? Wouldn't you do this
> > optimization always if there is any cursor plane image set?
> >
> > Or if you literally do mean that no input is sent to the VM at all
> > until the pointer is locked to that window, then why bother using the
> > guest cursor image without locking?
> >
> > I suppose different viewers could do different things, so maybe it's
> > not necessary to go into those details. Just the idea of the viewer
> > independently moving the cursor image to reduce hand-eye latency is
> > important, right?  
> 
> Yeah, the discussions of "locking" here are more implementation details 
> of how VMware's console works, and while  there are other consoles that 
> work this way, many of them don't.
> 
> So it's probably more confusing than helpful without some more 
> background that the other layers here don't need to worry about.

Cool.

> The main idea is that the client needs to know where the VM thinks the 
> mouse is to determine whether it /can/ safely accelerate it all the way 
> through to the client's cursor stack.  If something else in the VM is 
> moving the mouse around, like an additional remote connection, then the 
> client needs to fallback to an emulated cursor on the client side for a 
> while.

Good point.

> > The question of which input device corresponds to which cursor plane
> > might be good to answer too. I presume the VM runner is configured to
> > expose exactly one of each, so there can be only one association?  
> As far as I know, all of the VM consoles are written as though they 
> taking the place of what would the the physical monitors and input 
> devices on a native machine.  So they assume that there is one user, 
> sitting in front of one console, and all monitors/input devices are 
> being used by that user.

Ok, but having a single user does not mean that there cannot be
multiple independent pointers, especially on Wayland. The same with
keyboards.

> Any more complicated multi-user/multi-cursor setup would have to be 
> coordinated through a lot of layers (ie from the VM's userspace/kernel 
> and then through hypervisor/client-consoles), and as far as I know 
> nobody has tried to plumb that all the way through.  Even physical 
> multi-user/multi-console configurations like that are rare.

Right.

So if there a VM viewer client running on a Wayland system, that viewer
client may be presented with an arbitrary number of independent
pointer/keyboard/touchscreen input devices. Then it is up to the client
to pick one at a time to pass through to the VM.

That's fine.

I just think it would be good to document, that VM/viewer systems
expect to expose just a single pointer to the guest, hence it is
obvious which input device in the guest is associated with all the
cursor planes in the guest.

Btw. what do you do if a guest display server simultaneously uses
multiple cursor planes, assuming there are multiple outputs each with a
cursor plane? Or does the VM/viewer system limit the number of outputs
to one for the guest?

> >
> > Btw. for my own curiosity, what happens if there are two simultaneous
> > viewer instances open to the same VM/guest instance? Will they fight
> > over controlling the same pointer?  
> 
> I can't speak for all consoles, but VMware at least uses a bunch of 
> heuristics that typically boil down to which mouse input source has been 
> moving the cursor most recently.


Thanks,
pq

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

Re: [PATCH v4 2/8] drm/atomic: Add support for mouse hotspots

[Michael Banack]

On 7/4/23 01:08, Pekka Paalanen wrote:
> On Mon, 3 Jul 2023 14:06:56 -0700
> Michael Banack <banackm@vmware.com> wrote:
>
>> Hi, I can speak to the virtual mouse/console half of this from the
>> VMware-side.
>>
>> I believe Zack's preparing a new set of comments here that can speak to
>> most of your concerns, but I'll answer some of the other questions directly.
>>
>> On 6/29/23 01:03, Pekka Paalanen wrote:
>>> Is it really required that the hotspot coordinates fall inside the
>>> cursor plane? Will the atomic commit be rejected otherwise?
>> Most console systems require the hotspot to get within the cursor image,
>> but in theory it's semantically meaningful to have it extend outside the
>> image.
>>
>> VMware's clients in particular will clamp the hotspot to the dimension
>> of the cursor image if we receive one that's out of bounds.
>>
>> So I would assume the right thing to do here would be to allow it and
>> let the clients figure out how to best handle it.
> Hi,
>
> if it is normal that clients clamp the hotspot to inside the cursor
> image, then I would come to the opposite conclusion: KMS UAPI needs to
> require the hotspot to be within the cursor image. Otherwise the
> results would be unpredictable, if clients still continue to clamp it
> anyway. I would assume that clients in use today are not prepared to
> handle hotspot outside the cursor image.
>
> It is also not a big deal to require that. I think it would be very rare
> to not have hotspot inside the cursor image, and even if it happened,
> the only consequence would be that the guest display server falls back
> to rendered cursor instead of a cursor plane. That may happen any time
> anyway, if an application sets e.g. a huge cursor that exceeds cursor
> plane size limits.
Hypervisors are normally more privileged than the kernel, so any 
hypervisor/remoting client here really should be dealing with this case 
rather than trusting the kernel to handle it for them.

 From that perspective, we would normally try to preserve the 
application/guest semantics as much as possible, and then that gives us 
the ability to deal with this on the hypervisor side if it turns out 
that there's a critical case with the hotspot outside the image that we 
need to handle.

But that said, while I've seen real Guests do this in the past, I don't 
recall seeing this from any modern operating systems, so I don't think 
it's a big deal for us either way.

>
> What I'm after with the question is that the requirement must be spelled
> out clearly if it is there, or not even hinted at if it's not there.
Agreed.

>>> The question of which input device corresponds to which cursor plane
>>> might be good to answer too. I presume the VM runner is configured to
>>> expose exactly one of each, so there can be only one association?
>> As far as I know, all of the VM consoles are written as though they
>> taking the place of what would the the physical monitors and input
>> devices on a native machine.  So they assume that there is one user,
>> sitting in front of one console, and all monitors/input devices are
>> being used by that user.
> Ok, but having a single user does not mean that there cannot be
> multiple independent pointers, especially on Wayland. The same with
> keyboards.

True, and if the userspace is doing anything complicated here, the 
hypervisor has to be responsible for ensuring that whatever it's doing 
works with that, or else this system won't work.  I don't know that the 
kernel is in a good position to police that.

>
>> Any more complicated multi-user/multi-cursor setup would have to be
>> coordinated through a lot of layers (ie from the VM's userspace/kernel
>> and then through hypervisor/client-consoles), and as far as I know
>> nobody has tried to plumb that all the way through.  Even physical
>> multi-user/multi-console configurations like that are rare.
> Right.
>
> So if there a VM viewer client running on a Wayland system, that viewer
> client may be presented with an arbitrary number of independent
> pointer/keyboard/touchscreen input devices. Then it is up to the client
> to pick one at a time to pass through to the VM.
>
> That's fine.
>
> I just think it would be good to document, that VM/viewer systems
> expect to expose just a single pointer to the guest, hence it is
> obvious which input device in the guest is associated with all the
> cursor planes in the guest.

I don't have a problem adding something that suggests what we think the 
hypervisors are doing, but I would be a little cautious trying to 
prescribe what the hypervisors should be doing here.

I certainly can't speak for all of them, but we at least do a lot of odd 
tricks to keep this coordinated that violate what would normally be 
abstraction layers in a physical system such as having the mouse and the 
display adapter collude.  Ultimately it's the hypervisor that is 
responsible for doing the synchronization correctly, and the kernel 
really isn't involved there besides ferrying the right information down.

>
> Btw. what do you do if a guest display server simultaneously uses
> multiple cursor planes, assuming there are multiple outputs each with a
> cursor plane? Or does the VM/viewer system limit the number of outputs
> to one for the guest?

Zack would have to confirm what the vmwgfx driver does, but the VMware 
virtual display hardware at least only has one cursor position.  So I 
would assume that vmwgfx tries to only expose one plane and the rest get 
emulated, or else it just picks one to set live, but I'm not an expert 
on vmwgfx.

Normally we try to run a userspace agent in the Guest that also helps 
coordinate screen positions/resolutions to match what the user wanted on 
their client.  So when a user connects and requests from our UI that 
they want the screens to be a particular configuration, we then send a 
message to the userspace agent which coordinates with the display 
manager to request that setup.  You can certainly manually configure 
modes with things like rotation/topologies that break the console mouse, 
but we try not to put the user into that state as much as possible.  
Multiple cursors in the Guest display manager probably fall into that 
category.


--Michael Banack

Re: [PATCH v4 2/8] drm/atomic: Add support for mouse hotspots

[Pekka Paalanen]

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

On Wed, 5 Jul 2023 09:08:07 -0700
Michael Banack <banackm@vmware.com> wrote:

> On 7/4/23 01:08, Pekka Paalanen wrote:
> > On Mon, 3 Jul 2023 14:06:56 -0700
> > Michael Banack <banackm@vmware.com> wrote:
> >  
> >> Hi, I can speak to the virtual mouse/console half of this from the
> >> VMware-side.
> >>
> >> I believe Zack's preparing a new set of comments here that can speak to
> >> most of your concerns, but I'll answer some of the other questions directly.
> >>
> >> On 6/29/23 01:03, Pekka Paalanen wrote:  
> >>> Is it really required that the hotspot coordinates fall inside the
> >>> cursor plane? Will the atomic commit be rejected otherwise?  
> >> Most console systems require the hotspot to get within the cursor image,
> >> but in theory it's semantically meaningful to have it extend outside the
> >> image.
> >>
> >> VMware's clients in particular will clamp the hotspot to the dimension
> >> of the cursor image if we receive one that's out of bounds.
> >>
> >> So I would assume the right thing to do here would be to allow it and
> >> let the clients figure out how to best handle it.  
> > Hi,
> >
> > if it is normal that clients clamp the hotspot to inside the cursor
> > image, then I would come to the opposite conclusion: KMS UAPI needs to
> > require the hotspot to be within the cursor image. Otherwise the
> > results would be unpredictable, if clients still continue to clamp it
> > anyway. I would assume that clients in use today are not prepared to
> > handle hotspot outside the cursor image.
> >
> > It is also not a big deal to require that. I think it would be very rare
> > to not have hotspot inside the cursor image, and even if it happened,
> > the only consequence would be that the guest display server falls back
> > to rendered cursor instead of a cursor plane. That may happen any time
> > anyway, if an application sets e.g. a huge cursor that exceeds cursor
> > plane size limits.  
> Hypervisors are normally more privileged than the kernel, so any 
> hypervisor/remoting client here really should be dealing with this case 
> rather than trusting the kernel to handle it for them.

Sorry, handle what? Trust the guest kernel to do what?

Personally I'm only interested in the KMS UAPI the guest kernel offers
to guest userspace, and requiring hotspot to be inside the cursor image
is fine. I don't think it needs even a strong justification, it's what
most would likely expect, and expectations are good to record in spec.

The UAPI contract is between (guest) kernel and (guest) userspace, and
I expect the kernel to fully enforce that towards the userspace.

I understand that hypervisors cannot trust guest kernels for security,
but I also think that's a different matter.

>  From that perspective, we would normally try to preserve the 
> application/guest semantics as much as possible, and then that gives us 
> the ability to deal with this on the hypervisor side if it turns out 
> that there's a critical case with the hotspot outside the image that we 
> need to handle.
> 
> But that said, while I've seen real Guests do this in the past, I don't 
> recall seeing this from any modern operating systems, so I don't think 
> it's a big deal for us either way.
> 
> >
> > What I'm after with the question is that the requirement must be spelled
> > out clearly if it is there, or not even hinted at if it's not there.  
> Agreed.
> 
> >>> The question of which input device corresponds to which cursor plane
> >>> might be good to answer too. I presume the VM runner is configured to
> >>> expose exactly one of each, so there can be only one association?  
> >> As far as I know, all of the VM consoles are written as though they
> >> taking the place of what would the the physical monitors and input
> >> devices on a native machine.  So they assume that there is one user,
> >> sitting in front of one console, and all monitors/input devices are
> >> being used by that user.  
> > Ok, but having a single user does not mean that there cannot be
> > multiple independent pointers, especially on Wayland. The same with
> > keyboards.  
> 
> True, and if the userspace is doing anything complicated here, the 
> hypervisor has to be responsible for ensuring that whatever it's doing 
> works with that, or else this system won't work.  I don't know that the 
> kernel is in a good position to police that.

What do you mean by policing here?

Isn't it the hypervisor that determines what virtual input devices will
be available to the guest OS? Therefore, the hypervisor is in a
position to expose exactly one pointer device and exactly one
cursor plane to guest OS which means the guest OS cannot get the
association wrong. If that's the general and expected hypervisor
policy, then there is no need to design explicit device association in
the guest kernel UAPI. If so, I'd like it to be mentioned in the kernel
docs, too.

> >  
> >> Any more complicated multi-user/multi-cursor setup would have to be
> >> coordinated through a lot of layers (ie from the VM's userspace/kernel
> >> and then through hypervisor/client-consoles), and as far as I know
> >> nobody has tried to plumb that all the way through.  Even physical
> >> multi-user/multi-console configurations like that are rare.  
> > Right.
> >
> > So if there a VM viewer client running on a Wayland system, that viewer
> > client may be presented with an arbitrary number of independent
> > pointer/keyboard/touchscreen input devices. Then it is up to the client
> > to pick one at a time to pass through to the VM.
> >
> > That's fine.
> >
> > I just think it would be good to document, that VM/viewer systems
> > expect to expose just a single pointer to the guest, hence it is
> > obvious which input device in the guest is associated with all the
> > cursor planes in the guest.  
> 
> I don't have a problem adding something that suggests what we think the 
> hypervisors are doing, but I would be a little cautious trying to 
> prescribe what the hypervisors should be doing here.

If the UAPI has been designed to cater for specific hypervisor
configurations, then I think the assumptions should definitely be
documented in the UAPI. Hypervisor developers can look at the UAPI and
see what it caters for and what it doesn't. It's not a spec for what
hypervisors must or must not do, but an explanation of what works and
what doesn't given that guest userspace is forced to follow the UAPI.

If there is no record of how the input vs. output device association is
expected to be handled, I will be raising questions about it until it
is.

Having limitations is fine, but they need to be documented.

> I certainly can't speak for all of them, but we at least do a lot of odd 
> tricks to keep this coordinated that violate what would normally be 
> abstraction layers in a physical system such as having the mouse and the 
> display adapter collude.  Ultimately it's the hypervisor that is 
> responsible for doing the synchronization correctly, and the kernel 
> really isn't involved there besides ferrying the right information down.

Are you happy with that, having to chase and special-case guest OS quirks?

Or would you rather know how a guest Linux kernel expects and enforces
guest userspace to behave, and develop for that, making all Linux OSs
look fairly similar?

You have a golden opportunity here to define how a Linux guest OS needs
to behave. When it's enshrined in Linux UAPI, it will hold for decades,
too.

> >
> > Btw. what do you do if a guest display server simultaneously uses
> > multiple cursor planes, assuming there are multiple outputs each with a
> > cursor plane? Or does the VM/viewer system limit the number of outputs
> > to one for the guest?  
> 
> Zack would have to confirm what the vmwgfx driver does, but the VMware 
> virtual display hardware at least only has one cursor position.  So I 
> would assume that vmwgfx tries to only expose one plane and the rest get 
> emulated, or else it just picks one to set live, but I'm not an expert 
> on vmwgfx.

Right. I would not expect a guest driver to invent more virtual devices
than what the hypervisor exposes.

I believe that using universal planes KMS UAPI, a guest display driver
can also expose a single cursor plane that can migrate between CRTCs.

> Normally we try to run a userspace agent in the Guest that also helps 
> coordinate screen positions/resolutions to match what the user wanted on 
> their client.  So when a user connects and requests from our UI that 
> they want the screens to be a particular configuration, we then send a 
> message to the userspace agent which coordinates with the display 
> manager to request that setup.  You can certainly manually configure 
> modes with things like rotation/topologies that break the console mouse, 
> but we try not to put the user into that state as much as possible.  
> Multiple cursors in the Guest display manager probably fall into that 
> category.

That sounds like something that only works with Xorg as the guest
display server, as X11 allows you to do that, and Wayland does not.

You could do similar things through the guest kernel display driver by
manufacturing hotplug events and changing read-only KMS properties
accordingly, at least to some degree.

At some point, extending KMS for virtualized use cases stops being
reasonable and it would be better to connect to the guest using VNC,
RDP, or such. But I think adding hotspot properties is on the
reasonable side and far from that line.


Thanks,
pq

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

Re: [PATCH v4 2/8] drm/atomic: Add support for mouse hotspots

[Michael Banack]

On 7/6/23 01:01, Pekka Paalanen wrote:
> On Wed, 5 Jul 2023 09:08:07 -0700
> Michael Banack <banackm@vmware.com> wrote:
>
>> On 7/4/23 01:08, Pekka Paalanen wrote:
>>> On Mon, 3 Jul 2023 14:06:56 -0700
>>> Michael Banack <banackm@vmware.com> wrote:
>>>   
>>>> Hi, I can speak to the virtual mouse/console half of this from the
>>>> VMware-side.
>>>>
>>>> I believe Zack's preparing a new set of comments here that can speak to
>>>> most of your concerns, but I'll answer some of the other questions directly.
>>>>
>>>> On 6/29/23 01:03, Pekka Paalanen wrote:
>>>>> Is it really required that the hotspot coordinates fall inside the
>>>>> cursor plane? Will the atomic commit be rejected otherwise?
>>>> Most console systems require the hotspot to get within the cursor image,
>>>> but in theory it's semantically meaningful to have it extend outside the
>>>> image.
>>>>
>>>> VMware's clients in particular will clamp the hotspot to the dimension
>>>> of the cursor image if we receive one that's out of bounds.
>>>>
>>>> So I would assume the right thing to do here would be to allow it and
>>>> let the clients figure out how to best handle it.
>>> Hi,
>>>
>>> if it is normal that clients clamp the hotspot to inside the cursor
>>> image, then I would come to the opposite conclusion: KMS UAPI needs to
>>> require the hotspot to be within the cursor image. Otherwise the
>>> results would be unpredictable, if clients still continue to clamp it
>>> anyway. I would assume that clients in use today are not prepared to
>>> handle hotspot outside the cursor image.
>>>
>>> It is also not a big deal to require that. I think it would be very rare
>>> to not have hotspot inside the cursor image, and even if it happened,
>>> the only consequence would be that the guest display server falls back
>>> to rendered cursor instead of a cursor plane. That may happen any time
>>> anyway, if an application sets e.g. a huge cursor that exceeds cursor
>>> plane size limits.
>> Hypervisors are normally more privileged than the kernel, so any
>> hypervisor/remoting client here really should be dealing with this case
>> rather than trusting the kernel to handle it for them.
> Sorry, handle what? Trust the guest kernel to do what?
>
> Personally I'm only interested in the KMS UAPI the guest kernel offers
> to guest userspace, and requiring hotspot to be inside the cursor image
> is fine. I don't think it needs even a strong justification, it's what
> most would likely expect, and expectations are good to record in spec.
>
> The UAPI contract is between (guest) kernel and (guest) userspace, and
> I expect the kernel to fully enforce that towards the userspace.
>
> I understand that hypervisors cannot trust guest kernels for security,
> but I also think that's a different matter.

You were saying that results would be unpredictable if the kernel 
allowed hotspots to be outside the dimensions of the cursor image. I'm 
not clear in what way you think that would cause unpredictable results, 
or what problems that would cause?

Essentially setting the hotspot properties means that the hypervisor 
console can choose to either draw the cursor where the plane is located, 
or use the cursor-plane + hotspot information to draw the cursor where 
the user's mouse is on the client.

That works the same whether the hotspot is clamped or not.  We mostly 
use clamping to avoid pathological cases (like a hotspot ot MAX_UINT32), 
and get away with it because real Guest applications that do this are 
very rare.
>>>>> The question of which input device corresponds to which cursor plane
>>>>> might be good to answer too. I presume the VM runner is configured to
>>>>> expose exactly one of each, so there can be only one association?
>>>> As far as I know, all of the VM consoles are written as though they
>>>> taking the place of what would the the physical monitors and input
>>>> devices on a native machine.  So they assume that there is one user,
>>>> sitting in front of one console, and all monitors/input devices are
>>>> being used by that user.
>>> Ok, but having a single user does not mean that there cannot be
>>> multiple independent pointers, especially on Wayland. The same with
>>> keyboards.
>> True, and if the userspace is doing anything complicated here, the
>> hypervisor has to be responsible for ensuring that whatever it's doing
>> works with that, or else this system won't work.  I don't know that the
>> kernel is in a good position to police that.
> What do you mean by policing here?
>
> Isn't it the hypervisor that determines what virtual input devices will
> be available to the guest OS? Therefore, the hypervisor is in a
> position to expose exactly one pointer device and exactly one
> cursor plane to guest OS which means the guest OS cannot get the
> association wrong. If that's the general and expected hypervisor
> policy, then there is no need to design explicit device association in
> the guest kernel UAPI. If so, I'd like it to be mentioned in the kernel
> docs, too.

I'm not entirely sure how to fit what you're calling a "pointer" into my 
mental model of what the hypervisor is doing...

For a typical Linux Guest, we currently expose 3+ virtual mouse devices, 
and choose which one to send input to based on what their guest drivers 
are doing, and what kind of input we got from the client.  We expect the 
input from all of those to end up in the same user desktop session, 
which we expect to own all the virtual screens, and that the user the 
only gets one cursor image from that.

But we think of that as being a contract between the user desktop and 
the hypervisor, not the graphics/mouse drivers.  I might be out of touch 
with how Wayland/KMS thinks of this, but normally the user desktop is 
receiving the mouse events (in terms of either relative dx/dy, or 
absolute mouse device coordinates [0, MAX_UINT32] or something) and 
mapping those onto specific pixels in the user's desktop, which then 
gets passed up to the graphics driver as the location of the mouse cursor.

So I guess I'm not clear on what kind of usermode<=>kernel contract you 
want here if the kernel isn't what's owning the translation between the 
mouse input and the cursor position.  The hypervisor awkwardly has to 
straddle both the input/graphics domain, and we do so by making 
assumptions about how the user desktop is going to behave.

 From VMware's perspective, I think it's fair to document that all input 
devices are expected to feed into the same single cursor plane.  Or to 
generalize that slightly, if a virtual graphics driver chose to expose 
multiple cursor planes, then I think noting that it's the hypervisor's 
responsibility to ensure that it's synchronizing the correct cursor 
hotspot with the correct user pointer is probably also fair, but we 
would be extrapolating past what anyone is doing today (as far as I'm 
aware).

>
>>>   
>>>> Any more complicated multi-user/multi-cursor setup would have to be
>>>> coordinated through a lot of layers (ie from the VM's userspace/kernel
>>>> and then through hypervisor/client-consoles), and as far as I know
>>>> nobody has tried to plumb that all the way through.  Even physical
>>>> multi-user/multi-console configurations like that are rare.
>>> Right.
>>>
>>> So if there a VM viewer client running on a Wayland system, that viewer
>>> client may be presented with an arbitrary number of independent
>>> pointer/keyboard/touchscreen input devices. Then it is up to the client
>>> to pick one at a time to pass through to the VM.
>>>
>>> That's fine.
>>>
>>> I just think it would be good to document, that VM/viewer systems
>>> expect to expose just a single pointer to the guest, hence it is
>>> obvious which input device in the guest is associated with all the
>>> cursor planes in the guest.
>> I don't have a problem adding something that suggests what we think the
>> hypervisors are doing, but I would be a little cautious trying to
>> prescribe what the hypervisors should be doing here.
> If the UAPI has been designed to cater for specific hypervisor
> configurations, then I think the assumptions should definitely be
> documented in the UAPI. Hypervisor developers can look at the UAPI and
> see what it caters for and what it doesn't. It's not a spec for what
> hypervisors must or must not do, but an explanation of what works and
> what doesn't given that guest userspace is forced to follow the UAPI.
>
> If there is no record of how the input vs. output device association is
> expected to be handled, I will be raising questions about it until it
> is.
>
> Having limitations is fine, but they need to be documented.

I think my confusion here is that if we were to try and support 
multi-user or multi-pointer sessions, our instinct would probably be to 
bypass the kernel entirely and work with a userspace<->hypervisor 
channel for communicating what the user desktops think the session 
topology is.

But as I noted above, I think it's fair to document that this is all 
assumed to be working in an environment where there is one cursor plane 
shared across all displays, and all input devices used by the hypervisor 
are processed as part of that session.  (If that's what you're looking 
for...)

>
>> I certainly can't speak for all of them, but we at least do a lot of odd
>> tricks to keep this coordinated that violate what would normally be
>> abstraction layers in a physical system such as having the mouse and the
>> display adapter collude.  Ultimately it's the hypervisor that is
>> responsible for doing the synchronization correctly, and the kernel
>> really isn't involved there besides ferrying the right information down.
> Are you happy with that, having to chase and special-case guest OS quirks?
>
> Or would you rather know how a guest Linux kernel expects and enforces
> guest userspace to behave, and develop for that, making all Linux OSs
> look fairly similar?
>
> You have a golden opportunity here to define how a Linux guest OS needs
> to behave. When it's enshrined in Linux UAPI, it will hold for decades,
> too.

I mean, we're certainly happy to make this as nice as possible for 
ourselves and others, but when we're trying to support OS's from the 
last 30+ years, we end up with a lot of quirks no matter what we do.

I mentioned earlier about the display<=>input mapping, but the model we 
use internally is closer to what a desktop manager is doing that a 
kernel.  So each virtual display is rooted at a point in the topology 
that corresponds to the user desktop's idea of how the mouse moves 
around the screens, and then we use that to map client mouse coordinates 
into whatever space the input device is using so that the guest's 
desktop send the mouse to the correct location.

I'm not a KMS expert either, but I thought that the X11/Wayland 
component was still doing that display<=>mouse mapping and the kernel 
just matched up the display images with the monitors.

>
>>> Btw. what do you do if a guest display server simultaneously uses
>>> multiple cursor planes, assuming there are multiple outputs each with a
>>> cursor plane? Or does the VM/viewer system limit the number of outputs
>>> to one for the guest?
>> Zack would have to confirm what the vmwgfx driver does, but the VMware
>> virtual display hardware at least only has one cursor position.  So I
>> would assume that vmwgfx tries to only expose one plane and the rest get
>> emulated, or else it just picks one to set live, but I'm not an expert
>> on vmwgfx.
> Right. I would not expect a guest driver to invent more virtual devices
> than what the hypervisor exposes.
>
> I believe that using universal planes KMS UAPI, a guest display driver
> can also expose a single cursor plane that can migrate between CRTCs.
>
>> Normally we try to run a userspace agent in the Guest that also helps
>> coordinate screen positions/resolutions to match what the user wanted on
>> their client.  So when a user connects and requests from our UI that
>> they want the screens to be a particular configuration, we then send a
>> message to the userspace agent which coordinates with the display
>> manager to request that setup.  You can certainly manually configure
>> modes with things like rotation/topologies that break the console mouse,
>> but we try not to put the user into that state as much as possible.
>> Multiple cursors in the Guest display manager probably fall into that
>> category.
> That sounds like something that only works with Xorg as the guest
> display server, as X11 allows you to do that, and Wayland does not.
>
> You could do similar things through the guest kernel display driver by
> manufacturing hotplug events and changing read-only KMS properties
> accordingly, at least to some degree.
Yeah, what we have now is definitely X11-focused.  We've certainly 
thought about using hotplug events for controlling the display updates, 
and might move that direction someday.

>
> At some point, extending KMS for virtualized use cases stops being
> reasonable and it would be better to connect to the guest using VNC,
> RDP, or such. But I think adding hotspot properties is on the
> reasonable side and far from that line.

Possibly, yeah.  I mean, so far I don't think we're talking much about 
additional extensions (beyond the hotspot), but rather additional 
restrictions on what the desktop manager is doing.  But if more exotic 
usage of KMS becomes normal then that would be an interesting time to 
look at other options.

--Michael Banack

Re: [PATCH v4 2/8] drm/atomic: Add support for mouse hotspots

[Pekka Paalanen]

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

On Thu, 6 Jul 2023 09:23:46 -0700
Michael Banack <banackm@vmware.com> wrote:

> On 7/6/23 01:01, Pekka Paalanen wrote:
> > On Wed, 5 Jul 2023 09:08:07 -0700
> > Michael Banack <banackm@vmware.com> wrote:
> >  
> >> On 7/4/23 01:08, Pekka Paalanen wrote:  
> >>> On Mon, 3 Jul 2023 14:06:56 -0700
> >>> Michael Banack <banackm@vmware.com> wrote:
> >>>     
> >>>> Hi, I can speak to the virtual mouse/console half of this from the
> >>>> VMware-side.
> >>>>
> >>>> I believe Zack's preparing a new set of comments here that can speak to
> >>>> most of your concerns, but I'll answer some of the other questions directly.
> >>>>
> >>>> On 6/29/23 01:03, Pekka Paalanen wrote:  
> >>>>> Is it really required that the hotspot coordinates fall inside the
> >>>>> cursor plane? Will the atomic commit be rejected otherwise?  
> >>>> Most console systems require the hotspot to get within the cursor image,
> >>>> but in theory it's semantically meaningful to have it extend outside the
> >>>> image.
> >>>>
> >>>> VMware's clients in particular will clamp the hotspot to the dimension
> >>>> of the cursor image if we receive one that's out of bounds.
> >>>>
> >>>> So I would assume the right thing to do here would be to allow it and
> >>>> let the clients figure out how to best handle it.  
> >>> Hi,
> >>>
> >>> if it is normal that clients clamp the hotspot to inside the cursor
> >>> image, then I would come to the opposite conclusion: KMS UAPI needs to
> >>> require the hotspot to be within the cursor image. Otherwise the
> >>> results would be unpredictable, if clients still continue to clamp it
> >>> anyway. I would assume that clients in use today are not prepared to
> >>> handle hotspot outside the cursor image.
> >>>
> >>> It is also not a big deal to require that. I think it would be very rare
> >>> to not have hotspot inside the cursor image, and even if it happened,
> >>> the only consequence would be that the guest display server falls back
> >>> to rendered cursor instead of a cursor plane. That may happen any time
> >>> anyway, if an application sets e.g. a huge cursor that exceeds cursor
> >>> plane size limits.  
> >> Hypervisors are normally more privileged than the kernel, so any
> >> hypervisor/remoting client here really should be dealing with this case
> >> rather than trusting the kernel to handle it for them.  
> > Sorry, handle what? Trust the guest kernel to do what?
> >
> > Personally I'm only interested in the KMS UAPI the guest kernel offers
> > to guest userspace, and requiring hotspot to be inside the cursor image
> > is fine. I don't think it needs even a strong justification, it's what
> > most would likely expect, and expectations are good to record in spec.
> >
> > The UAPI contract is between (guest) kernel and (guest) userspace, and
> > I expect the kernel to fully enforce that towards the userspace.
> >
> > I understand that hypervisors cannot trust guest kernels for security,
> > but I also think that's a different matter.  
> 
> You were saying that results would be unpredictable if the kernel 
> allowed hotspots to be outside the dimensions of the cursor image. I'm 
> not clear in what way you think that would cause unpredictable results, 
> or what problems that would cause?

That statement was based on the assumption that existing hypervisors
and VM viewer applications are not prepared to deal with hotspots
outside of cursor image. Therefore, if a guest is upgraded to a version
that uses hotspots outside of cursor images, and the VM stack is not
updated, it will malfunction.

Therefore it is best to model the new UAPI in a way that is compatible
with existing VM stack, especially since allowing this new feature
(hotspots outside of cursor image) has no known benefits.

Below I see my assumption was incorrect, but it still causes you to
fall back to something less optimal.

> Essentially setting the hotspot properties means that the hypervisor 
> console can choose to either draw the cursor where the plane is located, 
> or use the cursor-plane + hotspot information to draw the cursor where 
> the user's mouse is on the client.
> 
> That works the same whether the hotspot is clamped or not.  We mostly 
> use clamping to avoid pathological cases (like a hotspot ot MAX_UINT32), 
> and get away with it because real Guest applications that do this are 
> very rare.

My point here is that you can design the new Linux UAPI to help you:
you can rule out cases that would lead to non-optimal behaviour, like
falling back to the drawing of cursor plane you mention when it would
be better to commandeer the cursor plane with the hotspot information.

> >>>>> The question of which input device corresponds to which cursor plane
> >>>>> might be good to answer too. I presume the VM runner is configured to
> >>>>> expose exactly one of each, so there can be only one association?  
> >>>> As far as I know, all of the VM consoles are written as though they
> >>>> taking the place of what would the the physical monitors and input
> >>>> devices on a native machine.  So they assume that there is one user,
> >>>> sitting in front of one console, and all monitors/input devices are
> >>>> being used by that user.  
> >>> Ok, but having a single user does not mean that there cannot be
> >>> multiple independent pointers, especially on Wayland. The same with
> >>> keyboards.  
> >> True, and if the userspace is doing anything complicated here, the
> >> hypervisor has to be responsible for ensuring that whatever it's doing
> >> works with that, or else this system won't work.  I don't know that the
> >> kernel is in a good position to police that.  
> > What do you mean by policing here?
> >
> > Isn't it the hypervisor that determines what virtual input devices will
> > be available to the guest OS? Therefore, the hypervisor is in a
> > position to expose exactly one pointer device and exactly one
> > cursor plane to guest OS which means the guest OS cannot get the
> > association wrong. If that's the general and expected hypervisor
> > policy, then there is no need to design explicit device association in
> > the guest kernel UAPI. If so, I'd like it to be mentioned in the kernel
> > docs, too.  
> 
> I'm not entirely sure how to fit what you're calling a "pointer" into my 
> mental model of what the hypervisor is doing...

My definition: A pointer is a pointing input device that requires a
cursor image to be drawn at the right location for it to be usable.

> For a typical Linux Guest, we currently expose 3+ virtual mouse devices, 
> and choose which one to send input to based on what their guest drivers 
> are doing, and what kind of input we got from the client.  We expect the 
> input from all of those to end up in the same user desktop session, 
> which we expect to own all the virtual screens, and that the user the 
> only gets one cursor image from that.

Why do you need to expose so many pointer devices? Just curious.

If you do expose multiple mouse (pointer) devices, then guest OS can
choose to use each of them as a different independent cursor on the
same desktop. The only thing stopping that is that it's not usually
useful, so it's not done.

Therefore, what you need to document in the Linux UAPI instead is that
*all* pointer devices are associated with the *same* cursor plane. That
forbids the multi-pointer pointer scenario the VM stack cannot handle.

> But we think of that as being a contract between the user desktop and 
> the hypervisor, not the graphics/mouse drivers.  I might be out of touch 
> with how Wayland/KMS thinks of this, but normally the user desktop is 
> receiving the mouse events (in terms of either relative dx/dy, or 
> absolute mouse device coordinates [0, MAX_UINT32] or something) and 
> mapping those onto specific pixels in the user's desktop, which then 
> gets passed up to the graphics driver as the location of the mouse cursor.

The contract between the guest desktop and the hypervisor is the Linux
UAPI contract. There is no other contract that generic guest userspace
knows of. The UAPI is defined by Linux, and implemented by Linux device
drivers and shared subsystem cores. Wayland is not part of that
contract, but DRM KMS is. Specifically, it is the Wayland or X11
display server that uses the contract on behalf of the guest desktop.

On other parts, yes, you're correct.

> So I guess I'm not clear on what kind of usermode<=>kernel contract you 
> want here if the kernel isn't what's owning the translation between the 
> mouse input and the cursor position.  The hypervisor awkwardly has to 
> straddle both the input/graphics domain, and we do so by making 
> assumptions about how the user desktop is going to behave.

Correct. In order to reduce that awkwardness, I encourage you to write
down the expectations and requirements in this new Linux UAPI (the KMS
cursor place hotspot property). Then you can be much more confident on
how a random Linux desktop will behave.

It will also help the reviewers here to understand what the new UAPI is
and how it is supposed to work.

>  From VMware's perspective, I think it's fair to document that all input 
> devices are expected to feed into the same single cursor plane.  Or to 
> generalize that slightly, if a virtual graphics driver chose to expose 
> multiple cursor planes, then I think noting that it's the hypervisor's 
> responsibility to ensure that it's synchronizing the correct cursor 
> hotspot with the correct user pointer is probably also fair, but we 
> would be extrapolating past what anyone is doing today (as far as I'm 
> aware).

Right. I think it's best to leave multiple cursor planes scenario out
for now, because there is no way the hypervisor could know which is
which for the guest userspace until you add more Linux UAPI to
communicate that.

People developing KMS, and for KMS, are accustomed to every CRTC
(output) potentially having its own cursor plane. Hence, it is a
surprise to rely on there being essentially only one cursor plane on
the whole DRM device.

> >>>     
> >>>> Any more complicated multi-user/multi-cursor setup would have to be
> >>>> coordinated through a lot of layers (ie from the VM's userspace/kernel
> >>>> and then through hypervisor/client-consoles), and as far as I know
> >>>> nobody has tried to plumb that all the way through.  Even physical
> >>>> multi-user/multi-console configurations like that are rare.  
> >>> Right.
> >>>
> >>> So if there a VM viewer client running on a Wayland system, that viewer
> >>> client may be presented with an arbitrary number of independent
> >>> pointer/keyboard/touchscreen input devices. Then it is up to the client
> >>> to pick one at a time to pass through to the VM.
> >>>
> >>> That's fine.
> >>>
> >>> I just think it would be good to document, that VM/viewer systems
> >>> expect to expose just a single pointer to the guest, hence it is
> >>> obvious which input device in the guest is associated with all the
> >>> cursor planes in the guest.  
> >> I don't have a problem adding something that suggests what we think the
> >> hypervisors are doing, but I would be a little cautious trying to
> >> prescribe what the hypervisors should be doing here.  
> > If the UAPI has been designed to cater for specific hypervisor
> > configurations, then I think the assumptions should definitely be
> > documented in the UAPI. Hypervisor developers can look at the UAPI and
> > see what it caters for and what it doesn't. It's not a spec for what
> > hypervisors must or must not do, but an explanation of what works and
> > what doesn't given that guest userspace is forced to follow the UAPI.
> >
> > If there is no record of how the input vs. output device association is
> > expected to be handled, I will be raising questions about it until it
> > is.
> >
> > Having limitations is fine, but they need to be documented.  
> 
> I think my confusion here is that if we were to try and support 
> multi-user or multi-pointer sessions, our instinct would probably be to 
> bypass the kernel entirely and work with a userspace<->hypervisor 
> channel for communicating what the user desktops think the session 
> topology is.

In that case it is outside of the Linux UAPI scope, and that is fine.
You would not use Linux DRM devices for output or Linux input devices
for input. I mentioned VNC and RDP previously as options. RDP has even
extensions to take advantage of device and memory sharing with the host
AFAIK.

> But as I noted above, I think it's fair to document that this is all 
> assumed to be working in an environment where there is one cursor plane 
> shared across all displays, and all input devices used by the hypervisor 
> are processed as part of that session.  (If that's what you're looking 
> for...)

Yes!

> >  
> >> I certainly can't speak for all of them, but we at least do a lot of odd
> >> tricks to keep this coordinated that violate what would normally be
> >> abstraction layers in a physical system such as having the mouse and the
> >> display adapter collude.  Ultimately it's the hypervisor that is
> >> responsible for doing the synchronization correctly, and the kernel
> >> really isn't involved there besides ferrying the right information down.  
> > Are you happy with that, having to chase and special-case guest OS quirks?
> >
> > Or would you rather know how a guest Linux kernel expects and enforces
> > guest userspace to behave, and develop for that, making all Linux OSs
> > look fairly similar?
> >
> > You have a golden opportunity here to define how a Linux guest OS needs
> > to behave. When it's enshrined in Linux UAPI, it will hold for decades,
> > too.  
> 
> I mean, we're certainly happy to make this as nice as possible for 
> ourselves and others, but when we're trying to support OS's from the 
> last 30+ years, we end up with a lot of quirks no matter what we do.
> 
> I mentioned earlier about the display<=>input mapping, but the model we 
> use internally is closer to what a desktop manager is doing that a 
> kernel.  So each virtual display is rooted at a point in the topology 
> that corresponds to the user desktop's idea of how the mouse moves 
> around the screens, and then we use that to map client mouse coordinates 
> into whatever space the input device is using so that the guest's 
> desktop send the mouse to the correct location.
> 
> I'm not a KMS expert either, but I thought that the X11/Wayland 
> component was still doing that display<=>mouse mapping and the kernel 
> just matched up the display images with the monitors.

Correct.

However, in your case, the userspace (Wayland or X11 display server) is
not free to choose any arbitrary input-cursor association they might
want. You have a specific expectation that all pointing devices control
the same pointer. Since the hotspot property is exclusive to your use
case, I think it make sense to write down the expectations with the
hotspot property. Guest userspace has to explicitly program for the
hotspot property anyway, so it can also take care of your requirements.

This is the whole reason why paravirtualized cursor planes are being
hidden from atomic KMS userspace unless userspace promises to use the
hotspot property correctly: some Wayland compositors are happy to put
regular windows on cursor planes when there is no other use for a
cursor plane from time to time, to avoid CPU or GPU composition. After
all, the main use of KMS planes is to off-load parts of desktop
composition to dedicated display hardware in order to save power and
improve performance.

Btw. rather than with the KMS hotspot property literally, these things
could also be documented with the flag
(DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT) that userspace uses to promise
that it handles the hotspot property correctly.


Thanks,
pq

> >  
> >>> Btw. what do you do if a guest display server simultaneously uses
> >>> multiple cursor planes, assuming there are multiple outputs each with a
> >>> cursor plane? Or does the VM/viewer system limit the number of outputs
> >>> to one for the guest?  
> >> Zack would have to confirm what the vmwgfx driver does, but the VMware
> >> virtual display hardware at least only has one cursor position.  So I
> >> would assume that vmwgfx tries to only expose one plane and the rest get
> >> emulated, or else it just picks one to set live, but I'm not an expert
> >> on vmwgfx.  
> > Right. I would not expect a guest driver to invent more virtual devices
> > than what the hypervisor exposes.
> >
> > I believe that using universal planes KMS UAPI, a guest display driver
> > can also expose a single cursor plane that can migrate between CRTCs.
> >  
> >> Normally we try to run a userspace agent in the Guest that also helps
> >> coordinate screen positions/resolutions to match what the user wanted on
> >> their client.  So when a user connects and requests from our UI that
> >> they want the screens to be a particular configuration, we then send a
> >> message to the userspace agent which coordinates with the display
> >> manager to request that setup.  You can certainly manually configure
> >> modes with things like rotation/topologies that break the console mouse,
> >> but we try not to put the user into that state as much as possible.
> >> Multiple cursors in the Guest display manager probably fall into that
> >> category.  
> > That sounds like something that only works with Xorg as the guest
> > display server, as X11 allows you to do that, and Wayland does not.
> >
> > You could do similar things through the guest kernel display driver by
> > manufacturing hotplug events and changing read-only KMS properties
> > accordingly, at least to some degree.  
> Yeah, what we have now is definitely X11-focused.  We've certainly 
> thought about using hotplug events for controlling the display updates, 
> and might move that direction someday.
> 
> >
> > At some point, extending KMS for virtualized use cases stops being
> > reasonable and it would be better to connect to the guest using VNC,
> > RDP, or such. But I think adding hotspot properties is on the
> > reasonable side and far from that line.  
> 
> Possibly, yeah.  I mean, so far I don't think we're talking much about 
> additional extensions (beyond the hotspot), but rather additional 
> restrictions on what the desktop manager is doing.  But if more exotic 
> usage of KMS becomes normal then that would be an interesting time to 
> look at other options.
> 
> --Michael Banack


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

Re: [PATCH v4 2/8] drm/atomic: Add support for mouse hotspots

[Michael Banack]

On 7/7/23 01:38, Pekka Paalanen wrote:
>
> That statement was based on the assumption that existing hypervisors
> and VM viewer applications are not prepared to deal with hotspots
> outside of cursor image. Therefore, if a guest is upgraded to a version
> that uses hotspots outside of cursor images, and the VM stack is not
> updated, it will malfunction.
>
> Therefore it is best to model the new UAPI in a way that is compatible
> with existing VM stack, especially since allowing this new feature
> (hotspots outside of cursor image) has no known benefits.
>
> Below I see my assumption was incorrect, but it still causes you to
> fall back to something less optimal.

Okay, right.  That's why I was saying that it's not a big deal either 
way to VMware, but I wanted to at least make the case for allowing 
somewhat arbitrary hotspots just because it is semantically meaningful, 
and I don't know if any other hypervisors care about allowing it more 
than we do.


>> Essentially setting the hotspot properties means that the hypervisor
>> console can choose to either draw the cursor where the plane is located,
>> or use the cursor-plane + hotspot information to draw the cursor where
>> the user's mouse is on the client.
>>
>> That works the same whether the hotspot is clamped or not.  We mostly
>> use clamping to avoid pathological cases (like a hotspot ot MAX_UINT32),
>> and get away with it because real Guest applications that do this are
>> very rare.
> My point here is that you can design the new Linux UAPI to help you:
> you can rule out cases that would lead to non-optimal behaviour, like
> falling back to the drawing of cursor plane you mention when it would
> be better to commandeer the cursor plane with the hotspot information.

We can't though, because we can't trust the guest kernel any more than 
the kernel can trust userspace.

So we need to handle these cases one way or another, both for older 
guests, and to ensure we don't have some security issue from a malicious 
guest kernel.

>
>>>>>>> The question of which input device corresponds to which cursor plane
>>>>>>> might be good to answer too. I presume the VM runner is configured to
>>>>>>> expose exactly one of each, so there can be only one association?
>>>>>> As far as I know, all of the VM consoles are written as though they
>>>>>> taking the place of what would the the physical monitors and input
>>>>>> devices on a native machine.  So they assume that there is one user,
>>>>>> sitting in front of one console, and all monitors/input devices are
>>>>>> being used by that user.
>>>>> Ok, but having a single user does not mean that there cannot be
>>>>> multiple independent pointers, especially on Wayland. The same with
>>>>> keyboards.
>>>> True, and if the userspace is doing anything complicated here, the
>>>> hypervisor has to be responsible for ensuring that whatever it's doing
>>>> works with that, or else this system won't work.  I don't know that the
>>>> kernel is in a good position to police that.
>>> What do you mean by policing here?
>>>
>>> Isn't it the hypervisor that determines what virtual input devices will
>>> be available to the guest OS? Therefore, the hypervisor is in a
>>> position to expose exactly one pointer device and exactly one
>>> cursor plane to guest OS which means the guest OS cannot get the
>>> association wrong. If that's the general and expected hypervisor
>>> policy, then there is no need to design explicit device association in
>>> the guest kernel UAPI. If so, I'd like it to be mentioned in the kernel
>>> docs, too.
>> I'm not entirely sure how to fit what you're calling a "pointer" into my
>> mental model of what the hypervisor is doing...
> My definition: A pointer is a pointing input device that requires a
> cursor image to be drawn at the right location for it to be usable.
Right, but normal desktops (and our consoles) expect multiple input 
devices to feed into a single cursor.  So the connection between the 
on-screen cursor and the corresponding input-devices is not clear to me 
when you start talking about multiple pointers, even without any 
hypervisors in the picture.

>
>> For a typical Linux Guest, we currently expose 3+ virtual mouse devices,
>> and choose which one to send input to based on what their guest drivers
>> are doing, and what kind of input we got from the client.  We expect the
>> input from all of those to end up in the same user desktop session,
>> which we expect to own all the virtual screens, and that the user the
>> only gets one cursor image from that.
> Why do you need to expose so many pointer devices? Just curious.
For one, we don't know what drivers are actually going to be running in 
the Guest.  If someone configured the Guest to not support the PS/2 
mouse, or didn't have USB support compiled in, we still need to be able 
to send mouse input.  (We actually ran into this for years with some 
Linux distro installers, where they had frozen their installer with some 
weird/older kernel configs and just didn't support our preferred vmmouse 
device.)  So we plug them all in at boot, and then try to pick the one 
that looks the most active.

But we also need to be able to send both absolute/relative events, and 
we had trouble getting Guests to support both of those coming from the 
same virtual mouse device, so if the client changes mouse modes we would 
route those to the appropriate device dynamically.

There's some other quirky situations, like some absolute virtual mice 
not supporting the entire multimon topology correctly or mouse buttons 
not applying properly when things get split across mouse devices, but 
those are less common.

>
> If you do expose multiple mouse (pointer) devices, then guest OS can
> choose to use each of them as a different independent cursor on the
> same desktop. The only thing stopping that is that it's not usually
> useful, so it's not done.
>
> Therefore, what you need to document in the Linux UAPI instead is that
> *all* pointer devices are associated with the *same* cursor plane. That
> forbids the multi-pointer pointer scenario the VM stack cannot handle.
At least all mouse input devices that the hypervisor console is going to 
use to send input to that desktop, yeah.  (You could still have 
non-hypervisor input sources that don't enter the picture, like some 
kind of passthrough/remote device or something.)

>> So I guess I'm not clear on what kind of usermode<=>kernel contract you
>> want here if the kernel isn't what's owning the translation between the
>> mouse input and the cursor position.  The hypervisor awkwardly has to
>> straddle both the input/graphics domain, and we do so by making
>> assumptions about how the user desktop is going to behave.
> Correct. In order to reduce that awkwardness, I encourage you to write
> down the expectations and requirements in this new Linux UAPI (the KMS
> cursor place hotspot property). Then you can be much more confident on
> how a random Linux desktop will behave.
>
> It will also help the reviewers here to understand what the new UAPI is
> and how it is supposed to work.

The cursor hotspot is I think fairly straightforward, as far as what 
that means for how hypervisor clients are expected to draw the mouse, 
and Zack's working on that part.

My point was that how the hypervisor then sends input is sort of outside 
the scope of the graphics portion here, and I think probably outside the 
current Linux UAPI entirely (unless there's some other input/topology 
system somewhere else I'm not familiar with).

> However, in your case, the userspace (Wayland or X11 display server) is
> not free to choose any arbitrary input-cursor association they might
> want. You have a specific expectation that all pointing devices control
> the same pointer. Since the hotspot property is exclusive to your use
> case, I think it make sense to write down the expectations with the
> hotspot property. Guest userspace has to explicitly program for the
> hotspot property anyway, so it can also take care of your requirements.
I see your point, and I can see the value in documenting something to 
that effect, if only because it's /useful/ for userspaces trying to work 
with this.  (And the only way anyone is using this today.)

But I'm still a little fuzzy on what portion of that is within the Linux 
UAPI scope for the hotspot...

It seems like it might be more useful to restrict the scope of the Linux 
UAPI contract for how the hotspot property works to just how userspace 
can expect the hypervisors to display it on screen, and not try to tie 
in any expectations for how mouse input is going to work.

Like, VMware is using virtual mouse devices here, but another hypervisor 
might have no kernel mouse device and instead inject input entirely 
through a userspace daemon?  So even trying to express the input part of 
the contract in terms of kernel input devices I'm not sure makes sense.

I guess my fear is that I don't want to lock this down in a way that 
excludes some well-meaning hypervisor that implements the graphics part 
correctly but does something just weird enough on the input side to not 
be considered compliant.

So I guess I would vote for trying to include something to that effect 
as context or explanation, but not try to strictly define how that works?

--Michael Banack

Re: [PATCH v4 2/8] drm/atomic: Add support for mouse hotspots

[Pekka Paalanen]

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

On Fri, 7 Jul 2023 13:54:21 -0700
Michael Banack <banackm@vmware.com> wrote:

> On 7/7/23 01:38, Pekka Paalanen wrote:

...

> >>>>>>> The question of which input device corresponds to which cursor plane
> >>>>>>> might be good to answer too. I presume the VM runner is configured to
> >>>>>>> expose exactly one of each, so there can be only one association?  
> >>>>>> As far as I know, all of the VM consoles are written as though they
> >>>>>> taking the place of what would the the physical monitors and input
> >>>>>> devices on a native machine.  So they assume that there is one user,
> >>>>>> sitting in front of one console, and all monitors/input devices are
> >>>>>> being used by that user.  
> >>>>> Ok, but having a single user does not mean that there cannot be
> >>>>> multiple independent pointers, especially on Wayland. The same with
> >>>>> keyboards.  
> >>>> True, and if the userspace is doing anything complicated here, the
> >>>> hypervisor has to be responsible for ensuring that whatever it's doing
> >>>> works with that, or else this system won't work.  I don't know that the
> >>>> kernel is in a good position to police that.  
> >>> What do you mean by policing here?
> >>>
> >>> Isn't it the hypervisor that determines what virtual input devices will
> >>> be available to the guest OS? Therefore, the hypervisor is in a
> >>> position to expose exactly one pointer device and exactly one
> >>> cursor plane to guest OS which means the guest OS cannot get the
> >>> association wrong. If that's the general and expected hypervisor
> >>> policy, then there is no need to design explicit device association in
> >>> the guest kernel UAPI. If so, I'd like it to be mentioned in the kernel
> >>> docs, too.  
> >> I'm not entirely sure how to fit what you're calling a "pointer" into my
> >> mental model of what the hypervisor is doing...  
> > My definition: A pointer is a pointing input device that requires a
> > cursor image to be drawn at the right location for it to be usable.  
> Right, but normal desktops (and our consoles) expect multiple input 
> devices to feed into a single cursor.  So the connection between the 
> on-screen cursor and the corresponding input-devices is not clear to me 
> when you start talking about multiple pointers, even without any 
> hypervisors in the picture.

The connection is simple: there is an independent on-screen cursor for
each logical pointer. How that cursor is drawn is irrelevant to the end
user, and Wayland compositors (a type of a display server) will use any
means necessary to draw it.

Each logical pointer has one cursor that is independent from all other
logical pointers. Each logical pointer can have any number of input
devices controlling it. The assignments are decided by the userspace
and implemented in a display (window system) server.

This has been ingrained into the fundamental design of Wayland, even if
the feature is rarely used in practise. The window system may expose
multiple independent pointers to applications, and each pointer may also
interact with the same window simultaneously. This naturally leads to
the question "which cursor goes with which input device?", and the
kernel, or anything below it, does not know that if there are multiple
possibilities.


> >> So I guess I'm not clear on what kind of usermode<=>kernel contract you
> >> want here if the kernel isn't what's owning the translation between the
> >> mouse input and the cursor position.  The hypervisor awkwardly has to
> >> straddle both the input/graphics domain, and we do so by making
> >> assumptions about how the user desktop is going to behave.  
> > Correct. In order to reduce that awkwardness, I encourage you to write
> > down the expectations and requirements in this new Linux UAPI (the KMS
> > cursor place hotspot property). Then you can be much more confident on
> > how a random Linux desktop will behave.
> >
> > It will also help the reviewers here to understand what the new UAPI is
> > and how it is supposed to work.  
> 
> The cursor hotspot is I think fairly straightforward, as far as what 
> that means for how hypervisor clients are expected to draw the mouse, 
> and Zack's working on that part.
> 
> My point was that how the hypervisor then sends input is sort of outside 
> the scope of the graphics portion here, and I think probably outside the 
> current Linux UAPI entirely (unless there's some other input/topology 
> system somewhere else I'm not familiar with).

I would not say that the hotspot property is in any way obvious. I've
spent my whole Wayland and compositor developer career of over 10 years
never seeing the need of the kernel knowing the hotspot myself, because
I never use VMWare like tools.

You cannot describe why hotspot property is needed or how it works
without explaining the input side. The hotspot property is actually
really weird, because its existence requires combining the input system
with the graphics system for it to make sense. The input system used to
be out of scope indeed, but this addition forces it in scope. This is
the first time that I know of when the kernel or components below the
kernel needs to know, hence there is no existing infrastructure in
Linux to record that topology or anything.

(Sidetrack: for ultra-fast displays, say 1000 Hz refresh for seamless
hand-eye coordination, it could make very much sense for userspace to
off-load arbitrary KMS plane positioning through the kernel into
hardware in general, tying the plane position to some input device
position temporarily. It might make some sense even with today's
consumer hardware. So I don't think the concept is fundamentally
limited to paravirtualized environments. But going there would need a
lot more work than I am willing to suggest to VMWare to tackle to just
make their own products better.)

> > However, in your case, the userspace (Wayland or X11 display server) is
> > not free to choose any arbitrary input-cursor association they might
> > want. You have a specific expectation that all pointing devices control
> > the same pointer. Since the hotspot property is exclusive to your use
> > case, I think it make sense to write down the expectations with the
> > hotspot property. Guest userspace has to explicitly program for the
> > hotspot property anyway, so it can also take care of your requirements.  
> I see your point, and I can see the value in documenting something to 
> that effect, if only because it's /useful/ for userspaces trying to work 
> with this.  (And the only way anyone is using this today.)
> 
> But I'm still a little fuzzy on what portion of that is within the Linux 
> UAPI scope for the hotspot...
> 
> It seems like it might be more useful to restrict the scope of the Linux 
> UAPI contract for how the hotspot property works to just how userspace 
> can expect the hypervisors to display it on screen, and not try to tie 
> in any expectations for how mouse input is going to work.
> 
> Like, VMware is using virtual mouse devices here, but another hypervisor 
> might have no kernel mouse device and instead inject input entirely 
> through a userspace daemon?  So even trying to express the input part of 
> the contract in terms of kernel input devices I'm not sure makes sense.
> 
> I guess my fear is that I don't want to lock this down in a way that 
> excludes some well-meaning hypervisor that implements the graphics part 
> correctly but does something just weird enough on the input side to not 
> be considered compliant.
> 
> So I guess I would vote for trying to include something to that effect 
> as context or explanation, but not try to strictly define how that works?

Yes, exactly.


Thanks,
pq

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

Re: [PATCH v4 2/8] drm/atomic: Add support for mouse hotspots

[Michael Banack]

On 7/10/23 01:17, Pekka Paalanen wrote:
> On Fri, 7 Jul 2023 13:54:21 -0700
> Michael Banack <banackm@vmware.com> wrote:
>
>> On 7/7/23 01:38, Pekka Paalanen wrote:
> ...
>
>>>>>>>>> The question of which input device corresponds to which cursor plane
>>>>>>>>> might be good to answer too. I presume the VM runner is configured to
>>>>>>>>> expose exactly one of each, so there can be only one association?
>>>>>>>> As far as I know, all of the VM consoles are written as though they
>>>>>>>> taking the place of what would the the physical monitors and input
>>>>>>>> devices on a native machine.  So they assume that there is one user,
>>>>>>>> sitting in front of one console, and all monitors/input devices are
>>>>>>>> being used by that user.
>>>>>>> Ok, but having a single user does not mean that there cannot be
>>>>>>> multiple independent pointers, especially on Wayland. The same with
>>>>>>> keyboards.
>>>>>> True, and if the userspace is doing anything complicated here, the
>>>>>> hypervisor has to be responsible for ensuring that whatever it's doing
>>>>>> works with that, or else this system won't work.  I don't know that the
>>>>>> kernel is in a good position to police that.
>>>>> What do you mean by policing here?
>>>>>
>>>>> Isn't it the hypervisor that determines what virtual input devices will
>>>>> be available to the guest OS? Therefore, the hypervisor is in a
>>>>> position to expose exactly one pointer device and exactly one
>>>>> cursor plane to guest OS which means the guest OS cannot get the
>>>>> association wrong. If that's the general and expected hypervisor
>>>>> policy, then there is no need to design explicit device association in
>>>>> the guest kernel UAPI. If so, I'd like it to be mentioned in the kernel
>>>>> docs, too.
>>>> I'm not entirely sure how to fit what you're calling a "pointer" into my
>>>> mental model of what the hypervisor is doing...
>>> My definition: A pointer is a pointing input device that requires a
>>> cursor image to be drawn at the right location for it to be usable.
>> Right, but normal desktops (and our consoles) expect multiple input
>> devices to feed into a single cursor.  So the connection between the
>> on-screen cursor and the corresponding input-devices is not clear to me
>> when you start talking about multiple pointers, even without any
>> hypervisors in the picture.
> The connection is simple: there is an independent on-screen cursor for
> each logical pointer. How that cursor is drawn is irrelevant to the end
> user, and Wayland compositors (a type of a display server) will use any
> means necessary to draw it.
>
> Each logical pointer has one cursor that is independent from all other
> logical pointers. Each logical pointer can have any number of input
> devices controlling it. The assignments are decided by the userspace
> and implemented in a display (window system) server.
>
> This has been ingrained into the fundamental design of Wayland, even if
> the feature is rarely used in practise. The window system may expose
> multiple independent pointers to applications, and each pointer may also
> interact with the same window simultaneously. This naturally leads to
> the question "which cursor goes with which input device?", and the
> kernel, or anything below it, does not know that if there are multiple
> possibilities.

Right, but the whole notion of a "pointer" is also not a kernel concept 
as far as I know?  The kernel only knows that it's requested to display 
a particular set of screen contents on an particular CRTC output.  What 
we're asking for here is for the new HOTSPOT property to allow the final 
desired screen contents to have some opt-in flexibility on how it gets 
displayed to meet the needs of the para-virtualized drivers and their 
use of cursors.

I think trying to extend that beyond the display side to include the 
mouse/input semantics when those are not already part of the kernel 
interface isn't going to work well.

>
>
>>>> So I guess I'm not clear on what kind of usermode<=>kernel contract you
>>>> want here if the kernel isn't what's owning the translation between the
>>>> mouse input and the cursor position.  The hypervisor awkwardly has to
>>>> straddle both the input/graphics domain, and we do so by making
>>>> assumptions about how the user desktop is going to behave.
>>> Correct. In order to reduce that awkwardness, I encourage you to write
>>> down the expectations and requirements in this new Linux UAPI (the KMS
>>> cursor place hotspot property). Then you can be much more confident on
>>> how a random Linux desktop will behave.
>>>
>>> It will also help the reviewers here to understand what the new UAPI is
>>> and how it is supposed to work.
>> The cursor hotspot is I think fairly straightforward, as far as what
>> that means for how hypervisor clients are expected to draw the mouse,
>> and Zack's working on that part.
>>
>> My point was that how the hypervisor then sends input is sort of outside
>> the scope of the graphics portion here, and I think probably outside the
>> current Linux UAPI entirely (unless there's some other input/topology
>> system somewhere else I'm not familiar with).
> I would not say that the hotspot property is in any way obvious. I've
> spent my whole Wayland and compositor developer career of over 10 years
> never seeing the need of the kernel knowing the hotspot myself, because
> I never use VMWare like tools.
Sorry, I meant straight-forward as in well-defined, not as in obvious.  
It should definitely be documented.

We can just clearly specify the graphics contract here in a way that is 
trickier for the mouse/input parts.

>
> You cannot describe why hotspot property is needed or how it works
> without explaining the input side. The hotspot property is actually
> really weird, because its existence requires combining the input system
> with the graphics system for it to make sense. The input system used to
> be out of scope indeed, but this addition forces it in scope. This is
> the first time that I know of when the kernel or components below the
> kernel needs to know, hence there is no existing infrastructure in
> Linux to record that topology or anything.

I think the problem here is going to be that each of the userspaces and 
para-virtual drivers handle this differently...

They all have their own mechanisms to pass the topology/input-mapping up 
to the hypervisor (either custom IOCTLs on their graphics driver, or 
else backdoor-style from userspace), and how that works at the userspace 
layer (ie X11/Wayland) is differently.

>
> (Sidetrack: for ultra-fast displays, say 1000 Hz refresh for seamless
> hand-eye coordination, it could make very much sense for userspace to
> off-load arbitrary KMS plane positioning through the kernel into
> hardware in general, tying the plane position to some input device
> position temporarily. It might make some sense even with today's
> consumer hardware. So I don't think the concept is fundamentally
> limited to paravirtualized environments. But going there would need a
> lot more work than I am willing to suggest to VMWare to tackle to just
> make their own products better.)

Sure, but to do that we would need to standardize how the input mapping 
happens, and create standard kernel interfaces for the topology 
information, and relative mouse acceleration curves, etc. That's the 
piece that I'm saying isn't standardized enough now to be able to spell 
that out in documentation now, because there is no standard way of 
handling all that.

As a hand-waiving, "this is kind of how it works", it's useful to 
document.  But if you're pushing for a hard "this is how the Linux 
kernel handles mouse input", then I don't think any of these components 
are ready to commit to the mouse/input handling as a Linux UAPI.

>> So I guess I would vote for trying to include something to that effect
>> as context or explanation, but not try to strictly define how that works?
> Yes, exactly.

Okay, if we can keep the mouse/input stuff on the fuzzy side then I 
think we're on the same page.

Thanks!
--Michael Banack

Re: [PATCH v4 2/8] drm/atomic: Add support for mouse hotspots

[Pekka Paalanen]

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

On Mon, 10 Jul 2023 10:46:56 -0700
Michael Banack <banackm@vmware.com> wrote:

> On 7/10/23 01:17, Pekka Paalanen wrote:
> > On Fri, 7 Jul 2023 13:54:21 -0700
> > Michael Banack <banackm@vmware.com> wrote:

...

> >> So I guess I would vote for trying to include something to that effect
> >> as context or explanation, but not try to strictly define how that works?  
> > Yes, exactly.  
> 
> Okay, if we can keep the mouse/input stuff on the fuzzy side then I 
> think we're on the same page.

Very much of the fuzzy side, yes! All I am saying is that one cannot
explain the hotspot property without saying anything about it being
connected with input devices in some way. The very key I want to see
documented is that all cursor-needing pointing input devices and all
KMS cursor planes exposed to the guest OS are meant to be associated
with the same single conceptual pointer. That is all.


Thanks,
pq

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

Re: [PATCH v4 2/8] drm/atomic: Add support for mouse hotspots

[Javier Martinez Canillas]

Pekka Paalanen <ppaalanen@gmail.com> writes:

Hello folks,

> On Mon, 10 Jul 2023 10:46:56 -0700
> Michael Banack <banackm@vmware.com> wrote:
>
>> On 7/10/23 01:17, Pekka Paalanen wrote:
>> > On Fri, 7 Jul 2023 13:54:21 -0700
>> > Michael Banack <banackm@vmware.com> wrote:
>
> ...
>
>> >> So I guess I would vote for trying to include something to that effect
>> >> as context or explanation, but not try to strictly define how that works?  
>> > Yes, exactly.  
>> 
>> Okay, if we can keep the mouse/input stuff on the fuzzy side then I 
>> think we're on the same page.
>
> Very much of the fuzzy side, yes! All I am saying is that one cannot
> explain the hotspot property without saying anything about it being
> connected with input devices in some way. The very key I want to see
> documented is that all cursor-needing pointing input devices and all
> KMS cursor planes exposed to the guest OS are meant to be associated
> with the same single conceptual pointer. That is all.
>

So if I understand correctly Pekka doesn't have any issues with the actual
implementation and is just asking for better documentation ?

How can we move this series forward? Maybe we can land this set and add an
explanation / more verbose uAPI documentation as a follow-up patches ?

Or do you think that the everything must be merged together and another
revision be posted ? The sooner we could land this, the sooner that should
be able to drop virtio-gpu from the mutter atomic deny list, and be able
to use the damage handling work that has been done across the virt stack.

>
> Thanks,
> pq

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v4 2/8] drm/atomic: Add support for mouse hotspots

[Javier Martinez Canillas]

Javier Martinez Canillas <javierm@redhat.com> writes:

> Pekka Paalanen <ppaalanen@gmail.com> writes:
>
> Hello folks,
>
>> On Mon, 10 Jul 2023 10:46:56 -0700
>> Michael Banack <banackm@vmware.com> wrote:
>>
>>> On 7/10/23 01:17, Pekka Paalanen wrote:
>>> > On Fri, 7 Jul 2023 13:54:21 -0700
>>> > Michael Banack <banackm@vmware.com> wrote:
>>
>> ...
>>
>>> >> So I guess I would vote for trying to include something to that effect
>>> >> as context or explanation, but not try to strictly define how that works?  
>>> > Yes, exactly.  
>>> 
>>> Okay, if we can keep the mouse/input stuff on the fuzzy side then I 
>>> think we're on the same page.
>>
>> Very much of the fuzzy side, yes! All I am saying is that one cannot
>> explain the hotspot property without saying anything about it being
>> connected with input devices in some way. The very key I want to see
>> documented is that all cursor-needing pointing input devices and all
>> KMS cursor planes exposed to the guest OS are meant to be associated
>> with the same single conceptual pointer. That is all.
>>
>
> So if I understand correctly Pekka doesn't have any issues with the actual
> implementation and is just asking for better documentation ?
>
> How can we move this series forward? Maybe we can land this set and add an
> explanation / more verbose uAPI documentation as a follow-up patches ?
>

Maxime pointed out to me that is not only about more verbose uAPI but that
patch 2/8 doesn't have uAPI docs for the new HOTSPOT_{X,Y} properties, so
at the very least this should be added.

Zack said that would post a v5, we will have to wait for that.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v4 2/8] drm/atomic: Add support for mouse hotspots

[Pekka Paalanen]

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

On Tue, 18 Jul 2023 10:41:32 +0200
Javier Martinez Canillas <javierm@redhat.com> wrote:

> Pekka Paalanen <ppaalanen@gmail.com> writes:
> 
> Hello folks,
> 
> > On Mon, 10 Jul 2023 10:46:56 -0700
> > Michael Banack <banackm@vmware.com> wrote:
> >  
> >> On 7/10/23 01:17, Pekka Paalanen wrote:  
> >> > On Fri, 7 Jul 2023 13:54:21 -0700
> >> > Michael Banack <banackm@vmware.com> wrote:  
> >
> > ...
> >  
> >> >> So I guess I would vote for trying to include something to that effect
> >> >> as context or explanation, but not try to strictly define how that works?    
> >> > Yes, exactly.    
> >> 
> >> Okay, if we can keep the mouse/input stuff on the fuzzy side then I 
> >> think we're on the same page.  
> >
> > Very much of the fuzzy side, yes! All I am saying is that one cannot
> > explain the hotspot property without saying anything about it being
> > connected with input devices in some way. The very key I want to see
> > documented is that all cursor-needing pointing input devices and all
> > KMS cursor planes exposed to the guest OS are meant to be associated
> > with the same single conceptual pointer. That is all.
> >  
> 
> So if I understand correctly Pekka doesn't have any issues with the actual
> implementation and is just asking for better documentation ?

Correct!


Thanks,
pq

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