[v3 0/7] Add Multi Segment Gamma Support

[v3 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 GAMMA_MODE property interface. This is an enum
property with values as blob_id's and exposes
the various gamma modes supported and the lut ranges  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. It can then set one of the modes using this
enum property.

Lut values will be sent through already available GAMMA_LUT
blob property.

It also introduces a CLIENT CAP for advanced GAMMA_MODE.
 This is for user to set the and use advance gamma mode and older
userspace can continue using the legacy paths.

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

v3: Converged to 1 property interface and introduced a Client cap
as suggested by Ville. Fixed review comments received.

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

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

 drivers/gpu/drm/drm_atomic_uapi.c    |   8 +
 drivers/gpu/drm/drm_color_mgmt.c     |  77 ++++
 drivers/gpu/drm/drm_ioctl.c          |   5 +
 drivers/gpu/drm/i915/i915_reg.h      |  17 +
 drivers/gpu/drm/i915/intel_color.c   | 735 ++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_display.c |   3 +
 include/drm/drm_atomic.h             |   1 +
 include/drm/drm_color_mgmt.h         |   8 +
 include/drm/drm_crtc.h               |  17 +
 include/drm/drm_file.h               |   8 +
 include/drm/drm_mode_config.h        |   6 +
 include/uapi/drm/drm.h               |   2 +
 include/uapi/drm/drm_mode.h          |  38 ++
 13 files changed, 918 insertions(+), 7 deletions(-)

-- 
1.9.1

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

[v3 1/7] drm: Add gamma mode property

[Uma Shankar]

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

Add a gamma mode 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 select one of the modes exposed as blob_id as an
enum and set the respective mode.

It can then create the LUT and send it to driver using
already available GAMMA_LUT property as blob.

v2: Addressed Sam Ravnborg's review comments. Implemented
gamma mode with just one property and renamed the current
one to GAMMA_MODE property as recommended by Ville.

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 |  5 +++
 drivers/gpu/drm/drm_color_mgmt.c  | 77 +++++++++++++++++++++++++++++++++++++++
 include/drm/drm_color_mgmt.h      |  8 ++++
 include/drm/drm_crtc.h            |  7 ++++
 include/drm/drm_mode_config.h     |  6 +++
 include/uapi/drm/drm_mode.h       | 38 +++++++++++++++++++
 6 files changed, 141 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index ea797d4..d85e0c9 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -459,6 +459,9 @@ 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) {
+		state->gamma_mode = val;
+		state->color_mgmt_changed |= replaced;
 	} else if (property == config->prop_out_fence_ptr) {
 		s32 __user *fence_ptr = u64_to_user_ptr(val);
 
@@ -495,6 +498,8 @@ 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_property)
+		*val = state->gamma_mode;
 	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..4d6792d 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -176,6 +176,83 @@ 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_ENUM,
+				   "GAMMA_MODE", num_values);
+	if (!prop)
+		return -ENOMEM;
+
+	config->gamma_mode_property = prop;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_color_create_gamma_mode_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_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..f18e9b8 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_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,
+				   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..f2e60bd 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: This is a blob_id and exposes the platform capabilties
+	 * wrt to various gamma modes and the respective lut ranges. This also
+	 * helps user select a gamma mode amongst the supported ones.
+	 */
+	u32 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 7f60e8e..8f961c5b 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -761,6 +761,12 @@ 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. This also exposes
+	 * the lut ranges of the various supported gamma modes to userspace.
+	 */
+	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 83cd163..e70b7f8 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -630,6 +630,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 BIT(0)
+/* LUT is for degamma (before CTM) */
+#define DRM_MODE_LUT_DEGAMMA BIT(1)
+/* linearly interpolate between the points */
+#define DRM_MODE_LUT_INTERPOLATE BIT(2)
+/*
+ * the last value of the previous range is the
+ * first value of the current range.
+ */
+#define DRM_MODE_LUT_REUSE_LAST BIT(3)
+/* the curve must be non-decreasing */
+#define DRM_MODE_LUT_NON_DECREASING BIT(4)
+/* the curve is reflected across origin for negative inputs */
+#define DRM_MODE_LUT_REFLECT_NEGATIVE BIT(5)
+/* the same curve (red) is used for blue and green channels as well */
+#define DRM_MODE_LUT_SINGLE_CHANNEL BIT(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: [v3 1/7] drm: Add gamma mode property

[Daniel Vetter]

On Fri, Apr 12, 2019 at 03:50:57PM +0530, Uma Shankar wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Add a gamma mode 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 select one of the modes exposed as blob_id as an
> enum and set the respective mode.
> 
> It can then create the LUT and send it to driver using
> already available GAMMA_LUT property as blob.
> 
> v2: Addressed Sam Ravnborg's review comments. Implemented
> gamma mode with just one property and renamed the current
> one to GAMMA_MODE property as recommended by Ville.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>

Please also extend the CTM property docs, see

https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#color-management-properties

And especially how GAMMA_MODE interacts with everything else we have
already. I think the current comments don't really explain well how this
is supposed to be used.

Also, since this is quite a complicated data structure, can't we do at
least some basic validation in the core code?
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic_uapi.c |  5 +++
>  drivers/gpu/drm/drm_color_mgmt.c  | 77 +++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_color_mgmt.h      |  8 ++++
>  include/drm/drm_crtc.h            |  7 ++++
>  include/drm/drm_mode_config.h     |  6 +++
>  include/uapi/drm/drm_mode.h       | 38 +++++++++++++++++++
>  6 files changed, 141 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index ea797d4..d85e0c9 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -459,6 +459,9 @@ 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) {
> +		state->gamma_mode = val;
> +		state->color_mgmt_changed |= replaced;
>  	} else if (property == config->prop_out_fence_ptr) {
>  		s32 __user *fence_ptr = u64_to_user_ptr(val);
>  
> @@ -495,6 +498,8 @@ 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_property)
> +		*val = state->gamma_mode;
>  	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..4d6792d 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -176,6 +176,83 @@ 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_ENUM,
> +				   "GAMMA_MODE", num_values);
> +	if (!prop)
> +		return -ENOMEM;
> +
> +	config->gamma_mode_property = prop;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_color_create_gamma_mode_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_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..f18e9b8 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_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,
> +				   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..f2e60bd 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: This is a blob_id and exposes the platform capabilties
> +	 * wrt to various gamma modes and the respective lut ranges. This also
> +	 * helps user select a gamma mode amongst the supported ones.
> +	 */
> +	u32 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 7f60e8e..8f961c5b 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -761,6 +761,12 @@ 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. This also exposes
> +	 * the lut ranges of the various supported gamma modes to userspace.
> +	 */
> +	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 83cd163..e70b7f8 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -630,6 +630,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 BIT(0)
> +/* LUT is for degamma (before CTM) */
> +#define DRM_MODE_LUT_DEGAMMA BIT(1)
> +/* linearly interpolate between the points */
> +#define DRM_MODE_LUT_INTERPOLATE BIT(2)
> +/*
> + * the last value of the previous range is the
> + * first value of the current range.
> + */
> +#define DRM_MODE_LUT_REUSE_LAST BIT(3)
> +/* the curve must be non-decreasing */
> +#define DRM_MODE_LUT_NON_DECREASING BIT(4)
> +/* the curve is reflected across origin for negative inputs */
> +#define DRM_MODE_LUT_REFLECT_NEGATIVE BIT(5)
> +/* the same curve (red) is used for blue and green channels as well */
> +#define DRM_MODE_LUT_SINGLE_CHANNEL BIT(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

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

[v3 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.

v2: Defined and advertise the gamma modes supported on
various platforms as suggested by Ville.

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 | 566 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 562 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index ca341a9..c433215 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -383,6 +383,20 @@ static u32 ilk_lut_10(const struct drm_color_lut *color)
 		drm_color_lut_extract(color->blue, 10);
 }
 
+static bool i9xx_has_10bit_lut(struct drm_i915_private *dev_priv)
+{
+        /*
+         * Bspec:
+         " "NOTE: The 8-bit (non-10-bit) mode is the only
+         *  mode supported by BrookDale-G and Springdale-G."
+         * and
+         * "NOTE: The 8-bit (non-10-bit) mode is the only
+         * mode supported by Alviso and Grantsdale."
+         */
+        return !IS_I845G(dev_priv) && !IS_I865G(dev_priv) &&
+                !IS_I915G(dev_priv) && !IS_I915GM(dev_priv);
+}
+
 /* Loads the legacy palette/gamma unit for the CRTC. */
 static void i9xx_load_luts_internal(const struct intel_crtc_state *crtc_state,
 				    const struct drm_property_blob *blob)
