[RFC 0/4] drm: add generic KMS blob properties for image enhancement

[RFC 0/4] drm: add generic KMS blob properties for image enhancement

[Rahul Sharma]

From: Rahul Sharma <Rahul.Sharma@samsung.com>

Patch set has following proposal.

1) Add writable KMS blob properties
	patch 1: drm: allow to create blank writable blob properties
	patch 2: drm: add ioctl to write into binary blob KMS properties

2) Add generic image enhancement properties. Added as per exynos hardware
	requirements (to start).
	patch 3: drm: add generic blob properties for image enhancement

3) Allow drivers to create writable blob properties
	patch 4: drm: export create and destroy function for blob properties

Initial discussion before preparing this RFC is at
http://comments.gmane.org/gmane.linux.kernel.samsung-soc/27278

This series is based on Dave's drm-next branch at
http://cgit.freedesktop.org/~airlied/linux/

Rahul Sharma (4):
  drm: allow to create blank writable blob properties
  drm: add ioctl to write into binary blob KMS properties
  drm: add generic blob properties for image enhancement
  drm: export create and destroy function for blob properties

 drivers/gpu/drm/drm_crtc.c  |  201 +++++++++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/drm_drv.c   |    1 +
 include/drm/drm_crtc.h      |   19 ++++
 include/uapi/drm/drm.h      |    1 +
 include/uapi/drm/drm_mode.h |   49 +++++++++++
 5 files changed, 265 insertions(+), 6 deletions(-)

-- 
1.7.9.5

[RFC 1/4] drm: allow to create blank writable blob properties

[Rahul Sharma]

There is no provision to create a blob property without
providing binary data. This data is needed to fill inside
the blob.

With subsequent patches, blob properties are modified to
receive well defined structures by the user application.
DRM creates a blank blob (initialized with all zeros) which
can be filled by user application through set_blob ioctl.

Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com>
---
 drivers/gpu/drm/drm_crtc.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 35ea15d..9a2215c 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3190,7 +3190,7 @@ static struct drm_property_blob *drm_property_create_blob(struct drm_device *dev
 	struct drm_property_blob *blob;
 	int ret;
 
-	if (!length || !data)
+	if (!length)
 		return NULL;
 
 	blob = kzalloc(sizeof(struct drm_property_blob)+length, GFP_KERNEL);
@@ -3205,7 +3205,8 @@ static struct drm_property_blob *drm_property_create_blob(struct drm_device *dev
 
 	blob->length = length;
 
-	memcpy(blob->data, data, length);
+	if (data)
+		memcpy(blob->data, data, length);
 
 	list_add_tail(&blob->head, &dev->mode_config.property_blob_list);
 	return blob;
-- 
1.7.9.5

[RFC 2/4] drm: add ioctl to write into binary blob KMS properties

[Rahul Sharma]

Add a new ioctl to common drm framework which can be used to
set variable length binary data from the user space. 'Blob'
is the only KMS property which can hold more than 64 bits. So
far, it has been implemented as read only property for user
application (only used for EDID data).

Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com>
---
 drivers/gpu/drm/drm_crtc.c  |   73 ++++++++++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/drm_drv.c   |    1 +
 include/drm/drm_crtc.h      |    2 ++
 include/uapi/drm/drm.h      |    1 +
 include/uapi/drm/drm_mode.h |    8 +++++
 5 files changed, 84 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 9a2215c..a2b87a5 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3300,7 +3300,6 @@ static bool drm_property_change_is_valid(struct drm_property *property,
 			valid_mask |= (1ULL << property->values[i]);
 		return !(value & ~valid_mask);
 	} else if (property->flags & DRM_MODE_PROP_BLOB) {
-		/* Only the driver knows */
 		return true;
 	} else {
 		int i;
@@ -3492,6 +3491,78 @@ out:
 	return ret;
 }
 
+int drm_mode_setblob_ioctl(struct drm_device *dev, void *data,
+			      struct drm_file *file_priv)
+{
+	struct drm_mode_object *arg_obj;
+	struct drm_mode_object *blob_obj;
+	struct drm_mode_object *prop_obj;
+	struct drm_property *property;
+	struct drm_mode_set_blob *arg = data;
+	struct drm_property_blob *blob;
+	int ret = -EINVAL, i;
+	void __user *blob_ptr;
+
+	if (!drm_core_check_feature(dev, DRIVER_MODESET))
+		return -EINVAL;
+
+	drm_modeset_lock_all(dev);
+
+	blob_obj = drm_mode_object_find(dev, arg->blob_id, DRM_MODE_OBJECT_BLOB);
+	if (!blob_obj)
+		goto done;
+
+	blob = obj_to_blob(blob_obj);
+
+	arg_obj = drm_mode_object_find(dev, arg->obj_id, arg->obj_type);
+	if (!arg_obj)
+		goto done;
+
+	if (!arg_obj->properties)
+		goto done;
+
+	for (i = 0; i < arg_obj->properties->count; i++)
+		if (arg_obj->properties->values[i] == arg->blob_id)
+			break;
+
+	if (i == arg_obj->properties->count)
+		goto done;
+
+	prop_obj = drm_mode_object_find(dev, arg_obj->properties->ids[i],
+					DRM_MODE_OBJECT_PROPERTY);
+	if (!prop_obj)
+		goto done;
+	property = obj_to_property(prop_obj);
+
+	if (!drm_property_change_is_valid(property, arg->blob_id))
+		goto done;
+
+	if (arg->length == blob->length) {
+		blob_ptr = (void __user *)(unsigned long)arg->data;
+		if (copy_from_user(blob->data, blob_ptr, blob->length)) {
+			ret = -EFAULT;
+			goto done;
+		}
+	}
+
+	switch (arg_obj->type) {
+	case DRM_MODE_OBJECT_CONNECTOR:
+		ret = drm_mode_connector_set_obj_prop(arg_obj, property,
+						      arg->blob_id);
+		break;
+	case DRM_MODE_OBJECT_CRTC:
+		ret = drm_mode_crtc_set_obj_prop(arg_obj, property, arg->blob_id);
+		break;
+	case DRM_MODE_OBJECT_PLANE:
+		ret = drm_mode_plane_set_obj_prop(arg_obj, property, arg->blob_id);
+		break;
+	}
+
+done:
+	drm_modeset_unlock_all(dev);
+	return ret;
+}
+
 int drm_mode_connector_attach_encoder(struct drm_connector *connector,
 				      struct drm_encoder *encoder)
 {
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 345be03..7cdb501 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_SETPROPBLOB, drm_mode_setblob_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 };
 
 #define DRM_CORE_IOCTL_COUNT	ARRAY_SIZE( drm_ioctls )
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index f764654..82f2016 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1124,6 +1124,8 @@ extern int drm_mode_getproperty_ioctl(struct drm_device *dev,
 				      void *data, struct drm_file *file_priv);
 extern int drm_mode_getblob_ioctl(struct drm_device *dev,
 				  void *data, struct drm_file *file_priv);
+extern int drm_mode_setblob_ioctl(struct drm_device *dev,
+				void *data, struct drm_file *file_priv);
 extern int drm_mode_connector_property_set_ioctl(struct drm_device *dev,
 					      void *data, struct drm_file *file_priv);
 extern int drm_mode_getencoder(struct drm_device *dev,
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index b06c8ed..d91139b 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -760,6 +760,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_SETPROPBLOB	DRM_IOWR(0xBC, struct drm_mode_set_blob)
 
 /**
  * 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 f104c26..1d8216d 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -295,6 +295,14 @@ struct drm_mode_get_blob {
 	__u64 data;
 };
 
+struct drm_mode_set_blob {
+	__u32 blob_id;
+	__u32 obj_id;
+	__u32 obj_type;
+	__u32 length;
+	__u64 data;
+};
+
 struct drm_mode_fb_cmd {
 	__u32 fb_id;
 	__u32 width, height;
-- 
1.7.9.5

[RFC 3/4] drm: add generic blob properties for image enhancement

[Rahul Sharma]

Add generic KMS blob properties to core drm framework. These
are writable blob properties which can be used to set Image
Enhancement parameters. The properties which are added here
are meant for color reproduction, color saturation and edge
enhancement.

Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com>
---
 drivers/gpu/drm/drm_crtc.c  |  115 +++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_crtc.h      |   13 +++++
 include/uapi/drm/drm_mode.h |   41 +++++++++++++++
 3 files changed, 169 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index a2b87a5..8771abf 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1260,6 +1260,121 @@ int drm_mode_create_dirty_info_property(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_mode_create_dirty_info_property);
 
+/**
+ * drm_mode_create_color_saturation_property - create property for color saturation
+ * @dev: DRM device
+ *
+ */
+int drm_mode_create_color_saturation_property(
+			struct drm_device *dev)
+{
+	struct drm_mode_color_saturation *params;
+	struct drm_property *prop;
+	char prop_name[] = "color saturation";
+
+	if (dev->mode_config.color_saturation_property ||
+		dev->mode_config.color_saturation_blob_ptr)
+		return -EEXIST;
+
+	prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
+			prop_name, 0);
+	if (!prop) {
+		DRM_ERROR("fail to create %s property.\n", prop_name);
+		return -ENOMEM;
+	}
+
+	dev->mode_config.color_saturation_blob_ptr =
+		drm_property_create_blob(dev, sizeof(*params),
+			NULL);
+	if (!dev->mode_config.color_saturation_blob_ptr) {
+		DRM_ERROR("failed to allocate blob for %s.\n", prop_name);
+		drm_property_destroy(dev, prop);
+		return -ENOMEM;
+	}
+
+	dev->mode_config.color_saturation_property = prop;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_mode_create_color_saturation_property);
+
+/**
+ * drm_mode_create_color_reproduction_property - create property for color reproduction
+ * @dev: DRM device
+ *
+ */
+int drm_mode_create_color_reproduction_property(
+			struct drm_device *dev)
+{
+	struct drm_mode_color_reproduction *params;
+	struct drm_property *prop;
+	char prop_name[] = "color reproduction";
+
+	if (dev->mode_config.color_reproduction_property ||
+		dev->mode_config.color_reproduction_blob_ptr)
+		return -EEXIST;
+
+	prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
+			prop_name, 0);
+	if (!prop) {
+		DRM_ERROR("fail to create %s property.\n", prop_name);
+		return -ENOMEM;
+	}
+
+	dev->mode_config.color_reproduction_blob_ptr =
+		drm_property_create_blob(dev, sizeof(*params),
+			NULL);
+	if (!dev->mode_config.color_reproduction_blob_ptr) {
+		DRM_ERROR("failed to allocate blob for %s\n", prop_name);
+		drm_property_destroy(dev, prop);
+		return -ENOMEM;
+	}
+
+	dev->mode_config.color_reproduction_property = prop;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_mode_create_color_reproduction_property);
+
+
+/**
+ * drm_mode_create_edge_enhancement_property - create property for edge enhancement
+ * @dev: DRM device
+ *
+ */
+int drm_mode_create_edge_enhancement_property(
+			struct drm_device *dev)
+{
+	struct drm_mode_edge_enhancement *params;
+	struct drm_property *prop;
+	char prop_name[] = "edge enhancement";
+
+	if (dev->mode_config.edge_enhancement_property ||
+		dev->mode_config.edge_enhancement_blob_ptr)
+		return -EEXIST;
+
+	prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
+			prop_name, 0);
+	if (!prop) {
+		DRM_ERROR("fail to create %s property.\n", prop_name);
+		return -ENOMEM;
+	}
+
+	dev->mode_config.edge_enhancement_blob_ptr =
+		drm_property_create_blob(dev, sizeof(*params),
+			NULL);
+	if (!dev->mode_config.edge_enhancement_blob_ptr) {
+		DRM_ERROR("failed to allocate blob for %s\n", prop_name);
+		drm_property_destroy(dev, prop);
+		return -ENOMEM;
+	}
+
+	dev->mode_config.edge_enhancement_property = prop;
+
+	return  0;
+}
+EXPORT_SYMBOL(drm_mode_create_edge_enhancement_property);
+
 static int drm_mode_group_init(struct drm_device *dev, struct drm_mode_group *group)
 {
 	uint32_t total_objects = 0;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 82f2016..df7b178 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -901,6 +901,13 @@ struct drm_mode_config {
 	/* Optional properties */
 	struct drm_property *scaling_mode_property;
 	struct drm_property *dirty_info_property;
+	struct drm_property *color_saturation_property;
+	struct drm_property *color_reproduction_property;
+	struct drm_property *edge_enhancement_property;
+
+	struct drm_property_blob *color_saturation_blob_ptr;
+	struct drm_property_blob *color_reproduction_blob_ptr;
+	struct drm_property_blob *edge_enhancement_blob_ptr;
 
 	/* dumb ioctl parameters */
 	uint32_t preferred_depth, prefer_shadow;
@@ -1079,6 +1086,12 @@ extern int drm_mode_create_tv_properties(struct drm_device *dev, int num_formats
 extern int drm_mode_create_scaling_mode_property(struct drm_device *dev);
 extern int drm_mode_create_dirty_info_property(struct drm_device *dev);
 extern const char *drm_get_encoder_name(const struct drm_encoder *encoder);
+extern int drm_mode_create_color_saturation_property(
+		struct drm_device *dev);
+extern int drm_mode_create_color_reproduction_property(
+		struct drm_device *dev);
+extern int drm_mode_create_edge_enhancement_property(
+		struct drm_device *dev);
 
 extern int drm_mode_connector_attach_encoder(struct drm_connector *connector,
 					     struct drm_encoder *encoder);
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 1d8216d..d28f82d 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -504,4 +504,45 @@ struct drm_mode_destroy_dumb {
 	uint32_t handle;
 };
 
+/* set up parameters for finer color saturation */
+struct drm_mode_color_saturation {
+	/* hue gain for individual colors */
+	uint16_t hue_gain_red;
+	uint16_t hue_gain_green;
+	uint16_t hue_gain_blue;
+	uint16_t hue_gain_cyan;
+	uint16_t hue_gain_magenta;
+	uint16_t hue_gain_yellow;
+	/* hue gain for overall display */
+	uint16_t hue_gain_overall;
+};
+
+/* set up parameters for standard color reproduction */
+struct drm_mode_color_reproduction {
+	/* 16 bit rgb value for primary colors */
+	uint16_t red_rgb[3];
+	uint16_t green_rgb[3];
+	uint16_t blue_rgb[3];
+	uint16_t cyan_rgb[3];
+	uint16_t magenta_rgb[3];
+	uint16_t yellow_rgb[3];
+	uint16_t white_rgb[3];
+	uint16_t black_rgb[3];
+};
+
+/* set up parameters for edge enhancement */
+struct drm_mode_edge_enhancement {
+	/* threshold values for edge and background*/
+	uint16_t edge_th;
+	uint16_t background_th;
+	/* postive gain */
+	uint16_t pg_edge;
+	uint16_t pg_flat;
+	uint16_t pg_background;
+	/* negative gain */
+	uint16_t ng_edge;
+	uint16_t ng_flat;
+	uint16_t ng_background;
+};
+
 #endif
-- 
1.7.9.5

[RFC 4/4] drm: export create and destroy function for blob properties

[Rahul Sharma]

Drm drivers can also create and attach private blob properties.
This patch exports functions to create and destroy blob properties.

Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com>
---
 drivers/gpu/drm/drm_crtc.c |    8 +++++---
 include/drm/drm_crtc.h     |    4 ++++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 8771abf..f1939c2 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3299,8 +3299,8 @@ done:
 	return ret;
 }
 
-static struct drm_property_blob *drm_property_create_blob(struct drm_device *dev, int length,
-							  void *data)
+struct drm_property_blob *drm_property_create_blob(struct drm_device *dev,
+			int length, void *data)
 {
 	struct drm_property_blob *blob;
 	int ret;
@@ -3326,14 +3326,16 @@ static struct drm_property_blob *drm_property_create_blob(struct drm_device *dev
 	list_add_tail(&blob->head, &dev->mode_config.property_blob_list);
 	return blob;
 }
+EXPORT_SYMBOL(drm_property_create_blob);
 
-static void drm_property_destroy_blob(struct drm_device *dev,
+void drm_property_destroy_blob(struct drm_device *dev,
 			       struct drm_property_blob *blob)
 {
 	drm_mode_object_put(dev, &blob->base);
 	list_del(&blob->head);
 	kfree(blob);
 }
+EXPORT_SYMBOL(drm_property_destroy_blob);
 
 int drm_mode_getblob_ioctl(struct drm_device *dev,
 			   void *data, struct drm_file *file_priv)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index df7b178..8bf7fb2 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1077,6 +1077,10 @@ struct drm_property *drm_property_create_bitmask(struct drm_device *dev,
 struct drm_property *drm_property_create_range(struct drm_device *dev, int flags,
 					 const char *name,
 					 uint64_t min, uint64_t max);
+extern struct drm_property_blob *drm_property_create_blob(struct drm_device *dev,
+					int length, void *data);
+extern void drm_property_destroy_blob(struct drm_device *dev,
+					struct drm_property_blob *blob);
 extern void drm_property_destroy(struct drm_device *dev, struct drm_property *property);
 extern int drm_property_add_enum(struct drm_property *property, int index,
 				 uint64_t value, const char *name);
-- 
1.7.9.5

[RFC 3/4] drm: add generic blob properties for image enhancement

[Rahul Sharma]

Add generic KMS blob properties to core drm framework. These
are writable blob properties which can be used to set Image
Enhancement parameters. The properties which are added here
are meant for color reproduction, color saturation and edge
enhancement.

Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com>
---
 drivers/gpu/drm/drm_crtc.c  |  115 +++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_crtc.h      |   13 +++++
 include/uapi/drm/drm_mode.h |   41 +++++++++++++++
 3 files changed, 169 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index a2b87a5..8771abf 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1260,6 +1260,121 @@ int drm_mode_create_dirty_info_property(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_mode_create_dirty_info_property);
 
+/**
+ * drm_mode_create_color_saturation_property - create property for color saturation
+ * @dev: DRM device
+ *
+ */
+int drm_mode_create_color_saturation_property(
+			struct drm_device *dev)
+{
+	struct drm_mode_color_saturation *params;
+	struct drm_property *prop;
+	char prop_name[] = "color saturation";
+
+	if (dev->mode_config.color_saturation_property ||
+		dev->mode_config.color_saturation_blob_ptr)
+		return -EEXIST;
+
+	prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
+			prop_name, 0);
+	if (!prop) {
+		DRM_ERROR("fail to create %s property.\n", prop_name);
+		return -ENOMEM;
+	}
+
+	dev->mode_config.color_saturation_blob_ptr =
+		drm_property_create_blob(dev, sizeof(*params),
+			NULL);
+	if (!dev->mode_config.color_saturation_blob_ptr) {
+		DRM_ERROR("failed to allocate blob for %s.\n", prop_name);
+		drm_property_destroy(dev, prop);
+		return -ENOMEM;
+	}
+
+	dev->mode_config.color_saturation_property = prop;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_mode_create_color_saturation_property);
+
+/**
+ * drm_mode_create_color_reproduction_property - create property for color reproduction
+ * @dev: DRM device
+ *
+ */
+int drm_mode_create_color_reproduction_property(
+			struct drm_device *dev)
+{
+	struct drm_mode_color_reproduction *params;
+	struct drm_property *prop;
+	char prop_name[] = "color reproduction";
+
+	if (dev->mode_config.color_reproduction_property ||
+		dev->mode_config.color_reproduction_blob_ptr)
+		return -EEXIST;
+
+	prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
+			prop_name, 0);
+	if (!prop) {
+		DRM_ERROR("fail to create %s property.\n", prop_name);
+		return -ENOMEM;
+	}
+
+	dev->mode_config.color_reproduction_blob_ptr =
+		drm_property_create_blob(dev, sizeof(*params),
+			NULL);
+	if (!dev->mode_config.color_reproduction_blob_ptr) {
+		DRM_ERROR("failed to allocate blob for %s\n", prop_name);
+		drm_property_destroy(dev, prop);
+		return -ENOMEM;
+	}
+
+	dev->mode_config.color_reproduction_property = prop;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_mode_create_color_reproduction_property);
+
+
+/**
+ * drm_mode_create_edge_enhancement_property - create property for edge enhancement
+ * @dev: DRM device
+ *
+ */
+int drm_mode_create_edge_enhancement_property(
+			struct drm_device *dev)
+{
+	struct drm_mode_edge_enhancement *params;
+	struct drm_property *prop;
+	char prop_name[] = "edge enhancement";
+
+	if (dev->mode_config.edge_enhancement_property ||
+		dev->mode_config.edge_enhancement_blob_ptr)
+		return -EEXIST;
+
+	prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
+			prop_name, 0);
+	if (!prop) {
+		DRM_ERROR("fail to create %s property.\n", prop_name);
+		return -ENOMEM;
+	}
+
+	dev->mode_config.edge_enhancement_blob_ptr =
+		drm_property_create_blob(dev, sizeof(*params),
+			NULL);
+	if (!dev->mode_config.edge_enhancement_blob_ptr) {
+		DRM_ERROR("failed to allocate blob for %s\n", prop_name);
+		drm_property_destroy(dev, prop);
+		return -ENOMEM;
+	}
+
+	dev->mode_config.edge_enhancement_property = prop;
+
+	return  0;
+}
+EXPORT_SYMBOL(drm_mode_create_edge_enhancement_property);
+
 static int drm_mode_group_init(struct drm_device *dev, struct drm_mode_group *group)
 {
 	uint32_t total_objects = 0;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 82f2016..df7b178 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -901,6 +901,13 @@ struct drm_mode_config {
 	/* Optional properties */
 	struct drm_property *scaling_mode_property;
 	struct drm_property *dirty_info_property;
+	struct drm_property *color_saturation_property;
+	struct drm_property *color_reproduction_property;
+	struct drm_property *edge_enhancement_property;
+
+	struct drm_property_blob *color_saturation_blob_ptr;
+	struct drm_property_blob *color_reproduction_blob_ptr;
+	struct drm_property_blob *edge_enhancement_blob_ptr;
 
 	/* dumb ioctl parameters */
 	uint32_t preferred_depth, prefer_shadow;
@@ -1079,6 +1086,12 @@ extern int drm_mode_create_tv_properties(struct drm_device *dev, int num_formats
 extern int drm_mode_create_scaling_mode_property(struct drm_device *dev);
 extern int drm_mode_create_dirty_info_property(struct drm_device *dev);
 extern const char *drm_get_encoder_name(const struct drm_encoder *encoder);
+extern int drm_mode_create_color_saturation_property(
+		struct drm_device *dev);
+extern int drm_mode_create_color_reproduction_property(
+		struct drm_device *dev);
+extern int drm_mode_create_edge_enhancement_property(
+		struct drm_device *dev);
 
 extern int drm_mode_connector_attach_encoder(struct drm_connector *connector,
 					     struct drm_encoder *encoder);
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 1d8216d..d28f82d 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -504,4 +504,45 @@ struct drm_mode_destroy_dumb {
 	uint32_t handle;
 };
 
+/* set up parameters for finer color saturation */
+struct drm_mode_color_saturation {
+	/* hue gain for individual colors */
+	uint16_t hue_gain_red;
+	uint16_t hue_gain_green;
+	uint16_t hue_gain_blue;
+	uint16_t hue_gain_cyan;
+	uint16_t hue_gain_magenta;
+	uint16_t hue_gain_yellow;
+	/* hue gain for overall display */
+	uint16_t hue_gain_overall;
+};
+
+/* set up parameters for standard color reproduction */
+struct drm_mode_color_reproduction {
+	/* 16 bit rgb value for primary colors */
+	uint16_t red_rgb[3];
+	uint16_t green_rgb[3];
+	uint16_t blue_rgb[3];
+	uint16_t cyan_rgb[3];
+	uint16_t magenta_rgb[3];
+	uint16_t yellow_rgb[3];
+	uint16_t white_rgb[3];
+	uint16_t black_rgb[3];
+};
+
+/* set up parameters for edge enhancement */
+struct drm_mode_edge_enhancement {
+	/* threshold values for edge and background*/
+	uint16_t edge_th;
+	uint16_t background_th;
+	/* postive gain */
+	uint16_t pg_edge;
+	uint16_t pg_flat;
+	uint16_t pg_background;
+	/* negative gain */
+	uint16_t ng_edge;
+	uint16_t ng_flat;
+	uint16_t ng_background;
+};
+
 #endif
-- 
1.7.9.5

Re: [RFC 3/4] drm: add generic blob properties for image enhancement

[Daniel Vetter]

On Thu, Mar 06, 2014 at 11:42:13AM +0530, Rahul Sharma wrote:
> Add generic KMS blob properties to core drm framework. These
> are writable blob properties which can be used to set Image
> Enhancement parameters. The properties which are added here
> are meant for color reproduction, color saturation and edge
> enhancement.
> 
> Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com>
> ---
>  drivers/gpu/drm/drm_crtc.c  |  115 +++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_crtc.h      |   13 +++++
>  include/uapi/drm/drm_mode.h |   41 +++++++++++++++
>  3 files changed, 169 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index a2b87a5..8771abf 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1260,6 +1260,121 @@ int drm_mode_create_dirty_info_property(struct drm_device *dev)
>  }
>  EXPORT_SYMBOL(drm_mode_create_dirty_info_property);
>  
> +/**
> + * drm_mode_create_color_saturation_property - create property for color saturation
> + * @dev: DRM device
> + *
> + */
> +int drm_mode_create_color_saturation_property(
> +			struct drm_device *dev)
> +{
> +	struct drm_mode_color_saturation *params;
> +	struct drm_property *prop;
> +	char prop_name[] = "color saturation";
> +
> +	if (dev->mode_config.color_saturation_property ||
> +		dev->mode_config.color_saturation_blob_ptr)
> +		return -EEXIST;
> +
> +	prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> +			prop_name, 0);
> +	if (!prop) {
> +		DRM_ERROR("fail to create %s property.\n", prop_name);
> +		return -ENOMEM;
> +	}
> +
> +	dev->mode_config.color_saturation_blob_ptr =
> +		drm_property_create_blob(dev, sizeof(*params),
> +			NULL);
> +	if (!dev->mode_config.color_saturation_blob_ptr) {
> +		DRM_ERROR("failed to allocate blob for %s.\n", prop_name);
> +		drm_property_destroy(dev, prop);
> +		return -ENOMEM;
> +	}
> +
> +	dev->mode_config.color_saturation_property = prop;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_mode_create_color_saturation_property);
> +
> +/**
> + * drm_mode_create_color_reproduction_property - create property for color reproduction
> + * @dev: DRM device
> + *
> + */
> +int drm_mode_create_color_reproduction_property(
> +			struct drm_device *dev)
> +{
> +	struct drm_mode_color_reproduction *params;
> +	struct drm_property *prop;
> +	char prop_name[] = "color reproduction";
> +
> +	if (dev->mode_config.color_reproduction_property ||
> +		dev->mode_config.color_reproduction_blob_ptr)
> +		return -EEXIST;
> +
> +	prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> +			prop_name, 0);
> +	if (!prop) {
> +		DRM_ERROR("fail to create %s property.\n", prop_name);
> +		return -ENOMEM;
> +	}
> +
> +	dev->mode_config.color_reproduction_blob_ptr =
> +		drm_property_create_blob(dev, sizeof(*params),
> +			NULL);
> +	if (!dev->mode_config.color_reproduction_blob_ptr) {
> +		DRM_ERROR("failed to allocate blob for %s\n", prop_name);
> +		drm_property_destroy(dev, prop);
> +		return -ENOMEM;
> +	}
> +
> +	dev->mode_config.color_reproduction_property = prop;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_mode_create_color_reproduction_property);
> +
> +
> +/**
> + * drm_mode_create_edge_enhancement_property - create property for edge enhancement
> + * @dev: DRM device
> + *
> + */
> +int drm_mode_create_edge_enhancement_property(
> +			struct drm_device *dev)
> +{
> +	struct drm_mode_edge_enhancement *params;
> +	struct drm_property *prop;
> +	char prop_name[] = "edge enhancement";
> +
> +	if (dev->mode_config.edge_enhancement_property ||
> +		dev->mode_config.edge_enhancement_blob_ptr)
> +		return -EEXIST;
> +
> +	prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> +			prop_name, 0);
> +	if (!prop) {
> +		DRM_ERROR("fail to create %s property.\n", prop_name);
> +		return -ENOMEM;
> +	}
> +
> +	dev->mode_config.edge_enhancement_blob_ptr =
> +		drm_property_create_blob(dev, sizeof(*params),
> +			NULL);
> +	if (!dev->mode_config.edge_enhancement_blob_ptr) {
> +		DRM_ERROR("failed to allocate blob for %s\n", prop_name);
> +		drm_property_destroy(dev, prop);
> +		return -ENOMEM;
> +	}
> +
> +	dev->mode_config.edge_enhancement_property = prop;
> +
> +	return  0;
> +}
> +EXPORT_SYMBOL(drm_mode_create_edge_enhancement_property);
> +
>  static int drm_mode_group_init(struct drm_device *dev, struct drm_mode_group *group)
>  {
>  	uint32_t total_objects = 0;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 82f2016..df7b178 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -901,6 +901,13 @@ struct drm_mode_config {
>  	/* Optional properties */
>  	struct drm_property *scaling_mode_property;
>  	struct drm_property *dirty_info_property;
> +	struct drm_property *color_saturation_property;
> +	struct drm_property *color_reproduction_property;
> +	struct drm_property *edge_enhancement_property;
> +
> +	struct drm_property_blob *color_saturation_blob_ptr;
> +	struct drm_property_blob *color_reproduction_blob_ptr;
> +	struct drm_property_blob *edge_enhancement_blob_ptr;
>  
>  	/* dumb ioctl parameters */
>  	uint32_t preferred_depth, prefer_shadow;
> @@ -1079,6 +1086,12 @@ extern int drm_mode_create_tv_properties(struct drm_device *dev, int num_formats
>  extern int drm_mode_create_scaling_mode_property(struct drm_device *dev);
>  extern int drm_mode_create_dirty_info_property(struct drm_device *dev);
>  extern const char *drm_get_encoder_name(const struct drm_encoder *encoder);
> +extern int drm_mode_create_color_saturation_property(
> +		struct drm_device *dev);
> +extern int drm_mode_create_color_reproduction_property(
> +		struct drm_device *dev);
> +extern int drm_mode_create_edge_enhancement_property(
> +		struct drm_device *dev);
>  
>  extern int drm_mode_connector_attach_encoder(struct drm_connector *connector,
>  					     struct drm_encoder *encoder);
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 1d8216d..d28f82d 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -504,4 +504,45 @@ struct drm_mode_destroy_dumb {
>  	uint32_t handle;
>  };
>  
> +/* set up parameters for finer color saturation */
> +struct drm_mode_color_saturation {
> +	/* hue gain for individual colors */
> +	uint16_t hue_gain_red;
> +	uint16_t hue_gain_green;
> +	uint16_t hue_gain_blue;
> +	uint16_t hue_gain_cyan;
> +	uint16_t hue_gain_magenta;
> +	uint16_t hue_gain_yellow;
> +	/* hue gain for overall display */
> +	uint16_t hue_gain_overall;
> +};
> +
> +/* set up parameters for standard color reproduction */
> +struct drm_mode_color_reproduction {
> +	/* 16 bit rgb value for primary colors */
> +	uint16_t red_rgb[3];
> +	uint16_t green_rgb[3];
> +	uint16_t blue_rgb[3];
> +	uint16_t cyan_rgb[3];
> +	uint16_t magenta_rgb[3];
> +	uint16_t yellow_rgb[3];
> +	uint16_t white_rgb[3];
> +	uint16_t black_rgb[3];
> +};
> +
> +/* set up parameters for edge enhancement */
> +struct drm_mode_edge_enhancement {
> +	/* threshold values for edge and background*/
> +	uint16_t edge_th;
> +	uint16_t background_th;
> +	/* postive gain */
> +	uint16_t pg_edge;
> +	uint16_t pg_flat;
> +	uint16_t pg_background;
> +	/* negative gain */
> +	uint16_t ng_edge;
> +	uint16_t ng_flat;
> +	uint16_t ng_background;
> +};

Tbh I don't really understand why we can't use normal properties for this.
We might want to add a new RBG type of property (or YUV fwiw, both with
and without alpha) to make this standardized (maybe with some fixed point
value for non-normalizes).

If the only reason you group them is to atomically commit them, then the
only thing we need is the atomic ioctl, which can shovel entire groups of
properties over the wire at once.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC 3/4] drm: add generic blob properties for image enhancement

[Rahul Sharma]

Hi Daniel,

On 7 March 2014 14:06, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Mar 06, 2014 at 11:42:13AM +0530, Rahul Sharma wrote:
>> Add generic KMS blob properties to core drm framework. These
>> are writable blob properties which can be used to set Image
>> Enhancement parameters. The properties which are added here
>> are meant for color reproduction, color saturation and edge
>> enhancement.
>>
>> Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com>
>> ---
>>  drivers/gpu/drm/drm_crtc.c  |  115 +++++++++++++++++++++++++++++++++++++++++++
>>  include/drm/drm_crtc.h      |   13 +++++
>>  include/uapi/drm/drm_mode.h |   41 +++++++++++++++
>>  3 files changed, 169 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index a2b87a5..8771abf 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -1260,6 +1260,121 @@ int drm_mode_create_dirty_info_property(struct drm_device *dev)
>>  }
>>  EXPORT_SYMBOL(drm_mode_create_dirty_info_property);
>>
>> +/**
>> + * drm_mode_create_color_saturation_property - create property for color saturation
>> + * @dev: DRM device
>> + *
>> + */
>> +int drm_mode_create_color_saturation_property(
>> +                     struct drm_device *dev)
>> +{
>> +     struct drm_mode_color_saturation *params;
>> +     struct drm_property *prop;
>> +     char prop_name[] = "color saturation";
>> +
>> +     if (dev->mode_config.color_saturation_property ||
>> +             dev->mode_config.color_saturation_blob_ptr)
>> +             return -EEXIST;
>> +
>> +     prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
>> +                     prop_name, 0);
>> +     if (!prop) {
>> +             DRM_ERROR("fail to create %s property.\n", prop_name);
>> +             return -ENOMEM;
>> +     }
>> +
>> +     dev->mode_config.color_saturation_blob_ptr =
>> +             drm_property_create_blob(dev, sizeof(*params),
>> +                     NULL);
>> +     if (!dev->mode_config.color_saturation_blob_ptr) {
>> +             DRM_ERROR("failed to allocate blob for %s.\n", prop_name);
>> +             drm_property_destroy(dev, prop);
>> +             return -ENOMEM;
>> +     }
>> +
>> +     dev->mode_config.color_saturation_property = prop;
>> +
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL(drm_mode_create_color_saturation_property);
>> +
>> +/**
>> + * drm_mode_create_color_reproduction_property - create property for color reproduction
>> + * @dev: DRM device
>> + *
>> + */
>> +int drm_mode_create_color_reproduction_property(
>> +                     struct drm_device *dev)
>> +{
>> +     struct drm_mode_color_reproduction *params;
>> +     struct drm_property *prop;
>> +     char prop_name[] = "color reproduction";
>> +
>> +     if (dev->mode_config.color_reproduction_property ||
>> +             dev->mode_config.color_reproduction_blob_ptr)
>> +             return -EEXIST;
>> +
>> +     prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
>> +                     prop_name, 0);
>> +     if (!prop) {
>> +             DRM_ERROR("fail to create %s property.\n", prop_name);
>> +             return -ENOMEM;
>> +     }
>> +
>> +     dev->mode_config.color_reproduction_blob_ptr =
>> +             drm_property_create_blob(dev, sizeof(*params),
>> +                     NULL);
>> +     if (!dev->mode_config.color_reproduction_blob_ptr) {
>> +             DRM_ERROR("failed to allocate blob for %s\n", prop_name);
>> +             drm_property_destroy(dev, prop);
>> +             return -ENOMEM;
>> +     }
>> +
>> +     dev->mode_config.color_reproduction_property = prop;
>> +
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL(drm_mode_create_color_reproduction_property);
>> +
>> +
>> +/**
>> + * drm_mode_create_edge_enhancement_property - create property for edge enhancement
>> + * @dev: DRM device
>> + *
>> + */
>> +int drm_mode_create_edge_enhancement_property(
>> +                     struct drm_device *dev)
>> +{
>> +     struct drm_mode_edge_enhancement *params;
>> +     struct drm_property *prop;
>> +     char prop_name[] = "edge enhancement";
>> +
>> +     if (dev->mode_config.edge_enhancement_property ||
>> +             dev->mode_config.edge_enhancement_blob_ptr)
>> +             return -EEXIST;
>> +
>> +     prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
>> +                     prop_name, 0);
>> +     if (!prop) {
>> +             DRM_ERROR("fail to create %s property.\n", prop_name);
>> +             return -ENOMEM;
>> +     }
>> +
>> +     dev->mode_config.edge_enhancement_blob_ptr =
>> +             drm_property_create_blob(dev, sizeof(*params),
>> +                     NULL);
>> +     if (!dev->mode_config.edge_enhancement_blob_ptr) {
>> +             DRM_ERROR("failed to allocate blob for %s\n", prop_name);
>> +             drm_property_destroy(dev, prop);
>> +             return -ENOMEM;
>> +     }
>> +
>> +     dev->mode_config.edge_enhancement_property = prop;
>> +
>> +     return  0;
>> +}
>> +EXPORT_SYMBOL(drm_mode_create_edge_enhancement_property);
>> +
>>  static int drm_mode_group_init(struct drm_device *dev, struct drm_mode_group *group)
>>  {
>>       uint32_t total_objects = 0;
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index 82f2016..df7b178 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -901,6 +901,13 @@ struct drm_mode_config {
>>       /* Optional properties */
>>       struct drm_property *scaling_mode_property;
>>       struct drm_property *dirty_info_property;
>> +     struct drm_property *color_saturation_property;
>> +     struct drm_property *color_reproduction_property;
>> +     struct drm_property *edge_enhancement_property;
>> +
>> +     struct drm_property_blob *color_saturation_blob_ptr;
>> +     struct drm_property_blob *color_reproduction_blob_ptr;
>> +     struct drm_property_blob *edge_enhancement_blob_ptr;
>>
>>       /* dumb ioctl parameters */
>>       uint32_t preferred_depth, prefer_shadow;
>> @@ -1079,6 +1086,12 @@ extern int drm_mode_create_tv_properties(struct drm_device *dev, int num_formats
>>  extern int drm_mode_create_scaling_mode_property(struct drm_device *dev);
>>  extern int drm_mode_create_dirty_info_property(struct drm_device *dev);
>>  extern const char *drm_get_encoder_name(const struct drm_encoder *encoder);
>> +extern int drm_mode_create_color_saturation_property(
>> +             struct drm_device *dev);
>> +extern int drm_mode_create_color_reproduction_property(
>> +             struct drm_device *dev);
>> +extern int drm_mode_create_edge_enhancement_property(
>> +             struct drm_device *dev);
>>
>>  extern int drm_mode_connector_attach_encoder(struct drm_connector *connector,
>>                                            struct drm_encoder *encoder);
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index 1d8216d..d28f82d 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -504,4 +504,45 @@ struct drm_mode_destroy_dumb {
>>       uint32_t handle;
>>  };
>>
>> +/* set up parameters for finer color saturation */
>> +struct drm_mode_color_saturation {
>> +     /* hue gain for individual colors */
>> +     uint16_t hue_gain_red;
>> +     uint16_t hue_gain_green;
>> +     uint16_t hue_gain_blue;
>> +     uint16_t hue_gain_cyan;
>> +     uint16_t hue_gain_magenta;
>> +     uint16_t hue_gain_yellow;
>> +     /* hue gain for overall display */
>> +     uint16_t hue_gain_overall;
>> +};
>> +
>> +/* set up parameters for standard color reproduction */
>> +struct drm_mode_color_reproduction {
>> +     /* 16 bit rgb value for primary colors */
>> +     uint16_t red_rgb[3];
>> +     uint16_t green_rgb[3];
>> +     uint16_t blue_rgb[3];
>> +     uint16_t cyan_rgb[3];
>> +     uint16_t magenta_rgb[3];
>> +     uint16_t yellow_rgb[3];
>> +     uint16_t white_rgb[3];
>> +     uint16_t black_rgb[3];
>> +};
>> +
>> +/* set up parameters for edge enhancement */
>> +struct drm_mode_edge_enhancement {
>> +     /* threshold values for edge and background*/
>> +     uint16_t edge_th;
>> +     uint16_t background_th;
>> +     /* postive gain */
>> +     uint16_t pg_edge;
>> +     uint16_t pg_flat;
>> +     uint16_t pg_background;
>> +     /* negative gain */
>> +     uint16_t ng_edge;
>> +     uint16_t ng_flat;
>> +     uint16_t ng_background;
>> +};
>
> Tbh I don't really understand why we can't use normal properties for this.
> We might want to add a new RBG type of property (or YUV fwiw, both with
> and without alpha) to make this standardized (maybe with some fixed point
> value for non-normalizes).

I dropped Normal properties (the ones which accepts 64 bit data)
because there will be too many of them. As you can see the
count is going upto 24 parameters, means 24 such properties, as
each can carry one parameter. This is too much of overhead.
Please correct me if you mean something different.

I am not sure what are RGB type properties? I know only about 64-bit
normal properties which are too compact to carry above structures. Are
you talking about set_gamma type of ioctls. Each "set_gamma" type
ioctl needs a addition in callbacks till down the layer which looks bit
unnecessary, while writable blob properties are using same set_property
and get_property infrastructure.

>
> If the only reason you group them is to atomically commit them, then the
> only thing we need is the atomic ioctl, which can shovel entire groups of
> properties over the wire at once.

Atomic commit is not an explicit requirement. But splitting them to
many small pieces (in case of normal properties) are resulting into
many user-kernel switching overhead, which seems avoidable.

IF we have a reason to keep blobs Readable, I can work on alternate
methods like 3 new ioclts for saturation, reproduction and edge
enhancement. But as if now writable blob properties seems a better
method to me.

Regards,
Rahul Sharma.

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 3/4] drm: add generic blob properties for image enhancement

[Matt Roper]

On Fri, Mar 07, 2014 at 03:50:54PM +0530, Rahul Sharma wrote:
> Hi Daniel,
> 
> On 7 March 2014 14:06, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Thu, Mar 06, 2014 at 11:42:13AM +0530, Rahul Sharma wrote:
> >> Add generic KMS blob properties to core drm framework. These
> >> are writable blob properties which can be used to set Image
> >> Enhancement parameters. The properties which are added here
> >> are meant for color reproduction, color saturation and edge
> >> enhancement.
> >>
> >> Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com>
...
> > Tbh I don't really understand why we can't use normal properties for this.
> > We might want to add a new RBG type of property (or YUV fwiw, both with
> > and without alpha) to make this standardized (maybe with some fixed point
> > value for non-normalizes).
> 
> I dropped Normal properties (the ones which accepts 64 bit data)
> because there will be too many of them. As you can see the
> count is going upto 24 parameters, means 24 such properties, as
> each can carry one parameter. This is too much of overhead.
> Please correct me if you mean something different.
> 
> I am not sure what are RGB type properties? I know only about 64-bit
> normal properties which are too compact to carry above structures. Are
> you talking about set_gamma type of ioctls. Each "set_gamma" type
> ioctl needs a addition in callbacks till down the layer which looks bit
> unnecessary, while writable blob properties are using same set_property
> and get_property infrastructure.
> 
> >
> > If the only reason you group them is to atomically commit them, then the
> > only thing we need is the atomic ioctl, which can shovel entire groups of
> > properties over the wire at once.
> 
> Atomic commit is not an explicit requirement. But splitting them to
> many small pieces (in case of normal properties) are resulting into
> many user-kernel switching overhead, which seems avoidable.

The idea with atomic modeset / nuclear pageflip is that you collect a
whole bunch of individual property updates into a "property set"
container in userspace (libdrm).  When you're done setting all the
values you want and "commit" the property set, all of the values that
you collected get passed to the kernel in one go via a new ioctl (and
are then updated in an atomic manner by the DRM driver).  So performing
your 24 different property updates via the upcoming atomic API's
shouldn't be a problem and shouldn't add any unnecessary overhead.

The kernel-side of atomic modeset / nuclear pageflip is still a WIP, so
it isn't upstream yet, but you can see the userspace-facing API in Rob
Clark's repo here:

        https://github.com/robclark/libdrm/blob/atomic/xf86drmMode.h

Note the drmModePropertySet{Alloc,Add,Commit,Free}() functions near the
bottom.


Matt

-- 
Matt Roper
Graphics Software Engineer
ISG Platform Enabling & Development
Intel Corporation
(916) 356-2795

Re: [RFC 3/4] drm: add generic blob properties for image enhancement

[Rahul Sharma]

On 8 March 2014 06:16, Matt Roper <matthew.d.roper@intel.com> wrote:
> On Fri, Mar 07, 2014 at 03:50:54PM +0530, Rahul Sharma wrote:
>> Hi Daniel,
>>
>> On 7 March 2014 14:06, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > On Thu, Mar 06, 2014 at 11:42:13AM +0530, Rahul Sharma wrote:
>> >> Add generic KMS blob properties to core drm framework. These
>> >> are writable blob properties which can be used to set Image
>> >> Enhancement parameters. The properties which are added here
>> >> are meant for color reproduction, color saturation and edge
>> >> enhancement.
>> >>
>> >> Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com>
> ...
>> > Tbh I don't really understand why we can't use normal properties for this.
>> > We might want to add a new RBG type of property (or YUV fwiw, both with
>> > and without alpha) to make this standardized (maybe with some fixed point
>> > value for non-normalizes).
>>
>> I dropped Normal properties (the ones which accepts 64 bit data)
>> because there will be too many of them. As you can see the
>> count is going upto 24 parameters, means 24 such properties, as
>> each can carry one parameter. This is too much of overhead.
>> Please correct me if you mean something different.
>>
>> I am not sure what are RGB type properties? I know only about 64-bit
>> normal properties which are too compact to carry above structures. Are
>> you talking about set_gamma type of ioctls. Each "set_gamma" type
>> ioctl needs a addition in callbacks till down the layer which looks bit
>> unnecessary, while writable blob properties are using same set_property
>> and get_property infrastructure.
>>
>> >
>> > If the only reason you group them is to atomically commit them, then the
>> > only thing we need is the atomic ioctl, which can shovel entire groups of
>> > properties over the wire at once.
>>
>> Atomic commit is not an explicit requirement. But splitting them to
>> many small pieces (in case of normal properties) are resulting into
>> many user-kernel switching overhead, which seems avoidable.
>
> The idea with atomic modeset / nuclear pageflip is that you collect a
> whole bunch of individual property updates into a "property set"
> container in userspace (libdrm).  When you're done setting all the
> values you want and "commit" the property set, all of the values that
> you collected get passed to the kernel in one go via a new ioctl (and
> are then updated in an atomic manner by the DRM driver).  So performing
> your 24 different property updates via the upcoming atomic API's
> shouldn't be a problem and shouldn't add any unnecessary overhead.
>
> The kernel-side of atomic modeset / nuclear pageflip is still a WIP, so
> it isn't upstream yet, but you can see the userspace-facing API in Rob
> Clark's repo here:
>
>         https://github.com/robclark/libdrm/blob/atomic/xf86drmMode.h
>
> Note the drmModePropertySet{Alloc,Add,Commit,Free}() functions near the
> bottom.
>

Thanks Matt,

I went through the share link. Got the idea of atomic
modeset. It is a good solution for setting 24 properties
in one go. But another issue is still unaddressed. I
mean still need to declare 24 properties which are not
"Really 24 different properties".

These are sub-parameters to 3 properties (color
reproduction, color saturation and edge enhancement).
Splitting them to 24 pieces, just because we don't have a
a provision in KMS, is a work around to get things
working. And, properties like "saturation_hue_gain_red",
"saturation_hue_gain_green" ... will be undoubtedly ugly.

In Rob Clark's tree, I noticed

extern int drmModePropertySetAddBlob(drmModePropertySetPtr set,
    uint32_t object_id,
    uint32_t property_id,
    uint64_t length,
    void *blob);

Seems, already some implementation of writable blobs is
there. I am not able to see the kernel though.

I request experts, to review this solution again. I found it
quite useful and hold good with the idea of Atomic modeset.

Regards,
Rahul Sharma

>
> Matt
>
> --
> Matt Roper
> Graphics Software Engineer
> ISG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

Re: [RFC 3/4] drm: add generic blob properties for image enhancement

[Daniel Vetter]

On Mon, Mar 10, 2014 at 09:50:14AM +0530, Rahul Sharma wrote:
> On 8 March 2014 06:16, Matt Roper <matthew.d.roper@intel.com> wrote:
> > On Fri, Mar 07, 2014 at 03:50:54PM +0530, Rahul Sharma wrote:
> >> Hi Daniel,
> >>
> >> On 7 March 2014 14:06, Daniel Vetter <daniel@ffwll.ch> wrote:
> >> > On Thu, Mar 06, 2014 at 11:42:13AM +0530, Rahul Sharma wrote:
> >> >> Add generic KMS blob properties to core drm framework. These
> >> >> are writable blob properties which can be used to set Image
> >> >> Enhancement parameters. The properties which are added here
> >> >> are meant for color reproduction, color saturation and edge
> >> >> enhancement.
> >> >>
> >> >> Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com>
> > ...
> >> > Tbh I don't really understand why we can't use normal properties for this.
> >> > We might want to add a new RBG type of property (or YUV fwiw, both with
> >> > and without alpha) to make this standardized (maybe with some fixed point
> >> > value for non-normalizes).
> >>
> >> I dropped Normal properties (the ones which accepts 64 bit data)
> >> because there will be too many of them. As you can see the
> >> count is going upto 24 parameters, means 24 such properties, as
> >> each can carry one parameter. This is too much of overhead.
> >> Please correct me if you mean something different.
> >>
> >> I am not sure what are RGB type properties? I know only about 64-bit
> >> normal properties which are too compact to carry above structures. Are
> >> you talking about set_gamma type of ioctls. Each "set_gamma" type
> >> ioctl needs a addition in callbacks till down the layer which looks bit
> >> unnecessary, while writable blob properties are using same set_property
> >> and get_property infrastructure.
> >>
> >> >
> >> > If the only reason you group them is to atomically commit them, then the
> >> > only thing we need is the atomic ioctl, which can shovel entire groups of
> >> > properties over the wire at once.
> >>
> >> Atomic commit is not an explicit requirement. But splitting them to
> >> many small pieces (in case of normal properties) are resulting into
> >> many user-kernel switching overhead, which seems avoidable.
> >
> > The idea with atomic modeset / nuclear pageflip is that you collect a
> > whole bunch of individual property updates into a "property set"
> > container in userspace (libdrm).  When you're done setting all the
> > values you want and "commit" the property set, all of the values that
> > you collected get passed to the kernel in one go via a new ioctl (and
> > are then updated in an atomic manner by the DRM driver).  So performing
> > your 24 different property updates via the upcoming atomic API's
> > shouldn't be a problem and shouldn't add any unnecessary overhead.
> >
> > The kernel-side of atomic modeset / nuclear pageflip is still a WIP, so
> > it isn't upstream yet, but you can see the userspace-facing API in Rob
> > Clark's repo here:
> >
> >         https://github.com/robclark/libdrm/blob/atomic/xf86drmMode.h
> >
> > Note the drmModePropertySet{Alloc,Add,Commit,Free}() functions near the
> > bottom.
> >
> 
> Thanks Matt,
> 
> I went through the share link. Got the idea of atomic
> modeset. It is a good solution for setting 24 properties
> in one go. But another issue is still unaddressed. I
> mean still need to declare 24 properties which are not
> "Really 24 different properties".
> 
> These are sub-parameters to 3 properties (color
> reproduction, color saturation and edge enhancement).
> Splitting them to 24 pieces, just because we don't have a
> a provision in KMS, is a work around to get things
> working. And, properties like "saturation_hue_gain_red",
> "saturation_hue_gain_green" ... will be undoubtedly ugly.
> 
> In Rob Clark's tree, I noticed
> 
> extern int drmModePropertySetAddBlob(drmModePropertySetPtr set,
>     uint32_t object_id,
>     uint32_t property_id,
>     uint64_t length,
>     void *blob);
> 
> Seems, already some implementation of writable blobs is
> there. I am not able to see the kernel though.
> 
> I request experts, to review this solution again. I found it
> quite useful and hold good with the idea of Atomic modeset.

Like I've said I think creating a standard property for RBG and RBGA
values makes sense, to group this stuff a bit. But I don't think we need
to group things further.

Some clear definitions of these properties is needed though - Sagar from
our display group is working on that a bit.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch