✗ Fi.CI.CHECKPATCH: warning for Add Multi Segment Gamma Support

✗ Fi.CI.CHECKPATCH: warning for Add Multi Segment Gamma Support

[Patchwork]

== Series Details ==

Series: Add Multi Segment Gamma Support
URL   : https://patchwork.freedesktop.org/series/58169/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
21ee2af67f85 drm/i915: Add gamma mode property
a2f2f0bff701 drm/i915: Add intel crtc set and get property callback
e83f5dac54cc drm/i915: Add Support for Multi Segment Gamma Mode
103a131a2e19 drm/i915: Implement get set property handler for multi segment gamma
-:79: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#79: FILE: drivers/gpu/drm/i915/intel_display.c:13772:
+		ret = intel_atomic_replace_property_blob_from_id(dev,
+					&intel_crtc_state->multi_segment_gamma_lut,

total: 0 errors, 0 warnings, 1 checks, 87 lines checked
cd708119e5a6 drm/i915/icl: Add register definitions for Multi Segmented gamma
286cdabed18f drm/i915/icl: Add support for multi segmented gamma mode
-:61: CHECK:SPACING: space preferred before that '|' (ctx:VxE)
#61: FILE: drivers/gpu/drm/i915/intel_color.c:591:
+					(GEN11_GET_LS6BITS_OF_LUT(lut[i].green) << 10)|
 					                                              ^

total: 0 errors, 0 warnings, 1 checks, 152 lines checked
d23e8eac8e99 drm/i915: Add multi segment gamma for icl

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

✗ Fi.CI.SPARSE: warning for Add Multi Segment Gamma Support

[Patchwork]

== Series Details ==

Series: Add Multi Segment Gamma Support
URL   : https://patchwork.freedesktop.org/series/58169/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915: Add gamma mode property
+drivers/gpu/drm/i915/intel_color.c:131:1: warning: symbol 'intel_attach_gamma_mode_property' was not declared. Should it be static?
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3558:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3560:16: warning: expression using sizeof(void)

Commit: drm/i915: Add intel crtc set and get property callback
+                                ^~~~~~~~
+drivers/gpu/drm/i915/intel_display.c:13691:5: warning: symbol 'intel_crtc_atomic_get_property' was not declared. Should it be static?
+drivers/gpu/drm/i915/intel_display.c:13730:32: error: ‘replaced’ undeclared (first use in this function); did you mean ‘replace_fd’?
+drivers/gpu/drm/i915/intel_display.c:13730:32: note: each undeclared identifier is reported only once for each function it appears in
+drivers/gpu/drm/i915/intel_display.c:13730:46: error: undefined identifier 'replaced'
+drivers/gpu/drm/i915/intel_display.c: In function ‘intel_crtc_atomic_set_property’:
+make[1]: *** [drivers/gpu/drm/] Error 2
+make[2]: *** [drivers/gpu/drm/i915] Error 2
+make[3]: *** [drivers/gpu/drm/i915/intel_display.o] Error 1
+make: *** [sub-make] Error 2
+                                replace_fd
+   state->color_mgmt_changed |= replaced;

Commit: drm/i915: Add Support for Multi Segment Gamma Mode
+drivers/gpu/drm/i915/intel_color.c:153:1: warning: symbol 'intel_attach_multi_segment_gamma_mode_property' was not declared. Should it be static?
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3560:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3561:16: warning: expression using sizeof(void)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)

Commit: drm/i915: Implement get set property handler for multi segment gamma
-                                ^~~~~~~~
+drivers/gpu/drm/i915/gvt/gtt.c:757:9:    expected void [noderef] <asn:4>**slot
+drivers/gpu/drm/i915/gvt/gtt.c:757:9:    expected void **slot
+drivers/gpu/drm/i915/gvt/gtt.c:757:9:    expected void **slot
+drivers/gpu/drm/i915/gvt/gtt.c:757:9:    expected void **slot
+drivers/gpu/drm/i915/gvt/gtt.c:757:9:    got void [noderef] <asn:4>**
+drivers/gpu/drm/i915/gvt/gtt.c:757:9:    got void [noderef] <asn:4>**
+drivers/gpu/drm/i915/gvt/gtt.c:757:9:    got void [noderef] <asn:4>**
+drivers/gpu/drm/i915/gvt/gtt.c:757:9:    got void **slot
+drivers/gpu/drm/i915/gvt/gtt.c:757:9: warning: incorrect type in argument 1 (different address spaces)
+drivers/gpu/drm/i915/gvt/gtt.c:757:9: warning: incorrect type in assignment (different address spaces)
+drivers/gpu/drm/i915/gvt/gtt.c:757:9: warning: incorrect type in assignment (different address spaces)
+drivers/gpu/drm/i915/gvt/gtt.c:757:9: warning: incorrect type in assignment (different address spaces)
+drivers/gpu/drm/i915/gvt/gtt.c:758:45:    expected void [noderef] <asn:4>**slot
+drivers/gpu/drm/i915/gvt/gtt.c:758:45:    got void **slot
+drivers/gpu/drm/i915/gvt/gtt.c:758:45: warning: incorrect type in argument 1 (different address spaces)
+drivers/gpu/drm/i915/gvt/mmio.c:282:23: warning: memcpy with byte count of 279040
+drivers/gpu/drm/i915/gvt/mmio.c:283:23: warning: memcpy with byte count of 279040
+drivers/gpu/drm/i915/gvt/vgpu.c:196:48: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/gvt/vgpu.c:196:48: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/gvt/vgpu.c:196:48: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/gvt/vgpu.c:196:48: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/gvt/vgpu.c:196:48: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/gvt/vgpu.c:196:48: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/gvt/vgpu.c:196:48: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/gvt/vgpu.c:196:48: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/gvt/vgpu.c:196:48: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/gvt/vgpu.c:196:48: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/gvt/vgpu.c:196:48: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/gvt/vgpu.c:196:48: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/gvt/vgpu.c:196:48: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/gvt/vgpu.c:196:48: warning: expression using sizeof(void)
-O:drivers/gpu/drm/i915/intel_display.c:13730:32: error: ‘replaced’ undeclared (first use in this function); did you mean ‘replace_fd’?
-O:drivers/gpu/drm/i915/intel_display.c:13730:32: note: each undeclared identifier is reported only once for each function it appears in
-O:drivers/gpu/drm/i915/intel_display.c:13730:46: error: undefined identifier 'replaced'
+drivers/gpu/drm/i915/intel_display.c:13755:5: warning: symbol 'intel_crtc_atomic_set_property' was not declared. Should it be static?
-drivers/gpu/drm/i915/intel_display.c: In function ‘intel_crtc_atomic_set_property’:
+./include/linux/overflow.h:251:13: error: incorrect type in conditional
+./include/linux/overflow.h:251:13: error: incorrect type in conditional
+./include/linux/overflow.h:251:13: error: undefined identifier '__builtin_mul_overflow'
+./include/linux/overflow.h:251:13: error: undefined identifier '__builtin_mul_overflow'
+./include/linux/overflow.h:251:13:    got void
+./include/linux/overflow.h:251:13:    got void
+./include/linux/overflow.h:251:13: warning: call with no type!
+./include/linux/overflow.h:251:13: warning: call with no type!
+./include/linux/slab.h:664:13: error: undefined identifier '__builtin_mul_overflow'
+./include/linux/slab.h:664:13: error: undefined identifier '__builtin_mul_overflow'
+./include/linux/slab.h:664:13: warning: call with no type!
+./include/linux/slab.h:664:13: warning: call with no type!
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
-make[1]: *** [drivers/gpu/drm/] Error 2
-make[2]: *** [drivers/gpu/drm/i915] Error 2
-make[3]: *** [drivers/gpu/drm/i915/intel_display.o] Error 1
-make[3]: *** Waiting for unfinished jobs....
-make: *** [sub-make] Error 2
-                                replace_fd
-   state->color_mgmt_changed |= replaced;

Commit: drm/i915/icl: Add register definitions for Multi Segmented gamma
Okay!

Commit: drm/i915/icl: Add support for multi segmented gamma mode
Okay!

Commit: drm/i915: Add multi segment gamma for icl
Okay!

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

[RFC v1 0/7] Add Multi Segment Gamma Support

[Uma Shankar]

This series adds support for programmable gamma modes and
exposes a property interface for the same. Also added,
support for mult segment gamma mode introduced in ICL+

Uma Shankar (7):
  drm/i915: Add gamma mode property
  drm/i915: Add intel crtc set and get property callback
  drm/i915: Add Support for Multi Segment Gamma Mode
  drm/i915: Implement get set property handler for multi segment gamma
  drm/i915/icl: Add register definitions for Multi Segmented gamma
  drm/i915/icl: Add support for multi segmented gamma mode
  drm/i915: Add multi segment gamma for icl

 drivers/gpu/drm/i915/i915_drv.h      |   3 +
 drivers/gpu/drm/i915/i915_reg.h      |  17 +++
 drivers/gpu/drm/i915/intel_color.c   | 194 ++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_display.c | 104 +++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  15 +++
 include/uapi/drm/i915_drm.h          |  14 +++
 6 files changed, 332 insertions(+), 15 deletions(-)

-- 
1.9.1

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

[RFC v1 1/7] drm/i915: Add gamma mode property

[Uma Shankar]

Gen platforms support multiple gamma modes, currently
it's hard coded to operate only in 1 specific mode.
This patch adds a property to make gamma mode programmable.
User can select which mode should be used for a particular
usecase or scenario.

Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h    |  2 ++
 drivers/gpu/drm/i915/intel_color.c | 46 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h   |  3 +++
 3 files changed, 51 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c65c2e6..02231ae 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1735,6 +1735,8 @@ struct drm_i915_private {
 	struct drm_property *broadcast_rgb_property;
 	struct drm_property *force_audio_property;
 
+	struct drm_property *gamma_mode_property;
+
 	/* hda/i915 audio component */
 	struct i915_audio_component *audio_component;
 	bool audio_component_registered;
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 467fd1a..9d43d19 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -92,6 +92,19 @@
 	0x0800, 0x0100, 0x0800,
 };
 
+#define LEGACY_PALETTE_MODE_8BIT		BIT(0)
+#define PRECISION_PALETTE_MODE_10BIT		BIT(1)
+#define INTERPOLATED_GAMMA_MODE_12BIT		BIT(2)
+#define MULTI_SEGMENTED_GAMMA_MODE_12BIT	BIT(3)
+#define SPLIT_GAMMA_MODE_12BIT			BIT(4)
+
+#define INTEL_GAMMA_MODE_MASK (\
+		LEGACY_PALETTE_MODE_8BIT | \
+		PRECISION_PALETTE_MODE_10BIT | \
+		INTERPOLATED_GAMMA_MODE_12BIT | \
+		MULTI_SEGMENTED_GAMMA_MODE_12BIT | \
+		BIT_SPLIT_GAMMA_MODE_12BIT)
+
 static bool lut_is_legacy(const struct drm_property_blob *lut)
 {
 	return drm_color_lut_size(lut) == LEGACY_LUT_LENGTH;
@@ -105,6 +118,37 @@ static bool crtc_state_is_legacy_gamma(const struct intel_crtc_state *crtc_state
 		lut_is_legacy(crtc_state->base.gamma_lut);
 }
 
+static const struct drm_prop_enum_list gamma_mode_supported[] = {
+	{ LEGACY_PALETTE_MODE_8BIT, "8 Bit Legacy Palette Mode" },
+	{ PRECISION_PALETTE_MODE_10BIT, "10 Bit Precision Palette Mode" },
+	{ INTERPOLATED_GAMMA_MODE_12BIT, "12 Bit Interploated Gamma Mode" },
+	{ MULTI_SEGMENTED_GAMMA_MODE_12BIT,
+			"12 Bit Multi Segmented Gamma Mode" },
+	{ SPLIT_GAMMA_MODE_12BIT, "12 Bit Split Gamma Mode" },
+};
+
+void
+intel_attach_gamma_mode_property(struct intel_crtc *crtc)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_property *prop;
+
+	prop = dev_priv->gamma_mode_property;
+	if (!prop) {
+		prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
+						"Gamma Mode",
+						gamma_mode_supported,
+						ARRAY_SIZE(gamma_mode_supported));
+		if (!prop)
+			return;
+
+		dev_priv->gamma_mode_property = prop;
+	}
+
+	drm_object_attach_property(&crtc->base.base, prop, 0);
+}
+
 /*
  * When using limited range, multiply the matrix given by userspace by
  * the matrix that we would use for the limited range.
@@ -907,4 +951,6 @@ void intel_color_init(struct intel_crtc *crtc)
 					   INTEL_INFO(dev_priv)->color.degamma_lut_size,
 					   true,
 					   INTEL_INFO(dev_priv)->color.gamma_lut_size);
+
+	intel_attach_gamma_mode_property(crtc);
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d9f188e..fd84fe9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1034,6 +1034,9 @@ struct intel_crtc_state {
 	u8 nv12_planes;
 	u8 c8_planes;
 
+	/* Gamma mode type programmed on the pipe */
+	u32 gamma_mode_type;
+
 	/* bitmask of planes that will be updated during the commit */
 	u8 update_planes;
 
-- 
1.9.1

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

[RFC v1 2/7] drm/i915: Add intel crtc set and get property callback

[Uma Shankar]

Add intel crtc set and get property callbacks. Currently
added for gamma mode property set and get implementation.

Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 58 ++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 61acbaf..7604f16 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13679,6 +13679,62 @@ static int intel_atomic_commit(struct drm_device *dev,
 	return 0;
 }
 
