diff mbox

[RFCv1,11/12] drm: Atomic modeset ioctl

Message ID 1381020350-1125-12-git-send-email-robdclark@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Rob Clark Oct. 6, 2013, 12:45 a.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The atomic modeset ioctl cna 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. For setting values to blob properties, the property value
indicates the length of the data, and the actual data is passed via
another blob pointer.

The caller can demand non-blocking operation from the ioctl, and if the
driver can't satisfy that requirement an error will be returned.

The caller can also request to receive asynchronous completion events
after the operation has reached the hardware. An event is sent for each
object specified by the caller, whether or not the actual state of
that object changed. Each event also carries a framebuffer ID, which
indicates to user space that the specified object is no longer
accessing that framebuffer.

TODO: detailed error reporting?

v1: original
v2: rebase on uapi changes, and drm state structs.. -- Rob Clark
v3: rebase, missing padding in drm_event_atomic.. -- Rob Clark

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c  | 159 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_drv.c   |   1 +
 include/drm/drmP.h          |   6 ++
 include/drm/drm_crtc.h      |   2 +
 include/uapi/drm/drm.h      |  12 ++++
 include/uapi/drm/drm_mode.h |  16 +++++
 6 files changed, 196 insertions(+)

Comments

Ville Syrjälä Oct. 7, 2013, 1:28 p.m. UTC | #1
On Sat, Oct 05, 2013 at 08:45:49PM -0400, Rob Clark wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The atomic modeset ioctl cna 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. For setting values to blob properties, the property value
> indicates the length of the data, and the actual data is passed via
> another blob pointer.
> 
> The caller can demand non-blocking operation from the ioctl, and if the
> driver can't satisfy that requirement an error will be returned.
> 
> The caller can also request to receive asynchronous completion events
> after the operation has reached the hardware. An event is sent for each
> object specified by the caller, whether or not the actual state of
> that object changed. Each event also carries a framebuffer ID, which
> indicates to user space that the specified object is no longer
> accessing that framebuffer.
> 
> TODO: detailed error reporting?
> 
> v1: original
> v2: rebase on uapi changes, and drm state structs.. -- Rob Clark
> v3: rebase, missing padding in drm_event_atomic.. -- Rob Clark
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c  | 159 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_drv.c   |   1 +
>  include/drm/drmP.h          |   6 ++
>  include/drm/drm_crtc.h      |   2 +
>  include/uapi/drm/drm.h      |  12 ++++
>  include/uapi/drm/drm_mode.h |  16 +++++
>  6 files changed, 196 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 09396a9..910e5c6 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -4338,3 +4338,162 @@ void drm_mode_config_cleanup(struct drm_device *dev)
>  	idr_destroy(&dev->mode_config.crtc_idr);
>  }
>  EXPORT_SYMBOL(drm_mode_config_cleanup);
> +
> +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);
> +	uint64_t __user *blob_values_ptr = (uint64_t __user *)(unsigned long)(arg->blob_values_ptr);
> +	unsigned int copied_objs = 0;
> +	unsigned int copied_props = 0;
> +	unsigned int copied_blobs = 0;
> +	void *state;
> +	int ret = 0;
> +	unsigned int i, j;
> +
> +	if (arg->flags & ~(DRM_MODE_ATOMIC_TEST_ONLY |
> +			DRM_MODE_ATOMIC_EVENT | DRM_MODE_ATOMIC_NONBLOCK))
> +		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_ATOMIC_EVENT))
> +		return -EINVAL;
> +
> +	mutex_lock(&dev->mode_config.mutex);

I'm pretty sure I had a version w/ fixed locking...

git://gitorious.org/vsyrjala/linux.git rebased_drm_atomic_24

> +
> +	state = dev->driver->atomic_begin(dev, arg->flags);
> +	if (IS_ERR(state)) {
> +		ret = PTR_ERR(state);
> +		goto unlock;
> +	}
> +
> +	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 out;
> +		}
> +
> +		obj = drm_mode_object_find(dev, obj_id, DRM_MODE_OBJECT_ANY);
> +		if (!obj || !obj->properties) {
> +			ret = -ENOENT;
> +			goto out;
> +		}
> +
> +		if (arg->flags & DRM_MODE_ATOMIC_EVENT) {
> +			// XXX create atomic event instead..

Some people are apparently more comfortable with a per-crtc event
rather than the per-obj events I added. So I think we might want
both in the end.

> +			struct drm_pending_vblank_event *e =
> +				create_vblank_event(dev, file_priv, arg->user_data);
> +			if (!e) {
> +				ret = -ENOMEM;
> +				goto out;
> +			}
> +			ret = dev->driver->atomic_set_event(dev, state, obj, e);
> +			if (ret) {
> +				destroy_vblank_event(dev, file_priv, e);
> +				goto out;
> +			}
> +		}
> +
> +		if (get_user(count_props, count_props_ptr + copied_objs)) {
> +			ret = -EFAULT;
> +			goto out;
> +		}
> +
> +		copied_objs++;
> +
> +		for (j = 0; j < count_props; j++) {
> +			uint32_t prop_id;
> +			uint64_t prop_value;
> +			struct drm_mode_object *prop_obj;
> +			struct drm_property *prop;
> +			void *blob_data = NULL;
> +
> +			if (get_user(prop_id, props_ptr + copied_props)) {
> +				ret = -EFAULT;
> +				goto out;
> +			}
> +
> +			if (!object_has_prop(obj, prop_id)) {
> +				ret = -EINVAL;
> +				goto out;
> +			}
> +
> +			prop_obj = drm_mode_object_find(dev, prop_id,
> +					DRM_MODE_OBJECT_PROPERTY);
> +			if (!prop_obj) {
> +				ret = -ENOENT;
> +				goto out;
> +			}
> +			prop = obj_to_property(prop_obj);
> +
> +			if (get_user(prop_value, prop_values_ptr + copied_props)) {
> +				ret = -EFAULT;
> +				goto out;
> +			}
> +
> +			if (!drm_property_change_is_valid(prop, prop_value)) {
> +				ret = -EINVAL;
> +				goto out;
> +			}
> +
> +			if ((prop->flags & DRM_MODE_PROP_BLOB) && prop_value) {
> +				uint64_t blob_ptr;
> +
> +				if (get_user(blob_ptr, blob_values_ptr + copied_blobs)) {
> +					ret = -EFAULT;
> +					goto out;
> +				}
> +
> +				blob_data = kmalloc(prop_value, GFP_KERNEL);
> +				if (!blob_data) {
> +					ret = -ENOMEM;
> +					goto out;
> +				}
> +
> +				if (copy_from_user(blob_data, (void __user *)(unsigned long)blob_ptr, prop_value)) {
> +					kfree(blob_data);
> +					ret = -EFAULT;
> +					goto out;
> +				}
> +			}
> +
> +			/* User space sends the blob pointer even if we
> +			 * don't use it (length==0).
> +			 */
> +			if (prop->flags & DRM_MODE_PROP_BLOB)
> +				copied_blobs++;
> +
> +			/* The driver will be in charge of blob_data from now on. */
> +			ret = drm_mode_set_obj_prop(obj, state, prop,
> +					prop_value, blob_data);
> +			if (ret)
> +				goto out;
> +
> +			copied_props++;
> +		}
> +	}
> +
> +	ret = dev->driver->atomic_check(dev, state);
> +	if (ret)
> +		goto out;
> +
> +	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)
> +		goto out;
> +
> +	ret = dev->driver->atomic_commit(dev, state);
> +
> + out:
> +	dev->driver->atomic_end(dev, state);
> + unlock:
> +	mutex_unlock(&dev->mode_config.mutex);
> +
> +	return ret;
> +}
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index e572dd2..43e04ae 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -166,6 +166,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/drmP.h b/include/drm/drmP.h
> index 4c3de22..f282733 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1158,6 +1158,12 @@ struct drm_pending_vblank_event {
>  	struct drm_event_vblank event;
>  };
>  
> +struct drm_pending_atomic_event {
> +	struct drm_pending_event base;
> +	int pipe;
> +	struct drm_event_atomic event;
> +};
> +
>  /**
>   * DRM device structure. This structure represent a complete card that
>   * may contain multiple heads.
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 86a5a00..fa3956e 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1313,6 +1313,8 @@ extern int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>  					     struct drm_file *file_priv);
>  extern int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
>  					   struct drm_file *file_priv);
> +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 ece8678..efb1461 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -733,6 +733,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
> @@ -774,6 +775,17 @@ struct drm_event_vblank {
>  	__u32 reserved;
>  };
>  
> +struct drm_event_atomic {
> +	struct drm_event base;
> +	__u64 user_data;
> +	__u32 tv_sec;
> +	__u32 tv_usec;
> +	__u32 sequence;
> +	__u32 obj_id;
> +	__u32 old_fb_id;
> +	__u32 reserved;
> +};
> +
>  #define DRM_CAP_DUMB_BUFFER 0x1
>  #define DRM_CAP_VBLANK_HIGH_CRTC 0x2
>  #define DRM_CAP_DUMB_PREFERRED_DEPTH 0x3
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 6d4f089..03a473c 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -489,4 +489,20 @@ struct drm_mode_destroy_dumb {
>  	uint32_t handle;
>  };
>  
> +#define DRM_MODE_ATOMIC_TEST_ONLY (1<<0)
> +#define DRM_MODE_ATOMIC_EVENT (1<<1)
> +#define DRM_MODE_ATOMIC_NONBLOCK (1<<2)
> +
> +/* FIXME come up with some sane error reporting mechanism? */
> +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;
> +	__u64 user_data;
> +};
> +
>  #endif
> -- 
> 1.8.3.1
Rob Clark Oct. 7, 2013, 1:55 p.m. UTC | #2
On Mon, Oct 7, 2013 at 9:28 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Sat, Oct 05, 2013 at 08:45:49PM -0400, Rob Clark wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> The atomic modeset ioctl cna 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. For setting values to blob properties, the property value
>> indicates the length of the data, and the actual data is passed via
>> another blob pointer.
>>
>> The caller can demand non-blocking operation from the ioctl, and if the
>> driver can't satisfy that requirement an error will be returned.
>>
>> The caller can also request to receive asynchronous completion events
>> after the operation has reached the hardware. An event is sent for each
>> object specified by the caller, whether or not the actual state of
>> that object changed. Each event also carries a framebuffer ID, which
>> indicates to user space that the specified object is no longer
>> accessing that framebuffer.
>>
>> TODO: detailed error reporting?
>>
>> v1: original
>> v2: rebase on uapi changes, and drm state structs.. -- Rob Clark
>> v3: rebase, missing padding in drm_event_atomic.. -- Rob Clark
>>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>  drivers/gpu/drm/drm_crtc.c  | 159 ++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/drm_drv.c   |   1 +
>>  include/drm/drmP.h          |   6 ++
>>  include/drm/drm_crtc.h      |   2 +
>>  include/uapi/drm/drm.h      |  12 ++++
>>  include/uapi/drm/drm_mode.h |  16 +++++
>>  6 files changed, 196 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index 09396a9..910e5c6 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -4338,3 +4338,162 @@ void drm_mode_config_cleanup(struct drm_device *dev)
>>       idr_destroy(&dev->mode_config.crtc_idr);
>>  }
>>  EXPORT_SYMBOL(drm_mode_config_cleanup);
>> +
>> +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);
>> +     uint64_t __user *blob_values_ptr = (uint64_t __user *)(unsigned long)(arg->blob_values_ptr);
>> +     unsigned int copied_objs = 0;
>> +     unsigned int copied_props = 0;
>> +     unsigned int copied_blobs = 0;
>> +     void *state;
>> +     int ret = 0;
>> +     unsigned int i, j;
>> +
>> +     if (arg->flags & ~(DRM_MODE_ATOMIC_TEST_ONLY |
>> +                     DRM_MODE_ATOMIC_EVENT | DRM_MODE_ATOMIC_NONBLOCK))
>> +             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_ATOMIC_EVENT))
>> +             return -EINVAL;
>> +
>> +     mutex_lock(&dev->mode_config.mutex);
>
> I'm pretty sure I had a version w/ fixed locking...
>
> git://gitorious.org/vsyrjala/linux.git rebased_drm_atomic_24

oh, hmm.. actually the locking should no longer be in the ioctl fxn,
but should be pushed down to the commit.  looks like I missed this.  I
need to dig up some actual test code for atomic ioctl ;-)

