[v2 0/7] Add Multi Segment Gamma Support

[v2 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 multi segment gamma mode introduced in ICL+

It creates 2 property interfaces :
1. GAMMA_MODE_CAPS: This is immutable property and exposes
the various gamma modes supported and the lut ranges. This
is an enum property with element as blob id. Getting the
blob id in userspace, user can get the mode supported and
also the range of gamma mode supported with number of lut
coefficients.

2. GAMMA_MODE: This is for user to set the gamma mode and
send the lut values for that particular mode.

v2: Used Ville's design and approach to define the interfaces.
Addressed Matt Roper's review feedback and re-ordered the
patches.

Uma Shankar (5):
  drm: Add gamma mode property
  drm/i915/icl: Add register definitions for Multi Segmented gamma
  drm/i915/icl: Add support for multi segmented gamma mode
  drm/i915: Add gamma mode caps property
  drm/i915: Attach gamma mode property

Ville Syrjälä (2):
  drm: Add gamma mode caps property
  drm/i915: Define color lut range structure

 drivers/gpu/drm/drm_atomic_uapi.c    |  13 +
 drivers/gpu/drm/drm_color_mgmt.c     | 110 +++++++++
 drivers/gpu/drm/i915/i915_reg.h      |  17 ++
 drivers/gpu/drm/i915/intel_color.c   | 465 ++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_display.c |   5 +
 include/drm/drm_color_mgmt.h         |  11 +
 include/drm/drm_crtc.h               |  17 ++
 include/drm/drm_mode_config.h        |  10 +
 include/uapi/drm/drm_mode.h          |  49 ++++
 9 files changed, 690 insertions(+), 7 deletions(-)

-- 
1.9.1

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

[v2 1/7] drm: Add gamma mode caps property

[Uma Shankar]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add a gamma mode capability property to enable various kind of
gamma modes supported by platforms like: Interpolated, Split,
Multi Segmented etc. Userspace can get this property and
should be able to get the platform capabilties wrt various
gamma modes possible and the possible ranges.

It can then create the LUT and send it to driver using
another gamma mode property as blob.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c |  3 ++
 drivers/gpu/drm/drm_color_mgmt.c  | 78 +++++++++++++++++++++++++++++++++++++++
 include/drm/drm_color_mgmt.h      |  8 ++++
 include/drm/drm_crtc.h            |  7 ++++
 include/drm/drm_mode_config.h     |  5 +++
 include/uapi/drm/drm_mode.h       | 38 +++++++++++++++++++
 6 files changed, 139 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index ea797d4..03df2a4 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -495,6 +495,9 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 		*val = (state->mode_blob) ? state->mode_blob->base.id : 0;
 	else if (property == config->prop_vrr_enabled)
 		*val = state->vrr_enabled;
+	else if (property == config->gamma_mode_caps_property)
+		*val = (state->gamma_mode_caps) ?
+			state->gamma_mode_caps->base.id : 0;
 	else if (property == config->degamma_lut_property)
 		*val = (state->degamma_lut) ? state->degamma_lut->base.id : 0;
 	else if (property == config->ctm_property)
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index d5d34d0..054f0ed 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -176,6 +176,84 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
 }
 EXPORT_SYMBOL(drm_crtc_enable_color_mgmt);
 
+void drm_crtc_attach_gamma_mode_caps_property(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_mode_config *config = &dev->mode_config;
+
+	if (!config->gamma_mode_caps_property)
+		return;
+
+	drm_object_attach_property(&crtc->base,
+				   config->gamma_mode_caps_property, 0);
+}
+EXPORT_SYMBOL(drm_crtc_attach_gamma_mode_caps_property);
+
+int drm_color_create_gamma_mode_caps_property(struct drm_device *dev,
+					      int num_values)
+{
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_property *prop;
+
+	prop = drm_property_create(dev,
+				   DRM_MODE_PROP_ENUM |
+				   DRM_MODE_PROP_IMMUTABLE,
+				   "GAMMA_MODE_CAPS", num_values);
+	if (!prop)
+		return -ENOMEM;
+
+	config->gamma_mode_caps_property = prop;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_color_create_gamma_mode_caps_property);
+
+int drm_color_add_gamma_mode_range(struct drm_device *dev,
+				   const char *name,
+				   const struct drm_color_lut_range *ranges,
+				   size_t length)
+{
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_property_blob *blob;
+	struct drm_property *prop;
+	int num_ranges = length / sizeof(ranges[0]);
+	int i, ret, num_types_0;
+
+	if (WARN_ON(length == 0 || length % sizeof(ranges[0]) != 0))
+		return -EINVAL;
+
+	num_types_0 = hweight8(ranges[0].flags & (DRM_MODE_LUT_GAMMA |
+						  DRM_MODE_LUT_DEGAMMA));
+	if (num_types_0 == 0)
+		return -EINVAL;
+
+	for (i = 1; i < num_ranges; i++) {
+		int num_types = hweight8(ranges[i].flags & (DRM_MODE_LUT_GAMMA |
+							    DRM_MODE_LUT_DEGAMMA));
+
+		/* either all ranges have DEGAMMA|GAMMA or none have it */
+		if (num_types_0 != num_types)
+			return -EINVAL;
+	}
+
+	prop = config->gamma_mode_caps_property;
+	if (!prop)
+		return -EINVAL;
+
+	blob = drm_property_create_blob(dev, length, ranges);
+	if (IS_ERR(blob))
+		return PTR_ERR(blob);
+
+	ret = drm_property_add_enum(prop, blob->base.id, name);
+	if (ret) {
+		drm_property_blob_put(blob);
+		return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_color_add_gamma_mode_range);
+
 /**
  * drm_mode_crtc_set_gamma_size - set the gamma table size
  * @crtc: CRTC to set the gamma table size for
diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
index d1c662d..e0f94db 100644
--- a/include/drm/drm_color_mgmt.h
+++ b/include/drm/drm_color_mgmt.h
@@ -51,6 +51,14 @@ static inline int drm_color_lut_size(const struct drm_property_blob *blob)
 	return blob->length / sizeof(struct drm_color_lut);
 }
 
+int drm_color_create_gamma_mode_caps_property(struct drm_device *dev,
+					      int num_values);
+void drm_crtc_attach_gamma_mode_caps_property(struct drm_crtc *crtc);
+int drm_color_add_gamma_mode_range(struct drm_device *dev,
+				   const char *name,
+				   const struct drm_color_lut_range *ranges,
+				   size_t length);
+
 enum drm_color_encoding {
 	DRM_COLOR_YCBCR_BT601,
 	DRM_COLOR_YCBCR_BT709,
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 58ad983..cdfda90 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -249,6 +249,13 @@ struct drm_crtc_state {
 	struct drm_property_blob *mode_blob;
 
 	/**
+	 * @gamma_mode:
+	 *
+	 * FIXME
+	 */
+	struct drm_property_blob *gamma_mode_caps;
+
+	/**
 	 * @degamma_lut:
 	 *
 	 * Lookup table for converting framebuffer pixel data before apply the
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 7f60e8e..7b20355 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -761,6 +761,11 @@ struct drm_mode_config {
 	 */
 	struct drm_property *content_type_property;
 	/**
+	 * @gamma_mode_property: Optional CRTC property to enumerate and
+	 * select the mode of the crtc gamma/degmama LUTs.
+	 */
+	struct drm_property *gamma_mode_caps_property;
+	/**
 	 * @degamma_lut_property: Optional CRTC property to set the LUT used to
 	 * convert the framebuffer's colors to linear gamma.
 	 */
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 09d7296..e475f4a 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -629,6 +629,44 @@ struct drm_color_lut {
 	__u16 reserved;
 };
 
+/*
+ * DRM_MODE_LUT_GAMMA|DRM_MODE_LUT_DEGAMMA is legal and means the LUT
+ * can be used for either purpose, but not simultaneously. To expose
+ * modes that support gamma and degamma simultaneously the gamma mode
+ * must declare distinct DRM_MODE_LUT_GAMMA and DRM_MODE_LUT_DEGAMMA
+ * ranges.
+ */
+/* LUT is for gamma (after CTM) */
+#define DRM_MODE_LUT_GAMMA (1 << 0)
+/* LUT is for degamma (before CTM) */
+#define DRM_MODE_LUT_DEGAMMA (1 << 1)
+/* linearly interpolate between the points */
+#define DRM_MODE_LUT_INTERPOLATE (1 << 2)
+/*
+ * the last value of the previous range is the
+ * first value of the current range.
+ */
+#define DRM_MODE_LUT_REUSE_LAST (1 << 3)
+/* the curve must be non-decreasing */
+#define DRM_MODE_LUT_NON_DECREASING (1 << 4)
+/* the curve is reflected across origin for negative inputs */
+#define DRM_MODE_LUT_REFLECT_NEGATIVE (1 << 5)
+/* the same curve (red) is used for blue and green channels as well */
+#define DRM_MODE_LUT_SINGLE_CHANNEL (1 << 6)
+
+struct drm_color_lut_range {
+	/* DRM_MODE_LUT_* */
+	__u32 flags;
+	/* number of points on the curve */
+	__u16 count;
+	/* input/output bits per component */
+	__u8 input_bpc, output_bpc;
+	/* input start/end values */
+	__s32 start, end;
+	/* output min/max values */
+	__s32 min, max;
+};
+
 #define DRM_MODE_PAGE_FLIP_EVENT 0x01
 #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
 #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
-- 
1.9.1

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

Re: [v2 1/7] drm: Add gamma mode caps property

[Sam Ravnborg]

Hi Uma.

> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 58ad983..cdfda90 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -249,6 +249,13 @@ struct drm_crtc_state {
>  	struct drm_property_blob *mode_blob;
>  
>  	/**
> +	 * @gamma_mode:
> +	 *
> +	 * FIXME
> +	 */
> +	struct drm_property_blob *gamma_mode_caps;

This looks wrong as "gamma_mode" != "gamma_mode_caps".
Could you fix it, and also add some documentation to help
the next reader.

> +/*
> + * DRM_MODE_LUT_GAMMA|DRM_MODE_LUT_DEGAMMA is legal and means the LUT
> + * can be used for either purpose, but not simultaneously. To expose
> + * modes that support gamma and degamma simultaneously the gamma mode
> + * must declare distinct DRM_MODE_LUT_GAMMA and DRM_MODE_LUT_DEGAMMA
> + * ranges.
> + */
> +/* LUT is for gamma (after CTM) */
> +#define DRM_MODE_LUT_GAMMA (1 << 0)
> +/* LUT is for degamma (before CTM) */
> +#define DRM_MODE_LUT_DEGAMMA (1 << 1)
> +/* linearly interpolate between the points */
> +#define DRM_MODE_LUT_INTERPOLATE (1 << 2)

Use BIT(2), and same for the other bits above and below?

> +/*
> + * the last value of the previous range is the
> + * first value of the current range.
> + */
> +#define DRM_MODE_LUT_REUSE_LAST (1 << 3)
> +/* the curve must be non-decreasing */
> +#define DRM_MODE_LUT_NON_DECREASING (1 << 4)
> +/* the curve is reflected across origin for negative inputs */
> +#define DRM_MODE_LUT_REFLECT_NEGATIVE (1 << 5)
> +/* the same curve (red) is used for blue and green channels as well */
> +#define DRM_MODE_LUT_SINGLE_CHANNEL (1 << 6)

	Sam

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

Re: [v2 1/7] drm: Add gamma mode caps property

[Shankar, Uma]

>-----Original Message-----
>From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of Sam
>Ravnborg
>Sent: Tuesday, April 2, 2019 12:03 AM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org;
>emil.l.velikov@gmail.com; dri-devel@lists.freedesktop.org;
>seanpaul@chromium.org; Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
><maarten.lankhorst@intel.com>
>Subject: Re: [v2 1/7] drm: Add gamma mode caps property
>
>Hi Uma.
>
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index
>> 58ad983..cdfda90 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -249,6 +249,13 @@ struct drm_crtc_state {
>>  	struct drm_property_blob *mode_blob;
>>
>>  	/**
>> +	 * @gamma_mode:
>> +	 *
>> +	 * FIXME
>> +	 */
>> +	struct drm_property_blob *gamma_mode_caps;
>
>This looks wrong as "gamma_mode" != "gamma_mode_caps".
>Could you fix it, and also add some documentation to help the next reader.

Hi Sam,
Thanks for the review.

Sure will update the naming and add more documentation once we have the
design agreement on this. Idea is to converge to just one property.

Regards,
Uma Shankar


>
>> +/*
>> + * DRM_MODE_LUT_GAMMA|DRM_MODE_LUT_DEGAMMA is legal and means
>the LUT
>> + * can be used for either purpose, but not simultaneously. To expose
>> + * modes that support gamma and degamma simultaneously the gamma mode
>> + * must declare distinct DRM_MODE_LUT_GAMMA and
>DRM_MODE_LUT_DEGAMMA
>> + * ranges.
>> + */
>> +/* LUT is for gamma (after CTM) */
>> +#define DRM_MODE_LUT_GAMMA (1 << 0)
>> +/* LUT is for degamma (before CTM) */ #define DRM_MODE_LUT_DEGAMMA (1
>> +<< 1)
>> +/* linearly interpolate between the points */ #define
>> +DRM_MODE_LUT_INTERPOLATE (1 << 2)
>
>Use BIT(2), and same for the other bits above and below?

Sure, will update this.

>> +/*
>> + * the last value of the previous range is the
>> + * first value of the current range.
>> + */
>> +#define DRM_MODE_LUT_REUSE_LAST (1 << 3)
>> +/* the curve must be non-decreasing */ #define
>> +DRM_MODE_LUT_NON_DECREASING (1 << 4)
>> +/* the curve is reflected across origin for negative inputs */
>> +#define DRM_MODE_LUT_REFLECT_NEGATIVE (1 << 5)
>> +/* the same curve (red) is used for blue and green channels as well
>> +*/ #define DRM_MODE_LUT_SINGLE_CHANNEL (1 << 6)
>
>	Sam
>
>_______________________________________________
>dri-devel mailing list
>dri-devel@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[v2 2/7] drm/i915: Define color lut range structure

[Uma Shankar]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

This defines the color lut ranges for 10bit and multi
segmented gamma range for ICL.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
 drivers/gpu/drm/i915/intel_color.c | 301 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 297 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index f2907cf..84d93ec 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -1083,9 +1083,279 @@ static int icl_color_check(struct intel_crtc_state *crtc_state)
 	return 0;
 }
 
+enum {
+	I9XX_LUT_SIZE_8BIT = 256,
+	I9XX_LUT_SIZE_10BIT = 129,
+
+	ILK_LUT_SIZE_10BIT = 1024,
+	ILK_LUT_SIZE_12BIT = 513,
+
+	IVB_LUT_SIZE_SPLIT = 512,
+
+	CHV_LUT_SIZE_CGM_DEGAMMA = 65,
+	CHV_LUT_SIZE_CGM_GAMMA = 257,
+};
+
+#define I9XX_GAMMA_8 \
+	{ \
+		.flags = DRM_MODE_LUT_GAMMA, \
+		.count = 256, \
+		.input_bpc = 8, .output_bpc = 8, \
+		.start = 0, .end = (1 << 8) - 1, \
+		.min = 0, .max = (1 << 8) - 1, \
+	}
+
+static const struct drm_color_lut_range i9xx_gamma_8[] = {
+	I9XX_GAMMA_8,
+};
+
+static const struct drm_color_lut_range i9xx_gamma_10_slope[] = {
+	{
+		.flags = (DRM_MODE_LUT_GAMMA |
+			  DRM_MODE_LUT_INTERPOLATE |
+			  DRM_MODE_LUT_NON_DECREASING),
+		.count = 129,
+		.input_bpc = 10, .output_bpc = 10,
+		.start = 0, .end = 1 << 10,
+		.min = 0, .max = (1 << 10) - 1,
+	},
+};
+
+#define I965_GAMMA_10 \
+	{ \
+		.flags = (DRM_MODE_LUT_GAMMA | \
+			  DRM_MODE_LUT_INTERPOLATE | \
+			  DRM_MODE_LUT_NON_DECREASING), \
+		.count = 128, \
+		.input_bpc = 10, .output_bpc = 16, \
+		.start = 0, .end = (1 << 10) - (1 << 10) / 128, \
+		.min = 0, .max = (1 << 16) - 1, \
+	}, \
+	/* PIPEGCMAX */ \
+	{ \
+		.flags = (DRM_MODE_LUT_GAMMA | \
+			  DRM_MODE_LUT_INTERPOLATE | \
+			  DRM_MODE_LUT_REUSE_LAST | \
+			  DRM_MODE_LUT_NON_DECREASING), \
+		.count = 1, \
+		.input_bpc = 10, .output_bpc = 16, \
+		.start = (1 << 10) - (1 << 10) / 128, .end = 1 << 10, \
+		.min = 0, .max = 1 << 16, \
+	}
+
+static const struct drm_color_lut_range i965_gamma_10[] = {
+	I965_GAMMA_10,
+};
+
+static const struct drm_color_lut_range ilk_gamma_degamma_8[] = {
+	{
+		.flags = (DRM_MODE_LUT_GAMMA |
+			  DRM_MODE_LUT_DEGAMMA),
+		.count = 256,
+		.input_bpc = 8, .output_bpc = 8,
+		.start = 0, .end = (1 << 8) - 1,
+		.min = 0, .max = (1 << 8) - 1,
+	},
+};
+
+static const struct drm_color_lut_range ilk_gamma_degamma_10[] = {
+	{
+		.flags = (DRM_MODE_LUT_GAMMA |
+			  DRM_MODE_LUT_DEGAMMA),
+		.count = 1024,
+		.input_bpc = 10, .output_bpc = 10,
+		.start = 0, .end = (1 << 10) - 1,
+		.min = 0, .max = (1 << 10) - 1,
+	},
+};
+
+static const struct drm_color_lut_range ilk_gamma_degamma_12p4[] = {
+	{
+		.flags = (DRM_MODE_LUT_GAMMA |
+			  DRM_MODE_LUT_DEGAMMA |
+			  DRM_MODE_LUT_INTERPOLATE |
+			  DRM_MODE_LUT_NON_DECREASING),
+		.count = 512,
+		.input_bpc = 12, .output_bpc = 16,
+		.start = 0, .end = (1 << 12) - (1 << 12) / 512,
+		.min = 0, .max = (1 << 16) - 1,
+	},
+	/* PIPEGCMAX */
+	{
+		.flags = (DRM_MODE_LUT_GAMMA |
+			  DRM_MODE_LUT_DEGAMMA |
+			  DRM_MODE_LUT_INTERPOLATE |
+			  DRM_MODE_LUT_REUSE_LAST |
+			  DRM_MODE_LUT_NON_DECREASING),
+		.count = 1,
+		.input_bpc = 10, .output_bpc = 16,
+		.start = (1 << 12) - (1 << 12) / 512, .end = 1 << 12,
+		.min = 0, .max = 1 << 16,
+	},
+};
+
+static const struct drm_color_lut_range glk_gamma_10[] = {
+	{
+		.flags = (DRM_MODE_LUT_GAMMA |
+			  DRM_MODE_LUT_REFLECT_NEGATIVE),
+		.count = 1024,
+		.input_bpc = 10, .output_bpc = 10,
+		.start = 0, .end = (1 << 10) - 1,
+		.min = 0, .max = (1 << 10) - 1,
+	},
+	/* PAL_EXT_GC_MAX */
+	{
+		.flags = (DRM_MODE_LUT_GAMMA |
+			  DRM_MODE_LUT_REFLECT_NEGATIVE |
+			  DRM_MODE_LUT_INTERPOLATE |
+			  DRM_MODE_LUT_REUSE_LAST |
+			  DRM_MODE_LUT_NON_DECREASING),
+		.count = 1,
+		.input_bpc = 10, .output_bpc = 16,
+		.start = 1 << 10, .end = 3 << 10,
+		.min = 0, .max = (8 << 16) - 1,
+	},
+	/* PAL_EXT2_GC_MAX */
+	{
+		.flags = (DRM_MODE_LUT_GAMMA |
+			  DRM_MODE_LUT_REFLECT_NEGATIVE |
+			  DRM_MODE_LUT_INTERPOLATE |
+			  DRM_MODE_LUT_REUSE_LAST |
+			  DRM_MODE_LUT_NON_DECREASING),
+		.count = 1,
+		.input_bpc = 10, .output_bpc = 16,
+		.start = 3 << 12, .end = 7 << 12,
+		.min = 0, .max = (8 << 16) - 1,
+	},
+};
+
+/* FIXME input bpc? */
+static const struct drm_color_lut_range glk_gamma_12p4[] = {
+	{
+		.flags = (DRM_MODE_LUT_GAMMA |
+			  DRM_MODE_LUT_REFLECT_NEGATIVE |
+			  DRM_MODE_LUT_INTERPOLATE |
+			  DRM_MODE_LUT_NON_DECREASING),
+		.count = 512,
+		.input_bpc = 16, .output_bpc = 16,
+		.start = 0, .end = (1 << 16) - (1 << 16) / 512,
+		.min = 0, .max = (1 << 16) - 1,
+	},
+	/* PAL_GC_MAX */
+	{
+		.flags = (DRM_MODE_LUT_GAMMA |
+			  DRM_MODE_LUT_REFLECT_NEGATIVE |
+			  DRM_MODE_LUT_INTERPOLATE |
+			  DRM_MODE_LUT_REUSE_LAST |
+			  DRM_MODE_LUT_NON_DECREASING),
+		.count = 1,
+		.input_bpc = 16, .output_bpc = 16,
+		.start = (1 << 16) - (1 << 16) / 512, .end = 1 << 16,
+		.min = 0, .max = 1 << 16,
+	},
+	/* PAL_EXT_GC_MAX */
+	{
+		.flags = (DRM_MODE_LUT_GAMMA |
+			  DRM_MODE_LUT_REFLECT_NEGATIVE |
+			  DRM_MODE_LUT_INTERPOLATE |
+			  DRM_MODE_LUT_REUSE_LAST |
+			  DRM_MODE_LUT_NON_DECREASING),
+		.count = 1,
+		.input_bpc = 16, .output_bpc = 16,
+		.start = 1 << 16, .end = 3 << 16,
+		.min = 0, .max = (8 << 16) - 1,
+	},
+	/* PAL_EXT2_GC_MAX */
+	{
+		.flags = (DRM_MODE_LUT_GAMMA |
+			  DRM_MODE_LUT_REFLECT_NEGATIVE |
+			  DRM_MODE_LUT_INTERPOLATE |
+			  DRM_MODE_LUT_REUSE_LAST |
+			  DRM_MODE_LUT_NON_DECREASING),
+		.count = 1,
+		.input_bpc = 16, .output_bpc = 16,
+		.start = 3 << 16, .end = 7 << 16,
+		.min = 0, .max = (8 << 16) - 1,
+	},
+};
+
+ /* FIXME input bpc? */
+static const struct drm_color_lut_range icl_multi_seg_gamma[] = {
+	/* segment 1 aka. super fine segment */
+	{
+		.flags = (DRM_MODE_LUT_GAMMA |
+			  DRM_MODE_LUT_REFLECT_NEGATIVE |
+			  DRM_MODE_LUT_INTERPOLATE |
+			  DRM_MODE_LUT_NON_DECREASING),
+		.count = 9,
+		.input_bpc = 24, .output_bpc = 16,
+		.start = 0, .end = (1 << 24) / (128 * 256),
+		.min = 0, .max = (1 << 16) - 1,
+	},
+	/* segment 2 aka. fine segment */
+	{
+		.flags = (DRM_MODE_LUT_GAMMA |
+			  DRM_MODE_LUT_REFLECT_NEGATIVE |
+			  DRM_MODE_LUT_INTERPOLATE |
+			  DRM_MODE_LUT_NON_DECREASING),
+		.count = 257,
+		.input_bpc = 24, .output_bpc = 16,
+		.start = 0, .end = (1 << 24) / 128,
+		.min = 0, .max = (1 << 16) - 1,
+	},
+	/* segment 3 aka. coarse segment */
+	{
+		.flags = (DRM_MODE_LUT_GAMMA |
+			  DRM_MODE_LUT_REFLECT_NEGATIVE |
+			  DRM_MODE_LUT_INTERPOLATE |
+			  DRM_MODE_LUT_NON_DECREASING),
+		.count = 257,
+		.input_bpc = 24, .output_bpc = 16,
+		.start = 0, .end = (1 << 24) - (1 << 24) / 256,
+		.min = 0, .max = (1 << 16) - 1,
+	},
+	/* segment 3 aka. coarse segment / PAL_GC_MAX */
+	{
+		.flags = (DRM_MODE_LUT_GAMMA |
+			  DRM_MODE_LUT_REFLECT_NEGATIVE |
+			  DRM_MODE_LUT_INTERPOLATE |
+			  DRM_MODE_LUT_REUSE_LAST |
+			  DRM_MODE_LUT_NON_DECREASING),
+		.count = 1,
+		.input_bpc = 24, .output_bpc = 16,
+		.start = (1 << 24) - (1 << 24) / 256, .end = 1 << 24,
+		.min = 0, .max = 1 << 16,
+	},
+	/* PAL_EXT_GC_MAX */
+	{
+		.flags = (DRM_MODE_LUT_GAMMA |
+			  DRM_MODE_LUT_REFLECT_NEGATIVE |
+			  DRM_MODE_LUT_INTERPOLATE |
+			  DRM_MODE_LUT_REUSE_LAST |
+			  DRM_MODE_LUT_NON_DECREASING),
+		.count = 1,
+		.input_bpc = 24, .output_bpc = 16,
+		.start = 1 << 24, .end = 3 << 24,
+		.min = 0, .max = (8 << 16) - 1,
+	},
+	/* PAL_EXT2_GC_MAX */
+	{
+		.flags = (DRM_MODE_LUT_GAMMA |
+			  DRM_MODE_LUT_REFLECT_NEGATIVE |
+			  DRM_MODE_LUT_INTERPOLATE |
+			  DRM_MODE_LUT_REUSE_LAST |
+			  DRM_MODE_LUT_NON_DECREASING),
+		.count = 1,
+		.input_bpc = 24, .output_bpc = 16,
+		.start = 3 << 24, .end = 7 << 24,
+		.min = 0, .max = (8 << 16) - 1,
+	},
+};
+
 void intel_color_init(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	int degamma_lut_size, gamma_lut_size;
 
 	drm_mode_crtc_set_gamma_size(&crtc->base, 256);
 
@@ -1100,14 +1370,37 @@ void intel_color_init(struct intel_crtc *crtc)
 			dev_priv->display.load_luts = i9xx_load_luts;
 		}
 	} else {
-		if (INTEL_GEN(dev_priv) >= 11)
+		if (INTEL_GEN(dev_priv) >= 11) {
 			dev_priv->display.color_check = icl_color_check;
-		else if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
+
+			/* don't advertize the >= 1.0 entries */
+			degamma_lut_size = 0;
+			gamma_lut_size = ILK_LUT_SIZE_10BIT;
+
+			drm_color_add_gamma_mode_range(&dev_priv->drm,
+						       "8bit gamma",
+						       i9xx_gamma_8,
+						       sizeof(i9xx_gamma_8));
+			drm_color_add_gamma_mode_range(&dev_priv->drm,
+						       "10bit gamma",
+							glk_gamma_10,
+							sizeof(glk_gamma_10));
+			drm_color_add_gamma_mode_range(&dev_priv->drm,
+						       "interpolated gamma",
+							glk_gamma_12p4,
+							sizeof(glk_gamma_12p4));
+			drm_color_add_gamma_mode_range(&dev_priv->drm,
+						       "multi-segmented gamma",
+							icl_multi_seg_gamma,
+							sizeof(icl_multi_seg_gamma));
+		} else if (INTEL_GEN(dev_priv) >= 10 ||
+			   IS_GEMINILAKE(dev_priv)) {
 			dev_priv->display.color_check = glk_color_check;
-		else if (INTEL_GEN(dev_priv) >= 8)
+		} else if (INTEL_GEN(dev_priv) >= 8) {
 			dev_priv->display.color_check = bdw_color_check;
-		else
+		} else {
 			dev_priv->display.color_check = ilk_color_check;
+		}
 
 		if (INTEL_GEN(dev_priv) >= 9)
 			dev_priv->display.color_commit = skl_color_commit;
-- 
1.9.1

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

Re: [v2 2/7] drm/i915: Define color lut range structure

[Ville Syrjälä]

On Mon, Apr 01, 2019 at 11:00:06PM +0530, Uma Shankar wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> This defines the color lut ranges for 10bit and multi
> segmented gamma range for ICL.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_color.c | 301 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 297 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index f2907cf..84d93ec 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -1083,9 +1083,279 @@ static int icl_color_check(struct intel_crtc_state *crtc_state)
>  	return 0;
>  }
>  
> +enum {
> +	I9XX_LUT_SIZE_8BIT = 256,
> +	I9XX_LUT_SIZE_10BIT = 129,
> +
> +	ILK_LUT_SIZE_10BIT = 1024,
> +	ILK_LUT_SIZE_12BIT = 513,
> +
> +	IVB_LUT_SIZE_SPLIT = 512,
> +
> +	CHV_LUT_SIZE_CGM_DEGAMMA = 65,
> +	CHV_LUT_SIZE_CGM_GAMMA = 257,
> +};
> +
> +#define I9XX_GAMMA_8 \
> +	{ \
> +		.flags = DRM_MODE_LUT_GAMMA, \
> +		.count = 256, \
> +		.input_bpc = 8, .output_bpc = 8, \
> +		.start = 0, .end = (1 << 8) - 1, \
> +		.min = 0, .max = (1 << 8) - 1, \
> +	}
> +
> +static const struct drm_color_lut_range i9xx_gamma_8[] = {
> +	I9XX_GAMMA_8,
> +};
> +
> +static const struct drm_color_lut_range i9xx_gamma_10_slope[] = {
> +	{
> +		.flags = (DRM_MODE_LUT_GAMMA |
> +			  DRM_MODE_LUT_INTERPOLATE |
> +			  DRM_MODE_LUT_NON_DECREASING),
> +		.count = 129,
> +		.input_bpc = 10, .output_bpc = 10,
> +		.start = 0, .end = 1 << 10,
> +		.min = 0, .max = (1 << 10) - 1,
> +	},
> +};

Step 1 should probably be to advertise these for the gamma modes we
currently have, on every platform.

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

Re: [v2 2/7] drm/i915: Define color lut range structure

[Shankar, Uma]

>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Monday, April 8, 2019 3:39 PM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Lankhorst,
>Maarten <maarten.lankhorst@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>;
>Sharma, Shashank <shashank.sharma@intel.com>; emil.l.velikov@gmail.com;
>brian.starkey@arm.com; dcastagna@chromium.org; seanpaul@chromium.org;
>harry.wentland@amd.com; Roper, Matthew D <matthew.d.roper@intel.com>
>Subject: Re: [v2 2/7] drm/i915: Define color lut range structure
>
>On Mon, Apr 01, 2019 at 11:00:06PM +0530, Uma Shankar wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> This defines the color lut ranges for 10bit and multi segmented gamma
>> range for ICL.
>>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_color.c | 301
>> ++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 297 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_color.c
>> b/drivers/gpu/drm/i915/intel_color.c
>> index f2907cf..84d93ec 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -1083,9 +1083,279 @@ static int icl_color_check(struct intel_crtc_state
>*crtc_state)
>>  	return 0;
>>  }
>>
>> +enum {
>> +	I9XX_LUT_SIZE_8BIT = 256,
>> +	I9XX_LUT_SIZE_10BIT = 129,
>> +
>> +	ILK_LUT_SIZE_10BIT = 1024,
>> +	ILK_LUT_SIZE_12BIT = 513,
>> +
>> +	IVB_LUT_SIZE_SPLIT = 512,
>> +
>> +	CHV_LUT_SIZE_CGM_DEGAMMA = 65,
>> +	CHV_LUT_SIZE_CGM_GAMMA = 257,
>> +};
>> +
>> +#define I9XX_GAMMA_8 \
>> +	{ \
>> +		.flags = DRM_MODE_LUT_GAMMA, \
>> +		.count = 256, \
>> +		.input_bpc = 8, .output_bpc = 8, \
>> +		.start = 0, .end = (1 << 8) - 1, \
>> +		.min = 0, .max = (1 << 8) - 1, \
>> +	}
>> +
>> +static const struct drm_color_lut_range i9xx_gamma_8[] = {
>> +	I9XX_GAMMA_8,
>> +};
>> +
>> +static const struct drm_color_lut_range i9xx_gamma_10_slope[] = {
>> +	{
>> +		.flags = (DRM_MODE_LUT_GAMMA |
>> +			  DRM_MODE_LUT_INTERPOLATE |
>> +			  DRM_MODE_LUT_NON_DECREASING),
>> +		.count = 129,
>> +		.input_bpc = 10, .output_bpc = 10,
>> +		.start = 0, .end = 1 << 10,
>> +		.min = 0, .max = (1 << 10) - 1,
>> +	},
>> +};
>
>Step 1 should probably be to advertise these for the gamma modes we currently have,
>on every platform.

Ok, focussed here mainly on multi-segmented gamma. But will add all the gamma modes
supported on various platforms.

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

[v2 3/7] drm: Add gamma mode property

[Uma Shankar]

Add Gamma Mode property to set the gamma mode
(Interploated, Split, Multi Segmented etc) from the
list obtained through the gamma mode caps property.

Create the blob and send to driver for programming
the luts to the appropriate registers and setting
the chosen gamma mode.

Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c | 10 ++++++++++
 drivers/gpu/drm/drm_color_mgmt.c  | 32 ++++++++++++++++++++++++++++++++
 include/drm/drm_color_mgmt.h      |  3 +++
 include/drm/drm_crtc.h            |  7 +++++++
 include/drm/drm_mode_config.h     |  5 +++++
 include/uapi/drm/drm_mode.h       | 11 +++++++++++
 6 files changed, 68 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 03df2a4..d3008ea 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -459,6 +459,14 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 					&replaced);
 		state->color_mgmt_changed |= replaced;
 		return ret;
+	} else if (property == config->gamma_mode_property) {
+		ret = drm_atomic_replace_property_blob_from_id(dev,
+					&state->gamma_mode,
+					val,
+					-1, sizeof(struct drm_color_mode_lut),
+					&replaced);
+		state->color_mgmt_changed |= replaced;
+		return ret;
 	} else if (property == config->prop_out_fence_ptr) {
 		s32 __user *fence_ptr = u64_to_user_ptr(val);
 
@@ -498,6 +506,8 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 	else if (property == config->gamma_mode_caps_property)
 		*val = (state->gamma_mode_caps) ?
 			state->gamma_mode_caps->base.id : 0;
+	else if (property == config->gamma_mode_property)
+		*val = (state->gamma_mode) ? state->gamma_mode->base.id : 0;
 	else if (property == config->degamma_lut_property)
 		*val = (state->degamma_lut) ? state->degamma_lut->base.id : 0;
 	else if (property == config->ctm_property)
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index 054f0ed..cba1d6d 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -176,6 +176,38 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
 }
 EXPORT_SYMBOL(drm_crtc_enable_color_mgmt);
 
