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ä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > > > > > > > > > Add a new immutable plane property by which a plane can advertise > > > > > > a handful of recommended plane sizes. This would be mostly exposed > > > > > > by cursor planes as a slightly more capable replacement for > > > > > > the DRM_CAP_CURSOR_WIDTH/HEIGHT caps, which can only declare > > > > > > a one size fits all limit for the whole device. > > > > > > > > > > > > Currently eg. amdgpu/i915/nouveau just advertize the max cursor > > > > > > size via the cursor size caps. But always using the max sized > > > > > > cursor can waste a surprising amount of power, so a better > > > > > > stragey is desirable. > > > > > > > > > > > > Most other drivers don't specify any cursor size at all, in > > > > > > which case the ioctl code just claims that 64x64 is a great > > > > > > choice. Whether that is actually true is debatable. > > > > > > > > > > > > A poll of various compositor developers informs us that > > > > > > blindly probing with setcursor/atomic ioctl to determine > > > > > > suitable cursor sizes is not acceptable, thus the > > > > > > introduction of the new property to supplant the cursor > > > > > > size caps. The compositor will now be free to select a > > > > > > more optimal cursor size from the short list of options. > > > > > > > > > > > > Note that the reported sizes (either via the property or the > > > > > > caps) make no claims about things such as plane scaling. So > > > > > > these things should only really be consulted for simple > > > > > > "cursor like" use cases. > > > > > > > > > > > > v2: Try to add some docs > > > > > > 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@xxxxxxxxxxx> > > > > > > Cc: Jonas Ådahl <jadahl@xxxxxxxxxx> > > > > > > Cc: Daniel Stone <daniel@xxxxxxxxxxxxx> > > > > > > Cc: Pekka Paalanen <pekka.paalanen@xxxxxxxxxxxxx> > > > > > > Acked-by: Harry Wentland <harry.wentland@xxxxxxx> > > > > > > Acked-by: Pekka Paalanen <pekka.paalanen@xxxxxxxxxxxxx> > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > > > --- > > > > > > 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
