[prev in list] [next in list] [prev in thread] [next in thread] 

List:       dri-devel
Subject:    Re: [PATCH v5 2/9] drm/atomic: Add support for mouse hotspots
From:       Simon Ser <contact () emersion ! fr>
Date:       2023-07-20 9:20:40
Message-ID: U-YIMqatVKkZH9FlOrhSidTMeJ4y4YUl0eWHhVWdModT28d-uXePM4S1snszE0g6ERCjizuOp5_aypLlnRg47PM8_xJdLu-pPHESIp150i8= () emersion ! fr
[Download RAW message or body]

On Wednesday, July 19th, 2023 at 03:42, Zack Rusin <zack@kde.org> wrote:

> From: Zack Rusin <zackr@vmware.com>
> 
> Atomic modesetting code lacked support for specifying mouse cursor
> hotspots. The legacy kms DRM_IOCTL_MODE_CURSOR2 had support for setting
> the hotspot but the functionality was not implemented in the new atomic
> paths.
> 
> Due to the lack of hotspots in the atomic paths userspace compositors
> completely disable atomic modesetting for drivers that require it (i.e.
> all paravirtualized drivers).
> 
> This change adds hotspot properties to the atomic codepaths throughtout
> the DRM core and will allow enabling atomic modesetting for virtualized
> drivers in the userspace.
> 
> Signed-off-by: Zack Rusin <zackr@vmware.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>  drivers/gpu/drm/drm_atomic_state_helper.c | 14 +++++++
>  drivers/gpu/drm/drm_atomic_uapi.c         | 20 +++++++++
>  drivers/gpu/drm/drm_plane.c               | 50 +++++++++++++++++++++++
>  include/drm/drm_plane.h                   | 14 +++++++
>  4 files changed, 98 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> index 784e63d70a42..54975de44a0e 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -275,6 +275,20 @@ void __drm_atomic_helper_plane_state_reset(struct drm_plane_state *plane_state,
>  			plane_state->normalized_zpos = val;
>  		}
>  	}
> +
> +	if (plane->hotspot_x_property) {
> +		if (!drm_object_property_get_default_value(&plane->base,
> +							   plane->hotspot_x_property,
> +							   &val))
> +			plane_state->hotspot_x = val;
> +	}
> +
> +	if (plane->hotspot_y_property) {
> +		if (!drm_object_property_get_default_value(&plane->base,
> +							   plane->hotspot_y_property,
> +							   &val))
> +			plane_state->hotspot_y = val;
> +	}
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_plane_state_reset);
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 98d3b10c08ae..07a7b3f18df2 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -593,6 +593,22 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>  	} else if (plane->funcs->atomic_set_property) {
>  		return plane->funcs->atomic_set_property(plane, state,
>  				property, val);
> +	} else if (property == plane->hotspot_x_property) {
> +		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> +			drm_dbg_atomic(plane->dev,
> +				       "[PLANE:%d:%s] is not a cursor plane: 0x%llx\n",
> +				       plane->base.id, plane->name, val);
> +			return -EINVAL;
> +		}

Hm, it sounds a bit weird to catch this case here. Wouldn't it be a driver bug
to attach the hotspot props to a plane which isn't a cursor? Wouldn't it make
more sense to WARN in drm_plane_create_hotspot_properties() if a driver
attempts to do so?

> +		state->hotspot_x = val;
> +	} else if (property == plane->hotspot_y_property) {
> +		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> +			drm_dbg_atomic(plane->dev,
> +				       "[PLANE:%d:%s] is not a cursor plane: 0x%llx\n",
> +				       plane->base.id, plane->name, val);
> +			return -EINVAL;
> +		}
> +		state->hotspot_y = val;
>  	} else {
>  		drm_dbg_atomic(plane->dev,
>  			       "[PLANE:%d:%s] unknown property [PROP:%d:%s]\n",
> @@ -653,6 +669,10 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>  		*val = state->scaling_filter;
>  	} else if (plane->funcs->atomic_get_property) {
>  		return plane->funcs->atomic_get_property(plane, state, property, val);
> +	} else if (property == plane->hotspot_x_property) {
> +		*val = state->hotspot_x;
> +	} else if (property == plane->hotspot_y_property) {
> +		*val = state->hotspot_y;
>  	} else {
>  		drm_dbg_atomic(dev,
>  			       "[PLANE:%d:%s] unknown property [PROP:%d:%s]\n",
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index c6bbb0c209f4..eaca367bdc7e 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -230,6 +230,47 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
>  	return 0;
>  }
> 
> +/**
> + * drm_plane_create_hotspot_properties - creates the mouse hotspot
> + * properties and attaches them to the given cursor plane
> + *
> + * @plane: drm cursor plane
> + *
> + * This function enables the mouse hotspot property on a given
> + * cursor plane.
> + *
> + * RETURNS:
> + * Zero for success or -errno
> + */
> +static int drm_plane_create_hotspot_properties(struct drm_plane *plane)
> +{
> +	struct drm_property *prop_x;
> +	struct drm_property *prop_y;
> +
> +	drm_WARN_ON(plane->dev,
> +		    !drm_core_check_feature(plane->dev,
> +					    DRIVER_CURSOR_HOTSPOT));
> +
> +	prop_x = drm_property_create_signed_range(plane->dev, 0, "HOTSPOT_X",
> +						  INT_MIN, INT_MAX);

This is an atomic-only property, the DRM_MODE_PROP_ATOMIC flag should be passed.

> +	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;
> +}
[prev in list] [next in list] [prev in thread] [next in thread]