>> +
>> +     state = dev->driver->atomic_begin(dev, arg->flags);
>> +     if (IS_ERR(state)) {
>> +             ret = PTR_ERR(state);
>> +             goto unlock;
>> +     }
>> +
>> +     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 out;
>> +             }
>> +
>> +             obj = drm_mode_object_find(dev, obj_id, DRM_MODE_OBJECT_ANY);
>> +             if (!obj || !obj->properties) {
>> +                     ret = -ENOENT;
>> +                     goto out;
>> +             }
>> +
>> +             if (arg->flags & DRM_MODE_ATOMIC_EVENT) {
>> +                     // XXX create atomic event instead..
>
> Some people are apparently more comfortable with a per-crtc event
> rather than the per-obj events I added. So I think we might want
> both in the end.

yeah, sorting out events is a bit 'TODO' at the moment.  I do kind of
like per-crtc event.. it seems easier to implement and I'm not really
sure what finer event granularity buys us.

BR,
-R

>> +                     struct drm_pending_vblank_event *e =
>> +                             create_vblank_event(dev, file_priv, arg->user_data);
>> +                     if (!e) {
>> +                             ret = -ENOMEM;
>> +                             goto out;
>> +                     }
>> +                     ret = dev->driver->atomic_set_event(dev, state, obj, e);
>> +                     if (ret) {
>> +                             destroy_vblank_event(dev, file_priv, e);
>> +                             goto out;
>> +                     }
>> +             }
>> +
>> +             if (get_user(count_props, count_props_ptr + copied_objs)) {
>> +                     ret = -EFAULT;
>> +                     goto out;
>> +             }
>> +
>> +             copied_objs++;
>> +
>> +             for (j = 0; j < count_props; j++) {
>> +                     uint32_t prop_id;
>> +                     uint64_t prop_value;
>> +                     struct drm_mode_object *prop_obj;
>> +                     struct drm_property *prop;
>> +                     void *blob_data = NULL;
>> +
>> +                     if (get_user(prop_id, props_ptr + copied_props)) {
>> +                             ret = -EFAULT;
>> +                             goto out;
>> +                     }
>> +
>> +                     if (!object_has_prop(obj, prop_id)) {
>> +                             ret = -EINVAL;
>> +                             goto out;
>> +                     }
>> +
>> +                     prop_obj = drm_mode_object_find(dev, prop_id,
>> +                                     DRM_MODE_OBJECT_PROPERTY);
>> +                     if (!prop_obj) {
>> +                             ret = -ENOENT;
>> +                             goto out;
>> +                     }
>> +                     prop = obj_to_property(prop_obj);
>> +
>> +                     if (get_user(prop_value, prop_values_ptr + copied_props)) {
>> +                             ret = -EFAULT;
>> +                             goto out;
>> +                     }
>> +
>> +                     if (!drm_property_change_is_valid(prop, prop_value)) {
>> +                             ret = -EINVAL;
>> +                             goto out;
>> +                     }
>> +
>> +                     if ((prop->flags & DRM_MODE_PROP_BLOB) && prop_value) {
>> +                             uint64_t blob_ptr;
>> +
>> +                             if (get_user(blob_ptr, blob_values_ptr + copied_blobs)) {
>> +                                     ret = -EFAULT;
>> +                                     goto out;
>> +                             }
>> +
>> +                             blob_data = kmalloc(prop_value, GFP_KERNEL);
>> +                             if (!blob_data) {
>> +                                     ret = -ENOMEM;
>> +                                     goto out;
>> +                             }
>> +
>> +                             if (copy_from_user(blob_data, (void __user *)(unsigned long)blob_ptr, prop_value)) {
>> +                                     kfree(blob_data);
>> +                                     ret = -EFAULT;
>> +                                     goto out;
>> +                             }
>> +                     }
>> +
>> +                     /* User space sends the blob pointer even if we
>> +                      * don't use it (length==0).
>> +                      */
>> +                     if (prop->flags & DRM_MODE_PROP_BLOB)
>> +                             copied_blobs++;
>> +
>> +                     /* The driver will be in charge of blob_data from now on. */
>> +                     ret = drm_mode_set_obj_prop(obj, state, prop,
>> +                                     prop_value, blob_data);
>> +                     if (ret)
>> +                             goto out;
>> +
>> +                     copied_props++;
>> +             }
>> +     }
>> +
>> +     ret = dev->driver->atomic_check(dev, state);
>> +     if (ret)
>> +             goto out;
>> +
>> +     if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)
>> +             goto out;
>> +
>> +     ret = dev->driver->atomic_commit(dev, state);
>> +
>> + out:
>> +     dev->driver->atomic_end(dev, state);
>> + unlock:
>> +     mutex_unlock(&dev->mode_config.mutex);
>> +
>> +     return ret;
>> +}
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index e572dd2..43e04ae 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -166,6 +166,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/drmP.h b/include/drm/drmP.h
>> index 4c3de22..f282733 100644
>> --- a/include/drm/drmP.h
>> +++ b/include/drm/drmP.h
>> @@ -1158,6 +1158,12 @@ struct drm_pending_vblank_event {
>>       struct drm_event_vblank event;
>>  };
>>
>> +struct drm_pending_atomic_event {
>> +     struct drm_pending_event base;
>> +     int pipe;
>> +     struct drm_event_atomic event;
>> +};
>> +
>>  /**
>>   * DRM device structure. This structure represent a complete card that
>>   * may contain multiple heads.
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index 86a5a00..fa3956e 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -1313,6 +1313,8 @@ extern int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>>                                            struct drm_file *file_priv);
>>  extern int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
>>                                          struct drm_file *file_priv);
>> +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 ece8678..efb1461 100644
>> --- a/include/uapi/drm/drm.h
>> +++ b/include/uapi/drm/drm.h
>> @@ -733,6 +733,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
>> @@ -774,6 +775,17 @@ struct drm_event_vblank {
>>       __u32 reserved;
>>  };
>>
>> +struct drm_event_atomic {
>> +     struct drm_event base;
>> +     __u64 user_data;
>> +     __u32 tv_sec;
>> +     __u32 tv_usec;
>> +     __u32 sequence;
>> +     __u32 obj_id;
>> +     __u32 old_fb_id;
>> +     __u32 reserved;
>> +};
>> +
>>  #define DRM_CAP_DUMB_BUFFER 0x1
>>  #define DRM_CAP_VBLANK_HIGH_CRTC 0x2
>>  #define DRM_CAP_DUMB_PREFERRED_DEPTH 0x3
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index 6d4f089..03a473c 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -489,4 +489,20 @@ struct drm_mode_destroy_dumb {
>>       uint32_t handle;
>>  };
>>
>> +#define DRM_MODE_ATOMIC_TEST_ONLY (1<<0)
>> +#define DRM_MODE_ATOMIC_EVENT (1<<1)
>> +#define DRM_MODE_ATOMIC_NONBLOCK (1<<2)
>> +
>> +/* FIXME come up with some sane error reporting mechanism? */
>> +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;
>> +     __u64 user_data;
>> +};
>> +
>>  #endif
>> --
>> 1.8.3.1
>
> --
> Ville Syrjälä
> Intel OTC
Ville Syrjälä Oct. 7, 2013, 2:18 p.m. UTC | #3
On Mon, Oct 07, 2013 at 09:55:47AM -0400, Rob Clark wrote:
> On Mon, Oct 7, 2013 at 9:28 AM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Sat, Oct 05, 2013 at 08:45:49PM -0400, Rob Clark wrote:
> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>
> >> The atomic modeset ioctl cna 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. For setting values to blob properties, the property value
> >> indicates the length of the data, and the actual data is passed via
> >> another blob pointer.
> >>
> >> The caller can demand non-blocking operation from the ioctl, and if the
> >> driver can't satisfy that requirement an error will be returned.
> >>
> >> The caller can also request to receive asynchronous completion events
> >> after the operation has reached the hardware. An event is sent for each
> >> object specified by the caller, whether or not the actual state of
> >> that object changed. Each event also carries a framebuffer ID, which
> >> indicates to user space that the specified object is no longer
> >> accessing that framebuffer.
> >>
> >> TODO: detailed error reporting?
> >>
> >> v1: original
> >> v2: rebase on uapi changes, and drm state structs.. -- Rob Clark
> >> v3: rebase, missing padding in drm_event_atomic.. -- Rob Clark
> >>
> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/drm_crtc.c  | 159 ++++++++++++++++++++++++++++++++++++++++++++
> >>  drivers/gpu/drm/drm_drv.c   |   1 +
> >>  include/drm/drmP.h          |   6 ++
> >>  include/drm/drm_crtc.h      |   2 +
> >>  include/uapi/drm/drm.h      |  12 ++++
> >>  include/uapi/drm/drm_mode.h |  16 +++++
> >>  6 files changed, 196 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> >> index 09396a9..910e5c6 100644
> >> --- a/drivers/gpu/drm/drm_crtc.c
> >> +++ b/drivers/gpu/drm/drm_crtc.c
> >> @@ -4338,3 +4338,162 @@ void drm_mode_config_cleanup(struct drm_device *dev)
> >>       idr_destroy(&dev->mode_config.crtc_idr);
> >>  }
> >>  EXPORT_SYMBOL(drm_mode_config_cleanup);
> >> +
> >> +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);
> >> +     uint64_t __user *blob_values_ptr = (uint64_t __user *)(unsigned long)(arg->blob_values_ptr);
> >> +     unsigned int copied_objs = 0;
> >> +     unsigned int copied_props = 0;
> >> +     unsigned int copied_blobs = 0;
> >> +     void *state;
> >> +     int ret = 0;
> >> +     unsigned int i, j;
> >> +
> >> +     if (arg->flags & ~(DRM_MODE_ATOMIC_TEST_ONLY |
> >> +                     DRM_MODE_ATOMIC_EVENT | DRM_MODE_ATOMIC_NONBLOCK))
> >> +             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_ATOMIC_EVENT))
> >> +             return -EINVAL;
> >> +
> >> +     mutex_lock(&dev->mode_config.mutex);
> >
> > I'm pretty sure I had a version w/ fixed locking...
> >
> > git://gitorious.org/vsyrjala/linux.git rebased_drm_atomic_24
> 
> oh, hmm.. actually the locking should no longer be in the ioctl fxn,
> but should be pushed down to the commit.  looks like I missed this.  I
> need to dig up some actual test code for atomic ioctl ;-)

It can't be in commit. At the very least it has to be around
check+commit, and also you need to hold some locks when you're copying
the current config over to your temp storage. That's assuming you store
_everything_ in the temp storage and so it doesn't matter if the current
state can change between the copy and check+commit.

> 
> >> +
> >> +     state = dev->driver->atomic_begin(dev, arg->flags);
> >> +     if (IS_ERR(state)) {
> >> +             ret = PTR_ERR(state);
> >> +             goto unlock;
> >> +     }
> >> +
> >> +     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 out;
> >> +             }
> >> +
> >> +             obj = drm_mode_object_find(dev, obj_id, DRM_MODE_OBJECT_ANY);
> >> +             if (!obj || !obj->properties) {
> >> +                     ret = -ENOENT;
> >> +                     goto out;
> >> +             }
> >> +
> >> +             if (arg->flags & DRM_MODE_ATOMIC_EVENT) {
> >> +                     // XXX create atomic event instead..
> >
> > Some people are apparently more comfortable with a per-crtc event
> > rather than the per-obj events I added. So I think we might want
> > both in the end.
> 
> yeah, sorting out events is a bit 'TODO' at the moment.  I do kind of
> like per-crtc event.. it seems easier to implement and I'm not really
> sure what finer event granularity buys us.

Mainly it gets you the old fb. If you limit youself to < vrefresh it's
not a big deal, but going faster than that and you start to want that
information.

> 
> BR,
> -R
> 
 >> +                     struct drm_pending_vblank_event *e =
> >> +                             create_vblank_event(dev, file_priv, arg->user_data);
> >> +                     if (!e) {
> >> +                             ret = -ENOMEM;
> >> +                             goto out;
> >> +                     }
> >> +                     ret = dev->driver->atomic_set_event(dev, state, obj, e);
> >> +                     if (ret) {
> >> +                             destroy_vblank_event(dev, file_priv, e);
> >> +                             goto out;
> >> +                     }
> >> +             }
> >> +
> >> +             if (get_user(count_props, count_props_ptr + copied_objs)) {
> >> +                     ret = -EFAULT;
> >> +                     goto out;
> >> +             }
> >> +
> >> +             copied_objs++;
> >> +
> >> +             for (j = 0; j < count_props; j++) {
> >> +                     uint32_t prop_id;
> >> +                     uint64_t prop_value;
> >> +                     struct drm_mode_object *prop_obj;
> >> +                     struct drm_property *prop;
> >> +                     void *blob_data = NULL;
> >> +
> >> +                     if (get_user(prop_id, props_ptr + copied_props)) {
> >> +                             ret = -EFAULT;
> >> +                             goto out;
> >> +                     }
> >> +
> >> +                     if (!object_has_prop(obj, prop_id)) {
> >> +                             ret = -EINVAL;
> >> +                             goto out;
> >> +                     }
> >> +
> >> +                     prop_obj = drm_mode_object_find(dev, prop_id,
> >> +                                     DRM_MODE_OBJECT_PROPERTY);
> >> +                     if (!prop_obj) {
> >> +                             ret = -ENOENT;
> >> +                             goto out;
> >> +                     }
> >> +                     prop = obj_to_property(prop_obj);
> >> +
> >> +                     if (get_user(prop_value, prop_values_ptr + copied_props)) {
> >> +                             ret = -EFAULT;
> >> +                             goto out;
> >> +                     }
> >> +
> >> +                     if (!drm_property_change_is_valid(prop, prop_value)) {
> >> +                             ret = -EINVAL;
> >> +                             goto out;
> >> +                     }
> >> +
> >> +                     if ((prop->flags & DRM_MODE_PROP_BLOB) && prop_value) {
> >> +                             uint64_t blob_ptr;
> >> +
> >> +                             if (get_user(blob_ptr, blob_values_ptr + copied_blobs)) {
> >> +                                     ret = -EFAULT;
> >> +                                     goto out;
> >> +                             }
> >> +
> >> +                             blob_data = kmalloc(prop_value, GFP_KERNEL);
> >> +                             if (!blob_data) {
> >> +                                     ret = -ENOMEM;
> >> +                                     goto out;
> >> +                             }
> >> +
> >> +                             if (copy_from_user(blob_data, (void __user *)(unsigned long)blob_ptr, prop_value)) {
> >> +                                     kfree(blob_data);
> >> +                                     ret = -EFAULT;
> >> +                                     goto out;
> >> +                             }
> >> +                     }
> >> +
> >> +                     /* User space sends the blob pointer even if we
> >> +                      * don't use it (length==0).
> >> +                      */
> >> +                     if (prop->flags & DRM_MODE_PROP_BLOB)
> >> +                             copied_blobs++;
> >> +
> >> +                     /* The driver will be in charge of blob_data from now on. */
> >> +                     ret = drm_mode_set_obj_prop(obj, state, prop,
> >> +                                     prop_value, blob_data);
> >> +                     if (ret)
> >> +                             goto out;
> >> +
> >> +                     copied_props++;
> >> +             }
> >> +     }
> >> +
> >> +     ret = dev->driver->atomic_check(dev, state);
> >> +     if (ret)
> >> +             goto out;
> >> +
> >> +     if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)
> >> +             goto out;
> >> +
> >> +     ret = dev->driver->atomic_commit(dev, state);
> >> +
> >> + out:
> >> +     dev->driver->atomic_end(dev, state);
> >> + unlock:
> >> +     mutex_unlock(&dev->mode_config.mutex);
> >> +
> >> +     return ret;
> >> +}
> >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> >> index e572dd2..43e04ae 100644
> >> --- a/drivers/gpu/drm/drm_drv.c
> >> +++ b/drivers/gpu/drm/drm_drv.c
> >> @@ -166,6 +166,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/drmP.h b/include/drm/drmP.h
> >> index 4c3de22..f282733 100644
> >> --- a/include/drm/drmP.h
> >> +++ b/include/drm/drmP.h
> >> @@ -1158,6 +1158,12 @@ struct drm_pending_vblank_event {
> >>       struct drm_event_vblank event;
> >>  };
> >>
> >> +struct drm_pending_atomic_event {
> >> +     struct drm_pending_event base;
> >> +     int pipe;
> >> +     struct drm_event_atomic event;
> >> +};
> >> +
> >>  /**
> >>   * DRM device structure. This structure represent a complete card that
> >>   * may contain multiple heads.
> >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> >> index 86a5a00..fa3956e 100644
> >> --- a/include/drm/drm_crtc.h
> >> +++ b/include/drm/drm_crtc.h
> >> @@ -1313,6 +1313,8 @@ extern int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> >>                                            struct drm_file *file_priv);
> >>  extern int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
> >>                                          struct drm_file *file_priv);
> >> +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 ece8678..efb1461 100644
> >> --- a/include/uapi/drm/drm.h
> >> +++ b/include/uapi/drm/drm.h
> >> @@ -733,6 +733,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
> >> @@ -774,6 +775,17 @@ struct drm_event_vblank {
> >>       __u32 reserved;
> >>  };
> >>
> >> +struct drm_event_atomic {
> >> +     struct drm_event base;
> >> +     __u64 user_data;
> >> +     __u32 tv_sec;
> >> +     __u32 tv_usec;
> >> +     __u32 sequence;
> >> +     __u32 obj_id;
> >> +     __u32 old_fb_id;
> >> +     __u32 reserved;
> >> +};
> >> +
> >>  #define DRM_CAP_DUMB_BUFFER 0x1
> >>  #define DRM_CAP_VBLANK_HIGH_CRTC 0x2
> >>  #define DRM_CAP_DUMB_PREFERRED_DEPTH 0x3
> >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> >> index 6d4f089..03a473c 100644
> >> --- a/include/uapi/drm/drm_mode.h
> >> +++ b/include/uapi/drm/drm_mode.h
> >> @@ -489,4 +489,20 @@ struct drm_mode_destroy_dumb {
> >>       uint32_t handle;
> >>  };
> >>
> >> +#define DRM_MODE_ATOMIC_TEST_ONLY (1<<0)
> >> +#define DRM_MODE_ATOMIC_EVENT (1<<1)
> >> +#define DRM_MODE_ATOMIC_NONBLOCK (1<<2)
> >> +
> >> +/* FIXME come up with some sane error reporting mechanism? */
> >> +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;
> >> +     __u64 user_data;
> >> +};
> >> +
> >>  #endif
> >> --
> >> 1.8.3.1
> >
> > --
> > Ville Syrjälä
> > Intel OTC
Rob Clark Oct. 7, 2013, 2:39 p.m. UTC | #4
On Mon, Oct 7, 2013 at 10:18 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Mon, Oct 07, 2013 at 09:55:47AM -0400, Rob Clark wrote:
>> On Mon, Oct 7, 2013 at 9:28 AM, Ville Syrjälä
>> <ville.syrjala@linux.intel.com> wrote:
>> > On Sat, Oct 05, 2013 at 08:45:49PM -0400, Rob Clark wrote:
>> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >>
>> >> The atomic modeset ioctl cna 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. For setting values to blob properties, the property value
>> >> indicates the length of the data, and the actual data is passed via
>> >> another blob pointer.
>> >>
>> >> The caller can demand non-blocking operation from the ioctl, and if the
>> >> driver can't satisfy that requirement an error will be returned.
>> >>
>> >> The caller can also request to receive asynchronous completion events
>> >> after the operation has reached the hardware. An event is sent for each
>> >> object specified by the caller, whether or not the actual state of
>> >> that object changed. Each event also carries a framebuffer ID, which
>> >> indicates to user space that the specified object is no longer
>> >> accessing that framebuffer.
>> >>
>> >> TODO: detailed error reporting?
>> >>
>> >> v1: original
>> >> v2: rebase on uapi changes, and drm state structs.. -- Rob Clark
>> >> v3: rebase, missing padding in drm_event_atomic.. -- Rob Clark
>> >>
>> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >> ---
>> >>  drivers/gpu/drm/drm_crtc.c  | 159 ++++++++++++++++++++++++++++++++++++++++++++
>> >>  drivers/gpu/drm/drm_drv.c   |   1 +
>> >>  include/drm/drmP.h          |   6 ++
>> >>  include/drm/drm_crtc.h      |   2 +
>> >>  include/uapi/drm/drm.h      |  12 ++++
>> >>  include/uapi/drm/drm_mode.h |  16 +++++
>> >>  6 files changed, 196 insertions(+)
>> >>
>> >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> >> index 09396a9..910e5c6 100644
>> >> --- a/drivers/gpu/drm/drm_crtc.c
>> >> +++ b/drivers/gpu/drm/drm_crtc.c
>> >> @@ -4338,3 +4338,162 @@ void drm_mode_config_cleanup(struct drm_device *dev)
>> >>       idr_destroy(&dev->mode_config.crtc_idr);
>> >>  }
>> >>  EXPORT_SYMBOL(drm_mode_config_cleanup);
>> >> +
>> >> +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);
>> >> +     uint64_t __user *blob_values_ptr = (uint64_t __user *)(unsigned long)(arg->blob_values_ptr);
>> >> +     unsigned int copied_objs = 0;
>> >> +     unsigned int copied_props = 0;
>> >> +     unsigned int copied_blobs = 0;
>> >> +     void *state;
>> >> +     int ret = 0;
>> >> +     unsigned int i, j;
>> >> +
>> >> +     if (arg->flags & ~(DRM_MODE_ATOMIC_TEST_ONLY |
>> >> +                     DRM_MODE_ATOMIC_EVENT | DRM_MODE_ATOMIC_NONBLOCK))
>> >> +             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_ATOMIC_EVENT))
>> >> +             return -EINVAL;
>> >> +
>> >> +     mutex_lock(&dev->mode_config.mutex);
>> >
>> > I'm pretty sure I had a version w/ fixed locking...
>> >
>> > git://gitorious.org/vsyrjala/linux.git rebased_drm_atomic_24
>>
>> oh, hmm.. actually the locking should no longer be in the ioctl fxn,
>> but should be pushed down to the commit.  looks like I missed this.  I
>> need to dig up some actual test code for atomic ioctl ;-)
>
> It can't be in commit. At the very least it has to be around
> check+commit, and also you need to hold some locks when you're copying
> the current config over to your temp storage. That's assuming you store
> _everything_ in the temp storage and so it doesn't matter if the current
> state can change between the copy and check+commit.
>

hmm.. I start to wish we did atomic first, before the fine grained
locking re-work ;-)

