[PATCH 0/6] Intel Color Manager Framework

[PATCH 0/6] Intel Color Manager Framework

[Shashank Sharma]

Color manager is a new framework in i915 driver, which provides
a unified interface for various color correction methods supported
by intel hardwares. The high level overview of this change is: 

1. Intel hardwares support various color correction methods like
	a. csc(wide gamut) correction
	b. gamma correction
	c. hue/saturation correction
	d. contrast/brightness correction

2. Color manager provides a sysfs based interface and a common
   protocol (named as color EDID) for all the color correction
   properties. There is a command and data parser in the framework
   to extract command and data. 

3. The interested dispalys can call for a color-manager register
   from their corresponding init functions, like we have added for
   DSI and EDP interfaces. The corresponding entry will be
   /sys/class/drm/<card-name-connector-name>/color-manager

4. Color EDID: is a simple protocol for the communication from user about
   what to do, how to do, and where to do. The first value passed from
   userspace, will be color manager command value, which will be
   interpreted as: (sample command value: 0x 01 01 00 FF)
   <property[1Byte]> <enable/disable[1Byte]> <identifier[1Byte]>
   <no of data blocks[1Byte]>
	a. property: This byte represents color manager property, listed
	===========  as CSC correction, gamma correction, hue and
	saturation correction and brightness and contrast correction.
	For example 01 in above sample tells color manager to change
	Gamma correction.
	
	b. enable: This byte represents if we want to enable or disable
	=========  the mentioned property.
	For example 01 in above sample tells color-manager to enable the property.

	c. identifier: This byte tells the targeted pipe/plane for the property
	=============  change. There are few correction values which
		       can be selectively applied on a particular plane.
	For example 00 in above sample tells change in PIPE A.

	d. size: This byte tells how many data blocks will be followed by
	======== this command block.
	For example FF in above sample tells there are 255 correction values coming up.

5. For example (read/write):
   To enable csc on pipeA with 6 correction values:
   echo 0x00010006,0x111111,0x222222,0x333333,0x444444,0x555555,0x66666
		> /sys/class/drm/<connector>/color-manager
   
   To disable gamma on pipeA:
   echo 0x01000000 > /sys/class/drm/<connector>/color-manager

   To check the current status of color-manager:
   cat /sys/class/drm/<connector>/color-manager

6. The current implementation: 
   The current implementation is just limited to valleyview, that too on primary
   displays. But the design is modular and hence can be extended for all other
   devices. Once we agree upon the basic framework, I am going to extend it for
   all possible platforms. You can see current check for is_valleyview.

7. About the patches: 
   First patch adds the basic framework, parsers and header. 
   Second to fifth patch add support for one property each
   	CSC, Gamma, Contrast and Brightness, Hue and Sat.
   Sixth patch adds save and restore functions to maintain color corrections
   	across suspend resume cycles.

Shashank Sharma (6):
  drm/i915: Add Color manager framework
  drm/i915: Color Manager: Add CSC color correction
  drm/i915: Color manager: Add Gamma correction
  drm/i915: Color manager: brightness/contrast
  drm/i915: Color manager: hue/saturation correction
  drm/i915: Save color manager status

 drivers/gpu/drm/i915/Makefile        |    1 +
 drivers/gpu/drm/i915/i915_drv.h      |   29 +
 drivers/gpu/drm/i915/intel_clrmgr.c  | 1455 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_clrmgr.h  |  238 ++++++
 drivers/gpu/drm/i915/intel_display.c |    5 +-
 drivers/gpu/drm/i915/intel_dp.c      |    4 +
 drivers/gpu/drm/i915/intel_dsi.c     |    4 +
 7 files changed, 1734 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_clrmgr.c
 create mode 100644 drivers/gpu/drm/i915/intel_clrmgr.h

-- 
1.7.10.4

[PATCH 1/6] drm/i915: Add Color manager framework

[Shashank Sharma]

Intel color manager is a new framework to provide control over
few of the color properties (supported by intel Gen 7 and+
hardware) via sysfs interface. Currently supported properties
are:
1. CSC correction (wide gamute)
2. Gamma correction
3. Hue and Saturation correction
4. Brightness and contrast correction

This patch contains basic implementation of color manager
framework consisting:
1. A register function, which gets called from a dispaly
   while initializing its DRM connector(for example eDP
   and Mipi).
2. Read and write functions for /sysfs interface
3. A command and a data parser.
4. Dummy prototypes for color correction functions.

The sysfs entry will be created at:
/sys/class/drm/<connector-name>/color-manager
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
 drivers/gpu/drm/i915/Makefile       |    1 +
 drivers/gpu/drm/i915/i915_drv.h     |   29 ++
 drivers/gpu/drm/i915/intel_clrmgr.c |  643 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_clrmgr.h |  238 +++++++++++++
 drivers/gpu/drm/i915/intel_dp.c     |    4 +
 drivers/gpu/drm/i915/intel_dsi.c    |    4 +
 6 files changed, 919 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/intel_clrmgr.c
 create mode 100644 drivers/gpu/drm/i915/intel_clrmgr.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 4850494..0d8d877 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -40,6 +40,7 @@ i915-y := i915_drv.o i915_dma.o i915_irq.o \
 	  intel_sprite.o \
 	  intel_opregion.o \
 	  intel_sideband.o \
+	  intel_clrmgr.o \
 	  intel_uncore.o \
 	  dvo_ch7xxx.o \
 	  dvo_ch7017.o \
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b4587ac..6c8cbc3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -79,6 +79,13 @@ enum plane {
 };
 #define plane_name(p) ((p) + 'A')
 
