[RFC] [GPU][DRM][PROPERTY] -Added a new ioctl in Linux DRM KMS driver.

[RFC] [GPU][DRM][PROPERTY] -Added a new ioctl in Linux DRM KMS driver.

[Satendra Singh Thakur]

From: satendra singh thakur <satendra.t@samsung.com>

-Added a new ioctl in Linux DRM KMS driver.
 This ioctl allows user to set the values of an object’s multiple
 properties in one go.
-In the absence of such ioctl, User would be calling one ioctl to set each
 property value;
 Thus, user needs to call N ioctls to set values of N properties of an
 object,  which is a time consuming and costly because each ioctl involves
  a system call entering/exiting kernel (context switch).
-This ioctl will set N properties (provided that HW allows it)
 in a single context switch
-This ioctl can be used to set multiple properties of ether a plane
 or a crtc or a connector (i.e. single object )
-This ioctl can't be used to set one property of a plane and
 one property of crtc in one go.

Signed-off-by: Satendra Singh Thakur <satendra.t@samsung.com>
---
 drivers/gpu/drm/drm_crtc_internal.h |    9 +-
 drivers/gpu/drm/drm_ioctl.c         |    1 +
 drivers/gpu/drm/drm_mode_object.c   |  183 +++++++++++++++++++++++++++++++++++
 include/uapi/drm/drm.h              |    1 +
 include/uapi/drm/drm_mode.h         |    8 ++
 5 files changed, 201 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index cdf6860..4bcae10 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -115,6 +115,12 @@ int drm_mode_object_get_properties(struct drm_mode_object *obj, bool atomic,
 				   uint32_t __user *prop_ptr,
 				   uint64_t __user *prop_values,
 				   uint32_t *arg_count_props);
+
+int drm_mode_object_set_properties(struct drm_device *dev,
+	struct drm_mode_object *obj,
+	bool atomic, uint32_t __user *prop_ptr,
+	uint64_t __user *prop_values, uint32_t *arg_count_props);
+
 struct drm_property *drm_mode_obj_find_prop_id(struct drm_mode_object *obj,
 					       uint32_t prop_id);
 
@@ -124,7 +130,8 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
 				      struct drm_file *file_priv);
 int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
 				    struct drm_file *file_priv);
-
+int drm_mode_obj_set_properties_ioctl(struct drm_device *dev, void *data,
+					struct drm_file *file_priv);
 /* drm_encoder.c */
 int drm_encoder_register_all(struct drm_device *dev);
 void drm_encoder_unregister_all(struct drm_device *dev);
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index fed22c2..bbad577 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -631,6 +631,7 @@ int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_MAP_DUMB, drm_mode_mmap_dumb_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_DESTROY_DUMB, drm_mode_destroy_dumb_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	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_SETPROPERTIES, drm_mode_obj_set_properties_ioctl, DRM_MASTER|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),
diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
index 9f17085..685500d 100644
--- a/drivers/gpu/drm/drm_mode_object.c
+++ b/drivers/gpu/drm/drm_mode_object.c
@@ -436,3 +436,186 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
 	drm_modeset_unlock_all(dev);
 	return ret;
 }