+void drm_crtc_attach_gamma_mode_property(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_mode_config *config = &dev->mode_config;
+
+	if (!config->gamma_mode_property)
+		return;
+
+	drm_object_attach_property(&crtc->base,
+				   config->gamma_mode_property, 0);
+}
+EXPORT_SYMBOL(drm_crtc_attach_gamma_mode_property);
+
+int drm_color_create_gamma_mode_property(struct drm_device *dev,
+					 int num_values)
+{
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_property *prop;
+
+	prop = drm_property_create(dev,
+				   DRM_MODE_PROP_BLOB |
+				   DRM_MODE_PROP_ATOMIC,
+				   "GAMMA_MODE", num_values);
+	if (!prop)
+		return -ENOMEM;
+
+	config->gamma_mode_property = prop;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_color_create_gamma_mode_property);
+
 void drm_crtc_attach_gamma_mode_caps_property(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
index e0f94db..4306e07 100644
--- a/include/drm/drm_color_mgmt.h
+++ b/include/drm/drm_color_mgmt.h
@@ -54,6 +54,9 @@ static inline int drm_color_lut_size(const struct drm_property_blob *blob)
 int drm_color_create_gamma_mode_caps_property(struct drm_device *dev,
 					      int num_values);
 void drm_crtc_attach_gamma_mode_caps_property(struct drm_crtc *crtc);
+int drm_color_create_gamma_mode_property(struct drm_device *dev,
+					 int num_values);
+void drm_crtc_attach_gamma_mode_property(struct drm_crtc *crtc);
 int drm_color_add_gamma_mode_range(struct drm_device *dev,
 				   const char *name,
 				   const struct drm_color_lut_range *ranges,
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index cdfda90..bc8a2e7 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -256,6 +256,13 @@ struct drm_crtc_state {
 	struct drm_property_blob *gamma_mode_caps;
 
 	/**
+	 * @gamma_mode:
+	 *
+	 * FIXME
+	 */
+	struct drm_property_blob *gamma_mode;
+
+	/**
 	 * @degamma_lut:
 	 *
 	 * Lookup table for converting framebuffer pixel data before apply the
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 7b20355..f5bb807 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -766,6 +766,11 @@ struct drm_mode_config {
 	 */
 	struct drm_property *gamma_mode_caps_property;
 	/**
+	 * @gamma_mode_property: Optional CRTC property to enumerate and
+	 * select the mode of the crtc gamma/degmama LUTs.
+	 */
+	struct drm_property *gamma_mode_property;
+	/**
 	 * @degamma_lut_property: Optional CRTC property to set the LUT used to
 	 * convert the framebuffer's colors to linear gamma.
 	 */
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index e475f4a..e84389d 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -667,6 +667,17 @@ struct drm_color_lut_range {
 	__s32 min, max;
 };
 
+struct drm_color_mode_lut {
+	/* DRM_MODE_LUT_* */
+	__u32 flags;
+	/* number of points on the curve */
+	__u32 count;
+	/* Name of Gamma Mode */
+	char name[DRM_PROP_NAME_LEN];
+	/* Pointer to Lut elements */
+	__u64 lut;
+};
+
 #define DRM_MODE_PAGE_FLIP_EVENT 0x01
 #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
 #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
-- 
1.9.1

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

Re: [v2 3/7] drm: Add gamma mode property

[Sam Ravnborg]

Hi Uma.

> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -256,6 +256,13 @@ struct drm_crtc_state {
>  	struct drm_property_blob *gamma_mode_caps;
>  
>  	/**
> +	 * @gamma_mode:
> +	 *
> +	 * FIXME
> +	 */
> +	struct drm_property_blob *gamma_mode;

HEre the name matches, but please add documentation too.

>  
> +struct drm_color_mode_lut {
> +	/* DRM_MODE_LUT_* */
> +	__u32 flags;
> +	/* number of points on the curve */
> +	__u32 count;
> +	/* Name of Gamma Mode */
> +	char name[DRM_PROP_NAME_LEN];
> +	/* Pointer to Lut elements */
> +	__u64 lut;
> +};
From an alignment point of view the __u64 should come before the char name[].
But the above may be fine depending on DRM_PROP_NAME_LEN

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

RE: [v2 3/7] drm: Add gamma mode property

[Shankar, Uma]

>-----Original Message-----
>From: Sam Ravnborg [mailto:sam@ravnborg.org]
>Sent: Tuesday, April 2, 2019 12:07 AM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org;
>dcastagna@chromium.org; emil.l.velikov@gmail.com; seanpaul@chromium.org;
>Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
><maarten.lankhorst@intel.com>
>Subject: Re: [v2 3/7] drm: Add gamma mode property
>
>Hi Uma.
>
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -256,6 +256,13 @@ struct drm_crtc_state {
>>  	struct drm_property_blob *gamma_mode_caps;
>>
>>  	/**
>> +	 * @gamma_mode:
>> +	 *
>> +	 * FIXME
>> +	 */
>> +	struct drm_property_blob *gamma_mode;
>
>HEre the name matches, but please add documentation too.

Sure, will add documentation explaining how this should get used once we
have the agreement on design and approach. 

>>
>> +struct drm_color_mode_lut {
>> +	/* DRM_MODE_LUT_* */
>> +	__u32 flags;
>> +	/* number of points on the curve */
>> +	__u32 count;
>> +	/* Name of Gamma Mode */
>> +	char name[DRM_PROP_NAME_LEN];
>> +	/* Pointer to Lut elements */
>> +	__u64 lut;
>> +};
>From an alignment point of view the __u64 should come before the char name[].
>But the above may be fine depending on DRM_PROP_NAME_LEN

Yeah, but can re-order this. Thanks for the review.

Regards,
Uma Shankar

>
>	Sam
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[v2 4/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 341f03e..f95f82f 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7218,6 +7218,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)
@@ -10157,6 +10158,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

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

[v2 5/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 | 161 ++++++++++++++++++++++++++++++++++++-
 include/drm/drm_crtc.h             |   3 +
 2 files changed, 161 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 84d93ec..d81de32 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -57,6 +57,12 @@
 
 #define ILK_CSC_POSTOFF_LIMITED_RANGE (16 * (1 << 12) / 255)
 
+#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)
+
 static const u16 ilk_csc_off_zero[3] = {};
 
 static const u16 ilk_csc_coeff_identity[9] = {
@@ -92,6 +98,9 @@
 	0x0800, 0x0100, 0x0800,
 };
 
+#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;
@@ -670,16 +679,149 @@ static void glk_load_luts(const struct intel_crtc_state *crtc_state)
 		bdw_load_gamma_lut(crtc_state, 0);
 }
 
+static void icl_program_coarse_segment_lut(const struct intel_crtc_state
+					   *crtc_state,
+					   struct drm_color_lut *gamma_lut,
+					   u32 offset)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	const struct drm_color_lut *lut = gamma_lut;
+	enum pipe pipe = crtc->pipe;
+	u32 i, lut_size, word;
+
+	WARN_ON(offset & ~PAL_PREC_INDEX_VALUE_MASK);
+
+	I915_WRITE(PREC_PAL_INDEX(pipe),
+		   (offset ? PAL_PREC_SPLIT_MODE : 0) |
+		   PAL_PREC_AUTO_INCREMENT |
+		   offset);
+
+	if (lut && crtc_state->base.gamma_mode_type ==
+				MULTI_SEGMENTED_GAMMA_MODE_12BIT) {
+		lut_size = 9 + 514;
+		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_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_DATA(pipe), word);
+		}
+	}
+	
+	/*
+	 * Program the max register to clamp values > 1.0.
+	 * ToDo: Extend the ABI to be able to program values
+	 * from 1.0
+	 */
+	I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), (1 << 16));
+	I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), (1 << 16));
+	I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), (1 << 16));
+
+	/*
+	 * Program the max register to clamp values > 1.0.
+	 * ToDo: Extend the ABI to be able to program values
+	 * from 1.0 to 3.0
+	 */
+	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 0), (1 << 16));
+	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 1), (1 << 16));
+	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 2), (1 << 16));
+
+	/*
+	 * Program the gc max 2 register to clamp values > 1.0.
+	 * ToDo: Extend the ABI to be able to program values
+	 * from 3.0 to 7.0
+	*/
+	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
+		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 0), (1 << 16));
+		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 1), (1 << 16));
+		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 2), (1 << 16));
+	}
+}
+
+static void icl_program_fine_segment_lut(const struct intel_crtc_state
+					 *crtc_state,
+					 struct drm_color_lut *gamma_lut,
+					 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, word, 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 (gamma_lut) {
+		struct drm_color_lut *lut =
+			(struct drm_color_lut *)gamma_lut;
+
+		for (i = 0; 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);
+		}
+	}
+}
+
+static void icl_load_gamma_multi_segmented_lut(const struct intel_crtc_state
+					       *crtc_state, u32 offset)
+{
+	const struct drm_property_blob *gamma_mode_blob =
+					crtc_state->base.gamma_mode;
+	struct drm_color_lut *gamma_lut;
+	int ret;
+
+	if (gamma_mode_blob) {
+		struct drm_color_mode_lut *arg = gamma_mode_blob->data;
+		u64 __user *props_ptr = (u64 __user *)(unsigned long)(arg->lut);
+
+		DRM_INFO("Gamma Mode=%s\n", arg->name);
+		gamma_lut = kmalloc((sizeof(struct drm_color_lut) * arg->count),
+				    GFP_KERNEL);
+		ret = copy_from_user(gamma_lut, props_ptr,
+				     sizeof(struct drm_color_lut) * arg->count);
+	}
+
+	icl_program_fine_segment_lut(crtc_state, gamma_lut, 0);
+	icl_program_coarse_segment_lut(crtc_state, gamma_lut, 0);
+}
+
 static void icl_load_luts(const struct intel_crtc_state *crtc_state)
 {
 	if (crtc_state->base.degamma_lut)
 		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->base.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)
@@ -1034,10 +1176,20 @@ static int glk_color_check(struct intel_crtc_state *crtc_state)
 	return 0;
 }
 
-static u32 icl_gamma_mode(const struct intel_crtc_state *crtc_state)
+static u32 icl_gamma_mode(struct intel_crtc_state *crtc_state)
 {
+	const struct drm_property_blob *gamma_mode_blob =
+					crtc_state->base.gamma_mode;
+	struct drm_color_mode_lut *arg;
 	u32 gamma_mode = 0;
 
+	if (gamma_mode_blob) {
+		arg = gamma_mode_blob->data;
+		if (!strcmp(arg->name, "multi-segmented gamma"))
+			crtc_state->base.gamma_mode_type =
+				MULTI_SEGMENTED_GAMMA_MODE_12BIT;
+	}
+
 	if (crtc_state->base.degamma_lut)
 		gamma_mode |= PRE_CSC_GAMMA_ENABLE;
 
@@ -1048,6 +1200,9 @@ static u32 icl_gamma_mode(const struct intel_crtc_state *crtc_state)
 	if (!crtc_state->base.gamma_lut ||
 	    crtc_state_is_legacy_gamma(crtc_state))
 		gamma_mode |= GAMMA_MODE_MODE_8BIT;
+	else if (crtc_state->base.gamma_mode_type ==
+		 MULTI_SEGMENTED_GAMMA_MODE_12BIT)
+		gamma_mode |= GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED;
 	else
 		gamma_mode |= GAMMA_MODE_MODE_10BIT;
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index bc8a2e7..a8d0b4c 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -262,6 +262,9 @@ struct drm_crtc_state {
 	 */
 	struct drm_property_blob *gamma_mode;
 
+	/* Gamma mode type programmed on the pipe */
+	u32 gamma_mode_type;
+
 	/**
 	 * @degamma_lut:
 	 *
-- 
1.9.1

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

Re: [v2 5/7] drm/i915/icl: Add support for multi segmented gamma mode

[Ville Syrjälä]

On Mon, Apr 01, 2019 at 11:00:09PM +0530, Uma Shankar wrote:
> 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 | 161 ++++++++++++++++++++++++++++++++++++-
>  include/drm/drm_crtc.h             |   3 +
>  2 files changed, 161 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 84d93ec..d81de32 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -57,6 +57,12 @@
>  
>  #define ILK_CSC_POSTOFF_LIMITED_RANGE (16 * (1 << 12) / 255)
>  
> +#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)
> +
>  static const u16 ilk_csc_off_zero[3] = {};
>  
>  static const u16 ilk_csc_coeff_identity[9] = {
> @@ -92,6 +98,9 @@
>  	0x0800, 0x0100, 0x0800,
>  };
>  
> +#define GEN11_GET_MS10BITS_OF_LUT(lut) (((lut) >> 6) & 0x3FF)
> +#define GEN11_GET_LS6BITS_OF_LUT(lut)  ((lut) & 0x3F)

I think this is just the ilk+ 12.4 format. IIRC I had some kind of
ilk_lut_12p4_ldw/udw() funcs for these in my branch somewhere.
I'd like to see somehting like that in this patch.

> +
>  static bool lut_is_legacy(const struct drm_property_blob *lut)
>  {
>  	return drm_color_lut_size(lut) == LEGACY_LUT_LENGTH;
> @@ -670,16 +679,149 @@ static void glk_load_luts(const struct intel_crtc_state *crtc_state)
>  		bdw_load_gamma_lut(crtc_state, 0);
>  }
>  
> +static void icl_program_coarse_segment_lut(const struct intel_crtc_state
> +					   *crtc_state,
> +					   struct drm_color_lut *gamma_lut,
> +					   u32 offset)

I think to match the current pattern we should have something like:

icl_load_lut_coarse_segment(struct intel_crtc *crtc,
                            const struct drm_property_blob *blob);
icl_load_lut_fine_segment(struct intel_crtc *crtc,
                          const struct drm_property_blob *blob);

> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	const struct drm_color_lut *lut = gamma_lut;
> +	enum pipe pipe = crtc->pipe;
> +	u32 i, lut_size, word;
> +
> +	WARN_ON(offset & ~PAL_PREC_INDEX_VALUE_MASK);
> +
> +	I915_WRITE(PREC_PAL_INDEX(pipe),
> +		   (offset ? PAL_PREC_SPLIT_MODE : 0) |
> +		   PAL_PREC_AUTO_INCREMENT |
> +		   offset);
> +
> +	if (lut && crtc_state->base.gamma_mode_type ==
> +				MULTI_SEGMENTED_GAMMA_MODE_12BIT) {
> +		lut_size = 9 + 514;
> +		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_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_DATA(pipe), word);
> +		}
> +	}
> +	
> +	/*
> +	 * Program the max register to clamp values > 1.0.
> +	 * ToDo: Extend the ABI to be able to program values
> +	 * from 1.0
> +	 */
> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), (1 << 16));
> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), (1 << 16));
> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), (1 << 16));
> +
> +	/*
> +	 * Program the max register to clamp values > 1.0.
> +	 * ToDo: Extend the ABI to be able to program values
> +	 * from 1.0 to 3.0
> +	 */
> +	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 0), (1 << 16));
> +	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 1), (1 << 16));
> +	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 2), (1 << 16));

Doubled thing. But we can just drop these and call
ivb_load_lut_10_max() from higher up instead.

> +
> +	/*
> +	 * Program the gc max 2 register to clamp values > 1.0.
> +	 * ToDo: Extend the ABI to be able to program values
> +	 * from 3.0 to 7.0
> +	*/
> +	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
> +		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 0), (1 << 16));
> +		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 1), (1 << 16));
> +		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 2), (1 << 16));
> +	}
> +}
> +
> +static void icl_program_fine_segment_lut(const struct intel_crtc_state
> +					 *crtc_state,
> +					 struct drm_color_lut *gamma_lut,
> +					 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, word, 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 (gamma_lut) {
> +		struct drm_color_lut *lut =
> +			(struct drm_color_lut *)gamma_lut;
> +
> +		for (i = 0; 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);
> +		}
> +	}
> +}
> +
> +static void icl_load_gamma_multi_segmented_lut(const struct intel_crtc_state
> +					       *crtc_state, u32 offset)
> +{
> +	const struct drm_property_blob *gamma_mode_blob =
> +					crtc_state->base.gamma_mode;
> +	struct drm_color_lut *gamma_lut;
> +	int ret;
> +
> +	if (gamma_mode_blob) {
> +		struct drm_color_mode_lut *arg = gamma_mode_blob->data;
> +		u64 __user *props_ptr = (u64 __user *)(unsigned long)(arg->lut);
> +
> +		DRM_INFO("Gamma Mode=%s\n", arg->name);
> +		gamma_lut = kmalloc((sizeof(struct drm_color_lut) * arg->count),
> +				    GFP_KERNEL);
> +		ret = copy_from_user(gamma_lut, props_ptr,
> +				     sizeof(struct drm_color_lut) * arg->count);
> +	}

What's this doing here?

> +
> +	icl_program_fine_segment_lut(crtc_state, gamma_lut, 0);
> +	icl_program_coarse_segment_lut(crtc_state, gamma_lut, 0);
> +}
> +
>  static void icl_load_luts(const struct intel_crtc_state *crtc_state)
>  {
>  	if (crtc_state->base.degamma_lut)
>  		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->base.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)
> @@ -1034,10 +1176,20 @@ static int glk_color_check(struct intel_crtc_state *crtc_state)
>  	return 0;
>  }
>  
> -static u32 icl_gamma_mode(const struct intel_crtc_state *crtc_state)
> +static u32 icl_gamma_mode(struct intel_crtc_state *crtc_state)
>  {
> +	const struct drm_property_blob *gamma_mode_blob =
> +					crtc_state->base.gamma_mode;
> +	struct drm_color_mode_lut *arg;
>  	u32 gamma_mode = 0;
>  
> +	if (gamma_mode_blob) {
> +		arg = gamma_mode_blob->data;
> +		if (!strcmp(arg->name, "multi-segmented gamma"))
> +			crtc_state->base.gamma_mode_type =
> +				MULTI_SEGMENTED_GAMMA_MODE_12BIT;
> +	}

With the blob_enum this kind of stuff should disappear I think.

> +
>  	if (crtc_state->base.degamma_lut)
>  		gamma_mode |= PRE_CSC_GAMMA_ENABLE;
>  
> @@ -1048,6 +1200,9 @@ static u32 icl_gamma_mode(const struct intel_crtc_state *crtc_state)
>  	if (!crtc_state->base.gamma_lut ||
>  	    crtc_state_is_legacy_gamma(crtc_state))
>  		gamma_mode |= GAMMA_MODE_MODE_8BIT;
> +	else if (crtc_state->base.gamma_mode_type ==
> +		 MULTI_SEGMENTED_GAMMA_MODE_12BIT)
> +		gamma_mode |= GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED;
>  	else
>  		gamma_mode |= GAMMA_MODE_MODE_10BIT;
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index bc8a2e7..a8d0b4c 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -262,6 +262,9 @@ struct drm_crtc_state {
>  	 */
>  	struct drm_property_blob *gamma_mode;
>  
> +	/* Gamma mode type programmed on the pipe */
> +	u32 gamma_mode_type;
> +
>  	/**
>  	 * @degamma_lut:
>  	 *
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [v2 5/7] drm/i915/icl: Add support for multi segmented gamma mode

[Shankar, Uma]

>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Monday, April 8, 2019 3:50 PM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org;
>dcastagna@chromium.org; seanpaul@chromium.org; Syrjala, Ville
><ville.syrjala@intel.com>; harry.wentland@amd.com; Lankhorst, Maarten
><maarten.lankhorst@intel.com>
>Subject: Re: [Intel-gfx] [v2 5/7] drm/i915/icl: Add support for multi segmented
>gamma mode
>
>On Mon, Apr 01, 2019 at 11:00:09PM +0530, Uma Shankar wrote:
>> 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 | 161
>++++++++++++++++++++++++++++++++++++-
>>  include/drm/drm_crtc.h             |   3 +
>>  2 files changed, 161 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_color.c
>> b/drivers/gpu/drm/i915/intel_color.c
>> index 84d93ec..d81de32 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -57,6 +57,12 @@
>>
>>  #define ILK_CSC_POSTOFF_LIMITED_RANGE (16 * (1 << 12) / 255)
>>
>> +#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)
>> +
>>  static const u16 ilk_csc_off_zero[3] = {};
>>
>>  static const u16 ilk_csc_coeff_identity[9] = { @@ -92,6 +98,9 @@
>>  	0x0800, 0x0100, 0x0800,
>>  };
>>
>> +#define GEN11_GET_MS10BITS_OF_LUT(lut) (((lut) >> 6) & 0x3FF) #define
>> +GEN11_GET_LS6BITS_OF_LUT(lut)  ((lut) & 0x3F)
>
>I think this is just the ilk+ 12.4 format. IIRC I had some kind of
>ilk_lut_12p4_ldw/udw() funcs for these in my branch somewhere.
>I'd like to see somehting like that in this patch.

Sure, I will add those and use the same. Something similar to (i965_lut_10p6_udw)

>> +
>>  static bool lut_is_legacy(const struct drm_property_blob *lut)  {
>>  	return drm_color_lut_size(lut) == LEGACY_LUT_LENGTH; @@ -670,16
>> +679,149 @@ static void glk_load_luts(const struct intel_crtc_state *crtc_state)
>>  		bdw_load_gamma_lut(crtc_state, 0);
>>  }
>>
>> +static void icl_program_coarse_segment_lut(const struct intel_crtc_state
>> +					   *crtc_state,
>> +					   struct drm_color_lut *gamma_lut,
>> +					   u32 offset)
>
>I think to match the current pattern we should have something like:
>
>icl_load_lut_coarse_segment(struct intel_crtc *crtc,
>                            const struct drm_property_blob *blob);
>icl_load_lut_fine_segment(struct intel_crtc *crtc,
>                          const struct drm_property_blob *blob);

Ok, will update this.

>> +{
>> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> +	const struct drm_color_lut *lut = gamma_lut;
>> +	enum pipe pipe = crtc->pipe;
>> +	u32 i, lut_size, word;
>> +
>> +	WARN_ON(offset & ~PAL_PREC_INDEX_VALUE_MASK);
>> +
>> +	I915_WRITE(PREC_PAL_INDEX(pipe),
>> +		   (offset ? PAL_PREC_SPLIT_MODE : 0) |
>> +		   PAL_PREC_AUTO_INCREMENT |
>> +		   offset);
>> +
>> +	if (lut && crtc_state->base.gamma_mode_type ==
>> +				MULTI_SEGMENTED_GAMMA_MODE_12BIT) {
>> +		lut_size = 9 + 514;
>> +		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_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_DATA(pipe), word);
>> +		}
>> +	}
>> +
>> +	/*
>> +	 * Program the max register to clamp values > 1.0.
>> +	 * ToDo: Extend the ABI to be able to program values
>> +	 * from 1.0
>> +	 */
>> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), (1 << 16));
>> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), (1 << 16));
>> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), (1 << 16));
>> +
>> +	/*
>> +	 * Program the max register to clamp values > 1.0.
>> +	 * ToDo: Extend the ABI to be able to program values
>> +	 * from 1.0 to 3.0
>> +	 */
>> +	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 0), (1 << 16));
>> +	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 1), (1 << 16));
>> +	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 2), (1 << 16));
>
>Doubled thing. But we can just drop these and call
>ivb_load_lut_10_max() from higher up instead.

Actually one is GC_MAX and another EXT_GC_MAX. There seem to be 3 set of
registers to be programed here.  Sure, I will use the ivb helper here to optimize this.

>> +
>> +	/*
>> +	 * Program the gc max 2 register to clamp values > 1.0.
>> +	 * ToDo: Extend the ABI to be able to program values
>> +	 * from 3.0 to 7.0
>> +	*/
>> +	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
>> +		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 0), (1 << 16));
>> +		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 1), (1 << 16));
>> +		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 2), (1 << 16));
>> +	}
>> +}
>> +
>> +static void icl_program_fine_segment_lut(const struct intel_crtc_state
>> +					 *crtc_state,
>> +					 struct drm_color_lut *gamma_lut,
>> +					 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, word, 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 (gamma_lut) {
>> +		struct drm_color_lut *lut =
>> +			(struct drm_color_lut *)gamma_lut;
>> +
>> +		for (i = 0; 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);
>> +		}
>> +	}
>> +}
>> +
>> +static void icl_load_gamma_multi_segmented_lut(const struct intel_crtc_state
>> +					       *crtc_state, u32 offset)
>> +{
>> +	const struct drm_property_blob *gamma_mode_blob =
>> +					crtc_state->base.gamma_mode;
>> +	struct drm_color_lut *gamma_lut;
>> +	int ret;
>> +
>> +	if (gamma_mode_blob) {
>> +		struct drm_color_mode_lut *arg = gamma_mode_blob->data;
>> +		u64 __user *props_ptr = (u64 __user *)(unsigned long)(arg->lut);
>> +
>> +		DRM_INFO("Gamma Mode=%s\n", arg->name);
>> +		gamma_lut = kmalloc((sizeof(struct drm_color_lut) * arg->count),
>> +				    GFP_KERNEL);
>> +		ret = copy_from_user(gamma_lut, props_ptr,
>> +				     sizeof(struct drm_color_lut) * arg->count);
>> +	}
>
>What's this doing here?

This is to get the lut values in driver from userspace. The blob passed from user is
struct drm_color_mode_lut {
       /* DRM_MODE_LUT_* */
       __u32 flags;
       /* number of points on the curve */
       __u32 count;
       /* Name of Gamma Mode */
       char name[DRM_PROP_NAME_LEN];
       /* Pointer to Lut elements */
       __u64 lut;
};

The lut values are passed as pointer in "lut", we are copying the same to kernel
memory space here.

>> +
>> +	icl_program_fine_segment_lut(crtc_state, gamma_lut, 0);
>> +	icl_program_coarse_segment_lut(crtc_state, gamma_lut, 0); }
>> +
>>  static void icl_load_luts(const struct intel_crtc_state *crtc_state)
>> {
>>  	if (crtc_state->base.degamma_lut)
>>  		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->base.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) @@ -1034,10 +1176,20 @@ static int glk_color_check(struct
>intel_crtc_state *crtc_state)
>>  	return 0;
>>  }
>>
>> -static u32 icl_gamma_mode(const struct intel_crtc_state *crtc_state)
>> +static u32 icl_gamma_mode(struct intel_crtc_state *crtc_state)
>>  {
>> +	const struct drm_property_blob *gamma_mode_blob =
>> +					crtc_state->base.gamma_mode;
>> +	struct drm_color_mode_lut *arg;
>>  	u32 gamma_mode = 0;
>>
>> +	if (gamma_mode_blob) {
>> +		arg = gamma_mode_blob->data;
>> +		if (!strcmp(arg->name, "multi-segmented gamma"))
>> +			crtc_state->base.gamma_mode_type =
>> +				MULTI_SEGMENTED_GAMMA_MODE_12BIT;
>> +	}
>
>With the blob_enum this kind of stuff should disappear I think.

I feel keeping 2 separate properties should be good as we can keep the capabilities
immutable. And with the above structure userspace should be able to set any supported
gamma_mode.  Let me know your opinion on the same.

Regards,
Uma Shankar

>> +
>>  	if (crtc_state->base.degamma_lut)
>>  		gamma_mode |= PRE_CSC_GAMMA_ENABLE;
>>
>> @@ -1048,6 +1200,9 @@ static u32 icl_gamma_mode(const struct intel_crtc_state
>*crtc_state)
>>  	if (!crtc_state->base.gamma_lut ||
>>  	    crtc_state_is_legacy_gamma(crtc_state))
>>  		gamma_mode |= GAMMA_MODE_MODE_8BIT;
>> +	else if (crtc_state->base.gamma_mode_type ==
>> +		 MULTI_SEGMENTED_GAMMA_MODE_12BIT)
>> +		gamma_mode |=
>GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED;
>>  	else
>>  		gamma_mode |= GAMMA_MODE_MODE_10BIT;
>>
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index
>> bc8a2e7..a8d0b4c 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -262,6 +262,9 @@ struct drm_crtc_state {
>>  	 */
>>  	struct drm_property_blob *gamma_mode;
>>
>> +	/* Gamma mode type programmed on the pipe */
>> +	u32 gamma_mode_type;
>> +
>>  	/**
>>  	 * @degamma_lut:
>>  	 *
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>--
>Ville Syrjälä
>Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[v2 6/7] drm/i915: Add gamma mode caps property

[Uma Shankar]

Create the gamma mode caps property and attach
to crtc.

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

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index d81de32..d7f835e 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -1528,6 +1528,8 @@ void intel_color_init(struct intel_crtc *crtc)
 		if (INTEL_GEN(dev_priv) >= 11) {
 			dev_priv->display.color_check = icl_color_check;
 
+			drm_crtc_attach_gamma_mode_caps_property(&crtc->base);
+
 			/* don't advertize the >= 1.0 entries */
 			degamma_lut_size = 0;
 			gamma_lut_size = ILK_LUT_SIZE_10BIT;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8576a7f..3b62b2b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15606,6 +15606,9 @@ int intel_modeset_init(struct drm_device *dev)
 		      INTEL_INFO(dev_priv)->num_pipes,
 		      INTEL_INFO(dev_priv)->num_pipes > 1 ? "s" : "");
 
+	if (INTEL_GEN(dev_priv) >= 11)
+		drm_color_create_gamma_mode_caps_property(&dev_priv->drm, 4);
+
 	for_each_pipe(dev_priv, pipe) {
 		ret = intel_crtc_init(dev_priv, pipe);
 		if (ret) {
-- 
1.9.1

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

[v2 7/7] drm/i915: Attach gamma mode property

[Uma Shankar]

Attach the gamma mode property to allow userspace
set the gamma mode and provide the luts for the
same.

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

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index d7f835e..95c49d4 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -1529,6 +1529,7 @@ void intel_color_init(struct intel_crtc *crtc)
 			dev_priv->display.color_check = icl_color_check;
 
 			drm_crtc_attach_gamma_mode_caps_property(&crtc->base);
+			drm_crtc_attach_gamma_mode_property(&crtc->base);
 
 			/* don't advertize the >= 1.0 entries */
 			degamma_lut_size = 0;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3b62b2b..4cab112 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15606,8 +15606,10 @@ int intel_modeset_init(struct drm_device *dev)
 		      INTEL_INFO(dev_priv)->num_pipes,
 		      INTEL_INFO(dev_priv)->num_pipes > 1 ? "s" : "");
 
-	if (INTEL_GEN(dev_priv) >= 11)
+	if (INTEL_GEN(dev_priv) >= 11) {
 		drm_color_create_gamma_mode_caps_property(&dev_priv->drm, 4);
+		drm_color_create_gamma_mode_property(&dev_priv->drm, 0);
+	}
 
 	for_each_pipe(dev_priv, pipe) {
 		ret = intel_crtc_init(dev_priv, pipe);
-- 
1.9.1

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

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

[Patchwork]

== Series Details ==

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

== Summary ==

$ dim checkpatch origin/drm-tip
75ad704a2b21 drm: Add gamma mode caps property
659fe8427470 drm/i915: Define color lut range structure
-:61: ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#61: FILE: drivers/gpu/drm/i915/intel_color.c:1124:
+#define I965_GAMMA_10 \
+	{ \
+		.flags = (DRM_MODE_LUT_GAMMA | \
+			  DRM_MODE_LUT_INTERPOLATE | \
+			  DRM_MODE_LUT_NON_DECREASING), \
+		.count = 128, \
+		.input_bpc = 10, .output_bpc = 16, \
+		.start = 0, .end = (1 << 10) - (1 << 10) / 128, \
+		.min = 0, .max = (1 << 16) - 1, \
+	}, \
+	/* PIPEGCMAX */ \
+	{ \
+		.flags = (DRM_MODE_LUT_GAMMA | \
+			  DRM_MODE_LUT_INTERPOLATE | \
+			  DRM_MODE_LUT_REUSE_LAST | \
+			  DRM_MODE_LUT_NON_DECREASING), \
+		.count = 1, \
+		.input_bpc = 10, .output_bpc = 16, \
+		.start = (1 << 10) - (1 << 10) / 128, .end = 1 << 10, \
+		.min = 0, .max = 1 << 16, \
+	}

total: 1 errors, 0 warnings, 0 checks, 320 lines checked
a059b6bf4fe1 drm: Add gamma mode property
-:26: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#26: FILE: drivers/gpu/drm/drm_atomic_uapi.c:464:
+		ret = drm_atomic_replace_property_blob_from_id(dev,
+					&state->gamma_mode,

total: 0 errors, 0 warnings, 1 checks, 110 lines checked
a5deda67bb81 drm/i915/icl: Add register definitions for Multi Segmented gamma
3b03f07ba428 drm/i915/icl: Add support for multi segmented gamma mode
-:79: ERROR:TRAILING_WHITESPACE: trailing whitespace
#79: FILE: drivers/gpu/drm/i915/intel_color.c:719:
+^I$

-:102: WARNING:BLOCK_COMMENT_STYLE: Block comments should align the * on each line
#102: FILE: drivers/gpu/drm/i915/intel_color.c:742:
+	 * from 3.0 to 7.0
+	*/

total: 1 errors, 1 warnings, 0 checks, 211 lines checked
7e97b366c05e drm/i915: Add gamma mode caps property
a7d35ed03858 drm/i915: Attach gamma mode property

_______________________________________________
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 (rev2)

[Patchwork]

== Series Details ==

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

== Summary ==

CI Bug Log - changes from CI_DRM_5854 -> Patchwork_12653
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_12653 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_12653, 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/2/mbox/

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

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

### IGT changes ###

#### Possible regressions ####

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

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

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

### IGT changes ###

#### Issues hit ####

  * igt@kms_pipe_crc_basic@read-crc-pipe-b:
    - fi-byt-clapper:     PASS -> FAIL [fdo#107362]

  * igt@runner@aborted:
    - fi-icl-u2:          NOTRUN -> FAIL [k.org#202973]
    - fi-apl-guc:         NOTRUN -> FAIL [fdo#108622] / [fdo#109720]

  
#### Possible fixes ####

  * igt@i915_selftest@live_contexts:
    - fi-skl-gvtdvm:      DMESG-FAIL [fdo#110235 ] -> PASS

  * igt@i915_selftest@live_uncore:
    - fi-ivb-3770:        DMESG-FAIL [fdo#110210] -> PASS

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
    - fi-byt-clapper:     FAIL [fdo#103191] / [fdo#107362] -> PASS

  
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#108622]: https://bugs.freedesktop.org/show_bug.cgi?id=108622
  [fdo#109720]: https://bugs.freedesktop.org/show_bug.cgi?id=109720
  [fdo#110210]: https://bugs.freedesktop.org/show_bug.cgi?id=110210
  [fdo#110235 ]: https://bugs.freedesktop.org/show_bug.cgi?id=110235 
  [k.org#202973]: https://bugzilla.kernel.org/show_bug.cgi?id=202973


Participating hosts (51 -> 40)
------------------------------

  Missing    (11): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-j1900 fi-hsw-peppy fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-gdg-551 fi-pnv-d510 fi-bdw-samus 


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

    * Linux: CI_DRM_5854 -> Patchwork_12653

  CI_DRM_5854: 3b59d4fb5ba3fb53a87c7a4495d81b0a25aa96eb @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4919: f539e21e934019f0196fee646f351b4e30a8c341 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12653: a7d35ed03858e1ceb89578d7bf0bb7d9196e0a9a @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

a7d35ed03858 drm/i915: Attach gamma mode property
7e97b366c05e drm/i915: Add gamma mode caps property
3b03f07ba428 drm/i915/icl: Add support for multi segmented gamma mode
a5deda67bb81 drm/i915/icl: Add register definitions for Multi Segmented gamma
a059b6bf4fe1 drm: Add gamma mode property
659fe8427470 drm/i915: Define color lut range structure
75ad704a2b21 drm: Add gamma mode caps property

== Logs ==

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

Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support

[Ville Syrjälä]

On Mon, Apr 01, 2019 at 11:00:04PM +0530, Uma Shankar wrote:
> This series adds support for programmable gamma modes and
> exposes a property interface for the same. Also added,
> support for multi segment gamma mode introduced in ICL+
> 
> It creates 2 property interfaces :
> 1. GAMMA_MODE_CAPS: This is immutable property and exposes
> the various gamma modes supported and the lut ranges. This
> is an enum property with element as blob id. Getting the
> blob id in userspace, user can get the mode supported and
> also the range of gamma mode supported with number of lut
> coefficients.
> 
> 2. GAMMA_MODE: This is for user to set the gamma mode and
> send the lut values for that particular mode.

I think we should just go for the BLOB_ENUM prop type instead.
Then the possible values and the current value are all part
of the same prop.

> 
> v2: Used Ville's design and approach to define the interfaces.
> Addressed Matt Roper's review feedback and re-ordered the
> patches.
> 
> Uma Shankar (5):
>   drm: Add gamma mode property
>   drm/i915/icl: Add register definitions for Multi Segmented gamma
>   drm/i915/icl: Add support for multi segmented gamma mode
>   drm/i915: Add gamma mode caps property
>   drm/i915: Attach gamma mode property
> 
> Ville Syrjälä (2):
>   drm: Add gamma mode caps property
>   drm/i915: Define color lut range structure
> 
>  drivers/gpu/drm/drm_atomic_uapi.c    |  13 +
>  drivers/gpu/drm/drm_color_mgmt.c     | 110 +++++++++
>  drivers/gpu/drm/i915/i915_reg.h      |  17 ++
>  drivers/gpu/drm/i915/intel_color.c   | 465 ++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_display.c |   5 +
>  include/drm/drm_color_mgmt.h         |  11 +
>  include/drm/drm_crtc.h               |  17 ++
>  include/drm/drm_mode_config.h        |  10 +
>  include/uapi/drm/drm_mode.h          |  49 ++++
>  9 files changed, 690 insertions(+), 7 deletions(-)
> 
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

RE: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support

[Shankar, Uma]

>-----Original Message-----
>From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of Ville
>Syrjälä
>Sent: Friday, April 5, 2019 9:42 PM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri-
>devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville
><ville.syrjala@intel.com>; Lankhorst, Maarten <maarten.lankhorst@intel.com>
>Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support
>
>On Mon, Apr 01, 2019 at 11:00:04PM +0530, Uma Shankar wrote:
>> This series adds support for programmable gamma modes and exposes a
>> property interface for the same. Also added, support for multi segment
>> gamma mode introduced in ICL+
>>
>> It creates 2 property interfaces :
>> 1. GAMMA_MODE_CAPS: This is immutable property and exposes the various
>> gamma modes supported and the lut ranges. This is an enum property
>> with element as blob id. Getting the blob id in userspace, user can
>> get the mode supported and also the range of gamma mode supported with
>> number of lut coefficients.
>>
>> 2. GAMMA_MODE: This is for user to set the gamma mode and send the lut
>> values for that particular mode.
>
>I think we should just go for the BLOB_ENUM prop type instead.
>Then the possible values and the current value are all part of the same prop.

Hi Ville,
With the current approach, we have enum property with values as blob_ids 
(representing platform capabilities). This should not get modified and needs to
be immutable.

Userspace can query the property and get the blob using the blob_ids. Thereby
getting all the platform capabilities.

Now to set the LUT values, he can use another blob property and pass the
luts.  This is inline to how gamma/degamma is implemented where we have
one immutable LUT_SIZE property (indicating number of luts) and another blob
property for actual lut values.

Regards,
Uma Shankar

>>
>> v2: Used Ville's design and approach to define the interfaces.
>> Addressed Matt Roper's review feedback and re-ordered the patches.
>>
>> Uma Shankar (5):
>>   drm: Add gamma mode property
>>   drm/i915/icl: Add register definitions for Multi Segmented gamma
>>   drm/i915/icl: Add support for multi segmented gamma mode
>>   drm/i915: Add gamma mode caps property
>>   drm/i915: Attach gamma mode property
>>
>> Ville Syrjälä (2):
>>   drm: Add gamma mode caps property
>>   drm/i915: Define color lut range structure
>>
>>  drivers/gpu/drm/drm_atomic_uapi.c    |  13 +
>>  drivers/gpu/drm/drm_color_mgmt.c     | 110 +++++++++
>>  drivers/gpu/drm/i915/i915_reg.h      |  17 ++
>>  drivers/gpu/drm/i915/intel_color.c   | 465
>++++++++++++++++++++++++++++++++++-
>>  drivers/gpu/drm/i915/intel_display.c |   5 +
>>  include/drm/drm_color_mgmt.h         |  11 +
>>  include/drm/drm_crtc.h               |  17 ++
>>  include/drm/drm_mode_config.h        |  10 +
>>  include/uapi/drm/drm_mode.h          |  49 ++++
>>  9 files changed, 690 insertions(+), 7 deletions(-)
>>
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>--
>Ville Syrjälä
>Intel
>_______________________________________________
>dri-devel mailing list
>dri-devel@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support

[Ville Syrjälä]

On Mon, Apr 08, 2019 at 12:26:23PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of Ville
> >Syrjälä
> >Sent: Friday, April 5, 2019 9:42 PM
> >To: Shankar, Uma <uma.shankar@intel.com>
> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri-
> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville
> ><ville.syrjala@intel.com>; Lankhorst, Maarten <maarten.lankhorst@intel.com>
> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support
> >
> >On Mon, Apr 01, 2019 at 11:00:04PM +0530, Uma Shankar wrote:
> >> This series adds support for programmable gamma modes and exposes a
> >> property interface for the same. Also added, support for multi segment
> >> gamma mode introduced in ICL+
> >>
> >> It creates 2 property interfaces :
> >> 1. GAMMA_MODE_CAPS: This is immutable property and exposes the various
> >> gamma modes supported and the lut ranges. This is an enum property
> >> with element as blob id. Getting the blob id in userspace, user can
> >> get the mode supported and also the range of gamma mode supported with
> >> number of lut coefficients.
> >>
> >> 2. GAMMA_MODE: This is for user to set the gamma mode and send the lut
> >> values for that particular mode.
> >
> >I think we should just go for the BLOB_ENUM prop type instead.
> >Then the possible values and the current value are all part of the same prop.
> 
> Hi Ville,
> With the current approach, we have enum property with values as blob_ids 
> (representing platform capabilities). This should not get modified and needs to
> be immutable.

That's not quite what we want. We want to let the user modify the
current value so that they can actually select the gamma mode.
Otherwise we need yet another prop for it, or we have to deduce it
from the LUT size (that apporach would actually work for i915 but
may not work for other drivers/hardware).

> 
> Userspace can query the property and get the blob using the blob_ids. Thereby
> getting all the platform capabilities.
> 
> Now to set the LUT values, he can use another blob property and pass the
> luts.  This is inline to how gamma/degamma is implemented where we have
> one immutable LUT_SIZE property (indicating number of luts) and another blob
> property for actual lut values.
> 
> Regards,
> Uma Shankar
> 
> >>
> >> v2: Used Ville's design and approach to define the interfaces.
> >> Addressed Matt Roper's review feedback and re-ordered the patches.
> >>
> >> Uma Shankar (5):
> >>   drm: Add gamma mode property
> >>   drm/i915/icl: Add register definitions for Multi Segmented gamma
> >>   drm/i915/icl: Add support for multi segmented gamma mode
> >>   drm/i915: Add gamma mode caps property
> >>   drm/i915: Attach gamma mode property
> >>
> >> Ville Syrjälä (2):
> >>   drm: Add gamma mode caps property
> >>   drm/i915: Define color lut range structure
> >>
> >>  drivers/gpu/drm/drm_atomic_uapi.c    |  13 +
> >>  drivers/gpu/drm/drm_color_mgmt.c     | 110 +++++++++
> >>  drivers/gpu/drm/i915/i915_reg.h      |  17 ++
> >>  drivers/gpu/drm/i915/intel_color.c   | 465
> >++++++++++++++++++++++++++++++++++-
> >>  drivers/gpu/drm/i915/intel_display.c |   5 +
> >>  include/drm/drm_color_mgmt.h         |  11 +
> >>  include/drm/drm_crtc.h               |  17 ++
> >>  include/drm/drm_mode_config.h        |  10 +
> >>  include/uapi/drm/drm_mode.h          |  49 ++++
> >>  9 files changed, 690 insertions(+), 7 deletions(-)
> >>
> >> --
> >> 1.9.1
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> >--
> >Ville Syrjälä
> >Intel
> >_______________________________________________
> >dri-devel mailing list
> >dri-devel@lists.freedesktop.org
> >https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [v2 0/7] Add Multi Segment Gamma Support

[Shankar, Uma]

>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Monday, April 8, 2019 6:01 PM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri-
>devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville
><ville.syrjala@intel.com>; Lankhorst, Maarten <maarten.lankhorst@intel.com>
>Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support
>
>On Mon, Apr 08, 2019 at 12:26:23PM +0000, Shankar, Uma wrote:
>>
>>
>> >-----Original Message-----
>> >From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On
>> >Behalf Of Ville Syrjälä
>> >Sent: Friday, April 5, 2019 9:42 PM
>> >To: Shankar, Uma <uma.shankar@intel.com>
>> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri-
>> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville
>> ><ville.syrjala@intel.com>; Lankhorst, Maarten
>> ><maarten.lankhorst@intel.com>
>> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support
>> >
>> >On Mon, Apr 01, 2019 at 11:00:04PM +0530, Uma Shankar wrote:
>> >> This series adds support for programmable gamma modes and exposes a
>> >> property interface for the same. Also added, support for multi
>> >> segment gamma mode introduced in ICL+
>> >>
>> >> It creates 2 property interfaces :
>> >> 1. GAMMA_MODE_CAPS: This is immutable property and exposes the
>> >> various gamma modes supported and the lut ranges. This is an enum
>> >> property with element as blob id. Getting the blob id in userspace,
>> >> user can get the mode supported and also the range of gamma mode
>> >> supported with number of lut coefficients.
>> >>
>> >> 2. GAMMA_MODE: This is for user to set the gamma mode and send the
>> >> lut values for that particular mode.
>> >
>> >I think we should just go for the BLOB_ENUM prop type instead.
>> >Then the possible values and the current value are all part of the same prop.
>>
>> Hi Ville,
>> With the current approach, we have enum property with values as
>> blob_ids (representing platform capabilities). This should not get
>> modified and needs to be immutable.
>
>That's not quite what we want. We want to let the user modify the current value so
>that they can actually select the gamma mode.
>Otherwise we need yet another prop for it, or we have to deduce it from the LUT size
>(that apporach would actually work for i915 but may not work for other
>drivers/hardware).
>
>>
>> Userspace can query the property and get the blob using the blob_ids.
>> Thereby getting all the platform capabilities.
>>
>> Now to set the LUT values, he can use another blob property and pass
>> the luts.  This is inline to how gamma/degamma is implemented where we
>> have one immutable LUT_SIZE property (indicating number of luts) and
>> another blob property for actual lut values..

Hi Ville,
Just to clarify and clear some doubts :)

We should be able to set the gamma mode using the blob enum value.  Userspace will get the list
enum vals (blob ids with mode name embedded) and select one and do a setprop to set a mode.
Driver will get the blob_id and will be able to get the mode to be set.  So exposing capabilities and
setting the mode should be possible with this one property. I hope my understanding is correct.

Now to send the actual blob values to be set, we need to use some other property interface. Should we
use the currently available  "gamma blob (gamma_lut_property)" property to send the lut values. 
The challenge there is that it currently uses 16 bit lut values 
struct drm_color_lut {
        __u16 red;
        __u16 green;
        __u16 blue;
        __u16 reserved;
};
which is not sufficient for HDR usecases. Or should we need a new property for advance lut/extended lut like below:
https://patchwork.freedesktop.org/patch/294732/?series=30875&rev=7

What do you suggest ?

Note: I have currently used 16bit values only to get the feedback on the approach.

Regards,
Uma Shankar

>>
>> >>
>> >> v2: Used Ville's design and approach to define the interfaces.
>> >> Addressed Matt Roper's review feedback and re-ordered the patches.
>> >>
>> >> Uma Shankar (5):
>> >>   drm: Add gamma mode property
>> >>   drm/i915/icl: Add register definitions for Multi Segmented gamma
>> >>   drm/i915/icl: Add support for multi segmented gamma mode
>> >>   drm/i915: Add gamma mode caps property
>> >>   drm/i915: Attach gamma mode property
>> >>
>> >> Ville Syrjälä (2):
>> >>   drm: Add gamma mode caps property
>> >>   drm/i915: Define color lut range structure
>> >>
>> >>  drivers/gpu/drm/drm_atomic_uapi.c    |  13 +
>> >>  drivers/gpu/drm/drm_color_mgmt.c     | 110 +++++++++
>> >>  drivers/gpu/drm/i915/i915_reg.h      |  17 ++
>> >>  drivers/gpu/drm/i915/intel_color.c   | 465
>> >++++++++++++++++++++++++++++++++++-
>> >>  drivers/gpu/drm/i915/intel_display.c |   5 +
>> >>  include/drm/drm_color_mgmt.h         |  11 +
>> >>  include/drm/drm_crtc.h               |  17 ++
>> >>  include/drm/drm_mode_config.h        |  10 +
>> >>  include/uapi/drm/drm_mode.h          |  49 ++++
>> >>  9 files changed, 690 insertions(+), 7 deletions(-)
>> >>
>> >> --
>> >> 1.9.1
>> >>
>> >> _______________________________________________
>> >> Intel-gfx mailing list
>> >> Intel-gfx@lists.freedesktop.org
>> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >
>> >--
>> >Ville Syrjälä
>> >Intel
>> >_______________________________________________
>> >dri-devel mailing list
>> >dri-devel@lists.freedesktop.org
>> >https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>--
>Ville Syrjälä
>Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support

[Ville Syrjälä]

On Mon, Apr 08, 2019 at 02:40:51PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >Sent: Monday, April 8, 2019 6:01 PM
> >To: Shankar, Uma <uma.shankar@intel.com>
> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri-
> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville
> ><ville.syrjala@intel.com>; Lankhorst, Maarten <maarten.lankhorst@intel.com>
> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support
> >
> >On Mon, Apr 08, 2019 at 12:26:23PM +0000, Shankar, Uma wrote:
> >>
> >>
> >> >-----Original Message-----
> >> >From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On
> >> >Behalf Of Ville Syrjälä
> >> >Sent: Friday, April 5, 2019 9:42 PM
> >> >To: Shankar, Uma <uma.shankar@intel.com>
> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri-
> >> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville
> >> ><ville.syrjala@intel.com>; Lankhorst, Maarten
> >> ><maarten.lankhorst@intel.com>
> >> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support
> >> >
> >> >On Mon, Apr 01, 2019 at 11:00:04PM +0530, Uma Shankar wrote:
> >> >> This series adds support for programmable gamma modes and exposes a
> >> >> property interface for the same. Also added, support for multi
> >> >> segment gamma mode introduced in ICL+
> >> >>
> >> >> It creates 2 property interfaces :
> >> >> 1. GAMMA_MODE_CAPS: This is immutable property and exposes the
> >> >> various gamma modes supported and the lut ranges. This is an enum
> >> >> property with element as blob id. Getting the blob id in userspace,
> >> >> user can get the mode supported and also the range of gamma mode
> >> >> supported with number of lut coefficients.
> >> >>
> >> >> 2. GAMMA_MODE: This is for user to set the gamma mode and send the
> >> >> lut values for that particular mode.
> >> >
> >> >I think we should just go for the BLOB_ENUM prop type instead.
> >> >Then the possible values and the current value are all part of the same prop.
> >>
> >> Hi Ville,
> >> With the current approach, we have enum property with values as
> >> blob_ids (representing platform capabilities). This should not get
> >> modified and needs to be immutable.
> >
> >That's not quite what we want. We want to let the user modify the current value so
> >that they can actually select the gamma mode.
> >Otherwise we need yet another prop for it, or we have to deduce it from the LUT size
> >(that apporach would actually work for i915 but may not work for other
> >drivers/hardware).
> >
> >>
> >> Userspace can query the property and get the blob using the blob_ids.
> >> Thereby getting all the platform capabilities.
> >>
> >> Now to set the LUT values, he can use another blob property and pass
> >> the luts.  This is inline to how gamma/degamma is implemented where we
> >> have one immutable LUT_SIZE property (indicating number of luts) and
> >> another blob property for actual lut values..
> 
> Hi Ville,
> Just to clarify and clear some doubts :)
> 
> We should be able to set the gamma mode using the blob enum value.  Userspace will get the list
> enum vals (blob ids with mode name embedded) and select one and do a setprop to set a mode.
> Driver will get the blob_id and will be able to get the mode to be set.  So exposing capabilities and
> setting the mode should be possible with this one property. I hope my understanding is correct.
> 
> Now to send the actual blob values to be set, we need to use some other property interface. Should we
> use the currently available  "gamma blob (gamma_lut_property)" property to send the lut values. 
> The challenge there is that it currently uses 16 bit lut values 
> struct drm_color_lut {
>         __u16 red;
>         __u16 green;
>         __u16 blue;
>         __u16 reserved;
> };
> which is not sufficient for HDR usecases. Or should we need a new property for advance lut/extended lut like below:
> https://patchwork.freedesktop.org/patch/294732/?series=30875&rev=7
> 
> What do you suggest ?

I was thinking that we might get away with reusing the current props and
just change the interpretation of the data when gamma_mode is set. But
I'm not sure that's going to work out so well if one client sets this up
and then another client comes along that doesn't understand the new
props at all. But even with separate props I think we might still end up
in a mess because the new client wouldn't know how to unset the higher
precision LUT before setting up the old style prop and the kernel would
then refuse the operation with with both props being set.

So I think we might need a client cap for this which simply changes how
the data in the existing props is represented. So internally we could
always store things in the new high precision format, but we'd convert
to/from the old format when dealing with an older client.

> 
> Note: I have currently used 16bit values only to get the feedback on the approach.
> 
> Regards,
> Uma Shankar
> 
> >>
> >> >>
> >> >> v2: Used Ville's design and approach to define the interfaces.
> >> >> Addressed Matt Roper's review feedback and re-ordered the patches.
> >> >>
> >> >> Uma Shankar (5):
> >> >>   drm: Add gamma mode property
> >> >>   drm/i915/icl: Add register definitions for Multi Segmented gamma
> >> >>   drm/i915/icl: Add support for multi segmented gamma mode
> >> >>   drm/i915: Add gamma mode caps property
> >> >>   drm/i915: Attach gamma mode property
> >> >>
> >> >> Ville Syrjälä (2):
> >> >>   drm: Add gamma mode caps property
> >> >>   drm/i915: Define color lut range structure
> >> >>
> >> >>  drivers/gpu/drm/drm_atomic_uapi.c    |  13 +
> >> >>  drivers/gpu/drm/drm_color_mgmt.c     | 110 +++++++++
> >> >>  drivers/gpu/drm/i915/i915_reg.h      |  17 ++
> >> >>  drivers/gpu/drm/i915/intel_color.c   | 465
> >> >++++++++++++++++++++++++++++++++++-
> >> >>  drivers/gpu/drm/i915/intel_display.c |   5 +
> >> >>  include/drm/drm_color_mgmt.h         |  11 +
> >> >>  include/drm/drm_crtc.h               |  17 ++
> >> >>  include/drm/drm_mode_config.h        |  10 +
> >> >>  include/uapi/drm/drm_mode.h          |  49 ++++
> >> >>  9 files changed, 690 insertions(+), 7 deletions(-)
> >> >>
> >> >> --
> >> >> 1.9.1
> >> >>
> >> >> _______________________________________________
> >> >> Intel-gfx mailing list
> >> >> Intel-gfx@lists.freedesktop.org
> >> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >> >
> >> >--
> >> >Ville Syrjälä
> >> >Intel
> >> >_______________________________________________
> >> >dri-devel mailing list
> >> >dri-devel@lists.freedesktop.org
> >> >https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> >--
> >Ville Syrjälä
> >Intel

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

RE: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support

[Shankar, Uma]

>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Monday, April 8, 2019 8:27 PM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri-
>devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville
><ville.syrjala@intel.com>; Lankhorst, Maarten <maarten.lankhorst@intel.com>
>Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support
>
>On Mon, Apr 08, 2019 at 02:40:51PM +0000, Shankar, Uma wrote:
>>
>>
>> >-----Original Message-----
>> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>> >Sent: Monday, April 8, 2019 6:01 PM
>> >To: Shankar, Uma <uma.shankar@intel.com>
>> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri-
>> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville
>> ><ville.syrjala@intel.com>; Lankhorst, Maarten
>> ><maarten.lankhorst@intel.com>
>> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support
>> >
>> >On Mon, Apr 08, 2019 at 12:26:23PM +0000, Shankar, Uma wrote:
>> >>
>> >>
>> >> >-----Original Message-----
>> >> >From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org]
>> >> >On Behalf Of Ville Syrjälä
>> >> >Sent: Friday, April 5, 2019 9:42 PM
>> >> >To: Shankar, Uma <uma.shankar@intel.com>
>> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri-
>> >> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville
>> >> ><ville.syrjala@intel.com>; Lankhorst, Maarten
>> >> ><maarten.lankhorst@intel.com>
>> >> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support
>> >> >
>> >> >On Mon, Apr 01, 2019 at 11:00:04PM +0530, Uma Shankar wrote:
>> >> >> This series adds support for programmable gamma modes and
>> >> >> exposes a property interface for the same. Also added, support
>> >> >> for multi segment gamma mode introduced in ICL+
>> >> >>
>> >> >> It creates 2 property interfaces :
>> >> >> 1. GAMMA_MODE_CAPS: This is immutable property and exposes the
>> >> >> various gamma modes supported and the lut ranges. This is an
>> >> >> enum property with element as blob id. Getting the blob id in
>> >> >> userspace, user can get the mode supported and also the range of
>> >> >> gamma mode supported with number of lut coefficients.
>> >> >>
>> >> >> 2. GAMMA_MODE: This is for user to set the gamma mode and send
>> >> >> the lut values for that particular mode.
>> >> >
>> >> >I think we should just go for the BLOB_ENUM prop type instead.
>> >> >Then the possible values and the current value are all part of the same prop.
>> >>
>> >> Hi Ville,
>> >> With the current approach, we have enum property with values as
>> >> blob_ids (representing platform capabilities). This should not get
>> >> modified and needs to be immutable.
>> >
>> >That's not quite what we want. We want to let the user modify the
>> >current value so that they can actually select the gamma mode.
>> >Otherwise we need yet another prop for it, or we have to deduce it
>> >from the LUT size (that apporach would actually work for i915 but may
>> >not work for other drivers/hardware).
>> >
>> >>
>> >> Userspace can query the property and get the blob using the blob_ids.
>> >> Thereby getting all the platform capabilities.
>> >>
>> >> Now to set the LUT values, he can use another blob property and
>> >> pass the luts.  This is inline to how gamma/degamma is implemented
>> >> where we have one immutable LUT_SIZE property (indicating number of
>> >> luts) and another blob property for actual lut values..
>>
>> Hi Ville,
>> Just to clarify and clear some doubts :)
>>
>> We should be able to set the gamma mode using the blob enum value.
>> Userspace will get the list enum vals (blob ids with mode name embedded) and
>select one and do a setprop to set a mode.
>> Driver will get the blob_id and will be able to get the mode to be
>> set.  So exposing capabilities and setting the mode should be possible with this one
>property. I hope my understanding is correct.
>>
>> Now to send the actual blob values to be set, we need to use some
>> other property interface. Should we use the currently available  "gamma blob
>(gamma_lut_property)" property to send the lut values.
>> The challenge there is that it currently uses 16 bit lut values struct
>> drm_color_lut {
>>         __u16 red;
>>         __u16 green;
>>         __u16 blue;
>>         __u16 reserved;
>> };
>> which is not sufficient for HDR usecases. Or should we need a new property for
>advance lut/extended lut like below:
>> https://patchwork.freedesktop.org/patch/294732/?series=30875&rev=7
>>
>> What do you suggest ?
>
>I was thinking that we might get away with reusing the current props and just change
>the interpretation of the data when gamma_mode is set. But I'm not sure that's going
>to work out so well if one client sets this up and then another client comes along that
>doesn't understand the new props at all. But even with separate props I think we
>might still end up in a mess because the new client wouldn't know how to unset the
>higher precision LUT before setting up the old style prop and the kernel would then
>refuse the operation with with both props being set.
>
>So I think we might need a client cap for this which simply changes how the data in
>the existing props is represented. So internally we could always store things in the
>new high precision format, but we'd convert to/from the old format when dealing
>with an older client.

We could also say that if a legacy gamma_mode_property is set (which will be used by
legacy apps or apps not aware of new interface), in driver we will simply unset the
earlier gamma_mode and fallback to legacy mode (whatever it was for a particular
platform). This way we should be able to deal with this situation and an explicit unset
may not be needed. What do you think ?

>>
>> Note: I have currently used 16bit values only to get the feedback on the approach.
>>
>> Regards,
>> Uma Shankar
>>
>> >>
>> >> >>
>> >> >> v2: Used Ville's design and approach to define the interfaces.
>> >> >> Addressed Matt Roper's review feedback and re-ordered the patches.
>> >> >>
>> >> >> Uma Shankar (5):
>> >> >>   drm: Add gamma mode property
>> >> >>   drm/i915/icl: Add register definitions for Multi Segmented gamma
>> >> >>   drm/i915/icl: Add support for multi segmented gamma mode
>> >> >>   drm/i915: Add gamma mode caps property
>> >> >>   drm/i915: Attach gamma mode property
>> >> >>
>> >> >> Ville Syrjälä (2):
>> >> >>   drm: Add gamma mode caps property
>> >> >>   drm/i915: Define color lut range structure
>> >> >>
>> >> >>  drivers/gpu/drm/drm_atomic_uapi.c    |  13 +
>> >> >>  drivers/gpu/drm/drm_color_mgmt.c     | 110 +++++++++
>> >> >>  drivers/gpu/drm/i915/i915_reg.h      |  17 ++
>> >> >>  drivers/gpu/drm/i915/intel_color.c   | 465
>> >> >++++++++++++++++++++++++++++++++++-
>> >> >>  drivers/gpu/drm/i915/intel_display.c |   5 +
>> >> >>  include/drm/drm_color_mgmt.h         |  11 +
>> >> >>  include/drm/drm_crtc.h               |  17 ++
>> >> >>  include/drm/drm_mode_config.h        |  10 +
>> >> >>  include/uapi/drm/drm_mode.h          |  49 ++++
>> >> >>  9 files changed, 690 insertions(+), 7 deletions(-)
>> >> >>
>> >> >> --
>> >> >> 1.9.1
>> >> >>
>> >> >> _______________________________________________
>> >> >> Intel-gfx mailing list
>> >> >> Intel-gfx@lists.freedesktop.org
>> >> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >> >
>> >> >--
>> >> >Ville Syrjälä
>> >> >Intel
>> >> >_______________________________________________
>> >> >dri-devel mailing list
>> >> >dri-devel@lists.freedesktop.org
>> >> >https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> >
>> >--
>> >Ville Syrjälä
>> >Intel
>
>--
>Ville Syrjälä
>Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [v2 0/7] Add Multi Segment Gamma Support

[Ville Syrjälä]

On Mon, Apr 08, 2019 at 03:40:39PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >Sent: Monday, April 8, 2019 8:27 PM
> >To: Shankar, Uma <uma.shankar@intel.com>
> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri-
> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville
> ><ville.syrjala@intel.com>; Lankhorst, Maarten <maarten.lankhorst@intel.com>
> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support
> >
> >On Mon, Apr 08, 2019 at 02:40:51PM +0000, Shankar, Uma wrote:
> >>
> >>
> >> >-----Original Message-----
> >> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >> >Sent: Monday, April 8, 2019 6:01 PM
> >> >To: Shankar, Uma <uma.shankar@intel.com>
> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri-
> >> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville
> >> ><ville.syrjala@intel.com>; Lankhorst, Maarten
> >> ><maarten.lankhorst@intel.com>
> >> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support
> >> >
> >> >On Mon, Apr 08, 2019 at 12:26:23PM +0000, Shankar, Uma wrote:
> >> >>
> >> >>
> >> >> >-----Original Message-----
> >> >> >From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org]
> >> >> >On Behalf Of Ville Syrjälä
> >> >> >Sent: Friday, April 5, 2019 9:42 PM
> >> >> >To: Shankar, Uma <uma.shankar@intel.com>
> >> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri-
> >> >> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville
> >> >> ><ville.syrjala@intel.com>; Lankhorst, Maarten
> >> >> ><maarten.lankhorst@intel.com>
> >> >> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support
> >> >> >
> >> >> >On Mon, Apr 01, 2019 at 11:00:04PM +0530, Uma Shankar wrote:
> >> >> >> This series adds support for programmable gamma modes and
> >> >> >> exposes a property interface for the same. Also added, support
> >> >> >> for multi segment gamma mode introduced in ICL+
> >> >> >>
> >> >> >> It creates 2 property interfaces :
> >> >> >> 1. GAMMA_MODE_CAPS: This is immutable property and exposes the
> >> >> >> various gamma modes supported and the lut ranges. This is an
> >> >> >> enum property with element as blob id. Getting the blob id in
> >> >> >> userspace, user can get the mode supported and also the range of
> >> >> >> gamma mode supported with number of lut coefficients.
> >> >> >>
> >> >> >> 2. GAMMA_MODE: This is for user to set the gamma mode and send
> >> >> >> the lut values for that particular mode.
> >> >> >
> >> >> >I think we should just go for the BLOB_ENUM prop type instead.
> >> >> >Then the possible values and the current value are all part of the same prop.
> >> >>
> >> >> Hi Ville,
> >> >> With the current approach, we have enum property with values as
> >> >> blob_ids (representing platform capabilities). This should not get
> >> >> modified and needs to be immutable.
> >> >
> >> >That's not quite what we want. We want to let the user modify the
> >> >current value so that they can actually select the gamma mode.
> >> >Otherwise we need yet another prop for it, or we have to deduce it
> >> >from the LUT size (that apporach would actually work for i915 but may
> >> >not work for other drivers/hardware).
> >> >
> >> >>
> >> >> Userspace can query the property and get the blob using the blob_ids.
> >> >> Thereby getting all the platform capabilities.
> >> >>
> >> >> Now to set the LUT values, he can use another blob property and
> >> >> pass the luts.  This is inline to how gamma/degamma is implemented
> >> >> where we have one immutable LUT_SIZE property (indicating number of
> >> >> luts) and another blob property for actual lut values..
> >>
> >> Hi Ville,
> >> Just to clarify and clear some doubts :)
> >>
> >> We should be able to set the gamma mode using the blob enum value.
> >> Userspace will get the list enum vals (blob ids with mode name embedded) and
> >select one and do a setprop to set a mode.
> >> Driver will get the blob_id and will be able to get the mode to be
> >> set.  So exposing capabilities and setting the mode should be possible with this one
> >property. I hope my understanding is correct.
> >>
> >> Now to send the actual blob values to be set, we need to use some
> >> other property interface. Should we use the currently available  "gamma blob
> >(gamma_lut_property)" property to send the lut values.
> >> The challenge there is that it currently uses 16 bit lut values struct
> >> drm_color_lut {
> >>         __u16 red;
> >>         __u16 green;
> >>         __u16 blue;
> >>         __u16 reserved;
> >> };
> >> which is not sufficient for HDR usecases. Or should we need a new property for
> >advance lut/extended lut like below:
> >> https://patchwork.freedesktop.org/patch/294732/?series=30875&rev=7
> >>
> >> What do you suggest ?
> >
> >I was thinking that we might get away with reusing the current props and just change
> >the interpretation of the data when gamma_mode is set. But I'm not sure that's going
> >to work out so well if one client sets this up and then another client comes along that
> >doesn't understand the new props at all. But even with separate props I think we
> >might still end up in a mess because the new client wouldn't know how to unset the
> >higher precision LUT before setting up the old style prop and the kernel would then
> >refuse the operation with with both props being set.
> >
> >So I think we might need a client cap for this which simply changes how the data in
> >the existing props is represented. So internally we could always store things in the
> >new high precision format, but we'd convert to/from the old format when dealing
> >with an older client.
> 
> We could also say that if a legacy gamma_mode_property is set (which will be used by
> legacy apps or apps not aware of new interface), in driver we will simply unset the
> earlier gamma_mode and fallback to legacy mode (whatever it was for a particular
> platform). This way we should be able to deal with this situation and an explicit unset
> may not be needed. What do you think ?

I don't want (non-immutable) properties that magically change values.
That way lies madness.

> 
> >>
> >> Note: I have currently used 16bit values only to get the feedback on the approach.
> >>
> >> Regards,
> >> Uma Shankar
> >>
> >> >>
> >> >> >>
> >> >> >> v2: Used Ville's design and approach to define the interfaces.
> >> >> >> Addressed Matt Roper's review feedback and re-ordered the patches.
> >> >> >>
> >> >> >> Uma Shankar (5):
> >> >> >>   drm: Add gamma mode property
> >> >> >>   drm/i915/icl: Add register definitions for Multi Segmented gamma
> >> >> >>   drm/i915/icl: Add support for multi segmented gamma mode
> >> >> >>   drm/i915: Add gamma mode caps property
> >> >> >>   drm/i915: Attach gamma mode property
> >> >> >>
> >> >> >> Ville Syrjälä (2):
> >> >> >>   drm: Add gamma mode caps property
> >> >> >>   drm/i915: Define color lut range structure
> >> >> >>
> >> >> >>  drivers/gpu/drm/drm_atomic_uapi.c    |  13 +
> >> >> >>  drivers/gpu/drm/drm_color_mgmt.c     | 110 +++++++++
> >> >> >>  drivers/gpu/drm/i915/i915_reg.h      |  17 ++
> >> >> >>  drivers/gpu/drm/i915/intel_color.c   | 465
> >> >> >++++++++++++++++++++++++++++++++++-
> >> >> >>  drivers/gpu/drm/i915/intel_display.c |   5 +
> >> >> >>  include/drm/drm_color_mgmt.h         |  11 +
> >> >> >>  include/drm/drm_crtc.h               |  17 ++
> >> >> >>  include/drm/drm_mode_config.h        |  10 +
> >> >> >>  include/uapi/drm/drm_mode.h          |  49 ++++
> >> >> >>  9 files changed, 690 insertions(+), 7 deletions(-)
> >> >> >>
> >> >> >> --
> >> >> >> 1.9.1
> >> >> >>
> >> >> >> _______________________________________________
> >> >> >> Intel-gfx mailing list
> >> >> >> Intel-gfx@lists.freedesktop.org
> >> >> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >> >> >
> >> >> >--
> >> >> >Ville Syrjälä
> >> >> >Intel
> >> >> >_______________________________________________
> >> >> >dri-devel mailing list
> >> >> >dri-devel@lists.freedesktop.org
> >> >> >https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >> >
> >> >--
> >> >Ville Syrjälä
> >> >Intel
> >
> >--
> >Ville Syrjälä
> >Intel

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

Re: [v2 0/7] Add Multi Segment Gamma Support

[Shankar, Uma]

>-----Original Message-----
>From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of Ville
>Syrjälä
>Sent: Monday, April 8, 2019 9:15 PM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri-
>devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville
><ville.syrjala@intel.com>; Lankhorst, Maarten <maarten.lankhorst@intel.com>
>Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support
>
>On Mon, Apr 08, 2019 at 03:40:39PM +0000, Shankar, Uma wrote:
>>
>>
>> >-----Original Message-----
>> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>> >Sent: Monday, April 8, 2019 8:27 PM
>> >To: Shankar, Uma <uma.shankar@intel.com>
>> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri-
>> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville
>> ><ville.syrjala@intel.com>; Lankhorst, Maarten
>> ><maarten.lankhorst@intel.com>
>> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support
>> >
>> >On Mon, Apr 08, 2019 at 02:40:51PM +0000, Shankar, Uma wrote:
>> >>
>> >>
>> >> >-----Original Message-----
>> >> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>> >> >Sent: Monday, April 8, 2019 6:01 PM
>> >> >To: Shankar, Uma <uma.shankar@intel.com>
>> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri-
>> >> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville
>> >> ><ville.syrjala@intel.com>; Lankhorst, Maarten
>> >> ><maarten.lankhorst@intel.com>
>> >> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support
>> >> >
>> >> >On Mon, Apr 08, 2019 at 12:26:23PM +0000, Shankar, Uma wrote:
>> >> >>
>> >> >>
>> >> >> >-----Original Message-----
>> >> >> >From: dri-devel
>> >> >> >[mailto:dri-devel-bounces@lists.freedesktop.org]
>> >> >> >On Behalf Of Ville Syrjälä
>> >> >> >Sent: Friday, April 5, 2019 9:42 PM
>> >> >> >To: Shankar, Uma <uma.shankar@intel.com>
>> >> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org;
>> >> >> >dri- devel@lists.freedesktop.org; seanpaul@chromium.org;
>> >> >> >Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
>> >> >> ><maarten.lankhorst@intel.com>
>> >> >> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma
>> >> >> >Support
>> >> >> >
>> >> >> >On Mon, Apr 01, 2019 at 11:00:04PM +0530, Uma Shankar wrote:
>> >> >> >> This series adds support for programmable gamma modes and
>> >> >> >> exposes a property interface for the same. Also added,
>> >> >> >> support for multi segment gamma mode introduced in ICL+
>> >> >> >>
>> >> >> >> It creates 2 property interfaces :
>> >> >> >> 1. GAMMA_MODE_CAPS: This is immutable property and exposes
>> >> >> >> the various gamma modes supported and the lut ranges. This is
>> >> >> >> an enum property with element as blob id. Getting the blob id
>> >> >> >> in userspace, user can get the mode supported and also the
>> >> >> >> range of gamma mode supported with number of lut coefficients.
>> >> >> >>
>> >> >> >> 2. GAMMA_MODE: This is for user to set the gamma mode and
>> >> >> >> send the lut values for that particular mode.
>> >> >> >
>> >> >> >I think we should just go for the BLOB_ENUM prop type instead.
>> >> >> >Then the possible values and the current value are all part of the same prop.
>> >> >>
>> >> >> Hi Ville,
>> >> >> With the current approach, we have enum property with values as
>> >> >> blob_ids (representing platform capabilities). This should not
>> >> >> get modified and needs to be immutable.
>> >> >
>> >> >That's not quite what we want. We want to let the user modify the
>> >> >current value so that they can actually select the gamma mode.
>> >> >Otherwise we need yet another prop for it, or we have to deduce it
>> >> >from the LUT size (that apporach would actually work for i915 but
>> >> >may not work for other drivers/hardware).
>> >> >
>> >> >>
>> >> >> Userspace can query the property and get the blob using the blob_ids.
>> >> >> Thereby getting all the platform capabilities.
>> >> >>
>> >> >> Now to set the LUT values, he can use another blob property and
>> >> >> pass the luts.  This is inline to how gamma/degamma is
>> >> >> implemented where we have one immutable LUT_SIZE property
>> >> >> (indicating number of
>> >> >> luts) and another blob property for actual lut values..
>> >>
>> >> Hi Ville,
>> >> Just to clarify and clear some doubts :)
>> >>
>> >> We should be able to set the gamma mode using the blob enum value.
>> >> Userspace will get the list enum vals (blob ids with mode name
>> >> embedded) and
>> >select one and do a setprop to set a mode.
>> >> Driver will get the blob_id and will be able to get the mode to be
>> >> set.  So exposing capabilities and setting the mode should be
>> >> possible with this one
>> >property. I hope my understanding is correct.
>> >>
>> >> Now to send the actual blob values to be set, we need to use some
>> >> other property interface. Should we use the currently available
>> >> "gamma blob
>> >(gamma_lut_property)" property to send the lut values.
>> >> The challenge there is that it currently uses 16 bit lut values
>> >> struct drm_color_lut {
>> >>         __u16 red;
>> >>         __u16 green;
>> >>         __u16 blue;
>> >>         __u16 reserved;
>> >> };
>> >> which is not sufficient for HDR usecases. Or should we need a new
>> >> property for
>> >advance lut/extended lut like below:
>> >> https://patchwork.freedesktop.org/patch/294732/?series=30875&rev=7
>> >>
>> >> What do you suggest ?
>> >
>> >I was thinking that we might get away with reusing the current props
>> >and just change the interpretation of the data when gamma_mode is
>> >set. But I'm not sure that's going to work out so well if one client
>> >sets this up and then another client comes along that doesn't
>> >understand the new props at all. But even with separate props I think
>> >we might still end up in a mess because the new client wouldn't know
>> >how to unset the higher precision LUT before setting up the old style prop and the
>kernel would then refuse the operation with with both props being set.
>> >
>> >So I think we might need a client cap for this which simply changes
>> >how the data in the existing props is represented. So internally we
>> >could always store things in the new high precision format, but we'd
>> >convert to/from the old format when dealing with an older client.
>>
>> We could also say that if a legacy gamma_mode_property is set (which
>> will be used by legacy apps or apps not aware of new interface), in
>> driver we will simply unset the earlier gamma_mode and fallback to
>> legacy mode (whatever it was for a particular platform). This way we
>> should be able to deal with this situation and an explicit unset may not be needed.
>What do you think ?
>
>I don't want (non-immutable) properties that magically change values.
>That way lies madness.

Oh ok. So do you suggest that we add some kind of  flag to be set by user, based on
which we take either legacy or advance_gamma path. Is my understanding correct ?

>>
>> >>
>> >> Note: I have currently used 16bit values only to get the feedback on the
>approach.
>> >>
>> >> Regards,
>> >> Uma Shankar
>> >>
>> >> >>
>> >> >> >>
>> >> >> >> v2: Used Ville's design and approach to define the interfaces.
>> >> >> >> Addressed Matt Roper's review feedback and re-ordered the patches.
>> >> >> >>
>> >> >> >> Uma Shankar (5):
>> >> >> >>   drm: Add gamma mode property
>> >> >> >>   drm/i915/icl: Add register definitions for Multi Segmented gamma
>> >> >> >>   drm/i915/icl: Add support for multi segmented gamma mode
>> >> >> >>   drm/i915: Add gamma mode caps property
>> >> >> >>   drm/i915: Attach gamma mode property
>> >> >> >>
>> >> >> >> Ville Syrjälä (2):
>> >> >> >>   drm: Add gamma mode caps property
>> >> >> >>   drm/i915: Define color lut range structure
>> >> >> >>
>> >> >> >>  drivers/gpu/drm/drm_atomic_uapi.c    |  13 +
>> >> >> >>  drivers/gpu/drm/drm_color_mgmt.c     | 110 +++++++++
>> >> >> >>  drivers/gpu/drm/i915/i915_reg.h      |  17 ++
>> >> >> >>  drivers/gpu/drm/i915/intel_color.c   | 465
>> >> >> >++++++++++++++++++++++++++++++++++-
>> >> >> >>  drivers/gpu/drm/i915/intel_display.c |   5 +
>> >> >> >>  include/drm/drm_color_mgmt.h         |  11 +
>> >> >> >>  include/drm/drm_crtc.h               |  17 ++
>> >> >> >>  include/drm/drm_mode_config.h        |  10 +
>> >> >> >>  include/uapi/drm/drm_mode.h          |  49 ++++
>> >> >> >>  9 files changed, 690 insertions(+), 7 deletions(-)
>> >> >> >>
>> >> >> >> --
>> >> >> >> 1.9.1
>> >> >> >>
>> >> >> >> _______________________________________________
>> >> >> >> Intel-gfx mailing list
>> >> >> >> Intel-gfx@lists.freedesktop.org
>> >> >> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >> >> >
>> >> >> >--
>> >> >> >Ville Syrjälä
>> >> >> >Intel
>> >> >> >_______________________________________________
>> >> >> >dri-devel mailing list
>> >> >> >dri-devel@lists.freedesktop.org
>> >> >> >https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> >> >
>> >> >--
>> >> >Ville Syrjälä
>> >> >Intel
>> >
>> >--
>> >Ville Syrjälä
>> >Intel
>
>--
>Ville Syrjälä
>Intel
>_______________________________________________
>dri-devel mailing list
>dri-devel@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support

[Ville Syrjälä]

On Mon, Apr 08, 2019 at 03:59:51PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of Ville
> >Syrjälä
> >Sent: Monday, April 8, 2019 9:15 PM
> >To: Shankar, Uma <uma.shankar@intel.com>
> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri-
> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville
> ><ville.syrjala@intel.com>; Lankhorst, Maarten <maarten.lankhorst@intel.com>
> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support
> >
> >On Mon, Apr 08, 2019 at 03:40:39PM +0000, Shankar, Uma wrote:
> >>
> >>
> >> >-----Original Message-----
> >> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >> >Sent: Monday, April 8, 2019 8:27 PM
> >> >To: Shankar, Uma <uma.shankar@intel.com>
> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri-
> >> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville
> >> ><ville.syrjala@intel.com>; Lankhorst, Maarten
> >> ><maarten.lankhorst@intel.com>
> >> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support
> >> >
> >> >On Mon, Apr 08, 2019 at 02:40:51PM +0000, Shankar, Uma wrote:
> >> >>
> >> >>
> >> >> >-----Original Message-----
> >> >> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >> >> >Sent: Monday, April 8, 2019 6:01 PM
> >> >> >To: Shankar, Uma <uma.shankar@intel.com>
> >> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri-
> >> >> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville
> >> >> ><ville.syrjala@intel.com>; Lankhorst, Maarten
> >> >> ><maarten.lankhorst@intel.com>
> >> >> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support
> >> >> >
> >> >> >On Mon, Apr 08, 2019 at 12:26:23PM +0000, Shankar, Uma wrote:
> >> >> >>
> >> >> >>
> >> >> >> >-----Original Message-----
> >> >> >> >From: dri-devel
> >> >> >> >[mailto:dri-devel-bounces@lists.freedesktop.org]
> >> >> >> >On Behalf Of Ville Syrjälä
> >> >> >> >Sent: Friday, April 5, 2019 9:42 PM
> >> >> >> >To: Shankar, Uma <uma.shankar@intel.com>
> >> >> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org;
> >> >> >> >dri- devel@lists.freedesktop.org; seanpaul@chromium.org;
> >> >> >> >Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
> >> >> >> ><maarten.lankhorst@intel.com>
> >> >> >> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma
> >> >> >> >Support
> >> >> >> >
> >> >> >> >On Mon, Apr 01, 2019 at 11:00:04PM +0530, Uma Shankar wrote:
> >> >> >> >> This series adds support for programmable gamma modes and
> >> >> >> >> exposes a property interface for the same. Also added,
> >> >> >> >> support for multi segment gamma mode introduced in ICL+
> >> >> >> >>
> >> >> >> >> It creates 2 property interfaces :
> >> >> >> >> 1. GAMMA_MODE_CAPS: This is immutable property and exposes
> >> >> >> >> the various gamma modes supported and the lut ranges. This is
> >> >> >> >> an enum property with element as blob id. Getting the blob id
> >> >> >> >> in userspace, user can get the mode supported and also the
> >> >> >> >> range of gamma mode supported with number of lut coefficients.
> >> >> >> >>
> >> >> >> >> 2. GAMMA_MODE: This is for user to set the gamma mode and
> >> >> >> >> send the lut values for that particular mode.
> >> >> >> >
> >> >> >> >I think we should just go for the BLOB_ENUM prop type instead.
> >> >> >> >Then the possible values and the current value are all part of the same prop.
> >> >> >>
> >> >> >> Hi Ville,
> >> >> >> With the current approach, we have enum property with values as
> >> >> >> blob_ids (representing platform capabilities). This should not
> >> >> >> get modified and needs to be immutable.
> >> >> >
> >> >> >That's not quite what we want. We want to let the user modify the
> >> >> >current value so that they can actually select the gamma mode.
> >> >> >Otherwise we need yet another prop for it, or we have to deduce it
> >> >> >from the LUT size (that apporach would actually work for i915 but
> >> >> >may not work for other drivers/hardware).
> >> >> >
> >> >> >>
> >> >> >> Userspace can query the property and get the blob using the blob_ids.
> >> >> >> Thereby getting all the platform capabilities.
> >> >> >>
> >> >> >> Now to set the LUT values, he can use another blob property and
> >> >> >> pass the luts.  This is inline to how gamma/degamma is
> >> >> >> implemented where we have one immutable LUT_SIZE property
> >> >> >> (indicating number of
> >> >> >> luts) and another blob property for actual lut values..
> >> >>
> >> >> Hi Ville,
> >> >> Just to clarify and clear some doubts :)
> >> >>
> >> >> We should be able to set the gamma mode using the blob enum value.
> >> >> Userspace will get the list enum vals (blob ids with mode name
> >> >> embedded) and
> >> >select one and do a setprop to set a mode.
> >> >> Driver will get the blob_id and will be able to get the mode to be
> >> >> set.  So exposing capabilities and setting the mode should be
> >> >> possible with this one
> >> >property. I hope my understanding is correct.
> >> >>
> >> >> Now to send the actual blob values to be set, we need to use some
> >> >> other property interface. Should we use the currently available
> >> >> "gamma blob
> >> >(gamma_lut_property)" property to send the lut values.
> >> >> The challenge there is that it currently uses 16 bit lut values
> >> >> struct drm_color_lut {
> >> >>         __u16 red;
> >> >>         __u16 green;
> >> >>         __u16 blue;
> >> >>         __u16 reserved;
> >> >> };
> >> >> which is not sufficient for HDR usecases. Or should we need a new
> >> >> property for
> >> >advance lut/extended lut like below:
> >> >> https://patchwork.freedesktop.org/patch/294732/?series=30875&rev=7
> >> >>
> >> >> What do you suggest ?
> >> >
> >> >I was thinking that we might get away with reusing the current props
> >> >and just change the interpretation of the data when gamma_mode is
> >> >set. But I'm not sure that's going to work out so well if one client
> >> >sets this up and then another client comes along that doesn't
> >> >understand the new props at all. But even with separate props I think
> >> >we might still end up in a mess because the new client wouldn't know
> >> >how to unset the higher precision LUT before setting up the old style prop and the
> >kernel would then refuse the operation with with both props being set.
> >> >
> >> >So I think we might need a client cap for this which simply changes
> >> >how the data in the existing props is represented. So internally we
> >> >could always store things in the new high precision format, but we'd
> >> >convert to/from the old format when dealing with an older client.
> >>
> >> We could also say that if a legacy gamma_mode_property is set (which
> >> will be used by legacy apps or apps not aware of new interface), in
> >> driver we will simply unset the earlier gamma_mode and fallback to
> >> legacy mode (whatever it was for a particular platform). This way we
> >> should be able to deal with this situation and an explicit unset may not be needed.
> >What do you think ?
> >
> >I don't want (non-immutable) properties that magically change values.
> >That way lies madness.
> 
> Oh ok. So do you suggest that we add some kind of  flag to be set by user, based on
> which we take either legacy or advance_gamma path. Is my understanding correct ?

Yeah, just another client cap. I can't immediately think of a nicer way
to extend the precision.


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

Re: [v2 0/7] Add Multi Segment Gamma Support

[Shankar, Uma]

>-----Original Message-----
>From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of Ville
>Syrjälä
>Sent: Monday, April 8, 2019 9:38 PM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri-
>devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville
><ville.syrjala@intel.com>; Lankhorst, Maarten <maarten.lankhorst@intel.com>
>Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support
>
>On Mon, Apr 08, 2019 at 03:59:51PM +0000, Shankar, Uma wrote:
>>
>>
>> >-----Original Message-----
>> >From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On
>> >Behalf Of Ville Syrjälä
>> >Sent: Monday, April 8, 2019 9:15 PM
>> >To: Shankar, Uma <uma.shankar@intel.com>
>> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri-
>> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville
>> ><ville.syrjala@intel.com>; Lankhorst, Maarten
>> ><maarten.lankhorst@intel.com>
>> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support
>> >
>> >On Mon, Apr 08, 2019 at 03:40:39PM +0000, Shankar, Uma wrote:
>> >>
>> >>
>> >> >-----Original Message-----
>> >> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>> >> >Sent: Monday, April 8, 2019 8:27 PM
>> >> >To: Shankar, Uma <uma.shankar@intel.com>
>> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri-
>> >> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville
>> >> ><ville.syrjala@intel.com>; Lankhorst, Maarten
>> >> ><maarten.lankhorst@intel.com>
>> >> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support
>> >> >
>> >> >On Mon, Apr 08, 2019 at 02:40:51PM +0000, Shankar, Uma wrote:
>> >> >>
>> >> >>
>> >> >> >-----Original Message-----
>> >> >> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>> >> >> >Sent: Monday, April 8, 2019 6:01 PM
>> >> >> >To: Shankar, Uma <uma.shankar@intel.com>
>> >> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org;
>> >> >> >dri- devel@lists.freedesktop.org; seanpaul@chromium.org;
>> >> >> >Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
>> >> >> ><maarten.lankhorst@intel.com>
>> >> >> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma
>> >> >> >Support
>> >> >> >
>> >> >> >On Mon, Apr 08, 2019 at 12:26:23PM +0000, Shankar, Uma wrote:
>> >> >> >>
>> >> >> >>
>> >> >> >> >-----Original Message-----
>> >> >> >> >From: dri-devel
>> >> >> >> >[mailto:dri-devel-bounces@lists.freedesktop.org]
>> >> >> >> >On Behalf Of Ville Syrjälä
>> >> >> >> >Sent: Friday, April 5, 2019 9:42 PM
>> >> >> >> >To: Shankar, Uma <uma.shankar@intel.com>
>> >> >> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org;
>> >> >> >> >dri- devel@lists.freedesktop.org; seanpaul@chromium.org;
>> >> >> >> >Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
>> >> >> >> ><maarten.lankhorst@intel.com>
>> >> >> >> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma
>> >> >> >> >Support
>> >> >> >> >
>> >> >> >> >On Mon, Apr 01, 2019 at 11:00:04PM +0530, Uma Shankar wrote:
>> >> >> >> >> This series adds support for programmable gamma modes and
>> >> >> >> >> exposes a property interface for the same. Also added,
>> >> >> >> >> support for multi segment gamma mode introduced in ICL+
>> >> >> >> >>
>> >> >> >> >> It creates 2 property interfaces :
>> >> >> >> >> 1. GAMMA_MODE_CAPS: This is immutable property and exposes
>> >> >> >> >> the various gamma modes supported and the lut ranges. This
>> >> >> >> >> is an enum property with element as blob id. Getting the
>> >> >> >> >> blob id in userspace, user can get the mode supported and
>> >> >> >> >> also the range of gamma mode supported with number of lut
>coefficients.
>> >> >> >> >>
>> >> >> >> >> 2. GAMMA_MODE: This is for user to set the gamma mode and
>> >> >> >> >> send the lut values for that particular mode.
>> >> >> >> >
>> >> >> >> >I think we should just go for the BLOB_ENUM prop type instead.
>> >> >> >> >Then the possible values and the current value are all part of the same
>prop.
>> >> >> >>
>> >> >> >> Hi Ville,
>> >> >> >> With the current approach, we have enum property with values
>> >> >> >> as blob_ids (representing platform capabilities). This should
>> >> >> >> not get modified and needs to be immutable.
>> >> >> >
>> >> >> >That's not quite what we want. We want to let the user modify
>> >> >> >the current value so that they can actually select the gamma mode.
>> >> >> >Otherwise we need yet another prop for it, or we have to deduce
>> >> >> >it from the LUT size (that apporach would actually work for
>> >> >> >i915 but may not work for other drivers/hardware).
>> >> >> >
>> >> >> >>
>> >> >> >> Userspace can query the property and get the blob using the blob_ids.
>> >> >> >> Thereby getting all the platform capabilities.
>> >> >> >>
>> >> >> >> Now to set the LUT values, he can use another blob property
>> >> >> >> and pass the luts.  This is inline to how gamma/degamma is
>> >> >> >> implemented where we have one immutable LUT_SIZE property
>> >> >> >> (indicating number of
>> >> >> >> luts) and another blob property for actual lut values..
>> >> >>
>> >> >> Hi Ville,
>> >> >> Just to clarify and clear some doubts :)
>> >> >>
>> >> >> We should be able to set the gamma mode using the blob enum value.
>> >> >> Userspace will get the list enum vals (blob ids with mode name
>> >> >> embedded) and
>> >> >select one and do a setprop to set a mode.
>> >> >> Driver will get the blob_id and will be able to get the mode to
>> >> >> be set.  So exposing capabilities and setting the mode should be
>> >> >> possible with this one
>> >> >property. I hope my understanding is correct.
>> >> >>
>> >> >> Now to send the actual blob values to be set, we need to use
>> >> >> some other property interface. Should we use the currently
>> >> >> available "gamma blob
>> >> >(gamma_lut_property)" property to send the lut values.
>> >> >> The challenge there is that it currently uses 16 bit lut values
>> >> >> struct drm_color_lut {
>> >> >>         __u16 red;
>> >> >>         __u16 green;
>> >> >>         __u16 blue;
>> >> >>         __u16 reserved;
>> >> >> };
>> >> >> which is not sufficient for HDR usecases. Or should we need a
>> >> >> new property for
>> >> >advance lut/extended lut like below:
>> >> >> https://patchwork.freedesktop.org/patch/294732/?series=30875&rev
>> >> >> =7
>> >> >>
>> >> >> What do you suggest ?
>> >> >
>> >> >I was thinking that we might get away with reusing the current
>> >> >props and just change the interpretation of the data when
>> >> >gamma_mode is set. But I'm not sure that's going to work out so
>> >> >well if one client sets this up and then another client comes
>> >> >along that doesn't understand the new props at all. But even with
>> >> >separate props I think we might still end up in a mess because the
>> >> >new client wouldn't know how to unset the higher precision LUT
>> >> >before setting up the old style prop and the
>> >kernel would then refuse the operation with with both props being set.
>> >> >
>> >> >So I think we might need a client cap for this which simply
>> >> >changes how the data in the existing props is represented. So
>> >> >internally we could always store things in the new high precision
>> >> >format, but we'd convert to/from the old format when dealing with an older
>client.
>> >>
>> >> We could also say that if a legacy gamma_mode_property is set
>> >> (which will be used by legacy apps or apps not aware of new
>> >> interface), in driver we will simply unset the earlier gamma_mode
>> >> and fallback to legacy mode (whatever it was for a particular
>> >> platform). This way we should be able to deal with this situation and an explicit
>unset may not be needed.
>> >What do you think ?
>> >
>> >I don't want (non-immutable) properties that magically change values.
>> >That way lies madness.
>>
>> Oh ok. So do you suggest that we add some kind of  flag to be set by
>> user, based on which we take either legacy or advance_gamma path. Is my
>understanding correct ?
>
>Yeah, just another client cap. I can't immediately think of a nicer way to extend the
>precision.

Sure Ville, I am implementing based on this suggestion.  Basically will expose a new 
advance_gamma_mode flag as a client cap. Something like:

#define DRM_CLIENT_CAP_ADVANCE_GAMMA_MODES     6

The new users will set this and we will assume that advance gamma mode paths is active.
If the flag is not set, driver will take the legacy path and enable default gamma_mode for
that particular platform.

In both cases, we will use the existing GAMMA_LUT blob property to send the lut values
from userspace. 

Will send out the next version soon. Thanks for your valuable feedback and suggestions.

Regards,
Uma Shankar

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

Re: [v2 0/7] Add Multi Segment Gamma Support

[Ville Syrjälä]

On Wed, Apr 10, 2019 at 01:20:44PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of Ville
> >Syrjälä
> >Sent: Monday, April 8, 2019 9:38 PM
> >To: Shankar, Uma <uma.shankar@intel.com>
> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri-
> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville
> ><ville.syrjala@intel.com>; Lankhorst, Maarten <maarten.lankhorst@intel.com>
> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support
> >
> >On Mon, Apr 08, 2019 at 03:59:51PM +0000, Shankar, Uma wrote:
> >>
> >>
> >> >-----Original Message-----
> >> >From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On
> >> >Behalf Of Ville Syrjälä
> >> >Sent: Monday, April 8, 2019 9:15 PM
> >> >To: Shankar, Uma <uma.shankar@intel.com>
> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri-
> >> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville
> >> ><ville.syrjala@intel.com>; Lankhorst, Maarten
> >> ><maarten.lankhorst@intel.com>
> >> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support
> >> >
> >> >On Mon, Apr 08, 2019 at 03:40:39PM +0000, Shankar, Uma wrote:
> >> >>
> >> >>
> >> >> >-----Original Message-----
> >> >> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >> >> >Sent: Monday, April 8, 2019 8:27 PM
> >> >> >To: Shankar, Uma <uma.shankar@intel.com>
> >> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri-
> >> >> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville
> >> >> ><ville.syrjala@intel.com>; Lankhorst, Maarten
> >> >> ><maarten.lankhorst@intel.com>
> >> >> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support
> >> >> >
> >> >> >On Mon, Apr 08, 2019 at 02:40:51PM +0000, Shankar, Uma wrote:
> >> >> >>
> >> >> >>
> >> >> >> >-----Original Message-----
> >> >> >> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >> >> >> >Sent: Monday, April 8, 2019 6:01 PM
> >> >> >> >To: Shankar, Uma <uma.shankar@intel.com>
> >> >> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org;
> >> >> >> >dri- devel@lists.freedesktop.org; seanpaul@chromium.org;
> >> >> >> >Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
> >> >> >> ><maarten.lankhorst@intel.com>
> >> >> >> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma
> >> >> >> >Support
> >> >> >> >
> >> >> >> >On Mon, Apr 08, 2019 at 12:26:23PM +0000, Shankar, Uma wrote:
> >> >> >> >>
> >> >> >> >>
> >> >> >> >> >-----Original Message-----
> >> >> >> >> >From: dri-devel
> >> >> >> >> >[mailto:dri-devel-bounces@lists.freedesktop.org]
> >> >> >> >> >On Behalf Of Ville Syrjälä
> >> >> >> >> >Sent: Friday, April 5, 2019 9:42 PM
> >> >> >> >> >To: Shankar, Uma <uma.shankar@intel.com>
> >> >> >> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org;
> >> >> >> >> >dri- devel@lists.freedesktop.org; seanpaul@chromium.org;
> >> >> >> >> >Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
> >> >> >> >> ><maarten.lankhorst@intel.com>
> >> >> >> >> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma
> >> >> >> >> >Support
> >> >> >> >> >
> >> >> >> >> >On Mon, Apr 01, 2019 at 11:00:04PM +0530, Uma Shankar wrote:
> >> >> >> >> >> This series adds support for programmable gamma modes and
> >> >> >> >> >> exposes a property interface for the same. Also added,
> >> >> >> >> >> support for multi segment gamma mode introduced in ICL+
> >> >> >> >> >>
> >> >> >> >> >> It creates 2 property interfaces :
> >> >> >> >> >> 1. GAMMA_MODE_CAPS: This is immutable property and exposes
> >> >> >> >> >> the various gamma modes supported and the lut ranges. This
> >> >> >> >> >> is an enum property with element as blob id. Getting the
> >> >> >> >> >> blob id in userspace, user can get the mode supported and
> >> >> >> >> >> also the range of gamma mode supported with number of lut
> >coefficients.
> >> >> >> >> >>
> >> >> >> >> >> 2. GAMMA_MODE: This is for user to set the gamma mode and
> >> >> >> >> >> send the lut values for that particular mode.
> >> >> >> >> >
> >> >> >> >> >I think we should just go for the BLOB_ENUM prop type instead.
> >> >> >> >> >Then the possible values and the current value are all part of the same
> >prop.
> >> >> >> >>
> >> >> >> >> Hi Ville,
> >> >> >> >> With the current approach, we have enum property with values
> >> >> >> >> as blob_ids (representing platform capabilities). This should
> >> >> >> >> not get modified and needs to be immutable.
> >> >> >> >
> >> >> >> >That's not quite what we want. We want to let the user modify
> >> >> >> >the current value so that they can actually select the gamma mode.
> >> >> >> >Otherwise we need yet another prop for it, or we have to deduce
> >> >> >> >it from the LUT size (that apporach would actually work for
> >> >> >> >i915 but may not work for other drivers/hardware).
> >> >> >> >
> >> >> >> >>
> >> >> >> >> Userspace can query the property and get the blob using the blob_ids.
> >> >> >> >> Thereby getting all the platform capabilities.
> >> >> >> >>
> >> >> >> >> Now to set the LUT values, he can use another blob property
> >> >> >> >> and pass the luts.  This is inline to how gamma/degamma is
> >> >> >> >> implemented where we have one immutable LUT_SIZE property
> >> >> >> >> (indicating number of
> >> >> >> >> luts) and another blob property for actual lut values..
> >> >> >>
> >> >> >> Hi Ville,
> >> >> >> Just to clarify and clear some doubts :)
> >> >> >>
> >> >> >> We should be able to set the gamma mode using the blob enum value.
> >> >> >> Userspace will get the list enum vals (blob ids with mode name
> >> >> >> embedded) and
> >> >> >select one and do a setprop to set a mode.
> >> >> >> Driver will get the blob_id and will be able to get the mode to
> >> >> >> be set.  So exposing capabilities and setting the mode should be
> >> >> >> possible with this one
> >> >> >property. I hope my understanding is correct.
> >> >> >>
> >> >> >> Now to send the actual blob values to be set, we need to use
> >> >> >> some other property interface. Should we use the currently
> >> >> >> available "gamma blob
> >> >> >(gamma_lut_property)" property to send the lut values.
> >> >> >> The challenge there is that it currently uses 16 bit lut values
> >> >> >> struct drm_color_lut {
> >> >> >>         __u16 red;
> >> >> >>         __u16 green;
> >> >> >>         __u16 blue;
> >> >> >>         __u16 reserved;
> >> >> >> };
> >> >> >> which is not sufficient for HDR usecases. Or should we need a
> >> >> >> new property for
> >> >> >advance lut/extended lut like below:
> >> >> >> https://patchwork.freedesktop.org/patch/294732/?series=30875&rev
> >> >> >> =7
> >> >> >>
> >> >> >> What do you suggest ?
> >> >> >
> >> >> >I was thinking that we might get away with reusing the current
> >> >> >props and just change the interpretation of the data when
> >> >> >gamma_mode is set. But I'm not sure that's going to work out so
> >> >> >well if one client sets this up and then another client comes
> >> >> >along that doesn't understand the new props at all. But even with
> >> >> >separate props I think we might still end up in a mess because the
> >> >> >new client wouldn't know how to unset the higher precision LUT
> >> >> >before setting up the old style prop and the
> >> >kernel would then refuse the operation with with both props being set.
> >> >> >
> >> >> >So I think we might need a client cap for this which simply
> >> >> >changes how the data in the existing props is represented. So
> >> >> >internally we could always store things in the new high precision
> >> >> >format, but we'd convert to/from the old format when dealing with an older
> >client.
> >> >>
> >> >> We could also say that if a legacy gamma_mode_property is set
> >> >> (which will be used by legacy apps or apps not aware of new
> >> >> interface), in driver we will simply unset the earlier gamma_mode
> >> >> and fallback to legacy mode (whatever it was for a particular
> >> >> platform). This way we should be able to deal with this situation and an explicit
> >unset may not be needed.
> >> >What do you think ?
> >> >
> >> >I don't want (non-immutable) properties that magically change values.
> >> >That way lies madness.
> >>
> >> Oh ok. So do you suggest that we add some kind of  flag to be set by
> >> user, based on which we take either legacy or advance_gamma path. Is my
> >understanding correct ?
> >
> >Yeah, just another client cap. I can't immediately think of a nicer way to extend the
> >precision.
> 
> Sure Ville, I am implementing based on this suggestion.  Basically will expose a new 
> advance_gamma_mode flag as a client cap. Something like:
> 
> #define DRM_CLIENT_CAP_ADVANCE_GAMMA_MODES     6
> 
> The new users will set this and we will assume that advance gamma mode paths is active.
> If the flag is not set, driver will take the legacy path and enable default gamma_mode for
> that particular platform.

It's maybe a still a bit nasty because we basically have to ignore the
gamma mode prop entirely in that case. Not quite sure what is the best
way to shoehorn this in. I guess the main problem is what we would
report to userspace via the (DE)GAMMA_MODE props if the previous client
left gamma_mode set in a way that conflicts with what
(DE)GAMMA_LUT_SIZE suggests it should be.

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

RE: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support

[Shankar, Uma]

>>
>> >-----Original Message-----
>> >From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On
>> >Behalf Of Ville Syrjälä
>> >Sent: Monday, April 8, 2019 9:38 PM
>> >To: Shankar, Uma <uma.shankar@intel.com>
>> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri-
>> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville
>> ><ville.syrjala@intel.com>; Lankhorst, Maarten
>> ><maarten.lankhorst@intel.com>
>> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support
>> >
>> >On Mon, Apr 08, 2019 at 03:59:51PM +0000, Shankar, Uma wrote:
>> >>
>> >>
>> >> >-----Original Message-----
>> >> >From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org]
>> >> >On Behalf Of Ville Syrjälä
>> >> >Sent: Monday, April 8, 2019 9:15 PM
>> >> >To: Shankar, Uma <uma.shankar@intel.com>
>> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri-
>> >> >devel@lists.freedesktop.org; seanpaul@chromium.org; Syrjala, Ville
>> >> ><ville.syrjala@intel.com>; Lankhorst, Maarten
>> >> ><maarten.lankhorst@intel.com>
>> >> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma Support
>> >> >
>> >> >On Mon, Apr 08, 2019 at 03:40:39PM +0000, Shankar, Uma wrote:
>> >> >>
>> >> >>
>> >> >> >-----Original Message-----
>> >> >> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>> >> >> >Sent: Monday, April 8, 2019 8:27 PM
>> >> >> >To: Shankar, Uma <uma.shankar@intel.com>
>> >> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org;
>> >> >> >dri- devel@lists.freedesktop.org; seanpaul@chromium.org;
>> >> >> >Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
>> >> >> ><maarten.lankhorst@intel.com>
>> >> >> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma
>> >> >> >Support
>> >> >> >
>> >> >> >On Mon, Apr 08, 2019 at 02:40:51PM +0000, Shankar, Uma wrote:
>> >> >> >>
>> >> >> >>
>> >> >> >> >-----Original Message-----
>> >> >> >> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>> >> >> >> >Sent: Monday, April 8, 2019 6:01 PM
>> >> >> >> >To: Shankar, Uma <uma.shankar@intel.com>
>> >> >> >> >Cc: dcastagna@chromium.org; intel-gfx@lists.freedesktop.org;
>> >> >> >> >dri- devel@lists.freedesktop.org; seanpaul@chromium.org;
>> >> >> >> >Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
>> >> >> >> ><maarten.lankhorst@intel.com>
>> >> >> >> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma
>> >> >> >> >Support
>> >> >> >> >
>> >> >> >> >On Mon, Apr 08, 2019 at 12:26:23PM +0000, Shankar, Uma wrote:
>> >> >> >> >>
>> >> >> >> >>
>> >> >> >> >> >-----Original Message-----
>> >> >> >> >> >From: dri-devel
>> >> >> >> >> >[mailto:dri-devel-bounces@lists.freedesktop.org]
>> >> >> >> >> >On Behalf Of Ville Syrjälä
>> >> >> >> >> >Sent: Friday, April 5, 2019 9:42 PM
>> >> >> >> >> >To: Shankar, Uma <uma.shankar@intel.com>
>> >> >> >> >> >Cc: dcastagna@chromium.org;
>> >> >> >> >> >intel-gfx@lists.freedesktop.org;
>> >> >> >> >> >dri- devel@lists.freedesktop.org; seanpaul@chromium.org;
>> >> >> >> >> >Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst,
>> >> >> >> >> >Maarten <maarten.lankhorst@intel.com>
>> >> >> >> >> >Subject: Re: [Intel-gfx] [v2 0/7] Add Multi Segment Gamma
>> >> >> >> >> >Support
>> >> >> >> >> >
>> >> >> >> >> >On Mon, Apr 01, 2019 at 11:00:04PM +0530, Uma Shankar wrote:
>> >> >> >> >> >> This series adds support for programmable gamma modes
>> >> >> >> >> >> and exposes a property interface for the same. Also
>> >> >> >> >> >> added, support for multi segment gamma mode introduced
>> >> >> >> >> >> in ICL+
>> >> >> >> >> >>
>> >> >> >> >> >> It creates 2 property interfaces :
>> >> >> >> >> >> 1. GAMMA_MODE_CAPS: This is immutable property and
>> >> >> >> >> >> exposes the various gamma modes supported and the lut
>> >> >> >> >> >> ranges. This is an enum property with element as blob
>> >> >> >> >> >> id. Getting the blob id in userspace, user can get the
>> >> >> >> >> >> mode supported and also the range of gamma mode
>> >> >> >> >> >> supported with number of lut
>> >coefficients.
>> >> >> >> >> >>
>> >> >> >> >> >> 2. GAMMA_MODE: This is for user to set the gamma mode
>> >> >> >> >> >> and send the lut values for that particular mode.
>> >> >> >> >> >
>> >> >> >> >> >I think we should just go for the BLOB_ENUM prop type instead.
>> >> >> >> >> >Then the possible values and the current value are all
>> >> >> >> >> >part of the same
>> >prop.
>> >> >> >> >>
>> >> >> >> >> Hi Ville,
>> >> >> >> >> With the current approach, we have enum property with
>> >> >> >> >> values as blob_ids (representing platform capabilities).
>> >> >> >> >> This should not get modified and needs to be immutable.
>> >> >> >> >
>> >> >> >> >That's not quite what we want. We want to let the user
>> >> >> >> >modify the current value so that they can actually select the gamma
>mode.
>> >> >> >> >Otherwise we need yet another prop for it, or we have to
>> >> >> >> >deduce it from the LUT size (that apporach would actually
>> >> >> >> >work for
>> >> >> >> >i915 but may not work for other drivers/hardware).
>> >> >> >> >
>> >> >> >> >>
>> >> >> >> >> Userspace can query the property and get the blob using the blob_ids.
>> >> >> >> >> Thereby getting all the platform capabilities.
>> >> >> >> >>
>> >> >> >> >> Now to set the LUT values, he can use another blob
>> >> >> >> >> property and pass the luts.  This is inline to how
>> >> >> >> >> gamma/degamma is implemented where we have one immutable
>> >> >> >> >> LUT_SIZE property (indicating number of
>> >> >> >> >> luts) and another blob property for actual lut values..
>> >> >> >>
>> >> >> >> Hi Ville,
>> >> >> >> Just to clarify and clear some doubts :)
>> >> >> >>
>> >> >> >> We should be able to set the gamma mode using the blob enum value.
>> >> >> >> Userspace will get the list enum vals (blob ids with mode
>> >> >> >> name
>> >> >> >> embedded) and
>> >> >> >select one and do a setprop to set a mode.
>> >> >> >> Driver will get the blob_id and will be able to get the mode
>> >> >> >> to be set.  So exposing capabilities and setting the mode
>> >> >> >> should be possible with this one
>> >> >> >property. I hope my understanding is correct.
>> >> >> >>
>> >> >> >> Now to send the actual blob values to be set, we need to use
>> >> >> >> some other property interface. Should we use the currently
>> >> >> >> available "gamma blob
>> >> >> >(gamma_lut_property)" property to send the lut values.
>> >> >> >> The challenge there is that it currently uses 16 bit lut
>> >> >> >> values struct drm_color_lut {
>> >> >> >>         __u16 red;
>> >> >> >>         __u16 green;
>> >> >> >>         __u16 blue;
>> >> >> >>         __u16 reserved;
>> >> >> >> };
>> >> >> >> which is not sufficient for HDR usecases. Or should we need a
>> >> >> >> new property for
>> >> >> >advance lut/extended lut like below:
>> >> >> >> https://patchwork.freedesktop.org/patch/294732/?series=30875&
>> >> >> >> rev
>> >> >> >> =7
>> >> >> >>
>> >> >> >> What do you suggest ?
>> >> >> >
>> >> >> >I was thinking that we might get away with reusing the current
>> >> >> >props and just change the interpretation of the data when
>> >> >> >gamma_mode is set. But I'm not sure that's going to work out so
>> >> >> >well if one client sets this up and then another client comes
>> >> >> >along that doesn't understand the new props at all. But even
>> >> >> >with separate props I think we might still end up in a mess
>> >> >> >because the new client wouldn't know how to unset the higher
>> >> >> >precision LUT before setting up the old style prop and the
>> >> >kernel would then refuse the operation with with both props being set.
>> >> >> >
>> >> >> >So I think we might need a client cap for this which simply
>> >> >> >changes how the data in the existing props is represented. So
>> >> >> >internally we could always store things in the new high
>> >> >> >precision format, but we'd convert to/from the old format when
>> >> >> >dealing with an older
>> >client.
>> >> >>
>> >> >> We could also say that if a legacy gamma_mode_property is set
>> >> >> (which will be used by legacy apps or apps not aware of new
>> >> >> interface), in driver we will simply unset the earlier
>> >> >> gamma_mode and fallback to legacy mode (whatever it was for a
>> >> >> particular platform). This way we should be able to deal with
>> >> >> this situation and an explicit
>> >unset may not be needed.
>> >> >What do you think ?
>> >> >
>> >> >I don't want (non-immutable) properties that magically change values.
>> >> >That way lies madness.
>> >>
>> >> Oh ok. So do you suggest that we add some kind of  flag to be set
>> >> by user, based on which we take either legacy or advance_gamma
>> >> path. Is my
>> >understanding correct ?
>> >
>> >Yeah, just another client cap. I can't immediately think of a nicer
>> >way to extend the precision.
>>
>> Sure Ville, I am implementing based on this suggestion.  Basically
>> will expose a new advance_gamma_mode flag as a client cap. Something like:
>>
>> #define DRM_CLIENT_CAP_ADVANCE_GAMMA_MODES     6
>>
>> The new users will set this and we will assume that advance gamma mode paths is
>active.
>> If the flag is not set, driver will take the legacy path and enable
>> default gamma_mode for that particular platform.
>
>It's maybe a still a bit nasty because we basically have to ignore the gamma mode
>prop entirely in that case. Not quite sure what is the best way to shoehorn this in. I
>guess the main problem is what we would report to userspace via the
>(DE)GAMMA_MODE props if the previous client left gamma_mode set in a way that
>conflicts with what (DE)GAMMA_LUT_SIZE suggests it should be.

If user sets the Client Cap then he will ignore the DE)GAMMA_LUT_SIZE properties and will rely
on the new property interface. Data passed as blob in already available GAMMA_LUT property
will be taken in conjunction with the mode set by new GAMMA_MODE property . Now if this app
finishes and new user comes up, and if it doesn't sets the Client CAP that means its not planning to use
new interface, as part of gamma_lut programming we will fallback to linear lut with default mode.

This anyways would have been the state if the first app was not there. And if the new app wants to use legacy
method to change (DE)GAMMA lut he can get the default size in LUT_SIZE property which will still be good for
legacy and work with that. I feel this will solve the issues we may get and both can co-exist.

Regards,
Uma Shankar

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