+enum sprite_plane {
+	SPRITE_PLANE_A = 0,
+	SPRITE_PLANE_B = 1,
+	SPRITE_PLANE_C = 0,
+	SPRITE_PLANE_D = 1,
+};
+
 #define sprite_name(p, s) ((p) * INTEL_INFO(dev)->num_sprites + (s) + 'A')
 
 enum port {
@@ -1391,6 +1398,25 @@ struct intel_pipe_crc {
 	wait_queue_head_t wq;
 };
 
+/*
+  * Intel color manager structures.
+  * Used to represent the current status of
+  * color manager parameters
+  */
+struct clrmgr_pipe_status {
+	bool csc_enabled;
+	bool gamma_enabled;
+	bool hs_enabled;
+	bool cb_enabled;
+	bool gamma_s_enabled;
+	int planeid;
+};
+
+struct clrmgr_map {
+	struct drm_connector *connector;
+	struct clrmgr_pipe_status *pstatus;
+};
+
 typedef struct drm_i915_private {
 	struct drm_device *dev;
 	struct kmem_cache *slab;
@@ -1594,6 +1620,8 @@ typedef struct drm_i915_private {
 	struct i915_dri1_state dri1;
 	/* Old ums support infrastructure, same warning applies. */
 	struct i915_ums_state ums;
+	/* Color manager current status */
+	struct clrmgr_map clrmgr_status;
 } drm_i915_private_t;
 
 static inline struct drm_i915_private *to_i915(const struct drm_device *dev)
@@ -2008,6 +2036,7 @@ extern void intel_uncore_early_sanitize(struct drm_device *dev);
 extern void intel_uncore_init(struct drm_device *dev);
 extern void intel_uncore_check_errors(struct drm_device *dev);
 extern void intel_uncore_fini(struct drm_device *dev);
+extern bool intel_clrmgr_register(struct drm_connector *connector);
 
 void
 i915_enable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe,
diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c b/drivers/gpu/drm/i915/intel_clrmgr.c
new file mode 100644
index 0000000..2c826f3
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_clrmgr.c
@@ -0,0 +1,643 @@
+/*
+ * Copyright © 2008 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ * Shashank Sharma <shashank.sharma@intel.com>
+ * Uma Shankar <uma.shankar@intel.com>
+ * Shobhit Kumar <shobhit.kumar@intel.com>
+ */
+
+#include <linux/device.h>
+#include "drmP.h"
+#include "intel_drv.h"
+#include "i915_drm.h"
+#include "i915_drv.h"
+#include "intel_clrmgr.h"
+
+/* Sprite register default gamma values */
+u32 default_sprite_gamma_vals[] = {
+	0, 0, 0, 0, 0, 0
+};
+
+/* Gamma lookup table for Sprite planes */
+u32 gamma_sprite_softlut[GAMMA_SP_MAX_COUNT] = {
+	0, 0, 0, 0, 0, 1023
+};
+
+/* Gamma soft lookup table for default gamma =1.0 */
+u32 gamma_softlut[GAMMA_CORRECT_MAX_COUNT] =  {
+	0x000000, 0x0, 0x020202, 0x0, 0x040404, 0x0, 0x060606, 0x0,
+	0x080808, 0x0, 0x0A0A0A, 0x0, 0x0C0C0C, 0x0, 0x0E0E0E, 0x0,
+	0x101010, 0x0, 0x121212, 0x0, 0x141414, 0x0, 0x161616, 0x0,
+	0x181818, 0x0, 0x1A1A1A, 0x0, 0x1C1C1C, 0x0, 0x1E1E1E, 0x0,
+	0x202020, 0x0, 0x222222, 0x0, 0x242424, 0x0, 0x262626, 0x0,
+	0x282828, 0x0, 0x2A2A2A, 0x0, 0x2C2C2C, 0x0, 0x2E2E2E, 0x0,
+	0x303030, 0x0, 0x323232, 0x0, 0x343434, 0x0, 0x363636, 0x0,
+	0x383838, 0x0, 0x3A3A3A, 0x0, 0x3C3C3C, 0x0, 0x3E3E3E, 0x0,
+	0x404040, 0x0, 0x424242, 0x0, 0x444444, 0x0, 0x464646, 0x0,
+	0x484848, 0x0, 0x4A4A4A, 0x0, 0x4C4C4C, 0x0, 0x4E4E4E, 0x0,
+	0x505050, 0x0, 0x525252, 0x0, 0x545454, 0x0, 0x565656, 0x0,
+	0x585858, 0x0, 0x5A5A5A, 0x0, 0x5C5C5C, 0x0, 0x5E5E5E, 0x0,
+	0x606060, 0x0, 0x626262, 0x0, 0x646464, 0x0, 0x666666, 0x0,
+	0x686868, 0x0, 0x6A6A6A, 0x0, 0x6C6C6C, 0x0, 0x6E6E6E, 0x0,
+	0x707070, 0x0, 0x727272, 0x0, 0x747474, 0x0, 0x767676, 0x0,
+	0x787878, 0x0, 0x7A7A7A, 0x0, 0x7C7C7C, 0x0, 0x7E7E7E, 0x0,
+	0x808080, 0x0, 0x828282, 0x0, 0x848484, 0x0, 0x868686, 0x0,
+	0x888888, 0x0, 0x8A8A8A, 0x0, 0x8C8C8C, 0x0, 0x8E8E8E, 0x0,
+	0x909090, 0x0, 0x929292, 0x0, 0x949494, 0x0, 0x969696, 0x0,
+	0x989898, 0x0, 0x9A9A9A, 0x0, 0x9C9C9C, 0x0, 0x9E9E9E, 0x0,
+	0xA0A0A0, 0x0, 0xA2A2A2, 0x0, 0xA4A4A4, 0x0, 0xA6A6A6, 0x0,
+	0xA8A8A8, 0x0, 0xAAAAAA, 0x0, 0xACACAC, 0x0, 0xAEAEAE, 0x0,
+	0xB0B0B0, 0x0, 0xB2B2B2, 0x0, 0xB4B4B4, 0x0, 0xB6B6B6, 0x0,
+	0xB8B8B8, 0x0, 0xBABABA, 0x0, 0xBCBCBC, 0x0, 0xBEBEBE, 0x0,
+	0xC0C0C0, 0x0, 0xC2C2C2, 0x0, 0xC4C4C4, 0x0, 0xC6C6C6, 0x0,
+	0xC8C8C8, 0x0, 0xCACACA, 0x0, 0xCCCCCC, 0x0, 0xCECECE, 0x0,
+	0xD0D0D0, 0x0, 0xD2D2D2, 0x0, 0xD4D4D4, 0x0, 0xD6D6D6, 0x0,
+	0xD8D8D8, 0x0, 0xDADADA, 0x0, 0xDCDCDC, 0x0, 0xDEDEDE, 0x0,
+	0xE0E0E0, 0x0, 0xE2E2E2, 0x0, 0xE4E4E4, 0x0, 0xE6E6E6, 0x0,
+	0xE8E8E8, 0x0, 0xEAEAEA, 0x0, 0xECECEC, 0x0, 0xEEEEEE, 0x0,
+	0xF0F0F0, 0x0, 0xF2F2F2, 0x0, 0xF4F4F4, 0x0, 0xF6F6F6, 0x0,
+	0xF8F8F8, 0x0, 0xFAFAFA, 0x0, 0xFCFCFC, 0x0, 0xFEFEFE, 0x0
+};
+
+/* Color space conversion coff's */
+u32 csc_softlut[CSC_MAX_COEFF_COUNT] = {
+	1024,	 0, 67108864, 0, 0, 1024
+};
+
+u32 cb_softlut[CB_MAX_COEFF_COUNT] = {
+	0x80
+};
+
+u32 hs_softlut[HS_MAX_COEFF_COUNT] = {
+	0x1000000
+};
+
+u32 *clrmgr_luts[] = {
+	csc_softlut,
+	gamma_softlut,
+	cb_softlut,
+	hs_softlut,
+	gamma_sprite_softlut
+};
+
+/* Hue Saturation defaults */
+struct hue_saturationlut saved_hsvals[VLV_NO_SPRITE_REG] = {
+	{sprite_a, 0x1000000},
+	{sprite_b, 0x1000000},
+	{sprite_c, 0x1000000},
+	{sprite_d, 0x1000000}
+};
+
+/* Contrast brightness defaults */
+struct cont_brightlut saved_cbvals[VLV_NO_SPRITE_REG] = {
+	{sprite_a, 0x80},
+	{sprite_b, 0x80},
+	{sprite_c, 0x80},
+	{sprite_d, 0x80}
+};
+
+/* Get no of pipes in SOC */
+static int get_no_of_pipes(struct drm_device *dev)
+{
+	if (!dev) {
+		DRM_ERROR("NULL input to get no of pipes");
+		return 0;
+	}
+
+	if (IS_VALLEYVIEW(dev))
+		return VLV_NO_OF_PIPES;
+	if (IS_HASWELL(dev))
+		return HSW_NO_OF_PIPES;
+
+	return 0;
+}
+
+static bool intel_clrmgr_disable_hs(struct drm_device *dev, int identifier)
+{
+	return true;
+}
+
+static bool intel_clrmgr_disable_cb(struct drm_device *dev, int identifier)
+{
+	return true;
+}
+
+static bool intel_clrmgr_disable_gamma(struct drm_device *dev, int identifier)
+{
+	return true;
+}
+
+static void intel_clrmgr_disable_csc(struct drm_device *dev, int identifier)
+{
+}
+
+static bool intel_clrmgr_enable_hs(struct drm_device *dev, int identifier)
+{
+	return true;
+}
+static bool intel_clrmgr_enable_cb(struct drm_device *dev, int identifier)
+{
+	return true;
+}
+
+static bool intel_clrmgr_enable_gamma(struct drm_device *dev, int identifier)
+{
+	return true;
+}
+
+static bool intel_clrmgr_enable_csc(struct drm_device *dev, int identifier)
+{
+	return true;
+}
+
+/*
+* Enable a color manager property
+* This function assumes all the validation is done
+* by the caller
+*/
+static bool intel_clrmgr_enable_property(struct drm_device *dev,
+		int property, int identifier)
+{
+	switch (property) {
+	case clrmgr_csc:
+		intel_clrmgr_enable_csc(dev, identifier);
+		break;
+	case clrmgr_gamma:
+	case clrmgr_gammaspr:
+		intel_clrmgr_enable_gamma(dev, identifier);
+		break;
+	case clrmgr_cb:
+		intel_clrmgr_enable_cb(dev, identifier);
+		break;
+	case clrmgr_hs:
+		intel_clrmgr_enable_hs(dev, identifier);
+		break;
+	default:
+		DRM_ERROR("Clrmgr: Enable, invalid property %d", property);
+		return false;
+	}
+
+	return true;
+}
+
+/*
+* Disable a color manager property
+* This function assumes all the validation is done
+* by the caller
+*/
+static bool intel_clrmgr_disable_property(struct drm_device *dev,
+		int property, int identifier)
+{
+	switch (property) {
+	case clrmgr_csc:
+		intel_clrmgr_disable_csc(dev, identifier);
+		break;
+	case clrmgr_gamma:
+		intel_clrmgr_disable_gamma(dev, identifier);
+		break;
+	case clrmgr_cb:
+		intel_clrmgr_disable_cb(dev, identifier);
+		break;
+	case clrmgr_hs:
+		intel_clrmgr_disable_hs(dev, identifier);
+		break;
+	}
+	DRM_DEBUG_DRIVER("Clrmgr: %s disabled",
+		clrmgr_properties[property]);
+	return true;
+}
+
+/*
+* _parse_clrmgr_data: Extract the data from color manager buffer
+* The data parser follows a strict grammar.
+===============================
+* -The values must start with 0x and
+* -The values must be seperated by a comma
+* -There mustn't be a space after/before comma
+* -No non alphanumeric at start
+* -Sample: 0x5678,0xABCD
+*/
+int _parse_clrmgr_data(uint *dest, char *src, int max)
+{
+	int size = 0;
+	int bytes = 0;
+	char *populate = NULL;
+
+	/* Check grammar */
+	if (!dest || !src || *src != '0') {
+		DRM_ERROR("Clrmgr: Invalid input to parser");
+		return -EINVAL;
+	}
+
+	/* Extract values from buffer */
+	while ((size < max) && (*src != '\n')) {
+		populate = strsep(&src, ",");
+		if (!populate) {
+			if (size < max)
+				DRM_ERROR("Clrmgr: Parser: %d values missing",
+					max-size);
+			break;
+		}
+
+		/* Consider ',' also for length */
+		bytes += (strlen(populate)+1);
+		if (kstrtouint((const char *)populate, CLRMGR_PARSE_BASE,
+			&dest[size++])) {
+			DRM_ERROR("ClrMgr Parse: Invalid limit\n");
+			return -EINVAL;
+		}
+
+		if (CLRMGR_DEBUG_ENABLE)
+			DRM_DEBUG_DRIVER("Parse data: dest[%d] = 0x%x",
+			size-1, dest[size-1]);
+
+		/* End of data */
+		if (src == NULL || *src == '\0') {
+			if (size < max)
+				DRM_ERROR("Clrmgr: Parser: %d values missing",
+				max-size);
+			break;
+		}
+	}
+
+	DRM_DEBUG_DRIVER("Clrmgr: Parser: Loaded %d bytes from source", bytes);
+	return bytes;
+}
+
+/*
+* _extract_cmd
+* Actual extraction and interpratation of a
+* color manager command
+*/
+int _extract_cmd(const char *buf)
+{
+	u8 count = 2;
+	u8 update = 0;
+	int cmd = 0;
+
+	if (!buf) {
+		DRM_ERROR("Clrmgr: Apply: invalid input to extract_cmd\n");
+		return -1;
+	}
+
+	/* Check for alphanumeric chars */
+	while (count--) {
+		if (*buf >= 'A' && *buf <= 'F')
+			update = (*buf - 'A' + CLRMGR_HEX_ADDITION);
+		else if (*buf >= 'a' && *buf <= 'f')
+			update = (*buf - 'a' + CLRMGR_HEX_ADDITION);
+		else if (*buf >= '0' && *buf <= '9')
+			update = (*buf - '0');
+		else {
+			DRM_ERROR("Clrmgr: extract_cmd: Stupid input");
+			return -1;
+		}
+		cmd = cmd * CLRMGR_PARSE_BASE + update;
+		buf++;
+	}
+	DRM_DEBUG_DRIVER("Clrmgr: Extarcted: %d", cmd);
+	return cmd;
+}
+
+
+/*
+* _parse_clrmgr_cmd:
+* Extract command from color EDID
+*Color EDID (4 bytes) :
+*================
+*Byte 0	: Property to modify
+*Byte 1	: Enable /Disable property
+*Byte 2	: Identifier (Plane/Pipe)
+*Byte 3	: How many data bytes are following this byte
+*
+*	<1Byte>	  <1Byte>	               <1Byte>		<1Byte>
+*<<=property=>,<=enable/disable=>,<=identifier=>,<=No of data blocks=>,
+*<0xdata>,<0xdata>,<0xdata> ..... <0xdata>
+* Sample command+ data : 0x01010001,0x1234
+* This is to enable Gamma on Pipe A, with val=0x1234
+*/
+bool _parse_clrmgr_cmd(const char *ubuf, struct clrmgr_cmd *cmd)
+{
+	int ret = 0;
+
+	/* Validate input, command must start with 0x */
+	if (!ubuf || !cmd || *ubuf != '0') {
+		DRM_ERROR("Clrmgr: Apply: invalid input to parse_command\n");
+		return false;
+	}
+
+	/* Extract property to be changed */
+	ret = _extract_cmd(ubuf + CLR_EDID_PROPERTY);
+	if (ret < 0) {
+		DRM_ERROR("Clrmgr: Parse: extract property failed\n");
+		return false;
+	}
+	cmd->property = ret;
+
+	/* Extract enabled/disable choice */
+	ret = _extract_cmd(ubuf + CLR_EDID_ENABLE);
+	if (ret  < 0) {
+		DRM_ERROR("Clrmgr: Parse: extract enable failed\n");
+		return false;
+	}
+	cmd->enable = (ret ? true : false);
+
+	/* Extract Identifier of pipe/plane */
+	ret = _extract_cmd(ubuf + CLR_EDID_IDENTIFIER);
+	if (ret < 0) {
+		DRM_ERROR("Clrmgr: Parse: extract identifier failed\n");
+		return false;
+	}
+	cmd->identifier = ret;
+
+	/* Extract no of data bytes following */
+	ret = _extract_cmd(ubuf + CLR_EDID_SIZE);
+	if (ret < 0) {
+		DRM_ERROR("Clrmgr: Parse: extract size failed\n");
+		return false;
+	}
+	cmd->size = ret;
+	return true;
+}
+
+/*
+* intel_clrmgr_apply:
+* Parse, decode and Apply a change.
+*/
+bool intel_clrmgr_apply(struct drm_device *dev,
+	const char *ubuf, size_t count)
+{
+	bool ret = false;
+	struct clrmgr_cmd cmd = {0, 0, 0, 0};
+	char *raw_data = NULL;
+
+	if (!ubuf || !count) {
+		DRM_ERROR("Clrmgr: Apply: insufficient data\n");
+		return -EINVAL;
+	}
+
+	/* Parse command */
+	if (!_parse_clrmgr_cmd(ubuf, &cmd)) {
+		DRM_ERROR("Clrmgr: Command parsing failed");
+		return false;
+	}
+
+	/* Validate property */
+	if (cmd.property < clrmgr_csc || cmd.property > clrmgr_gammaspr) {
+		DRM_ERROR("Clrmgr: Invalid input, propery Max=%d, Min=%d",
+			clrmgr_csc, clrmgr_gammaspr);
+		return false;
+	}
+
+	/* Validate Identifier */
+	if (cmd.identifier < pipe_a || cmd.identifier > sprite_d) {
+		DRM_ERROR("Clrmgr: Invalid input, identifier Max=%d, Min=%d",
+			pipe_a, sprite_d);
+		return false;
+	}
+
+	if (cmd.enable) {
+		/* Validite size, min 1 block of data is required */
+		if (cmd.size > clrmgr_maxsize[cmd.property]  ||
+			cmd.size < CLR_MGR_PARSE_MIN) {
+			DRM_ERROR("Clrmgr: Invalid size=%d, range %d to %d",
+				(int)count, CLR_MGR_PARSE_MIN,
+				clrmgr_maxsize[cmd.property]);
+			return false;
+		}
+
+		raw_data = kzalloc(count, GFP_KERNEL);
+		if (!raw_data) {
+			DRM_ERROR("Clrmgr: Out of memory");
+			return false;
+		}
+
+		/* Get the data */
+		memcpy((void *)raw_data,
+			(const void *)&ubuf[CLR_EDID_DATA], count);
+
+		/* Parse data and load corresponsing soft LUT */
+		if (_parse_clrmgr_data(clrmgr_luts[cmd.property], raw_data,
+				cmd.size) < 0) {
+			DRM_ERROR("Clrmgr: Parse failed");
+			ret = false;
+			goto FREE_AND_RETURN;
+		}
+
+		/* Data loaded, now do changes in property */
+		if (!intel_clrmgr_enable_property(dev, cmd.property,
+			cmd.identifier)) {
+			DRM_ERROR("Clrmgr: Enable property %s failed",
+				clrmgr_properties[cmd.property]);
+			ret = false;
+			goto FREE_AND_RETURN;
+		}
+	} else {
+		/* Validite size, for disable just command is enough */
+		if (cmd.size) {
+			DRM_ERROR("Clrmgr: Invalid input, No data required");
+			return false;
+		}
+
+		/* Disable specified property */
+		if (!intel_clrmgr_disable_property(dev, cmd.property,
+			cmd.identifier)) {
+			DRM_ERROR("Clrmgr: Disable property %s failed",
+				clrmgr_properties[cmd.property]);
+			return false;
+		}
+	}
+
+	ret = true;
+	DRM_DEBUG_DRIVER("Clrmgr: apply success");
+
+FREE_AND_RETURN:
+	kfree(raw_data);
+	return ret;
+}
+
+/*
+  * Color manager write function.
+  * Current interface is /sys/class/drm/<connector-name>/color-manager
+  * Shows the current status of color manager features
+*/
+ssize_t intel_clrmgr_write(struct file *filp, struct kobject *kobj,
+		struct bin_attribute *ba, char *ubuf,
+		loff_t offset, size_t count)
+{
+	struct device *connector_dev = container_of(kobj, struct device, kobj);
+	struct drm_connector *connector = dev_get_drvdata(connector_dev);
+	struct drm_device *dev = connector->dev;
+
+	DRM_DEBUG_DRIVER("Clrmgr write");
+
+	/* Validate input */
+	if (!count || !ubuf) {
+		DRM_ERROR("Clrmgr: insufficient data\n");
+		return -EINVAL;
+	}
+
+	/* Parse the color EDID, apply the change */
+	if (!intel_clrmgr_apply(dev, ubuf, count)) {
+		DRM_ERROR("Clrmgr: Parse and apply failed\n");
+		return -1;
+	}
+
+	DRM_DEBUG_DRIVER("Clrmgr: Write success\n");
+	return count;
+}
+
+/*
+  * Color manager read function.
+  * Current interface is /sys/class/drm/<connector-name>/color-manager
+  * Shows the current status of color manager features
+*/
+ssize_t intel_clrmgr_read(struct file *filp, struct kobject *kobj,
+		struct bin_attribute *ba, char *buf, loff_t off, size_t count)
+{
+	u16 size = 0;
+	u8 pipe_count = 0;
+	const char *p = NULL;
+	struct device *connector_dev = container_of(kobj, struct device, kobj);
+	struct drm_connector *connector = dev_get_drvdata(connector_dev);
+	struct drm_device *dev = connector->dev;
+	drm_i915_private_t *dev_priv = dev->dev_private;
+	struct clrmgr_pipe_status *pstatus = dev_priv->clrmgr_status.pstatus;
+
+	if (!pstatus) {
+		DRM_DEBUG_DRIVER("Clrmgr not initialized yet");
+		return 0;
+	}
+
+	/* One page read is enough */
+	if (off)
+		return 0;
+
+	/* Get no of pipes in this arch */
+	pipe_count = get_no_of_pipes(dev);
+	if (!pipe_count) {
+		DRM_ERROR("This Gen device is not supported, cant get pipes\n");
+		return false;
+	}
+
+	/* Load per pipe color status */
+	do {
+		size += sprintf(buf + size, "PIPE %c\n", ('A' + pipe_count-1));
+		size += sprintf(buf + size, "=====\n");
+		p = clrmgr_properties[clrmgr_csc];
+		size += sprintf(buf + size, "1.%s %s\n", p,
+		pstatus[pipe_count-1].csc_enabled ? "Enabled" : "Disabled");
+		p = clrmgr_properties[clrmgr_gamma];
+		size += sprintf(buf + size, "2.%s %s\n", p,
+		pstatus[pipe_count-1].gamma_enabled ? "Enabled" : "Disabled");
+		p = clrmgr_properties[clrmgr_cb];
+		size += sprintf(buf + size, "3.%s %s\n", p,
+		pstatus[pipe_count-1].cb_enabled ? "Enabled" : "Disabled");
+		p = clrmgr_properties[clrmgr_hs];
+		size += sprintf(buf + size, "4.%s %s\n", p,
+		pstatus[pipe_count-1].hs_enabled ? "Enabled" : "Disabled");
+		p = clrmgr_properties[clrmgr_gammaspr];
+		size += sprintf(buf + size, "5.%s %s\n", p,
+		pstatus[pipe_count-1].gamma_s_enabled ? "Enabled" : "Disabled");
+	} while (--pipe_count);
+
+	DRM_DEBUG_DRIVER("Clrmgr Read done, %d bytes", size);
+	return size;
+}
+
+static struct bin_attribute clrmgr_attr = {
+	.attr.name = "color-manager",
+	.attr.mode = 0644,
+	.size = 0,
+	.read = intel_clrmgr_read,
+	.write = intel_clrmgr_write
+};
+
+/* Register color manager with a connector
+  * The connecter init function should call this function
+  * The current implementation is for Primary/Fix panels Like Mipi/EDP
+*/
+bool intel_clrmgr_register(struct drm_connector *connector)
+{
+	int pipe_count = 0;
+	struct clrmgr_pipe_status *pstatus = NULL;
+	struct drm_device *dev = connector->dev;
+	drm_i915_private_t *dev_priv = dev->dev_private;
+
+	/* Clr mgr is available for Gen 7 and + for now */
+	if (INTEL_INFO(dev)->gen < CLRMGR_GEN_THRESHOLD) {
+		DRM_ERROR("Clrmgr: This Gen device is not supported\n");
+		return false;
+	}
+
+	/* Todo: Current implementation supports only VLV.
+	Extend this for HSW and other gen 7+ devices */
+	if (!IS_VALLEYVIEW(dev)) {
+		DRM_ERROR("Clrmgr: Current implementation supports only VLV\n");
+		return false;
+	}
+
+	/* Create sysfs entry for color manager */
+	if (sysfs_create_bin_file(&connector->kdev->kobj, &clrmgr_attr)) {
+		DRM_ERROR("Clrmgr:  %s Register Color interface failed\n",
+			drm_get_connector_name(connector));
+		return false;
+	}
+
+	/* Get no of pipes of the arch */
+	pipe_count = get_no_of_pipes(dev);
+	if (!pipe_count) {
+		DRM_ERROR("Clrmgr: Cant get pipe info\n");
+		return false;
+	}
+
+	/* Load color status of the pipes */
+	pstatus = kzalloc(pipe_count * sizeof(struct clrmgr_pipe_status),
+				GFP_KERNEL);
+	if (!pstatus) {
+		DRM_ERROR("Clrmgr: %s Out of memory\n",
+			drm_get_connector_name(connector));
+		return false;
+	}
+
+	/* Initialize color status of the pipe,
+	  * planeid will be filled with specific enable call
+	  */
+	do {
+		pstatus[pipe_count-1].planeid = -1;
+		pstatus[pipe_count-1].csc_enabled = false;
+		pstatus[pipe_count-1].gamma_enabled = false;
+		pstatus[pipe_count-1].hs_enabled = false;
+		pstatus[pipe_count-1].cb_enabled = false;
+		pstatus[pipe_count-1].gamma_s_enabled = false;
+	} while (--pipe_count);
+
+	/* Load color manager status */
+	dev_priv->clrmgr_status.pstatus = pstatus;
+	dev_priv->clrmgr_status.connector = connector;
+
+	DRM_DEBUG_DRIVER("Clrmgr: Successfully registered for %s",
+			drm_get_connector_name(connector));
+	return true;
+}
+EXPORT_SYMBOL(intel_clrmgr_register);
diff --git a/drivers/gpu/drm/i915/intel_clrmgr.h b/drivers/gpu/drm/i915/intel_clrmgr.h
new file mode 100644
index 0000000..25d3491
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_clrmgr.h
@@ -0,0 +1,238 @@
+/*
+ * Copyright © 2008 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ * Shashank Sharma <shashank.sharma@intel.com>
+ * Uma Shankar <uma.shankar@intel.com>
+ */
+
+#ifndef _I915_CLR_MNGR_H_
+#define _I915_CLR_MNGR_H_
+
+
+struct cont_brightlut {
+	short sprite_no;
+	u32 val;
+};
+
+struct hue_saturationlut {
+	short sprite_no;
+	u32 val;
+};
+
+/* Debugging support */
+#define CLRMGR_DEBUG_ENABLE		0
+
+/* General defines */
+#define CLR_MGR_PARSE_MAX		256
+#define CLR_MGR_PARSE_MIN		1
+#define VLV_NO_SPRITE_REG				4
+#define SIZE_STATUS				10
+#define CLR_EDID_PROPERTY			2
+#define CLR_EDID_ENABLE			(CLR_EDID_PROPERTY + 2)
+#define CLR_EDID_IDENTIFIER		(CLR_EDID_ENABLE + 2)
+#define CLR_EDID_SIZE				(CLR_EDID_IDENTIFIER + 2)
+#define CLR_EDID_DATA				(CLR_EDID_SIZE + 3)
+#define CLRMGR_GEN_THRESHOLD	7
+#define VLV_NO_OF_PIPES			2
+#define HSW_NO_OF_PIPES			3
+#define CLRMGR_PARSE_BASE		16
+#define CLRMGR_HEX_ADDITION		10
+
+/* Pipe level gamma correction defines */
+#define PIPECONF_GAMMA			(1<<24)
+#define GAMMA_SP_MAX_COUNT		6
+#define GAMMA_MAX_VAL			1024
+#define SHIFTBY6(val)				(val<<6)
+#define GAMMA_CORRECT_MAX_COUNT 256
+
+#define PIPEA_MAX_RED		(dev_priv->info.display_mmio_offset + 0x70010)
+#define PIPEA_MAX_GREEN	(dev_priv->info.display_mmio_offset + 0x70014)
+#define PIPEA_MAX_BLUE	(dev_priv->info.display_mmio_offset + 0x70018)
+
+/* Sprite gamma correction regs */
+#define GAMMA_SPA_GAMC0		(dev_priv->info.display_mmio_offset + 0x721F4)
+#define GAMMA_SPB_GAMC0		(dev_priv->info.display_mmio_offset + 0x722F4)
+#define GAMMA_SPC_GAMC0		(dev_priv->info.display_mmio_offset + 0x723F4)
+#define GAMMA_SPD_GAMC0		(dev_priv->info.display_mmio_offset + 0x724F4)
+#define GAMMA_SP_REG_OFFSET	0x100
+
+/* Sprite control regs */
+#define GAMMA_SPA_CNTRL		(dev_priv->info.display_mmio_offset + 0x72180)
+#define GAMMA_SPB_CNTRL		(dev_priv->info.display_mmio_offset + 0x72280)
+#define GAMMA_SPC_CNTRL		(dev_priv->info.display_mmio_offset + 0x72380)
+#define GAMMA_SPD_CNTRL		(dev_priv->info.display_mmio_offset + 0x72480)
+#define GAMMA_SP_CTL_OFFSET	0x100
+#define GAMMA_ENABLE_SPR		(1<<30)
+#define GET_SPRITE_CTL(plid)	(GAMMA_SPA_CNTRL +		\
+				((plid - sprite_a) * GAMMA_SP_CTL_OFFSET))
+#define GET_SPRITE_REG(plid)	(GAMMA_SPA_GAMC0 +		\
+				((plid - sprite_a) * GAMMA_SP_REG_OFFSET))
+
+/* CSC defines and Control Register */
+#define	_PIPEACSC		(dev_priv->info.display_mmio_offset + 0x600b0)
+#define	_PIPEBCSC		(dev_priv->info.display_mmio_offset + 0x610b0)
+#define	PIPECSC(p)		(p ? _PIPEACSC : _PIPEBCSC)
+#define	PIPECONF_CSC_ENABLE		(1<<15)
+#define	CSC_MAX_COEFF_COUNT	6
+
+
+/* Sprite Contrast and Brightness Registers */
+#define CB_MAX_COEFF_COUNT	1
+#define SPRITEA_CB_REG		(dev_priv->info.display_mmio_offset + 0x721d0)
+#define SPRITEB_CB_REG		(dev_priv->info.display_mmio_offset + 0x722d0)
+#define SPRITEC_CB_REG		(dev_priv->info.display_mmio_offset + 0x723d0)
+#define SPRITED_CB_REG		(dev_priv->info.display_mmio_offset + 0x724d0)
+#define SPRITE_CB_OFFSET		0x100
+#define GET_SPRITE_CB(plid)		(SPRITEA_CB_REG +		\
+					(plid * SPRITE_CB_OFFSET))
+#define CB_DEFAULT_VAL		0x80
+
+/* Sprite Hue and Saturation Registers */
+#define HS_MAX_COEFF_COUNT	1
+#define SPRITEA_HS_REG		0x721d4
+#define SPRITEB_HS_REG		0x722d4
+#define SPRITEC_HS_REG		0x723d4
+#define SPRITED_HS_REG		0x724d4
+#define HS_DEFAULT_VAL		0x1000000
+#define SPRITE_HS_OFFSET		0x100
+#define GET_SPRITE_HS(plid)		(SPRITEA_HS_REG +		\
+						(plid * SPRITE_HS_OFFSET))
+
+
+/* Color manager properties */
+const char	*clrmgr_properties[] = {
+	"CSC_CORRECTION",
+	"GAMMA CORRECTION",
+	"BRIGHTNESS/CONTRAST",
+	"HUE/SATURATION",
+	"GAMMA CORRECTTION SPRITE"
+};
+
+/* Color manager max values */
+int clrmgr_maxsize[] = {
+	CSC_MAX_COEFF_COUNT,
+	GAMMA_CORRECT_MAX_COUNT,
+	CB_MAX_COEFF_COUNT,
+	HS_MAX_COEFF_COUNT,
+	GAMMA_SP_MAX_COUNT
+};
+
+/* Color manager features */
+enum clrmgr_features {
+	clrmgr_csc = 0,
+	clrmgr_gamma,
+	clrmgr_cb,
+	clrmgr_hs,
+	clrmgr_gammaspr
+};
+
+struct clrmgr_cmd {
+	bool enable;
+	int size;
+	int identifier;
+	enum clrmgr_features property;
+};
+
+/* Color manager features */
+enum clrmgr_identifiers {
+	pipe_a = 0,
+	pipe_b,
+	pipe_c,
+	plane_a,
+	plane_b,
+	plane_c,
+	sprite_a,
+	sprite_b,
+	sprite_c,
+	sprite_d
+};
+
+/* Required for sysfs calls */
+extern u32 csc_softlut[CSC_MAX_COEFF_COUNT];
+extern u32 gamma_softlut[GAMMA_CORRECT_MAX_COUNT];
+extern u32 gamma_sprite_softlut[GAMMA_SP_MAX_COUNT];
+extern u32 cb_softlut[CB_MAX_COEFF_COUNT];
+extern u32 hs_softlut[HS_MAX_COEFF_COUNT];
+extern void intel_crtc_load_lut(struct drm_crtc *crtc);
+
+/* Data dump */
+#define CLR_LIMIT_INTERNAL		1
+
+#if CLR_LIMIT_INTERNAL
+static inline int _validate_pipe(int identifier)
+{
+	if (identifier != pipe_a) {
+		DRM_ERROR("This functionality is only for PIPE A\n");
+		return -ENOSYS;
+	}
+	return 0;
+}
+static inline int _validate_plane(int identifier)
+{
+	if (identifier != plane_a) {
+		DRM_ERROR("This functionality is only for Plane A\n");
+		return -ENOSYS;
+	}
+	return 0;
+}
+static inline int _validate_sprite(int identifier)
+{
+	if (identifier != sprite_a && identifier != sprite_b) {
+		DRM_ERROR("This functionality is only for Sprite A/B\n");
+		return -ENOSYS;
+	}
+	return 0;
+}
+#else
+static inline int _validate_pipe(int identifier)
+{
+	return 0;
+}
+static inline int _validate_plane(int identifier)
+{
+	return 0;
+}
+static inline int _validate_sprite(int identifier)
+{
+	return 0;
+}
+#endif
+#define clrmgr_get_drvdata(d) dev_get_drvdata(d)
+#define _get_pipe_from_plane(pid) (pid - plane_a)
+
+
+/* Prototypes */
+int parse_clrmgr_input(uint *dest, char *src, int max, int read);
+int do_intel_enable_csc(struct drm_device *dev, void *data,
+				struct drm_crtc *crtc);
+bool intel_pipe_has_type(const struct drm_crtc *crtc, int type);
+void do_intel_disable_csc(struct drm_device *dev, struct drm_crtc *crtc);
+int intel_crtc_enable_gamma(struct drm_device *dev, u32 identifier);
+int intel_crtc_disable_gamma(struct drm_device *dev, u32 identifier);
+int intel_sprite_cb_adjust(struct drm_device *dev,
+		struct cont_brightlut *cb_ptr);
+int intel_sprite_hs_adjust(struct drm_device *dev,
+		struct hue_saturationlut *hs_ptr);
+void intel_save_clr_mgr_status(struct drm_device *dev);
+bool intel_restore_clr_mgr_status(struct drm_device *dev);
+#endif
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1ac4b11..10ef04e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3898,6 +3898,10 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 		I915_WRITE(PEG_BAND_GAP_DATA, (temp & ~0xf) | 0xd);
 	}
 
+	/* Register color manager interface */
+	if (!intel_clrmgr_register(connector))
+		DRM_ERROR("DP: Failed to register color manager features");
+
 	return true;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 3ee1db1..ff9bbd8 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -627,6 +627,10 @@ bool intel_dsi_init(struct drm_device *dev)
 	fixed_mode->type |= DRM_MODE_TYPE_PREFERRED;
 	intel_panel_init(&intel_connector->panel, fixed_mode, NULL);
 
+	/* Register to color manager interface */
+	if (!intel_clrmgr_register(connector))
+		DRM_ERROR("Mipi: Failed to register color manager features");
+
 	return true;
 
 err:
-- 
1.7.10.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/6] drm/i915: Color Manager: Add CSC color correction

[Shashank Sharma]

This patch is first extension to color manager framework.
It adds implementataion of color manager property CSC
correction (wide gamute) in intel color manager framework.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/intel_clrmgr.c |  117 +++++++++++++++++++++++++++++++++--
 1 file changed, 112 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c b/drivers/gpu/drm/i915/intel_clrmgr.c
index 2c826f3..363e3e6 100644
--- a/drivers/gpu/drm/i915/intel_clrmgr.c
+++ b/drivers/gpu/drm/i915/intel_clrmgr.c
@@ -146,11 +146,6 @@ static bool intel_clrmgr_disable_gamma(struct drm_device *dev, int identifier)
 {
 	return true;
 }
-
-static void intel_clrmgr_disable_csc(struct drm_device *dev, int identifier)
-{
-}
-
 static bool intel_clrmgr_enable_hs(struct drm_device *dev, int identifier)
 {
 	return true;
@@ -165,8 +160,120 @@ static bool intel_clrmgr_enable_gamma(struct drm_device *dev, int identifier)
 	return true;
 }
 
