| Message ID | 20230627035839.496399-3-zack@kde.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix cursor planes with virtualized drivers | expand |
On Mon, 26 Jun 2023 23:58:33 -0400 Zack Rusin <zack@kde.org> wrote: > From: Zack Rusin <zackr@vmware.com> > > Atomic modesetting code lacked support for specifying mouse cursor > hotspots. The legacy kms DRM_IOCTL_MODE_CURSOR2 had support for setting > the hotspot but the functionality was not implemented in the new atomic > paths. > > Due to the lack of hotspots in the atomic paths userspace compositors > completely disable atomic modesetting for drivers that require it (i.e. > all paravirtualized drivers). > > This change adds hotspot properties to the atomic codepaths throughtout > the DRM core and will allow enabling atomic modesetting for virtualized > drivers in the userspace. > > Signed-off-by: Zack Rusin <zackr@vmware.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: David Airlie <airlied@linux.ie> > Cc: Daniel Vetter <daniel@ffwll.ch> > --- > drivers/gpu/drm/drm_atomic_state_helper.c | 14 +++++++ > drivers/gpu/drm/drm_atomic_uapi.c | 20 +++++++++ > drivers/gpu/drm/drm_plane.c | 51 +++++++++++++++++++++++ > include/drm/drm_plane.h | 15 +++++++ > 4 files changed, 100 insertions(+) Hi Zack, where is the UAPI documentation for these new properties? I mean something ending up in the HTML docs like what other properties have in e.g. https://www.kernel.org/doc/html/latest/gpu/drm-kms.html#plane-composition-properties Otherwise looks fine to me. Could drm_plane_create_hotspot_properties() perhaps assert that the plane type is CURSOR? Thanks, pq > > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c > index 784e63d70a42..54975de44a0e 100644 > --- a/drivers/gpu/drm/drm_atomic_state_helper.c > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c > @@ -275,6 +275,20 @@ void __drm_atomic_helper_plane_state_reset(struct drm_plane_state *plane_state, > plane_state->normalized_zpos = val; > } > } > + > + if (plane->hotspot_x_property) { > + if (!drm_object_property_get_default_value(&plane->base, > + plane->hotspot_x_property, > + &val)) > + plane_state->hotspot_x = val; > + } > + > + if (plane->hotspot_y_property) { > + if (!drm_object_property_get_default_value(&plane->base, > + plane->hotspot_y_property, > + &val)) > + plane_state->hotspot_y = val; > + } > } > EXPORT_SYMBOL(__drm_atomic_helper_plane_state_reset); > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c > index 98d3b10c08ae..07a7b3f18df2 100644 > --- a/drivers/gpu/drm/drm_atomic_uapi.c > +++ b/drivers/gpu/drm/drm_atomic_uapi.c > @@ -593,6 +593,22 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, > } else if (plane->funcs->atomic_set_property) { > return plane->funcs->atomic_set_property(plane, state, > property, val); > + } else if (property == plane->hotspot_x_property) { > + if (plane->type != DRM_PLANE_TYPE_CURSOR) { > + drm_dbg_atomic(plane->dev, > + "[PLANE:%d:%s] is not a cursor plane: 0x%llx\n", > + plane->base.id, plane->name, val); > + return -EINVAL; > + } > + state->hotspot_x = val; > + } else if (property == plane->hotspot_y_property) { > + if (plane->type != DRM_PLANE_TYPE_CURSOR) { > + drm_dbg_atomic(plane->dev, > + "[PLANE:%d:%s] is not a cursor plane: 0x%llx\n", > + plane->base.id, plane->name, val); > + return -EINVAL; > + } > + state->hotspot_y = val; > } else { > drm_dbg_atomic(plane->dev, > "[PLANE:%d:%s] unknown property [PROP:%d:%s]\n", > @@ -653,6 +669,10 @@ drm_atomic_plane_get_property(struct drm_plane *plane, > *val = state->scaling_filter; > } else if (plane->funcs->atomic_get_property) { > return plane->funcs->atomic_get_property(plane, state, property, val); > + } else if (property == plane->hotspot_x_property) { > + *val = state->hotspot_x; > + } else if (property == plane->hotspot_y_property) { > + *val = state->hotspot_y; > } else { > drm_dbg_atomic(dev, > "[PLANE:%d:%s] unknown property [PROP:%d:%s]\n", > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > index a4a39f4834e2..ff1cc810d8f8 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -348,6 +348,10 @@ static int __drm_universal_plane_init(struct drm_device *dev, > drm_object_attach_property(&plane->base, config->prop_src_w, 0); > drm_object_attach_property(&plane->base, config->prop_src_h, 0); > } > + if (drm_core_check_feature(dev, DRIVER_CURSOR_HOTSPOT) && > + type == DRM_PLANE_TYPE_CURSOR) { > + drm_plane_create_hotspot_properties(plane); > + } > > if (format_modifier_count) > create_in_format_blob(dev, plane); > @@ -1067,6 +1071,11 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, > > fb->hot_x = req->hot_x; > fb->hot_y = req->hot_y; > + > + if (plane->hotspot_x_property && plane->state) > + plane->state->hotspot_x = req->hot_x; > + if (plane->hotspot_y_property && plane->state) > + plane->state->hotspot_y = req->hot_y; > } else { > fb = NULL; > } > @@ -1595,3 +1604,45 @@ int drm_plane_create_scaling_filter_property(struct drm_plane *plane, > return 0; > } > EXPORT_SYMBOL(drm_plane_create_scaling_filter_property); > + > +/** > + * drm_plane_create_hotspot_properties - creates the mouse hotspot > + * properties and attaches them to the given cursor plane > + * > + * @plane: drm cursor plane > + * > + * This function lets driver to enable the mouse hotspot property on a given > + * cursor plane. > + * > + * RETURNS: > + * Zero for success or -errno > + */ > +int drm_plane_create_hotspot_properties(struct drm_plane *plane) > +{ > + struct drm_property *prop_x; > + struct drm_property *prop_y; > + > + drm_WARN_ON(plane->dev, > + !drm_core_check_feature(plane->dev, > + DRIVER_CURSOR_HOTSPOT)); > + > + prop_x = drm_property_create_signed_range(plane->dev, 0, "HOTSPOT_X", > + INT_MIN, INT_MAX); > + if (IS_ERR(prop_x)) > + return PTR_ERR(prop_x); > + > + prop_y = drm_property_create_signed_range(plane->dev, 0, "HOTSPOT_Y", > + INT_MIN, INT_MAX); > + if (IS_ERR(prop_y)) { > + drm_property_destroy(plane->dev, prop_x); > + return PTR_ERR(prop_y); > + } > + > + drm_object_attach_property(&plane->base, prop_x, 0); > + drm_object_attach_property(&plane->base, prop_y, 0); > + plane->hotspot_x_property = prop_x; > + plane->hotspot_y_property = prop_y; > + > + return 0; > +} > +EXPORT_SYMBOL(drm_plane_create_hotspot_properties); > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > index 51291983ea44..cf269d5de278 100644 > --- a/include/drm/drm_plane.h > +++ b/include/drm/drm_plane.h > @@ -116,6 +116,10 @@ struct drm_plane_state { > /** @src_h: height of visible portion of plane (in 16.16) */ > uint32_t src_h, src_w; > > + /** @hotspot_x: x offset to mouse cursor hotspot */ > + /** @hotspot_y: y offset to mouse cursor hotspot */ > + int32_t hotspot_x, hotspot_y; > + > /** > * @alpha: > * Opacity of the plane with 0 as completely transparent and 0xffff as > @@ -748,6 +752,16 @@ struct drm_plane { > * scaling. > */ > struct drm_property *scaling_filter_property; > + > + /** > + * @hotspot_x_property: property to set mouse hotspot x offset. > + */ > + struct drm_property *hotspot_x_property; > + > + /** > + * @hotspot_y_property: property to set mouse hotspot y offset. > + */ > + struct drm_property *hotspot_y_property; > }; > > #define obj_to_plane(x) container_of(x, struct drm_plane, base) > @@ -945,5 +959,6 @@ 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_create_hotspot_properties(struct drm_plane *plane); > > #endif
Zack Rusin <zack@kde.org> writes: > From: Zack Rusin <zackr@vmware.com> > > Atomic modesetting code lacked support for specifying mouse cursor > hotspots. The legacy kms DRM_IOCTL_MODE_CURSOR2 had support for setting > the hotspot but the functionality was not implemented in the new atomic > paths. > > Due to the lack of hotspots in the atomic paths userspace compositors > completely disable atomic modesetting for drivers that require it (i.e. > all paravirtualized drivers). > > This change adds hotspot properties to the atomic codepaths throughtout > the DRM core and will allow enabling atomic modesetting for virtualized > drivers in the userspace. > > Signed-off-by: Zack Rusin <zackr@vmware.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: David Airlie <airlied@linux.ie> > Cc: Daniel Vetter <daniel@ffwll.ch> > --- Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Pekka Paalanen <ppaalanen@gmail.com> writes: > On Mon, 26 Jun 2023 23:58:33 -0400 > Zack Rusin <zack@kde.org> wrote: > >> From: Zack Rusin <zackr@vmware.com> >> >> Atomic modesetting code lacked support for specifying mouse cursor >> hotspots. The legacy kms DRM_IOCTL_MODE_CURSOR2 had support for setting >> the hotspot but the functionality was not implemented in the new atomic >> paths. >> >> Due to the lack of hotspots in the atomic paths userspace compositors >> completely disable atomic modesetting for drivers that require it (i.e. >> all paravirtualized drivers). >> >> This change adds hotspot properties to the atomic codepaths throughtout >> the DRM core and will allow enabling atomic modesetting for virtualized >> drivers in the userspace. >> >> Signed-off-by: Zack Rusin <zackr@vmware.com> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Cc: Maxime Ripard <mripard@kernel.org> >> Cc: Thomas Zimmermann <tzimmermann@suse.de> >> Cc: David Airlie <airlied@linux.ie> >> Cc: Daniel Vetter <daniel@ffwll.ch> >> --- >> drivers/gpu/drm/drm_atomic_state_helper.c | 14 +++++++ >> drivers/gpu/drm/drm_atomic_uapi.c | 20 +++++++++ >> drivers/gpu/drm/drm_plane.c | 51 +++++++++++++++++++++++ >> include/drm/drm_plane.h | 15 +++++++ >> 4 files changed, 100 insertions(+) > > Hi Zack, > > where is the UAPI documentation for these new properties? I mean > something ending up in the HTML docs like what other properties have in > e.g. https://www.kernel.org/doc/html/latest/gpu/drm-kms.html#plane-composition-properties > > Otherwise looks fine to me. Could drm_plane_create_hotspot_properties() > perhaps assert that the plane type is CURSOR? > I thought the same when reviewing but then I noticed this function is only called from __drm_universal_plane_init() if type is DRM_PLANE_TYPE_CURSOR.
On Tue, 27 Jun 2023 10:56:39 +0200 Javier Martinez Canillas <javierm@redhat.com> wrote: > Pekka Paalanen <ppaalanen@gmail.com> writes: > > > On Mon, 26 Jun 2023 23:58:33 -0400 > > Zack Rusin <zack@kde.org> wrote: > > > >> From: Zack Rusin <zackr@vmware.com> > >> > >> Atomic modesetting code lacked support for specifying mouse cursor > >> hotspots. The legacy kms DRM_IOCTL_MODE_CURSOR2 had support for setting > >> the hotspot but the functionality was not implemented in the new atomic > >> paths. > >> > >> Due to the lack of hotspots in the atomic paths userspace compositors > >> completely disable atomic modesetting for drivers that require it (i.e. > >> all paravirtualized drivers). > >> > >> This change adds hotspot properties to the atomic codepaths throughtout > >> the DRM core and will allow enabling atomic modesetting for virtualized > >> drivers in the userspace. > >> > >> Signed-off-by: Zack Rusin <zackr@vmware.com> > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> Cc: Maxime Ripard <mripard@kernel.org> > >> Cc: Thomas Zimmermann <tzimmermann@suse.de> > >> Cc: David Airlie <airlied@linux.ie> > >> Cc: Daniel Vetter <daniel@ffwll.ch> > >> --- > >> drivers/gpu/drm/drm_atomic_state_helper.c | 14 +++++++ > >> drivers/gpu/drm/drm_atomic_uapi.c | 20 +++++++++ > >> drivers/gpu/drm/drm_plane.c | 51 +++++++++++++++++++++++ > >> include/drm/drm_plane.h | 15 +++++++ > >> 4 files changed, 100 insertions(+) > > > > Hi Zack, > > > > where is the UAPI documentation for these new properties? I mean > > something ending up in the HTML docs like what other properties have in > > e.g. https://www.kernel.org/doc/html/latest/gpu/drm-kms.html#plane-composition-properties > > > > Otherwise looks fine to me. Could drm_plane_create_hotspot_properties() > > perhaps assert that the plane type is CURSOR? > > > > I thought the same when reviewing but then I noticed this function is only > called from __drm_universal_plane_init() if type is DRM_PLANE_TYPE_CURSOR. Right, so why bother checking for DRIVER_CURSOR_HOTSPOT either? Shouldn't the function be 'static' too, not exported, and not added to a header? Thanks, pq
Pekka Paalanen <ppaalanen@gmail.com> writes: > On Tue, 27 Jun 2023 10:56:39 +0200 > Javier Martinez Canillas <javierm@redhat.com> wrote: > [...] >> > Hi Zack, >> > >> > where is the UAPI documentation for these new properties? I mean >> > something ending up in the HTML docs like what other properties have in >> > e.g. https://www.kernel.org/doc/html/latest/gpu/drm-kms.html#plane-composition-properties >> > >> > Otherwise looks fine to me. Could drm_plane_create_hotspot_properties() >> > perhaps assert that the plane type is CURSOR? >> > >> >> I thought the same when reviewing but then I noticed this function is only >> called from __drm_universal_plane_init() if type is DRM_PLANE_TYPE_CURSOR. > > Right, so why bother checking for DRIVER_CURSOR_HOTSPOT either? > Shouldn't the function be 'static' too, not exported, and not added to > a header? > Agreed. It should either be a static helper function in drm_plane.c and not an exported symbol (in which case the checks are superflous as you said) or the function should not make assumptions about what was checked by the callers. I believe that the former would be better and only make it accessible to drivers if that is found to be needed later. > > Thanks, > pq
diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c index 784e63d70a42..54975de44a0e 100644 --- a/drivers/gpu/drm/drm_atomic_state_helper.c +++ b/drivers/gpu/drm/drm_atomic_state_helper.c @@ -275,6 +275,20 @@ void __drm_atomic_helper_plane_state_reset(struct drm_plane_state *plane_state, plane_state->normalized_zpos = val; } } + + if (plane->hotspot_x_property) { + if (!drm_object_property_get_default_value(&plane->base, + plane->hotspot_x_property, + &val)) + plane_state->hotspot_x = val; + } + + if (plane->hotspot_y_property) { + if (!drm_object_property_get_default_value(&plane->base, + plane->hotspot_y_property, + &val)) + plane_state->hotspot_y = val; + } } EXPORT_SYMBOL(__drm_atomic_helper_plane_state_reset); diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 98d3b10c08ae..07a7b3f18df2 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -593,6 +593,22 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, } else if (plane->funcs->atomic_set_property) { return plane->funcs->atomic_set_property(plane, state, property, val); + } else if (property == plane->hotspot_x_property) { + if (plane->type != DRM_PLANE_TYPE_CURSOR) { + drm_dbg_atomic(plane->dev, + "[PLANE:%d:%s] is not a cursor plane: 0x%llx\n", + plane->base.id, plane->name, val); + return -EINVAL; + } + state->hotspot_x = val; + } else if (property == plane->hotspot_y_property) { + if (plane->type != DRM_PLANE_TYPE_CURSOR) { + drm_dbg_atomic(plane->dev, + "[PLANE:%d:%s] is not a cursor plane: 0x%llx\n", + plane->base.id, plane->name, val); + return -EINVAL; + } + state->hotspot_y = val; } else { drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] unknown property [PROP:%d:%s]\n", @@ -653,6 +669,10 @@ drm_atomic_plane_get_property(struct drm_plane *plane, *val = state->scaling_filter; } else if (plane->funcs->atomic_get_property) { return plane->funcs->atomic_get_property(plane, state, property, val); + } else if (property == plane->hotspot_x_property) { + *val = state->hotspot_x; + } else if (property == plane->hotspot_y_property) { + *val = state->hotspot_y; } else { drm_dbg_atomic(dev, "[PLANE:%d:%s] unknown property [PROP:%d:%s]\n", diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index a4a39f4834e2..ff1cc810d8f8 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -348,6 +348,10 @@ static int __drm_universal_plane_init(struct drm_device *dev, drm_object_attach_property(&plane->base, config->prop_src_w, 0); drm_object_attach_property(&plane->base, config->prop_src_h, 0); } + if (drm_core_check_feature(dev, DRIVER_CURSOR_HOTSPOT) && + type == DRM_PLANE_TYPE_CURSOR) { + drm_plane_create_hotspot_properties(plane); + } if (format_modifier_count) create_in_format_blob(dev, plane); @@ -1067,6 +1071,11 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, fb->hot_x = req->hot_x; fb->hot_y = req->hot_y; + + if (plane->hotspot_x_property && plane->state) + plane->state->hotspot_x = req->hot_x; + if (plane->hotspot_y_property && plane->state) + plane->state->hotspot_y = req->hot_y; } else { fb = NULL; } @@ -1595,3 +1604,45 @@ int drm_plane_create_scaling_filter_property(struct drm_plane *plane, return 0; } EXPORT_SYMBOL(drm_plane_create_scaling_filter_property); + +/** + * drm_plane_create_hotspot_properties - creates the mouse hotspot + * properties and attaches them to the given cursor plane + * + * @plane: drm cursor plane + * + * This function lets driver to enable the mouse hotspot property on a given + * cursor plane. + * + * RETURNS: + * Zero for success or -errno + */ +int drm_plane_create_hotspot_properties(struct drm_plane *plane) +{ + struct drm_property *prop_x; + struct drm_property *prop_y; + + drm_WARN_ON(plane->dev, + !drm_core_check_feature(plane->dev, + DRIVER_CURSOR_HOTSPOT)); + + prop_x = drm_property_create_signed_range(plane->dev, 0, "HOTSPOT_X", + INT_MIN, INT_MAX); + if (IS_ERR(prop_x)) + return PTR_ERR(prop_x); + + prop_y = drm_property_create_signed_range(plane->dev, 0, "HOTSPOT_Y", + INT_MIN, INT_MAX); + if (IS_ERR(prop_y)) { + drm_property_destroy(plane->dev, prop_x); + return PTR_ERR(prop_y); + } + + drm_object_attach_property(&plane->base, prop_x, 0); + drm_object_attach_property(&plane->base, prop_y, 0); + plane->hotspot_x_property = prop_x; + plane->hotspot_y_property = prop_y; + + return 0; +} +EXPORT_SYMBOL(drm_plane_create_hotspot_properties); diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 51291983ea44..cf269d5de278 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -116,6 +116,10 @@ struct drm_plane_state { /** @src_h: height of visible portion of plane (in 16.16) */ uint32_t src_h, src_w; + /** @hotspot_x: x offset to mouse cursor hotspot */ + /** @hotspot_y: y offset to mouse cursor hotspot */ + int32_t hotspot_x, hotspot_y; + /** * @alpha: * Opacity of the plane with 0 as completely transparent and 0xffff as @@ -748,6 +752,16 @@ struct drm_plane { * scaling. */ struct drm_property *scaling_filter_property; + + /** + * @hotspot_x_property: property to set mouse hotspot x offset. + */ + struct drm_property *hotspot_x_property; + + /** + * @hotspot_y_property: property to set mouse hotspot y offset. + */ + struct drm_property *hotspot_y_property; }; #define obj_to_plane(x) container_of(x, struct drm_plane, base) @@ -945,5 +959,6 @@ 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_create_hotspot_properties(struct drm_plane *plane); #endif