+
+/**
+ * drm_mode_object_set_properties
+ * helper for drm_mode_obj_set_properties_ioctl which can be used to set
+ * values of an object's numerous properties
+ * @dev: DRM device
+ * @obj: Object pointer
+ * @atomic: Flag that indicates atomocity
+ * @prop_ptr: pointer to array of prop IDs
+ * @prop_values: pointer to array of prop values
+ * @arg_count_props: pointer to total num of properties
+ *
+ * This function helps to set the values for an object's several properties
+ * in one go.
+ * Called by drm_mode_obj_set_properties_ioctl
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ * Fills prop_values with -1 for failed prop IDs.
+ * Fills arg_count_props with total successful props
+ */
+int drm_mode_object_set_properties(struct drm_device *dev,
+		struct drm_mode_object *obj, bool atomic,
+		uint32_t __user *prop_ptr,
+		uint64_t __user *prop_values, uint32_t *arg_count_props)
+{
+	struct drm_mode_object *ref = NULL;
+	int props_count;
+	int i, j, ret = 0, set = 0;
+	/* user requested property id, value, count*/
+	uint32_t prop_id_req;
+	uint64_t prop_val_req;
+	uint32_t props_count_req = *arg_count_props;
+
+	props_count = obj->properties->count;
+	if (!props_count) {
+		DRM_DEBUG_KMS("0 props_count\n");
+		return -EINVAL;
+	}
+	/*
+	 * if user sends arg_count_props = 0
+	 * user will get *arg_count_props = props_count
+	 */
+	if (!props_count_req) {
+		*arg_count_props = props_count;
+		return 0;
+	}
+	/* if user wish to set number of props <= props_count, its allowed*/
+	if (props_count_req > props_count) {
+		DRM_DEBUG_KMS("Invalid props_count\
+		 %u\n", props_count_req);
+		return -EINVAL;
+	}
+	for (j = 0; j < props_count_req; j++) {
+		if (get_user(prop_id_req, prop_ptr + j)) {
+			DRM_DEBUG_KMS("get_user failed\
+			 for prop_id %u\n", prop_id_req);
+			return -EFAULT;
+		}
+		if (get_user(prop_val_req, prop_values + j)) {
+			DRM_DEBUG_KMS("get_user failed\
+			 for prop_val %llu\n", prop_val_req);
+			return -EFAULT;
+		}
+		for (i = 0; i < props_count; i++) {
+			struct drm_property *prop =
+			obj->properties->properties[i];
+			/*in case prop inside the array
+			 *obj->properties->properties[]
+			 *are less than props_count
+			 */
+			if (!prop)
+				continue;
+			if (prop->base.id != prop_id_req)
+					continue;
+			if ((prop->flags & DRM_MODE_PROP_ATOMIC) && !atomic)
+				continue;
+			if (!drm_property_change_valid_get(prop,
+						prop_val_req, &ref)) {
+				DRM_DEBUG_KMS("change_valid_get failed\
+				 for prop id %u, prop val %llu\n",\
+				prop_id_req, prop_val_req);
+				/*copy -1 to prop_values array
+				 * to let the user know that this prop id
+				 * failed
+				 */
+				if (put_user((uint64_t) -1, prop_values + j)) {
+					DRM_DEBUG_KMS("put_user failed\
+					 for prop_val %d\n", -1);
+					return -EFAULT;
+				}
+				continue;
+			}
+			drm_modeset_lock_all(dev);
+			switch (obj->type) {
+			case DRM_MODE_OBJECT_CONNECTOR:
+				ret = drm_mode_connector_set_obj_prop(obj,
+						prop, prop_val_req);
+				break;
+			case DRM_MODE_OBJECT_CRTC:
+				ret = drm_mode_crtc_set_obj_prop(obj,
+				prop, prop_val_req);
+				break;
+			case DRM_MODE_OBJECT_PLANE:
+				ret = drm_mode_plane_set_obj_prop(
+				obj_to_plane(obj), prop, prop_val_req);
+				break;
+			default:
+				DRM_DEBUG_KMS("Invalid object type %u\n",\
+				obj->type);
+				drm_modeset_unlock_all(dev);
+				drm_property_change_valid_put(prop, ref);
+				return -EINVAL;
+			}
+			drm_modeset_unlock_all(dev);
+			drm_property_change_valid_put(prop, ref);
+			if (ret) {
+				/*Even if one prop failed to set
+				 *continue setting others , so that
+				 *max props can be set and this ioctl/feature
+				 *is utilized to max extent
+				 */
+				DRM_DEBUG_KMS("drm_mode_<connector|crtc|plane>\
+				_set_obj_prop failed for obj id %u and obj type\
+				 %u, prop id %u, prop val %llu\n", obj->id,\
+				 obj->type, prop_id_req, prop_val_req);
+				/*The properties which failed
+				 *user gets -1 in prop_values
+				 *for those prop IDs,
+				 *if ret is passed, it may coincide with
+				 *some prop val, therefore -1 is passed
+				 */
+				if (put_user((uint64_t) -1, prop_values + j)) {
+					DRM_DEBUG_KMS("put_user failed\
+					 for prop_val %d\n", -1);
+					return -EFAULT;
+				}
+			} else {
+				set++;
+			}
+		}
+	}
+	DRM_DEBUG_KMS("Total %u props set successfully\n", set);
+	*arg_count_props = set;
+	return ret;
+}
+
+/**
+ * drm_mode_obj_set_properties_ioctl - sets values of an object's multiple
+ * properties
+ * @dev: DRM device
+ * @data: ioctl data
+ * @file_priv: DRM file info
+ * This function sets the values for an object's several properties in one go.
+ * It internally utilizes the helper function set_properties
+ * Called by the user via ioctl.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_mode_obj_set_properties_ioctl(struct drm_device *dev, void *data,
+				      struct drm_file *file_priv)
+{
+	struct drm_mode_obj_set_properties *arg = data;
+	struct drm_mode_object *obj;
+	int ret = 0;
+
+	if (!drm_core_check_feature(dev, DRIVER_MODESET))
+		return -EINVAL;
+
+
+	obj = drm_mode_object_find(dev, arg->obj_id, arg->obj_type);
+	if (!obj)
+		return -ENOENT;
+	if (!obj->properties)
+		return -EINVAL;
+	ret = drm_mode_object_set_properties(dev, obj, file_priv->atomic,
+		(uint32_t __user *)(unsigned long)(arg->props_ptr),
+		(uint64_t __user *)(unsigned long)(arg->prop_values_ptr),
+			&arg->count_props);
+
+	return ret;
+}
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index b2c5284..a43838d 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -813,6 +813,7 @@ struct drm_prime_handle {
 #define DRM_IOCTL_MODE_ATOMIC		DRM_IOWR(0xBC, struct drm_mode_atomic)
 #define DRM_IOCTL_MODE_CREATEPROPBLOB	DRM_IOWR(0xBD, struct drm_mode_create_blob)
 #define DRM_IOCTL_MODE_DESTROYPROPBLOB	DRM_IOWR(0xBE, struct drm_mode_destroy_blob)
+#define DRM_IOCTL_MODE_OBJ_SETPROPERTIES DRM_IOWR(0xBF, struct drm_mode_obj_set_properties)
 
 /**
  * Device specific ioctls should only be in their respective headers
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index ce7efe2..05df931 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -362,6 +362,14 @@ struct drm_mode_obj_get_properties {
 	__u32 obj_type;
 };
 
+struct drm_mode_obj_set_properties {
+	__u64 props_ptr;
+	__u64 prop_values_ptr;
+	__u32 count_props;
+	__u32 obj_id;
+	__u32 obj_type;
+};
+
 struct drm_mode_obj_set_property {
 	__u64 value;
 	__u32 prop_id;
-- 
1.7.9.5

Re: [RFC] [GPU][DRM][PROPERTY] -Added a new ioctl in Linux DRM KMS driver.

[Daniel Stone]

Hi Satendra,

On 20 January 2017 at 08:12, Satendra Singh Thakur
<satendra.t@samsung.com> wrote:
> -Added a new ioctl in Linux DRM KMS driver.
>  This ioctl allows user to set the values of an object’s multiple
>  properties in one go.
> -In the absence of such ioctl, User would be calling one ioctl to set each
>  property value;
>  Thus, user needs to call N ioctls to set values of N properties of an
>  object,  which is a time consuming and costly because each ioctl involves
>   a system call entering/exiting kernel (context switch).
> -This ioctl will set N properties (provided that HW allows it)
>  in a single context switch
> -This ioctl can be used to set multiple properties of ether a plane
>  or a crtc or a connector (i.e. single object )
> -This ioctl can't be used to set one property of a plane and
>  one property of crtc in one go.

The atomic API already exists to set multiple properties at once, and
has the advantage of being, well, atomic. The API you propose
duplicates atomic, but it also has the possibility of only
half-succeeding, leaving the device in a strange halfway state which
is not what the user intended.

Cheers,
Daniel