@@ -1221,10 +1235,420 @@ 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,
+};
+
+#define CHV_CGM_DEGAMMA \
+        { \
+                .flags = (DRM_MODE_LUT_DEGAMMA | \
+                          DRM_MODE_LUT_INTERPOLATE | \
+                          DRM_MODE_LUT_NON_DECREASING), \
+                .count = 65, \
+                .input_bpc = 10, .output_bpc = 14, \
+                .start = 0, .end = 1 << 10, \
+                .min = 0, .max = (1 << 14) - 1, \
+        }
+#define CHV_CGM_GAMMA \
+        { \
+                .flags = (DRM_MODE_LUT_GAMMA | \
+                          DRM_MODE_LUT_INTERPOLATE | \
+                          DRM_MODE_LUT_NON_DECREASING), \
+                .count = 257, \
+                .input_bpc = 14, .output_bpc = 10, \
+                .start = 0, .end = 1 << 14, \
+                .min = 0, .max = (1 << 10) - 1, \
+        }
+
+static const struct drm_color_lut_range chv_cgm_degamma[] = {
+        CHV_CGM_DEGAMMA,
+};
+
+static const struct drm_color_lut_range chv_cgm_gamma[] = {
+        CHV_CGM_GAMMA,
+};
+
+static const struct drm_color_lut_range chv_cgm_degamma_i9xx_gamma_8[] = {
+        CHV_CGM_DEGAMMA,
+        I9XX_GAMMA_8,
+};
+
+static const struct drm_color_lut_range chv_cgm_degamma_i965_gamma_10[] = {
+        CHV_CGM_DEGAMMA,
+        I965_GAMMA_10,
+};
+
+static const struct drm_color_lut_range chv_cgm_degamma_cgm_degamma[] = {
+        CHV_CGM_DEGAMMA,
+        CHV_CGM_GAMMA,
+};
+
+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 ivb_gamma_degamma_10[] = {
+        {
+                .flags = (DRM_MODE_LUT_GAMMA |
+                          DRM_MODE_LUT_DEGAMMA |
+                          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_DEGAMMA |
+                          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,
+        }
+};
+
+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,
+	},
+};
+
+static const struct drm_color_lut_range ivb_split_gamma[] = {
+        {
+                .flags = (DRM_MODE_LUT_DEGAMMA |
+                          DRM_MODE_LUT_REFLECT_NEGATIVE),
+                .count = 512,
+                .input_bpc = 9, .output_bpc = 10,
+                .start = 0, .end = (1 << 9) - 1,
+                .min = 0, .max = (1 << 10) - 1,
+        },
+        /* PAL_EXT_GC_MAX */
+        {
+                .flags = (DRM_MODE_LUT_DEGAMMA |
+                          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 << 9, .end = 3 << 9,
+                .min = 0, .max = (8 << 16) - 1,
+        },
+        {
+                .flags = DRM_MODE_LUT_GAMMA,
+                .count = 512,
+                .input_bpc = 9, .output_bpc = 10,
+                .start = 0, .end = (1 << 9) - 1,
+                .min = 0, .max = (1 << 10) - 1,
+        },
+};
+
+/* FIXME input bpc? */
+static const struct drm_color_lut_range ivb_gamma_degamma_12p4[] = {
+        {
+                .flags = (DRM_MODE_LUT_GAMMA |
+                          DRM_MODE_LUT_DEGAMMA |
+                          DRM_MODE_LUT_REFLECT_NEGATIVE |
+                          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,
+        },
+        /* PAL_GC_MAX */
+        {
+                .flags = (DRM_MODE_LUT_GAMMA |
+                          DRM_MODE_LUT_DEGAMMA |
+                          DRM_MODE_LUT_REFLECT_NEGATIVE |
+                          DRM_MODE_LUT_INTERPOLATE |
+                          DRM_MODE_LUT_REUSE_LAST |
+                          DRM_MODE_LUT_NON_DECREASING),
+                .count = 1,
+                .input_bpc = 12, .output_bpc = 16,
+                .start = (1 << 12) - (1 << 12) / 512, .end = 1 << 12,
+                .min = 0, .max = 1 << 16,
+        },
+        /* PAL_EXT_GC_MAX */
+        {
+                .flags = (DRM_MODE_LUT_GAMMA |
+                          DRM_MODE_LUT_DEGAMMA |
+                          DRM_MODE_LUT_REFLECT_NEGATIVE |
+                          DRM_MODE_LUT_INTERPOLATE |
+                          DRM_MODE_LUT_REUSE_LAST |
+                          DRM_MODE_LUT_NON_DECREASING),
+                .count = 1,
+                .input_bpc = 12, .output_bpc = 16,
+                .start = 1 << 12, .end = 3 << 12,
+                .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);
 	bool has_ctm = INTEL_INFO(dev_priv)->color.degamma_lut_size != 0;
+	int degamma_lut_size, gamma_lut_size;
 
 	drm_mode_crtc_set_gamma_size(&crtc->base, 256);
 
@@ -1233,25 +1657,159 @@ void intel_color_init(struct intel_crtc *crtc)
 			dev_priv->display.color_check = chv_color_check;
 			dev_priv->display.color_commit = i9xx_color_commit;
 			dev_priv->display.load_luts = cherryview_load_luts;
+
+			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,
+						       "interpolated gamma",
+						       i965_gamma_10,
+						       sizeof(i965_gamma_10));
+			drm_color_add_gamma_mode_range(&dev_priv->drm,
+						       "CGM gamma",
+						       	chv_cgm_gamma,
+							sizeof(chv_cgm_gamma));
+			drm_color_add_gamma_mode_range(&dev_priv->drm,
+						       "CGM degamma",
+						       	chv_cgm_degamma,
+							sizeof(chv_cgm_degamma));
+			drm_color_add_gamma_mode_range(&dev_priv->drm,
+						       "CGM degamma with 8bit gamma",
+						       	chv_cgm_degamma_i9xx_gamma_8,
+							sizeof(chv_cgm_degamma));
+			drm_color_add_gamma_mode_range(&dev_priv->drm,
+						       "CGM degamma with 10bit interpolated gamma",
+						       	chv_cgm_degamma_i965_gamma_10,
+							sizeof(chv_cgm_degamma));
+			drm_color_add_gamma_mode_range(&dev_priv->drm,
+						       "CGM degamma with CGM gamma",
+						       	chv_cgm_degamma_cgm_degamma,
+							sizeof(chv_cgm_degamma));
 		} else if (INTEL_GEN(dev_priv) >= 4) {
 			dev_priv->display.color_check = i9xx_color_check;
 			dev_priv->display.color_commit = i9xx_color_commit;
 			dev_priv->display.load_luts = i965_load_luts;
+
+			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,
+						       "interpolated gamma",
+						       i965_gamma_10,
+						       sizeof(i965_gamma_10));
 		} else {
 			dev_priv->display.color_check = i9xx_color_check;
 			dev_priv->display.color_commit = i9xx_color_commit;
 			dev_priv->display.load_luts = i9xx_load_luts;
+
+			degamma_lut_size = 0;
+			gamma_lut_size = 0;
+			has_ctm = false;
+
+			drm_color_add_gamma_mode_range(&dev_priv->drm,
+						       "8bit gamma",
+						       	i9xx_gamma_8,
+							sizeof(i9xx_gamma_8));
+
+			if (i9xx_has_10bit_lut(dev_priv)) {
+				/* 10bit interpolated gamma */
+				gamma_lut_size = I9XX_LUT_SIZE_10BIT;
+
+				drm_color_add_gamma_mode_range(&dev_priv->drm,
+							       "interpolated gamma",
+							       	i9xx_gamma_10_slope,
+								sizeof(i9xx_gamma_10_slope));
+			}
 		}
 	} 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) >= 7)
+			/* don't advertize the >= 1.0 entries */
+			degamma_lut_size = 0;
+			gamma_lut_size = ILK_LUT_SIZE_10BIT;
+			has_ctm = true;
+
+			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));
+		} else if (INTEL_GEN(dev_priv) >= 7) {
 			dev_priv->display.color_check = ivb_color_check;
-		else
+
+			/* don't advertize the >= 1.0 entries */
+			degamma_lut_size = IVB_LUT_SIZE_SPLIT;
+			gamma_lut_size = IVB_LUT_SIZE_SPLIT;
+			has_ctm = true;
+
+			drm_color_add_gamma_mode_range(&dev_priv->drm,
+						       "8bit gamma or degamma",
+						       ilk_gamma_degamma_8,
+						       sizeof(ilk_gamma_degamma_8));
+			drm_color_add_gamma_mode_range(&dev_priv->drm,
+						       "split gamma",
+						       ivb_split_gamma,
+						       sizeof(ivb_split_gamma));
+			drm_color_add_gamma_mode_range(&dev_priv->drm,
+						       "10bit gamma or degamma",
+						       ivb_gamma_degamma_10,
+						       sizeof(ivb_gamma_degamma_10));
+			drm_color_add_gamma_mode_range(&dev_priv->drm,
+						       "interpolated gamma or degamma",
+						       	ivb_gamma_degamma_12p4,
+							sizeof(ivb_gamma_degamma_12p4));
+		} else {
 			dev_priv->display.color_check = ilk_color_check;
 
+			degamma_lut_size = 0;
+			gamma_lut_size = ILK_LUT_SIZE_10BIT;
+			has_ctm = true;
+
+			drm_color_add_gamma_mode_range(&dev_priv->drm,
+						       "8bit gamma or degamma",
+						       	ilk_gamma_degamma_8,
+							sizeof(ilk_gamma_degamma_8));
+			drm_color_add_gamma_mode_range(&dev_priv->drm,
+						       "10bit gamma or degamma",
+						       ilk_gamma_degamma_10,
+						       sizeof(ilk_gamma_degamma_10));
+			drm_color_add_gamma_mode_range(&dev_priv->drm,
+						       "interpolated gamma or degamma",
+						       ilk_gamma_degamma_12p4,
+						       sizeof(ilk_gamma_degamma_12p4));
+		}
+
 		if (INTEL_GEN(dev_priv) >= 9)
 			dev_priv->display.color_commit = skl_color_commit;
 		else if (IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
-- 
1.9.1

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

[v3 3/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 9c206e8..5554b0c 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7196,6 +7196,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)
@@ -10136,6 +10137,22 @@ enum skl_power_gate {
 #define PRE_CSC_GAMC_INDEX(pipe)	_MMIO_PIPE(pipe, _PRE_CSC_GAMC_INDEX_A, _PRE_CSC_GAMC_INDEX_B)
 #define PRE_CSC_GAMC_DATA(pipe)		_MMIO_PIPE(pipe, _PRE_CSC_GAMC_DATA_A, _PRE_CSC_GAMC_DATA_B)
 
+/* Add registers for Gen11 Multi Segmented Gamma Mode */
+#define _PAL_PREC_MULTI_SEG_INDEX_A	0x4A408
+#define _PAL_PREC_MULTI_SEG_INDEX_B	0x4AC08
+#define  PAL_PREC_MULTI_SEGMENT_AUTO_INCREMENT		BIT(15)
+#define  PAL_PREC_MULTI_SEGMENT_INDEX_VALUE_MASK	(0x1f << 0)
+
+#define _PAL_PREC_MULTI_SEG_DATA_A	0x4A40C
+#define _PAL_PREC_MULTI_SEG_DATA_B	0x4AC0C
+
+#define PREC_PAL_MULTI_SEG_INDEX(pipe)	_MMIO_PIPE(pipe, \
+					_PAL_PREC_MULTI_SEG_INDEX_A, \
+					_PAL_PREC_MULTI_SEG_INDEX_B)
+#define PREC_PAL_MULTI_SEG_DATA(pipe)	_MMIO_PIPE(pipe, \
+					_PAL_PREC_MULTI_SEG_DATA_A, \
+					_PAL_PREC_MULTI_SEG_DATA_B)
+
 /* pipe CSC & degamma/gamma LUTs on CHV */
 #define _CGM_PIPE_A_CSC_COEFF01	(VLV_DISPLAY_BASE + 0x67900)
 #define _CGM_PIPE_A_CSC_COEFF23	(VLV_DISPLAY_BASE + 0x67904)
