| Message ID | 20230208040911.12590-2-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Add plane SIZE_HINTS property | expand |
Hi Ville,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on drm-tip/drm-tip]
url: https://github.com/intel-lab-lkp/linux/commits/Ville-Syrjala/drm-Introduce-plane-SIZE_HINTS-property/20230208-121141
base: git://anongit.freedesktop.org/drm/drm-tip drm-tip
patch link: https://lore.kernel.org/r/20230208040911.12590-2-ville.syrjala%40linux.intel.com
patch subject: [Intel-gfx] [PATCH 1/2] drm: Introduce plane SIZE_HINTS property
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230208/202302081339.rLZ6hx1B-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/419642b423bdc6e337b5daabecb51173dd206cb5
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Ville-Syrjala/drm-Introduce-plane-SIZE_HINTS-property/20230208-121141
git checkout 419642b423bdc6e337b5daabecb51173dd206cb5
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/gpu/drm/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/drm_plane.c:1601: warning: expecting prototype for drm_plane_add_size_hint_property(). Prototype was for drm_plane_add_size_hints_property() instead
vim +1601 drivers/gpu/drm/drm_plane.c
1585
1586 /**
1587 * drm_plane_add_size_hint_property - create a size hint property
1588 *
1589 * @plane: drm plane
1590 * @hints: size hints
1591 * @num_hints: number of size hints
1592 *
1593 * Create a size hints property for the plane.
1594 *
1595 * RETURNS:
1596 * Zero for success or -errno
1597 */
1598 int drm_plane_add_size_hints_property(struct drm_plane *plane,
1599 const struct drm_plane_size_hint *hints,
1600 int num_hints)
> 1601 {
Hi Ville,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on drm-tip/drm-tip]
url: https://github.com/intel-lab-lkp/linux/commits/Ville-Syrjala/drm-Introduce-plane-SIZE_HINTS-property/20230208-121141
base: git://anongit.freedesktop.org/drm/drm-tip drm-tip
patch link: https://lore.kernel.org/r/20230208040911.12590-2-ville.syrjala%40linux.intel.com
patch subject: [Intel-gfx] [PATCH 1/2] drm: Introduce plane SIZE_HINTS property
config: x86_64-rhel-8.3-kselftests (https://download.01.org/0day-ci/archive/20230208/202302081454.AoNxo1Kr-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/419642b423bdc6e337b5daabecb51173dd206cb5
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Ville-Syrjala/drm-Introduce-plane-SIZE_HINTS-property/20230208-121141
git checkout 419642b423bdc6e337b5daabecb51173dd206cb5
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 olddefconfig
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/gpu/drm/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/drm_plane.c:1601: warning: expecting prototype for drm_plane_add_size_hint_property(). Prototype was for drm_plane_add_size_hints_property() instead
vim +1601 drivers/gpu/drm/drm_plane.c
1585
1586 /**
1587 * drm_plane_add_size_hint_property - create a size hint property
1588 *
1589 * @plane: drm plane
1590 * @hints: size hints
1591 * @num_hints: number of size hints
1592 *
1593 * Create a size hints property for the plane.
1594 *
1595 * RETURNS:
1596 * Zero for success or -errno
1597 */
1598 int drm_plane_add_size_hints_property(struct drm_plane *plane,
1599 const struct drm_plane_size_hint *hints,
1600 int num_hints)
> 1601 {
On Wed, 8 Feb 2023 06:09:10 +0200 Ville Syrjala <ville.syrjala@linux.intel.com> 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. > > 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> > 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 | 33 +++++++++++++++++++++++++++++++ > include/drm/drm_mode_config.h | 5 +++++ > include/drm/drm_plane.h | 4 ++++ > include/uapi/drm/drm_mode.h | 5 +++++ > 5 files changed, 54 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..d0a277f4be1f 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -1582,3 +1582,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..ed9f6938dca1 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_property: 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..4f0551d7f481 100644 > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -849,6 +849,11 @@ struct drm_color_lut { > __u16 reserved; > }; > > +struct drm_plane_size_hint { > + __u16 width; > + __u16 height; > +}; Hi Ville, uAPI documentation is missing. When there is just a single recommended size listed, userspace has it easy: allocate a pair of DRM dumb buffers for each active CRTC, do atomic test commits with those, and if the test succeeds, copy in the pixels and fill the padding. What if we have multiple or a huge number of possible sizes? Probably for each KMS reconfiguration cycle (which could be up to every refresh cycle) userspace would need to allocate a new dumb buffer to have the right size, and then test. Is that something we can actually afford to do? I don't know. Therefore maybe this proposal is a good compromise. The driver lists *few* sizes that are roughly equally likely to work, that is, if the cursor plane does not work, it's not because of the size. Userspace pre-allocates DRM dumb buffers for each size. When attempting to use a cursor plane, userspace picks the smallest size that suffices, and tests only with that. Seems fine to me. The uAPI docs should explain what userspace is expected to do with it. Thanks, pq > + > /** > * struct hdr_metadata_infoframe - HDR Metadata Infoframe Data. > *
On Wed, Feb 08, 2023 at 02:13:12PM +0200, Pekka Paalanen wrote: > On Wed, 8 Feb 2023 06:09:10 +0200 > Ville Syrjala <ville.syrjala@linux.intel.com> 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. > > > > 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> > > 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 | 33 +++++++++++++++++++++++++++++++ > > include/drm/drm_mode_config.h | 5 +++++ > > include/drm/drm_plane.h | 4 ++++ > > include/uapi/drm/drm_mode.h | 5 +++++ > > 5 files changed, 54 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..d0a277f4be1f 100644 > > --- a/drivers/gpu/drm/drm_plane.c > > +++ b/drivers/gpu/drm/drm_plane.c > > @@ -1582,3 +1582,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..ed9f6938dca1 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_property: 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..4f0551d7f481 100644 > > --- a/include/uapi/drm/drm_mode.h > > +++ b/include/uapi/drm/drm_mode.h > > @@ -849,6 +849,11 @@ struct drm_color_lut { > > __u16 reserved; > > }; > > > > +struct drm_plane_size_hint { > > + __u16 width; > > + __u16 height; > > +}; > > Hi Ville, > > uAPI documentation is missing. > > When there is just a single recommended size listed, userspace has it > easy: allocate a pair of DRM dumb buffers for each active CRTC, do > atomic test commits with those, and if the test succeeds, copy in the > pixels and fill the padding. > > What if we have multiple or a huge number of possible sizes? Probably > for each KMS reconfiguration cycle (which could be up to every refresh > cycle) userspace would need to allocate a new dumb buffer to have the > right size, and then test. Is that something we can actually afford to > do? I don't know. Why would you allocate multiple buffers? Just allocate one with the max size and then you can reuse it at any smaller size as needed. > > Therefore maybe this proposal is a good compromise. The driver lists > *few* sizes that are roughly equally likely to work, that is, if the > cursor plane does not work, it's not because of the size. Userspace > pre-allocates DRM dumb buffers for each size. When attempting to use a > cursor plane, userspace picks the smallest size that suffices, and tests > only with that. Yeah, don't see why you ever retry multiple times with increased cursor size. If the small size doesn't work then surely the larger size won't either (larger size requiring more hw resources to operate). > > Seems fine to me. The uAPI docs should explain what userspace is > expected to do with it. > > > Thanks, > pq > > > + > > /** > > * struct hdr_metadata_infoframe - HDR Metadata Infoframe Data. > > *
On 2/7/23 23:09, 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. > > 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> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> This would be great to have. amdgpu could take advantage of it as well. I didn't have a thorough look at the details of this implementation but like what it does, so this is Acked-by: Harry Wentland <harry.wentland@amd.com> Harry > --- > drivers/gpu/drm/drm_mode_config.c | 7 +++++++ > drivers/gpu/drm/drm_plane.c | 33 +++++++++++++++++++++++++++++++ > include/drm/drm_mode_config.h | 5 +++++ > include/drm/drm_plane.h | 4 ++++ > include/uapi/drm/drm_mode.h | 5 +++++ > 5 files changed, 54 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..d0a277f4be1f 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -1582,3 +1582,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..ed9f6938dca1 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_property: 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..4f0551d7f481 100644 > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -849,6 +849,11 @@ struct drm_color_lut { > __u16 reserved; > }; > > +struct drm_plane_size_hint { > + __u16 width; > + __u16 height; > +}; > + > /** > * struct hdr_metadata_infoframe - HDR Metadata Infoframe Data. > *
On Wed, Feb 08, 2023 at 03:03:49PM +0200, Ville Syrjälä wrote: > On Wed, Feb 08, 2023 at 02:13:12PM +0200, Pekka Paalanen wrote: > > On Wed, 8 Feb 2023 06:09:10 +0200 > > Ville Syrjala <ville.syrjala@linux.intel.com> 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. > > > > > > 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> > > > 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 | 33 +++++++++++++++++++++++++++++++ > > > include/drm/drm_mode_config.h | 5 +++++ > > > include/drm/drm_plane.h | 4 ++++ > > > include/uapi/drm/drm_mode.h | 5 +++++ > > > 5 files changed, 54 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..d0a277f4be1f 100644 > > > --- a/drivers/gpu/drm/drm_plane.c > > > +++ b/drivers/gpu/drm/drm_plane.c > > > @@ -1582,3 +1582,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..ed9f6938dca1 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_property: 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..4f0551d7f481 100644 > > > --- a/include/uapi/drm/drm_mode.h > > > +++ b/include/uapi/drm/drm_mode.h > > > @@ -849,6 +849,11 @@ struct drm_color_lut { > > > __u16 reserved; > > > }; > > > > > > +struct drm_plane_size_hint { > > > + __u16 width; > > > + __u16 height; > > > +}; > > > > Hi Ville, > > > > uAPI documentation is missing. > > > > When there is just a single recommended size listed, userspace has it > > easy: allocate a pair of DRM dumb buffers for each active CRTC, do > > atomic test commits with those, and if the test succeeds, copy in the > > pixels and fill the padding. > > > > What if we have multiple or a huge number of possible sizes? Probably > > for each KMS reconfiguration cycle (which could be up to every refresh > > cycle) userspace would need to allocate a new dumb buffer to have the > > right size, and then test. Is that something we can actually afford to > > do? I don't know. > > Why would you allocate multiple buffers? Just allocate one > with the max size and then you can reuse it at any smaller > size as needed. Unfortunately the use of gbm here means the stride would be wrong for the smaller sizes. So I guess a different buffer for each size is what you need to do. Unless you can just ignore the original stride when filling the buffer. > > > > > Therefore maybe this proposal is a good compromise. The driver lists > > *few* sizes that are roughly equally likely to work, that is, if the > > cursor plane does not work, it's not because of the size. Userspace > > pre-allocates DRM dumb buffers for each size. When attempting to use a > > cursor plane, userspace picks the smallest size that suffices, and tests > > only with that. > > Yeah, don't see why you ever retry multiple times with increased > cursor size. If the small size doesn't work then surely the > larger size won't either (larger size requiring more hw resources > to operate). > > > > > Seems fine to me. The uAPI docs should explain what userspace is > > expected to do with it. > > > > > > Thanks, > > pq > > > > > + > > > /** > > > * struct hdr_metadata_infoframe - HDR Metadata Infoframe Data. > > > * > > -- > Ville Syrjälä > Intel
On Wed, 8 Feb 2023 23:16:56 +0200 Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Wed, Feb 08, 2023 at 03:03:49PM +0200, Ville Syrjälä wrote: > > On Wed, Feb 08, 2023 at 02:13:12PM +0200, Pekka Paalanen wrote: > > > On Wed, 8 Feb 2023 06:09:10 +0200 > > > Ville Syrjala <ville.syrjala@linux.intel.com> 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. > > > > > > > > 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> > > > > 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 | 33 +++++++++++++++++++++++++++++++ > > > > include/drm/drm_mode_config.h | 5 +++++ > > > > include/drm/drm_plane.h | 4 ++++ > > > > include/uapi/drm/drm_mode.h | 5 +++++ > > > > 5 files changed, 54 insertions(+) ... > > > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > > > > index 46becedf5b2f..4f0551d7f481 100644 > > > > --- a/include/uapi/drm/drm_mode.h > > > > +++ b/include/uapi/drm/drm_mode.h > > > > @@ -849,6 +849,11 @@ struct drm_color_lut { > > > > __u16 reserved; > > > > }; > > > > > > > > +struct drm_plane_size_hint { > > > > + __u16 width; > > > > + __u16 height; > > > > +}; > > > > > > Hi Ville, > > > > > > uAPI documentation is missing. > > > > > > When there is just a single recommended size listed, userspace has it > > > easy: allocate a pair of DRM dumb buffers for each active CRTC, do > > > atomic test commits with those, and if the test succeeds, copy in the > > > pixels and fill the padding. > > > > > > What if we have multiple or a huge number of possible sizes? Probably > > > for each KMS reconfiguration cycle (which could be up to every refresh > > > cycle) userspace would need to allocate a new dumb buffer to have the > > > right size, and then test. Is that something we can actually afford to > > > do? I don't know. > > > > Why would you allocate multiple buffers? Just allocate one > > with the max size and then you can reuse it at any smaller > > size as needed. We also need to double-buffer. > Unfortunately the use of gbm here means the stride would > be wrong for the smaller sizes. So I guess a different > buffer for each size is what you need to do. Unless you > can just ignore the original stride when filling the buffer. Why would the stride be wrong? AddFB2 carries explicit stride. You mean hardware would likely not accept the row padding? When filling the buffer, we can simply fill the real buffer size. That's what we already do to pad smaller surfaces up to the cursor size. So essentially we would just re-do AddFB2 with any smaller size while keeping the real stride. That's a good idea. Maybe we could refine this so that userspace uses the stride and height implied by the caps for allocation, and then use the exact cursor image size for AddFB2? And have drivers pick any size between those two they can use. The kernel would need the userspace to promise that the padding is always zero-initialized, so the driver can simply scan out any area of the buffer it needs. Then we don't need SIZE_HINTS. Thanks, pq
On Thu, 9 Feb 2023 13:51:05 +0200 Pekka Paalanen <pekka.paalanen@collabora.com> wrote: > Maybe we could refine this so that userspace uses the stride and height > implied by the caps for allocation, and then use the exact cursor image > size for AddFB2? And have drivers pick any size between those two they > can use. The kernel would need the userspace to promise that the > padding is always zero-initialized, so the driver can simply scan out > any area of the buffer it needs. > > Then we don't need SIZE_HINTS. Would there be any problem with this? If this works, it would seem the superior solution to me, because userspace does not need to guess or test for the exact right size. Simply allocate at the CAP size, pad the cursor image with transparent pixels, and let the kernel scan out the optimal area. And if the kernel needs to do a pixel format conversion, it only needs to do the optimal minimum amount of work. Thanks, pq
On Tue, Feb 14, 2023 at 11:42:27AM +0200, Pekka Paalanen wrote: > On Thu, 9 Feb 2023 13:51:05 +0200 > Pekka Paalanen <pekka.paalanen@collabora.com> wrote: > > > Maybe we could refine this so that userspace uses the stride and height > > implied by the caps for allocation, and then use the exact cursor image > > size for AddFB2? And have drivers pick any size between those two they > > can use. The kernel would need the userspace to promise that the > > padding is always zero-initialized, so the driver can simply scan out > > any area of the buffer it needs. > > > > Then we don't need SIZE_HINTS. > > Would there be any problem with this? > > If this works, it would seem the superior solution to me, because > userspace does not need to guess or test for the exact right size. > Simply allocate at the CAP size, pad the cursor image with transparent > pixels, and let the kernel scan out the optimal area. No, the hardware cannot scan out a smaller area because the stride will be wrong. > > And if the kernel needs to do a pixel format conversion, it only needs > to do the optimal minimum amount of work. Involving the CPU (or GPU I suppose but that could involve big extra latencies) in the kernel to massage the pixels around every time seems extremely sub-optimal. Seems like we might as well use a software cursor at that point.
On Tue, 14 Feb 2023 12:27:45 +0200 Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Tue, Feb 14, 2023 at 11:42:27AM +0200, Pekka Paalanen wrote: > > On Thu, 9 Feb 2023 13:51:05 +0200 > > Pekka Paalanen <pekka.paalanen@collabora.com> wrote: > > > > > Maybe we could refine this so that userspace uses the stride and height > > > implied by the caps for allocation, and then use the exact cursor image > > > size for AddFB2? And have drivers pick any size between those two they > > > can use. The kernel would need the userspace to promise that the > > > padding is always zero-initialized, so the driver can simply scan out > > > any area of the buffer it needs. > > > > > > Then we don't need SIZE_HINTS. > > > > Would there be any problem with this? > > > > If this works, it would seem the superior solution to me, because > > userspace does not need to guess or test for the exact right size. > > Simply allocate at the CAP size, pad the cursor image with transparent > > pixels, and let the kernel scan out the optimal area. > > No, the hardware cannot scan out a smaller area because the > stride will be wrong. In another email of yours you said that hardware requires stride to be equivalent to the width. So it's not that hardware supports only specific strides, it must equal to width. That's really unfortunate and surprising. > > > > And if the kernel needs to do a pixel format conversion, it only needs > > to do the optimal minimum amount of work. > > Involving the CPU (or GPU I suppose but that could involve big extra > latencies) in the kernel to massage the pixels around every time > seems extremely sub-optimal. Seems like we might as well use a > software cursor at that point. I meant drivers that already do that anyway, because they cannot scan out ARGB8888 on the cursor plane. Thanks, pq
On Tue, Feb 14, 2023 at 01:17:45PM +0200, Pekka Paalanen wrote: > On Tue, 14 Feb 2023 12:27:45 +0200 > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > On Tue, Feb 14, 2023 at 11:42:27AM +0200, Pekka Paalanen wrote: > > > On Thu, 9 Feb 2023 13:51:05 +0200 > > > Pekka Paalanen <pekka.paalanen@collabora.com> wrote: > > > > > > > Maybe we could refine this so that userspace uses the stride and height > > > > implied by the caps for allocation, and then use the exact cursor image > > > > size for AddFB2? And have drivers pick any size between those two they > > > > can use. The kernel would need the userspace to promise that the > > > > padding is always zero-initialized, so the driver can simply scan out > > > > any area of the buffer it needs. > > > > > > > > Then we don't need SIZE_HINTS. > > > > > > Would there be any problem with this? > > > > > > If this works, it would seem the superior solution to me, because > > > userspace does not need to guess or test for the exact right size. > > > Simply allocate at the CAP size, pad the cursor image with transparent > > > pixels, and let the kernel scan out the optimal area. > > > > No, the hardware cannot scan out a smaller area because the > > stride will be wrong. > > In another email of yours you said that hardware requires stride to be > equivalent to the width. So it's not that hardware supports only > specific strides, it must equal to width. That's really unfortunate and > surprising. Yeah, probably some Windows legacy hangover that refuses to die. Ye olde Intel gen2 desktop chipsets (i845/i865) had a somewhat programmable stride for cursors (still POT, but could exceed the width), but the mobile chipsets (i830/i85x) did not. Unfortunately the mobile lineage won out and we've been stuck with this limitation ever since.
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..d0a277f4be1f 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -1582,3 +1582,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..ed9f6938dca1 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_property: 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..4f0551d7f481 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -849,6 +849,11 @@ struct drm_color_lut { __u16 reserved; }; +struct drm_plane_size_hint { + __u16 width; + __u16 height; +}; + /** * struct hdr_metadata_infoframe - HDR Metadata Infoframe Data. *