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

Wed Feb 22 18:34:00 UTC 2023

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ä <ville.syrjala at linux.intel.com>
> > > > > > > 
> > > > > > > Add a new immutable plane property by which a plane can advertise
> > > > > > > a handful of recommended plane sizes. This would be mostly exposed
> > > > > > > by cursor planes as a slightly more capable replacement for
> > > > > > > the DRM_CAP_CURSOR_WIDTH/HEIGHT caps, which can only declare
> > > > > > > a one size fits all limit for the whole device.
> > > > > > > 
> > > > > > > Currently eg. amdgpu/i915/nouveau just advertize the max cursor
> > > > > > > size via the cursor size caps. But always using the max sized
> > > > > > > cursor can waste a surprising amount of power, so a better
> > > > > > > stragey is desirable.
> > > > > > > 
> > > > > > > Most other drivers don't specify any cursor size at all, in
> > > > > > > which case the ioctl code just claims that 64x64 is a great
> > > > > > > choice. Whether that is actually true is debatable.
> > > > > > > 
> > > > > > > A poll of various compositor developers informs us that
> > > > > > > blindly probing with setcursor/atomic ioctl to determine
> > > > > > > suitable cursor sizes is not acceptable, thus the
> > > > > > > introduction of the new property to supplant the cursor
> > > > > > > size caps. The compositor will now be free to select a
> > > > > > > more optimal cursor size from the short list of options.
> > > > > > > 
> > > > > > > Note that the reported sizes (either via the property or the
> > > > > > > caps) make no claims about things such as plane scaling. So
> > > > > > > these things should only really be consulted for simple
> > > > > > > "cursor like" use cases.
> > > > > > > 
> > > > > > > v2: Try to add some docs
> > > > > > > 
> > > > > > > Cc: Simon Ser <contact at emersion.fr>
> > > > > > > Cc: Jonas Ådahl <jadahl at redhat.com>
> > > > > > > Cc: Daniel Stone <daniel at fooishbar.org>
> > > > > > > Cc: Pekka Paalanen <pekka.paalanen at collabora.com>
> > > > > > > Acked-by: Harry Wentland <harry.wentland at amd.com>
> > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/drm_mode_config.c |  7 +++++
> > > > > > >  drivers/gpu/drm/drm_plane.c       | 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.
> > 
> > How about various ARM drivers, where the cursor plane is a regular
> > overlay plane with an artificial 'cursor' stamp on it?
> 
> They don't even bother with the size cap currently. So the
> generic ioctl code currently just decides that 64x64 is good
> enough for them.
> 
> > 
> > Either way, the documentation creates an impossible expectation -
> > drivers, existing of future, that does not "support for a very limited
> > set of sizes" but actually any size below a limit, can't communicate to
> > userspace that it can handle cursor buffers with an arbitrary sizes,
> > without userspace breaking on todays kernels.
> 
> I don't see how anything would break. You just won't get a 100%
> optimal result potentially if you don't declare any smaller
> sizes. And I think we can always specify that magic 0x0 value
> later (or a new cap/etc) should an actual user for it appear.

Or maybe better than the 0x0 magic value inside the blob would
be to just have the property with value 0 (ie. no blob at all)?

-- 
Ville Syrjälä
Intel