>>
>> >> +
>> >> +     state = dev->driver->atomic_begin(dev, arg->flags);
>> >> +     if (IS_ERR(state)) {
>> >> +             ret = PTR_ERR(state);
>> >> +             goto unlock;
>> >> +     }
>> >> +
>> >> +     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 out;
>> >> +             }
>> >> +
>> >> +             obj = drm_mode_object_find(dev, obj_id, DRM_MODE_OBJECT_ANY);
>> >> +             if (!obj || !obj->properties) {
>> >> +                     ret = -ENOENT;
>> >> +                     goto out;
>> >> +             }
>> >> +
>> >> +             if (arg->flags & DRM_MODE_ATOMIC_EVENT) {
>> >> +                     // XXX create atomic event instead..
>> >
>> > Some people are apparently more comfortable with a per-crtc event
>> > rather than the per-obj events I added. So I think we might want
>> > both in the end.
>>
>> yeah, sorting out events is a bit 'TODO' at the moment.  I do kind of
>> like per-crtc event.. it seems easier to implement and I'm not really
>> sure what finer event granularity buys us.
>
> Mainly it gets you the old fb. If you limit youself to < vrefresh it's
> not a big deal, but going faster than that and you start to want that
> information.
>

oh, right.. well I guess it wouldn't be too hard to add the current
primary plane fb_id to the existing event, which might be sufficient.
At any rate, I don't mind adding new event flags to atomic ioctl once
we get atomic in place.

BR,
-R

>>
>> BR,
>> -R
>>
>  >> +                     struct drm_pending_vblank_event *e =
>> >> +                             create_vblank_event(dev, file_priv, arg->user_data);
>> >> +                     if (!e) {
>> >> +                             ret = -ENOMEM;
>> >> +                             goto out;
>> >> +                     }
>> >> +                     ret = dev->driver->atomic_set_event(dev, state, obj, e);
>> >> +                     if (ret) {
>> >> +                             destroy_vblank_event(dev, file_priv, e);
>> >> +                             goto out;
>> >> +                     }
>> >> +             }
>> >> +
>> >> +             if (get_user(count_props, count_props_ptr + copied_objs)) {
>> >> +                     ret = -EFAULT;
>> >> +                     goto out;
>> >> +             }
>> >> +
>> >> +             copied_objs++;
>> >> +
>> >> +             for (j = 0; j < count_props; j++) {
>> >> +                     uint32_t prop_id;
>> >> +                     uint64_t prop_value;
>> >> +                     struct drm_mode_object *prop_obj;
>> >> +                     struct drm_property *prop;
>> >> +                     void *blob_data = NULL;
>> >> +
>> >> +                     if (get_user(prop_id, props_ptr + copied_props)) {
>> >> +                             ret = -EFAULT;
>> >> +                             goto out;
>> >> +                     }
>> >> +
>> >> +                     if (!object_has_prop(obj, prop_id)) {
>> >> +                             ret = -EINVAL;
>> >> +                             goto out;
>> >> +                     }
>> >> +
>> >> +                     prop_obj = drm_mode_object_find(dev, prop_id,
>> >> +                                     DRM_MODE_OBJECT_PROPERTY);
>> >> +                     if (!prop_obj) {
>> >> +                             ret = -ENOENT;
>> >> +                             goto out;
>> >> +                     }
>> >> +                     prop = obj_to_property(prop_obj);
>> >> +
>> >> +                     if (get_user(prop_value, prop_values_ptr + copied_props)) {
>> >> +                             ret = -EFAULT;
>> >> +                             goto out;
>> >> +                     }
>> >> +
>> >> +                     if (!drm_property_change_is_valid(prop, prop_value)) {
>> >> +                             ret = -EINVAL;
>> >> +                             goto out;
>> >> +                     }
>> >> +
>> >> +                     if ((prop->flags & DRM_MODE_PROP_BLOB) && prop_value) {
>> >> +                             uint64_t blob_ptr;
>> >> +
>> >> +                             if (get_user(blob_ptr, blob_values_ptr + copied_blobs)) {
>> >> +                                     ret = -EFAULT;
>> >> +                                     goto out;
>> >> +                             }
>> >> +
>> >> +                             blob_data = kmalloc(prop_value, GFP_KERNEL);
>> >> +                             if (!blob_data) {
>> >> +                                     ret = -ENOMEM;
>> >> +                                     goto out;
>> >> +                             }
>> >> +
>> >> +                             if (copy_from_user(blob_data, (void __user *)(unsigned long)blob_ptr, prop_value)) {
>> >> +                                     kfree(blob_data);
>> >> +                                     ret = -EFAULT;
>> >> +                                     goto out;
>> >> +                             }
>> >> +                     }
>> >> +
>> >> +                     /* User space sends the blob pointer even if we
>> >> +                      * don't use it (length==0).
>> >> +                      */
>> >> +                     if (prop->flags & DRM_MODE_PROP_BLOB)
>> >> +                             copied_blobs++;
>> >> +
>> >> +                     /* The driver will be in charge of blob_data from now on. */
>> >> +                     ret = drm_mode_set_obj_prop(obj, state, prop,
>> >> +                                     prop_value, blob_data);
>> >> +                     if (ret)
>> >> +                             goto out;
>> >> +
>> >> +                     copied_props++;
>> >> +             }
>> >> +     }
>> >> +
>> >> +     ret = dev->driver->atomic_check(dev, state);
>> >> +     if (ret)
>> >> +             goto out;
>> >> +
>> >> +     if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)
>> >> +             goto out;
>> >> +
>> >> +     ret = dev->driver->atomic_commit(dev, state);
>> >> +
>> >> + out:
>> >> +     dev->driver->atomic_end(dev, state);
>> >> + unlock:
>> >> +     mutex_unlock(&dev->mode_config.mutex);
>> >> +
>> >> +     return ret;
>> >> +}
>> >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> >> index e572dd2..43e04ae 100644
>> >> --- a/drivers/gpu/drm/drm_drv.c
>> >> +++ b/drivers/gpu/drm/drm_drv.c
>> >> @@ -166,6 +166,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/drmP.h b/include/drm/drmP.h
>> >> index 4c3de22..f282733 100644
>> >> --- a/include/drm/drmP.h
>> >> +++ b/include/drm/drmP.h
>> >> @@ -1158,6 +1158,12 @@ struct drm_pending_vblank_event {
>> >>       struct drm_event_vblank event;
>> >>  };
>> >>
>> >> +struct drm_pending_atomic_event {
>> >> +     struct drm_pending_event base;
>> >> +     int pipe;
>> >> +     struct drm_event_atomic event;
>> >> +};
>> >> +
>> >>  /**
>> >>   * DRM device structure. This structure represent a complete card that
>> >>   * may contain multiple heads.
>> >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> >> index 86a5a00..fa3956e 100644
>> >> --- a/include/drm/drm_crtc.h
>> >> +++ b/include/drm/drm_crtc.h
>> >> @@ -1313,6 +1313,8 @@ extern int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>> >>                                            struct drm_file *file_priv);
>> >>  extern int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
>> >>                                          struct drm_file *file_priv);
>> >> +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 ece8678..efb1461 100644
>> >> --- a/include/uapi/drm/drm.h
>> >> +++ b/include/uapi/drm/drm.h
>> >> @@ -733,6 +733,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
>> >> @@ -774,6 +775,17 @@ struct drm_event_vblank {
>> >>       __u32 reserved;
>> >>  };
>> >>
>> >> +struct drm_event_atomic {
>> >> +     struct drm_event base;
>> >> +     __u64 user_data;
>> >> +     __u32 tv_sec;
>> >> +     __u32 tv_usec;
>> >> +     __u32 sequence;
>> >> +     __u32 obj_id;
>> >> +     __u32 old_fb_id;
>> >> +     __u32 reserved;
>> >> +};
>> >> +
>> >>  #define DRM_CAP_DUMB_BUFFER 0x1
>> >>  #define DRM_CAP_VBLANK_HIGH_CRTC 0x2
>> >>  #define DRM_CAP_DUMB_PREFERRED_DEPTH 0x3
>> >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> >> index 6d4f089..03a473c 100644
>> >> --- a/include/uapi/drm/drm_mode.h
>> >> +++ b/include/uapi/drm/drm_mode.h
>> >> @@ -489,4 +489,20 @@ struct drm_mode_destroy_dumb {
>> >>       uint32_t handle;
>> >>  };
>> >>
>> >> +#define DRM_MODE_ATOMIC_TEST_ONLY (1<<0)
>> >> +#define DRM_MODE_ATOMIC_EVENT (1<<1)
>> >> +#define DRM_MODE_ATOMIC_NONBLOCK (1<<2)
>> >> +
>> >> +/* FIXME come up with some sane error reporting mechanism? */
>> >> +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;
>> >> +     __u64 user_data;
>> >> +};
>> >> +
>> >>  #endif
>> >> --
>> >> 1.8.3.1
>> >
>> > --
>> > Ville Syrjälä
>> > Intel OTC
>
> --
> Ville Syrjälä
> Intel OTC
Ville Syrjälä Oct. 7, 2013, 3:05 p.m. UTC | #5
On Mon, Oct 07, 2013 at 10:39:01AM -0400, Rob Clark wrote:
> On Mon, Oct 7, 2013 at 10:18 AM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Mon, Oct 07, 2013 at 09:55:47AM -0400, Rob Clark wrote:
> >> On Mon, Oct 7, 2013 at 9:28 AM, Ville Syrjälä
> >> <ville.syrjala@linux.intel.com> wrote:
> >> > On Sat, Oct 05, 2013 at 08:45:49PM -0400, Rob Clark wrote:
> >> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> >>
> >> >> The atomic modeset ioctl cna 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. For setting values to blob properties, the property value
> >> >> indicates the length of the data, and the actual data is passed via
> >> >> another blob pointer.
> >> >>
> >> >> The caller can demand non-blocking operation from the ioctl, and if the
> >> >> driver can't satisfy that requirement an error will be returned.
> >> >>
> >> >> The caller can also request to receive asynchronous completion events
> >> >> after the operation has reached the hardware. An event is sent for each
> >> >> object specified by the caller, whether or not the actual state of
> >> >> that object changed. Each event also carries a framebuffer ID, which
> >> >> indicates to user space that the specified object is no longer
> >> >> accessing that framebuffer.
> >> >>
> >> >> TODO: detailed error reporting?
> >> >>
> >> >> v1: original
> >> >> v2: rebase on uapi changes, and drm state structs.. -- Rob Clark
> >> >> v3: rebase, missing padding in drm_event_atomic.. -- Rob Clark
> >> >>
> >> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> >> ---
> >> >>  drivers/gpu/drm/drm_crtc.c  | 159 ++++++++++++++++++++++++++++++++++++++++++++
> >> >>  drivers/gpu/drm/drm_drv.c   |   1 +
> >> >>  include/drm/drmP.h          |   6 ++
> >> >>  include/drm/drm_crtc.h      |   2 +
> >> >>  include/uapi/drm/drm.h      |  12 ++++
> >> >>  include/uapi/drm/drm_mode.h |  16 +++++
> >> >>  6 files changed, 196 insertions(+)
> >> >>
> >> >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> >> >> index 09396a9..910e5c6 100644
> >> >> --- a/drivers/gpu/drm/drm_crtc.c
> >> >> +++ b/drivers/gpu/drm/drm_crtc.c
> >> >> @@ -4338,3 +4338,162 @@ void drm_mode_config_cleanup(struct drm_device *dev)
> >> >>       idr_destroy(&dev->mode_config.crtc_idr);
> >> >>  }
> >> >>  EXPORT_SYMBOL(drm_mode_config_cleanup);
> >> >> +
> >> >> +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);
> >> >> +     uint64_t __user *blob_values_ptr = (uint64_t __user *)(unsigned long)(arg->blob_values_ptr);
> >> >> +     unsigned int copied_objs = 0;
> >> >> +     unsigned int copied_props = 0;
> >> >> +     unsigned int copied_blobs = 0;
> >> >> +     void *state;
> >> >> +     int ret = 0;
> >> >> +     unsigned int i, j;
> >> >> +
> >> >> +     if (arg->flags & ~(DRM_MODE_ATOMIC_TEST_ONLY |
> >> >> +                     DRM_MODE_ATOMIC_EVENT | DRM_MODE_ATOMIC_NONBLOCK))
> >> >> +             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_ATOMIC_EVENT))
> >> >> +             return -EINVAL;
> >> >> +
> >> >> +     mutex_lock(&dev->mode_config.mutex);
> >> >
> >> > I'm pretty sure I had a version w/ fixed locking...
> >> >
> >> > git://gitorious.org/vsyrjala/linux.git rebased_drm_atomic_24
> >>
> >> oh, hmm.. actually the locking should no longer be in the ioctl fxn,
> >> but should be pushed down to the commit.  looks like I missed this.  I
> >> need to dig up some actual test code for atomic ioctl ;-)
> >
> > It can't be in commit. At the very least it has to be around
> > check+commit, and also you need to hold some locks when you're copying
> > the current config over to your temp storage. That's assuming you store
> > _everything_ in the temp storage and so it doesn't matter if the current
> > state can change between the copy and check+commit.
> >
> 
> hmm.. I start to wish we did atomic first, before the fine grained
> locking re-work ;-)

I don't see it mattering for this particular problem if we just grab
the big lock anyway.

But as I already stated, I would prefer to solve the plane locking 
before doing atomic. Whether we have per-plane locks or not can
actually matter for this code, especially when dealing with planes
that can move between crtcs.

And we still have locking issues to solve in the .check+commit steps
when we have to deal with shared resources across the entire device.
We can't simply use some small local lock in the .check step since 
something relevant might have changed by the time we reach .commit.
I guess we could re-check such things at the very start of commit,
before we've clobbered any state, and then update the global state
to match if everything looks good, before we again drop the local
lock.

> 
> >>
> >> >> +
> >> >> +     state = dev->driver->atomic_begin(dev, arg->flags);
> >> >> +     if (IS_ERR(state)) {
> >> >> +             ret = PTR_ERR(state);
> >> >> +             goto unlock;
> >> >> +     }
> >> >> +
> >> >> +     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 out;
> >> >> +             }
> >> >> +
> >> >> +             obj = drm_mode_object_find(dev, obj_id, DRM_MODE_OBJECT_ANY);
> >> >> +             if (!obj || !obj->properties) {
> >> >> +                     ret = -ENOENT;
> >> >> +                     goto out;
> >> >> +             }
> >> >> +
> >> >> +             if (arg->flags & DRM_MODE_ATOMIC_EVENT) {
> >> >> +                     // XXX create atomic event instead..
> >> >
> >> > Some people are apparently more comfortable with a per-crtc event
> >> > rather than the per-obj events I added. So I think we might want
> >> > both in the end.
> >>
> >> yeah, sorting out events is a bit 'TODO' at the moment.  I do kind of
> >> like per-crtc event.. it seems easier to implement and I'm not really
> >> sure what finer event granularity buys us.
> >
> > Mainly it gets you the old fb. If you limit youself to < vrefresh it's
> > not a big deal, but going faster than that and you start to want that
> > information.
> >
> 
> oh, right.. well I guess it wouldn't be too hard to add the current
> primary plane fb_id to the existing event, which might be sufficient.
>
> At any rate, I don't mind adding new event flags to atomic ioctl once
> we get atomic in place.

Yeah adding new events isn't that hard. We just need to make sure
things make sense. The code you posted doesn't :P

You add the event to every object, even though you only wanted one
event per crtc. The same object could also be listed multiple times in
the object list, so the code would even add multiple events for the
same object.

> 
> BR,
> -R
> 
> >>
> >> BR,
> >> -R
> >>
> >  >> +                     struct drm_pending_vblank_event *e =
> >> >> +                             create_vblank_event(dev, file_priv, arg->user_data);
> >> >> +                     if (!e) {
> >> >> +                             ret = -ENOMEM;
> >> >> +                             goto out;
> >> >> +                     }
> >> >> +                     ret = dev->driver->atomic_set_event(dev, state, obj, e);
> >> >> +                     if (ret) {
> >> >> +                             destroy_vblank_event(dev, file_priv, e);
> >> >> +                             goto out;
> >> >> +                     }
> >> >> +             }
> >> >> +
> >> >> +             if (get_user(count_props, count_props_ptr + copied_objs)) {
> >> >> +                     ret = -EFAULT;
> >> >> +                     goto out;
> >> >> +             }
> >> >> +
> >> >> +             copied_objs++;
> >> >> +
> >> >> +             for (j = 0; j < count_props; j++) {
> >> >> +                     uint32_t prop_id;
> >> >> +                     uint64_t prop_value;
> >> >> +                     struct drm_mode_object *prop_obj;
> >> >> +                     struct drm_property *prop;
> >> >> +                     void *blob_data = NULL;
> >> >> +
> >> >> +                     if (get_user(prop_id, props_ptr + copied_props)) {
> >> >> +                             ret = -EFAULT;
> >> >> +                             goto out;
> >> >> +                     }
> >> >> +
> >> >> +                     if (!object_has_prop(obj, prop_id)) {
> >> >> +                             ret = -EINVAL;
> >> >> +                             goto out;
> >> >> +                     }
> >> >> +
> >> >> +                     prop_obj = drm_mode_object_find(dev, prop_id,
> >> >> +                                     DRM_MODE_OBJECT_PROPERTY);
> >> >> +                     if (!prop_obj) {
> >> >> +                             ret = -ENOENT;
> >> >> +                             goto out;
> >> >> +                     }
> >> >> +                     prop = obj_to_property(prop_obj);
> >> >> +
> >> >> +                     if (get_user(prop_value, prop_values_ptr + copied_props)) {
> >> >> +                             ret = -EFAULT;
> >> >> +                             goto out;
> >> >> +                     }
> >> >> +
> >> >> +                     if (!drm_property_change_is_valid(prop, prop_value)) {
> >> >> +                             ret = -EINVAL;
> >> >> +                             goto out;
> >> >> +                     }
> >> >> +
> >> >> +                     if ((prop->flags & DRM_MODE_PROP_BLOB) && prop_value) {
> >> >> +                             uint64_t blob_ptr;
> >> >> +
> >> >> +                             if (get_user(blob_ptr, blob_values_ptr + copied_blobs)) {
> >> >> +                                     ret = -EFAULT;
> >> >> +                                     goto out;
> >> >> +                             }
> >> >> +
> >> >> +                             blob_data = kmalloc(prop_value, GFP_KERNEL);
> >> >> +                             if (!blob_data) {
> >> >> +                                     ret = -ENOMEM;
> >> >> +                                     goto out;
> >> >> +                             }
> >> >> +
> >> >> +                             if (copy_from_user(blob_data, (void __user *)(unsigned long)blob_ptr, prop_value)) {
> >> >> +                                     kfree(blob_data);
> >> >> +                                     ret = -EFAULT;
> >> >> +                                     goto out;
> >> >> +                             }
> >> >> +                     }
> >> >> +
> >> >> +                     /* User space sends the blob pointer even if we
> >> >> +                      * don't use it (length==0).
> >> >> +                      */
> >> >> +                     if (prop->flags & DRM_MODE_PROP_BLOB)
> >> >> +                             copied_blobs++;
> >> >> +
> >> >> +                     /* The driver will be in charge of blob_data from now on. */
> >> >> +                     ret = drm_mode_set_obj_prop(obj, state, prop,
> >> >> +                                     prop_value, blob_data);
> >> >> +                     if (ret)
> >> >> +                             goto out;
> >> >> +
> >> >> +                     copied_props++;
> >> >> +             }
> >> >> +     }
> >> >> +
> >> >> +     ret = dev->driver->atomic_check(dev, state);
> >> >> +     if (ret)
> >> >> +             goto out;
> >> >> +
> >> >> +     if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)
> >> >> +             goto out;
> >> >> +
> >> >> +     ret = dev->driver->atomic_commit(dev, state);
> >> >> +
> >> >> + out:
> >> >> +     dev->driver->atomic_end(dev, state);
> >> >> + unlock:
> >> >> +     mutex_unlock(&dev->mode_config.mutex);
> >> >> +
> >> >> +     return ret;
> >> >> +}
> >> >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> >> >> index e572dd2..43e04ae 100644
> >> >> --- a/drivers/gpu/drm/drm_drv.c
> >> >> +++ b/drivers/gpu/drm/drm_drv.c
> >> >> @@ -166,6 +166,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/drmP.h b/include/drm/drmP.h
> >> >> index 4c3de22..f282733 100644
> >> >> --- a/include/drm/drmP.h
> >> >> +++ b/include/drm/drmP.h
> >> >> @@ -1158,6 +1158,12 @@ struct drm_pending_vblank_event {
> >> >>       struct drm_event_vblank event;
> >> >>  };
> >> >>
> >> >> +struct drm_pending_atomic_event {
> >> >> +     struct drm_pending_event base;
> >> >> +     int pipe;
> >> >> +     struct drm_event_atomic event;
> >> >> +};
> >> >> +
> >> >>  /**
> >> >>   * DRM device structure. This structure represent a complete card that
> >> >>   * may contain multiple heads.
> >> >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> >> >> index 86a5a00..fa3956e 100644
> >> >> --- a/include/drm/drm_crtc.h
> >> >> +++ b/include/drm/drm_crtc.h
> >> >> @@ -1313,6 +1313,8 @@ extern int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> >> >>                                            struct drm_file *file_priv);
> >> >>  extern int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
> >> >>                                          struct drm_file *file_priv);
> >> >> +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 ece8678..efb1461 100644
> >> >> --- a/include/uapi/drm/drm.h
> >> >> +++ b/include/uapi/drm/drm.h
> >> >> @@ -733,6 +733,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
> >> >> @@ -774,6 +775,17 @@ struct drm_event_vblank {
> >> >>       __u32 reserved;
> >> >>  };
> >> >>
> >> >> +struct drm_event_atomic {
> >> >> +     struct drm_event base;
> >> >> +     __u64 user_data;
> >> >> +     __u32 tv_sec;
> >> >> +     __u32 tv_usec;
> >> >> +     __u32 sequence;
> >> >> +     __u32 obj_id;
> >> >> +     __u32 old_fb_id;
> >> >> +     __u32 reserved;
> >> >> +};
> >> >> +
> >> >>  #define DRM_CAP_DUMB_BUFFER 0x1
> >> >>  #define DRM_CAP_VBLANK_HIGH_CRTC 0x2
> >> >>  #define DRM_CAP_DUMB_PREFERRED_DEPTH 0x3
> >> >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> >> >> index 6d4f089..03a473c 100644
> >> >> --- a/include/uapi/drm/drm_mode.h
> >> >> +++ b/include/uapi/drm/drm_mode.h
> >> >> @@ -489,4 +489,20 @@ struct drm_mode_destroy_dumb {
> >> >>       uint32_t handle;
> >> >>  };
> >> >>
> >> >> +#define DRM_MODE_ATOMIC_TEST_ONLY (1<<0)
> >> >> +#define DRM_MODE_ATOMIC_EVENT (1<<1)
> >> >> +#define DRM_MODE_ATOMIC_NONBLOCK (1<<2)
> >> >> +
> >> >> +/* FIXME come up with some sane error reporting mechanism? */
> >> >> +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;
> >> >> +     __u64 user_data;
> >> >> +};
> >> >> +
> >> >>  #endif
> >> >> --
> >> >> 1.8.3.1
> >> >
> >> > --
> >> > Ville Syrjälä
> >> > Intel OTC
> >
> > --
> > Ville Syrjälä
> > Intel OTC
Rob Clark Oct. 7, 2013, 3:20 p.m. UTC | #6
On Mon, Oct 7, 2013 at 11:05 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Mon, Oct 07, 2013 at 10:39:01AM -0400, Rob Clark wrote:
>> On Mon, Oct 7, 2013 at 10:18 AM, Ville Syrjälä
>> <ville.syrjala@linux.intel.com> wrote:
>> > On Mon, Oct 07, 2013 at 09:55:47AM -0400, Rob Clark wrote:
>> >> On Mon, Oct 7, 2013 at 9:28 AM, Ville Syrjälä
>> >> <ville.syrjala@linux.intel.com> wrote:
>> >> > On Sat, Oct 05, 2013 at 08:45:49PM -0400, Rob Clark wrote:
>> >> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >> >>
>> >> >> The atomic modeset ioctl cna 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. For setting values to blob properties, the property value
>> >> >> indicates the length of the data, and the actual data is passed via
>> >> >> another blob pointer.
>> >> >>
>> >> >> The caller can demand non-blocking operation from the ioctl, and if the
>> >> >> driver can't satisfy that requirement an error will be returned.
>> >> >>
>> >> >> The caller can also request to receive asynchronous completion events
>> >> >> after the operation has reached the hardware. An event is sent for each
>> >> >> object specified by the caller, whether or not the actual state of
>> >> >> that object changed. Each event also carries a framebuffer ID, which
>> >> >> indicates to user space that the specified object is no longer
>> >> >> accessing that framebuffer.
>> >> >>
>> >> >> TODO: detailed error reporting?
>> >> >>
>> >> >> v1: original
>> >> >> v2: rebase on uapi changes, and drm state structs.. -- Rob Clark
>> >> >> v3: rebase, missing padding in drm_event_atomic.. -- Rob Clark
>> >> >>
>> >> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >> >> ---
>> >> >>  drivers/gpu/drm/drm_crtc.c  | 159 ++++++++++++++++++++++++++++++++++++++++++++
>> >> >>  drivers/gpu/drm/drm_drv.c   |   1 +
>> >> >>  include/drm/drmP.h          |   6 ++
>> >> >>  include/drm/drm_crtc.h      |   2 +
>> >> >>  include/uapi/drm/drm.h      |  12 ++++
>> >> >>  include/uapi/drm/drm_mode.h |  16 +++++
>> >> >>  6 files changed, 196 insertions(+)
>> >> >>
>> >> >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> >> >> index 09396a9..910e5c6 100644
>> >> >> --- a/drivers/gpu/drm/drm_crtc.c
>> >> >> +++ b/drivers/gpu/drm/drm_crtc.c
>> >> >> @@ -4338,3 +4338,162 @@ void drm_mode_config_cleanup(struct drm_device *dev)
>> >> >>       idr_destroy(&dev->mode_config.crtc_idr);
>> >> >>  }
>> >> >>  EXPORT_SYMBOL(drm_mode_config_cleanup);
>> >> >> +
>> >> >> +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);
>> >> >> +     uint64_t __user *blob_values_ptr = (uint64_t __user *)(unsigned long)(arg->blob_values_ptr);
>> >> >> +     unsigned int copied_objs = 0;
>> >> >> +     unsigned int copied_props = 0;
>> >> >> +     unsigned int copied_blobs = 0;
>> >> >> +     void *state;
>> >> >> +     int ret = 0;
>> >> >> +     unsigned int i, j;
>> >> >> +
>> >> >> +     if (arg->flags & ~(DRM_MODE_ATOMIC_TEST_ONLY |
>> >> >> +                     DRM_MODE_ATOMIC_EVENT | DRM_MODE_ATOMIC_NONBLOCK))
>> >> >> +             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_ATOMIC_EVENT))
>> >> >> +             return -EINVAL;
>> >> >> +
>> >> >> +     mutex_lock(&dev->mode_config.mutex);
>> >> >
>> >> > I'm pretty sure I had a version w/ fixed locking...
>> >> >
>> >> > git://gitorious.org/vsyrjala/linux.git rebased_drm_atomic_24
>> >>
>> >> oh, hmm.. actually the locking should no longer be in the ioctl fxn,
>> >> but should be pushed down to the commit.  looks like I missed this.  I
>> >> need to dig up some actual test code for atomic ioctl ;-)
>> >
>> > It can't be in commit. At the very least it has to be around
>> > check+commit, and also you need to hold some locks when you're copying
>> > the current config over to your temp storage. That's assuming you store
>> > _everything_ in the temp storage and so it doesn't matter if the current
>> > state can change between the copy and check+commit.
>> >
>>
>> hmm.. I start to wish we did atomic first, before the fine grained
>> locking re-work ;-)
>
> I don't see it mattering for this particular problem if we just grab
> the big lock anyway.
>
> But as I already stated, I would prefer to solve the plane locking
> before doing atomic. Whether we have per-plane locks or not can
> actually matter for this code, especially when dealing with planes
> that can move between crtcs.

right, and I think the only sane way to deal w/ per-plane locking is
ww_mutex..  but then at least for crtc move case we can just grab both
old and new crtc lock

> And we still have locking issues to solve in the .check+commit steps
> when we have to deal with shared resources across the entire device.
> We can't simply use some small local lock in the .check step since
> something relevant might have changed by the time we reach .commit.
> I guess we could re-check such things at the very start of commit,
> before we've clobbered any state, and then update the global state
> to match if everything looks good, before we again drop the local
> lock.

I suppose an easy thing would be per-dev atomic to generate sequence
#'s for state updates.. then it would be pretty easy to check if there
has been a state update since atomic_begin()..

