LinuxLists.cc - [PATCH v6 0/9] Fix cursor planes with virtualized drivers

2023-10-23 07:47:55

Subject: [PATCH v6 0/9] Fix cursor planes with virtualized drivers

v6: Shift DRIVER_CURSOR_HOTSPOT flag bit to BIT(9), since BIT(8)
was already taken by DRIVER_GEM_GPUVA.

v5: Add a change with documentation from Michael, based on his discussion
with Pekka and bump the kernel version DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT
might be introduced with to 6.6.

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

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

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

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

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

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

Link to the IGT test covering this patch (already merged):
https://lists.freedesktop.org/archives/igt-dev/2023-July/058427.html

Mutter patch:
https://lists.freedesktop.org/archives/igt-dev/2023-July/058427.html

Michael Banack (1):
drm: Introduce documentation for hotspot properties

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

Documentation/gpu/drm-kms.rst | 6 ++
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 | 120 +++++++++++++++++++++-
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 | 2 +-
drivers/gpu/drm/virtio/virtgpu_plane.c | 8 +-
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 2 +-
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 9 +-
include/drm/drm_drv.h | 9 ++
include/drm/drm_file.h | 12 +++
include/drm/drm_framebuffer.h | 12 ---
include/drm/drm_plane.h | 14 +++
include/uapi/drm/drm.h | 25 +++++
18 files changed, 245 insertions(+), 39 deletions(-)

--
2.41.0

2023-10-23 07:47:55

by Albert Esteve

[permalink] [raw]

Subject: [PATCH v6 2/9] drm/atomic: Add support for mouse hotspots

From: Zack Rusin <[email protected]>

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 <[email protected]>
Cc: Maarten Lankhorst <[email protected]>
Cc: Maxime Ripard <[email protected]>
Cc: Thomas Zimmermann <[email protected]>
Cc: David Airlie <[email protected]>
Cc: Daniel Vetter <[email protected]>
Reviewed-by: Javier Martinez Canillas <[email protected]>
---
drivers/gpu/drm/drm_atomic_state_helper.c | 14 +++++++
drivers/gpu/drm/drm_atomic_uapi.c | 20 +++++++++
drivers/gpu/drm/drm_plane.c | 50 +++++++++++++++++++++++
include/drm/drm_plane.h | 14 +++++++
4 files changed, 98 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index 784e63d70a421..54975de44a0e3 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 98d3b10c08ae1..07a7b3f18df26 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -593,6 +593,22 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
} else if (plane->funcs->atomic_set_property) {
return plane->funcs->atomic_set_property(plane, state,
property, val);
+ } else if (property == plane->hotspot_x_property) {
+ if (plane->type != DRM_PLANE_TYPE_CURSOR) {
+ drm_dbg_atomic(plane->dev,
+ "[PLANE:%d:%s] is not a cursor plane: 0x%llx\n",
+ plane->base.id, plane->name, val);
+ return -EINVAL;
+ }
+ state->hotspot_x = val;
+ } else if (property == plane->hotspot_y_property) {
+ if (plane->type != DRM_PLANE_TYPE_CURSOR) {
+ drm_dbg_atomic(plane->dev,
+ "[PLANE:%d:%s] is not a cursor plane: 0x%llx\n",
+ plane->base.id, plane->name, val);
+ return -EINVAL;
+ }
+ state->hotspot_y = val;
} else {
drm_dbg_atomic(plane->dev,
"[PLANE:%d:%s] unknown property [PROP:%d:%s]\n",
@@ -653,6 +669,10 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
*val = state->scaling_filter;
} else if (plane->funcs->atomic_get_property) {
return plane->funcs->atomic_get_property(plane, state, property, val);
+ } else if (property == plane->hotspot_x_property) {
+ *val = state->hotspot_x;
+ } else if (property == plane->hotspot_y_property) {
+ *val = state->hotspot_y;
} else {
drm_dbg_atomic(dev,
"[PLANE:%d:%s] unknown property [PROP:%d:%s]\n",
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index c6bbb0c209f47..eaca367bdc7e7 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -230,6 +230,47 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
return 0;
}

+/**
+ * drm_plane_create_hotspot_properties - creates the mouse hotspot
+ * properties and attaches them to the given cursor plane
+ *
+ * @plane: drm cursor plane
+ *
+ * This function enables the mouse hotspot property on a given
+ * cursor plane.
+ *
+ * RETURNS:
+ * Zero for success or -errno
+ */
+static int drm_plane_create_hotspot_properties(struct drm_plane *plane)
+{
+ struct drm_property *prop_x;
+ struct drm_property *prop_y;
+
+ drm_WARN_ON(plane->dev,
+ !drm_core_check_feature(plane->dev,
+ DRIVER_CURSOR_HOTSPOT));
+
+ prop_x = drm_property_create_signed_range(plane->dev, 0, "HOTSPOT_X",
+ INT_MIN, INT_MAX);
+ if (IS_ERR(prop_x))
+ return PTR_ERR(prop_x);
+
+ prop_y = drm_property_create_signed_range(plane->dev, 0, "HOTSPOT_Y",
+ INT_MIN, INT_MAX);
+ if (IS_ERR(prop_y)) {
+ drm_property_destroy(plane->dev, prop_x);
+ return PTR_ERR(prop_y);
+ }
+
+ drm_object_attach_property(&plane->base, prop_x, 0);
+ drm_object_attach_property(&plane->base, prop_y, 0);
+ plane->hotspot_x_property = prop_x;
+ plane->hotspot_y_property = prop_y;
+
+ return 0;
+}
+
__printf(9, 0)
static int __drm_universal_plane_init(struct drm_device *dev,
struct drm_plane *plane,
@@ -348,6 +389,10 @@ static int __drm_universal_plane_init(struct drm_device *dev,
drm_object_attach_property(&plane->base, config->prop_src_w, 0);
drm_object_attach_property(&plane->base, config->prop_src_h, 0);
}
+ if (drm_core_check_feature(dev, DRIVER_CURSOR_HOTSPOT) &&
+ type == DRM_PLANE_TYPE_CURSOR) {
+ drm_plane_create_hotspot_properties(plane);
+ }

if (format_modifier_count)
create_in_format_blob(dev, plane);
@@ -1067,6 +1112,11 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc,

fb->hot_x = req->hot_x;
fb->hot_y = req->hot_y;
+
+ if (plane->hotspot_x_property && plane->state)
+ plane->state->hotspot_x = req->hot_x;
+ if (plane->hotspot_y_property && plane->state)
+ plane->state->hotspot_y = req->hot_y;
} else {
fb = NULL;
}
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 79d62856defbf..e2c671585775b 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -116,6 +116,10 @@ struct drm_plane_state {
/** @src_h: height of visible portion of plane (in 16.16) */
uint32_t src_h, src_w;

+ /** @hotspot_x: x offset to mouse cursor hotspot */
+ /** @hotspot_y: y offset to mouse cursor hotspot */
+ int32_t hotspot_x, hotspot_y;
+
/**
* @alpha:
* Opacity of the plane with 0 as completely transparent and 0xffff as
@@ -748,6 +752,16 @@ struct drm_plane {
* scaling.
*/
struct drm_property *scaling_filter_property;
+
+ /**
+ * @hotspot_x_property: property to set mouse hotspot x offset.
+ */
+ struct drm_property *hotspot_x_property;
+
+ /**
+ * @hotspot_y_property: property to set mouse hotspot y offset.
+ */
+ struct drm_property *hotspot_y_property;
};

#define obj_to_plane(x) container_of(x, struct drm_plane, base)
--
2.41.0

2023-10-23 07:48:09

by Albert Esteve

[permalink] [raw]

Subject: [PATCH v6 6/9] drm/virtio: Use the hotspot properties from cursor planes

2023-10-23 07:48:20

by Albert Esteve

[permalink] [raw]

Subject: [PATCH v6 8/9] drm: Introduce DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT

From: Zack Rusin <[email protected]>

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

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

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

Signed-off-by: Zack Rusin <[email protected]>
Cc: Maarten Lankhorst <[email protected]>
Cc: Maxime Ripard <[email protected]>
Cc: Thomas Zimmermann <[email protected]>
Cc: David Airlie <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: [email protected]
Acked-by: Pekka Paalanen <[email protected]>
Reviewed-by: Javier Martinez Canillas <[email protected]>
---
drivers/gpu/drm/drm_ioctl.c | 9 +++++++++
include/uapi/drm/drm.h | 25 +++++++++++++++++++++++++
2 files changed, 34 insertions(+)

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index f03ffbacfe9b4..e535b58521533 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -361,6 +361,15 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
return -EINVAL;
file_priv->writeback_connectors = req->value;
break;
+ case DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT:
+ if (!drm_core_check_feature(dev, DRIVER_CURSOR_HOTSPOT))
+ return -EOPNOTSUPP;
+ if (!file_priv->atomic)
+ return -EINVAL;
+ if (req->value > 1)
+ return -EINVAL;
+ file_priv->supports_virtualized_cursor_plane = req->value;
+ break;
default:
return -EINVAL;
}
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 794c1d857677d..fc0c267f3f3ed 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -842,6 +842,31 @@ struct drm_get_cap {
*/
#define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS 5

+/**
+ * DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT
+ *
+ * Drivers for para-virtualized hardware (e.g. vmwgfx, qxl, virtio and
+ * virtualbox) have additional restrictions for cursor planes (thus
+ * making cursor planes on those drivers not truly universal,) e.g.
+ * they need cursor planes to act like one would expect from a mouse
+ * cursor and have correctly set hotspot properties.
+ * If this client cap is not set the DRM core will hide cursor plane on
+ * those virtualized drivers because not setting it implies that the
+ * client is not capable of dealing with those extra restictions.
+ * Clients which do set cursor hotspot and treat the cursor plane
+ * like a mouse cursor should set this property.
+ * The client must enable &DRM_CLIENT_CAP_ATOMIC first.
+ *
+ * Setting this property on drivers which do not special case
+ * cursor planes (i.e. non-virtualized drivers) will return
+ * EOPNOTSUPP, which can be used by userspace to gauge
+ * requirements of the hardware/drivers they're running on.
+ *
+ * This capability is always supported for atomic-capable virtualized
+ * drivers starting from kernel version 6.6.
+ */
+#define DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT 6
+
/* DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
struct drm_set_client_cap {
__u64 capability;
--
2.41.0

2023-10-23 07:48:22

by Albert Esteve

[permalink] [raw]

Subject: [PATCH v6 3/9] drm/vmwgfx: Use the hotspot properties from cursor planes

2023-10-23 07:48:24

by Albert Esteve

[permalink] [raw]

Subject: [PATCH v6 9/9] drm: Introduce documentation for hotspot properties

From: Michael Banack <[email protected]>

To clarify the intent and reasoning behind the hotspot properties
introduce userspace documentation that goes over cursor handling
in para-virtualized environments.

The documentation is generic enough to not special case for any
specific hypervisor and should apply equally to all.

Signed-off-by: Zack Rusin <[email protected]>
---
Documentation/gpu/drm-kms.rst | 6 ++++
drivers/gpu/drm/drm_plane.c | 58 ++++++++++++++++++++++++++++++++++-
2 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
index a0c83fc481264..158cdcc9351f9 100644
--- a/Documentation/gpu/drm-kms.rst
+++ b/Documentation/gpu/drm-kms.rst
@@ -577,6 +577,12 @@ Variable Refresh Properties
.. kernel-doc:: drivers/gpu/drm/drm_connector.c
:doc: Variable refresh properties

+Cursor Hotspot Properties
+---------------------------
+
+.. kernel-doc:: drivers/gpu/drm/drm_plane.c
+ :doc: hotspot properties
+
Existing KMS Properties
-----------------------

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 1dc00ad4c33c3..f3f2eae83cca8 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -230,6 +230,61 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
return 0;
}

+/**
+ * DOC: hotspot properties
+ *
+ * HOTSPOT_X: property to set mouse hotspot x offset.
+ * HOTSPOT_Y: property to set mouse hotspot y offset.
+ *
+ * When the plane is being used as a cursor image to display a mouse pointer,
+ * the "hotspot" is the offset within the cursor image where mouse events
+ * are expected to go.
+ *
+ * Positive values move the hotspot from the top-left corner of the cursor
+ * plane towards the right and bottom.
+ *
+ * Most display drivers do not need this information because the
+ * hotspot is not actually connected to anything visible on screen.
+ * However, this is necessary for display drivers like the para-virtualized
+ * drivers (eg qxl, vbox, virtio, vmwgfx), that are attached to a user console
+ * with a mouse pointer. Since these consoles are often being remoted over a
+ * network, they would otherwise have to wait to display the pointer movement to
+ * the user until a full network round-trip has occurred. New mouse events have
+ * to be sent from the user's console, over the network to the virtual input
+ * devices, forwarded to the desktop for processing, and then the cursor plane's
+ * position can be updated and sent back to the user's console over the network.
+ * Instead, with the hotspot information, the console can anticipate the new
+ * location, and draw the mouse cursor there before the confirmation comes in.
+ * To do that correctly, the user's console must be able predict how the
+ * desktop will process mouse events, which normally requires the desktop's
+ * mouse topology information, ie where each CRTC sits in the mouse coordinate
+ * space. This is typically sent to the para-virtualized drivers using some
+ * driver-specific method, and the driver then forwards it to the console by
+ * way of the virtual display device or hypervisor.
+ *
+ * The assumption is generally made that there is only one cursor plane being
+ * used this way at a time, and that the desktop is feeding all mouse devices
+ * into the same global pointer. Para-virtualized drivers that require this
+ * should only be exposing a single cursor plane, or find some other way
+ * to coordinate with a userspace desktop that supports multiple pointers.
+ * If the hotspot properties are set, the cursor plane is therefore assumed to be
+ * used only for displaying a mouse cursor image, and the position of the combined
+ * cursor plane + offset can therefore be used for coordinating with input from a
+ * mouse device.
+ *
+ * The cursor will then be drawn either at the location of the plane in the CRTC
+ * console, or as a free-floating cursor plane on the user's console
+ * corresponding to their desktop mouse position.
+ *
+ * DRM clients which would like to work correctly on drivers which expose
+ * hotspot properties should advertise DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT.
+ * Setting this property on drivers which do not special case
+ * cursor planes will return EOPNOTSUPP, which can be used by userspace to
+ * gauge requirements of the hardware/drivers they're running on. Advertising
+ * DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT implies that the userspace client will be
+ * correctly setting the hotspot properties.
+ */
+
/**
* drm_plane_create_hotspot_properties - creates the mouse hotspot
* properties and attaches them to the given cursor plane
@@ -237,7 +292,8 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
* @plane: drm cursor plane
*
* This function enables the mouse hotspot property on a given
- * cursor plane.
+ * cursor plane. Look at the documentation for hotspot properties
+ * to get a better understanding for what they're used for.
*
* RETURNS:
* Zero for success or -errno
--
2.41.0

2023-10-23 07:48:27

by Albert Esteve

[permalink] [raw]

Subject: [PATCH v6 4/9] drm/qxl: Use the hotspot properties from cursor planes

From: Zack Rusin <[email protected]>

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 <[email protected]>
Reviewed-by: Gerd Hoffmann <[email protected]>
Cc: Dave Airlie <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: [email protected]
Cc: [email protected]
Reviewed-by: Javier Martinez Canillas <[email protected]>
---
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 6492a70e3c396..5d689e0d3586c 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.41.0

2023-10-23 07:48:36

by Albert Esteve

[permalink] [raw]

Subject: [PATCH v6 5/9] drm/vboxvideo: Use the hotspot properties from cursor planes

2023-10-23 07:48:52

by Albert Esteve

[permalink] [raw]

Subject: [PATCH v6 7/9] drm: Remove legacy cursor hotspot code

From: Zack Rusin <[email protected]>

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

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

Signed-off-by: Zack Rusin <[email protected]>
Cc: Maarten Lankhorst <[email protected]>
Cc: Maxime Ripard <[email protected]>
Cc: Thomas Zimmermann <[email protected]>
Cc: David Airlie <[email protected]>
Cc: Daniel Vetter <[email protected]>
Reviewed-by: Javier Martinez Canillas <[email protected]>
---
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 eaca367bdc7e7..1dc00ad4c33c3 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1110,9 +1110,6 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc,
return PTR_ERR(fb);
}

- fb->hot_x = req->hot_x;
- fb->hot_y = req->hot_y;
-
if (plane->hotspot_x_property && plane->state)
plane->state->hotspot_x = req->hot_x;
if (plane->hotspot_y_property && plane->state)
diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
index 0dcc07b686548..1e108c1789b1e 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.41.0

2023-10-23 07:56:08

by Simon Ser

[permalink] [raw]

Subject: Re: [PATCH v6 0/9] Fix cursor planes with virtualized drivers

On Monday, October 23rd, 2023 at 09:46, Albert Esteve <[email protected]> wrote:

> Link to the IGT test covering this patch (already merged):
> https://lists.freedesktop.org/archives/igt-dev/2023-July/058427.html

Hmm. IGT should not be merged before the kernel, because as long as the
kernel is not merged there might be some uAPI changes.

> Mutter patch:
> https://lists.freedesktop.org/archives/igt-dev/2023-July/058427.html

Seems like this link is same as IGT? Copy-pasta fail maybe?

2023-10-23 08:19:39

by Simon Ser

[permalink] [raw]

Subject: Re: [PATCH v6 0/9] Fix cursor planes with virtualized drivers

On Monday, October 23rd, 2023 at 10:14, Albert Esteve <[email protected]> wrote:

> On Mon, Oct 23, 2023 at 9:55 AM Simon Ser <[email protected]> wrote:
>
> > On Monday, October 23rd, 2023 at 09:46, Albert Esteve <[email protected]> wrote:
> >
> > > Link to the IGT test covering this patch (already merged):
> > > https://lists.freedesktop.org/archives/igt-dev/2023-July/058427.html
> >
> > Hmm. IGT should not be merged before the kernel, because as long as the
> > kernel is not merged there might be some uAPI changes.
>
> Right, but uAPI header was not updated on the IGT side. As per suggestion of the
> maintainers, I added a static variable that matches the definition on this patch:
> https://lists.freedesktop.org/archives/igt-dev/2023-August/058803.html
>
> +/**
> + * Clients which do set cursor hotspot and treat the cursor plane
> + * like a mouse cursor should set this property.
> + */
> +#define LOCAL_DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT 6
>
> Once this patch gets upstreamed, the localized definition will be removed,
> replaced by the real one.

What if this patch gets delayed and another patch using the same number
is merged into the kernel first? What if someone finds a design flaw in
the uAPI and it needs to be completely changed? The IGT test would then
be completely broken.

As a rule of thumb: never merge user-space patches before kernel. As
soon as the kernel part is merged, it's fine to locally copy definitions
if desirable.

> > > Mutter patch:
> > > https://lists.freedesktop.org/archives/igt-dev/2023-July/058427.html
> >
> > Seems like this link is same as IGT? Copy-pasta fail maybe?
>
> Ah yes, my bad, this is the correct link:
> https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/3337

Thanks!

2023-10-23 08:23:16

by Pekka Paalanen

[permalink] [raw]

Subject: Re: [PATCH v6 9/9] drm: Introduce documentation for hotspot properties

On Mon, 23 Oct 2023 09:46:13 +0200
Albert Esteve <[email protected]> wrote:

> From: Michael Banack <[email protected]>
>
> To clarify the intent and reasoning behind the hotspot properties
> introduce userspace documentation that goes over cursor handling
> in para-virtualized environments.
>
> The documentation is generic enough to not special case for any
> specific hypervisor and should apply equally to all.
>
> Signed-off-by: Zack Rusin <[email protected]>

Hi,

the below doc text:

Acked-by: Pekka Paalanen <[email protected]>

Thanks,
pq

> ---
> Documentation/gpu/drm-kms.rst | 6 ++++
> drivers/gpu/drm/drm_plane.c | 58 ++++++++++++++++++++++++++++++++++-
> 2 files changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index a0c83fc481264..158cdcc9351f9 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -577,6 +577,12 @@ Variable Refresh Properties
> .. kernel-doc:: drivers/gpu/drm/drm_connector.c
> :doc: Variable refresh properties
>
> +Cursor Hotspot Properties
> +---------------------------
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_plane.c
> + :doc: hotspot properties
> +
> Existing KMS Properties
> -----------------------
>
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 1dc00ad4c33c3..f3f2eae83cca8 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -230,6 +230,61 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
> return 0;
> }
>
> +/**
> + * DOC: hotspot properties
> + *
> + * HOTSPOT_X: property to set mouse hotspot x offset.
> + * HOTSPOT_Y: property to set mouse hotspot y offset.
> + *
> + * When the plane is being used as a cursor image to display a mouse pointer,
> + * the "hotspot" is the offset within the cursor image where mouse events
> + * are expected to go.
> + *
> + * Positive values move the hotspot from the top-left corner of the cursor
> + * plane towards the right and bottom.
> + *
> + * Most display drivers do not need this information because the
> + * hotspot is not actually connected to anything visible on screen.
> + * However, this is necessary for display drivers like the para-virtualized
> + * drivers (eg qxl, vbox, virtio, vmwgfx), that are attached to a user console
> + * with a mouse pointer. Since these consoles are often being remoted over a
> + * network, they would otherwise have to wait to display the pointer movement to
> + * the user until a full network round-trip has occurred. New mouse events have
> + * to be sent from the user's console, over the network to the virtual input
> + * devices, forwarded to the desktop for processing, and then the cursor plane's
> + * position can be updated and sent back to the user's console over the network.
> + * Instead, with the hotspot information, the console can anticipate the new
> + * location, and draw the mouse cursor there before the confirmation comes in.
> + * To do that correctly, the user's console must be able predict how the
> + * desktop will process mouse events, which normally requires the desktop's
> + * mouse topology information, ie where each CRTC sits in the mouse coordinate
> + * space. This is typically sent to the para-virtualized drivers using some
> + * driver-specific method, and the driver then forwards it to the console by
> + * way of the virtual display device or hypervisor.
> + *
> + * The assumption is generally made that there is only one cursor plane being
> + * used this way at a time, and that the desktop is feeding all mouse devices
> + * into the same global pointer. Para-virtualized drivers that require this
> + * should only be exposing a single cursor plane, or find some other way
> + * to coordinate with a userspace desktop that supports multiple pointers.
> + * If the hotspot properties are set, the cursor plane is therefore assumed to be
> + * used only for displaying a mouse cursor image, and the position of the combined
> + * cursor plane + offset can therefore be used for coordinating with input from a
> + * mouse device.
> + *
> + * The cursor will then be drawn either at the location of the plane in the CRTC
> + * console, or as a free-floating cursor plane on the user's console
> + * corresponding to their desktop mouse position.
> + *
> + * DRM clients which would like to work correctly on drivers which expose
> + * hotspot properties should advertise DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT.
> + * Setting this property on drivers which do not special case
> + * cursor planes will return EOPNOTSUPP, which can be used by userspace to
> + * gauge requirements of the hardware/drivers they're running on. Advertising
> + * DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT implies that the userspace client will be
> + * correctly setting the hotspot properties.
> + */
> +
> /**
> * drm_plane_create_hotspot_properties - creates the mouse hotspot
> * properties and attaches them to the given cursor plane
> @@ -237,7 +292,8 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
> * @plane: drm cursor plane
> *
> * This function enables the mouse hotspot property on a given
> - * cursor plane.
> + * cursor plane. Look at the documentation for hotspot properties
> + * to get a better understanding for what they're used for.
> *
> * RETURNS:
> * Zero for success or -errno

Attachments:

(No filename) (849.00 B)
OpenPGP digital signature

2023-10-23 21:30:52

by Javier Martinez Canillas

[permalink] [raw]

Subject: Re: [PATCH v6 9/9] drm: Introduce documentation for hotspot properties

Albert Esteve <[email protected]> writes:

> From: Michael Banack <[email protected]>
>
> To clarify the intent and reasoning behind the hotspot properties
> introduce userspace documentation that goes over cursor handling
> in para-virtualized environments.
>
> The documentation is generic enough to not special case for any
> specific hypervisor and should apply equally to all.
>
> Signed-off-by: Zack Rusin <[email protected]>

The author is Michael Banack but it's missing a SoB from them.
I don't think there's a need to resend for this, can be added
when applying. But either Michael or Zack should confirm that
is the correct thing to do for this patch.

The doc itself looks great to me and it clarifies a lot about
cursor hotspots.

Reviewed-by: Javier Martinez Canillas <[email protected]>

--
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

2023-10-24 19:34:37

by Michael Banack

[permalink] [raw]

Subject: Re: [PATCH v6 9/9] drm: Introduce documentation for hotspot properties

Yes, that patch should be:

Signed-off-by: Michael Banack <[email protected]>

--Michael Banack

On 10/23/23 14:29, Javier Martinez Canillas wrote:
> Albert Esteve <[email protected]> writes:
>
>> From: Michael Banack <[email protected]>
>>
>> To clarify the intent and reasoning behind the hotspot properties
>> introduce userspace documentation that goes over cursor handling
>> in para-virtualized environments.
>>
>> The documentation is generic enough to not special case for any
>> specific hypervisor and should apply equally to all.
>>
>> Signed-off-by: Zack Rusin <[email protected]>
> The author is Michael Banack but it's missing a SoB from them.
> I don't think there's a need to resend for this, can be added
> when applying. But either Michael or Zack should confirm that
> is the correct thing to do for this patch.
>
> The doc itself looks great to me and it clarifies a lot about
> cursor hotspots.
>
> Reviewed-by: Javier Martinez Canillas <[email protected]>
>

2023-10-25 06:36:59

by Javier Martinez Canillas

[permalink] [raw]

Subject: Re: [PATCH v6 9/9] drm: Introduce documentation for hotspot properties

Michael Banack <[email protected]> writes:

Hello Michael,

> Yes, that patch should be:
>
> Signed-off-by: Michael Banack <[email protected]>
>

Great, thanks for the confirmation.

> --Michael Banack
>

--
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

2023-11-22 12:50:18

by Javier Martinez Canillas

[permalink] [raw]

Subject: Re: [PATCH v6 0/9] Fix cursor planes with virtualized drivers

Albert Esteve <[email protected]> writes:

Hello,

[...]

>
>> > Mutter patch:
>> > https://lists.freedesktop.org/archives/igt-dev/2023-July/058427.html
>>
>> Seems like this link is same as IGT? Copy-pasta fail maybe?
>>
>>
> Ah yes, my bad, this is the correct link:
> https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/3337

The mutter chages are already in good shape and the MR has even be
approved by a mutter developer. Any objections to merge the series ?

--
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

2023-11-23 22:13:39

by Simon Ser

[permalink] [raw]

Subject: Re: [PATCH v6 0/9] Fix cursor planes with virtualized drivers

On Wednesday, November 22nd, 2023 at 13:49, Javier Martinez Canillas <[email protected]> wrote:

> Any objections to merge the series ?

No objections from me :)

2023-11-24 10:57:03

by Javier Martinez Canillas

[permalink] [raw]

Subject: Re: [PATCH v6 0/9] Fix cursor planes with virtualized drivers

Simon Ser <[email protected]> writes:

Hello Simon,

> On Wednesday, November 22nd, 2023 at 13:49, Javier Martinez Canillas <[email protected]> wrote:
>
>> Any objections to merge the series ?
>
> No objections from me :)
>

Perfect, I'll merge this series then to unblock the mutter MR. Thanks again!

--
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

2023-11-24 14:42:11

by Javier Martinez Canillas

[permalink] [raw]

Subject: Re: [PATCH v6 0/9] Fix cursor planes with virtualized drivers

Albert Esteve <[email protected]> writes:

> v6: Shift DRIVER_CURSOR_HOTSPOT flag bit to BIT(9), since BIT(8)
> was already taken by DRIVER_GEM_GPUVA.
>
> v5: Add a change with documentation from Michael, based on his discussion
> with Pekka and bump the kernel version DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT
> might be introduced with to 6.6.
>
> v4: Make drm_plane_create_hotspot_properties static, rename
> DRM_CLIENT_CAP_VIRTUALIZED_CURSOR_PLANE to DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT
> and some minor stylistic fixes for things found by Javier and Pekka
> in v3.
>
> v3: Renames, fixes and cleanups suggested by Daniel, Simon and Pekka
> after v2. There's no major changes in functionality. Please let me know
> if I missed anything, it's been a while since v2.
>
> Virtualized drivers have had a lot of issues with cursor support on top
> of atomic modesetting. This set both fixes the long standing problems
> with atomic kms and virtualized drivers and adds code to let userspace
> use atomic kms on virtualized drivers while preserving functioning
> seamless cursors between the host and guest.
>
> The first change in the set is one that should be backported as far as
> possible, likely 5.4 stable, because earlier stable kernels do not have
> virtualbox driver. The change makes virtualized drivers stop exposing
> a cursor plane for atomic clients, this fixes mouse cursor on all well
> formed compositors which will automatically fallback to software cursor.
>
> The rest of the changes until the last one ports the legacy hotspot code
> to atomic plane properties.
>
> Finally the last change introduces userspace API to let userspace
> clients advertise the fact that they are aware of additional restrictions
> placed upon the cursor plane by virtualized drivers and lets them use
> atomic kms with virtualized drivers (the clients are expected to set
> hotspots correctly when advertising support for virtual cursor plane).
>
> Link to the IGT test covering this patch (already merged):
> https://lists.freedesktop.org/archives/igt-dev/2023-July/058427.html
>
> Mutter patch:
> https://lists.freedesktop.org/archives/igt-dev/2023-July/058427.html
>
> Michael Banack (1):
> drm: Introduce documentation for hotspot properties
>
> Zack Rusin (8):
> drm: Disable the cursor plane on atomic contexts with virtualized
> drivers
> drm/atomic: Add support for mouse hotspots
> drm/vmwgfx: Use the hotspot properties from cursor planes
> drm/qxl: Use the hotspot properties from cursor planes
> drm/vboxvideo: Use the hotspot properties from cursor planes
> drm/virtio: Use the hotspot properties from cursor planes
> drm: Remove legacy cursor hotspot code
> drm: Introduce DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT
>

Pushed to drm-misc (drm-misc-next). Thanks!

--
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat