Re: [PATCH 13/13] RFC: drm: Atomic modeset ioctl

[Daniel Vetter <daniel@ffwll.ch>]

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@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel