diff mbox

[RFCv3,12/14] drm: Atomic modeset ioctl

Message ID 1384980493-25499-13-git-send-email-robdclark@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Rob Clark Nov. 20, 2013, 8:48 p.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
v4: drop atomic event, align flags w/ pageflip (atomic flags should be
    a strick superset of pageflip flags to keep things easier for the
    drivers)

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c  | 157 +++++++++++++++++++++++++++++++++++++++++++-
 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 |  21 ++++++
 6 files changed, 198 insertions(+), 1 deletion(-)

Comments

Inki Dae Nov. 22, 2013, 8:35 a.m. UTC | #1
Hi Rob and Ville,


2013/11/21 Rob Clark <robdclark@gmail.com>:
> 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
> v4: drop atomic event, align flags w/ pageflip (atomic flags should be
>     a strick superset of pageflip flags to keep things easier for the
>     drivers)
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c  | 157 +++++++++++++++++++++++++++++++++++++++++++-
>  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 |  21 ++++++
>  6 files changed, 198 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 4b40a39..8368ef5 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -4037,7 +4037,8 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>                 return -ENOENT;
>         crtc = obj_to_crtc(obj);
>
> -       state = dev->driver->atomic_begin(dev, page_flip->flags);
> +       state = dev->driver->atomic_begin(dev,
> +                       page_flip->flags | DRM_MODE_ATOMIC_NONBLOCK);
>         if (IS_ERR(state))
>                 return PTR_ERR(state);
>
> @@ -4451,3 +4452,157 @@ 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)
> +{

Build error at this function like below,

        drivers/built-in.o: In function `drm_mode_atomic_ioctl':
        of_iommu.c:(.text+0x6d854): undefined reference to `__get_user_bad'
        of_iommu.c:(.text+0x6d940): undefined reference to `__get_user_bad'

And built correctly with the below change,

        diff --git a/arch/arm/lib/getuser.S b/arch/arm/lib/getuser.S
        index 9b06bb4..0f4f469 100644
        --- a/arch/arm/lib/getuser.S
        +++ b/arch/arm/lib/getuser.S
        @@ -66,7 +66,7 @@ ENTRY(__get_user_4)
                mov     pc, lr
         ENDPROC(__get_user_4)

        -__get_user_bad:
        +ENTRY(__get_user_bad)
                mov     r2, #0
                mov     r0, #-EFAULT
                mov     pc, lr

I guess __get_user_bad is not global but drm_mode_atomic_ioctl
function tries to refer this function as extern.

Is it build error only I can see?

Thanks,
Inki Dae
Rob Clark Nov. 22, 2013, 12:34 p.m. UTC | #2
On Fri, Nov 22, 2013 at 3:35 AM, Inki Dae <inki.dae@samsung.com> wrote:
> Hi Rob and Ville,
>
>
> 2013/11/21 Rob Clark <robdclark@gmail.com>:
>> 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
>> v4: drop atomic event, align flags w/ pageflip (atomic flags should be
>>     a strick superset of pageflip flags to keep things easier for the
>>     drivers)
>>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>  drivers/gpu/drm/drm_crtc.c  | 157 +++++++++++++++++++++++++++++++++++++++++++-
>>  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 |  21 ++++++
>>  6 files changed, 198 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index 4b40a39..8368ef5 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -4037,7 +4037,8 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>>                 return -ENOENT;
>>         crtc = obj_to_crtc(obj);
>>
>> -       state = dev->driver->atomic_begin(dev, page_flip->flags);
>> +       state = dev->driver->atomic_begin(dev,
>> +                       page_flip->flags | DRM_MODE_ATOMIC_NONBLOCK);
>>         if (IS_ERR(state))
>>                 return PTR_ERR(state);
>>
>> @@ -4451,3 +4452,157 @@ 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)
>> +{
>
> Build error at this function like below,
>
>         drivers/built-in.o: In function `drm_mode_atomic_ioctl':
>         of_iommu.c:(.text+0x6d854): undefined reference to `__get_user_bad'
>         of_iommu.c:(.text+0x6d940): undefined reference to `__get_user_bad'
>
> And built correctly with the below change,
>
>         diff --git a/arch/arm/lib/getuser.S b/arch/arm/lib/getuser.S
>         index 9b06bb4..0f4f469 100644
>         --- a/arch/arm/lib/getuser.S
>         +++ b/arch/arm/lib/getuser.S
>         @@ -66,7 +66,7 @@ ENTRY(__get_user_4)
>                 mov     pc, lr
>          ENDPROC(__get_user_4)
>
>         -__get_user_bad:
>         +ENTRY(__get_user_bad)
>                 mov     r2, #0
>                 mov     r0, #-EFAULT
>                 mov     pc, lr
>
> I guess __get_user_bad is not global but drm_mode_atomic_ioctl
> function tries to refer this function as extern.

no, actually __get_user_bad is (I think) supposed to be a build error.

You need this patch:

http://cgit.freedesktop.org/~robclark/linux/commit/?h=global-thermonuclear-war-5&id=1e94fad78b44d38ee3deca8f0694391e7db0e4cc

(Or I think Russell King had a slightly updated version of that.. I
need to work out with him about getting some version of that patch
merged.)

BR,
-R

> Is it build error only I can see?
>
> Thanks,
> Inki Dae
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 4b40a39..8368ef5 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -4037,7 +4037,8 @@  int drm_mode_page_flip_ioctl(struct drm_device *dev,
 		return -ENOENT;
 	crtc = obj_to_crtc(obj);
 
-	state = dev->driver->atomic_begin(dev, page_flip->flags);
+	state = dev->driver->atomic_begin(dev,
+			page_flip->flags | DRM_MODE_ATOMIC_NONBLOCK);
 	if (IS_ERR(state))
 		return PTR_ERR(state);
 
@@ -4451,3 +4452,157 @@  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_FLAGS)
+		return -EINVAL;
+
+	/* can't test and expect an event at the same time. */
+	if ((arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) &&
+			(arg->flags & DRM_MODE_PAGE_FLIP_EVENT))
+		return -EINVAL;
+
+	state = dev->driver->atomic_begin(dev, arg->flags);
+	if (IS_ERR(state)) {
+		ret = PTR_ERR(state);
+		goto out;
+	}
+
+	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 ((obj->type == DRM_MODE_OBJECT_CRTC) &&
+				(arg->flags & DRM_MODE_PAGE_FLIP_EVENT)) {
+			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);
+
+	return ret;
+}
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index d9137e4..cdadc1c 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -167,6 +167,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 4c54c88..0a372b2 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1157,6 +1157,12 @@  struct drm_vblank_crtc {
 					   once per disable */
 };
 
+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 2531658..6aae449 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1301,6 +1301,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 9b24d65..148c6c0 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -759,6 +759,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
@@ -800,6 +801,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;
+};
+
 /* typedef area */
 #ifndef __KERNEL__
 typedef struct drm_clip_rect drm_clip_rect_t;
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index a913953..7d34ee0 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -508,6 +508,27 @@  struct drm_mode_destroy_dumb {
 	uint32_t handle;
 };
 
+/* page-flip flags are valid, plus: */
+#define DRM_MODE_ATOMIC_TEST_ONLY 0x0100
+#define DRM_MODE_ATOMIC_NONBLOCK  0x0200
 #define DRM_MODE_ATOMIC_NOLOCK    0x8000  /* only used internally */
 
+#define DRM_MODE_ATOMIC_FLAGS (\
+		DRM_MODE_PAGE_FLIP_EVENT |\
+		DRM_MODE_PAGE_FLIP_ASYNC |\
+		DRM_MODE_ATOMIC_TEST_ONLY |\
+		DRM_MODE_ATOMIC_NONBLOCK)
+
+/* 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