[PATCH 0/6] Color Management for DRM framework v10

[PATCH 0/6] Color Management for DRM framework v10

[Lionel Landwerlin]

Hi,

Here is an update on the color management serie by Shashank.
This one squashes things a bit to make it a bit easier to review.

This also includes a number of fixes,

for Braswell :

 - Read the right values from the degamma blob.

for Broadwell :

 - Always reset the lut index when writing the values for split mode,
    otherwise writing the degamma lut twice will set the gamma lut on
       the second write.

Cheers,

-
Lionel

Shashank Sharma (6):
  drm: Create Color Management DRM properties
  drm/i915: Add set property interface for CRTC
  drm/i915: Disable plane gamma
  drm/i915/chv: Implement color management
  drm/i915/bdw+: Implement color management
  drm/i915: Register color correction capabilities

 drivers/gpu/drm/drm_atomic.c               |  67 ++-
 drivers/gpu/drm/drm_atomic_helper.c        |   9 +
 drivers/gpu/drm/drm_crtc.c                 |  32 ++
 drivers/gpu/drm/i915/Makefile              |   3 +-
 drivers/gpu/drm/i915/i915_drv.c            |  19 +-
 drivers/gpu/drm/i915/i915_drv.h            |   2 +
 drivers/gpu/drm/i915/i915_reg.h            |  67 ++-
 drivers/gpu/drm/i915/intel_color_manager.c | 857 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_color_manager.h | 111 ++++
 drivers/gpu/drm/i915/intel_display.c       |   6 +-
 drivers/gpu/drm/i915/intel_drv.h           |   4 +
 drivers/gpu/drm/i915/intel_sprite.c        |   7 +-
 include/drm/drm_crtc.h                     |  24 +
 include/uapi/drm/drm.h                     |  30 +
 14 files changed, 1228 insertions(+), 10 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_color_manager.c
 create mode 100644 drivers/gpu/drm/i915/intel_color_manager.h

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

[PATCH 1/6] drm: Create Color Management DRM properties

[Lionel Landwerlin]

From: Shashank Sharma <shashank.sharma@intel.com>

Color Management is an extension to DRM framework to allow hardware color
correction and enhancement capabilities.

DRM color manager supports these color properties:
1. "ctm": Color transformation matrix property, where a
   color transformation matrix of 9 correction values gets
   applied as correction.
2. "palette_before_ctm": for corrections which get applied
   beore color transformation matrix correction.
3. "palette_after_ctm": for corrections which get applied
   after color transformation matrix correction.

These color correction capabilities may differ per platform, supporting
various different no. of correction coefficients. So DRM color manager
support few properties using which a user space can query the platform's
capability, and prepare color correction accordingly.
These query properties are:
1. cm_coeff_after_ctm_property
2. cm_coeff_before_ctm_property
  (CTM is fix to 9 coefficients across industry)

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
---
 drivers/gpu/drm/drm_atomic.c        | 67 +++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/drm_atomic_helper.c |  9 +++++
 drivers/gpu/drm/drm_crtc.c          | 32 ++++++++++++++++++
 include/drm/drm_crtc.h              | 24 +++++++++++++
 include/uapi/drm/drm.h              | 30 +++++++++++++++++
 5 files changed, 160 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 6a21e5c..ebdb98d 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -388,6 +388,38 @@ int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state,
 EXPORT_SYMBOL(drm_atomic_set_mode_prop_for_crtc);
 
 /**
+ * drm_atomic_crtc_set_blob - find and set a blob
+ * @state_blob: reference pointer to the color blob in the crtc_state
+ * @blob_id: blob_id coming from set_property() call
+ *
+ * Set a color correction blob (originating from a set blob property) on the
+ * desired CRTC state. This function will take reference of the blob property
+ * in the CRTC state, finds the blob based on blob_id (which comes from
+ * set_property call) and set the blob at the proper place.
+ *
+ * RETURNS:
+ * Zero on success, error code on failure.
+ */
+static int drm_atomic_crtc_set_blob(struct drm_device *dev,
+	struct drm_property_blob **state_blob, 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 (*state_blob)
+		drm_property_unreference_blob(*state_blob);
+
+	/* Attach the blob to be committed in state */
+	*state_blob = blob;
+	return 0;
+}
+
+/**
  * drm_atomic_crtc_set_property - set property on CRTC
  * @crtc: the drm CRTC to set a property on
  * @state: the state object to update with the new property value
@@ -419,8 +451,31 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 		ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
 		drm_property_unreference_blob(mode);
 		return ret;
-	}
-	else if (crtc->funcs->atomic_set_property)
+	} else if (property == config->cm_palette_after_ctm_property) {
+		ret = drm_atomic_crtc_set_blob(dev,
+				&state->palette_after_ctm_blob, val);
+		if (ret)
+			DRM_DEBUG_KMS("Failed to load blob palette_after_ctm\n");
+		else
+			state->color_correction_changed = true;
+		return ret;
+	} else if (property == config->cm_palette_before_ctm_property) {
+		ret = drm_atomic_crtc_set_blob(dev,
+				&state->palette_before_ctm_blob, val);
+		if (ret)
+			DRM_DEBUG_KMS("Failed to load blob palette_before_ctm\n");
+		else
+			state->color_correction_changed = true;
+		return ret;
+	} else if (property == config->cm_ctm_property) {
+		ret = drm_atomic_crtc_set_blob(dev,
+				&state->ctm_blob, val);
+		if (ret)
+			DRM_DEBUG_KMS("Failed to load blob ctm\n");
+		else
+			state->color_correction_changed = true;
+		return ret;
+	} else if (crtc->funcs->atomic_set_property)
 		return crtc->funcs->atomic_set_property(crtc, state, property, val);
 	else
 		return -EINVAL;
@@ -456,6 +511,14 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
 		*val = state->active;
 	else if (property == config->prop_mode_id)
 		*val = (state->mode_blob) ? state->mode_blob->base.id : 0;
+	else if (property == config->cm_palette_after_ctm_property)
+		*val = (state->palette_after_ctm_blob) ?
+			state->palette_after_ctm_blob->base.id : 0;
+	else if (property == config->cm_palette_before_ctm_property)
+		*val = (state->palette_before_ctm_blob) ?
+			state->palette_before_ctm_blob->base.id : 0;
+	else if (property == config->cm_ctm_property)
+		*val = (state->ctm_blob) ? state->ctm_blob->base.id : 0;
 	else if (crtc->funcs->atomic_get_property)
 		return crtc->funcs->atomic_get_property(crtc, state, property, val);
 	else
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 268d37f..bd325da 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2448,6 +2448,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;
@@ -2492,6 +2498,9 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc,
 					    struct drm_crtc_state *state)
 {
 	drm_property_unreference_blob(state->mode_blob);
+	drm_property_unreference_blob(state->ctm_blob);
+	drm_property_unreference_blob(state->palette_after_ctm_blob);
+	drm_property_unreference_blob(state->palette_before_ctm_blob);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
 
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 62fa95f..2e02d0f 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1542,6 +1542,38 @@ 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, "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;
+
+	/* DRM properties to query color capabilities */
+	prop = drm_property_create(dev, DRM_MODE_PROP_IMMUTABLE,
+			"COEFFICIENTS_BEFORE_CTM", 0);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.cm_coeff_before_ctm_property = prop;
+
+	prop = drm_property_create(dev, DRM_MODE_PROP_IMMUTABLE,
+			"COEFFICIENTS_AFTER_CTM", 0);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.cm_coeff_after_ctm_property = prop;
+
 	return 0;
 }
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 3b040b3..8326765 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -305,11 +305,15 @@ struct drm_plane_helper_funcs;
  * @mode_changed: crtc_state->mode or crtc_state->enable has been changed
  * @active_changed: crtc_state->active has been toggled.
  * @connectors_changed: connectors to this crtc have been updated
+ * @color_correction_changed: color correction blob in this crtc got updated
  * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
  * @last_vblank_count: for helpers and drivers to capture the vblank of the
  * 	update to ensure framebuffer cleanup isn't done too early
  * @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings
  * @mode: current mode timings
+ * @palette_before_ctm_blob: blob for color corrections to be applied after CTM
+ * @palette_after_ctm_blob: blob for color corrections to be applied before CTM
+ * @ctm_blob: blob for CTM color correction
  * @event: optional pointer to a DRM event to signal upon completion of the
  * 	state update
  * @state: backpointer to global drm_atomic_state
@@ -331,6 +335,7 @@ struct drm_crtc_state {
 	bool mode_changed : 1;
 	bool active_changed : 1;
 	bool connectors_changed : 1;
+	bool color_correction_changed : 1;
 
 	/* attached planes bitmask:
 	 * WARNING: transitional helpers do not maintain plane_mask so
@@ -350,6 +355,11 @@ struct drm_crtc_state {
 	/* blob property to expose current mode to atomic userspace */
 	struct drm_property_blob *mode_blob;
 
+	/* Color management 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;
@@ -2019,6 +2029,11 @@ struct drm_mode_config_funcs {
  * @property_blob_list: list of all the blob property objects
  * @blob_lock: mutex for blob property allocation and management
  * @*_property: core property tracking
+ * @cm_palette_before_ctm_property: color corrections before CTM block
+ * @cm_palette_after_ctm_property: color corrections after CTM block
+ * @cm_ctm_property: color transformation matrix correction
+ * @cm_coeff_before_ctm_property: query no of correction coeffi before CTM
+ * @cm_coeff_after_ctm_property: query no of correction coeffi after CTM
  * @preferred_depth: preferred RBG pixel depth, used by fb helpers
  * @prefer_shadow: hint to userspace to prefer shadow-fb rendering
  * @async_page_flip: does this device support async flips on the primary plane?
@@ -2124,6 +2139,15 @@ struct drm_mode_config {
 	struct drm_property *suggested_x_property;
 	struct drm_property *suggested_y_property;
 
+	/* Color correction properties */
+	struct drm_property *cm_palette_before_ctm_property;
+	struct drm_property *cm_palette_after_ctm_property;
+	struct drm_property *cm_ctm_property;
+
+	/* Color correction query */
+	struct drm_property *cm_coeff_before_ctm_property;
+	struct drm_property *cm_coeff_after_ctm_property;
+
 	/* dumb ioctl parameters */
 	uint32_t preferred_depth, prefer_shadow;
 
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index b4e92eb..24e4520 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -830,6 +830,36 @@ struct drm_event_vblank {
 	__u32 reserved;
 };
 
+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;
+	__u32 reserved;
+};
+
+struct drm_palette {
+	/*
+	 * Starting of palette LUT in R32G32B32 format.
+	 * Each of RGB value is in U8.24 fixed point format.
+	 */
+	struct drm_r32g32b32 lut[0];
+};
+
+struct drm_ctm {
+	/*
+	 * 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;
-- 
2.6.3

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

Re: [PATCH 1/6] drm: Create Color Management DRM properties

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 11342 bytes --]

Hi Shashank,

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on next-20151217]
[cannot apply to v4.4-rc5]

url:    https://github.com/0day-ci/linux/commits/Lionel-Landwerlin/Color-Management-for-DRM-framework-v10/20151218-025917
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/i915/i915_irq.c:2647: warning: No description found for parameter 'wedged'
   drivers/gpu/drm/i915/i915_irq.c:2647: warning: No description found for parameter 'fmt'
>> drivers/gpu/drm/drm_atomic.c:405: warning: No description found for parameter 'dev'
   include/drm/drm_crtc.h:324: warning: No description found for parameter 'mode_blob'
   include/drm/drm_crtc.h:751: warning: No description found for parameter 'tile_blob_ptr'
   include/drm/drm_crtc.h:790: warning: No description found for parameter 'rotation'
   include/drm/drm_crtc.h:886: warning: No description found for parameter 'mutex'
   include/drm/drm_crtc.h:886: warning: No description found for parameter 'helper_private'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tile_idr'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'delayed_event'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'edid_property'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'dpms_property'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'path_property'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tile_property'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'plane_type_property'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'rotation_property'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'prop_src_x'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'prop_src_y'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'prop_src_w'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'prop_src_h'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'prop_crtc_x'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'prop_crtc_y'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'prop_crtc_w'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'prop_crtc_h'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'prop_fb_id'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'prop_crtc_id'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'prop_active'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'prop_mode_id'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'dvi_i_subconnector_property'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'dvi_i_select_subconnector_property'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tv_subconnector_property'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tv_select_subconnector_property'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tv_mode_property'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tv_left_margin_property'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tv_right_margin_property'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tv_top_margin_property'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tv_bottom_margin_property'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tv_brightness_property'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tv_contrast_property'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tv_flicker_reduction_property'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tv_overscan_property'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tv_saturation_property'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tv_hue_property'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'scaling_mode_property'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'aspect_ratio_property'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'dirty_info_property'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'suggested_x_property'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'suggested_y_property'
   include/drm/drm_crtc.h:1185: warning: No description found for parameter 'allow_fb_modifiers'
   include/drm/drm_fb_helper.h:148: warning: No description found for parameter 'connector_info'
   include/drm/drm_dp_helper.h:749: warning: No description found for parameter 'i2c_nack_count'
   include/drm/drm_dp_helper.h:749: warning: No description found for parameter 'i2c_defer_count'
   drivers/gpu/drm/drm_dp_mst_topology.c:2237: warning: No description found for parameter 'connector'
   include/drm/drm_dp_mst_helper.h:98: warning: No description found for parameter 'cached_edid'
   include/drm/drm_dp_mst_helper.h:98: warning: No description found for parameter 'has_audio'
   include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'max_dpcd_transaction_bytes'
   include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'sink_count'
   include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'total_slots'
   include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'avail_slots'
   include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'total_pbn'
   include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'qlock'
   include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'tx_msg_downq'
   include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'tx_msg_upq'
   include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'tx_down_in_progress'
   include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'tx_up_in_progress'
   include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'payload_lock'
   include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'proposed_vcpis'
   include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'payloads'
   include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'payload_mask'
   include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'vcpi_mask'
   include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'tx_waitq'
   include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'work'
   include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'tx_work'
   include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'destroy_connector_list'
   include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'destroy_connector_lock'
   include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'destroy_connector_work'
   drivers/gpu/drm/drm_dp_mst_topology.c:2237: warning: No description found for parameter 'connector'
   drivers/gpu/drm/drm_irq.c:173: warning: No description found for parameter 'flags'
   include/drm/drmP.h:168: warning: No description found for parameter 'fmt'
   include/drm/drmP.h:184: warning: No description found for parameter 'fmt'
   include/drm/drmP.h:202: warning: No description found for parameter 'fmt'
   include/drm/drmP.h:247: warning: No description found for parameter 'dev'
   include/drm/drmP.h:247: warning: No description found for parameter 'data'
   include/drm/drmP.h:247: warning: No description found for parameter 'file_priv'
   include/drm/drmP.h:280: warning: No description found for parameter 'ioctl'
   include/drm/drmP.h:280: warning: No description found for parameter '_func'
   include/drm/drmP.h:280: warning: No description found for parameter '_flags'
   include/drm/drmP.h:360: warning: cannot understand function prototype: 'struct drm_lock_data '
   include/drm/drmP.h:413: warning: cannot understand function prototype: 'struct drm_driver '
   include/drm/drmP.h:663: warning: cannot understand function prototype: 'struct drm_info_list '
   include/drm/drmP.h:673: warning: cannot understand function prototype: 'struct drm_info_node '
   include/drm/drmP.h:683: warning: cannot understand function prototype: 'struct drm_minor '
   include/drm/drmP.h:731: warning: cannot understand function prototype: 'struct drm_device '
   drivers/gpu/drm/i915/intel_runtime_pm.c:2180: warning: No description found for parameter 'resume'
   drivers/gpu/drm/i915/intel_runtime_pm.c:2180: warning: No description found for parameter 'resume'
   drivers/gpu/drm/i915/i915_irq.c:2647: warning: No description found for parameter 'wedged'
   drivers/gpu/drm/i915/i915_irq.c:2647: warning: No description found for parameter 'fmt'
   drivers/gpu/drm/i915/i915_irq.c:2647: warning: No description found for parameter 'wedged'
   drivers/gpu/drm/i915/i915_irq.c:2647: warning: No description found for parameter 'fmt'
   drivers/gpu/drm/i915/i915_irq.c:2647: warning: No description found for parameter 'wedged'
   drivers/gpu/drm/i915/i915_irq.c:2647: warning: No description found for parameter 'fmt'
   drivers/gpu/drm/i915/i915_irq.c:2647: warning: No description found for parameter 'wedged'
   drivers/gpu/drm/i915/i915_irq.c:2647: warning: No description found for parameter 'fmt'

vim +/dev +405 drivers/gpu/drm/drm_atomic.c

   389	
   390	/**
   391	 * drm_atomic_crtc_set_blob - find and set a blob
   392	 * @state_blob: reference pointer to the color blob in the crtc_state
   393	 * @blob_id: blob_id coming from set_property() call
   394	 *
   395	 * Set a color correction blob (originating from a set blob property) on the
   396	 * desired CRTC state. This function will take reference of the blob property
   397	 * in the CRTC state, finds the blob based on blob_id (which comes from
   398	 * set_property call) and set the blob at the proper place.
   399	 *
   400	 * RETURNS:
   401	 * Zero on success, error code on failure.
   402	 */
   403	static int drm_atomic_crtc_set_blob(struct drm_device *dev,
   404		struct drm_property_blob **state_blob, uint32_t blob_id)
 > 405	{
   406		struct drm_property_blob *blob;
   407	
   408		blob = drm_property_lookup_blob(dev, blob_id);
   409		if (!blob) {
   410			DRM_DEBUG_KMS("Invalid Blob ID\n");
   411			return -EINVAL;
   412		}
   413	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 6125 bytes --]

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

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

Re: [PATCH 1/6] drm: Create Color Management DRM properties

[Daniel Stone]

[Paging danvet to the bottom paragraphs re client-cap ...]

Hi Lionel,
I've still got quite a few concerns about the implementation as it
stands. Some are minor quibbles (e.g. can't unset blob IDs), some are
larger design issues, some are rehashed comments from previous series,
and some are new now I've looked at it afresh.

On 17 December 2015 at 18:57, Lionel Landwerlin
<lionel.g.landwerlin@intel.com> wrote:
> DRM color manager supports these color properties:
> 1. "ctm": Color transformation matrix property, where a
>    color transformation matrix of 9 correction values gets
>    applied as correction.
> 2. "palette_before_ctm": for corrections which get applied
>    beore color transformation matrix correction.
> 3. "palette_after_ctm": for corrections which get applied
>    after color transformation matrix correction.

These all appear to be pretty common, at least for the platforms I've
checked. So, sounds good. You might want to document that before and
after palettes are often referred to as degamma and gamma,
respectively, though.

>  /**
> + * drm_atomic_crtc_set_blob - find and set a blob
> + * @state_blob: reference pointer to the color blob in the crtc_state
> + * @blob_id: blob_id coming from set_property() call
> + *
> + * Set a color correction blob (originating from a set blob property) on the
> + * desired CRTC state. This function will take reference of the blob property
> + * in the CRTC state, finds the blob based on blob_id (which comes from
> + * set_property call) and set the blob at the proper place.
> + *
> + * RETURNS:
> + * Zero on success, error code on failure.
> + */
> +static int drm_atomic_crtc_set_blob(struct drm_device *dev,
> +       struct drm_property_blob **state_blob, 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;
> +       }

This needs to handle blob_id == 0, to unset it. Initialising 'blob' to
NULL and wrapping the lookup_blob check in if (blob_id != 0) should do
it. While you're at it, a more helpful error message (e.g. actually
listing the blob ID) might be in order. And IGT.

Making crtc_state->MODE_ID use this helper, and kms_atomic not
breaking, would be a fairly good indication that you've got it right.
;)