+/**
+ * intel_crtc_atomic_get_property - hook for crtc->atomic_get_property.
+ * @crtc: Crtc to get the property for.
+ * @state: Crtc state to retrieve the property from.
+ * @property: Property to retrieve.
+ * @val: Return value for the property.
+ *
+ * Returns the atomic property value for a crtc.
+ */
+int intel_crtc_atomic_get_property(struct drm_crtc *crtc,
+				   const struct drm_crtc_state *state,
+				   struct drm_property *property,
+				   uint64_t *val)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(state);
+
+	if (property == dev_priv->gamma_mode_property) {
+		*val = intel_crtc_state->gamma_mode_type;
+	} else {
+		DRM_DEBUG_ATOMIC("Unknown property %s\n", property->name);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/**
+ * intel_crtc_atomic_set_property - hook for crtc->atomic_set_property.
+ * @crtc: Crtc to set the property for.
+ * @state: Crtc state to set the property on.
+ * @property: Property to set.
+ * @val: New value for the property.
+ *
+ * Sets the atomic property value for a crtc.
+ */
+int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
+				   struct drm_crtc_state *state,
+				   struct drm_property *property,
+				   uint64_t val)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(state);
+
+	if (property == dev_priv->gamma_mode_property) {
+		intel_crtc_state->gamma_mode_type = val;
+		state->color_mgmt_changed |= replaced;
+		return 0;
+	}
+
+	DRM_DEBUG_ATOMIC("Unknown property %s\n", property->name);
+	return -EINVAL;
+}
+
 static const struct drm_crtc_funcs intel_crtc_funcs = {
 	.gamma_set = drm_atomic_helper_legacy_gamma_set,
 	.set_config = drm_atomic_helper_set_config,
@@ -13689,6 +13745,8 @@ static int intel_atomic_commit(struct drm_device *dev,
 	.set_crc_source = intel_crtc_set_crc_source,
 	.verify_crc_source = intel_crtc_verify_crc_source,
 	.get_crc_sources = intel_crtc_get_crc_sources,
+	.atomic_get_property = intel_crtc_atomic_get_property,
+	.atomic_set_property = intel_crtc_atomic_set_property,
 };
 
 struct wait_rps_boost {
-- 
1.9.1

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

[RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode

[Uma Shankar]

Multi Segment Gamma Mode is added in Gen11+ platforms.
Added a property interface to enable that.

Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h    |  1 +
 drivers/gpu/drm/i915/intel_color.c | 23 +++++++++++++++++++++++
 include/uapi/drm/i915_drm.h        | 14 ++++++++++++++
 3 files changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 02231ae..f20d418 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1736,6 +1736,7 @@ struct drm_i915_private {
 	struct drm_property *force_audio_property;
 
 	struct drm_property *gamma_mode_property;
+	struct drm_property *multi_segment_gamma_mode_property;
 
 	/* hda/i915 audio component */
 	struct i915_audio_component *audio_component;
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 9d43d19..399d63d 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -149,6 +149,26 @@ static bool crtc_state_is_legacy_gamma(const struct intel_crtc_state *crtc_state
 	drm_object_attach_property(&crtc->base.base, prop, 0);
 }
 
+void
+intel_attach_multi_segment_gamma_mode_property(struct intel_crtc *crtc)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_property *prop;
+
+	prop = dev_priv->multi_segment_gamma_mode_property;
+	if (!prop) {
+		prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
+					   "Multi-segment Gamma", 0);
+		if (!prop)
+			return;
+
+		dev_priv->multi_segment_gamma_mode_property = prop;
+	}
+
+	drm_object_attach_property(&crtc->base.base, prop, 0);
+}
+
 /*
  * When using limited range, multiply the matrix given by userspace by
  * the matrix that we would use for the limited range.
@@ -953,4 +973,7 @@ void intel_color_init(struct intel_crtc *crtc)
 					   INTEL_INFO(dev_priv)->color.gamma_lut_size);
 
 	intel_attach_gamma_mode_property(crtc);
+
+	if (INTEL_GEN(dev_priv) >= 11)
+		intel_attach_multi_segment_gamma_mode_property(crtc);
 }
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index aa2d4c7..8f1974e 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1842,6 +1842,20 @@ struct drm_i915_query_topology_info {
 	__u8 data[];
 };
 
+/*
+ * Structure for muti segmented gamma lut
+ */
+struct multi_segment_gamma_lut {
+	/* Number of Lut Segments */
+	__u8 segment_cnt;
+	/* Precison of LUT entries in bits */
+	__u8 precision_bits;
+	/* Pointer having number of LUT elements in each segment */
+	__u32 *segment_lut_cnt_ptr;
+	/* Pointer to store exact lut values for each segment */
+	__u32 *segment_lut_ptr;
+};
+
 #if defined(__cplusplus)
 }
 #endif
-- 
1.9.1

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

[RFC v1 4/7] drm/i915: Implement get set property handler for multi segment gamma

[Uma Shankar]

Implement get and set property handler for multi segment gamma
mode.

Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 46 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     | 12 ++++++++++
 2 files changed, 58 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7604f16..8ac9728 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13679,6 +13679,39 @@ static int intel_atomic_commit(struct drm_device *dev,
 	return 0;
 }
 
+static int
+intel_atomic_replace_property_blob_from_id(struct drm_device *dev,
+					   struct drm_property_blob **blob,
+					   u64 blob_id,
+					   ssize_t expected_size,
+					   ssize_t expected_elem_size,
+					   bool *replaced)
+{
+	struct drm_property_blob *new_blob = NULL;
+
+	if (blob_id != 0) {
+		new_blob = drm_property_lookup_blob(dev, blob_id);
+		if (!new_blob)
+			return -EINVAL;
+
+		if (expected_size > 0 &&
+		    new_blob->length != expected_size) {
+			drm_property_blob_put(new_blob);
+			return -EINVAL;
+		}
+		if (expected_elem_size > 0 &&
+		    new_blob->length % expected_elem_size != 0) {
+			drm_property_blob_put(new_blob);
+			return -EINVAL;
+		}
+	}
+
+	*replaced |= drm_property_replace_blob(blob, new_blob);
+	drm_property_blob_put(new_blob);
+
+	return 0;
+}
+
 /**
  * intel_crtc_atomic_get_property - hook for crtc->atomic_get_property.
  * @crtc: Crtc to get the property for.
@@ -13699,6 +13732,9 @@ int intel_crtc_atomic_get_property(struct drm_crtc *crtc,
 
 	if (property == dev_priv->gamma_mode_property) {
 		*val = intel_crtc_state->gamma_mode_type;
+	} else if (property == dev_priv->multi_segment_gamma_mode_property) {
+		*val = (intel_crtc_state->multi_segment_gamma_lut) ?
+			intel_crtc_state->multi_segment_gamma_lut->base.id : 0;
 	} else {
 		DRM_DEBUG_ATOMIC("Unknown property %s\n", property->name);
 		return -EINVAL;
@@ -13724,11 +13760,21 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(state);
+	bool replaced = false;
+	int ret;
 
 	if (property == dev_priv->gamma_mode_property) {
 		intel_crtc_state->gamma_mode_type = val;
 		state->color_mgmt_changed |= replaced;
 		return 0;
+	} else if (property == dev_priv->multi_segment_gamma_mode_property) {
+		ret = intel_atomic_replace_property_blob_from_id(dev,
+					&intel_crtc_state->multi_segment_gamma_lut,
+					val, -1,
+					sizeof(struct multi_segment_gamma_lut),
+					&replaced);
+		state->color_mgmt_changed |= replaced;
+		return ret;
 	}
 
 	DRM_DEBUG_ATOMIC("Unknown property %s\n", property->name);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index fd84fe9..7fccd28 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1037,6 +1037,18 @@ struct intel_crtc_state {
 	/* Gamma mode type programmed on the pipe */
 	u32 gamma_mode_type;
 
+	/**
+	 * @multi_segment_gamma_lut:
+	 *
+	 * Lookup table for converting pixel data after the color conversion
+	 * matrix @ctm.  See drm_crtc_enable_color_mgmt(). The blob (if not
+	 * NULL) is an array of &struct drm_color_lut.
+	 */
+	struct drm_property_blob *multi_segment_gamma_lut;
+
+	/* State to check for Gamma Mode Color Changes */
+	bool color_mgmt_changed : 1;
+
 	/* bitmask of planes that will be updated during the commit */
 	u8 update_planes;
 
-- 
1.9.1

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

[RFC v1 5/7] drm/i915/icl: Add register definitions for Multi Segmented gamma

[Uma Shankar]

Add macros to define multi segmented gamma registers

Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 31a3020..44ca13b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7209,6 +7209,7 @@ enum {
 #define  GAMMA_MODE_MODE_10BIT	(1 << 0)
 #define  GAMMA_MODE_MODE_12BIT	(2 << 0)
 #define  GAMMA_MODE_MODE_SPLIT	(3 << 0)
+#define  GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED	(3 << 0)
 
 /* DMC/CSR */
 #define CSR_PROGRAM(i)		_MMIO(0x80000 + (i) * 4)
@@ -10147,6 +10148,22 @@ enum skl_power_gate {
 #define PRE_CSC_GAMC_INDEX(pipe)	_MMIO_PIPE(pipe, _PRE_CSC_GAMC_INDEX_A, _PRE_CSC_GAMC_INDEX_B)
 #define PRE_CSC_GAMC_DATA(pipe)		_MMIO_PIPE(pipe, _PRE_CSC_GAMC_DATA_A, _PRE_CSC_GAMC_DATA_B)
 
+/* Add registers for Gen11 Multi Segmented Gamma Mode */
+#define _PAL_PREC_MULTI_SEG_INDEX_A	0x4A408
+#define _PAL_PREC_MULTI_SEG_INDEX_B	0x4AC08
+#define  PAL_PREC_MULTI_SEGMENT_AUTO_INCREMENT		BIT(15)
+#define  PAL_PREC_MULTI_SEGMENT_INDEX_VALUE_MASK	(0x1f << 0)
+
+#define _PAL_PREC_MULTI_SEG_DATA_A	0x4A40C
+#define _PAL_PREC_MULTI_SEG_DATA_B	0x4AC0C
+
+#define PREC_PAL_MULTI_SEG_INDEX(pipe)	_MMIO_PIPE(pipe, \
+					_PAL_PREC_MULTI_SEG_INDEX_A, \
+					_PAL_PREC_MULTI_SEG_INDEX_B)
+#define PREC_PAL_MULTI_SEG_DATA(pipe)	_MMIO_PIPE(pipe, \
+					_PAL_PREC_MULTI_SEG_DATA_A, \
+					_PAL_PREC_MULTI_SEG_DATA_B)
+
 /* pipe CSC & degamma/gamma LUTs on CHV */
 #define _CGM_PIPE_A_CSC_COEFF01	(VLV_DISPLAY_BASE + 0x67900)
 #define _CGM_PIPE_A_CSC_COEFF23	(VLV_DISPLAY_BASE + 0x67904)
-- 
1.9.1

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

[RFC v1 6/7] drm/i915/icl: Add support for multi segmented gamma mode

[Uma Shankar]

Gen11 introduced a new gamma mode i.e, multi segmented
gamma mode. Added support for the same.

Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
 drivers/gpu/drm/i915/intel_color.c | 105 +++++++++++++++++++++++++++++++------
 1 file changed, 90 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 399d63d..7733c256 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -105,6 +105,9 @@
 		MULTI_SEGMENTED_GAMMA_MODE_12BIT | \
 		BIT_SPLIT_GAMMA_MODE_12BIT)
 
+#define GEN11_GET_MS10BITS_OF_LUT(lut) (((lut) >> 6) & 0x3FF)
+#define GEN11_GET_LS6BITS_OF_LUT(lut)  ((lut) & 0x3F)
+
 static bool lut_is_legacy(const struct drm_property_blob *lut)
 {
 	return drm_color_lut_size(lut) == LEGACY_LUT_LENGTH;
@@ -538,6 +541,10 @@ static void bdw_load_degamma_lut(const struct intel_crtc_state *crtc_state)
 	if (degamma_lut) {
 		const struct drm_color_lut *lut = degamma_lut->data;
 
+		if (crtc_state->gamma_mode_type ==
+				MULTI_SEGMENTED_GAMMA_MODE_12BIT)
+			lut += 9;
+
 		for (i = 0; i < lut_size; i++) {
 			u32 word =
 			drm_color_lut_extract(lut[i].red, 10) << 20 |
@@ -563,6 +570,7 @@ static void bdw_load_gamma_lut(const struct intel_crtc_state *crtc_state, u32 of
 	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
 	u32 i, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
 	enum pipe pipe = crtc->pipe;
+	u32 word;
 
 	WARN_ON(offset & ~PAL_PREC_INDEX_VALUE_MASK);
 
@@ -574,13 +582,32 @@ static void bdw_load_gamma_lut(const struct intel_crtc_state *crtc_state, u32 of
 	if (gamma_lut) {
 		const struct drm_color_lut *lut = gamma_lut->data;
 
-		for (i = 0; i < lut_size; i++) {
-			u32 word =
-			(drm_color_lut_extract(lut[i].red, 10) << 20) |
-			(drm_color_lut_extract(lut[i].green, 10) << 10) |
-			drm_color_lut_extract(lut[i].blue, 10);
-
-			I915_WRITE(PREC_PAL_DATA(pipe), word);
+		if (crtc_state->gamma_mode_type ==
+				MULTI_SEGMENTED_GAMMA_MODE_12BIT) {
+			lut_size = 9 + 512;
+			for (i = 9; i < lut_size; i++) {
+				/* For Even Index */
+				word = (GEN11_GET_LS6BITS_OF_LUT(lut[i].red) << 20) |
+					(GEN11_GET_LS6BITS_OF_LUT(lut[i].green) << 10)|
+					GEN11_GET_LS6BITS_OF_LUT(lut[i].blue);
+
+					 I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe), word);
+
+					 /* For ODD index */
+					 word = (GEN11_GET_MS10BITS_OF_LUT(lut[i].red) << 20) |
+						(GEN11_GET_MS10BITS_OF_LUT(lut[i].green) << 10) |
+						GEN11_GET_MS10BITS_OF_LUT(lut[i].blue);
+
+					I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe), word);
+			}
+		} else {
+			for (i = 0; i < lut_size; i++) {
+				word = (drm_color_lut_extract(lut[i].red, 10) << 20) |
+					(drm_color_lut_extract(lut[i].green, 10) << 10) |
+					drm_color_lut_extract(lut[i].blue, 10);
+
+				I915_WRITE(PREC_PAL_DATA(pipe), word);
+			}
 		}
 
 		/* Program the max register to clamp values > 1.0. */
@@ -685,15 +712,57 @@ static void glk_load_luts(const struct intel_crtc_state *crtc_state)
 		bdw_load_gamma_lut(crtc_state, 0);
 }
 
+static void icl_load_gamma_multi_segmented_lut(const struct intel_crtc_state
+					       *crtc_state, u32 offset)
+{
+	struct drm_crtc *crtc = crtc_state->base.crtc;
+	struct drm_device *dev = crtc_state->base.crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	enum pipe pipe = to_intel_crtc(crtc)->pipe;
+	u32 i, lut_size = 9;
+
+	WARN_ON(offset & ~PAL_PREC_MULTI_SEGMENT_INDEX_VALUE_MASK);
+
+	I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe),
+		   (PAL_PREC_AUTO_INCREMENT | offset));
+
+	if (crtc_state->base.gamma_lut) {
+		struct drm_color_lut *lut =
+			(struct drm_color_lut *)crtc_state->base.gamma_lut->data;
+
+		for (i = 0; i < lut_size; i++) {
+			u32 word;
+
+			/* For Even Index */
+			word = (GEN11_GET_LS6BITS_OF_LUT(lut[i].red) << 20) |
+			       (GEN11_GET_LS6BITS_OF_LUT(lut[i].green) << 10) |
+			       GEN11_GET_LS6BITS_OF_LUT(lut[i].blue);
+			I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe), word);
+
+			/* For ODD index */
+			word = (GEN11_GET_MS10BITS_OF_LUT(lut[i].red) << 20) |
+			       (GEN11_GET_MS10BITS_OF_LUT(lut[i].green) << 10) |
+			       GEN11_GET_MS10BITS_OF_LUT(lut[i].blue);
+			I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe), word);
+		}
+	}
+
+	bdw_load_gamma_lut(crtc_state, 0);
+}
+
 static void icl_load_luts(const struct intel_crtc_state *crtc_state)
 {
 	glk_load_degamma_lut(crtc_state);
 
-	if (crtc_state_is_legacy_gamma(crtc_state))
+	if (crtc_state_is_legacy_gamma(crtc_state)) {
 		i9xx_load_luts(crtc_state);
-	else
+	} else if (crtc_state->gamma_mode_type ==
+		   MULTI_SEGMENTED_GAMMA_MODE_12BIT) {
+		icl_load_gamma_multi_segmented_lut(crtc_state, 0);
+	} else {
 		/* ToDo: Add support for multi segment gamma LUT */
 		bdw_load_gamma_lut(crtc_state, 0);
+	}
 }
 
 static void cherryview_load_luts(const struct intel_crtc_state *crtc_state)
