On Thu, Feb 09, 2023 at 03:16:23PM +0100, Jonas Ådahl wrote:
> On Wed, Feb 08, 2023 at 11:10:16PM +0200, Ville Syrjala 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.
> >
> > v2: Try to add some docs
> >
> > Cc: Simon Ser <contact@xxxxxxxxxxx>
> > Cc: Jonas Ådahl <jadahl@xxxxxxxxxx>
> > Cc: Daniel Stone <daniel@xxxxxxxxxxxxx>
> > Cc: Pekka Paalanen <pekka.paalanen@xxxxxxxxxxxxx>
> > Acked-by: Harry Wentland <harry.wentland@xxxxxxx>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/drm_mode_config.c | 7 +++++
> > drivers/gpu/drm/drm_plane.c | 48 +++++++++++++++++++++++++++++++
> > include/drm/drm_mode_config.h | 5 ++++
> > include/drm/drm_plane.h | 4 +++
> > include/uapi/drm/drm_mode.h | 11 +++++++
> > 5 files changed, 75 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..ae51b1f83755 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -140,6 +140,21 @@
> > * 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.
> > + *
> > + * For optimal usage userspace should pick the smallest size
> > + * that satisfies its own requirements.
> > + *
> > + * The blob contains an array of struct drm_plane_size_hint.
> > + *
> > + * Drivers should only attach this property to planes that
> > + * support a very limited set of sizes (eg. cursor planes
> > + * on typical hardware).
>
> This is a bit awkward since problematic drivers would only expose
> this property if they are new enough.
>
> A way to avoid this is for all new drivers expose this property, but
> special case the size 0x0 as "everything below the max limit goes". Then
> userspace can do (ignoring the missing cap fallback).
I don't think there are any drivers that need that.
So I'm thinking we can just ignore that for now.
>
> if (has(SIZE_HINTS))
> size = figure_out_size(SIZE_HINTS,
> DRM_CAP_CURSOR_WIDTH,
> DRM_CAP_CURSOR_HEIGHT,
> preferred_size);
> else
> size = DRM_CAP_CURSOR_WIDTH x DRM_CAP_CURSOR_WIDTH;
>
> With `figure_out_size()` knowing how to deal with 0x0 in the size hints
> to use `preferred_size` directly.
>
>
> Jonas
>
> > */
> >
> > static unsigned int drm_num_planes(struct drm_device *dev)
> > @@ -1582,3 +1597,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..5bc8aed9b445 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_propertty: 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..9d7c5967264f 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -849,6 +849,17 @@ struct drm_color_lut {
> > __u16 reserved;
> > };
> >
> > +/**
> > + * struct drm_plane_size_hint - Plane size hints
> > + *
> > + * The plane SIZE_HINTS property blob contains an
> > + * array of struct drm_plane_size_hint.
> > + */
> > +struct drm_plane_size_hint {
> > + __u16 width;
> > + __u16 height;
> > +};
> > +
> > /**
> > * struct hdr_metadata_infoframe - HDR Metadata Infoframe Data.
> > *
> > --
> > 2.39.1
> >
--
Ville Syrjälä
Intel