> @@ -305,11 +305,15 @@ struct drm_plane_helper_funcs;
>   * @mode_changed: crtc_state->mode or crtc_state->enable has been changed
>   * @active_changed: crtc_state->active has been toggled.
>   * @connectors_changed: connectors to this crtc have been updated
> + * @color_correction_changed: color correction blob in this crtc got updated
>   * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
>   * @last_vblank_count: for helpers and drivers to capture the vblank of the
>   *     update to ensure framebuffer cleanup isn't done too early
>   * @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings
>   * @mode: current mode timings
> + * @palette_before_ctm_blob: blob for color corrections to be applied after CTM
> + * @palette_after_ctm_blob: blob for color corrections to be applied before CTM
> + * @ctm_blob: blob for CTM color correction

For instance, documenting (de)gamma terminology here might be nice.

> @@ -2019,6 +2029,11 @@ struct drm_mode_config_funcs {
>   * @property_blob_list: list of all the blob property objects
>   * @blob_lock: mutex for blob property allocation and management
>   * @*_property: core property tracking
> + * @cm_palette_before_ctm_property: color corrections before CTM block
> + * @cm_palette_after_ctm_property: color corrections after CTM block
> + * @cm_ctm_property: color transformation matrix correction
> + * @cm_coeff_before_ctm_property: query no of correction coeffi before CTM
> + * @cm_coeff_after_ctm_property: query no of correction coeffi after CTM

'coeffi'? These also aren't sufficient either (see my descent into
madness in the BDW patch), I don't think.

> +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;
> +       __u32 reserved;
> +};

Wait, it's U8.24 (i.e. 0 -> 255.09999999403953517), but the supported
range is [0..1.0]? What am I missing?

> +struct drm_palette {
> +       /*
> +        * Starting of palette LUT in R32G32B32 format.
> +        * Each of RGB value is in U8.24 fixed point format.
> +        */
> +       struct drm_r32g32b32 lut[0];
> +};

Why the indirection; can we just expose the member directly?

> +struct drm_ctm {
> +       /*
> +        * 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];
> +};

Perhaps ditto, but this seems more valid. As a total bikeshed,
'ctm_coeff' is redundant in a structure named drm_ctm anyway. Having
these namespaced into something consistent (e.g. drm_color_ or
whatever) might be nice too.

All in all, I have to admit I'm fairly baffled as to how I'd actually
use this from generic userspace. Not being able to divine the correct
list length (see BDW) is a showstopper. Having repeated conflicting
comments on the range (8.24 or 0.24?) is greatly confusing. Sign
wraparound on CHV is confusing, as is the '8-bit' vs. '10-bit' modes
which appear to do exactly the same thing (but with one more entry).
Not supporting blob == NULL to disable is pretty much a non-starter.

I also don't have a good answer on how to support non-CM-aware
clients. Currently they'll just blindly carry around the correction
factors from previous clients, which is _really_ bad: imagine you have
Weston ported to use KMS/CM to correct perfectly, and then start
Mutter/GNOME which still corrects perfectly, but using lcms and
various client-side compensation rather than the CM engine. Your
output will now be double-corrected, i.e. wrong, because Mutter won't
know to reset the CM properties.

About the only thing I can think of is to add a
DRM_CLIENT_CAP_COLOR_MGMT, and modify drm_atomic_*_duplicate_state to
take client caps (passed in from file_priv with the atomic ioctl, or
explicitly set by other kernel code, according to its capabilities),
and if the CM cap is not set, clear out the blobs when duplicating
state.

All of this also needs to be backed up by a lot more extensive IGT,
including disabling (from _every_ mode, not just 12-bit mode on BDW)
via setting blob == NULL, testing the various depths (I still don't
understand the difference between 8 and 10-bit on CHV), legacy gamma
hooks when using CM, testing reset/dumb clients when the above
duplicate_state issue is resolved, and especially the boundary cases
in conversions (e.g. the sign wraparound on CHV). The documentation
also _really_ needs fixing to be consistent with the code, and
consistent with itself. When that's done, I think I'll be
better-placed to do a second pass review, because after however many
revisions, I still can't clearly see how it would be used from generic
userspace (as opposed to an Intel-specific colour manager).

Sorry this probably isn't quite the Christmas present you'd hoped for ...

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

Re: [PATCH 1/6] drm: Create Color Management DRM properties

[Daniel Vetter]

On Fri, Dec 18, 2015 at 04:53:28PM +0000, Daniel Stone wrote:
> [Paging danvet to the bottom paragraphs re client-cap ...]
> 
> Hi Lionel,
> I've still got quite a few concerns about the implementation as it
> stands. Some are minor quibbles (e.g. can't unset blob IDs), some are
> larger design issues, some are rehashed comments from previous series,
> and some are new now I've looked at it afresh.
> 
> On 17 December 2015 at 18:57, Lionel Landwerlin
> <lionel.g.landwerlin@intel.com> wrote:
> > DRM color manager supports these color properties:
> > 1. "ctm": Color transformation matrix property, where a
> >    color transformation matrix of 9 correction values gets
> >    applied as correction.
> > 2. "palette_before_ctm": for corrections which get applied
> >    beore color transformation matrix correction.
> > 3. "palette_after_ctm": for corrections which get applied
> >    after color transformation matrix correction.
> 
> These all appear to be pretty common, at least for the platforms I've
> checked. So, sounds good. You might want to document that before and
> after palettes are often referred to as degamma and gamma,
> respectively, though.
> 
> >  /**
> > + * drm_atomic_crtc_set_blob - find and set a blob
> > + * @state_blob: reference pointer to the color blob in the crtc_state
> > + * @blob_id: blob_id coming from set_property() call
> > + *
> > + * Set a color correction blob (originating from a set blob property) on the
> > + * desired CRTC state. This function will take reference of the blob property
> > + * in the CRTC state, finds the blob based on blob_id (which comes from
> > + * set_property call) and set the blob at the proper place.
> > + *
> > + * RETURNS:
> > + * Zero on success, error code on failure.
> > + */
> > +static int drm_atomic_crtc_set_blob(struct drm_device *dev,
> > +       struct drm_property_blob **state_blob, 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;
> > +       }
> 
> This needs to handle blob_id == 0, to unset it. Initialising 'blob' to
> NULL and wrapping the lookup_blob check in if (blob_id != 0) should do
> it. While you're at it, a more helpful error message (e.g. actually
> listing the blob ID) might be in order. And IGT.
> 
> Making crtc_state->MODE_ID use this helper, and kms_atomic not
> breaking, would be a fairly good indication that you've got it right.
> ;)
> 
> > @@ -305,11 +305,15 @@ struct drm_plane_helper_funcs;
> >   * @mode_changed: crtc_state->mode or crtc_state->enable has been changed
> >   * @active_changed: crtc_state->active has been toggled.
> >   * @connectors_changed: connectors to this crtc have been updated
> > + * @color_correction_changed: color correction blob in this crtc got updated
> >   * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
> >   * @last_vblank_count: for helpers and drivers to capture the vblank of the
> >   *     update to ensure framebuffer cleanup isn't done too early
> >   * @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings
> >   * @mode: current mode timings
> > + * @palette_before_ctm_blob: blob for color corrections to be applied after CTM
> > + * @palette_after_ctm_blob: blob for color corrections to be applied before CTM
> > + * @ctm_blob: blob for CTM color correction
> 
> For instance, documenting (de)gamma terminology here might be nice.
> 
> > @@ -2019,6 +2029,11 @@ struct drm_mode_config_funcs {
> >   * @property_blob_list: list of all the blob property objects
> >   * @blob_lock: mutex for blob property allocation and management
> >   * @*_property: core property tracking
> > + * @cm_palette_before_ctm_property: color corrections before CTM block
> > + * @cm_palette_after_ctm_property: color corrections after CTM block
> > + * @cm_ctm_property: color transformation matrix correction
> > + * @cm_coeff_before_ctm_property: query no of correction coeffi before CTM
> > + * @cm_coeff_after_ctm_property: query no of correction coeffi after CTM
> 
> 'coeffi'? These also aren't sufficient either (see my descent into
> madness in the BDW patch), I don't think.

Yeah there's a question about how to best document new atomic extensions.
Traditionally we haven't documented the properties themselves (hence the
*_property), and I think that makes sense since it's a very large pile.
Two options left:
- Extensively document all the properties for a new feature in the drm
  core function to attach the props to a plane/crtc.

- Document them in the drm_*_state structures. There we can even use the
  new per-member kerneldoc layout to go into great details.

Probably best to do both and link between the two places.
> 
> > +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;
> > +       __u32 reserved;
> > +};
> 
> Wait, it's U8.24 (i.e. 0 -> 255.09999999403953517), but the supported
> range is [0..1.0]? What am I missing?

Probably means:
- all platforms MUST support [0.0, 1.0] range, inclusive
- platforms CAN support larger values (which can make sense in degamma
  tables if your CTM will shrink things down again).

