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

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

[Zack Rusin]

From: Zack Rusin <zackr@vmware.com>

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_SUPPORTS_VIRTUAL_CURSOR_PLANE

 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               | 59 ++++++++++++++++++++++-
 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                     | 10 ++++
 include/drm/drm_file.h                    | 12 +++++
 include/drm/drm_framebuffer.h             | 12 -----
 include/drm/drm_plane.h                   | 15 ++++++
 include/uapi/drm/drm.h                    | 17 +++++++
 17 files changed, 173 insertions(+), 39 deletions(-)

-- 
2.34.1

[PATCH v2 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
---
 drivers/gpu/drm/drm_plane.c          | 11 +++++++++++
 drivers/gpu/drm/qxl/qxl_drv.c        |  2 +-
 drivers/gpu/drm/vboxvideo/vbox_drv.c |  2 +-
 drivers/gpu/drm/virtio/virtgpu_drv.c |  3 ++-
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c  |  2 +-
 include/drm/drm_drv.h                | 10 ++++++++++
 include/drm/drm_file.h               | 12 ++++++++++++
 7 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 726f2f163c26..e1e2a65c7119 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -667,6 +667,17 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data,
 		    !file_priv->universal_planes)
 			continue;
 
+		/*
+		 * Unless userspace supports virtual cursor plane
+		 * then if we're running on virtual driver do not
+		 * advertise cursor planes because they'll be broken
+		 */
+		if (plane->type == DRM_PLANE_TYPE_CURSOR &&
+		    drm_core_check_feature(dev, DRIVER_VIRTUAL)	&&
+		    file_priv->atomic &&
+		    !file_priv->supports_virtual_cursor_plane)
+			continue;
+
 		if (drm_lease_held(file_priv, plane->base.id)) {
 			if (count < plane_resp->count_planes &&
 			    put_user(plane->base.id, plane_ptr + count))
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index 1cb6f0c224bb..0e4212e05caa 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.c
+++ b/drivers/gpu/drm/qxl/qxl_drv.c
@@ -281,7 +281,7 @@ static const struct drm_ioctl_desc qxl_ioctls[] = {
 };
 
 static struct drm_driver qxl_driver = {
-	.driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
+	.driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_VIRTUAL,
 
 	.dumb_create = qxl_mode_dumb_create,
 	.dumb_map_offset = drm_gem_ttm_dumb_map_offset,
diff --git a/drivers/gpu/drm/vboxvideo/vbox_drv.c b/drivers/gpu/drm/vboxvideo/vbox_drv.c
index f4f2bd79a7cb..84e75bcc3384 100644
--- a/drivers/gpu/drm/vboxvideo/vbox_drv.c
+++ b/drivers/gpu/drm/vboxvideo/vbox_drv.c
@@ -176,7 +176,7 @@ DEFINE_DRM_GEM_FOPS(vbox_fops);
 
 static const struct drm_driver driver = {
 	.driver_features =
-	    DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
+	    DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC | DRIVER_VIRTUAL,
 
 	.lastclose = drm_fb_helper_lastclose,
 
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
index 5f25a8d15464..3c5bb006159a 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -198,7 +198,8 @@ MODULE_AUTHOR("Alon Levy");
 DEFINE_DRM_GEM_FOPS(virtio_gpu_driver_fops);
 
 static const struct drm_driver driver = {
-	.driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER | DRIVER_ATOMIC,
+	.driver_features =
+		DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER | DRIVER_ATOMIC | DRIVER_VIRTUAL,
 	.open = virtio_gpu_driver_open,
 	.postclose = virtio_gpu_driver_postclose,
 
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index 01a5b47e95f9..712f6ad0b014 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -1581,7 +1581,7 @@ static const struct file_operations vmwgfx_driver_fops = {
 
 static const struct drm_driver driver = {
 	.driver_features =
-	DRIVER_MODESET | DRIVER_RENDER | DRIVER_ATOMIC | DRIVER_GEM,
+	DRIVER_MODESET | DRIVER_RENDER | DRIVER_ATOMIC | DRIVER_GEM | DRIVER_VIRTUAL,
 	.ioctls = vmw_ioctls,
 	.num_ioctls = ARRAY_SIZE(vmw_ioctls),
 	.master_set = vmw_master_set,
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index f6159acb8856..c4cd7fc350d9 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -94,6 +94,16 @@ enum drm_driver_feature {
 	 * synchronization of command submission.
 	 */
 	DRIVER_SYNCOBJ_TIMELINE         = BIT(6),
+	/**
+	 * @DRIVER_VIRTUAL:
+	 *
+	 * Driver is running on top of virtual hardware. The most significant
+	 * implication of this is a requirement of special handling of the
+	 * cursor plane (e.g. cursor plane has to actually track the mouse
+	 * cursor and the clients are required to set hotspot in order for
+	 * the cursor planes to work correctly).
+	 */
+	DRIVER_VIRTUAL                  = BIT(7),
 
 	/* IMPORTANT: Below are all the legacy flags, add new ones above. */
 
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index e0a73a1e2df7..3e5c36891161 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -223,6 +223,18 @@ struct drm_file {
 	 */
 	bool is_master;
 
+	/**
+	 * @supports_virtual_cursor_plane:
+	 *
+	 * This client is capable of handling the cursor plane with the
+	 * restrictions imposed on it by the virtualized drivers.
+	 *
+	 * The implies that the cursor plane has to behave like a cursor
+	 * i.e. track cursor movement. It also requires setting of the
+	 * hotspot properties by the client on the cursor plane.
+	 */
+	bool supports_virtual_cursor_plane;
+
 	/**
 	 * @master:
 	 *
-- 
2.34.1

[PATCH v2 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>
---
 drivers/gpu/drm/drm_atomic_state_helper.c | 14 +++++++
 drivers/gpu/drm/drm_atomic_uapi.c         | 20 ++++++++++
 drivers/gpu/drm/drm_plane.c               | 47 +++++++++++++++++++++++
 include/drm/drm_plane.h                   | 15 ++++++++
 4 files changed, 96 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index bf31b9d92094..9c4fda0b35e8 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 79730fa1dd8e..6132540c97a9 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -575,6 +575,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",
@@ -635,6 +651,10 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
 		*val = state->scaling_filter;
 	} else if (plane->funcs->atomic_get_property) {
 		return plane->funcs->atomic_get_property(plane, state, property, val);
+	} else if (property == plane->hotspot_x_property) {
+		*val = state->hotspot_x;
+	} else if (property == plane->hotspot_y_property) {
+		*val = state->hotspot_y;
 	} else {
 		return -EINVAL;
 	}
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index e1e2a65c7119..0a6a1b5adf82 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -343,6 +343,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_VIRTUAL) &&
+	    type == DRM_PLANE_TYPE_CURSOR) {
+		drm_plane_create_hotspot_properties(plane);
+	}
 
 	if (format_modifier_count)
 		create_in_format_blob(dev, plane);
@@ -1054,6 +1058,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;
 		}
@@ -1582,3 +1591,41 @@ int drm_plane_create_scaling_filter_property(struct drm_plane *plane,
 	return 0;
 }
 EXPORT_SYMBOL(drm_plane_create_scaling_filter_property);
+
+/**
+ * drm_plane_create_hotspot_properties - creates the mouse hotspot
+ * properties and attaches them to the given cursor plane
+ *
+ * @plane: drm cursor plane
+ *
+ * This function lets driver to enable the mouse hotspot property on a given
+ * cursor plane.
+ *
+ * RETURNS:
+ * Zero for success or -errno
+ */
+int drm_plane_create_hotspot_properties(struct drm_plane *plane)
+{
+	struct drm_property *prop_x;
+	struct drm_property *prop_y;
+
+	prop_x = drm_property_create_signed_range(plane->dev, 0, "HOTSPOT_X",
+						  INT_MIN, INT_MAX);
+	if (IS_ERR(prop_x))
+		return PTR_ERR(prop_x);
+
+	prop_y = drm_property_create_signed_range(plane->dev, 0, "HOTSPOT_Y",
+						  INT_MIN, INT_MAX);
+	if (IS_ERR(prop_y)) {
+		drm_property_destroy(plane->dev, prop_x);
+		return PTR_ERR(prop_y);
+	}
+
+	drm_object_attach_property(&plane->base, prop_x, 0);
+	drm_object_attach_property(&plane->base, prop_y, 0);
+	plane->hotspot_x_property = prop_x;
+	plane->hotspot_y_property = prop_y;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_plane_create_hotspot_properties);
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 89ea54652e87..09d3e3791e67 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -116,6 +116,10 @@ struct drm_plane_state {
 	/** @src_h: height of visible portion of plane (in 16.16) */
 	uint32_t src_h, src_w;
 
+	/** @hotspot_x: x offset to mouse cursor hotspot */
+	/** @hotspot_y: y offset to mouse cursor hotspot */
+	int32_t hotspot_x, hotspot_y;
+
 	/**
 	 * @alpha:
 	 * Opacity of the plane with 0 as completely transparent and 0xffff as
@@ -748,6 +752,16 @@ struct drm_plane {
 	 * scaling.
 	 */
 	struct drm_property *scaling_filter_property;
+
+	/**
+	 * @hotspot_x_property: property to set mouse hotspot x offset.
+	 */
+	struct drm_property *hotspot_x_property;
+
+	/**
+	 * @hotspot_y_property: property to set mouse hotspot y offset.
+	 */
+	struct drm_property *hotspot_y_property;
 };
 
 #define obj_to_plane(x) container_of(x, struct drm_plane, base)
@@ -907,5 +921,6 @@ drm_plane_get_damage_clips(const struct drm_plane_state *state);
 
 int drm_plane_create_scaling_filter_property(struct drm_plane *plane,
 					     unsigned int supported_filters);
+int drm_plane_create_hotspot_properties(struct drm_plane *plane);
 
 #endif
-- 
2.34.1

[PATCH v2 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: Martin Krastev <krastevm@vmware.com>
Cc: Maaz Mombasawala <mombasawalam@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 ff2f735bbe7a..3d3f73109199 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -652,13 +652,8 @@ vmw_du_cursor_plane_atomic_update(struct drm_plane *plane,
 	struct vmw_plane_state *vps = vmw_plane_state_to_vps(new_state);
 	s32 hotspot_x, hotspot_y;
 
-	hotspot_x = du->hotspot_x;
-	hotspot_y = du->hotspot_y;
-
-	if (new_state->fb) {
-		hotspot_x += new_state->fb->hot_x;
-		hotspot_y += new_state->fb->hot_y;
-	}
+	hotspot_x = du->hotspot_x + new_state->hotspot_x;
+	hotspot_y = du->hotspot_y + new_state->hotspot_y;
 
 	du->cursor_surface = vps->surf;
 	du->cursor_bo = vps->bo;
-- 
2.34.1

[PATCH v2 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>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: virtualization@lists.linux-foundation.org
Cc: spice-devel@lists.freedesktop.org
---
 drivers/gpu/drm/qxl/qxl_display.c | 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 2e8949863d6b..f5a90be84e93 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.34.1

[PATCH v2 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>
---
 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 fa0d73ce07bc..ca3134adb104 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.34.1

[PATCH v2 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>
Cc: David Airlie <airlied@linux.ie>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Gurchetan Singh <gurchetansingh@chromium.org>
Cc: Chia-I Wu <olvaffe@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: virtualization@lists.linux-foundation.org
---
 drivers/gpu/drm/virtio/virtgpu_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 6d3cc9e238a4..21c8adf51c6c 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -331,16 +331,16 @@ static void virtio_gpu_cursor_plane_update(struct drm_plane *plane,
 		DRM_DEBUG("update, handle %d, pos +%d+%d, hot %d,%d\n", handle,
 			  plane->state->crtc_x,
 			  plane->state->crtc_y,
-			  plane->state->fb ? plane->state->fb->hot_x : 0,
-			  plane->state->fb ? plane->state->fb->hot_y : 0);
+			  plane->state->hotspot_x,
+			  plane->state->hotspot_y);
 		output->cursor.hdr.type =
 			cpu_to_le32(VIRTIO_GPU_CMD_UPDATE_CURSOR);
 		output->cursor.resource_id = cpu_to_le32(handle);
 		if (plane->state->fb) {
 			output->cursor.hot_x =
-				cpu_to_le32(plane->state->fb->hot_x);
+				cpu_to_le32(plane->state->hotspot_x);
 			output->cursor.hot_y =
-				cpu_to_le32(plane->state->fb->hot_y);
+				cpu_to_le32(plane->state->hotspot_y);
 		} else {
 			output->cursor.hot_x = cpu_to_le32(0);
 			output->cursor.hot_y = cpu_to_le32(0);
-- 
2.34.1

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

[Zack Rusin]

From: Zack Rusin <zackr@vmware.com>

Atomic modesetting support mouse cursor offsets via the hotspot
properties that are creates on cursor planes. All drivers which
support hotspot are atomic and the legacy code has been implemented
in terms of the atomic properties as well.

Due to the above the lagacy cursor hotspot code is no longer used or
needed and can be removed.

Signed-off-by: Zack Rusin <zackr@vmware.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 drivers/gpu/drm/drm_plane.c   |  3 ---
 include/drm/drm_framebuffer.h | 12 ------------
 2 files changed, 15 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 0a6a1b5adf82..8dc5b2818d93 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1056,9 +1056,6 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc,
 				return PTR_ERR(fb);
 			}
 
-			fb->hot_x = req->hot_x;
-			fb->hot_y = req->hot_y;
-
 			if (plane->hotspot_x_property && plane->state)
 				plane->state->hotspot_x = req->hot_x;
 			if (plane->hotspot_y_property && plane->state)
diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
index f67c5b7bcb68..c306ae2e2d47 100644
--- a/include/drm/drm_framebuffer.h
+++ b/include/drm/drm_framebuffer.h
@@ -188,18 +188,6 @@ struct drm_framebuffer {
 	 * DRM_MODE_FB_MODIFIERS.
 	 */
 	int flags;
-	/**
-	 * @hot_x: X coordinate of the cursor hotspot. Used by the legacy cursor
-	 * IOCTL when the driver supports cursor through a DRM_PLANE_TYPE_CURSOR
-	 * universal plane.
-	 */
-	int hot_x;
-	/**
-	 * @hot_y: Y coordinate of the cursor hotspot. Used by the legacy cursor
-	 * IOCTL when the driver supports cursor through a DRM_PLANE_TYPE_CURSOR
-	 * universal plane.
-	 */
-	int hot_y;
 	/**
 	 * @filp_head: Placed on &drm_file.fbs, protected by &drm_file.fbs_lock.
 	 */
-- 
2.34.1

[PATCH v2 8/8] drm: Introduce DRM_CLIENT_CAP_SUPPORTS_VIRTUAL_CURSOR_PLANE

[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_SUPPORTS_VIRTUAL_CURSOR_PLANE 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
---
 drivers/gpu/drm/drm_ioctl.c |  9 +++++++++
 include/uapi/drm/drm.h      | 17 +++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 8faad23dc1d8..f10590b061d7 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -362,6 +362,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_SUPPORTS_VIRTUAL_CURSOR_PLANE:
+		if (!drm_core_check_feature(dev, DRIVER_VIRTUAL))
+			return -EOPNOTSUPP;
+		if (!file_priv->atomic)
+			return -EINVAL;
+		if (req->value > 1)
+			return -EINVAL;
+		file_priv->supports_virtual_cursor_plane = req->value;
+		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 642808520d92..f24a1941abca 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -836,6 +836,23 @@ struct drm_get_cap {
  */
 #define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS	5
 
+/**
+ * DRM_CLIENT_CAP_SUPPORTS_VIRTUAL_CURSOR_PLANE
+ *
+ * If set to 1, the DRM core will expose cursor plane to be used for
+ * virtualized mouse cursor, without virtualized drivers will not expose
+ * the cursor plane in atomic contexts. Cursor planes in virtualized
+ * drivers come with some additional restrictions and are not truly
+ * universal, e.g. they need to act like one would expect from a mouse
+ * cursor and have correctly set hotspot properties.
+ * The client must enable &DRM_CLIENT_CAP_ATOMIC first.
+ *
+ * This capability is always supported for atomic-capable virtualized drivers
+ * starting from kernel version 5.21.
+ */
+#define DRM_CLIENT_CAP_SUPPORTS_VIRTUAL_CURSOR_PLANE	6
+
+
 /* DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
 struct drm_set_client_cap {
 	__u64 capability;
-- 
2.34.1

Re: [PATCH v2 0/8] Fix cursor planes with virtualized drivers

[Pekka Paalanen]

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

On Mon, 11 Jul 2022 23:32:38 -0400
Zack Rusin <zack@kde.org> wrote:

> From: Zack Rusin <zackr@vmware.com>
> 
> 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).

Hi Zack,

thank you very much for this revision, I am generally very happy how
this looks.

I read through all the patches, and all the commit messages and code
comments say good things. I'm not really knowledgeable enough to review
the code itself.

Otherwise I would already give my Ack for everything here, but I did
have some questions inspired by patches 5 and 8. See my replies to them.


Thanks,
pq


> 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_SUPPORTS_VIRTUAL_CURSOR_PLANE
> 
>  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               | 59 ++++++++++++++++++++++-
>  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                     | 10 ++++
>  include/drm/drm_file.h                    | 12 +++++
>  include/drm/drm_framebuffer.h             | 12 -----
>  include/drm/drm_plane.h                   | 15 ++++++
>  include/uapi/drm/drm.h                    | 17 +++++++
>  17 files changed, 173 insertions(+), 39 deletions(-)
> 


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

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

[Pekka Paalanen]

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

On Mon, 11 Jul 2022 23:32:43 -0400
Zack Rusin <zack@kde.org> wrote:

> 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>
> ---
>  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 fa0d73ce07bc..ca3134adb104 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);

Hi,

this looks like silent clamping of the hotspot coordinates.

Should the DRM core perhaps already ensure that the hotspot must reside
inside the plane (fb) boundaries and reject the atomic (TEST_ONLY)
commit when it's outside?

Or if this restriction is not universal, maybe this driver should
reject invalid hotspots rather than silently mangle them?

Also, if supports_virtual_cursor_plane is false, should there not be
somewhere code that will ignore the values set to the atomic hotspot
properties?

When one KMS client switches to another, the first one being hotspot
aware and the latter not, and both atomic, then the latter KMS client
who doesn't know to drive the hotspot will inherit potentially invalid
hotspot from the previous KMS client. Does something prevent that from
being a problem?

The old KMS client may have left the virtual cursor plane visible, and
the new KMS client doesn't even know the virtual cursor plane exists
because it doesn't set the DRM client cap. Something should probably
ensure the virtual cursor plane gets disabled, right?


Thanks,
pq

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

Re: [PATCH v2 8/8] drm: Introduce DRM_CLIENT_CAP_SUPPORTS_VIRTUAL_CURSOR_PLANE

[Simon Ser]

On Tuesday, July 12th, 2022 at 05:32, Zack Rusin <zack@kde.org> wrote:

> To do that introduce DRM_CLIENT_CAP_SUPPORTS_VIRTUAL_CURSOR_PLANE

Nit: maybe we can name this "DRM_CLIENT_CAP_VIRTUAL_CURSOR_PLANE" or
even "DRM_CLIENT_CAP_VIRTUAL_CURSORS", since "CAP" already implies that
this is a capability that the DRM client supports.

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

[Gerd Hoffmann]

On Mon, Jul 11, 2022 at 11:32:42PM -0400, Zack Rusin wrote:
> 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: Dave Airlie <airlied@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: virtualization@lists.linux-foundation.org
> Cc: spice-devel@lists.freedesktop.org

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

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

[Gerd Hoffmann]

On Mon, Jul 11, 2022 at 11:32:44PM -0400, Zack Rusin wrote:
> 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: David Airlie <airlied@linux.ie>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Gurchetan Singh <gurchetansingh@chromium.org>
> Cc: Chia-I Wu <olvaffe@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: virtualization@lists.linux-foundation.org

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

Re: [PATCH v2 0/8] Fix cursor planes with virtualized drivers

[Simon Ser]

Overall this looks pretty good to me, thanks for working on this!

Acked-by: Simon Ser <contact@emersion.fr>

Re: [PATCH v2 8/8] drm: Introduce DRM_CLIENT_CAP_SUPPORTS_VIRTUAL_CURSOR_PLANE

[Pekka Paalanen]

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

On Mon, 11 Jul 2022 23:32:46 -0400
Zack Rusin <zack@kde.org> wrote:

> 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_SUPPORTS_VIRTUAL_CURSOR_PLANE 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
> ---
>  drivers/gpu/drm/drm_ioctl.c |  9 +++++++++
>  include/uapi/drm/drm.h      | 17 +++++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 8faad23dc1d8..f10590b061d7 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -362,6 +362,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_SUPPORTS_VIRTUAL_CURSOR_PLANE:
> +		if (!drm_core_check_feature(dev, DRIVER_VIRTUAL))
> +			return -EOPNOTSUPP;

Ah!

Is userspace expected to use this EOPNOTSUPP error to tell virtual
cursor planes apart from generic hardware planes that were just tagged
as cursor?

I am thinking about how an atomic KMS client can do both:
- use generic cursor plane for generic content, and
- use a virtual cursor plane correctly

One possibility here is for the atomic KMS client:
- try to set DRM_CLIENT_CAP_SUPPORTS_VIRTUAL_CURSOR_PLANE
  - if it succeeds, all cursor planes it can see are virtual
  - if it fails, all cursor planes it can see are generic

That sounds good to me, because I don't think the same DRM device would
want to expose both kinds of cursor planes.

However, this should be added in the DRM documentation facing userspace.

> +		if (!file_priv->atomic)
> +			return -EINVAL;
> +		if (req->value > 1)
> +			return -EINVAL;
> +		file_priv->supports_virtual_cursor_plane = req->value;
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 642808520d92..f24a1941abca 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -836,6 +836,23 @@ struct drm_get_cap {
>   */
>  #define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS	5
>  
> +/**
> + * DRM_CLIENT_CAP_SUPPORTS_VIRTUAL_CURSOR_PLANE
> + *
> + * If set to 1, the DRM core will expose cursor plane to be used for
> + * virtualized mouse cursor, without virtualized drivers will not expose
> + * the cursor plane in atomic contexts. Cursor planes in virtualized

I would suggest some re-wording here, because at first I read this like:
Without virtualized drivers, setting this cap will hide all cursor
planes from userspace.

It's also a bit unclear on what happens when userspace sets this cap on
a non-virtual driver. From the code I see that it will fail, but that
might be a surprise to userspace developers. I suppose explaining the
virtual vs. generic cursor plane detection will address this.

> + * drivers come with some additional restrictions and are not truly
> + * universal, e.g. they need to act like one would expect from a mouse
> + * cursor and have correctly set hotspot properties.
> + * The client must enable &DRM_CLIENT_CAP_ATOMIC first.
> + *
> + * This capability is always supported for atomic-capable virtualized drivers
> + * starting from kernel version 5.21.
> + */
> +#define DRM_CLIENT_CAP_SUPPORTS_VIRTUAL_CURSOR_PLANE	6
> +
> +
>  /* DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
>  struct drm_set_client_cap {
>  	__u64 capability;

Thanks,
pq

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

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

[Zack Rusin]

On Tue, 2022-07-12 at 10:56 +0300, Pekka Paalanen wrote:
> On Mon, 11 Jul 2022 23:32:43 -0400
> Zack Rusin <zack@kde.org> wrote:
> 
> > 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>
> > ---
> >  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 fa0d73ce07bc..ca3134adb104 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);
> 
> Hi,
> 
> this looks like silent clamping of the hotspot coordinates.
> 
> Should the DRM core perhaps already ensure that the hotspot must reside
> inside the plane (fb) boundaries and reject the atomic (TEST_ONLY)
> commit when it's outside?
> 
> Or if this restriction is not universal, maybe this driver should
> reject invalid hotspots rather than silently mangle them?

TBH, I'm not really sure why vboxvideo is clamping those values. I didn't want to
introduce any regressions by changing it here, but the hotspot code never specified
that the hotspot offsets have to be positive or even within the plane. In a quick
search I couldn't find real world cursors that were doing anything like that though
so I just left it.

> Also, if supports_virtual_cursor_plane is false, should there not be
> somewhere code that will ignore the values set to the atomic hotspot
> properties?

Hmm, good question. I'm not sure if there's a case where that could be possible:
without supports_virtual_cursor we either won't have a cursor plane or the client
won't be atomic and will only be allowed to use the old legacy kms ioctl's, i.e.
drmModeSetCursor2.

> When one KMS client switches to another, the first one being hotspot
> aware and the latter not, and both atomic, then the latter KMS client
> who doesn't know to drive the hotspot will inherit potentially invalid
> hotspot from the previous KMS client. Does something prevent that from
> being a problem?

That switch will result in plane state reset, ending in
__drm_atomic_helper_plane_state_reset which will reset both hotspot properties (set
them to 0).

> The old KMS client may have left the virtual cursor plane visible, and
> the new KMS client doesn't even know the virtual cursor plane exists
> because it doesn't set the DRM client cap. Something should probably
> ensure the virtual cursor plane gets disabled, right?

Hah, that's also a good question. I *think* the same code to above would be ran,
i.e. plane reset which should result in the plane disappearing and the new client
not being able to drive it anymore. At least when running gnome-shell, switching to
weston and then switching to gnome-shell things look ok, but I haven't tried running
such clients at the same time.

z 

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

[Pekka Paalanen]

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

On Wed, 13 Jul 2022 03:35:57 +0000
Zack Rusin <zackr@vmware.com> wrote:

> On Tue, 2022-07-12 at 10:56 +0300, Pekka Paalanen wrote:
> > On Mon, 11 Jul 2022 23:32:43 -0400
> > Zack Rusin <zack@kde.org> wrote:
> >   
> > > 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>
> > > ---
> > >  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 fa0d73ce07bc..ca3134adb104 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);  
> > 
> > Hi,
> > 
> > this looks like silent clamping of the hotspot coordinates.
> > 
> > Should the DRM core perhaps already ensure that the hotspot must reside
> > inside the plane (fb) boundaries and reject the atomic (TEST_ONLY)
> > commit when it's outside?
> > 
> > Or if this restriction is not universal, maybe this driver should
> > reject invalid hotspots rather than silently mangle them?  
> 
> TBH, I'm not really sure why vboxvideo is clamping those values. I didn't want to
> introduce any regressions by changing it here, but the hotspot code never specified
> that the hotspot offsets have to be positive or even within the plane. In a quick
> search I couldn't find real world cursors that were doing anything like that though
> so I just left it.
> 
> > Also, if supports_virtual_cursor_plane is false, should there not be
> > somewhere code that will ignore the values set to the atomic hotspot
> > properties?  
> 
> Hmm, good question. I'm not sure if there's a case where that could be possible:
> without supports_virtual_cursor we either won't have a cursor plane or the client
> won't be atomic and will only be allowed to use the old legacy kms ioctl's, i.e.
> drmModeSetCursor2.
> 
> > When one KMS client switches to another, the first one being hotspot
> > aware and the latter not, and both atomic, then the latter KMS client
> > who doesn't know to drive the hotspot will inherit potentially invalid
> > hotspot from the previous KMS client. Does something prevent that from
> > being a problem?  
> 
> That switch will result in plane state reset, ending in
> __drm_atomic_helper_plane_state_reset which will reset both hotspot properties (set
> them to 0).

It will?

To my knowledge, KMS has never reset anything when one KMS client
switches to the next, so that's new.

What triggers it?

Only if you actually switch to fbdev/fbcon instead of another userspace
KMS client, the fbdev code might reset some things, but not all.

> > The old KMS client may have left the virtual cursor plane visible, and
> > the new KMS client doesn't even know the virtual cursor plane exists
> > because it doesn't set the DRM client cap. Something should probably
> > ensure the virtual cursor plane gets disabled, right?  
> 
> Hah, that's also a good question. I *think* the same code to above would be ran,
> i.e. plane reset which should result in the plane disappearing and the new client
> not being able to drive it anymore. At least when running gnome-shell, switching to
> weston and then switching to gnome-shell things look ok, but I haven't tried running
> such clients at the same time.

That's an interesting experiment, but how is "at the same time"
different from what you tested?

As i mentioned above, if you switch to fbcon in between, then you are
not switching from one userspace KMS client to the next.


Thanks,
pq

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

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

[Zack Rusin]

On Wed, 2022-07-13 at 10:20 +0300, Pekka Paalanen wrote:
> On Wed, 13 Jul 2022 03:35:57 +0000
> Zack Rusin <zackr@vmware.com> wrote:
> 
> > On Tue, 2022-07-12 at 10:56 +0300, Pekka Paalanen wrote:
> > > On Mon, 11 Jul 2022 23:32:43 -0400
> > > Zack Rusin <zack@kde.org> wrote:
> > >   
> > > > 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>
> > > > ---
> > > >  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 fa0d73ce07bc..ca3134adb104 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);  
> > > 
> > > Hi,
> > > 
> > > this looks like silent clamping of the hotspot coordinates.
> > > 
> > > Should the DRM core perhaps already ensure that the hotspot must reside
> > > inside the plane (fb) boundaries and reject the atomic (TEST_ONLY)
> > > commit when it's outside?
> > > 
> > > Or if this restriction is not universal, maybe this driver should
> > > reject invalid hotspots rather than silently mangle them?  
> > 
> > TBH, I'm not really sure why vboxvideo is clamping those values. I didn't want to
> > introduce any regressions by changing it here, but the hotspot code never specified
> > that the hotspot offsets have to be positive or even within the plane. In a quick
> > search I couldn't find real world cursors that were doing anything like that though
> > so I just left it.
> > 
> > > Also, if supports_virtual_cursor_plane is false, should there not be
> > > somewhere code that will ignore the values set to the atomic hotspot
> > > properties?  
> > 
> > Hmm, good question. I'm not sure if there's a case where that could be possible:
> > without supports_virtual_cursor we either won't have a cursor plane or the client
> > won't be atomic and will only be allowed to use the old legacy kms ioctl's, i.e.
> > drmModeSetCursor2.
> > 
> > > When one KMS client switches to another, the first one being hotspot
> > > aware and the latter not, and both atomic, then the latter KMS client
> > > who doesn't know to drive the hotspot will inherit potentially invalid
> > > hotspot from the previous KMS client. Does something prevent that from
> > > being a problem?  
> > 
> > That switch will result in plane state reset, ending in
> > __drm_atomic_helper_plane_state_reset which will reset both hotspot properties (set
> > them to 0).
> 
> It will?
> 
> To my knowledge, KMS has never reset anything when one KMS client
> switches to the next, so that's new.
> 
> What triggers it?
> 
> Only if you actually switch to fbdev/fbcon instead of another userspace
> KMS client, the fbdev code might reset some things, but not all.
> 
> > > The old KMS client may have left the virtual cursor plane visible, and
> > > the new KMS client doesn't even know the virtual cursor plane exists
> > > because it doesn't set the DRM client cap. Something should probably
> > > ensure the virtual cursor plane gets disabled, right?  
> > 
> > Hah, that's also a good question. I *think* the same code to above would be ran,
> > i.e. plane reset which should result in the plane disappearing and the new client
> > not being able to drive it anymore. At least when running gnome-shell, switching to
> > weston and then switching to gnome-shell things look ok, but I haven't tried running
> > such clients at the same time.
> 
> That's an interesting experiment, but how is "at the same time"
> different from what you tested?
> 
> As i mentioned above, if you switch to fbcon in between, then you are
> not switching from one userspace KMS client to the next.

FWIW, running weston inside gnome-shell as a window also works fine. And running 
weston-simple-damage --width=60 --height=60 which, currently would make that client
pop up in the cursor plane, while running weston in a window inside gnome-shell also
works. I'm not sure what are the paths clients are taking in those cases so I don't
want to speculate but I'd be happy to try any other experiments or cases you think
might break.

z 

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

[Pekka Paalanen]

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

On Wed, 13 Jul 2022 13:35:56 +0000
Zack Rusin <zackr@vmware.com> wrote:

> On Wed, 2022-07-13 at 10:20 +0300, Pekka Paalanen wrote:
> > On Wed, 13 Jul 2022 03:35:57 +0000
> > Zack Rusin <zackr@vmware.com> wrote:
> >   
> > > On Tue, 2022-07-12 at 10:56 +0300, Pekka Paalanen wrote:  
> > > > On Mon, 11 Jul 2022 23:32:43 -0400
> > > > Zack Rusin <zack@kde.org> wrote:
> > > >     
> > > > > 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>
> > > > > ---
> > > > >  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 fa0d73ce07bc..ca3134adb104 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);    
> > > > 
> > > > Hi,
> > > > 
> > > > this looks like silent clamping of the hotspot coordinates.
> > > > 
> > > > Should the DRM core perhaps already ensure that the hotspot must reside
> > > > inside the plane (fb) boundaries and reject the atomic (TEST_ONLY)
> > > > commit when it's outside?
> > > > 
> > > > Or if this restriction is not universal, maybe this driver should
> > > > reject invalid hotspots rather than silently mangle them?    
> > > 
> > > TBH, I'm not really sure why vboxvideo is clamping those values. I didn't want to
> > > introduce any regressions by changing it here, but the hotspot code never specified
> > > that the hotspot offsets have to be positive or even within the plane. In a quick
> > > search I couldn't find real world cursors that were doing anything like that though
> > > so I just left it.
> > >   
> > > > Also, if supports_virtual_cursor_plane is false, should there not be
> > > > somewhere code that will ignore the values set to the atomic hotspot
> > > > properties?    
> > > 
> > > Hmm, good question. I'm not sure if there's a case where that could be possible:
> > > without supports_virtual_cursor we either won't have a cursor plane or the client
> > > won't be atomic and will only be allowed to use the old legacy kms ioctl's, i.e.
> > > drmModeSetCursor2.
> > >   
> > > > When one KMS client switches to another, the first one being hotspot
> > > > aware and the latter not, and both atomic, then the latter KMS client
> > > > who doesn't know to drive the hotspot will inherit potentially invalid
> > > > hotspot from the previous KMS client. Does something prevent that from
> > > > being a problem?    
> > > 
> > > That switch will result in plane state reset, ending in
> > > __drm_atomic_helper_plane_state_reset which will reset both hotspot properties (set
> > > them to 0).  
> > 
> > It will?
> > 
> > To my knowledge, KMS has never reset anything when one KMS client
> > switches to the next, so that's new.
> > 
> > What triggers it?
> > 
> > Only if you actually switch to fbdev/fbcon instead of another userspace
> > KMS client, the fbdev code might reset some things, but not all.
> >   
> > > > The old KMS client may have left the virtual cursor plane visible, and
> > > > the new KMS client doesn't even know the virtual cursor plane exists
> > > > because it doesn't set the DRM client cap. Something should probably
> > > > ensure the virtual cursor plane gets disabled, right?    
> > > 
> > > Hah, that's also a good question. I *think* the same code to above would be ran,
> > > i.e. plane reset which should result in the plane disappearing and the new client
> > > not being able to drive it anymore. At least when running gnome-shell, switching to
> > > weston and then switching to gnome-shell things look ok, but I haven't tried running
> > > such clients at the same time.  
> > 
> > That's an interesting experiment, but how is "at the same time"
> > different from what you tested?
> > 
> > As i mentioned above, if you switch to fbcon in between, then you are
> > not switching from one userspace KMS client to the next.  
> 
> FWIW, running weston inside gnome-shell as a window also works fine.

Of course it does. :-)

It won't be using DRM or KMS at all in that case. Weston acts as a
Wayland or X11 client in that case (while also being a Wayland
compositor towards its own clients).

> And running 
> weston-simple-damage --width=60 --height=60 which, currently would make that client
> pop up in the cursor plane, while running weston in a window inside gnome-shell also
> works. I'm not sure what are the paths clients are taking in those cases so I don't
> want to speculate but I'd be happy to try any other experiments or cases you think
> might break.

Right, I do believe that case is solved. The switching case is
different though.

The test for the switching case would be something like this:

- patch Mutter to set CAP_VIRTUAL_CURSOR, drop the blacklisting, set
  hotspot properties correctly, and make sure it uses the virtual
  cursor plane when possible

- launch GNOME on one VT

- launch Weston on another VT, make sure it uses drm-backend.so and not
  e.g. wayland-backend.so (the logs tell you, defaulting to stdout or
  stderr, I never remember which)

- switch to GNOME VT, check the cursor plane is used, switch to Weston VT

- observe if you have a dead cursor sprite on screen left over from GNOME

- Make sure Mutter or GDM did not "sanitize" KMS state before releasing
  DRM master for Weston

The last part is important, because the kernel must not rely on
userspace to sanitize KMS state on its way out. E.g. forced session
switching will not give the chance even, and I suppose most KMS clients
also don't bother. Sanitizing KMS on switch-out is also generally
avoided to avoid unnecessary modesets which take time and cause more
flicker, even if you're not aiming for seamless handover.

Since Weston never sees the virtual cursor plane even exists, there is
no way Weston could reset it. That's why it is important for the kernel
to reset the virtual cursor plane in this case. It is the fact that the
kernel does not expose the cursor plane at all to Weston that makes
this situation unprecedented, needing something completely new in the
kernel.

Maybe it's easier to patch some other compositor like Sway than Mutter
to use the new virtual cursor stuff, and then test between the two
compositor instances, one with the CAP and one without, and see how
VT-switching between them works.



Thanks,
pq

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

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

[Martin Krastev (VMware)]

From: Martin Krastev <krastevm@vmware.com>


On 12.07.22 г. 6:32 ч., Zack Rusin wrote:
> 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: Martin Krastev <krastevm@vmware.com>
> Cc: Maaz Mombasawala <mombasawalam@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 ff2f735bbe7a..3d3f73109199 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -652,13 +652,8 @@ vmw_du_cursor_plane_atomic_update(struct drm_plane *plane,
>   	struct vmw_plane_state *vps = vmw_plane_state_to_vps(new_state);
>   	s32 hotspot_x, hotspot_y;
>   
> -	hotspot_x = du->hotspot_x;
> -	hotspot_y = du->hotspot_y;
> -
> -	if (new_state->fb) {
> -		hotspot_x += new_state->fb->hot_x;
> -		hotspot_y += new_state->fb->hot_y;
> -	}
> +	hotspot_x = du->hotspot_x + new_state->hotspot_x;
> +	hotspot_y = du->hotspot_y + new_state->hotspot_y;
>   
>   	du->cursor_surface = vps->surf;
>   	du->cursor_bo = vps->bo;


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

Regards,
Martin

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

[Daniel Vetter]

On Mon, Jul 11, 2022 at 11:32:39PM -0400, Zack Rusin wrote:
> From: Zack Rusin <zackr@vmware.com>
> 
> Cursor planes on virtualized drivers have special meaning and require
> that the clients handle them in specific ways, e.g. the cursor plane
> should react to the mouse movement the way a mouse cursor would be
> expected to and the client is required to set hotspot properties on it
> in order for the mouse events to be routed correctly.
> 
> This breaks the contract as specified by the "universal planes". Fix it
> by disabling the cursor planes on virtualized drivers while adding
> a foundation on top of which it's possible to special case mouse cursor
> planes for clients that want it.
> 
> Disabling the cursor planes makes some kms compositors which were broken,
> e.g. Weston, fallback to software cursor which works fine or at least
> better than currently while having no effect on others, e.g. gnome-shell
> or kwin, which put virtualized drivers on a deny-list when running in
> atomic context to make them fallback to legacy kms and avoid this issue.
> 
> Signed-off-by: Zack Rusin <zackr@vmware.com>
> Fixes: 681e7ec73044 ("drm: Allow userspace to ask for universal plane list (v2)")
> Cc: <stable@vger.kernel.org> # v5.4+
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Gurchetan Singh <gurchetansingh@chromium.org>
> Cc: Chia-I Wu <olvaffe@gmail.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: virtualization@lists.linux-foundation.org
> Cc: spice-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/drm_plane.c          | 11 +++++++++++
>  drivers/gpu/drm/qxl/qxl_drv.c        |  2 +-
>  drivers/gpu/drm/vboxvideo/vbox_drv.c |  2 +-
>  drivers/gpu/drm/virtio/virtgpu_drv.c |  3 ++-
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c  |  2 +-
>  include/drm/drm_drv.h                | 10 ++++++++++
>  include/drm/drm_file.h               | 12 ++++++++++++
>  7 files changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 726f2f163c26..e1e2a65c7119 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -667,6 +667,17 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data,
>  		    !file_priv->universal_planes)
>  			continue;
>  
> +		/*
> +		 * Unless userspace supports virtual cursor plane
> +		 * then if we're running on virtual driver do not
> +		 * advertise cursor planes because they'll be broken
> +		 */
> +		if (plane->type == DRM_PLANE_TYPE_CURSOR &&
> +		    drm_core_check_feature(dev, DRIVER_VIRTUAL)	&&
> +		    file_priv->atomic &&
> +		    !file_priv->supports_virtual_cursor_plane)
> +			continue;
> +
>  		if (drm_lease_held(file_priv, plane->base.id)) {
>  			if (count < plane_resp->count_planes &&
>  			    put_user(plane->base.id, plane_ptr + count))
> diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
> index 1cb6f0c224bb..0e4212e05caa 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.c
> +++ b/drivers/gpu/drm/qxl/qxl_drv.c
> @@ -281,7 +281,7 @@ static const struct drm_ioctl_desc qxl_ioctls[] = {
>  };
>  
>  static struct drm_driver qxl_driver = {
> -	.driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
> +	.driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_VIRTUAL,
>  
>  	.dumb_create = qxl_mode_dumb_create,
>  	.dumb_map_offset = drm_gem_ttm_dumb_map_offset,
> diff --git a/drivers/gpu/drm/vboxvideo/vbox_drv.c b/drivers/gpu/drm/vboxvideo/vbox_drv.c
> index f4f2bd79a7cb..84e75bcc3384 100644
> --- a/drivers/gpu/drm/vboxvideo/vbox_drv.c
> +++ b/drivers/gpu/drm/vboxvideo/vbox_drv.c
> @@ -176,7 +176,7 @@ DEFINE_DRM_GEM_FOPS(vbox_fops);
>  
>  static const struct drm_driver driver = {
>  	.driver_features =
> -	    DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
> +	    DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC | DRIVER_VIRTUAL,
>  
>  	.lastclose = drm_fb_helper_lastclose,
>  
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
> index 5f25a8d15464..3c5bb006159a 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
> @@ -198,7 +198,8 @@ MODULE_AUTHOR("Alon Levy");
>  DEFINE_DRM_GEM_FOPS(virtio_gpu_driver_fops);
>  
>  static const struct drm_driver driver = {
> -	.driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER | DRIVER_ATOMIC,
> +	.driver_features =
> +		DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER | DRIVER_ATOMIC | DRIVER_VIRTUAL,
>  	.open = virtio_gpu_driver_open,
>  	.postclose = virtio_gpu_driver_postclose,
>  
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index 01a5b47e95f9..712f6ad0b014 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -1581,7 +1581,7 @@ static const struct file_operations vmwgfx_driver_fops = {
>  
>  static const struct drm_driver driver = {
>  	.driver_features =
> -	DRIVER_MODESET | DRIVER_RENDER | DRIVER_ATOMIC | DRIVER_GEM,
> +	DRIVER_MODESET | DRIVER_RENDER | DRIVER_ATOMIC | DRIVER_GEM | DRIVER_VIRTUAL,
>  	.ioctls = vmw_ioctls,
>  	.num_ioctls = ARRAY_SIZE(vmw_ioctls),
>  	.master_set = vmw_master_set,
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index f6159acb8856..c4cd7fc350d9 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -94,6 +94,16 @@ enum drm_driver_feature {
>  	 * synchronization of command submission.
>  	 */
>  	DRIVER_SYNCOBJ_TIMELINE         = BIT(6),
> +	/**
> +	 * @DRIVER_VIRTUAL:
> +	 *
> +	 * Driver is running on top of virtual hardware. The most significant
> +	 * implication of this is a requirement of special handling of the
> +	 * cursor plane (e.g. cursor plane has to actually track the mouse
> +	 * cursor and the clients are required to set hotspot in order for
> +	 * the cursor planes to work correctly).
> +	 */
> +	DRIVER_VIRTUAL                  = BIT(7),

I think the naming here is unfortunate, because people will vonder why
e.g. vkms doesn't set this, and then add it, and confuse stuff completely.

Also it feels a bit wrong to put this onto the driver, when really it's a
cursor flag. I guess you can make it some kind of flag in the drm_plane
structure, or a new plane type, but putting it there instead of into the
"random pile of midlayer-mistake driver flags" would be a lot better.

Otherwise I think the series looks roughly how I'd expect it to look.
-Daniel

>  
>  	/* IMPORTANT: Below are all the legacy flags, add new ones above. */
>  
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index e0a73a1e2df7..3e5c36891161 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -223,6 +223,18 @@ struct drm_file {
>  	 */
>  	bool is_master;
>  
> +	/**
> +	 * @supports_virtual_cursor_plane:
> +	 *
> +	 * This client is capable of handling the cursor plane with the
> +	 * restrictions imposed on it by the virtualized drivers.
> +	 *
> +	 * The implies that the cursor plane has to behave like a cursor
> +	 * i.e. track cursor movement. It also requires setting of the
> +	 * hotspot properties by the client on the cursor plane.
> +	 */
> +	bool supports_virtual_cursor_plane;
> +
>  	/**
>  	 * @master:
>  	 *
> -- 
> 2.34.1
> 

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

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

[Javier Martinez Canillas]

Daniel Vetter <daniel@ffwll.ch> writes:

> On Mon, Jul 11, 2022 at 11:32:39PM -0400, Zack Rusin wrote:
>> From: Zack Rusin <zackr@vmware.com>
>> 
>> Cursor planes on virtualized drivers have special meaning and require
>> that the clients handle them in specific ways, e.g. the cursor plane
>> should react to the mouse movement the way a mouse cursor would be
>> expected to and the client is required to set hotspot properties on it
>> in order for the mouse events to be routed correctly.
>> 
>> This breaks the contract as specified by the "universal planes". Fix it
>> by disabling the cursor planes on virtualized drivers while adding
>> a foundation on top of which it's possible to special case mouse cursor
>> planes for clients that want it.
>> 
>> Disabling the cursor planes makes some kms compositors which were broken,
>> e.g. Weston, fallback to software cursor which works fine or at least
>> better than currently while having no effect on others, e.g. gnome-shell
>> or kwin, which put virtualized drivers on a deny-list when running in
>> atomic context to make them fallback to legacy kms and avoid this issue.
>> 
>> Signed-off-by: Zack Rusin <zackr@vmware.com>
>> Fixes: 681e7ec73044 ("drm: Allow userspace to ask for universal plane list (v2)")

[...]

>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>> index f6159acb8856..c4cd7fc350d9 100644
>> --- a/include/drm/drm_drv.h
>> +++ b/include/drm/drm_drv.h
>> @@ -94,6 +94,16 @@ enum drm_driver_feature {
>>  	 * synchronization of command submission.
>>  	 */
>>  	DRIVER_SYNCOBJ_TIMELINE         = BIT(6),
>> +	/**
>> +	 * @DRIVER_VIRTUAL:
>> +	 *
>> +	 * Driver is running on top of virtual hardware. The most significant
>> +	 * implication of this is a requirement of special handling of the
>> +	 * cursor plane (e.g. cursor plane has to actually track the mouse
>> +	 * cursor and the clients are required to set hotspot in order for
>> +	 * the cursor planes to work correctly).
>> +	 */
>> +	DRIVER_VIRTUAL                  = BIT(7),
>
> I think the naming here is unfortunate, because people will vonder why
> e.g. vkms doesn't set this, and then add it, and confuse stuff completely.
>
> Also it feels a bit wrong to put this onto the driver, when really it's a
> cursor flag. I guess you can make it some kind of flag in the drm_plane
> structure, or a new plane type, but putting it there instead of into the
> "random pile of midlayer-mistake driver flags" would be a lot better.
>
> Otherwise I think the series looks roughly how I'd expect it to look.
> -Daniel
>

AFAICT this is the only remaining thing to be addressed for this series ?

Zack, are you planning to re-spin a v3 of this patch-set? Asking because
we want to take virtio-gpu out of the atomic KMS deny list in mutter, but
first need this to land.

If you think that won't be able to do it in the short term, Bilal (Cc'ed)
or me would be glad to help with that.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

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

[Zack Rusin]

On Tue, 2023-05-02 at 11:32 +0200, Javier Martinez Canillas wrote:
> !! External Email
> 
> Daniel Vetter <daniel@ffwll.ch> writes:
> 
> > On Mon, Jul 11, 2022 at 11:32:39PM -0400, Zack Rusin wrote:
> > > From: Zack Rusin <zackr@vmware.com>
> > > 
> > > Cursor planes on virtualized drivers have special meaning and require
> > > that the clients handle them in specific ways, e.g. the cursor plane
> > > should react to the mouse movement the way a mouse cursor would be
> > > expected to and the client is required to set hotspot properties on it
> > > in order for the mouse events to be routed correctly.
> > > 
> > > This breaks the contract as specified by the "universal planes". Fix it
> > > by disabling the cursor planes on virtualized drivers while adding
> > > a foundation on top of which it's possible to special case mouse cursor
> > > planes for clients that want it.
> > > 
> > > Disabling the cursor planes makes some kms compositors which were broken,
> > > e.g. Weston, fallback to software cursor which works fine or at least
> > > better than currently while having no effect on others, e.g. gnome-shell
> > > or kwin, which put virtualized drivers on a deny-list when running in
> > > atomic context to make them fallback to legacy kms and avoid this issue.
> > > 
> > > Signed-off-by: Zack Rusin <zackr@vmware.com>
> > > Fixes: 681e7ec73044 ("drm: Allow userspace to ask for universal plane list
> > > (v2)")
> 
> [...]
> 
> > > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> > > index f6159acb8856..c4cd7fc350d9 100644
> > > --- a/include/drm/drm_drv.h
> > > +++ b/include/drm/drm_drv.h
> > > @@ -94,6 +94,16 @@ enum drm_driver_feature {
> > >       * synchronization of command submission.
> > >       */
> > >      DRIVER_SYNCOBJ_TIMELINE         = BIT(6),
> > > +    /**
> > > +     * @DRIVER_VIRTUAL:
> > > +     *
> > > +     * Driver is running on top of virtual hardware. The most significant
> > > +     * implication of this is a requirement of special handling of the
> > > +     * cursor plane (e.g. cursor plane has to actually track the mouse
> > > +     * cursor and the clients are required to set hotspot in order for
> > > +     * the cursor planes to work correctly).
> > > +     */
> > > +    DRIVER_VIRTUAL                  = BIT(7),
> > 
> > I think the naming here is unfortunate, because people will vonder why
> > e.g. vkms doesn't set this, and then add it, and confuse stuff completely.
> > 
> > Also it feels a bit wrong to put this onto the driver, when really it's a
> > cursor flag. I guess you can make it some kind of flag in the drm_plane
> > structure, or a new plane type, but putting it there instead of into the
> > "random pile of midlayer-mistake driver flags" would be a lot better.
> > 
> > Otherwise I think the series looks roughly how I'd expect it to look.
> > -Daniel
> > 
> 
> AFAICT this is the only remaining thing to be addressed for this series ?

No, there was more. tbh I haven't had the time to think about whether the above
makes sense to me, e.g. I'm not sure if having virtualized drivers expose "support
universal planes" and adding another plane which is not universal (the only
"universal" plane on them being the default one) makes more sense than a flag that
says "this driver requires a cursor in the cursor plane". There's certainly a huge
difference in how userspace would be required to handle it and it's way uglier with
two different cursor planes. i.e. there's a lot of ways in which this could be
cleaner in the kernel but they all require significant changes to userspace, that go
way beyond "attach hotspot info to this plane". I'd like to avoid approaches that
mean running with atomic kms requires completely separate paths for virtualized
drivers because no one will ever support and maintain it.

It's not a trivial thing because it's fundamentally hard to untangle the fact the
virtualized drivers have been advertising universal plane support without ever
supporting universal planes. Especially because most new userspace in general checks
for "universal planes" to expose atomic kms paths.

The other thing blocking this series was the testing of all the edge cases, I think
Simon and Pekka had some ideas for things to test (e.g. run mutter with support for
this and wayland without support for this in at the same time in different consoles
and see what happens). I never had the time to do that either.

> Zack, are you planning to re-spin a v3 of this patch-set? Asking because
> we want to take virtio-gpu out of the atomic KMS deny list in mutter, but
> first need this to land.
> 
> If you think that won't be able to do it in the short term, Bilal (Cc'ed)
> or me would be glad to help with that.

This has been on my todo for a while I just never had the time to go through all the
remaining issues. Fundamentally it's not so much a technical issue anymore, it's
about picking the least broken solution and trying to make the best out of a pretty
bad situation. In general it's hard to paint a bikeshed if all you have is a million
shades of gray ;)

z

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

[Javier Martinez Canillas]

Zack Rusin <zackr@vmware.com> writes:

> On Tue, 2023-05-02 at 11:32 +0200, Javier Martinez Canillas wrote:
>> !! External Email
>> 
>> Daniel Vetter <daniel@ffwll.ch> writes:
>> 
>> > On Mon, Jul 11, 2022 at 11:32:39PM -0400, Zack Rusin wrote:
>> > > From: Zack Rusin <zackr@vmware.com>
>> > > 
>> > > Cursor planes on virtualized drivers have special meaning and require
>> > > that the clients handle them in specific ways, e.g. the cursor plane
>> > > should react to the mouse movement the way a mouse cursor would be
>> > > expected to and the client is required to set hotspot properties on it
>> > > in order for the mouse events to be routed correctly.
>> > > 
>> > > This breaks the contract as specified by the "universal planes". Fix it
>> > > by disabling the cursor planes on virtualized drivers while adding
>> > > a foundation on top of which it's possible to special case mouse cursor
>> > > planes for clients that want it.
>> > > 
>> > > Disabling the cursor planes makes some kms compositors which were broken,
>> > > e.g. Weston, fallback to software cursor which works fine or at least
>> > > better than currently while having no effect on others, e.g. gnome-shell
>> > > or kwin, which put virtualized drivers on a deny-list when running in
>> > > atomic context to make them fallback to legacy kms and avoid this issue.
>> > > 
>> > > Signed-off-by: Zack Rusin <zackr@vmware.com>
>> > > Fixes: 681e7ec73044 ("drm: Allow userspace to ask for universal plane list
>> > > (v2)")
>> 
>> [...]
>> 
>> > > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>> > > index f6159acb8856..c4cd7fc350d9 100644
>> > > --- a/include/drm/drm_drv.h
>> > > +++ b/include/drm/drm_drv.h
>> > > @@ -94,6 +94,16 @@ enum drm_driver_feature {
>> > >       * synchronization of command submission.
>> > >       */
>> > >      DRIVER_SYNCOBJ_TIMELINE         = BIT(6),
>> > > +    /**
>> > > +     * @DRIVER_VIRTUAL:
>> > > +     *
>> > > +     * Driver is running on top of virtual hardware. The most significant
>> > > +     * implication of this is a requirement of special handling of the
>> > > +     * cursor plane (e.g. cursor plane has to actually track the mouse
>> > > +     * cursor and the clients are required to set hotspot in order for
>> > > +     * the cursor planes to work correctly).
>> > > +     */
>> > > +    DRIVER_VIRTUAL                  = BIT(7),
>> > 
>> > I think the naming here is unfortunate, because people will vonder why
>> > e.g. vkms doesn't set this, and then add it, and confuse stuff completely.
>> > 
>> > Also it feels a bit wrong to put this onto the driver, when really it's a
>> > cursor flag. I guess you can make it some kind of flag in the drm_plane
>> > structure, or a new plane type, but putting it there instead of into the
>> > "random pile of midlayer-mistake driver flags" would be a lot better.
>> > 
>> > Otherwise I think the series looks roughly how I'd expect it to look.
>> > -Daniel
>> > 
>> 
>> AFAICT this is the only remaining thing to be addressed for this series ?
>
> No, there was more. tbh I haven't had the time to think about whether the above
> makes sense to me, e.g. I'm not sure if having virtualized drivers expose "support
> universal planes" and adding another plane which is not universal (the only
> "universal" plane on them being the default one) makes more sense than a flag that
> says "this driver requires a cursor in the cursor plane". There's certainly a huge
> difference in how userspace would be required to handle it and it's way uglier with
> two different cursor planes. i.e. there's a lot of ways in which this could be
> cleaner in the kernel but they all require significant changes to userspace, that go
> way beyond "attach hotspot info to this plane". I'd like to avoid approaches that
> mean running with atomic kms requires completely separate paths for virtualized
> drivers because no one will ever support and maintain it.
>
> It's not a trivial thing because it's fundamentally hard to untangle the fact the
> virtualized drivers have been advertising universal plane support without ever
> supporting universal planes. Especially because most new userspace in general checks
> for "universal planes" to expose atomic kms paths.
>

After some discussion on the #dri-devel, your approach makes sense and the
only contention point is the name of the driver feature flag name. The one
you are using (DRIVER_VIRTUAL) seems to be too broad and generic (the fact
that vkms won't set and is a virtual driver as well, is a good example).

Maybe something like DRIVER_CURSOR_HOTSPOT or DRIVER_CURSOR_COMMANDEERING
would be more accurate and self explanatory ?

> The other thing blocking this series was the testing of all the edge cases, I think
> Simon and Pekka had some ideas for things to test (e.g. run mutter with support for
> this and wayland without support for this in at the same time in different consoles
> and see what happens). I never had the time to do that either.
>

I understand that every new feature needs tests but I fail to see why
the bar is higher for this feature than others? I would prefer if this
series are not blocked due some potential issues on hypothetical corner
cases that might not happen in practice. Or do people really run two or
more compositors on different console and switch between them ?

>> Zack, are you planning to re-spin a v3 of this patch-set? Asking because
>> we want to take virtio-gpu out of the atomic KMS deny list in mutter, but
>> first need this to land.
>> 
>> If you think that won't be able to do it in the short term, Bilal (Cc'ed)
>> or me would be glad to help with that.
>
> This has been on my todo for a while I just never had the time to go through all the
> remaining issues. Fundamentally it's not so much a technical issue anymore, it's
> about picking the least broken solution and trying to make the best out of a pretty
> bad situation. In general it's hard to paint a bikeshed if all you have is a million
> shades of gray ;)
>

Agreed. And I believe that other than the driver cap name, everyone agrees
with the color of your bikeshed :)

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

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

[Pekka Paalanen]

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

On Wed, 3 May 2023 03:35:29 +0000
Zack Rusin <zackr@vmware.com> wrote:

> On Tue, 2023-05-02 at 11:32 +0200, Javier Martinez Canillas wrote:
> > !! External Email
> > 
> > Daniel Vetter <daniel@ffwll.ch> writes:
> >   
> > > On Mon, Jul 11, 2022 at 11:32:39PM -0400, Zack Rusin wrote:  
> > > > From: Zack Rusin <zackr@vmware.com>
> > > > 
> > > > Cursor planes on virtualized drivers have special meaning and require
> > > > that the clients handle them in specific ways, e.g. the cursor plane
> > > > should react to the mouse movement the way a mouse cursor would be
> > > > expected to and the client is required to set hotspot properties on it
> > > > in order for the mouse events to be routed correctly.
> > > > 
> > > > This breaks the contract as specified by the "universal planes". Fix it
> > > > by disabling the cursor planes on virtualized drivers while adding
> > > > a foundation on top of which it's possible to special case mouse cursor
> > > > planes for clients that want it.
> > > > 
> > > > Disabling the cursor planes makes some kms compositors which were broken,
> > > > e.g. Weston, fallback to software cursor which works fine or at least
> > > > better than currently while having no effect on others, e.g. gnome-shell
> > > > or kwin, which put virtualized drivers on a deny-list when running in
> > > > atomic context to make them fallback to legacy kms and avoid this issue.
> > > > 
> > > > Signed-off-by: Zack Rusin <zackr@vmware.com>
> > > > Fixes: 681e7ec73044 ("drm: Allow userspace to ask for universal plane list
> > > > (v2)")  
> > 
> > [...]
> >   
> > > > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> > > > index f6159acb8856..c4cd7fc350d9 100644
> > > > --- a/include/drm/drm_drv.h
> > > > +++ b/include/drm/drm_drv.h
> > > > @@ -94,6 +94,16 @@ enum drm_driver_feature {
> > > >       * synchronization of command submission.
> > > >       */
> > > >      DRIVER_SYNCOBJ_TIMELINE         = BIT(6),
> > > > +    /**
> > > > +     * @DRIVER_VIRTUAL:
> > > > +     *
> > > > +     * Driver is running on top of virtual hardware. The most significant
> > > > +     * implication of this is a requirement of special handling of the
> > > > +     * cursor plane (e.g. cursor plane has to actually track the mouse
> > > > +     * cursor and the clients are required to set hotspot in order for
> > > > +     * the cursor planes to work correctly).
> > > > +     */
> > > > +    DRIVER_VIRTUAL                  = BIT(7),  
> > > 
> > > I think the naming here is unfortunate, because people will vonder why
> > > e.g. vkms doesn't set this, and then add it, and confuse stuff completely.
> > > 
> > > Also it feels a bit wrong to put this onto the driver, when really it's a
> > > cursor flag. I guess you can make it some kind of flag in the drm_plane
> > > structure, or a new plane type, but putting it there instead of into the
> > > "random pile of midlayer-mistake driver flags" would be a lot better.
> > > 
> > > Otherwise I think the series looks roughly how I'd expect it to look.
> > > -Daniel
> > >   
> > 
> > AFAICT this is the only remaining thing to be addressed for this series ?  
> 
> No, there was more. tbh I haven't had the time to think about whether the above
> makes sense to me, e.g. I'm not sure if having virtualized drivers expose "support
> universal planes" and adding another plane which is not universal (the only
> "universal" plane on them being the default one) makes more sense than a flag that
> says "this driver requires a cursor in the cursor plane". There's certainly a huge
> difference in how userspace would be required to handle it and it's way uglier with
> two different cursor planes. i.e. there's a lot of ways in which this could be
> cleaner in the kernel but they all require significant changes to userspace, that go
> way beyond "attach hotspot info to this plane".

> I'd like to avoid approaches that
> mean running with atomic kms requires completely separate paths for virtualized
> drivers because no one will ever support and maintain it.

Hi Zack,

you'd like to avoid that, but fundamentally that really is what has to
happen in userspace for *nested* KMS drivers (VKMS is a virtual driver
but not part of the interest group here) to reach optimality.

It really is a different path. I see no way around that. But if you
accept that fact, then you could possibly gain a lot more benefits by
asking userspace to handle nested KMS drivers differently. What those
benefits are exactly I'm not sure, but I have a feeling there should be
some, where the knowledge of running on a nested KMS driver allows for
better decisions that are not possible if the nested KMS driver just
pretends to be like any other KMS hardware driver.

You can get up to some level of interoperability by pretending to be
just like any other KMS driver, but if you want to optimize things, I
feel that's a whole different story. It's a trade-off.

I think frame timing is one thing. A nested KMS driver increases
the depth of the "swapchain" between the guest KMS app and the actual
hardware. This is unexpected if userspace does not know it is running
on a nested KMS driver.

The existing KMS uAPI, both legacy and atomic, have been written for
classic hardware. One fundamental result of that is the page flip
completion event, it signals two things simultaneously: the new
framebuffer is in use, and a new flip can be programmed.
On a nested driver, these two are not the same thing: the nested driver
can take another flip before the new framebuffer is actually being used
in the host. More importantly, the nested driver can take a new flip
before the old replaced framebuffer has actually been retired.

(The above can tie into the question of making the KMS swapchain deeper
in general, also for classic scanout design, in connection to
present-not-before-timestamp queueing at KMS level.)

However, as long as these two are the same event, it can decimate the
framerate on a nested driver, because userspace is not prepared for a
swapchain depth of greater than one. Or, the nested KMS driver gives up
on zero-copy. Or, you need a fragile timing arrangement that
essentially needs to be hand-configured in at least one display system,
guest or host.

Somewhat related, there is also the matter of KMS drivers (hardware,
nested, and virtual) that do not lock their page flip events to a
hardware scanout cycle (because there is none) but "complete" any flip
immediately. That too requires explicit handling in userspace, because
you simply do not have a scanout cycle to lock on to.

We already have an example where userspace is explicitly helping
"unusual" KMS drivers: FB_DAMAGE_CLIPS. While educating userspace does
take considerable effort, I'd like to believe it is doable, and it is
also necessary for optimality. Excellent KMS documentation is key,
naturally.

Of course, it is up to you and other people to decide to want to do the
work or not. I just feel you could potentially gain a lot if you decide
to take on that fight.

> It's not a trivial thing because it's fundamentally hard to untangle the fact the
> virtualized drivers have been advertising universal plane support without ever
> supporting universal planes. Especially because most new userspace in general checks
> for "universal planes" to expose atomic kms paths.

That's not just userspace, it's built into the kernel UAPI design that
you cannot have atomic without universal planes.


Thanks,
pq

> The other thing blocking this series was the testing of all the edge cases, I think
> Simon and Pekka had some ideas for things to test (e.g. run mutter with support for
> this and wayland without support for this in at the same time in different consoles
> and see what happens). I never had the time to do that either.
> 
> > Zack, are you planning to re-spin a v3 of this patch-set? Asking because
> > we want to take virtio-gpu out of the atomic KMS deny list in mutter, but
> > first need this to land.
> > 
> > If you think that won't be able to do it in the short term, Bilal (Cc'ed)
> > or me would be glad to help with that.  
> 
> This has been on my todo for a while I just never had the time to go through all the
> remaining issues. Fundamentally it's not so much a technical issue anymore, it's
> about picking the least broken solution and trying to make the best out of a pretty
> bad situation. In general it's hard to paint a bikeshed if all you have is a million
> shades of gray ;)
> 
> z
> 


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

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

[Daniel Vetter]

On Tue, 12 Jul 2022 at 05:33, 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>
> ---
>  drivers/gpu/drm/drm_atomic_state_helper.c | 14 +++++++
>  drivers/gpu/drm/drm_atomic_uapi.c         | 20 ++++++++++
>  drivers/gpu/drm/drm_plane.c               | 47 +++++++++++++++++++++++
>  include/drm/drm_plane.h                   | 15 ++++++++
>  4 files changed, 96 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> index bf31b9d92094..9c4fda0b35e8 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 79730fa1dd8e..6132540c97a9 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -575,6 +575,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",
> @@ -635,6 +651,10 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>                 *val = state->scaling_filter;
>         } else if (plane->funcs->atomic_get_property) {
>                 return plane->funcs->atomic_get_property(plane, state, property, val);
> +       } else if (property == plane->hotspot_x_property) {
> +               *val = state->hotspot_x;
> +       } else if (property == plane->hotspot_y_property) {
> +               *val = state->hotspot_y;
>         } else {
>                 return -EINVAL;
>         }
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index e1e2a65c7119..0a6a1b5adf82 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -343,6 +343,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_VIRTUAL) &&
> +           type == DRM_PLANE_TYPE_CURSOR) {
> +               drm_plane_create_hotspot_properties(plane);
> +       }
>
>         if (format_modifier_count)
>                 create_in_format_blob(dev, plane);
> @@ -1054,6 +1058,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;
>                 }
> @@ -1582,3 +1591,41 @@ int drm_plane_create_scaling_filter_property(struct drm_plane *plane,
>         return 0;
>  }
>  EXPORT_SYMBOL(drm_plane_create_scaling_filter_property);
> +
> +/**
> + * drm_plane_create_hotspot_properties - creates the mouse hotspot
> + * properties and attaches them to the given cursor plane
> + *
> + * @plane: drm cursor plane
> + *
> + * This function lets driver to enable the mouse hotspot property on a given
> + * cursor plane.
> + *
> + * RETURNS:
> + * Zero for success or -errno
> + */
> +int drm_plane_create_hotspot_properties(struct drm_plane *plane)
> +{

So I don't really like the DRIVER_VIRTUAL flag as mentioned in another
reply, but that's a bit a bikeshed so *meh*.

What is more real is that driver flags in general have the issue that
you need to keep two entirely separate things in sync: The actual
driver code in the cursor support and the flag somewhere totally else.
That's why I don't like them, and a flag on the plane would be a lot
better. And this function here could then set that flag to make sure
things _never_ get out of sync.

Minimally we need a WARN_ON here that asserts that the DRIVER_VIRTUAL
flag is set.
-Daniel

> +       struct drm_property *prop_x;
> +       struct drm_property *prop_y;
> +
> +       prop_x = drm_property_create_signed_range(plane->dev, 0, "HOTSPOT_X",
> +                                                 INT_MIN, INT_MAX);
> +       if (IS_ERR(prop_x))
> +               return PTR_ERR(prop_x);
> +
> +       prop_y = drm_property_create_signed_range(plane->dev, 0, "HOTSPOT_Y",
> +                                                 INT_MIN, INT_MAX);
> +       if (IS_ERR(prop_y)) {
> +               drm_property_destroy(plane->dev, prop_x);
> +               return PTR_ERR(prop_y);
> +       }
> +
> +       drm_object_attach_property(&plane->base, prop_x, 0);
> +       drm_object_attach_property(&plane->base, prop_y, 0);
> +       plane->hotspot_x_property = prop_x;
> +       plane->hotspot_y_property = prop_y;
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(drm_plane_create_hotspot_properties);
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 89ea54652e87..09d3e3791e67 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -116,6 +116,10 @@ struct drm_plane_state {
>         /** @src_h: height of visible portion of plane (in 16.16) */
>         uint32_t src_h, src_w;
>
> +       /** @hotspot_x: x offset to mouse cursor hotspot */
> +       /** @hotspot_y: y offset to mouse cursor hotspot */
> +       int32_t hotspot_x, hotspot_y;
> +
>         /**
>          * @alpha:
>          * Opacity of the plane with 0 as completely transparent and 0xffff as
> @@ -748,6 +752,16 @@ struct drm_plane {
>          * scaling.
>          */
>         struct drm_property *scaling_filter_property;
> +
> +       /**
> +        * @hotspot_x_property: property to set mouse hotspot x offset.
> +        */
> +       struct drm_property *hotspot_x_property;
> +
> +       /**
> +        * @hotspot_y_property: property to set mouse hotspot y offset.
> +        */
> +       struct drm_property *hotspot_y_property;
>  };
>
>  #define obj_to_plane(x) container_of(x, struct drm_plane, base)
> @@ -907,5 +921,6 @@ drm_plane_get_damage_clips(const struct drm_plane_state *state);
>
>  int drm_plane_create_scaling_filter_property(struct drm_plane *plane,
>                                              unsigned int supported_filters);
> +int drm_plane_create_hotspot_properties(struct drm_plane *plane);
>
>  #endif
> --
> 2.34.1
>


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

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

[Pekka Paalanen]

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

On Wed, 03 May 2023 09:48:54 +0200
Javier Martinez Canillas <javierm@redhat.com> wrote:

> Zack Rusin <zackr@vmware.com> writes:
> 
> > On Tue, 2023-05-02 at 11:32 +0200, Javier Martinez Canillas wrote:  
> >> !! External Email
> >> 
> >> Daniel Vetter <daniel@ffwll.ch> writes:
> >>   
> >> > On Mon, Jul 11, 2022 at 11:32:39PM -0400, Zack Rusin wrote:  
> >> > > From: Zack Rusin <zackr@vmware.com>
> >> > > 
> >> > > Cursor planes on virtualized drivers have special meaning and require
> >> > > that the clients handle them in specific ways, e.g. the cursor plane
> >> > > should react to the mouse movement the way a mouse cursor would be
> >> > > expected to and the client is required to set hotspot properties on it
> >> > > in order for the mouse events to be routed correctly.
> >> > > 
> >> > > This breaks the contract as specified by the "universal planes". Fix it
> >> > > by disabling the cursor planes on virtualized drivers while adding
> >> > > a foundation on top of which it's possible to special case mouse cursor
> >> > > planes for clients that want it.
> >> > > 
> >> > > Disabling the cursor planes makes some kms compositors which were broken,
> >> > > e.g. Weston, fallback to software cursor which works fine or at least
> >> > > better than currently while having no effect on others, e.g. gnome-shell
> >> > > or kwin, which put virtualized drivers on a deny-list when running in
> >> > > atomic context to make them fallback to legacy kms and avoid this issue.
> >> > > 
> >> > > Signed-off-by: Zack Rusin <zackr@vmware.com>
> >> > > Fixes: 681e7ec73044 ("drm: Allow userspace to ask for universal plane list
> >> > > (v2)")  
> >> 
> >> [...]
> >>   
> >> > > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> >> > > index f6159acb8856..c4cd7fc350d9 100644
> >> > > --- a/include/drm/drm_drv.h
> >> > > +++ b/include/drm/drm_drv.h
> >> > > @@ -94,6 +94,16 @@ enum drm_driver_feature {
> >> > >       * synchronization of command submission.
> >> > >       */
> >> > >      DRIVER_SYNCOBJ_TIMELINE         = BIT(6),
> >> > > +    /**
> >> > > +     * @DRIVER_VIRTUAL:
> >> > > +     *
> >> > > +     * Driver is running on top of virtual hardware. The most significant
> >> > > +     * implication of this is a requirement of special handling of the
> >> > > +     * cursor plane (e.g. cursor plane has to actually track the mouse
> >> > > +     * cursor and the clients are required to set hotspot in order for
> >> > > +     * the cursor planes to work correctly).
> >> > > +     */
> >> > > +    DRIVER_VIRTUAL                  = BIT(7),  
> >> > 
> >> > I think the naming here is unfortunate, because people will vonder why
> >> > e.g. vkms doesn't set this, and then add it, and confuse stuff completely.
> >> > 
> >> > Also it feels a bit wrong to put this onto the driver, when really it's a
> >> > cursor flag. I guess you can make it some kind of flag in the drm_plane
> >> > structure, or a new plane type, but putting it there instead of into the
> >> > "random pile of midlayer-mistake driver flags" would be a lot better.
> >> > 
> >> > Otherwise I think the series looks roughly how I'd expect it to look.
> >> > -Daniel
> >> >   
> >> 
> >> AFAICT this is the only remaining thing to be addressed for this series ?  
> >
> > No, there was more. tbh I haven't had the time to think about whether the above
> > makes sense to me, e.g. I'm not sure if having virtualized drivers expose "support
> > universal planes" and adding another plane which is not universal (the only
> > "universal" plane on them being the default one) makes more sense than a flag that
> > says "this driver requires a cursor in the cursor plane". There's certainly a huge
> > difference in how userspace would be required to handle it and it's way uglier with
> > two different cursor planes. i.e. there's a lot of ways in which this could be
> > cleaner in the kernel but they all require significant changes to userspace, that go
> > way beyond "attach hotspot info to this plane". I'd like to avoid approaches that
> > mean running with atomic kms requires completely separate paths for virtualized
> > drivers because no one will ever support and maintain it.
> >
> > It's not a trivial thing because it's fundamentally hard to untangle the fact the
> > virtualized drivers have been advertising universal plane support without ever
> > supporting universal planes. Especially because most new userspace in general checks
> > for "universal planes" to expose atomic kms paths.
> >  
> 
> After some discussion on the #dri-devel, your approach makes sense and the
> only contention point is the name of the driver feature flag name. The one
> you are using (DRIVER_VIRTUAL) seems to be too broad and generic (the fact
> that vkms won't set and is a virtual driver as well, is a good example).
> 
> Maybe something like DRIVER_CURSOR_HOTSPOT or DRIVER_CURSOR_COMMANDEERING
> would be more accurate and self explanatory ?
> 
> > The other thing blocking this series was the testing of all the edge cases, I think
> > Simon and Pekka had some ideas for things to test (e.g. run mutter with support for
> > this and wayland without support for this in at the same time in different consoles
> > and see what happens). I never had the time to do that either.
> >  
> 
> I understand that every new feature needs tests but I fail to see why
> the bar is higher for this feature than others? I would prefer if this
> series are not blocked due some potential issues on hypothetical corner
> cases that might not happen in practice. Or do people really run two or
> more compositors on different console and switch between them ?

I have no recollection at all about what was talked about this, but in
certain virtualized cases you will always have two display systems
simultaneously:
- the guest display system which uses the nested KMS driver in the
  guest VM, which presents to
- a VM viewer application on the host, which presents via Wayland to
- the host display system which uses a hardware KMS driver.

Maybe it was more like that to test?


Thanks,
pq

> >> Zack, are you planning to re-spin a v3 of this patch-set? Asking because
> >> we want to take virtio-gpu out of the atomic KMS deny list in mutter, but
> >> first need this to land.
> >> 
> >> If you think that won't be able to do it in the short term, Bilal (Cc'ed)
> >> or me would be glad to help with that.  
> >
> > This has been on my todo for a while I just never had the time to go through all the
> > remaining issues. Fundamentally it's not so much a technical issue anymore, it's
> > about picking the least broken solution and trying to make the best out of a pretty
> > bad situation. In general it's hard to paint a bikeshed if all you have is a million
> > shades of gray ;)
> >  
> 
> Agreed. And I believe that other than the driver cap name, everyone agrees
> with the color of your bikeshed :)
> 


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

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

[Zack Rusin]

On Wed, 2023-05-03 at 10:54 +0300, Pekka Paalanen wrote:
> On Wed, 3 May 2023 03:35:29 +0000
> Zack Rusin <zackr@vmware.com> wrote:
> 
> > On Tue, 2023-05-02 at 11:32 +0200, Javier Martinez Canillas wrote:
> > > !! External Email
> > > 
> > > Daniel Vetter <daniel@ffwll.ch> writes:
> > >   
> > > > On Mon, Jul 11, 2022 at 11:32:39PM -0400, Zack Rusin wrote:  
> > > > > From: Zack Rusin <zackr@vmware.com>
> > > > > 
> > > > > Cursor planes on virtualized drivers have special meaning and require
> > > > > that the clients handle them in specific ways, e.g. the cursor plane
> > > > > should react to the mouse movement the way a mouse cursor would be
> > > > > expected to and the client is required to set hotspot properties on it
> > > > > in order for the mouse events to be routed correctly.
> > > > > 
> > > > > This breaks the contract as specified by the "universal planes". Fix it
> > > > > by disabling the cursor planes on virtualized drivers while adding
> > > > > a foundation on top of which it's possible to special case mouse cursor
> > > > > planes for clients that want it.
> > > > > 
> > > > > Disabling the cursor planes makes some kms compositors which were broken,
> > > > > e.g. Weston, fallback to software cursor which works fine or at least
> > > > > better than currently while having no effect on others, e.g. gnome-shell
> > > > > or kwin, which put virtualized drivers on a deny-list when running in
> > > > > atomic context to make them fallback to legacy kms and avoid this issue.
> > > > > 
> > > > > Signed-off-by: Zack Rusin <zackr@vmware.com>
> > > > > Fixes: 681e7ec73044 ("drm: Allow userspace to ask for universal plane list
> > > > > (v2)")  
> > > 
> > > [...]
> > >   
> > > > > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> > > > > index f6159acb8856..c4cd7fc350d9 100644
> > > > > --- a/include/drm/drm_drv.h
> > > > > +++ b/include/drm/drm_drv.h
> > > > > @@ -94,6 +94,16 @@ enum drm_driver_feature {
> > > > >       * synchronization of command submission.
> > > > >       */
> > > > >      DRIVER_SYNCOBJ_TIMELINE         = BIT(6),
> > > > > +    /**
> > > > > +     * @DRIVER_VIRTUAL:
> > > > > +     *
> > > > > +     * Driver is running on top of virtual hardware. The most significant
> > > > > +     * implication of this is a requirement of special handling of the
> > > > > +     * cursor plane (e.g. cursor plane has to actually track the mouse
> > > > > +     * cursor and the clients are required to set hotspot in order for
> > > > > +     * the cursor planes to work correctly).
> > > > > +     */
> > > > > +    DRIVER_VIRTUAL                  = BIT(7),  
> > > > 
> > > > I think the naming here is unfortunate, because people will vonder why
> > > > e.g. vkms doesn't set this, and then add it, and confuse stuff completely.
> > > > 
> > > > Also it feels a bit wrong to put this onto the driver, when really it's a
> > > > cursor flag. I guess you can make it some kind of flag in the drm_plane
> > > > structure, or a new plane type, but putting it there instead of into the
> > > > "random pile of midlayer-mistake driver flags" would be a lot better.
> > > > 
> > > > Otherwise I think the series looks roughly how I'd expect it to look.
> > > > -Daniel
> > > >   
> > > 
> > > AFAICT this is the only remaining thing to be addressed for this series ?  
> > 
> > No, there was more. tbh I haven't had the time to think about whether the above
> > makes sense to me, e.g. I'm not sure if having virtualized drivers expose
> > "support
> > universal planes" and adding another plane which is not universal (the only
> > "universal" plane on them being the default one) makes more sense than a flag
> > that
> > says "this driver requires a cursor in the cursor plane". There's certainly a
> > huge
> > difference in how userspace would be required to handle it and it's way uglier
> > with
> > two different cursor planes. i.e. there's a lot of ways in which this could be
> > cleaner in the kernel but they all require significant changes to userspace,
> > that go
> > way beyond "attach hotspot info to this plane".
> 
> > I'd like to avoid approaches that
> > mean running with atomic kms requires completely separate paths for virtualized
> > drivers because no one will ever support and maintain it.
> 
> Hi Zack,
> 
> you'd like to avoid that, but fundamentally that really is what has to
> happen in userspace for *nested* KMS drivers (VKMS is a virtual driver
> but not part of the interest group here) to reach optimality.
> 
> It really is a different path. I see no way around that. But if you
> accept that fact, then you could possibly gain a lot more benefits by
> asking userspace to handle nested KMS drivers differently. What those
> benefits are exactly I'm not sure, but I have a feeling there should be
> some, where the knowledge of running on a nested KMS driver allows for
> better decisions that are not possible if the nested KMS driver just
> pretends to be like any other KMS hardware driver.

I'm not quite sure I buy the "virtualized drivers return immediately from a flip and
require two extra integers on the cursor plane, so there's no possible way drm api
could handle that" argument because it seems rather flimsy. If the premise is that
the paravirtualized drivers are so different that drm uapi can not handle them then
why would they stay in drm? What's the benefit if they'll have their own uapi and
their own interfaces?

I'd flip the argument and say you'd be a lot happier if you accepted that universal
planes aren't really universal no matter what. If weston puts a spreadsheet app in a
cursor plane presumably users would be concerned that their mouse cursor disappears
when they enter the spreadsheet app and responding to their concerns with "it's
cool, the planes are universal" wouldn't quite work. Something needs to special case
cursor plane no matter what. Anyway, I think we went through this argument of what
exactly "universal" refers to and whatever the definition of it is why I don't see
why two extra integers on cursor planes violates any part of it so I'll let it go. 

I have nothing against any of those solutions - from creating whole new kms uapi for
paravirtualized drivers, through creating a new subsystem for paravirtualized
drivers. I just have no interest in implementing those myself right now but if
someone implemented mutter/kwin code on top of either a new subsystem for
paravirtualized drivers or implemented atomic kms on top of some new api's for them,
we'll be sure to port vmwgfx to that. 

z

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

[Zack Rusin]

On Wed, 2023-05-03 at 09:48 +0200, Javier Martinez Canillas wrote:
> Zack Rusin <zackr@vmware.com> writes:
> 
> > On Tue, 2023-05-02 at 11:32 +0200, Javier Martinez Canillas wrote:
> > > !! External Email
> > > 
> > > Daniel Vetter <daniel@ffwll.ch> writes:
> > > 
> > > > On Mon, Jul 11, 2022 at 11:32:39PM -0400, Zack Rusin wrote:
> > > > > From: Zack Rusin <zackr@vmware.com>
> > > > > 
> > > > > Cursor planes on virtualized drivers have special meaning and require
> > > > > that the clients handle them in specific ways, e.g. the cursor plane
> > > > > should react to the mouse movement the way a mouse cursor would be
> > > > > expected to and the client is required to set hotspot properties on it
> > > > > in order for the mouse events to be routed correctly.
> > > > > 
> > > > > This breaks the contract as specified by the "universal planes". Fix it
> > > > > by disabling the cursor planes on virtualized drivers while adding
> > > > > a foundation on top of which it's possible to special case mouse cursor
> > > > > planes for clients that want it.
> > > > > 
> > > > > Disabling the cursor planes makes some kms compositors which were broken,
> > > > > e.g. Weston, fallback to software cursor which works fine or at least
> > > > > better than currently while having no effect on others, e.g. gnome-shell
> > > > > or kwin, which put virtualized drivers on a deny-list when running in
> > > > > atomic context to make them fallback to legacy kms and avoid this issue.
> > > > > 
> > > > > Signed-off-by: Zack Rusin <zackr@vmware.com>
> > > > > Fixes: 681e7ec73044 ("drm: Allow userspace to ask for universal plane list
> > > > > (v2)")
> > > 
> > > [...]
> > > 
> > > > > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> > > > > index f6159acb8856..c4cd7fc350d9 100644
> > > > > --- a/include/drm/drm_drv.h
> > > > > +++ b/include/drm/drm_drv.h
> > > > > @@ -94,6 +94,16 @@ enum drm_driver_feature {
> > > > >       * synchronization of command submission.
> > > > >       */
> > > > >      DRIVER_SYNCOBJ_TIMELINE         = BIT(6),
> > > > > +    /**
> > > > > +     * @DRIVER_VIRTUAL:
> > > > > +     *
> > > > > +     * Driver is running on top of virtual hardware. The most significant
> > > > > +     * implication of this is a requirement of special handling of the
> > > > > +     * cursor plane (e.g. cursor plane has to actually track the mouse
> > > > > +     * cursor and the clients are required to set hotspot in order for
> > > > > +     * the cursor planes to work correctly).
> > > > > +     */
> > > > > +    DRIVER_VIRTUAL                  = BIT(7),
> > > > 
> > > > I think the naming here is unfortunate, because people will vonder why
> > > > e.g. vkms doesn't set this, and then add it, and confuse stuff completely.
> > > > 
> > > > Also it feels a bit wrong to put this onto the driver, when really it's a
> > > > cursor flag. I guess you can make it some kind of flag in the drm_plane
> > > > structure, or a new plane type, but putting it there instead of into the
> > > > "random pile of midlayer-mistake driver flags" would be a lot better.
> > > > 
> > > > Otherwise I think the series looks roughly how I'd expect it to look.
> > > > -Daniel
> > > > 
> > > 
> > > AFAICT this is the only remaining thing to be addressed for this series ?
> > 
> > No, there was more. tbh I haven't had the time to think about whether the above
> > makes sense to me, e.g. I'm not sure if having virtualized drivers expose
> > "support
> > universal planes" and adding another plane which is not universal (the only
> > "universal" plane on them being the default one) makes more sense than a flag
> > that
> > says "this driver requires a cursor in the cursor plane". There's certainly a
> > huge
> > difference in how userspace would be required to handle it and it's way uglier
> > with
> > two different cursor planes. i.e. there's a lot of ways in which this could be
> > cleaner in the kernel but they all require significant changes to userspace,
> > that go
> > way beyond "attach hotspot info to this plane". I'd like to avoid approaches
> > that
> > mean running with atomic kms requires completely separate paths for virtualized
> > drivers because no one will ever support and maintain it.
> > 
> > It's not a trivial thing because it's fundamentally hard to untangle the fact
> > the
> > virtualized drivers have been advertising universal plane support without ever
> > supporting universal planes. Especially because most new userspace in general
> > checks
> > for "universal planes" to expose atomic kms paths.
> > 
> 
> After some discussion on the #dri-devel, your approach makes sense and the
> only contention point is the name of the driver feature flag name. The one
> you are using (DRIVER_VIRTUAL) seems to be too broad and generic (the fact
> that vkms won't set and is a virtual driver as well, is a good example).
> 
> Maybe something like DRIVER_CURSOR_HOTSPOT or DRIVER_CURSOR_COMMANDEERING
> would be more accurate and self explanatory ?

Sure, or even more verbose DRIVER_NEEDS_CURSOR_PLANE_HOTSPOT, but it sounds like
Pekka doesn't agree with this approach. As I mentioned in my response to him, I'd be
happy with any approach that gets paravirtualized drivers working with atomic kms,
but atm I don't have enough time to be creating a new kernel subsystem or a new set
of uapi's for paravirtualized drivers and then porting mutter/kwin to those.

z

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

[Pekka Paalanen]

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

On Thu, 4 May 2023 01:43:51 +0000
Zack Rusin <zackr@vmware.com> wrote:

> On Wed, 2023-05-03 at 10:54 +0300, Pekka Paalanen wrote:
> > On Wed, 3 May 2023 03:35:29 +0000
> > Zack Rusin <zackr@vmware.com> wrote:
> >   
> > > On Tue, 2023-05-02 at 11:32 +0200, Javier Martinez Canillas wrote:  
> > > > !! External Email
> > > > 
> > > > Daniel Vetter <daniel@ffwll.ch> writes:
> > > >     
> > > > > On Mon, Jul 11, 2022 at 11:32:39PM -0400, Zack Rusin wrote:    
> > > > > > From: Zack Rusin <zackr@vmware.com>
> > > > > > 
> > > > > > Cursor planes on virtualized drivers have special meaning and require
> > > > > > that the clients handle them in specific ways, e.g. the cursor plane
> > > > > > should react to the mouse movement the way a mouse cursor would be
> > > > > > expected to and the client is required to set hotspot properties on it
> > > > > > in order for the mouse events to be routed correctly.
> > > > > > 
> > > > > > This breaks the contract as specified by the "universal planes". Fix it
> > > > > > by disabling the cursor planes on virtualized drivers while adding
> > > > > > a foundation on top of which it's possible to special case mouse cursor
> > > > > > planes for clients that want it.
> > > > > > 
> > > > > > Disabling the cursor planes makes some kms compositors which were broken,
> > > > > > e.g. Weston, fallback to software cursor which works fine or at least
> > > > > > better than currently while having no effect on others, e.g. gnome-shell
> > > > > > or kwin, which put virtualized drivers on a deny-list when running in
> > > > > > atomic context to make them fallback to legacy kms and avoid this issue.
> > > > > > 
> > > > > > Signed-off-by: Zack Rusin <zackr@vmware.com>
> > > > > > Fixes: 681e7ec73044 ("drm: Allow userspace to ask for universal plane list
> > > > > > (v2)")    
> > > > 
> > > > [...]
> > > >     
> > > > > > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> > > > > > index f6159acb8856..c4cd7fc350d9 100644
> > > > > > --- a/include/drm/drm_drv.h
> > > > > > +++ b/include/drm/drm_drv.h
> > > > > > @@ -94,6 +94,16 @@ enum drm_driver_feature {
> > > > > >       * synchronization of command submission.
> > > > > >       */
> > > > > >      DRIVER_SYNCOBJ_TIMELINE         = BIT(6),
> > > > > > +    /**
> > > > > > +     * @DRIVER_VIRTUAL:
> > > > > > +     *
> > > > > > +     * Driver is running on top of virtual hardware. The most significant
> > > > > > +     * implication of this is a requirement of special handling of the
> > > > > > +     * cursor plane (e.g. cursor plane has to actually track the mouse
> > > > > > +     * cursor and the clients are required to set hotspot in order for
> > > > > > +     * the cursor planes to work correctly).
> > > > > > +     */
> > > > > > +    DRIVER_VIRTUAL                  = BIT(7),    
> > > > > 
> > > > > I think the naming here is unfortunate, because people will vonder why
> > > > > e.g. vkms doesn't set this, and then add it, and confuse stuff completely.
> > > > > 
> > > > > Also it feels a bit wrong to put this onto the driver, when really it's a
> > > > > cursor flag. I guess you can make it some kind of flag in the drm_plane
> > > > > structure, or a new plane type, but putting it there instead of into the
> > > > > "random pile of midlayer-mistake driver flags" would be a lot better.
> > > > > 
> > > > > Otherwise I think the series looks roughly how I'd expect it to look.
> > > > > -Daniel
> > > > >     
> > > > 
> > > > AFAICT this is the only remaining thing to be addressed for this series ?    
> > > 
> > > No, there was more. tbh I haven't had the time to think about whether the above
> > > makes sense to me, e.g. I'm not sure if having virtualized drivers expose
> > > "support
> > > universal planes" and adding another plane which is not universal (the only
> > > "universal" plane on them being the default one) makes more sense than a flag
> > > that
> > > says "this driver requires a cursor in the cursor plane". There's certainly a
> > > huge
> > > difference in how userspace would be required to handle it and it's way uglier
> > > with
> > > two different cursor planes. i.e. there's a lot of ways in which this could be
> > > cleaner in the kernel but they all require significant changes to userspace,
> > > that go
> > > way beyond "attach hotspot info to this plane".  
> >   
> > > I'd like to avoid approaches that
> > > mean running with atomic kms requires completely separate paths for virtualized
> > > drivers because no one will ever support and maintain it.  
> > 
> > Hi Zack,
> > 
> > you'd like to avoid that, but fundamentally that really is what has to
> > happen in userspace for *nested* KMS drivers (VKMS is a virtual driver
> > but not part of the interest group here) to reach optimality.
> > 
> > It really is a different path. I see no way around that. But if you
> > accept that fact, then you could possibly gain a lot more benefits by
> > asking userspace to handle nested KMS drivers differently. What those
> > benefits are exactly I'm not sure, but I have a feeling there should be
> > some, where the knowledge of running on a nested KMS driver allows for
> > better decisions that are not possible if the nested KMS driver just
> > pretends to be like any other KMS hardware driver.  
> 
> I'm not quite sure I buy the "virtualized drivers return immediately from a flip and
> require two extra integers on the cursor plane, so there's no possible way drm api
> could handle that" argument because it seems rather flimsy. If the premise is that
> the paravirtualized drivers are so different that drm uapi can not handle them then
> why would they stay in drm? What's the benefit if they'll have their own uapi and
> their own interfaces?

Hi Zack,

I never implied to go that far as you make it sound here.

I only pointed out that there definitely are behavioral differences
that userspace MUST acknowledge and handle. The cursor plane being
special is just the start.

It does not invalidate the whole existing KMS UAPI, but *if* you aim
for optimal performance, you cannot ignore the differences or paper
over them either.

This NOT a NAK to this patch series in any way!

> I'd flip the argument and say you'd be a lot happier if you accepted that universal
> planes aren't really universal no matter what. If weston puts a spreadsheet app in a
> cursor plane presumably users would be concerned that their mouse cursor disappears
> when they enter the spreadsheet app and responding to their concerns with "it's
> cool, the planes are universal" wouldn't quite work. Something needs to special case
> cursor plane no matter what. Anyway, I think we went through this argument of what
> exactly "universal" refers to and whatever the definition of it is why I don't see
> why two extra integers on cursor planes violates any part of it so I'll let it go. 

If you say so.

But then you need userspace to set those two integers. The concept of
those two integers does not even exist without explicit care in
userspace. You are already letting go of your goal to not need special
case code or changes in userspace like you said in (copied from above):

> > > I'd like to avoid approaches that
> > > mean running with atomic kms requires completely separate paths for virtualized
> > > drivers because no one will ever support and maintain it.  

You want to make cursor planes special and not universal, which means
userspace needs special code to use them at all. That's a separate code
path. That is good! That is the way to get more performance through
better userspace decisions.

> I have nothing against any of those solutions - from creating whole new kms uapi for
> paravirtualized drivers, through creating a new subsystem for paravirtualized
> drivers. I just have no interest in implementing those myself right now but if
> someone implemented mutter/kwin code on top of either a new subsystem for
> paravirtualized drivers or implemented atomic kms on top of some new api's for them,
> we'll be sure to port vmwgfx to that. 

Again, I said half a meter, but you are jumping a mile.

I think you are seeing my comments as NAKs towards everything you try
to do. That's not my intention, and I cannot NAK anything anyway. I am
only pointing out that you seem too fixed to these details to see the
rest of the problems that hinder performance of nested KMS drivers.

See for example
https://gitlab.freedesktop.org/wayland/weston/-/issues/514

It's a nested KMS use case with qemu (would vmwgfx be any different?),
where the goal is to have zero-copy direct scanout from guest
application through guest KMS planes, qemu, host Weston onto host KMS
planes. The direct scanout works, but due to the UAPI design I
explained in my previous email, the only way to make that actually run
at ~full framerate is to very carefully manually tune the timings of
both guest and host Weston instance.

If you deem the cost of gaining that performance too high, that's fine.

However, I do absolutely believe, that if you wanted it, any Wayland
compositor project that aims to be in any way a performant general
purpose compositor for desktops or more would be willing to follow you
in using new UAPI that would make life better with nested KMS drivers.
You just have to ask, instead of assume that userspace won't hear you.
After all, working better is in their interest as well.

I *am* trying to push you forward, not stop you.

Sorry, I'll try to keep quiet from now on since I don't have any stakes
here.


Thanks,
pq

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

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

[Pekka Paalanen]

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

On Thu, 4 May 2023 01:50:25 +0000
Zack Rusin <zackr@vmware.com> wrote:

> On Wed, 2023-05-03 at 09:48 +0200, Javier Martinez Canillas wrote:
> > Zack Rusin <zackr@vmware.com> writes:
> >   
> > > On Tue, 2023-05-02 at 11:32 +0200, Javier Martinez Canillas wrote:  

> > > > AFAICT this is the only remaining thing to be addressed for this series ?  
> > > 
> > > No, there was more. tbh I haven't had the time to think about whether the above
> > > makes sense to me, e.g. I'm not sure if having virtualized drivers expose
> > > "support
> > > universal planes" and adding another plane which is not universal (the only
> > > "universal" plane on them being the default one) makes more sense than a flag
> > > that
> > > says "this driver requires a cursor in the cursor plane". There's certainly a
> > > huge
> > > difference in how userspace would be required to handle it and it's way uglier
> > > with
> > > two different cursor planes. i.e. there's a lot of ways in which this could be
> > > cleaner in the kernel but they all require significant changes to userspace,
> > > that go
> > > way beyond "attach hotspot info to this plane". I'd like to avoid approaches
> > > that
> > > mean running with atomic kms requires completely separate paths for virtualized
> > > drivers because no one will ever support and maintain it.
> > > 
> > > It's not a trivial thing because it's fundamentally hard to untangle the fact
> > > the
> > > virtualized drivers have been advertising universal plane support without ever
> > > supporting universal planes. Especially because most new userspace in general
> > > checks
> > > for "universal planes" to expose atomic kms paths.
> > >   
> > 
> > After some discussion on the #dri-devel, your approach makes sense and the
> > only contention point is the name of the driver feature flag name. The one
> > you are using (DRIVER_VIRTUAL) seems to be too broad and generic (the fact
> > that vkms won't set and is a virtual driver as well, is a good example).
> > 
> > Maybe something like DRIVER_CURSOR_HOTSPOT or DRIVER_CURSOR_COMMANDEERING
> > would be more accurate and self explanatory ?  
> 
> Sure, or even more verbose DRIVER_NEEDS_CURSOR_PLANE_HOTSPOT, but it sounds like
> Pekka doesn't agree with this approach. As I mentioned in my response to him, I'd be
> happy with any approach that gets paravirtualized drivers working with atomic kms,
> but atm I don't have enough time to be creating a new kernel subsystem or a new set
> of uapi's for paravirtualized drivers and then porting mutter/kwin to those.

It seems I have not been clear enough, apologies. Once more, in short:

Zack, I'm worried about this statement from you (copied from above):

> > > I'd like to avoid approaches that mean running with atomic kms
> > > requires completely separate paths for virtualized drivers
> > > because no one will ever support and maintain it.

It feels like you are intentionally limiting your own design options
for the fear of "no one will ever support it". I'm worried that over
the coming years, that will lead to a hard to use, hard to maintain
patchwork of vague or undocumented or just too many little UAPI details.

Please, don't limit your designs. There are good reasons why nested KMS
drivers behave fundamentally differently to most KMS hardware drivers.
Userspace that does not or cannot take that into account is unavoidably
crippled.


Thanks,
pq

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

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

[Jonas Ådahl]

On Thu, May 04, 2023 at 01:39:04PM +0300, Pekka Paalanen wrote:
> On Thu, 4 May 2023 01:50:25 +0000
> Zack Rusin <zackr@vmware.com> wrote:
> 
> > On Wed, 2023-05-03 at 09:48 +0200, Javier Martinez Canillas wrote:
> > > Zack Rusin <zackr@vmware.com> writes:
> > >   
> > > > On Tue, 2023-05-02 at 11:32 +0200, Javier Martinez Canillas wrote:  
> 
> > > > > AFAICT this is the only remaining thing to be addressed for this series ?  
> > > > 
> > > > No, there was more. tbh I haven't had the time to think about whether the above
> > > > makes sense to me, e.g. I'm not sure if having virtualized drivers expose
> > > > "support
> > > > universal planes" and adding another plane which is not universal (the only
> > > > "universal" plane on them being the default one) makes more sense than a flag
> > > > that
> > > > says "this driver requires a cursor in the cursor plane". There's certainly a
> > > > huge
> > > > difference in how userspace would be required to handle it and it's way uglier
> > > > with
> > > > two different cursor planes. i.e. there's a lot of ways in which this could be
> > > > cleaner in the kernel but they all require significant changes to userspace,
> > > > that go
> > > > way beyond "attach hotspot info to this plane". I'd like to avoid approaches
> > > > that
> > > > mean running with atomic kms requires completely separate paths for virtualized
> > > > drivers because no one will ever support and maintain it.
> > > > 
> > > > It's not a trivial thing because it's fundamentally hard to untangle the fact
> > > > the
> > > > virtualized drivers have been advertising universal plane support without ever
> > > > supporting universal planes. Especially because most new userspace in general
> > > > checks
> > > > for "universal planes" to expose atomic kms paths.
> > > >   
> > > 
> > > After some discussion on the #dri-devel, your approach makes sense and the
> > > only contention point is the name of the driver feature flag name. The one
> > > you are using (DRIVER_VIRTUAL) seems to be too broad and generic (the fact
> > > that vkms won't set and is a virtual driver as well, is a good example).
> > > 
> > > Maybe something like DRIVER_CURSOR_HOTSPOT or DRIVER_CURSOR_COMMANDEERING
> > > would be more accurate and self explanatory ?  
> > 
> > Sure, or even more verbose DRIVER_NEEDS_CURSOR_PLANE_HOTSPOT, but it sounds like
> > Pekka doesn't agree with this approach. As I mentioned in my response to him, I'd be
> > happy with any approach that gets paravirtualized drivers working with atomic kms,
> > but atm I don't have enough time to be creating a new kernel subsystem or a new set
> > of uapi's for paravirtualized drivers and then porting mutter/kwin to those.
> 
> It seems I have not been clear enough, apologies. Once more, in short:
> 
> Zack, I'm worried about this statement from you (copied from above):
> 
> > > > I'd like to avoid approaches that mean running with atomic kms
> > > > requires completely separate paths for virtualized drivers
> > > > because no one will ever support and maintain it.
> 
> It feels like you are intentionally limiting your own design options
> for the fear of "no one will ever support it". I'm worried that over
> the coming years, that will lead to a hard to use, hard to maintain
> patchwork of vague or undocumented or just too many little UAPI details.
> 
> Please, don't limit your designs. There are good reasons why nested KMS
> drivers behave fundamentally differently to most KMS hardware drivers.
> Userspace that does not or cannot take that into account is unavoidably
> crippled.

From a compositor side, there is a valid reason to minimize the uAPI
difference between "nested virtual machine" code paths and "running on
actual hardware" code paths, which is to let virtual machines with a
viewer connected to KMS act as a testing environment, rather than a
production environment. Running a production environment in a virtual
machine doesn't really need to use KMS at all.

When using virtual machines for testing, I want to minimize the amount
of differentation between running on hardware and running in the VM
because otherwise the parts that are tested are not the same.

I realize that hotpspots and the cursor moving viewer side contradicts
that to some degree, but still, from a graphical testing witha VM point
of view, one has to compromise, as testing isn't just for the KMS layer,
but for the DE and distribution as a whole.


Jonas

> 
> 
> Thanks,
> pq

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

[Pekka Paalanen]

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

On Thu, 4 May 2023 13:27:22 +0200
Jonas Ådahl <jadahl@gmail.com> wrote:

> On Thu, May 04, 2023 at 01:39:04PM +0300, Pekka Paalanen wrote:
> > On Thu, 4 May 2023 01:50:25 +0000
> > Zack Rusin <zackr@vmware.com> wrote:
> >   
> > > On Wed, 2023-05-03 at 09:48 +0200, Javier Martinez Canillas wrote:  
> > > > Zack Rusin <zackr@vmware.com> writes:
> > > >     
> > > > > On Tue, 2023-05-02 at 11:32 +0200, Javier Martinez Canillas wrote:    
> >   
> > > > > > AFAICT this is the only remaining thing to be addressed for this series ?    
> > > > > 
> > > > > No, there was more. tbh I haven't had the time to think about whether the above
> > > > > makes sense to me, e.g. I'm not sure if having virtualized drivers expose
> > > > > "support
> > > > > universal planes" and adding another plane which is not universal (the only
> > > > > "universal" plane on them being the default one) makes more sense than a flag
> > > > > that
> > > > > says "this driver requires a cursor in the cursor plane". There's certainly a
> > > > > huge
> > > > > difference in how userspace would be required to handle it and it's way uglier
> > > > > with
> > > > > two different cursor planes. i.e. there's a lot of ways in which this could be
> > > > > cleaner in the kernel but they all require significant changes to userspace,
> > > > > that go
> > > > > way beyond "attach hotspot info to this plane". I'd like to avoid approaches
> > > > > that
> > > > > mean running with atomic kms requires completely separate paths for virtualized
> > > > > drivers because no one will ever support and maintain it.
> > > > > 
> > > > > It's not a trivial thing because it's fundamentally hard to untangle the fact
> > > > > the
> > > > > virtualized drivers have been advertising universal plane support without ever
> > > > > supporting universal planes. Especially because most new userspace in general
> > > > > checks
> > > > > for "universal planes" to expose atomic kms paths.
> > > > >     
> > > > 
> > > > After some discussion on the #dri-devel, your approach makes sense and the
> > > > only contention point is the name of the driver feature flag name. The one
> > > > you are using (DRIVER_VIRTUAL) seems to be too broad and generic (the fact
> > > > that vkms won't set and is a virtual driver as well, is a good example).
> > > > 
> > > > Maybe something like DRIVER_CURSOR_HOTSPOT or DRIVER_CURSOR_COMMANDEERING
> > > > would be more accurate and self explanatory ?    
> > > 
> > > Sure, or even more verbose DRIVER_NEEDS_CURSOR_PLANE_HOTSPOT, but it sounds like
> > > Pekka doesn't agree with this approach. As I mentioned in my response to him, I'd be
> > > happy with any approach that gets paravirtualized drivers working with atomic kms,
> > > but atm I don't have enough time to be creating a new kernel subsystem or a new set
> > > of uapi's for paravirtualized drivers and then porting mutter/kwin to those.  
> > 
> > It seems I have not been clear enough, apologies. Once more, in short:
> > 
> > Zack, I'm worried about this statement from you (copied from above):
> >   
> > > > > I'd like to avoid approaches that mean running with atomic kms
> > > > > requires completely separate paths for virtualized drivers
> > > > > because no one will ever support and maintain it.  
> > 
> > It feels like you are intentionally limiting your own design options
> > for the fear of "no one will ever support it". I'm worried that over
> > the coming years, that will lead to a hard to use, hard to maintain
> > patchwork of vague or undocumented or just too many little UAPI details.
> > 
> > Please, don't limit your designs. There are good reasons why nested KMS
> > drivers behave fundamentally differently to most KMS hardware drivers.
> > Userspace that does not or cannot take that into account is unavoidably
> > crippled.  
> 
> From a compositor side, there is a valid reason to minimize the uAPI
> difference between "nested virtual machine" code paths and "running on
> actual hardware" code paths, which is to let virtual machines with a
> viewer connected to KMS act as a testing environment, rather than a
> production environment. Running a production environment in a virtual
> machine doesn't really need to use KMS at all.
> 
> When using virtual machines for testing, I want to minimize the amount
> of differentation between running on hardware and running in the VM
> because otherwise the parts that are tested are not the same.
> 
> I realize that hotpspots and the cursor moving viewer side contradicts
> that to some degree, but still, from a graphical testing witha VM point
> of view, one has to compromise, as testing isn't just for the KMS layer,
> but for the DE and distribution as a whole.

Right, I'm looking at this from the production use only point of view,
and not as any kind of testing environment, not for compositor KMS
driving bits at least. Using a virtualized driver for KMS testing seems
so very... manual to me, and like you said, it's not representative of
"real" behaviour.

As for the best choice for production use, KMS in guest OS is
attractive because it offers zero-copy direct scanout to the host
hardware, with the right stack. OTOH, I think RDP has extensions that
could enable that too, and if the end point is not host hardware
display then KMS use in guest is not the best idea indeed.

I don't recall any mention of actual use cases here recently. I agree
the intended use makes a huge difference. Testing KMS userspace and
production use are almost the opposite goals for virtualized drivers.


Thanks,
pq

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