+/*
+* intel_disable_csc
+* Disable color space conversion on PIPE
+* idenifier is pipe identifier
+*/
+void
+intel_disable_csc(struct drm_device *dev, int identifier)
+{
+	u32 pipeconf = 0;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (_validate_pipe(identifier))
+		return;
+
+	/* Disable CSC on PIPE */
+	pipeconf = I915_READ(PIPECONF(identifier));
+	pipeconf &= ~(PIPECONF_CSC_ENABLE);
+	I915_WRITE(PIPECONF(identifier), pipeconf);
+	POSTING_READ(PIPECONF(identifier));
+
+	DRM_DEBUG_DRIVER("CSC disabled on PIPE %c\n",
+		identifier == pipe_a ? 'A' : 'B');
+
+	return;
+}
+
+/*
+* intel_clrmgr_disable_csc
+* Disable property CSC
+* identifier = pipe identifier
+*/
+static void intel_clrmgr_disable_csc(struct drm_device *dev, int identifier)
+{
+	drm_i915_private_t *dev_priv = dev->dev_private;
+	struct clrmgr_pipe_status *pstatus = dev_priv->clrmgr_status.pstatus;
+
+	if (!pstatus) {
+		DRM_ERROR("Clrmgr: color manager not initialized");
+		return;
+	}
+
+	intel_disable_csc(dev, identifier);
+	pstatus->csc_enabled = false;
+	DRM_DEBUG_DRIVER("Clrmgr: CSC disabled\n");
+}
+
+/*
+* intel_enable_csc
+* Enable color space conversion on PIPE
+* data is correction values to be written
+* identifier is pipe identifier
+*/
+int
+intel_enable_csc(struct drm_device *dev, u32 *data, int identifier)
+{
+	int count = 0;
+	int pipe = 0;
+	u32 csc_reg = 0;
+	u32 pipeconf = 0;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (!data) {
+		DRM_ERROR("NULL input to enable CSC");
+		return -EINVAL;
+	}
+
+	if (identifier != pipe_a) {
+		DRM_ERROR("CSC is supported on PIPEA only for now");
+		return -EINVAL;
+	}
+
+	pipeconf = I915_READ(PIPECONF(identifier));
+	pipeconf |= PIPECONF_CSC_ENABLE;
+	csc_reg = PIPECSC(identifier);
+
+	/* Enable csc correction */
+	I915_WRITE(PIPECONF(identifier), pipeconf);
+	POSTING_READ(PIPECONF(identifier));
+
+	/* Write csc coeff to csc regs */
+	while (count < CSC_MAX_COEFF_COUNT) {
+		I915_WRITE(csc_reg + (4 * count), ((u32 *)data)[count]);
+		count++;
+	}
+
+	DRM_DEBUG_DRIVER("CSC enabled on PIPE %c\n",
+		pipe == pipe_a ? 'A' : 'B');
+
+	return 0;
+}
+
+/*
+* intel_clrmgr_enable_csc
+* Enable property CSC
+* identifier = pipe identifier
+*/
 static bool intel_clrmgr_enable_csc(struct drm_device *dev, int identifier)
 {
+	drm_i915_private_t *dev_priv = dev->dev_private;
+	struct clrmgr_pipe_status *pstatus = dev_priv->clrmgr_status.pstatus;
+
+	if (!pstatus) {
+		DRM_ERROR("Clrmgr: color manager not initialized");
+		return false;
+	}
+
+	if (intel_enable_csc(dev, clrmgr_luts[clrmgr_csc],
+			identifier)) {
+		DRM_ERROR("Clrmgr: Enable CSC failed");
+		return false;
+	}
+
+	pstatus->csc_enabled = true;
+	DRM_DEBUG_DRIVER("Clrmgr: CSC Enabled");
 	return true;
 }
 
-- 
1.7.10.4

[PATCH 3/6] drm/i915: Color manager: Add Gamma correction

[Shashank Sharma]

This patch is second extension to color manager framework.
It adds implementataion of color manager property Gamma
correction in intel color manager framework.

Gamma correction is possible at various levels like:
1. PIPE level = (primary plane + sprite)
2. Plane level = (primary plane only)
3. Sprote level = (Sprite plane only)
This patch supports all three levels of correction.
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/intel_clrmgr.c  |  424 +++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_display.c |    5 +-
 2 files changed, 422 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c b/drivers/gpu/drm/i915/intel_clrmgr.c
index 363e3e6..1297e60 100644
--- a/drivers/gpu/drm/i915/intel_clrmgr.c
+++ b/drivers/gpu/drm/i915/intel_clrmgr.c
@@ -35,12 +35,14 @@
 
 /* Sprite register default gamma values */
 u32 default_sprite_gamma_vals[] = {
-	0, 0, 0, 0, 0, 0
+	0x00080808, 0x00101010, 0x00202020,
+	0x00404040, 0x00808080, 0x00C0C0C0
 };
 
 /* Gamma lookup table for Sprite planes */
 u32 gamma_sprite_softlut[GAMMA_SP_MAX_COUNT] = {
-	0, 0, 0, 0, 0, 1023
+	0x00080808, 0x00101010, 0x00202020,
+	0x00404040, 0x00808080, 0x00C0C0C0
 };
 
 /* Gamma soft lookup table for default gamma =1.0 */
@@ -142,24 +144,436 @@ static bool intel_clrmgr_disable_cb(struct drm_device *dev, int identifier)
 	return true;
 }
 
-static bool intel_clrmgr_disable_gamma(struct drm_device *dev, int identifier)
+static bool intel_clrmgr_enable_hs(struct drm_device *dev, int identifier)
 {
 	return true;
 }
-static bool intel_clrmgr_enable_hs(struct drm_device *dev, int identifier)
+static bool intel_clrmgr_enable_cb(struct drm_device *dev, int identifier)
 {
 	return true;
 }
-static bool intel_clrmgr_enable_cb(struct drm_device *dev, int identifier)
+
+/* Reset palette registers for gamma disabling */
+static int intel_clrmgr_reset_lut(struct drm_device *dev)
+{
+	struct drm_crtc *crtc = NULL;
+
+	/* Search for fix panel CRTC,
+	Assumption: Either MIPI or EDP is fix panel */
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		if (intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP) ||
+			intel_pipe_has_type(crtc, INTEL_OUTPUT_DSI))
+			break;
+	}
+
+	if (!crtc) {
+		DRM_ERROR("No CRTC for fix panel??");
+		return -1;
+	}
+
+	/* Reset pal regs */
+	intel_crtc_load_lut(crtc);
+	return 0;
+}
+
+
+/* Disable gamma correction for sprite planes  */
+int intel_disable_sprite_gamma(struct drm_device *dev, u32 identifier)
+{
+	u32 count = 0;
+	u32 status = 0;
+	u32 controlreg = 0;
+	u32 correctreg = 0;
+	u32 *lut = default_sprite_gamma_vals;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct clrmgr_pipe_status *pstatus = dev_priv->clrmgr_status.pstatus;
+
+	if (!pstatus) {
+		DRM_ERROR("Clrmgr: color manager not initialized");
+		return false;
+	}
+
+	/* Validate input */
+	if (identifier < sprite_a || identifier > sprite_d) {
+		DRM_ERROR("Clrmgr: Invalid sprite input to disable gamma\n");
+		return -ENOSYS;
+	}
+
+	/* Enable this check if we want to restrict clr mgr to
+	internal panel only */
+	if (_validate_sprite(identifier))
+		return -ENOSYS;
+
+	/* Load control and correcttion register */
+	controlreg = GET_SPRITE_CTL(identifier);
+	correctreg = GET_SPRITE_REG(identifier);
+
+	/* Disable gamma due to PIPE  */
+	status = I915_READ(controlreg);
+	status &= ~(GAMMA_ENABLE_SPR);
+	I915_WRITE(controlreg, status);
+
+	/*
+	* To disable plane level gamma correction, Write default
+	* values on sprite gamma regs
+	*/
+	while (count < GAMMA_SP_MAX_COUNT) {
+		/* Write and read */
+		I915_WRITE(correctreg - 4 * count, lut[count]);
+		status = I915_READ(correctreg - 4 * count++);
+	}
+
+	pstatus->gamma_s_enabled = false;
+	/* TODO: Reset gamma table default */
+	DRM_DEBUG_DRIVER("Gamma on Sprite %c disabled\n",
+		(identifier == sprite_a) ? 'A' : 'B');
+
+	return 0;
+}
+
+/* Disable gamma correction on Primary display */
+int intel_disable_plane_gamma(struct drm_device *dev, u32 identifier)
+{
+	u32 pipe = 0;
+	u32 status = 0;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct clrmgr_pipe_status *pstatus = dev_priv->clrmgr_status.pstatus;
+
+	if (!pstatus) {
+		DRM_ERROR("Clrmgr: color manager not initialized");
+		return false;
+	}
+
+	/* Enable this check if we want to restrict clr mgr to
+	internal panel only */
+	if (_validate_plane(identifier))
+		return -ENOSYS;
+
+	if (intel_clrmgr_reset_lut(dev)) {
+		DRM_ERROR("Reset LUT failed\n");
+		return -1;
+	}
+
+	pipe = _get_pipe_from_plane(identifier);
+	if (pipe < pipe_a || pipe > pipe_c) {
+		DRM_ERROR("Clrmgr: Cant enable plane gamma, invalid pipe\n");
+		return -ENOSYS;
+	}
+
+	/* Disable gamma on PIPE config  */
+	status = I915_READ(PIPECONF(pipe));
+	status &= ~(PIPECONF_GAMMA);
+	I915_WRITE(PIPECONF(pipe), status);
+
+	pstatus->gamma_enabled = false;
+	/* TODO: Reset gamma table default */
+	DRM_DEBUG("Gamma disabled on Pipe\n");
+	return 0;
+}
+
+
+/* Disable gamma correction on Primary display */
+int intel_disable_pipe_gamma(struct drm_device *dev, u32 identifier)
 {
+	u32 status = 0;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct clrmgr_pipe_status *pstatus = dev_priv->clrmgr_status.pstatus;
+
+	if (!pstatus) {
+		DRM_ERROR("Clrmgr: color manager not initialized");
+		return false;
+	}
+
+	/* Enable this check if we want to restrict clr mgr to
+	internal panel only */
+	if (_validate_pipe(identifier))
+		return -ENOSYS;
+
+	if (intel_clrmgr_reset_lut(dev)) {
+		DRM_ERROR("Reset LUT failed\n");
+		return -1;
+	}
+
+	/* Disable gamma on PIPE config  */
+	status = I915_READ(PIPECONF(identifier));
+	status &= ~(PIPECONF_GAMMA);
+	I915_WRITE(PIPECONF(identifier), status);
+
+	/* Disable gamma on sprite_a  */
+	status = I915_READ(GAMMA_SPA_CNTRL);
+	status &= ~(GAMMA_ENABLE_SPR);
+	I915_WRITE(GAMMA_SPA_CNTRL, status);
+
+	/* Disable gamma on sprite_b  */
+	status = I915_READ(GAMMA_SPB_CNTRL);
+	status &= ~(GAMMA_ENABLE_SPR);
+	I915_WRITE(GAMMA_SPB_CNTRL, status);
+
+	pstatus->gamma_enabled = false;
+	/* TODO: Reset gamma table default */
+	DRM_DEBUG("Gamma disabled on Pipe\n");
+	return 0;
+}
+
+/* Load gamma correction values corresponding to supplied
+gamma and program palette accordingly */
+int intel_crtc_disable_gamma(struct drm_device *dev, u32 identifier)
+{
+	switch (identifier) {
+
+	/* Whole pipe level correction disable */
+	case pipe_a:
+	case pipe_b:
+		return intel_disable_pipe_gamma(dev, identifier);
+
+	/* Primary planes */
+	case plane_a:
+	case plane_b:
+		return intel_disable_plane_gamma(dev, identifier);
+
+	/* Sprite planes */
+	case sprite_a:
+	case sprite_b:
+	case sprite_c:
+	case sprite_d:
+		return intel_disable_sprite_gamma(dev, identifier);
+
+	default:
+		DRM_ERROR("Invalid panel ID to Gamma enabled\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/* Disable Gamma correction */
+static bool intel_clrmgr_disable_gamma(struct drm_device *dev, int identifier)
+{
+	if (intel_crtc_disable_gamma(dev, identifier)) {
+		DRM_ERROR("\nClrmgr: Gamma disable failed");
+		return false;
+	}
+
 	return true;
 }
 
+
+
+/* Load palette registers for gamma correction */
+static void intel_clrmgr_load_palette(struct drm_device *dev, int identifier)
+{
+	u32 count = 0;
+	u32 odd = 0;
+	u32 even = 0;
+	u32 palreg = 0;
+	u32 *lut = clrmgr_luts[clrmgr_gamma];
+	drm_i915_private_t *dev_priv = dev->dev_private;
+
+	palreg = PALETTE(identifier);
+	DRM_DEBUG_DRIVER("Loading gamma palette from %x to %x",
+		palreg, palreg+GAMMA_CORRECT_MAX_COUNT-1);
+
+	/* 10.6 mode Gamma Implementation */
+	while (count < GAMMA_CORRECT_MAX_COUNT) {
+		/* Get the gamma corrected value from table */
+		odd = lut[count];
+		even = lut[count + 1];
+
+		/* Write even and odd parts in palette regs*/
+		I915_WRITE(palreg + 4 * count, even);
+		I915_WRITE(palreg + 4 * ++count, odd);
+		count++;
+	}
+
+	/* Write max values in 11.6 format */
+	I915_WRITE(PIPEA_MAX_BLUE, SHIFTBY6(GAMMA_MAX_VAL));
+	I915_WRITE(PIPEA_MAX_GREEN, SHIFTBY6(GAMMA_MAX_VAL));
+	I915_WRITE(PIPEA_MAX_RED, SHIFTBY6(GAMMA_MAX_VAL));
+}
+
+
+/* Enable gamma correction on sprite plane level */
+int intel_enable_sprite_gamma(struct drm_device *dev, int planeid)
+{
+	u32 count = 0;
+	u32 status = 0;
+	u32 controlreg = 0;
+	u32 correctreg = 0;
+	u32 *lut = clrmgr_luts[clrmgr_gammaspr];
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct clrmgr_pipe_status *pstatus = dev_priv->clrmgr_status.pstatus;
+
+	if (!pstatus) {
+		DRM_ERROR("Clrmgr: color manager not initialized");
+		return false;
+	}
+
+	/* Validate plane */
+	if (planeid < sprite_a || planeid > sprite_d) {
+		DRM_ERROR("Clrmgr: Invalid sprite object to gamma enable\n");
+		return -EINVAL;
+	}
+
+	/* Load control and correcttion register */
+	controlreg = GET_SPRITE_CTL(planeid);
+	correctreg = GET_SPRITE_REG(planeid);
+
+	/* Restore default gamma cofficients in gamma regs*/
+	while (count < GAMMA_SP_MAX_COUNT) {
+		I915_WRITE(correctreg - 4 * count, lut[count]);
+		status = I915_READ(correctreg - 4 * count++);
+	}
+
+	/*
+	* Disable PIPE level gamma correction effect on sprite.
+	* Set sprite gamma control bit = 0 to enable the plane specific
+	* gamma. If this bit is enabled, sprite will follow the gamma
+	* correction on PIPE not plane.
+	*/
+	status = I915_READ(controlreg);
+	status &= ~GAMMA_ENABLE_SPR;
+	I915_WRITE(controlreg, status);
+
+	pstatus->gamma_s_enabled = true;
+	DRM_DEBUG("Gamma applied on plane sprite%c\n",
+		(planeid == sprite_a) ? 'A' : 'B');
+
+	return 0;
+}
+
+
+/*
+* Gamma correction at Plane level */
+int intel_enable_plane_gamma(struct drm_device *dev, u32 identifier)
+{
+	u32 status = 0;
+	u32 pipe = 0;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct clrmgr_pipe_status *pstatus = dev_priv->clrmgr_status.pstatus;
+
+	if (!pstatus) {
+		DRM_ERROR("Clrmgr: color manager not initialized");
+		return false;
+	}
+
+	/* Enable this check if we want to restrict clr mgr to
+	internal panel only */
+	if (_validate_plane(identifier))
+		return -ENOSYS;
+
+	pipe = _get_pipe_from_plane(identifier);
+	if (pipe < pipe_a || pipe > pipe_c) {
+		DRM_ERROR("Clrmgr: Cant enable plane gamma, invalid pipe\n");
+		return -ENOSYS;
+	}
+
+	/* Load palette */
+	intel_clrmgr_load_palette(dev, pipe);
+
+	/* Enable gamma on PLANE A  */
+	status = I915_READ(PIPECONF(pipe));
+	status |= PIPECONF_GAMMA;
+	I915_WRITE(PIPECONF(pipe), status);
+
+	/* Shashank:
+	=========
+	Disable Sprite gamma for PIPE
+	correction here */
+
+	pstatus->gamma_enabled = true;
+	DRM_DEBUG("Gamma enabled on Plane A\n");
+
+	return 0;
+}
+
+
+/*
+* Gamma correction at PIPE level:
+* This function applies gamma correction Primary as well as Sprite planes
+* assosiated with this PIPE. Assumptions are:
+* Plane A is internal display primary panel.
+* Sprite A and B are interal display's sprite planes.
+*/
+int intel_enable_pipe_gamma(struct drm_device *dev, u32 identifier)
+{
+	u32 status = 0;
+	drm_i915_private_t *dev_priv = dev->dev_private;
+	struct clrmgr_pipe_status *pstatus = dev_priv->clrmgr_status.pstatus;
+
+	if (!pstatus) {
+		DRM_ERROR("Clrmgr: color manager not initialized");
+		return false;
+	}
+
+	/* Enable this check if we want to restrict clr mgr to
+	internal panel only */
+	if (_validate_pipe(identifier))
+		return -ENOSYS;
+
+	/* Load palette */
+	intel_clrmgr_load_palette(dev, identifier);
+
+	/* Set palette in 10bit mode gamma for PIPE */
+	status = I915_READ(PIPECONF(identifier));
+	status |= PIPECONF_GAMMA;
+	I915_WRITE(PIPECONF(identifier), status);
+
+	/* Enable gamma on Sprite plane A */
+	status = I915_READ(GAMMA_SPA_CNTRL);
+	status |= GAMMA_ENABLE_SPR;
+	I915_WRITE(GAMMA_SPA_CNTRL, status);
+
+	/* Enable gamma on Sprite plane B */
+	status = I915_READ(GAMMA_SPB_CNTRL);
+	status |= GAMMA_ENABLE_SPR;
+	I915_WRITE(GAMMA_SPB_CNTRL, status);
+
+	pstatus->gamma_enabled = true;
+	DRM_DEBUG("Gamma enabled on Pipe A\n");
+	return 0;
+}
+
+
+/* Load gamma correction values corresponding to supplied
+gamma and program palette accordingly */
+int intel_crtc_enable_gamma(struct drm_device *dev, u32 identifier)
+{
+	switch (identifier) {
+	/* Whole pipe level correction */
+	case pipe_a:
+	case pipe_b:
+		return intel_enable_pipe_gamma(dev, identifier);
+
+	/* Primary display planes */
+	case plane_a:
+	case plane_b:
+		return intel_enable_plane_gamma(dev, identifier);
+
+	/* Sprite planes */
+	case sprite_a:
+	case sprite_b:
+	case sprite_c:
+	case sprite_d:
+		return intel_enable_sprite_gamma(dev, identifier);
+
+	default:
+		DRM_ERROR("Invalid identifier to enable gamma\n");
+		return -EINVAL;
+	}
+}
+
 static bool intel_clrmgr_enable_gamma(struct drm_device *dev, int identifier)
 {
+	if (intel_crtc_enable_gamma(dev, identifier)) {
+		DRM_ERROR("\nClrmgr: Gamma enable failed");
+		return false;
+	}
+
+	DRM_DEBUG("\nClrmgr: Gamma successfully enabled");
 	return true;
 }
 
+
 /*
 * intel_disable_csc
 * Disable color space conversion on PIPE
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e0b7d06..1dd2d72 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -341,7 +341,7 @@ static void vlv_clock(int refclk, intel_clock_t *clock)
 /**
  * Returns whether any output on the specified pipe is of the specified type
  */
-static bool intel_pipe_has_type(struct drm_crtc *crtc, int type)
+bool intel_pipe_has_type(struct drm_crtc *crtc, int type)
 {
 	struct drm_device *dev = crtc->dev;
 	struct intel_encoder *encoder;
@@ -3492,7 +3492,7 @@ void hsw_disable_ips(struct intel_crtc *crtc)
 }
 
 /** Loads the palette/gamma unit for the CRTC with the prepared values */
-static void intel_crtc_load_lut(struct drm_crtc *crtc)
+void intel_crtc_load_lut(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3537,6 +3537,7 @@ static void intel_crtc_load_lut(struct drm_crtc *crtc)
 	if (reenable_ips)
 		hsw_enable_ips(intel_crtc);
 }
+EXPORT_SYMBOL(intel_crtc_load_lut);
 
 static void ironlake_crtc_enable(struct drm_crtc *crtc)
 {
-- 
1.7.10.4

[PATCH 4/6] drm/i915: Color manager: brightness/contrast

[Shashank Sharma]

This patch is third extension to color manager framework.
It adds implementataion of color manager property "Brightness
and Contrast correctio"n in intel color manager framework.
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/intel_clrmgr.c |   98 ++++++++++++++++++++++++++++++++++-
 1 file changed, 96 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c b/drivers/gpu/drm/i915/intel_clrmgr.c
index 1297e60..db13e2b 100644
--- a/drivers/gpu/drm/i915/intel_clrmgr.c
+++ b/drivers/gpu/drm/i915/intel_clrmgr.c
@@ -134,25 +134,119 @@ static int get_no_of_pipes(struct drm_device *dev)
 	return 0;
 }
 
+/* Check Sprite status */
+static bool is_sprite_enabled(struct drm_i915_private *dev_priv,
+			enum pipe pipe, enum plane plane)
+{
+	int reg;
+	u32 val;
+
+	reg = SPCNTR(pipe, plane);
+	val = I915_READ(reg);
+	return val & SP_ENABLE;
+}
+
 static bool intel_clrmgr_disable_hs(struct drm_device *dev, int identifier)
 {
 	return true;
 }
 
-static bool intel_clrmgr_disable_cb(struct drm_device *dev, int identifier)
+static bool intel_clrmgr_enable_hs(struct drm_device *dev, int identifier)
 {
 	return true;
 }
 
-static bool intel_clrmgr_enable_hs(struct drm_device *dev, int identifier)
+/* Tune Contrast Brightness Value for Sprite */
+int intel_sprite_cb_adjust(struct drm_device *dev,
+		struct cont_brightlut *cb_ptr)
+{
+	drm_i915_private_t *dev_priv = dev->dev_private;
+
+	if (!dev_priv || !cb_ptr) {
+		DRM_ERROR("Contrast Brightness: Invalid Arguments\n");
+		return -EINVAL;
+	}
+
+	switch (cb_ptr->sprite_no) {
+	/* Sprite plane */
+	case sprite_a:
+		if (is_sprite_enabled(dev_priv, PIPE_A, SPRITE_PLANE_A))
+			I915_WRITE(SPRITEA_CB_REG, cb_ptr->val);
+		break;
+
+	case sprite_b:
+		if (is_sprite_enabled(dev_priv, PIPE_A, SPRITE_PLANE_B))
+			I915_WRITE(SPRITEB_CB_REG, cb_ptr->val);
+		break;
+
+	case sprite_c:
+		if (is_sprite_enabled(dev_priv, PIPE_B, SPRITE_PLANE_C))
+			I915_WRITE(SPRITEC_CB_REG, cb_ptr->val);
+		break;
+
+	case sprite_d:
+		if (is_sprite_enabled(dev_priv, PIPE_B, SPRITE_PLANE_D))
+			I915_WRITE(SPRITED_CB_REG, cb_ptr->val);
+		break;
+
+	default:
+		DRM_ERROR("Invalid Sprite Number\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/* Disable correction for contrast/brightness */
+static bool intel_clrmgr_disable_cb(struct drm_device *dev, int identifier)
 {
+	struct cont_brightlut cb;
+	drm_i915_private_t *dev_priv = dev->dev_private;
+	struct clrmgr_pipe_status *pstatus = dev_priv->clrmgr_status.pstatus;
+
+	if (!pstatus) {
+		DRM_ERROR("Clrmgr: color manager not initialized");
+		return false;
+	}
+
+	cb.sprite_no = identifier;
+	cb.val = CB_DEFAULT_VAL;
+
+	if (intel_sprite_cb_adjust(dev, &cb)) {
+		DRM_ERROR("\nClrmgr: Contrast/Brigtness disable failed");
+		return false;
+	}
+
+	pstatus->cb_enabled = false;
 	return true;
+
 }
+
+/* Enable correction for contrast/brightness */
 static bool intel_clrmgr_enable_cb(struct drm_device *dev, int identifier)
 {
+	struct cont_brightlut cb;
+	drm_i915_private_t *dev_priv = dev->dev_private;
+	struct clrmgr_pipe_status *pstatus = dev_priv->clrmgr_status.pstatus;
+
+	if (!pstatus) {
+		DRM_ERROR("Clrmgr: color manager not initialized");
+		return false;
+	}
+
+	cb.sprite_no = identifier;
+	cb.val = clrmgr_luts[clrmgr_cb][CB_MAX_COEFF_COUNT-1];
+
+	if (intel_sprite_cb_adjust(dev, &cb)) {
+		DRM_ERROR("\nClrmgr: Contrast/Brigtness enable failed");
+		return false;
+	}
+
+	pstatus->cb_enabled = true;
 	return true;
 }
 
+
 /* Reset palette registers for gamma disabling */
 static int intel_clrmgr_reset_lut(struct drm_device *dev)
 {
-- 
1.7.10.4

[PATCH 5/6] drm/i915: Color manager: hue/saturation correction

[Shashank Sharma]

This patch is fourth extension to color manager framework.
It adds implementataion of color manager property "Hue and
Saturation correction" in intel color manager framework.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/intel_clrmgr.c |   84 +++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c b/drivers/gpu/drm/i915/intel_clrmgr.c
index db13e2b..9d2203e 100644
--- a/drivers/gpu/drm/i915/intel_clrmgr.c
+++ b/drivers/gpu/drm/i915/intel_clrmgr.c
@@ -146,16 +146,100 @@ static bool is_sprite_enabled(struct drm_i915_private *dev_priv,
 	return val & SP_ENABLE;
 }
 
+/* Tune Hue Saturation Value for Sprite */
+int intel_sprite_hs_adjust(struct drm_device *dev,
+		struct hue_saturationlut *hs_ptr)
+{
+	drm_i915_private_t *dev_priv = dev->dev_private;
+	if (!dev_priv || !hs_ptr) {
+		DRM_ERROR("Hue Saturation: Invalid Arguments\n");
+		return -EINVAL;
+	}
+
+	switch (hs_ptr->sprite_no) {
+	/* Sprite plane */
+	case sprite_a:
+		if (is_sprite_enabled(dev_priv, PIPE_A, SPRITE_PLANE_A))
+			I915_WRITE(SPRITEA_HS_REG, hs_ptr->val);
+		break;
+
+	case sprite_b:
+		if (is_sprite_enabled(dev_priv, PIPE_A, SPRITE_PLANE_B))
+			I915_WRITE(SPRITEB_HS_REG, hs_ptr->val);
+		break;
+
+	case sprite_c:
+		if (is_sprite_enabled(dev_priv, PIPE_B, SPRITE_PLANE_C))
+			I915_WRITE(SPRITEC_HS_REG, hs_ptr->val);
+		break;
+
+	case sprite_d:
+		if (is_sprite_enabled(dev_priv, PIPE_B, SPRITE_PLANE_D))
+			I915_WRITE(SPRITED_HS_REG, hs_ptr->val);
+		break;
+	default:
+		DRM_ERROR("Invalid Sprite Number\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/*
+* Disable corrcetion for Hue/Saturation
+* This is currently supported for Sprite planes only
+*/
 static bool intel_clrmgr_disable_hs(struct drm_device *dev, int identifier)
 {
+	struct hue_saturationlut hs;
+	drm_i915_private_t *dev_priv = dev->dev_private;
+	struct clrmgr_pipe_status *pstatus = dev_priv->clrmgr_status.pstatus;
+
+	if (!pstatus) {
+		DRM_ERROR("Clrmgr: color manager not initialized");
+		return false;
+	}
+
+	hs.sprite_no = identifier;
+	hs.val = HS_DEFAULT_VAL;
+
+	if (intel_sprite_hs_adjust(dev, &hs)) {
+		DRM_ERROR("\nClrmgr: Hue/Saturation disable failed");
+		return false;
+	}
+
+	pstatus->hs_enabled = false;
 	return true;
 }
 
+/*
+* Enable corrcetion for Hue/Saturation
+* This is currently supported for Sprite planes only
+*/
 static bool intel_clrmgr_enable_hs(struct drm_device *dev, int identifier)
 {
+	struct hue_saturationlut hs;
+	drm_i915_private_t *dev_priv = dev->dev_private;
+	struct clrmgr_pipe_status *pstatus = dev_priv->clrmgr_status.pstatus;
+
+	if (!pstatus) {
+		DRM_ERROR("Clrmgr: color manager not initialized");
+		return false;
+	}
+
+	hs.sprite_no = identifier;
+	hs.val = clrmgr_luts[clrmgr_hs][CLR_MGR_PARSE_MIN-1];
+
+	if (intel_sprite_hs_adjust(dev, &hs)) {
+		DRM_ERROR("\nClrmgr: Hue/Saturation enable failed");
+		return false;
+	}
+
+	pstatus->hs_enabled = true;
 	return true;
 }
 
+
 /* Tune Contrast Brightness Value for Sprite */
 int intel_sprite_cb_adjust(struct drm_device *dev,
 		struct cont_brightlut *cb_ptr)
-- 
1.7.10.4

[PATCH 6/6] drm/i915: Save color manager status

[Shashank Sharma]

This patch adds functions:
1. save_color_manager_status: to save color manager
correction values during a display suspend and
2. restore_color_manager_status: to restore color
correction values during display resume time.

This will help to give consistent color correction across
suspend resume cycles

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Vikas Korjani <vikas.korjani@intel.com>
---
 drivers/gpu/drm/i915/intel_clrmgr.c |  113 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_clrmgr.h |    2 +-
 2 files changed, 114 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c b/drivers/gpu/drm/i915/intel_clrmgr.c
index 9d2203e..bb5d072 100644
--- a/drivers/gpu/drm/i915/intel_clrmgr.c
+++ b/drivers/gpu/drm/i915/intel_clrmgr.c
@@ -926,6 +926,119 @@ static bool intel_clrmgr_disable_property(struct drm_device *dev,
 	return true;
 }
 
+
+/* Apply saved values of contrast/brightness */
+static bool intel_restore_cb(struct drm_device *dev)
+{
+	int count = 0;
+
+	while (count < VLV_NO_SPRITE_REG) {
+		if (intel_sprite_cb_adjust(dev, &saved_cbvals[count++])) {
+			DRM_ERROR("Color Restore: Error restoring CB\n");
+			return false;
+		}
+	}
+	return true;
+}
+
+/* Apply saved values of hue/saturation */
+static bool intel_restore_hs(struct drm_device *dev)
+{
+	int count = 0;
+
+	while (count < VLV_NO_SPRITE_REG) {
+		if (intel_sprite_hs_adjust(dev, &saved_hsvals[count++])) {
+			DRM_ERROR("Color Restore: Error restoring HS\n");
+			return false;
+		}
+	}
+	return true;
+}
+
+/* Sustain color manager corrections across suspend/resume */
+bool intel_restore_clr_mgr_status(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct clrmgr_pipe_status *pstatus = dev_priv->clrmgr_status.pstatus;
+
+	/* Validate input */
+	if (!dev_priv) {
+		DRM_ERROR("Color Restore: Invalid input\n");
+		return false;
+	}
+
+	/* If gamma enabled, restore gamma */
+	if (pstatus->gamma_enabled) {
+		if (intel_clrmgr_enable_gamma(dev, pipe_a)) {
+			DRM_ERROR("Color Restore: gamma failed\n");
+			return false;
+		}
+	}
+
+	/* If csc enabled, restore csc */
+	if (pstatus->csc_enabled) {
+		if (intel_clrmgr_enable_csc(dev, pipe_a)) {
+			DRM_ERROR("Color Restore: CSC failed\n");
+			return false;
+		}
+	}
+
+	/* Restore hue staturation */
+	if (pstatus->hs_enabled) {
+		if (!intel_restore_hs(dev)) {
+			DRM_ERROR("Color Restore: Restore hue/sat failed\n");
+			return false;
+		}
+	}
+
+	/* Restore contrast brightness */
+	if (pstatus->cb_enabled) {
+		if (!intel_restore_cb(dev)) {
+			DRM_ERROR("Color Restore: Restore CB failed\n");
+			return false;
+		}
+	}
+
+	DRM_DEBUG("Color Restore: Restore success\n");
+	return true;
+}
+EXPORT_SYMBOL(intel_restore_clr_mgr_status);
+
+/* Save current contrast brightness values */
+void intel_save_cb_status(struct drm_device *dev)
+{
+	int count = 0;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	while (count <= VLV_NO_SPRITE_REG) {
+		saved_cbvals[count].val =
+			I915_READ(GET_SPRITE_HS(count));
+		count++;
+	}
+}
+
+/* Save current hue saturation values */
+void intel_save_hs_status(struct drm_device *dev)
+{
+	int count = 0;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	while (count <= VLV_NO_SPRITE_REG) {
+			saved_hsvals[count].val =
+				I915_READ(GET_SPRITE_CB(count));
+			count++;
+	}
+}
+
+/* Save the required register values to be restored */
+void intel_save_clr_mgr_status(struct drm_device *dev)
+{
+	intel_save_hs_status(dev);
+	intel_save_cb_status(dev);
+}
+EXPORT_SYMBOL(intel_save_clr_mgr_status);
+
+
 /*
 * _parse_clrmgr_data: Extract the data from color manager buffer
 * The data parser follows a strict grammar.
diff --git a/drivers/gpu/drm/i915/intel_clrmgr.h b/drivers/gpu/drm/i915/intel_clrmgr.h
index 25d3491..6fe0592 100644
--- a/drivers/gpu/drm/i915/intel_clrmgr.h
+++ b/drivers/gpu/drm/i915/intel_clrmgr.h
@@ -45,7 +45,7 @@ struct hue_saturationlut {
 /* General defines */
 #define CLR_MGR_PARSE_MAX		256
 #define CLR_MGR_PARSE_MIN		1
-#define VLV_NO_SPRITE_REG				4
+#define VLV_NO_SPRITE_REG			4
 #define SIZE_STATUS				10
 #define CLR_EDID_PROPERTY			2
 #define CLR_EDID_ENABLE			(CLR_EDID_PROPERTY + 2)
-- 
1.7.10.4

Re: [Intel-gfx] [PATCH 0/6] Intel Color Manager Framework

[Ville Syrjälä]

On Thu, Feb 20, 2014 at 06:07:21PM +0530, Shashank Sharma wrote:
> Color manager is a new framework in i915 driver, which provides
> a unified interface for various color correction methods supported
> by intel hardwares. The high level overview of this change is: 

Would have been good to discuss this idea before implementing it. The
plan is to use kms properties for this kind of stuff which allows us
to hook it up with the upcoming atomic modeset API. Just yesterday there
was some discussion on #dri-devel about exposing user settable blob
properties even before the atomic modeset API lands (it was always the
plan for the atomic modeset API anyway). So based on a cursory glance,
this looks like it's going in the wrong direction.

Also ideally the properties should be hardware agnostic, so a generic
userspace could use them regardless of the hardware/driver. Obviously
that might not be possible in all cases, but we should at least spend
a bit of effort on trying to make that happen for most properties.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 0/6] Intel Color Manager Framework

[Sharma, Shashank]

Hi Ville/All,

We gave a presentation on design on this framework, few months ago, in one of our common forum with OTC folks. 
We  discussed, took review comments, and re-designed the framework, as per the feedbacks. 

We also discussed the benefits of providing the controls directly from /sysfs over going for a UI manager based property settings.
So I don't understand where are we going wrong, can you please elaborate a bit ? 

This is just a basic design, and once go ahead with this, we can always work on making hardware agnostic, as you recommended.  

IMHO, controls from /sysfs would be a very generic interface for all linux/drm based platform, where any userspace can read/write and control properties. 
We don't even need a UI manager or a minimum executable to play around, just a small script can do. But we can always write something on top of this,
to be included in any UI framework or property. 
 
Regards
Shashank    

-----Original Message-----
From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
Sent: Thursday, February 20, 2014 6:41 PM
To: Sharma, Shashank
Cc: intel-gfx@lists.freedesktop.org; Shankar, Uma; dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 0/6] Intel Color Manager Framework

On Thu, Feb 20, 2014 at 06:07:21PM +0530, Shashank Sharma wrote:
> Color manager is a new framework in i915 driver, which provides a 
> unified interface for various color correction methods supported by 
> intel hardwares. The high level overview of this change is:

Would have been good to discuss this idea before implementing it. The plan is to use kms properties for this kind of stuff which allows us to hook it up with the upcoming atomic modeset API. Just yesterday there was some discussion on #dri-devel about exposing user settable blob properties even before the atomic modeset API lands (it was always the plan for the atomic modeset API anyway). So based on a cursory glance, this looks like it's going in the wrong direction.

Also ideally the properties should be hardware agnostic, so a generic userspace could use them regardless of the hardware/driver. Obviously that might not be possible in all cases, but we should at least spend a bit of effort on trying to make that happen for most properties.

--
Ville Syrjälä
Intel OTC

Re: [PATCH 0/6] Intel Color Manager Framework

[Ville Syrjälä]

On Fri, Feb 21, 2014 at 03:34:43AM +0000, Sharma, Shashank wrote:
> Hi Ville/All,
> 
> We gave a presentation on design on this framework, few months ago, in one of our common forum with OTC folks. 
> We  discussed, took review comments, and re-designed the framework, as per the feedbacks. 

Apparently I wasn't there. And in any case it would be better to discuss
it on dri-devel where people outside Intel can give their opinion.

> 
> We also discussed the benefits of providing the controls directly from /sysfs over going for a UI manager based property settings.
> So I don't understand where are we going wrong, can you please elaborate a bit ? 

The most important issue from my POV is that it can't be part of the
atomic modeset.

Another issue is that it make the whole API inconsistent. Some stuff
through ioctl, some stuff through sysfs, some stuff through whatever the
next guy thinks of. It's not pretty. I've worked in the past with a
driver where I had to poke at various standardish ioctls, custom ioctls,
and sysfs to make it do anything useful, and I have no interest in
repeating that experience. sysfs is especially painful since you have
do the string<->binary conversions all over the place, and also you
en up doing open+read/write+close cycles for every little thing.

It also adds more entrypoints into the driver for us to worry about.
That means extra worries about the power management stuff and locking
at the very least.

Also the rules of sysfs say "one item per file". The only allowed
exception to this rule I know of is hardware provided blobs (like
EDID, PCI ROM etc.). Your current implementation breaks this rule
blatantly.

> 
> This is just a basic design, and once go ahead with this, we can always work on making hardware agnostic, as you recommended.  
> 
> IMHO, controls from /sysfs would be a very generic interface for all linux/drm based platform, where any userspace can read/write and control properties. 
> We don't even need a UI manager or a minimum executable to play around, just a small script can do. But we can always write something on top of this,
> to be included in any UI framework or property.

If there's a real need to get at properties through sysfs, then we could
think about exposing them all. But that may presents some issues where
the current master suddenly gets confused about its state since someone
else went behind its back and changed a bunch of stuff.

>  
> Regards
> Shashank    
> 
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
> Sent: Thursday, February 20, 2014 6:41 PM
> To: Sharma, Shashank
> Cc: intel-gfx@lists.freedesktop.org; Shankar, Uma; dri-devel@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 0/6] Intel Color Manager Framework
> 
> On Thu, Feb 20, 2014 at 06:07:21PM +0530, Shashank Sharma wrote:
> > Color manager is a new framework in i915 driver, which provides a 
> > unified interface for various color correction methods supported by 
> > intel hardwares. The high level overview of this change is:
> 
> Would have been good to discuss this idea before implementing it. The plan is to use kms properties for this kind of stuff which allows us to hook it up with the upcoming atomic modeset API. Just yesterday there was some discussion on #dri-devel about exposing user settable blob properties even before the atomic modeset API lands (it was always the plan for the atomic modeset API anyway). So based on a cursory glance, this looks like it's going in the wrong direction.
> 
> Also ideally the properties should be hardware agnostic, so a generic userspace could use them regardless of the hardware/driver. Obviously that might not be possible in all cases, but we should at least spend a bit of effort on trying to make that happen for most properties.
> 
> --
> Ville Syrjälä
> Intel OTC

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 0/6] Intel Color Manager Framework

[Sharma, Shashank]

Hi Ville, 

Thanks for your time and comments. 
I can understand two basic problems what you see in this implementation: 

1.  The most important issue from my POV is that it can't be part of the atomic modeset.    
2.  it make the whole API inconsistent. 

I am not sure if its good to block all current implementation because we have thought something for this in atomic modeset. 
I think even in atomic modeset we need the core implementation like this, but the interface would be different, which might come in from of a DRM property. 
So at that time we can use this core implementation as it is, only the interfaces/framework needs to be changed. 

In this way we can always go ahead with a current implementation, and can just change the interfaces to fit in to the final interface like DRM property in atomic modeset.
Or you can suggest us the expected interface, and we can work on modifying that as per expectation.
 
Please correct me if any of my assumptions are not right, or not feasible, or if I am just a moron :) .
    
Regards
Shashank 
-----Original Message-----
From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
Sent: Friday, February 21, 2014 2:47 PM
To: Sharma, Shashank
Cc: intel-gfx@lists.freedesktop.org; Shankar, Uma; dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 0/6] Intel Color Manager Framework

On Fri, Feb 21, 2014 at 03:34:43AM +0000, Sharma, Shashank wrote:
> Hi Ville/All,
> 
> We gave a presentation on design on this framework, few months ago, in one of our common forum with OTC folks. 
> We  discussed, took review comments, and re-designed the framework, as per the feedbacks. 

Apparently I wasn't there. And in any case it would be better to discuss it on dri-devel where people outside Intel can give their opinion.

> 
> We also discussed the benefits of providing the controls directly from /sysfs over going for a UI manager based property settings.
> So I don't understand where are we going wrong, can you please elaborate a bit ? 

The most important issue from my POV is that it can't be part of the atomic modeset.

Another issue is that it make the whole API inconsistent. Some stuff through ioctl, some stuff through sysfs, some stuff through whatever the next guy thinks of. It's not pretty. I've worked in the past with a driver where I had to poke at various standardish ioctls, custom ioctls, and sysfs to make it do anything useful, and I have no interest in repeating that experience. sysfs is especially painful since you have do the string<->binary conversions all over the place, and also you en up doing open+read/write+close cycles for every little thing.

It also adds more entrypoints into the driver for us to worry about.
That means extra worries about the power management stuff and locking at the very least.

Also the rules of sysfs say "one item per file". The only allowed exception to this rule I know of is hardware provided blobs (like EDID, PCI ROM etc.). Your current implementation breaks this rule blatantly.

> 
> This is just a basic design, and once go ahead with this, we can always work on making hardware agnostic, as you recommended.  
> 
> IMHO, controls from /sysfs would be a very generic interface for all linux/drm based platform, where any userspace can read/write and control properties. 
> We don't even need a UI manager or a minimum executable to play 
> around, just a small script can do. But we can always write something on top of this, to be included in any UI framework or property.

If there's a real need to get at properties through sysfs, then we could think about exposing them all. But that may presents some issues where the current master suddenly gets confused about its state since someone else went behind its back and changed a bunch of stuff.

>  
> Regards
> Shashank    
> 
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Thursday, February 20, 2014 6:41 PM
> To: Sharma, Shashank
> Cc: intel-gfx@lists.freedesktop.org; Shankar, Uma; 
> dri-devel@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 0/6] Intel Color Manager Framework
> 
> On Thu, Feb 20, 2014 at 06:07:21PM +0530, Shashank Sharma wrote:
> > Color manager is a new framework in i915 driver, which provides a 
> > unified interface for various color correction methods supported by 
> > intel hardwares. The high level overview of this change is:
> 
> Would have been good to discuss this idea before implementing it. The plan is to use kms properties for this kind of stuff which allows us to hook it up with the upcoming atomic modeset API. Just yesterday there was some discussion on #dri-devel about exposing user settable blob properties even before the atomic modeset API lands (it was always the plan for the atomic modeset API anyway). So based on a cursory glance, this looks like it's going in the wrong direction.
> 
> Also ideally the properties should be hardware agnostic, so a generic userspace could use them regardless of the hardware/driver. Obviously that might not be possible in all cases, but we should at least spend a bit of effort on trying to make that happen for most properties.
> 
> --
> Ville Syrjälä
> Intel OTC

--
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH 0/6] Intel Color Manager Framework

[Ville Syrjälä]

On Fri, Feb 21, 2014 at 02:20:24PM +0000, Sharma, Shashank wrote:
> Hi Ville, 
> 
> Thanks for your time and comments. 
> I can understand two basic problems what you see in this implementation: 
> 
> 1.  The most important issue from my POV is that it can't be part of the atomic modeset.    
> 2.  it make the whole API inconsistent. 
> 
> I am not sure if its good to block all current implementation because we have thought something for this in atomic modeset. 
> I think even in atomic modeset we need the core implementation like this, but the interface would be different, which might come in from of a DRM property. 
> So at that time we can use this core implementation as it is, only the interfaces/framework needs to be changed. 
> 
> In this way we can always go ahead with a current implementation, and can just change the interfaces to fit in to the final interface like DRM property in atomic modeset.
> Or you can suggest us the expected interface, and we can work on modifying that as per expectation.

The exptected interface will be range properties for stuff like
brightness, contrast etc. controls. There are already such things as
connector properties, but we're going to want something similar as
plane or crtc properties. One thing that worries me about such
properties though is whether we can make them hardware agnostic and
yet allow userspace precise control over the final image. That is, if we
map some fixed input range to a hardware specific output range, userspace
can't know how the actual output will change when the input changes. On
the other hand if the input is hardware specific, userspace can't know
what value to put in there to get the expected change on the output side.

For bigger stuff like CSC matrices and gamma ramps we will want to use
some reasonably well defined blobs. Ie. the internal strucuture of the
blob has to be documented and it shouldn't contain more than necessary.
Ie. just the CSC matrix coefficients for one matrix, or just the entries
for a single gamma ramp. Again ideally we should make the blobs hardware
agnostic, but still allow precise control over the output data.

I think this is going to involve first going over our hardware features,
trying to find the common patterns between different generations. If
there's a way to make something that works across the board for us, or
at least across a wide range, then we should also ask for some input on
dri-devel whether the proposed property would work for other people. We
may need to define new property types to more precisely define what the
value of the property actually means.

>  
> Please correct me if any of my assumptions are not right, or not feasible, or if I am just a moron :) .

The implementation itself has to be tied into the pipe config (and
eventual plane config) stuff, which are the structures meant to house
the full device state, which will then be applied in one go.

At the moment it looks like you're writing a bunch of registers from
various places w/o much thought into how those things interact with
anything else. For instance, what's the story with your use of the
pipe CSC unit vs. the already existing "Broadcast RGB" property?

>     
> Regards
> Shashank 
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
> Sent: Friday, February 21, 2014 2:47 PM
> To: Sharma, Shashank
> Cc: intel-gfx@lists.freedesktop.org; Shankar, Uma; dri-devel@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 0/6] Intel Color Manager Framework
> 
> On Fri, Feb 21, 2014 at 03:34:43AM +0000, Sharma, Shashank wrote:
> > Hi Ville/All,
> > 
> > We gave a presentation on design on this framework, few months ago, in one of our common forum with OTC folks. 
> > We  discussed, took review comments, and re-designed the framework, as per the feedbacks. 
> 
> Apparently I wasn't there. And in any case it would be better to discuss it on dri-devel where people outside Intel can give their opinion.
> 
> > 
> > We also discussed the benefits of providing the controls directly from /sysfs over going for a UI manager based property settings.
> > So I don't understand where are we going wrong, can you please elaborate a bit ? 
> 
> The most important issue from my POV is that it can't be part of the atomic modeset.
> 
> Another issue is that it make the whole API inconsistent. Some stuff through ioctl, some stuff through sysfs, some stuff through whatever the next guy thinks of. It's not pretty. I've worked in the past with a driver where I had to poke at various standardish ioctls, custom ioctls, and sysfs to make it do anything useful, and I have no interest in repeating that experience. sysfs is especially painful since you have do the string<->binary conversions all over the place, and also you en up doing open+read/write+close cycles for every little thing.
> 
> It also adds more entrypoints into the driver for us to worry about.
> That means extra worries about the power management stuff and locking at the very least.
> 
> Also the rules of sysfs say "one item per file". The only allowed exception to this rule I know of is hardware provided blobs (like EDID, PCI ROM etc.). Your current implementation breaks this rule blatantly.
> 
> > 
> > This is just a basic design, and once go ahead with this, we can always work on making hardware agnostic, as you recommended.  
> > 
> > IMHO, controls from /sysfs would be a very generic interface for all linux/drm based platform, where any userspace can read/write and control properties. 
> > We don't even need a UI manager or a minimum executable to play 
> > around, just a small script can do. But we can always write something on top of this, to be included in any UI framework or property.
> 
> If there's a real need to get at properties through sysfs, then we could think about exposing them all. But that may presents some issues where the current master suddenly gets confused about its state since someone else went behind its back and changed a bunch of stuff.
> 
> >  
> > Regards
> > Shashank    
> > 
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > Sent: Thursday, February 20, 2014 6:41 PM
> > To: Sharma, Shashank
> > Cc: intel-gfx@lists.freedesktop.org; Shankar, Uma; 
> > dri-devel@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 0/6] Intel Color Manager Framework
> > 
> > On Thu, Feb 20, 2014 at 06:07:21PM +0530, Shashank Sharma wrote:
> > > Color manager is a new framework in i915 driver, which provides a 
> > > unified interface for various color correction methods supported by 
> > > intel hardwares. The high level overview of this change is:
> > 
> > Would have been good to discuss this idea before implementing it. The plan is to use kms properties for this kind of stuff which allows us to hook it up with the upcoming atomic modeset API. Just yesterday there was some discussion on #dri-devel about exposing user settable blob properties even before the atomic modeset API lands (it was always the plan for the atomic modeset API anyway). So based on a cursory glance, this looks like it's going in the wrong direction.
> > 
> > Also ideally the properties should be hardware agnostic, so a generic userspace could use them regardless of the hardware/driver. Obviously that might not be possible in all cases, but we should at least spend a bit of effort on trying to make that happen for most properties.
> > 
> > --
> > Ville Syrjälä
> > Intel OTC
> 
> --
> Ville Syrjälä
> Intel OTC

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 0/6] Intel Color Manager Framework

