[v3,1/2] drm: Introduce plane SIZE_HINTS property

Commit Message

Ville Syrjälä March 13, 2023, 4:33 p.m. UTC

From: Ville Syrjälä <[e-mail]>

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
v3: Specify that value 0 is reserved for future use (basic idea from Jonas)
    Drop the note about typical hardware (Pekka)

Cc: Simon Ser <[e-mail]>
Cc: Jonas Ådahl <[e-mail]>
Cc: Daniel Stone <[e-mail]>
Cc: Pekka Paalanen <[e-mail]>
Acked-by: Harry Wentland <[e-mail]>
Acked-by: Pekka Paalanen <[e-mail]>
Signed-off-by: Ville Syrjälä <[e-mail]>
---
 drivers/gpu/drm/drm_mode_config.c |  7 ++++
 drivers/gpu/drm/drm_plane.c       | 53 +++++++++++++++++++++++++++++++
 include/drm/drm_mode_config.h     |  5 +++
 include/drm/drm_plane.h           |  4 +++
 include/uapi/drm/drm_mode.h       | 11 +++++++
 5 files changed, 80 insertions(+)

Comments

Jonas Ådahl March 17, 2023, 10:34 a.m. UTC | #1

On Mon, Mar 13, 2023 at 06:33:11PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <[e-mail]>
> 
> 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
> v3: Specify that value 0 is reserved for future use (basic idea from Jonas)
>     Drop the note about typical hardware (Pekka)
> 
> Cc: Simon Ser <[e-mail]>
> Cc: Jonas Ådahl <[e-mail]>
> Cc: Daniel Stone <[e-mail]>
> Cc: Pekka Paalanen <[e-mail]>
> Acked-by: Harry Wentland <[e-mail]>
> Acked-by: Pekka Paalanen <[e-mail]>
> Signed-off-by: Ville Syrjälä <[e-mail]>
> ---
>  drivers/gpu/drm/drm_mode_config.c |  7 ++++
>  drivers/gpu/drm/drm_plane.c       | 53 +++++++++++++++++++++++++++++++
>  include/drm/drm_mode_config.h     |  5 +++
>  include/drm/drm_plane.h           |  4 +++
>  include/uapi/drm/drm_mode.h       | 11 +++++++
>  5 files changed, 80 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..d2a6fdff1a57 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -140,6 +140,26 @@
>   *     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.
> + *
> + *     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.

Does this intend to mean userspace has the kernel's blessing on choosing
an arbitrary size as long as it's smaller than &DRM_CAP_CURSOR_WIDTH x
&DRM_CAP_CURSOR_HEIGHT?

It's a bit to vague for me to make a confident interpretation whether I
can, or whether I should pretend I didn't see SIZE_HINTS and apply the
old logic, meaning only using the exact cap size.


Jonas

>   */
>  
>  static unsigned int drm_num_planes(struct drm_device *dev)
> @@ -1582,3 +1602,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.2
>

Ville Syrjälä March 17, 2023, 11:33 a.m. UTC | #2

On Fri, Mar 17, 2023 at 11:34:16AM +0100, Jonas Ådahl wrote:
> On Mon, Mar 13, 2023 at 06:33:11PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <[e-mail]>
> > 
> > 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
> > v3: Specify that value 0 is reserved for future use (basic idea from Jonas)
> >     Drop the note about typical hardware (Pekka)
> > 
> > Cc: Simon Ser <[e-mail]>
> > Cc: Jonas Ådahl <[e-mail]>
> > Cc: Daniel Stone <[e-mail]>
> > Cc: Pekka Paalanen <[e-mail]>
> > Acked-by: Harry Wentland <[e-mail]>
> > Acked-by: Pekka Paalanen <[e-mail]>
> > Signed-off-by: Ville Syrjälä <[e-mail]>
> > ---
> >  drivers/gpu/drm/drm_mode_config.c |  7 ++++
> >  drivers/gpu/drm/drm_plane.c       | 53 +++++++++++++++++++++++++++++++
> >  include/drm/drm_mode_config.h     |  5 +++
> >  include/drm/drm_plane.h           |  4 +++
> >  include/uapi/drm/drm_mode.h       | 11 +++++++
> >  5 files changed, 80 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..d2a6fdff1a57 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -140,6 +140,26 @@
> >   *     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.
> > + *
> > + *     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.
> 
> Does this intend to mean userspace has the kernel's blessing on choosing
> an arbitrary size as long as it's smaller than &DRM_CAP_CURSOR_WIDTH x
> &DRM_CAP_CURSOR_HEIGHT?
> 
> It's a bit to vague for me to make a confident interpretation whether I
> can, or whether I should pretend I didn't see SIZE_HINTS and apply the
> old logic, meaning only using the exact cap size.