>>
>> >>
>> >> >> +
>> >> >> +     state = dev->driver->atomic_begin(dev, arg->flags);
>> >> >> +     if (IS_ERR(state)) {
>> >> >> +             ret = PTR_ERR(state);
>> >> >> +             goto unlock;
>> >> >> +     }
>> >> >> +
>> >> >> +     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 out;
>> >> >> +             }
>> >> >> +
>> >> >> +             obj = drm_mode_object_find(dev, obj_id, DRM_MODE_OBJECT_ANY);
>> >> >> +             if (!obj || !obj->properties) {
>> >> >> +                     ret = -ENOENT;
>> >> >> +                     goto out;
>> >> >> +             }
>> >> >> +
>> >> >> +             if (arg->flags & DRM_MODE_ATOMIC_EVENT) {
>> >> >> +                     // XXX create atomic event instead..
>> >> >
>> >> > Some people are apparently more comfortable with a per-crtc event
>> >> > rather than the per-obj events I added. So I think we might want
>> >> > both in the end.
>> >>
>> >> yeah, sorting out events is a bit 'TODO' at the moment.  I do kind of
>> >> like per-crtc event.. it seems easier to implement and I'm not really
>> >> sure what finer event granularity buys us.
>> >
>> > Mainly it gets you the old fb. If you limit youself to < vrefresh it's
>> > not a big deal, but going faster than that and you start to want that
>> > information.
>> >
>>
>> oh, right.. well I guess it wouldn't be too hard to add the current
>> primary plane fb_id to the existing event, which might be sufficient.
>>
>> At any rate, I don't mind adding new event flags to atomic ioctl once
>> we get atomic in place.
>
> Yeah adding new events isn't that hard. We just need to make sure
> things make sense. The code you posted doesn't :P
>
> You add the event to every object, even though you only wanted one
> event per crtc. The same object could also be listed multiple times in
> the object list, so the code would even add multiple events for the
> same object.

yeah, current atomic patch is almost an afterthought in this series.
I still need to get planes working on msm and find some test code (I
guess you have some somewhere?)..  so other than just rebasing it
enough to compile, I didn't really spend any time on it yet.  Right
now I was more interested in getting folks to have a look at the
atomic funcs and helpers.

BR,
-R

>>
>> BR,
>> -R
>>
>> >>
>> >> BR,
>> >> -R
>> >>
>> >  >> +                     struct drm_pending_vblank_event *e =
>> >> >> +                             create_vblank_event(dev, file_priv, arg->user_data);
>> >> >> +                     if (!e) {
>> >> >> +                             ret = -ENOMEM;
>> >> >> +                             goto out;
>> >> >> +                     }
>> >> >> +                     ret = dev->driver->atomic_set_event(dev, state, obj, e);
>> >> >> +                     if (ret) {
>> >> >> +                             destroy_vblank_event(dev, file_priv, e);
>> >> >> +                             goto out;
>> >> >> +                     }
>> >> >> +             }
>> >> >> +
>> >> >> +             if (get_user(count_props, count_props_ptr + copied_objs)) {
>> >> >> +                     ret = -EFAULT;
>> >> >> +                     goto out;
>> >> >> +             }
>> >> >> +
>> >> >> +             copied_objs++;
>> >> >> +
>> >> >> +             for (j = 0; j < count_props; j++) {
>> >> >> +                     uint32_t prop_id;
>> >> >> +                     uint64_t prop_value;
>> >> >> +                     struct drm_mode_object *prop_obj;
>> >> >> +                     struct drm_property *prop;
>> >> >> +                     void *blob_data = NULL;
>> >> >> +
>> >> >> +                     if (get_user(prop_id, props_ptr + copied_props)) {
>> >> >> +                             ret = -EFAULT;
>> >> >> +                             goto out;
>> >> >> +                     }
>> >> >> +
>> >> >> +                     if (!object_has_prop(obj, prop_id)) {
>> >> >> +                             ret = -EINVAL;
>> >> >> +                             goto out;
>> >> >> +                     }
>> >> >> +
>> >> >> +                     prop_obj = drm_mode_object_find(dev, prop_id,
>> >> >> +                                     DRM_MODE_OBJECT_PROPERTY);
>> >> >> +                     if (!prop_obj) {
>> >> >> +                             ret = -ENOENT;
>> >> >> +                             goto out;
>> >> >> +                     }
>> >> >> +                     prop = obj_to_property(prop_obj);
>> >> >> +
>> >> >> +                     if (get_user(prop_value, prop_values_ptr + copied_props)) {
>> >> >> +                             ret = -EFAULT;
>> >> >> +                             goto out;
>> >> >> +                     }
>> >> >> +
>> >> >> +                     if (!drm_property_change_is_valid(prop, prop_value)) {
>> >> >> +                             ret = -EINVAL;
>> >> >> +                             goto out;
>> >> >> +                     }
>> >> >> +
>> >> >> +                     if ((prop->flags & DRM_MODE_PROP_BLOB) && prop_value) {
>> >> >> +                             uint64_t blob_ptr;
>> >> >> +
>> >> >> +                             if (get_user(blob_ptr, blob_values_ptr + copied_blobs)) {
>> >> >> +                                     ret = -EFAULT;
>> >> >> +                                     goto out;
>> >> >> +                             }
>> >> >> +
>> >> >> +                             blob_data = kmalloc(prop_value, GFP_KERNEL);
>> >> >> +                             if (!blob_data) {
>> >> >> +                                     ret = -ENOMEM;
>> >> >> +                                     goto out;
>> >> >> +                             }
>> >> >> +
>> >> >> +                             if (copy_from_user(blob_data, (void __user *)(unsigned long)blob_ptr, prop_value)) {
>> >> >> +                                     kfree(blob_data);
>> >> >> +                                     ret = -EFAULT;
>> >> >> +                                     goto out;
>> >> >> +                             }
>> >> >> +                     }
>> >> >> +
>> >> >> +                     /* User space sends the blob pointer even if we
>> >> >> +                      * don't use it (length==0).
>> >> >> +                      */
>> >> >> +                     if (prop->flags & DRM_MODE_PROP_BLOB)
>> >> >> +                             copied_blobs++;
>> >> >> +
>> >> >> +                     /* The driver will be in charge of blob_data from now on. */
>> >> >> +                     ret = drm_mode_set_obj_prop(obj, state, prop,
>> >> >> +                                     prop_value, blob_data);
>> >> >> +                     if (ret)
>> >> >> +                             goto out;
>> >> >> +
>> >> >> +                     copied_props++;
>> >> >> +             }
>> >> >> +     }
>> >> >> +
>> >> >> +     ret = dev->driver->atomic_check(dev, state);
>> >> >> +     if (ret)
>> >> >> +             goto out;
>> >> >> +
>> >> >> +     if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)
>> >> >> +             goto out;
>> >> >> +
>> >> >> +     ret = dev->driver->atomic_commit(dev, state);
>> >> >> +
>> >> >> + out:
>> >> >> +     dev->driver->atomic_end(dev, state);
>> >> >> + unlock:
>> >> >> +     mutex_unlock(&dev->mode_config.mutex);
>> >> >> +
>> >> >> +     return ret;
>> >> >> +}
>> >> >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> >> >> index e572dd2..43e04ae 100644
>> >> >> --- a/drivers/gpu/drm/drm_drv.c
>> >> >> +++ b/drivers/gpu/drm/drm_drv.c
>> >> >> @@ -166,6 +166,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/drmP.h b/include/drm/drmP.h
>> >> >> index 4c3de22..f282733 100644
>> >> >> --- a/include/drm/drmP.h
>> >> >> +++ b/include/drm/drmP.h
>> >> >> @@ -1158,6 +1158,12 @@ struct drm_pending_vblank_event {
>> >> >>       struct drm_event_vblank event;
>> >> >>  };
>> >> >>
>> >> >> +struct drm_pending_atomic_event {
>> >> >> +     struct drm_pending_event base;
>> >> >> +     int pipe;
>> >> >> +     struct drm_event_atomic event;
>> >> >> +};
>> >> >> +
>> >> >>  /**
>> >> >>   * DRM device structure. This structure represent a complete card that
>> >> >>   * may contain multiple heads.
>> >> >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> >> >> index 86a5a00..fa3956e 100644
>> >> >> --- a/include/drm/drm_crtc.h
>> >> >> +++ b/include/drm/drm_crtc.h
>> >> >> @@ -1313,6 +1313,8 @@ extern int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>> >> >>                                            struct drm_file *file_priv);
>> >> >>  extern int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
>> >> >>                                          struct drm_file *file_priv);
>> >> >> +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 ece8678..efb1461 100644
>> >> >> --- a/include/uapi/drm/drm.h
>> >> >> +++ b/include/uapi/drm/drm.h
>> >> >> @@ -733,6 +733,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
>> >> >> @@ -774,6 +775,17 @@ struct drm_event_vblank {
>> >> >>       __u32 reserved;
>> >> >>  };
>> >> >>
>> >> >> +struct drm_event_atomic {
>> >> >> +     struct drm_event base;
>> >> >> +     __u64 user_data;
>> >> >> +     __u32 tv_sec;
>> >> >> +     __u32 tv_usec;
>> >> >> +     __u32 sequence;
>> >> >> +     __u32 obj_id;
>> >> >> +     __u32 old_fb_id;
>> >> >> +     __u32 reserved;
>> >> >> +};
>> >> >> +
>> >> >>  #define DRM_CAP_DUMB_BUFFER 0x1
>> >> >>  #define DRM_CAP_VBLANK_HIGH_CRTC 0x2
>> >> >>  #define DRM_CAP_DUMB_PREFERRED_DEPTH 0x3
>> >> >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> >> >> index 6d4f089..03a473c 100644
>> >> >> --- a/include/uapi/drm/drm_mode.h
>> >> >> +++ b/include/uapi/drm/drm_mode.h
>> >> >> @@ -489,4 +489,20 @@ struct drm_mode_destroy_dumb {
>> >> >>       uint32_t handle;
>> >> >>  };
>> >> >>
>> >> >> +#define DRM_MODE_ATOMIC_TEST_ONLY (1<<0)
>> >> >> +#define DRM_MODE_ATOMIC_EVENT (1<<1)
>> >> >> +#define DRM_MODE_ATOMIC_NONBLOCK (1<<2)
>> >> >> +
>> >> >> +/* FIXME come up with some sane error reporting mechanism? */
>> >> >> +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;
>> >> >> +     __u64 user_data;
>> >> >> +};
>> >> >> +
>> >> >>  #endif
>> >> >> --
>> >> >> 1.8.3.1
>> >> >
>> >> > --
>> >> > Ville Syrjälä
>> >> > Intel OTC
>> >
>> > --
>> > Ville Syrjälä
>> > Intel OTC
>
> --
> Ville Syrjälä
> Intel OTC
Daniel Vetter Oct. 7, 2013, 5:56 p.m. UTC | #7
On Mon, Oct 07, 2013 at 11:20:54AM -0400, Rob Clark wrote:
> yeah, current atomic patch is almost an afterthought in this series.
> I still need to get planes working on msm and find some test code (I
> guess you have some somewhere?)..  so other than just rebasing it
> enough to compile, I didn't really spend any time on it yet.  Right
> now I was more interested in getting folks to have a look at the
> atomic funcs and helpers.

One in-kernel test user we have is the fbdev restore code - it needs to
get rid of all planes, cursors and set up its new config on all crtcs. I
think this would be a fairly nice (albeit really basic testcase): You can
set up a bunch of planes/cursors, then vt-switch to the fbcon - this way
we can exercise the atomic stuff in drivers a bit better even without
needing to add any ioctl.
-Daniel
Rob Clark Oct. 7, 2013, 6:49 p.m. UTC | #8
On Mon, Oct 7, 2013 at 1:56 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Oct 07, 2013 at 11:20:54AM -0400, Rob Clark wrote:
>> yeah, current atomic patch is almost an afterthought in this series.
>> I still need to get planes working on msm and find some test code (I
>> guess you have some somewhere?)..  so other than just rebasing it
>> enough to compile, I didn't really spend any time on it yet.  Right
>> now I was more interested in getting folks to have a look at the
>> atomic funcs and helpers.
>
> One in-kernel test user we have is the fbdev restore code - it needs to
> get rid of all planes, cursors and set up its new config on all crtcs. I
> think this would be a fairly nice (albeit really basic testcase): You can
> set up a bunch of planes/cursors, then vt-switch to the fbcon - this way
> we can exercise the atomic stuff in drivers a bit better even without
> needing to add any ioctl.

right..  well, we do have code that disables the planes on fbdev
restore, and fbdev restore (and initial modeset) still hit
crtc->set_config directly.  But once those are converted it would be
cleaner / more elegant to do fbdev restore w/ atomic.