[Rob Clark]

On Fri, Feb 21, 2014 at 9:20 AM, Sharma, Shashank
<shashank.sharma@intel.com> wrote:
> Hi Ville,
>
> Thanks for your time and comments.
> I can understand two basic problems what you see in this implementation:
>
> 1.  The most important issue from my POV is that it can't be part of the atomic modeset.
> 2.  it make the whole API inconsistent.
>
> I am not sure if its good to block all current implementation because we have thought something for this in atomic modeset.
> I think even in atomic modeset we need the core implementation like this, but the interface would be different, which might come in from of a DRM property.
> So at that time we can use this core implementation as it is, only the interfaces/framework needs to be changed.

What I would suggest is re-form the userspace facing API to be
properties.. if needed we can add a setblobprop ioctl (Sean Paul was,
I think, adding that already).  Then userspace and use setprop ioctls
for now, and optionally atomic ioctl later when it is in place.  No
reason for it to be blocked waiting for atomic.

btw, I didn't look into the patches yet, but full-nak on idea of
exposing via sysfs.  This should be the sort of thing that is set by
the process that has mastership on the drm device, which we can't
enforce via sysfs.  Using properties seems like the way to go.

BR,
-R

> In this way we can always go ahead with a current implementation, and can just change the interfaces to fit in to the final interface like DRM property in atomic modeset.
> Or you can suggest us the expected interface, and we can work on modifying that as per expectation.

Re: [PATCH 0/6] Intel Color Manager Framework

[Alex Deucher]

On Fri, Feb 21, 2014 at 9:46 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Fri, Feb 21, 2014 at 02:20:24PM +0000, Sharma, Shashank wrote:
>> Hi Ville,
>>
>> Thanks for your time and comments.
>> I can understand two basic problems what you see in this implementation:
>>
>> 1.  The most important issue from my POV is that it can't be part of the atomic modeset.
>> 2.  it make the whole API inconsistent.
>>
>> I am not sure if its good to block all current implementation because we have thought something for this in atomic modeset.
>> I think even in atomic modeset we need the core implementation like this, but the interface would be different, which might come in from of a DRM property.
>> So at that time we can use this core implementation as it is, only the interfaces/framework needs to be changed.
>>
>> In this way we can always go ahead with a current implementation, and can just change the interfaces to fit in to the final interface like DRM property in atomic modeset.
>> Or you can suggest us the expected interface, and we can work on modifying that as per expectation.
>
> The exptected interface will be range properties for stuff like
> brightness, contrast etc. controls. There are already such things as
> connector properties, but we're going to want something similar as
> plane or crtc properties. One thing that worries me about such
> properties though is whether we can make them hardware agnostic and
> yet allow userspace precise control over the final image. That is, if we
> map some fixed input range to a hardware specific output range, userspace
> can't know how the actual output will change when the input changes. On
> the other hand if the input is hardware specific, userspace can't know
> what value to put in there to get the expected change on the output side.
>
> For bigger stuff like CSC matrices and gamma ramps we will want to use
> some reasonably well defined blobs. Ie. the internal strucuture of the
> blob has to be documented and it shouldn't contain more than necessary.
> Ie. just the CSC matrix coefficients for one matrix, or just the entries
> for a single gamma ramp. Again ideally we should make the blobs hardware
> agnostic, but still allow precise control over the output data.
>
> I think this is going to involve first going over our hardware features,
> trying to find the common patterns between different generations. If
> there's a way to make something that works across the board for us, or
> at least across a wide range, then we should also ask for some input on
> dri-devel whether the proposed property would work for other people. We
> may need to define new property types to more precisely define what the
> value of the property actually means.
>

Our hardware has similar features, so I'm sure there will be quite a
bit of common ground.  I also vote for properties.

Alex

Re: [PATCH 0/6] Intel Color Manager Framework

[Sean Paul]

On Fri, Feb 21, 2014 at 9:49 AM, Rob Clark <robdclark@gmail.com> wrote:
> On Fri, Feb 21, 2014 at 9:20 AM, Sharma, Shashank
> <shashank.sharma@intel.com> wrote:
>> Hi Ville,
>>
>> Thanks for your time and comments.
>> I can understand two basic problems what you see in this implementation:
>>
>> 1.  The most important issue from my POV is that it can't be part of the atomic modeset.
>> 2.  it make the whole API inconsistent.
>>
>> I am not sure if its good to block all current implementation because we have thought something for this in atomic modeset.
>> I think even in atomic modeset we need the core implementation like this, but the interface would be different, which might come in from of a DRM property.
>> So at that time we can use this core implementation as it is, only the interfaces/framework needs to be changed.
>
> What I would suggest is re-form the userspace facing API to be
> properties.. if needed we can add a setblobprop ioctl (Sean Paul was,
> I think, adding that already).

This is news to me ;-)

I'm pulling the atomic stuff into the CrOS tree, but I think Rahul
Sharma is the guy you're looking for wrt setblobprop
(http://www.spinics.net/lists/dri-devel/msg54010.html).

Sean


> Then userspace and use setprop ioctls
> for now, and optionally atomic ioctl later when it is in place.  No
> reason for it to be blocked waiting for atomic.
>
> btw, I didn't look into the patches yet, but full-nak on idea of
> exposing via sysfs.  This should be the sort of thing that is set by
> the process that has mastership on the drm device, which we can't
> enforce via sysfs.  Using properties seems like the way to go.
>
> BR,
> -R
>
>> In this way we can always go ahead with a current implementation, and can just change the interfaces to fit in to the final interface like DRM property in atomic modeset.
>> Or you can suggest us the expected interface, and we can work on modifying that as per expectation.

Re: [Intel-gfx] [PATCH 0/6] Intel Color Manager Framework

[Stéphane Marchesin]

[-- Attachment #1.1: Type: text/plain, Size: 1980 bytes --]

On Thu, Feb 20, 2014 at 5:11 AM, Ville Syrjälä <
ville.syrjala@linux.intel.com> wrote:

> On Thu, Feb 20, 2014 at 06:07:21PM +0530, Shashank Sharma wrote:
> > Color manager is a new framework in i915 driver, which provides
> > a unified interface for various color correction methods supported
> > by intel hardwares. The high level overview of this change is:
>
> Would have been good to discuss this idea before implementing it. The
> plan is to use kms properties for this kind of stuff which allows us
> to hook it up with the upcoming atomic modeset API. Just yesterday there
> was some discussion on #dri-devel about exposing user settable blob
> properties even before the atomic modeset API lands (it was always the
> plan for the atomic modeset API anyway). So based on a cursory glance,
> this looks like it's going in the wrong direction.
>

+1. We'e looking into hooking up color correction controls, and if the
interface isn't standard our user space won't be portable across drivers.
There are multiple reasons for using drm properties:
- the KMS interface already provides a way to set the gamma ramp, which
this code seems to replicate.
- the KMS interface allows us to name properties independently and
enumerate them. It seems like right now you can't enumerate properties or
guess what "property 0" is. I'd rather set the "Color conversion matrix"
than remember to set "property 0" (and even then, I'm not really sure it
exists).
- you can reuse the get/set infrastructure which is already in place

Another thing that came out of the discussion on irc is that we should
standardize the properties. For example we could use a text file describing
the name of the controls and the format of the data (something similar to
the device tree bindings). That way user space can expect "color conversion
matrix" to mean the same thing everywhere, to get the same data as input,
and to work the same way on all platforms.

Stéphane

[-- Attachment #1.2: Type: text/html, Size: 2564 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/6] Intel Color Manager Framework

[Sharma, Shashank]

[-- Attachment #1.1: Type: text/plain, Size: 2665 bytes --]

On Thu, Feb 20, 2014 at 5:11 AM, Ville Syrjälä <ville.syrjala@linux.intel.com<mailto:ville.syrjala@linux.intel.com>> wrote:
On Thu, Feb 20, 2014 at 06:07:21PM +0530, Shashank Sharma wrote:
>> Color manager is a new framework in i915 driver, which provides
>> a unified interface for various color correction methods supported
>> by intel hardwares. The high level overview of this change is:

>Would have been good to discuss this idea before implementing it. The
>plan is to use kms properties for this kind of stuff which allows us
>to hook it up with the upcoming atomic modeset API. Just yesterday there
>was some discussion on #dri-devel about exposing user settable blob
>properties even before the atomic modeset API lands (it was always the
>plan for the atomic modeset API anyway). So based on a cursory glance,
>this looks like it's going in the wrong direction.

+1. We'e looking into hooking up color correction controls, and if the interface isn't standard our user space won't be portable across drivers. There are multiple reasons for using drm properties:
- the KMS interface already provides a way to set the gamma ramp, which this code seems to replicate.

The current KMS interface just initializes the gamma soft LUT palette registers, in 8 bit mode corresponding to unit gamma. It's impossible to apply accurate values corresponding to gamma=2.2 or 1.5 from KMS
Because for that we need to program palette registers in 10.6 bit mode of hardware.
>- the KMS interface allows us to name properties independently and enumerate them. It seems like right now you can't enumerate properties or guess what "property 0" is. I'd rather set the "Color conversion matrix" than remember to set >"property 0" (and even then, I'm not really sure it exists).

All the properties are getting enumerated in color manager register function. The framework defines proper identifiers and mapping for each property, and every property is having a corresponding soft-lut to be loaded with correction values.

- you can reuse the get/set infrastructure which is already in place


>Another thing that came out of the discussion on irc is that we should standardize the properties. For example we could use a text file describing the name of the controls and the format of the data (something similar to the device tree >bindings). That way user space can expect "color conversion matrix" to mean the same thing everywhere, to get the same data as input, and to work the same way on all platforms.
If you can please have a look on the header file, we are almost doing the same thing, in form of a protocol.

Stéphane

Regards
Shashank

[-- Attachment #1.2: Type: text/html, Size: 6898 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/6] Intel Color Manager Framework

[Sharma, Shashank]

>> Hi Ville,
>> 
>> Thanks for your time and comments. 
>> I can understand two basic problems what you see in this implementation: 
>> 
>> 1.  The most important issue from my POV is that it can't be part of the atomic modeset.    
>> 2.  it make the whole API inconsistent. 
>> 
>> I am not sure if its good to block all current implementation because we have thought something for this in atomic modeset. 
>> I think even in atomic modeset we need the core implementation like this, but the interface would be different, which might come in from of a DRM property. 
>> So at that time we can use this core implementation as it is, only the interfaces/framework needs to be changed. 
>> 
>> In this way we can always go ahead with a current implementation, and can just change the interfaces to fit in to the final interface like DRM property in atomic modeset.
>> Or you can suggest us the expected interface, and we can work on modifying that as per expectation.

>The exptected interface will be range properties for stuff like brightness, contrast etc. controls. There are already such things as connector properties, but we're going to want something similar as plane or crtc properties. One thing that worries me about >such properties though is whether we can make them hardware agnostic and yet allow userspace precise control over the final image. That is, if we map some fixed input range to a hardware specific output range, userspace can't know how the actual >output will change when the input changes. On the other hand if the input is hardware specific, userspace can't know what value to put in there to get the expected change on the output side.
>For bigger stuff like CSC matrices and gamma ramps we will want to use some reasonably well defined blobs. Ie. the internal strucuture of the blob has to be documented and it shouldn't contain more than necessary.
>Ie. just the CSC matrix coefficients for one matrix, or just the entries for a single gamma ramp. Again ideally we should make the blobs hardware agnostic, but still allow precise control over the output data.
>I think this is going to involve first going over our hardware features, trying to find the common patterns between different generations. If there's a way to make something that works across the board for us, or at least across a wide range, then we >should also ask for some input on dri-devel whether the proposed property would work for other people. We may need to define new property types to more precisely define what the value of the property actually means.

Actually this is what we had done, but we just picked a wrong interface. The reason behind picking sysfs was that we were worried about the increasing no of IOCTL getting listed. 
We just created a superset of all required inputs for different properties, and then defined a data protocol (color EDID).   
>> Please correct me if any of my assumptions are not right, or not feasible, or if I am just a moron :) .

>The implementation itself has to be tied into the pipe config (and eventual plane config) stuff, which are the structures meant to house the full device state, which will then be applied in one go.
>At the moment it looks like you're writing a bunch of registers from various places w/o much thought into how those things interact with anything else. For instance, what's the story with your use of the pipe CSC unit vs. the already existing "Broadcast >RGB" property?

If you have a close look at the header, We are already defining a pipe status map, which at any time tells you, what's the color status of the pipe, just as an independent implementation, instead of a DRM property. As you already know, there is no relation between DRM property and this implementation, we are not doing anything there.  

Probably, I will spend some more time in how can I club this framework in DRM property, and re-implement the patch accordingly, and come back. 
At that time, as you suggested, I can take inputs from dri-devel for the actual implementation.

Regards
Shashank
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Friday, February 21, 2014 2:47 PM
> To: Sharma, Shashank
> Cc: intel-gfx@lists.freedesktop.org; Shankar, Uma; 
> dri-devel@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 0/6] Intel Color Manager Framework
> 
> On Fri, Feb 21, 2014 at 03:34:43AM +0000, Sharma, Shashank wrote:
> > Hi Ville/All,
> > 
> > We gave a presentation on design on this framework, few months ago, in one of our common forum with OTC folks. 
> > We  discussed, took review comments, and re-designed the framework, as per the feedbacks. 
> 
> Apparently I wasn't there. And in any case it would be better to discuss it on dri-devel where people outside Intel can give their opinion.
> 
> > 
> > We also discussed the benefits of providing the controls directly from /sysfs over going for a UI manager based property settings.
> > So I don't understand where are we going wrong, can you please elaborate a bit ? 
> 
> The most important issue from my POV is that it can't be part of the atomic modeset.
> 
> Another issue is that it make the whole API inconsistent. Some stuff through ioctl, some stuff through sysfs, some stuff through whatever the next guy thinks of. It's not pretty. I've worked in the past with a driver where I had to poke at various standardish ioctls, custom ioctls, and sysfs to make it do anything useful, and I have no interest in repeating that experience. sysfs is especially painful since you have do the string<->binary conversions all over the place, and also you en up doing open+read/write+close cycles for every little thing.
> 
> It also adds more entrypoints into the driver for us to worry about.
> That means extra worries about the power management stuff and locking at the very least.
> 
> Also the rules of sysfs say "one item per file". The only allowed exception to this rule I know of is hardware provided blobs (like EDID, PCI ROM etc.). Your current implementation breaks this rule blatantly.
> 
> > 
> > This is just a basic design, and once go ahead with this, we can always work on making hardware agnostic, as you recommended.  
> > 
> > IMHO, controls from /sysfs would be a very generic interface for all linux/drm based platform, where any userspace can read/write and control properties. 
> > We don't even need a UI manager or a minimum executable to play 
> > around, just a small script can do. But we can always write something on top of this, to be included in any UI framework or property.
> 
> If there's a real need to get at properties through sysfs, then we could think about exposing them all. But that may presents some issues where the current master suddenly gets confused about its state since someone else went behind its back and changed a bunch of stuff.
> 
> >  
> > Regards
> > Shashank    
> > 
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > Sent: Thursday, February 20, 2014 6:41 PM
> > To: Sharma, Shashank
> > Cc: intel-gfx@lists.freedesktop.org; Shankar, Uma; 
> > dri-devel@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 0/6] Intel Color Manager Framework
> > 
> > On Thu, Feb 20, 2014 at 06:07:21PM +0530, Shashank Sharma wrote:
> > > Color manager is a new framework in i915 driver, which provides a 
> > > unified interface for various color correction methods supported 
> > > by intel hardwares. The high level overview of this change is:
> > 
> > Would have been good to discuss this idea before implementing it. The plan is to use kms properties for this kind of stuff which allows us to hook it up with the upcoming atomic modeset API. Just yesterday there was some discussion on #dri-devel about exposing user settable blob properties even before the atomic modeset API lands (it was always the plan for the atomic modeset API anyway). So based on a cursory glance, this looks like it's going in the wrong direction.
> > 
> > Also ideally the properties should be hardware agnostic, so a generic userspace could use them regardless of the hardware/driver. Obviously that might not be possible in all cases, but we should at least spend a bit of effort on trying to make that happen for most properties.
> > 
> > --
> > Ville Syrjälä
> > Intel OTC
> 
> --
> Ville Syrjälä
> Intel OTC

--
Ville Syrjälä
Intel OTC

Re: [PATCH 0/6] Intel Color Manager Framework

[Stéphane Marchesin]

[-- Attachment #1.1: Type: text/plain, Size: 3422 bytes --]

On Fri, Feb 21, 2014 at 7:49 PM, Sharma, Shashank <shashank.sharma@intel.com
> wrote:

>    On Thu, Feb 20, 2014 at 5:11 AM, Ville Syrjälä <
> ville.syrjala@linux.intel.com> wrote:
>
> On Thu, Feb 20, 2014 at 06:07:21PM +0530, Shashank Sharma wrote:
> >> Color manager is a new framework in i915 driver, which provides
> >> a unified interface for various color correction methods supported
> >> by intel hardwares. The high level overview of this change is:
>
> >Would have been good to discuss this idea before implementing it. The
> >plan is to use kms properties for this kind of stuff which allows us
> >to hook it up with the upcoming atomic modeset API. Just yesterday there
> >was some discussion on #dri-devel about exposing user settable blob
> >properties even before the atomic modeset API lands (it was always the
> >plan for the atomic modeset API anyway). So based on a cursory glance,
> >this looks like it's going in the wrong direction.
>
>
>
> +1. We'e looking into hooking up color correction controls, and if the
> interface isn't standard our user space won't be portable across drivers.
> There are multiple reasons for using drm properties:
>
> - the KMS interface already provides a way to set the gamma ramp, which
> this code seems to replicate.
>
>
>
> The current KMS interface just initializes the gamma soft LUT palette
> registers, in 8 bit mode corresponding to unit gamma. It's impossible to
> apply accurate values corresponding to gamma=2.2 or 1.5 from KMS
>
> Because for that we need to program palette registers in 10.6 bit mode of
> hardware.
>

Then the existing interface should be extended. Otherwise you have two ways
to do the same thing...



>   >- the KMS interface allows us to name properties independently and
> enumerate them. It seems like right now you can't enumerate properties or
> guess what "property 0" is. I'd rather set the "Color conversion matrix"
> than remember to set >"property 0" (and even then, I'm not really sure it
> exists).
>
>
>
> All the properties are getting enumerated in color manager register
> function. The framework defines proper identifiers and mapping for each
> property, and every property is having a corresponding soft-lut to be
> loaded with correction values.
>

Correct me if I'm wrong, but I don't see a way for user space to query the
presence/absence of a given property. KMS allows that.



>
>
> - you can reuse the get/set infrastructure which is already in place
>
>
>
>
>
> >Another thing that came out of the discussion on irc is that we should
> standardize the properties. For example we could use a text file describing
> the name of the controls and the format of the data (something similar to
> the device tree >bindings). That way user space can expect "color
> conversion matrix" to mean the same thing everywhere, to get the same data
> as input, and to work the same way on all platforms.
>
> If you can please have a look on the header file, we are almost doing the
> same thing, in form of a protocol.
>

This protocol however is not extensible. With the KMS interface I can
already do the following from user space:
- query the existence of a given property
- set each property in a portable fashion (for example the same gamma ramp
code works on all DRM drivers)
- easily match properties to a given crtc

Stéphane

[-- Attachment #1.2: Type: text/html, Size: 7067 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/6] Intel Color Manager Framework

[Sharma, Shashank]

[-- Attachment #1.1: Type: text/plain, Size: 3244 bytes --]

Thanks for your comments Stéphane
Please find mine inline.


In general, I got the overall recommendations that if this implementation comes from generic DRM property, it would be easy to club with general interfaces, and atomic modeset calls.
I will work on this, and will come back with modified patches.

Regards
Shashank
From: Stéphane Marchesin [mailto:stephane.marchesin@gmail.com]
Sent: Monday, February 24, 2014 9:35 AM
To: Sharma, Shashank
Cc: Ville Syrjälä; Intel Graphics Development; Shankar, Uma; dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 0/6] Intel Color Manager Framework

>+1. We'e looking into hooking up color correction controls, and if the interface isn't standard our user space won't be portable across drivers. There are multiple reasons for using drm properties:
>- the KMS interface already provides a way to set the gamma ramp, which this code seems to replicate.
 >The current KMS interface just initializes the gamma soft LUT palette registers, in 8 bit mode corresponding to unit gamma. It's impossible to apply accurate values corresponding to gamma=2.2 or 1.5 from KMS
>Because for that we need to program palette registers in 10.6 bit mode of hardware.
>Then the existing interface should be extended. Otherwise you have two ways to do the same thing...

Agree.

>- the KMS interface allows us to name properties independently and enumerate them. It seems like right now you can't enumerate properties or guess what "property 0" is. I'd rather set the "Color conversion matrix" than remember to set >"property 0" (and even then, I'm not really sure it exists).

All the properties are getting enumerated in color manager register function. The framework defines proper identifiers and mapping for each property, and every property is having a corresponding soft-lut to be loaded with correction values.
Correct me if I'm wrong, but I don't see a way for user space to query the presence/absence of a given property. KMS allows that.
The color manager read function dumps the no of properties, and current status of the property. But I agree its better interface to have it in form of property, as far as the central control is concerned.

- you can reuse the get/set infrastructure which is already in place


>Another thing that came out of the discussion on irc is that we should standardize the properties. For example we could use a text file describing the name of the controls and the format of the data (something similar to the device tree >bindings). That way user space can expect "color conversion matrix" to mean the same thing everywhere, to get the same data as input, and to work the same way on all platforms.
If you can please have a look on the header file, we are almost doing the same thing, in form of a protocol.
>This protocol however is not extensible. With the KMS interface I can already do the following from user space:
>- query the existence of a given property
>- set each property in a portable fashion (for example the same gamma ramp code works on all DRM drivers)
>- easily match properties to a given crtc

Actually each of this is possible from color manager read/write, read dumps information per pipe basis.



[-- Attachment #1.2: Type: text/html, Size: 11362 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 0/6] Intel Color Manager Framework

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3285 bytes --]

On Fri, Feb 21, 2014 at 10:41:14AM -0500, Alex Deucher wrote:
> On Fri, Feb 21, 2014 at 9:46 AM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Feb 21, 2014 at 02:20:24PM +0000, Sharma, Shashank wrote:
> >> Hi Ville,
> >>
> >> Thanks for your time and comments.
> >> I can understand two basic problems what you see in this implementation:
> >>
> >> 1.  The most important issue from my POV is that it can't be part of the atomic modeset.
> >> 2.  it make the whole API inconsistent.
> >>
> >> I am not sure if its good to block all current implementation because we have thought something for this in atomic modeset.
> >> I think even in atomic modeset we need the core implementation like this, but the interface would be different, which might come in from of a DRM property.
> >> So at that time we can use this core implementation as it is, only the interfaces/framework needs to be changed.
> >>
> >> In this way we can always go ahead with a current implementation, and can just change the interfaces to fit in to the final interface like DRM property in atomic modeset.
> >> Or you can suggest us the expected interface, and we can work on modifying that as per expectation.
> >
> > The exptected interface will be range properties for stuff like
> > brightness, contrast etc. controls. There are already such things as
> > connector properties, but we're going to want something similar as
> > plane or crtc properties. One thing that worries me about such
> > properties though is whether we can make them hardware agnostic and
> > yet allow userspace precise control over the final image. That is, if we
> > map some fixed input range to a hardware specific output range, userspace
> > can't know how the actual output will change when the input changes. On
> > the other hand if the input is hardware specific, userspace can't know
> > what value to put in there to get the expected change on the output side.
> >
> > For bigger stuff like CSC matrices and gamma ramps we will want to use
> > some reasonably well defined blobs. Ie. the internal strucuture of the
> > blob has to be documented and it shouldn't contain more than necessary.
> > Ie. just the CSC matrix coefficients for one matrix, or just the entries
> > for a single gamma ramp. Again ideally we should make the blobs hardware
> > agnostic, but still allow precise control over the output data.
> >
> > I think this is going to involve first going over our hardware features,
> > trying to find the common patterns between different generations. If
> > there's a way to make something that works across the board for us, or
> > at least across a wide range, then we should also ask for some input on
> > dri-devel whether the proposed property would work for other people. We
> > may need to define new property types to more precisely define what the
> > value of the property actually means.
> >
> 
> Our hardware has similar features, so I'm sure there will be quite a
> bit of common ground.  I also vote for properties.

Thirded. Tegra should be able to use a hardware-agnostic description of
these as well. I wonder if perhaps VESA or some other standard already
defines such a format for some of these properties.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel