[PATCH 00/23] Color Management for DRM

[PATCH 00/23] Color Management for DRM

[Shashank Sharma]

This patch set adds Color Manager implementation in DRM layer. Color Manager
is an extension in DRM framework to support color correction/enhancement.

Various Hardware platforms can support several color correction capabilities.
Color Manager provides abstraction of these capabilities and allows a user
space UI agent to correct/enhance the display using the DRM property interface.

How is this going to work?
==========================
1. This patch series adds a few new properties in DRM framework. These properties are:
        a. color_capabilities property (type blob)
        b. Color Transformation Matrix property for corrections like CSC (called CTM, type blob)
        c. Palette correction properties for corrections like gamma fixup (called palette_correction, type blob) 
2. Also, this patch series adds few structures to indicate specifications of a property like size, no_of_samples for correction etc.
3. These properties are present in mode_config.
4. When the platform's display driver loads, it fills up the values of color_capabilities property using the standard structures (added in step 2).
   For example, Intel's I915 driver adds following color correction capabilities:
        a. gamma correction capability as palette correction property, with 257 correction coefficients and a max/min value
        b. csc correction capability as CTM correction property, with 3x3 transformation matrix values and max/min values 
5. Now when userspace comes up, it queries the platform's color capabilities by doing a get_property() on color_capabilities DRM property
6. Reading the blob, the userspace understands the color capabilities of the platform.
   For example, userspace will understand it can support:
        a. palette_correction with 257 coefficients
        b. CSC correction with 3x3 = 9 values 
7. To set color correction values, userspace:
        a. creates a blob using the create_blob_ioctl in standard palette_correction structure format, with the correction values
        b. calls the set_property_ioctl with the blob_id as value for the property 
8. Driver refers to the blob, gets the correction values and applies the correction in HW.
9. To get currently applied color correction values, userspace:
        a. calls a get_property_ioctl on that color property
        b. gets the blob_id for the currently applied correction from DRM infrastructure
        c. gets the blob using get_blob_ioctl and hence the currently applied values

That's all! :)

About the patch series:
=======================
The patch series first adds the color management support in DRM layer.
Then it adds the color management framework in I915 layer. 
After that, it implements platform specific core color correction functios. 

Intel color manager registers color correction with DRM color manager in this way:
 - CSC transformation is registered as CTM DRM property
 - Gamma correction is registered as palette_after_ctm DRM property
 - Degamma correction is registered as palette_before_ctm DRM property

Our thanks to all the reviewers who have given valuable comments in terms
of design and implementation to our previous sets of patches. Special mention
of thanks should go to Matt Roper for all his inputs/suggestions in implementation
of this module, using DRM atomic CRTC commit path.

V2: Worked on review comments from Matt, Jim, Thierry, Rob.
V3: Worked on review comments from Matt, Jim, Rob:
 - Jim, Rob:
   ========
   Re-arranged the whole patch series in the sequence of features, currently:
   First 5 patches add color management support in DRM layer
   Next 7 patches add Intel color management framework in I915 driver
   Next 5 patches add color correction for CHV (gamma, degamma and CSC)
   Next 2 patches enable color management, by attaching the properties to CRTC(Matt)
   Next 4 patches add color correction for BDW (gamma, degamma)
 - Matt:
   =====
   Patch 3: Added refernce/unreference for blob
   Patch 7: return -EINVAL and added debug message
   Patch 8: check for valid blob, from create blob
            moved call to intel_crtc_attach_color_prop in the end of full implementation (CHV)
   Patch 9: DRM_ERROR->DRM_DEBUG for NULL blob case
   Patch 13: Added static for internal functions
   Patch 20-24: renamed gen9_* functions to bdw_*
   Added new variables in device_info structure num_samples_after_ctm and num_samples_before_ctm
   Added new function in patch 8 to load capabilities based on device_info across all platforms

Shashank Sharma (23):
  drm: Create Color Management DRM properties
  drm: Add structure for querying palette color capabilities
  drm: Add color correction blobs in CRTC state
  drm: Add drm structures for palette color property
  drm: Add structure to set/get a CTM color property
  drm/i915: Add atomic set property interface for CRTC
  drm/i915: Add atomic get property interface for CRTC
  drm/i915: Create color management files
  drm/i915: Register pipe color capabilities
  drm/i915: Add gamma correction handlers
  drm/i915: Add pipe deGamma correction handlers
  drm/i915: Add pipe CSC correction handlers
  drm/i915: CHV: Load gamma color correction values
  drm/i915: CHV: Pipe level Gamma correction
  drm/i915: CHV: Load degamma color correction values
  drm/i915: CHV: Pipe level degamma correction
  drm/i915: CHV: Pipe level CSC correction
  drm/i915: Commit color changes to CRTC
  drm/i915: Attach color properties to CRTC
  drm/i915: BDW: Load gamma correction values
  drm/i915: BDW: Pipe level Gamma correction
  drm/i915: BDW: Load degamma correction values
  drm/i915: BDW: Pipe level degamma correction

 drivers/gpu/drm/drm_atomic_helper.c        |  12 +
 drivers/gpu/drm/drm_crtc.c                 |  26 +
 drivers/gpu/drm/i915/Makefile              |   3 +-
 drivers/gpu/drm/i915/i915_drv.c            |  17 +
 drivers/gpu/drm/i915/i915_drv.h            |   2 +
 drivers/gpu/drm/i915/i915_reg.h            |  41 +-
 drivers/gpu/drm/i915/intel_atomic.c        |  56 ++
 drivers/gpu/drm/i915/intel_color_manager.c | 852 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_color_manager.h |  99 ++++
 drivers/gpu/drm/i915/intel_display.c       |   6 +
 drivers/gpu/drm/i915/intel_drv.h           |  22 +
 include/drm/drm_crtc.h                     |  11 +
 include/uapi/drm/drm.h                     |  50 ++
 13 files changed, 1195 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_color_manager.c
 create mode 100644 drivers/gpu/drm/i915/intel_color_manager.h

-- 
1.9.1

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

[PATCH 01/23] drm: Create Color Management DRM properties

[Shashank Sharma]

From: Kausal Malladi <kausalmalladi@gmail.com>

Color Management is an extension to Kernel display framework. It allows
abstraction of hardware color correction and enhancement capabilities by
virtue of DRM properties.

This patch initializes color management framework by :
1. Introducing new pointers in DRM mode_config structure to
   carry CTM and Palette color correction properties.
2. Creating these DRM properties in DRM standard properties creation
   sequence.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
---
 drivers/gpu/drm/drm_crtc.c | 26 ++++++++++++++++++++++++++
 include/drm/drm_crtc.h     |  6 ++++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 9b9c4b4..d809c67 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1472,6 +1472,32 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
 		return -ENOMEM;
 	dev->mode_config.prop_mode_id = prop;
 
+	/* Color Management properties */
+	prop = drm_property_create(dev,
+			DRM_MODE_PROP_BLOB | DRM_MODE_PROP_IMMUTABLE,
+			"CRTC_PALETTE_CAPABILITIES", 0);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.cm_crtc_palette_capabilities_property = prop;
+
+	prop = drm_property_create(dev,
+			DRM_MODE_PROP_BLOB, "PALETTE_AFTER_CTM", 0);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.cm_palette_after_ctm_property = prop;
+
+	prop = drm_property_create(dev,
+			DRM_MODE_PROP_BLOB, "PALETTE_BEFORE_CTM", 0);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.cm_palette_before_ctm_property = prop;
+
+	prop = drm_property_create(dev,
+			DRM_MODE_PROP_BLOB, "CTM", 0);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.cm_ctm_property = prop;
+
 	return 0;
 }
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index c0366e9..c35531e 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1153,6 +1153,12 @@ struct drm_mode_config {
 	struct drm_property *suggested_x_property;
 	struct drm_property *suggested_y_property;
 
+	/* Color Management Properties */
+	struct drm_property *cm_crtc_palette_capabilities_property;
+	struct drm_property *cm_palette_before_ctm_property;
+	struct drm_property *cm_palette_after_ctm_property;
+	struct drm_property *cm_ctm_property;
+
 	/* dumb ioctl parameters */
 	uint32_t preferred_depth, prefer_shadow;
 
-- 
1.9.1

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

[PATCH 02/23] drm: Add structure for querying palette color capabilities

[Shashank Sharma]

From: Kausal Malladi <kausalmalladi@gmail.com>

The DRM color management framework is targeting various hardware
platforms and drivers. Different platforms can have different color
correction and enhancement capabilities.

A commom user space application can query these capabilities using the
DRM property interface. Each driver can fill this property with its
platform's palette color capabilities.

This patch adds new structure in DRM layer for querying palette color
capabilities. This structure will be used by all user space
agents to configure appropriate color configurations.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
---
 include/uapi/drm/drm.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 3801584..e3c642f 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -829,6 +829,17 @@ struct drm_event_vblank {
 	__u32 reserved;
 };
 
+struct drm_palette_caps {
+	/* Structure version. Should be 1 currently */
+	__u32 version;
+	/* For padding and future use */
+	__u32 reserved;
+	/* This may be 0 if not supported. e.g. plane palette or VLV pipe */
+	__u32 num_samples_before_ctm;
+	/* This will be non-zero for pipe. May be zero for planes on some HW */
+	__u32 num_samples_after_ctm;
+};
+
 /* typedef area */
 #ifndef __KERNEL__
 typedef struct drm_clip_rect drm_clip_rect_t;
-- 
1.9.1

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

[PATCH 03/23] drm: Add color correction blobs in CRTC state

[Shashank Sharma]

From: Kausal Malladi <kausalmalladi@gmail.com>

This patch adds new variables in CRTC state, to hold respective color
correction blobs. These blobs will be required during the atomic commit
for writing the color correction values in correction registers.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 12 ++++++++++++
 include/drm/drm_crtc.h              |  5 +++++
 2 files changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 94d6c8e..1fe91ee 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2168,6 +2168,12 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
 
 	if (state->mode_blob)
 		drm_property_reference_blob(state->mode_blob);
+	if (state->ctm_blob)
+		drm_property_reference_blob(state->ctm_blob);
+	if (state->palette_after_ctm_blob)
+		drm_property_reference_blob(state->palette_after_ctm_blob);
+	if (state->palette_before_ctm_blob)
+		drm_property_reference_blob(state->palette_before_ctm_blob);
 	state->mode_changed = false;
 	state->active_changed = false;
 	state->planes_changed = false;
@@ -2213,6 +2219,12 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc,
 {
 	if (state->mode_blob)
 		drm_property_unreference_blob(state->mode_blob);
+	if (state->ctm_blob)
+		drm_property_unreference_blob(state->ctm_blob);
+	if (state->palette_after_ctm_blob)
+		drm_property_unreference_blob(state->palette_after_ctm_blob);
+	if (state->palette_before_ctm_blob)
+		drm_property_unreference_blob(state->palette_before_ctm_blob);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index c35531e..fcbe682 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -302,6 +302,11 @@ struct drm_crtc_state {
 	/* blob property to expose current mode to atomic userspace */
 	struct drm_property_blob *mode_blob;
 
+	/* blob properties to hold the color properties' blobs */
+	struct drm_property_blob *palette_before_ctm_blob;
+	struct drm_property_blob *palette_after_ctm_blob;
+	struct drm_property_blob *ctm_blob;
+
 	struct drm_pending_vblank_event *event;
 
 	struct drm_atomic_state *state;
-- 
1.9.1

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

[PATCH 04/23] drm: Add drm structures for palette color property

[Shashank Sharma]

From: Kausal Malladi <kausalmalladi@gmail.com>

This patch adds new structures in DRM layer for Palette color
correction.These structures will be used by user space agents
to configure appropriate number of samples and Palette LUT for
a platform.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
---
 include/uapi/drm/drm.h | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index e3c642f..f72b916 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -840,6 +840,33 @@ struct drm_palette_caps {
 	__u32 num_samples_after_ctm;
 };
 
+struct drm_r32g32b32 {
+	/*
+	 * Data is in U8.24 fixed point format.
+	 * All platforms support values within [0, 1.0] range,
+	 * for Red, Green and Blue colors.
+	 */
+	__u32 r32;
+	__u32 g32;
+	__u32 b32;
+};
+
+struct drm_palette {
+	/* Structure version. Should be 1 currently */
+	__u32 version;
+	/*
+	 * This has to be a supported value during get call.
+	 * Feature will be disabled if this is 0 while set
+	 */
+	__u32 num_samples;
+	/*
+	 * Starting of palette LUT in R32G32B32 format.
+	 * Each of RGB value is in U8.24 fixed point format.
+	 * Actual number of samples will depend upon num_samples
+	 */
+	struct drm_r32g32b32 lut[0];
+};
+
 /* typedef area */
 #ifndef __KERNEL__
 typedef struct drm_clip_rect drm_clip_rect_t;
-- 
1.9.1

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

[PATCH 05/23] drm: Add structure to set/get a CTM color property

[Shashank Sharma]

From: Kausal Malladi <kausalmalladi@gmail.com>

Color Manager framework defines a color correction property for color
space transformation and Gamut mapping. This property is called CTM (Color
Transformation Matrix).

This patch adds a new structure in DRM layer for CTM.
This structure can be used by all user space agents to
configure CTM coefficients for color correction.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
---
 include/uapi/drm/drm.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index f72b916..9580772 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -867,6 +867,18 @@ struct drm_palette {
 	struct drm_r32g32b32 lut[0];
 };
 
+struct drm_ctm {
+	/* Structure version. Should be 1 currently */
+	__u32 version;
+	/*
+	 * Each value is in S31.32 format.
+	 * This is 3x3 matrix in row major format.
+	 * Integer part will be clipped to nearest
+	 * max/min boundary as supported by the HW platform.
+	 */
+	__s64 ctm_coeff[9];
+};
+
 /* typedef area */
 #ifndef __KERNEL__
 typedef struct drm_clip_rect drm_clip_rect_t;
-- 
1.9.1

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

[PATCH 06/23] drm/i915: Add atomic set property interface for CRTC

[Shashank Sharma]

From: Kausal Malladi <kausalmalladi@gmail.com>

This patch adds atomic set property interface for Intel CRTC. This
interface will be used for set operation on any DRM properties.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
---
 drivers/gpu/drm/i915/intel_atomic.c  | 9 +++++++++
 drivers/gpu/drm/i915/intel_display.c | 2 ++
 drivers/gpu/drm/i915/intel_drv.h     | 4 ++++
 3 files changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index f1975f2..d5b7eb1 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -309,3 +309,12 @@ void intel_atomic_state_clear(struct drm_atomic_state *s)
 	drm_atomic_state_default_clear(&state->base);
 	state->dpll_set = false;
 }
+
+int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
+				   struct drm_crtc_state *state,
+				   struct drm_property *property,
+				   uint64_t val)
+{
+	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
+	return -EINVAL;
+}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fc00867..444ea30 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13192,6 +13192,8 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
 	.set_config = drm_atomic_helper_set_config,
 	.destroy = intel_crtc_destroy,
 	.page_flip = intel_crtc_page_flip,
+	.set_property = drm_atomic_helper_crtc_set_property,
+	.atomic_set_property = intel_crtc_atomic_set_property,
 	.atomic_duplicate_state = intel_crtc_duplicate_state,
 	.atomic_destroy_state = intel_crtc_destroy_state,
 };
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 02a755a..510f559 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1450,6 +1450,10 @@ intel_atomic_get_crtc_state(struct drm_atomic_state *state,
 int intel_atomic_setup_scalers(struct drm_device *dev,
 	struct intel_crtc *intel_crtc,
 	struct intel_crtc_state *crtc_state);
+int intel_crtc_atomic_set_property(struct drm_crtc *plane,
+				   struct drm_crtc_state *state,
+				   struct drm_property *property,
+				   uint64_t val);
 
 /* intel_atomic_plane.c */
 struct intel_plane_state *intel_create_plane_state(struct drm_plane *plane);
-- 
1.9.1

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

[PATCH 07/23] drm/i915: Add atomic get property interface for CRTC

[Shashank Sharma]

From: Kausal Malladi <kausalmalladi@gmail.com>

This patch adds atomic get property interface for Intel CRTC. This
interface will be used for get operation on any non-core DRM properties.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
---
 drivers/gpu/drm/i915/intel_atomic.c  | 9 +++++++++
 drivers/gpu/drm/i915/intel_display.c | 1 +
 drivers/gpu/drm/i915/intel_drv.h     | 4 ++++
 3 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index d5b7eb1..500d2998 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -318,3 +318,12 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
 	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
 	return -EINVAL;
 }
+
+int intel_crtc_atomic_get_property(struct drm_crtc *crtc,
+				   const struct drm_crtc_state *state,
+				   struct drm_property *property,
+				   uint64_t *val)
+{
+	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
+	return -EINVAL;
+}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 444ea30..010acca 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13194,6 +13194,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
 	.page_flip = intel_crtc_page_flip,
 	.set_property = drm_atomic_helper_crtc_set_property,
 	.atomic_set_property = intel_crtc_atomic_set_property,
+	.atomic_get_property = intel_crtc_atomic_get_property,
 	.atomic_duplicate_state = intel_crtc_duplicate_state,
 	.atomic_destroy_state = intel_crtc_destroy_state,
 };
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 510f559..e27e754 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1454,6 +1454,10 @@ int intel_crtc_atomic_set_property(struct drm_crtc *plane,
 				   struct drm_crtc_state *state,
 				   struct drm_property *property,
 				   uint64_t val);
+int intel_crtc_atomic_get_property(struct drm_crtc *plane,
+				   const struct drm_crtc_state *state,
+				   struct drm_property *property,
+				   uint64_t *val);
 
 /* intel_atomic_plane.c */
 struct intel_plane_state *intel_create_plane_state(struct drm_plane *plane);
-- 
1.9.1

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

[PATCH 08/23] drm/i915: Create color management files

[Shashank Sharma]

From: Kausal Malladi <kausalmalladi@gmail.com>

This patch create new files intel_color_manager.c which
will contain the core color correction code for I915 driver
and its header intel_color_manager.h

The per color property patches coming up in this patch series
will fill the appropriate functions in this file.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
---
 drivers/gpu/drm/i915/Makefile              |  3 ++-
 drivers/gpu/drm/i915/intel_color_manager.c | 33 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_color_manager.h | 29 ++++++++++++++++++++++++++
 3 files changed, 64 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/i915/intel_color_manager.c
 create mode 100644 drivers/gpu/drm/i915/intel_color_manager.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 44d290a..56caf9e 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -64,7 +64,8 @@ i915-y += intel_audio.o \
 	  intel_overlay.o \
 	  intel_psr.o \
 	  intel_sideband.o \
-	  intel_sprite.o
+	  intel_sprite.o \
+	  intel_color_manager.o
 i915-$(CONFIG_ACPI)		+= intel_acpi.o intel_opregion.o
 i915-$(CONFIG_DRM_FBDEV_EMULATION)	+= intel_fbdev.o
 
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
new file mode 100644
index 0000000..7357d99
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -0,0 +1,33 @@
+/*
+ * Copyright © 2015 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>
+ * Kausal Malladi <Kausal.Malladi@intel.com>
+ */
+
+#include "intel_color_manager.h"
+
+void intel_attach_color_properties_to_crtc(struct drm_device *dev,
+		struct drm_mode_object *mode_obj)
+{
+}
diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
new file mode 100644
index 0000000..04c921d
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_color_manager.h
@@ -0,0 +1,29 @@
+/*
+ * Copyright © 2015 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>
+ * Kausal Malladi <Kausal.Malladi@intel.com>
+ */
+#include <drm/drmP.h>
+#include <drm/drm_crtc_helper.h>
+#include "i915_drv.h"
-- 
1.9.1

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

[PATCH 09/23] drm/i915: Register pipe color capabilities

[Shashank Sharma]

DRM color manager contains these color properties:

1. "crtc_palette_capabilities_property": to allow a
core driver to load and showcase its color correction
capabilities to user space.
2. "ctm": Color transformation matrix property, where a
color transformation matrix of 9 correction values gets
applied as correction.
3. "palette_before_ctm": for corrections which get applied
beore color transformation matrix correction.
4. "palette_after_ctm": for corrections which get applied
after color transformation matrix correction.

Intel color manager registers:
1. Gamma correction property as "palette_after_ctm" property
2. Degamma correction capability as "palette_bafore_ctm" property
capability as "palette_after_ctm" DRM color property hook.
3. CSC as "ctm" property.

This patch does the following:
1. Add a function which loads the platform's color correction
capabilities in the cm_crtc_palette_capabilities_property structure.
2. Attaches the cm_crtc_palette_capabilities_property to every CRTC
getting initiaized.
3. Adds two new parameters "num_samples_after_ctm" and
"num_samples_before_ctm" in intel_device_info as gamma and
degamma coefficients vary per platform basis.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h            |  2 ++
 drivers/gpu/drm/i915/intel_color_manager.c | 45 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_color_manager.h |  2 ++
 3 files changed, 49 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3bf8a9b..22de2cb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -798,6 +798,8 @@ struct intel_device_info {
 	u8 num_sprites[I915_MAX_PIPES];
 	u8 gen;
 	u8 ring_mask; /* Rings supported by the HW */
+	u16 num_samples_after_ctm;
+	u16 num_samples_before_ctm;
 	DEV_INFO_FOR_EACH_FLAG(DEFINE_FLAG, SEP_SEMICOLON);
 	/* Register offsets for the various display pipes and transcoders */
 	int pipe_offsets[I915_MAX_TRANSCODERS];
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
index 7357d99..77f58f2 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.c
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -27,7 +27,52 @@
 
 #include "intel_color_manager.h"
 
+int get_pipe_capabilities(struct drm_device *dev,
+		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
+{
+	struct drm_property_blob *blob;
+
+	/*
+	 * This function loads best capability for gamma correction
+	 * For example:
+	 * CHV best Gamma correction (CGM unit, 10 bit)
+	 * has 257 entries, best degamma is 65 entries
+	 */
+	palette_caps->version = COLOR_STRUCT_VERSION;
+	palette_caps->num_samples_after_ctm =
+				INTEL_INFO(dev)->num_samples_after_ctm;
+	palette_caps->num_samples_before_ctm =
+				INTEL_INFO(dev)->num_samples_before_ctm;
+	blob = drm_property_create_blob(dev, sizeof(struct drm_palette_caps),
+			(const void *) palette_caps);
+	if (IS_ERR_OR_NULL(blob)) {
+		DRM_ERROR("Create blob for capabilities failed\n");
+		return PTR_ERR(blob);
+	}
+
+	return blob->base.id;
+}
+
 void intel_attach_color_properties_to_crtc(struct drm_device *dev,
 		struct drm_mode_object *mode_obj)
 {
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_palette_caps *palette_caps;
+	struct drm_crtc *crtc;
+	int capabilities_blob_id;
+
+	crtc = obj_to_crtc(mode_obj);
+	palette_caps = kzalloc(sizeof(struct drm_palette_caps),
+			GFP_KERNEL);
+	capabilities_blob_id = get_pipe_capabilities(dev,
+						palette_caps, crtc);
+
+	if (config->cm_crtc_palette_capabilities_property) {
+		drm_object_attach_property(mode_obj,
+			config->cm_crtc_palette_capabilities_property,
+			capabilities_blob_id);
+		DRM_DEBUG_DRIVER("Capabilities attached to CRTC\n");
+	}
+
+	kfree(palette_caps);
 }
diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
index 04c921d..1a42244 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.h
+++ b/drivers/gpu/drm/i915/intel_color_manager.h
@@ -27,3 +27,5 @@
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
 #include "i915_drv.h"
+
+#define COLOR_STRUCT_VERSION			1
-- 
1.9.1

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

[PATCH 10/23] drm/i915: Add gamma correction handlers

[Shashank Sharma]

I915 driver registers gamma correction as palette correction
property with DRM layer. This patch adds set_property() and get_property()
handlers for pipe level gamma correction.

The set function attaches the Gamma correction blob to CRTC state, these
values will be committed during atomic commit.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
---
 drivers/gpu/drm/i915/intel_atomic.c        | 20 ++++++++++++++++++++
 drivers/gpu/drm/i915/intel_color_manager.c | 21 +++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h           |  5 +++++
 3 files changed, 46 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 500d2998..0b61fef 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -315,6 +315,13 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
 				   struct drm_property *property,
 				   uint64_t val)
 {
+	struct drm_device *dev = crtc->dev;
+	struct drm_mode_config *config = &dev->mode_config;
+
+	if (property == config->cm_palette_after_ctm_property)
+		return intel_color_manager_set_pipe_gamma(dev, state,
+				&crtc->base, val);
+
 	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
 	return -EINVAL;
 }
@@ -324,6 +331,19 @@ int intel_crtc_atomic_get_property(struct drm_crtc *crtc,
 				   struct drm_property *property,
 				   uint64_t *val)
 {
+	struct drm_device *dev = crtc->dev;
+	struct drm_mode_config *config = &dev->mode_config;
+
+	if (property == config->cm_palette_after_ctm_property) {
+		*val = (state->palette_after_ctm_blob) ?
+			state->palette_after_ctm_blob->base.id : 0;
+		goto found;
+	}
+
 	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
 	return -EINVAL;
+
+found:
+	DRM_DEBUG_KMS("Found property %s\n", property->name);
+	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
index 77f58f2..9421bb6 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.c
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -27,6 +27,27 @@
 
 #include "intel_color_manager.h"
 
+int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
+		struct drm_crtc_state *crtc_state,
+		struct drm_mode_object *obj, uint32_t blob_id)
+{
+	struct drm_property_blob *blob;
+
+	blob = drm_property_lookup_blob(dev, blob_id);
+	if (!blob) {
+		DRM_DEBUG_KMS("Invalid Blob ID\n");
+		return -EINVAL;
+	}
+
+	if (crtc_state->palette_after_ctm_blob)
+		drm_property_unreference_blob(
+			crtc_state->palette_after_ctm_blob);
+
+	/* Attach the blob to be committed in state */
+	crtc_state->palette_after_ctm_blob = blob;
+	return 0;
+}
+
 int get_pipe_capabilities(struct drm_device *dev,
 		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e27e754..d0193e2 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1466,4 +1466,9 @@ void intel_plane_destroy_state(struct drm_plane *plane,
 			       struct drm_plane_state *state);
 extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
 
+/* intel_color_manager.c */
+int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
+		struct drm_crtc_state *crtc_state,
+		struct drm_mode_object *obj, uint32_t blob_id);
+
 #endif /* __INTEL_DRV_H__ */
-- 
1.9.1

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

[PATCH 11/23] drm/i915: Add pipe deGamma correction handlers

[Shashank Sharma]

This patch adds set_property and get_property handlers for deGamma
color correction capability at Pipe level.

Set function just attaches the deGamma correction blob to CRTC state, which
will be later committed in the atomic commit path.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
---
 drivers/gpu/drm/i915/intel_atomic.c        | 10 ++++++++++
 drivers/gpu/drm/i915/intel_color_manager.c | 20 ++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h           |  4 +++-
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 0b61fef..b504b4f 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -322,6 +322,10 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
 		return intel_color_manager_set_pipe_gamma(dev, state,
 				&crtc->base, val);
 
+	if (property == config->cm_palette_before_ctm_property)
+		return intel_color_manager_set_pipe_degamma(dev, state,
+				&crtc->base, val);
+
 	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
 	return -EINVAL;
 }
@@ -340,6 +344,12 @@ int intel_crtc_atomic_get_property(struct drm_crtc *crtc,
 		goto found;
 	}
 
+	if (property == config->cm_palette_before_ctm_property) {
+		*val = (state->palette_before_ctm_blob) ?
+			state->palette_before_ctm_blob->base.id : 0;
+		goto found;
+	}
+
 	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
 	return -EINVAL;
 
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
index 9421bb6..8890b09 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.c
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -27,6 +27,26 @@
 
 #include "intel_color_manager.h"
 
+int intel_color_manager_set_pipe_degamma(struct drm_device *dev,
+		struct drm_crtc_state *crtc_state,
+		struct drm_mode_object *obj, uint32_t blob_id)
+{
+	struct drm_property_blob *blob;
+
+	blob = drm_property_lookup_blob(dev, blob_id);
+	if (!blob) {
+		DRM_ERROR("Invalid Blob ID\n");
+		return -EINVAL;
+	}
+
+	if (crtc_state->palette_before_ctm_blob)
+		drm_property_unreference_blob(
+			crtc_state->palette_before_ctm_blob);
+
+	crtc_state->palette_before_ctm_blob = blob;
+	return 0;
+}
+
 int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
 		struct drm_crtc_state *crtc_state,
 		struct drm_mode_object *obj, uint32_t blob_id)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d0193e2..0298dd0 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1470,5 +1470,7 @@ extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
 int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
 		struct drm_crtc_state *crtc_state,
 		struct drm_mode_object *obj, uint32_t blob_id);
-
+int intel_color_manager_set_pipe_degamma(struct drm_device *dev,
+		struct drm_crtc_state *crtc_state,
+		struct drm_mode_object *obj, uint32_t blob_id);
 #endif /* __INTEL_DRV_H__ */
-- 
1.9.1

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

[PATCH 12/23] drm/i915: Add pipe CSC correction handlers

[Shashank Sharma]

This patch adds set_property and get_property handlers for pipe
level CSC color correction for CHV/BSW platform. The set
function just attaches the CSC blob to CRTC state, that later
gets committed using atomic path.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
---
 drivers/gpu/drm/i915/intel_atomic.c        |  8 ++++++++
 drivers/gpu/drm/i915/intel_color_manager.c | 19 +++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h           |  3 +++
 3 files changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index b504b4f..57eafca 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -325,6 +325,9 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
 	if (property == config->cm_palette_before_ctm_property)
 		return intel_color_manager_set_pipe_degamma(dev, state,
 				&crtc->base, val);