@@ -910,16 +979,22 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
 	    drm_color_lut_check(gamma_lut, gamma_tests))
 		return -EINVAL;
 
-	if (INTEL_GEN(dev_priv) >= 11)
-		crtc_state->gamma_mode = GAMMA_MODE_MODE_10BIT |
-					 PRE_CSC_GAMMA_ENABLE |
+	if (INTEL_GEN(dev_priv) >= 11) {
+		crtc_state->gamma_mode = PRE_CSC_GAMMA_ENABLE |
 					 POST_CSC_GAMMA_ENABLE;
-	else if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
+		if (crtc_state->gamma_mode_type ==
+				MULTI_SEGMENTED_GAMMA_MODE_12BIT)
+			crtc_state->gamma_mode |=
+				GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED;
+		else
+			crtc_state->gamma_mode |= GAMMA_MODE_MODE_10BIT;
+	} else if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
 		crtc_state->gamma_mode = GAMMA_MODE_MODE_10BIT;
-	else if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
+	} else if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) {
 		crtc_state->gamma_mode = GAMMA_MODE_MODE_SPLIT;
-	else
+	} else {
 		crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
+	}
 
 	if (INTEL_GEN(dev_priv) >= 11) {
 		if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB ||
-- 
1.9.1

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

[RFC v1 7/7] drm/i915: Add multi segment gamma for icl

[Uma Shankar]

Added support for ICL platform multi segment gamma
capabilties and attached the property, exposing the
same to userspace.

Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
 drivers/gpu/drm/i915/intel_color.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 7733c256..1e9f784 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -1011,6 +1011,8 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
 void intel_color_init(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	struct multi_segment_gamma_lut multi_segment_lut;
+
 
 	drm_mode_crtc_set_gamma_size(&crtc->base, 256);
 
@@ -1049,6 +1051,24 @@ void intel_color_init(struct intel_crtc *crtc)
 
 	intel_attach_gamma_mode_property(crtc);
 
-	if (INTEL_GEN(dev_priv) >= 11)
+	if (IS_ICELAKE(dev_priv)) {
+		multi_segment_lut.segment_cnt = 3;
+		multi_segment_lut.precision_bits = 16;
+		multi_segment_lut.segment_lut_cnt_ptr = kzalloc(3 * sizeof(int),
+								GFP_KERNEL);
+		if (!multi_segment_lut.segment_lut_cnt_ptr)
+			return;
+		multi_segment_lut.segment_lut_cnt_ptr[0] = 9;
+		multi_segment_lut.segment_lut_cnt_ptr[1] = 256;
+		multi_segment_lut.segment_lut_cnt_ptr[2] = 256;
+
 		intel_attach_multi_segment_gamma_mode_property(crtc);
+
+		drm_property_replace_global_blob(crtc->base.dev,
+						 &crtc->config->multi_segment_gamma_lut,
+						 sizeof(struct multi_segment_gamma_lut),
+						 &multi_segment_lut,
+						 &crtc->base.base,
+						 dev_priv->multi_segment_gamma_mode_property);
+	}
 }
-- 
1.9.1

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

✗ Fi.CI.BAT: failure for Add Multi Segment Gamma Support

[Patchwork]

== Series Details ==

Series: Add Multi Segment Gamma Support
URL   : https://patchwork.freedesktop.org/series/58169/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5770 -> Patchwork_12509
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_12509 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_12509, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/58169/revisions/1/mbox/

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_12509:

### IGT changes ###

#### Possible regressions ####

  * igt@runner@aborted:
    - fi-ilk-650:         NOTRUN -> FAIL
    - fi-pnv-d510:        NOTRUN -> FAIL
    - fi-bdw-gvtdvm:      NOTRUN -> FAIL
    - fi-hsw-peppy:       NOTRUN -> FAIL
    - fi-gdg-551:         NOTRUN -> FAIL
    - fi-snb-2520m:       NOTRUN -> FAIL
    - fi-hsw-4770:        NOTRUN -> FAIL
    - fi-whl-u:           NOTRUN -> FAIL
    - fi-icl-u3:          NOTRUN -> FAIL
    - fi-ivb-3770:        NOTRUN -> FAIL
    - fi-byt-j1900:       NOTRUN -> FAIL
    - fi-bsw-n3050:       NOTRUN -> FAIL
    - fi-blb-e6850:       NOTRUN -> FAIL
    - fi-bsw-kefka:       NOTRUN -> FAIL
    - fi-hsw-4770r:       NOTRUN -> FAIL
    - fi-byt-clapper:     NOTRUN -> FAIL
    - fi-bdw-5557u:       NOTRUN -> FAIL
    - fi-bwr-2160:        NOTRUN -> FAIL
    - fi-byt-n2820:       NOTRUN -> FAIL
    - fi-elk-e7500:       NOTRUN -> FAIL

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@runner@aborted:
    - {fi-icl-y}:         NOTRUN -> FAIL

  
Known issues
------------

  Here are the changes found in Patchwork_12509 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@runner@aborted:
    - fi-cfl-8109u:       NOTRUN -> FAIL [k.org#202107] / [k.org#202109]
    - fi-bxt-j4205:       NOTRUN -> FAIL [fdo#109516]
    - fi-skl-iommu:       NOTRUN -> FAIL [fdo#104108]
    - fi-cfl-guc:         NOTRUN -> FAIL [k.org#202107] / [k.org#202109]
    - fi-kbl-7567u:       NOTRUN -> FAIL [fdo#108903] / [fdo#108904] / [fdo#108905]
    - fi-skl-guc:         NOTRUN -> FAIL [fdo#104108]
    - fi-skl-6700k2:      NOTRUN -> FAIL [fdo#104108]
    - fi-kbl-x1275:       NOTRUN -> FAIL [fdo#108903] / [fdo#108904] / [fdo#108905]
    - fi-cfl-8700k:       NOTRUN -> FAIL [k.org#202107] / [k.org#202109]
    - fi-skl-6600u:       NOTRUN -> FAIL [fdo#104108]
    - fi-kbl-8809g:       NOTRUN -> FAIL [fdo#108903] / [fdo#108904] / [fdo#108905]
    - fi-apl-guc:         NOTRUN -> FAIL [fdo#109373]
    - fi-kbl-r:           NOTRUN -> FAIL [fdo#108903] / [fdo#108904] / [fdo#108905]
    - fi-skl-6770hq:      NOTRUN -> FAIL [fdo#104108]
    - fi-kbl-guc:         NOTRUN -> FAIL [fdo#108903] / [fdo#108904] / [fdo#108905]
    - fi-skl-gvtdvm:      NOTRUN -> FAIL [fdo#104108]
    - fi-snb-2600:        NOTRUN -> FAIL [fdo#108929]
    - fi-skl-6260u:       NOTRUN -> FAIL [fdo#104108]

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#108903]: https://bugs.freedesktop.org/show_bug.cgi?id=108903
  [fdo#108904]: https://bugs.freedesktop.org/show_bug.cgi?id=108904
  [fdo#108905]: https://bugs.freedesktop.org/show_bug.cgi?id=108905
  [fdo#108929]: https://bugs.freedesktop.org/show_bug.cgi?id=108929
  [fdo#109373]: https://bugs.freedesktop.org/show_bug.cgi?id=109373
  [fdo#109516]: https://bugs.freedesktop.org/show_bug.cgi?id=109516
  [k.org#202107]: https://bugzilla.kernel.org/show_bug.cgi?id=202107
  [k.org#202109]: https://bugzilla.kernel.org/show_bug.cgi?id=202109


Participating hosts (44 -> 42)
------------------------------

  Additional (2): fi-bsw-kefka fi-bsw-n3050 
  Missing    (4): fi-kbl-soraka fi-bsw-cyan fi-bdw-samus fi-hsw-4200u 


Build changes
-------------

    * Linux: CI_DRM_5770 -> Patchwork_12509

  CI_DRM_5770: 7f60fa0ec6f20661a49a3eeed6e4b0a175783cf6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4888: 71ad19eb8fe4f0eecae3bf063e107293b90b9abc @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12509: d23e8eac8e99b570921ae2225b87f1cea6e4828f @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

d23e8eac8e99 drm/i915: Add multi segment gamma for icl
286cdabed18f drm/i915/icl: Add support for multi segmented gamma mode
cd708119e5a6 drm/i915/icl: Add register definitions for Multi Segmented gamma
103a131a2e19 drm/i915: Implement get set property handler for multi segment gamma
e83f5dac54cc drm/i915: Add Support for Multi Segment Gamma Mode
a2f2f0bff701 drm/i915: Add intel crtc set and get property callback
21ee2af67f85 drm/i915: Add gamma mode property

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12509/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode

[Lankhorst, Maarten]

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

tis 2019-03-19 klockan 14:00 +0530 skrev Uma Shankar:
> Multi Segment Gamma Mode is added in Gen11+ platforms.
> Added a property interface to enable that.
> 
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h    |  1 +
>  drivers/gpu/drm/i915/intel_color.c | 23 +++++++++++++++++++++++
>  include/uapi/drm/i915_drm.h        | 14 ++++++++++++++
>  3 files changed, 38 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 02231ae..f20d418 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1736,6 +1736,7 @@ struct drm_i915_private {
>  	struct drm_property *force_audio_property;
>  
>  	struct drm_property *gamma_mode_property;
> +	struct drm_property *multi_segment_gamma_mode_property;

Seems to me both properties should be part of drm core?

>  	/* hda/i915 audio component */
>  	struct i915_audio_component *audio_component;
> diff --git a/drivers/gpu/drm/i915/intel_color.c
> b/drivers/gpu/drm/i915/intel_color.c
> index 9d43d19..399d63d 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -149,6 +149,26 @@ static bool crtc_state_is_legacy_gamma(const
> struct intel_crtc_state *crtc_state
>  	drm_object_attach_property(&crtc->base.base, prop, 0);
>  }
>  
> +void
> +intel_attach_multi_segment_gamma_mode_property(struct intel_crtc
> *crtc)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_property *prop;
> +
> +	prop = dev_priv->multi_segment_gamma_mode_property;
> +	if (!prop) {
> +		prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> +					   "Multi-segment Gamma",
> 0);
> +		if (!prop)
> +			return;
> +
> +		dev_priv->multi_segment_gamma_mode_property = prop;
> +	}
> +
> +	drm_object_attach_property(&crtc->base.base, prop, 0);
> +}
> +
>  /*
>   * When using limited range, multiply the matrix given by userspace
> by
>   * the matrix that we would use for the limited range.
> @@ -953,4 +973,7 @@ void intel_color_init(struct intel_crtc *crtc)
>  					   INTEL_INFO(dev_priv)-
> >color.gamma_lut_size);
>  
>  	intel_attach_gamma_mode_property(crtc);
> +
> +	if (INTEL_GEN(dev_priv) >= 11)
> +		intel_attach_multi_segment_gamma_mode_property(crtc)
> ;
>  }
> diff --git a/include/uapi/drm/i915_drm.h
> b/include/uapi/drm/i915_drm.h
> index aa2d4c7..8f1974e 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1842,6 +1842,20 @@ struct drm_i915_query_topology_info {
>  	__u8 data[];
>  };
>  
> +/*
> + * Structure for muti segmented gamma lut
> + */
> +struct multi_segment_gamma_lut {
> +	/* Number of Lut Segments */
> +	__u8 segment_cnt;
> +	/* Precison of LUT entries in bits */
> +	__u8 precision_bits;
> +	/* Pointer having number of LUT elements in each segment */
> +	__u32 *segment_lut_cnt_ptr;
> +	/* Pointer to store exact lut values for each segment */
> +	__u32 *segment_lut_ptr;
> +};
> 
And perhaps a variation of this as description for all gamma mode
types.

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3282 bytes --]

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

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

Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode

[Ville Syrjälä]

On Tue, Mar 19, 2019 at 10:46:27AM +0200, Lankhorst, Maarten wrote:
> tis 2019-03-19 klockan 14:00 +0530 skrev Uma Shankar:
> > Multi Segment Gamma Mode is added in Gen11+ platforms.
> > Added a property interface to enable that.
> > 
> > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h    |  1 +
> >  drivers/gpu/drm/i915/intel_color.c | 23 +++++++++++++++++++++++
> >  include/uapi/drm/i915_drm.h        | 14 ++++++++++++++
> >  3 files changed, 38 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 02231ae..f20d418 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1736,6 +1736,7 @@ struct drm_i915_private {
> >  	struct drm_property *force_audio_property;
> >  
> >  	struct drm_property *gamma_mode_property;
> > +	struct drm_property *multi_segment_gamma_mode_property;
> 
> Seems to me both properties should be part of drm core?
> 
> >  	/* hda/i915 audio component */
> >  	struct i915_audio_component *audio_component;
> > diff --git a/drivers/gpu/drm/i915/intel_color.c
> > b/drivers/gpu/drm/i915/intel_color.c
> > index 9d43d19..399d63d 100644
> > --- a/drivers/gpu/drm/i915/intel_color.c
> > +++ b/drivers/gpu/drm/i915/intel_color.c
> > @@ -149,6 +149,26 @@ static bool crtc_state_is_legacy_gamma(const
> > struct intel_crtc_state *crtc_state
> >  	drm_object_attach_property(&crtc->base.base, prop, 0);
> >  }
> >  
> > +void
> > +intel_attach_multi_segment_gamma_mode_property(struct intel_crtc
> > *crtc)
> > +{
> > +	struct drm_device *dev = crtc->base.dev;
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	struct drm_property *prop;
> > +
> > +	prop = dev_priv->multi_segment_gamma_mode_property;
> > +	if (!prop) {
> > +		prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> > +					   "Multi-segment Gamma",
> > 0);
> > +		if (!prop)
> > +			return;
> > +
> > +		dev_priv->multi_segment_gamma_mode_property = prop;
> > +	}
> > +
> > +	drm_object_attach_property(&crtc->base.base, prop, 0);
> > +}
> > +
> >  /*
> >   * When using limited range, multiply the matrix given by userspace
> > by
> >   * the matrix that we would use for the limited range.
> > @@ -953,4 +973,7 @@ void intel_color_init(struct intel_crtc *crtc)
> >  					   INTEL_INFO(dev_priv)-
> > >color.gamma_lut_size);
> >  
> >  	intel_attach_gamma_mode_property(crtc);
> > +
> > +	if (INTEL_GEN(dev_priv) >= 11)
> > +		intel_attach_multi_segment_gamma_mode_property(crtc)
> > ;
> >  }
> > diff --git a/include/uapi/drm/i915_drm.h
> > b/include/uapi/drm/i915_drm.h
> > index aa2d4c7..8f1974e 100644
> > --- a/include/uapi/drm/i915_drm.h
> > +++ b/include/uapi/drm/i915_drm.h
> > @@ -1842,6 +1842,20 @@ struct drm_i915_query_topology_info {
> >  	__u8 data[];
> >  };
> >  
> > +/*
> > + * Structure for muti segmented gamma lut
> > + */
> > +struct multi_segment_gamma_lut {
> > +	/* Number of Lut Segments */
> > +	__u8 segment_cnt;
> > +	/* Precison of LUT entries in bits */
> > +	__u8 precision_bits;
> > +	/* Pointer having number of LUT elements in each segment */
> > +	__u32 *segment_lut_cnt_ptr;
> > +	/* Pointer to store exact lut values for each segment */
> > +	__u32 *segment_lut_ptr;
> > +};
> > 
> And perhaps a variation of this as description for all gamma mode
> types.

This is my old idea how to represent fancier LUTs:
https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af17904c21300ff95
https://github.com/vsyrjala/linux/commit/74ffa5d441702c53830f6d71bb687bb0ae5aa58f

Each distinct segment of the curve would be just described by
a fixed size range descriptor, and the entire blob would be made up
of however many of those are needed.

That way we don't even need any multi-segment uapi for setting up 
the LUT. The blob for that would just contain as many entries as the
LUT needs in that specific mode.

-- 
Ville Syrjälä
Intel
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

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

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

Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode

[Shankar, Uma]

>-----Original Message-----
>From: Syrjala, Ville
>Sent: Tuesday, March 19, 2019 10:29 PM
>To: Lankhorst, Maarten <maarten.lankhorst@intel.com>
>Cc: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org;
>Sharma, Shashank <shashank.sharma@intel.com>; Roper, Matthew D
><matthew.d.roper@intel.com>
>Subject: Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode
>
>On Tue, Mar 19, 2019 at 10:46:27AM +0200, Lankhorst, Maarten wrote:
>> tis 2019-03-19 klockan 14:00 +0530 skrev Uma Shankar:
>> > Multi Segment Gamma Mode is added in Gen11+ platforms.
>> > Added a property interface to enable that.
>> >
>> > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/i915_drv.h    |  1 +
>> >  drivers/gpu/drm/i915/intel_color.c | 23 +++++++++++++++++++++++
>> >  include/uapi/drm/i915_drm.h        | 14 ++++++++++++++
>> >  3 files changed, 38 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> > b/drivers/gpu/drm/i915/i915_drv.h index 02231ae..f20d418 100644
>> > --- a/drivers/gpu/drm/i915/i915_drv.h
>> > +++ b/drivers/gpu/drm/i915/i915_drv.h
>> > @@ -1736,6 +1736,7 @@ struct drm_i915_private {
>> >  	struct drm_property *force_audio_property;
>> >
>> >  	struct drm_property *gamma_mode_property;
>> > +	struct drm_property *multi_segment_gamma_mode_property;
>>
>> Seems to me both properties should be part of drm core?

Sure Maarten, we can move gamma_mode property to drm.

>>
>> >  	/* hda/i915 audio component */
>> >  	struct i915_audio_component *audio_component; diff --git
>> > a/drivers/gpu/drm/i915/intel_color.c
>> > b/drivers/gpu/drm/i915/intel_color.c
>> > index 9d43d19..399d63d 100644
>> > --- a/drivers/gpu/drm/i915/intel_color.c
>> > +++ b/drivers/gpu/drm/i915/intel_color.c
>> > @@ -149,6 +149,26 @@ static bool crtc_state_is_legacy_gamma(const
>> > struct intel_crtc_state *crtc_state
>> >  	drm_object_attach_property(&crtc->base.base, prop, 0);  }
>> >
>> > +void
>> > +intel_attach_multi_segment_gamma_mode_property(struct intel_crtc
>> > *crtc)
>> > +{
>> > +	struct drm_device *dev = crtc->base.dev;
>> > +	struct drm_i915_private *dev_priv = to_i915(dev);
>> > +	struct drm_property *prop;
>> > +
>> > +	prop = dev_priv->multi_segment_gamma_mode_property;
>> > +	if (!prop) {
>> > +		prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
>> > +					   "Multi-segment Gamma",
>> > 0);
>> > +		if (!prop)
>> > +			return;
>> > +
>> > +		dev_priv->multi_segment_gamma_mode_property = prop;
>> > +	}
>> > +
>> > +	drm_object_attach_property(&crtc->base.base, prop, 0); }
>> > +
>> >  /*
>> >   * When using limited range, multiply the matrix given by userspace
>> > by
>> >   * the matrix that we would use for the limited range.
>> > @@ -953,4 +973,7 @@ void intel_color_init(struct intel_crtc *crtc)
>> >  					   INTEL_INFO(dev_priv)-
>> > >color.gamma_lut_size);
>> >
>> >  	intel_attach_gamma_mode_property(crtc);
>> > +
>> > +	if (INTEL_GEN(dev_priv) >= 11)
>> > +		intel_attach_multi_segment_gamma_mode_property(crtc)
>> > ;
>> >  }
>> > diff --git a/include/uapi/drm/i915_drm.h
>> > b/include/uapi/drm/i915_drm.h index aa2d4c7..8f1974e 100644
>> > --- a/include/uapi/drm/i915_drm.h
>> > +++ b/include/uapi/drm/i915_drm.h
>> > @@ -1842,6 +1842,20 @@ struct drm_i915_query_topology_info {
>> >  	__u8 data[];
>> >  };
>> >
>> > +/*
>> > + * Structure for muti segmented gamma lut  */ struct
>> > +multi_segment_gamma_lut {
>> > +	/* Number of Lut Segments */
>> > +	__u8 segment_cnt;
>> > +	/* Precison of LUT entries in bits */
>> > +	__u8 precision_bits;
>> > +	/* Pointer having number of LUT elements in each segment */
>> > +	__u32 *segment_lut_cnt_ptr;
>> > +	/* Pointer to store exact lut values for each segment */
>> > +	__u32 *segment_lut_ptr;
>> > +};
>> >
>> And perhaps a variation of this as description for all gamma mode
>> types.
>
>This is my old idea how to represent fancier LUTs:
>https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af17904c2130
>0ff95
>https://github.com/vsyrjala/linux/commit/74ffa5d441702c53830f6d71bb687bb0ae5a
>a58f
>
>Each distinct segment of the curve would be just described by a fixed size range
>descriptor, and the entire blob would be made up of however many of those are
>needed.
>
>That way we don't even need any multi-segment uapi for setting up the LUT. The blob
>for that would just contain as many entries as the LUT needs in that specific mode.

Hi Ville,
This also sounds good.  What is your suggestion on this. Any recommendation on how to take this
forward.

Thanks & Regards,
Uma Shankar

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

Re: [RFC v1 1/7] drm/i915: Add gamma mode property

[Matt Roper]

On Tue, Mar 19, 2019 at 02:00:12PM +0530, Uma Shankar wrote:
> Gen platforms support multiple gamma modes, currently
> it's hard coded to operate only in 1 specific mode.
> This patch adds a property to make gamma mode programmable.
> User can select which mode should be used for a particular
> usecase or scenario.
> 
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>

I haven't reviewed the series in depth, but I'm a bit confused on how
userspace would approach using this property.  This seems to be exposing
hardware implementation details that I wouldn't expect userspace to need
to worry about (plus I don't think any of the property values here
convey any specific meaning to someone who hasn't read the Intel
bspec/PRM).  E.g., why does userspace care about "split gamma" when the
driver takes care of the programming details and userspace never sees
the actual way the registers are laid out and written?

My understanding is that what really matters is how many table entries
there are for userspace to fill in, what input range(s) they cover, and
how the bits of each table entry are interpreted.  I think we'd want to
handle this in a vendor-agnostic way in the DRM core if possible; most
of the display servers that get used these days are cross-platform and
probably won't want to add Intel-specific logic (or platform-specific
logic if we wind up with a different set of options on future Intel
platforms).

> ---
>  drivers/gpu/drm/i915/i915_drv.h    |  2 ++
>  drivers/gpu/drm/i915/intel_color.c | 46 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h   |  3 +++
>  3 files changed, 51 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c65c2e6..02231ae 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1735,6 +1735,8 @@ struct drm_i915_private {
>  	struct drm_property *broadcast_rgb_property;
>  	struct drm_property *force_audio_property;
>  
> +	struct drm_property *gamma_mode_property;
> +
>  	/* hda/i915 audio component */
>  	struct i915_audio_component *audio_component;
>  	bool audio_component_registered;
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 467fd1a..9d43d19 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -92,6 +92,19 @@
>  	0x0800, 0x0100, 0x0800,
>  };
>  
> +#define LEGACY_PALETTE_MODE_8BIT		BIT(0)
> +#define PRECISION_PALETTE_MODE_10BIT		BIT(1)
> +#define INTERPOLATED_GAMMA_MODE_12BIT		BIT(2)
> +#define MULTI_SEGMENTED_GAMMA_MODE_12BIT	BIT(3)
> +#define SPLIT_GAMMA_MODE_12BIT			BIT(4)
> +
> +#define INTEL_GAMMA_MODE_MASK (\
> +		LEGACY_PALETTE_MODE_8BIT | \
> +		PRECISION_PALETTE_MODE_10BIT | \
> +		INTERPOLATED_GAMMA_MODE_12BIT | \
> +		MULTI_SEGMENTED_GAMMA_MODE_12BIT | \
> +		BIT_SPLIT_GAMMA_MODE_12BIT)

Is the "BIT_" prefix on this last one a typo?  I assume this was
supposed to just be the SPLIT_GAMMA_MODE_12BIT defined above?

> +
>  static bool lut_is_legacy(const struct drm_property_blob *lut)
>  {
>  	return drm_color_lut_size(lut) == LEGACY_LUT_LENGTH;
> @@ -105,6 +118,37 @@ static bool crtc_state_is_legacy_gamma(const struct intel_crtc_state *crtc_state
>  		lut_is_legacy(crtc_state->base.gamma_lut);
>  }
>  
> +static const struct drm_prop_enum_list gamma_mode_supported[] = {
> +	{ LEGACY_PALETTE_MODE_8BIT, "8 Bit Legacy Palette Mode" },
> +	{ PRECISION_PALETTE_MODE_10BIT, "10 Bit Precision Palette Mode" },
> +	{ INTERPOLATED_GAMMA_MODE_12BIT, "12 Bit Interploated Gamma Mode" },
> +	{ MULTI_SEGMENTED_GAMMA_MODE_12BIT,
> +			"12 Bit Multi Segmented Gamma Mode" },
> +	{ SPLIT_GAMMA_MODE_12BIT, "12 Bit Split Gamma Mode" },
> +};
> +
> +void
> +intel_attach_gamma_mode_property(struct intel_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_property *prop;
> +
> +	prop = dev_priv->gamma_mode_property;
> +	if (!prop) {
> +		prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
> +						"Gamma Mode",
> +						gamma_mode_supported,
> +						ARRAY_SIZE(gamma_mode_supported));

If we do expose hardware-specific gamma modes like this, then I think
we'd want to create this property with a platform-specific list of
modes so that userspace doesn't even have the options for modes that
aren't supported on the platform they're running on.

> +		if (!prop)
> +			return;
> +
> +		dev_priv->gamma_mode_property = prop;
> +	}
> +
> +	drm_object_attach_property(&crtc->base.base, prop, 0);
> +}
> +
>  /*
>   * When using limited range, multiply the matrix given by userspace by
>   * the matrix that we would use for the limited range.
> @@ -907,4 +951,6 @@ void intel_color_init(struct intel_crtc *crtc)
>  					   INTEL_INFO(dev_priv)->color.degamma_lut_size,
>  					   true,
>  					   INTEL_INFO(dev_priv)->color.gamma_lut_size);
> +
> +	intel_attach_gamma_mode_property(crtc);

It looks like we're exposing the property to userspace in this patch,
but we don't finish wiring up the functionality until later patches in
the series; that's going to make things confusing if someone bisects
over this range of patches.  It would be best to hold off on exposing
new interfaces like this to userspace until the end of the
implementation when they're fully functional.


Matt

>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d9f188e..fd84fe9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1034,6 +1034,9 @@ struct intel_crtc_state {
>  	u8 nv12_planes;
>  	u8 c8_planes;
>  
> +	/* Gamma mode type programmed on the pipe */
> +	u32 gamma_mode_type;
> +
>  	/* bitmask of planes that will be updated during the commit */
>  	u8 update_planes;
>  
> -- 
> 1.9.1
> 

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

Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode

[Matt Roper]

On Wed, Mar 20, 2019 at 10:03:16AM -0700, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Syrjala, Ville
> >Sent: Tuesday, March 19, 2019 10:29 PM
> >To: Lankhorst, Maarten <maarten.lankhorst@intel.com>
> >Cc: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org;
> >Sharma, Shashank <shashank.sharma@intel.com>; Roper, Matthew D
> ><matthew.d.roper@intel.com>
> >Subject: Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode
> >
> >On Tue, Mar 19, 2019 at 10:46:27AM +0200, Lankhorst, Maarten wrote:
> >> tis 2019-03-19 klockan 14:00 +0530 skrev Uma Shankar:
> >> > Multi Segment Gamma Mode is added in Gen11+ platforms.
> >> > Added a property interface to enable that.
> >> >
> >> > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/i915_drv.h    |  1 +
> >> >  drivers/gpu/drm/i915/intel_color.c | 23 +++++++++++++++++++++++
> >> >  include/uapi/drm/i915_drm.h        | 14 ++++++++++++++
> >> >  3 files changed, 38 insertions(+)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> >> > b/drivers/gpu/drm/i915/i915_drv.h index 02231ae..f20d418 100644
> >> > --- a/drivers/gpu/drm/i915/i915_drv.h
> >> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> > @@ -1736,6 +1736,7 @@ struct drm_i915_private {
> >> >  	struct drm_property *force_audio_property;
> >> >
> >> >  	struct drm_property *gamma_mode_property;
> >> > +	struct drm_property *multi_segment_gamma_mode_property;
> >>
> >> Seems to me both properties should be part of drm core?
> 
> Sure Maarten, we can move gamma_mode property to drm.
> 
> >>
> >> >  	/* hda/i915 audio component */
> >> >  	struct i915_audio_component *audio_component; diff --git
> >> > a/drivers/gpu/drm/i915/intel_color.c
> >> > b/drivers/gpu/drm/i915/intel_color.c
> >> > index 9d43d19..399d63d 100644
> >> > --- a/drivers/gpu/drm/i915/intel_color.c
> >> > +++ b/drivers/gpu/drm/i915/intel_color.c
> >> > @@ -149,6 +149,26 @@ static bool crtc_state_is_legacy_gamma(const
> >> > struct intel_crtc_state *crtc_state
> >> >  	drm_object_attach_property(&crtc->base.base, prop, 0);  }
> >> >
> >> > +void
> >> > +intel_attach_multi_segment_gamma_mode_property(struct intel_crtc
> >> > *crtc)
> >> > +{
> >> > +	struct drm_device *dev = crtc->base.dev;
> >> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> >> > +	struct drm_property *prop;
> >> > +
> >> > +	prop = dev_priv->multi_segment_gamma_mode_property;
> >> > +	if (!prop) {
> >> > +		prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> >> > +					   "Multi-segment Gamma",
> >> > 0);
> >> > +		if (!prop)
> >> > +			return;
> >> > +
> >> > +		dev_priv->multi_segment_gamma_mode_property = prop;
> >> > +	}
> >> > +
> >> > +	drm_object_attach_property(&crtc->base.base, prop, 0); }
> >> > +
> >> >  /*
> >> >   * When using limited range, multiply the matrix given by userspace
> >> > by
> >> >   * the matrix that we would use for the limited range.
> >> > @@ -953,4 +973,7 @@ void intel_color_init(struct intel_crtc *crtc)
> >> >  					   INTEL_INFO(dev_priv)-
> >> > >color.gamma_lut_size);
> >> >
> >> >  	intel_attach_gamma_mode_property(crtc);
> >> > +
> >> > +	if (INTEL_GEN(dev_priv) >= 11)
> >> > +		intel_attach_multi_segment_gamma_mode_property(crtc)
> >> > ;
> >> >  }
> >> > diff --git a/include/uapi/drm/i915_drm.h
> >> > b/include/uapi/drm/i915_drm.h index aa2d4c7..8f1974e 100644
> >> > --- a/include/uapi/drm/i915_drm.h
> >> > +++ b/include/uapi/drm/i915_drm.h
> >> > @@ -1842,6 +1842,20 @@ struct drm_i915_query_topology_info {
> >> >  	__u8 data[];
> >> >  };
> >> >
> >> > +/*
> >> > + * Structure for muti segmented gamma lut  */ struct
> >> > +multi_segment_gamma_lut {
> >> > +	/* Number of Lut Segments */
> >> > +	__u8 segment_cnt;
> >> > +	/* Precison of LUT entries in bits */
> >> > +	__u8 precision_bits;
> >> > +	/* Pointer having number of LUT elements in each segment */
> >> > +	__u32 *segment_lut_cnt_ptr;
> >> > +	/* Pointer to store exact lut values for each segment */
> >> > +	__u32 *segment_lut_ptr;

I'm confused how this blob is supposed to be used.  It seems like the
pointers in here won't be meaningful across the userspace/kernel copy?
It also looks like 'segment_lut_ptr' doesn't appear again in this patch
series.

Like Ville describes below, I'd expect a property describing segmented
gamma to basically be a collection of structures that describe a given
range of the regular main LUT:  the start/end indices of the LUT that
hold the segment, how the bits of table entries in this segment should
be interpreted, etc.

Whatever we come up with, it will definitely be important to add some
kerneldoc that describes how the property is used/interpreted.



Matt

> >> > +};
> >> >
> >> And perhaps a variation of this as description for all gamma mode
> >> types.
> >
> >This is my old idea how to represent fancier LUTs:
> >https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af17904c2130
> >0ff95
> >https://github.com/vsyrjala/linux/commit/74ffa5d441702c53830f6d71bb687bb0ae5a
> >a58f
> >
> >Each distinct segment of the curve would be just described by a fixed size range
> >descriptor, and the entire blob would be made up of however many of those are
> >needed.
> >
> >That way we don't even need any multi-segment uapi for setting up the LUT. The blob
> >for that would just contain as many entries as the LUT needs in that specific mode.
> 
> Hi Ville,
> This also sounds good.  What is your suggestion on this. Any recommendation on how to take this
> forward.
> 
> Thanks & Regards,
> Uma Shankar
> 
> >--
> >Ville Syrjälä
> >Intel

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

Re: [RFC v1 7/7] drm/i915: Add multi segment gamma for icl

[Matt Roper]

On Tue, Mar 19, 2019 at 02:00:18PM +0530, Uma Shankar wrote:
> Added support for ICL platform multi segment gamma
> capabilties and attached the property, exposing the
> same to userspace.
> 
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_color.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 7733c256..1e9f784 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -1011,6 +1011,8 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
>  void intel_color_init(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	struct multi_segment_gamma_lut multi_segment_lut;
> +
>  
>  	drm_mode_crtc_set_gamma_size(&crtc->base, 256);
>  
> @@ -1049,6 +1051,24 @@ void intel_color_init(struct intel_crtc *crtc)
>  
>  	intel_attach_gamma_mode_property(crtc);
>  
> -	if (INTEL_GEN(dev_priv) >= 11)
> +	if (IS_ICELAKE(dev_priv)) {

Is it intentional to limit this specifically to Icelake?  This is
basically the opposite of the type of generalization that we've been
moving toward with patches like

        commit 2dd24a9c2c8d767a976da37d59680f09b9d111ab
        Author: Rodrigo Vivi <rodrigo.vivi@intel.com>
        Date:   Fri Mar 8 13:42:58 2019 -0800

            drm/i915/gen11+: First assume next platforms will inherit stuff

Also, we already support another gen11 platform (or will once the
mailing list patches land) in the form of EHL; is there any reason that
this shouldn't apply to it?

> +		multi_segment_lut.segment_cnt = 3;
> +		multi_segment_lut.precision_bits = 16;
> +		multi_segment_lut.segment_lut_cnt_ptr = kzalloc(3 * sizeof(int),
> +								GFP_KERNEL);
> +		if (!multi_segment_lut.segment_lut_cnt_ptr)
> +			return;
> +		multi_segment_lut.segment_lut_cnt_ptr[0] = 9;
> +		multi_segment_lut.segment_lut_cnt_ptr[1] = 256;
> +		multi_segment_lut.segment_lut_cnt_ptr[2] = 256;

On this patch and the previous one we have a few magic numbers
representing how the segmented gamma is laid out.  Can we move stuff
like this into the device info structure and then just setup the
appropriate userspace properties on any platform that has the device
info fields filled in (i.e., the same way we do the basic color
management properties)?


Matt

> +
>  		intel_attach_multi_segment_gamma_mode_property(crtc);
> +
> +		drm_property_replace_global_blob(crtc->base.dev,
> +						 &crtc->config->multi_segment_gamma_lut,
> +						 sizeof(struct multi_segment_gamma_lut),
> +						 &multi_segment_lut,
> +						 &crtc->base.base,
> +						 dev_priv->multi_segment_gamma_mode_property);
> +	}
>  }
> -- 
> 1.9.1
> 

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

Re: [RFC v1 1/7] drm/i915: Add gamma mode property

[Shankar, Uma]

>-----Original Message-----
>From: Roper, Matthew D
>Sent: Friday, March 22, 2019 2:50 AM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Lankhorst, Maarten
><maarten.lankhorst@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; Sharma,
>Shashank <shashank.sharma@intel.com>
>Subject: Re: [RFC v1 1/7] drm/i915: Add gamma mode property
>
>On Tue, Mar 19, 2019 at 02:00:12PM +0530, Uma Shankar wrote:
>> Gen platforms support multiple gamma modes, currently it's hard coded
>> to operate only in 1 specific mode.
>> This patch adds a property to make gamma mode programmable.
>> User can select which mode should be used for a particular usecase or
>> scenario.
>>
>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>
>I haven't reviewed the series in depth, but I'm a bit confused on how userspace would
>approach using this property.  This seems to be exposing hardware implementation
>details that I wouldn't expect userspace to need to worry about (plus I don't think any
>of the property values here convey any specific meaning to someone who hasn't read
>the Intel bspec/PRM).  E.g., why does userspace care about "split gamma" when the
>driver takes care of the programming details and userspace never sees the actual way
>the registers are laid out and written?
>My understanding is that what really matters is how many table entries there are for
>userspace to fill in, what input range(s) they cover, and how the bits of each table
>entry are interpreted.  I think we'd want to handle this in a vendor-agnostic way in the
>DRM core if possible; most of the display servers that get used these days are cross-
>platform and probably won't want to add Intel-specific logic (or platform-specific
>logic if we wind up with a different set of options on future Intel platforms).

Ok, will try to re-structure this to make it vendor agnostic way. Also will add enough
documentation for the usage of the property. 

@Ville- What do you recommend or suggest for these interfaces.

>> ---
>>  drivers/gpu/drm/i915/i915_drv.h    |  2 ++
>>  drivers/gpu/drm/i915/intel_color.c | 46
>++++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_drv.h   |  3 +++
>>  3 files changed, 51 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h index c65c2e6..02231ae 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1735,6 +1735,8 @@ struct drm_i915_private {
>>  	struct drm_property *broadcast_rgb_property;
>>  	struct drm_property *force_audio_property;
>>
>> +	struct drm_property *gamma_mode_property;
>> +
>>  	/* hda/i915 audio component */
>>  	struct i915_audio_component *audio_component;
>>  	bool audio_component_registered;
>> diff --git a/drivers/gpu/drm/i915/intel_color.c
>> b/drivers/gpu/drm/i915/intel_color.c
>> index 467fd1a..9d43d19 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -92,6 +92,19 @@
>>  	0x0800, 0x0100, 0x0800,
>>  };
>>
>> +#define LEGACY_PALETTE_MODE_8BIT		BIT(0)
>> +#define PRECISION_PALETTE_MODE_10BIT		BIT(1)
>> +#define INTERPOLATED_GAMMA_MODE_12BIT		BIT(2)
>> +#define MULTI_SEGMENTED_GAMMA_MODE_12BIT	BIT(3)
>> +#define SPLIT_GAMMA_MODE_12BIT			BIT(4)
>> +
>> +#define INTEL_GAMMA_MODE_MASK (\
>> +		LEGACY_PALETTE_MODE_8BIT | \
>> +		PRECISION_PALETTE_MODE_10BIT | \
>> +		INTERPOLATED_GAMMA_MODE_12BIT | \
>> +		MULTI_SEGMENTED_GAMMA_MODE_12BIT | \
>> +		BIT_SPLIT_GAMMA_MODE_12BIT)
>
>Is the "BIT_" prefix on this last one a typo?  I assume this was supposed to just be the
>SPLIT_GAMMA_MODE_12BIT defined above?

Yes, will fix this.

>> +
>>  static bool lut_is_legacy(const struct drm_property_blob *lut)  {
>>  	return drm_color_lut_size(lut) == LEGACY_LUT_LENGTH; @@ -105,6
>> +118,37 @@ static bool crtc_state_is_legacy_gamma(const struct intel_crtc_state
>*crtc_state
>>  		lut_is_legacy(crtc_state->base.gamma_lut);
>>  }
>>
>> +static const struct drm_prop_enum_list gamma_mode_supported[] = {
>> +	{ LEGACY_PALETTE_MODE_8BIT, "8 Bit Legacy Palette Mode" },
>> +	{ PRECISION_PALETTE_MODE_10BIT, "10 Bit Precision Palette Mode" },
>> +	{ INTERPOLATED_GAMMA_MODE_12BIT, "12 Bit Interploated Gamma
>Mode" },
>> +	{ MULTI_SEGMENTED_GAMMA_MODE_12BIT,
>> +			"12 Bit Multi Segmented Gamma Mode" },
>> +	{ SPLIT_GAMMA_MODE_12BIT, "12 Bit Split Gamma Mode" }, };
>> +
>> +void
>> +intel_attach_gamma_mode_property(struct intel_crtc *crtc) {
>> +	struct drm_device *dev = crtc->base.dev;
>> +	struct drm_i915_private *dev_priv = to_i915(dev);
>> +	struct drm_property *prop;
>> +
>> +	prop = dev_priv->gamma_mode_property;
>> +	if (!prop) {
>> +		prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
>> +						"Gamma Mode",
>> +						gamma_mode_supported,
>> +
>	ARRAY_SIZE(gamma_mode_supported));
>
>If we do expose hardware-specific gamma modes like this, then I think we'd want to
>create this property with a platform-specific list of modes so that userspace doesn't
>even have the options for modes that aren't supported on the platform they're
>running on.

Ok, will add the property enum based on platform  capabilities.

>> +		if (!prop)
>> +			return;
>> +
>> +		dev_priv->gamma_mode_property = prop;
>> +	}
>> +
>> +	drm_object_attach_property(&crtc->base.base, prop, 0); }
>> +
>>  /*
>>   * When using limited range, multiply the matrix given by userspace by
>>   * the matrix that we would use for the limited range.
>> @@ -907,4 +951,6 @@ void intel_color_init(struct intel_crtc *crtc)
>>  					   INTEL_INFO(dev_priv)-
>>color.degamma_lut_size,
>>  					   true,
>>  					   INTEL_INFO(dev_priv)-
>>color.gamma_lut_size);
>> +
>> +	intel_attach_gamma_mode_property(crtc);
>
>It looks like we're exposing the property to userspace in this patch, but we don't finish
>wiring up the functionality until later patches in the series; that's going to make things
>confusing if someone bisects over this range of patches.  It would be best to hold off
>on exposing new interfaces like this to userspace until the end of the implementation
>when they're fully functional.

Ok, will move the attach to a later patch when all the necessary ingredients are in place.

Regards,
Uma Shankar

>
>Matt
>
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index d9f188e..fd84fe9 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1034,6 +1034,9 @@ struct intel_crtc_state {
>>  	u8 nv12_planes;
>>  	u8 c8_planes;
>>
>> +	/* Gamma mode type programmed on the pipe */
>> +	u32 gamma_mode_type;
>> +
>>  	/* bitmask of planes that will be updated during the commit */
>>  	u8 update_planes;
>>
>> --
>> 1.9.1
>>
>
>--
>Matt Roper
>Graphics Software Engineer
>IoTG Platform Enabling & Development
>Intel Corporation
>(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode

[Shankar, Uma]

>-----Original Message-----
>From: Roper, Matthew D
>Sent: Friday, March 22, 2019 3:23 AM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
><maarten.lankhorst@intel.com>; intel-gfx@lists.freedesktop.org; Sharma, Shashank
><shashank.sharma@intel.com>
>Subject: Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode
>
>On Wed, Mar 20, 2019 at 10:03:16AM -0700, Shankar, Uma wrote:
>>
>>
>> >-----Original Message-----
>> >From: Syrjala, Ville
>> >Sent: Tuesday, March 19, 2019 10:29 PM
>> >To: Lankhorst, Maarten <maarten.lankhorst@intel.com>
>> >Cc: Shankar, Uma <uma.shankar@intel.com>;
>> >intel-gfx@lists.freedesktop.org; Sharma, Shashank
>> ><shashank.sharma@intel.com>; Roper, Matthew D
>> ><matthew.d.roper@intel.com>
>> >Subject: Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment
>> >Gamma Mode
>> >
>> >On Tue, Mar 19, 2019 at 10:46:27AM +0200, Lankhorst, Maarten wrote:
>> >> tis 2019-03-19 klockan 14:00 +0530 skrev Uma Shankar:
>> >> > Multi Segment Gamma Mode is added in Gen11+ platforms.
>> >> > Added a property interface to enable that.
>> >> >
>> >> > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> >> > ---
>> >> >  drivers/gpu/drm/i915/i915_drv.h    |  1 +
>> >> >  drivers/gpu/drm/i915/intel_color.c | 23 +++++++++++++++++++++++
>> >> >  include/uapi/drm/i915_drm.h        | 14 ++++++++++++++
>> >> >  3 files changed, 38 insertions(+)
>> >> >
>> >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> >> > b/drivers/gpu/drm/i915/i915_drv.h index 02231ae..f20d418 100644
>> >> > --- a/drivers/gpu/drm/i915/i915_drv.h
>> >> > +++ b/drivers/gpu/drm/i915/i915_drv.h
>> >> > @@ -1736,6 +1736,7 @@ struct drm_i915_private {
>> >> >  	struct drm_property *force_audio_property;
>> >> >
>> >> >  	struct drm_property *gamma_mode_property;
>> >> > +	struct drm_property *multi_segment_gamma_mode_property;
>> >>
>> >> Seems to me both properties should be part of drm core?
>>
>> Sure Maarten, we can move gamma_mode property to drm.
>>
>> >>
>> >> >  	/* hda/i915 audio component */
>> >> >  	struct i915_audio_component *audio_component; diff --git
>> >> > a/drivers/gpu/drm/i915/intel_color.c
>> >> > b/drivers/gpu/drm/i915/intel_color.c
>> >> > index 9d43d19..399d63d 100644
>> >> > --- a/drivers/gpu/drm/i915/intel_color.c
>> >> > +++ b/drivers/gpu/drm/i915/intel_color.c
>> >> > @@ -149,6 +149,26 @@ static bool crtc_state_is_legacy_gamma(const
>> >> > struct intel_crtc_state *crtc_state
>> >> >  	drm_object_attach_property(&crtc->base.base, prop, 0);  }
>> >> >
>> >> > +void
>> >> > +intel_attach_multi_segment_gamma_mode_property(struct intel_crtc
>> >> > *crtc)
>> >> > +{
>> >> > +	struct drm_device *dev = crtc->base.dev;
>> >> > +	struct drm_i915_private *dev_priv = to_i915(dev);
>> >> > +	struct drm_property *prop;
>> >> > +
>> >> > +	prop = dev_priv->multi_segment_gamma_mode_property;
>> >> > +	if (!prop) {
>> >> > +		prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
>> >> > +					   "Multi-segment Gamma",
>> >> > 0);
>> >> > +		if (!prop)
>> >> > +			return;
>> >> > +
>> >> > +		dev_priv->multi_segment_gamma_mode_property = prop;
>> >> > +	}
>> >> > +
>> >> > +	drm_object_attach_property(&crtc->base.base, prop, 0); }
>> >> > +
>> >> >  /*
>> >> >   * When using limited range, multiply the matrix given by
>> >> > userspace by
>> >> >   * the matrix that we would use for the limited range.
>> >> > @@ -953,4 +973,7 @@ void intel_color_init(struct intel_crtc *crtc)
>> >> >  					   INTEL_INFO(dev_priv)-
>> >> > >color.gamma_lut_size);
>> >> >
>> >> >  	intel_attach_gamma_mode_property(crtc);
>> >> > +
>> >> > +	if (INTEL_GEN(dev_priv) >= 11)
>> >> > +		intel_attach_multi_segment_gamma_mode_property(crtc)
>> >> > ;
>> >> >  }
>> >> > diff --git a/include/uapi/drm/i915_drm.h
>> >> > b/include/uapi/drm/i915_drm.h index aa2d4c7..8f1974e 100644
>> >> > --- a/include/uapi/drm/i915_drm.h
>> >> > +++ b/include/uapi/drm/i915_drm.h
>> >> > @@ -1842,6 +1842,20 @@ struct drm_i915_query_topology_info {
>> >> >  	__u8 data[];
>> >> >  };
>> >> >
>> >> > +/*
>> >> > + * Structure for muti segmented gamma lut  */ struct
>> >> > +multi_segment_gamma_lut {
>> >> > +	/* Number of Lut Segments */
>> >> > +	__u8 segment_cnt;
>> >> > +	/* Precison of LUT entries in bits */
>> >> > +	__u8 precision_bits;
>> >> > +	/* Pointer having number of LUT elements in each segment */
>> >> > +	__u32 *segment_lut_cnt_ptr;
>> >> > +	/* Pointer to store exact lut values for each segment */
>> >> > +	__u32 *segment_lut_ptr;
>
>I'm confused how this blob is supposed to be used.  It seems like the pointers in here
>won't be meaningful across the userspace/kernel copy?
>It also looks like 'segment_lut_ptr' doesn't appear again in this patch series.
>
>Like Ville describes below, I'd expect a property describing segmented gamma to
>basically be a collection of structures that describe a given range of the regular main
>LUT:  the start/end indices of the LUT that hold the segment, how the bits of table
>entries in this segment should be interpreted, etc.

Ok, will try to align to that approach and come up with the next version for this series.

>Whatever we come up with, it will definitely be important to add some kerneldoc that
>describes how the property is used/interpreted.

Sure, I will add detailed docs explaining the usage and expectation from userspace.

Thanks Matt for your comments.
	
Regards,
Uma Shankar

>
>
>Matt
>
>> >> > +};
>> >> >
>> >> And perhaps a variation of this as description for all gamma mode
>> >> types.
>> >
>> >This is my old idea how to represent fancier LUTs:
>> >https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af179
>> >04c2130
>> >0ff95
>> >https://github.com/vsyrjala/linux/commit/74ffa5d441702c53830f6d71bb68
>> >7bb0ae5a
>> >a58f
>> >
>> >Each distinct segment of the curve would be just described by a fixed
>> >size range descriptor, and the entire blob would be made up of
>> >however many of those are needed.
>> >
>> >That way we don't even need any multi-segment uapi for setting up the
>> >LUT. The blob for that would just contain as many entries as the LUT needs in that
>specific mode.
>>
>> Hi Ville,
>> This also sounds good.  What is your suggestion on this. Any
>> recommendation on how to take this forward.
>>
>> Thanks & Regards,
>> Uma Shankar
>>
>> >--
>> >Ville Syrjälä
>> >Intel
>
>--
>Matt Roper
>Graphics Software Engineer
>IoTG Platform Enabling & Development
>Intel Corporation
>(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC v1 7/7] drm/i915: Add multi segment gamma for icl

[Shankar, Uma]

>-----Original Message-----
>From: Roper, Matthew D
>Sent: Friday, March 22, 2019 3:34 AM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Lankhorst, Maarten
><maarten.lankhorst@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; Sharma,
>Shashank <shashank.sharma@intel.com>
>Subject: Re: [RFC v1 7/7] drm/i915: Add multi segment gamma for icl
>
>On Tue, Mar 19, 2019 at 02:00:18PM +0530, Uma Shankar wrote:
>> Added support for ICL platform multi segment gamma capabilties and
>> attached the property, exposing the same to userspace.
>>
>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_color.c | 22 +++++++++++++++++++++-
>>  1 file changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_color.c
>> b/drivers/gpu/drm/i915/intel_color.c
>> index 7733c256..1e9f784 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -1011,6 +1011,8 @@ int intel_color_check(struct intel_crtc_state
>> *crtc_state)  void intel_color_init(struct intel_crtc *crtc)  {
>>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> +	struct multi_segment_gamma_lut multi_segment_lut;
>> +
>>
>>  	drm_mode_crtc_set_gamma_size(&crtc->base, 256);
>>
>> @@ -1049,6 +1051,24 @@ void intel_color_init(struct intel_crtc *crtc)
>>
>>  	intel_attach_gamma_mode_property(crtc);
>>
>> -	if (INTEL_GEN(dev_priv) >= 11)
>> +	if (IS_ICELAKE(dev_priv)) {
>
>Is it intentional to limit this specifically to Icelake?  This is basically the opposite of the
>type of generalization that we've been moving toward with patches like
>
>        commit 2dd24a9c2c8d767a976da37d59680f09b9d111ab
>        Author: Rodrigo Vivi <rodrigo.vivi@intel.com>
>        Date:   Fri Mar 8 13:42:58 2019 -0800
>
>            drm/i915/gen11+: First assume next platforms will inherit stuff
>
>Also, we already support another gen11 platform (or will once the mailing list patches
>land) in the form of EHL; is there any reason that this shouldn't apply to it?

Yeah, it will/can-be re-used for future platforms with no/some slight changes. I will modify this
to align to our overall feature enabling plans in driver.

>
>> +		multi_segment_lut.segment_cnt = 3;
>> +		multi_segment_lut.precision_bits = 16;
>> +		multi_segment_lut.segment_lut_cnt_ptr = kzalloc(3 * sizeof(int),
>> +								GFP_KERNEL);
>> +		if (!multi_segment_lut.segment_lut_cnt_ptr)
>> +			return;
>> +		multi_segment_lut.segment_lut_cnt_ptr[0] = 9;
>> +		multi_segment_lut.segment_lut_cnt_ptr[1] = 256;
>> +		multi_segment_lut.segment_lut_cnt_ptr[2] = 256;
>
>On this patch and the previous one we have a few magic numbers representing how
>the segmented gamma is laid out.  Can we move stuff like this into the device info
>structure and then just setup the appropriate userspace properties on any platform
>that has the device info fields filled in (i.e., the same way we do the basic color
>management properties)?

Sure, I can move it to platform device info structures and make it transparent here.

Regards,
Uma Shankar
>
>Matt
>
>> +
>>  		intel_attach_multi_segment_gamma_mode_property(crtc);
>> +
>> +		drm_property_replace_global_blob(crtc->base.dev,
>> +						 &crtc->config-
>>multi_segment_gamma_lut,
>> +						 sizeof(struct
>multi_segment_gamma_lut),
>> +						 &multi_segment_lut,
>> +						 &crtc->base.base,
>> +						 dev_priv-
>>multi_segment_gamma_mode_property);
>> +	}
>>  }
>> --
>> 1.9.1
>>
>
>--
>Matt Roper
>Graphics Software Engineer
>IoTG Platform Enabling & Development
>Intel Corporation
>(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC v1 1/7] drm/i915: Add gamma mode property

[Brian Starkey]

Hi,

On Fri, Mar 22, 2019 at 01:06:01PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Roper, Matthew D
> >Sent: Friday, March 22, 2019 2:50 AM
> >To: Shankar, Uma <uma.shankar@intel.com>
> >Cc: intel-gfx@lists.freedesktop.org; Lankhorst, Maarten
> ><maarten.lankhorst@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; Sharma,
> >Shashank <shashank.sharma@intel.com>
> >Subject: Re: [RFC v1 1/7] drm/i915: Add gamma mode property
> >
> >On Tue, Mar 19, 2019 at 02:00:12PM +0530, Uma Shankar wrote:
> >> Gen platforms support multiple gamma modes, currently it's hard coded
> >> to operate only in 1 specific mode.
> >> This patch adds a property to make gamma mode programmable.
> >> User can select which mode should be used for a particular usecase or
> >> scenario.
> >>
> >> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> >
> >I haven't reviewed the series in depth, but I'm a bit confused on how userspace would
> >approach using this property.  This seems to be exposing hardware implementation
> >details that I wouldn't expect userspace to need to worry about (plus I don't think any
> >of the property values here convey any specific meaning to someone who hasn't read
> >the Intel bspec/PRM).  E.g., why does userspace care about "split gamma" when the
> >driver takes care of the programming details and userspace never sees the actual way
> >the registers are laid out and written?
> >My understanding is that what really matters is how many table entries there are for
> >userspace to fill in, what input range(s) they cover, and how the bits of each table
> >entry are interpreted.  I think we'd want to handle this in a vendor-agnostic way in the
> >DRM core if possible; most of the display servers that get used these days are cross-
> >platform and probably won't want to add Intel-specific logic (or platform-specific
> >logic if we wind up with a different set of options on future Intel platforms).
> 
> Ok, will try to re-structure this to make it vendor agnostic way. Also will add enough
> documentation for the usage of the property. 
> 
> @Ville- What do you recommend or suggest for these interfaces.

Just to add to the conversation - some of our HW has fixed LUTs, which
isn't really very well exposed by the current UAPI. We do it by having
the kernel driver just look at the userspace-provided blob, and it if
matches the fixed curve we accept it.

I thought one way to expose this would be to have kernel-created blobs
representing the fixed LUTs, which userspace could query to figure out
what LUTs were available.

That might not need to interact with the proposals here, but perhaps
if there's going to be some kind kind of gamma-mode enumeration, fixed
LUTs could be considered there, too.

Cheers,
-Brian

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

Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode

[Ville Syrjälä]

On Wed, Mar 20, 2019 at 05:03:16PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Syrjala, Ville
> >Sent: Tuesday, March 19, 2019 10:29 PM
> >To: Lankhorst, Maarten <maarten.lankhorst@intel.com>
> >Cc: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org;
> >Sharma, Shashank <shashank.sharma@intel.com>; Roper, Matthew D
> ><matthew.d.roper@intel.com>
> >Subject: Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode
> >
> >On Tue, Mar 19, 2019 at 10:46:27AM +0200, Lankhorst, Maarten wrote:
> >> tis 2019-03-19 klockan 14:00 +0530 skrev Uma Shankar:
> >> > Multi Segment Gamma Mode is added in Gen11+ platforms.
> >> > Added a property interface to enable that.
> >> >
> >> > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/i915_drv.h    |  1 +
> >> >  drivers/gpu/drm/i915/intel_color.c | 23 +++++++++++++++++++++++
> >> >  include/uapi/drm/i915_drm.h        | 14 ++++++++++++++
> >> >  3 files changed, 38 insertions(+)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> >> > b/drivers/gpu/drm/i915/i915_drv.h index 02231ae..f20d418 100644
> >> > --- a/drivers/gpu/drm/i915/i915_drv.h
> >> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> > @@ -1736,6 +1736,7 @@ struct drm_i915_private {
> >> >  	struct drm_property *force_audio_property;
> >> >
> >> >  	struct drm_property *gamma_mode_property;
> >> > +	struct drm_property *multi_segment_gamma_mode_property;
> >>
> >> Seems to me both properties should be part of drm core?
> 
> Sure Maarten, we can move gamma_mode property to drm.
> 
> >>
> >> >  	/* hda/i915 audio component */
> >> >  	struct i915_audio_component *audio_component; diff --git
> >> > a/drivers/gpu/drm/i915/intel_color.c
> >> > b/drivers/gpu/drm/i915/intel_color.c
> >> > index 9d43d19..399d63d 100644
> >> > --- a/drivers/gpu/drm/i915/intel_color.c
> >> > +++ b/drivers/gpu/drm/i915/intel_color.c
> >> > @@ -149,6 +149,26 @@ static bool crtc_state_is_legacy_gamma(const
> >> > struct intel_crtc_state *crtc_state
> >> >  	drm_object_attach_property(&crtc->base.base, prop, 0);  }
> >> >
> >> > +void
> >> > +intel_attach_multi_segment_gamma_mode_property(struct intel_crtc
> >> > *crtc)
> >> > +{
> >> > +	struct drm_device *dev = crtc->base.dev;
> >> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> >> > +	struct drm_property *prop;
> >> > +
> >> > +	prop = dev_priv->multi_segment_gamma_mode_property;
> >> > +	if (!prop) {
> >> > +		prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> >> > +					   "Multi-segment Gamma",
> >> > 0);
> >> > +		if (!prop)
> >> > +			return;
> >> > +
> >> > +		dev_priv->multi_segment_gamma_mode_property = prop;
> >> > +	}
> >> > +
> >> > +	drm_object_attach_property(&crtc->base.base, prop, 0); }
> >> > +
> >> >  /*
> >> >   * When using limited range, multiply the matrix given by userspace
> >> > by
> >> >   * the matrix that we would use for the limited range.
> >> > @@ -953,4 +973,7 @@ void intel_color_init(struct intel_crtc *crtc)
> >> >  					   INTEL_INFO(dev_priv)-
> >> > >color.gamma_lut_size);
> >> >
> >> >  	intel_attach_gamma_mode_property(crtc);
> >> > +
> >> > +	if (INTEL_GEN(dev_priv) >= 11)
> >> > +		intel_attach_multi_segment_gamma_mode_property(crtc)
> >> > ;
> >> >  }
> >> > diff --git a/include/uapi/drm/i915_drm.h
> >> > b/include/uapi/drm/i915_drm.h index aa2d4c7..8f1974e 100644
> >> > --- a/include/uapi/drm/i915_drm.h
> >> > +++ b/include/uapi/drm/i915_drm.h
> >> > @@ -1842,6 +1842,20 @@ struct drm_i915_query_topology_info {
> >> >  	__u8 data[];
> >> >  };
> >> >
> >> > +/*
> >> > + * Structure for muti segmented gamma lut  */ struct
> >> > +multi_segment_gamma_lut {
> >> > +	/* Number of Lut Segments */
> >> > +	__u8 segment_cnt;
> >> > +	/* Precison of LUT entries in bits */
> >> > +	__u8 precision_bits;
> >> > +	/* Pointer having number of LUT elements in each segment */
> >> > +	__u32 *segment_lut_cnt_ptr;
> >> > +	/* Pointer to store exact lut values for each segment */
> >> > +	__u32 *segment_lut_ptr;
> >> > +};
> >> >
> >> And perhaps a variation of this as description for all gamma mode
> >> types.
> >
> >This is my old idea how to represent fancier LUTs:
> >https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af17904c2130
> >0ff95
> >https://github.com/vsyrjala/linux/commit/74ffa5d441702c53830f6d71bb687bb0ae5a
> >a58f
> >
> >Each distinct segment of the curve would be just described by a fixed size range
> >descriptor, and the entire blob would be made up of however many of those are
> >needed.
> >
> >That way we don't even need any multi-segment uapi for setting up the LUT. The blob
> >for that would just contain as many entries as the LUT needs in that specific mode.
> 
> Hi Ville,
> This also sounds good.  What is your suggestion on this. Any recommendation on how to take this
> forward.

I think my design should be more or less feasible to use for
userspace. I didn't actually get as far as to write userspace
code for it, but my idea for x11 was something along these lines:

1) find a non-interpolated gamma mode with num_entries >= 1<<screen bpc
2) if none was found pick an interpolated gamma mode with
   input/output bpc >= screen bpc
3) fall back to whatever is left

That approach should work for i915 + intel ddx at least. In theory it
should be find for generic userspace too, but maybe it would need
a few extra tweaks.

Also we need fp16 to actually be able to test the interpolated 
modes fully. We now have that for icl, but I still need to post
my "fp16 for everything gen4+" followup series. And after that
we need to glue it all together in igt.

Anyways, I've not worked on this branch myself in a while because
I want to get the current gamma support fixed/cleaned up first.
I have at least one more series after the current one gets
reviewed. And after that we still have the current limited
range/yuv vs. ctm vs. degamma/gamma bugs to sort out.

So yeah, I'd prefer to make the current stuff sensible before
spending time adding fancier stuff on top. But in the meantime
if you want to pick up where I left off with this gamma mode
I'd be fine with that.

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

Re: [RFC v1 1/7] drm/i915: Add gamma mode property

[Ville Syrjälä]

On Fri, Mar 22, 2019 at 01:39:04PM +0000, Brian Starkey wrote:
> Hi,
> 
> On Fri, Mar 22, 2019 at 01:06:01PM +0000, Shankar, Uma wrote:
> > 
> > 
> > >-----Original Message-----
> > >From: Roper, Matthew D
> > >Sent: Friday, March 22, 2019 2:50 AM
> > >To: Shankar, Uma <uma.shankar@intel.com>
> > >Cc: intel-gfx@lists.freedesktop.org; Lankhorst, Maarten
> > ><maarten.lankhorst@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; Sharma,
> > >Shashank <shashank.sharma@intel.com>
> > >Subject: Re: [RFC v1 1/7] drm/i915: Add gamma mode property
> > >
> > >On Tue, Mar 19, 2019 at 02:00:12PM +0530, Uma Shankar wrote:
> > >> Gen platforms support multiple gamma modes, currently it's hard coded
> > >> to operate only in 1 specific mode.
> > >> This patch adds a property to make gamma mode programmable.
> > >> User can select which mode should be used for a particular usecase or
> > >> scenario.
> > >>
> > >> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> > >
> > >I haven't reviewed the series in depth, but I'm a bit confused on how userspace would
> > >approach using this property.  This seems to be exposing hardware implementation
> > >details that I wouldn't expect userspace to need to worry about (plus I don't think any
> > >of the property values here convey any specific meaning to someone who hasn't read
> > >the Intel bspec/PRM).  E.g., why does userspace care about "split gamma" when the
> > >driver takes care of the programming details and userspace never sees the actual way
> > >the registers are laid out and written?
> > >My understanding is that what really matters is how many table entries there are for
> > >userspace to fill in, what input range(s) they cover, and how the bits of each table
> > >entry are interpreted.  I think we'd want to handle this in a vendor-agnostic way in the
> > >DRM core if possible; most of the display servers that get used these days are cross-
> > >platform and probably won't want to add Intel-specific logic (or platform-specific
> > >logic if we wind up with a different set of options on future Intel platforms).
> > 
> > Ok, will try to re-structure this to make it vendor agnostic way. Also will add enough
> > documentation for the usage of the property. 
> > 
> > @Ville- What do you recommend or suggest for these interfaces.
> 
> Just to add to the conversation - some of our HW has fixed LUTs, which
> isn't really very well exposed by the current UAPI. We do it by having
> the kernel driver just look at the userspace-provided blob, and it if
> matches the fixed curve we accept it.

So you just have say "Use the sRGB curve" bit in some register etc.?

I think we should be able to integrate that somehow into my design:
https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af17904c21300ff95                                       

Eg. just add a new DRM_MODE_LUT_FIXED_CURVE flag, and then I suppose
just add another member into the struct to indicate which curve it
is. And we could embed the fixed blob ID in there as well. That way
userspace wouldn't even have to actually get the blob for the curve.

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

Re: [RFC v1 1/7] drm/i915: Add gamma mode property

[Brian Starkey]

On Fri, Mar 22, 2019 at 04:02:57PM +0200, Ville Syrjälä wrote:
> On Fri, Mar 22, 2019 at 01:39:04PM +0000, Brian Starkey wrote:
> > Hi,
> > 
> > On Fri, Mar 22, 2019 at 01:06:01PM +0000, Shankar, Uma wrote:
> > > 
> > > 
> > > >-----Original Message-----
> > > >From: Roper, Matthew D
> > > >Sent: Friday, March 22, 2019 2:50 AM
> > > >To: Shankar, Uma <uma.shankar@intel.com>
> > > >Cc: intel-gfx@lists.freedesktop.org; Lankhorst, Maarten
> > > ><maarten.lankhorst@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; Sharma,
> > > >Shashank <shashank.sharma@intel.com>
> > > >Subject: Re: [RFC v1 1/7] drm/i915: Add gamma mode property
> > > >
> > > >On Tue, Mar 19, 2019 at 02:00:12PM +0530, Uma Shankar wrote:
> > > >> Gen platforms support multiple gamma modes, currently it's hard coded
> > > >> to operate only in 1 specific mode.
> > > >> This patch adds a property to make gamma mode programmable.
> > > >> User can select which mode should be used for a particular usecase or
> > > >> scenario.
> > > >>
> > > >> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> > > >
> > > >I haven't reviewed the series in depth, but I'm a bit confused on how userspace would
> > > >approach using this property.  This seems to be exposing hardware implementation
> > > >details that I wouldn't expect userspace to need to worry about (plus I don't think any
> > > >of the property values here convey any specific meaning to someone who hasn't read
> > > >the Intel bspec/PRM).  E.g., why does userspace care about "split gamma" when the
> > > >driver takes care of the programming details and userspace never sees the actual way
> > > >the registers are laid out and written?
> > > >My understanding is that what really matters is how many table entries there are for
> > > >userspace to fill in, what input range(s) they cover, and how the bits of each table
> > > >entry are interpreted.  I think we'd want to handle this in a vendor-agnostic way in the
> > > >DRM core if possible; most of the display servers that get used these days are cross-
> > > >platform and probably won't want to add Intel-specific logic (or platform-specific
> > > >logic if we wind up with a different set of options on future Intel platforms).
> > > 
> > > Ok, will try to re-structure this to make it vendor agnostic way. Also will add enough
> > > documentation for the usage of the property. 
> > > 
> > > @Ville- What do you recommend or suggest for these interfaces.
> > 
> > Just to add to the conversation - some of our HW has fixed LUTs, which
> > isn't really very well exposed by the current UAPI. We do it by having
> > the kernel driver just look at the userspace-provided blob, and it if
> > matches the fixed curve we accept it.
> 
> So you just have say "Use the sRGB curve" bit in some register etc.?

Yep, precisely. In the future, maybe multiple fixed LUTs to choose
from.

> 
> I think we should be able to integrate that somehow into my design:
> https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af17904c21300ff95                                       
> 
> Eg. just add a new DRM_MODE_LUT_FIXED_CURVE flag, and then I suppose
> just add another member into the struct to indicate which curve it
> is. And we could embed the fixed blob ID in there as well. That way
> userspace wouldn't even have to actually get the blob for the curve.

Yeah that (ENUM | BLOB) API let's us be quite "rich" with the
interface, so it certainly could fit in there.

If I understand the code correctly, each value in the enum list is the
ID of a blob, and userspace can query that blob to figure out what the
entry means (instead of needing _really really long_ descriptive
strings).

I wonder if that property type in general is a little _too_ rich. I
worry that it would be easy to just defer loads of vendor-specific
detail into the blob, making the API look "generic" on the surface,
when really it's effectively a list of vendor-defined (void *)s.

...that said, in some cases vendor-defined (void *)s is what we might
want (e.g. scaler coefficient tables).

Still looks like a neat idea, perhaps just needs some policy.

Thanks,
-Brian

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

Re: [RFC v1 1/7] drm/i915: Add gamma mode property

[Ville Syrjälä]

On Fri, Mar 22, 2019 at 03:42:43PM +0000, Brian Starkey wrote:
> On Fri, Mar 22, 2019 at 04:02:57PM +0200, Ville Syrjälä wrote:
> > On Fri, Mar 22, 2019 at 01:39:04PM +0000, Brian Starkey wrote:
> > > Hi,
> > > 
> > > On Fri, Mar 22, 2019 at 01:06:01PM +0000, Shankar, Uma wrote:
> > > > 
> > > > 
> > > > >-----Original Message-----
> > > > >From: Roper, Matthew D
> > > > >Sent: Friday, March 22, 2019 2:50 AM
> > > > >To: Shankar, Uma <uma.shankar@intel.com>
> > > > >Cc: intel-gfx@lists.freedesktop.org; Lankhorst, Maarten
> > > > ><maarten.lankhorst@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; Sharma,
> > > > >Shashank <shashank.sharma@intel.com>
> > > > >Subject: Re: [RFC v1 1/7] drm/i915: Add gamma mode property
> > > > >
> > > > >On Tue, Mar 19, 2019 at 02:00:12PM +0530, Uma Shankar wrote:
> > > > >> Gen platforms support multiple gamma modes, currently it's hard coded
> > > > >> to operate only in 1 specific mode.
> > > > >> This patch adds a property to make gamma mode programmable.
> > > > >> User can select which mode should be used for a particular usecase or
> > > > >> scenario.
> > > > >>
> > > > >> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> > > > >
> > > > >I haven't reviewed the series in depth, but I'm a bit confused on how userspace would
> > > > >approach using this property.  This seems to be exposing hardware implementation
> > > > >details that I wouldn't expect userspace to need to worry about (plus I don't think any
> > > > >of the property values here convey any specific meaning to someone who hasn't read
> > > > >the Intel bspec/PRM).  E.g., why does userspace care about "split gamma" when the
> > > > >driver takes care of the programming details and userspace never sees the actual way
> > > > >the registers are laid out and written?
> > > > >My understanding is that what really matters is how many table entries there are for
> > > > >userspace to fill in, what input range(s) they cover, and how the bits of each table
> > > > >entry are interpreted.  I think we'd want to handle this in a vendor-agnostic way in the
> > > > >DRM core if possible; most of the display servers that get used these days are cross-
> > > > >platform and probably won't want to add Intel-specific logic (or platform-specific
> > > > >logic if we wind up with a different set of options on future Intel platforms).
> > > > 
> > > > Ok, will try to re-structure this to make it vendor agnostic way. Also will add enough
> > > > documentation for the usage of the property. 
> > > > 
> > > > @Ville- What do you recommend or suggest for these interfaces.
> > > 
> > > Just to add to the conversation - some of our HW has fixed LUTs, which
> > > isn't really very well exposed by the current UAPI. We do it by having
> > > the kernel driver just look at the userspace-provided blob, and it if
> > > matches the fixed curve we accept it.
> > 
> > So you just have say "Use the sRGB curve" bit in some register etc.?
> 
> Yep, precisely. In the future, maybe multiple fixed LUTs to choose
> from.
> 
> > 
> > I think we should be able to integrate that somehow into my design:
> > https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af17904c21300ff95                                       
> > 
> > Eg. just add a new DRM_MODE_LUT_FIXED_CURVE flag, and then I suppose
> > just add another member into the struct to indicate which curve it
> > is. And we could embed the fixed blob ID in there as well. That way
> > userspace wouldn't even have to actually get the blob for the curve.
> 
> Yeah that (ENUM | BLOB) API let's us be quite "rich" with the
> interface, so it certainly could fit in there.
> 
> If I understand the code correctly, each value in the enum list is the
> ID of a blob, and userspace can query that blob to figure out what the
> entry means (instead of needing _really really long_ descriptive
> strings).
> 
> I wonder if that property type in general is a little _too_ rich. I
> worry that it would be easy to just defer loads of vendor-specific
> detail into the blob, making the API look "generic" on the surface,
> when really it's effectively a list of vendor-defined (void *)s.
> 
> ...that said, in some cases vendor-defined (void *)s is what we might
> want (e.g. scaler coefficient tables).
> 
> Still looks like a neat idea, perhaps just needs some policy.

Yeah, I couldn't come up with anything better really. The options that
I see are as you say really long descriptive string, or we'd have to
update all userspace whenever a new enum value is added so that it
can decide what to do with it.

If we go with this idea, then I would definitely want to NAK any patch
that tries to abuse this for "we just need to expose these random
piles of hardware specific data".

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

Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode

[Shankar, Uma]

>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Friday, March 22, 2019 7:23 PM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
><maarten.lankhorst@intel.com>; intel-gfx@lists.freedesktop.org
>Subject: Re: [Intel-gfx] [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma
>Mode
>
>On Wed, Mar 20, 2019 at 05:03:16PM +0000, Shankar, Uma wrote:
>>
>>
>> >-----Original Message-----
>> >From: Syrjala, Ville
>> >Sent: Tuesday, March 19, 2019 10:29 PM
>> >To: Lankhorst, Maarten <maarten.lankhorst@intel.com>
>> >Cc: Shankar, Uma <uma.shankar@intel.com>;
>> >intel-gfx@lists.freedesktop.org; Sharma, Shashank
>> ><shashank.sharma@intel.com>; Roper, Matthew D
>> ><matthew.d.roper@intel.com>
>> >Subject: Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment
>> >Gamma Mode
>> >
>> >On Tue, Mar 19, 2019 at 10:46:27AM +0200, Lankhorst, Maarten wrote:
>> >> tis 2019-03-19 klockan 14:00 +0530 skrev Uma Shankar:
>> >> > Multi Segment Gamma Mode is added in Gen11+ platforms.
>> >> > Added a property interface to enable that.
>> >> >
>> >> > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> >> > ---
>> >> >  drivers/gpu/drm/i915/i915_drv.h    |  1 +
>> >> >  drivers/gpu/drm/i915/intel_color.c | 23 +++++++++++++++++++++++
>> >> >  include/uapi/drm/i915_drm.h        | 14 ++++++++++++++
>> >> >  3 files changed, 38 insertions(+)
>> >> >
>> >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> >> > b/drivers/gpu/drm/i915/i915_drv.h index 02231ae..f20d418 100644
>> >> > --- a/drivers/gpu/drm/i915/i915_drv.h
>> >> > +++ b/drivers/gpu/drm/i915/i915_drv.h
>> >> > @@ -1736,6 +1736,7 @@ struct drm_i915_private {
>> >> >  	struct drm_property *force_audio_property;
>> >> >
>> >> >  	struct drm_property *gamma_mode_property;
>> >> > +	struct drm_property *multi_segment_gamma_mode_property;
>> >>
>> >> Seems to me both properties should be part of drm core?
>>
>> Sure Maarten, we can move gamma_mode property to drm.
>>
>> >>
>> >> >  	/* hda/i915 audio component */
>> >> >  	struct i915_audio_component *audio_component; diff --git
>> >> > a/drivers/gpu/drm/i915/intel_color.c
>> >> > b/drivers/gpu/drm/i915/intel_color.c
>> >> > index 9d43d19..399d63d 100644
>> >> > --- a/drivers/gpu/drm/i915/intel_color.c
>> >> > +++ b/drivers/gpu/drm/i915/intel_color.c
>> >> > @@ -149,6 +149,26 @@ static bool crtc_state_is_legacy_gamma(const
>> >> > struct intel_crtc_state *crtc_state
>> >> >  	drm_object_attach_property(&crtc->base.base, prop, 0);  }
>> >> >
>> >> > +void
>> >> > +intel_attach_multi_segment_gamma_mode_property(struct intel_crtc
>> >> > *crtc)
>> >> > +{
>> >> > +	struct drm_device *dev = crtc->base.dev;
>> >> > +	struct drm_i915_private *dev_priv = to_i915(dev);
>> >> > +	struct drm_property *prop;
>> >> > +
>> >> > +	prop = dev_priv->multi_segment_gamma_mode_property;
>> >> > +	if (!prop) {
>> >> > +		prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
>> >> > +					   "Multi-segment Gamma",
>> >> > 0);
>> >> > +		if (!prop)
>> >> > +			return;
>> >> > +
>> >> > +		dev_priv->multi_segment_gamma_mode_property = prop;
>> >> > +	}
>> >> > +
>> >> > +	drm_object_attach_property(&crtc->base.base, prop, 0); }
>> >> > +
>> >> >  /*
>> >> >   * When using limited range, multiply the matrix given by
>> >> > userspace by
>> >> >   * the matrix that we would use for the limited range.
>> >> > @@ -953,4 +973,7 @@ void intel_color_init(struct intel_crtc *crtc)
>> >> >  					   INTEL_INFO(dev_priv)-
>> >> > >color.gamma_lut_size);
>> >> >
>> >> >  	intel_attach_gamma_mode_property(crtc);
>> >> > +
>> >> > +	if (INTEL_GEN(dev_priv) >= 11)
>> >> > +		intel_attach_multi_segment_gamma_mode_property(crtc)
>> >> > ;
>> >> >  }
>> >> > diff --git a/include/uapi/drm/i915_drm.h
>> >> > b/include/uapi/drm/i915_drm.h index aa2d4c7..8f1974e 100644
>> >> > --- a/include/uapi/drm/i915_drm.h
>> >> > +++ b/include/uapi/drm/i915_drm.h
>> >> > @@ -1842,6 +1842,20 @@ struct drm_i915_query_topology_info {
>> >> >  	__u8 data[];
>> >> >  };
>> >> >
>> >> > +/*
>> >> > + * Structure for muti segmented gamma lut  */ struct
>> >> > +multi_segment_gamma_lut {
>> >> > +	/* Number of Lut Segments */
>> >> > +	__u8 segment_cnt;
>> >> > +	/* Precison of LUT entries in bits */
>> >> > +	__u8 precision_bits;
>> >> > +	/* Pointer having number of LUT elements in each segment */
>> >> > +	__u32 *segment_lut_cnt_ptr;
>> >> > +	/* Pointer to store exact lut values for each segment */
>> >> > +	__u32 *segment_lut_ptr;
>> >> > +};
>> >> >
>> >> And perhaps a variation of this as description for all gamma mode
>> >> types.
>> >
>> >This is my old idea how to represent fancier LUTs:
>> >https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af179
>> >04c2130
>> >0ff95
>> >https://github.com/vsyrjala/linux/commit/74ffa5d441702c53830f6d71bb68
>> >7bb0ae5a
>> >a58f
>> >
>> >Each distinct segment of the curve would be just described by a fixed
>> >size range descriptor, and the entire blob would be made up of
>> >however many of those are needed.
>> >
>> >That way we don't even need any multi-segment uapi for setting up the
>> >LUT. The blob for that would just contain as many entries as the LUT needs in that
>specific mode.
>>
>> Hi Ville,
>> This also sounds good.  What is your suggestion on this. Any
>> recommendation on how to take this forward.
>
>I think my design should be more or less feasible to use for userspace. I didn't actually
>get as far as to write userspace code for it, but my idea for x11 was something along
>these lines:
>
>1) find a non-interpolated gamma mode with num_entries >= 1<<screen bpc
>2) if none was found pick an interpolated gamma mode with
>   input/output bpc >= screen bpc
>3) fall back to whatever is left
>
>That approach should work for i915 + intel ddx at least. In theory it should be find for
>generic userspace too, but maybe it would need a few extra tweaks.
>
>Also we need fp16 to actually be able to test the interpolated modes fully. We now
>have that for icl, but I still need to post my "fp16 for everything gen4+" followup
>series. And after that we need to glue it all together in igt.
>
>Anyways, I've not worked on this branch myself in a while because I want to get the
>current gamma support fixed/cleaned up first.
>I have at least one more series after the current one gets reviewed. And after that we
>still have the current limited range/yuv vs. ctm vs. degamma/gamma bugs to sort out.
>
>So yeah, I'd prefer to make the current stuff sensible before spending time adding
>fancier stuff on top. But in the meantime if you want to pick up where I left off with
>this gamma mode I'd be fine with that.

Hi Ville,
Thanks for sharing these details and also the design what you have is really nice.

I was trying to build on top of it and trying to check the interface before floating the next
version. There seems to be a limitation in creating a property with both BLOB and ENUM
flags.

/* Only one legacy type at a time please */
        if (legacy_type && !is_power_of_2(legacy_type))
                return false;

Any suggestion on how to deal with this limitation.

Thanks & Regards,
Uma Shankar

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

Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode

[Ville Syrjälä]

On Thu, Mar 28, 2019 at 07:00:30PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >Sent: Friday, March 22, 2019 7:23 PM
> >To: Shankar, Uma <uma.shankar@intel.com>
> >Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
> ><maarten.lankhorst@intel.com>; intel-gfx@lists.freedesktop.org
> >Subject: Re: [Intel-gfx] [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma
> >Mode
> >
> >On Wed, Mar 20, 2019 at 05:03:16PM +0000, Shankar, Uma wrote:
> >>
> >>
> >> >-----Original Message-----
> >> >From: Syrjala, Ville
> >> >Sent: Tuesday, March 19, 2019 10:29 PM
> >> >To: Lankhorst, Maarten <maarten.lankhorst@intel.com>
> >> >Cc: Shankar, Uma <uma.shankar@intel.com>;
> >> >intel-gfx@lists.freedesktop.org; Sharma, Shashank
> >> ><shashank.sharma@intel.com>; Roper, Matthew D
> >> ><matthew.d.roper@intel.com>
> >> >Subject: Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment
> >> >Gamma Mode
> >> >
> >> >On Tue, Mar 19, 2019 at 10:46:27AM +0200, Lankhorst, Maarten wrote:
> >> >> tis 2019-03-19 klockan 14:00 +0530 skrev Uma Shankar:
> >> >> > Multi Segment Gamma Mode is added in Gen11+ platforms.
> >> >> > Added a property interface to enable that.
> >> >> >
> >> >> > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> >> >> > ---
> >> >> >  drivers/gpu/drm/i915/i915_drv.h    |  1 +
> >> >> >  drivers/gpu/drm/i915/intel_color.c | 23 +++++++++++++++++++++++
> >> >> >  include/uapi/drm/i915_drm.h        | 14 ++++++++++++++
> >> >> >  3 files changed, 38 insertions(+)
> >> >> >
> >> >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> >> >> > b/drivers/gpu/drm/i915/i915_drv.h index 02231ae..f20d418 100644
> >> >> > --- a/drivers/gpu/drm/i915/i915_drv.h
> >> >> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> >> > @@ -1736,6 +1736,7 @@ struct drm_i915_private {
> >> >> >  	struct drm_property *force_audio_property;
> >> >> >
> >> >> >  	struct drm_property *gamma_mode_property;
> >> >> > +	struct drm_property *multi_segment_gamma_mode_property;
> >> >>
> >> >> Seems to me both properties should be part of drm core?
> >>
> >> Sure Maarten, we can move gamma_mode property to drm.
> >>
> >> >>
> >> >> >  	/* hda/i915 audio component */
> >> >> >  	struct i915_audio_component *audio_component; diff --git
> >> >> > a/drivers/gpu/drm/i915/intel_color.c
> >> >> > b/drivers/gpu/drm/i915/intel_color.c
> >> >> > index 9d43d19..399d63d 100644
> >> >> > --- a/drivers/gpu/drm/i915/intel_color.c
> >> >> > +++ b/drivers/gpu/drm/i915/intel_color.c
> >> >> > @@ -149,6 +149,26 @@ static bool crtc_state_is_legacy_gamma(const
> >> >> > struct intel_crtc_state *crtc_state
> >> >> >  	drm_object_attach_property(&crtc->base.base, prop, 0);  }
> >> >> >
> >> >> > +void
> >> >> > +intel_attach_multi_segment_gamma_mode_property(struct intel_crtc
> >> >> > *crtc)
> >> >> > +{
> >> >> > +	struct drm_device *dev = crtc->base.dev;
> >> >> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> >> >> > +	struct drm_property *prop;
> >> >> > +
> >> >> > +	prop = dev_priv->multi_segment_gamma_mode_property;
> >> >> > +	if (!prop) {
> >> >> > +		prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> >> >> > +					   "Multi-segment Gamma",
> >> >> > 0);
> >> >> > +		if (!prop)
> >> >> > +			return;
> >> >> > +
> >> >> > +		dev_priv->multi_segment_gamma_mode_property = prop;
> >> >> > +	}
> >> >> > +
> >> >> > +	drm_object_attach_property(&crtc->base.base, prop, 0); }
> >> >> > +
> >> >> >  /*
> >> >> >   * When using limited range, multiply the matrix given by
> >> >> > userspace by
> >> >> >   * the matrix that we would use for the limited range.
> >> >> > @@ -953,4 +973,7 @@ void intel_color_init(struct intel_crtc *crtc)
> >> >> >  					   INTEL_INFO(dev_priv)-
> >> >> > >color.gamma_lut_size);
> >> >> >
> >> >> >  	intel_attach_gamma_mode_property(crtc);
> >> >> > +
> >> >> > +	if (INTEL_GEN(dev_priv) >= 11)
> >> >> > +		intel_attach_multi_segment_gamma_mode_property(crtc)
> >> >> > ;
> >> >> >  }
> >> >> > diff --git a/include/uapi/drm/i915_drm.h
> >> >> > b/include/uapi/drm/i915_drm.h index aa2d4c7..8f1974e 100644
> >> >> > --- a/include/uapi/drm/i915_drm.h
> >> >> > +++ b/include/uapi/drm/i915_drm.h
> >> >> > @@ -1842,6 +1842,20 @@ struct drm_i915_query_topology_info {
> >> >> >  	__u8 data[];
> >> >> >  };
> >> >> >
> >> >> > +/*
> >> >> > + * Structure for muti segmented gamma lut  */ struct
> >> >> > +multi_segment_gamma_lut {
> >> >> > +	/* Number of Lut Segments */
> >> >> > +	__u8 segment_cnt;
> >> >> > +	/* Precison of LUT entries in bits */
> >> >> > +	__u8 precision_bits;
> >> >> > +	/* Pointer having number of LUT elements in each segment */
> >> >> > +	__u32 *segment_lut_cnt_ptr;
> >> >> > +	/* Pointer to store exact lut values for each segment */
> >> >> > +	__u32 *segment_lut_ptr;
> >> >> > +};
> >> >> >
> >> >> And perhaps a variation of this as description for all gamma mode
> >> >> types.
> >> >
> >> >This is my old idea how to represent fancier LUTs:
> >> >https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af179
> >> >04c2130
> >> >0ff95
> >> >https://github.com/vsyrjala/linux/commit/74ffa5d441702c53830f6d71bb68
> >> >7bb0ae5a
> >> >a58f
> >> >
> >> >Each distinct segment of the curve would be just described by a fixed
> >> >size range descriptor, and the entire blob would be made up of
> >> >however many of those are needed.
> >> >
> >> >That way we don't even need any multi-segment uapi for setting up the
> >> >LUT. The blob for that would just contain as many entries as the LUT needs in that
> >specific mode.
> >>
> >> Hi Ville,
> >> This also sounds good.  What is your suggestion on this. Any
> >> recommendation on how to take this forward.
> >
> >I think my design should be more or less feasible to use for userspace. I didn't actually
> >get as far as to write userspace code for it, but my idea for x11 was something along
> >these lines:
> >
> >1) find a non-interpolated gamma mode with num_entries >= 1<<screen bpc
> >2) if none was found pick an interpolated gamma mode with
> >   input/output bpc >= screen bpc
> >3) fall back to whatever is left
> >
> >That approach should work for i915 + intel ddx at least. In theory it should be find for
> >generic userspace too, but maybe it would need a few extra tweaks.
> >
> >Also we need fp16 to actually be able to test the interpolated modes fully. We now
> >have that for icl, but I still need to post my "fp16 for everything gen4+" followup
> >series. And after that we need to glue it all together in igt.
> >
> >Anyways, I've not worked on this branch myself in a while because I want to get the
> >current gamma support fixed/cleaned up first.
> >I have at least one more series after the current one gets reviewed. And after that we
> >still have the current limited range/yuv vs. ctm vs. degamma/gamma bugs to sort out.
> >
> >So yeah, I'd prefer to make the current stuff sensible before spending time adding
> >fancier stuff on top. But in the meantime if you want to pick up where I left off with
> >this gamma mode I'd be fine with that.
> 
> Hi Ville,
> Thanks for sharing these details and also the design what you have is really nice.
> 
> I was trying to build on top of it and trying to check the interface before floating the next
> version. There seems to be a limitation in creating a property with both BLOB and ENUM
> flags.
> 
> /* Only one legacy type at a time please */
>         if (legacy_type && !is_power_of_2(legacy_type))
>                 return false;
> 
> Any suggestion on how to deal with this limitation.

I think we should just define a new BLOB_ENUM type. Trying to mix two
old types like this could end in explosions with existing userspace so
safer to make it a totally distinct type.

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