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

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- 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 --]