[PATCH 13/13] RFC: drm: Atomic modeset ioctl
Daniel Vetter
daniel at ffwll.ch
Wed Dec 17 07:04:15 PST 2014
On Tue, Dec 16, 2014 at 06:05:41PM -0500, Rob Clark wrote:
> The atomic modeset ioctl can be used to push any number of new values
> for object properties. The driver can then check the full device
> configuration as single unit, and try to apply the changes atomically.
>
> The ioctl simply takes a list of object IDs and property IDs and their
> values.
>
> Originally based on a patch from Ville Syrjälä, although it has mutated
> (mutilated?) enough since then that you probably shouldn't blame it on
> him ;-)
>
> TODO:
> * hide behind moduleparam initially
Yeah we need this, and I think we really need this (plus
file_priv->atomic_kms to allow new userspace to enable atomic, similar to
file_priv->universal_planes) even for patches 6, 10&11.
> * either bring back blob property support, or add ioctls for create/
> destroy blob properties so they can be passed in by id in this
> ioctl. I'm leaning towards the latter approach as (a) it is more
> in line with how blob properties are read, and (b) the atomic
> ioctl function is big enough already.
The blob approach is a bit more work, since we need to copypaste all the
logic we have for per-filpriv framebuffers: cleanup lists & refcounting.
Not sure whether we should just go with lots more properties, they're
cheap ;-) But I'm ok with this blob approach, too. Adding blobs to the
atomic ioctl itself feels a bit wrong.
One thing I have to consider with my intel hat on is the bridge we need to
adf. Everyting plain 64bit props would be a bit easier to marshal through
adf into atomic interfaces. But only slightly, we can just add an in-line
set of blobs at the end and write them directly, too. So either approach
is fine really.
Below a few comments all over, but looks good overall.
Cheers, Daniel
>
> Signed-off-by: Rob Clark <robdclark at gmail.com>
> ---
> drivers/gpu/drm/drm_atomic.c | 308 +++++++++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/drm_crtc.c | 4 +-
> drivers/gpu/drm/drm_ioctl.c | 1 +
> include/drm/drm_crtc.h | 6 +
> include/uapi/drm/drm.h | 1 +
> include/uapi/drm/drm_mode.h | 21 +++
> 6 files changed, 339 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 71b48a0..e7a45ec 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -960,3 +960,311 @@ int drm_atomic_async_commit(struct drm_atomic_state *state)
> return config->funcs->atomic_commit(state->dev, state, true);
> }
> EXPORT_SYMBOL(drm_atomic_async_commit);
> +
> +/*
> + * The big monstor ioctl
> + */
> +
> +static struct drm_pending_vblank_event *create_vblank_event(
> + struct drm_device *dev, struct drm_file *file_priv, uint64_t user_data)
> +{
> + struct drm_pending_vblank_event *e = NULL;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&dev->event_lock, flags);
> + if (file_priv->event_space < sizeof e->event) {
> + spin_unlock_irqrestore(&dev->event_lock, flags);
> + goto out;
> + }
> + file_priv->event_space -= sizeof e->event;
> + spin_unlock_irqrestore(&dev->event_lock, flags);
> +
> + e = kzalloc(sizeof *e, GFP_KERNEL);
> + if (e == NULL) {
> + spin_lock_irqsave(&dev->event_lock, flags);
> + file_priv->event_space += sizeof e->event;
> + spin_unlock_irqrestore(&dev->event_lock, flags);
> + goto out;
> + }
> +
> + e->event.base.type = DRM_EVENT_FLIP_COMPLETE;
> + e->event.base.length = sizeof e->event;
> + e->event.user_data = user_data;
> + e->base.event = &e->event.base;
> + e->base.file_priv = file_priv;
> + e->base.destroy = (void (*) (struct drm_pending_event *)) kfree;
> +
> +out:
> + return e;
> +}
> +
> +static void destroy_vblank_event(struct drm_device *dev,
> + struct drm_file *file_priv, struct drm_pending_vblank_event *e)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&dev->event_lock, flags);
> + file_priv->event_space += sizeof e->event;
> + spin_unlock_irqrestore(&dev->event_lock, flags);
> + kfree(e);
> +}
I guess these two are the skeleton functions for the refactoring you've
started? Just an aside really.
> +int drm_mode_atomic_ioctl(struct drm_device *dev,
> + void *data, struct drm_file *file_priv)
> +{
> + struct drm_mode_atomic *arg = data;
> + uint32_t __user *objs_ptr = (uint32_t __user *)(unsigned long)(arg->objs_ptr);
> + uint32_t __user *count_props_ptr = (uint32_t __user *)(unsigned long)(arg->count_props_ptr);
> + uint32_t __user *props_ptr = (uint32_t __user *)(unsigned long)(arg->props_ptr);
> + uint64_t __user *prop_values_ptr = (uint64_t __user *)(unsigned long)(arg->prop_values_ptr);
> + unsigned int copied_objs, copied_props;
> + struct drm_atomic_state *state;
> + struct drm_modeset_acquire_ctx ctx;
> + struct drm_plane *plane;
> + unsigned plane_mask = 0;
> + int ret = 0;
> + unsigned int i, j;
> +
> + if (arg->flags & ~DRM_MODE_ATOMIC_FLAGS)
> + return -EINVAL;
> +
> + /* can't test and expect an event at the same time. */
> + if ((arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) &&
> + (arg->flags & DRM_MODE_PAGE_FLIP_EVENT))
> + return -EINVAL;
Maybe check for async implies nonblock, too?
And I think we need to write the flags from userspace into
drm_atomic_state. At least for most hw async flips have fairly strict
requirements, a lot stricter than normal flips at least.
Or we just disallow async updates for now through atomic.
> +
> + drm_modeset_acquire_init(&ctx, 0);
> +
> + state = drm_atomic_state_alloc(dev);
> + if (!state)
> + return -ENOMEM;
> +
> + state->acquire_ctx = &ctx;
> +
> +retry:
> + copied_objs = 0;
> + copied_props = 0;
> +
> + for (i = 0; i < arg->count_objs; i++) {
> + uint32_t obj_id, count_props;
> + struct drm_mode_object *obj;
> +
> + if (get_user(obj_id, objs_ptr + copied_objs)) {
> + ret = -EFAULT;
> + goto fail;
> + }
> +
> + obj = drm_mode_object_find(dev, obj_id, DRM_MODE_OBJECT_ANY);
> + if (!obj || !obj->properties) {
> + ret = -ENOENT;
> + goto fail;
> + }
> +
> + if (obj->type == DRM_MODE_OBJECT_PLANE) {
> + plane = obj_to_plane(obj);
> + plane_mask |= (1 << drm_plane_index(plane));
> + plane->old_fb = plane->fb;
> + }
> +
> + if (get_user(count_props, count_props_ptr + copied_objs)) {
> + ret = -EFAULT;
> + goto fail;
> + }
> +
> + copied_objs++;
> +
> + for (j = 0; j < count_props; j++) {
If it's not too horrible I think extracting the inner properties loop
would be really nice to tame the monster.
> + uint32_t prop_id;
> + uint64_t prop_value;
> + struct drm_property *prop;
> + struct drm_mode_object *ref;
> +
> + if (get_user(prop_id, props_ptr + copied_props)) {
> + ret = -EFAULT;
> + goto fail;
> + }
> +
> + prop = drm_property_find(dev, prop_id);
> + if (!prop) {
> + ret = -ENOENT;
> + goto fail;
> + }
> +
> + if (get_user(prop_value, prop_values_ptr + copied_props)) {
> + ret = -EFAULT;
> + goto fail;
> + }
> +
> + if (!drm_property_change_valid_get(prop, prop_value, &ref)) {
> + ret = -EINVAL;
> + goto fail;
> + }
> +
> + switch (obj->type) {
> + case DRM_MODE_OBJECT_CONNECTOR: {
> + struct drm_connector *connector = obj_to_connector(obj);
> + struct drm_connector_state *connector_state;
> +
> + connector_state = drm_atomic_get_connector_state(state, connector);
> + if (IS_ERR(connector_state)) {
> + ret = PTR_ERR(connector_state);
> + break;
> + }
> +
> + ret = drm_atomic_connector_set_property(connector,
> + connector_state, prop, prop_value);
> + break;
> + }
> + case DRM_MODE_OBJECT_CRTC: {
> + struct drm_crtc *crtc = obj_to_crtc(obj);
> + struct drm_crtc_state *crtc_state;
> +
> + crtc_state = drm_atomic_get_crtc_state(state, crtc);
> + if (IS_ERR(crtc_state)) {
> + ret = PTR_ERR(crtc_state);
> + break;
> + }
> +
> + ret = drm_atomic_crtc_set_property(crtc,
> + crtc_state, prop, prop_value);
> + break;
> + }
> + case DRM_MODE_OBJECT_PLANE: {
> + struct drm_plane *plane = obj_to_plane(obj);
> + struct drm_plane_state *plane_state;
> +
> + plane_state = drm_atomic_get_plane_state(state, plane);
> + if (IS_ERR(plane_state)) {
> + ret = PTR_ERR(plane_state);
> + break;
> + }
> +
> + ret = drm_atomic_plane_set_property(plane,
> + plane_state, prop, prop_value);
> + break;
> + }
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + copied_props++;
> +
> + drm_property_change_valid_put(prop, ref);
> +
> + if (ret)
> + goto fail;
> + }
> + }
> +
> + if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> + int ncrtcs = dev->mode_config.num_crtc;
> +
> + for (i = 0; i < ncrtcs; i++) {
> + struct drm_crtc_state *crtc_state = state->crtc_states[i];
> + struct drm_pending_vblank_event *e;
> +
> + if (!crtc_state)
> + continue;
> +
> + e = create_vblank_event(dev, file_priv, arg->user_data);
> + if (!e) {
> + ret = -ENOMEM;
> + goto fail;
> + }
> +
> + crtc_state->event = e;
> + }
> + }
> +
> + /* sanity check incoming state, because danvet wanted this
> + * to be an even bigger monster ioctl and wanted to be able
> + * to WARN_ON() in these cases in the helpers:
> + */
> + drm_for_each_plane_mask(plane, dev, plane_mask) {
> + struct drm_plane_state *plane_state =
> + drm_atomic_get_plane_state(state, plane);
> +
> + if (IS_ERR(plane_state)) {
> + ret = PTR_ERR(plane_state);
> + goto fail;
> + }
> +
> + if (WARN_ON(plane_state->crtc && !plane_state->fb)) {
> + DRM_DEBUG_KMS("CRTC set but no FB\n");
> + ret = -EINVAL;
> + goto fail;
> + } else if (WARN_ON(plane_state->fb && !plane_state->crtc)) {
> + DRM_DEBUG_KMS("FB set but no CRTC\n");
> + ret = -EINVAL;
> + goto fail;
> + }
> + }
With my suggestion to move the checks from heleprs to check_only we could
remove the above hunk as redundant.
> +
> + if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
> + ret = drm_atomic_check_only(state);
> + /* _check_only() does not free state, unlike _commit() */
> + drm_atomic_state_free(state);
> + } else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
> + ret = drm_atomic_async_commit(state);
> + } else {
> + ret = drm_atomic_commit(state);
> + }
> +
> + /* if succeeded, fixup legacy plane crtc/fb ptrs before dropping
> + * locks (ie. while it is still safe to deref plane->state). We
> + * need to do this here because the driver entry points cannot
> + * distinguish between legacy and atomic ioctls.
> + */
> + drm_for_each_plane_mask(plane, dev, plane_mask) {
> + if (ret == 0) {
> + struct drm_framebuffer *new_fb = plane->state->fb;
> + if (new_fb)
> + drm_framebuffer_reference(new_fb);
> + plane->fb = new_fb;
> + plane->crtc = plane->state->crtc;
> + } else {
> + plane->old_fb = NULL;
> + }
> + if (plane->old_fb) {
> + drm_framebuffer_unreference(plane->old_fb);
> + plane->old_fb = NULL;
> + }
> + }
> +
> + drm_modeset_drop_locks(&ctx);
> + drm_modeset_acquire_fini(&ctx);
> +
> + return ret;
> +
> +fail:
> + if (ret == -EDEADLK)
> + goto backoff;
> +
> + if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> + int ncrtcs = dev->mode_config.num_crtc;
> +
> + for (i = 0; i < ncrtcs; i++) {
> + struct drm_crtc_state *crtc_state = state->crtc_states[i];
> +
> + if (!crtc_state)
> + continue;
> +
> + destroy_vblank_event(dev, file_priv, crtc_state->event);
> + crtc_state->event = NULL;
> + }
> + }
> +
> + drm_atomic_state_free(state);
> +
> + drm_modeset_drop_locks(&ctx);
> + drm_modeset_acquire_fini(&ctx);
> +
> + return ret;
> +
> +backoff:
> + drm_atomic_state_clear(state);
> + drm_modeset_backoff(&ctx);
> +
> + goto retry;
> +}
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 39c3e06..948d04d 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -4299,7 +4299,7 @@ EXPORT_SYMBOL(drm_mode_connector_update_edid_property);
> * object to which the property is attached has a chance to take it's own
> * reference).
> */
> -static bool drm_property_change_valid_get(struct drm_property *property,
> +bool drm_property_change_valid_get(struct drm_property *property,
> uint64_t value, struct drm_mode_object **ref)
> {
> if (property->flags & DRM_MODE_PROP_IMMUTABLE)
> @@ -4353,7 +4353,7 @@ static bool drm_property_change_valid_get(struct drm_property *property,
> }
> }
>
> -static void drm_property_change_valid_put(struct drm_property *property,
> +void drm_property_change_valid_put(struct drm_property *property,
> struct drm_mode_object *ref)
> {
> if (!ref)
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 00587a1..fe76815 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -620,6 +620,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
> DRM_IOCTL_DEF(DRM_IOCTL_MODE_OBJ_GETPROPERTIES, drm_mode_obj_get_properties_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> DRM_IOCTL_DEF(DRM_IOCTL_MODE_OBJ_SETPROPERTY, drm_mode_obj_set_property_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> DRM_IOCTL_DEF(DRM_IOCTL_MODE_CURSOR2, drm_mode_cursor2_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> + DRM_IOCTL_DEF(DRM_IOCTL_MODE_ATOMIC, drm_mode_atomic_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> };
>
> #define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls )
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 48fcd98..8330676 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1332,6 +1332,10 @@ extern int drm_mode_create_scaling_mode_property(struct drm_device *dev);
> extern int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
> extern int drm_mode_create_dirty_info_property(struct drm_device *dev);
> extern int drm_mode_create_suggested_offset_properties(struct drm_device *dev);
> +extern bool drm_property_change_valid_get(struct drm_property *property,
> + uint64_t value, struct drm_mode_object **ref);
> +extern void drm_property_change_valid_put(struct drm_property *property,
> + struct drm_mode_object *ref);
>
> extern int drm_mode_connector_attach_encoder(struct drm_connector *connector,
> struct drm_encoder *encoder);
> @@ -1423,6 +1427,8 @@ extern int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
> extern int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
> struct drm_property *property,
> uint64_t value);
> +extern int drm_mode_atomic_ioctl(struct drm_device *dev,
> + void *data, struct drm_file *file_priv);
>
> extern void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
> int *bpp);
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index b0b8556..9cea966 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -777,6 +777,7 @@ struct drm_prime_handle {
> #define DRM_IOCTL_MODE_OBJ_GETPROPERTIES DRM_IOWR(0xB9, struct drm_mode_obj_get_properties)
> #define DRM_IOCTL_MODE_OBJ_SETPROPERTY DRM_IOWR(0xBA, struct drm_mode_obj_set_property)
> #define DRM_IOCTL_MODE_CURSOR2 DRM_IOWR(0xBB, struct drm_mode_cursor2)
> +#define DRM_IOCTL_MODE_ATOMIC DRM_IOWR(0xBC, struct drm_mode_atomic)
>
> /**
> * Device specific ioctls should only be in their respective headers
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 86574b0..3459778 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -519,4 +519,25 @@ struct drm_mode_destroy_dumb {
> uint32_t handle;
> };
>
> +/* page-flip flags are valid, plus: */
> +#define DRM_MODE_ATOMIC_TEST_ONLY 0x0100
> +#define DRM_MODE_ATOMIC_NONBLOCK 0x0200
> +
> +#define DRM_MODE_ATOMIC_FLAGS (\
> + DRM_MODE_PAGE_FLIP_EVENT |\
> + DRM_MODE_PAGE_FLIP_ASYNC |\
> + DRM_MODE_ATOMIC_TEST_ONLY |\
> + DRM_MODE_ATOMIC_NONBLOCK)
Just for future-proofing: Should we add an ATOMIC_ALLOW_MODESET right away
(and add checks in the helpers for that case, should just be a check for
crtc_state->modeset_changed at the very end of
atomic_helper_check_modeset?
I really think we need to clarify this before we make the abi stable,
otherwise there will be surprise compositors doing a full modeset once per
frame. Which is totally uncool ofc. But doesn't need to be done right
away, but a small patch on top would be great (since it's so simple with
helpers at least).
> +
> +struct drm_mode_atomic {
> + __u32 flags;
> + __u32 count_objs;
> + __u64 objs_ptr;
> + __u64 count_props_ptr;
> + __u64 props_ptr;
> + __u64 prop_values_ptr;
> + __u64 blob_values_ptr; /* remove? */
> + __u64 user_data;
> +};
> +
> #endif
> --
> 2.1.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
More information about the dri-devel
mailing list