> 
> > +struct drm_palette {
> > +       /*
> > +        * Starting of palette LUT in R32G32B32 format.
> > +        * Each of RGB value is in U8.24 fixed point format.
> > +        */
> > +       struct drm_r32g32b32 lut[0];
> > +};
> 
> Why the indirection; can we just expose the member directly?
> 
> > +struct drm_ctm {
> > +       /*
> > +        * 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];
> > +};
> 
> Perhaps ditto, but this seems more valid. As a total bikeshed,
> 'ctm_coeff' is redundant in a structure named drm_ctm anyway. Having
> these namespaced into something consistent (e.g. drm_color_ or
> whatever) might be nice too.
> 
> All in all, I have to admit I'm fairly baffled as to how I'd actually
> use this from generic userspace. Not being able to divine the correct
> list length (see BDW) is a showstopper. Having repeated conflicting
> comments on the range (8.24 or 0.24?) is greatly confusing. Sign
> wraparound on CHV is confusing, as is the '8-bit' vs. '10-bit' modes
> which appear to do exactly the same thing (but with one more entry).
> Not supporting blob == NULL to disable is pretty much a non-starter.
> 
> I also don't have a good answer on how to support non-CM-aware
> clients. Currently they'll just blindly carry around the correction
> factors from previous clients, which is _really_ bad: imagine you have
> Weston ported to use KMS/CM to correct perfectly, and then start
> Mutter/GNOME which still corrects perfectly, but using lcms and
> various client-side compensation rather than the CM engine. Your
> output will now be double-corrected, i.e. wrong, because Mutter won't
> know to reset the CM properties.

Hm yeah, I think legacy entry points should remap to atomic ones.
Otherwise things are massively inconsistent (and we have a problem
figuring out when/which table applies in the driver). One problem with
that approach is that the legacy table has the assumption of a fixed
256 entries (well we expose the size somewhere, but that's what everyone
uses). At least on some platforms, the full-blown table used by these
patches isn't even an integer multiple of that.

> About the only thing I can think of is to add a
> DRM_CLIENT_CAP_COLOR_MGMT, and modify drm_atomic_*_duplicate_state to
> take client caps (passed in from file_priv with the atomic ioctl, or
> explicitly set by other kernel code, according to its capabilities),
> and if the CM cap is not set, clear out the blobs when duplicating
> state.
> 
> All of this also needs to be backed up by a lot more extensive IGT,
> including disabling (from _every_ mode, not just 12-bit mode on BDW)
> via setting blob == NULL, testing the various depths (I still don't
> understand the difference between 8 and 10-bit on CHV), legacy gamma
> hooks when using CM, testing reset/dumb clients when the above
> duplicate_state issue is resolved, and especially the boundary cases
> in conversions (e.g. the sign wraparound on CHV). The documentation
> also _really_ needs fixing to be consistent with the code, and
> consistent with itself. When that's done, I think I'll be
> better-placed to do a second pass review, because after however many
> revisions, I still can't clearly see how it would be used from generic
> userspace (as opposed to an Intel-specific colour manager).

One bit I'm not sure (and which isn't documented here) is that there's
supposed to be a read-only hint property telling new generic userspace
what tables are expected. This might mean you're not always using the most
awesome configuration, but it should at least work. That part of the
design seems to be undocumented.
-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 1/6] drm: Create Color Management DRM properties

[Daniel Stone]

Hi,

On 21 December 2015 at 12:38, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Dec 18, 2015 at 04:53:28PM +0000, Daniel Stone wrote:
>> > +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;
>> > +       __u32 reserved;
>> > +};
>>
>> Wait, it's U8.24 (i.e. 0 -> 255.09999999403953517), but the supported
>> range is [0..1.0]? What am I missing?
>
> Probably means:
> - all platforms MUST support [0.0, 1.0] range, inclusive
> - platforms CAN support larger values (which can make sense in degamma
>   tables if your CTM will shrink things down again).

Ah, makes sense.

>> I also don't have a good answer on how to support non-CM-aware
>> clients. Currently they'll just blindly carry around the correction
>> factors from previous clients, which is _really_ bad: imagine you have
>> Weston ported to use KMS/CM to correct perfectly, and then start
>> Mutter/GNOME which still corrects perfectly, but using lcms and
>> various client-side compensation rather than the CM engine. Your
>> output will now be double-corrected, i.e. wrong, because Mutter won't
>> know to reset the CM properties.
>
> Hm yeah, I think legacy entry points should remap to atomic ones.
> Otherwise things are massively inconsistent (and we have a problem
> figuring out when/which table applies in the driver). One problem with
> that approach is that the legacy table has the assumption of a fixed
> 256 entries (well we expose the size somewhere, but that's what everyone
> uses). At least on some platforms, the full-blown table used by these
> patches isn't even an integer multiple of that.

It's not even a legacy vs. atomic thing, this can happen in
pure-atomic as well. Same as the render-compression plane property
that I just replied to here as well.

- Weston starts and sets up palette/CTM properties
- VT switch to Mutter, which isn't aware of new properties
- Mutter atomically sets new plane state, containing framebuffer which
is already colour-corrected for the target
- result is now double-corrected as Mutter didn't know to unset the
old properties

IOW, in the current model, any user of CM has to explicitly unset it
before handover (not always actually possible), or any generic
userspace must unset every property it sees and doesn't know about,
hoping that setting to 0 will do the right thing.

Or we could add another client cap, which would prevent the CM
properties from ever being exposed to userspace which doesn't know how
to work with it. Passing the client caps through to
plane_duplicate_state also means that we can unset the CM properties
at this early point for unaware clients. This would mean that we
wouldn't have to have code to explicitly unset it in, e.g., every
legacy hook.

>> About the only thing I can think of is to add a
>> DRM_CLIENT_CAP_COLOR_MGMT, and modify drm_atomic_*_duplicate_state to
>> take client caps (passed in from file_priv with the atomic ioctl, or
>> explicitly set by other kernel code, according to its capabilities),
>> and if the CM cap is not set, clear out the blobs when duplicating
>> state.

(As here.)

>> All of this also needs to be backed up by a lot more extensive IGT,
>> including disabling (from _every_ mode, not just 12-bit mode on BDW)
>> via setting blob == NULL, testing the various depths (I still don't
>> understand the difference between 8 and 10-bit on CHV), legacy gamma
>> hooks when using CM, testing reset/dumb clients when the above
>> duplicate_state issue is resolved, and especially the boundary cases
>> in conversions (e.g. the sign wraparound on CHV). The documentation
>> also _really_ needs fixing to be consistent with the code, and
>> consistent with itself. When that's done, I think I'll be
>> better-placed to do a second pass review, because after however many
>> revisions, I still can't clearly see how it would be used from generic
>> userspace (as opposed to an Intel-specific colour manager).
>
> One bit I'm not sure (and which isn't documented here) is that there's
> supposed to be a read-only hint property telling new generic userspace
> what tables are expected. This might mean you're not always using the most
> awesome configuration, but it should at least work. That part of the
> design seems to be undocumented.

Yeah, there is a single 'length' property per table, but given the
dizzying array of options available, I'm not really sure if that's
sufficient.

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

Re: [PATCH 1/6] drm: Create Color Management DRM properties

[Daniel Vetter]

On Wed, Dec 23, 2015 at 09:47:00AM +0000, Daniel Stone wrote:
> Hi,
> 
> On 21 December 2015 at 12:38, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Fri, Dec 18, 2015 at 04:53:28PM +0000, Daniel Stone wrote:
> >> > +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;
> >> > +       __u32 reserved;
> >> > +};
> >>
> >> Wait, it's U8.24 (i.e. 0 -> 255.09999999403953517), but the supported
> >> range is [0..1.0]? What am I missing?
> >
> > Probably means:
> > - all platforms MUST support [0.0, 1.0] range, inclusive
> > - platforms CAN support larger values (which can make sense in degamma
> >   tables if your CTM will shrink things down again).
> 
> Ah, makes sense.
> 
> >> I also don't have a good answer on how to support non-CM-aware
> >> clients. Currently they'll just blindly carry around the correction
> >> factors from previous clients, which is _really_ bad: imagine you have
> >> Weston ported to use KMS/CM to correct perfectly, and then start
> >> Mutter/GNOME which still corrects perfectly, but using lcms and
> >> various client-side compensation rather than the CM engine. Your
> >> output will now be double-corrected, i.e. wrong, because Mutter won't
> >> know to reset the CM properties.
> >
> > Hm yeah, I think legacy entry points should remap to atomic ones.
> > Otherwise things are massively inconsistent (and we have a problem
> > figuring out when/which table applies in the driver). One problem with
> > that approach is that the legacy table has the assumption of a fixed
> > 256 entries (well we expose the size somewhere, but that's what everyone
> > uses). At least on some platforms, the full-blown table used by these
> > patches isn't even an integer multiple of that.
> 
> It's not even a legacy vs. atomic thing, this can happen in
> pure-atomic as well. Same as the render-compression plane property
> that I just replied to here as well.
> 
> - Weston starts and sets up palette/CTM properties
> - VT switch to Mutter, which isn't aware of new properties
> - Mutter atomically sets new plane state, containing framebuffer which
> is already colour-corrected for the target
> - result is now double-corrected as Mutter didn't know to unset the
> old properties
> 
> IOW, in the current model, any user of CM has to explicitly unset it
> before handover (not always actually possible), or any generic
> userspace must unset every property it sees and doesn't know about,
> hoping that setting to 0 will do the right thing.
> 
> Or we could add another client cap, which would prevent the CM
> properties from ever being exposed to userspace which doesn't know how
> to work with it. Passing the client caps through to
> plane_duplicate_state also means that we can unset the CM properties
> at this early point for unaware clients. This would mean that we
> wouldn't have to have code to explicitly unset it in, e.g., every
> legacy hook.

We don't have any support for unsetting anything really. Same argument you
make for CM here applies to rotation too. The only generic solution (if
you really care about this) would be for logind to once sample all atomic
state on boot-up, and force-restore that state when switching. Restoring
atomic state doesn't require userspace to understanding the semantics
really.

This kind of force-restoring is probably something we should implement in
the fbdev emulation, which would cover about 99% of all cases where
developers/users want to run different compositors. Or fbdev should simply
reset CM state, like it does for rotation already (that part is easy to
add, but indeed seems to be missing).

Trying to create an ad-hoc solution (using opt-in flags) to this problem
for every single feature seems pointless imo.

> >> About the only thing I can think of is to add a
> >> DRM_CLIENT_CAP_COLOR_MGMT, and modify drm_atomic_*_duplicate_state to
> >> take client caps (passed in from file_priv with the atomic ioctl, or
> >> explicitly set by other kernel code, according to its capabilities),
> >> and if the CM cap is not set, clear out the blobs when duplicating
> >> state.
> 
> (As here.)
> 
> >> All of this also needs to be backed up by a lot more extensive IGT,
> >> including disabling (from _every_ mode, not just 12-bit mode on BDW)
> >> via setting blob == NULL, testing the various depths (I still don't
> >> understand the difference between 8 and 10-bit on CHV), legacy gamma
> >> hooks when using CM, testing reset/dumb clients when the above
> >> duplicate_state issue is resolved, and especially the boundary cases
> >> in conversions (e.g. the sign wraparound on CHV). The documentation
> >> also _really_ needs fixing to be consistent with the code, and
> >> consistent with itself. When that's done, I think I'll be
> >> better-placed to do a second pass review, because after however many
> >> revisions, I still can't clearly see how it would be used from generic
> >> userspace (as opposed to an Intel-specific colour manager).
> >
> > One bit I'm not sure (and which isn't documented here) is that there's
> > supposed to be a read-only hint property telling new generic userspace
> > what tables are expected. This might mean you're not always using the most
> > awesome configuration, but it should at least work. That part of the
> > design seems to be undocumented.
> 
> Yeah, there is a single 'length' property per table, but given the
> dizzying array of options available, I'm not really sure if that's
> sufficient.

The idea is to just give a hint to generic userspace. Fancy userspace that
wants to exploit all the other options needs to have some baked-in
knowledge of the hardware it runs on. This is kinda similar to how you can
do dumb buffers, but for fancy ones userspace (at least currently) needs
to just know how to allocate stuff. Imo there's just plain limits to how
generic KMS can be. And it's not worth pushing beyond those.
-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 1/6] drm: Create Color Management DRM properties

[Daniel Stone]

Hi,

On 5 January 2016 at 10:23, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Dec 23, 2015 at 09:47:00AM +0000, Daniel Stone wrote:
>> It's not even a legacy vs. atomic thing, this can happen in
>> pure-atomic as well. Same as the render-compression plane property
>> that I just replied to here as well.
>>
>> - Weston starts and sets up palette/CTM properties
>> - VT switch to Mutter, which isn't aware of new properties
>> - Mutter atomically sets new plane state, containing framebuffer which
>> is already colour-corrected for the target
>> - result is now double-corrected as Mutter didn't know to unset the
>> old properties
>>
>> IOW, in the current model, any user of CM has to explicitly unset it
>> before handover (not always actually possible), or any generic
>> userspace must unset every property it sees and doesn't know about,
>> hoping that setting to 0 will do the right thing.
>>
>> Or we could add another client cap, which would prevent the CM
>> properties from ever being exposed to userspace which doesn't know how
>> to work with it. Passing the client caps through to
>> plane_duplicate_state also means that we can unset the CM properties
>> at this early point for unaware clients. This would mean that we
>> wouldn't have to have code to explicitly unset it in, e.g., every
>> legacy hook.
>
> We don't have any support for unsetting anything really. Same argument you
> make for CM here applies to rotation too. The only generic solution (if
> you really care about this) would be for logind to once sample all atomic
> state on boot-up, and force-restore that state when switching. Restoring
> atomic state doesn't require userspace to understanding the semantics
> really.

Sure, but on the other hand, rotation has been around ~forever, and is
implemented in multiple places. Colour management is a lot less
obvious.

> This kind of force-restoring is probably something we should implement in
> the fbdev emulation, which would cover about 99% of all cases where
> developers/users want to run different compositors. Or fbdev should simply
> reset CM state, like it does for rotation already (that part is easy to
> add, but indeed seems to be missing).
>
> Trying to create an ad-hoc solution (using opt-in flags) to this problem
> for every single feature seems pointless imo.

Fair enough, I guess it could be more difficult, so how about a new
flag to the atomic ioctl which requests state be _reset_ to scratch
rather than duplicated? That way, clients could be really sure they
weren't going to get screwed by rotation / colour management / render
compression / whatever.

>> Yeah, there is a single 'length' property per table, but given the
>> dizzying array of options available, I'm not really sure if that's
>> sufficient.
>
> The idea is to just give a hint to generic userspace. Fancy userspace that
> wants to exploit all the other options needs to have some baked-in
> knowledge of the hardware it runs on. This is kinda similar to how you can
> do dumb buffers, but for fancy ones userspace (at least currently) needs
> to just know how to allocate stuff. Imo there's just plain limits to how
> generic KMS can be. And it's not worth pushing beyond those.

I get the point, but am really struggling to make sense of the options
presented, and how a halfway-sensible generic userspace would work.
Exposing length/depth tuples might not be the worst idea; it also
sounds like there's more to dig out in terms of index interpolation as
well. But these are much smaller objections than having unwitting
clients inherit the CM state ...

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

Re: [PATCH 1/6] drm: Create Color Management DRM properties

[Ville Syrjälä]

On Mon, Jan 11, 2016 at 08:37:09PM +0000, Daniel Stone wrote:
> Hi,
> 
> On 5 January 2016 at 10:23, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Dec 23, 2015 at 09:47:00AM +0000, Daniel Stone wrote:
> >> It's not even a legacy vs. atomic thing, this can happen in
> >> pure-atomic as well. Same as the render-compression plane property
> >> that I just replied to here as well.
> >>
> >> - Weston starts and sets up palette/CTM properties
> >> - VT switch to Mutter, which isn't aware of new properties
> >> - Mutter atomically sets new plane state, containing framebuffer which
> >> is already colour-corrected for the target
> >> - result is now double-corrected as Mutter didn't know to unset the
> >> old properties
> >>
> >> IOW, in the current model, any user of CM has to explicitly unset it
> >> before handover (not always actually possible), or any generic
> >> userspace must unset every property it sees and doesn't know about,
> >> hoping that setting to 0 will do the right thing.
> >>
> >> Or we could add another client cap, which would prevent the CM
> >> properties from ever being exposed to userspace which doesn't know how
> >> to work with it. Passing the client caps through to
> >> plane_duplicate_state also means that we can unset the CM properties
> >> at this early point for unaware clients. This would mean that we
> >> wouldn't have to have code to explicitly unset it in, e.g., every
> >> legacy hook.
> >
> > We don't have any support for unsetting anything really. Same argument you
> > make for CM here applies to rotation too. The only generic solution (if
> > you really care about this) would be for logind to once sample all atomic
> > state on boot-up, and force-restore that state when switching. Restoring
> > atomic state doesn't require userspace to understanding the semantics
> > really.
> 
> Sure, but on the other hand, rotation has been around ~forever, and is
> implemented in multiple places. Colour management is a lot less
> obvious.
> 
> > This kind of force-restoring is probably something we should implement in
> > the fbdev emulation, which would cover about 99% of all cases where
> > developers/users want to run different compositors. Or fbdev should simply
> > reset CM state, like it does for rotation already (that part is easy to
> > add, but indeed seems to be missing).
> >
> > Trying to create an ad-hoc solution (using opt-in flags) to this problem
> > for every single feature seems pointless imo.
> 
> Fair enough, I guess it could be more difficult, so how about a new
> flag to the atomic ioctl which requests state be _reset_ to scratch
> rather than duplicated? That way, clients could be really sure they
> weren't going to get screwed by rotation / colour management / render
> compression / whatever.

One question is what exactly is scratch? Eg. I have BYT tablet here
which has the display mounted upside down so in theory the reset state
should be 180 degree rotated. I have a patch hiding in some branch
to make the fbdev helper take over the rotation from the BIOS. I suppose
I should also dig into the VBT to see if there's some rotation indicators
there for the cases when you boot with HDMI plugged in. But I digress.

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

Re: [PATCH 1/6] drm: Create Color Management DRM properties

[Daniel Vetter]

On Thu, Dec 17, 2015 at 06:57:53PM +0000, Lionel Landwerlin wrote:
> From: Shashank Sharma <shashank.sharma@intel.com>
> 
> Color Management is an extension to DRM framework to allow hardware color
> correction and enhancement capabilities.
> 
> DRM color manager supports these color properties:
> 1. "ctm": Color transformation matrix property, where a
>    color transformation matrix of 9 correction values gets
>    applied as correction.
> 2. "palette_before_ctm": for corrections which get applied
>    beore color transformation matrix correction.
> 3. "palette_after_ctm": for corrections which get applied
>    after color transformation matrix correction.
> 
> These color correction capabilities may differ per platform, supporting
> various different no. of correction coefficients. So DRM color manager
> support few properties using which a user space can query the platform's
> capability, and prepare color correction accordingly.
> These query properties are:
> 1. cm_coeff_after_ctm_property
> 2. cm_coeff_before_ctm_property
>   (CTM is fix to 9 coefficients across industry)
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
> ---
>  drivers/gpu/drm/drm_atomic.c        | 67 +++++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/drm_atomic_helper.c |  9 +++++
>  drivers/gpu/drm/drm_crtc.c          | 32 ++++++++++++++++++
>  include/drm/drm_crtc.h              | 24 +++++++++++++
>  include/uapi/drm/drm.h              | 30 +++++++++++++++++
>  5 files changed, 160 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 6a21e5c..ebdb98d 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -388,6 +388,38 @@ int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state,
>  EXPORT_SYMBOL(drm_atomic_set_mode_prop_for_crtc);
>  
>  /**
> + * drm_atomic_crtc_set_blob - find and set a blob
> + * @state_blob: reference pointer to the color blob in the crtc_state
> + * @blob_id: blob_id coming from set_property() call
> + *
> + * Set a color correction blob (originating from a set blob property) on the
> + * desired CRTC state. This function will take reference of the blob property
> + * in the CRTC state, finds the blob based on blob_id (which comes from
> + * set_property call) and set the blob at the proper place.
> + *
> + * RETURNS:
> + * Zero on success, error code on failure.
> + */
> +static int drm_atomic_crtc_set_blob(struct drm_device *dev,
> +	struct drm_property_blob **state_blob, uint32_t blob_id)

I think we need to at least split this into a ctm and gamma table variant
to be able to have a size check for the passed-in blob:
- ctm must exactly match the size we expect
- gamma table must be an integer multiple of the drm_r32g32b32 struct.
  Also we should probably have a little helper to compute the size of the
  gamma lut in entries (just the size/sizeof(drm_r32g32b32) division
  really).


The other bits that's missing here is a drm core function to attach these
properties to planes/crtc. We don't want every driver to roll their own
implementation, since it'll lead to fun divergence between different
implementations. Also, that main function could be used to place the ABI
rules someplace nice (like how generic userspace is supposed to interact
with color manager props).
-Daniel

> +{
> +	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 (*state_blob)
> +		drm_property_unreference_blob(*state_blob);
> +
> +	/* Attach the blob to be committed in state */
> +	*state_blob = blob;
> +	return 0;
> +}
> +
> +/**
>   * drm_atomic_crtc_set_property - set property on CRTC
>   * @crtc: the drm CRTC to set a property on
>   * @state: the state object to update with the new property value
> @@ -419,8 +451,31 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  		ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
>  		drm_property_unreference_blob(mode);
>  		return ret;
> -	}
> -	else if (crtc->funcs->atomic_set_property)
> +	} else if (property == config->cm_palette_after_ctm_property) {
> +		ret = drm_atomic_crtc_set_blob(dev,
> +				&state->palette_after_ctm_blob, val);
> +		if (ret)
> +			DRM_DEBUG_KMS("Failed to load blob palette_after_ctm\n");
> +		else
> +			state->color_correction_changed = true;
> +		return ret;
> +	} else if (property == config->cm_palette_before_ctm_property) {
> +		ret = drm_atomic_crtc_set_blob(dev,
> +				&state->palette_before_ctm_blob, val);
> +		if (ret)
> +			DRM_DEBUG_KMS("Failed to load blob palette_before_ctm\n");
> +		else
> +			state->color_correction_changed = true;
> +		return ret;
> +	} else if (property == config->cm_ctm_property) {
> +		ret = drm_atomic_crtc_set_blob(dev,
> +				&state->ctm_blob, val);
> +		if (ret)
> +			DRM_DEBUG_KMS("Failed to load blob ctm\n");
> +		else
> +			state->color_correction_changed = true;
> +		return ret;
> +	} else if (crtc->funcs->atomic_set_property)
>  		return crtc->funcs->atomic_set_property(crtc, state, property, val);
>  	else
>  		return -EINVAL;
> @@ -456,6 +511,14 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
>  		*val = state->active;
>  	else if (property == config->prop_mode_id)
>  		*val = (state->mode_blob) ? state->mode_blob->base.id : 0;
> +	else if (property == config->cm_palette_after_ctm_property)
> +		*val = (state->palette_after_ctm_blob) ?
> +			state->palette_after_ctm_blob->base.id : 0;
> +	else if (property == config->cm_palette_before_ctm_property)
> +		*val = (state->palette_before_ctm_blob) ?
> +			state->palette_before_ctm_blob->base.id : 0;
> +	else if (property == config->cm_ctm_property)
> +		*val = (state->ctm_blob) ? state->ctm_blob->base.id : 0;
>  	else if (crtc->funcs->atomic_get_property)
>  		return crtc->funcs->atomic_get_property(crtc, state, property, val);
>  	else
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 268d37f..bd325da 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2448,6 +2448,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;
> @@ -2492,6 +2498,9 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc,
>  					    struct drm_crtc_state *state)
>  {
>  	drm_property_unreference_blob(state->mode_blob);
> +	drm_property_unreference_blob(state->ctm_blob);
> +	drm_property_unreference_blob(state->palette_after_ctm_blob);
> +	drm_property_unreference_blob(state->palette_before_ctm_blob);
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
>  
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 62fa95f..2e02d0f 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1542,6 +1542,38 @@ 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, "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;
> +
> +	/* DRM properties to query color capabilities */
> +	prop = drm_property_create(dev, DRM_MODE_PROP_IMMUTABLE,
> +			"COEFFICIENTS_BEFORE_CTM", 0);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.cm_coeff_before_ctm_property = prop;
> +
> +	prop = drm_property_create(dev, DRM_MODE_PROP_IMMUTABLE,
> +			"COEFFICIENTS_AFTER_CTM", 0);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.cm_coeff_after_ctm_property = prop;
> +
>  	return 0;
>  }
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 3b040b3..8326765 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -305,11 +305,15 @@ struct drm_plane_helper_funcs;
>   * @mode_changed: crtc_state->mode or crtc_state->enable has been changed
>   * @active_changed: crtc_state->active has been toggled.
>   * @connectors_changed: connectors to this crtc have been updated
> + * @color_correction_changed: color correction blob in this crtc got updated
>   * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
>   * @last_vblank_count: for helpers and drivers to capture the vblank of the
>   * 	update to ensure framebuffer cleanup isn't done too early
>   * @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings
>   * @mode: current mode timings
> + * @palette_before_ctm_blob: blob for color corrections to be applied after CTM
> + * @palette_after_ctm_blob: blob for color corrections to be applied before CTM
> + * @ctm_blob: blob for CTM color correction
>   * @event: optional pointer to a DRM event to signal upon completion of the
>   * 	state update
>   * @state: backpointer to global drm_atomic_state
> @@ -331,6 +335,7 @@ struct drm_crtc_state {
>  	bool mode_changed : 1;
>  	bool active_changed : 1;
>  	bool connectors_changed : 1;
> +	bool color_correction_changed : 1;
>  
>  	/* attached planes bitmask:
>  	 * WARNING: transitional helpers do not maintain plane_mask so
> @@ -350,6 +355,11 @@ struct drm_crtc_state {
>  	/* blob property to expose current mode to atomic userspace */
>  	struct drm_property_blob *mode_blob;
>  
> +	/* Color management 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;
> @@ -2019,6 +2029,11 @@ struct drm_mode_config_funcs {
>   * @property_blob_list: list of all the blob property objects
>   * @blob_lock: mutex for blob property allocation and management
>   * @*_property: core property tracking
> + * @cm_palette_before_ctm_property: color corrections before CTM block
> + * @cm_palette_after_ctm_property: color corrections after CTM block
> + * @cm_ctm_property: color transformation matrix correction
> + * @cm_coeff_before_ctm_property: query no of correction coeffi before CTM
> + * @cm_coeff_after_ctm_property: query no of correction coeffi after CTM
>   * @preferred_depth: preferred RBG pixel depth, used by fb helpers
>   * @prefer_shadow: hint to userspace to prefer shadow-fb rendering
>   * @async_page_flip: does this device support async flips on the primary plane?
> @@ -2124,6 +2139,15 @@ struct drm_mode_config {
>  	struct drm_property *suggested_x_property;
>  	struct drm_property *suggested_y_property;
>  
> +	/* Color correction properties */
> +	struct drm_property *cm_palette_before_ctm_property;
> +	struct drm_property *cm_palette_after_ctm_property;
> +	struct drm_property *cm_ctm_property;
> +
> +	/* Color correction query */
> +	struct drm_property *cm_coeff_before_ctm_property;
> +	struct drm_property *cm_coeff_after_ctm_property;
> +
>  	/* dumb ioctl parameters */
>  	uint32_t preferred_depth, prefer_shadow;
>  
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index b4e92eb..24e4520 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -830,6 +830,36 @@ struct drm_event_vblank {
>  	__u32 reserved;
>  };
>  
> +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;
> +	__u32 reserved;
> +};
> +
> +struct drm_palette {
> +	/*
> +	 * Starting of palette LUT in R32G32B32 format.
> +	 * Each of RGB value is in U8.24 fixed point format.
> +	 */
> +	struct drm_r32g32b32 lut[0];
> +};
> +
> +struct drm_ctm {
> +	/*
> +	 * 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;
> -- 
> 2.6.3
> 
> _______________________________________________
> 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

[PATCH 2/6] drm/i915: Add set property interface for CRTC

[Lionel Landwerlin]

From: Shashank Sharma <shashank.sharma@intel.com>

This patch adds 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_display.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index abd2d29..e04906d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13610,6 +13610,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
 	.page_flip = intel_crtc_page_flip,
 	.atomic_duplicate_state = intel_crtc_duplicate_state,
 	.atomic_destroy_state = intel_crtc_destroy_state,
+	.set_property = drm_atomic_helper_crtc_set_property,
 };
 
 static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv,
-- 
2.6.3

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

[PATCH 3/6] drm/i915: Disable plane gamma

[Lionel Landwerlin]

From: Shashank Sharma <shashank.sharma@intel.com>

In plane enabling sequence, plane gamma bit is by default enabled.
Plane gamma gets higher priority than pipe gamma, if both enabled.

This patch disables plane gamma from sequence. If required, plane
gamma can be enabled via the color manager drm interface.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Kumar, Kiran S <kiran.s.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 drivers/gpu/drm/i915/intel_sprite.c  | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e04906d..a05ba28 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2829,7 +2829,7 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
 
 	pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
 
-	dspcntr = DISPPLANE_GAMMA_ENABLE;
+	dspcntr = (DISPPLANE_GAMMA_ENABLE | PLANE_CTL_PLANE_GAMMA_DISABLE);
 
 	dspcntr |= DISPLAY_PLANE_ENABLE;
 
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 4ff7a1f..337167f 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -203,7 +203,8 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 
 	plane_ctl = PLANE_CTL_ENABLE |
 		PLANE_CTL_PIPE_GAMMA_ENABLE |
-		PLANE_CTL_PIPE_CSC_ENABLE;
+		PLANE_CTL_PIPE_CSC_ENABLE |
+		PLANE_CTL_PLANE_GAMMA_DISABLE;
 
 	plane_ctl |= skl_plane_ctl_format(fb->pixel_format);
 	plane_ctl |= skl_plane_ctl_tiling(fb->modifier[0]);
@@ -402,7 +403,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	 * Enable gamma to match primary/cursor plane behaviour.
 	 * FIXME should be user controllable via propertiesa.
 	 */
-	sprctl |= SP_GAMMA_ENABLE;
+	sprctl |= (SP_GAMMA_ENABLE | PLANE_CTL_PLANE_GAMMA_DISABLE);
 
 	if (obj->tiling_mode != I915_TILING_NONE)
 		sprctl |= SP_TILED;
@@ -521,7 +522,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	 * Enable gamma to match primary/cursor plane behaviour.
 	 * FIXME should be user controllable via propertiesa.
 	 */
-	sprctl |= SPRITE_GAMMA_ENABLE;
+	sprctl |= (SPRITE_GAMMA_ENABLE | PLANE_CTL_PLANE_GAMMA_DISABLE);
 
 	if (obj->tiling_mode != I915_TILING_NONE)
 		sprctl |= SPRITE_TILED;
-- 
2.6.3

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

[PATCH 4/6] drm/i915/chv: Implement color management

[Lionel Landwerlin]

From: Shashank Sharma <shashank.sharma@intel.com>

Implement degamma, csc and gamma steps on CHV.

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/i915_drv.c            |   5 +-
 drivers/gpu/drm/i915/i915_drv.h            |   2 +
 drivers/gpu/drm/i915/i915_reg.h            |  26 +++
 drivers/gpu/drm/i915/intel_color_manager.c | 353 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_color_manager.h |  91 ++++++++
 drivers/gpu/drm/i915/intel_display.c       |   2 +
 drivers/gpu/drm/i915/intel_drv.h           |   4 +
 8 files changed, 484 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

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 0851de07..c66d56a 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/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 3ac616d..4396300 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>
@@ -311,7 +312,9 @@ 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,
-	.is_cherryview = 1,
+	.is_valleyview = 1,
+	.num_samples_after_ctm = CHV_10BIT_GAMMA_MAX_VALS,
+	.num_samples_before_ctm = CHV_DEGAMMA_MAX_VALS,
 	.display_mmio_offset = VLV_DISPLAY_BASE,
 	GEN_CHV_PIPEOFFSETS,
 	CURSOR_OFFSETS,
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1d28d90..4eb0fab 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -793,6 +793,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/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 007ae83..36bb320 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -8154,4 +8154,30 @@ enum skl_disp_power_wells {
 #define GEN9_VEBOX_MOCS(i)	_MMIO(0xcb00 + (i) * 4)	/* Video MOCS registers */
 #define GEN9_BLT_MOCS(i)	_MMIO(0xcc00 + (i) * 4)	/* Blitter MOCS registers */
 
+/* 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))
+
+#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))
+
+#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
new file mode 100644
index 0000000..02eee98
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -0,0 +1,353 @@
+/*
+ * 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"
+
+static u16 chv_prepare_csc_coeff(s64 csc_coeff)
+{
+	u16 csc_s3_12_format = 0;
+	u16 csc_int_value;
+	u16 csc_fract_value;
+
+	if (csc_coeff < 0)
+		csc_s3_12_format = CSC_COEFF_SIGN;
+	csc_coeff = abs(csc_coeff);
+	csc_coeff += CHV_CSC_FRACT_ROUNDOFF;
+	if (csc_coeff > CHV_CSC_COEFF_MAX + 1)
+		csc_coeff = CHV_CSC_COEFF_MAX + 1;
+	else if (csc_coeff > CHV_CSC_COEFF_MAX)
+		csc_coeff = CHV_CSC_COEFF_MAX;
+
+	csc_int_value = csc_coeff >> CHV_CSC_COEFF_SHIFT;
+	csc_int_value <<= CHV_CSC_COEFF_INT_SHIFT;
+
+	csc_fract_value = csc_coeff >> 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)
+{
+	u16 temp;
+	u32 word = 0;
+	int count = 0;
+	i915_reg_t val;
+	struct drm_ctm *csc_data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	enum pipe pipe;
+
+	if (WARN_ON(!blob))
+		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;
+	pipe = to_intel_crtc(crtc)->pipe;
+
+	/* Disable CSC functionality */
+	val = _MMIO(_PIPE_CGM_CONTROL(pipe));
+	I915_WRITE(val, I915_READ(val) & (~CGM_CSC_EN));
+
+	DRM_DEBUG_DRIVER("Disabled CSC Functionality on Pipe %c\n",
+			pipe_name(pipe));
+
+	val = _MMIO(_PIPE_CSC_BASE(pipe));
+
+	/*
+	* First 8 of 9 CSC correction values go in pair, to first
+	* 4 CSC register (bit 0:15 and 16:31)
+	*/
+	while (count < CSC_MAX_VALS - 1) {
+		temp = chv_prepare_csc_coeff(
+					csc_data->ctm_coeff[count]);
+		SET_BITS(word, temp, 0, 16);
+		count++;
+
+		temp = chv_prepare_csc_coeff(
+				csc_data->ctm_coeff[count]);
+		SET_BITS(word, temp, 16, 16);
+		count++;
+
+		I915_WRITE(val, word);
+		val.reg += 4;
+	}
+
+	/* 9th coeff goes to 5th register, bit 0:16 */
+	temp = chv_prepare_csc_coeff(
+			csc_data->ctm_coeff[count]);
+	SET_BITS(word, temp, 0, 16);
+	I915_WRITE(val, word);
+
+	/* Enable CSC functionality */
+	val = _MMIO(_PIPE_CGM_CONTROL(pipe));
+	I915_WRITE(val, I915_READ(val) | 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)
+{
+	u16 red_fract, green_fract, blue_fract;
+	u32 red, green, blue;
+	u32 num_samples;
+	u32 word = 0;
+	u32 count, cgm_control, cgm_degamma_reg;
+	i915_reg_t val;
+	enum pipe pipe;
+	struct drm_palette *degamma_data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_r32g32b32 *correction_values = NULL;
+	struct drm_crtc_state *state = crtc->state;
+
+	if (WARN_ON(!blob))
+		return -EINVAL;
+
+	degamma_data = (struct drm_palette *)blob->data;
+	pipe = to_intel_crtc(crtc)->pipe;
+	num_samples = blob->length / sizeof(struct drm_r32g32b32);
+
+	switch (num_samples) {
+	case GAMMA_DISABLE_VALS:
+		/* Disable DeGamma functionality on Pipe - CGM Block */
+		val = _MMIO(_PIPE_CGM_CONTROL(pipe));
+		cgm_control = I915_READ(val);
+		cgm_control &= ~CGM_DEGAMMA_EN;
+		state->palette_before_ctm_blob = NULL;
+
+		I915_WRITE(val, cgm_control);
+		DRM_DEBUG_DRIVER("DeGamma disabled on Pipe %c\n",
+				pipe_name(pipe));
+		break;
+
+	case CHV_DEGAMMA_MAX_VALS:
+		cgm_degamma_reg = _PIPE_DEGAMMA_BASE(pipe);
+		count = 0;
+		correction_values = degamma_data->lut;
+		while (count < CHV_DEGAMMA_MAX_VALS) {
+			blue = correction_values[count].b32;
+			green = correction_values[count].g32;
+			red = correction_values[count].r32;
+
+			if (blue > CHV_MAX_GAMMA)
+				blue = CHV_MAX_GAMMA;
+
+			if (green > CHV_MAX_GAMMA)
+				green = CHV_MAX_GAMMA;
+
+			if (red > CHV_MAX_GAMMA)
+				red = CHV_MAX_GAMMA;
+
+			blue_fract = GET_BITS(blue, 10, 14);
+			green_fract = GET_BITS(green, 10, 14);
+			red_fract = GET_BITS(red, 10, 14);
+
+			/* Green (29:16) and Blue (13:0) in DWORD1 */
+			SET_BITS(word, green_fract, 16, 14);
+			SET_BITS(word, blue_fract, 0, 14);
+			val = _MMIO(cgm_degamma_reg);
+			I915_WRITE(val, word);
+			cgm_degamma_reg += 4;
+
+			/* Red (13:0) to be written to DWORD2 */
+			word = red_fract;
+			val = _MMIO(cgm_degamma_reg);
+			I915_WRITE(val, word);
+			cgm_degamma_reg += 4;
+			count++;
+		}
+
+		DRM_DEBUG_DRIVER("DeGamma LUT loaded for Pipe %c\n",
+				pipe_name(pipe));
+
+		/* Enable DeGamma on Pipe */
+		val = _MMIO(_PIPE_CGM_CONTROL(pipe));
+		I915_WRITE(val, I915_READ(val) | CGM_DEGAMMA_EN);
+		DRM_DEBUG_DRIVER("DeGamma correction enabled on Pipe %c\n",
+				pipe_name(pipe));
+		break;
+
+	default:
+		DRM_ERROR("Invalid number of samples for DeGamma LUT\n");
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int chv_set_gamma(struct drm_device *dev, struct drm_property_blob *blob,
+		struct drm_crtc *crtc)
+{
+	enum pipe pipe;
+	u16 red_fract, green_fract, blue_fract;
+	u32 red, green, blue, num_samples;
+	u32 word = 0;
+	u32 count, cgm_gamma_reg, cgm_control;
+	i915_reg_t val;
+	struct drm_r32g32b32 *correction_values;
+	struct drm_palette *gamma_data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc_state *state = crtc->state;
+
+	if (WARN_ON(!blob))
+		return -EINVAL;
+
+	gamma_data = (struct drm_palette *)blob->data;
+	pipe = to_intel_crtc(crtc)->pipe;
+	num_samples = blob->length / sizeof(struct drm_r32g32b32);
+
+	switch (num_samples) {
+	case GAMMA_DISABLE_VALS:
+
+		/* Disable Gamma functionality on Pipe - CGM Block */
+		val = _MMIO(_PIPE_CGM_CONTROL(pipe));
+		cgm_control = I915_READ(val);
+		cgm_control &= ~CGM_GAMMA_EN;
+		I915_WRITE(val, cgm_control);
+		state->palette_after_ctm_blob = NULL;
+		DRM_DEBUG_DRIVER("Gamma disabled on Pipe %c\n",
+			pipe_name(pipe));
+		return 0;
+
+	case CHV_8BIT_GAMMA_MAX_VALS:
+	case CHV_10BIT_GAMMA_MAX_VALS:
+
+		count = 0;
+		cgm_gamma_reg = _PIPE_GAMMA_BASE(pipe);
+		correction_values = gamma_data->lut;
+
+		while (count < num_samples) {
+			blue = correction_values[count].b32;
+			green = correction_values[count].g32;
+			red = correction_values[count].r32;
+
+			if (blue > CHV_MAX_GAMMA)
+				blue = CHV_MAX_GAMMA;
+
+			if (green > CHV_MAX_GAMMA)
+				green = CHV_MAX_GAMMA;
+
+			if (red > CHV_MAX_GAMMA)
+				red = CHV_MAX_GAMMA;
+
+			/* get MSB 10 bits from fraction part (14:23) */
+			blue_fract = GET_BITS(blue, 14, 10);
+			green_fract = GET_BITS(green, 14, 10);
+			red_fract = GET_BITS(red, 14, 10);
+
+			/* Green (25:16) and Blue (9:0) to be written */
+			SET_BITS(word, green_fract, 16, 10);
+			SET_BITS(word, blue_fract, 0, 10);
+			val = _MMIO(cgm_gamma_reg);
+			I915_WRITE(val, word);
+			cgm_gamma_reg += 4;
+
+			/* Red (9:0) to be written */
+			word = red_fract;
+			val = _MMIO(cgm_gamma_reg);
+			I915_WRITE(val, word);
+			cgm_gamma_reg += 4;
+			count++;
+		}
+
+		/* Enable (CGM) Gamma on Pipe */
+		val = _MMIO(_PIPE_CGM_CONTROL(pipe));
+		I915_WRITE(val, I915_READ(val) | CGM_GAMMA_EN);
+		DRM_DEBUG_DRIVER("CGM Gamma enabled on Pipe %c\n",
+			pipe_name(pipe));
+		return 0;
+
+	default:
+		DRM_ERROR("Invalid number of samples (%u) for Gamma LUT\n",
+				num_samples);
+		return -EINVAL;
+	}
+}
+
+void intel_color_manager_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;
+
+	/*
+	* CRTC level color correction, once applied on the
+	* pipe, goes on forever, until disabled, so there is no
+	* need to program all those correction registers on every
+	* commit. Do this only when a new correction applied.
+	*/
+	if (!crtc_state->color_correction_changed)
+		return;
+
+	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");
+	}
+
+	crtc_state->color_correction_changed = false;
+}
+
+void intel_crtc_attach_color_properties(struct drm_crtc *crtc)
+{
+}
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..04185ac
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_color_manager.h
@@ -0,0 +1,91 @@
+/*
+ * 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"
+
+/* Color management bit utilities */
+#define GET_BIT_MASK(n) ((1 << n) - 1)
+
+/* Read bits of a word from bit no. 'start'(lsb) till 'n' bits */
+#define GET_BITS(x, start, nbits) ((x >> start) & GET_BIT_MASK(nbits))
+
+/* Round off by adding 1 to the immediate lower bit */
+#define GET_BITS_ROUNDOFF(x, start, nbits) \
+	((GET_BITS(x, start, (nbits + 1)) + 1) >> 1)
+
+/* Clear bits of a word from bit no. 'start' till nbits */
+#define CLEAR_BITS(x, start, nbits) ( \
+		x &= ~((GET_BIT_MASK(nbits) << start)))
+
+/* Write bit_pattern of no_bits bits in a target word */
+#define SET_BITS(target, bit_pattern, start_bit, no_bits) \
+		do { \
+			CLEAR_BITS(target, start_bit, no_bits); \
+			target |= (bit_pattern << start_bit);  \
+		} while (0)
+
+/* CHV */
+#define CHV_10BIT_GAMMA_MAX_VALS		257
+#define CHV_DEGAMMA_MAX_VALS                   65
+
+/* No of coeff for disabling gamma is 0 */
+#define GAMMA_DISABLE_VALS			0
+
+/* Gamma on CHV */
+#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
+#define CHV_MAX_GAMMA                          ((1 << 24) - 1)
+
+/*
+ * CSC on CHV
+ * 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                12
+#define CSC_COEFF_SIGN                         (1 << 15)
+#define CHV_CSC_COEFF_FRACT_SHIFT              20
+#define CSC_MAX_VALS                           9
+
+/* Degamma on CHV */
+#define CHV_DEGAMMA_MSB_SHIFT                  2
+#define CHV_DEGAMMA_GREEN_SHIFT                16
+
+/* CHV CGM Block */
+#define CGM_GAMMA_EN                           (1 << 2)
+#define CGM_CSC_EN                             (1 << 1)
+#define CGM_DEGAMMA_EN                         (1 << 0)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a05ba28..b9eb507 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13940,6 +13940,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_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 d523ebb..2ee655a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1604,4 +1604,8 @@ 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 */
+void intel_crtc_attach_color_properties(struct drm_crtc *crtc);
+void intel_color_manager_commit(struct drm_device *dev,
+				struct drm_crtc_state *crtc_state);
 #endif /* __INTEL_DRV_H__ */
-- 
2.6.3

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

Re: [PATCH 4/6] drm/i915/chv: Implement color management

[Daniel Stone]

Hi,

On 17 December 2015 at 18:57, Lionel Landwerlin
<lionel.g.landwerlin@intel.com> wrote:
> @@ -311,7 +312,9 @@ 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,
> -       .is_cherryview = 1,
> +       .is_valleyview = 1,

... ?!

> +       .num_samples_after_ctm = CHV_10BIT_GAMMA_MAX_VALS,
> +       .num_samples_before_ctm = CHV_DEGAMMA_MAX_VALS,

Switch the order of these please.

> +static u16 chv_prepare_csc_coeff(s64 csc_coeff)
> +{
> +       u16 csc_s3_12_format = 0;
> +       u16 csc_int_value;
> +       u16 csc_fract_value;
> +
> +       if (csc_coeff < 0)
> +               csc_s3_12_format = CSC_COEFF_SIGN;
> +       csc_coeff = abs(csc_coeff);
> +       csc_coeff += CHV_CSC_FRACT_ROUNDOFF;
> +       if (csc_coeff > CHV_CSC_COEFF_MAX + 1)
> +               csc_coeff = CHV_CSC_COEFF_MAX + 1;
> +       else if (csc_coeff > CHV_CSC_COEFF_MAX)
> +               csc_coeff = CHV_CSC_COEFF_MAX;
> +
> +       csc_int_value = csc_coeff >> CHV_CSC_COEFF_SHIFT;
> +       csc_int_value <<= CHV_CSC_COEFF_INT_SHIFT;
> +
> +       csc_fract_value = csc_coeff >> CHV_CSC_COEFF_FRACT_SHIFT;
> +
> +       csc_s3_12_format |= csc_int_value | csc_fract_value;

Um. I'm at a loss trying to disentangle these. Here, (1 << 15) /
0x8000 is used as the sign bit; fair enough.

But, anything above 0x7ffffffff generates 0x8000 as a result, which
s3.12 would lead me to read as -7.999...; 0x7ffffffff itself generates
+7.999.... So huge positive numbers wrap around into your minimum
signed value. Is this deliberate? Is it covered by IGT?

> +static int chv_set_csc(struct drm_device *dev, struct drm_property_blob *blob,
> +               struct drm_crtc *crtc)
> +{
> +       u16 temp;
> +       u32 word = 0;
> +       int count = 0;
> +       i915_reg_t val;
> +       struct drm_ctm *csc_data;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       enum pipe pipe;
> +
> +       if (WARN_ON(!blob))
> +               return -EINVAL;

Why not let blob == NULL just disable CSC here? (How do you even disable it?)

> +       if (blob->length != sizeof(struct drm_ctm)) {
> +               DRM_ERROR("Invalid length of data received\n");
> +               return -EINVAL;
> +       }

This needs to be checked earlier (in the atomic_set_property hook):
failing commit is not OK.

> +static int chv_set_degamma(struct drm_device *dev,
> +       struct drm_property_blob *blob, struct drm_crtc *crtc)

What I said about the documentation around post-CTM vs. degamma makes
even more sense now. ;)

> +{
> +       u16 red_fract, green_fract, blue_fract;
> +       u32 red, green, blue;
> +       u32 num_samples;
> +       u32 word = 0;
> +       u32 count, cgm_control, cgm_degamma_reg;
> +       i915_reg_t val;
> +       enum pipe pipe;
> +       struct drm_palette *degamma_data;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct drm_r32g32b32 *correction_values = NULL;
> +       struct drm_crtc_state *state = crtc->state;
> +
> +       if (WARN_ON(!blob))
> +               return -EINVAL;

Same comment as above.

> +       degamma_data = (struct drm_palette *)blob->data;
> +       pipe = to_intel_crtc(crtc)->pipe;
> +       num_samples = blob->length / sizeof(struct drm_r32g32b32);
> +
> +       switch (num_samples) {
> +       case GAMMA_DISABLE_VALS:
> +               /* Disable DeGamma functionality on Pipe - CGM Block */

Just use blob == NULL for this.

> +               val = _MMIO(_PIPE_CGM_CONTROL(pipe));
> +               cgm_control = I915_READ(val);
> +               cgm_control &= ~CGM_DEGAMMA_EN;
> +               state->palette_before_ctm_blob = NULL;

This is really broken. In order to disable, you create a blob with len
== 0, which instantly drops itself without unreferencing, i.e. no
leak.

Also, I thought RMW of registers was frowned upon? Maybe not.

> +       default:
> +               DRM_ERROR("Invalid number of samples for DeGamma LUT\n");
> +               return -EINVAL;
> +       }

This could just become another early-exit path. But again, this needs
to be checked in, well, atomic_check, not commit.

> +static int chv_set_gamma(struct drm_device *dev, struct drm_property_blob *blob,
> +               struct drm_crtc *crtc)
> +{
> +       enum pipe pipe;
> +       u16 red_fract, green_fract, blue_fract;
> +       u32 red, green, blue, num_samples;
> +       u32 word = 0;
> +       u32 count, cgm_gamma_reg, cgm_control;
> +       i915_reg_t val;
> +       struct drm_r32g32b32 *correction_values;
> +       struct drm_palette *gamma_data;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct drm_crtc_state *state = crtc->state;
> +
> +       if (WARN_ON(!blob))
> +               return -EINVAL;

As above.

> +       switch (num_samples) {
> +       case GAMMA_DISABLE_VALS:

Ditto.

> +               /* Disable Gamma functionality on Pipe - CGM Block */
> +               val = _MMIO(_PIPE_CGM_CONTROL(pipe));
> +               cgm_control = I915_READ(val);
> +               cgm_control &= ~CGM_GAMMA_EN;
> +               I915_WRITE(val, cgm_control);
> +               state->palette_after_ctm_blob = NULL;

AOL.

> +       case CHV_8BIT_GAMMA_MAX_VALS:
> +       case CHV_10BIT_GAMMA_MAX_VALS:

Hm. At no point does the following code account for the difference
between these two. What happens if you set once with
10BIT_GAMMA_MAX_VALS followed by once with 8BIT_MAX_VALS? Won't that
break expectation of the second user, because you never wrote to the
257th entry?

Also, this just reads 10 bits out of the values regardless. Has 8-bit
been tested on CHV? It seems like it'd be totally broken, as this just
assumes a 10-bit range regardless.

> +       default:
> +               DRM_ERROR("Invalid number of samples (%u) for Gamma LUT\n",
> +                               num_samples);
> +               return -EINVAL;
> +       }

See previous.

> +void intel_color_manager_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;

Why is this initialised, especially when not actually used as a return value?

> +       blob = crtc_state->palette_after_ctm_blob;
> +       if (blob) {

Drop the conditional and pass it through regardless, so you can use
NULL to disable.

> +               /* 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");
> +       }

So we just error but carry on? Again, this (and all the following)
need to be in check, not commit. And you can just drop the 'success'
message.

> +void intel_crtc_attach_color_properties(struct drm_crtc *crtc)
> +{
> +}

... ?

> +/* Color management bit utilities */
> +#define GET_BIT_MASK(n) ((1 << n) - 1)
> +
> +/* Read bits of a word from bit no. 'start'(lsb) till 'n' bits */
> +#define GET_BITS(x, start, nbits) ((x >> start) & GET_BIT_MASK(nbits))
>
> +/* Round off by adding 1 to the immediate lower bit */
> +#define GET_BITS_ROUNDOFF(x, start, nbits) \
> +       ((GET_BITS(x, start, (nbits + 1)) + 1) >> 1)
> +
> +/* Clear bits of a word from bit no. 'start' till nbits */
> +#define CLEAR_BITS(x, start, nbits) ( \
> +               x &= ~((GET_BIT_MASK(nbits) << start)))
> +
> +/* Write bit_pattern of no_bits bits in a target word */
> +#define SET_BITS(target, bit_pattern, start_bit, no_bits) \
> +               do { \
> +                       CLEAR_BITS(target, start_bit, no_bits); \
> +                       target |= (bit_pattern << start_bit);  \
> +               } while (0)

Is there actually no common set of macros for these? I would also
expect to see this as lowest_bit -> highest_bit, rather than start_bit
-> (start_bit -> start_bit + nbits/no_bits).

> +/* CHV */
> +#define CHV_10BIT_GAMMA_MAX_VALS               257

Delete this, as it's already declared in the 'Gamma on CHV' block.

> +#define CHV_DEGAMMA_MAX_VALS                   65

Move this to that block.

> +/* No of coeff for disabling gamma is 0 */
> +#define GAMMA_DISABLE_VALS                     0

... and delete this.

> +/* Gamma on CHV */
> +#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
> +#define CHV_MAX_GAMMA                          ((1 << 24) - 1)

Shouldn't drm_palette or similar declare that the range of gamma
values is [0,(1 << 24) -1], or just do some normalisation, rather than
relying on people to know that CHV has a different gamma range than
the type would lead you to believe?

> +/*
> + * 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
> + */

As with (de)gamma, might help to note that CSC is CTM.

Also, this means that the comment I noted in the drm_ctm structure is
wrong: the range really is [0..8.0) rather than [0..1.0].
> +#define CSC_MAX_VALS                           9

Isn't MAX_VALS fixed at 9 everywhere ... ?

> +/* Degamma on CHV */
> +#define CHV_DEGAMMA_MSB_SHIFT                  2
> +#define CHV_DEGAMMA_GREEN_SHIFT                16

Slightly baffling, and also unused ... but what does it mean?

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

Re: [PATCH 4/6] drm/i915/chv: Implement color management

[Daniel Vetter]

On Thu, Dec 17, 2015 at 06:57:56PM +0000, Lionel Landwerlin wrote:
> From: Shashank Sharma <shashank.sharma@intel.com>
> 
> Implement degamma, csc and gamma steps on CHV.
> 
> 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/i915_drv.c            |   5 +-
>  drivers/gpu/drm/i915/i915_drv.h            |   2 +
>  drivers/gpu/drm/i915/i915_reg.h            |  26 +++
>  drivers/gpu/drm/i915/intel_color_manager.c | 353 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_color_manager.h |  91 ++++++++
>  drivers/gpu/drm/i915/intel_display.c       |   2 +
>  drivers/gpu/drm/i915/intel_drv.h           |   4 +
>  8 files changed, 484 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
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 0851de07..c66d56a 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/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 3ac616d..4396300 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>
> @@ -311,7 +312,9 @@ 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,
> -	.is_cherryview = 1,
> +	.is_valleyview = 1,
> +	.num_samples_after_ctm = CHV_10BIT_GAMMA_MAX_VALS,
> +	.num_samples_before_ctm = CHV_DEGAMMA_MAX_VALS,
>  	.display_mmio_offset = VLV_DISPLAY_BASE,
>  	GEN_CHV_PIPEOFFSETS,
>  	CURSOR_OFFSETS,
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1d28d90..4eb0fab 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -793,6 +793,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/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 007ae83..36bb320 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -8154,4 +8154,30 @@ enum skl_disp_power_wells {
>  #define GEN9_VEBOX_MOCS(i)	_MMIO(0xcb00 + (i) * 4)	/* Video MOCS registers */
>  #define GEN9_BLT_MOCS(i)	_MMIO(0xcc00 + (i) * 4)	/* Blitter MOCS registers */
>  
> +/* 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))
> +
> +#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))
> +
> +#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
> new file mode 100644
> index 0000000..02eee98
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -0,0 +1,353 @@
> +/*
> + * 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"
> +
> +static u16 chv_prepare_csc_coeff(s64 csc_coeff)
> +{
> +	u16 csc_s3_12_format = 0;
> +	u16 csc_int_value;
> +	u16 csc_fract_value;
> +
> +	if (csc_coeff < 0)
> +		csc_s3_12_format = CSC_COEFF_SIGN;
> +	csc_coeff = abs(csc_coeff);
> +	csc_coeff += CHV_CSC_FRACT_ROUNDOFF;
> +	if (csc_coeff > CHV_CSC_COEFF_MAX + 1)
> +		csc_coeff = CHV_CSC_COEFF_MAX + 1;
> +	else if (csc_coeff > CHV_CSC_COEFF_MAX)
> +		csc_coeff = CHV_CSC_COEFF_MAX;
> +
> +	csc_int_value = csc_coeff >> CHV_CSC_COEFF_SHIFT;
> +	csc_int_value <<= CHV_CSC_COEFF_INT_SHIFT;
> +
> +	csc_fract_value = csc_coeff >> 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)
> +{
> +	u16 temp;
> +	u32 word = 0;
> +	int count = 0;
> +	i915_reg_t val;
> +	struct drm_ctm *csc_data;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	enum pipe pipe;
> +
> +	if (WARN_ON(!blob))
> +		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;
> +	pipe = to_intel_crtc(crtc)->pipe;
> +
> +	/* Disable CSC functionality */
> +	val = _MMIO(_PIPE_CGM_CONTROL(pipe));
> +	I915_WRITE(val, I915_READ(val) & (~CGM_CSC_EN));
> +
> +	DRM_DEBUG_DRIVER("Disabled CSC Functionality on Pipe %c\n",
> +			pipe_name(pipe));
> +
> +	val = _MMIO(_PIPE_CSC_BASE(pipe));
> +
> +	/*
> +	* First 8 of 9 CSC correction values go in pair, to first
> +	* 4 CSC register (bit 0:15 and 16:31)
> +	*/
> +	while (count < CSC_MAX_VALS - 1) {
> +		temp = chv_prepare_csc_coeff(
> +					csc_data->ctm_coeff[count]);
> +		SET_BITS(word, temp, 0, 16);
> +		count++;
> +
> +		temp = chv_prepare_csc_coeff(
> +				csc_data->ctm_coeff[count]);
> +		SET_BITS(word, temp, 16, 16);
> +		count++;
> +
> +		I915_WRITE(val, word);
> +		val.reg += 4;
> +	}
> +
> +	/* 9th coeff goes to 5th register, bit 0:16 */
> +	temp = chv_prepare_csc_coeff(
> +			csc_data->ctm_coeff[count]);
> +	SET_BITS(word, temp, 0, 16);
> +	I915_WRITE(val, word);
> +
> +	/* Enable CSC functionality */
> +	val = _MMIO(_PIPE_CGM_CONTROL(pipe));
> +	I915_WRITE(val, I915_READ(val) | 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)
> +{
> +	u16 red_fract, green_fract, blue_fract;
> +	u32 red, green, blue;
> +	u32 num_samples;
> +	u32 word = 0;
> +	u32 count, cgm_control, cgm_degamma_reg;
> +	i915_reg_t val;
> +	enum pipe pipe;
> +	struct drm_palette *degamma_data;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_r32g32b32 *correction_values = NULL;
> +	struct drm_crtc_state *state = crtc->state;
> +
> +	if (WARN_ON(!blob))
> +		return -EINVAL;
> +
> +	degamma_data = (struct drm_palette *)blob->data;
> +	pipe = to_intel_crtc(crtc)->pipe;
> +	num_samples = blob->length / sizeof(struct drm_r32g32b32);
> +
> +	switch (num_samples) {
> +	case GAMMA_DISABLE_VALS:

If we implement my suggestion for a num_samples helper, we could put the
NULL->0 samples logic into that one and nicely unify things. And probably
we should also reject a gamma table with size 0 as nonsens then.

> +		/* Disable DeGamma functionality on Pipe - CGM Block */
> +		val = _MMIO(_PIPE_CGM_CONTROL(pipe));
> +		cgm_control = I915_READ(val);
> +		cgm_control &= ~CGM_DEGAMMA_EN;
> +		state->palette_before_ctm_blob = NULL;
> +
> +		I915_WRITE(val, cgm_control);
> +		DRM_DEBUG_DRIVER("DeGamma disabled on Pipe %c\n",
> +				pipe_name(pipe));
> +		break;
> +
> +	case CHV_DEGAMMA_MAX_VALS:
> +		cgm_degamma_reg = _PIPE_DEGAMMA_BASE(pipe);
> +		count = 0;
> +		correction_values = degamma_data->lut;
> +		while (count < CHV_DEGAMMA_MAX_VALS) {
> +			blue = correction_values[count].b32;
> +			green = correction_values[count].g32;
> +			red = correction_values[count].r32;
> +
> +			if (blue > CHV_MAX_GAMMA)
> +				blue = CHV_MAX_GAMMA;
> +
> +			if (green > CHV_MAX_GAMMA)
> +				green = CHV_MAX_GAMMA;
> +
> +			if (red > CHV_MAX_GAMMA)
> +				red = CHV_MAX_GAMMA;
> +
> +			blue_fract = GET_BITS(blue, 10, 14);
> +			green_fract = GET_BITS(green, 10, 14);
> +			red_fract = GET_BITS(red, 10, 14);
> +
> +			/* Green (29:16) and Blue (13:0) in DWORD1 */
> +			SET_BITS(word, green_fract, 16, 14);
> +			SET_BITS(word, blue_fract, 0, 14);
> +			val = _MMIO(cgm_degamma_reg);
> +			I915_WRITE(val, word);
> +			cgm_degamma_reg += 4;
> +
> +			/* Red (13:0) to be written to DWORD2 */
> +			word = red_fract;
> +			val = _MMIO(cgm_degamma_reg);
> +			I915_WRITE(val, word);
> +			cgm_degamma_reg += 4;
> +			count++;
> +		}
> +
> +		DRM_DEBUG_DRIVER("DeGamma LUT loaded for Pipe %c\n",
> +				pipe_name(pipe));
> +
> +		/* Enable DeGamma on Pipe */
> +		val = _MMIO(_PIPE_CGM_CONTROL(pipe));
> +		I915_WRITE(val, I915_READ(val) | CGM_DEGAMMA_EN);
> +		DRM_DEBUG_DRIVER("DeGamma correction enabled on Pipe %c\n",
> +				pipe_name(pipe));
> +		break;
> +
> +	default:
> +		DRM_ERROR("Invalid number of samples for DeGamma LUT\n");
> +		return -EINVAL;

This is a relict from pre-atomic and won't work anymore: There's no way to
pass error code from _commit functions up to userspace, and there's also
no way to abort an atomic commit at that stage. Input validation must
happen in atomic_check callbacks (or compute_config, as we tend to call
them in i915 itself).
-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

[PATCH 5/6] drm/i915/bdw+: Implement color management

[Lionel Landwerlin]

From: Shashank Sharma <shashank.sharma@intel.com>

Implement degamma, csc and gamma steps on BDW+.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
---
 drivers/gpu/drm/i915/i915_drv.c            |  14 +
 drivers/gpu/drm/i915/i915_reg.h            |  43 ++-
 drivers/gpu/drm/i915/intel_color_manager.c | 451 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_color_manager.h |  20 ++
 4 files changed, 525 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 4396300..395e5ad 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -289,22 +289,30 @@ static const struct intel_device_info intel_haswell_m_info = {
 static const struct intel_device_info intel_broadwell_d_info = {
 	HSW_FEATURES,
 	.gen = 8,
+	.num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS,
+	.num_samples_before_ctm = BDW_DEGAMMA_MAX_VALS,
 };
 
 static const struct intel_device_info intel_broadwell_m_info = {
 	HSW_FEATURES,
 	.gen = 8, .is_mobile = 1,
+	.num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS,
+	.num_samples_before_ctm = BDW_DEGAMMA_MAX_VALS,
 };
 
 static const struct intel_device_info intel_broadwell_gt3d_info = {
 	HSW_FEATURES,
 	.gen = 8,
+	.num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS,
+	.num_samples_before_ctm = BDW_DEGAMMA_MAX_VALS,
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
 };
 
 static const struct intel_device_info intel_broadwell_gt3m_info = {
 	HSW_FEATURES,
 	.gen = 8, .is_mobile = 1,
+	.num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS,
+	.num_samples_before_ctm = BDW_DEGAMMA_MAX_VALS,
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
 };
 
@@ -324,12 +332,16 @@ static const struct intel_device_info intel_skylake_info = {
 	HSW_FEATURES,
 	.is_skylake = 1,
 	.gen = 9,
+	.num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS,
+	.num_samples_before_ctm = BDW_DEGAMMA_MAX_VALS,
 };
 
 static const struct intel_device_info intel_skylake_gt3_info = {
 	HSW_FEATURES,
 	.is_skylake = 1,
 	.gen = 9,
+	.num_samples_before_ctm = BDW_DEGAMMA_MAX_VALS,
+	.num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS,
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
 };
 
@@ -339,6 +351,8 @@ 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_samples_before_ctm = BDW_DEGAMMA_MAX_VALS,
 	.num_pipes = 3,
 	.has_ddi = 1,
 	.has_fpga_dbg = 1,
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 36bb320..c4da842 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5748,11 +5748,19 @@ enum skl_disp_power_wells {
 /* legacy palette */
 #define _LGC_PALETTE_A           0x4a000
 #define _LGC_PALETTE_B           0x4a800
-#define LGC_PALETTE(pipe, i) _MMIO(_PIPE(pipe, _LGC_PALETTE_A, _LGC_PALETTE_B) + (i) * 4)
+#define _LGC_PALETTE_C           0x4b000
+#define LGC_PALETTE(pipe, i) _MMIO(_PIPE3(pipe, \
+					  _LGC_PALETTE_A, \
+					  _LGC_PALETTE_B, \
+					  _LGC_PALETTE_C) + (i) * 4)
 
 #define _GAMMA_MODE_A		0x4a480
 #define _GAMMA_MODE_B		0x4ac80
-#define GAMMA_MODE(pipe) _MMIO_PIPE(pipe, _GAMMA_MODE_A, _GAMMA_MODE_B)
+#define _GAMMA_MODE_C		0x4b480
+#define GAMMA_MODE(pipe) _MMIO(_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)
@@ -8178,6 +8186,35 @@ 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 PAL_PREC_GC_MAX_A			0x4A410
+#define PAL_PREC_GC_MAX_B			0x4AC10
+#define PAL_PREC_GC_MAX_C			0x4B410
+#define PAL_PREC_EXT_GC_MAX_A			0x4A420
+#define PAL_PREC_EXT_GC_MAX_B			0x4AC20
+#define PAL_PREC_EXT_GC_MAX_C			0x4B420
+
+#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))
+#define _PREC_PAL_GC_MAX(pipe) \
+	(_PIPE3(pipe, PAL_PREC_GC_MAX_A, PAL_PREC_GC_MAX_B, PAL_PREC_GC_MAX_C))
+#define _PREC_PAL_EXT_GC_MAX(pipe) \
+	(_PIPE3(pipe, PAL_PREC_EXT_GC_MAX_A, PAL_PREC_EXT_GC_MAX_B, PAL_PREC_EXT_GC_MAX_C))
+
+
+/* BDW CSC correction */
+#define CSC_COEFF_A				0x49010
+#define CSC_COEFF_B				0x49110
+#define CSC_COEFF_C				0x49210
+#define _PIPE_CSC_COEFF(pipe) \
+	(_PIPE3(pipe, CSC_COEFF_A, CSC_COEFF_B, CSC_COEFF_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 02eee98..bca07c1 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.c
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -27,6 +27,451 @@
 
 #include "intel_color_manager.h"
 
+static void bdw_write_8bit_gamma_legacy(struct drm_device *dev,
+	struct drm_r32g32b32 *correction_values, i915_reg_t palette)
+{
+	u16 blue_fract, green_fract, red_fract;
+	u32 blue, green, red;
+	u32 count = 0;
+	u32 word = 0;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	while (count < BDW_8BIT_GAMMA_MAX_VALS) {
+		blue = correction_values[count].b32;
+		green = correction_values[count].g32;
+		red = correction_values[count].r32;
+
+		/*
+		* Maximum possible gamma correction value supported
+		* for BDW is 0xFFFFFFFF, so clamp the values accordingly
+		*/
+		if (blue >= BDW_MAX_GAMMA)
+			blue = BDW_MAX_GAMMA;
+		if (green >= BDW_MAX_GAMMA)
+			green = BDW_MAX_GAMMA;
+		if (red >= BDW_MAX_GAMMA)
+			red = BDW_MAX_GAMMA;
+
+		blue_fract = GET_BITS(blue, 16, 8);
+		green_fract = GET_BITS(green, 16, 8);
+		red_fract = GET_BITS(red, 16, 8);
+
+		/* Blue (7:0) Green (15:8) and Red (23:16) */
+		SET_BITS(word, blue_fract, 0, 8);
+		SET_BITS(word, green_fract, 8, 8);
+		SET_BITS(word, red_fract, 16, 8);
+		I915_WRITE(palette, word);
+		palette.reg += 4;
+		count++;
+	}
+}
+
+static void bdw_write_10bit_gamma_precision(struct drm_device *dev,
+	struct drm_r32g32b32 *correction_values, i915_reg_t pal_prec_data,
+			u32 no_of_coeff)
+{
+	u16 blue_fract, green_fract, red_fract;
+	u32 word = 0;
+	u32 count = 0;
+	u32 blue, green, red;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	while (count < no_of_coeff) {
+
+		blue = correction_values[count].b32;
+		green = correction_values[count].g32;
+		red = correction_values[count].r32;
+
+		/*
+		* Maximum possible gamma correction value supported
+		* for BDW is 0xFFFFFFFF, so clamp the values accordingly
+		*/
+		if (blue >= BDW_MAX_GAMMA)
+			blue = BDW_MAX_GAMMA;
+		if (green >= BDW_MAX_GAMMA)
+			green = BDW_MAX_GAMMA;
+		if (red >= BDW_MAX_GAMMA)
+			red = BDW_MAX_GAMMA;
+
+		/*
+		* Gamma correction values are sent in 8.24 format
+		* with 8 int and 24 fraction bits. BDW 10 bit gamma
+		* unit expects correction registers to be programmed in
+		* 0.10 format, with 0 int and 16 fraction bits. So take
+		* MSB 10 bit values(bits 23-14) from the fraction part and
+		* prepare the correction registers.
+		*/
+		blue_fract = GET_BITS(blue, 14, 10);
+		green_fract = GET_BITS(green, 14, 10);
+		red_fract = GET_BITS(red, 14, 10);
+
+		/* Arrange: Red (29:20) Green (19:10) and Blue (9:0) */
+		SET_BITS(word, red_fract, 20, 10);
+		SET_BITS(word, green_fract, 10, 10);
+		SET_BITS(word, blue_fract, 0, 10);
+		I915_WRITE(pal_prec_data, word);
+		count++;
+	}
+	DRM_DEBUG_DRIVER("Gamma correction programmed\n");
+}
+
+static void bdw_write_12bit_gamma_precision(struct drm_device *dev,
+	struct drm_r32g32b32 *correction_values, i915_reg_t pal_prec_data,
+		enum pipe pipe)
+{
+	uint16_t blue_fract, green_fract, red_fract;
+	uint32_t gcmax;
+	uint32_t word = 0;
+	uint32_t count = 0;
+	i915_reg_t gcmax_reg;
+	u32 blue, green, red;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	/* Program first 512 values in precision palette */
+	while (count < BDW_12BIT_GAMMA_MAX_VALS - 1) {
+
+		blue = correction_values[count].b32;
+		green = correction_values[count].g32;
+		red = correction_values[count].r32;
+
+		/*
+		* Maximum possible gamma correction value supported
+		* for BDW is 0xFFFFFFFF, so clamp the values accordingly
+		*/
+		if (blue >= BDW_MAX_GAMMA)
+			blue = BDW_MAX_GAMMA;
+		if (green >= BDW_MAX_GAMMA)
+			green = BDW_MAX_GAMMA;
+		if (red >= BDW_MAX_GAMMA)
+			red = BDW_MAX_GAMMA;
+
+		/*
+		* Framework's general gamma format is 8.24 (8 int 16 fraction)
+		* BDW Platform's supported gamma format is 16 bit correction
+		* values in 0.16 format. So extract higher 16 fraction bits
+		* from 8.24 gamma correction values.
+		*/
+		red_fract = GET_BITS(red, 8, 16);
+		green_fract = GET_BITS(green, 8, 16);
+		blue_fract = GET_BITS(blue, 8, 16);
+
+		/*
+		* From the bspec:
+		* For 12 bit gamma correction, program precision palette
+		* with 16 bits per color in a 0.16 format with 0 integer and
+		* 16 fractional bits (upper 10 bits in odd indexes, lower 6
+		* bits in even indexes)
+		*/
+
+		/* Even index: Lower 6 bits from correction should go as MSB */
+		SET_BITS(word, GET_BITS(red_fract, 0, 6), 24, 6);
+		SET_BITS(word, GET_BITS(green_fract, 0, 6), 14, 6);
+		SET_BITS(word, GET_BITS(blue_fract, 0, 6), 4, 6);
+		I915_WRITE(pal_prec_data, word);
+
+		word = 0x0;
+		/* Odd index: Upper 10 bits of correction should go as MSB */
+		SET_BITS(word, GET_BITS(red_fract, 6, 10), 20, 10);
+		SET_BITS(word, GET_BITS(green_fract, 6, 10), 10, 10);
+		SET_BITS(word, GET_BITS(blue_fract, 6, 10), 0, 10);
+
+		I915_WRITE(pal_prec_data, word);
+		count++;
+	}
+
+	/* Now program the 513th value in GCMAX regs */
+	word = 0;
+	gcmax_reg = _MMIO(_PREC_PAL_GC_MAX(pipe));
+	gcmax = min_t(u32, GET_BITS(correction_values[count].r32, 8, 17),
+				BDW_MAX_GAMMA);
+	SET_BITS(word, gcmax, 0, 17);
+	I915_WRITE(gcmax_reg, word);
+	gcmax_reg.reg += 4;
+
+	word = 0;
+	gcmax = min_t(u32, GET_BITS(correction_values[count].g32, 8, 17),
+				BDW_MAX_GAMMA);
+	SET_BITS(word, gcmax, 0, 17);
+	I915_WRITE(gcmax_reg, word);
+	gcmax_reg.reg += 4;
+
+	word = 0;
+	gcmax = min_t(u32, GET_BITS(correction_values[count].b32, 8, 17),
+				BDW_MAX_GAMMA);
+	SET_BITS(word, gcmax, 0, 17);
+	I915_WRITE(gcmax_reg, 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;
+	i915_reg_t pal_prec_data = LGC_PALETTE(pipe, 0);
+
+	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.reg += 4;
+		count++;
+	}
+}
+
+static int bdw_set_gamma(struct drm_device *dev, struct drm_property_blob *blob,
+			struct drm_crtc *crtc)
+{
+	enum pipe pipe;
+	int num_samples;
+	i915_reg_t pal_prec_index, pal_prec_data;
+	u32 mode, index, word = 0;
+	struct drm_palette *gamma_data;
+	struct drm_crtc_state *state = crtc->state;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_r32g32b32 *correction_values = NULL;
+
+	if (WARN_ON(!blob))
+		return -EINVAL;
+
+	gamma_data = (struct drm_palette *)blob->data;
+	pipe = to_intel_crtc(crtc)->pipe;
+	num_samples = blob->length / sizeof(struct drm_r32g32b32);
+
+	pal_prec_index = _MMIO(_PREC_PAL_INDEX(pipe));
+	pal_prec_data = _MMIO(_PREC_PAL_DATA(pipe));
+	correction_values = (struct drm_r32g32b32 *)&gamma_data->lut;
+
+	switch (num_samples) {
+	case GAMMA_DISABLE_VALS:
+		/* Disable Gamma functionality on Pipe */
+		DRM_DEBUG_DRIVER("Disabling gamma on Pipe %c\n",
+			pipe_name(pipe));
+		if ((mode & GAMMA_MODE_MODE_MASK) == GAMMA_MODE_MODE_12BIT)
+			bdw_reset_gamma(dev_priv, pipe);
+		state->palette_after_ctm_blob = NULL;
+		word = GAMMA_MODE_MODE_8BIT;
+		break;
+
+	case BDW_8BIT_GAMMA_MAX_VALS:
+		/* Legacy palette */
+		bdw_write_8bit_gamma_legacy(dev, correction_values,
+				LGC_PALETTE(pipe, 0));
+		word = GAMMA_MODE_MODE_8BIT;
+		break;
+
+	case BDW_SPLITGAMMA_MAX_VALS:
+		index = num_samples;
+		index |= BDW_INDEX_AUTO_INCREMENT | BDW_INDEX_SPLIT_MODE;
+		I915_WRITE(pal_prec_index, index);
+		bdw_write_10bit_gamma_precision(dev, correction_values,
+			pal_prec_data, BDW_SPLITGAMMA_MAX_VALS);
+		word = GAMMA_MODE_MODE_SPLIT;
+		break;
+
+	case BDW_12BIT_GAMMA_MAX_VALS:
+		index = BDW_INDEX_AUTO_INCREMENT;
+		I915_WRITE(pal_prec_index, index);
+		bdw_write_12bit_gamma_precision(dev, correction_values,
+			pal_prec_data, pipe);
+		word = GAMMA_MODE_MODE_12BIT;
+		break;
+
+	case BDW_10BIT_GAMMA_MAX_VALS:
+		index = BDW_INDEX_AUTO_INCREMENT;
+		I915_WRITE(pal_prec_index, index);
+		bdw_write_10bit_gamma_precision(dev, correction_values,
+			pal_prec_data, BDW_10BIT_GAMMA_MAX_VALS);
+		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 bdw_set_degamma(struct drm_device *dev,
+	struct drm_property_blob *blob, struct drm_crtc *crtc)
+{
+	enum pipe pipe;
+	int num_samples;
+	u32 index, mode;
+	i915_reg_t pal_prec_index, pal_prec_data;
+	struct drm_palette *degamma_data;
+	struct drm_crtc_state *state = crtc->state;
+	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;
+	pipe = to_intel_crtc(crtc)->pipe;
+	num_samples = blob->length / sizeof(struct drm_r32g32b32);
+
+	switch (num_samples) {
+	case GAMMA_DISABLE_VALS:
+		/* Disable degamma on Pipe */
+		mode = I915_READ(GAMMA_MODE(pipe)) & ~GAMMA_MODE_MODE_MASK;
+		I915_WRITE(GAMMA_MODE(pipe), mode | GAMMA_MODE_MODE_8BIT);
+
+		state->palette_before_ctm_blob = NULL;
+		DRM_DEBUG_DRIVER("Disabling degamma on Pipe %c\n",
+			pipe_name(pipe));
+		break;
+
+	case BDW_SPLITGAMMA_MAX_VALS:
+		pal_prec_index = _MMIO(_PREC_PAL_INDEX(pipe));
+		pal_prec_data = _MMIO(_PREC_PAL_DATA(pipe));
+		correction_values = degamma_data->lut;
+
+		index = BDW_INDEX_AUTO_INCREMENT | BDW_INDEX_SPLIT_MODE;
+		I915_WRITE(pal_prec_index, index);
+
+		bdw_write_10bit_gamma_precision(dev,
+						correction_values,
+						pal_prec_data,
+						BDW_SPLITGAMMA_MAX_VALS);
+
+		/* Enable degamma on Pipe */
+		mode = I915_READ(GAMMA_MODE(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));
+		break;
+
+	default:
+		DRM_ERROR("Invalid number of samples\n");
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static uint32_t bdw_prepare_csc_coeff(int64_t coeff)
+{
+	uint32_t reg_val, ls_bit_pos, exponent_bits, sign_bit = 0;
+	int32_t mantissa;
+	uint64_t abs_coeff;
+
+	coeff = min_t(int64_t, coeff, BDW_CSC_COEFF_MAX_VAL);
+	coeff = max_t(int64_t, coeff, BDW_CSC_COEFF_MIN_VAL);
+
+	abs_coeff = abs(coeff);
+	if (abs_coeff < (BDW_CSC_COEFF_UNITY_VAL >> 3)) {
+		/* abs_coeff < 0.125 */
+		exponent_bits = 3;
+		ls_bit_pos = 19;
+	} else if (abs_coeff >= (BDW_CSC_COEFF_UNITY_VAL >> 3) &&
+		abs_coeff < (BDW_CSC_COEFF_UNITY_VAL >> 2)) {
+		/* abs_coeff >= 0.125 && val < 0.25 */
+		exponent_bits = 2;
+		ls_bit_pos = 20;
+	} else if (abs_coeff >= (BDW_CSC_COEFF_UNITY_VAL >> 2)
+		&& abs_coeff < (BDW_CSC_COEFF_UNITY_VAL >> 1)) {
+		/* abs_coeff >= 0.25 && val < 0.5 */
+		exponent_bits = 1;
+		ls_bit_pos = 21;
+	} else if (abs_coeff >= (BDW_CSC_COEFF_UNITY_VAL >> 1)
+		&& abs_coeff < BDW_CSC_COEFF_UNITY_VAL) {
+		/* abs_coeff >= 0.5 && val < 1.0 */
+		exponent_bits = 0;
+		ls_bit_pos = 22;
+	} else if (abs_coeff >= BDW_CSC_COEFF_UNITY_VAL &&
+		abs_coeff < (BDW_CSC_COEFF_UNITY_VAL << 1)) {
+		/* abs_coeff >= 1.0 && val < 2.0 */
+		exponent_bits = 7;
+		ls_bit_pos = 23;
+	} else {
+		/* abs_coeff >= 2.0 && val < 4.0 */
+		exponent_bits = 6;
+		ls_bit_pos = 24;
+	}
+
+	mantissa = GET_BITS_ROUNDOFF(abs_coeff, ls_bit_pos, CSC_MAX_VALS);
+	if (coeff < 0)
+		sign_bit = 1;
+
+	reg_val = 0;
+	SET_BITS(reg_val, exponent_bits, 12, 3);
+	SET_BITS(reg_val, mantissa, 3, 9);
+	SET_BITS(reg_val, sign_bit, 15, 1);
+	return reg_val;
+}
+
+static int bdw_set_csc(struct drm_device *dev, struct drm_property_blob *blob,
+		struct drm_crtc *crtc)
+{
+	enum pipe pipe;
+	enum plane plane;
+	int temp, word;
+	int count = 0;
+	u32 plane_ctl, mode;
+	i915_reg_t reg;
+	struct drm_ctm *csc_data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (WARN_ON(!blob))
+		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;
+	pipe = to_intel_crtc(crtc)->pipe;
+	plane = to_intel_crtc(crtc)->plane;
+
+	plane_ctl = I915_READ(PLANE_CTL(pipe, plane));
+	plane_ctl |= PLANE_CTL_PIPE_CSC_ENABLE;
+	I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl);
+	reg = _MMIO(_PIPE_CSC_COEFF(pipe));
+
+	/*
+	* BDW CSC correction coefficients are written like this:
+	* first two values go in a pair, into first register(0:15 and 16:31)
+	* third one alone goes into second register (16:31). Same
+	* pattern repeats for 3 times = 3 * 3 = 9 values.
+	*/
+	while (count < CSC_MAX_VALS) {
+		word = 0;
+		temp = bdw_prepare_csc_coeff(csc_data->ctm_coeff[count++]);
+		SET_BITS(word, temp, 16, 16);
+
+		temp = bdw_prepare_csc_coeff(csc_data->ctm_coeff[count++]);
+		SET_BITS(word, temp, 0, 16);
+
+		I915_WRITE(reg, word);
+		reg.reg += 4;
+
+		word = 0;
+		temp = bdw_prepare_csc_coeff(csc_data->ctm_coeff[count++]);
+		SET_BITS(word, temp, 16, 16);
+		I915_WRITE(reg, word);
+		reg.reg += 4;
+	}
+
+	/* Enable CSC functionality */
+	mode = I915_READ(PIPE_CSC_MODE(pipe));
+	mode |= CSC_POSITION_BEFORE_GAMMA;
+	I915_WRITE(PIPE_CSC_MODE(pipe), mode);
+	DRM_DEBUG_DRIVER("CSC enabled on Pipe %c\n", pipe_name(pipe));
+	return 0;
+}
+
 static u16 chv_prepare_csc_coeff(s64 csc_coeff)
 {
 	u16 csc_s3_12_format = 0;
@@ -314,6 +759,8 @@ void intel_color_manager_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");
@@ -326,6 +773,8 @@ void intel_color_manager_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");
@@ -338,6 +787,8 @@ void intel_color_manager_commit(struct drm_device *dev,
 		/* CSC correction */
 		if (IS_CHERRYVIEW(dev))
 			ret = chv_set_csc(dev, blob, crtc);
+		else if (IS_BROADWELL(dev) || IS_GEN9(dev))
+			ret = bdw_set_csc(dev, blob, crtc);
 
 		if (ret)
 			DRM_ERROR("set CSC correction failed\n");
diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
index 04185ac..3ceec3d 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.h
+++ b/drivers/gpu/drm/i915/intel_color_manager.h
@@ -89,3 +89,23 @@
 #define CGM_GAMMA_EN                           (1 << 2)
 #define CGM_CSC_EN                             (1 << 1)
 #define CGM_DEGAMMA_EN                         (1 << 0)
+
+/* BDW CSC */
+/* 1.0000000 in S31.32 format */
+#define BDW_CSC_COEFF_UNITY_VAL	0x100000000
+/* 3.9921875 in S31.32 format */
+#define BDW_CSC_COEFF_MAX_VAL	0x3FE000000
+/*-4.000000 in S31.32 format */
+#define BDW_CSC_COEFF_MIN_VAL	0xFFFFFFFC00000000
+
+/* Gamma on BDW */
+#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
+#define BDW_MAX_GAMMA                         ((1 << 24) - 1)
+#define BDW_INDEX_AUTO_INCREMENT               (1 << 15)
+#define BDW_INDEX_SPLIT_MODE                   (1 << 31)
+
+/* Degamma on BDW */
+#define BDW_DEGAMMA_MAX_VALS                   512
-- 
2.6.3

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

Re: [PATCH 5/6] drm/i915/bdw+: Implement color management

[Daniel Stone]

Hi,

On 17 December 2015 at 18:57, Lionel Landwerlin
<lionel.g.landwerlin@intel.com> wrote:
> @@ -289,22 +289,30 @@ static const struct intel_device_info intel_haswell_m_info = {
>  static const struct intel_device_info intel_broadwell_d_info = {
>         HSW_FEATURES,
>         .gen = 8,
> +       .num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS,
> +       .num_samples_before_ctm = BDW_DEGAMMA_MAX_VALS,
>  };

Nitpick I know, but please invert before and after for the sake of my sanity.

> +static void bdw_write_8bit_gamma_legacy(struct drm_device *dev,
> +       struct drm_r32g32b32 *correction_values, i915_reg_t palette)
> +{
> +       u16 blue_fract, green_fract, red_fract;
> +       u32 blue, green, red;
> +       u32 count = 0;
> +       u32 word = 0;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +       while (count < BDW_8BIT_GAMMA_MAX_VALS) {
> +               blue = correction_values[count].b32;
> +               green = correction_values[count].g32;
> +               red = correction_values[count].r32;
> +
> +               /*
> +               * Maximum possible gamma correction value supported
> +               * for BDW is 0xFFFFFFFF, so clamp the values accordingly
> +               */

Eh? 0xFFFFFFFF != (1 << 24) - 1.

> +               if (blue >= BDW_MAX_GAMMA)
> +                       blue = BDW_MAX_GAMMA;
> +               if (green >= BDW_MAX_GAMMA)
> +                       green = BDW_MAX_GAMMA;
> +               if (red >= BDW_MAX_GAMMA)
> +                       red = BDW_MAX_GAMMA;

> rather than >=.

> +               /*
> +               * Gamma correction values are sent in 8.24 format
> +               * with 8 int and 24 fraction bits. BDW 10 bit gamma
> +               * unit expects correction registers to be programmed in
> +               * 0.10 format, with 0 int and 16 fraction bits. So take

'0.10 format, with 0 int and 16 fraction bits' ...

> +               * MSB 10 bit values(bits 23-14) from the fraction part and
> +               * prepare the correction registers.
> +               */

The comment here is helpful, but colour me confused: we just discard
the 8 integer bits anyway? Why do they exist? Every time I think I've
worked out which of [0,1.0] and [0,8.0) is the correct range, I
realise I'm wrong.

> +static void bdw_write_12bit_gamma_precision(struct drm_device *dev,
> +       struct drm_r32g32b32 *correction_values, i915_reg_t pal_prec_data,
> +               enum pipe pipe)

Why does this take a pipe where all the others don't, and also not
take a num_coeff, also unlike the others?

> +       /* Program first 512 values in precision palette */
> +       while (count < BDW_12BIT_GAMMA_MAX_VALS - 1) {

Um, this is a for loop.

> +               /*
> +               * Maximum possible gamma correction value supported
> +               * for BDW is 0xFFFFFFFF, so clamp the values accordingly
> +               */

Again, not 0xffffffff.

> +               /*
> +               * Framework's general gamma format is 8.24 (8 int 16 fraction)

8 int 16?!

> +               * BDW Platform's supported gamma format is 16 bit correction
> +               * values in 0.16 format. So extract higher 16 fraction bits
> +               * from 8.24 gamma correction values.

Isn't this the 12-bit mode? Colour me stupid, but what makes this
12-bit rather than 16-bit? Anyway.

> +               */
> +               red_fract = GET_BITS(red, 8, 16);
> +               green_fract = GET_BITS(green, 8, 16);
> +               blue_fract = GET_BITS(blue, 8, 16);

Okay, at least the range is consistent here - still [0,1.0], with the
integer component totally ignored.

> +       gamma_data = (struct drm_palette *)blob->data;
> +       pipe = to_intel_crtc(crtc)->pipe;
> +       num_samples = blob->length / sizeof(struct drm_r32g32b32);
> +
> +       pal_prec_index = _MMIO(_PREC_PAL_INDEX(pipe));
> +       pal_prec_data = _MMIO(_PREC_PAL_DATA(pipe));
> +       correction_values = (struct drm_r32g32b32 *)&gamma_data->lut;
> +
> +       switch (num_samples) {
> +       case GAMMA_DISABLE_VALS:
> +               /* Disable Gamma functionality on Pipe */
> +               DRM_DEBUG_DRIVER("Disabling gamma on Pipe %c\n",
> +                       pipe_name(pipe));
> +               if ((mode & GAMMA_MODE_MODE_MASK) == GAMMA_MODE_MODE_12BIT)
> +                       bdw_reset_gamma(dev_priv, pipe);
> +               state->palette_after_ctm_blob = NULL;
> +               word = GAMMA_MODE_MODE_8BIT;
> +               break;

Right, so we program the gamma unit to 8-bit mode (sensible!), and
write a linear palette (sensible!) ... but only if we were previously
in 12-bit gamma mode. What? So, if I start with 10-bit gamma and then
disable the gamma unit, my gamma won't be disabled, but will be an
8-bit interpretation of the previous 10-bit gamma ramp? Ouch. Also
broken when going from 8-bit -> disabled. Surely just make that check
unconditional.

(Again, this should also be blob == NULL, not a blob with length 0
which is subsequently leaked. Wait, how do you even create a length-0
blob ... ?!)

> +       case BDW_8BIT_GAMMA_MAX_VALS:
> +               /* Legacy palette */
> +               bdw_write_8bit_gamma_legacy(dev, correction_values,
> +                               LGC_PALETTE(pipe, 0));
> +               word = GAMMA_MODE_MODE_8BIT;
> +               break;
> +
> +       case BDW_SPLITGAMMA_MAX_VALS:
> +               index = num_samples;
> +               index |= BDW_INDEX_AUTO_INCREMENT | BDW_INDEX_SPLIT_MODE;
> +               I915_WRITE(pal_prec_index, index);
> +               bdw_write_10bit_gamma_precision(dev, correction_values,
> +                       pal_prec_data, BDW_SPLITGAMMA_MAX_VALS);
> +               word = GAMMA_MODE_MODE_SPLIT;
> +               break;
> +
> +       case BDW_12BIT_GAMMA_MAX_VALS:
> +               index = BDW_INDEX_AUTO_INCREMENT;
> +               I915_WRITE(pal_prec_index, index);
> +               bdw_write_12bit_gamma_precision(dev, correction_values,
> +                       pal_prec_data, pipe);
> +               word = GAMMA_MODE_MODE_12BIT;
> +               break;
> +
> +       case BDW_10BIT_GAMMA_MAX_VALS:
> +               index = BDW_INDEX_AUTO_INCREMENT;
> +               I915_WRITE(pal_prec_index, index);
> +               bdw_write_10bit_gamma_precision(dev, correction_values,
> +                       pal_prec_data, BDW_10BIT_GAMMA_MAX_VALS);
> +               word = GAMMA_MODE_MODE_10BIT;
> +               break;

This is pretty upsetting; we appear to have:
  - 8-bit gamma: length 256 on BDW and CHV (consistency! but CHV seems
to just use 10-bit anyway)
  - 10-bit gamma: length 257 on CHV, 1024 on BDW (or 512 for split)
  - 12-bit gamma: length 513 on BDW

How is generic userspace supposed to determine this? We will never
have these kinds of per-device lookup tables. Using semi-arbitrary
length values to determine this is absolutely the wrong approach. How
about exposing it through properties? Maybe declare
PALETTE_AFTER_CTM_8BIT (hardcoded to 256), PALETTE_AFTER_CTM_10BIT,
etc, with the lengths required? Or maybe an array of values, sorted in
ascending order of entry depth (i.e. [ 8BIT_LENGTH, 10BIT_LENGTH,
12BIT_LENGTH ]).

Is split-gamma mode actually bound to 10-bit (and that table length)
only, or is it independent of mode? I don't really know what to
suggest about that one.

> +       switch (num_samples) {
> +       case GAMMA_DISABLE_VALS:
> +               /* Disable degamma on Pipe */
> +               mode = I915_READ(GAMMA_MODE(pipe)) & ~GAMMA_MODE_MODE_MASK;
> +               I915_WRITE(GAMMA_MODE(pipe), mode | GAMMA_MODE_MODE_8BIT);

Why aren't these writes coalesced as they are in bdw_set_gamma?

> +               state->palette_before_ctm_blob = NULL;
> +               DRM_DEBUG_DRIVER("Disabling degamma on Pipe %c\n",
> +                       pipe_name(pipe));
> +               break;

blob == NULL.

> +       case BDW_SPLITGAMMA_MAX_VALS:
> +               pal_prec_index = _MMIO(_PREC_PAL_INDEX(pipe));
> +               pal_prec_data = _MMIO(_PREC_PAL_DATA(pipe));
> +               correction_values = degamma_data->lut;
> +
> +               index = BDW_INDEX_AUTO_INCREMENT | BDW_INDEX_SPLIT_MODE;
> +               I915_WRITE(pal_prec_index, index);
> +
> +               bdw_write_10bit_gamma_precision(dev,
> +                                               correction_values,
> +                                               pal_prec_data,
> +                                               BDW_SPLITGAMMA_MAX_VALS);
> +
> +               /* Enable degamma on Pipe */
> +               mode = I915_READ(GAMMA_MODE(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));
> +               break;

Ouch, so it's 8-bit or split, nothing else. Again, that's going to be
pretty hard for generic userspace to figure out.

> +static uint32_t bdw_prepare_csc_coeff(int64_t coeff)

CHV is using s64 rather than int64_t here.

> +#define BDW_MAX_GAMMA                         ((1 << 24) - 1)

Well, at least this is consistent with CHV.

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

[PATCH 6/6] drm/i915: Register color correction capabilities

[Lionel Landwerlin]

From: Shashank Sharma <shashank.sharma@intel.com>

Register cm_coeff_after_ctm_property & cm_coeff_before_ctm_property
indicating the size of the LUT to be supplied to palette_after_ctm &
palette_before_ctm and also register the ctm property to enable color
correction matrix.

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 | 53 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c       |  1 +
 2 files changed, 54 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
index bca07c1..b50665b 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.c
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -801,4 +801,57 @@ void intel_color_manager_commit(struct drm_device *dev,
 
 void intel_crtc_attach_color_properties(struct drm_crtc *crtc)
 {
+	struct drm_device *dev = crtc->dev;
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_mode_object *mode_obj = &crtc->base;
+
+	/*
+	* Register:
+	* =========
+	* Gamma correction as palette_after_ctm property
+	* Degamma correction as palette_before_ctm property
+	*
+	* Load:
+	* =====
+	* no. of coefficients supported on this platform for gamma
+	* and degamma with the query properties. A user
+	* space agent should read these query property, and prepare
+	* the color correction values accordingly. Its expected from the
+	* driver to load the right number of coefficients during the init
+	* phase.
+	*/
+	if (config->cm_coeff_after_ctm_property) {
+		drm_object_attach_property(mode_obj,
+			config->cm_coeff_after_ctm_property,
+		INTEL_INFO(dev)->num_samples_after_ctm);
+		DRM_DEBUG_DRIVER("Gamma query property initialized\n");
+	}
+
+	if (config->cm_coeff_before_ctm_property) {
+		drm_object_attach_property(mode_obj,
+			config->cm_coeff_before_ctm_property,
+		INTEL_INFO(dev)->num_samples_before_ctm);
+		DRM_DEBUG_DRIVER("Degamma query property initialized\n");
+	}
+
+	/* Gamma correction */
+	if (config->cm_palette_after_ctm_property) {
+		drm_object_attach_property(mode_obj,
+			config->cm_palette_after_ctm_property, 0);
+		DRM_DEBUG_DRIVER("gamma property attached to CRTC\n");
+	}
+
+	/* Degamma correction */
+	if (config->cm_palette_before_ctm_property) {
+		drm_object_attach_property(mode_obj,
+			config->cm_palette_before_ctm_property, 0);
+		DRM_DEBUG_DRIVER("degamma property attached to CRTC\n");
+	}
+
+	/* CSC */
+	if (config->cm_ctm_property) {
+		drm_object_attach_property(mode_obj,
+			config->cm_ctm_property, 0);
+		DRM_DEBUG_DRIVER("CSC property attached to CRTC\n");
+	}
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b9eb507..7cf56cb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14264,6 +14264,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	intel_crtc->cursor_size = ~0;
 
 	intel_crtc->wm.cxsr_allowed = true;
+	intel_crtc_attach_color_properties(&intel_crtc->base);
 
 	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
 	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
-- 
2.6.3

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

Re: [PATCH 6/6] drm/i915: Register color correction capabilities

[Daniel Stone]

Hi,

On 17 December 2015 at 18:57, Lionel Landwerlin
<lionel.g.landwerlin@intel.com> wrote:
>  void intel_crtc_attach_color_properties(struct drm_crtc *crtc)
>  {
> +       struct drm_device *dev = crtc->dev;
> +       struct drm_mode_config *config = &dev->mode_config;
> +       struct drm_mode_object *mode_obj = &crtc->base;
> +
> +       /*
> +       * Register:
> +       * =========
> +       * Gamma correction as palette_after_ctm property
> +       * Degamma correction as palette_before_ctm property
> +       *
> +       * Load:
> +       * =====
> +       * no. of coefficients supported on this platform for gamma
> +       * and degamma with the query properties. A user
> +       * space agent should read these query property, and prepare
> +       * the color correction values accordingly. Its expected from the
> +       * driver to load the right number of coefficients during the init
> +       * phase.
> +       */
> +       if (config->cm_coeff_after_ctm_property) {
> +               drm_object_attach_property(mode_obj,
> +                       config->cm_coeff_after_ctm_property,
> +               INTEL_INFO(dev)->num_samples_after_ctm);
> +               DRM_DEBUG_DRIVER("Gamma query property initialized\n");
> +       }
> +
> +       if (config->cm_coeff_before_ctm_property) {
> +               drm_object_attach_property(mode_obj,
> +                       config->cm_coeff_before_ctm_property,
> +               INTEL_INFO(dev)->num_samples_before_ctm);
> +               DRM_DEBUG_DRIVER("Degamma query property initialized\n");
> +       }
> +
> +       /* Gamma correction */
> +       if (config->cm_palette_after_ctm_property) {
> +               drm_object_attach_property(mode_obj,
> +                       config->cm_palette_after_ctm_property, 0);
> +               DRM_DEBUG_DRIVER("gamma property attached to CRTC\n");
> +       }
> +
> +       /* Degamma correction */
> +       if (config->cm_palette_before_ctm_property) {
> +               drm_object_attach_property(mode_obj,
> +                       config->cm_palette_before_ctm_property, 0);
> +               DRM_DEBUG_DRIVER("degamma property attached to CRTC\n");
> +       }
> +
> +       /* CSC */
> +       if (config->cm_ctm_property) {
> +               drm_object_attach_property(mode_obj,
> +                       config->cm_ctm_property, 0);
> +               DRM_DEBUG_DRIVER("CSC property attached to CRTC\n");
> +       }
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b9eb507..7cf56cb 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14264,6 +14264,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>         intel_crtc->cursor_size = ~0;
>
>         intel_crtc->wm.cxsr_allowed = true;
> +       intel_crtc_attach_color_properties(&intel_crtc->base);

NAK. These properties must not be registered unless they actually
support colour management (i.e., are CHV/BDW). You can also drop the
debug prints about attaching properties to the CRTC.

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

Re: [PATCH 0/6] Color Management for DRM framework v10

[Sharma, Shashank]

Hi Lionel, Rob

NACK. 
We never agreed to squash the patches. 
The patches should be as it is, in sequence.

Regards
Shashank
-----Original Message-----
From: Landwerlin, Lionel G 
Sent: Friday, December 18, 2015 12:28 AM
To: intel-gfx@lists.freedesktop.org
Cc: Sharma, Shashank; Landwerlin, Lionel G
Subject: [PATCH 0/6] Color Management for DRM framework v10

Hi,

Here is an update on the color management serie by Shashank.
This one squashes things a bit to make it a bit easier to review.

This also includes a number of fixes,

for Braswell :

 - Read the right values from the degamma blob.

for Broadwell :

 - Always reset the lut index when writing the values for split mode,
    otherwise writing the degamma lut twice will set the gamma lut on
       the second write.

Cheers,

-
Lionel

Shashank Sharma (6):
  drm: Create Color Management DRM properties
  drm/i915: Add set property interface for CRTC
  drm/i915: Disable plane gamma
  drm/i915/chv: Implement color management
  drm/i915/bdw+: Implement color management
  drm/i915: Register color correction capabilities

 drivers/gpu/drm/drm_atomic.c               |  67 ++-
 drivers/gpu/drm/drm_atomic_helper.c        |   9 +
 drivers/gpu/drm/drm_crtc.c                 |  32 ++
 drivers/gpu/drm/i915/Makefile              |   3 +-
 drivers/gpu/drm/i915/i915_drv.c            |  19 +-
 drivers/gpu/drm/i915/i915_drv.h            |   2 +
 drivers/gpu/drm/i915/i915_reg.h            |  67 ++-
 drivers/gpu/drm/i915/intel_color_manager.c | 857 +++++++++++++++++++++++++++++  drivers/gpu/drm/i915/intel_color_manager.h | 111 ++++
 drivers/gpu/drm/i915/intel_display.c       |   6 +-
 drivers/gpu/drm/i915/intel_drv.h           |   4 +
 drivers/gpu/drm/i915/intel_sprite.c        |   7 +-
 include/drm/drm_crtc.h                     |  24 +
 include/uapi/drm/drm.h                     |  30 +
 14 files changed, 1228 insertions(+), 10 deletions(-)  create mode 100644 drivers/gpu/drm/i915/intel_color_manager.c
 create mode 100644 drivers/gpu/drm/i915/intel_color_manager.h

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