+	if (property == config->cm_ctm_property)
+		return intel_color_manager_set_pipe_csc(dev, state,
+				&crtc->base, val);
 
 	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
 	return -EINVAL;
@@ -350,6 +353,11 @@ int intel_crtc_atomic_get_property(struct drm_crtc *crtc,
 		goto found;
 	}
 
+	if (property == config->cm_ctm_property) {
+		*val = (state->ctm_blob) ? state->ctm_blob->base.id : 0;
+		goto found;
+	}
+
 	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
 	return -EINVAL;
 
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
index 8890b09..2ba3fd7 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.c
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -27,6 +27,25 @@
 
 #include "intel_color_manager.h"
 
+int intel_color_manager_set_pipe_csc(struct drm_device *dev,
+		struct drm_crtc_state *crtc_state,
+		struct drm_mode_object *obj, uint32_t blob_id)
+{
+	struct drm_property_blob *blob;
+
+	blob = drm_property_lookup_blob(dev, blob_id);
+	if (!blob) {
+		DRM_ERROR("Invalid Blob ID\n");
+		return -EINVAL;
+	}
+
+	if (crtc_state->ctm_blob)
+		drm_property_unreference_blob(crtc_state->ctm_blob);
+
+	crtc_state->ctm_blob = blob;
+	return 0;
+}
+
 int intel_color_manager_set_pipe_degamma(struct drm_device *dev,
 		struct drm_crtc_state *crtc_state,
 		struct drm_mode_object *obj, uint32_t blob_id)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0298dd0..4d571a2f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1473,4 +1473,7 @@ int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
 int intel_color_manager_set_pipe_degamma(struct drm_device *dev,
 		struct drm_crtc_state *crtc_state,
 		struct drm_mode_object *obj, uint32_t blob_id);
+int intel_color_manager_set_pipe_csc(struct drm_device *dev,
+		struct drm_crtc_state *crtc_state,
+		struct drm_mode_object *obj, uint32_t blob_id);
 #endif /* __INTEL_DRV_H__ */
-- 
1.9.1

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

[PATCH 13/23] drm/i915: CHV: Load gamma color correction values

[Shashank Sharma]

DRM color manager allows the driver to showcase its best color
correction capabilities using the cm_crtc_palette_capabilities_property.
Driver loads the no. of coefficients for various color correction
as per the platform, during the init time.

This patch adds no of coefficitents for best gamma color correction
modes possible in CHV, in device info structure, which is:
Gamma(10 bit, CGM HW unit):- 257 coeff
These values will be loaded in cm_crtc_palette_capabilities_property
during the CRTC init section, by color manager's attach function.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c            | 2 ++
 drivers/gpu/drm/i915/intel_color_manager.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index bdec64c..04a8e45 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -34,6 +34,7 @@
 #include "i915_drv.h"
 #include "i915_trace.h"
 #include "intel_drv.h"
+#include "intel_color_manager.h"
 
 #include <linux/console.h>
 #include <linux/module.h>
@@ -349,6 +350,7 @@ static const struct intel_device_info intel_cherryview_info = {
 	.gen = 8, .num_pipes = 3,
 	.need_gfx_hws = 1, .has_hotplug = 1,
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
+	.num_samples_after_ctm = CHV_10BIT_GAMMA_MAX_VALS,
 	.is_valleyview = 1,
 	.display_mmio_offset = VLV_DISPLAY_BASE,
 	GEN_CHV_PIPEOFFSETS,
diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
index 1a42244..99ef646 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.h
+++ b/drivers/gpu/drm/i915/intel_color_manager.h
@@ -29,3 +29,4 @@
 #include "i915_drv.h"
 
 #define COLOR_STRUCT_VERSION			1
+#define CHV_10BIT_GAMMA_MAX_VALS		257
-- 
1.9.1

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

[PATCH 14/23] drm/i915: CHV: Load degamma color correction values

[Shashank Sharma]

DRM color manager allows the driver to showcase its best color
correction capabilities using the cm_crtc_palette_capabilities_property.

This patch adds no of coefficitents for degamma color correction
modes possible in CHV, in device info structure, which is:
CGM Degamma(10 bit, CGM HW unit):- 65 coeff

These values will be loaded in cm_crtc_palette_capabilities_property
during the CRTC init section, by color manager's attach function.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c            | 1 +
 drivers/gpu/drm/i915/intel_color_manager.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 04a8e45..03db841 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -351,6 +351,7 @@ static const struct intel_device_info intel_cherryview_info = {
 	.need_gfx_hws = 1, .has_hotplug = 1,
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
 	.num_samples_after_ctm = CHV_10BIT_GAMMA_MAX_VALS,
+	.num_samples_before_ctm = CHV_DEGAMMA_MAX_VALS,
 	.is_valleyview = 1,
 	.display_mmio_offset = VLV_DISPLAY_BASE,
 	GEN_CHV_PIPEOFFSETS,
diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
index 99ef646..402b825 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.h
+++ b/drivers/gpu/drm/i915/intel_color_manager.h
@@ -30,3 +30,4 @@
 
 #define COLOR_STRUCT_VERSION			1
 #define CHV_10BIT_GAMMA_MAX_VALS		257
+#define CHV_DEGAMMA_MAX_VALS			65
-- 
1.9.1

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

[PATCH 15/23] drm/i915: CHV: Pipe level Gamma correction

[Shashank Sharma]

From: Kausal Malladi <kausalmalladi@gmail.com>

CHV/BSW platform supports two different pipe level gamma
correction modes, which are:
1. Legacy 8-bit mode
2. 10-bit CGM (Color Gamut Mapping) mode

This patch does the following:
1. Attaches Gamma property to CRTC
3. Adds the core Gamma correction function for CHV/BSW
4. Adds Gamma correction macros

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
---
 drivers/gpu/drm/i915/i915_reg.h            |  12 +++
 drivers/gpu/drm/i915/intel_color_manager.c | 127 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_color_manager.h |  21 +++++
 3 files changed, 160 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 67bf205..93aa1f9 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7977,4 +7977,16 @@ enum skl_disp_power_wells {
 #define GEN9_VEBOX_MOCS_0	0xcb00	/* Video MOCS base register*/
 #define GEN9_BLT_MOCS_0		0xcc00	/* Blitter MOCS base register*/
 
+/* Color Management */
+#define PIPEA_CGM_CONTROL			(VLV_DISPLAY_BASE + 0x67A00)
+#define PIPEB_CGM_CONTROL			(VLV_DISPLAY_BASE + 0x69A00)
+#define PIPEC_CGM_CONTROL			(VLV_DISPLAY_BASE + 0x6BA00)
+#define PIPEA_CGM_GAMMA			(VLV_DISPLAY_BASE + 0x67000)
+#define PIPEB_CGM_GAMMA			(VLV_DISPLAY_BASE + 0x69000)
+#define PIPEC_CGM_GAMMA			(VLV_DISPLAY_BASE + 0x6B000)
+#define _PIPE_CGM_CONTROL(pipe) \
+	(_PIPE3(pipe, PIPEA_CGM_CONTROL, PIPEB_CGM_CONTROL, PIPEC_CGM_CONTROL))
+#define _PIPE_GAMMA_BASE(pipe) \
+	(_PIPE3(pipe, PIPEA_CGM_GAMMA, PIPEB_CGM_GAMMA, PIPEC_CGM_GAMMA))
+
 #endif /* _I915_REG_H_ */
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
index 2ba3fd7..222924d 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.c
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -66,6 +66,127 @@ int intel_color_manager_set_pipe_degamma(struct drm_device *dev,
 	return 0;
 }
 
+static int chv_set_gamma(struct drm_device *dev, struct drm_property_blob *blob,
+		  struct drm_crtc *crtc)
+{
+	bool flag = false;
+	enum pipe pipe;
+	u8 red_int, blue_int, green_int;
+	u16 red_fract, green_fract, blue_fract;
+	u32 red, green, blue;
+	u32 cgm_control_reg = 0;
+	u32 cgm_gamma_reg = 0;
+	u32 count = 0, num_samples, word;
+	int ret = 0, length;
+	struct drm_r32g32b32 *correction_values = NULL;
+	struct drm_palette *gamma_data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (WARN_ON(!blob))
+		return -EINVAL;
+
+	gamma_data = (struct drm_palette *)blob->data;
+
+	if (gamma_data->version != GAMMA_DATA_STRUCT_VERSION) {
+		DRM_DEBUG_KMS("Invalid Gamma Data struct version\n");
+		return -EINVAL;
+	}
+
+	pipe = to_intel_crtc(crtc)->pipe;
+	num_samples = gamma_data->num_samples;
+	length = num_samples * sizeof(struct drm_r32g32b32);
+
+	switch (num_samples) {
+	case 0:
+
+		/* Disable Gamma functionality on Pipe - CGM Block */
+		cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe));
+		cgm_control_reg &= ~CGM_GAMMA_EN;
+		I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg);
+
+		DRM_DEBUG_DRIVER("Gamma disabled on Pipe %c\n",
+				pipe_name(pipe));
+		ret = 0;
+		break;
+
+	case CHV_8BIT_GAMMA_MAX_VALS:
+	case CHV_10BIT_GAMMA_MAX_VALS:
+
+		count = 0;
+		cgm_gamma_reg = _PIPE_GAMMA_BASE(pipe);
+		correction_values = (struct drm_r32g32b32 *)&gamma_data->lut;
+
+		while (count < num_samples) {
+			blue = correction_values[count].b32;
+			green = correction_values[count].g32;
+			red = correction_values[count].r32;
+
+			blue_int = _GAMMA_INT_PART(blue);
+			if (blue_int > GAMMA_INT_MAX)
+				blue = CHV_MAX_GAMMA;
+
+			green_int = _GAMMA_INT_PART(green);
+			if (green_int > GAMMA_INT_MAX)
+				green = CHV_MAX_GAMMA;
+
+			red_int = _GAMMA_INT_PART(red);
+			if (red_int > GAMMA_INT_MAX)
+				red = CHV_MAX_GAMMA;
+
+			blue_fract = _GAMMA_FRACT_PART(blue);
+			green_fract = _GAMMA_FRACT_PART(green);
+			red_fract = _GAMMA_FRACT_PART(red);
+
+			blue_fract >>= CHV_10BIT_GAMMA_MSB_SHIFT;
+			green_fract >>= CHV_10BIT_GAMMA_MSB_SHIFT;
+			red_fract >>= CHV_10BIT_GAMMA_MSB_SHIFT;
+
+			/* Green (25:16) and Blue (9:0) to be written */
+			word = green_fract;
+			word <<= CHV_GAMMA_SHIFT_GREEN;
+			word |= blue_fract;
+			I915_WRITE(cgm_gamma_reg, word);
+			cgm_gamma_reg += 4;
+
+			/* Red (9:0) to be written */
+			word = red_fract;
+			I915_WRITE(cgm_gamma_reg, word);
+
+			cgm_gamma_reg += 4;
+			count++;
+
+			/*
+			 * On CHV, the best supported Gamma capability is
+			 * CGM block, that takes 257 samples.
+			 * If user gives 256 samples (legacy mode), then
+			 * duplicate the 256th value to 257th.
+			 * "flag" is used to make sure it happens only once
+			 */
+			if (num_samples == CHV_8BIT_GAMMA_MAX_VALS
+					&& count == CHV_8BIT_GAMMA_MAX_VALS
+					&& !flag) {
+				count--;
+				flag = true;
+			}
+		}
+
+		/* Enable (CGM) Gamma on Pipe */
+		I915_WRITE(_PIPE_CGM_CONTROL(pipe),
+			I915_READ(_PIPE_CGM_CONTROL(pipe))
+				| CGM_GAMMA_EN);
+		DRM_DEBUG_DRIVER("CGM Gamma enabled on Pipe %c\n",
+				pipe_name(pipe));
+		ret = 0;
+		break;
+
+	default:
+		DRM_ERROR("Invalid number of samples for Gamma LUT\n");
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
 int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
 		struct drm_crtc_state *crtc_state,
 		struct drm_mode_object *obj, uint32_t blob_id)
@@ -134,5 +255,11 @@ void intel_attach_color_properties_to_crtc(struct drm_device *dev,
 		DRM_DEBUG_DRIVER("Capabilities attached to CRTC\n");
 	}
 
+	if (config->cm_palette_after_ctm_property) {
+		drm_object_attach_property(mode_obj,
+			config->cm_palette_after_ctm_property, 0);
+		DRM_DEBUG_DRIVER("palette after CTM attached to CRTC\n");
+	}
+
 	kfree(palette_caps);
 }
diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
index 402b825..5f800c9 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.h
+++ b/drivers/gpu/drm/i915/intel_color_manager.h
@@ -28,6 +28,27 @@
 #include <drm/drm_crtc_helper.h>
 #include "i915_drv.h"
 
+#define _GAMMA_INT_PART(value)			(value >> GAMMA_INT_SHIFT)
+#define _GAMMA_FRACT_PART(value)		(value >> GAMMA_FRACT_SHIFT)
+
 #define COLOR_STRUCT_VERSION			1
 #define CHV_10BIT_GAMMA_MAX_VALS		257
 #define CHV_DEGAMMA_MAX_VALS			65
+
+/* Gamma correction */
+#define GAMMA_DATA_STRUCT_VERSION		1
+#define CHV_10BIT_GAMMA_MAX_VALS		257
+#define CHV_8BIT_GAMMA_MAX_VALS		256
+#define CHV_10BIT_GAMMA_MSB_SHIFT		6
+#define CHV_GAMMA_SHIFT_GREEN			16
+
+/* Gamma values are u8.24 format */
+#define GAMMA_INT_SHIFT			24
+#define GAMMA_FRACT_SHIFT			8
+/* Max int value for Gamma is 1 */
+#define GAMMA_INT_MAX				1
+/* Max value for Gamma on CHV */
+#define CHV_MAX_GAMMA				0x10000
+
+/* CHV CGM Block */
+#define CGM_GAMMA_EN				(1 << 2)
-- 
1.9.1

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

[PATCH 16/23] drm/i915: CHV: Pipe level degamma correction

[Shashank Sharma]

CHV/BSW supports DeGamma color correction, which linearizes all
the non-linear color values. This will be applied before Color
Transformation.

This patch does the following:
1. Attach deGamma property to CRTC
2. Add the core function to program DeGamma correction values for
   CHV/BSW platform
2. Add DeGamma correction macros/defines

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
---
 drivers/gpu/drm/i915/i915_reg.h            |   6 ++
 drivers/gpu/drm/i915/intel_color_manager.c | 111 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_color_manager.h |   6 ++
 3 files changed, 123 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 93aa1f9..3dde6a8 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7989,4 +7989,10 @@ enum skl_disp_power_wells {
 #define _PIPE_GAMMA_BASE(pipe) \
 	(_PIPE3(pipe, PIPEA_CGM_GAMMA, PIPEB_CGM_GAMMA, PIPEC_CGM_GAMMA))
 
+#define PIPEA_CGM_DEGAMMA                      (VLV_DISPLAY_BASE + 0x66000)
+#define PIPEB_CGM_DEGAMMA                      (VLV_DISPLAY_BASE + 0x68000)
+#define PIPEC_CGM_DEGAMMA                      (VLV_DISPLAY_BASE + 0x6A000)
+#define _PIPE_DEGAMMA_BASE(pipe) \
+	(_PIPE3(pipe, PIPEA_CGM_DEGAMMA, PIPEB_CGM_DEGAMMA, PIPEC_CGM_DEGAMMA))
+
 #endif /* _I915_REG_H_ */
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
index 222924d..e2ffbdc 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.c
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -66,6 +66,111 @@ int intel_color_manager_set_pipe_degamma(struct drm_device *dev,
 	return 0;
 }
 
+static int chv_set_degamma(struct drm_device *dev,
+	struct drm_property_blob *blob, struct drm_crtc *crtc)
+{
+	struct drm_palette *degamma_data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 cgm_control_reg = 0;
+	u32 cgm_degamma_reg = 0;
+	enum pipe pipe;
+	u32 red, green, blue;
+	u8 red_int, green_int, blue_int;
+	u16 red_fract, green_fract, blue_fract;
+	u32 count = 0;
+	struct drm_r32g32b32 *correction_values = NULL;
+	u32 num_samples;
+	u32 word;
+	int ret = 0, length;
+
+	if (!blob) {
+		DRM_ERROR("NULL Blob\n");
+		return -EINVAL;
+	}
+
+	degamma_data = (struct drm_palette *)blob->data;
+
+	if (degamma_data->version != DEGAMMA_DATA_STRUCT_VERSION) {
+		DRM_ERROR("Invalid DeGamma Data struct version\n");
+		return -EINVAL;
+	}
+
+	pipe = to_intel_crtc(crtc)->pipe;
+	num_samples = degamma_data->num_samples;
+	length = num_samples * sizeof(struct drm_r32g32b32);
+
+	if (num_samples == 0) {
+
+		/* Disable DeGamma functionality on Pipe - CGM Block */
+		cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe));
+		cgm_control_reg &= ~CGM_DEGAMMA_EN;
+		I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg);
+
+		DRM_DEBUG_DRIVER("DeGamma disabled on Pipe %c\n",
+				pipe_name(pipe));
+		ret = 0;
+	} else if (num_samples == CHV_DEGAMMA_MAX_VALS) {
+		cgm_degamma_reg = _PIPE_DEGAMMA_BASE(pipe);
+
+		count = 0;
+		correction_values = (struct drm_r32g32b32 *)&degamma_data->lut;
+		while (count < CHV_DEGAMMA_MAX_VALS) {
+			blue = correction_values[count].b32;
+			green = correction_values[count].g32;
+			red = correction_values[count].r32;
+
+			blue_int = _GAMMA_INT_PART(blue);
+			if (blue_int > GAMMA_INT_MAX)
+				blue = CHV_MAX_GAMMA;
+			green_int = _GAMMA_INT_PART(green);
+			if (green_int > GAMMA_INT_MAX)
+				green = CHV_MAX_GAMMA;
+			red_int = _GAMMA_INT_PART(red);
+			if (red_int > GAMMA_INT_MAX)
+				red = CHV_MAX_GAMMA;
+
+			blue_fract = _GAMMA_FRACT_PART(blue);
+			green_fract = _GAMMA_FRACT_PART(green);
+			red_fract = _GAMMA_FRACT_PART(red);
+
+			blue_fract >>= CHV_DEGAMMA_MSB_SHIFT;
+			green_fract >>= CHV_DEGAMMA_MSB_SHIFT;
+			red_fract >>= CHV_DEGAMMA_MSB_SHIFT;
+
+			/* Green (29:16) and Blue (13:0) in DWORD1 */
+			word = green_fract;
+			word <<= CHV_DEGAMMA_GREEN_SHIFT;
+			word = word | blue;
+			I915_WRITE(cgm_degamma_reg, word);
+
+			cgm_degamma_reg += 4;
+
+			/* Red (13:0) to be written to DWORD2 */
+			word = red_fract;
+			I915_WRITE(cgm_degamma_reg, word);
+
+			cgm_degamma_reg += 4;
+			count++;
+		}
+
+		DRM_DEBUG_DRIVER("DeGamma LUT loaded for Pipe %c\n",
+				pipe_name(pipe));
+
+		/* Enable DeGamma on Pipe */
+		I915_WRITE(_PIPE_CGM_CONTROL(pipe),
+			I915_READ(_PIPE_CGM_CONTROL(pipe)) | CGM_DEGAMMA_EN);
+
+		DRM_DEBUG_DRIVER("DeGamma correction enabled on Pipe %c\n",
+				pipe_name(pipe));
+		ret = 0;
+	} else {
+		DRM_ERROR("Invalid number of samples for DeGamma LUT\n");
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
 static int chv_set_gamma(struct drm_device *dev, struct drm_property_blob *blob,
 		  struct drm_crtc *crtc)
 {
@@ -261,5 +366,11 @@ void intel_attach_color_properties_to_crtc(struct drm_device *dev,
 		DRM_DEBUG_DRIVER("palette after CTM attached to CRTC\n");
 	}
 
+	if (config->cm_palette_before_ctm_property) {
+		drm_object_attach_property(mode_obj,
+			config->cm_palette_before_ctm_property, 0);
+		DRM_DEBUG_DRIVER("palette before CTM attached to CRTC\n");
+	}
+
 	kfree(palette_caps);
 }
diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
index 5f800c9..5eaa0f3 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.h
+++ b/drivers/gpu/drm/i915/intel_color_manager.h
@@ -50,5 +50,11 @@
 /* Max value for Gamma on CHV */
 #define CHV_MAX_GAMMA				0x10000
 
+/* DeGamma correction */
+#define DEGAMMA_DATA_STRUCT_VERSION        1
+#define CHV_DEGAMMA_MSB_SHIFT                  2
+#define CHV_DEGAMMA_GREEN_SHIFT                16
+
 /* CHV CGM Block */
 #define CGM_GAMMA_EN				(1 << 2)
+#define CGM_DEGAMMA_EN                         (1 << 0)
-- 
1.9.1

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

[PATCH 17/23] drm/i915: CHV: Pipe level CSC correction

[Shashank Sharma]

CHV/BSW supports Color Space Conversion (CSC) using a 3x3 matrix
that needs to be programmed into CGM (Color Gamut Mapping) registers.

This patch does the following:
1. Attaches CSC property to CRTC
2. Adds the core function to program CSC correction values
3. Adds CSC correction macros

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
---
 drivers/gpu/drm/i915/i915_reg.h            |  8 +++
 drivers/gpu/drm/i915/intel_color_manager.c | 99 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_color_manager.h | 20 ++++++
 3 files changed, 127 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 3dde6a8..5c7759d 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7995,4 +7995,12 @@ enum skl_disp_power_wells {
 #define _PIPE_DEGAMMA_BASE(pipe) \
 	(_PIPE3(pipe, PIPEA_CGM_DEGAMMA, PIPEB_CGM_DEGAMMA, PIPEC_CGM_DEGAMMA))
 
+#define PIPEA_CGM_CSC				(VLV_DISPLAY_BASE + 0x67900)
+#define PIPEB_CGM_CSC				(VLV_DISPLAY_BASE + 0x69900)
+#define PIPEC_CGM_CSC				(VLV_DISPLAY_BASE + 0x6B900)
+#define _PIPE_CSC_BASE(pipe) \
+	(_PIPE3(pipe, PIPEA_CGM_CSC, PIPEB_CGM_CSC, PIPEC_CGM_CSC))
+
+
+
 #endif /* _I915_REG_H_ */
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
index e2ffbdc..22b708f 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.c
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -66,6 +66,99 @@ int intel_color_manager_set_pipe_degamma(struct drm_device *dev,
 	return 0;
 }
 
+static s16 get_csc_s3_12_format(s64 csc_value)
+{
+	s32 csc_int_value;
+	u32 csc_fract_value;
+	s16 csc_s3_12_format;
+
+	if (csc_value >= 0) {
+		csc_value += CHV_CSC_FRACT_ROUNDOFF;
+		if (csc_value > CHV_CSC_COEFF_MAX)
+			csc_value = CHV_CSC_COEFF_MAX;
+	} else {
+		csc_value = -csc_value;
+		csc_value += CHV_CSC_FRACT_ROUNDOFF;
+		if (csc_value > CHV_CSC_COEFF_MAX + 1)
+			csc_value = CHV_CSC_COEFF_MAX + 1;
+		csc_value = -csc_value;
+	}
+
+	csc_int_value = csc_value >> CHV_CSC_COEFF_SHIFT;
+	csc_int_value <<= CHV_CSC_COEFF_INT_SHIFT;
+	if (csc_value < 0)
+		csc_int_value |= CSC_COEFF_SIGN;
+	csc_fract_value = csc_value;
+	csc_fract_value >>= CHV_CSC_COEFF_FRACT_SHIFT;
+	csc_s3_12_format = csc_int_value | csc_fract_value;
+
+	return csc_s3_12_format;
+}
+
+static int chv_set_csc(struct drm_device *dev, struct drm_property_blob *blob,
+		struct drm_crtc *crtc)
+{
+	struct drm_ctm *csc_data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 reg;
+	enum pipe pipe;
+	s32 word, temp;
+	int count = 0;
+
+	if (!blob) {
+		DRM_ERROR("NULL Blob\n");
+		return -EINVAL;
+	}
+
+	if (blob->length != sizeof(struct drm_ctm)) {
+		DRM_ERROR("Invalid length of data received\n");
+		return -EINVAL;
+	}
+
+	csc_data = (struct drm_ctm *)blob->data;
+	if (csc_data->version != CSC_DATA_STRUCT_VERSION) {
+		DRM_ERROR("Invalid CSC Data struct version\n");
+		return -EINVAL;
+	}
+
+	pipe = to_intel_crtc(crtc)->pipe;
+
+	/* Disable CSC functionality */
+	reg = _PIPE_CGM_CONTROL(pipe);
+	I915_WRITE(reg, I915_READ(reg) & (~CGM_CSC_EN));
+
+	DRM_DEBUG_DRIVER("Disabled CSC Functionality on Pipe %c\n",
+			pipe_name(pipe));
+
+	reg = _PIPE_CSC_BASE(pipe);
+	while (count < CSC_MAX_VALS) {
+		word = get_csc_s3_12_format(csc_data->ctm_coeff[count]);
+
+		/*
+		 * Last value to be written in 1 register.
+		 * Otherwise, each pair of CSC values go
+		 * into 1 register
+		 */
+		if (count != (CSC_MAX_VALS - 1)) {
+			count++;
+			temp = get_csc_s3_12_format(csc_data->ctm_coeff[count]);
+			word |= temp;
+		}
+		I915_WRITE(reg, word);
+		reg += 4;
+		count++;
+	}
+
+	DRM_DEBUG_DRIVER("All CSC values written to registers\n");
+
+	/* Enable CSC functionality */
+	reg = _PIPE_CGM_CONTROL(pipe);
+	I915_WRITE(reg, I915_READ(reg) | CGM_CSC_EN);
+	DRM_DEBUG_DRIVER("CSC enabled on Pipe %c\n", pipe_name(pipe));
+
+	return 0;
+}
+
 static int chv_set_degamma(struct drm_device *dev,
 	struct drm_property_blob *blob, struct drm_crtc *crtc)
 {
@@ -372,5 +465,11 @@ void intel_attach_color_properties_to_crtc(struct drm_device *dev,
 		DRM_DEBUG_DRIVER("palette before CTM attached to CRTC\n");
 	}
 
+	if (config->cm_ctm_property) {
+		drm_object_attach_property(mode_obj,
+			config->cm_ctm_property, 0);
+		DRM_DEBUG_DRIVER("CTM property attached to CRTC\n");
+	}
+
 	kfree(palette_caps);
 }
diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
index 5eaa0f3..6e5158e 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.h
+++ b/drivers/gpu/drm/i915/intel_color_manager.h
@@ -55,6 +55,26 @@
 #define CHV_DEGAMMA_MSB_SHIFT                  2
 #define CHV_DEGAMMA_GREEN_SHIFT                16
 
+/* CSC correction */
+#define CSC_DATA_STRUCT_VERSION		1
+/*
+ * Fractional part is 32 bit, and we need only 12 MSBs for programming
+ * into registers. ROUNDOFF is required to minimize loss of precision.
+ */
+#define CHV_CSC_FRACT_ROUNDOFF			(1 << 19)
+/*
+ * CSC values are 64-bit values. For CHV, the maximum CSC value that
+ * user can program is 7.99999..., which can be represented in fixed point
+ * S31.32 format like this, with all fractional bits as 1
+ */
+#define CHV_CSC_COEFF_MAX			0x00000007FFFFFFFF
+#define CHV_CSC_COEFF_SHIFT			32
+#define CHV_CSC_COEFF_INT_SHIFT		28
+#define CSC_COEFF_SIGN				(1 << 31)
+#define CHV_CSC_COEFF_FRACT_SHIFT		20
+#define CSC_MAX_VALS				9
+
 /* CHV CGM Block */
 #define CGM_GAMMA_EN				(1 << 2)
+#define CGM_CSC_EN                             (1 << 1)
 #define CGM_DEGAMMA_EN                         (1 << 0)
-- 
1.9.1

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

[PATCH 18/23] drm/i915: Commit color changes to CRTC

[Shashank Sharma]

The color correction blob values are loaded during set_property
calls. This patch adds a function to find the blob and apply the
correction values to the display registers, during the atomic
commit call.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
---
 drivers/gpu/drm/i915/intel_color_manager.c | 46 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c       |  2 ++
 drivers/gpu/drm/i915/intel_drv.h           |  2 ++
 3 files changed, 50 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
index 22b708f..d33a2be 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.c
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -406,6 +406,52 @@ int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
 	return 0;
 }
 
+void intel_color_manager_crtc_commit(struct drm_device *dev,
+		struct drm_crtc_state *crtc_state)
+{
+	struct drm_property_blob *blob;
+	struct drm_crtc *crtc = crtc_state->crtc;
+	int ret = -EINVAL;
+
+	blob = crtc_state->palette_after_ctm_blob;
+	if (blob) {
+		/* Gamma correction is platform specific */
+		if (IS_CHERRYVIEW(dev))
+			ret = chv_set_gamma(dev, blob, crtc);
+
+		if (ret)
+			DRM_ERROR("set Gamma correction failed\n");
+		else
+			DRM_DEBUG_DRIVER("Gamma correction success\n");
+	}
+
+	blob = crtc_state->palette_before_ctm_blob;
+	if (blob) {
+		/* degamma correction */
+		if (IS_CHERRYVIEW(dev))
+			ret = chv_set_degamma(dev, blob, crtc);
+
+		if (ret)
+			DRM_ERROR("set degamma correction failed\n");
+		else
+			DRM_DEBUG_DRIVER("degamma correction success\n");
+	}
+
+	blob = crtc_state->ctm_blob;
+	if (blob) {
+		/* CSC correction */
+		if (IS_CHERRYVIEW(dev))
+			ret = chv_set_csc(dev, blob, crtc);
+
+		if (ret)
+			DRM_ERROR("set CSC correction failed\n");
+		else
+			DRM_DEBUG_DRIVER("CSC correction success\n");
+	}
+
+}
+
+
 int get_pipe_capabilities(struct drm_device *dev,
 		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
 {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 010acca..9b9d267 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13487,6 +13487,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
 		intel_update_pipe_config(intel_crtc, old_intel_state);
 	else if (INTEL_INFO(dev)->gen >= 9)
 		skl_detach_scalers(intel_crtc);
+
+	intel_color_manager_crtc_commit(dev, crtc->state);
 }
 
 static void intel_finish_crtc_commit(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 4d571a2f..f87a99d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1476,4 +1476,6 @@ int intel_color_manager_set_pipe_degamma(struct drm_device *dev,
 int intel_color_manager_set_pipe_csc(struct drm_device *dev,
 		struct drm_crtc_state *crtc_state,
 		struct drm_mode_object *obj, uint32_t blob_id);
+void intel_color_manager_crtc_commit(struct drm_device *dev,
+		struct drm_crtc_state *crtc_state);
 #endif /* __INTEL_DRV_H__ */
-- 
1.9.1

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

[PATCH 19/23] drm/i915: Attach color properties to CRTC

[Shashank Sharma]

Function intel_attach_color_properties_to_crtc attaches a
color property to its CRTC object. This patch calls this
function from crtc initialization sequence.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
---
 drivers/gpu/drm/i915/intel_display.c | 1 +
 drivers/gpu/drm/i915/intel_drv.h     | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9b9d267..8b5f266 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13818,6 +13818,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	intel_crtc->cursor_size = ~0;
 
 	intel_crtc->wm.cxsr_allowed = true;
+	intel_attach_color_properties_to_crtc(dev, &intel_crtc->base.base);
 
 	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
 	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f87a99d..f992345 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1478,4 +1478,6 @@ int intel_color_manager_set_pipe_csc(struct drm_device *dev,
 		struct drm_mode_object *obj, uint32_t blob_id);
 void intel_color_manager_crtc_commit(struct drm_device *dev,
 		struct drm_crtc_state *crtc_state);
+void intel_attach_color_properties_to_crtc(struct drm_device *dev,
+		struct drm_mode_object *mode_obj);
 #endif /* __INTEL_DRV_H__ */
-- 
1.9.1

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

[PATCH 20/23] drm/i915: BDW: Load gamma correction values

[Shashank Sharma]

I915 color manager registers pipe gamma correction as palette
correction after CTM property.

For BDW and higher platforms, split gamma correction is the best
gamma correction. This patch adds the no of coefficients(512) for
split gamma correction as "num_samples_after_ctm" parameter in device
info structures, for all of those.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c            | 7 +++++++
 drivers/gpu/drm/i915/intel_color_manager.h | 2 ++
 2 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 03db841..69afed3 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -302,6 +302,7 @@ static const struct intel_device_info intel_broadwell_d_info = {
 	.gen = 8, .num_pipes = 3,
 	.need_gfx_hws = 1, .has_hotplug = 1,
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
+	.num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS,
 	.has_llc = 1,
 	.has_ddi = 1,
 	.has_fpga_dbg = 1,
@@ -314,6 +315,7 @@ static const struct intel_device_info intel_broadwell_m_info = {
 	.gen = 8, .is_mobile = 1, .num_pipes = 3,
 	.need_gfx_hws = 1, .has_hotplug = 1,
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
+	.num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS,
 	.has_llc = 1,
 	.has_ddi = 1,
 	.has_fpga_dbg = 1,
@@ -326,6 +328,7 @@ static const struct intel_device_info intel_broadwell_gt3d_info = {
 	.gen = 8, .num_pipes = 3,
 	.need_gfx_hws = 1, .has_hotplug = 1,
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
+	.num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS,
 	.has_llc = 1,
 	.has_ddi = 1,
 	.has_fpga_dbg = 1,
@@ -338,6 +341,7 @@ static const struct intel_device_info intel_broadwell_gt3m_info = {
 	.gen = 8, .is_mobile = 1, .num_pipes = 3,
 	.need_gfx_hws = 1, .has_hotplug = 1,
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
+	.num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS,
 	.has_llc = 1,
 	.has_ddi = 1,
 	.has_fpga_dbg = 1,
@@ -363,6 +367,7 @@ static const struct intel_device_info intel_skylake_info = {
 	.gen = 9, .num_pipes = 3,
 	.need_gfx_hws = 1, .has_hotplug = 1,
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
+	.num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS,
 	.has_llc = 1,
 	.has_ddi = 1,
 	.has_fpga_dbg = 1,
@@ -376,6 +381,7 @@ static const struct intel_device_info intel_skylake_gt3_info = {
 	.gen = 9, .num_pipes = 3,
 	.need_gfx_hws = 1, .has_hotplug = 1,
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
+	.num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS,
 	.has_llc = 1,
 	.has_ddi = 1,
 	.has_fpga_dbg = 1,
@@ -389,6 +395,7 @@ static const struct intel_device_info intel_broxton_info = {
 	.gen = 9,
 	.need_gfx_hws = 1, .has_hotplug = 1,
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
+	.num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS,
 	.num_pipes = 3,
 	.has_ddi = 1,
 	.has_fpga_dbg = 1,
diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
index 6e5158e..3687989 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.h
+++ b/drivers/gpu/drm/i915/intel_color_manager.h
@@ -42,6 +42,8 @@
 #define CHV_10BIT_GAMMA_MSB_SHIFT		6
 #define CHV_GAMMA_SHIFT_GREEN			16
 
+#define BDW_SPLITGAMMA_MAX_VALS		512
+
 /* Gamma values are u8.24 format */
 #define GAMMA_INT_SHIFT			24
 #define GAMMA_FRACT_SHIFT			8
-- 
1.9.1

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

[PATCH 21/23] drm/i915: BDW: Pipe level Gamma correction

[Shashank Sharma]

BDW/SKL/BXT platforms support various Gamma correction modes
which are:
1. Legacy 8-bit mode
2. 10-bit mode
3. 10-bit Split Gamma mode
4. 12-bit mode

This patch does the following:
1. Adds the core function to program Gamma correction values
   for BDW/SKL/BXT platforms
2. Adds Gamma correction macros/defines

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
---
 drivers/gpu/drm/i915/i915_reg.h            |  17 +-
 drivers/gpu/drm/i915/intel_color_manager.c | 270 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_color_manager.h |  15 ++
 3 files changed, 300 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 5c7759d..88f4e41 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5634,7 +5634,9 @@ enum skl_disp_power_wells {
 
 #define _GAMMA_MODE_A		0x4a480
 #define _GAMMA_MODE_B		0x4ac80
-#define GAMMA_MODE(pipe) _PIPE(pipe, _GAMMA_MODE_A, _GAMMA_MODE_B)
+#define _GAMMA_MODE_C		0x4b480
+#define GAMMA_MODE(pipe) \
+	_PIPE3(pipe, _GAMMA_MODE_A, _GAMMA_MODE_B, _GAMMA_MODE_C)
 #define GAMMA_MODE_MODE_MASK	(3 << 0)
 #define GAMMA_MODE_MODE_8BIT	(0 << 0)
 #define GAMMA_MODE_MODE_10BIT	(1 << 0)
@@ -8001,6 +8003,17 @@ enum skl_disp_power_wells {
 #define _PIPE_CSC_BASE(pipe) \
 	(_PIPE3(pipe, PIPEA_CGM_CSC, PIPEB_CGM_CSC, PIPEC_CGM_CSC))
 
-
+/* BDW gamma correction */
+#define PAL_PREC_INDEX_A                       0x4A400
+#define PAL_PREC_INDEX_B                       0x4AC00
+#define PAL_PREC_INDEX_C                       0x4B400
+#define PAL_PREC_DATA_A                                0x4A404
+#define PAL_PREC_DATA_B                                0x4AC04
+#define PAL_PREC_DATA_C                                0x4B404
+
+#define _PREC_PAL_INDEX(pipe) \
+	(_PIPE3(pipe, PAL_PREC_INDEX_A, PAL_PREC_INDEX_B, PAL_PREC_INDEX_C))
+#define _PREC_PAL_DATA(pipe) \
+	(_PIPE3(pipe, PAL_PREC_DATA_A, PAL_PREC_DATA_B, PAL_PREC_DATA_C))
 
 #endif /* _I915_REG_H_ */
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
index d33a2be..d935fd8 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.c
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -264,6 +264,274 @@ static int chv_set_degamma(struct drm_device *dev,
 	return ret;
 }
 
+static u32 bdw_write_10bit_gamma_precision(u32 red, u32 green, u32 blue)
+{
+	u32 word;
+	u8 blue_int, green_int, red_int;
+	u16 blue_fract, green_fract, red_fract;
+
+	blue_int = _GAMMA_INT_PART(blue);
+	if (blue_int > GAMMA_INT_MAX)
+		blue = BDW_MAX_GAMMA;
+
+	green_int = _GAMMA_INT_PART(green);
+	if (green_int > GAMMA_INT_MAX)
+		green = BDW_MAX_GAMMA;
+
+	red_int = _GAMMA_INT_PART(red);
+	if (red_int > GAMMA_INT_MAX)
+		red = BDW_MAX_GAMMA;
+
+	blue_fract = _GAMMA_FRACT_PART(blue);
+	green_fract = _GAMMA_FRACT_PART(green);
+	red_fract = _GAMMA_FRACT_PART(red);
+
+	blue_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
+	green_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
+	red_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
+
+	/* Red (29:20) Green (19:10) and Blue (9:0) */
+	word = red_fract;
+	word <<= BDW_GAMMA_SHIFT;
+	word = word | green_fract;
+	word <<= BDW_GAMMA_SHIFT;
+	word = word | blue_fract;
+
+	return word;
+}
+
+/* Apply unity gamma for gamma reset */
+static void bdw_reset_gamma(struct drm_i915_private *dev_priv,
+			enum pipe pipe)
+{
+	u16 count = 0;
+	u32 val;
+	u32 pal_prec_data = LGC_PALETTE(pipe);
+
+	DRM_DEBUG_DRIVER("\n");
+
+	/* Reset the palette for unit gamma */
+	while (count < BDW_8BIT_GAMMA_MAX_VALS) {
+		/* Red (23:16) Green (15:8) and Blue (7:0) */
+		val = (count << 16) | (count << 8) | count;
+		I915_WRITE(pal_prec_data, val);
+		pal_prec_data += 4;
+		count++;
+	}
+}
+
+static int bdw_set_gamma(struct drm_device *dev, struct drm_property_blob *blob,
+		   struct drm_crtc *crtc)
+{
+	u8 blue_int, green_int, red_int;
+	u16 blue_fract, green_fract, red_fract;
+	u16 blue_odd, green_odd, red_odd;
+	u16 blue_even, green_even, red_even;
+
+	enum pipe pipe;
+	int count, num_samples;
+	u32 blue, green, red;
+	u32 mode, pal_prec_index, pal_prec_data;
+	u32 index, word;
+	struct drm_palette *gamma_data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_r32g32b32 *correction_values = NULL;
+
+	if (!blob) {
+		DRM_ERROR("Null Blob\n");
+		return -EINVAL;
+	}
+
+	gamma_data = (struct drm_palette *)blob->data;
+
+	if (gamma_data->version != GAMMA_DATA_STRUCT_VERSION) {
+		DRM_DEBUG_KMS("Invalid Gamma Data struct version\n");
+		return -EINVAL;
+	}
+
+	pipe = to_intel_crtc(crtc)->pipe;
+	num_samples = gamma_data->num_samples;
+
+	pal_prec_index = _PREC_PAL_INDEX(pipe);
+	pal_prec_data = _PREC_PAL_DATA(pipe);
+
+	correction_values = (struct drm_r32g32b32 *)&gamma_data->lut;
+	index = I915_READ(pal_prec_index);
+	switch (num_samples) {
+
+	case 0:
+
+		/* Disable Gamma functionality on Pipe */
+		DRM_DEBUG_DRIVER("Disabling gamma on Pipe %c\n",
+				pipe_name(pipe));
+		bdw_reset_gamma(dev_priv, pipe);
+		word = GAMMA_MODE_MODE_8BIT;
+		break;
+
+	case BDW_8BIT_GAMMA_MAX_VALS:
+
+		/* Legacy palette */
+		pal_prec_data = LGC_PALETTE(pipe);
+
+		count = 0;
+		while (count < BDW_8BIT_GAMMA_MAX_VALS) {
+			blue = correction_values[count].b32;
+			green = correction_values[count].g32;
+			red = correction_values[count].r32;
+
+			blue_int = _GAMMA_INT_PART(blue);
+			if (blue_int > GAMMA_INT_MAX)
+				blue = BDW_MAX_GAMMA;
+			green_int = _GAMMA_INT_PART(green);
+			if (green_int > GAMMA_INT_MAX)
+				green = BDW_MAX_GAMMA;
+			red_int = _GAMMA_INT_PART(red);
+			if (red_int > GAMMA_INT_MAX)
+				red = BDW_MAX_GAMMA;
+
+			blue_fract = _GAMMA_FRACT_PART(blue);
+			green_fract = _GAMMA_FRACT_PART(green);
+			red_fract = _GAMMA_FRACT_PART(red);
+
+			blue_fract >>= BDW_8BIT_GAMMA_MSB_SHIFT;
+			green_fract >>= BDW_8BIT_GAMMA_MSB_SHIFT;
+			red_fract >>= BDW_8BIT_GAMMA_MSB_SHIFT;
+
+			/* Red (23:16) Green (15:8) and Blue (7:0) */
+			word = red_fract;
+			word <<= BDW_8BIT_GAMMA_SHIFT;
+			word = word | green_fract;
+			word <<= BDW_8BIT_GAMMA_SHIFT;
+			word = word | blue_fract;
+
+			I915_WRITE(pal_prec_data, word);
+			pal_prec_data += 4;
+
+			count++;
+		}
+
+		word = GAMMA_MODE_MODE_8BIT;
+		break;
+
+	case BDW_SPLITGAMMA_MAX_VALS:
+
+		index |= BDW_INDEX_AUTO_INCREMENT | BDW_INDEX_SPLIT_MODE;
+		I915_WRITE(pal_prec_index, index);
+
+		count = 0;
+		while (count < BDW_SPLITGAMMA_MAX_VALS) {
+			blue = correction_values[count].b32;
+			green = correction_values[count].g32;
+			red = correction_values[count].r32;
+
+			word = bdw_write_10bit_gamma_precision(red,
+					green, blue);
+			I915_WRITE(pal_prec_data, word);
+			count++;
+		}
+
+		word = GAMMA_MODE_MODE_SPLIT;
+		break;
+
+	case BDW_12BIT_GAMMA_MAX_VALS:
+
+		index |= BDW_INDEX_AUTO_INCREMENT;
+		index &= ~BDW_INDEX_SPLIT_MODE;
+		I915_WRITE(pal_prec_index, index);
+
+		count = 0;
+		while (count < BDW_12BIT_GAMMA_MAX_VALS) {
+			blue = correction_values[count].b32;
+			green = correction_values[count].g32;
+			red = correction_values[count].r32;
+
+			blue_int = _GAMMA_INT_PART(blue);
+			if (blue_int > GAMMA_INT_MAX)
+				blue = BDW_MAX_GAMMA;
+			green_int = _GAMMA_INT_PART(green);
+			if (green_int > GAMMA_INT_MAX)
+				green = BDW_MAX_GAMMA;
+			red_int = _GAMMA_INT_PART(red);
+			if (red_int > GAMMA_INT_MAX)
+				red = BDW_MAX_GAMMA;
+
+			blue_fract = _GAMMA_FRACT_PART(blue);
+			green_fract = _GAMMA_FRACT_PART(green);
+			red_fract = _GAMMA_FRACT_PART(red);
+
+			/* Odd index */
+			if (count % 2 == 0) {
+				blue_odd = blue_fract >>
+					BDW_12BIT_GAMMA_ODD_SHIFT;
+				green_odd = green_fract >>
+					BDW_12BIT_GAMMA_ODD_SHIFT;
+				red_odd = red_fract >>
+					BDW_12BIT_GAMMA_ODD_SHIFT;
+
+				word = red_odd;
+				word = word << BDW_GAMMA_SHIFT;
+				word = word | green_odd;
+				word = word << BDW_GAMMA_SHIFT;
+				word = word | blue_odd;
+			} else {
+				blue_even = blue_fract <<
+					BDW_12BIT_GAMMA_EVEN_SHIFT1 >>
+					BDW_12BIT_GAMMA_EVEN_SHIFT2;
+				green_even = green_fract <<
+					BDW_12BIT_GAMMA_EVEN_SHIFT1 >>
+					BDW_12BIT_GAMMA_EVEN_SHIFT2;
+				red_even = red_fract <<
+					BDW_12BIT_GAMMA_EVEN_SHIFT1 >>
+					BDW_12BIT_GAMMA_EVEN_SHIFT2;
+
+				word = red_even;
+				word = word << BDW_GAMMA_SHIFT;
+				word = word | green_even;
+				word = word << BDW_GAMMA_SHIFT;
+				word = word | blue_even;
+			}
+			I915_WRITE(pal_prec_data, word);
+			count++;
+		}
+
+		word = GAMMA_MODE_MODE_12BIT;
+		break;
+
+	case BDW_10BIT_GAMMA_MAX_VALS:
+
+		index |= BDW_INDEX_AUTO_INCREMENT;
+		index &= ~BDW_INDEX_SPLIT_MODE;
+		I915_WRITE(pal_prec_index, index);
+
+		count = 0;
+		while (count < BDW_10BIT_GAMMA_MAX_VALS) {
+			blue = correction_values[count].b32;
+			green = correction_values[count].g32;
+			red = correction_values[count].r32;
+
+			word = bdw_write_10bit_gamma_precision(red,
+						green, blue);
+			I915_WRITE(pal_prec_data, word);
+			count++;
+		}
+
+		word = GAMMA_MODE_MODE_10BIT;
+		break;
+
+	default:
+		DRM_ERROR("Invalid number of samples\n");
+		return -EINVAL;
+	}
+
+	/* Set gamma mode on pipe control reg */
+	mode = I915_READ(GAMMA_MODE(pipe));
+	mode &= ~GAMMA_MODE_MODE_MASK;
+	I915_WRITE(GAMMA_MODE(pipe), mode | word);
+	DRM_DEBUG_DRIVER("Gamma applied on pipe %c\n",
+			pipe_name(pipe));
+	return 0;
+}
+
 static int chv_set_gamma(struct drm_device *dev, struct drm_property_blob *blob,
 		  struct drm_crtc *crtc)
 {
@@ -418,6 +686,8 @@ void intel_color_manager_crtc_commit(struct drm_device *dev,
 		/* Gamma correction is platform specific */
 		if (IS_CHERRYVIEW(dev))
 			ret = chv_set_gamma(dev, blob, crtc);
+		else if (IS_BROADWELL(dev) || IS_GEN9(dev))
+			ret = bdw_set_gamma(dev, blob, crtc);
 
 		if (ret)
 			DRM_ERROR("set Gamma correction failed\n");
diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
index 3687989..17fcf3d 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.h
+++ b/drivers/gpu/drm/i915/intel_color_manager.h
@@ -43,6 +43,9 @@
 #define CHV_GAMMA_SHIFT_GREEN			16
 
 #define BDW_SPLITGAMMA_MAX_VALS		512
+#define BDW_8BIT_GAMMA_MAX_VALS               256
+#define BDW_10BIT_GAMMA_MAX_VALS              1024
+#define BDW_12BIT_GAMMA_MAX_VALS              513
 
 /* Gamma values are u8.24 format */
 #define GAMMA_INT_SHIFT			24
@@ -52,6 +55,18 @@
 /* Max value for Gamma on CHV */
 #define CHV_MAX_GAMMA				0x10000
 
+/* Gen 9 */
+#define BDW_MAX_GAMMA				0x10000
+#define BDW_10BIT_GAMMA_MSB_SHIFT		6
+#define BDW_GAMMA_SHIFT			10
+#define BDW_INDEX_AUTO_INCREMENT		(1 << 15)
+#define BDW_INDEX_SPLIT_MODE			(1 << 31)
+#define BDW_8BIT_GAMMA_MSB_SHIFT		8
+#define BDW_8BIT_GAMMA_SHIFT			8
+#define BDW_12BIT_GAMMA_ODD_SHIFT		6
+#define BDW_12BIT_GAMMA_EVEN_SHIFT1		10
+#define BDW_12BIT_GAMMA_EVEN_SHIFT2		6
+
 /* DeGamma correction */
 #define DEGAMMA_DATA_STRUCT_VERSION        1
 #define CHV_DEGAMMA_MSB_SHIFT                  2
-- 
1.9.1

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

[PATCH 22/23] drm/i915: BDW: Load degamma correction values

[Shashank Sharma]

I915 color manager registers pipe degamma correction as palette
correction before CTM, DRM property.

This patch adds the no of coefficients(65) for degamma correction
as "num_samples_before_ctm" parameter in device info structures,
for BDW and higher platforms.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c            | 7 +++++++
 drivers/gpu/drm/i915/intel_color_manager.h | 2 ++
 2 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 69afed3..23ad2cc 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -303,6 +303,7 @@ static const struct intel_device_info intel_broadwell_d_info = {
 	.need_gfx_hws = 1, .has_hotplug = 1,
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
 	.num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS,
+	.num_samples_before_ctm = BDW_DEGAMMA_MAX_VALS,
 	.has_llc = 1,
 	.has_ddi = 1,
 	.has_fpga_dbg = 1,
@@ -316,6 +317,7 @@ static const struct intel_device_info intel_broadwell_m_info = {
 	.need_gfx_hws = 1, .has_hotplug = 1,
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
 	.num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS,
+	.num_samples_before_ctm = BDW_DEGAMMA_MAX_VALS,
 	.has_llc = 1,
 	.has_ddi = 1,
 	.has_fpga_dbg = 1,
@@ -329,6 +331,7 @@ static const struct intel_device_info intel_broadwell_gt3d_info = {
 	.need_gfx_hws = 1, .has_hotplug = 1,
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
 	.num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS,
+	.num_samples_before_ctm = BDW_DEGAMMA_MAX_VALS,
 	.has_llc = 1,
 	.has_ddi = 1,
 	.has_fpga_dbg = 1,
@@ -342,6 +345,7 @@ static const struct intel_device_info intel_broadwell_gt3m_info = {
 	.need_gfx_hws = 1, .has_hotplug = 1,
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
 	.num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS,
+	.num_samples_before_ctm = BDW_DEGAMMA_MAX_VALS,
 	.has_llc = 1,
 	.has_ddi = 1,
 	.has_fpga_dbg = 1,
@@ -368,6 +372,7 @@ static const struct intel_device_info intel_skylake_info = {
 	.need_gfx_hws = 1, .has_hotplug = 1,
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
 	.num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS,
+	.num_samples_before_ctm = BDW_DEGAMMA_MAX_VALS,
 	.has_llc = 1,
 	.has_ddi = 1,
 	.has_fpga_dbg = 1,
@@ -382,6 +387,7 @@ static const struct intel_device_info intel_skylake_gt3_info = {
 	.need_gfx_hws = 1, .has_hotplug = 1,
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
 	.num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS,
+	.num_samples_before_ctm = BDW_DEGAMMA_MAX_VALS,
 	.has_llc = 1,
 	.has_ddi = 1,
 	.has_fpga_dbg = 1,
@@ -396,6 +402,7 @@ static const struct intel_device_info intel_broxton_info = {
 	.need_gfx_hws = 1, .has_hotplug = 1,
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
 	.num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS,
+	.num_samples_before_ctm = BDW_DEGAMMA_MAX_VALS,
 	.num_pipes = 3,
 	.has_ddi = 1,
 	.has_fpga_dbg = 1,
diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
index 17fcf3d..a428825 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.h
+++ b/drivers/gpu/drm/i915/intel_color_manager.h
@@ -72,6 +72,8 @@
 #define CHV_DEGAMMA_MSB_SHIFT                  2
 #define CHV_DEGAMMA_GREEN_SHIFT                16
 
+#define BDW_DEGAMMA_MAX_VALS			512
+
 /* CSC correction */
 #define CSC_DATA_STRUCT_VERSION		1
 /*
-- 
1.9.1

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

[PATCH 23/23] drm/i915: BDW: Pipe level degamma correction

[Shashank Sharma]

BDW/SKL/BXT supports Degamma color correction feature, which
linearizes the non-linearity due to gamma encoded color values.
This will be applied before Color Transformation.

This patch does the following:
1. Adds the core function to program DeGamma correction values for
   BDW/SKL/BXT platform
2. Adds DeGamma correction macros/defines

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
---
 drivers/gpu/drm/i915/intel_color_manager.c | 61 ++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
index d935fd8..46f880d 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.c
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -532,6 +532,65 @@ static int bdw_set_gamma(struct drm_device *dev, struct drm_property_blob *blob,
 	return 0;
 }
 
+static int bdw_set_degamma(struct drm_device *dev,
+	struct drm_property_blob *blob, struct drm_crtc *crtc)
+{
+	enum pipe pipe;
+	int num_samples, length;
+	int count = 0;
+	u32 index, word;
+	u32 blue, green, red;
+	u32 mode, pal_prec_index, pal_prec_data;
+	struct drm_palette *degamma_data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_r32g32b32 *correction_values = NULL;
+
+	if (WARN_ON(!blob))
+		return -EINVAL;
+
+	degamma_data = (struct drm_palette *)blob->data;
+	if (degamma_data->version != DEGAMMA_DATA_STRUCT_VERSION) {
+		DRM_ERROR("Invalid DeGamma Data struct version\n");
+		return -EINVAL;
+	}
+
+	pipe = to_intel_crtc(crtc)->pipe;
+	num_samples = degamma_data->num_samples;
+	if (num_samples != BDW_SPLITGAMMA_MAX_VALS) {
+		DRM_ERROR("Invalid number of samples\n");
+		return -EINVAL;
+	}
+
+	length = num_samples * sizeof(struct drm_r32g32b32);
+	mode = I915_READ(GAMMA_MODE(pipe));
+	pal_prec_index = _PREC_PAL_INDEX(pipe);
+	pal_prec_data = _PREC_PAL_DATA(pipe);
+
+	correction_values = (struct drm_r32g32b32 *)&degamma_data->lut;
+	index = I915_READ(pal_prec_index);
+	index |= BDW_INDEX_AUTO_INCREMENT | BDW_INDEX_SPLIT_MODE;
+	I915_WRITE(pal_prec_index, index);
+
+	while (count < num_samples) {
+		blue = correction_values[count].b32;
+		green = correction_values[count].g32;
+		red = correction_values[count].r32;
+
+		word = bdw_write_10bit_gamma_precision(red, green, blue);
+		I915_WRITE(pal_prec_data, word);
+		count++;
+	}
+
+	/* Enable DeGamma on Pipe */
+	mode &= ~GAMMA_MODE_MODE_MASK;
+	I915_WRITE(GAMMA_MODE(pipe), mode | GAMMA_MODE_MODE_SPLIT);
+
+	DRM_DEBUG_DRIVER("DeGamma correction enabled on Pipe %c\n",
+			pipe_name(pipe));
+
+	return 0;
+}
+
 static int chv_set_gamma(struct drm_device *dev, struct drm_property_blob *blob,
 		  struct drm_crtc *crtc)
 {
@@ -700,6 +759,8 @@ void intel_color_manager_crtc_commit(struct drm_device *dev,
 		/* degamma correction */
 		if (IS_CHERRYVIEW(dev))
 			ret = chv_set_degamma(dev, blob, crtc);
+		else if (IS_BROADWELL(dev) || IS_GEN9(dev))
+			ret = bdw_set_degamma(dev, blob, crtc);
 
 		if (ret)
 			DRM_ERROR("set degamma correction failed\n");
-- 
1.9.1

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

Re: [PATCH 01/23] drm: Create Color Management DRM properties

[Matt Roper]

On Wed, Sep 16, 2015 at 11:06:58PM +0530, Shashank Sharma wrote:
> From: Kausal Malladi <kausalmalladi@gmail.com>
> 
> Color Management is an extension to Kernel display framework. It allows
> abstraction of hardware color correction and enhancement capabilities by
> virtue of DRM properties.
> 
> This patch initializes color management framework by :
> 1. Introducing new pointers in DRM mode_config structure to
>    carry CTM and Palette color correction properties.
> 2. Creating these DRM properties in DRM standard properties creation
>    sequence.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>

We should probably update the property section of
Documentation/DocBook/drm.tmpl with this patch as well to include these
new properties in the table (that docbook ultimately generates
documentation that looks like
https://kernel.org/doc/htmldocs/drm/drm-kms-properties.html ).

One minor note:  people not involved in color management probably won't
immediately figure out what "CTM" stands for, so you might want to just
add a comment somewhere that spells out the full "color transformation
matrix" term.


Matt


> ---
>  drivers/gpu/drm/drm_crtc.c | 26 ++++++++++++++++++++++++++
>  include/drm/drm_crtc.h     |  6 ++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 9b9c4b4..d809c67 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1472,6 +1472,32 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>  		return -ENOMEM;
>  	dev->mode_config.prop_mode_id = prop;
>  
> +	/* Color Management properties */
> +	prop = drm_property_create(dev,
> +			DRM_MODE_PROP_BLOB | DRM_MODE_PROP_IMMUTABLE,
> +			"CRTC_PALETTE_CAPABILITIES", 0);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.cm_crtc_palette_capabilities_property = prop;
> +
> +	prop = drm_property_create(dev,
> +			DRM_MODE_PROP_BLOB, "PALETTE_AFTER_CTM", 0);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.cm_palette_after_ctm_property = prop;
> +
> +	prop = drm_property_create(dev,
> +			DRM_MODE_PROP_BLOB, "PALETTE_BEFORE_CTM", 0);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.cm_palette_before_ctm_property = prop;
> +
> +	prop = drm_property_create(dev,
> +			DRM_MODE_PROP_BLOB, "CTM", 0);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.cm_ctm_property = prop;
> +
>  	return 0;
>  }
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index c0366e9..c35531e 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1153,6 +1153,12 @@ struct drm_mode_config {
>  	struct drm_property *suggested_x_property;
>  	struct drm_property *suggested_y_property;
>  
> +	/* Color Management Properties */
> +	struct drm_property *cm_crtc_palette_capabilities_property;
> +	struct drm_property *cm_palette_before_ctm_property;
> +	struct drm_property *cm_palette_after_ctm_property;
> +	struct drm_property *cm_ctm_property;
> +
>  	/* dumb ioctl parameters */
>  	uint32_t preferred_depth, prefer_shadow;
>  
> -- 
> 1.9.1
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 02/23] drm: Add structure for querying palette color capabilities

[Matt Roper]

On Wed, Sep 16, 2015 at 11:06:59PM +0530, Shashank Sharma wrote:
> From: Kausal Malladi <kausalmalladi@gmail.com>
> 
> The DRM color management framework is targeting various hardware
> platforms and drivers. Different platforms can have different color
> correction and enhancement capabilities.
> 
> A commom user space application can query these capabilities using the
> DRM property interface. Each driver can fill this property with its
> platform's palette color capabilities.
> 
> This patch adds new structure in DRM layer for querying palette color
> capabilities. This structure will be used by all user space
> agents to configure appropriate color configurations.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>

I think you provided an explanation on a previous code review cycle, but
I forget the details now...what's the benefit to using a blob for caps
rather than having these be individual properties?  Individual
properties seems more natural to me, but I think you had a justification
for blobbing them together; that reasoning would be good to include in
the commit message.


Matt

> ---
>  include/uapi/drm/drm.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 3801584..e3c642f 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -829,6 +829,17 @@ struct drm_event_vblank {
>  	__u32 reserved;
>  };
>  
> +struct drm_palette_caps {
> +	/* Structure version. Should be 1 currently */
> +	__u32 version;
> +	/* For padding and future use */
> +	__u32 reserved;
> +	/* This may be 0 if not supported. e.g. plane palette or VLV pipe */
> +	__u32 num_samples_before_ctm;
> +	/* This will be non-zero for pipe. May be zero for planes on some HW */
> +	__u32 num_samples_after_ctm;
> +};
> +
>  /* typedef area */
>  #ifndef __KERNEL__
>  typedef struct drm_clip_rect drm_clip_rect_t;
> -- 
> 1.9.1
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/23] drm: Add drm structures for palette color property

[Ville Syrjälä]

On Wed, Sep 16, 2015 at 11:07:01PM +0530, Shashank Sharma wrote:
> From: Kausal Malladi <kausalmalladi@gmail.com>
> 
> This patch adds new structures in DRM layer for Palette color
> correction.These structures will be used by user space agents
> to configure appropriate number of samples and Palette LUT for
> a platform.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
> ---
>  include/uapi/drm/drm.h | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index e3c642f..f72b916 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -840,6 +840,33 @@ struct drm_palette_caps {
>  	__u32 num_samples_after_ctm;
>  };
>  
> +struct drm_r32g32b32 {
> +	/*
> +	 * Data is in U8.24 fixed point format.
> +	 * All platforms support values within [0, 1.0] range,
> +	 * for Red, Green and Blue colors.
> +	 */
> +	__u32 r32;
> +	__u32 g32;
> +	__u32 b32;
> +};
> +
> +struct drm_palette {
> +	/* Structure version. Should be 1 currently */
> +	__u32 version;

I don't think we want to version the blobs. For one, we don't have a way
for userspace to ask for a specific version, so the kernel wouldn't know
which version it should return to each client.

If we ever need to come up with new version of a specific blob, I think we
just have to define another property for it. Either that or we'd some kind
of client cap stuff to negotiate the used version between the kernel and
a specific client.

Well, I suppose we migth be able to borrow the "flags+extend at the end"
trick we sometimes use for ioctl parameters to work for blobs too. But I
have a feeling we don't want to go there.

So yeah, I think we should go with the "blob layout is fixed for each
property" approach. Versioning then happens by introducing new versions
of the same property if needed.

> +	/*
> +	 * This has to be a supported value during get call.
> +	 * Feature will be disabled if this is 0 while set
> +	 */
> +	__u32 num_samples;
> +	/*
> +	 * Starting of palette LUT in R32G32B32 format.
> +	 * Each of RGB value is in U8.24 fixed point format.
> +	 * Actual number of samples will depend upon num_samples
> +	 */
> +	struct drm_r32g32b32 lut[0];
> +};
> +
>  /* typedef area */
>  #ifndef __KERNEL__
>  typedef struct drm_clip_rect drm_clip_rect_t;
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/23] drm: Add drm structures for palette color property

[Sharma, Shashank]

Regards
Shashank

On 9/21/2015 10:16 PM, Ville Syrjälä wrote:
> On Wed, Sep 16, 2015 at 11:07:01PM +0530, Shashank Sharma wrote:
>> From: Kausal Malladi <kausalmalladi@gmail.com>
>>
>> This patch adds new structures in DRM layer for Palette color
>> correction.These structures will be used by user space agents
>> to configure appropriate number of samples and Palette LUT for
>> a platform.
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
>> ---
>>   include/uapi/drm/drm.h | 27 +++++++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>> index e3c642f..f72b916 100644
>> --- a/include/uapi/drm/drm.h
>> +++ b/include/uapi/drm/drm.h
>> @@ -840,6 +840,33 @@ struct drm_palette_caps {
>>   	__u32 num_samples_after_ctm;
>>   };
>>
>> +struct drm_r32g32b32 {
>> +	/*
>> +	 * Data is in U8.24 fixed point format.
>> +	 * All platforms support values within [0, 1.0] range,
>> +	 * for Red, Green and Blue colors.
>> +	 */
>> +	__u32 r32;
>> +	__u32 g32;
>> +	__u32 b32;
>> +};
>> +
>> +struct drm_palette {
>> +	/* Structure version. Should be 1 currently */
>> +	__u32 version;
>
> I don't think we want to version the blobs. For one, we don't have a way
> for userspace to ask for a specific version, so the kernel wouldn't know
> which version it should return to each client.
>
> If we ever need to come up with new version of a specific blob, I think we
> just have to define another property for it. Either that or we'd some kind
> of client cap stuff to negotiate the used version between the kernel and
> a specific client.
>
> Well, I suppose we migth be able to borrow the "flags+extend at the end"
> trick we sometimes use for ioctl parameters to work for blobs too. But I
> have a feeling we don't want to go there.
>
> So yeah, I think we should go with the "blob layout is fixed for each
> property" approach. Versioning then happens by introducing new versions
> of the same property if needed.
>
Well, reason behind adding this version was, as this framework was a new 
development, we wanted to keep scope for further changes on request from 
other drivers/modules. But yes, I agree, its kind of overhead, so we can 
also remove it. Will do that in next patch set.
>> +	/*
>> +	 * This has to be a supported value during get call.
>> +	 * Feature will be disabled if this is 0 while set
>> +	 */
>> +	__u32 num_samples;
>> +	/*
>> +	 * Starting of palette LUT in R32G32B32 format.
>> +	 * Each of RGB value is in U8.24 fixed point format.
>> +	 * Actual number of samples will depend upon num_samples
>> +	 */
>> +	struct drm_r32g32b32 lut[0];
>> +};
>> +
>>   /* typedef area */
>>   #ifndef __KERNEL__
>>   typedef struct drm_clip_rect drm_clip_rect_t;
>> --
>> 1.9.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/23] drm: Add structure for querying palette color capabilities

[Daniel Vetter]

On Wed, Sep 16, 2015 at 10:51:31AM -0700, Matt Roper wrote:
> On Wed, Sep 16, 2015 at 11:06:59PM +0530, Shashank Sharma wrote:
> > From: Kausal Malladi <kausalmalladi@gmail.com>
> > 
> > The DRM color management framework is targeting various hardware
> > platforms and drivers. Different platforms can have different color
> > correction and enhancement capabilities.
> > 
> > A commom user space application can query these capabilities using the
> > DRM property interface. Each driver can fill this property with its
> > platform's palette color capabilities.
> > 
> > This patch adds new structure in DRM layer for querying palette color
> > capabilities. This structure will be used by all user space
> > agents to configure appropriate color configurations.
> > 
> > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> > Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
> 
> I think you provided an explanation on a previous code review cycle, but
> I forget the details now...what's the benefit to using a blob for caps
> rather than having these be individual properties?  Individual
> properties seems more natural to me, but I think you had a justification
> for blobbing them together; that reasoning would be good to include in
> the commit message.

Yeah I'm leaning slightly towards individual props too, that would give us
a bit more freedom with placing them (e.g. if someone comes up with funky
hw where before_ctm and ctm are per-plane and after_ctm is on the crtc,
with only some planes support the before_ctm gamma table).

Also if you do per-prop properties instead of the blob you can drop the
version/reserved fields, since properties are inheritedly designed to be
extendible. So no need to revision them again (it only leads to more code
that might break).
-Daniel

> 
> 
> Matt
> 
> > ---
> >  include/uapi/drm/drm.h | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > index 3801584..e3c642f 100644
> > --- a/include/uapi/drm/drm.h
> > +++ b/include/uapi/drm/drm.h
> > @@ -829,6 +829,17 @@ struct drm_event_vblank {
> >  	__u32 reserved;
> >  };
> >  
> > +struct drm_palette_caps {
> > +	/* Structure version. Should be 1 currently */
> > +	__u32 version;
> > +	/* For padding and future use */
> > +	__u32 reserved;
> > +	/* This may be 0 if not supported. e.g. plane palette or VLV pipe */
> > +	__u32 num_samples_before_ctm;
> > +	/* This will be non-zero for pipe. May be zero for planes on some HW */
> > +	__u32 num_samples_after_ctm;
> > +};
> > +
> >  /* typedef area */
> >  #ifndef __KERNEL__
> >  typedef struct drm_clip_rect drm_clip_rect_t;
> > -- 
> > 1.9.1
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/23] drm: Add drm structures for palette color property

[Daniel Vetter]

On Wed, Sep 16, 2015 at 11:07:01PM +0530, Shashank Sharma wrote:
> From: Kausal Malladi <kausalmalladi@gmail.com>
> 
> This patch adds new structures in DRM layer for Palette color
> correction.These structures will be used by user space agents
> to configure appropriate number of samples and Palette LUT for
> a platform.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
> ---
>  include/uapi/drm/drm.h | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index e3c642f..f72b916 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -840,6 +840,33 @@ struct drm_palette_caps {
>  	__u32 num_samples_after_ctm;
>  };
>  
> +struct drm_r32g32b32 {
> +	/*
> +	 * Data is in U8.24 fixed point format.
> +	 * All platforms support values within [0, 1.0] range,
> +	 * for Red, Green and Blue colors.
> +	 */
> +	__u32 r32;
> +	__u32 g32;
> +	__u32 b32;

It's not strictly required, but adding a __u32 reserved here to align the
struct to 64 bits seems good imo. Slight overhead but meh about that.

> +};
> +
> +struct drm_palette {
> +	/* Structure version. Should be 1 currently */
> +	__u32 version;

Definitely great practice to take compat into account and definitely
needed for the first design using ioctls but I don't think we need this
here. Properties are already extinsible themselves: We can just greate a
"ctm-v2", "ctm-v3" if the layout changes, and since the actual ctm matrix
is stored in the drm_crtc_state any compat code on the kernel will be
shared.

Aside: For an ioctl the recommended way to handle backwards compat and
extensions in drm is with a flags bitfield. That's more flexible than a
linear version field, and extending the ioctl struct at the end is already
handled by the drm core in a transparent fashion (it 0-fills either kernel
or userspace side).

> +	/*
> +	 * This has to be a supported value during get call.
> +	 * Feature will be disabled if this is 0 while set
> +	 */
> +	__u32 num_samples;

blob properties already have a size, storing it again in the blob is
redundnant. Instead I think a small helper to get the number of samples
for a given gamma table blob would be needed.

Cheers, Daniel

> +	/*
> +	 * Starting of palette LUT in R32G32B32 format.
> +	 * Each of RGB value is in U8.24 fixed point format.
> +	 * Actual number of samples will depend upon num_samples
> +	 */
> +	struct drm_r32g32b32 lut[0];
> +};
> +
>  /* typedef area */
>  #ifndef __KERNEL__
>  typedef struct drm_clip_rect drm_clip_rect_t;
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 05/23] drm: Add structure to set/get a CTM color property

[Daniel Vetter]

On Wed, Sep 16, 2015 at 11:07:02PM +0530, Shashank Sharma wrote:
> From: Kausal Malladi <kausalmalladi@gmail.com>
> 
> Color Manager framework defines a color correction property for color
> space transformation and Gamut mapping. This property is called CTM (Color
> Transformation Matrix).
> 
> This patch adds a new structure in DRM layer for CTM.
> This structure can be used by all user space agents to
> configure CTM coefficients for color correction.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
> ---
>  include/uapi/drm/drm.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index f72b916..9580772 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -867,6 +867,18 @@ struct drm_palette {
>  	struct drm_r32g32b32 lut[0];
>  };
>  
> +struct drm_ctm {
> +	/* Structure version. Should be 1 currently */
> +	__u32 version;

Same thing here, no version needed for properties.

> +	/*
> +	 * Each value is in S31.32 format.
> +	 * This is 3x3 matrix in row major format.
> +	 * Integer part will be clipped to nearest
> +	 * max/min boundary as supported by the HW platform.
> +	 */
> +	__s64 ctm_coeff[9];
> +};
> +
>  /* typedef area */
>  #ifndef __KERNEL__
>  typedef struct drm_clip_rect drm_clip_rect_t;
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 10/23] drm/i915: Add gamma correction handlers

[Daniel Vetter]

On Wed, Sep 16, 2015 at 11:07:07PM +0530, Shashank Sharma wrote:
> I915 driver registers gamma correction as palette correction
> property with DRM layer. This patch adds set_property() and get_property()
> handlers for pipe level gamma correction.
> 
> The set function attaches the Gamma correction blob to CRTC state, these
> values will be committed during atomic commit.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic.c        | 20 ++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_color_manager.c | 21 +++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h           |  5 +++++
>  3 files changed, 46 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 500d2998..0b61fef 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -315,6 +315,13 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
>  				   struct drm_property *property,
>  				   uint64_t val)
>  {
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	if (property == config->cm_palette_after_ctm_property)
> +		return intel_color_manager_set_pipe_gamma(dev, state,
> +				&crtc->base, val);
> +
>  	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
>  	return -EINVAL;
>  }
> @@ -324,6 +331,19 @@ int intel_crtc_atomic_get_property(struct drm_crtc *crtc,
>  				   struct drm_property *property,
>  				   uint64_t *val)
>  {
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	if (property == config->cm_palette_after_ctm_property) {
> +		*val = (state->palette_after_ctm_blob) ?
> +			state->palette_after_ctm_blob->base.id : 0;
> +		goto found;
> +	}

Since color manager properties are meant as a new standardize KMS
extension (we put them into the core drm_crtc_state) the get/set support
should also be in the core. See e.g. how the rotation property is handled
in drm_atomic_plane_get/set_property. So all this code should be added to
drm_atomic_crtc_get/set_property.


> +
>  	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
>  	return -EINVAL;
> +
> +found:
> +	DRM_DEBUG_KMS("Found property %s\n", property->name);
> +	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
> index 77f58f2..9421bb6 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.c
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -27,6 +27,27 @@
>  
>  #include "intel_color_manager.h"
>  
> +int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
> +		struct drm_crtc_state *crtc_state,
> +		struct drm_mode_object *obj, uint32_t blob_id)
> +{
> +	struct drm_property_blob *blob;
> +
> +	blob = drm_property_lookup_blob(dev, blob_id);
> +	if (!blob) {
> +		DRM_DEBUG_KMS("Invalid Blob ID\n");
> +		return -EINVAL;
> +	}
> +
> +	if (crtc_state->palette_after_ctm_blob)
> +		drm_property_unreference_blob(
> +			crtc_state->palette_after_ctm_blob);
> +
> +	/* Attach the blob to be committed in state */
> +	crtc_state->palette_after_ctm_blob = blob;
> +	return 0;
> +}

What is this used for? It looks a bit like legacy property code, and we
have a generic helper to make that happen
(drm_atomic_helper_crtc_set_property).

Generally please don't add functions/structs without also adding a user,
it means that reviewers have to constantly jump around in your patch
series to figure out how something is used. Instead if you want to split
things up really fine add a stub function frist (but including relevant
callers) and then fill out the bits separately.
-Daniel

> +
>  int get_pipe_capabilities(struct drm_device *dev,
>  		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e27e754..d0193e2 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1466,4 +1466,9 @@ void intel_plane_destroy_state(struct drm_plane *plane,
>  			       struct drm_plane_state *state);
>  extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
>  
> +/* intel_color_manager.c */
> +int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
> +		struct drm_crtc_state *crtc_state,
> +		struct drm_mode_object *obj, uint32_t blob_id);
> +
>  #endif /* __INTEL_DRV_H__ */
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/23] drm/i915: Add gamma correction handlers

[Daniel Vetter]

On Tue, Sep 22, 2015 at 03:15:11PM +0200, Daniel Vetter wrote:
> On Wed, Sep 16, 2015 at 11:07:07PM +0530, Shashank Sharma wrote:
> > I915 driver registers gamma correction as palette correction
> > property with DRM layer. This patch adds set_property() and get_property()
> > handlers for pipe level gamma correction.
> > 
> > The set function attaches the Gamma correction blob to CRTC state, these
> > values will be committed during atomic commit.
> > 
> > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> > Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
> > ---
> >  drivers/gpu/drm/i915/intel_atomic.c        | 20 ++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_color_manager.c | 21 +++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h           |  5 +++++
> >  3 files changed, 46 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> > index 500d2998..0b61fef 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic.c
> > @@ -315,6 +315,13 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
> >  				   struct drm_property *property,
> >  				   uint64_t val)
> >  {
> > +	struct drm_device *dev = crtc->dev;
> > +	struct drm_mode_config *config = &dev->mode_config;
> > +
> > +	if (property == config->cm_palette_after_ctm_property)
> > +		return intel_color_manager_set_pipe_gamma(dev, state,
> > +				&crtc->base, val);
> > +
> >  	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
> >  	return -EINVAL;
> >  }
> > @@ -324,6 +331,19 @@ int intel_crtc_atomic_get_property(struct drm_crtc *crtc,
> >  				   struct drm_property *property,
> >  				   uint64_t *val)
> >  {
> > +	struct drm_device *dev = crtc->dev;
> > +	struct drm_mode_config *config = &dev->mode_config;
> > +
> > +	if (property == config->cm_palette_after_ctm_property) {
> > +		*val = (state->palette_after_ctm_blob) ?
> > +			state->palette_after_ctm_blob->base.id : 0;
> > +		goto found;
> > +	}
> 
> Since color manager properties are meant as a new standardize KMS
> extension (we put them into the core drm_crtc_state) the get/set support
> should also be in the core. See e.g. how the rotation property is handled
> in drm_atomic_plane_get/set_property. So all this code should be added to
> drm_atomic_crtc_get/set_property.

I've forgotten to explain how drivers can then opt-in KMS extensions if we
have the decode support unconditionally there and also register the props
unconditionally: That's done by only attaching these standardized props to
a crtc/plane if the corresponding object supports it.
-Daniel

> 
> 
> > +
> >  	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
> >  	return -EINVAL;
> > +
> > +found:
> > +	DRM_DEBUG_KMS("Found property %s\n", property->name);
> > +	return 0;
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
> > index 77f58f2..9421bb6 100644
> > --- a/drivers/gpu/drm/i915/intel_color_manager.c
> > +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> > @@ -27,6 +27,27 @@
> >  
> >  #include "intel_color_manager.h"
> >  
> > +int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
> > +		struct drm_crtc_state *crtc_state,
> > +		struct drm_mode_object *obj, uint32_t blob_id)
> > +{
> > +	struct drm_property_blob *blob;
> > +
> > +	blob = drm_property_lookup_blob(dev, blob_id);
> > +	if (!blob) {
> > +		DRM_DEBUG_KMS("Invalid Blob ID\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (crtc_state->palette_after_ctm_blob)
> > +		drm_property_unreference_blob(
> > +			crtc_state->palette_after_ctm_blob);
> > +
> > +	/* Attach the blob to be committed in state */
> > +	crtc_state->palette_after_ctm_blob = blob;
> > +	return 0;
> > +}
> 
> What is this used for? It looks a bit like legacy property code, and we
> have a generic helper to make that happen
> (drm_atomic_helper_crtc_set_property).
> 
> Generally please don't add functions/structs without also adding a user,
> it means that reviewers have to constantly jump around in your patch
> series to figure out how something is used. Instead if you want to split
> things up really fine add a stub function frist (but including relevant
> callers) and then fill out the bits separately.
> -Daniel
> 
> > +
> >  int get_pipe_capabilities(struct drm_device *dev,
> >  		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
> >  {
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index e27e754..d0193e2 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1466,4 +1466,9 @@ void intel_plane_destroy_state(struct drm_plane *plane,
> >  			       struct drm_plane_state *state);
> >  extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
> >  
> > +/* intel_color_manager.c */
> > +int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
> > +		struct drm_crtc_state *crtc_state,
> > +		struct drm_mode_object *obj, uint32_t blob_id);
> > +
> >  #endif /* __INTEL_DRV_H__ */
> > -- 
> > 1.9.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/23] drm/i915: Register pipe color capabilities

[Daniel Vetter]

On Wed, Sep 16, 2015 at 11:07:06PM +0530, Shashank Sharma wrote:
> DRM color manager contains these color properties:
> 
> 1. "crtc_palette_capabilities_property": to allow a
> core driver to load and showcase its color correction
> capabilities to user space.
> 2. "ctm": Color transformation matrix property, where a
> color transformation matrix of 9 correction values gets
> applied as correction.
> 3. "palette_before_ctm": for corrections which get applied
> beore color transformation matrix correction.
> 4. "palette_after_ctm": for corrections which get applied
> after color transformation matrix correction.
> 
> Intel color manager registers:
> 1. Gamma correction property as "palette_after_ctm" property
> 2. Degamma correction capability as "palette_bafore_ctm" property
> capability as "palette_after_ctm" DRM color property hook.
> 3. CSC as "ctm" property.
> 
> This patch does the following:
> 1. Add a function which loads the platform's color correction
> capabilities in the cm_crtc_palette_capabilities_property structure.
> 2. Attaches the cm_crtc_palette_capabilities_property to every CRTC
> getting initiaized.
> 3. Adds two new parameters "num_samples_after_ctm" and
> "num_samples_before_ctm" in intel_device_info as gamma and
> degamma coefficients vary per platform basis.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h            |  2 ++
>  drivers/gpu/drm/i915/intel_color_manager.c | 45 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_color_manager.h |  2 ++
>  3 files changed, 49 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3bf8a9b..22de2cb 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -798,6 +798,8 @@ struct intel_device_info {
>  	u8 num_sprites[I915_MAX_PIPES];
>  	u8 gen;
>  	u8 ring_mask; /* Rings supported by the HW */
> +	u16 num_samples_after_ctm;
> +	u16 num_samples_before_ctm;
>  	DEV_INFO_FOR_EACH_FLAG(DEFINE_FLAG, SEP_SEMICOLON);
>  	/* Register offsets for the various display pipes and transcoders */
>  	int pipe_offsets[I915_MAX_TRANSCODERS];
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
> index 7357d99..77f58f2 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.c
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -27,7 +27,52 @@
>  
>  #include "intel_color_manager.h"
>  
> +int get_pipe_capabilities(struct drm_device *dev,
> +		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
> +{
> +	struct drm_property_blob *blob;
> +
> +	/*
> +	 * This function loads best capability for gamma correction
> +	 * For example:
> +	 * CHV best Gamma correction (CGM unit, 10 bit)
> +	 * has 257 entries, best degamma is 65 entries
> +	 */
> +	palette_caps->version = COLOR_STRUCT_VERSION;
> +	palette_caps->num_samples_after_ctm =
> +				INTEL_INFO(dev)->num_samples_after_ctm;
> +	palette_caps->num_samples_before_ctm =
> +				INTEL_INFO(dev)->num_samples_before_ctm;
> +	blob = drm_property_create_blob(dev, sizeof(struct drm_palette_caps),
> +			(const void *) palette_caps);
> +	if (IS_ERR_OR_NULL(blob)) {
> +		DRM_ERROR("Create blob for capabilities failed\n");
> +		return PTR_ERR(blob);
> +	}
> +
> +	return blob->base.id;
> +}
> +
>  void intel_attach_color_properties_to_crtc(struct drm_device *dev,
>  		struct drm_mode_object *mode_obj)
>  {
> +	struct drm_mode_config *config = &dev->mode_config;
> +	struct drm_palette_caps *palette_caps;
> +	struct drm_crtc *crtc;
> +	int capabilities_blob_id;
> +
> +	crtc = obj_to_crtc(mode_obj);
> +	palette_caps = kzalloc(sizeof(struct drm_palette_caps),
> +			GFP_KERNEL);
> +	capabilities_blob_id = get_pipe_capabilities(dev,
> +						palette_caps, crtc);
> +
> +	if (config->cm_crtc_palette_capabilities_property) {
> +		drm_object_attach_property(mode_obj,
> +			config->cm_crtc_palette_capabilities_property,
> +			capabilities_blob_id);

I didn't find any code to attach the before/after_ctm gamma properties and
the ctm property. Also I think a small helper would be really nice here in
the drm core:

drm_crtc_attach_cc_properties(struct drm_crtc *crtc,
			      int gamma_before_ctm,
			      bool ctm,
			      int gamm_after_ctm)

And then that directly constructs the palette caps from the given values
(if they're non-0) and also attaches the properties to the crtc (if
they're non-0 for gamma, treu for the ctm).

If that attach_property code really isn't there I wonder a bit how this
all works, we /should/ be filtering out properties which aren't attached
to the object.
-Daniel

> +		DRM_DEBUG_DRIVER("Capabilities attached to CRTC\n");
> +	}
> +
> +	kfree(palette_caps);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
> index 04c921d..1a42244 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.h
> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
> @@ -27,3 +27,5 @@
>  #include <drm/drmP.h>
>  #include <drm/drm_crtc_helper.h>
>  #include "i915_drv.h"
> +
> +#define COLOR_STRUCT_VERSION			1
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 00/23] Color Management for DRM

[Daniel Vetter]

On Wed, Sep 16, 2015 at 11:06:57PM +0530, Shashank Sharma wrote:
> This patch set adds Color Manager implementation in DRM layer. Color Manager
> is an extension in DRM framework to support color correction/enhancement.
> 
> Various Hardware platforms can support several color correction capabilities.
> Color Manager provides abstraction of these capabilities and allows a user
> space UI agent to correct/enhance the display using the DRM property interface.
> 
> How is this going to work?
> ==========================
> 1. This patch series adds a few new properties in DRM framework. These properties are:
>         a. color_capabilities property (type blob)
>         b. Color Transformation Matrix property for corrections like CSC (called CTM, type blob)
>         c. Palette correction properties for corrections like gamma fixup (called palette_correction, type blob) 
> 2. Also, this patch series adds few structures to indicate specifications of a property like size, no_of_samples for correction etc.
> 3. These properties are present in mode_config.
> 4. When the platform's display driver loads, it fills up the values of color_capabilities property using the standard structures (added in step 2).
>    For example, Intel's I915 driver adds following color correction capabilities:
>         a. gamma correction capability as palette correction property, with 257 correction coefficients and a max/min value
>         b. csc correction capability as CTM correction property, with 3x3 transformation matrix values and max/min values 
> 5. Now when userspace comes up, it queries the platform's color capabilities by doing a get_property() on color_capabilities DRM property
> 6. Reading the blob, the userspace understands the color capabilities of the platform.
>    For example, userspace will understand it can support:
>         a. palette_correction with 257 coefficients
>         b. CSC correction with 3x3 = 9 values 
> 7. To set color correction values, userspace:
>         a. creates a blob using the create_blob_ioctl in standard palette_correction structure format, with the correction values
>         b. calls the set_property_ioctl with the blob_id as value for the property 
> 8. Driver refers to the blob, gets the correction values and applies the correction in HW.
> 9. To get currently applied color correction values, userspace:
>         a. calls a get_property_ioctl on that color property
>         b. gets the blob_id for the currently applied correction from DRM infrastructure
>         c. gets the blob using get_blob_ioctl and hence the currently applied values
> 
> That's all! :)
> 
> About the patch series:
> =======================
> The patch series first adds the color management support in DRM layer.
> Then it adds the color management framework in I915 layer. 
> After that, it implements platform specific core color correction functios. 
> 
> Intel color manager registers color correction with DRM color manager in this way:
>  - CSC transformation is registered as CTM DRM property
>  - Gamma correction is registered as palette_after_ctm DRM property
>  - Degamma correction is registered as palette_before_ctm DRM property
> 
> Our thanks to all the reviewers who have given valuable comments in terms
> of design and implementation to our previous sets of patches. Special mention
> of thanks should go to Matt Roper for all his inputs/suggestions in implementation
> of this module, using DRM atomic CRTC commit path.
> 
> V2: Worked on review comments from Matt, Jim, Thierry, Rob.
> V3: Worked on review comments from Matt, Jim, Rob:
>  - Jim, Rob:
>    ========
>    Re-arranged the whole patch series in the sequence of features, currently:
>    First 5 patches add color management support in DRM layer
>    Next 7 patches add Intel color management framework in I915 driver
>    Next 5 patches add color correction for CHV (gamma, degamma and CSC)
>    Next 2 patches enable color management, by attaching the properties to CRTC(Matt)
>    Next 4 patches add color correction for BDW (gamma, degamma)
>  - Matt:
>    =====
>    Patch 3: Added refernce/unreference for blob
>    Patch 7: return -EINVAL and added debug message
>    Patch 8: check for valid blob, from create blob
>             moved call to intel_crtc_attach_color_prop in the end of full implementation (CHV)
>    Patch 9: DRM_ERROR->DRM_DEBUG for NULL blob case
>    Patch 13: Added static for internal functions
>    Patch 20-24: renamed gen9_* functions to bdw_*
>    Added new variables in device_info structure num_samples_after_ctm and num_samples_before_ctm
>    Added new function in patch 8 to load capabilities based on device_info across all platforms

Ok I finally got around to look at the kernel/userspace and drm core parts
in detail. There's a few minor bits to polish but nothing serious I think,
looks good overall (but I didn't really look at the i915 side).

One thing though is the kerneldoc part. You have some great detailed
comments in the structs about how this is all supposed to be used, but
afaict they're not pulled into the kerneldoc anywhere. Also we don't have
an overview section, but if you add the drm_crtc_attach_cc_properties
function like I suggested we could add that there (with links to all the
various structures).

There's also the giant drm props table but that one is a mess. My
suggestion would be to start a new DOC: section just for color correction,
with an integrated markdown table. Then add that as a subsection to KMS
property documentation. That way we'll have solid docs for this KMS
extension (something we should have for all the other stuff like rotation
or zpos too).

Thanks, Daniel

> 
> Shashank Sharma (23):
>   drm: Create Color Management DRM properties
>   drm: Add structure for querying palette color capabilities
>   drm: Add color correction blobs in CRTC state
>   drm: Add drm structures for palette color property
>   drm: Add structure to set/get a CTM color property
>   drm/i915: Add atomic set property interface for CRTC
>   drm/i915: Add atomic get property interface for CRTC
>   drm/i915: Create color management files
>   drm/i915: Register pipe color capabilities
>   drm/i915: Add gamma correction handlers
>   drm/i915: Add pipe deGamma correction handlers
>   drm/i915: Add pipe CSC correction handlers
>   drm/i915: CHV: Load gamma color correction values
>   drm/i915: CHV: Pipe level Gamma correction
>   drm/i915: CHV: Load degamma color correction values
>   drm/i915: CHV: Pipe level degamma correction
>   drm/i915: CHV: Pipe level CSC correction
>   drm/i915: Commit color changes to CRTC
>   drm/i915: Attach color properties to CRTC
>   drm/i915: BDW: Load gamma correction values
>   drm/i915: BDW: Pipe level Gamma correction
>   drm/i915: BDW: Load degamma correction values
>   drm/i915: BDW: Pipe level degamma correction
> 
>  drivers/gpu/drm/drm_atomic_helper.c        |  12 +
>  drivers/gpu/drm/drm_crtc.c                 |  26 +
>  drivers/gpu/drm/i915/Makefile              |   3 +-
>  drivers/gpu/drm/i915/i915_drv.c            |  17 +
>  drivers/gpu/drm/i915/i915_drv.h            |   2 +
>  drivers/gpu/drm/i915/i915_reg.h            |  41 +-
>  drivers/gpu/drm/i915/intel_atomic.c        |  56 ++
>  drivers/gpu/drm/i915/intel_color_manager.c | 852 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_color_manager.h |  99 ++++
>  drivers/gpu/drm/i915/intel_display.c       |   6 +
>  drivers/gpu/drm/i915/intel_drv.h           |  22 +
>  include/drm/drm_crtc.h                     |  11 +
>  include/uapi/drm/drm.h                     |  50 ++
>  13 files changed, 1195 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_color_manager.c
>  create mode 100644 drivers/gpu/drm/i915/intel_color_manager.h
> 
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 04/23] drm: Add drm structures for palette color property

[Emil Velikov]

On 22 September 2015 at 14:08, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Sep 16, 2015 at 11:07:01PM +0530, Shashank Sharma wrote:
>> From: Kausal Malladi <kausalmalladi@gmail.com>
>>
>> This patch adds new structures in DRM layer for Palette color
>> correction.These structures will be used by user space agents
>> to configure appropriate number of samples and Palette LUT for
>> a platform.
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
>> ---
>>  include/uapi/drm/drm.h | 27 +++++++++++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>>
>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>> index e3c642f..f72b916 100644
>> --- a/include/uapi/drm/drm.h
>> +++ b/include/uapi/drm/drm.h
[snip]
>> +struct drm_palette {
[snip]
> ... extending the ioctl struct at the end ...
Is this really going to work, considering that we currently have a
zero sized drm_r32g32b32 array at the end ?

Iirc someone mentioned that using a pointer to the data (over an
array) has drawbacks, although for the sake of me I cannot think of
any. Can anyone care to enlighten me, please ?

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

Re: [PATCH 04/23] drm: Add drm structures for palette color property

[Ville Syrjälä]

On Tue, Sep 22, 2015 at 02:53:54PM +0100, Emil Velikov wrote:
> On 22 September 2015 at 14:08, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Sep 16, 2015 at 11:07:01PM +0530, Shashank Sharma wrote:
> >> From: Kausal Malladi <kausalmalladi@gmail.com>
> >>
> >> This patch adds new structures in DRM layer for Palette color
> >> correction.These structures will be used by user space agents
> >> to configure appropriate number of samples and Palette LUT for
> >> a platform.
> >>
> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
> >> ---
> >>  include/uapi/drm/drm.h | 27 +++++++++++++++++++++++++++
> >>  1 file changed, 27 insertions(+)
> >>
> >> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> >> index e3c642f..f72b916 100644
> >> --- a/include/uapi/drm/drm.h
> >> +++ b/include/uapi/drm/drm.h
> [snip]
> >> +struct drm_palette {
> [snip]
> > ... extending the ioctl struct at the end ...
> Is this really going to work, considering that we currently have a
> zero sized drm_r32g32b32 array at the end ?

Well in this particular case it would be ugly. Sure, it can be done,
but we couldn't actually define the struct for it using C. Would be
a neat feature though. Someone should propose it for the next C
standard :)

In any case, for blobs I think we shouldn't extend them like ioctls.
In this case the blob is just an array of something, so the blob size
can of course vary depending how many elements are required for the
particual platform.

> Iirc someone mentioned that using a pointer to the data (over an
> array) has drawbacks, although for the sake of me I cannot think of
> any. Can anyone care to enlighten me, please ?

I guess an extensible array at the end of the struct is simply a nice
fit sometimes. Also makes it more obvious how much junk gets copied over
I suppose.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/23] drm: Add structure to set/get a CTM color property

[Ville Syrjälä]

On Wed, Sep 16, 2015 at 11:07:02PM +0530, Shashank Sharma wrote:
> From: Kausal Malladi <kausalmalladi@gmail.com>
> 
> Color Manager framework defines a color correction property for color
> space transformation and Gamut mapping. This property is called CTM (Color
> Transformation Matrix).
> 
> This patch adds a new structure in DRM layer for CTM.
> This structure can be used by all user space agents to
> configure CTM coefficients for color correction.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
> ---
>  include/uapi/drm/drm.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index f72b916..9580772 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -867,6 +867,18 @@ struct drm_palette {
>  	struct drm_r32g32b32 lut[0];
>  };
>  
> +struct drm_ctm {
> +	/* Structure version. Should be 1 currently */
> +	__u32 version;
> +	/*
> +	 * Each value is in S31.32 format.
> +	 * This is 3x3 matrix in row major format.

I'm not sure that exlains things. Maybe sketch the math equation here
(something like what I put in i915_reg.h for the CHV sprite CSC). Which
reminds me: what are we going to do about the pre/post offset stuff? I
was sort of expecting to see it here, but maybe you put those into
separate properties?

> +	 * Integer part will be clipped to nearest
> +	 * max/min boundary as supported by the HW platform.
> +	 */
> +	__s64 ctm_coeff[9];

ctm_ prefix seems superfluous. Also could maybe use some comment
explaining what ctm is?

> +};
> +
>  /* typedef area */
>  #ifndef __KERNEL__
>  typedef struct drm_clip_rect drm_clip_rect_t;
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/23] drm: Add drm structures for palette color property

[Emil Velikov]

Hi Ville,

On 22 September 2015 at 16:00, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Tue, Sep 22, 2015 at 02:53:54PM +0100, Emil Velikov wrote:
>> On 22 September 2015 at 14:08, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > On Wed, Sep 16, 2015 at 11:07:01PM +0530, Shashank Sharma wrote:
>> >> From: Kausal Malladi <kausalmalladi@gmail.com>
>> >>
>> >> This patch adds new structures in DRM layer for Palette color
>> >> correction.These structures will be used by user space agents
>> >> to configure appropriate number of samples and Palette LUT for
>> >> a platform.
>> >>
>> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> >> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
>> >> ---
>> >>  include/uapi/drm/drm.h | 27 +++++++++++++++++++++++++++
>> >>  1 file changed, 27 insertions(+)
>> >>
>> >> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>> >> index e3c642f..f72b916 100644
>> >> --- a/include/uapi/drm/drm.h
>> >> +++ b/include/uapi/drm/drm.h
>> [snip]
>> >> +struct drm_palette {
>> [snip]
>> > ... extending the ioctl struct at the end ...
>> Is this really going to work, considering that we currently have a
>> zero sized drm_r32g32b32 array at the end ?
>
> Well in this particular case it would be ugly. Sure, it can be done,
> but we couldn't actually define the struct for it using C. Would be
> a neat feature though. Someone should propose it for the next C
> standard :)
>
> In any case, for blobs I think we shouldn't extend them like ioctls.
> In this case the blob is just an array of something, so the blob size
> can of course vary depending how many elements are required for the
> particual platform.
>
Sure it makes sense on its own. I'm curious how likely it will be for
people to get confused/carried away and miss that part. Just a food
for thought.

>> Iirc someone mentioned that using a pointer to the data (over an
>> array) has drawbacks, although for the sake of me I cannot think of
>> any. Can anyone care to enlighten me, please ?
>
> I guess an extensible array at the end of the struct is simply a nice
> fit sometimes. Also makes it more obvious how much junk gets copied over
> I suppose.
>
So no serious 'issues' there, but it's mostly done for convenience.
Thanks for clarifying.

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

Re: [PATCH 02/23] drm: Add structure for querying palette color capabilities

[Sharma, Shashank]

Hi Matt, Daniel
Addressing the review comments from both of you here.

Regards
Shashank

On 9/22/2015 6:32 PM, Daniel Vetter wrote:
> On Wed, Sep 16, 2015 at 10:51:31AM -0700, Matt Roper wrote:
>> On Wed, Sep 16, 2015 at 11:06:59PM +0530, Shashank Sharma wrote:
>>> From: Kausal Malladi <kausalmalladi@gmail.com>
>>>
>>> The DRM color management framework is targeting various hardware
>>> platforms and drivers. Different platforms can have different color
>>> correction and enhancement capabilities.
>>>
>>> A commom user space application can query these capabilities using the
>>> DRM property interface. Each driver can fill this property with its
>>> platform's palette color capabilities.
>>>
>>> This patch adds new structure in DRM layer for querying palette color
>>> capabilities. This structure will be used by all user space
>>> agents to configure appropriate color configurations.
>>>
>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
>>
>> I think you provided an explanation on a previous code review cycle, but
>> I forget the details now...what's the benefit to using a blob for caps
>> rather than having these be individual properties?  Individual
>> properties seems more natural to me, but I think you had a justification
>> for blobbing them together; that reasoning would be good to include in
>> the commit message.
>
> Yeah I'm leaning slightly towards individual props too, that would give us
> a bit more freedom with placing them (e.g. if someone comes up with funky
> hw where before_ctm and ctm are per-plane and after_ctm is on the crtc,
> with only some planes support the before_ctm gamma table).
This was the part where we spent most of the time during the design 
review, and the reason we came up for this was:
- This is a read only property, which userspace would like to read only 
once, and cache the information. It was also Gary's opinion to keep this 
as single blob for all.
- Making individual property needs more information to be provided to
user space.
- This is a blob only for pipe level capabilities, the plane level blob 
will be separate from this.
- We can handle this HW also, by loading proper plane and pipe level 
capability blob. This is more convenient to have all the capabilities 
together at the same place, than keep on querying the same.
>
> Also if you do per-prop properties instead of the blob you can drop the
> version/reserved fields, since properties are inheritedly designed to be
> extendible. So no need to revision them again (it only leads to more code
> that might break).
> -Daniel
>
We are anyways planning to drop the version, as per Ville's comment.
- Shashank
>>
>>
>> Matt
>>
>>> ---
>>>   include/uapi/drm/drm.h | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>>>
>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>>> index 3801584..e3c642f 100644
>>> --- a/include/uapi/drm/drm.h
>>> +++ b/include/uapi/drm/drm.h
>>> @@ -829,6 +829,17 @@ struct drm_event_vblank {
>>>   	__u32 reserved;
>>>   };
>>>
>>> +struct drm_palette_caps {
>>> +	/* Structure version. Should be 1 currently */
>>> +	__u32 version;
>>> +	/* For padding and future use */
>>> +	__u32 reserved;
>>> +	/* This may be 0 if not supported. e.g. plane palette or VLV pipe */
>>> +	__u32 num_samples_before_ctm;
>>> +	/* This will be non-zero for pipe. May be zero for planes on some HW */
>>> +	__u32 num_samples_after_ctm;
>>> +};
>>> +
>>>   /* typedef area */
>>>   #ifndef __KERNEL__
>>>   typedef struct drm_clip_rect drm_clip_rect_t;
>>> --
>>> 1.9.1
>>>
>>
>> --
>> Matt Roper
>> Graphics Software Engineer
>> IoTG Platform Enabling & Development
>> Intel Corporation
>> (916) 356-2795
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/23] drm: Add drm structures for palette color property

[Sharma, Shashank]

Regards
Shashank

On 9/22/2015 6:38 PM, Daniel Vetter wrote:
> On Wed, Sep 16, 2015 at 11:07:01PM +0530, Shashank Sharma wrote:
>> From: Kausal Malladi <kausalmalladi@gmail.com>
>>
>> This patch adds new structures in DRM layer for Palette color
>> correction.These structures will be used by user space agents
>> to configure appropriate number of samples and Palette LUT for
>> a platform.
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
>> ---
>>   include/uapi/drm/drm.h | 27 +++++++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>> index e3c642f..f72b916 100644
>> --- a/include/uapi/drm/drm.h
>> +++ b/include/uapi/drm/drm.h
>> @@ -840,6 +840,33 @@ struct drm_palette_caps {
>>   	__u32 num_samples_after_ctm;
>>   };
>>
>> +struct drm_r32g32b32 {
>> +	/*
>> +	 * Data is in U8.24 fixed point format.
>> +	 * All platforms support values within [0, 1.0] range,
>> +	 * for Red, Green and Blue colors.
>> +	 */
>> +	__u32 r32;
>> +	__u32 g32;
>> +	__u32 b32;
>
> It's not strictly required, but adding a __u32 reserved here to align the
> struct to 64 bits seems good imo. Slight overhead but meh about that.
Humm, ok, we can check this out.
>
>> +};
>> +
>> +struct drm_palette {
>> +	/* Structure version. Should be 1 currently */
>> +	__u32 version;
>
> Definitely great practice to take compat into account and definitely
> needed for the first design using ioctls but I don't think we need this
> here. Properties are already extinsible themselves: We can just greate a
> "ctm-v2", "ctm-v3" if the layout changes, and since the actual ctm matrix
> is stored in the drm_crtc_state any compat code on the kernel will be
> shared.
>
> Aside: For an ioctl the recommended way to handle backwards compat and
> extensions in drm is with a flags bitfield. That's more flexible than a
> linear version field, and extending the ioctl struct at the end is already
> handled by the drm core in a transparent fashion (it 0-fills either kernel
> or userspace side).
>
Agree, we will drop this. Do you think we should add a flags field, or 
is it ok without it ?
>> +	/*
>> +	 * This has to be a supported value during get call.
>> +	 * Feature will be disabled if this is 0 while set
>> +	 */
>> +	__u32 num_samples;
>
> blob properties already have a size, storing it again in the blob is
> redundnant. Instead I think a small helper to get the number of samples
> for a given gamma table blob would be needed.
>
> Cheers, Daniel
Please note that they are different. One is the size of blob and other 
one is the num_samples supported by the property, in the current 
correction mode. If you check the design doc, num_sample serves the 
purpose of deciding which correction mode to be applied also. fox ex, 
for gamma, num_samples=0 indicates disable gamma, whereas 
num_samples=512 indicates split gamma mode.

Shashank
>
>> +	/*
>> +	 * Starting of palette LUT in R32G32B32 format.
>> +	 * Each of RGB value is in U8.24 fixed point format.
>> +	 * Actual number of samples will depend upon num_samples
>> +	 */
>> +	struct drm_r32g32b32 lut[0];
>> +};
>> +
>>   /* typedef area */
>>   #ifndef __KERNEL__
>>   typedef struct drm_clip_rect drm_clip_rect_t;
>> --
>> 1.9.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/23] drm: Add structure to set/get a CTM color property

[Sharma, Shashank]

Regards
Shashank

On 9/22/2015 6:38 PM, Daniel Vetter wrote:
> On Wed, Sep 16, 2015 at 11:07:02PM +0530, Shashank Sharma wrote:
>> From: Kausal Malladi <kausalmalladi@gmail.com>
>>
>> Color Manager framework defines a color correction property for color
>> space transformation and Gamut mapping. This property is called CTM (Color
>> Transformation Matrix).
>>
>> This patch adds a new structure in DRM layer for CTM.
>> This structure can be used by all user space agents to
>> configure CTM coefficients for color correction.
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
>> ---
>>   include/uapi/drm/drm.h | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>> index f72b916..9580772 100644
>> --- a/include/uapi/drm/drm.h
>> +++ b/include/uapi/drm/drm.h
>> @@ -867,6 +867,18 @@ struct drm_palette {
>>   	struct drm_r32g32b32 lut[0];
>>   };
>>
>> +struct drm_ctm {
>> +	/* Structure version. Should be 1 currently */
>> +	__u32 version;
>
> Same thing here, no version needed for properties.
Agree.
-Shashank
>
>> +	/*
>> +	 * Each value is in S31.32 format.
>> +	 * This is 3x3 matrix in row major format.
>> +	 * Integer part will be clipped to nearest
>> +	 * max/min boundary as supported by the HW platform.
>> +	 */
>> +	__s64 ctm_coeff[9];
>> +};
>> +
>>   /* typedef area */
>>   #ifndef __KERNEL__
>>   typedef struct drm_clip_rect drm_clip_rect_t;
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 10/23] drm/i915: Add gamma correction handlers

[Sharma, Shashank]

Regards
Shashank

On 9/22/2015 6:45 PM, Daniel Vetter wrote:
> On Wed, Sep 16, 2015 at 11:07:07PM +0530, Shashank Sharma wrote:
>> I915 driver registers gamma correction as palette correction
>> property with DRM layer. This patch adds set_property() and get_property()
>> handlers for pipe level gamma correction.
>>
>> The set function attaches the Gamma correction blob to CRTC state, these
>> values will be committed during atomic commit.
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_atomic.c        | 20 ++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_color_manager.c | 21 +++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_drv.h           |  5 +++++
>>   3 files changed, 46 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
>> index 500d2998..0b61fef 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -315,6 +315,13 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
>>   				   struct drm_property *property,
>>   				   uint64_t val)
>>   {
>> +	struct drm_device *dev = crtc->dev;
>> +	struct drm_mode_config *config = &dev->mode_config;
>> +
>> +	if (property == config->cm_palette_after_ctm_property)
>> +		return intel_color_manager_set_pipe_gamma(dev, state,
>> +				&crtc->base, val);
>> +
>>   	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
>>   	return -EINVAL;
>>   }
>> @@ -324,6 +331,19 @@ int intel_crtc_atomic_get_property(struct drm_crtc *crtc,
>>   				   struct drm_property *property,
>>   				   uint64_t *val)
>>   {
>> +	struct drm_device *dev = crtc->dev;
>> +	struct drm_mode_config *config = &dev->mode_config;
>> +
>> +	if (property == config->cm_palette_after_ctm_property) {
>> +		*val = (state->palette_after_ctm_blob) ?
>> +			state->palette_after_ctm_blob->base.id : 0;
>> +		goto found;
>> +	}
>
> Since color manager properties are meant as a new standardize KMS
> extension (we put them into the core drm_crtc_state) the get/set support
> should also be in the core. See e.g. how the rotation property is handled
> in drm_atomic_plane_get/set_property. So all this code should be added to
> drm_atomic_crtc_get/set_property.
Thanks, sounds like a good one. Will move this.
>
>
>> +
>>   	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
>>   	return -EINVAL;
>> +
>> +found:
>> +	DRM_DEBUG_KMS("Found property %s\n", property->name);
>> +	return 0;
>>   }
>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
>> index 77f58f2..9421bb6 100644
>> --- a/drivers/gpu/drm/i915/intel_color_manager.c
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
>> @@ -27,6 +27,27 @@
>>
>>   #include "intel_color_manager.h"
>>
>> +int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
>> +		struct drm_crtc_state *crtc_state,
>> +		struct drm_mode_object *obj, uint32_t blob_id)
>> +{
>> +	struct drm_property_blob *blob;
>> +
>> +	blob = drm_property_lookup_blob(dev, blob_id);
>> +	if (!blob) {
>> +		DRM_DEBUG_KMS("Invalid Blob ID\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (crtc_state->palette_after_ctm_blob)
>> +		drm_property_unreference_blob(
>> +			crtc_state->palette_after_ctm_blob);
>> +
>> +	/* Attach the blob to be committed in state */
>> +	crtc_state->palette_after_ctm_blob = blob;
>> +	return 0;
>> +}
>
> What is this used for? It looks a bit like legacy property code, and we
> have a generic helper to make that happen
> (drm_atomic_helper_crtc_set_property).
>
No Daniel, its not the legacy part. Please check atomic_begin() part in 
the next patches. This is like the top half of atomic set_property for 
CRTC and will be applicable for any property. As you already know, to 
set a property:
- userspace creates a blob, and sends the blob id via set_property() 
interface
- set_property() saves the blob_id in the corresponding pointer space in 
the crtc_state()
- From the atomic_commit_begin() we will extract the values from blob 
and start committing to the registers. Its kind of bottom_half() of 
atomic set_property for CRTC.
> Generally please don't add functions/structs without also adding a user,
> it means that reviewers have to constantly jump around in your patch
> series to figure out how something is used. Instead if you want to split
> things up really fine add a stub function frist (but including relevant
> callers) and then fill out the bits separately.
I think the above explanation should make it clear.
> -Daniel
>
>> +
>>   int get_pipe_capabilities(struct drm_device *dev,
>>   		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
>>   {
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index e27e754..d0193e2 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1466,4 +1466,9 @@ void intel_plane_destroy_state(struct drm_plane *plane,
>>   			       struct drm_plane_state *state);
>>   extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
>>
>> +/* intel_color_manager.c */
>> +int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
>> +		struct drm_crtc_state *crtc_state,
>> +		struct drm_mode_object *obj, uint32_t blob_id);
>> +
>>   #endif /* __INTEL_DRV_H__ */
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 09/23] drm/i915: Register pipe color capabilities

[Sharma, Shashank]

Regards
Shashank

On 9/22/2015 6:54 PM, Daniel Vetter wrote:
> On Wed, Sep 16, 2015 at 11:07:06PM +0530, Shashank Sharma wrote:
>> DRM color manager contains these color properties:
>>
>> 1. "crtc_palette_capabilities_property": to allow a
>> core driver to load and showcase its color correction
>> capabilities to user space.
>> 2. "ctm": Color transformation matrix property, where a
>> color transformation matrix of 9 correction values gets
>> applied as correction.
>> 3. "palette_before_ctm": for corrections which get applied
>> beore color transformation matrix correction.
>> 4. "palette_after_ctm": for corrections which get applied
>> after color transformation matrix correction.
>>
>> Intel color manager registers:
>> 1. Gamma correction property as "palette_after_ctm" property
>> 2. Degamma correction capability as "palette_bafore_ctm" property
>> capability as "palette_after_ctm" DRM color property hook.
>> 3. CSC as "ctm" property.
>>
>> This patch does the following:
>> 1. Add a function which loads the platform's color correction
>> capabilities in the cm_crtc_palette_capabilities_property structure.
>> 2. Attaches the cm_crtc_palette_capabilities_property to every CRTC
>> getting initiaized.
>> 3. Adds two new parameters "num_samples_after_ctm" and
>> "num_samples_before_ctm" in intel_device_info as gamma and
>> degamma coefficients vary per platform basis.
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h            |  2 ++
>>   drivers/gpu/drm/i915/intel_color_manager.c | 45 ++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_color_manager.h |  2 ++
>>   3 files changed, 49 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 3bf8a9b..22de2cb 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -798,6 +798,8 @@ struct intel_device_info {
>>   	u8 num_sprites[I915_MAX_PIPES];
>>   	u8 gen;
>>   	u8 ring_mask; /* Rings supported by the HW */
>> +	u16 num_samples_after_ctm;
>> +	u16 num_samples_before_ctm;
>>   	DEV_INFO_FOR_EACH_FLAG(DEFINE_FLAG, SEP_SEMICOLON);
>>   	/* Register offsets for the various display pipes and transcoders */
>>   	int pipe_offsets[I915_MAX_TRANSCODERS];
>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
>> index 7357d99..77f58f2 100644
>> --- a/drivers/gpu/drm/i915/intel_color_manager.c
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
>> @@ -27,7 +27,52 @@
>>
>>   #include "intel_color_manager.h"
>>
>> +int get_pipe_capabilities(struct drm_device *dev,
>> +		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
>> +{
>> +	struct drm_property_blob *blob;
>> +
>> +	/*
>> +	 * This function loads best capability for gamma correction
>> +	 * For example:
>> +	 * CHV best Gamma correction (CGM unit, 10 bit)
>> +	 * has 257 entries, best degamma is 65 entries
>> +	 */
>> +	palette_caps->version = COLOR_STRUCT_VERSION;
>> +	palette_caps->num_samples_after_ctm =
>> +				INTEL_INFO(dev)->num_samples_after_ctm;
>> +	palette_caps->num_samples_before_ctm =
>> +				INTEL_INFO(dev)->num_samples_before_ctm;
>> +	blob = drm_property_create_blob(dev, sizeof(struct drm_palette_caps),
>> +			(const void *) palette_caps);
>> +	if (IS_ERR_OR_NULL(blob)) {
>> +		DRM_ERROR("Create blob for capabilities failed\n");
>> +		return PTR_ERR(blob);
>> +	}
>> +
>> +	return blob->base.id;
>> +}
>> +
>>   void intel_attach_color_properties_to_crtc(struct drm_device *dev,
>>   		struct drm_mode_object *mode_obj)
>>   {
>> +	struct drm_mode_config *config = &dev->mode_config;
>> +	struct drm_palette_caps *palette_caps;
>> +	struct drm_crtc *crtc;
>> +	int capabilities_blob_id;
>> +
>> +	crtc = obj_to_crtc(mode_obj);
>> +	palette_caps = kzalloc(sizeof(struct drm_palette_caps),
>> +			GFP_KERNEL);
>> +	capabilities_blob_id = get_pipe_capabilities(dev,
>> +						palette_caps, crtc);
>> +
>> +	if (config->cm_crtc_palette_capabilities_property) {
>> +		drm_object_attach_property(mode_obj,
>> +			config->cm_crtc_palette_capabilities_property,
>> +			capabilities_blob_id);
>
> I didn't find any code to attach the before/after_ctm gamma properties and
> the ctm property. Also I think a small helper would be really nice here in
> the drm core:
>
Patch 15,16,17 adds those for gamma, degamma and CSC. This is just for 
the capabilities property.
> drm_crtc_attach_cc_properties(struct drm_crtc *crtc,
> 			      int gamma_before_ctm,
> 			      bool ctm,
> 			      int gamm_after_ctm)
>
> And then that directly constructs the palette caps from the given values
> (if they're non-0) and also attaches the properties to the crtc (if
> they're non-0 for gamma, treu for the ctm).
>
> If that attach_property code really isn't there I wonder a bit how this
> all works, we /should/ be filtering out properties which aren't attached
> to the object.
As I mentioned in the cover-letter, here is the sequence:
First 5 patches add color management support in DRM layer
Next 7 patches add Intel color management framework in I915 driver (This 
is still the framework part)
Next 5 patches add color correction for CHV (gamma, degamma and CSC)
(Here comes the core property and enabling part)
Next 2 patches enable color management, by attaching the properties to
  CRTC(Matt's comment on enable property code only when the job is done)
Next 4 patches add color correction for BDW (gamma, degamma)
> -Daniel
>
>> +		DRM_DEBUG_DRIVER("Capabilities attached to CRTC\n");
>> +	}
>> +
>> +	kfree(palette_caps);
>>   }
>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
>> index 04c921d..1a42244 100644
>> --- a/drivers/gpu/drm/i915/intel_color_manager.h
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
>> @@ -27,3 +27,5 @@
>>   #include <drm/drmP.h>
>>   #include <drm/drm_crtc_helper.h>
>>   #include "i915_drv.h"
>> +
>> +#define COLOR_STRUCT_VERSION			1
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 00/23] Color Management for DRM

[Sharma, Shashank]

Regards
Shashank

On 9/22/2015 6:57 PM, Daniel Vetter wrote:
> On Wed, Sep 16, 2015 at 11:06:57PM +0530, Shashank Sharma wrote:
>> This patch set adds Color Manager implementation in DRM layer. Color Manager
>> is an extension in DRM framework to support color correction/enhancement.
>>
>> Various Hardware platforms can support several color correction capabilities.
>> Color Manager provides abstraction of these capabilities and allows a user
>> space UI agent to correct/enhance the display using the DRM property interface.
>>
>> How is this going to work?
>> ==========================
>> 1. This patch series adds a few new properties in DRM framework. These properties are:
>>          a. color_capabilities property (type blob)
>>          b. Color Transformation Matrix property for corrections like CSC (called CTM, type blob)
>>          c. Palette correction properties for corrections like gamma fixup (called palette_correction, type blob)
>> 2. Also, this patch series adds few structures to indicate specifications of a property like size, no_of_samples for correction etc.
>> 3. These properties are present in mode_config.
>> 4. When the platform's display driver loads, it fills up the values of color_capabilities property using the standard structures (added in step 2).
>>     For example, Intel's I915 driver adds following color correction capabilities:
>>          a. gamma correction capability as palette correction property, with 257 correction coefficients and a max/min value
>>          b. csc correction capability as CTM correction property, with 3x3 transformation matrix values and max/min values
>> 5. Now when userspace comes up, it queries the platform's color capabilities by doing a get_property() on color_capabilities DRM property
>> 6. Reading the blob, the userspace understands the color capabilities of the platform.
>>     For example, userspace will understand it can support:
>>          a. palette_correction with 257 coefficients
>>          b. CSC correction with 3x3 = 9 values
>> 7. To set color correction values, userspace:
>>          a. creates a blob using the create_blob_ioctl in standard palette_correction structure format, with the correction values
>>          b. calls the set_property_ioctl with the blob_id as value for the property
>> 8. Driver refers to the blob, gets the correction values and applies the correction in HW.
>> 9. To get currently applied color correction values, userspace:
>>          a. calls a get_property_ioctl on that color property
>>          b. gets the blob_id for the currently applied correction from DRM infrastructure
>>          c. gets the blob using get_blob_ioctl and hence the currently applied values
>>
>> That's all! :)
>>
>> About the patch series:
>> =======================
>> The patch series first adds the color management support in DRM layer.
>> Then it adds the color management framework in I915 layer.
>> After that, it implements platform specific core color correction functios.
>>
>> Intel color manager registers color correction with DRM color manager in this way:
>>   - CSC transformation is registered as CTM DRM property
>>   - Gamma correction is registered as palette_after_ctm DRM property
>>   - Degamma correction is registered as palette_before_ctm DRM property
>>
>> Our thanks to all the reviewers who have given valuable comments in terms
>> of design and implementation to our previous sets of patches. Special mention
>> of thanks should go to Matt Roper for all his inputs/suggestions in implementation
>> of this module, using DRM atomic CRTC commit path.
>>
>> V2: Worked on review comments from Matt, Jim, Thierry, Rob.
>> V3: Worked on review comments from Matt, Jim, Rob:
>>   - Jim, Rob:
>>     ========
>>     Re-arranged the whole patch series in the sequence of features, currently:
>>     First 5 patches add color management support in DRM layer
>>     Next 7 patches add Intel color management framework in I915 driver
>>     Next 5 patches add color correction for CHV (gamma, degamma and CSC)
>>     Next 2 patches enable color management, by attaching the properties to CRTC(Matt)
>>     Next 4 patches add color correction for BDW (gamma, degamma)
>>   - Matt:
>>     =====
>>     Patch 3: Added refernce/unreference for blob
>>     Patch 7: return -EINVAL and added debug message
>>     Patch 8: check for valid blob, from create blob
>>              moved call to intel_crtc_attach_color_prop in the end of full implementation (CHV)
>>     Patch 9: DRM_ERROR->DRM_DEBUG for NULL blob case
>>     Patch 13: Added static for internal functions
>>     Patch 20-24: renamed gen9_* functions to bdw_*
>>     Added new variables in device_info structure num_samples_after_ctm and num_samples_before_ctm
>>     Added new function in patch 8 to load capabilities based on device_info across all platforms
>
> Ok I finally got around to look at the kernel/userspace and drm core parts
> in detail. There's a few minor bits to polish but nothing serious I think,
> looks good overall (but I didn't really look at the i915 side).
>
> One thing though is the kerneldoc part. You have some great detailed
> comments in the structs about how this is all supposed to be used, but
> afaict they're not pulled into the kerneldoc anywhere. Also we don't have
> an overview section, but if you add the drm_crtc_attach_cc_properties
> function like I suggested we could add that there (with links to all the
> various structures).
>
Now, after the explanation in the corresponding patch, do you still find 
a need for the function ?
> There's also the giant drm props table but that one is a mess. My
> suggestion would be to start a new DOC: section just for color correction,
> with an integrated markdown table. Then add that as a subsection to KMS
> property documentation. That way we'll have solid docs for this KMS
> extension (something we should have for all the other stuff like rotation
> or zpos too).
>
Yes, sure, I will do this.
> Thanks, Daniel
>
>>
>> Shashank Sharma (23):
>>    drm: Create Color Management DRM properties
>>    drm: Add structure for querying palette color capabilities
>>    drm: Add color correction blobs in CRTC state
>>    drm: Add drm structures for palette color property
>>    drm: Add structure to set/get a CTM color property
>>    drm/i915: Add atomic set property interface for CRTC
>>    drm/i915: Add atomic get property interface for CRTC
>>    drm/i915: Create color management files
>>    drm/i915: Register pipe color capabilities
>>    drm/i915: Add gamma correction handlers
>>    drm/i915: Add pipe deGamma correction handlers
>>    drm/i915: Add pipe CSC correction handlers
>>    drm/i915: CHV: Load gamma color correction values
>>    drm/i915: CHV: Pipe level Gamma correction
>>    drm/i915: CHV: Load degamma color correction values
>>    drm/i915: CHV: Pipe level degamma correction
>>    drm/i915: CHV: Pipe level CSC correction
>>    drm/i915: Commit color changes to CRTC
>>    drm/i915: Attach color properties to CRTC
>>    drm/i915: BDW: Load gamma correction values
>>    drm/i915: BDW: Pipe level Gamma correction
>>    drm/i915: BDW: Load degamma correction values
>>    drm/i915: BDW: Pipe level degamma correction
>>
>>   drivers/gpu/drm/drm_atomic_helper.c        |  12 +
>>   drivers/gpu/drm/drm_crtc.c                 |  26 +
>>   drivers/gpu/drm/i915/Makefile              |   3 +-
>>   drivers/gpu/drm/i915/i915_drv.c            |  17 +
>>   drivers/gpu/drm/i915/i915_drv.h            |   2 +
>>   drivers/gpu/drm/i915/i915_reg.h            |  41 +-
>>   drivers/gpu/drm/i915/intel_atomic.c        |  56 ++
>>   drivers/gpu/drm/i915/intel_color_manager.c | 852 +++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_color_manager.h |  99 ++++
>>   drivers/gpu/drm/i915/intel_display.c       |   6 +
>>   drivers/gpu/drm/i915/intel_drv.h           |  22 +
>>   include/drm/drm_crtc.h                     |  11 +
>>   include/uapi/drm/drm.h                     |  50 ++
>>   13 files changed, 1195 insertions(+), 2 deletions(-)
>>   create mode 100644 drivers/gpu/drm/i915/intel_color_manager.c
>>   create mode 100644 drivers/gpu/drm/i915/intel_color_manager.h
>>
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/23] drm: Create Color Management DRM properties

[Sharma, Shashank]

Sure Matt, 
We are planning to add documentation for color properties, as suggested by you and Daniel. 

Regards
Shashank
-----Original Message-----
From: Roper, Matthew D 
Sent: Wednesday, September 16, 2015 11:21 PM
To: Sharma, Shashank
Cc: Bish, Jim; Bradford, Robert; Smith, Gary K; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Vetter, Daniel; Matheson, Annie J; Mukherjee, Indranil; Palleti, Avinash Reddy; kausalmalladi@gmail.com
Subject: Re: [PATCH 01/23] drm: Create Color Management DRM properties

On Wed, Sep 16, 2015 at 11:06:58PM +0530, Shashank Sharma wrote:
> From: Kausal Malladi <kausalmalladi@gmail.com>
> 
> Color Management is an extension to Kernel display framework. It 
> allows abstraction of hardware color correction and enhancement 
> capabilities by virtue of DRM properties.
> 
> This patch initializes color management framework by :
> 1. Introducing new pointers in DRM mode_config structure to
>    carry CTM and Palette color correction properties.
> 2. Creating these DRM properties in DRM standard properties creation
>    sequence.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>

We should probably update the property section of Documentation/DocBook/drm.tmpl with this patch as well to include these new properties in the table (that docbook ultimately generates documentation that looks like https://kernel.org/doc/htmldocs/drm/drm-kms-properties.html ).

One minor note:  people not involved in color management probably won't immediately figure out what "CTM" stands for, so you might want to just add a comment somewhere that spells out the full "color transformation matrix" term.


Matt


> ---
>  drivers/gpu/drm/drm_crtc.c | 26 ++++++++++++++++++++++++++
>  include/drm/drm_crtc.h     |  6 ++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c 
> index 9b9c4b4..d809c67 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1472,6 +1472,32 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>  		return -ENOMEM;
>  	dev->mode_config.prop_mode_id = prop;
>  
> +	/* Color Management properties */
> +	prop = drm_property_create(dev,
> +			DRM_MODE_PROP_BLOB | DRM_MODE_PROP_IMMUTABLE,
> +			"CRTC_PALETTE_CAPABILITIES", 0);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.cm_crtc_palette_capabilities_property = prop;
> +
> +	prop = drm_property_create(dev,
> +			DRM_MODE_PROP_BLOB, "PALETTE_AFTER_CTM", 0);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.cm_palette_after_ctm_property = prop;
> +
> +	prop = drm_property_create(dev,
> +			DRM_MODE_PROP_BLOB, "PALETTE_BEFORE_CTM", 0);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.cm_palette_before_ctm_property = prop;
> +
> +	prop = drm_property_create(dev,
> +			DRM_MODE_PROP_BLOB, "CTM", 0);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.cm_ctm_property = prop;
> +
>  	return 0;
>  }
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 
> c0366e9..c35531e 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1153,6 +1153,12 @@ struct drm_mode_config {
>  	struct drm_property *suggested_x_property;
>  	struct drm_property *suggested_y_property;
>  
> +	/* Color Management Properties */
> +	struct drm_property *cm_crtc_palette_capabilities_property;
> +	struct drm_property *cm_palette_before_ctm_property;
> +	struct drm_property *cm_palette_after_ctm_property;
> +	struct drm_property *cm_ctm_property;
> +
>  	/* dumb ioctl parameters */
>  	uint32_t preferred_depth, prefer_shadow;
>  
> --
> 1.9.1
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/23] drm: Add structure for querying palette color capabilities

[Smith, Gary K]

Given that its only one word of info per LUT, I'm OK with it being two separate properties. 
I believe it was much more complex previously with a lot more info per LUT, which is probably why I preferred a blob.

Thanks
Gary

-----Original Message-----
From: Sharma, Shashank 
Sent: Wednesday, September 23, 2015 9:10 AM
To: Daniel Vetter; Roper, Matthew D
Cc: Matheson, Annie J; Bradford, Robert; Palleti, Avinash Reddy; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Mukherjee, Indranil; Bish, Jim; Smith, Gary K; kausalmalladi@gmail.com; Vetter, Daniel
Subject: Re: [PATCH 02/23] drm: Add structure for querying palette color capabilities

Hi Matt, Daniel
Addressing the review comments from both of you here.

Regards
Shashank

On 9/22/2015 6:32 PM, Daniel Vetter wrote:
> On Wed, Sep 16, 2015 at 10:51:31AM -0700, Matt Roper wrote:
>> On Wed, Sep 16, 2015 at 11:06:59PM +0530, Shashank Sharma wrote:
>>> From: Kausal Malladi <kausalmalladi@gmail.com>
>>>
>>> The DRM color management framework is targeting various hardware 
>>> platforms and drivers. Different platforms can have different color 
>>> correction and enhancement capabilities.
>>>
>>> A commom user space application can query these capabilities using 
>>> the DRM property interface. Each driver can fill this property with 
>>> its platform's palette color capabilities.
>>>
>>> This patch adds new structure in DRM layer for querying palette 
>>> color capabilities. This structure will be used by all user space 
>>> agents to configure appropriate color configurations.
>>>
>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
>>
>> I think you provided an explanation on a previous code review cycle, 
>> but I forget the details now...what's the benefit to using a blob for 
>> caps rather than having these be individual properties?  Individual 
>> properties seems more natural to me, but I think you had a 
>> justification for blobbing them together; that reasoning would be 
>> good to include in the commit message.
>
> Yeah I'm leaning slightly towards individual props too, that would 
> give us a bit more freedom with placing them (e.g. if someone comes up 
> with funky hw where before_ctm and ctm are per-plane and after_ctm is 
> on the crtc, with only some planes support the before_ctm gamma table).
This was the part where we spent most of the time during the design review, and the reason we came up for this was:
- This is a read only property, which userspace would like to read only once, and cache the information. It was also Gary's opinion to keep this as single blob for all.
- Making individual property needs more information to be provided to user space.
- This is a blob only for pipe level capabilities, the plane level blob will be separate from this.
- We can handle this HW also, by loading proper plane and pipe level capability blob. This is more convenient to have all the capabilities together at the same place, than keep on querying the same.
>
> Also if you do per-prop properties instead of the blob you can drop 
> the version/reserved fields, since properties are inheritedly designed 
> to be extendible. So no need to revision them again (it only leads to 
> more code that might break).
> -Daniel
>
We are anyways planning to drop the version, as per Ville's comment.
- Shashank
>>
>>
>> Matt
>>
>>> ---
>>>   include/uapi/drm/drm.h | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>>>
>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 
>>> 3801584..e3c642f 100644
>>> --- a/include/uapi/drm/drm.h
>>> +++ b/include/uapi/drm/drm.h
>>> @@ -829,6 +829,17 @@ struct drm_event_vblank {
>>>   	__u32 reserved;
>>>   };
>>>
>>> +struct drm_palette_caps {
>>> +	/* Structure version. Should be 1 currently */
>>> +	__u32 version;
>>> +	/* For padding and future use */
>>> +	__u32 reserved;
>>> +	/* This may be 0 if not supported. e.g. plane palette or VLV pipe */
>>> +	__u32 num_samples_before_ctm;
>>> +	/* This will be non-zero for pipe. May be zero for planes on some HW */
>>> +	__u32 num_samples_after_ctm;
>>> +};
>>> +
>>>   /* typedef area */
>>>   #ifndef __KERNEL__
>>>   typedef struct drm_clip_rect drm_clip_rect_t;
>>> --
>>> 1.9.1
>>>
>>
>> --
>> Matt Roper
>> Graphics Software Engineer
>> IoTG Platform Enabling & Development
>> Intel Corporation
>> (916) 356-2795
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

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

Re: [PATCH 02/23] drm: Add structure for querying palette color capabilities

[Sharma, Shashank]

This would be an interface/design change, to change from one blob of correction property, to split into multiple query properties for palette_before_blob and palette_after_blob. 
Please let me know if this is really required ?

Regards
Shashank
-----Original Message-----
From: Smith, Gary K 
Sent: Wednesday, September 23, 2015 3:17 PM
To: Sharma, Shashank; Daniel Vetter; Roper, Matthew D
Cc: Matheson, Annie J; Bradford, Robert; Palleti, Avinash Reddy; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Mukherjee, Indranil; Bish, Jim; kausalmalladi@gmail.com; Vetter, Daniel
Subject: RE: [PATCH 02/23] drm: Add structure for querying palette color capabilities

Given that its only one word of info per LUT, I'm OK with it being two separate properties. 
I believe it was much more complex previously with a lot more info per LUT, which is probably why I preferred a blob.

Thanks
Gary

-----Original Message-----
From: Sharma, Shashank
Sent: Wednesday, September 23, 2015 9:10 AM
To: Daniel Vetter; Roper, Matthew D
Cc: Matheson, Annie J; Bradford, Robert; Palleti, Avinash Reddy; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Mukherjee, Indranil; Bish, Jim; Smith, Gary K; kausalmalladi@gmail.com; Vetter, Daniel
Subject: Re: [PATCH 02/23] drm: Add structure for querying palette color capabilities

Hi Matt, Daniel
Addressing the review comments from both of you here.

Regards
Shashank

On 9/22/2015 6:32 PM, Daniel Vetter wrote:
> On Wed, Sep 16, 2015 at 10:51:31AM -0700, Matt Roper wrote:
>> On Wed, Sep 16, 2015 at 11:06:59PM +0530, Shashank Sharma wrote:
>>> From: Kausal Malladi <kausalmalladi@gmail.com>
>>>
>>> The DRM color management framework is targeting various hardware 
>>> platforms and drivers. Different platforms can have different color 
>>> correction and enhancement capabilities.
>>>
>>> A commom user space application can query these capabilities using 
>>> the DRM property interface. Each driver can fill this property with 
>>> its platform's palette color capabilities.
>>>
>>> This patch adds new structure in DRM layer for querying palette 
>>> color capabilities. This structure will be used by all user space 
>>> agents to configure appropriate color configurations.
>>>
>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
>>
>> I think you provided an explanation on a previous code review cycle, 
>> but I forget the details now...what's the benefit to using a blob for 
>> caps rather than having these be individual properties?  Individual 
>> properties seems more natural to me, but I think you had a 
>> justification for blobbing them together; that reasoning would be 
>> good to include in the commit message.
>
> Yeah I'm leaning slightly towards individual props too, that would 
> give us a bit more freedom with placing them (e.g. if someone comes up 
> with funky hw where before_ctm and ctm are per-plane and after_ctm is 
> on the crtc, with only some planes support the before_ctm gamma table).
This was the part where we spent most of the time during the design review, and the reason we came up for this was:
- This is a read only property, which userspace would like to read only once, and cache the information. It was also Gary's opinion to keep this as single blob for all.
- Making individual property needs more information to be provided to user space.
- This is a blob only for pipe level capabilities, the plane level blob will be separate from this.
- We can handle this HW also, by loading proper plane and pipe level capability blob. This is more convenient to have all the capabilities together at the same place, than keep on querying the same.
>
> Also if you do per-prop properties instead of the blob you can drop 
> the version/reserved fields, since properties are inheritedly designed 
> to be extendible. So no need to revision them again (it only leads to 
> more code that might break).
> -Daniel
>
We are anyways planning to drop the version, as per Ville's comment.
- Shashank
>>
>>
>> Matt
>>
>>> ---
>>>   include/uapi/drm/drm.h | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>>>
>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 
>>> 3801584..e3c642f 100644
>>> --- a/include/uapi/drm/drm.h
>>> +++ b/include/uapi/drm/drm.h
>>> @@ -829,6 +829,17 @@ struct drm_event_vblank {
>>>   	__u32 reserved;
>>>   };
>>>
>>> +struct drm_palette_caps {
>>> +	/* Structure version. Should be 1 currently */
>>> +	__u32 version;
>>> +	/* For padding and future use */
>>> +	__u32 reserved;
>>> +	/* This may be 0 if not supported. e.g. plane palette or VLV pipe */
>>> +	__u32 num_samples_before_ctm;
>>> +	/* This will be non-zero for pipe. May be zero for planes on some HW */
>>> +	__u32 num_samples_after_ctm;
>>> +};
>>> +
>>>   /* typedef area */
>>>   #ifndef __KERNEL__
>>>   typedef struct drm_clip_rect drm_clip_rect_t;
>>> --
>>> 1.9.1
>>>
>>
>> --
>> Matt Roper
>> Graphics Software Engineer
>> IoTG Platform Enabling & Development
>> Intel Corporation
>> (916) 356-2795
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/23] drm: Add drm structures for palette color property

[Daniel Vetter]

On Wed, Sep 23, 2015 at 01:45:16PM +0530, Sharma, Shashank wrote:
> Regards
> Shashank
> 
> On 9/22/2015 6:38 PM, Daniel Vetter wrote:
> >On Wed, Sep 16, 2015 at 11:07:01PM +0530, Shashank Sharma wrote:
> >>From: Kausal Malladi <kausalmalladi@gmail.com>
> >>
> >>This patch adds new structures in DRM layer for Palette color
> >>correction.These structures will be used by user space agents
> >>to configure appropriate number of samples and Palette LUT for
> >>a platform.
> >>
> >>Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >>Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
> >>---
> >>  include/uapi/drm/drm.h | 27 +++++++++++++++++++++++++++
> >>  1 file changed, 27 insertions(+)
> >>
> >>diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> >>index e3c642f..f72b916 100644
> >>--- a/include/uapi/drm/drm.h
> >>+++ b/include/uapi/drm/drm.h
> >>@@ -840,6 +840,33 @@ struct drm_palette_caps {
> >>  	__u32 num_samples_after_ctm;
> >>  };
> >>
> >>+struct drm_r32g32b32 {
> >>+	/*
> >>+	 * Data is in U8.24 fixed point format.
> >>+	 * All platforms support values within [0, 1.0] range,
> >>+	 * for Red, Green and Blue colors.
> >>+	 */
> >>+	__u32 r32;
> >>+	__u32 g32;
> >>+	__u32 b32;
> >
> >It's not strictly required, but adding a __u32 reserved here to align the
> >struct to 64 bits seems good imo. Slight overhead but meh about that.
> Humm, ok, we can check this out.
> >
> >>+};
> >>+
> >>+struct drm_palette {
> >>+	/* Structure version. Should be 1 currently */
> >>+	__u32 version;
> >
> >Definitely great practice to take compat into account and definitely
> >needed for the first design using ioctls but I don't think we need this
> >here. Properties are already extinsible themselves: We can just greate a
> >"ctm-v2", "ctm-v3" if the layout changes, and since the actual ctm matrix
> >is stored in the drm_crtc_state any compat code on the kernel will be
> >shared.
> >
> >Aside: For an ioctl the recommended way to handle backwards compat and
> >extensions in drm is with a flags bitfield. That's more flexible than a
> >linear version field, and extending the ioctl struct at the end is already
> >handled by the drm core in a transparent fashion (it 0-fills either kernel
> >or userspace side).
> >
> Agree, we will drop this. Do you think we should add a flags field, or is it
> ok without it ?

No need for a flag field since this is not an ioctl struct. That "Aside:"
was really meant as a comment aside and not relevant for properties.

> >>+	/*
> >>+	 * This has to be a supported value during get call.
> >>+	 * Feature will be disabled if this is 0 while set
> >>+	 */
> >>+	__u32 num_samples;
> >
> >blob properties already have a size, storing it again in the blob is
> >redundnant. Instead I think a small helper to get the number of samples
> >for a given gamma table blob would be needed.
> >
> >Cheers, Daniel
> Please note that they are different. One is the size of blob and other one
> is the num_samples supported by the property, in the current correction
> mode. If you check the design doc, num_sample serves the purpose of deciding
> which correction mode to be applied also. fox ex, for gamma, num_samples=0
> indicates disable gamma, whereas num_samples=512 indicates split gamma mode.

num_samples = blob_size/(sizeof(drm_r32g32b32));

I just think that this information is redundant and if userspace supplies
a gamma table with the wrong size we should just reject it. There's really
no reason for userspace to create a blob property where the size doesn't
exactly match the gamma table.

I guess again that this was needed for the ioctl where there's no sideband
for the size. But properties _are_ sized.

Also, you need to make sure that the property size is aligned and reject
the gamma table property if that's not the case, i.e.

	if (blob_size % sizeof(drm_r32g32b32))
		return -EINVAL;


Plus then driver-specific code to reject anything that's not one of the
supported sizes too.

Of course that also needs igt test coverage.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 09/23] drm/i915: Register pipe color capabilities

[Daniel Vetter]

On Wed, Sep 23, 2015 at 02:05:25PM +0530, Sharma, Shashank wrote:
> Regards
> Shashank
> 
> On 9/22/2015 6:54 PM, Daniel Vetter wrote:
> >On Wed, Sep 16, 2015 at 11:07:06PM +0530, Shashank Sharma wrote:
> >>DRM color manager contains these color properties:
> >>
> >>1. "crtc_palette_capabilities_property": to allow a
> >>core driver to load and showcase its color correction
> >>capabilities to user space.
> >>2. "ctm": Color transformation matrix property, where a
> >>color transformation matrix of 9 correction values gets
> >>applied as correction.
> >>3. "palette_before_ctm": for corrections which get applied
> >>beore color transformation matrix correction.
> >>4. "palette_after_ctm": for corrections which get applied
> >>after color transformation matrix correction.
> >>
> >>Intel color manager registers:
> >>1. Gamma correction property as "palette_after_ctm" property
> >>2. Degamma correction capability as "palette_bafore_ctm" property
> >>capability as "palette_after_ctm" DRM color property hook.
> >>3. CSC as "ctm" property.
> >>
> >>This patch does the following:
> >>1. Add a function which loads the platform's color correction
> >>capabilities in the cm_crtc_palette_capabilities_property structure.
> >>2. Attaches the cm_crtc_palette_capabilities_property to every CRTC
> >>getting initiaized.
> >>3. Adds two new parameters "num_samples_after_ctm" and
> >>"num_samples_before_ctm" in intel_device_info as gamma and
> >>degamma coefficients vary per platform basis.
> >>
> >>Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >>---
> >>  drivers/gpu/drm/i915/i915_drv.h            |  2 ++
> >>  drivers/gpu/drm/i915/intel_color_manager.c | 45 ++++++++++++++++++++++++++++++
> >>  drivers/gpu/drm/i915/intel_color_manager.h |  2 ++
> >>  3 files changed, 49 insertions(+)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >>index 3bf8a9b..22de2cb 100644
> >>--- a/drivers/gpu/drm/i915/i915_drv.h
> >>+++ b/drivers/gpu/drm/i915/i915_drv.h
> >>@@ -798,6 +798,8 @@ struct intel_device_info {
> >>  	u8 num_sprites[I915_MAX_PIPES];
> >>  	u8 gen;
> >>  	u8 ring_mask; /* Rings supported by the HW */
> >>+	u16 num_samples_after_ctm;
> >>+	u16 num_samples_before_ctm;
> >>  	DEV_INFO_FOR_EACH_FLAG(DEFINE_FLAG, SEP_SEMICOLON);
> >>  	/* Register offsets for the various display pipes and transcoders */
> >>  	int pipe_offsets[I915_MAX_TRANSCODERS];
> >>diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
> >>index 7357d99..77f58f2 100644
> >>--- a/drivers/gpu/drm/i915/intel_color_manager.c
> >>+++ b/drivers/gpu/drm/i915/intel_color_manager.c
> >>@@ -27,7 +27,52 @@
> >>
> >>  #include "intel_color_manager.h"
> >>
> >>+int get_pipe_capabilities(struct drm_device *dev,
> >>+		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
> >>+{
> >>+	struct drm_property_blob *blob;
> >>+
> >>+	/*
> >>+	 * This function loads best capability for gamma correction
> >>+	 * For example:
> >>+	 * CHV best Gamma correction (CGM unit, 10 bit)
> >>+	 * has 257 entries, best degamma is 65 entries
> >>+	 */
> >>+	palette_caps->version = COLOR_STRUCT_VERSION;
> >>+	palette_caps->num_samples_after_ctm =
> >>+				INTEL_INFO(dev)->num_samples_after_ctm;
> >>+	palette_caps->num_samples_before_ctm =
> >>+				INTEL_INFO(dev)->num_samples_before_ctm;
> >>+	blob = drm_property_create_blob(dev, sizeof(struct drm_palette_caps),
> >>+			(const void *) palette_caps);
> >>+	if (IS_ERR_OR_NULL(blob)) {
> >>+		DRM_ERROR("Create blob for capabilities failed\n");
> >>+		return PTR_ERR(blob);
> >>+	}
> >>+
> >>+	return blob->base.id;
> >>+}
> >>+
> >>  void intel_attach_color_properties_to_crtc(struct drm_device *dev,
> >>  		struct drm_mode_object *mode_obj)
> >>  {
> >>+	struct drm_mode_config *config = &dev->mode_config;
> >>+	struct drm_palette_caps *palette_caps;
> >>+	struct drm_crtc *crtc;
> >>+	int capabilities_blob_id;
> >>+
> >>+	crtc = obj_to_crtc(mode_obj);
> >>+	palette_caps = kzalloc(sizeof(struct drm_palette_caps),
> >>+			GFP_KERNEL);
> >>+	capabilities_blob_id = get_pipe_capabilities(dev,
> >>+						palette_caps, crtc);
> >>+
> >>+	if (config->cm_crtc_palette_capabilities_property) {
> >>+		drm_object_attach_property(mode_obj,
> >>+			config->cm_crtc_palette_capabilities_property,
> >>+			capabilities_blob_id);
> >
> >I didn't find any code to attach the before/after_ctm gamma properties and
> >the ctm property. Also I think a small helper would be really nice here in
> >the drm core:
> >
> Patch 15,16,17 adds those for gamma, degamma and CSC. This is just for the
> capabilities property.

Indeed, my mail search failed me.

> >drm_crtc_attach_cc_properties(struct drm_crtc *crtc,
> >			      int gamma_before_ctm,
> >			      bool ctm,
> >			      int gamm_after_ctm)
> >
> >And then that directly constructs the palette caps from the given values
> >(if they're non-0) and also attaches the properties to the crtc (if
> >they're non-0 for gamma, treu for the ctm).
> >
> >If that attach_property code really isn't there I wonder a bit how this
> >all works, we /should/ be filtering out properties which aren't attached
> >to the object.
> As I mentioned in the cover-letter, here is the sequence:
> First 5 patches add color management support in DRM layer
> Next 7 patches add Intel color management framework in I915 driver (This is
> still the framework part)
> Next 5 patches add color correction for CHV (gamma, degamma and CSC)
> (Here comes the core property and enabling part)
> Next 2 patches enable color management, by attaching the properties to
>  CRTC(Matt's comment on enable property code only when the job is done)
> Next 4 patches add color correction for BDW (gamma, degamma)

Yeah I just missed this, no idea why the search in my mail client didn't
turn up patches 15-17. I still think drm_crtc_attach_cc_properties in the
drm core would be good, instead of splitting part of the color manager
core support around the properties between the driver and core like you do
here.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 04/23] drm: Add drm structures for palette color property

[Sharma, Shashank]

Regards
Shashank

On 9/23/2015 6:19 PM, Daniel Vetter wrote:
> On Wed, Sep 23, 2015 at 01:45:16PM +0530, Sharma, Shashank wrote:
>> Regards
>> Shashank
>>
>> On 9/22/2015 6:38 PM, Daniel Vetter wrote:
>>> On Wed, Sep 16, 2015 at 11:07:01PM +0530, Shashank Sharma wrote:
>>>> From: Kausal Malladi <kausalmalladi@gmail.com>
>>>>
>>>> This patch adds new structures in DRM layer for Palette color
>>>> correction.These structures will be used by user space agents
>>>> to configure appropriate number of samples and Palette LUT for
>>>> a platform.
>>>>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
>>>> ---
>>>>   include/uapi/drm/drm.h | 27 +++++++++++++++++++++++++++
>>>>   1 file changed, 27 insertions(+)
>>>>
>>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>>>> index e3c642f..f72b916 100644
>>>> --- a/include/uapi/drm/drm.h
>>>> +++ b/include/uapi/drm/drm.h
>>>> @@ -840,6 +840,33 @@ struct drm_palette_caps {
>>>>   	__u32 num_samples_after_ctm;
>>>>   };
>>>>
>>>> +struct drm_r32g32b32 {
>>>> +	/*
>>>> +	 * Data is in U8.24 fixed point format.
>>>> +	 * All platforms support values within [0, 1.0] range,
>>>> +	 * for Red, Green and Blue colors.
>>>> +	 */
>>>> +	__u32 r32;
>>>> +	__u32 g32;
>>>> +	__u32 b32;
>>>
>>> It's not strictly required, but adding a __u32 reserved here to align the
>>> struct to 64 bits seems good imo. Slight overhead but meh about that.
>> Humm, ok, we can check this out.
>>>
>>>> +};
>>>> +
>>>> +struct drm_palette {
>>>> +	/* Structure version. Should be 1 currently */
>>>> +	__u32 version;
>>>
>>> Definitely great practice to take compat into account and definitely
>>> needed for the first design using ioctls but I don't think we need this
>>> here. Properties are already extinsible themselves: We can just greate a
>>> "ctm-v2", "ctm-v3" if the layout changes, and since the actual ctm matrix
>>> is stored in the drm_crtc_state any compat code on the kernel will be
>>> shared.
>>>
>>> Aside: For an ioctl the recommended way to handle backwards compat and
>>> extensions in drm is with a flags bitfield. That's more flexible than a
>>> linear version field, and extending the ioctl struct at the end is already
>>> handled by the drm core in a transparent fashion (it 0-fills either kernel
>>> or userspace side).
>>>
>> Agree, we will drop this. Do you think we should add a flags field, or is it
>> ok without it ?
>
> No need for a flag field since this is not an ioctl struct. That "Aside:"
> was really meant as a comment aside and not relevant for properties.
>
>>>> +	/*
>>>> +	 * This has to be a supported value during get call.
>>>> +	 * Feature will be disabled if this is 0 while set
>>>> +	 */
>>>> +	__u32 num_samples;
>>>
>>> blob properties already have a size, storing it again in the blob is
>>> redundnant. Instead I think a small helper to get the number of samples
>>> for a given gamma table blob would be needed.
>>>
>>> Cheers, Daniel
>> Please note that they are different. One is the size of blob and other one
>> is the num_samples supported by the property, in the current correction
>> mode. If you check the design doc, num_sample serves the purpose of deciding
>> which correction mode to be applied also. fox ex, for gamma, num_samples=0
>> indicates disable gamma, whereas num_samples=512 indicates split gamma mode.
>
> num_samples = blob_size/(sizeof(drm_r32g32b32));
>
> I just think that this information is redundant and if userspace supplies
> a gamma table with the wrong size we should just reject it. There's really
> no reason for userspace to create a blob property where the size doesn't
> exactly match the gamma table.
>
> I guess again that this was needed for the ioctl where there's no sideband
> for the size. But properties _are_ sized.
Again, this is what we decided in the design discussion. The driver will 
showcase the best option for property, but that doesn't stop a user 
space with more knowledge of HW to send other supported options. for 
example, in case of gamma, the driver supports all 3 possible modes:
- 8 bit legacy gamma (256 coeff)
- 10 bit split gamma (1024 coeff (512 + 512))
- 12 bit interpolated gamma (coeff 513)
So here, we have used the no of coeff to define which type of gamma we 
want to apply. So in the core gamma function you will find 4 cases:
switch(no_coeff)
case 0: disable gamma;
case 256: enable legacy gamma;
case 512: enable 10 bit split gamma;
case 513: enable 12 bit interpolated gamma;

This is the simplest implementation, and there is no need for any 
additional variable.
>
> Also, you need to make sure that the property size is aligned and reject
> the gamma table property if that's not the case, i.e.
>
> 	if (blob_size % sizeof(drm_r32g32b32))
> 		return -EINVAL;
>
>
> Plus then driver-specific code to reject anything that's not one of the
> supported sizes too.
>
> Of course that also needs igt test coverage.
> -Daniel
>
Shashank
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/23] drm/i915: Add gamma correction handlers

[Daniel Vetter]

On Wed, Sep 23, 2015 at 01:52:21PM +0530, Sharma, Shashank wrote:
> Regards
> Shashank
> 
> On 9/22/2015 6:45 PM, Daniel Vetter wrote:
> >On Wed, Sep 16, 2015 at 11:07:07PM +0530, Shashank Sharma wrote:
> >>I915 driver registers gamma correction as palette correction
> >>property with DRM layer. This patch adds set_property() and get_property()
> >>handlers for pipe level gamma correction.
> >>
> >>The set function attaches the Gamma correction blob to CRTC state, these
> >>values will be committed during atomic commit.
> >>
> >>Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >>Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
> >>---
> >>  drivers/gpu/drm/i915/intel_atomic.c        | 20 ++++++++++++++++++++
> >>  drivers/gpu/drm/i915/intel_color_manager.c | 21 +++++++++++++++++++++
> >>  drivers/gpu/drm/i915/intel_drv.h           |  5 +++++
> >>  3 files changed, 46 insertions(+)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> >>index 500d2998..0b61fef 100644
> >>--- a/drivers/gpu/drm/i915/intel_atomic.c
> >>+++ b/drivers/gpu/drm/i915/intel_atomic.c
> >>@@ -315,6 +315,13 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
> >>  				   struct drm_property *property,
> >>  				   uint64_t val)
> >>  {
> >>+	struct drm_device *dev = crtc->dev;
> >>+	struct drm_mode_config *config = &dev->mode_config;
> >>+
> >>+	if (property == config->cm_palette_after_ctm_property)
> >>+		return intel_color_manager_set_pipe_gamma(dev, state,
> >>+				&crtc->base, val);
> >>+
> >>  	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
> >>  	return -EINVAL;
> >>  }
> >>@@ -324,6 +331,19 @@ int intel_crtc_atomic_get_property(struct drm_crtc *crtc,
> >>  				   struct drm_property *property,
> >>  				   uint64_t *val)
> >>  {
> >>+	struct drm_device *dev = crtc->dev;
> >>+	struct drm_mode_config *config = &dev->mode_config;
> >>+
> >>+	if (property == config->cm_palette_after_ctm_property) {
> >>+		*val = (state->palette_after_ctm_blob) ?
> >>+			state->palette_after_ctm_blob->base.id : 0;
> >>+		goto found;
> >>+	}
> >
> >Since color manager properties are meant as a new standardize KMS
> >extension (we put them into the core drm_crtc_state) the get/set support
> >should also be in the core. See e.g. how the rotation property is handled
> >in drm_atomic_plane_get/set_property. So all this code should be added to
> >drm_atomic_crtc_get/set_property.
> Thanks, sounds like a good one. Will move this.

When moving this please don't forget to add the blob->length sanity checks
mentioned in another thread in this discussion.

> >
> >
> >>+
> >>  	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
> >>  	return -EINVAL;
> >>+
> >>+found:
> >>+	DRM_DEBUG_KMS("Found property %s\n", property->name);
> >>+	return 0;
> >>  }
> >>diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
> >>index 77f58f2..9421bb6 100644
> >>--- a/drivers/gpu/drm/i915/intel_color_manager.c
> >>+++ b/drivers/gpu/drm/i915/intel_color_manager.c
> >>@@ -27,6 +27,27 @@
> >>
> >>  #include "intel_color_manager.h"
> >>
> >>+int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
> >>+		struct drm_crtc_state *crtc_state,
> >>+		struct drm_mode_object *obj, uint32_t blob_id)
> >>+{
> >>+	struct drm_property_blob *blob;
> >>+
> >>+	blob = drm_property_lookup_blob(dev, blob_id);
> >>+	if (!blob) {
> >>+		DRM_DEBUG_KMS("Invalid Blob ID\n");
> >>+		return -EINVAL;
> >>+	}
> >>+
> >>+	if (crtc_state->palette_after_ctm_blob)
> >>+		drm_property_unreference_blob(
> >>+			crtc_state->palette_after_ctm_blob);
> >>+
> >>+	/* Attach the blob to be committed in state */
> >>+	crtc_state->palette_after_ctm_blob = blob;
> >>+	return 0;
> >>+}
> >
> >What is this used for? It looks a bit like legacy property code, and we
> >have a generic helper to make that happen
> >(drm_atomic_helper_crtc_set_property).
> >
> No Daniel, its not the legacy part. Please check atomic_begin() part in the
> next patches. This is like the top half of atomic set_property for CRTC and
> will be applicable for any property. As you already know, to set a property:
> - userspace creates a blob, and sends the blob id via set_property()
> interface
> - set_property() saves the blob_id in the corresponding pointer space in the
> crtc_state()
> - From the atomic_commit_begin() we will extract the values from blob and
> start committing to the registers. Its kind of bottom_half() of atomic
> set_property for CRTC.
> >Generally please don't add functions/structs without also adding a user,
> >it means that reviewers have to constantly jump around in your patch
> >series to figure out how something is used. Instead if you want to split
> >things up really fine add a stub function frist (but including relevant
> >callers) and then fill out the bits separately.
> I think the above explanation should make it clear.

In my defence I'd like to say that I was still jetlagged ;-) Reading
things now again it all looks good.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/23] drm: Add structure for querying palette color capabilities

[Daniel Vetter]

On Wed, Sep 23, 2015 at 11:57:58AM +0000, Sharma, Shashank wrote:
> This would be an interface/design change, to change from one blob of
> correction property, to split into multiple query properties for
> palette_before_blob and palette_after_blob.  Please let me know if this
> is really required ?

Yes I think splitting this up into one property per value is the right
approach. That's also why we split the before/after gamma tables and the
ctm each into it's own property.

And I don't think this is a design change of the interface - we still
exchange the exact same values with the exact semantics between kernel and
userspace, there's just a small difference in the actual transport
protocol. Those kinds of minor changes happen all the time when
upstreaming features. And that's the reason why we absolutely _must_ have
a close collaboration between the kernel and userspace side. Which
unfortunately on this feature here took a few months to get going, but
hopefully with shared git trees and talking on irc now that should be
easier.
-Daniel

> 
> Regards
> Shashank
> -----Original Message-----
> From: Smith, Gary K 
> Sent: Wednesday, September 23, 2015 3:17 PM
> To: Sharma, Shashank; Daniel Vetter; Roper, Matthew D
> Cc: Matheson, Annie J; Bradford, Robert; Palleti, Avinash Reddy; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Mukherjee, Indranil; Bish, Jim; kausalmalladi@gmail.com; Vetter, Daniel
> Subject: RE: [PATCH 02/23] drm: Add structure for querying palette color capabilities
> 
> Given that its only one word of info per LUT, I'm OK with it being two separate properties. 
> I believe it was much more complex previously with a lot more info per LUT, which is probably why I preferred a blob.
> 
> Thanks
> Gary
> 
> -----Original Message-----
> From: Sharma, Shashank
> Sent: Wednesday, September 23, 2015 9:10 AM
> To: Daniel Vetter; Roper, Matthew D
> Cc: Matheson, Annie J; Bradford, Robert; Palleti, Avinash Reddy; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Mukherjee, Indranil; Bish, Jim; Smith, Gary K; kausalmalladi@gmail.com; Vetter, Daniel
> Subject: Re: [PATCH 02/23] drm: Add structure for querying palette color capabilities
> 
> Hi Matt, Daniel
> Addressing the review comments from both of you here.
> 
> Regards
> Shashank
> 
> On 9/22/2015 6:32 PM, Daniel Vetter wrote:
> > On Wed, Sep 16, 2015 at 10:51:31AM -0700, Matt Roper wrote:
> >> On Wed, Sep 16, 2015 at 11:06:59PM +0530, Shashank Sharma wrote:
> >>> From: Kausal Malladi <kausalmalladi@gmail.com>
> >>>
> >>> The DRM color management framework is targeting various hardware 
> >>> platforms and drivers. Different platforms can have different color 
> >>> correction and enhancement capabilities.
> >>>
> >>> A commom user space application can query these capabilities using 
> >>> the DRM property interface. Each driver can fill this property with 
> >>> its platform's palette color capabilities.
> >>>
> >>> This patch adds new structure in DRM layer for querying palette 
> >>> color capabilities. This structure will be used by all user space 
> >>> agents to configure appropriate color configurations.
> >>>
> >>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >>> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
> >>
> >> I think you provided an explanation on a previous code review cycle, 
> >> but I forget the details now...what's the benefit to using a blob for 
> >> caps rather than having these be individual properties?  Individual 
> >> properties seems more natural to me, but I think you had a 
> >> justification for blobbing them together; that reasoning would be 
> >> good to include in the commit message.
> >
> > Yeah I'm leaning slightly towards individual props too, that would 
> > give us a bit more freedom with placing them (e.g. if someone comes up 
> > with funky hw where before_ctm and ctm are per-plane and after_ctm is 
> > on the crtc, with only some planes support the before_ctm gamma table).
> This was the part where we spent most of the time during the design review, and the reason we came up for this was:
> - This is a read only property, which userspace would like to read only once, and cache the information. It was also Gary's opinion to keep this as single blob for all.
> - Making individual property needs more information to be provided to user space.
> - This is a blob only for pipe level capabilities, the plane level blob will be separate from this.
> - We can handle this HW also, by loading proper plane and pipe level capability blob. This is more convenient to have all the capabilities together at the same place, than keep on querying the same.
> >
> > Also if you do per-prop properties instead of the blob you can drop 
> > the version/reserved fields, since properties are inheritedly designed 
> > to be extendible. So no need to revision them again (it only leads to 
> > more code that might break).
> > -Daniel
> >
> We are anyways planning to drop the version, as per Ville's comment.
> - Shashank
> >>
> >>
> >> Matt
> >>
> >>> ---
> >>>   include/uapi/drm/drm.h | 11 +++++++++++
> >>>   1 file changed, 11 insertions(+)
> >>>
> >>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 
> >>> 3801584..e3c642f 100644
> >>> --- a/include/uapi/drm/drm.h
> >>> +++ b/include/uapi/drm/drm.h
> >>> @@ -829,6 +829,17 @@ struct drm_event_vblank {
> >>>   	__u32 reserved;
> >>>   };
> >>>
> >>> +struct drm_palette_caps {
> >>> +	/* Structure version. Should be 1 currently */
> >>> +	__u32 version;
> >>> +	/* For padding and future use */
> >>> +	__u32 reserved;
> >>> +	/* This may be 0 if not supported. e.g. plane palette or VLV pipe */
> >>> +	__u32 num_samples_before_ctm;
> >>> +	/* This will be non-zero for pipe. May be zero for planes on some HW */
> >>> +	__u32 num_samples_after_ctm;
> >>> +};
> >>> +
> >>>   /* typedef area */
> >>>   #ifndef __KERNEL__
> >>>   typedef struct drm_clip_rect drm_clip_rect_t;
> >>> --
> >>> 1.9.1
> >>>
> >>
> >> --
> >> Matt Roper
> >> Graphics Software Engineer
> >> IoTG Platform Enabling & Development
> >> Intel Corporation
> >> (916) 356-2795
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> >

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/23] drm: Add drm structures for palette color property

[Daniel Vetter]

On Wed, Sep 23, 2015 at 06:29:31PM +0530, Sharma, Shashank wrote:
> Regards
> Shashank
> 
> On 9/23/2015 6:19 PM, Daniel Vetter wrote:
> >On Wed, Sep 23, 2015 at 01:45:16PM +0530, Sharma, Shashank wrote:
> >>Regards
> >>Shashank
> >>
> >>On 9/22/2015 6:38 PM, Daniel Vetter wrote:
> >>>On Wed, Sep 16, 2015 at 11:07:01PM +0530, Shashank Sharma wrote:
> >>>>From: Kausal Malladi <kausalmalladi@gmail.com>
> >>>>
> >>>>This patch adds new structures in DRM layer for Palette color
> >>>>correction.These structures will be used by user space agents
> >>>>to configure appropriate number of samples and Palette LUT for
> >>>>a platform.
> >>>>
> >>>>Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >>>>Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
> >>>>---
> >>>>  include/uapi/drm/drm.h | 27 +++++++++++++++++++++++++++
> >>>>  1 file changed, 27 insertions(+)
> >>>>
> >>>>diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> >>>>index e3c642f..f72b916 100644
> >>>>--- a/include/uapi/drm/drm.h
> >>>>+++ b/include/uapi/drm/drm.h
> >>>>@@ -840,6 +840,33 @@ struct drm_palette_caps {
> >>>>  	__u32 num_samples_after_ctm;
> >>>>  };
> >>>>
> >>>>+struct drm_r32g32b32 {
> >>>>+	/*
> >>>>+	 * Data is in U8.24 fixed point format.
> >>>>+	 * All platforms support values within [0, 1.0] range,
> >>>>+	 * for Red, Green and Blue colors.
> >>>>+	 */
> >>>>+	__u32 r32;
> >>>>+	__u32 g32;
> >>>>+	__u32 b32;
> >>>
> >>>It's not strictly required, but adding a __u32 reserved here to align the
> >>>struct to 64 bits seems good imo. Slight overhead but meh about that.
> >>Humm, ok, we can check this out.
> >>>
> >>>>+};
> >>>>+
> >>>>+struct drm_palette {
> >>>>+	/* Structure version. Should be 1 currently */
> >>>>+	__u32 version;
> >>>
> >>>Definitely great practice to take compat into account and definitely
> >>>needed for the first design using ioctls but I don't think we need this
> >>>here. Properties are already extinsible themselves: We can just greate a
> >>>"ctm-v2", "ctm-v3" if the layout changes, and since the actual ctm matrix
> >>>is stored in the drm_crtc_state any compat code on the kernel will be
> >>>shared.
> >>>
> >>>Aside: For an ioctl the recommended way to handle backwards compat and
> >>>extensions in drm is with a flags bitfield. That's more flexible than a
> >>>linear version field, and extending the ioctl struct at the end is already
> >>>handled by the drm core in a transparent fashion (it 0-fills either kernel
> >>>or userspace side).
> >>>
> >>Agree, we will drop this. Do you think we should add a flags field, or is it
> >>ok without it ?
> >
> >No need for a flag field since this is not an ioctl struct. That "Aside:"
> >was really meant as a comment aside and not relevant for properties.
> >
> >>>>+	/*
> >>>>+	 * This has to be a supported value during get call.
> >>>>+	 * Feature will be disabled if this is 0 while set
> >>>>+	 */
> >>>>+	__u32 num_samples;
> >>>
> >>>blob properties already have a size, storing it again in the blob is
> >>>redundnant. Instead I think a small helper to get the number of samples
> >>>for a given gamma table blob would be needed.
> >>>
> >>>Cheers, Daniel
> >>Please note that they are different. One is the size of blob and other one
> >>is the num_samples supported by the property, in the current correction
> >>mode. If you check the design doc, num_sample serves the purpose of deciding
> >>which correction mode to be applied also. fox ex, for gamma, num_samples=0
> >>indicates disable gamma, whereas num_samples=512 indicates split gamma mode.
> >
> >num_samples = blob_size/(sizeof(drm_r32g32b32));
> >
> >I just think that this information is redundant and if userspace supplies
> >a gamma table with the wrong size we should just reject it. There's really
> >no reason for userspace to create a blob property where the size doesn't
> >exactly match the gamma table.
> >
> >I guess again that this was needed for the ioctl where there's no sideband
> >for the size. But properties _are_ sized.
> Again, this is what we decided in the design discussion. The driver will
> showcase the best option for property, but that doesn't stop a user space
> with more knowledge of HW to send other supported options. for example, in
> case of gamma, the driver supports all 3 possible modes:
> - 8 bit legacy gamma (256 coeff)
> - 10 bit split gamma (1024 coeff (512 + 512))
> - 12 bit interpolated gamma (coeff 513)
> So here, we have used the no of coeff to define which type of gamma we want
> to apply. So in the core gamma function you will find 4 cases:
> switch(no_coeff)
> case 0: disable gamma;
> case 256: enable legacy gamma;
> case 512: enable 10 bit split gamma;
> case 513: enable 12 bit interpolated gamma;
> 
> This is the simplest implementation, and there is no need for any additional
> variable.

I'm confused, since this is exactly what I'm suggesting. My only
observation is that we don't need a separate num_samples field in the blob
structure itself since userspace already needs to tell the kernel the size
of the blob property separately. And we can derive num_samples from the
size of the blob easily (which means it'll make the necessary overflow
checks simpler).

You _must_ check drm_property_blob->length anyway (atm that seems to be
missing, but I didn't check with a full search), so might as well use that
to compute num_samples instead of just making sure they match.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/23] drm/i915: Add gamma correction handlers

[Sharma, Shashank]

On 9/23/2015 1:52 PM, Sharma, Shashank wrote:
>> Since color manager properties are meant as a new standardize KMS
>> extension (we put them into the core drm_crtc_state) the get/set support
>> should also be in the core. See e.g. how the rotation property is handled
>> in drm_atomic_plane_get/set_property. So all this code should be added to
>> drm_atomic_crtc_get/set_property.
> Thanks, sounds like a good one. Will move this.
Actually, while implementing this, I realized that this change is not 
required.
What we want to do in drm_atomic_crtc_get/set code is:
if (prop == config->cm_palette_after_ctm_property || prop == 	
	config->cm_palette_before_ctm_property) {
	crtc->funcs->atomic_get_property();
}

Which is already being done in the current code:
else if (crtc->funcs->atomic_get_property)
	return crtc->funcs->atomic_get_property(crtc, state, property, val);

so I dont really think we need this change.

Regards
Shashank

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

Re: [Intel-gfx] [PATCH 10/23] drm/i915: Add gamma correction handlers

[Daniel Vetter]

On Sat, Sep 26, 2015 at 09:18:48PM +0530, Sharma, Shashank wrote:
> On 9/23/2015 1:52 PM, Sharma, Shashank wrote:
> >>Since color manager properties are meant as a new standardize KMS
> >>extension (we put them into the core drm_crtc_state) the get/set support
> >>should also be in the core. See e.g. how the rotation property is handled
> >>in drm_atomic_plane_get/set_property. So all this code should be added to
> >>drm_atomic_crtc_get/set_property.
> >Thanks, sounds like a good one. Will move this.
> Actually, while implementing this, I realized that this change is not
> required.
> What we want to do in drm_atomic_crtc_get/set code is:
> if (prop == config->cm_palette_after_ctm_property || prop == 	
> 	config->cm_palette_before_ctm_property) {
> 	crtc->funcs->atomic_get_property();
> }
> 
> Which is already being done in the current code:
> else if (crtc->funcs->atomic_get_property)
> 	return crtc->funcs->atomic_get_property(crtc, state, property, val);

This code is to pass any property unknown to the drm core into the driver.
But since we want this to be a new drm core property set (that's why it's
in drm_crtc_state) the decoding should be done in the core too.

Note that atomic_get/set_property _only_ map between the property as seen
by userspace and the state structures. They're not allowed to do anything
else like compute derived state, check constraints or put the state into
the hw. That's for the atomic_check and atomic_commit callbacks. So for
this patchset here you should move all the code in the
atomic_get/set_property callbacks you add in i915 into the drm core. Like
it is doen for the rotation property.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [Intel-gfx] [PATCH 10/23] drm/i915: Add gamma correction handlers

[Sharma, Shashank]

Matt, your opinion about this ? 

Regards
Shashank
-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Monday, September 28, 2015 12:14 PM
To: Sharma, Shashank
Cc: Daniel Vetter; Roper, Matthew D; Bish, Jim; Bradford, Robert; Smith, Gary K; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Matheson, Annie J; kausalmalladi@gmail.com; Vetter, Daniel
Subject: Re: [Intel-gfx] [PATCH 10/23] drm/i915: Add gamma correction handlers

On Sat, Sep 26, 2015 at 09:18:48PM +0530, Sharma, Shashank wrote:
> On 9/23/2015 1:52 PM, Sharma, Shashank wrote:
> >>Since color manager properties are meant as a new standardize KMS 
> >>extension (we put them into the core drm_crtc_state) the get/set 
> >>support should also be in the core. See e.g. how the rotation 
> >>property is handled in drm_atomic_plane_get/set_property. So all 
> >>this code should be added to drm_atomic_crtc_get/set_property.
> >Thanks, sounds like a good one. Will move this.
> Actually, while implementing this, I realized that this change is not 
> required.
> What we want to do in drm_atomic_crtc_get/set code is:
> if (prop == config->cm_palette_after_ctm_property || prop == 	
> 	config->cm_palette_before_ctm_property) {
> 	crtc->funcs->atomic_get_property();
> }
> 
> Which is already being done in the current code:
> else if (crtc->funcs->atomic_get_property)
> 	return crtc->funcs->atomic_get_property(crtc, state, property, val);

This code is to pass any property unknown to the drm core into the driver.
But since we want this to be a new drm core property set (that's why it's in drm_crtc_state) the decoding should be done in the core too.

Note that atomic_get/set_property _only_ map between the property as seen by userspace and the state structures. They're not allowed to do anything else like compute derived state, check constraints or put the state into the hw. That's for the atomic_check and atomic_commit callbacks. So for this patchset here you should move all the code in the atomic_get/set_property callbacks you add in i915 into the drm core. Like it is doen for the rotation property.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 10/23] drm/i915: Add gamma correction handlers

[Matt Roper]

Yep, Daniel's right; for properties that are not specific to the driver,
the core core should take care of stuffing the values provided by
userspace into the state structures so that every driver that wants to
use these doesn't need to replicate that logic.  My bad for not catching
this in my earlier reviews; sorry about that.  Basically the change
required is to add another else clause to
drm_atomic_crtc_{get,set}_property(), before the core tries to call into
the driver-specific handler.  You'll notice that what you're doing with
color management blobs here is actually very similar to what's already
done for the existing mode blob, so you can take a look at that case for
an example.

Also, the functions like intel_color_manager_set_pipe_gamma() will get
moved into the DRM core and the "intel_" prefix will switch to "drm_"
since there's really nothing Intel-specific about what they're doing.

Sorry again for not noticing this and bringing it up on the earlier
reviews.  Fortunately the changes required should be pretty small.


Matt

On Mon, Sep 28, 2015 at 01:19:13AM -0700, Sharma, Shashank wrote:
> Matt, your opinion about this ? 
> 
> Regards
> Shashank
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Monday, September 28, 2015 12:14 PM
> To: Sharma, Shashank
> Cc: Daniel Vetter; Roper, Matthew D; Bish, Jim; Bradford, Robert; Smith, Gary K; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Matheson, Annie J; kausalmalladi@gmail.com; Vetter, Daniel
> Subject: Re: [Intel-gfx] [PATCH 10/23] drm/i915: Add gamma correction handlers
> 
> On Sat, Sep 26, 2015 at 09:18:48PM +0530, Sharma, Shashank wrote:
> > On 9/23/2015 1:52 PM, Sharma, Shashank wrote:
> > >>Since color manager properties are meant as a new standardize KMS 
> > >>extension (we put them into the core drm_crtc_state) the get/set 
> > >>support should also be in the core. See e.g. how the rotation 
> > >>property is handled in drm_atomic_plane_get/set_property. So all 
> > >>this code should be added to drm_atomic_crtc_get/set_property.
> > >Thanks, sounds like a good one. Will move this.
> > Actually, while implementing this, I realized that this change is not 
> > required.
> > What we want to do in drm_atomic_crtc_get/set code is:
> > if (prop == config->cm_palette_after_ctm_property || prop == 	
> > 	config->cm_palette_before_ctm_property) {
> > 	crtc->funcs->atomic_get_property();
> > }
> > 
> > Which is already being done in the current code:
> > else if (crtc->funcs->atomic_get_property)
> > 	return crtc->funcs->atomic_get_property(crtc, state, property, val);
> 
> This code is to pass any property unknown to the drm core into the driver.
> But since we want this to be a new drm core property set (that's why it's in drm_crtc_state) the decoding should be done in the core too.
> 
> Note that atomic_get/set_property _only_ map between the property as seen by userspace and the state structures. They're not allowed to do anything else like compute derived state, check constraints or put the state into the hw. That's for the atomic_check and atomic_commit callbacks. So for this patchset here you should move all the code in the atomic_get/set_property callbacks you add in i915 into the drm core. Like it is doen for the rotation property.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [Intel-gfx] [PATCH 10/23] drm/i915: Add gamma correction handlers

[Sharma, Shashank]

Ok, anyways it was sounding like a good idea. 
Will do the changes accordingly. 

Regards
Shashank
-----Original Message-----
From: Roper, Matthew D 
Sent: Tuesday, September 29, 2015 3:13 AM
To: Sharma, Shashank
Cc: Daniel Vetter; Bish, Jim; Bradford, Robert; Smith, Gary K; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Matheson, Annie J; kausalmalladi@gmail.com; Vetter, Daniel
Subject: Re: [Intel-gfx] [PATCH 10/23] drm/i915: Add gamma correction handlers

Yep, Daniel's right; for properties that are not specific to the driver, the core core should take care of stuffing the values provided by userspace into the state structures so that every driver that wants to use these doesn't need to replicate that logic.  My bad for not catching this in my earlier reviews; sorry about that.  Basically the change required is to add another else clause to drm_atomic_crtc_{get,set}_property(), before the core tries to call into the driver-specific handler.  You'll notice that what you're doing with color management blobs here is actually very similar to what's already done for the existing mode blob, so you can take a look at that case for an example.

Also, the functions like intel_color_manager_set_pipe_gamma() will get moved into the DRM core and the "intel_" prefix will switch to "drm_"
since there's really nothing Intel-specific about what they're doing.

Sorry again for not noticing this and bringing it up on the earlier reviews.  Fortunately the changes required should be pretty small.


Matt

On Mon, Sep 28, 2015 at 01:19:13AM -0700, Sharma, Shashank wrote:
> Matt, your opinion about this ? 
> 
> Regards
> Shashank
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of 
> Daniel Vetter
> Sent: Monday, September 28, 2015 12:14 PM
> To: Sharma, Shashank
> Cc: Daniel Vetter; Roper, Matthew D; Bish, Jim; Bradford, Robert; 
> Smith, Gary K; dri-devel@lists.freedesktop.org; 
> intel-gfx@lists.freedesktop.org; Matheson, Annie J; 
> kausalmalladi@gmail.com; Vetter, Daniel
> Subject: Re: [Intel-gfx] [PATCH 10/23] drm/i915: Add gamma correction 
> handlers
> 
> On Sat, Sep 26, 2015 at 09:18:48PM +0530, Sharma, Shashank wrote:
> > On 9/23/2015 1:52 PM, Sharma, Shashank wrote:
> > >>Since color manager properties are meant as a new standardize KMS 
> > >>extension (we put them into the core drm_crtc_state) the get/set 
> > >>support should also be in the core. See e.g. how the rotation 
> > >>property is handled in drm_atomic_plane_get/set_property. So all 
> > >>this code should be added to drm_atomic_crtc_get/set_property.
> > >Thanks, sounds like a good one. Will move this.
> > Actually, while implementing this, I realized that this change is 
> > not required.
> > What we want to do in drm_atomic_crtc_get/set code is:
> > if (prop == config->cm_palette_after_ctm_property || prop == 	
> > 	config->cm_palette_before_ctm_property) {
> > 	crtc->funcs->atomic_get_property();
> > }
> > 
> > Which is already being done in the current code:
> > else if (crtc->funcs->atomic_get_property)
> > 	return crtc->funcs->atomic_get_property(crtc, state, property, 
> > val);
> 
> This code is to pass any property unknown to the drm core into the driver.
> But since we want this to be a new drm core property set (that's why it's in drm_crtc_state) the decoding should be done in the core too.
> 
> Note that atomic_get/set_property _only_ map between the property as seen by userspace and the state structures. They're not allowed to do anything else like compute derived state, check constraints or put the state into the hw. That's for the atomic_check and atomic_commit callbacks. So for this patchset here you should move all the code in the atomic_get/set_property callbacks you add in i915 into the drm core. Like it is doen for the rotation property.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/23] drm/i915: Add gamma correction handlers

[Matheson, Annie J]

Thank you Shashank! 

Annie Matheson

> On Sep 28, 2015, at 9:29 PM, Sharma, Shashank <shashank.sharma@intel.com> wrote:
> 
> Ok, anyways it was sounding like a good idea. 
> Will do the changes accordingly. 
> 
> Regards
> Shashank
> -----Original Message-----
> From: Roper, Matthew D 
> Sent: Tuesday, September 29, 2015 3:13 AM
> To: Sharma, Shashank
> Cc: Daniel Vetter; Bish, Jim; Bradford, Robert; Smith, Gary K; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Matheson, Annie J; kausalmalladi@gmail.com; Vetter, Daniel
> Subject: Re: [Intel-gfx] [PATCH 10/23] drm/i915: Add gamma correction handlers
> 
> Yep, Daniel's right; for properties that are not specific to the driver, the core core should take care of stuffing the values provided by userspace into the state structures so that every driver that wants to use these doesn't need to replicate that logic.  My bad for not catching this in my earlier reviews; sorry about that.  Basically the change required is to add another else clause to drm_atomic_crtc_{get,set}_property(), before the core tries to call into the driver-specific handler.  You'll notice that what you're doing with color management blobs here is actually very similar to what's already done for the existing mode blob, so you can take a look at that case for an example.
> 
> Also, the functions like intel_color_manager_set_pipe_gamma() will get moved into the DRM core and the "intel_" prefix will switch to "drm_"
> since there's really nothing Intel-specific about what they're doing.
> 
> Sorry again for not noticing this and bringing it up on the earlier reviews.  Fortunately the changes required should be pretty small.
> 
> 
> Matt
> 
>> On Mon, Sep 28, 2015 at 01:19:13AM -0700, Sharma, Shashank wrote:
>> Matt, your opinion about this ? 
>> 
>> Regards
>> Shashank
>> -----Original Message-----
>> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of 
>> Daniel Vetter
>> Sent: Monday, September 28, 2015 12:14 PM
>> To: Sharma, Shashank
>> Cc: Daniel Vetter; Roper, Matthew D; Bish, Jim; Bradford, Robert; 
>> Smith, Gary K; dri-devel@lists.freedesktop.org; 
>> intel-gfx@lists.freedesktop.org; Matheson, Annie J; 
>> kausalmalladi@gmail.com; Vetter, Daniel
>> Subject: Re: [Intel-gfx] [PATCH 10/23] drm/i915: Add gamma correction 
>> handlers
>> 
>>> On Sat, Sep 26, 2015 at 09:18:48PM +0530, Sharma, Shashank wrote:
>>> On 9/23/2015 1:52 PM, Sharma, Shashank wrote:
>>>>> Since color manager properties are meant as a new standardize KMS 
>>>>> extension (we put them into the core drm_crtc_state) the get/set 
>>>>> support should also be in the core. See e.g. how the rotation 
>>>>> property is handled in drm_atomic_plane_get/set_property. So all 
>>>>> this code should be added to drm_atomic_crtc_get/set_property.
>>>> Thanks, sounds like a good one. Will move this.
>>> Actually, while implementing this, I realized that this change is 
>>> not required.
>>> What we want to do in drm_atomic_crtc_get/set code is:
>>> if (prop == config->cm_palette_after_ctm_property || prop ==    
>>>    config->cm_palette_before_ctm_property) {
>>>    crtc->funcs->atomic_get_property();
>>> }
>>> 
>>> Which is already being done in the current code:
>>> else if (crtc->funcs->atomic_get_property)
>>>    return crtc->funcs->atomic_get_property(crtc, state, property, 
>>> val);
>> 
>> This code is to pass any property unknown to the drm core into the driver.
>> But since we want this to be a new drm core property set (that's why it's in drm_crtc_state) the decoding should be done in the core too.
>> 
>> Note that atomic_get/set_property _only_ map between the property as seen by userspace and the state structures. They're not allowed to do anything else like compute derived state, check constraints or put the state into the hw. That's for the atomic_check and atomic_commit callbacks. So for this patchset here you should move all the code in the atomic_get/set_property callbacks you add in i915 into the drm core. Like it is doen for the rotation property.
>> -Daniel
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
> 
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 21/23] drm/i915: BDW: Pipe level Gamma correction

[Rob Bradford]

Hi Shashank, some feedback below that you would be great to get
addressed before your next version.

On Wed, 2015-09-16 at 23:07 +0530, Shashank Sharma wrote:
> BDW/SKL/BXT platforms support various Gamma correction modes
> which are:
> 1. Legacy 8-bit mode
> 2. 10-bit mode
> 3. 10-bit Split Gamma mode
> 4. 12-bit mode
> 
> This patch does the following:
> 1. Adds the core function to program Gamma correction values
>    for BDW/SKL/BXT platforms
> 2. Adds Gamma correction macros/defines
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h            |  17 +-
>  drivers/gpu/drm/i915/intel_color_manager.c | 270
> +++++++++++++++++++++++++++++

*snip*

> +static u32 bdw_write_10bit_gamma_precision(u32 red, u32 green, u32
> blue)
> +{
> +	u32 word;
> +	u8 blue_int, green_int, red_int;
> +	u16 blue_fract, green_fract, red_fract;
> +
> +	blue_int = _GAMMA_INT_PART(blue);
> +	if (blue_int > GAMMA_INT_MAX)
> +		blue = BDW_MAX_GAMMA;
> +
> +	green_int = _GAMMA_INT_PART(green);
> +	if (green_int > GAMMA_INT_MAX)
> +		green = BDW_MAX_GAMMA;
> +
> +	red_int = _GAMMA_INT_PART(red);
> +	if (red_int > GAMMA_INT_MAX)
> +		red = BDW_MAX_GAMMA;
> +
> +	blue_fract = _GAMMA_FRACT_PART(blue);
> +	green_fract = _GAMMA_FRACT_PART(green);
> +	red_fract = _GAMMA_FRACT_PART(red);
> +
> +	blue_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
> +	green_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
> +	red_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
> +
> +	/* Red (29:20) Green (19:10) and Blue (9:0) */
> +	word = red_fract;
> +	word <<= BDW_GAMMA_SHIFT;
> +	word = word | green_fract;
> +	word <<= BDW_GAMMA_SHIFT;
> +	word = word | blue_fract;
> +
> +	return word;
> +}
> +

I think the above function, and perhaps others in this series have the
same flaw with respect to maximum colour value.

In our discussions we agreed that we would follow the "GL style" where
maximum colour (i.e. 255 in 8-bit) would be represented by 1.0f. 1.0f
when converted to fixed point 8.24 notation is 1 << 24.

I observed that with my test code that a linear ramp (where the last
entry is set to 1.0) gives me black for white. I tracked it down to
this function.

In order to map 1.0 to the maximum value for the table entry in the
hardware this function needs to be changed. One way to achieve this
would be change the test "blue_int > GAMMA_INT_MAX" to be "blue_int >=
GAMMA_INT_MAX" but since GAMMA_INT_MAX = 1 then it might be clearer to
say blue_int > 0.

BDW_MAX_GAMMA also looks wrong. I think it should be (1 << 24) - 1 not
the 0x10000 (see below). As well as correct clamping it is also
necessary to scale as Daniel suggested on IRC:

"
13:54 < danvet> robster, so we'd need to rescale in the kernel from 1.0
to 0.1111111111b ?
13:55 < danvet> well substract 0.0000000001b for all values > 0.5
probably since float math in the kernel is evil
13:56 < danvet> also this probably needs an igt - check that all black
with an all 1.0 gamma table is the same as 
                all white with a linear one
"

You won't see this with your test program (color-correction.c) as the
largest value you program in appears to be 0.ff

*snip*

> +/* Gen 9 */
> +#define BDW_MAX_GAMMA				0x10000
> 

*snip*

I look forward to testing against your next version.

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

RE: [PATCH 21/23] drm/i915: BDW: Pipe level Gamma correction

[Sharma, Shashank]

Hey Rob, 

Thanks for the feedback, and the testing efforts. 
I will check into your suggestions and get back. 

Regards
Shashank
-----Original Message-----
From: Bradford, Robert 
Sent: Wednesday, September 30, 2015 8:02 PM
To: Sharma, Shashank; Roper, Matthew D; Bish, Jim; Smith, Gary K; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
Cc: Vetter, Daniel; Matheson, Annie J; Mukherjee, Indranil; Palleti, Avinash Reddy; kausalmalladi@gmail.com
Subject: Re: [PATCH 21/23] drm/i915: BDW: Pipe level Gamma correction

Hi Shashank, some feedback below that you would be great to get addressed before your next version.

On Wed, 2015-09-16 at 23:07 +0530, Shashank Sharma wrote:
> BDW/SKL/BXT platforms support various Gamma correction modes which 
> are:
> 1. Legacy 8-bit mode
> 2. 10-bit mode
> 3. 10-bit Split Gamma mode
> 4. 12-bit mode
> 
> This patch does the following:
> 1. Adds the core function to program Gamma correction values
>    for BDW/SKL/BXT platforms
> 2. Adds Gamma correction macros/defines
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h            |  17 +-
>  drivers/gpu/drm/i915/intel_color_manager.c | 270
> +++++++++++++++++++++++++++++

*snip*

> +static u32 bdw_write_10bit_gamma_precision(u32 red, u32 green, u32
> blue)
> +{
> +	u32 word;
> +	u8 blue_int, green_int, red_int;
> +	u16 blue_fract, green_fract, red_fract;
> +
> +	blue_int = _GAMMA_INT_PART(blue);
> +	if (blue_int > GAMMA_INT_MAX)
> +		blue = BDW_MAX_GAMMA;
> +
> +	green_int = _GAMMA_INT_PART(green);
> +	if (green_int > GAMMA_INT_MAX)
> +		green = BDW_MAX_GAMMA;
> +
> +	red_int = _GAMMA_INT_PART(red);
> +	if (red_int > GAMMA_INT_MAX)
> +		red = BDW_MAX_GAMMA;
> +
> +	blue_fract = _GAMMA_FRACT_PART(blue);
> +	green_fract = _GAMMA_FRACT_PART(green);
> +	red_fract = _GAMMA_FRACT_PART(red);
> +
> +	blue_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
> +	green_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
> +	red_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
> +
> +	/* Red (29:20) Green (19:10) and Blue (9:0) */
> +	word = red_fract;
> +	word <<= BDW_GAMMA_SHIFT;
> +	word = word | green_fract;
> +	word <<= BDW_GAMMA_SHIFT;
> +	word = word | blue_fract;
> +
> +	return word;
> +}
> +

I think the above function, and perhaps others in this series have the same flaw with respect to maximum colour value.

In our discussions we agreed that we would follow the "GL style" where maximum colour (i.e. 255 in 8-bit) would be represented by 1.0f. 1.0f when converted to fixed point 8.24 notation is 1 << 24.

I observed that with my test code that a linear ramp (where the last entry is set to 1.0) gives me black for white. I tracked it down to this function.

In order to map 1.0 to the maximum value for the table entry in the hardware this function needs to be changed. One way to achieve this would be change the test "blue_int > GAMMA_INT_MAX" to be "blue_int >= GAMMA_INT_MAX" but since GAMMA_INT_MAX = 1 then it might be clearer to say blue_int > 0.

BDW_MAX_GAMMA also looks wrong. I think it should be (1 << 24) - 1 not the 0x10000 (see below). As well as correct clamping it is also necessary to scale as Daniel suggested on IRC:

"
13:54 < danvet> robster, so we'd need to rescale in the kernel from 1.0 to 0.1111111111b ?
13:55 < danvet> well substract 0.0000000001b for all values > 0.5 probably since float math in the kernel is evil
13:56 < danvet> also this probably needs an igt - check that all black with an all 1.0 gamma table is the same as 
                all white with a linear one "

You won't see this with your test program (color-correction.c) as the largest value you program in appears to be 0.ff

*snip*

> +/* Gen 9 */
> +#define BDW_MAX_GAMMA				0x10000
> 

*snip*

I look forward to testing against your next version.

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

Re: [PATCH 21/23] drm/i915: BDW: Pipe level Gamma correction

[Matheson, Annie J]

Shashank,

Please let us know when the next patch series would be ready assuming you incorporate the feedback from Rob. 

I would like to hope we're done with feedback at this point. 

Thanks. 

Annie Matheson

> On Sep 30, 2015, at 9:25 AM, Sharma, Shashank <shashank.sharma@intel.com> wrote:
> 
> Hey Rob, 
> 
> Thanks for the feedback, and the testing efforts. 
> I will check into your suggestions and get back. 
> 
> Regards
> Shashank
> -----Original Message-----
> From: Bradford, Robert 
> Sent: Wednesday, September 30, 2015 8:02 PM
> To: Sharma, Shashank; Roper, Matthew D; Bish, Jim; Smith, Gary K; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
> Cc: Vetter, Daniel; Matheson, Annie J; Mukherjee, Indranil; Palleti, Avinash Reddy; kausalmalladi@gmail.com
> Subject: Re: [PATCH 21/23] drm/i915: BDW: Pipe level Gamma correction
> 
> Hi Shashank, some feedback below that you would be great to get addressed before your next version.
> 
>> On Wed, 2015-09-16 at 23:07 +0530, Shashank Sharma wrote:
>> BDW/SKL/BXT platforms support various Gamma correction modes which 
>> are:
>> 1. Legacy 8-bit mode
>> 2. 10-bit mode
>> 3. 10-bit Split Gamma mode
>> 4. 12-bit mode
>> 
>> This patch does the following:
>> 1. Adds the core function to program Gamma correction values
>>   for BDW/SKL/BXT platforms
>> 2. Adds Gamma correction macros/defines
>> 
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
>> ---
>> drivers/gpu/drm/i915/i915_reg.h            |  17 +-
>> drivers/gpu/drm/i915/intel_color_manager.c | 270
>> +++++++++++++++++++++++++++++
> 
> *snip*
> 
>> +static u32 bdw_write_10bit_gamma_precision(u32 red, u32 green, u32
>> blue)
>> +{
>> +    u32 word;
>> +    u8 blue_int, green_int, red_int;
>> +    u16 blue_fract, green_fract, red_fract;
>> +
>> +    blue_int = _GAMMA_INT_PART(blue);
>> +    if (blue_int > GAMMA_INT_MAX)
>> +        blue = BDW_MAX_GAMMA;
>> +
>> +    green_int = _GAMMA_INT_PART(green);
>> +    if (green_int > GAMMA_INT_MAX)
>> +        green = BDW_MAX_GAMMA;
>> +
>> +    red_int = _GAMMA_INT_PART(red);
>> +    if (red_int > GAMMA_INT_MAX)
>> +        red = BDW_MAX_GAMMA;
>> +
>> +    blue_fract = _GAMMA_FRACT_PART(blue);
>> +    green_fract = _GAMMA_FRACT_PART(green);
>> +    red_fract = _GAMMA_FRACT_PART(red);
>> +
>> +    blue_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
>> +    green_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
>> +    red_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
>> +
>> +    /* Red (29:20) Green (19:10) and Blue (9:0) */
>> +    word = red_fract;
>> +    word <<= BDW_GAMMA_SHIFT;
>> +    word = word | green_fract;
>> +    word <<= BDW_GAMMA_SHIFT;
>> +    word = word | blue_fract;
>> +
>> +    return word;
>> +}
>> +
> 
> I think the above function, and perhaps others in this series have the same flaw with respect to maximum colour value.
> 
> In our discussions we agreed that we would follow the "GL style" where maximum colour (i.e. 255 in 8-bit) would be represented by 1.0f. 1.0f when converted to fixed point 8.24 notation is 1 << 24.
> 
> I observed that with my test code that a linear ramp (where the last entry is set to 1.0) gives me black for white. I tracked it down to this function.
> 
> In order to map 1.0 to the maximum value for the table entry in the hardware this function needs to be changed. One way to achieve this would be change the test "blue_int > GAMMA_INT_MAX" to be "blue_int >= GAMMA_INT_MAX" but since GAMMA_INT_MAX = 1 then it might be clearer to say blue_int > 0.
> 
> BDW_MAX_GAMMA also looks wrong. I think it should be (1 << 24) - 1 not the 0x10000 (see below). As well as correct clamping it is also necessary to scale as Daniel suggested on IRC:
> 
> "
> 13:54 < danvet> robster, so we'd need to rescale in the kernel from 1.0 to 0.1111111111b ?
> 13:55 < danvet> well substract 0.0000000001b for all values > 0.5 probably since float math in the kernel is evil
> 13:56 < danvet> also this probably needs an igt - check that all black with an all 1.0 gamma table is the same as 
>                all white with a linear one "
> 
> You won't see this with your test program (color-correction.c) as the largest value you program in appears to be 0.ff
> 
> *snip*
> 
>> +/* Gen 9 */
>> +#define BDW_MAX_GAMMA                0x10000
> 
> *snip*
> 
> I look forward to testing against your next version.
> 
> Rob
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 21/23] drm/i915: BDW: Pipe level Gamma correction

[Ville Syrjälä]

On Wed, Sep 30, 2015 at 03:31:34PM +0100, Rob Bradford wrote:
> Hi Shashank, some feedback below that you would be great to get
> addressed before your next version.
> 
> On Wed, 2015-09-16 at 23:07 +0530, Shashank Sharma wrote:
> > BDW/SKL/BXT platforms support various Gamma correction modes
> > which are:
> > 1. Legacy 8-bit mode
> > 2. 10-bit mode
> > 3. 10-bit Split Gamma mode
> > 4. 12-bit mode
> > 
> > This patch does the following:
> > 1. Adds the core function to program Gamma correction values
> >    for BDW/SKL/BXT platforms
> > 2. Adds Gamma correction macros/defines
> > 
> > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> > Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h            |  17 +-
> >  drivers/gpu/drm/i915/intel_color_manager.c | 270
> > +++++++++++++++++++++++++++++
> 
> *snip*
> 
> > +static u32 bdw_write_10bit_gamma_precision(u32 red, u32 green, u32
> > blue)
> > +{
> > +	u32 word;
> > +	u8 blue_int, green_int, red_int;
> > +	u16 blue_fract, green_fract, red_fract;
> > +
> > +	blue_int = _GAMMA_INT_PART(blue);
> > +	if (blue_int > GAMMA_INT_MAX)
> > +		blue = BDW_MAX_GAMMA;
> > +
> > +	green_int = _GAMMA_INT_PART(green);
> > +	if (green_int > GAMMA_INT_MAX)
> > +		green = BDW_MAX_GAMMA;
> > +
> > +	red_int = _GAMMA_INT_PART(red);
> > +	if (red_int > GAMMA_INT_MAX)
> > +		red = BDW_MAX_GAMMA;
> > +
> > +	blue_fract = _GAMMA_FRACT_PART(blue);
> > +	green_fract = _GAMMA_FRACT_PART(green);
> > +	red_fract = _GAMMA_FRACT_PART(red);
> > +
> > +	blue_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
> > +	green_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
> > +	red_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
> > +
> > +	/* Red (29:20) Green (19:10) and Blue (9:0) */
> > +	word = red_fract;
> > +	word <<= BDW_GAMMA_SHIFT;
> > +	word = word | green_fract;
> > +	word <<= BDW_GAMMA_SHIFT;
> > +	word = word | blue_fract;
> > +
> > +	return word;
> > +}
> > +
> 
> I think the above function, and perhaps others in this series have the
> same flaw with respect to maximum colour value.
> 
> In our discussions we agreed that we would follow the "GL style" where
> maximum colour (i.e. 255 in 8-bit) would be represented by 1.0f. 1.0f
> when converted to fixed point 8.24 notation is 1 << 24.
> 
> I observed that with my test code that a linear ramp (where the last
> entry is set to 1.0) gives me black for white. I tracked it down to
> this function.
> 
> In order to map 1.0 to the maximum value for the table entry in the
> hardware this function needs to be changed. One way to achieve this
> would be change the test "blue_int > GAMMA_INT_MAX" to be "blue_int >=
> GAMMA_INT_MAX" but since GAMMA_INT_MAX = 1 then it might be clearer to
> say blue_int > 0.
> 
> BDW_MAX_GAMMA also looks wrong. I think it should be (1 << 24) - 1 not
> the 0x10000 (see below). As well as correct clamping it is also
> necessary to scale as Daniel suggested on IRC:
> 
> "
> 13:54 < danvet> robster, so we'd need to rescale in the kernel from 1.0
> to 0.1111111111b ?
> 13:55 < danvet> well substract 0.0000000001b for all values > 0.5
> probably since float math in the kernel is evil

Or just clamp to '(1<<bits)-1'. I think the end result is the same
actually.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH 21/23] drm/i915: BDW: Pipe level Gamma correction

[Sharma, Shashank]

Hi Annie, 
This might add a day or two, but considering this is a long weekend in India, you can expect the patches by mid next week. 

Regards
Shashank
-----Original Message-----
From: Matheson, Annie J 
Sent: Wednesday, September 30, 2015 10:01 PM
To: Sharma, Shashank
Cc: Bradford, Robert; Roper, Matthew D; Bish, Jim; Smith, Gary K; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Vetter, Daniel; Mukherjee, Indranil; Palleti, Avinash Reddy; kausalmalladi@gmail.com
Subject: Re: [PATCH 21/23] drm/i915: BDW: Pipe level Gamma correction

Shashank,

Please let us know when the next patch series would be ready assuming you incorporate the feedback from Rob. 

I would like to hope we're done with feedback at this point. 

Thanks. 

Annie Matheson

> On Sep 30, 2015, at 9:25 AM, Sharma, Shashank <shashank.sharma@intel.com> wrote:
> 
> Hey Rob, 
> 
> Thanks for the feedback, and the testing efforts. 
> I will check into your suggestions and get back. 
> 
> Regards
> Shashank
> -----Original Message-----
> From: Bradford, Robert 
> Sent: Wednesday, September 30, 2015 8:02 PM
> To: Sharma, Shashank; Roper, Matthew D; Bish, Jim; Smith, Gary K; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
> Cc: Vetter, Daniel; Matheson, Annie J; Mukherjee, Indranil; Palleti, Avinash Reddy; kausalmalladi@gmail.com
> Subject: Re: [PATCH 21/23] drm/i915: BDW: Pipe level Gamma correction
> 
> Hi Shashank, some feedback below that you would be great to get addressed before your next version.
> 
>> On Wed, 2015-09-16 at 23:07 +0530, Shashank Sharma wrote:
>> BDW/SKL/BXT platforms support various Gamma correction modes which 
>> are:
>> 1. Legacy 8-bit mode
>> 2. 10-bit mode
>> 3. 10-bit Split Gamma mode
>> 4. 12-bit mode
>> 
>> This patch does the following:
>> 1. Adds the core function to program Gamma correction values
>>   for BDW/SKL/BXT platforms
>> 2. Adds Gamma correction macros/defines
>> 
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
>> ---
>> drivers/gpu/drm/i915/i915_reg.h            |  17 +-
>> drivers/gpu/drm/i915/intel_color_manager.c | 270
>> +++++++++++++++++++++++++++++
> 
> *snip*
> 
>> +static u32 bdw_write_10bit_gamma_precision(u32 red, u32 green, u32
>> blue)
>> +{
>> +    u32 word;
>> +    u8 blue_int, green_int, red_int;
>> +    u16 blue_fract, green_fract, red_fract;
>> +
>> +    blue_int = _GAMMA_INT_PART(blue);
>> +    if (blue_int > GAMMA_INT_MAX)
>> +        blue = BDW_MAX_GAMMA;
>> +
>> +    green_int = _GAMMA_INT_PART(green);
>> +    if (green_int > GAMMA_INT_MAX)
>> +        green = BDW_MAX_GAMMA;
>> +
>> +    red_int = _GAMMA_INT_PART(red);
>> +    if (red_int > GAMMA_INT_MAX)
>> +        red = BDW_MAX_GAMMA;
>> +
>> +    blue_fract = _GAMMA_FRACT_PART(blue);
>> +    green_fract = _GAMMA_FRACT_PART(green);
>> +    red_fract = _GAMMA_FRACT_PART(red);
>> +
>> +    blue_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
>> +    green_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
>> +    red_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
>> +
>> +    /* Red (29:20) Green (19:10) and Blue (9:0) */
>> +    word = red_fract;
>> +    word <<= BDW_GAMMA_SHIFT;
>> +    word = word | green_fract;
>> +    word <<= BDW_GAMMA_SHIFT;
>> +    word = word | blue_fract;
>> +
>> +    return word;
>> +}
>> +
> 
> I think the above function, and perhaps others in this series have the same flaw with respect to maximum colour value.
> 
> In our discussions we agreed that we would follow the "GL style" where maximum colour (i.e. 255 in 8-bit) would be represented by 1.0f. 1.0f when converted to fixed point 8.24 notation is 1 << 24.
> 
> I observed that with my test code that a linear ramp (where the last entry is set to 1.0) gives me black for white. I tracked it down to this function.
> 
> In order to map 1.0 to the maximum value for the table entry in the hardware this function needs to be changed. One way to achieve this would be change the test "blue_int > GAMMA_INT_MAX" to be "blue_int >= GAMMA_INT_MAX" but since GAMMA_INT_MAX = 1 then it might be clearer to say blue_int > 0.
> 
> BDW_MAX_GAMMA also looks wrong. I think it should be (1 << 24) - 1 not the 0x10000 (see below). As well as correct clamping it is also necessary to scale as Daniel suggested on IRC:
> 
> "
> 13:54 < danvet> robster, so we'd need to rescale in the kernel from 1.0 to 0.1111111111b ?
> 13:55 < danvet> well substract 0.0000000001b for all values > 0.5 probably since float math in the kernel is evil
> 13:56 < danvet> also this probably needs an igt - check that all black with an all 1.0 gamma table is the same as 
>                all white with a linear one "
> 
> You won't see this with your test program (color-correction.c) as the largest value you program in appears to be 0.ff
> 
> *snip*
> 
>> +/* Gen 9 */
>> +#define BDW_MAX_GAMMA                0x10000
> 
> *snip*
> 
> I look forward to testing against your next version.
> 
> Rob
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel