On Wed, 8 Feb 2023 06:09:10 +0200
Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> 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.
>
> 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.
>
> Cc: Simon Ser <contact@xxxxxxxxxxx>
> Cc: Jonas Ådahl <jadahl@xxxxxxxxxx>
> Cc: Daniel Stone <daniel@xxxxxxxxxxxxx>
> Cc: Pekka Paalanen <pekka.paalanen@xxxxxxxxxxxxx>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/drm_mode_config.c | 7 +++++++
> drivers/gpu/drm/drm_plane.c | 33 +++++++++++++++++++++++++++++++
> include/drm/drm_mode_config.h | 5 +++++
> include/drm/drm_plane.h | 4 ++++
> include/uapi/drm/drm_mode.h | 5 +++++
> 5 files changed, 54 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index 87eb591fe9b5..21860f94a18c 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -374,6 +374,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 24e7998d1731..d0a277f4be1f 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1582,3 +1582,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)
> +{
> + 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);
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index e5b053001d22..ed9f6938dca1 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -949,6 +949,11 @@ struct drm_mode_config {
> */
> struct drm_property *modifiers_property;
>
> + /**
> + * @size_hints_property: Plane SIZE_HINTS property.
> + */
> + struct drm_property *size_hints_property;
> +
> /* cursor size */
> uint32_t cursor_width, cursor_height;
>
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 51291983ea44..1997d7d64b69 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -32,6 +32,7 @@
> #include <drm/drm_util.h>
>
> struct drm_crtc;
> +struct drm_plane_size_hint;
> struct drm_printer;
> struct drm_modeset_acquire_ctx;
>
> @@ -945,5 +946,8 @@ drm_plane_get_damage_clips(const struct drm_plane_state *state);
>
> int drm_plane_create_scaling_filter_property(struct drm_plane *plane,
> unsigned int supported_filters);
> +int drm_plane_add_size_hints_property(struct drm_plane *plane,
> + const struct drm_plane_size_hint *hints,
> + int num_hints);
>
> #endif
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 46becedf5b2f..4f0551d7f481 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -849,6 +849,11 @@ struct drm_color_lut {
> __u16 reserved;
> };
>
> +struct drm_plane_size_hint {
> + __u16 width;
> + __u16 height;
> +};
Hi Ville,
uAPI documentation is missing.
When there is just a single recommended size listed, userspace has it
easy: allocate a pair of DRM dumb buffers for each active CRTC, do
atomic test commits with those, and if the test succeeds, copy in the
pixels and fill the padding.
What if we have multiple or a huge number of possible sizes? Probably
for each KMS reconfiguration cycle (which could be up to every refresh
cycle) userspace would need to allocate a new dumb buffer to have the
right size, and then test. Is that something we can actually afford to
do? I don't know.
Therefore maybe this proposal is a good compromise. The driver lists
*few* sizes that are roughly equally likely to work, that is, if the
cursor plane does not work, it's not because of the size. Userspace
pre-allocates DRM dumb buffers for each size. When attempting to use a
cursor plane, userspace picks the smallest size that suffices, and tests
only with that.
Seems fine to me. The uAPI docs should explain what userspace is
expected to do with it.
Thanks,
pq
> +
> /**
> * struct hdr_metadata_infoframe - HDR Metadata Infoframe Data.
> *
