[PATCH v2 1/2] drm: Introduce plane SIZE_HINTS property

[Simon Ser]

On Tuesday, February 27th, 2024 at 20:35, Ville Syrjala <ville.syrjala at linux.intel.com> wrote:

> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> Add a new immutable plane property by which a plane can advertise
> a handful of recommended plane sizes. This would be mostly exposed
> by cursor planes as a slightly more capable replacement for
> the DRM_CAP_CURSOR_WIDTH/HEIGHT caps, which can only declare
> a one size fits all limit for the whole device.
> 
> Currently eg. amdgpu/i915/nouveau just advertize the max cursor
> size via the cursor size caps. But always using the max sized
> cursor can waste a surprising amount of power, so a better
> stragey is desirable.

Typo: strategy

> Most other drivers don't specify any cursor size at all, in
> which case the ioctl code just claims that 64x64 is a great
> choice. Whether that is actually true is debatable.
> 
> A poll of various compositor developers informs us that
> blindly probing with setcursor/atomic ioctl to determine
> suitable cursor sizes is not acceptable, thus the
> introduction of the new property to supplant the cursor
> size caps. The compositor will now be free to select a
> more optimal cursor size from the short list of options.
> 
> Note that the reported sizes (either via the property or the
> caps) make no claims about things such as plane scaling. So
> these things should only really be consulted for simple
> "cursor like" use cases.
> 
> Userspace consumer in the form of mutter seems ready:
> https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/3165

Do we need an IGT as well to merge this new uAPI?

> v2: Try to add some docs
> v3: Specify that value 0 is reserved for future use (basic idea from Jonas)
>     Drop the note about typical hardware (Pekka)
> v4: Update the docs to indicate the list is "in order of preference"
>     Add a a link to the mutter MR
> 
> Cc: Simon Ser <contact at emersion.fr>
> Cc: Jonas Ådahl <jadahl at redhat.com>
> Cc: Daniel Stone <daniel at fooishbar.org>
> Cc: Sameer Lattannavar <sameer.lattannavar at intel.com>
> Cc: Sebastian Wick <sebastian.wick at redhat.com>
> Acked-by: Harry Wentland <harry.wentland at amd.com>
> Acked-by: Pekka Paalanen <pekka.paalanen at collabora.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>  drivers/gpu/drm/drm_mode_config.c |  7 +++++
>  drivers/gpu/drm/drm_plane.c       | 52 +++++++++++++++++++++++++++++++
>  include/drm/drm_mode_config.h     |  5 +++
>  include/drm/drm_plane.h           |  4 +++
>  include/uapi/drm/drm_mode.h       | 11 +++++++
>  5 files changed, 79 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index 48fd2d67f352..568972258222 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -372,6 +372,13 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>  		return -ENOMEM;
>  	dev->mode_config.modifiers_property = prop;
> 
> +	prop = drm_property_create(dev,
> +				   DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_BLOB,
> +				   "SIZE_HINTS", 0);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.size_hints_property = prop;
> +
>  	return 0;
>  }
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 672c655c7a8e..4135ce16e608 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -140,6 +140,25 @@
>   *     DRM_FORMAT_MOD_LINEAR. Before linux kernel release v5.1 there have been
>   *     various bugs in this area with inconsistencies between the capability
>   *     flag and per-plane properties.
> + *
> + * SIZE_HINTS:
> + *     Blob property which contains the set of recommended plane size
> + *     which can used for simple "cursor like" use cases (eg. no scaling).
> + *     Using these hints frees userspace from extensive probing of
> + *     supported plane sizes through atomic/setcursor ioctls.
> + *
> + *     The blob contains an array of struct drm_plane_size_hint, in
> + *     order of preference. For optimal usage userspace should pick
> + *     the first size that satisfies its own requirements.
> + *
> + *     Drivers should only attach this property to planes that
> + *     support a very limited set of sizes.
> + *
> + *     Note that property value 0 (ie. no blob) is reserved for potential
> + *     future use. Current userspace is expected to ignore the property
> + *     if the value is 0, and fall back to some other means (eg.
> + *     &DRM_CAP_CURSOR_WIDTH and &DRM_CAP_CURSOR_HEIGHT) to determine
> + *     the appropriate plane size to use.
>   */
> 
>  static unsigned int drm_num_planes(struct drm_device *dev)
> @@ -1729,3 +1748,36 @@ int drm_plane_create_scaling_filter_property(struct drm_plane *plane,
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_plane_create_scaling_filter_property);
> +
> +/**
> + * drm_plane_add_size_hint_property - create a size hint property
> + *
> + * @plane: drm plane
> + * @hints: size hints
> + * @num_hints: number of size hints
> + *
> + * Create a size hints property for the plane.
> + *
> + * RETURNS:
> + * Zero for success or -errno
> + */
> +int drm_plane_add_size_hints_property(struct drm_plane *plane,
> +				      const struct drm_plane_size_hint *hints,
> +				      int num_hints)
> +{

Can we add a plane type check here, to make sure this is only attached to
cursor planes? If there is a use-case for other plane types, I'd prefer to
discuss about it first before expanding the property's scope.

With that:
Reviewed-by: Simon Ser <contact at emersion.fr>

> +	struct drm_device *dev = plane->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
> +	struct drm_property_blob *blob;
> +
> +	blob = drm_property_create_blob(dev,
> +					array_size(sizeof(hints[0]), num_hints),
> +					hints);
> +	if (IS_ERR(blob))
> +		return PTR_ERR(blob);
> +
> +	drm_object_attach_property(&plane->base, config->size_hints_property,
> +				   blob->base.id);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_plane_add_size_hints_property);