Re: [Intel-gfx] [PATCH v3 1/2] drm: Introduce plane SIZE_HINTS property

["Jonas Ådahl" <jadahl@gmail.com>]

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ä <ville.syrjala@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
> > > v3: Specify that value 0 is reserved for future use (basic idea from Jonas)
> > >     Drop the note about typical hardware (Pekka)
> > > 
> > > Cc: Simon Ser <contact@emersion.fr>
> > > Cc: Jonas Ådahl <jadahl@redhat.com>
> > > Cc: Daniel Stone <daniel@fooishbar.org>
> > > Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
> > > Acked-by: Harry Wentland <harry.wentland@amd.com>
> > > Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  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