BR,
-R

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Matt Roper Oct. 8, 2013, 6:35 p.m. UTC | #9
On Mon, Oct 07, 2013 at 05:18:02PM +0300, Ville Syrjälä wrote:
> On Mon, Oct 07, 2013 at 09:55:47AM -0400, Rob Clark wrote:
> > On Mon, Oct 7, 2013 at 9:28 AM, Ville Syrjälä
> > <ville.syrjala@linux.intel.com> wrote:
> > > On Sat, Oct 05, 2013 at 08:45:49PM -0400, Rob Clark wrote:
...
> > >> +
> > >> +     state = dev->driver->atomic_begin(dev, arg->flags);
> > >> +     if (IS_ERR(state)) {
> > >> +             ret = PTR_ERR(state);
> > >> +             goto unlock;
> > >> +     }
> > >> +
> > >> +     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 out;
> > >> +             }
> > >> +
> > >> +             obj = drm_mode_object_find(dev, obj_id, DRM_MODE_OBJECT_ANY);
> > >> +             if (!obj || !obj->properties) {
> > >> +                     ret = -ENOENT;
> > >> +                     goto out;
> > >> +             }
> > >> +
> > >> +             if (arg->flags & DRM_MODE_ATOMIC_EVENT) {
> > >> +                     // XXX create atomic event instead..
> > >
> > > Some people are apparently more comfortable with a per-crtc event
> > > rather than the per-obj events I added. So I think we might want
> > > both in the end.
> > 
> > yeah, sorting out events is a bit 'TODO' at the moment.  I do kind of
> > like per-crtc event.. it seems easier to implement and I'm not really
> > sure what finer event granularity buys us.
> 
> Mainly it gets you the old fb. If you limit youself to < vrefresh it's
> not a big deal, but going faster than that and you start to want that
> information.

Are there cases where userspace would really need this fb information
returned?  I'm having trouble envisioning a case where userspace
wouldn't have its own bookkeeping to know which fb's are currently
displayed on each plane and which fb's are pending.

If you're running a compositor like Weston, the redraw of a display is
generally tied to completion events, so there's no real desire/potential
for > vrefresh submission in most cases.  In this model, having a single
per-crtc "I'm done updating everything" event is easier to work with
than an event per object where you have to do some additional counting
to make sure you consume the right number of events before kicking off
the repaint.


> 
> > 
> > BR,
> > -R
> > 
>  >> +                     struct drm_pending_vblank_event *e =
> > >> +                             create_vblank_event(dev, file_priv, arg->user_data);
> > >> +                     if (!e) {
> > >> +                             ret = -ENOMEM;
> > >> +                             goto out;
> > >> +                     }
> > >> +                     ret = dev->driver->atomic_set_event(dev, state, obj, e);
> > >> +                     if (ret) {
> > >> +                             destroy_vblank_event(dev, file_priv, e);
> > >> +                             goto out;
> > >> +                     }
> > >> +             }

Did you mean to use the drm_pending_atomic_event/drm_event_atomic you
declare farther down?  It doesn't look like anything is actually using
them yet in this patch series.

> > >> +
> > >> +             if (get_user(count_props, count_props_ptr + copied_objs)) {
> > >> +                     ret = -EFAULT;
> > >> +                     goto out;
> > >> +             }
> > >> +
> > >> +             copied_objs++;
> > >> +
> > >> +             for (j = 0; j < count_props; j++) {
> > >> +                     uint32_t prop_id;
> > >> +                     uint64_t prop_value;
> > >> +                     struct drm_mode_object *prop_obj;
> > >> +                     struct drm_property *prop;
> > >> +                     void *blob_data = NULL;
> > >> +
> > >> +                     if (get_user(prop_id, props_ptr + copied_props)) {
> > >> +                             ret = -EFAULT;
> > >> +                             goto out;
> > >> +                     }
> > >> +
> > >> +                     if (!object_has_prop(obj, prop_id)) {
> > >> +                             ret = -EINVAL;
> > >> +                             goto out;
> > >> +                     }
> > >> +
> > >> +                     prop_obj = drm_mode_object_find(dev, prop_id,
> > >> +                                     DRM_MODE_OBJECT_PROPERTY);
> > >> +                     if (!prop_obj) {
> > >> +                             ret = -ENOENT;
> > >> +                             goto out;
> > >> +                     }
> > >> +                     prop = obj_to_property(prop_obj);
> > >> +
> > >> +                     if (get_user(prop_value, prop_values_ptr + copied_props)) {
> > >> +                             ret = -EFAULT;
> > >> +                             goto out;
> > >> +                     }
> > >> +
> > >> +                     if (!drm_property_change_is_valid(prop, prop_value)) {
> > >> +                             ret = -EINVAL;
> > >> +                             goto out;
> > >> +                     }
> > >> +
> > >> +                     if ((prop->flags & DRM_MODE_PROP_BLOB) && prop_value) {
> > >> +                             uint64_t blob_ptr;
> > >> +
> > >> +                             if (get_user(blob_ptr, blob_values_ptr + copied_blobs)) {
> > >> +                                     ret = -EFAULT;
> > >> +                                     goto out;
> > >> +                             }
> > >> +
> > >> +                             blob_data = kmalloc(prop_value, GFP_KERNEL);
> > >> +                             if (!blob_data) {
> > >> +                                     ret = -ENOMEM;
> > >> +                                     goto out;
> > >> +                             }
> > >> +
> > >> +                             if (copy_from_user(blob_data, (void __user *)(unsigned long)blob_ptr, prop_value)) {
> > >> +                                     kfree(blob_data);
> > >> +                                     ret = -EFAULT;
> > >> +                                     goto out;
> > >> +                             }
> > >> +                     }
> > >> +
> > >> +                     /* User space sends the blob pointer even if we
> > >> +                      * don't use it (length==0).
> > >> +                      */
> > >> +                     if (prop->flags & DRM_MODE_PROP_BLOB)
> > >> +                             copied_blobs++;
> > >> +
> > >> +                     /* The driver will be in charge of blob_data from now on. */
> > >> +                     ret = drm_mode_set_obj_prop(obj, state, prop,
> > >> +                                     prop_value, blob_data);
> > >> +                     if (ret)
> > >> +                             goto out;
> > >> +
> > >> +                     copied_props++;
> > >> +             }
> > >> +     }
> > >> +
> > >> +     ret = dev->driver->atomic_check(dev, state);
> > >> +     if (ret)
> > >> +             goto out;
> > >> +
> > >> +     if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)
> > >> +             goto out;
> > >> +
> > >> +     ret = dev->driver->atomic_commit(dev, state);
> > >> +
> > >> + out:
> > >> +     dev->driver->atomic_end(dev, state);
> > >> + unlock:
> > >> +     mutex_unlock(&dev->mode_config.mutex);
> > >> +
> > >> +     return ret;
> > >> +}
> > >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > >> index e572dd2..43e04ae 100644
> > >> --- a/drivers/gpu/drm/drm_drv.c
> > >> +++ b/drivers/gpu/drm/drm_drv.c
> > >> @@ -166,6 +166,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/drmP.h b/include/drm/drmP.h
> > >> index 4c3de22..f282733 100644
> > >> --- a/include/drm/drmP.h
> > >> +++ b/include/drm/drmP.h
> > >> @@ -1158,6 +1158,12 @@ struct drm_pending_vblank_event {
> > >>       struct drm_event_vblank event;
> > >>  };
> > >>
> > >> +struct drm_pending_atomic_event {
> > >> +     struct drm_pending_event base;
> > >> +     int pipe;
> > >> +     struct drm_event_atomic event;
> > >> +};
> > >> +
> > >>  /**
> > >>   * DRM device structure. This structure represent a complete card that
> > >>   * may contain multiple heads.
> > >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > >> index 86a5a00..fa3956e 100644
> > >> --- a/include/drm/drm_crtc.h
> > >> +++ b/include/drm/drm_crtc.h
> > >> @@ -1313,6 +1313,8 @@ extern int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> > >>                                            struct drm_file *file_priv);
> > >>  extern int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
> > >>                                          struct drm_file *file_priv);
> > >> +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 ece8678..efb1461 100644
> > >> --- a/include/uapi/drm/drm.h
> > >> +++ b/include/uapi/drm/drm.h
> > >> @@ -733,6 +733,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
> > >> @@ -774,6 +775,17 @@ struct drm_event_vblank {
> > >>       __u32 reserved;
> > >>  };
> > >>
> > >> +struct drm_event_atomic {
> > >> +     struct drm_event base;
> > >> +     __u64 user_data;
> > >> +     __u32 tv_sec;
> > >> +     __u32 tv_usec;
> > >> +     __u32 sequence;
> > >> +     __u32 obj_id;
> > >> +     __u32 old_fb_id;
> > >> +     __u32 reserved;
> > >> +};
> > >> +
> > >>  #define DRM_CAP_DUMB_BUFFER 0x1
> > >>  #define DRM_CAP_VBLANK_HIGH_CRTC 0x2
> > >>  #define DRM_CAP_DUMB_PREFERRED_DEPTH 0x3
> > >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > >> index 6d4f089..03a473c 100644
> > >> --- a/include/uapi/drm/drm_mode.h
> > >> +++ b/include/uapi/drm/drm_mode.h
> > >> @@ -489,4 +489,20 @@ struct drm_mode_destroy_dumb {
> > >>       uint32_t handle;
> > >>  };
> > >>
> > >> +#define DRM_MODE_ATOMIC_TEST_ONLY (1<<0)
> > >> +#define DRM_MODE_ATOMIC_EVENT (1<<1)
> > >> +#define DRM_MODE_ATOMIC_NONBLOCK (1<<2)
> > >> +
> > >> +/* FIXME come up with some sane error reporting mechanism? */
> > >> +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;
> > >> +     __u64 user_data;
> > >> +};
> > >> +
> > >>  #endif
> > >> --
> > >> 1.8.3.1
> > >
> > > --
> > > Ville Syrjälä
> > > Intel OTC
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Rob Clark Oct. 8, 2013, 6:46 p.m. UTC | #10
On Tue, Oct 8, 2013 at 2:35 PM, Matt Roper <matthew.d.roper@intel.com> wrote:
> On Mon, Oct 07, 2013 at 05:18:02PM +0300, Ville Syrjälä wrote:
>> On Mon, Oct 07, 2013 at 09:55:47AM -0400, Rob Clark wrote:
>> > On Mon, Oct 7, 2013 at 9:28 AM, Ville Syrjälä
>> > <ville.syrjala@linux.intel.com> wrote:
>> > > On Sat, Oct 05, 2013 at 08:45:49PM -0400, Rob Clark wrote:
> ...
>> > >> +
>> > >> +     state = dev->driver->atomic_begin(dev, arg->flags);
>> > >> +     if (IS_ERR(state)) {
>> > >> +             ret = PTR_ERR(state);
>> > >> +             goto unlock;
>> > >> +     }
>> > >> +
>> > >> +     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 out;
>> > >> +             }
>> > >> +
>> > >> +             obj = drm_mode_object_find(dev, obj_id, DRM_MODE_OBJECT_ANY);
>> > >> +             if (!obj || !obj->properties) {
>> > >> +                     ret = -ENOENT;
>> > >> +                     goto out;
>> > >> +             }
>> > >> +
>> > >> +             if (arg->flags & DRM_MODE_ATOMIC_EVENT) {
>> > >> +                     // XXX create atomic event instead..
>> > >
>> > > Some people are apparently more comfortable with a per-crtc event
>> > > rather than the per-obj events I added. So I think we might want
>> > > both in the end.
>> >
>> > yeah, sorting out events is a bit 'TODO' at the moment.  I do kind of
>> > like per-crtc event.. it seems easier to implement and I'm not really
>> > sure what finer event granularity buys us.
>>
>> Mainly it gets you the old fb. If you limit youself to < vrefresh it's
>> not a big deal, but going faster than that and you start to want that
>> information.
>
> Are there cases where userspace would really need this fb information
> returned?  I'm having trouble envisioning a case where userspace
> wouldn't have its own bookkeeping to know which fb's are currently
> displayed on each plane and which fb's are pending.

if you let userspace flip faster than vsync, then there is some
ambiguity about which buffer is being scanned out after the vblank..

but I think this shouldn't hold up atomic-modeset.. it should be safe
to spiff up the events as a later step.

> If you're running a compositor like Weston, the redraw of a display is
> generally tied to completion events, so there's no real desire/potential
> for > vrefresh submission in most cases.  In this model, having a single
> per-crtc "I'm done updating everything" event is easier to work with
> than an event per object where you have to do some additional counting
> to make sure you consume the right number of events before kicking off
> the repaint.
>
>
>>
>> >
>> > BR,
>> > -R
>> >
>>  >> +                     struct drm_pending_vblank_event *e =
>> > >> +                             create_vblank_event(dev, file_priv, arg->user_data);
>> > >> +                     if (!e) {
>> > >> +                             ret = -ENOMEM;
>> > >> +                             goto out;
>> > >> +                     }
>> > >> +                     ret = dev->driver->atomic_set_event(dev, state, obj, e);
>> > >> +                     if (ret) {
>> > >> +                             destroy_vblank_event(dev, file_priv, e);
>> > >> +                             goto out;
>> > >> +                     }
>> > >> +             }
>
> Did you mean to use the drm_pending_atomic_event/drm_event_atomic you
> declare farther down?  It doesn't look like anything is actually using
> them yet in this patch series.

right.. the event stuff is a bit messy at the moment as result of
merging of two different RFC patchsets about how to do atomic.

I think what I will end up doing is drop the atomic event, and fix up
the logic to only create/set vblank events for the crtc(s).

BR,
-R