-- 
1.9.1

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

[v3 4/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.

v2: Aligned to just 1 property interface as suggested by
Ville. Fixed Ville's review comments.

Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
 drivers/gpu/drm/i915/intel_color.c | 166 ++++++++++++++++++++++++++++++++++++-
 include/drm/drm_crtc.h             |   3 +
 2 files changed, 165 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index c433215..d4ce1ed 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -58,6 +58,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] = {
@@ -93,6 +99,22 @@
 	0x0800, 0x0100, 0x0800,
 };
 
+/* ilk+ "12.4" interpolated format (high 10 bits) */
+static u32 ilk_lut_12p4_ldw(const struct drm_color_lut *color)
+{
+	return (color->red >> 6) << 20 |
+		(color->green >> 6) << 10 |
+		(color->blue >> 6);
+}
+
+/* ilk+ "12.4" interpolated format (low 6 bits) */
+static u32 ilk_lut_12p4_udw(const struct drm_color_lut *color)
+{
+	return (color->red & 0x3f) << 24 |
+		(color->green & 0x3f) << 14 |
+		(color->blue & 0x3f);
+}
+
 static bool lut_is_legacy(const struct drm_property_blob *lut)
 {
 	return drm_color_lut_size(lut) == LEGACY_LUT_LENGTH;
@@ -781,6 +803,118 @@ static void glk_load_luts(const struct intel_crtc_state *crtc_state)
 	}
 }
 
+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 = ilk_lut_12p4_udw(&lut[i]);
+
+			I915_WRITE(PREC_PAL_DATA(pipe), word);
+
+			/* For ODD index */
+			word = ilk_lut_12p4_ldw(&lut[i]);
+
+			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 = ilk_lut_12p4_udw(&lut[i]);
+
+			I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe), word);
+
+			/* For ODD index */
+			word = ilk_lut_12p4_ldw(&lut[i]);
+
+			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_lut_blob =
+					crtc_state->base.gamma_lut;
+	struct drm_color_lut *gamma_lut = NULL;
+
+	if (gamma_lut_blob)
+		gamma_lut = gamma_lut_blob->data;
+
+	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)
 {
 	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
@@ -792,6 +926,9 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
 	if ((crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) ==
 	    GAMMA_MODE_MODE_8BIT) {
 		i9xx_load_luts(crtc_state);
+	} else if (crtc_state->base.gamma_mode_type ==
+		   MULTI_SEGMENTED_GAMMA_MODE_12BIT) {
+		icl_load_gamma_multi_segmented_lut(crtc_state, 0);
 	} else {
 		bdw_load_lut_10(crtc, gamma_lut, PAL_PREC_INDEX_VALUE(0));
 		ivb_load_lut_10_max(crtc);
@@ -1186,10 +1323,28 @@ 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)
 {
+	struct drm_device *dev = crtc_state->base.crtc->dev;
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_property *property = config->gamma_mode_property;
+	struct drm_property_enum *prop_enum;
+	u32 index = 0;
 	u32 gamma_mode = 0;
 
+	list_for_each_entry(prop_enum, &property->enum_list, head) {
+		if (prop_enum->value == crtc_state->base.gamma_mode) {
+			if (!strcmp(prop_enum->name,
+			    "multi-segmented gamma")) {
+				crtc_state->base.gamma_mode_type =
+				MULTI_SEGMENTED_GAMMA_MODE_12BIT;
+				DRM_INFO("multi-segment enabled\n");
+			}
+			break;
+		}
+		index++;
+	}
+
 	if (crtc_state->base.degamma_lut)
 		gamma_mode |= PRE_CSC_GAMMA_ENABLE;
 
@@ -1200,6 +1355,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;
 
@@ -1755,7 +1913,7 @@ void intel_color_init(struct intel_crtc *crtc)
 
 			drm_color_add_gamma_mode_range(&dev_priv->drm,
 						       "8bit gamma",
-						       	i9xx_gamma_8,
+							i9xx_gamma_8,
 							sizeof(i9xx_gamma_8));
 			drm_color_add_gamma_mode_range(&dev_priv->drm,
 						       "10bit gamma",
@@ -1798,8 +1956,8 @@ void intel_color_init(struct intel_crtc *crtc)
 
 			drm_color_add_gamma_mode_range(&dev_priv->drm,
 						       "8bit gamma or degamma",
-						       	ilk_gamma_degamma_8,
-							sizeof(ilk_gamma_degamma_8));
+						       ilk_gamma_degamma_8,
+						       sizeof(ilk_gamma_degamma_8));
 			drm_color_add_gamma_mode_range(&dev_priv->drm,
 						       "10bit gamma or degamma",
 						       ilk_gamma_degamma_10,
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index f2e60bd..f789359 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -255,6 +255,9 @@ struct drm_crtc_state {
 	 */
 	u32 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

[v3 5/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.

v2: Enabled just 1 property interface for gamma_mode,
as suggested by Ville.

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

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index d4ce1ed..edf5ff8 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -1884,6 +1884,9 @@ void intel_color_init(struct intel_crtc *crtc)
 	} else {
 		if (INTEL_GEN(dev_priv) >= 11) {
 			dev_priv->display.color_check = icl_color_check;
+
+			drm_crtc_attach_gamma_mode_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 f29a348..3e47935 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15680,6 +15680,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_property(&dev_priv->drm, 4);
+
 	for_each_pipe(dev_priv, pipe) {
 		ret = intel_crtc_init(dev_priv, pipe);
 		if (ret) {
-- 
1.9.1

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

[v3 6/7] drm: Add Client Cap for advance gamma mode

[Uma Shankar]

Introduced a client cap for advance cap mode
capability. Userspace should set this to get
to be able to use the new gamma_mode property.

If this is not set, driver will work in legacy
mode.

Suggested-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_ioctl.c       | 5 +++++
 include/drm/drm_atomic.h          | 1 +
 include/drm/drm_crtc.h            | 7 +++++++
 include/drm/drm_file.h            | 8 ++++++++
 include/uapi/drm/drm.h            | 2 ++
 6 files changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index d85e0c9..590c87a 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -974,6 +974,8 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
 			break;
 		}
 
+		crtc_state->advance_gamma_mode_active =
+					state->advance_gamma_mode_active;
 		ret = drm_atomic_crtc_set_property(crtc,
 				crtc_state, prop, prop_value);
 		break;
@@ -1297,6 +1299,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
 	state->acquire_ctx = &ctx;
 	state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET);
+	state->advance_gamma_mode_active = file_priv->advance_gamma_mode_active;
 
 retry:
 	copied_objs = 0;
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index d337f16..e593a4c 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -348,6 +348,11 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
 			return -EINVAL;
 		file_priv->writeback_connectors = req->value;
 		break;
+	case DRM_CLIENT_CAP_ADVANCE_GAMMA_MODES:
+		if (req->value > 1)
+			return -EINVAL;
+		file_priv->advance_gamma_mode_active = req->value;
+		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 824a5ed..02c1a68 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -338,6 +338,7 @@ struct drm_atomic_state {
 	 * states.
 	 */
 	bool duplicated : 1;
+	bool advance_gamma_mode_active : 1;
 	struct __drm_planes_state *planes;
 	struct __drm_crtcs_state *crtcs;
 	int num_connector;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index f789359..f11dc25 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -170,6 +170,11 @@ struct drm_crtc_state {
 	bool color_mgmt_changed : 1;
 
 	/**
+	 * This is to indicate advance gamma mode support
+	 */
+	bool advance_gamma_mode_active : 1;
+
+	/**
 	 * @no_vblank:
 	 *
 	 * Reflects the ability of a CRTC to send VBLANK events. This state
@@ -952,6 +957,8 @@ struct drm_crtc {
 	 */
 	bool enabled;
 
+	bool advance_gamma_mode_active : 1;
+
 	/**
 	 * @mode:
 	 *
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index 6710b61..b5aa24e 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -201,6 +201,14 @@ struct drm_file {
 	bool writeback_connectors;
 
 	/**
+	 * This is to enable advance gamma modes using
+	 * gamma_mode property
+	 *
+	 * True if client understands advance gamma
+	 */
+	bool advance_gamma_mode_active : 1;
+
+	/**
 	 * @is_master:
 	 *
 	 * This client is the creator of @master. Protected by struct
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 236b01a..abef966 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -696,6 +696,8 @@ struct drm_get_cap {
  */
 #define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS	5
 
+#define DRM_CLIENT_CAP_ADVANCE_GAMMA_MODES	6
+
 /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
 struct drm_set_client_cap {
 	__u64 capability;
-- 
1.9.1

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

Re: [v3 6/7] drm: Add Client Cap for advance gamma mode

[Lankhorst, Maarten]

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

fre 2019-04-12 klockan 15:51 +0530 skrev Uma Shankar:
> Introduced a client cap for advance cap mode
> capability. Userspace should set this to get
> to be able to use the new gamma_mode property.
> 
> If this is not set, driver will work in legacy
> mode.
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>

Nack, this doesn't seem like a sensible idea. We already guard it
behind the gamma mode property. Userspace shouldn't set the gamma mode
to a value it doesn't understand.

~Maarten

>  drivers/gpu/drm/drm_atomic_uapi.c | 3 +++
>  drivers/gpu/drm/drm_ioctl.c       | 5 +++++
>  include/drm/drm_atomic.h          | 1 +
>  include/drm/drm_crtc.h            | 7 +++++++
>  include/drm/drm_file.h            | 8 ++++++++
>  include/uapi/drm/drm.h            | 2 ++
>  6 files changed, 26 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index d85e0c9..590c87a 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -974,6 +974,8 @@ int drm_atomic_set_property(struct
> drm_atomic_state *state,
>  			break;
>  		}
>  
> +		crtc_state->advance_gamma_mode_active =
> +					state-
> >advance_gamma_mode_active;
>  		ret = drm_atomic_crtc_set_property(crtc,
>  				crtc_state, prop, prop_value);
>  		break;
> @@ -1297,6 +1299,7 @@ int drm_mode_atomic_ioctl(struct drm_device
> *dev,
>  	drm_modeset_acquire_init(&ctx,
> DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
>  	state->acquire_ctx = &ctx;
>  	state->allow_modeset = !!(arg->flags &
> DRM_MODE_ATOMIC_ALLOW_MODESET);
> +	state->advance_gamma_mode_active = file_priv-
> >advance_gamma_mode_active;
>  
>  retry:
>  	copied_objs = 0;
> diff --git a/drivers/gpu/drm/drm_ioctl.c
> b/drivers/gpu/drm/drm_ioctl.c
> index d337f16..e593a4c 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -348,6 +348,11 @@ static int drm_getcap(struct drm_device *dev,
> void *data, struct drm_file *file_
>  			return -EINVAL;
>  		file_priv->writeback_connectors = req->value;
>  		break;
> +	case DRM_CLIENT_CAP_ADVANCE_GAMMA_MODES:
> +		if (req->value > 1)
> +			return -EINVAL;
> +		file_priv->advance_gamma_mode_active = req->value;
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 824a5ed..02c1a68 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -338,6 +338,7 @@ struct drm_atomic_state {
>  	 * states.
>  	 */
>  	bool duplicated : 1;
> +	bool advance_gamma_mode_active : 1;
>  	struct __drm_planes_state *planes;
>  	struct __drm_crtcs_state *crtcs;
>  	int num_connector;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index f789359..f11dc25 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -170,6 +170,11 @@ struct drm_crtc_state {
>  	bool color_mgmt_changed : 1;
>  
>  	/**
> +	 * This is to indicate advance gamma mode support
> +	 */
> +	bool advance_gamma_mode_active : 1;
> +
> +	/**
>  	 * @no_vblank:
>  	 *
>  	 * Reflects the ability of a CRTC to send VBLANK events.
> This state
> @@ -952,6 +957,8 @@ struct drm_crtc {
>  	 */
>  	bool enabled;
>  
> +	bool advance_gamma_mode_active : 1;
> +
>  	/**
>  	 * @mode:
>  	 *
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index 6710b61..b5aa24e 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -201,6 +201,14 @@ struct drm_file {
>  	bool writeback_connectors;
>  
>  	/**
> +	 * This is to enable advance gamma modes using
> +	 * gamma_mode property
> +	 *
> +	 * True if client understands advance gamma
> +	 */
> +	bool advance_gamma_mode_active : 1;
> +
> +	/**
>  	 * @is_master:
>  	 *
>  	 * This client is the creator of @master. Protected by
> struct
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 236b01a..abef966 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -696,6 +696,8 @@ struct drm_get_cap {
>   */
>  #define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS	5
>  
> +#define DRM_CLIENT_CAP_ADVANCE_GAMMA_MODES	6
> +
>  /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
>  struct drm_set_client_cap {
>  	__u64 capability;

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

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

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

Re: [Intel-gfx] [v3 6/7] drm: Add Client Cap for advance gamma mode

[Ville Syrjälä]

On Mon, Apr 15, 2019 at 10:57:52AM +0000, Lankhorst, Maarten wrote:
> fre 2019-04-12 klockan 15:51 +0530 skrev Uma Shankar:
> > Introduced a client cap for advance cap mode
> > capability. Userspace should set this to get
> > to be able to use the new gamma_mode property.
> > 
> > If this is not set, driver will work in legacy
> > mode.
> > 
> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> 
> Nack, this doesn't seem like a sensible idea. We already guard it
> behind the gamma mode property. Userspace shouldn't set the gamma mode
> to a value it doesn't understand.

Old userspace wouldn't know what a gamma_mode prop is. While a client
cap isn't an entirely satisfactory solution I can't think of a better
way at the moment.

Well, maybe the old "hey kernel, please reset all my props to some
sane default" idea could be another way to deal with this sort of stuff.

> 
> ~Maarten
> 
> >  drivers/gpu/drm/drm_atomic_uapi.c | 3 +++
> >  drivers/gpu/drm/drm_ioctl.c       | 5 +++++
> >  include/drm/drm_atomic.h          | 1 +
> >  include/drm/drm_crtc.h            | 7 +++++++
> >  include/drm/drm_file.h            | 8 ++++++++
> >  include/uapi/drm/drm.h            | 2 ++
> >  6 files changed, 26 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> > b/drivers/gpu/drm/drm_atomic_uapi.c
> > index d85e0c9..590c87a 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -974,6 +974,8 @@ int drm_atomic_set_property(struct
> > drm_atomic_state *state,
> >  			break;
> >  		}
> >  
> > +		crtc_state->advance_gamma_mode_active =
> > +					state-
> > >advance_gamma_mode_active;
> >  		ret = drm_atomic_crtc_set_property(crtc,
> >  				crtc_state, prop, prop_value);
> >  		break;
> > @@ -1297,6 +1299,7 @@ int drm_mode_atomic_ioctl(struct drm_device
> > *dev,
> >  	drm_modeset_acquire_init(&ctx,
> > DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
> >  	state->acquire_ctx = &ctx;
> >  	state->allow_modeset = !!(arg->flags &
> > DRM_MODE_ATOMIC_ALLOW_MODESET);
> > +	state->advance_gamma_mode_active = file_priv-
> > >advance_gamma_mode_active;
> >  
> >  retry:
> >  	copied_objs = 0;
> > diff --git a/drivers/gpu/drm/drm_ioctl.c
> > b/drivers/gpu/drm/drm_ioctl.c
> > index d337f16..e593a4c 100644
> > --- a/drivers/gpu/drm/drm_ioctl.c
> > +++ b/drivers/gpu/drm/drm_ioctl.c
> > @@ -348,6 +348,11 @@ static int drm_getcap(struct drm_device *dev,
> > void *data, struct drm_file *file_
> >  			return -EINVAL;
> >  		file_priv->writeback_connectors = req->value;
> >  		break;
> > +	case DRM_CLIENT_CAP_ADVANCE_GAMMA_MODES:
> > +		if (req->value > 1)
> > +			return -EINVAL;
> > +		file_priv->advance_gamma_mode_active = req->value;
> > +		break;
> >  	default:
> >  		return -EINVAL;
> >  	}
> > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > index 824a5ed..02c1a68 100644
> > --- a/include/drm/drm_atomic.h
> > +++ b/include/drm/drm_atomic.h
> > @@ -338,6 +338,7 @@ struct drm_atomic_state {
> >  	 * states.
> >  	 */
> >  	bool duplicated : 1;
> > +	bool advance_gamma_mode_active : 1;
> >  	struct __drm_planes_state *planes;
> >  	struct __drm_crtcs_state *crtcs;
> >  	int num_connector;
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index f789359..f11dc25 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -170,6 +170,11 @@ struct drm_crtc_state {
> >  	bool color_mgmt_changed : 1;
> >  
> >  	/**
> > +	 * This is to indicate advance gamma mode support
> > +	 */
> > +	bool advance_gamma_mode_active : 1;
> > +
> > +	/**
> >  	 * @no_vblank:
> >  	 *
> >  	 * Reflects the ability of a CRTC to send VBLANK events.
> > This state
> > @@ -952,6 +957,8 @@ struct drm_crtc {
> >  	 */
> >  	bool enabled;
> >  
> > +	bool advance_gamma_mode_active : 1;
> > +
> >  	/**
> >  	 * @mode:
> >  	 *
> > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> > index 6710b61..b5aa24e 100644
> > --- a/include/drm/drm_file.h
> > +++ b/include/drm/drm_file.h
> > @@ -201,6 +201,14 @@ struct drm_file {
> >  	bool writeback_connectors;
> >  
> >  	/**
> > +	 * This is to enable advance gamma modes using
> > +	 * gamma_mode property
> > +	 *
> > +	 * True if client understands advance gamma
> > +	 */
> > +	bool advance_gamma_mode_active : 1;
> > +
> > +	/**
> >  	 * @is_master:
> >  	 *
> >  	 * This client is the creator of @master. Protected by
> > struct
> > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > index 236b01a..abef966 100644
> > --- a/include/uapi/drm/drm.h
> > +++ b/include/uapi/drm/drm.h
> > @@ -696,6 +696,8 @@ struct drm_get_cap {
> >   */
> >  #define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS	5
> >  
> > +#define DRM_CLIENT_CAP_ADVANCE_GAMMA_MODES	6
> > +
> >  /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
> >  struct drm_set_client_cap {
> >  	__u64 capability;



> _______________________________________________
> 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: [v3 6/7] drm: Add Client Cap for advance gamma mode

[Lankhorst, Maarten]

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

mån 2019-04-15 klockan 15:43 +0300 skrev Ville Syrjälä:
> On Mon, Apr 15, 2019 at 10:57:52AM +0000, Lankhorst, Maarten wrote:
> > fre 2019-04-12 klockan 15:51 +0530 skrev Uma Shankar:
> > > Introduced a client cap for advance cap mode
> > > capability. Userspace should set this to get
> > > to be able to use the new gamma_mode property.
> > > 
> > > If this is not set, driver will work in legacy
> > > mode.
> > > 
> > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> > 
> > Nack, this doesn't seem like a sensible idea. We already guard it
> > behind the gamma mode property. Userspace shouldn't set the gamma
> > mode
> > to a value it doesn't understand.
> 
> Old userspace wouldn't know what a gamma_mode prop is. While a client
> cap isn't an entirely satisfactory solution I can't think of a better
> way at the moment.
> 
> Well, maybe the old "hey kernel, please reset all my props to some
> sane default" idea could be another way to deal with this sort of
> stuff.
Yes, but this approach wouldn't work, lot of other properties that can
cause problems when not reset, like plane alpha and blend mode. I don't
see why gamma is special in that case.

If userspace did set it to some special value, then presumably it knows
how to reset it too. It would be different if the split gamma mode was
the default. Then I would understand this, but right now this would
even break s/r.

~Maarten

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

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

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

Re: [v3 6/7] drm: Add Client Cap for advance gamma mode

[Sharma, Shashank]

> -----Original Message-----
> From: Lankhorst, Maarten
> Sent: Monday, April 15, 2019 4:28 PM
> To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org; dri-
> devel@lists.freedesktop.org
> Cc: Syrjala, Ville <ville.syrjala@intel.com>; emil.l.velikov@gmail.com;
> sam@ravnborg.org; Roper, Matthew D <matthew.d.roper@intel.com>;
> seanpaul@chromium.org; brian.starkey@arm.com; dcastagna@chromium.org;
> Sharma, Shashank <shashank.sharma@intel.com>
> Subject: Re: [v3 6/7] drm: Add Client Cap for advance gamma mode
> 
> fre 2019-04-12 klockan 15:51 +0530 skrev Uma Shankar:
> > Introduced a client cap for advance cap mode
> > capability. Userspace should set this to get
> > to be able to use the new gamma_mode property.
> >
> > If this is not set, driver will work in legacy
> > mode.
> >
> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> 
> Nack, this doesn't seem like a sensible idea. We already guard it
> behind the gamma mode property. Userspace shouldn't set the gamma mode
> to a value it doesn't understand.
> 
> ~Maarten

Hey Maarten, 
In that case, what do you suggest should be the right way to do this ? 

@Ville, any comments here ? 

Regards
Shashank
> 
> >  drivers/gpu/drm/drm_atomic_uapi.c | 3 +++
> >  drivers/gpu/drm/drm_ioctl.c       | 5 +++++
> >  include/drm/drm_atomic.h          | 1 +
> >  include/drm/drm_crtc.h            | 7 +++++++
> >  include/drm/drm_file.h            | 8 ++++++++
> >  include/uapi/drm/drm.h            | 2 ++
> >  6 files changed, 26 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> > b/drivers/gpu/drm/drm_atomic_uapi.c
> > index d85e0c9..590c87a 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -974,6 +974,8 @@ int drm_atomic_set_property(struct
> > drm_atomic_state *state,
> >  			break;
> >  		}
> >
> > +		crtc_state->advance_gamma_mode_active =
> > +					state-
> > >advance_gamma_mode_active;
> >  		ret = drm_atomic_crtc_set_property(crtc,
> >  				crtc_state, prop, prop_value);
> >  		break;
> > @@ -1297,6 +1299,7 @@ int drm_mode_atomic_ioctl(struct drm_device
> > *dev,
> >  	drm_modeset_acquire_init(&ctx,
> > DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
> >  	state->acquire_ctx = &ctx;
> >  	state->allow_modeset = !!(arg->flags &
> > DRM_MODE_ATOMIC_ALLOW_MODESET);
> > +	state->advance_gamma_mode_active = file_priv-
> > >advance_gamma_mode_active;
> >
> >  retry:
> >  	copied_objs = 0;
> > diff --git a/drivers/gpu/drm/drm_ioctl.c
> > b/drivers/gpu/drm/drm_ioctl.c
> > index d337f16..e593a4c 100644
> > --- a/drivers/gpu/drm/drm_ioctl.c
> > +++ b/drivers/gpu/drm/drm_ioctl.c
> > @@ -348,6 +348,11 @@ static int drm_getcap(struct drm_device *dev,
> > void *data, struct drm_file *file_
> >  			return -EINVAL;
> >  		file_priv->writeback_connectors = req->value;
> >  		break;
> > +	case DRM_CLIENT_CAP_ADVANCE_GAMMA_MODES:
> > +		if (req->value > 1)
> > +			return -EINVAL;
> > +		file_priv->advance_gamma_mode_active = req->value;
> > +		break;
> >  	default:
> >  		return -EINVAL;
> >  	}
> > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > index 824a5ed..02c1a68 100644
> > --- a/include/drm/drm_atomic.h
> > +++ b/include/drm/drm_atomic.h
> > @@ -338,6 +338,7 @@ struct drm_atomic_state {
> >  	 * states.
> >  	 */
> >  	bool duplicated : 1;
> > +	bool advance_gamma_mode_active : 1;
> >  	struct __drm_planes_state *planes;
> >  	struct __drm_crtcs_state *crtcs;
> >  	int num_connector;
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index f789359..f11dc25 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -170,6 +170,11 @@ struct drm_crtc_state {
> >  	bool color_mgmt_changed : 1;
> >
> >  	/**
> > +	 * This is to indicate advance gamma mode support
> > +	 */
> > +	bool advance_gamma_mode_active : 1;
> > +
> > +	/**
> >  	 * @no_vblank:
> >  	 *
> >  	 * Reflects the ability of a CRTC to send VBLANK events.
> > This state
> > @@ -952,6 +957,8 @@ struct drm_crtc {
> >  	 */
> >  	bool enabled;
> >
> > +	bool advance_gamma_mode_active : 1;
> > +
> >  	/**
> >  	 * @mode:
> >  	 *
> > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> > index 6710b61..b5aa24e 100644
> > --- a/include/drm/drm_file.h
> > +++ b/include/drm/drm_file.h
> > @@ -201,6 +201,14 @@ struct drm_file {
> >  	bool writeback_connectors;
> >
> >  	/**
> > +	 * This is to enable advance gamma modes using
> > +	 * gamma_mode property
> > +	 *
> > +	 * True if client understands advance gamma
> > +	 */
> > +	bool advance_gamma_mode_active : 1;
> > +
> > +	/**
> >  	 * @is_master:
> >  	 *
> >  	 * This client is the creator of @master. Protected by
> > struct
> > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > index 236b01a..abef966 100644
> > --- a/include/uapi/drm/drm.h
> > +++ b/include/uapi/drm/drm.h
> > @@ -696,6 +696,8 @@ struct drm_get_cap {
> >   */
> >  #define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS	5
> >
> > +#define DRM_CLIENT_CAP_ADVANCE_GAMMA_MODES	6
> > +
> >  /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
> >  struct drm_set_client_cap {
> >  	__u64 capability;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [v3 6/7] drm: Add Client Cap for advance gamma mode

[Lankhorst, Maarten]

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

mån 2019-04-15 klockan 19:26 +0530 skrev Sharma, Shashank:
> > -----Original Message-----
> > From: Lankhorst, Maarten
> > Sent: Monday, April 15, 2019 4:28 PM
> > To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedeskt
> > op.org; dri-
> > devel@lists.freedesktop.org
> > Cc: Syrjala, Ville <ville.syrjala@intel.com>; emil.l.velikov@gmail.
> > com;
> > sam@ravnborg.org; Roper, Matthew D <matthew.d.roper@intel.com>;
> > seanpaul@chromium.org; brian.starkey@arm.com; dcastagna@chromium.or
> > g;
> > Sharma, Shashank <shashank.sharma@intel.com>
> > Subject: Re: [v3 6/7] drm: Add Client Cap for advance gamma mode
> > 
> > fre 2019-04-12 klockan 15:51 +0530 skrev Uma Shankar:
> > > Introduced a client cap for advance cap mode
> > > capability. Userspace should set this to get
> > > to be able to use the new gamma_mode property.
> > > 
> > > If this is not set, driver will work in legacy
> > > mode.
> > > 
> > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> > 
> > Nack, this doesn't seem like a sensible idea. We already guard it
> > behind the gamma mode property. Userspace shouldn't set the gamma
> > mode
> > to a value it doesn't understand.
> > 
> > ~Maarten
> 
> Hey Maarten, 
> In that case, what do you suggest should be the right way to do this
> ? 
> 
> @Ville, any comments here ? 
> 
I would say drop this patch, and just enable segmented gamma
unconditionally, it's not the first property that can cause trouble
when not understood.

~Maarten

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

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

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

Re: [v3 6/7] drm: Add Client Cap for advance gamma mode

[Sharma, Shashank]

On 4/15/2019 7:42 PM, Lankhorst, Maarten wrote:
> mån 2019-04-15 klockan 19:26 +0530 skrev Sharma, Shashank:
>>> -----Original Message-----
>>> From: Lankhorst, Maarten
>>> Sent: Monday, April 15, 2019 4:28 PM
>>> To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedeskt
>>> op.org; dri-
>>> devel@lists.freedesktop.org
>>> Cc: Syrjala, Ville <ville.syrjala@intel.com>; emil.l.velikov@gmail.
>>> com;
>>> sam@ravnborg.org; Roper, Matthew D <matthew.d.roper@intel.com>;
>>> seanpaul@chromium.org; brian.starkey@arm.com; dcastagna@chromium.or
>>> g;
>>> Sharma, Shashank <shashank.sharma@intel.com>
>>> Subject: Re: [v3 6/7] drm: Add Client Cap for advance gamma mode
>>>
>>> fre 2019-04-12 klockan 15:51 +0530 skrev Uma Shankar:
>>>> Introduced a client cap for advance cap mode
>>>> capability. Userspace should set this to get
>>>> to be able to use the new gamma_mode property.
>>>>
>>>> If this is not set, driver will work in legacy
>>>> mode.
>>>>
>>>> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>>> Nack, this doesn't seem like a sensible idea. We already guard it
>>> behind the gamma mode property. Userspace shouldn't set the gamma
>>> mode
>>> to a value it doesn't understand.
>>>
>>> ~Maarten
>> Hey Maarten,
>> In that case, what do you suggest should be the right way to do this
>> ?
>>
>> @Ville, any comments here ?
>>
> I would say drop this patch, and just enable segmented gamma
> unconditionally, it's not the first property that can cause trouble
> when not understood.

Honestly, I kindof like this simple approach suggestion, which makes the 
property focused, and easy to use. But one of the benefits I see for 
this new gamma mode property was, that we can handle other advanced 
gamma modes like 10/12/split and multi-segmented gamma modes too, using 
this. So even though the single property is easy to use, but if we try 
to cover each of the gamma modes per single property, it might get 
confusing to see 4 different gamma mode properties, and how to set one 
of this.

Another problem I see is, the precision and no of entries in the LUT, 
for multi-segment gamma is different than traditional gamma, and we 
might break some (most ?) of the old userspaces. Do you think is there 
any way in which we can avoid this ?

- Shashank

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

Re: [v3 6/7] drm: Add Client Cap for advance gamma mode

[Daniel Vetter]

On Mon, Apr 15, 2019 at 4:14 PM Lankhorst, Maarten
<maarten.lankhorst@intel.com> wrote:
>
> mån 2019-04-15 klockan 19:26 +0530 skrev Sharma, Shashank:
> > > -----Original Message-----
> > > From: Lankhorst, Maarten
> > > Sent: Monday, April 15, 2019 4:28 PM
> > > To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedeskt
> > > op.org; dri-
> > > devel@lists.freedesktop.org
> > > Cc: Syrjala, Ville <ville.syrjala@intel.com>; emil.l.velikov@gmail.
> > > com;
> > > sam@ravnborg.org; Roper, Matthew D <matthew.d.roper@intel.com>;
> > > seanpaul@chromium.org; brian.starkey@arm.com; dcastagna@chromium.or
> > > g;
> > > Sharma, Shashank <shashank.sharma@intel.com>
> > > Subject: Re: [v3 6/7] drm: Add Client Cap for advance gamma mode
> > >
> > > fre 2019-04-12 klockan 15:51 +0530 skrev Uma Shankar:
> > > > Introduced a client cap for advance cap mode
> > > > capability. Userspace should set this to get
> > > > to be able to use the new gamma_mode property.
> > > >
> > > > If this is not set, driver will work in legacy
> > > > mode.
> > > >
> > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> > >
> > > Nack, this doesn't seem like a sensible idea. We already guard it
> > > behind the gamma mode property. Userspace shouldn't set the gamma
> > > mode
> > > to a value it doesn't understand.
> > >
> > > ~Maarten
> >
> > Hey Maarten,
> > In that case, what do you suggest should be the right way to do this
> > ?
> >
> > @Ville, any comments here ?
> >
> I would say drop this patch, and just enable segmented gamma
> unconditionally, it's not the first property that can cause trouble
> when not understood.

Yeah, thus far we went with "new properties should have the old
behaviour as default, no cap/flag needed". If you mix old&new
userspace and stuff starts looking funny, that's not a regression imo.
Also, it's a very uncommon use-case.

Wrt reset to default: fbdev emulation should do that for anything
that's too fancy, which is generally enough for the "developer of
compositors" use case. That part might be missing in the gamma/ctm
support in general I think.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [v3 6/7] drm: Add Client Cap for advance gamma mode

[Ville Syrjälä]

On Mon, Apr 15, 2019 at 09:20:55PM +0200, Daniel Vetter wrote:
> On Mon, Apr 15, 2019 at 4:14 PM Lankhorst, Maarten
> <maarten.lankhorst@intel.com> wrote:
> >
> > mån 2019-04-15 klockan 19:26 +0530 skrev Sharma, Shashank:
> > > > -----Original Message-----
> > > > From: Lankhorst, Maarten
> > > > Sent: Monday, April 15, 2019 4:28 PM
> > > > To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedeskt
> > > > op.org; dri-
> > > > devel@lists.freedesktop.org
> > > > Cc: Syrjala, Ville <ville.syrjala@intel.com>; emil.l.velikov@gmail.
> > > > com;
> > > > sam@ravnborg.org; Roper, Matthew D <matthew.d.roper@intel.com>;
> > > > seanpaul@chromium.org; brian.starkey@arm.com; dcastagna@chromium.or
> > > > g;
> > > > Sharma, Shashank <shashank.sharma@intel.com>
> > > > Subject: Re: [v3 6/7] drm: Add Client Cap for advance gamma mode
> > > >
> > > > fre 2019-04-12 klockan 15:51 +0530 skrev Uma Shankar:
> > > > > Introduced a client cap for advance cap mode
> > > > > capability. Userspace should set this to get
> > > > > to be able to use the new gamma_mode property.
> > > > >
> > > > > If this is not set, driver will work in legacy
> > > > > mode.
> > > > >
> > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> > > >
> > > > Nack, this doesn't seem like a sensible idea. We already guard it
> > > > behind the gamma mode property. Userspace shouldn't set the gamma
> > > > mode
> > > > to a value it doesn't understand.
> > > >
> > > > ~Maarten
> > >
> > > Hey Maarten,
> > > In that case, what do you suggest should be the right way to do this
> > > ?
> > >
> > > @Ville, any comments here ?
> > >
> > I would say drop this patch, and just enable segmented gamma
> > unconditionally, it's not the first property that can cause trouble
> > when not understood.
> 
> Yeah, thus far we went with "new properties should have the old
> behaviour as default, no cap/flag needed".

That's a given. But it doesn't help with the mixed userspace when one of
them doesn't know how to reset it back to default.

> If you mix old&new
> userspace and stuff starts looking funny, that's not a regression imo.
> Also, it's a very uncommon use-case.

I was mainly thinking of eg. wayland vs. X type of cases where the user
might want to switch. If the wayland compositor uses the new fancy gamma
thing but the xserver doesn't then this won't work anymore. But I guess
easier solution is to just add the relevant knowledge to the ddx. Which
should probably be a hard requirement for landing this.

> 
> Wrt reset to default: fbdev emulation should do that for anything
> that's too fancy, which is generally enough for the "developer of
> compositors" use case. That part might be missing in the gamma/ctm
> support in general I think.

Probably. IIRC even running X at 8bpp and then swithcing back to
fbcon leaves you with a gargbled palette.

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

[v3 7/7] drm/i915: Enable advance gamma mode

[Uma Shankar]

Enable advance gamma modes based on client
caps.

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

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index edf5ff8..36604c16 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -926,8 +926,9 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
 	if ((crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) ==
 	    GAMMA_MODE_MODE_8BIT) {
 		i9xx_load_luts(crtc_state);
-	} else if (crtc_state->base.gamma_mode_type ==
-		   MULTI_SEGMENTED_GAMMA_MODE_12BIT) {
+	} else if ((crtc_state->base.gamma_mode_type ==
+		   MULTI_SEGMENTED_GAMMA_MODE_12BIT) &&
+		  crtc_state->base.advance_gamma_mode_active) {
 		icl_load_gamma_multi_segmented_lut(crtc_state, 0);
 	} else {
 		bdw_load_lut_10(crtc, gamma_lut, PAL_PREC_INDEX_VALUE(0));
@@ -1352,12 +1353,13 @@ static u32 icl_gamma_mode(struct intel_crtc_state *crtc_state)
 	    !crtc_state->c8_planes)
 		gamma_mode |= POST_CSC_GAMMA_ENABLE;
 
-	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)
+	if (crtc_state->base.gamma_mode_type ==
+		MULTI_SEGMENTED_GAMMA_MODE_12BIT &&
+	     crtc_state->base.advance_gamma_mode_active)
 		gamma_mode |= GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED;
+	else if (!crtc_state->base.gamma_lut ||
+		 crtc_state_is_legacy_gamma(crtc_state))
+		gamma_mode |= GAMMA_MODE_MODE_8BIT;
 	else
 		gamma_mode |= GAMMA_MODE_MODE_10BIT;
 
-- 
1.9.1

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

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

[Daniel Vetter]

On Fri, Apr 12, 2019 at 03:50:56PM +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 GAMMA_MODE property interface. This is an enum
> property with values as blob_id's and exposes
> the various gamma modes supported and the lut ranges  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. It can then set one of the modes using this
> enum property.
> 
> Lut values will be sent through already available GAMMA_LUT
> blob property.
> 
> It also introduces a CLIENT CAP for advanced GAMMA_MODE.
>  This is for user to set the and use advance gamma mode and older
> userspace can continue using the legacy paths.
> 
> v2: Used Ville's design and approach to define the interfaces.
> Addressed Matt Roper's review feedback and re-ordered the
> patches.
> 
> v3: Converged to 1 property interface and introduced a Client cap
> as suggested by Ville. Fixed review comments received.
> 
> Uma Shankar (5):
>   drm/i915/icl: Add register definitions for Multi Segmented gamma
>   drm/i915/icl: Add support for multi segmented gamma mode
>   drm/i915: Attach gamma mode property
>   drm: Add Client Cap for advance gamma mode
>   drm/i915: Enable advance gamma mode
> 
> Ville Syrjälä (2):
>   drm: Add gamma mode property
>   drm/i915: Define color lut range structure

Bunch of higher level comments after some internal discussions:

- we need the userspace for this, can't design new uapi without involving
  the compositor folks for hdr.

- single property doesn't work: Once userspace has set it, the old blob
  property with the list of all options is gone. We need one read-only
  property for the list of options, plus a 2nd property that userspace can
  set. This is a general rule for more complex properties, where the usual
  property metadata isn't enough to describe the possible options.

- no caps for properties. Yes that gives us a theoretical problem, no in
  practice it doesn't matter, since people don't even care enough to make
  e.g. fbdev resetting work today for everything. Long form discussion,
  see here:

  https://blog.ffwll.ch/2016/01/vt-switching-with-atomic-modeset.html

  Nothing happened in this area ever since I typed this up, so I guess
  it's really not a real-world concern.

- Simplest path forward would be if we accept different LUT sizes than the
  one advertised (we already do that for legacy gamma, and this is
  officially what we had in mind too), and the kernel automatically picks
  the best lut configuration. Will be somewhat awkard for the
  multi-segment lut, but would decouple the uapi discussion a bit.

- Frankly the uapi proposed looks like fake generic - it tries to model
  all possibilities in a generic way, when really userspace needs to have
  special code for special pipelines. To me this feels like the pixel
  modifier discussion all over, where we had multi-year discussions on
  trying to describe everything in generic terms or just have fairly
  opaque enumeration of special cases. Both approaches have been tried.
  For this I'm leaning towards the opaque color pipeline description for
  the more fancy stuff.

  Either way, settling on the right uapi will take some time, and will
  need a pile of people to be involved.

Cheers, Daniel

> 
>  drivers/gpu/drm/drm_atomic_uapi.c    |   8 +
>  drivers/gpu/drm/drm_color_mgmt.c     |  77 ++++
>  drivers/gpu/drm/drm_ioctl.c          |   5 +
>  drivers/gpu/drm/i915/i915_reg.h      |  17 +
>  drivers/gpu/drm/i915/intel_color.c   | 735 ++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_display.c |   3 +
>  include/drm/drm_atomic.h             |   1 +
>  include/drm/drm_color_mgmt.h         |   8 +
>  include/drm/drm_crtc.h               |  17 +
>  include/drm/drm_file.h               |   8 +
>  include/drm/drm_mode_config.h        |   6 +
>  include/uapi/drm/drm.h               |   2 +
>  include/uapi/drm/drm_mode.h          |  38 ++
>  13 files changed, 918 insertions(+), 7 deletions(-)
> 
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

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

[Ville Syrjälä]

On Wed, Apr 17, 2019 at 09:28:19AM +0200, Daniel Vetter wrote:
> On Fri, Apr 12, 2019 at 03:50:56PM +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 GAMMA_MODE property interface. This is an enum
> > property with values as blob_id's and exposes
> > the various gamma modes supported and the lut ranges  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. It can then set one of the modes using this
> > enum property.
> > 
> > Lut values will be sent through already available GAMMA_LUT
> > blob property.
> > 
> > It also introduces a CLIENT CAP for advanced GAMMA_MODE.
> >  This is for user to set the and use advance gamma mode and older
> > userspace can continue using the legacy paths.
> > 
> > v2: Used Ville's design and approach to define the interfaces.
> > Addressed Matt Roper's review feedback and re-ordered the
> > patches.
> > 
> > v3: Converged to 1 property interface and introduced a Client cap
> > as suggested by Ville. Fixed review comments received.
> > 
> > Uma Shankar (5):
> >   drm/i915/icl: Add register definitions for Multi Segmented gamma
> >   drm/i915/icl: Add support for multi segmented gamma mode
> >   drm/i915: Attach gamma mode property
> >   drm: Add Client Cap for advance gamma mode
> >   drm/i915: Enable advance gamma mode
> > 
> > Ville Syrjälä (2):
> >   drm: Add gamma mode property
> >   drm/i915: Define color lut range structure
> 
> Bunch of higher level comments after some internal discussions:
> 
> - we need the userspace for this, can't design new uapi without involving
>   the compositor folks for hdr.
> 
> - single property doesn't work: Once userspace has set it, the old blob
>   property with the list of all options is gone. We need one read-only
>   property for the list of options, plus a 2nd property that userspace can
>   set. This is a general rule for more complex properties, where the usual
>   property metadata isn't enough to describe the possible options.

I guess no one understood my blob_enum idea? It's an enum where each
possible value is a blob. The only thing that changes is the current
value (which can only point to one of the enumerated blobs).

> 
> - no caps for properties. Yes that gives us a theoretical problem, no in
>   practice it doesn't matter, since people don't even care enough to make
>   e.g. fbdev resetting work today for everything. Long form discussion,
>   see here:
> 
>   https://blog.ffwll.ch/2016/01/vt-switching-with-atomic-modeset.html
> 
>   Nothing happened in this area ever since I typed this up, so I guess
>   it's really not a real-world concern.
> 
> - Simplest path forward would be if we accept different LUT sizes than the
>   one advertised (we already do that for legacy gamma, and this is
>   officially what we had in mind too), and the kernel automatically picks
>   the best lut configuration. Will be somewhat awkard for the
>   multi-segment lut, but would decouple the uapi discussion a bit.

It'll be ridiculously wasteful. IIRC we need a LUT with 32768 entries,
and then ~98% of those gets thrown away and never programmed to the
hardware.

> 
> - Frankly the uapi proposed looks like fake generic - it tries to model
>   all possibilities in a generic way, when really userspace needs to have
>   special code for special pipelines.

I think it can be used pretty easily. Userspace just has to decide
whether it wants a straight up LUT or whether an interpolated curve
is enough, and how much precision it needs. For x11 the logic would
be simple enough: 1. look for straight up LUT with num_entries >= 1<<bpc,
if that isn't found fall back to an interpolated curve with >= 1<<bpc
precision, and finally just fall back to whatever gives the best
results I suppose.

> To me this feels like the pixel
>   modifier discussion all over, where we had multi-year discussions on
>   trying to describe everything in generic terms or just have fairly
>   opaque enumeration of special cases. Both approaches have been tried.
>   For this I'm leaning towards the opaque color pipeline description for
>   the more fancy stuff.
> 
>   Either way, settling on the right uapi will take some time, and will
>   need a pile of people to be involved.
> 
> Cheers, Daniel
> 
> > 
> >  drivers/gpu/drm/drm_atomic_uapi.c    |   8 +
> >  drivers/gpu/drm/drm_color_mgmt.c     |  77 ++++
> >  drivers/gpu/drm/drm_ioctl.c          |   5 +
> >  drivers/gpu/drm/i915/i915_reg.h      |  17 +
> >  drivers/gpu/drm/i915/intel_color.c   | 735 ++++++++++++++++++++++++++++++++++-
> >  drivers/gpu/drm/i915/intel_display.c |   3 +
> >  include/drm/drm_atomic.h             |   1 +
> >  include/drm/drm_color_mgmt.h         |   8 +
> >  include/drm/drm_crtc.h               |  17 +
> >  include/drm/drm_file.h               |   8 +
> >  include/drm/drm_mode_config.h        |   6 +
> >  include/uapi/drm/drm.h               |   2 +
> >  include/uapi/drm/drm_mode.h          |  38 ++
> >  13 files changed, 918 insertions(+), 7 deletions(-)
> > 
> > -- 
> > 1.9.1
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

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

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

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

[Daniel Vetter]

On Wed, Apr 17, 2019 at 02:57:31PM +0300, Ville Syrjälä wrote:
> On Wed, Apr 17, 2019 at 09:28:19AM +0200, Daniel Vetter wrote:
> > On Fri, Apr 12, 2019 at 03:50:56PM +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 GAMMA_MODE property interface. This is an enum
> > > property with values as blob_id's and exposes
> > > the various gamma modes supported and the lut ranges  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. It can then set one of the modes using this
> > > enum property.
> > >
> > > Lut values will be sent through already available GAMMA_LUT
> > > blob property.
> > >
> > > It also introduces a CLIENT CAP for advanced GAMMA_MODE.
> > >  This is for user to set the and use advance gamma mode and older
> > > userspace can continue using the legacy paths.
> > >
> > > v2: Used Ville's design and approach to define the interfaces.
> > > Addressed Matt Roper's review feedback and re-ordered the
> > > patches.
> > >
> > > v3: Converged to 1 property interface and introduced a Client cap
> > > as suggested by Ville. Fixed review comments received.
> > >
> > > Uma Shankar (5):
> > >   drm/i915/icl: Add register definitions for Multi Segmented gamma
> > >   drm/i915/icl: Add support for multi segmented gamma mode
> > >   drm/i915: Attach gamma mode property
> > >   drm: Add Client Cap for advance gamma mode
> > >   drm/i915: Enable advance gamma mode
> > >
> > > Ville Syrjälä (2):
> > >   drm: Add gamma mode property
> > >   drm/i915: Define color lut range structure
> >
> > Bunch of higher level comments after some internal discussions:
> >
> > - we need the userspace for this, can't design new uapi without involving
> >   the compositor folks for hdr.
> >
> > - single property doesn't work: Once userspace has set it, the old blob
> >   property with the list of all options is gone. We need one read-only
> >   property for the list of options, plus a 2nd property that userspace can
> >   set. This is a general rule for more complex properties, where the usual
> >   property metadata isn't enough to describe the possible options.
>
> I guess no one understood my blob_enum idea? It's an enum where each
> possible value is a blob. The only thing that changes is the current
> value (which can only point to one of the enumerated blobs).

Uh yes that's not clear at all, and if we do go with this, I guess we
should have a pile of core code to make sure it validates and is
consistent.

>> > - no caps for properties. Yes that gives us a theoretical problem, no in
> >   practice it doesn't matter, since people don't even care enough to make
> >   e.g. fbdev resetting work today for everything. Long form discussion,
> >   see here:
> >
> >   https://blog.ffwll.ch/2016/01/vt-switching-with-atomic-modeset.html
> >
> >   Nothing happened in this area ever since I typed this up, so I guess
> >   it's really not a real-world concern.
> >
> > - Simplest path forward would be if we accept different LUT sizes than the
> >   one advertised (we already do that for legacy gamma, and this is
> >   officially what we had in mind too), and the kernel automatically picks
> >   the best lut configuration. Will be somewhat awkard for the
> >   multi-segment lut, but would decouple the uapi discussion a bit.
>
> It'll be ridiculously wasteful. IIRC we need a LUT with 32768 entries,
> and then ~98% of those gets thrown away and never programmed to the
> hardware.

Yeah it's a few MB, not that awesome really ...

> > - Frankly the uapi proposed looks like fake generic - it tries to model
> >   all possibilities in a generic way, when really userspace needs to have
> >   special code for special pipelines.
>
> I think it can be used pretty easily. Userspace just has to decide
> whether it wants a straight up LUT or whether an interpolated curve
> is enough, and how much precision it needs. For x11 the logic would
> be simple enough: 1. look for straight up LUT with num_entries >= 1<<bpc,
> if that isn't found fall back to an interpolated curve with >= 1<<bpc
> precision, and finally just fall back to whatever gives the best
> results I suppose.

Hm, there's also a bunch more defines about mirroring and non-negative and
other stuff that I have no idea how userspace should use it. I do think
some "here's the possible configs for color management" thing is needed,
I'm not sure userspace can do much with all the details provided in the
current series.
-Daniel

-Daniel

> > To me this feels like the pixel
> >   modifier discussion all over, where we had multi-year discussions on
> >   trying to describe everything in generic terms or just have fairly
> >   opaque enumeration of special cases. Both approaches have been tried.
> >   For this I'm leaning towards the opaque color pipeline description for
> >   the more fancy stuff.
> >
> >   Either way, settling on the right uapi will take some time, and will
> >   need a pile of people to be involved.
> >
> > Cheers, Daniel
> >
> > >
> > >  drivers/gpu/drm/drm_atomic_uapi.c    |   8 +
> > >  drivers/gpu/drm/drm_color_mgmt.c     |  77 ++++
> > >  drivers/gpu/drm/drm_ioctl.c          |   5 +
> > >  drivers/gpu/drm/i915/i915_reg.h      |  17 +
> > >  drivers/gpu/drm/i915/intel_color.c   | 735 ++++++++++++++++++++++++++++++++++-
> > >  drivers/gpu/drm/i915/intel_display.c |   3 +
> > >  include/drm/drm_atomic.h             |   1 +
> > >  include/drm/drm_color_mgmt.h         |   8 +
> > >  include/drm/drm_crtc.h               |  17 +
> > >  include/drm/drm_file.h               |   8 +
> > >  include/drm/drm_mode_config.h        |   6 +
> > >  include/uapi/drm/drm.h               |   2 +
> > >  include/uapi/drm/drm_mode.h          |  38 ++
> > >  13 files changed, 918 insertions(+), 7 deletions(-)
> > >
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>
> --
> Ville Syrjälä
> Intel
> ---------------------------------------------------------------------
> Intel Finland Oy
> Registered Address: PL 281, 00181 Helsinki
> Business Identity Code: 0357606 - 4
> Domiciled in Helsinki
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
>

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

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

[Ville Syrjälä]

On Thu, Apr 18, 2019 at 09:13:04AM +0200, Daniel Vetter wrote:
> On Wed, Apr 17, 2019 at 02:57:31PM +0300, Ville Syrjälä wrote:
> > On Wed, Apr 17, 2019 at 09:28:19AM +0200, Daniel Vetter wrote:
> > > On Fri, Apr 12, 2019 at 03:50:56PM +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 GAMMA_MODE property interface. This is an enum
> > > > property with values as blob_id's and exposes
> > > > the various gamma modes supported and the lut ranges  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. It can then set one of the modes using this
> > > > enum property.
> > > >
> > > > Lut values will be sent through already available GAMMA_LUT
> > > > blob property.
> > > >
> > > > It also introduces a CLIENT CAP for advanced GAMMA_MODE.
> > > >  This is for user to set the and use advance gamma mode and older
> > > > userspace can continue using the legacy paths.
> > > >
> > > > v2: Used Ville's design and approach to define the interfaces.
> > > > Addressed Matt Roper's review feedback and re-ordered the
> > > > patches.
> > > >
> > > > v3: Converged to 1 property interface and introduced a Client cap
> > > > as suggested by Ville. Fixed review comments received.
> > > >
> > > > Uma Shankar (5):
> > > >   drm/i915/icl: Add register definitions for Multi Segmented gamma
> > > >   drm/i915/icl: Add support for multi segmented gamma mode
> > > >   drm/i915: Attach gamma mode property
> > > >   drm: Add Client Cap for advance gamma mode
> > > >   drm/i915: Enable advance gamma mode
> > > >
> > > > Ville Syrjälä (2):
> > > >   drm: Add gamma mode property
> > > >   drm/i915: Define color lut range structure
> > >
> > > Bunch of higher level comments after some internal discussions:
> > >
> > > - we need the userspace for this, can't design new uapi without involving
> > >   the compositor folks for hdr.
> > >
> > > - single property doesn't work: Once userspace has set it, the old blob
> > >   property with the list of all options is gone. We need one read-only
> > >   property for the list of options, plus a 2nd property that userspace can
> > >   set. This is a general rule for more complex properties, where the usual
> > >   property metadata isn't enough to describe the possible options.
> >
> > I guess no one understood my blob_enum idea? It's an enum where each
> > possible value is a blob. The only thing that changes is the current
> > value (which can only point to one of the enumerated blobs).
> 
> Uh yes that's not clear at all, and if we do go with this, I guess we
> should have a pile of core code to make sure it validates and is
> consistent.
> 
> >> > - no caps for properties. Yes that gives us a theoretical problem, no in
> > >   practice it doesn't matter, since people don't even care enough to make
> > >   e.g. fbdev resetting work today for everything. Long form discussion,
> > >   see here:
> > >
> > >   https://blog.ffwll.ch/2016/01/vt-switching-with-atomic-modeset.html
> > >
> > >   Nothing happened in this area ever since I typed this up, so I guess
> > >   it's really not a real-world concern.
> > >
> > > - Simplest path forward would be if we accept different LUT sizes than the
> > >   one advertised (we already do that for legacy gamma, and this is
> > >   officially what we had in mind too), and the kernel automatically picks
> > >   the best lut configuration. Will be somewhat awkard for the
> > >   multi-segment lut, but would decouple the uapi discussion a bit.
> >
> > It'll be ridiculously wasteful. IIRC we need a LUT with 32768 entries,
> > and then ~98% of those gets thrown away and never programmed to the
> > hardware.
> 
> Yeah it's a few MB, not that awesome really ...
> 
> > > - Frankly the uapi proposed looks like fake generic - it tries to model
> > >   all possibilities in a generic way, when really userspace needs to have
> > >   special code for special pipelines.
> >
> > I think it can be used pretty easily. Userspace just has to decide
> > whether it wants a straight up LUT or whether an interpolated curve
> > is enough, and how much precision it needs. For x11 the logic would
> > be simple enough: 1. look for straight up LUT with num_entries >= 1<<bpc,
> > if that isn't found fall back to an interpolated curve with >= 1<<bpc
> > precision, and finally just fall back to whatever gives the best
> > results I suppose.
> 
> Hm, there's also a bunch more defines about mirroring and non-negative and
> other stuff that I have no idea how userspace should use it. I do think
> some "here's the possible configs for color management" thing is needed,
> I'm not sure userspace can do much with all the details provided in the
> current series.

The negative values would represent out of gamut colors, which
can definitely happen with xvYCC. Looks like the v4l folks
also considered this in their transfer func docs [1], which
are specifying the formulas extended for <0.0 values.

Also based on my chat with Ilia on irc at some point I got the
impression that nv hardware may have gamma LUTs which support
negative values without this mirroring trick. Hence I wanted to
make all the numbers signed rather than unsigned.

We could of course leave a bunch of these advanced things 
undefined until an actual use case comes around. But I wanted to
include it all in my initial proposal so that we could be more
confident that we're not painting ourselves in a corner that
would require yet another uapi to escape.

[1] https://www.linuxtv.org/downloads/v4l-dvb-apis-old/ch02s06.html

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

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

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

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

[Daniel Vetter]

On Thu, Apr 18, 2019 at 04:11:58PM +0300, Ville Syrjälä wrote:
> On Thu, Apr 18, 2019 at 09:13:04AM +0200, Daniel Vetter wrote:
> > On Wed, Apr 17, 2019 at 02:57:31PM +0300, Ville Syrjälä wrote:
> > > On Wed, Apr 17, 2019 at 09:28:19AM +0200, Daniel Vetter wrote:
> > > > On Fri, Apr 12, 2019 at 03:50:56PM +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 GAMMA_MODE property interface. This is an enum
> > > > > property with values as blob_id's and exposes
> > > > > the various gamma modes supported and the lut ranges  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. It can then set one of the modes using this
> > > > > enum property.
> > > > >
> > > > > Lut values will be sent through already available GAMMA_LUT
> > > > > blob property.
> > > > >
> > > > > It also introduces a CLIENT CAP for advanced GAMMA_MODE.
> > > > >  This is for user to set the and use advance gamma mode and older
> > > > > userspace can continue using the legacy paths.
> > > > >
> > > > > v2: Used Ville's design and approach to define the interfaces.
> > > > > Addressed Matt Roper's review feedback and re-ordered the
> > > > > patches.
> > > > >
> > > > > v3: Converged to 1 property interface and introduced a Client cap
> > > > > as suggested by Ville. Fixed review comments received.
> > > > >
> > > > > Uma Shankar (5):
> > > > >   drm/i915/icl: Add register definitions for Multi Segmented gamma
> > > > >   drm/i915/icl: Add support for multi segmented gamma mode
> > > > >   drm/i915: Attach gamma mode property
> > > > >   drm: Add Client Cap for advance gamma mode
> > > > >   drm/i915: Enable advance gamma mode
> > > > >
> > > > > Ville Syrjälä (2):
> > > > >   drm: Add gamma mode property
> > > > >   drm/i915: Define color lut range structure
> > > >
> > > > Bunch of higher level comments after some internal discussions:
> > > >
> > > > - we need the userspace for this, can't design new uapi without involving
> > > >   the compositor folks for hdr.
> > > >
> > > > - single property doesn't work: Once userspace has set it, the old blob
> > > >   property with the list of all options is gone. We need one read-only
> > > >   property for the list of options, plus a 2nd property that userspace can
> > > >   set. This is a general rule for more complex properties, where the usual
> > > >   property metadata isn't enough to describe the possible options.
> > >
> > > I guess no one understood my blob_enum idea? It's an enum where each
> > > possible value is a blob. The only thing that changes is the current
> > > value (which can only point to one of the enumerated blobs).
> > 
> > Uh yes that's not clear at all, and if we do go with this, I guess we
> > should have a pile of core code to make sure it validates and is
> > consistent.
> > 
> > >> > - no caps for properties. Yes that gives us a theoretical problem, no in
> > > >   practice it doesn't matter, since people don't even care enough to make
> > > >   e.g. fbdev resetting work today for everything. Long form discussion,
> > > >   see here:
> > > >
> > > >   https://blog.ffwll.ch/2016/01/vt-switching-with-atomic-modeset.html
> > > >
> > > >   Nothing happened in this area ever since I typed this up, so I guess
> > > >   it's really not a real-world concern.
> > > >
> > > > - Simplest path forward would be if we accept different LUT sizes than the
> > > >   one advertised (we already do that for legacy gamma, and this is
> > > >   officially what we had in mind too), and the kernel automatically picks
> > > >   the best lut configuration. Will be somewhat awkard for the
> > > >   multi-segment lut, but would decouple the uapi discussion a bit.
> > >
> > > It'll be ridiculously wasteful. IIRC we need a LUT with 32768 entries,
> > > and then ~98% of those gets thrown away and never programmed to the
> > > hardware.
> > 
> > Yeah it's a few MB, not that awesome really ...
> > 
> > > > - Frankly the uapi proposed looks like fake generic - it tries to model
> > > >   all possibilities in a generic way, when really userspace needs to have
> > > >   special code for special pipelines.
> > >
> > > I think it can be used pretty easily. Userspace just has to decide
> > > whether it wants a straight up LUT or whether an interpolated curve
> > > is enough, and how much precision it needs. For x11 the logic would
> > > be simple enough: 1. look for straight up LUT with num_entries >= 1<<bpc,
> > > if that isn't found fall back to an interpolated curve with >= 1<<bpc
> > > precision, and finally just fall back to whatever gives the best
> > > results I suppose.
> > 
> > Hm, there's also a bunch more defines about mirroring and non-negative and
> > other stuff that I have no idea how userspace should use it. I do think
> > some "here's the possible configs for color management" thing is needed,
> > I'm not sure userspace can do much with all the details provided in the
> > current series.
> 
> The negative values would represent out of gamut colors, which
> can definitely happen with xvYCC. Looks like the v4l folks
> also considered this in their transfer func docs [1], which
> are specifying the formulas extended for <0.0 values.
> 
> Also based on my chat with Ilia on irc at some point I got the
> impression that nv hardware may have gamma LUTs which support
> negative values without this mirroring trick. Hence I wanted to
> make all the numbers signed rather than unsigned.
> 
> We could of course leave a bunch of these advanced things 
> undefined until an actual use case comes around. But I wanted to
> include it all in my initial proposal so that we could be more
> confident that we're not painting ourselves in a corner that
> would require yet another uapi to escape.

Hm good point. I just feel like with just one driver (at least in kms) it
might be a bit early to go all in on color manager v2.0. From a kms
ecosystem pov we're still pretty busy rolling out the first one. But if we
need it now, I guess we'll need it now ...

> [1] https://www.linuxtv.org/downloads/v4l-dvb-apis-old/ch02s06.html
> 
> -- 
> Ville Syrjälä
> Intel
> ---------------------------------------------------------------------
> Intel Finland Oy
> Registered Address: PL 281, 00181 Helsinki 
> Business Identity Code: 0357606 - 4 
> Domiciled in Helsinki 
> 
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.

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