Using the exact cap size is the only thing more or less
guaranteed to work. But other approaches (such as probing
the size with atomic/cursor ioctls) can also be used.

Jonas Ådahl March 17, 2023, 12:21 p.m. UTC | #3

On Fri, Mar 17, 2023 at 01:33:25PM +0200, Ville Syrjälä wrote:
> On Fri, Mar 17, 2023 at 11:34:16AM +0100, Jonas Ådahl wrote:
> > On Mon, Mar 13, 2023 at 06:33:11PM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <[e-mail]>
> > > 
> > > 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
> > > v3: Specify that value 0 is reserved for future use (basic idea from Jonas)
> > >     Drop the note about typical hardware (Pekka)
> > > 
> > > Cc: Simon Ser <[e-mail]>
> > > Cc: Jonas Ådahl <[e-mail]>
> > > Cc: Daniel Stone <[e-mail]>
> > > Cc: Pekka Paalanen <[e-mail]>
> > > Acked-by: Harry Wentland <[e-mail]>
> > > Acked-by: Pekka Paalanen <[e-mail]>
> > > Signed-off-by: Ville Syrjälä <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/drm_mode_config.c |  7 ++++
> > >  drivers/gpu/drm/drm_plane.c       | 53 +++++++++++++++++++++++++++++++
> > >  include/drm/drm_mode_config.h     |  5 +++
> > >  include/drm/drm_plane.h           |  4 +++
> > >  include/uapi/drm/drm_mode.h       | 11 +++++++
> > >  5 files changed, 80 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..d2a6fdff1a57 100644
> > > --- a/drivers/gpu/drm/drm_plane.c
> > > +++ b/drivers/gpu/drm/drm_plane.c
> > > @@ -140,6 +140,26 @@
> > >   *     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.
> > > + *
> > > + *     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.
> > 
> > Does this intend to mean userspace has the kernel's blessing on choosing
> > an arbitrary size as long as it's smaller than &DRM_CAP_CURSOR_WIDTH x
> > &DRM_CAP_CURSOR_HEIGHT?
> > 
> > It's a bit to vague for me to make a confident interpretation whether I
> > can, or whether I should pretend I didn't see SIZE_HINTS and apply the
> > old logic, meaning only using the exact cap size.
> 
> Using the exact cap size is the only thing more or less
> guaranteed to work. But other approaches (such as probing
> the size with atomic/cursor ioctls) can also be used.

I think you should then just disallow drivers for exposing SIZE_HINTS
with the value 0, and make it a bug if they do, to let userspace know
when the value 0 means anything.

In other words, userspace should *not* ignore the property value being
0, but treat it as a kernel bug if there is a SIZE_HINTS only containing
a 0, until the value 0 has gotten any meaning. Otherwise I don't see how
it'll be usable in the future, since userspace doesn't know the
difference between 'legacy 0' and 'new 0' once it's defined to mean
anything.


Jonas

Jonas Ådahl March 17, 2023, 12:29 p.m. UTC | #4

On Fri, Mar 17, 2023 at 01:21:43PM +0100, Jonas Ådahl wrote:
> On Fri, Mar 17, 2023 at 01:33:25PM +0200, Ville Syrjälä wrote:
> > On Fri, Mar 17, 2023 at 11:34:16AM +0100, Jonas Ådahl wrote:
> > > On Mon, Mar 13, 2023 at 06:33:11PM +0200, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <[e-mail]>
> > > > 
> > > > 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
> > > > v3: Specify that value 0 is reserved for future use (basic idea from Jonas)
> > > >     Drop the note about typical hardware (Pekka)
> > > > 
> > > > Cc: Simon Ser <[e-mail]>
> > > > Cc: Jonas Ådahl <[e-mail]>
> > > > Cc: Daniel Stone <[e-mail]>
> > > > Cc: Pekka Paalanen <[e-mail]>
> > > > Acked-by: Harry Wentland <[e-mail]>
> > > > Acked-by: Pekka Paalanen <[e-mail]>
> > > > Signed-off-by: Ville Syrjälä <[e-mail]>
> > > > ---
> > > >  drivers/gpu/drm/drm_mode_config.c |  7 ++++
> > > >  drivers/gpu/drm/drm_plane.c       | 53 +++++++++++++++++++++++++++++++
> > > >  include/drm/drm_mode_config.h     |  5 +++
> > > >  include/drm/drm_plane.h           |  4 +++
> > > >  include/uapi/drm/drm_mode.h       | 11 +++++++
> > > >  5 files changed, 80 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..d2a6fdff1a57 100644
> > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > @@ -140,6 +140,26 @@
> > > >   *     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.
> > > > + *
> > > > + *     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.
> > > 
> > > Does this intend to mean userspace has the kernel's blessing on choosing
> > > an arbitrary size as long as it's smaller than &DRM_CAP_CURSOR_WIDTH x
> > > &DRM_CAP_CURSOR_HEIGHT?
> > > 
> > > It's a bit to vague for me to make a confident interpretation whether I
> > > can, or whether I should pretend I didn't see SIZE_HINTS and apply the
> > > old logic, meaning only using the exact cap size.
> > 
> > Using the exact cap size is the only thing more or less
> > guaranteed to work. But other approaches (such as probing
> > the size with atomic/cursor ioctls) can also be used.
> 
> I think you should then just disallow drivers for exposing SIZE_HINTS
> with the value 0, and make it a bug if they do, to let userspace know
> when the value 0 means anything.
> 
> In other words, userspace should *not* ignore the property value being
> 0, but treat it as a kernel bug if there is a SIZE_HINTS only containing
> a 0, until the value 0 has gotten any meaning. Otherwise I don't see how
> it'll be usable in the future, since userspace doesn't know the
> difference between 'legacy 0' and 'new 0' once it's defined to mean
> anything.

On a second thought, userspace needs to ignore it, to not fall apart
when running on never future kernels, you're right. Never mind.

I guess with "is reserved" implies that it's a bug if it's used before
it's defined to be anything, right?

Jonas

Ville Syrjälä March 17, 2023, 1:13 p.m. UTC | #5

On Fri, Mar 17, 2023 at 01:21:40PM +0100, Jonas Ådahl wrote:
> On Fri, Mar 17, 2023 at 01:33:25PM +0200, Ville Syrjälä wrote:
> > On Fri, Mar 17, 2023 at 11:34:16AM +0100, Jonas Ådahl wrote:
> > > On Mon, Mar 13, 2023 at 06:33:11PM +0200, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <[e-mail]>
> > > > 
> > > > 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
> > > > v3: Specify that value 0 is reserved for future use (basic idea from Jonas)
> > > >     Drop the note about typical hardware (Pekka)
> > > > 
> > > > Cc: Simon Ser <[e-mail]>
> > > > Cc: Jonas Ådahl <[e-mail]>
> > > > Cc: Daniel Stone <[e-mail]>
> > > > Cc: Pekka Paalanen <[e-mail]>
> > > > Acked-by: Harry Wentland <[e-mail]>
> > > > Acked-by: Pekka Paalanen <[e-mail]>
> > > > Signed-off-by: Ville Syrjälä <[e-mail]>
> > > > ---
> > > >  drivers/gpu/drm/drm_mode_config.c |  7 ++++
> > > >  drivers/gpu/drm/drm_plane.c       | 53 +++++++++++++++++++++++++++++++
> > > >  include/drm/drm_mode_config.h     |  5 +++
> > > >  include/drm/drm_plane.h           |  4 +++
> > > >  include/uapi/drm/drm_mode.h       | 11 +++++++
> > > >  5 files changed, 80 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..d2a6fdff1a57 100644
> > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > @@ -140,6 +140,26 @@
> > > >   *     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.
> > > > + *
> > > > + *     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.
> > > 
> > > Does this intend to mean userspace has the kernel's blessing on choosing
> > > an arbitrary size as long as it's smaller than &DRM_CAP_CURSOR_WIDTH x
> > > &DRM_CAP_CURSOR_HEIGHT?
> > > 
> > > It's a bit to vague for me to make a confident interpretation whether I
> > > can, or whether I should pretend I didn't see SIZE_HINTS and apply the
> > > old logic, meaning only using the exact cap size.
> > 
> > Using the exact cap size is the only thing more or less
> > guaranteed to work. But other approaches (such as probing
> > the size with atomic/cursor ioctls) can also be used.
> 
> I think you should then just disallow drivers for exposing SIZE_HINTS
> with the value 0, and make it a bug if they do, to let userspace know
> when the value 0 means anything.

Now I'm very confused. Previously you asked to have a special value
reserved for the case where the driver+hw can use any size below
the cap declated max. That is why I declared this blob=0 as a reserved
value for the future. But now you say you don't want that anymore?

> 
> In other words, userspace should *not* ignore the property value being
> 0, but treat it as a kernel bug if there is a SIZE_HINTS only containing
> a 0, until the value 0 has gotten any meaning. Otherwise I don't see how
> it'll be usable in the future, since userspace doesn't know the
> difference between 'legacy 0' and 'new 0' once it's defined to mean
> anything.
> 
> 
> Jonas

Ville Syrjälä March 17, 2023, 1:15 p.m. UTC | #6

On Fri, Mar 17, 2023 at 01:29:13PM +0100, Jonas Ådahl wrote:
> On Fri, Mar 17, 2023 at 01:21:43PM +0100, Jonas Ådahl wrote:
> > On Fri, Mar 17, 2023 at 01:33:25PM +0200, Ville Syrjälä wrote:
> > > On Fri, Mar 17, 2023 at 11:34:16AM +0100, Jonas Ådahl wrote:
> > > > On Mon, Mar 13, 2023 at 06:33:11PM +0200, Ville Syrjala wrote:
> > > > > From: Ville Syrjälä <[e-mail]>
> > > > > 
> > > > > 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
> > > > > v3: Specify that value 0 is reserved for future use (basic idea from Jonas)
> > > > >     Drop the note about typical hardware (Pekka)
> > > > > 
> > > > > Cc: Simon Ser <[e-mail]>
> > > > > Cc: Jonas Ådahl <[e-mail]>
> > > > > Cc: Daniel Stone <[e-mail]>
> > > > > Cc: Pekka Paalanen <[e-mail]>
> > > > > Acked-by: Harry Wentland <[e-mail]>
> > > > > Acked-by: Pekka Paalanen <[e-mail]>
> > > > > Signed-off-by: Ville Syrjälä <[e-mail]>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_mode_config.c |  7 ++++
> > > > >  drivers/gpu/drm/drm_plane.c       | 53 +++++++++++++++++++++++++++++++
> > > > >  include/drm/drm_mode_config.h     |  5 +++
> > > > >  include/drm/drm_plane.h           |  4 +++
> > > > >  include/uapi/drm/drm_mode.h       | 11 +++++++
> > > > >  5 files changed, 80 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..d2a6fdff1a57 100644
> > > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > > @@ -140,6 +140,26 @@
> > > > >   *     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.
> > > > > + *
> > > > > + *     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.
> > > > 
> > > > Does this intend to mean userspace has the kernel's blessing on choosing
> > > > an arbitrary size as long as it's smaller than &DRM_CAP_CURSOR_WIDTH x
> > > > &DRM_CAP_CURSOR_HEIGHT?
> > > > 
> > > > It's a bit to vague for me to make a confident interpretation whether I
> > > > can, or whether I should pretend I didn't see SIZE_HINTS and apply the
> > > > old logic, meaning only using the exact cap size.
> > > 
> > > Using the exact cap size is the only thing more or less
> > > guaranteed to work. But other approaches (such as probing
> > > the size with atomic/cursor ioctls) can also be used.
> > 
> > I think you should then just disallow drivers for exposing SIZE_HINTS
> > with the value 0, and make it a bug if they do, to let userspace know
> > when the value 0 means anything.
> > 
> > In other words, userspace should *not* ignore the property value being
> > 0, but treat it as a kernel bug if there is a SIZE_HINTS only containing
> > a 0, until the value 0 has gotten any meaning. Otherwise I don't see how
> > it'll be usable in the future, since userspace doesn't know the
> > difference between 'legacy 0' and 'new 0' once it's defined to mean
> > anything.
> 
> On a second thought, userspace needs to ignore it, to not fall apart
> when running on never future kernels, you're right. Never mind.

OK, I guess you still want it :)

> 
> I guess with "is reserved" implies that it's a bug if it's used before
> it's defined to be anything, right?

Yes. I didn't want to specify the actual behaviour right now since
we have no drivers lining up to implement any of it. So just trying
to keep the door slightly ajar for the future.

Jonas Ådahl March 17, 2023, 1:18 p.m. UTC | #7

On Fri, Mar 17, 2023 at 03:15:56PM +0200, Ville Syrjälä wrote:
> On Fri, Mar 17, 2023 at 01:29:13PM +0100, Jonas Ådahl wrote:
> > On Fri, Mar 17, 2023 at 01:21:43PM +0100, Jonas Ådahl wrote:
> > > On Fri, Mar 17, 2023 at 01:33:25PM +0200, Ville Syrjälä wrote:
> > > > On Fri, Mar 17, 2023 at 11:34:16AM +0100, Jonas Ådahl wrote:
> > > > > On Mon, Mar 13, 2023 at 06:33:11PM +0200, Ville Syrjala wrote:
> > > > > > From: Ville Syrjälä <[e-mail]>
> > > > > > 
> > > > > > 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
> > > > > > v3: Specify that value 0 is reserved for future use (basic idea from Jonas)
> > > > > >     Drop the note about typical hardware (Pekka)
> > > > > > 
> > > > > > Cc: Simon Ser <[e-mail]>
> > > > > > Cc: Jonas Ådahl <[e-mail]>
> > > > > > Cc: Daniel Stone <[e-mail]>
> > > > > > Cc: Pekka Paalanen <[e-mail]>
> > > > > > Acked-by: Harry Wentland <[e-mail]>
> > > > > > Acked-by: Pekka Paalanen <[e-mail]>
> > > > > > Signed-off-by: Ville Syrjälä <[e-mail]>
> > > > > > ---
> > > > > >  drivers/gpu/drm/drm_mode_config.c |  7 ++++
> > > > > >  drivers/gpu/drm/drm_plane.c       | 53 +++++++++++++++++++++++++++++++
> > > > > >  include/drm/drm_mode_config.h     |  5 +++
> > > > > >  include/drm/drm_plane.h           |  4 +++
> > > > > >  include/uapi/drm/drm_mode.h       | 11 +++++++
> > > > > >  5 files changed, 80 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..d2a6fdff1a57 100644
> > > > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > > > @@ -140,6 +140,26 @@
> > > > > >   *     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.
> > > > > > + *
> > > > > > + *     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.
> > > > > 
> > > > > Does this intend to mean userspace has the kernel's blessing on choosing
> > > > > an arbitrary size as long as it's smaller than &DRM_CAP_CURSOR_WIDTH x
> > > > > &DRM_CAP_CURSOR_HEIGHT?
> > > > > 
> > > > > It's a bit to vague for me to make a confident interpretation whether I
> > > > > can, or whether I should pretend I didn't see SIZE_HINTS and apply the
> > > > > old logic, meaning only using the exact cap size.
> > > > 
> > > > Using the exact cap size is the only thing more or less
> > > > guaranteed to work. But other approaches (such as probing
> > > > the size with atomic/cursor ioctls) can also be used.
> > > 
> > > I think you should then just disallow drivers for exposing SIZE_HINTS
> > > with the value 0, and make it a bug if they do, to let userspace know
> > > when the value 0 means anything.
> > > 
> > > In other words, userspace should *not* ignore the property value being
> > > 0, but treat it as a kernel bug if there is a SIZE_HINTS only containing
> > > a 0, until the value 0 has gotten any meaning. Otherwise I don't see how
> > > it'll be usable in the future, since userspace doesn't know the
> > > difference between 'legacy 0' and 'new 0' once it's defined to mean
> > > anything.
> > 
> > On a second thought, userspace needs to ignore it, to not fall apart
> > when running on never future kernels, you're right. Never mind.
> 
> OK, I guess you still want it :)
> 
> > 
> > I guess with "is reserved" implies that it's a bug if it's used before
> > it's defined to be anything, right?
> 
> Yes. I didn't want to specify the actual behaviour right now since
> we have no drivers lining up to implement any of it. So just trying
> to keep the door slightly ajar for the future.

Yep, thanks for that, and sorry about the confusion. As long as I in the
future can do 'if (value == 0) do_it_the_new_way()' (if they
materialize) I'm happy.


Jonas

Patch

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..d2a6fdff1a57 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -140,6 +140,26 @@ 
  *     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.
+ *
+ *     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)
@@ -1582,3 +1602,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.
  *