>> > >> +
>> > >> +             if (get_user(count_props, count_props_ptr + copied_objs)) {
>> > >> +                     ret = -EFAULT;
>> > >> +                     goto out;
>> > >> +             }
>> > >> +
>> > >> +             copied_objs++;
>> > >> +
>> > >> +             for (j = 0; j < count_props; j++) {
>> > >> +                     uint32_t prop_id;
>> > >> +                     uint64_t prop_value;
>> > >> +                     struct drm_mode_object *prop_obj;
>> > >> +                     struct drm_property *prop;
>> > >> +                     void *blob_data = NULL;
>> > >> +
>> > >> +                     if (get_user(prop_id, props_ptr + copied_props)) {
>> > >> +                             ret = -EFAULT;
>> > >> +                             goto out;
>> > >> +                     }
>> > >> +
>> > >> +                     if (!object_has_prop(obj, prop_id)) {
>> > >> +                             ret = -EINVAL;
>> > >> +                             goto out;
>> > >> +                     }
>> > >> +
>> > >> +                     prop_obj = drm_mode_object_find(dev, prop_id,
>> > >> +                                     DRM_MODE_OBJECT_PROPERTY);
>> > >> +                     if (!prop_obj) {
>> > >> +                             ret = -ENOENT;
>> > >> +                             goto out;
>> > >> +                     }
>> > >> +                     prop = obj_to_property(prop_obj);
>> > >> +
>> > >> +                     if (get_user(prop_value, prop_values_ptr + copied_props)) {
>> > >> +                             ret = -EFAULT;
>> > >> +                             goto out;
>> > >> +                     }
>> > >> +
>> > >> +                     if (!drm_property_change_is_valid(prop, prop_value)) {
>> > >> +                             ret = -EINVAL;
>> > >> +                             goto out;
>> > >> +                     }
>> > >> +
>> > >> +                     if ((prop->flags & DRM_MODE_PROP_BLOB) && prop_value) {
>> > >> +                             uint64_t blob_ptr;
>> > >> +
>> > >> +                             if (get_user(blob_ptr, blob_values_ptr + copied_blobs)) {
>> > >> +                                     ret = -EFAULT;
>> > >> +                                     goto out;
>> > >> +                             }
>> > >> +
>> > >> +                             blob_data = kmalloc(prop_value, GFP_KERNEL);
>> > >> +                             if (!blob_data) {
>> > >> +                                     ret = -ENOMEM;
>> > >> +                                     goto out;
>> > >> +                             }
>> > >> +
>> > >> +                             if (copy_from_user(blob_data, (void __user *)(unsigned long)blob_ptr, prop_value)) {
>> > >> +                                     kfree(blob_data);
>> > >> +                                     ret = -EFAULT;
>> > >> +                                     goto out;
>> > >> +                             }
>> > >> +                     }
>> > >> +
>> > >> +                     /* User space sends the blob pointer even if we
>> > >> +                      * don't use it (length==0).
>> > >> +                      */
>> > >> +                     if (prop->flags & DRM_MODE_PROP_BLOB)
>> > >> +                             copied_blobs++;
>> > >> +
>> > >> +                     /* The driver will be in charge of blob_data from now on. */
>> > >> +                     ret = drm_mode_set_obj_prop(obj, state, prop,
>> > >> +                                     prop_value, blob_data);
>> > >> +                     if (ret)
>> > >> +                             goto out;
>> > >> +
>> > >> +                     copied_props++;
>> > >> +             }
>> > >> +     }
>> > >> +
>> > >> +     ret = dev->driver->atomic_check(dev, state);
>> > >> +     if (ret)
>> > >> +             goto out;
>> > >> +
>> > >> +     if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)
>> > >> +             goto out;
>> > >> +
>> > >> +     ret = dev->driver->atomic_commit(dev, state);
>> > >> +
>> > >> + out:
>> > >> +     dev->driver->atomic_end(dev, state);
>> > >> + unlock:
>> > >> +     mutex_unlock(&dev->mode_config.mutex);
>> > >> +
>> > >> +     return ret;
>> > >> +}
>> > >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> > >> index e572dd2..43e04ae 100644
>> > >> --- a/drivers/gpu/drm/drm_drv.c
>> > >> +++ b/drivers/gpu/drm/drm_drv.c
>> > >> @@ -166,6 +166,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/drmP.h b/include/drm/drmP.h
>> > >> index 4c3de22..f282733 100644
>> > >> --- a/include/drm/drmP.h
>> > >> +++ b/include/drm/drmP.h
>> > >> @@ -1158,6 +1158,12 @@ struct drm_pending_vblank_event {
>> > >>       struct drm_event_vblank event;
>> > >>  };
>> > >>
>> > >> +struct drm_pending_atomic_event {
>> > >> +     struct drm_pending_event base;
>> > >> +     int pipe;
>> > >> +     struct drm_event_atomic event;
>> > >> +};
>> > >> +
>> > >>  /**
>> > >>   * DRM device structure. This structure represent a complete card that
>> > >>   * may contain multiple heads.
>> > >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> > >> index 86a5a00..fa3956e 100644
>> > >> --- a/include/drm/drm_crtc.h
>> > >> +++ b/include/drm/drm_crtc.h
>> > >> @@ -1313,6 +1313,8 @@ extern int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>> > >>                                            struct drm_file *file_priv);
>> > >>  extern int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
>> > >>                                          struct drm_file *file_priv);
>> > >> +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 ece8678..efb1461 100644
>> > >> --- a/include/uapi/drm/drm.h
>> > >> +++ b/include/uapi/drm/drm.h
>> > >> @@ -733,6 +733,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
>> > >> @@ -774,6 +775,17 @@ struct drm_event_vblank {
>> > >>       __u32 reserved;
>> > >>  };
>> > >>
>> > >> +struct drm_event_atomic {
>> > >> +     struct drm_event base;
>> > >> +     __u64 user_data;
>> > >> +     __u32 tv_sec;
>> > >> +     __u32 tv_usec;
>> > >> +     __u32 sequence;
>> > >> +     __u32 obj_id;
>> > >> +     __u32 old_fb_id;
>> > >> +     __u32 reserved;
>> > >> +};
>> > >> +
>> > >>  #define DRM_CAP_DUMB_BUFFER 0x1
>> > >>  #define DRM_CAP_VBLANK_HIGH_CRTC 0x2
>> > >>  #define DRM_CAP_DUMB_PREFERRED_DEPTH 0x3
>> > >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> > >> index 6d4f089..03a473c 100644
>> > >> --- a/include/uapi/drm/drm_mode.h
>> > >> +++ b/include/uapi/drm/drm_mode.h
>> > >> @@ -489,4 +489,20 @@ struct drm_mode_destroy_dumb {
>> > >>       uint32_t handle;
>> > >>  };
>> > >>
>> > >> +#define DRM_MODE_ATOMIC_TEST_ONLY (1<<0)
>> > >> +#define DRM_MODE_ATOMIC_EVENT (1<<1)
>> > >> +#define DRM_MODE_ATOMIC_NONBLOCK (1<<2)
>> > >> +
>> > >> +/* FIXME come up with some sane error reporting mechanism? */
>> > >> +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;
>> > >> +     __u64 user_data;
>> > >> +};
>> > >> +
>> > >>  #endif
>> > >> --
>> > >> 1.8.3.1
>> > >
>> > > --
>> > > Ville Syrjälä
>> > > Intel OTC
>>
>> --
>> Ville Syrjälä
>> Intel OTC
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Matt Roper
> Intel Corporation
> Embedded Media & Graphics Driver Group
> (916) 356-2795
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 09396a9..910e5c6 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -4338,3 +4338,162 @@  void drm_mode_config_cleanup(struct drm_device *dev)
 	idr_destroy(&dev->mode_config.crtc_idr);
 }
 EXPORT_SYMBOL(drm_mode_config_cleanup);
+
+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);
+	uint64_t __user *blob_values_ptr = (uint64_t __user *)(unsigned long)(arg->blob_values_ptr);
+	unsigned int copied_objs = 0;
+	unsigned int copied_props = 0;
+	unsigned int copied_blobs = 0;
+	void *state;
+	int ret = 0;
+	unsigned int i, j;
+
+	if (arg->flags & ~(DRM_MODE_ATOMIC_TEST_ONLY |
+			DRM_MODE_ATOMIC_EVENT | DRM_MODE_ATOMIC_NONBLOCK))
+		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_ATOMIC_EVENT))
+		return -EINVAL;
+
+	mutex_lock(&dev->mode_config.mutex);
+
+	state = dev->driver->atomic_begin(dev, arg->flags);
+	if (IS_ERR(state)) {
+		ret = PTR_ERR(state);
+		goto unlock;
+	}
+
+	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 out;
+		}
+
+		obj = drm_mode_object_find(dev, obj_id, DRM_MODE_OBJECT_ANY);
+		if (!obj || !obj->properties) {
+			ret = -ENOENT;
+			goto out;
+		}
+
+		if (arg->flags & DRM_MODE_ATOMIC_EVENT) {
+			// XXX create atomic event instead..
+			struct drm_pending_vblank_event *e =
+				create_vblank_event(dev, file_priv, arg->user_data);
+			if (!e) {
+				ret = -ENOMEM;
+				goto out;
+			}
+			ret = dev->driver->atomic_set_event(dev, state, obj, e);
+			if (ret) {
+				destroy_vblank_event(dev, file_priv, e);
+				goto out;
+			}
+		}
+
+		if (get_user(count_props, count_props_ptr + copied_objs)) {
+			ret = -EFAULT;
+			goto out;
+		}
+
+		copied_objs++;
+
+		for (j = 0; j < count_props; j++) {
+			uint32_t prop_id;
+			uint64_t prop_value;
+			struct drm_mode_object *prop_obj;
+			struct drm_property *prop;
+			void *blob_data = NULL;
+
+			if (get_user(prop_id, props_ptr + copied_props)) {
+				ret = -EFAULT;
+				goto out;
+			}
+
+			if (!object_has_prop(obj, prop_id)) {
+				ret = -EINVAL;
+				goto out;
+			}
+
+			prop_obj = drm_mode_object_find(dev, prop_id,
+					DRM_MODE_OBJECT_PROPERTY);
+			if (!prop_obj) {
+				ret = -ENOENT;
+				goto out;
+			}
+			prop = obj_to_property(prop_obj);
+
+			if (get_user(prop_value, prop_values_ptr + copied_props)) {
+				ret = -EFAULT;
+				goto out;
+			}
+
+			if (!drm_property_change_is_valid(prop, prop_value)) {
+				ret = -EINVAL;
+				goto out;
+			}
+
+			if ((prop->flags & DRM_MODE_PROP_BLOB) && prop_value) {
+				uint64_t blob_ptr;
+
+				if (get_user(blob_ptr, blob_values_ptr + copied_blobs)) {
+					ret = -EFAULT;
+					goto out;
+				}
+
+				blob_data = kmalloc(prop_value, GFP_KERNEL);
+				if (!blob_data) {
+					ret = -ENOMEM;
+					goto out;
+				}
+
+				if (copy_from_user(blob_data, (void __user *)(unsigned long)blob_ptr, prop_value)) {
+					kfree(blob_data);
+					ret = -EFAULT;
+					goto out;
+				}
+			}
+
+			/* User space sends the blob pointer even if we
+			 * don't use it (length==0).
+			 */
+			if (prop->flags & DRM_MODE_PROP_BLOB)
+				copied_blobs++;
+
+			/* The driver will be in charge of blob_data from now on. */
+			ret = drm_mode_set_obj_prop(obj, state, prop,
+					prop_value, blob_data);
+			if (ret)
+				goto out;
+
+			copied_props++;
+		}
+	}
+
+	ret = dev->driver->atomic_check(dev, state);
+	if (ret)
+		goto out;
+
+	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)
+		goto out;
+
+	ret = dev->driver->atomic_commit(dev, state);
+
+ out:
+	dev->driver->atomic_end(dev, state);
+ unlock:
+	mutex_unlock(&dev->mode_config.mutex);
+
+	return ret;
+}
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index e572dd2..43e04ae 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -166,6 +166,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/drmP.h b/include/drm/drmP.h
index 4c3de22..f282733 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1158,6 +1158,12 @@  struct drm_pending_vblank_event {
 	struct drm_event_vblank event;
 };
 
+struct drm_pending_atomic_event {
+	struct drm_pending_event base;
+	int pipe;
+	struct drm_event_atomic event;
+};
+
 /**
  * DRM device structure. This structure represent a complete card that
  * may contain multiple heads.
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 86a5a00..fa3956e 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1313,6 +1313,8 @@  extern int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
 					     struct drm_file *file_priv);
 extern int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
 					   struct drm_file *file_priv);
+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 ece8678..efb1461 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -733,6 +733,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
@@ -774,6 +775,17 @@  struct drm_event_vblank {
 	__u32 reserved;
 };
 
+struct drm_event_atomic {
+	struct drm_event base;
+	__u64 user_data;
+	__u32 tv_sec;
+	__u32 tv_usec;
+	__u32 sequence;
+	__u32 obj_id;
+	__u32 old_fb_id;
+	__u32 reserved;
+};
+
 #define DRM_CAP_DUMB_BUFFER 0x1
 #define DRM_CAP_VBLANK_HIGH_CRTC 0x2
 #define DRM_CAP_DUMB_PREFERRED_DEPTH 0x3
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 6d4f089..03a473c 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -489,4 +489,20 @@  struct drm_mode_destroy_dumb {
 	uint32_t handle;
 };
 
+#define DRM_MODE_ATOMIC_TEST_ONLY (1<<0)
+#define DRM_MODE_ATOMIC_EVENT (1<<1)
+#define DRM_MODE_ATOMIC_NONBLOCK (1<<2)
+
+/* FIXME come up with some sane error reporting mechanism? */
+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;
+	__u64 user_data;
+};
+
 #endif