* Re: [PATCH v3 1/8] drm: Disable the cursor plane on atomic contexts with virtualized drivers
2023-06-27 3:58 ` [PATCH v3 1/8] drm: Disable the cursor plane on atomic contexts with virtualized drivers Zack Rusin
@ 2023-06-27 8:18 ` Pekka Paalanen
2023-06-27 8:26 ` Javier Martinez Canillas
1 sibling, 0 replies; 3+ messages in thread
From: Pekka Paalanen @ 2023-06-27 8:18 UTC (permalink / raw)
To: Zack Rusin
Cc: Zack Rusin, dri-devel, krastevm, mombasawalam, banackm, iforbes,
javierm, contact, daniel, stable, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Dave Airlie,
Gerd Hoffmann, Hans de Goede, Gurchetan Singh, Chia-I Wu,
virtualization, spice-devel
[-- Attachment #1: Type: text/plain, Size: 7416 bytes --]
On Mon, 26 Jun 2023 23:58:32 -0400
Zack Rusin <zack@kde.org> 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)")
Sounds good to me.
Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Thanks,
pq
> 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 | 13 +++++++++++++
> drivers/gpu/drm/qxl/qxl_drv.c | 2 +-
> drivers/gpu/drm/vboxvideo/vbox_drv.c | 2 +-
> drivers/gpu/drm/virtio/virtgpu_drv.c | 2 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 2 +-
> include/drm/drm_drv.h | 9 +++++++++
> 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 24e7998d1731..a4a39f4834e2 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -678,6 +678,19 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data,
> !file_priv->universal_planes)
> continue;
>
> + /*
> + * If we're running on a virtualized driver then,
> + * unless userspace advertizes support for the
> + * virtualized cursor plane, disable cursor planes
> + * because they'll be broken due to missing cursor
> + * hotspot info.
> + */
> + if (plane->type == DRM_PLANE_TYPE_CURSOR &&
> + drm_core_check_feature(dev, DRIVER_CURSOR_HOTSPOT) &&
> + file_priv->atomic &&
> + !file_priv->supports_virtualized_cursor_plane)
> + continue;
> +
> if (drm_lease_held(file_priv, plane->base.id)) {
> if (count < plane_resp->count_planes &&
> put_user(plane->base.id, plane_ptr + count))
> diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
> index b30ede1cf62d..91930e84a9cd 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.c
> +++ b/drivers/gpu/drm/qxl/qxl_drv.c
> @@ -283,7 +283,7 @@ static const struct drm_ioctl_desc qxl_ioctls[] = {
> };
>
> static struct drm_driver qxl_driver = {
> - .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
> + .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_CURSOR_HOTSPOT,
>
> .dumb_create = qxl_mode_dumb_create,
> .dumb_map_offset = drm_gem_ttm_dumb_map_offset,
> diff --git a/drivers/gpu/drm/vboxvideo/vbox_drv.c b/drivers/gpu/drm/vboxvideo/vbox_drv.c
> index 4fee15c97c34..8ecd0863fad7 100644
> --- a/drivers/gpu/drm/vboxvideo/vbox_drv.c
> +++ b/drivers/gpu/drm/vboxvideo/vbox_drv.c
> @@ -172,7 +172,7 @@ DEFINE_DRM_GEM_FOPS(vbox_fops);
>
> static const struct drm_driver driver = {
> .driver_features =
> - DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
> + DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC | DRIVER_CURSOR_HOTSPOT,
>
> .fops = &vbox_fops,
> .name = DRIVER_NAME,
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
> index a7ec5a3770da..8f4bb8a4e952 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
> @@ -176,7 +176,7 @@ static const struct drm_driver driver = {
> * If KMS is disabled DRIVER_MODESET and DRIVER_ATOMIC are masked
> * out via drm_device::driver_features:
> */
> - .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER | DRIVER_ATOMIC,
> + .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER | DRIVER_ATOMIC | DRIVER_CURSOR_HOTSPOT,
> .open = virtio_gpu_driver_open,
> .postclose = virtio_gpu_driver_postclose,
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index 8b24ecf60e3e..d3e308fdfd5b 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -1611,7 +1611,7 @@ static const struct file_operations vmwgfx_driver_fops = {
>
> static const struct drm_driver driver = {
> .driver_features =
> - DRIVER_MODESET | DRIVER_RENDER | DRIVER_ATOMIC | DRIVER_GEM,
> + DRIVER_MODESET | DRIVER_RENDER | DRIVER_ATOMIC | DRIVER_GEM | DRIVER_CURSOR_HOTSPOT,
> .ioctls = vmw_ioctls,
> .num_ioctls = ARRAY_SIZE(vmw_ioctls),
> .master_set = vmw_master_set,
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index b77f2c7275b7..8303016665dd 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -104,6 +104,15 @@ enum drm_driver_feature {
> * acceleration should be handled by two drivers that are connected using auxiliary bus.
> */
> DRIVER_COMPUTE_ACCEL = BIT(7),
> + /**
> + * @DRIVER_CURSOR_HOTSPOT:
> + *
> + * Driver supports and requires cursor hotspot information in the
> + * cursor plane (e.g. cursor plane has to actually track the mouse
> + * cursor and the clients are required to set hotspot in order for
> + * the cursor planes to work correctly).
> + */
> + DRIVER_CURSOR_HOTSPOT = BIT(8),
>
> /* IMPORTANT: Below are all the legacy flags, add new ones above. */
>
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index 966912053cb0..91cf7f452f86 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -228,6 +228,18 @@ struct drm_file {
> */
> bool is_master;
>
> + /**
> + * @supports_virtualized_cursor_plane:
> + *
> + * This client is capable of handling the cursor plane with the
> + * restrictions imposed on it by the virtualized drivers.
> + *
> + * This implies that the cursor plane has to behave like a cursor
> + * i.e. track cursor movement. It also requires setting of the
> + * hotspot properties by the client on the cursor plane.
> + */
> + bool supports_virtualized_cursor_plane;
> +
> /**
> * @master:
> *
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v3 1/8] drm: Disable the cursor plane on atomic contexts with virtualized drivers
2023-06-27 3:58 ` [PATCH v3 1/8] drm: Disable the cursor plane on atomic contexts with virtualized drivers Zack Rusin
2023-06-27 8:18 ` Pekka Paalanen
@ 2023-06-27 8:26 ` Javier Martinez Canillas
1 sibling, 0 replies; 3+ messages in thread
From: Javier Martinez Canillas @ 2023-06-27 8:26 UTC (permalink / raw)
To: Zack Rusin, dri-devel
Cc: Maxime Ripard, spice-devel, Thomas Zimmermann, Hans de Goede,
David Airlie, banackm, Gurchetan Singh, krastevm, ppaalanen,
Dave Airlie, stable, iforbes, virtualization, mombasawalam,
Gerd Hoffmann
Zack Rusin <zack@kde.org> writes:
Hello Zack,
> 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 | 13 +++++++++++++
> drivers/gpu/drm/qxl/qxl_drv.c | 2 +-
> drivers/gpu/drm/vboxvideo/vbox_drv.c | 2 +-
> drivers/gpu/drm/virtio/virtgpu_drv.c | 2 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 2 +-
> include/drm/drm_drv.h | 9 +++++++++
> 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 24e7998d1731..a4a39f4834e2 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -678,6 +678,19 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data,
> !file_priv->universal_planes)
> continue;
>
> + /*
> + * If we're running on a virtualized driver then,
> + * unless userspace advertizes support for the
> + * virtualized cursor plane, disable cursor planes
> + * because they'll be broken due to missing cursor
> + * hotspot info.
> + */
> + if (plane->type == DRM_PLANE_TYPE_CURSOR &&
> + drm_core_check_feature(dev, DRIVER_CURSOR_HOTSPOT) &&
Nit: you have a tab instead of an space before && but this can just be
fixed when applying.
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
^ permalink raw reply [flat|nested] 3+ messages in thread