Re: [PATCH v2 1/2] drm: Introduce plane SIZE_HINTS property
On Wed, Feb 28, 2024 at 10:12:28AM +, Simon Ser wrote: > On Tuesday, February 27th, 2024 at 20:35, Ville Syrjala > wrote: > > > From: Ville Syrjälä > > > > 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? At least for i915 the current igts already cover a superset of what the property reports. But I guess we could add another test that explicitly tests the sizes reported by the hint as well, if not already covered by other tests. > > > 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 > > Cc: Jonas Ådahl > > Cc: Daniel Stone > > Cc: Sameer Lattannavar > > Cc: Sebastian Wick > > Acked-by: Harry Wentland > > Acked-by: Pekka Paalanen > > Signed-off-by: Ville Syrjälä > > --- > > 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. > > + * _CAP_CURSOR_WIDTH and _CAP_CURSOR_HEIGHT) to determine > > + * the appropriate plane size to use. > > */ > > > > static unsigned int drm_num_planes(struct drm_device *dev) > > @@
Re: [PATCH v2 1/2] drm: Introduce plane SIZE_HINTS property
Hi, On Tue, 27 Feb 2024 at 19:35, Ville Syrjala wrote: > 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. Acked-by: Daniel Stone Cheers, Daniel
Re: [PATCH v2 1/2] drm: Introduce plane SIZE_HINTS property
On Tuesday, February 27th, 2024 at 20:35, Ville Syrjala wrote: > From: Ville Syrjälä > > 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 > Cc: Jonas Ådahl > Cc: Daniel Stone > Cc: Sameer Lattannavar > Cc: Sebastian Wick > Acked-by: Harry Wentland > Acked-by: Pekka Paalanen > Signed-off-by: Ville Syrjälä > --- > 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. > + * _CAP_CURSOR_WIDTH and _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
Re: [PATCH v2 1/2] drm: Introduce plane SIZE_HINTS property
On Tue, Feb 27, 2024 at 09:35:22PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä > > 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. > > Userspace consumer in the form of mutter seems ready: > https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/3165 > > 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 > Cc: Jonas Ådahl > Cc: Daniel Stone > Cc: Sameer Lattannavar > Cc: Sebastian Wick > Acked-by: Harry Wentland > Acked-by: Pekka Paalanen > Signed-off-by: Ville Syrjälä > --- > 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. > + * _CAP_CURSOR_WIDTH and _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, > +
Re: [PATCH v2 1/2] drm: Introduce plane SIZE_HINTS property
On Tue, Feb 14, 2023 at 01:19:51PM +0200, Ville Syrjälä wrote: > On Tue, Feb 14, 2023 at 12:01:49PM +0100, Jonas Ådahl wrote: > > On Tue, Feb 14, 2023 at 12:28:44PM +0200, Ville Syrjälä wrote: > > > On Tue, Feb 14, 2023 at 10:54:27AM +0100, Jonas Ådahl wrote: > > > > On Tue, Feb 14, 2023 at 11:25:56AM +0200, Ville Syrjälä wrote: > > > > > 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ä > > > > > > > > > > > > > > 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 > > > > > > > Cc: Jonas Ådahl > > > > > > > Cc: Daniel Stone > > > > > > > Cc: Pekka Paalanen > > > > > > > Acked-by: Harry Wentland > > > > > > > Signed-off-by: Ville Syrjälä > > > > > > > --- > > > > > > > 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. > > > > > > > + * > > > >
Re: [PATCH v2 1/2] drm: Introduce plane SIZE_HINTS property
On Tue, Feb 14, 2023 at 02:27:19PM -0500, Harry Wentland wrote: > > > On 2/14/23 05:28, Ville Syrjälä wrote: > > On Tue, Feb 14, 2023 at 10:54:27AM +0100, Jonas Ådahl wrote: > >> On Tue, Feb 14, 2023 at 11:25:56AM +0200, Ville Syrjälä wrote: > >>> 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ä > > > > 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 > > Cc: Jonas Ådahl > > Cc: Daniel Stone > > Cc: Pekka Paalanen > > Acked-by: Harry Wentland > > Signed-off-by: Ville Syrjälä > > --- > > 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
Re: [PATCH v2 1/2] drm: Introduce plane SIZE_HINTS property
On 2/14/23 05:28, Ville Syrjälä wrote: On Tue, Feb 14, 2023 at 10:54:27AM +0100, Jonas Ådahl wrote: On Tue, Feb 14, 2023 at 11:25:56AM +0200, Ville Syrjälä wrote: 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ä 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 Cc: Jonas Ådahl Cc: Daniel Stone Cc: Pekka Paalanen Acked-by: Harry Wentland Signed-off-by: Ville Syrjälä --- 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. None the less, userspace not seeing SIZE_HINTS will be required to indefinitely use the existing "old" behavior using the size cap as the buffer size with a fallback, and drivers without any size limitations that, i.e. details that are hard to express with a set of accepted sizes, will still use the inoptimal buffer sizes. If I read [1] correctly, AMD has no particular size limitations other than a size limit, but without a SIZE_HINTS, userspace still needs to use the maximum size. Simon pointed out they have pretty much the same exact limits as i915. Ie. only a few power of two sizes, and stride must match width. That's an artificial limitation in the driver. The HW has no such limitation and it would be nice to drop that from our driver as well. Harry [1] https://gitlab.freedesktop.org/drm/intel/-/issues/7687#note_1760865 Jonas if
Re: [PATCH v2 1/2] drm: Introduce plane SIZE_HINTS property
On Tue, Feb 14, 2023 at 12:01:49PM +0100, Jonas Ådahl wrote: > On Tue, Feb 14, 2023 at 12:28:44PM +0200, Ville Syrjälä wrote: > > On Tue, Feb 14, 2023 at 10:54:27AM +0100, Jonas Ådahl wrote: > > > On Tue, Feb 14, 2023 at 11:25:56AM +0200, Ville Syrjälä wrote: > > > > 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ä > > > > > > > > > > > > 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 > > > > > > Cc: Jonas Ådahl > > > > > > Cc: Daniel Stone > > > > > > Cc: Pekka Paalanen > > > > > > Acked-by: Harry Wentland > > > > > > Signed-off-by: Ville Syrjälä > > > > > > --- > > > > > > 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
Re: [PATCH v2 1/2] drm: Introduce plane SIZE_HINTS property
On Tue, Feb 14, 2023 at 12:28:44PM +0200, Ville Syrjälä wrote: > On Tue, Feb 14, 2023 at 10:54:27AM +0100, Jonas Ådahl wrote: > > On Tue, Feb 14, 2023 at 11:25:56AM +0200, Ville Syrjälä wrote: > > > 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ä > > > > > > > > > > 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 > > > > > Cc: Jonas Ådahl > > > > > Cc: Daniel Stone > > > > > Cc: Pekka Paalanen > > > > > Acked-by: Harry Wentland > > > > > Signed-off-by: Ville Syrjälä > > > > > --- > > > > > 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
Re: [PATCH v2 1/2] drm: Introduce plane SIZE_HINTS property
On Tue, Feb 14, 2023 at 10:54:27AM +0100, Jonas Ådahl wrote: > On Tue, Feb 14, 2023 at 11:25:56AM +0200, Ville Syrjälä wrote: > > 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ä > > > > > > > > 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 > > > > Cc: Jonas Ådahl > > > > Cc: Daniel Stone > > > > Cc: Pekka Paalanen > > > > Acked-by: Harry Wentland > > > > Signed-off-by: Ville Syrjälä > > > > --- > > > > 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. > > None the less, userspace not seeing SIZE_HINTS will be required to > indefinitely use the existing "old" behavior using
Re: [PATCH v2 1/2] drm: Introduce plane SIZE_HINTS property
On Tue, Feb 14, 2023 at 11:25:56AM +0200, Ville Syrjälä wrote: > 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ä > > > > > > 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 > > > Cc: Jonas Ådahl > > > Cc: Daniel Stone > > > Cc: Pekka Paalanen > > > Acked-by: Harry Wentland > > > Signed-off-by: Ville Syrjälä > > > --- > > > 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. None the less, userspace not seeing SIZE_HINTS will be required to indefinitely use the existing "old" behavior using the size cap as the buffer size with a fallback, and drivers without any size limitations that, i.e. details that are hard to express with a set of accepted sizes, will still use the inoptimal buffer sizes. If I read [1] correctly, AMD has no particular size limitations other than a size limit, but without a SIZE_HINTS, userspace still
Re: [PATCH v2 1/2] drm: Introduce plane SIZE_HINTS property
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ä > > > > 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 > > Cc: Jonas Ådahl > > Cc: Daniel Stone > > Cc: Pekka Paalanen > > Acked-by: Harry Wentland > > Signed-off-by: Ville Syrjälä > > --- > > 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); > > + > >
Re: [PATCH v2 1/2] drm: Introduce plane SIZE_HINTS property
On Thu, 9 Feb 2023 15:10:38 +0200 Ville Syrjälä wrote: > On Thu, Feb 09, 2023 at 01:58:55PM +0200, Pekka Paalanen wrote: > > On Wed, 8 Feb 2023 23:10:16 +0200 > > Ville Syrjala wrote: > > > > > From: Ville Syrjälä > > > > > > 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 > > > Cc: Jonas Ådahl > > > Cc: Daniel Stone > > > Cc: Pekka Paalanen > > > Acked-by: Harry Wentland > > > Signed-off-by: Ville Syrjälä > > > --- > > > 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). > > > > Hi Ville, > > > > sounds good. Maybe a minor nit about "typical hardware". Would e.g. > > "legacy PC hardware" be more accurate? > > "legacy" doesn't feel quite right for current and upcoming hardware. It's an example, not everything. Although, I didn't expect current and upcoming hardware to keep such limitations either but to move towards universal rather than specialized planes. Maybe just drop the whole "(eg. ...)"? Thanks, pq pgpegT1nY7bt5.pgp Description: OpenPGP digital signature
Re: [PATCH v2 1/2] drm: Introduce plane SIZE_HINTS property
On Wed, Feb 08, 2023 at 11:10:16PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä > > 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 > Cc: Jonas Ådahl > Cc: Daniel Stone > Cc: Pekka Paalanen > Acked-by: Harry Wentland > Signed-off-by: Ville Syrjälä > --- > 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). 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, > +
Re: [PATCH v2 1/2] drm: Introduce plane SIZE_HINTS property
On Thu, Feb 09, 2023 at 01:58:55PM +0200, Pekka Paalanen wrote: > On Wed, 8 Feb 2023 23:10:16 +0200 > Ville Syrjala wrote: > > > From: Ville Syrjälä > > > > 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 > > Cc: Jonas Ådahl > > Cc: Daniel Stone > > Cc: Pekka Paalanen > > Acked-by: Harry Wentland > > Signed-off-by: Ville Syrjälä > > --- > > 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). > > Hi Ville, > > sounds good. Maybe a minor nit about "typical hardware". Would e.g. > "legacy PC hardware" be more accurate? "legacy" doesn't feel quite right for current and upcoming hardware. -- Ville Syrjälä Intel
Re: [PATCH v2 1/2] drm: Introduce plane SIZE_HINTS property
On Wed, 8 Feb 2023 23:10:16 +0200
Ville Syrjala wrote:
> From: Ville Syrjälä
>
> 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
> Cc: Jonas Ådahl
> Cc: Daniel Stone
> Cc: Pekka Paalanen
> Acked-by: Harry Wentland
> Signed-off-by: Ville Syrjälä
> ---
> 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).
Hi Ville,
sounds good. Maybe a minor nit about "typical hardware". Would e.g.
"legacy PC hardware" be more accurate?
Acked-by: Pekka Paalanen
but let's see if that other option (allocate cap size, make FB with
image size, guarantee zero padding) from my other email would be viable
too.
Thanks,
pq
> */
>
> 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 = >mode_config;
> + struct drm_property_blob *blob;
> +
> + blob = drm_property_create_blob(dev,
> + array_size(sizeof(hints[0]), num_hints),
> +
