[PATCH 00/18] Color Management for DRM

[PATCH 00/18] Color Management for DRM

[Shashank Sharma]

This patch set adds Color Manager implementation in DRM layer. Color Manager is
an extension in DRM framework to support color correction/enhancement. Various
Hardware platforms can support several color correction capabilities.
Color Manager provides abstraction of these capabilities and allows a
user space UI agent to correct/enhance the display using the DRM property interface.

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

That's all! :)

About the patch series:
=======================
The first patch adds fix for ensuring atomic commit for CRTC properties.
The subsequent patches add code for the framework, which will be common across all the Hardware platforms.
1. Create Color Management DRM properties
2. Attach color properties to CRTC
3. Add structures at DRM level for color management

The generic properties supported in this patch set are
1. Color Transformation Matrix (CTM) for generic usecases like color space conversion and Gamut Mapping
2. Palette correction before CTM for specific usecases like DeGamma color correction
3. Palette correction after CTM for specific usecases like Gamma color correction

In the subsequent patches, we are adding support for Gamma, DeGamma and CSC color properties for one of the Intel platforms.

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

V2: Worked on review comments from Matt, Jim, Thierry, Rob.

Shashank Sharma (18):
  drm: Create Color Management DRM properties
  drm/i915: Add atomic set property interface for CRTC
  drm/i915: Add atomic get property interface for CRTC
  drm: Add structure for querying palette color capabilities
  drm/i915: Initialize color manager and add gamma correction
  drm: Add color correction blobs in CRTC state
  drm: Add drm structures for palette color property
  drm/i915: Add pipe gamma correction handlers
  drm/i915: Pipe level Gamma correction for CHV/BSW
  drm/i915: Add pipe deGamma correction handlers
  drm/i915: Add DeGamma correction for CHV/BSW
  drm: Add structure for set/get a CTM color property
  drm/i915: Add set/get property handlers for CSC correction
  drm/i915: Add CSC correction for CHV/BSW
  drm/i915: Initialize Gen8 pipe gamma correction
  drm/i915: Gen8 pipe level Gamma correction
  drm/i915: Add DeGamma correction for BDW/SKL/BXT
  drm/i915: Add CSC correction for BDW/SKL/BXT

 drivers/gpu/drm/drm_crtc.c                 |  26 +
 drivers/gpu/drm/i915/Makefile              |   3 +-
 drivers/gpu/drm/i915/i915_reg.h            |  44 +-
 drivers/gpu/drm/i915/intel_atomic.c        |  43 ++
 drivers/gpu/drm/i915/intel_color_manager.c | 970 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_color_manager.h | 117 ++++
 drivers/gpu/drm/i915/intel_display.c       |   7 +
 drivers/gpu/drm/i915/intel_drv.h           |  23 +
 include/drm/drm_crtc.h                     |  11 +
 include/uapi/drm/drm.h                     |  50 ++
 10 files changed, 1292 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_color_manager.c
 create mode 100644 drivers/gpu/drm/i915/intel_color_manager.h

-- 
1.9.1

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

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

[Shashank Sharma]

From: Kausal Malladi <kausalmalladi@gmail.com>

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

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

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

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

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

[PATCH 02/18] drm/i915: Add atomic set property interface for CRTC

[Shashank Sharma]

From: Kausal Malladi <kausalmalladi@gmail.com>

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

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

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index e2531cf..922fecf 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -318,3 +318,12 @@ void intel_atomic_state_clear(struct drm_atomic_state *s)
 	drm_atomic_state_default_clear(&state->base);
 	state->dpll_set = false;
 }
+
+int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
+				   struct drm_crtc_state *state,
+				   struct drm_property *property,
+				   uint64_t val)
+{
+	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
+	return -EINVAL;
+}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 43b0f17..1fbf0ff 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13351,6 +13351,8 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
 	.set_config = drm_atomic_helper_set_config,
 	.destroy = intel_crtc_destroy,
 	.page_flip = intel_crtc_page_flip,
+	.set_property = drm_atomic_helper_crtc_set_property,
+	.atomic_set_property = intel_crtc_atomic_set_property,
 	.atomic_duplicate_state = intel_crtc_duplicate_state,
 	.atomic_destroy_state = intel_crtc_destroy_state,
 };
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 320c9e6..c0ae529 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1422,6 +1422,10 @@ intel_atomic_get_crtc_state(struct drm_atomic_state *state,
 int intel_atomic_setup_scalers(struct drm_device *dev,
 	struct intel_crtc *intel_crtc,
 	struct intel_crtc_state *crtc_state);
+int intel_crtc_atomic_set_property(struct drm_crtc *plane,
+				   struct drm_crtc_state *state,
+				   struct drm_property *property,
+				   uint64_t val);
 
 /* intel_atomic_plane.c */
 struct intel_plane_state *intel_create_plane_state(struct drm_plane *plane);
-- 
1.9.1

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

[PATCH 03/18] drm/i915: Add atomic get property interface for CRTC

[Shashank Sharma]

From: Kausal Malladi <kausalmalladi@gmail.com>

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

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

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 922fecf..8d04ee8 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -327,3 +327,11 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
 	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
 	return -EINVAL;
 }
+
+int intel_crtc_atomic_get_property(struct drm_crtc *crtc,
+				   const struct drm_crtc_state *state,
+				   struct drm_property *property,
+				   uint64_t *val)
+{
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1fbf0ff..1412e21 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13353,6 +13353,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
 	.page_flip = intel_crtc_page_flip,
 	.set_property = drm_atomic_helper_crtc_set_property,
 	.atomic_set_property = intel_crtc_atomic_set_property,
+	.atomic_get_property = intel_crtc_atomic_get_property,
 	.atomic_duplicate_state = intel_crtc_duplicate_state,
 	.atomic_destroy_state = intel_crtc_destroy_state,
 };
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c0ae529..b3dc138 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1426,6 +1426,10 @@ int intel_crtc_atomic_set_property(struct drm_crtc *plane,
 				   struct drm_crtc_state *state,
 				   struct drm_property *property,
 				   uint64_t val);
+int intel_crtc_atomic_get_property(struct drm_crtc *plane,
+				   const struct drm_crtc_state *state,
+				   struct drm_property *property,
+				   uint64_t *val);
 
 /* intel_atomic_plane.c */
 struct intel_plane_state *intel_create_plane_state(struct drm_plane *plane);
-- 
1.9.1

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

Re: [PATCH 03/18] drm/i915: Add atomic get property interface for CRTC

[Matt Roper]

On Thu, Aug 06, 2015 at 10:08:12PM +0530, Shashank Sharma wrote:
> From: Kausal Malladi <kausalmalladi@gmail.com>
> 
> This patch adds atomic get property interface for Intel CRTC. This
> interface will be used for get operation on any non-core DRM properties.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic.c  | 8 ++++++++
>  drivers/gpu/drm/i915/intel_display.c | 1 +
>  drivers/gpu/drm/i915/intel_drv.h     | 4 ++++
>  3 files changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 922fecf..8d04ee8 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -327,3 +327,11 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
>  	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
>  	return -EINVAL;
>  }
> +
> +int intel_crtc_atomic_get_property(struct drm_crtc *crtc,
> +				   const struct drm_crtc_state *state,
> +				   struct drm_property *property,
> +				   uint64_t *val)
> +{
> +	return 0;

I think this should be -EINVAL since it's an unrecognized property.
Probably add a DRM_DEBUG_KMS() message here like we have in
intel_plane_atomic_get_property() as well.


Matt

> +}
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1fbf0ff..1412e21 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13353,6 +13353,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
>  	.page_flip = intel_crtc_page_flip,
>  	.set_property = drm_atomic_helper_crtc_set_property,
>  	.atomic_set_property = intel_crtc_atomic_set_property,
> +	.atomic_get_property = intel_crtc_atomic_get_property,
>  	.atomic_duplicate_state = intel_crtc_duplicate_state,
>  	.atomic_destroy_state = intel_crtc_destroy_state,
>  };
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index c0ae529..b3dc138 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1426,6 +1426,10 @@ int intel_crtc_atomic_set_property(struct drm_crtc *plane,
>  				   struct drm_crtc_state *state,
>  				   struct drm_property *property,
>  				   uint64_t val);
> +int intel_crtc_atomic_get_property(struct drm_crtc *plane,
> +				   const struct drm_crtc_state *state,
> +				   struct drm_property *property,
> +				   uint64_t *val);
>  
>  /* intel_atomic_plane.c */
>  struct intel_plane_state *intel_create_plane_state(struct drm_plane *plane);
> -- 
> 1.9.1
> 

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

Re: [PATCH 03/18] drm/i915: Add atomic get property interface for CRTC

[Sharma, Shashank]

Thanks for the review Matt.

Regards
Shashank
On 8/22/2015 4:10 AM, Matt Roper wrote:
> On Thu, Aug 06, 2015 at 10:08:12PM +0530, Shashank Sharma wrote:
>> From: Kausal Malladi <kausalmalladi@gmail.com>
>>
>> This patch adds atomic get property interface for Intel CRTC. This
>> interface will be used for get operation on any non-core DRM properties.
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_atomic.c  | 8 ++++++++
>>   drivers/gpu/drm/i915/intel_display.c | 1 +
>>   drivers/gpu/drm/i915/intel_drv.h     | 4 ++++
>>   3 files changed, 13 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
>> index 922fecf..8d04ee8 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -327,3 +327,11 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
>>   	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
>>   	return -EINVAL;
>>   }
>> +
>> +int intel_crtc_atomic_get_property(struct drm_crtc *crtc,
>> +				   const struct drm_crtc_state *state,
>> +				   struct drm_property *property,
>> +				   uint64_t *val)
>> +{
>> +	return 0;
>
> I think this should be -EINVAL since it's an unrecognized property.
> Probably add a DRM_DEBUG_KMS() message here like we have in
> intel_plane_atomic_get_property() as well.
>
>
> Matt
Got it.
>
>> +}
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 1fbf0ff..1412e21 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13353,6 +13353,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
>>   	.page_flip = intel_crtc_page_flip,
>>   	.set_property = drm_atomic_helper_crtc_set_property,
>>   	.atomic_set_property = intel_crtc_atomic_set_property,
>> +	.atomic_get_property = intel_crtc_atomic_get_property,
>>   	.atomic_duplicate_state = intel_crtc_duplicate_state,
>>   	.atomic_destroy_state = intel_crtc_destroy_state,
>>   };
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index c0ae529..b3dc138 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1426,6 +1426,10 @@ int intel_crtc_atomic_set_property(struct drm_crtc *plane,
>>   				   struct drm_crtc_state *state,
>>   				   struct drm_property *property,
>>   				   uint64_t val);
>> +int intel_crtc_atomic_get_property(struct drm_crtc *plane,
>> +				   const struct drm_crtc_state *state,
>> +				   struct drm_property *property,
>> +				   uint64_t *val);
>>
>>   /* intel_atomic_plane.c */
>>   struct intel_plane_state *intel_create_plane_state(struct drm_plane *plane);
>> --
>> 1.9.1
>>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/18] drm/i915: Add atomic get property interface for CRTC

[Daniel Vetter]

On Sat, Aug 22, 2015 at 11:30:44AM +0530, Sharma, Shashank wrote:
> Thanks for the review Matt.
> 
> Regards
> Shashank
> On 8/22/2015 4:10 AM, Matt Roper wrote:
> >On Thu, Aug 06, 2015 at 10:08:12PM +0530, Shashank Sharma wrote:
> >>From: Kausal Malladi <kausalmalladi@gmail.com>
> >>
> >>This patch adds atomic get property interface for Intel CRTC. This
> >>interface will be used for get operation on any non-core DRM properties.
> >>
> >>Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >>Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
> >>---
> >>  drivers/gpu/drm/i915/intel_atomic.c  | 8 ++++++++
> >>  drivers/gpu/drm/i915/intel_display.c | 1 +
> >>  drivers/gpu/drm/i915/intel_drv.h     | 4 ++++
> >>  3 files changed, 13 insertions(+)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> >>index 922fecf..8d04ee8 100644
> >>--- a/drivers/gpu/drm/i915/intel_atomic.c
> >>+++ b/drivers/gpu/drm/i915/intel_atomic.c
> >>@@ -327,3 +327,11 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
> >>  	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
> >>  	return -EINVAL;
> >>  }
> >>+
> >>+int intel_crtc_atomic_get_property(struct drm_crtc *crtc,
> >>+				   const struct drm_crtc_state *state,
> >>+				   struct drm_property *property,
> >>+				   uint64_t *val)
> >>+{
> >>+	return 0;
> >
> >I think this should be -EINVAL since it's an unrecognized property.
> >Probably add a DRM_DEBUG_KMS() message here like we have in
> >intel_plane_atomic_get_property() as well.
> >
> >
> >Matt
> Got it.

Sounds like we need an igt for invalid properties ...
-Daniel

> >
> >>+}
> >>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>index 1fbf0ff..1412e21 100644
> >>--- a/drivers/gpu/drm/i915/intel_display.c
> >>+++ b/drivers/gpu/drm/i915/intel_display.c
> >>@@ -13353,6 +13353,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
> >>  	.page_flip = intel_crtc_page_flip,
> >>  	.set_property = drm_atomic_helper_crtc_set_property,
> >>  	.atomic_set_property = intel_crtc_atomic_set_property,
> >>+	.atomic_get_property = intel_crtc_atomic_get_property,
> >>  	.atomic_duplicate_state = intel_crtc_duplicate_state,
> >>  	.atomic_destroy_state = intel_crtc_destroy_state,
> >>  };
> >>diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >>index c0ae529..b3dc138 100644
> >>--- a/drivers/gpu/drm/i915/intel_drv.h
> >>+++ b/drivers/gpu/drm/i915/intel_drv.h
> >>@@ -1426,6 +1426,10 @@ int intel_crtc_atomic_set_property(struct drm_crtc *plane,
> >>  				   struct drm_crtc_state *state,
> >>  				   struct drm_property *property,
> >>  				   uint64_t val);
> >>+int intel_crtc_atomic_get_property(struct drm_crtc *plane,
> >>+				   const struct drm_crtc_state *state,
> >>+				   struct drm_property *property,
> >>+				   uint64_t *val);
> >>
> >>  /* intel_atomic_plane.c */
> >>  struct intel_plane_state *intel_create_plane_state(struct drm_plane *plane);
> >>--
> >>1.9.1
> >>
> >
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[PATCH 04/18] drm: Add structure for querying palette color capabilities

[Shashank Sharma]

From: Kausal Malladi <kausalmalladi@gmail.com>

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

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

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

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

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

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

[PATCH 05/18] drm/i915: Initialize color manager and add gamma correction

[Shashank Sharma]

From: Kausal Malladi <kausalmalladi@gmail.com>

As per Color Manager design, each driver is responsible to load its
palette color correction and enhancement capabilities in the form of
a DRM blob property, so that user space can query and read.

This patch does the following:
1. Create new files intel_color_manager(.c/.h)
2. Attach CRTC Palette Capabilities property to CRTC
3. Load all CHV platform specific gamma color capabilities
for CRTC into a blob that can be accessible by user space to
query capabilities via DRM property interface.

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

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 41fb8a9..303b903 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -60,7 +60,8 @@ i915-y += intel_audio.o \
 	  intel_overlay.o \
 	  intel_psr.o \
 	  intel_sideband.o \
-	  intel_sprite.o
+	  intel_sprite.o \
+	  intel_color_manager.o
 i915-$(CONFIG_ACPI)		+= intel_acpi.o intel_opregion.o
 i915-$(CONFIG_DRM_I915_FBDEV)	+= intel_fbdev.o
 
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
new file mode 100644
index 0000000..1c9c477
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -0,0 +1,83 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ * Shashank Sharma <shashank.sharma@intel.com>
+ * Kausal Malladi <Kausal.Malladi@intel.com>
+ */
+
+#include "intel_color_manager.h"
+
+int get_chv_pipe_gamma_capabilities(struct drm_device *dev,
+		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
+{
+	struct drm_property_blob *blob;
+
+	/*
+	 * This function exposes best capability for DeGamma and Gamma
+	 * For CHV, the DeGamma LUT has 65 entries
+	 * and the best Gamma capability has 257 entries (CGM unit)
+	 */
+	palette_caps->version = CHV_PALETTE_STRUCT_VERSION;
+	palette_caps->num_samples_before_ctm =
+		CHV_DEGAMMA_MAX_VALS;
+	palette_caps->num_samples_after_ctm =
+		CHV_10BIT_GAMMA_MAX_VALS;
+
+	blob = drm_property_create_blob(dev, sizeof(struct drm_palette_caps),
+			(const void *) palette_caps);
+
+	if (blob)
+		return blob->base.id;
+
+	return 0;
+}
+
+int get_pipe_gamma_capabilities(struct drm_device *dev,
+		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
+{
+	if (IS_CHERRYVIEW(dev))
+		return get_chv_pipe_gamma_capabilities(dev, palette_caps, crtc);
+	return -EINVAL;
+}
+
+void intel_attach_color_properties_to_crtc(struct drm_device *dev,
+		struct drm_mode_object *mode_obj)
+{
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_palette_caps *palette_caps;
+	struct drm_crtc *crtc;
+	int capabilities_blob_id;
+
+	if (IS_CHERRYVIEW(dev)) {
+		crtc = obj_to_crtc(mode_obj);
+
+		palette_caps = kzalloc(sizeof(struct drm_palette_caps),
+				GFP_KERNEL);
+		capabilities_blob_id = get_pipe_gamma_capabilities(dev, palette_caps, crtc);
+		kfree(palette_caps);
+		if (config->cm_crtc_palette_capabilities_property)
+			drm_object_attach_property(mode_obj,
+				config->cm_crtc_palette_capabilities_property,
+				capabilities_blob_id);
+	}
+}
diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
new file mode 100644
index 0000000..51aeb91
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_color_manager.h
@@ -0,0 +1,33 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ * Shashank Sharma <shashank.sharma@intel.com>
+ * Kausal Malladi <Kausal.Malladi@intel.com>
+ */
+#include <drm/drmP.h>
+#include <drm/drm_crtc_helper.h>
+#include "i915_drv.h"
+
+#define CHV_PALETTE_STRUCT_VERSION		1
+#define CHV_DEGAMMA_MAX_VALS			65
+#define CHV_10BIT_GAMMA_MAX_VALS		257
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1412e21..349a1c2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13975,6 +13975,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 
 	intel_crtc->wm.cxsr_allowed = true;
 
+	intel_attach_color_properties_to_crtc(dev, &intel_crtc->base.base);
+
 	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
 	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
 	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index b3dc138..dee5f91 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1438,4 +1438,8 @@ void intel_plane_destroy_state(struct drm_plane *plane,
 			       struct drm_plane_state *state);
 extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
 
+/* intel_color_manager.c */
+void intel_attach_color_properties_to_crtc(struct drm_device *dev,
+		struct drm_mode_object *mode_obj);
+
 #endif /* __INTEL_DRV_H__ */
-- 
1.9.1

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

Re: [PATCH 05/18] drm/i915: Initialize color manager and add gamma correction

[Matt Roper]

On Thu, Aug 06, 2015 at 10:08:14PM +0530, Shashank Sharma wrote:
> From: Kausal Malladi <kausalmalladi@gmail.com>
> 
> As per Color Manager design, each driver is responsible to load its
> palette color correction and enhancement capabilities in the form of
> a DRM blob property, so that user space can query and read.
> 
> This patch does the following:
> 1. Create new files intel_color_manager(.c/.h)
> 2. Attach CRTC Palette Capabilities property to CRTC
> 3. Load all CHV platform specific gamma color capabilities
> for CRTC into a blob that can be accessible by user space to
> query capabilities via DRM property interface.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
> ---
>  drivers/gpu/drm/i915/Makefile              |  3 +-
>  drivers/gpu/drm/i915/intel_color_manager.c | 83 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_color_manager.h | 33 ++++++++++++
>  drivers/gpu/drm/i915/intel_display.c       |  2 +
>  drivers/gpu/drm/i915/intel_drv.h           |  4 ++
>  5 files changed, 124 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_color_manager.c
>  create mode 100644 drivers/gpu/drm/i915/intel_color_manager.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 41fb8a9..303b903 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -60,7 +60,8 @@ i915-y += intel_audio.o \
>  	  intel_overlay.o \
>  	  intel_psr.o \
>  	  intel_sideband.o \
> -	  intel_sprite.o
> +	  intel_sprite.o \
> +	  intel_color_manager.o
>  i915-$(CONFIG_ACPI)		+= intel_acpi.o intel_opregion.o
>  i915-$(CONFIG_DRM_I915_FBDEV)	+= intel_fbdev.o
>  
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
> new file mode 100644
> index 0000000..1c9c477
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -0,0 +1,83 @@
> +/*
> + * Copyright © 2015 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + * Shashank Sharma <shashank.sharma@intel.com>
> + * Kausal Malladi <Kausal.Malladi@intel.com>
> + */
> +
> +#include "intel_color_manager.h"
> +
> +int get_chv_pipe_gamma_capabilities(struct drm_device *dev,
> +		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
> +{
> +	struct drm_property_blob *blob;
> +
> +	/*
> +	 * This function exposes best capability for DeGamma and Gamma
> +	 * For CHV, the DeGamma LUT has 65 entries
> +	 * and the best Gamma capability has 257 entries (CGM unit)
> +	 */
> +	palette_caps->version = CHV_PALETTE_STRUCT_VERSION;
> +	palette_caps->num_samples_before_ctm =
> +		CHV_DEGAMMA_MAX_VALS;
> +	palette_caps->num_samples_after_ctm =
> +		CHV_10BIT_GAMMA_MAX_VALS;
> +
> +	blob = drm_property_create_blob(dev, sizeof(struct drm_palette_caps),
> +			(const void *) palette_caps);
> +
> +	if (blob)
> +		return blob->base.id;

It's a corner case, but blob could be a non-NULL error code here (e.g.,
-ENOMEM).  We should probably check for that before we try to
dereference it.

> +
> +	return 0;
> +}
> +
> +int get_pipe_gamma_capabilities(struct drm_device *dev,
> +		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
> +{
> +	if (IS_CHERRYVIEW(dev))
> +		return get_chv_pipe_gamma_capabilities(dev, palette_caps, crtc);
> +	return -EINVAL;

We only call this function in the IS_CHERRYVIEW case at the moment, so I
realize the EINVAL return is technically dead code, but going forward
would it make more sense to return a valid capabilities blob that
explicitly tells userspace we have no capabilities?

> +}
> +
> +void intel_attach_color_properties_to_crtc(struct drm_device *dev,
> +		struct drm_mode_object *mode_obj)
> +{
> +	struct drm_mode_config *config = &dev->mode_config;
> +	struct drm_palette_caps *palette_caps;
> +	struct drm_crtc *crtc;
> +	int capabilities_blob_id;
> +
> +	if (IS_CHERRYVIEW(dev)) {
> +		crtc = obj_to_crtc(mode_obj);
> +
> +		palette_caps = kzalloc(sizeof(struct drm_palette_caps),
> +				GFP_KERNEL);
> +		capabilities_blob_id = get_pipe_gamma_capabilities(dev, palette_caps, crtc);
> +		kfree(palette_caps);
> +		if (config->cm_crtc_palette_capabilities_property)
> +			drm_object_attach_property(mode_obj,
> +				config->cm_crtc_palette_capabilities_property,
> +				capabilities_blob_id);
> +	}
> +}
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
> new file mode 100644
> index 0000000..51aeb91
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
> @@ -0,0 +1,33 @@
> +/*
> + * Copyright © 2015 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + * Shashank Sharma <shashank.sharma@intel.com>
> + * Kausal Malladi <Kausal.Malladi@intel.com>
> + */
> +#include <drm/drmP.h>
> +#include <drm/drm_crtc_helper.h>
> +#include "i915_drv.h"
> +
> +#define CHV_PALETTE_STRUCT_VERSION		1
> +#define CHV_DEGAMMA_MAX_VALS			65
> +#define CHV_10BIT_GAMMA_MAX_VALS		257
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1412e21..349a1c2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13975,6 +13975,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  
>  	intel_crtc->wm.cxsr_allowed = true;
>  
> +	intel_attach_color_properties_to_crtc(dev, &intel_crtc->base.base);
> +

I feel like we should hold off on actually calling this function until
the very end of your patch series.  If someone is bisecting through
histroy to track down a bug and they land on this commit, we'll be
advertising some capabilities to userspace that you don't really have
yet (since they only show up in the later patches of the series).
If we wait until the end of the series to "flip the switch" and enable
color management, then we don't have to worry about running partially
implemented features during a bisect session.


Matt

>  	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
>  	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
>  	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index b3dc138..dee5f91 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1438,4 +1438,8 @@ void intel_plane_destroy_state(struct drm_plane *plane,
>  			       struct drm_plane_state *state);
>  extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
>  
> +/* intel_color_manager.c */
> +void intel_attach_color_properties_to_crtc(struct drm_device *dev,
> +		struct drm_mode_object *mode_obj);
> +
>  #endif /* __INTEL_DRV_H__ */
> -- 
> 1.9.1
> 

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

Re: [PATCH 05/18] drm/i915: Initialize color manager and add gamma correction

[Sharma, Shashank]

Regards
Shashank

On 8/22/2015 4:10 AM, Matt Roper wrote:
> On Thu, Aug 06, 2015 at 10:08:14PM +0530, Shashank Sharma wrote:
>> From: Kausal Malladi <kausalmalladi@gmail.com>
>>
>> As per Color Manager design, each driver is responsible to load its
>> palette color correction and enhancement capabilities in the form of
>> a DRM blob property, so that user space can query and read.
>>
>> This patch does the following:
>> 1. Create new files intel_color_manager(.c/.h)
>> 2. Attach CRTC Palette Capabilities property to CRTC
>> 3. Load all CHV platform specific gamma color capabilities
>> for CRTC into a blob that can be accessible by user space to
>> query capabilities via DRM property interface.
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/Makefile              |  3 +-
>>   drivers/gpu/drm/i915/intel_color_manager.c | 83 ++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_color_manager.h | 33 ++++++++++++
>>   drivers/gpu/drm/i915/intel_display.c       |  2 +
>>   drivers/gpu/drm/i915/intel_drv.h           |  4 ++
>>   5 files changed, 124 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/gpu/drm/i915/intel_color_manager.c
>>   create mode 100644 drivers/gpu/drm/i915/intel_color_manager.h
>>
>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>> index 41fb8a9..303b903 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -60,7 +60,8 @@ i915-y += intel_audio.o \
>>   	  intel_overlay.o \
>>   	  intel_psr.o \
>>   	  intel_sideband.o \
>> -	  intel_sprite.o
>> +	  intel_sprite.o \
>> +	  intel_color_manager.o
>>   i915-$(CONFIG_ACPI)		+= intel_acpi.o intel_opregion.o
>>   i915-$(CONFIG_DRM_I915_FBDEV)	+= intel_fbdev.o
>>
>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
>> new file mode 100644
>> index 0000000..1c9c477
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
>> @@ -0,0 +1,83 @@
>> +/*
>> + * Copyright © 2015 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the next
>> + * paragraph) shall be included in all copies or substantial portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + *
>> + * Authors:
>> + * Shashank Sharma <shashank.sharma@intel.com>
>> + * Kausal Malladi <Kausal.Malladi@intel.com>
>> + */
>> +
>> +#include "intel_color_manager.h"
>> +
>> +int get_chv_pipe_gamma_capabilities(struct drm_device *dev,
>> +		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
>> +{
>> +	struct drm_property_blob *blob;
>> +
>> +	/*
>> +	 * This function exposes best capability for DeGamma and Gamma
>> +	 * For CHV, the DeGamma LUT has 65 entries
>> +	 * and the best Gamma capability has 257 entries (CGM unit)
>> +	 */
>> +	palette_caps->version = CHV_PALETTE_STRUCT_VERSION;
>> +	palette_caps->num_samples_before_ctm =
>> +		CHV_DEGAMMA_MAX_VALS;
>> +	palette_caps->num_samples_after_ctm =
>> +		CHV_10BIT_GAMMA_MAX_VALS;
>> +
>> +	blob = drm_property_create_blob(dev, sizeof(struct drm_palette_caps),
>> +			(const void *) palette_caps);
>> +
>> +	if (blob)
>> +		return blob->base.id;
>
> It's a corner case, but blob could be a non-NULL error code here (e.g.,
> -ENOMEM).  We should probably check for that before we try to
> dereference it.
>
Agree, will check it.
>> +
>> +	return 0;
>> +}
>> +
>> +int get_pipe_gamma_capabilities(struct drm_device *dev,
>> +		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
>> +{
>> +	if (IS_CHERRYVIEW(dev))
>> +		return get_chv_pipe_gamma_capabilities(dev, palette_caps, crtc);
>> +	return -EINVAL;
>
> We only call this function in the IS_CHERRYVIEW case at the moment, so I
> realize the EINVAL return is technically dead code, but going forward
> would it make more sense to return a valid capabilities blob that
> explicitly tells userspace we have no capabilities?
This function is more or less a platform check wrapper, which checks if 
color correction is called for a supported platform. In the next patch 
sets, we have IS_GEN9() and IS_BDW() coming here, and if we fail to find 
the right platform, we are returning -EINVAL for invalid platform.
May be a DRM_ERROR("Invalid platform for color correction") will do the 
justice ?
>
>> +}
>> +
>> +void intel_attach_color_properties_to_crtc(struct drm_device *dev,
>> +		struct drm_mode_object *mode_obj)
>> +{
>> +	struct drm_mode_config *config = &dev->mode_config;
>> +	struct drm_palette_caps *palette_caps;
>> +	struct drm_crtc *crtc;
>> +	int capabilities_blob_id;
>> +
>> +	if (IS_CHERRYVIEW(dev)) {
>> +		crtc = obj_to_crtc(mode_obj);
>> +
>> +		palette_caps = kzalloc(sizeof(struct drm_palette_caps),
>> +				GFP_KERNEL);
>> +		capabilities_blob_id = get_pipe_gamma_capabilities(dev, palette_caps, crtc);
>> +		kfree(palette_caps);
>> +		if (config->cm_crtc_palette_capabilities_property)
>> +			drm_object_attach_property(mode_obj,
>> +				config->cm_crtc_palette_capabilities_property,
>> +				capabilities_blob_id);
>> +	}
>> +}
>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
>> new file mode 100644
>> index 0000000..51aeb91
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
>> @@ -0,0 +1,33 @@
>> +/*
>> + * Copyright © 2015 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the next
>> + * paragraph) shall be included in all copies or substantial portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + *
>> + * Authors:
>> + * Shashank Sharma <shashank.sharma@intel.com>
>> + * Kausal Malladi <Kausal.Malladi@intel.com>
>> + */
>> +#include <drm/drmP.h>
>> +#include <drm/drm_crtc_helper.h>
>> +#include "i915_drv.h"
>> +
>> +#define CHV_PALETTE_STRUCT_VERSION		1
>> +#define CHV_DEGAMMA_MAX_VALS			65
>> +#define CHV_10BIT_GAMMA_MAX_VALS		257
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 1412e21..349a1c2 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13975,6 +13975,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>>
>>   	intel_crtc->wm.cxsr_allowed = true;
>>
>> +	intel_attach_color_properties_to_crtc(dev, &intel_crtc->base.base);
>> +
>
> I feel like we should hold off on actually calling this function until
> the very end of your patch series.  If someone is bisecting through
> histroy to track down a bug and they land on this commit, we'll be
> advertising some capabilities to userspace that you don't really have
> yet (since they only show up in the later patches of the series).
> If we wait until the end of the series to "flip the switch" and enable
> color management, then we don't have to worry about running partially
> implemented features during a bisect session.
>
>
> Matt
>
I was afraid of getting review comments like "add the function only when 
you are using it" so I added the call here. Also we thought it would be 
preferable to add this in the patch where actually initialize the color 
management. If you still think we should move it, I can do it.
>>   	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
>>   	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
>>   	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index b3dc138..dee5f91 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1438,4 +1438,8 @@ void intel_plane_destroy_state(struct drm_plane *plane,
>>   			       struct drm_plane_state *state);
>>   extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
>>
>> +/* intel_color_manager.c */
>> +void intel_attach_color_properties_to_crtc(struct drm_device *dev,
>> +		struct drm_mode_object *mode_obj);
>> +
>>   #endif /* __INTEL_DRV_H__ */
>> --
>> 1.9.1
>>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 06/18] drm: Add color correction blobs in CRTC state

[Shashank Sharma]

From: Kausal Malladi <kausalmalladi@gmail.com>

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

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

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 3c59045..504539a 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -304,6 +304,11 @@ struct drm_crtc_state {
 	/* blob property to expose current mode to atomic userspace */
 	struct drm_property_blob *mode_blob;
 
+	/* blob properties to hold the color properties' blobs */
+	struct drm_property_blob *palette_before_ctm_blob;
+	struct drm_property_blob *palette_after_ctm_blob;
+	struct drm_property_blob *ctm_blob;
+
 	struct drm_pending_vblank_event *event;
 
 	struct drm_atomic_state *state;
-- 
1.9.1

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

Re: [PATCH 06/18] drm: Add color correction blobs in CRTC state

[Matt Roper]

On Thu, Aug 06, 2015 at 10:08:15PM +0530, Shashank Sharma wrote:
> From: Kausal Malladi <kausalmalladi@gmail.com>
> 
> This patch adds new variables in CRTC state, to hold respective color
> correction blobs. These blobs will be required during the atomic commit
> for writing the color correction values in correction registers.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>

Since these are part of the state now, I believe you also need to add a
drm_property_reference_blob() call in
__drm_atomic_helper_crtc_duplicate_state and a
drm_property_unreference_blob() call in
__drm_atomic_helper_crtc_destroy_state so that we properly
increment/decrement the reference count as the state gets
duplicated/destroyed.  I don't see that later in the series, so this
might be the best patch to add it to.


Matt

> ---
>  include/drm/drm_crtc.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 3c59045..504539a 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -304,6 +304,11 @@ struct drm_crtc_state {
>  	/* blob property to expose current mode to atomic userspace */
>  	struct drm_property_blob *mode_blob;
>  
> +	/* blob properties to hold the color properties' blobs */
> +	struct drm_property_blob *palette_before_ctm_blob;
> +	struct drm_property_blob *palette_after_ctm_blob;
> +	struct drm_property_blob *ctm_blob;
> +
>  	struct drm_pending_vblank_event *event;
>  
>  	struct drm_atomic_state *state;
> -- 
> 1.9.1
> 

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

Re: [PATCH 06/18] drm: Add color correction blobs in CRTC state

[Sharma, Shashank]

Regards
Shashank

On 8/22/2015 4:10 AM, Matt Roper wrote:
> On Thu, Aug 06, 2015 at 10:08:15PM +0530, Shashank Sharma wrote:
>> From: Kausal Malladi <kausalmalladi@gmail.com>
>>
>> This patch adds new variables in CRTC state, to hold respective color
>> correction blobs. These blobs will be required during the atomic commit
>> for writing the color correction values in correction registers.
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
>
> Since these are part of the state now, I believe you also need to add a
> drm_property_reference_blob() call in
> __drm_atomic_helper_crtc_duplicate_state and a
> drm_property_unreference_blob() call in
> __drm_atomic_helper_crtc_destroy_state so that we properly
> increment/decrement the reference count as the state gets
> duplicated/destroyed.  I don't see that later in the series, so this
> might be the best patch to add it to.
>
>
> Matt
Oops, agree. Will add this.
>
>> ---
>>   include/drm/drm_crtc.h | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index 3c59045..504539a 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -304,6 +304,11 @@ struct drm_crtc_state {
>>   	/* blob property to expose current mode to atomic userspace */
>>   	struct drm_property_blob *mode_blob;
>>
>> +	/* blob properties to hold the color properties' blobs */
>> +	struct drm_property_blob *palette_before_ctm_blob;
>> +	struct drm_property_blob *palette_after_ctm_blob;
>> +	struct drm_property_blob *ctm_blob;
>> +
>>   	struct drm_pending_vblank_event *event;
>>
>>   	struct drm_atomic_state *state;
>> --
>> 1.9.1
>>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 07/18] drm: Add drm structures for palette color property

[Shashank Sharma]

From: Kausal Malladi <kausalmalladi@gmail.com>

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

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

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

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

[PATCH 08/18] drm/i915: Add pipe gamma correction handlers

[Shashank Sharma]

From: Kausal Malladi <kausalmalladi@gmail.com>

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

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

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

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 8d04ee8..9f55e6c 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -324,6 +324,13 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
 				   struct drm_property *property,
 				   uint64_t val)
 {
+	struct drm_device *dev = crtc->dev;
+	struct drm_mode_config *config = &dev->mode_config;
+
+	if (property == config->cm_palette_after_ctm_property)
+		return intel_color_manager_set_pipe_gamma(dev, state,
+				&crtc->base, val);
+
 	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
 	return -EINVAL;
 }
@@ -333,5 +340,12 @@ int intel_crtc_atomic_get_property(struct drm_crtc *crtc,
 				   struct drm_property *property,
 				   uint64_t *val)
 {
+	struct drm_device *dev = crtc->dev;
+	struct drm_mode_config *config = &dev->mode_config;
+
+	if (property == config->cm_palette_after_ctm_property)
+		*val = (state->palette_after_ctm_blob) ?
+			state->palette_after_ctm_blob->base.id : 0;
+
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
index 1c9c477..9a6126c 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.c
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -27,6 +27,26 @@
 
 #include "intel_color_manager.h"
 
+int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
+		struct drm_crtc_state *crtc_state,
+		struct drm_mode_object *obj, uint32_t blob_id)
+{
+	struct drm_property_blob *blob;
+
+	blob = drm_property_lookup_blob(dev, blob_id);
+	if (!blob) {
+		DRM_ERROR("Invalid Blob ID\n");
+		return -EINVAL;
+	}
+
+	if (crtc_state->palette_after_ctm_blob)
+		drm_property_unreference_blob(crtc_state->palette_after_ctm_blob);
+
+	/* Attach the blob to be commited in state */
+	crtc_state->palette_after_ctm_blob = blob;
+	return 0;
+}
+
 int get_chv_pipe_gamma_capabilities(struct drm_device *dev,
 		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index dee5f91..820ded7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1441,5 +1441,8 @@ extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
 /* intel_color_manager.c */
 void intel_attach_color_properties_to_crtc(struct drm_device *dev,
 		struct drm_mode_object *mode_obj);
+int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
+		struct drm_crtc_state *crtc_state,
+		struct drm_mode_object *obj, uint32_t blob_id);
 
 #endif /* __INTEL_DRV_H__ */
-- 
1.9.1

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

Re: [PATCH 08/18] drm/i915: Add pipe gamma correction handlers

[Matt Roper]

On Thu, Aug 06, 2015 at 10:08:17PM +0530, Shashank Sharma wrote:
> From: Kausal Malladi <kausalmalladi@gmail.com>
> 
> I915 driver registers gamma correction as palette correction
> property with DRM layer. This patch adds set_property() and get_property()
> handlers for pipe level gamma correction.
> 
> The set function attaches the Gamma correction blob to CRTC state, these
> values will be committed during atomic commit.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic.c        | 14 ++++++++++++++
>  drivers/gpu/drm/i915/intel_color_manager.c | 20 ++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h           |  3 +++
>  3 files changed, 37 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 8d04ee8..9f55e6c 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -324,6 +324,13 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
>  				   struct drm_property *property,
>  				   uint64_t val)
>  {
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	if (property == config->cm_palette_after_ctm_property)
> +		return intel_color_manager_set_pipe_gamma(dev, state,
> +				&crtc->base, val);
> +
>  	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
>  	return -EINVAL;
>  }
> @@ -333,5 +340,12 @@ int intel_crtc_atomic_get_property(struct drm_crtc *crtc,
>  				   struct drm_property *property,
>  				   uint64_t *val)
>  {
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	if (property == config->cm_palette_after_ctm_property)
> +		*val = (state->palette_after_ctm_blob) ?
> +			state->palette_after_ctm_blob->base.id : 0;
> +
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
> index 1c9c477..9a6126c 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.c
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -27,6 +27,26 @@
>  
>  #include "intel_color_manager.h"
>  
> +int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
> +		struct drm_crtc_state *crtc_state,
> +		struct drm_mode_object *obj, uint32_t blob_id)
> +{
> +	struct drm_property_blob *blob;
> +
> +	blob = drm_property_lookup_blob(dev, blob_id);
> +	if (!blob) {
> +		DRM_ERROR("Invalid Blob ID\n");

A user can trigger this error on demand, so I think we want to keep this
as DRM_DEBUG_KMS (same on patches #10 and 13).


Matt

> +		return -EINVAL;
> +	}
> +
> +	if (crtc_state->palette_after_ctm_blob)
> +		drm_property_unreference_blob(crtc_state->palette_after_ctm_blob);
> +
> +	/* Attach the blob to be commited in state */
> +	crtc_state->palette_after_ctm_blob = blob;
> +	return 0;
> +}
> +
>  int get_chv_pipe_gamma_capabilities(struct drm_device *dev,
>  		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index dee5f91..820ded7 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1441,5 +1441,8 @@ extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
>  /* intel_color_manager.c */
>  void intel_attach_color_properties_to_crtc(struct drm_device *dev,
>  		struct drm_mode_object *mode_obj);
> +int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
> +		struct drm_crtc_state *crtc_state,
> +		struct drm_mode_object *obj, uint32_t blob_id);
>  
>  #endif /* __INTEL_DRV_H__ */
> -- 
> 1.9.1
> 

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

Re: [PATCH 08/18] drm/i915: Add pipe gamma correction handlers

[Sharma, Shashank]

Regards
Shashank

On 8/22/2015 4:10 AM, Matt Roper wrote:
> On Thu, Aug 06, 2015 at 10:08:17PM +0530, Shashank Sharma wrote:
>> From: Kausal Malladi <kausalmalladi@gmail.com>
>>
>> I915 driver registers gamma correction as palette correction
>> property with DRM layer. This patch adds set_property() and get_property()
>> handlers for pipe level gamma correction.
>>
>> The set function attaches the Gamma correction blob to CRTC state, these
>> values will be committed during atomic commit.
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_atomic.c        | 14 ++++++++++++++
>>   drivers/gpu/drm/i915/intel_color_manager.c | 20 ++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_drv.h           |  3 +++
>>   3 files changed, 37 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
>> index 8d04ee8..9f55e6c 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -324,6 +324,13 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
>>   				   struct drm_property *property,
>>   				   uint64_t val)
>>   {
>> +	struct drm_device *dev = crtc->dev;
>> +	struct drm_mode_config *config = &dev->mode_config;
>> +
>> +	if (property == config->cm_palette_after_ctm_property)
>> +		return intel_color_manager_set_pipe_gamma(dev, state,
>> +				&crtc->base, val);
>> +
>>   	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
>>   	return -EINVAL;
>>   }
>> @@ -333,5 +340,12 @@ int intel_crtc_atomic_get_property(struct drm_crtc *crtc,
>>   				   struct drm_property *property,
>>   				   uint64_t *val)
>>   {
>> +	struct drm_device *dev = crtc->dev;
>> +	struct drm_mode_config *config = &dev->mode_config;
>> +
>> +	if (property == config->cm_palette_after_ctm_property)
>> +		*val = (state->palette_after_ctm_blob) ?
>> +			state->palette_after_ctm_blob->base.id : 0;
>> +
>>   	return 0;
>>   }
>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
>> index 1c9c477..9a6126c 100644
>> --- a/drivers/gpu/drm/i915/intel_color_manager.c
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
>> @@ -27,6 +27,26 @@
>>
>>   #include "intel_color_manager.h"
>>
>> +int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
>> +		struct drm_crtc_state *crtc_state,
>> +		struct drm_mode_object *obj, uint32_t blob_id)
>> +{
>> +	struct drm_property_blob *blob;
>> +
>> +	blob = drm_property_lookup_blob(dev, blob_id);
>> +	if (!blob) {
>> +		DRM_ERROR("Invalid Blob ID\n");
>
> A user can trigger this error on demand, so I think we want to keep this
> as DRM_DEBUG_KMS (same on patches #10 and 13).
>
Agree.
>
> Matt
>
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (crtc_state->palette_after_ctm_blob)
>> +		drm_property_unreference_blob(crtc_state->palette_after_ctm_blob);
>> +
>> +	/* Attach the blob to be commited in state */
>> +	crtc_state->palette_after_ctm_blob = blob;
>> +	return 0;
>> +}
>> +
>>   int get_chv_pipe_gamma_capabilities(struct drm_device *dev,
>>   		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
>>   {
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index dee5f91..820ded7 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1441,5 +1441,8 @@ extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
>>   /* intel_color_manager.c */
>>   void intel_attach_color_properties_to_crtc(struct drm_device *dev,
>>   		struct drm_mode_object *mode_obj);
>> +int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
>> +		struct drm_crtc_state *crtc_state,
>> +		struct drm_mode_object *obj, uint32_t blob_id);
>>
>>   #endif /* __INTEL_DRV_H__ */
>> --
>> 1.9.1
>>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 08/18] drm/i915: Add pipe gamma correction handlers

[Daniel Vetter]

On Sat, Aug 22, 2015 at 11:41:45AM +0530, Sharma, Shashank wrote:
> Regards
> Shashank
> 
> On 8/22/2015 4:10 AM, Matt Roper wrote:
> >On Thu, Aug 06, 2015 at 10:08:17PM +0530, Shashank Sharma wrote:
> >>From: Kausal Malladi <kausalmalladi@gmail.com>
> >>
> >>I915 driver registers gamma correction as palette correction
> >>property with DRM layer. This patch adds set_property() and get_property()
> >>handlers for pipe level gamma correction.
> >>
> >>The set function attaches the Gamma correction blob to CRTC state, these
> >>values will be committed during atomic commit.
> >>
> >>Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >>Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
> >>---
> >>  drivers/gpu/drm/i915/intel_atomic.c        | 14 ++++++++++++++
> >>  drivers/gpu/drm/i915/intel_color_manager.c | 20 ++++++++++++++++++++
> >>  drivers/gpu/drm/i915/intel_drv.h           |  3 +++
> >>  3 files changed, 37 insertions(+)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> >>index 8d04ee8..9f55e6c 100644
> >>--- a/drivers/gpu/drm/i915/intel_atomic.c
> >>+++ b/drivers/gpu/drm/i915/intel_atomic.c
> >>@@ -324,6 +324,13 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
> >>  				   struct drm_property *property,
> >>  				   uint64_t val)
> >>  {
> >>+	struct drm_device *dev = crtc->dev;
> >>+	struct drm_mode_config *config = &dev->mode_config;
> >>+
> >>+	if (property == config->cm_palette_after_ctm_property)
> >>+		return intel_color_manager_set_pipe_gamma(dev, state,
> >>+				&crtc->base, val);
> >>+
> >>  	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
> >>  	return -EINVAL;
> >>  }
> >>@@ -333,5 +340,12 @@ int intel_crtc_atomic_get_property(struct drm_crtc *crtc,
> >>  				   struct drm_property *property,
> >>  				   uint64_t *val)
> >>  {
> >>+	struct drm_device *dev = crtc->dev;
> >>+	struct drm_mode_config *config = &dev->mode_config;
> >>+
> >>+	if (property == config->cm_palette_after_ctm_property)
> >>+		*val = (state->palette_after_ctm_blob) ?
> >>+			state->palette_after_ctm_blob->base.id : 0;
> >>+
> >>  	return 0;
> >>  }
> >>diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
> >>index 1c9c477..9a6126c 100644
> >>--- a/drivers/gpu/drm/i915/intel_color_manager.c
> >>+++ b/drivers/gpu/drm/i915/intel_color_manager.c
> >>@@ -27,6 +27,26 @@
> >>
> >>  #include "intel_color_manager.h"
> >>
> >>+int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
> >>+		struct drm_crtc_state *crtc_state,
> >>+		struct drm_mode_object *obj, uint32_t blob_id)
> >>+{
> >>+	struct drm_property_blob *blob;
> >>+
> >>+	blob = drm_property_lookup_blob(dev, blob_id);
> >>+	if (!blob) {
> >>+		DRM_ERROR("Invalid Blob ID\n");
> >
> >A user can trigger this error on demand, so I think we want to keep this
> >as DRM_DEBUG_KMS (same on patches #10 and 13).
> >
> Agree.

Again needs a testcase in igt ...
-Daniel

> >
> >Matt
> >
> >>+		return -EINVAL;
> >>+	}
> >>+
> >>+	if (crtc_state->palette_after_ctm_blob)
> >>+		drm_property_unreference_blob(crtc_state->palette_after_ctm_blob);
> >>+
> >>+	/* Attach the blob to be commited in state */
> >>+	crtc_state->palette_after_ctm_blob = blob;
> >>+	return 0;
> >>+}
> >>+
> >>  int get_chv_pipe_gamma_capabilities(struct drm_device *dev,
> >>  		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
> >>  {
> >>diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >>index dee5f91..820ded7 100644
> >>--- a/drivers/gpu/drm/i915/intel_drv.h
> >>+++ b/drivers/gpu/drm/i915/intel_drv.h
> >>@@ -1441,5 +1441,8 @@ extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
> >>  /* intel_color_manager.c */
> >>  void intel_attach_color_properties_to_crtc(struct drm_device *dev,
> >>  		struct drm_mode_object *mode_obj);
> >>+int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
> >>+		struct drm_crtc_state *crtc_state,
> >>+		struct drm_mode_object *obj, uint32_t blob_id);
> >>
> >>  #endif /* __INTEL_DRV_H__ */
> >>--
> >>1.9.1
> >>
> >
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[PATCH 09/18] drm/i915: Pipe level Gamma correction for CHV/BSW

[Shashank Sharma]

From: Kausal Malladi <kausalmalladi@gmail.com>

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

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

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

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 3a77678..4997ebd 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7921,4 +7921,16 @@ enum skl_disp_power_wells {
 #define GEN9_VEBOX_MOCS_0	0xcb00	/* Video MOCS base register*/
 #define GEN9_BLT_MOCS_0		0xcc00	/* Blitter MOCS base register*/
 
+/* Color Management */
+#define PIPEA_CGM_CONTROL			(VLV_DISPLAY_BASE + 0x67A00)
+#define PIPEB_CGM_CONTROL			(VLV_DISPLAY_BASE + 0x69A00)
+#define PIPEC_CGM_CONTROL			(VLV_DISPLAY_BASE + 0x6BA00)
+#define PIPEA_CGM_GAMMA				(VLV_DISPLAY_BASE + 0x67000)
+#define PIPEB_CGM_GAMMA				(VLV_DISPLAY_BASE + 0x69000)
+#define PIPEC_CGM_GAMMA				(VLV_DISPLAY_BASE + 0x6B000)
+#define _PIPE_CGM_CONTROL(pipe) \
+	(_PIPE3(pipe, PIPEA_CGM_CONTROL, PIPEB_CGM_CONTROL, PIPEC_CGM_CONTROL))
+#define _PIPE_GAMMA_BASE(pipe) \
+	(_PIPE3(pipe, PIPEA_CGM_GAMMA, PIPEB_CGM_GAMMA, PIPEC_CGM_GAMMA))
+
 #endif /* _I915_REG_H_ */
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
index 9a6126c..f8c8d26 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.c
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -27,6 +27,149 @@
 
 #include "intel_color_manager.h"
 
+int chv_set_gamma(struct drm_device *dev, struct drm_property_blob *blob,
+		  struct drm_crtc *crtc)
+{
+	bool flag = false;
+	enum pipe pipe;
+	u8 red_int, blue_int, green_int;
+	u16 red_fract, green_fract, blue_fract;
+	u32 red, green, blue;
+	u32 cgm_control_reg = 0;
+	u32 cgm_gamma_reg = 0;
+	u32 count = 0, num_samples, word;
+	int ret = 0, length;
+	struct drm_r32g32b32 *correction_values = NULL;
+	struct drm_palette *gamma_data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (!blob) {
+		DRM_ERROR("NULL Blob\n");
+		return -EINVAL;
+	}
+
+	gamma_data = (struct drm_palette *)blob->data;
+
+	if (gamma_data->version != CHV_GAMMA_DATA_STRUCT_VERSION) {
+		DRM_ERROR("Invalid Gamma Data struct version\n");
+		return -EINVAL;
+	}
+
+	pipe = to_intel_crtc(crtc)->pipe;
+	num_samples = gamma_data->num_samples;
+	length = num_samples * sizeof(struct drm_r32g32b32);
+
+	switch (num_samples) {
+	case 0:
+
+		/* Disable Gamma functionality on Pipe - CGM Block */
+		cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe));
+		cgm_control_reg &= ~CGM_GAMMA_EN;
+		I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg);
+
+		DRM_DEBUG_DRIVER("Gamma disabled on Pipe %c\n",
+				pipe_name(pipe));
+		ret = 0;
+		break;
+
+	case CHV_8BIT_GAMMA_MAX_VALS:
+	case CHV_10BIT_GAMMA_MAX_VALS:
+
+		count = 0;
+		cgm_gamma_reg = _PIPE_GAMMA_BASE(pipe);
+		correction_values = (struct drm_r32g32b32 *)&gamma_data->lut;
+
+		while (count < num_samples) {
+			blue = correction_values[count].b32;
+			green = correction_values[count].g32;
+			red = correction_values[count].r32;
+
+			blue_int = _GAMMA_INT_PART(blue);
+			if (blue_int > GAMMA_INT_MAX)
+				blue = CHV_MAX_GAMMA;
+
+			green_int = _GAMMA_INT_PART(green);
+			if (green_int > GAMMA_INT_MAX)
+				green = CHV_MAX_GAMMA;
+
+			red_int = _GAMMA_INT_PART(red);
+			if (red_int > GAMMA_INT_MAX)
+				red = CHV_MAX_GAMMA;
+
+			blue_fract = _GAMMA_FRACT_PART(blue);
+			green_fract = _GAMMA_FRACT_PART(green);
+			red_fract = _GAMMA_FRACT_PART(red);
+
+			blue_fract >>= CHV_10BIT_GAMMA_MSB_SHIFT;
+			green_fract >>= CHV_10BIT_GAMMA_MSB_SHIFT;
+			red_fract >>= CHV_10BIT_GAMMA_MSB_SHIFT;
+
+			/* Green (25:16) and Blue (9:0) to be written */
+			word = green_fract;
+			word <<= CHV_GAMMA_SHIFT_GREEN;
+			word |= blue_fract;
+			I915_WRITE(cgm_gamma_reg, word);
+			cgm_gamma_reg += 4;
+
+			/* Red (9:0) to be written */
+			word = red_fract;
+			I915_WRITE(cgm_gamma_reg, word);
+
+			cgm_gamma_reg += 4;
+			count++;
+
+			/*
+			 * On CHV, the best supported Gamma capability is
+			 * CGM block, that takes 257 samples.
+			 * If user gives 256 samples (legacy mode), then
+			 * duplicate the 256th value to 257th.
+			 * "flag" is used to make sure it happens only once
+			 */
+			if (num_samples == CHV_8BIT_GAMMA_MAX_VALS
+					&& count == CHV_8BIT_GAMMA_MAX_VALS
+					&& !flag) {
+				count--;
+				flag = true;
+			}
+		}
+
+		/* Enable (CGM) Gamma on Pipe */
+		I915_WRITE(_PIPE_CGM_CONTROL(pipe),
+			I915_READ(_PIPE_CGM_CONTROL(pipe))
+				| CGM_GAMMA_EN);
+		DRM_DEBUG_DRIVER("CGM Gamma enabled on Pipe %c\n",
+				pipe_name(pipe));
+		ret = 0;
+		break;
+
+	default:
+		DRM_ERROR("Invalid number of samples for Gamma LUT\n");
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+void intel_color_manager_crtc_commit(struct drm_device *dev,
+		struct drm_crtc_state *crtc_state)
+{
+	struct drm_property_blob *blob;
+	struct drm_crtc *crtc = crtc_state->crtc;
+	int ret = -EINVAL;
+
+	blob = crtc_state->palette_after_ctm_blob;
+	if (blob) {
+		/* Gamma correction is platform specific */
+		if (IS_CHERRYVIEW(dev))
+			ret = chv_set_gamma(dev, blob, crtc);
+
+		if (ret)
+			DRM_ERROR("set Gamma correction failed\n");
+		else
+			DRM_DEBUG_DRIVER("Gamma correction success\n");
+	}
+}
+
 int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
 		struct drm_crtc_state *crtc_state,
 		struct drm_mode_object *obj, uint32_t blob_id)
@@ -99,5 +242,8 @@ void intel_attach_color_properties_to_crtc(struct drm_device *dev,
 			drm_object_attach_property(mode_obj,
 				config->cm_crtc_palette_capabilities_property,
 				capabilities_blob_id);
+		if (config->cm_palette_after_ctm_property)
+			drm_object_attach_property(mode_obj,
+				config->cm_palette_after_ctm_property, 0);
 	}
 }
diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
index 51aeb91..8bbac20 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.h
+++ b/drivers/gpu/drm/i915/intel_color_manager.h
@@ -28,6 +28,26 @@
 #include <drm/drm_crtc_helper.h>
 #include "i915_drv.h"
 
+#define _GAMMA_INT_PART(value)			(value >> GAMMA_INT_SHIFT)
+#define _GAMMA_FRACT_PART(value)		(value >> GAMMA_FRACT_SHIFT)
+
 #define CHV_PALETTE_STRUCT_VERSION		1
 #define CHV_DEGAMMA_MAX_VALS			65
 #define CHV_10BIT_GAMMA_MAX_VALS		257
+
+/* Gamma correction */
+#define CHV_GAMMA_DATA_STRUCT_VERSION		1
+#define CHV_10BIT_GAMMA_MAX_VALS		257
+#define CHV_8BIT_GAMMA_MAX_VALS			256
+#define CHV_10BIT_GAMMA_MSB_SHIFT		6
+#define CHV_GAMMA_SHIFT_GREEN			16
+/* Gamma values are u8.24 format */
+#define GAMMA_INT_SHIFT				24
+#define GAMMA_FRACT_SHIFT			8
+/* Max int value for Gamma is 1 */
+#define GAMMA_INT_MAX				1
+/* Max value for Gamma on CHV */
+#define CHV_MAX_GAMMA				0x10000
+
+/* CHV CGM Block */
+#define CGM_GAMMA_EN				(1 << 2)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 349a1c2..9d1a6c3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13641,6 +13641,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
 
 	if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
 		skl_detach_scalers(intel_crtc);
+
+	intel_color_manager_crtc_commit(dev, crtc->state);
 }
 
 static void intel_finish_crtc_commit(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 820ded7..de3e6e7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1444,5 +1444,7 @@ void intel_attach_color_properties_to_crtc(struct drm_device *dev,
 int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
 		struct drm_crtc_state *crtc_state,
 		struct drm_mode_object *obj, uint32_t blob_id);
+void intel_color_manager_crtc_commit(struct drm_device *dev,
+		struct drm_crtc_state *crtc_state);
 
 #endif /* __INTEL_DRV_H__ */
-- 
1.9.1

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

Re: [PATCH 09/18] drm/i915: Pipe level Gamma correction for CHV/BSW

[Matt Roper]

On Thu, Aug 06, 2015 at 10:08:18PM +0530, Shashank Sharma wrote:
> From: Kausal Malladi <kausalmalladi@gmail.com>
> 
> CHV/BSW platform supports two different pipe level gamma
> correction modes, which are:
> 1. Legacy 8-bit mode
> 2. 10-bit CGM (Color Gamut Mapping) mode
> 
> This patch does the following:
> 1. Attaches Gamma property to CRTC
> 3. Adds the core Gamma correction function for CHV/BSW
> 4. Adds Gamma correction macros
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h            |  12 +++
>  drivers/gpu/drm/i915/intel_color_manager.c | 146 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_color_manager.h |  20 ++++
>  drivers/gpu/drm/i915/intel_display.c       |   2 +
>  drivers/gpu/drm/i915/intel_drv.h           |   2 +
>  5 files changed, 182 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 3a77678..4997ebd 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7921,4 +7921,16 @@ enum skl_disp_power_wells {
>  #define GEN9_VEBOX_MOCS_0	0xcb00	/* Video MOCS base register*/
>  #define GEN9_BLT_MOCS_0		0xcc00	/* Blitter MOCS base register*/
>  
> +/* Color Management */
> +#define PIPEA_CGM_CONTROL			(VLV_DISPLAY_BASE + 0x67A00)
> +#define PIPEB_CGM_CONTROL			(VLV_DISPLAY_BASE + 0x69A00)
> +#define PIPEC_CGM_CONTROL			(VLV_DISPLAY_BASE + 0x6BA00)
> +#define PIPEA_CGM_GAMMA				(VLV_DISPLAY_BASE + 0x67000)
> +#define PIPEB_CGM_GAMMA				(VLV_DISPLAY_BASE + 0x69000)
> +#define PIPEC_CGM_GAMMA				(VLV_DISPLAY_BASE + 0x6B000)
> +#define _PIPE_CGM_CONTROL(pipe) \
> +	(_PIPE3(pipe, PIPEA_CGM_CONTROL, PIPEB_CGM_CONTROL, PIPEC_CGM_CONTROL))
> +#define _PIPE_GAMMA_BASE(pipe) \
> +	(_PIPE3(pipe, PIPEA_CGM_GAMMA, PIPEB_CGM_GAMMA, PIPEC_CGM_GAMMA))
> +
>  #endif /* _I915_REG_H_ */
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
> index 9a6126c..f8c8d26 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.c
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -27,6 +27,149 @@
>  
>  #include "intel_color_manager.h"
>  
> +int chv_set_gamma(struct drm_device *dev, struct drm_property_blob *blob,
> +		  struct drm_crtc *crtc)

It looks like this function is only called from this file, right?  We
can probably make it static in that case.

> +{
> +	bool flag = false;
> +	enum pipe pipe;
> +	u8 red_int, blue_int, green_int;
> +	u16 red_fract, green_fract, blue_fract;
> +	u32 red, green, blue;
> +	u32 cgm_control_reg = 0;
> +	u32 cgm_gamma_reg = 0;
> +	u32 count = 0, num_samples, word;
> +	int ret = 0, length;
> +	struct drm_r32g32b32 *correction_values = NULL;
> +	struct drm_palette *gamma_data;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (!blob) {
> +		DRM_ERROR("NULL Blob\n");
> +		return -EINVAL;
> +	}

The way the code stands right now, this should never be possible since
we don't even call this function if the blob is NULL, right?  In that
case we usually just handle this as

        if (WARN_ON(!blob))
                return -EINVAL;

to make it a little more obvious that this is truly a "this can never
happen" type of assertion.

Also, see my comment near the bottom about whether this would be a more
intuitive way to disable the current gamma values.


> +
> +	gamma_data = (struct drm_palette *)blob->data;
> +
> +	if (gamma_data->version != CHV_GAMMA_DATA_STRUCT_VERSION) {
> +		DRM_ERROR("Invalid Gamma Data struct version\n");

A user can trigger this on-demand (and thus spam your kernel log), so
this should probably be a DRM_DEBUG_KMS rather than a DRM_ERROR.

> +		return -EINVAL;
> +	}
> +
> +	pipe = to_intel_crtc(crtc)->pipe;
> +	num_samples = gamma_data->num_samples;
> +	length = num_samples * sizeof(struct drm_r32g32b32);
> +
> +	switch (num_samples) {
> +	case 0:
> +
> +		/* Disable Gamma functionality on Pipe - CGM Block */
> +		cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe));
> +		cgm_control_reg &= ~CGM_GAMMA_EN;
> +		I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg);
> +
> +		DRM_DEBUG_DRIVER("Gamma disabled on Pipe %c\n",
> +				pipe_name(pipe));
> +		ret = 0;
> +		break;
> +
> +	case CHV_8BIT_GAMMA_MAX_VALS:
> +	case CHV_10BIT_GAMMA_MAX_VALS:
> +
> +		count = 0;
> +		cgm_gamma_reg = _PIPE_GAMMA_BASE(pipe);
> +		correction_values = (struct drm_r32g32b32 *)&gamma_data->lut;
> +
> +		while (count < num_samples) {
> +			blue = correction_values[count].b32;
> +			green = correction_values[count].g32;
> +			red = correction_values[count].r32;
> +
> +			blue_int = _GAMMA_INT_PART(blue);
> +			if (blue_int > GAMMA_INT_MAX)
> +				blue = CHV_MAX_GAMMA;
> +
> +			green_int = _GAMMA_INT_PART(green);
> +			if (green_int > GAMMA_INT_MAX)
> +				green = CHV_MAX_GAMMA;
> +
> +			red_int = _GAMMA_INT_PART(red);
> +			if (red_int > GAMMA_INT_MAX)
> +				red = CHV_MAX_GAMMA;
> +
> +			blue_fract = _GAMMA_FRACT_PART(blue);
> +			green_fract = _GAMMA_FRACT_PART(green);
> +			red_fract = _GAMMA_FRACT_PART(red);
> +
> +			blue_fract >>= CHV_10BIT_GAMMA_MSB_SHIFT;
> +			green_fract >>= CHV_10BIT_GAMMA_MSB_SHIFT;
> +			red_fract >>= CHV_10BIT_GAMMA_MSB_SHIFT;
> +
> +			/* Green (25:16) and Blue (9:0) to be written */
> +			word = green_fract;
> +			word <<= CHV_GAMMA_SHIFT_GREEN;
> +			word |= blue_fract;
> +			I915_WRITE(cgm_gamma_reg, word);
> +			cgm_gamma_reg += 4;
> +
> +			/* Red (9:0) to be written */
> +			word = red_fract;
> +			I915_WRITE(cgm_gamma_reg, word);
> +
> +			cgm_gamma_reg += 4;
> +			count++;
> +
> +			/*
> +			 * On CHV, the best supported Gamma capability is
> +			 * CGM block, that takes 257 samples.
> +			 * If user gives 256 samples (legacy mode), then
> +			 * duplicate the 256th value to 257th.
> +			 * "flag" is used to make sure it happens only once
> +			 */
> +			if (num_samples == CHV_8BIT_GAMMA_MAX_VALS
> +					&& count == CHV_8BIT_GAMMA_MAX_VALS
> +					&& !flag) {
> +				count--;
> +				flag = true;
> +			}
> +		}
> +
> +		/* Enable (CGM) Gamma on Pipe */
> +		I915_WRITE(_PIPE_CGM_CONTROL(pipe),
> +			I915_READ(_PIPE_CGM_CONTROL(pipe))
> +				| CGM_GAMMA_EN);
> +		DRM_DEBUG_DRIVER("CGM Gamma enabled on Pipe %c\n",
> +				pipe_name(pipe));
> +		ret = 0;
> +		break;
> +
> +	default:
> +		DRM_ERROR("Invalid number of samples for Gamma LUT\n");
> +		ret = -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
> +void intel_color_manager_crtc_commit(struct drm_device *dev,
> +		struct drm_crtc_state *crtc_state)
> +{
> +	struct drm_property_blob *blob;
> +	struct drm_crtc *crtc = crtc_state->crtc;
> +	int ret = -EINVAL;
> +
> +	blob = crtc_state->palette_after_ctm_blob;
> +	if (blob) {
> +		/* Gamma correction is platform specific */
> +		if (IS_CHERRYVIEW(dev))
> +			ret = chv_set_gamma(dev, blob, crtc);
> +
> +		if (ret)
> +			DRM_ERROR("set Gamma correction failed\n");
> +		else
> +			DRM_DEBUG_DRIVER("Gamma correction success\n");
> +	}

If I'm understanding the code properly, then it looks like I can't
disable gamma correction by just removing the current blob (i.e., by
updating the blob ID to be 0)?  Instead I have to actually create a new,
valid blob that has num_samples set to zero inside the blob in order to
disable the functionality.  That isn't the behavior I would have
intuitively expected, but that's probably more of a call for the guys
working on the userspace code that actually uses this functionality.
Just thought I'd make a note of it since it seemed a bit surprising to
me.

(btw, I think all the comments on this patch also apply to patches #11
and 14)

Matt

> +}
> +
>  int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
>  		struct drm_crtc_state *crtc_state,
>  		struct drm_mode_object *obj, uint32_t blob_id)
> @@ -99,5 +242,8 @@ void intel_attach_color_properties_to_crtc(struct drm_device *dev,
>  			drm_object_attach_property(mode_obj,
>  				config->cm_crtc_palette_capabilities_property,
>  				capabilities_blob_id);
> +		if (config->cm_palette_after_ctm_property)
> +			drm_object_attach_property(mode_obj,
> +				config->cm_palette_after_ctm_property, 0);
>  	}
>  }
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
> index 51aeb91..8bbac20 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.h
> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
> @@ -28,6 +28,26 @@
>  #include <drm/drm_crtc_helper.h>
>  #include "i915_drv.h"
>  
> +#define _GAMMA_INT_PART(value)			(value >> GAMMA_INT_SHIFT)
> +#define _GAMMA_FRACT_PART(value)		(value >> GAMMA_FRACT_SHIFT)
> +
>  #define CHV_PALETTE_STRUCT_VERSION		1
>  #define CHV_DEGAMMA_MAX_VALS			65
>  #define CHV_10BIT_GAMMA_MAX_VALS		257
> +
> +/* Gamma correction */
> +#define CHV_GAMMA_DATA_STRUCT_VERSION		1
> +#define CHV_10BIT_GAMMA_MAX_VALS		257
> +#define CHV_8BIT_GAMMA_MAX_VALS			256
> +#define CHV_10BIT_GAMMA_MSB_SHIFT		6
> +#define CHV_GAMMA_SHIFT_GREEN			16
> +/* Gamma values are u8.24 format */
> +#define GAMMA_INT_SHIFT				24
> +#define GAMMA_FRACT_SHIFT			8
> +/* Max int value for Gamma is 1 */
> +#define GAMMA_INT_MAX				1
> +/* Max value for Gamma on CHV */
> +#define CHV_MAX_GAMMA				0x10000
> +
> +/* CHV CGM Block */
> +#define CGM_GAMMA_EN				(1 << 2)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 349a1c2..9d1a6c3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13641,6 +13641,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>  
>  	if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
>  		skl_detach_scalers(intel_crtc);
> +
> +	intel_color_manager_crtc_commit(dev, crtc->state);
>  }
>  
>  static void intel_finish_crtc_commit(struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 820ded7..de3e6e7 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1444,5 +1444,7 @@ void intel_attach_color_properties_to_crtc(struct drm_device *dev,
>  int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
>  		struct drm_crtc_state *crtc_state,
>  		struct drm_mode_object *obj, uint32_t blob_id);
> +void intel_color_manager_crtc_commit(struct drm_device *dev,
> +		struct drm_crtc_state *crtc_state);
>  
>  #endif /* __INTEL_DRV_H__ */
> -- 
> 1.9.1
> 

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

Re: [PATCH 09/18] drm/i915: Pipe level Gamma correction for CHV/BSW

[Sharma, Shashank]

Regards
Shashank

On 8/22/2015 4:11 AM, Matt Roper wrote:
> On Thu, Aug 06, 2015 at 10:08:18PM +0530, Shashank Sharma wrote:
>> From: Kausal Malladi <kausalmalladi@gmail.com>
>>
>> CHV/BSW platform supports two different pipe level gamma
>> correction modes, which are:
>> 1. Legacy 8-bit mode
>> 2. 10-bit CGM (Color Gamut Mapping) mode
>>
>> This patch does the following:
>> 1. Attaches Gamma property to CRTC
>> 3. Adds the core Gamma correction function for CHV/BSW
>> 4. Adds Gamma correction macros
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/i915_reg.h            |  12 +++
>>   drivers/gpu/drm/i915/intel_color_manager.c | 146 +++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_color_manager.h |  20 ++++
>>   drivers/gpu/drm/i915/intel_display.c       |   2 +
>>   drivers/gpu/drm/i915/intel_drv.h           |   2 +
>>   5 files changed, 182 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 3a77678..4997ebd 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -7921,4 +7921,16 @@ enum skl_disp_power_wells {
>>   #define GEN9_VEBOX_MOCS_0	0xcb00	/* Video MOCS base register*/
>>   #define GEN9_BLT_MOCS_0		0xcc00	/* Blitter MOCS base register*/
>>
>> +/* Color Management */
>> +#define PIPEA_CGM_CONTROL			(VLV_DISPLAY_BASE + 0x67A00)
>> +#define PIPEB_CGM_CONTROL			(VLV_DISPLAY_BASE + 0x69A00)
>> +#define PIPEC_CGM_CONTROL			(VLV_DISPLAY_BASE + 0x6BA00)
>> +#define PIPEA_CGM_GAMMA				(VLV_DISPLAY_BASE + 0x67000)
>> +#define PIPEB_CGM_GAMMA				(VLV_DISPLAY_BASE + 0x69000)
>> +#define PIPEC_CGM_GAMMA				(VLV_DISPLAY_BASE + 0x6B000)
>> +#define _PIPE_CGM_CONTROL(pipe) \
>> +	(_PIPE3(pipe, PIPEA_CGM_CONTROL, PIPEB_CGM_CONTROL, PIPEC_CGM_CONTROL))
>> +#define _PIPE_GAMMA_BASE(pipe) \
>> +	(_PIPE3(pipe, PIPEA_CGM_GAMMA, PIPEB_CGM_GAMMA, PIPEC_CGM_GAMMA))
>> +
>>   #endif /* _I915_REG_H_ */
>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
>> index 9a6126c..f8c8d26 100644
>> --- a/drivers/gpu/drm/i915/intel_color_manager.c
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
>> @@ -27,6 +27,149 @@
>>
>>   #include "intel_color_manager.h"
>>
>> +int chv_set_gamma(struct drm_device *dev, struct drm_property_blob *blob,
>> +		  struct drm_crtc *crtc)
>
> It looks like this function is only called from this file, right?  We
> can probably make it static in that case.
>
Yeah, this was the plan. We kept this non-static initially so that we 
can debug with KGDB when required, but then forgot to add static in the 
end :). Will do it for all platform specific core functions, for all 
properties.
>> +{
>> +	bool flag = false;
>> +	enum pipe pipe;
>> +	u8 red_int, blue_int, green_int;
>> +	u16 red_fract, green_fract, blue_fract;
>> +	u32 red, green, blue;
>> +	u32 cgm_control_reg = 0;
>> +	u32 cgm_gamma_reg = 0;
>> +	u32 count = 0, num_samples, word;
>> +	int ret = 0, length;
>> +	struct drm_r32g32b32 *correction_values = NULL;
>> +	struct drm_palette *gamma_data;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +	if (!blob) {
>> +		DRM_ERROR("NULL Blob\n");
>> +		return -EINVAL;
>> +	}
>
> The way the code stands right now, this should never be possible since
> we don't even call this function if the blob is NULL, right?  In that
> case we usually just handle this as
>
>          if (WARN_ON(!blob))
>                  return -EINVAL;
>
> to make it a little more obvious that this is truly a "this can never
> happen" type of assertion.
>
> Also, see my comment near the bottom about whether this would be a more
> intuitive way to disable the current gamma values.
>
Got it. Agree.
>
>> +
>> +	gamma_data = (struct drm_palette *)blob->data;
>> +
>> +	if (gamma_data->version != CHV_GAMMA_DATA_STRUCT_VERSION) {
>> +		DRM_ERROR("Invalid Gamma Data struct version\n");
>
> A user can trigger this on-demand (and thus spam your kernel log), so
> this should probably be a DRM_DEBUG_KMS rather than a DRM_ERROR.
>
We though we will keep this check only until the first revision of the 
structure is used, and once we have the next possible version, we will 
remove this. Do you think we can keep it by then ?
>> +		return -EINVAL;
>> +	}
>> +
>> +	pipe = to_intel_crtc(crtc)->pipe;
>> +	num_samples = gamma_data->num_samples;
>> +	length = num_samples * sizeof(struct drm_r32g32b32);
>> +
>> +	switch (num_samples) {
>> +	case 0:
>> +
>> +		/* Disable Gamma functionality on Pipe - CGM Block */
>> +		cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe));
>> +		cgm_control_reg &= ~CGM_GAMMA_EN;
>> +		I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg);
>> +
>> +		DRM_DEBUG_DRIVER("Gamma disabled on Pipe %c\n",
>> +				pipe_name(pipe));
>> +		ret = 0;
>> +		break;
>> +
>> +	case CHV_8BIT_GAMMA_MAX_VALS:
>> +	case CHV_10BIT_GAMMA_MAX_VALS:
>> +
>> +		count = 0;
>> +		cgm_gamma_reg = _PIPE_GAMMA_BASE(pipe);
>> +		correction_values = (struct drm_r32g32b32 *)&gamma_data->lut;
>> +
>> +		while (count < num_samples) {
>> +			blue = correction_values[count].b32;
>> +			green = correction_values[count].g32;
>> +			red = correction_values[count].r32;
>> +
>> +			blue_int = _GAMMA_INT_PART(blue);
>> +			if (blue_int > GAMMA_INT_MAX)
>> +				blue = CHV_MAX_GAMMA;
>> +
>> +			green_int = _GAMMA_INT_PART(green);
>> +			if (green_int > GAMMA_INT_MAX)
>> +				green = CHV_MAX_GAMMA;
>> +
>> +			red_int = _GAMMA_INT_PART(red);
>> +			if (red_int > GAMMA_INT_MAX)
>> +				red = CHV_MAX_GAMMA;
>> +
>> +			blue_fract = _GAMMA_FRACT_PART(blue);
>> +			green_fract = _GAMMA_FRACT_PART(green);
>> +			red_fract = _GAMMA_FRACT_PART(red);
>> +
>> +			blue_fract >>= CHV_10BIT_GAMMA_MSB_SHIFT;
>> +			green_fract >>= CHV_10BIT_GAMMA_MSB_SHIFT;
>> +			red_fract >>= CHV_10BIT_GAMMA_MSB_SHIFT;
>> +
>> +			/* Green (25:16) and Blue (9:0) to be written */
>> +			word = green_fract;
>> +			word <<= CHV_GAMMA_SHIFT_GREEN;
>> +			word |= blue_fract;
>> +			I915_WRITE(cgm_gamma_reg, word);
>> +			cgm_gamma_reg += 4;
>> +
>> +			/* Red (9:0) to be written */
>> +			word = red_fract;
>> +			I915_WRITE(cgm_gamma_reg, word);
>> +
>> +			cgm_gamma_reg += 4;
>> +			count++;
>> +
>> +			/*
>> +			 * On CHV, the best supported Gamma capability is
>> +			 * CGM block, that takes 257 samples.
>> +			 * If user gives 256 samples (legacy mode), then
>> +			 * duplicate the 256th value to 257th.
>> +			 * "flag" is used to make sure it happens only once
>> +			 */
>> +			if (num_samples == CHV_8BIT_GAMMA_MAX_VALS
>> +					&& count == CHV_8BIT_GAMMA_MAX_VALS
>> +					&& !flag) {
>> +				count--;
>> +				flag = true;
>> +			}
>> +		}
>> +
>> +		/* Enable (CGM) Gamma on Pipe */
>> +		I915_WRITE(_PIPE_CGM_CONTROL(pipe),
>> +			I915_READ(_PIPE_CGM_CONTROL(pipe))
>> +				| CGM_GAMMA_EN);
>> +		DRM_DEBUG_DRIVER("CGM Gamma enabled on Pipe %c\n",
>> +				pipe_name(pipe));
>> +		ret = 0;
>> +		break;
>> +
>> +	default:
>> +		DRM_ERROR("Invalid number of samples for Gamma LUT\n");
>> +		ret = -EINVAL;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +void intel_color_manager_crtc_commit(struct drm_device *dev,
>> +		struct drm_crtc_state *crtc_state)
>> +{
>> +	struct drm_property_blob *blob;
>> +	struct drm_crtc *crtc = crtc_state->crtc;
>> +	int ret = -EINVAL;
>> +
>> +	blob = crtc_state->palette_after_ctm_blob;
>> +	if (blob) {
>> +		/* Gamma correction is platform specific */
>> +		if (IS_CHERRYVIEW(dev))
>> +			ret = chv_set_gamma(dev, blob, crtc);
>> +
>> +		if (ret)
>> +			DRM_ERROR("set Gamma correction failed\n");
>> +		else
>> +			DRM_DEBUG_DRIVER("Gamma correction success\n");
>> +	}
>
> If I'm understanding the code properly, then it looks like I can't
> disable gamma correction by just removing the current blob (i.e., by
> updating the blob ID to be 0)?  Instead I have to actually create a new,
> valid blob that has num_samples set to zero inside the blob in order to
> disable the functionality.  That isn't the behavior I would have
> intuitively expected, but that's probably more of a call for the guys
> working on the userspace code that actually uses this functionality.
> Just thought I'd make a note of it since it seemed a bit surprising to
> me.
>
Actually, the way to disable gamma/degamma is to pass num_samples=0 in 
the blob. The same is agreed when we had this design discussion. Do you 
think it makes sense now ?
> (btw, I think all the comments on this patch also apply to patches #11
> and 14)
>
Ok, will take care of that.
> Matt
>
>> +}
>> +
>>   int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
>>   		struct drm_crtc_state *crtc_state,
>>   		struct drm_mode_object *obj, uint32_t blob_id)
>> @@ -99,5 +242,8 @@ void intel_attach_color_properties_to_crtc(struct drm_device *dev,
>>   			drm_object_attach_property(mode_obj,
>>   				config->cm_crtc_palette_capabilities_property,
>>   				capabilities_blob_id);
>> +		if (config->cm_palette_after_ctm_property)
>> +			drm_object_attach_property(mode_obj,
>> +				config->cm_palette_after_ctm_property, 0);
>>   	}
>>   }
>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
>> index 51aeb91..8bbac20 100644
>> --- a/drivers/gpu/drm/i915/intel_color_manager.h
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
>> @@ -28,6 +28,26 @@
>>   #include <drm/drm_crtc_helper.h>
>>   #include "i915_drv.h"
>>
>> +#define _GAMMA_INT_PART(value)			(value >> GAMMA_INT_SHIFT)
>> +#define _GAMMA_FRACT_PART(value)		(value >> GAMMA_FRACT_SHIFT)
>> +
>>   #define CHV_PALETTE_STRUCT_VERSION		1
>>   #define CHV_DEGAMMA_MAX_VALS			65
>>   #define CHV_10BIT_GAMMA_MAX_VALS		257
>> +
>> +/* Gamma correction */
>> +#define CHV_GAMMA_DATA_STRUCT_VERSION		1
>> +#define CHV_10BIT_GAMMA_MAX_VALS		257
>> +#define CHV_8BIT_GAMMA_MAX_VALS			256
>> +#define CHV_10BIT_GAMMA_MSB_SHIFT		6
>> +#define CHV_GAMMA_SHIFT_GREEN			16
>> +/* Gamma values are u8.24 format */
>> +#define GAMMA_INT_SHIFT				24
>> +#define GAMMA_FRACT_SHIFT			8
>> +/* Max int value for Gamma is 1 */
>> +#define GAMMA_INT_MAX				1
>> +/* Max value for Gamma on CHV */
>> +#define CHV_MAX_GAMMA				0x10000
>> +
>> +/* CHV CGM Block */
>> +#define CGM_GAMMA_EN				(1 << 2)
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 349a1c2..9d1a6c3 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13641,6 +13641,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>>
>>   	if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
>>   		skl_detach_scalers(intel_crtc);
>> +
>> +	intel_color_manager_crtc_commit(dev, crtc->state);
>>   }
>>
>>   static void intel_finish_crtc_commit(struct drm_crtc *crtc,
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 820ded7..de3e6e7 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1444,5 +1444,7 @@ void intel_attach_color_properties_to_crtc(struct drm_device *dev,
>>   int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
>>   		struct drm_crtc_state *crtc_state,
>>   		struct drm_mode_object *obj, uint32_t blob_id);
>> +void intel_color_manager_crtc_commit(struct drm_device *dev,
>> +		struct drm_crtc_state *crtc_state);
>>
>>   #endif /* __INTEL_DRV_H__ */
>> --
>> 1.9.1
>>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 10/18] drm/i915: Add pipe deGamma correction handlers

[Shashank Sharma]

From: Kausal Malladi <kausalmalladi@gmail.com>

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

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

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

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 9f55e6c..1d8cb09 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -331,6 +331,10 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
 		return intel_color_manager_set_pipe_gamma(dev, state,
 				&crtc->base, val);
 
+	if (property == config->cm_palette_before_ctm_property)
+		return intel_color_manager_set_pipe_degamma(dev, state,
+				&crtc->base, val);
+
 	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
 	return -EINVAL;
 }
@@ -346,6 +350,9 @@ int intel_crtc_atomic_get_property(struct drm_crtc *crtc,
 	if (property == config->cm_palette_after_ctm_property)
 		*val = (state->palette_after_ctm_blob) ?
 			state->palette_after_ctm_blob->base.id : 0;
+	if (property == config->cm_palette_before_ctm_property)
+		*val = (state->palette_before_ctm_blob) ?
+			state->palette_before_ctm_blob->base.id : 0;
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
index f8c8d26..5fc8e41 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.c
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -170,6 +170,25 @@ void intel_color_manager_crtc_commit(struct drm_device *dev,
 	}
 }
 
+int intel_color_manager_set_pipe_degamma(struct drm_device *dev,
+		struct drm_crtc_state *crtc_state,
+		struct drm_mode_object *obj, uint32_t blob_id)
+{
+	struct drm_property_blob *blob;
+
+	blob = drm_property_lookup_blob(dev, blob_id);
+	if (!blob) {
+		DRM_ERROR("Invalid Blob ID\n");
+		return -EINVAL;
+	}
+
+	if (crtc_state->palette_before_ctm_blob)
+		drm_property_unreference_blob(crtc_state->palette_before_ctm_blob);
+
+	crtc_state->palette_before_ctm_blob = blob;
+	return 0;
+}
+
 int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
 		struct drm_crtc_state *crtc_state,
 		struct drm_mode_object *obj, uint32_t blob_id)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index de3e6e7..d3b42ec 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1444,6 +1444,9 @@ void intel_attach_color_properties_to_crtc(struct drm_device *dev,
 int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
 		struct drm_crtc_state *crtc_state,
 		struct drm_mode_object *obj, uint32_t blob_id);
+int intel_color_manager_set_pipe_degamma(struct drm_device *dev,
+		struct drm_crtc_state *crtc_state,
+		struct drm_mode_object *obj, uint32_t blob_id);
 void intel_color_manager_crtc_commit(struct drm_device *dev,
 		struct drm_crtc_state *crtc_state);
 
-- 
1.9.1

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

[PATCH 11/18] drm/i915: Add DeGamma correction for CHV/BSW

[Shashank Sharma]

From: Kausal Malladi <kausalmalladi@gmail.com>

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

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

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

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 4997ebd..523aad9 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7928,9 +7928,14 @@ enum skl_disp_power_wells {
 #define PIPEA_CGM_GAMMA				(VLV_DISPLAY_BASE + 0x67000)
 #define PIPEB_CGM_GAMMA				(VLV_DISPLAY_BASE + 0x69000)
 #define PIPEC_CGM_GAMMA				(VLV_DISPLAY_BASE + 0x6B000)
+#define PIPEA_CGM_DEGAMMA			(VLV_DISPLAY_BASE + 0x66000)
+#define PIPEB_CGM_DEGAMMA			(VLV_DISPLAY_BASE + 0x68000)
+#define PIPEC_CGM_DEGAMMA			(VLV_DISPLAY_BASE + 0x6A000)
 #define _PIPE_CGM_CONTROL(pipe) \
 	(_PIPE3(pipe, PIPEA_CGM_CONTROL, PIPEB_CGM_CONTROL, PIPEC_CGM_CONTROL))
 #define _PIPE_GAMMA_BASE(pipe) \
 	(_PIPE3(pipe, PIPEA_CGM_GAMMA, PIPEB_CGM_GAMMA, PIPEC_CGM_GAMMA))
+#define _PIPE_DEGAMMA_BASE(pipe) \
+	(_PIPE3(pipe, PIPEA_CGM_DEGAMMA, PIPEB_CGM_DEGAMMA, PIPEC_CGM_DEGAMMA))
 
 #endif /* _I915_REG_H_ */
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
index 5fc8e41..ae92825 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.c
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -27,6 +27,111 @@
 
 #include "intel_color_manager.h"
 
+int chv_set_degamma(struct drm_device *dev, struct drm_property_blob *blob,
+		struct drm_crtc *crtc)
+{
+	struct drm_palette *degamma_data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 cgm_control_reg = 0;
+	u32 cgm_degamma_reg = 0;
+	enum pipe pipe;
+	u32 red, green, blue;
+	u8 red_int, green_int, blue_int;
+	u16 red_fract, green_fract, blue_fract;
+	u32 count = 0;
+	struct drm_r32g32b32 *correction_values = NULL;
+	u32 num_samples;
+	u32 word;
+	int ret = 0, length;
+
+	if (!blob) {
+		DRM_ERROR("NULL Blob\n");
+		return -EINVAL;
+	}
+
+	degamma_data = (struct drm_palette *)blob->data;
+
+	if (degamma_data->version != CHV_DEGAMMA_DATA_STRUCT_VERSION) {
+		DRM_ERROR("Invalid DeGamma Data struct version\n");
+		return -EINVAL;
+	}
+
+	pipe = to_intel_crtc(crtc)->pipe;
+	num_samples = degamma_data->num_samples;
+	length = num_samples * sizeof(struct drm_r32g32b32);
+
+	if (num_samples == 0) {
+
+		/* Disable DeGamma functionality on Pipe - CGM Block */
+		cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe));
+		cgm_control_reg &= ~CGM_DEGAMMA_EN;
+		I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg);
+
+		DRM_DEBUG_DRIVER("DeGamma disabled on Pipe %c\n",
+				pipe_name(pipe));
+		ret = 0;
+	} else if (num_samples == CHV_DEGAMMA_MAX_VALS) {
+		cgm_degamma_reg = _PIPE_DEGAMMA_BASE(pipe);
+
+		count = 0;
+		correction_values = (struct drm_r32g32b32 *)&degamma_data->lut;
+		while (count < CHV_DEGAMMA_MAX_VALS) {
+			blue = correction_values[count].b32;
+			green = correction_values[count].g32;
+			red = correction_values[count].r32;
+
+			blue_int = _GAMMA_INT_PART(blue);
+			if (blue_int > GAMMA_INT_MAX)
+				blue = CHV_MAX_GAMMA;
+			green_int = _GAMMA_INT_PART(green);
+			if (green_int > GAMMA_INT_MAX)
+				green = CHV_MAX_GAMMA;
+			red_int = _GAMMA_INT_PART(red);
+			if (red_int > GAMMA_INT_MAX)
+				red = CHV_MAX_GAMMA;
+
+			blue_fract = _GAMMA_FRACT_PART(blue);
+			green_fract = _GAMMA_FRACT_PART(green);
+			red_fract = _GAMMA_FRACT_PART(red);
+
+			blue_fract >>= CHV_DEGAMMA_MSB_SHIFT;
+			green_fract >>= CHV_DEGAMMA_MSB_SHIFT;
+			red_fract >>= CHV_DEGAMMA_MSB_SHIFT;
+
+			/* Green (29:16) and Blue (13:0) in DWORD1 */
+			word = green_fract;
+			word <<= CHV_DEGAMMA_GREEN_SHIFT;
+			word = word | blue;
+			I915_WRITE(cgm_degamma_reg, word);
+
+			cgm_degamma_reg += 4;
+
+			/* Red (13:0) to be written to DWORD2 */
+			word = red_fract;
+			I915_WRITE(cgm_degamma_reg, word);
+
+			cgm_degamma_reg += 4;
+			count++;
+		}
+
+		DRM_DEBUG_DRIVER("DeGamma LUT loaded for Pipe %c\n",
+				pipe_name(pipe));
+
+		/* Enable DeGamma on Pipe */
+		I915_WRITE(_PIPE_CGM_CONTROL(pipe),
+			I915_READ(_PIPE_CGM_CONTROL(pipe)) | CGM_DEGAMMA_EN);
+
+		DRM_DEBUG_DRIVER("DeGamma correction enabled on Pipe %c\n",
+				pipe_name(pipe));
+		ret = 0;
+	} else {
+		DRM_ERROR("Invalid number of samples for DeGamma LUT\n");
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
 int chv_set_gamma(struct drm_device *dev, struct drm_property_blob *blob,
 		  struct drm_crtc *crtc)
 {
@@ -168,6 +273,18 @@ void intel_color_manager_crtc_commit(struct drm_device *dev,
 		else
 			DRM_DEBUG_DRIVER("Gamma correction success\n");
 	}
+
+	blob = crtc_state->palette_before_ctm_blob;
+	if (blob) {
+		/* degamma correction */
+		if (IS_CHERRYVIEW(dev))
+			ret = chv_set_degamma(dev, blob, crtc);
+
+		if (ret)
+			DRM_ERROR("set degamma correction failed\n");
+		else
+			DRM_DEBUG_DRIVER("degamma correction success\n");
+	}
 }
 
 int intel_color_manager_set_pipe_degamma(struct drm_device *dev,
@@ -264,5 +381,8 @@ void intel_attach_color_properties_to_crtc(struct drm_device *dev,
 		if (config->cm_palette_after_ctm_property)
 			drm_object_attach_property(mode_obj,
 				config->cm_palette_after_ctm_property, 0);
+		if (config->cm_palette_before_ctm_property)
+			drm_object_attach_property(mode_obj,
+				config->cm_palette_before_ctm_property, 0);
 	}
 }
diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
index 8bbac20..6a4fff2 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.h
+++ b/drivers/gpu/drm/i915/intel_color_manager.h
@@ -49,5 +49,11 @@
 /* Max value for Gamma on CHV */
 #define CHV_MAX_GAMMA				0x10000
 
+/* DeGamma correction */
+#define CHV_DEGAMMA_DATA_STRUCT_VERSION		1
+#define CHV_DEGAMMA_MSB_SHIFT			2
+#define CHV_DEGAMMA_GREEN_SHIFT			16
+
 /* CHV CGM Block */
 #define CGM_GAMMA_EN				(1 << 2)
+#define CGM_DEGAMMA_EN				(1 << 0)
-- 
1.9.1

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

[PATCH 12/18] drm: Add structure for set/get a CTM color property

[Shashank Sharma]

From: Kausal Malladi <kausalmalladi@gmail.com>

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

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

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

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

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

[PATCH 13/18] drm/i915: Add set/get property handlers for CSC correction

[Shashank Sharma]

From: Kausal Malladi <kausalmalladi@gmail.com>

This patch adds set and get property handlers for CSC color correction
and enhancement capability at Pipe level on CHV/BSW platform. The set
function just attaches the CSC blob to CRTC state, that later
gets committed using atomic path.

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

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 1d8cb09..a6f0b71 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -334,6 +334,9 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
 	if (property == config->cm_palette_before_ctm_property)
 		return intel_color_manager_set_pipe_degamma(dev, state,
 				&crtc->base, val);
+	if (property == config->cm_ctm_property)
+		return intel_color_manager_set_pipe_csc(dev, state,
+				&crtc->base, val);
 
 	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
 	return -EINVAL;
@@ -353,6 +356,8 @@ int intel_crtc_atomic_get_property(struct drm_crtc *crtc,
 	if (property == config->cm_palette_before_ctm_property)
 		*val = (state->palette_before_ctm_blob) ?
 			state->palette_before_ctm_blob->base.id : 0;
+	if (property == config->cm_ctm_property)
+		*val = (state->ctm_blob) ? state->ctm_blob->base.id : 0;
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
index ae92825..3eea857 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.c
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -287,6 +287,25 @@ void intel_color_manager_crtc_commit(struct drm_device *dev,
 	}
 }
 
+int intel_color_manager_set_pipe_csc(struct drm_device *dev,
+		struct drm_crtc_state *crtc_state,
+		struct drm_mode_object *obj, uint32_t blob_id)
+{
+	struct drm_property_blob *blob;
+
+	blob = drm_property_lookup_blob(dev, blob_id);
+	if (!blob) {
+		DRM_ERROR("Invalid Blob ID\n");
+		return -EINVAL;
+	}
+
+	if (crtc_state->ctm_blob)
+		drm_property_unreference_blob(crtc_state->ctm_blob);
+
+	crtc_state->ctm_blob = blob;
+	return 0;
+}
+
 int intel_color_manager_set_pipe_degamma(struct drm_device *dev,
 		struct drm_crtc_state *crtc_state,
 		struct drm_mode_object *obj, uint32_t blob_id)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d3b42ec..cb80e81 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1447,6 +1447,9 @@ int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
 int intel_color_manager_set_pipe_degamma(struct drm_device *dev,
 		struct drm_crtc_state *crtc_state,
 		struct drm_mode_object *obj, uint32_t blob_id);
+int intel_color_manager_set_pipe_csc(struct drm_device *dev,
+		struct drm_crtc_state *crtc_state,
+		struct drm_mode_object *obj, uint32_t blob_id);
 void intel_color_manager_crtc_commit(struct drm_device *dev,
 		struct drm_crtc_state *crtc_state);
 
-- 
1.9.1

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

[PATCH 14/18] drm/i915: Add CSC correction for CHV/BSW

[Shashank Sharma]

From: Kausal Malladi <kausalmalladi@gmail.com>

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

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

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

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 523aad9..9ce259e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7931,11 +7931,16 @@ enum skl_disp_power_wells {
 #define PIPEA_CGM_DEGAMMA			(VLV_DISPLAY_BASE + 0x66000)
 #define PIPEB_CGM_DEGAMMA			(VLV_DISPLAY_BASE + 0x68000)
 #define PIPEC_CGM_DEGAMMA			(VLV_DISPLAY_BASE + 0x6A000)
+#define PIPEA_CGM_CSC				(VLV_DISPLAY_BASE + 0x67900)
+#define PIPEB_CGM_CSC				(VLV_DISPLAY_BASE + 0x69900)
+#define PIPEC_CGM_CSC				(VLV_DISPLAY_BASE + 0x6B900)
 #define _PIPE_CGM_CONTROL(pipe) \
 	(_PIPE3(pipe, PIPEA_CGM_CONTROL, PIPEB_CGM_CONTROL, PIPEC_CGM_CONTROL))
 #define _PIPE_GAMMA_BASE(pipe) \
 	(_PIPE3(pipe, PIPEA_CGM_GAMMA, PIPEB_CGM_GAMMA, PIPEC_CGM_GAMMA))
 #define _PIPE_DEGAMMA_BASE(pipe) \
 	(_PIPE3(pipe, PIPEA_CGM_DEGAMMA, PIPEB_CGM_DEGAMMA, PIPEC_CGM_DEGAMMA))
+#define _PIPE_CSC_BASE(pipe) \
+	(_PIPE3(pipe, PIPEA_CGM_CSC, PIPEB_CGM_CSC, PIPEC_CGM_CSC))
 
 #endif /* _I915_REG_H_ */
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
index 3eea857..5fa575b 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.c
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -27,6 +27,99 @@
 
 #include "intel_color_manager.h"
 
+s16 get_csc_s3_12_format(s64 csc_value)
+{
+	s32 csc_int_value;
+	u32 csc_fract_value;
+	s16 csc_s3_12_format;
+
+	if (csc_value >= 0) {
+		csc_value += CHV_CSC_FRACT_ROUNDOFF;
+		if (csc_value > CHV_CSC_COEFF_MAX)
+			csc_value = CHV_CSC_COEFF_MAX;
+	} else {
+		csc_value = -csc_value;
+		csc_value += CHV_CSC_FRACT_ROUNDOFF;
+		if (csc_value > CHV_CSC_COEFF_MAX + 1)
+			csc_value = CHV_CSC_COEFF_MAX + 1;
+		csc_value = -csc_value;
+	}
+
+	csc_int_value = csc_value >> CHV_CSC_COEFF_SHIFT;
+	csc_int_value <<= CHV_CSC_COEFF_INT_SHIFT;
+	if (csc_value < 0)
+		csc_int_value |= CSC_COEFF_SIGN;
+	csc_fract_value = csc_value;
+	csc_fract_value >>= CHV_CSC_COEFF_FRACT_SHIFT;
+	csc_s3_12_format = csc_int_value | csc_fract_value;
+
+	return csc_s3_12_format;
+}
+
+int chv_set_csc(struct drm_device *dev, struct drm_property_blob *blob,
+		struct drm_crtc *crtc)
+{
+	struct drm_ctm *csc_data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 reg;
+	enum pipe pipe;
+	s32 word, temp;
+	int count = 0;
+
+	if (!blob) {
+		DRM_ERROR("NULL Blob\n");
+		return -EINVAL;
+	}
+
+	if (blob->length != sizeof(struct drm_ctm)) {
+		DRM_ERROR("Invalid length of data received\n");
+		return -EINVAL;
+	}
+
+	csc_data = (struct drm_ctm *)blob->data;
+	if (csc_data->version != CHV_CSC_DATA_STRUCT_VERSION) {
+		DRM_ERROR("Invalid CSC Data struct version\n");
+		return -EINVAL;
+	}
+
+	pipe = to_intel_crtc(crtc)->pipe;
+
+	/* Disable CSC functionality */
+	reg = _PIPE_CGM_CONTROL(pipe);
+	I915_WRITE(reg, I915_READ(reg) & (~CGM_CSC_EN));
+
+	DRM_DEBUG_DRIVER("Disabled CSC Functionality on Pipe %c\n",
+			pipe_name(pipe));
+
+	reg = _PIPE_CSC_BASE(pipe);
+	while (count < CSC_MAX_VALS) {
+		word = get_csc_s3_12_format(csc_data->ctm_coeff[count]);
+
+		/*
+		 * Last value to be written in 1 register.
+		 * Otherwise, each pair of CSC values go
+		 * into 1 register
+		 */
+		if (count != (CSC_MAX_VALS - 1)) {
+			count++;
+			temp = get_csc_s3_12_format(csc_data->ctm_coeff[count]);
+			word |= temp;
+		}
+		I915_WRITE(reg, word);
+		reg += 4;
+		count++;
+	}
+
+	DRM_DEBUG_DRIVER("All CSC values written to registers\n");
+
+	/* Enable CSC functionality */
+	reg = _PIPE_CGM_CONTROL(pipe);
+	I915_WRITE(reg, I915_READ(reg) | CGM_CSC_EN);
+	DRM_DEBUG_DRIVER("CSC enabled on Pipe %c\n", pipe_name(pipe));
+
+	return 0;
+}
+
 int chv_set_degamma(struct drm_device *dev, struct drm_property_blob *blob,
 		struct drm_crtc *crtc)
 {
@@ -285,6 +378,18 @@ void intel_color_manager_crtc_commit(struct drm_device *dev,
 		else
 			DRM_DEBUG_DRIVER("degamma correction success\n");
 	}
+
+	blob = crtc_state->ctm_blob;
+	if (blob) {
+		/* CSC correction */
+		if (IS_CHERRYVIEW(dev))
+			ret = chv_set_csc(dev, blob, crtc);
+
+		if (ret)
+			DRM_ERROR("set CSC correction failed\n");
+		else
+			DRM_DEBUG_DRIVER("CSC correction success\n");
+	}
 }
 
 int intel_color_manager_set_pipe_csc(struct drm_device *dev,
@@ -403,5 +508,8 @@ void intel_attach_color_properties_to_crtc(struct drm_device *dev,
 		if (config->cm_palette_before_ctm_property)
 			drm_object_attach_property(mode_obj,
 				config->cm_palette_before_ctm_property, 0);
+		if (config->cm_ctm_property)
+			drm_object_attach_property(mode_obj,
+				config->cm_ctm_property, 0);
 	}
 }
diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
index 6a4fff2..b2ee847 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.h
+++ b/drivers/gpu/drm/i915/intel_color_manager.h
@@ -54,6 +54,26 @@
 #define CHV_DEGAMMA_MSB_SHIFT			2
 #define CHV_DEGAMMA_GREEN_SHIFT			16
 
+/* CSC correction */
+#define CHV_CSC_DATA_STRUCT_VERSION		1
+/*
+ * Fractional part is 32 bit, and we need only 12 MSBs for programming
+ * into registers. ROUNDOFF is required to minimize loss of precision.
+ */
+#define CHV_CSC_FRACT_ROUNDOFF			(1 << 19)
+/*
+ * CSC values are 64-bit values. For CHV, the maximum CSC value that
+ * user can program is 7.99999..., which can be represented in fixed point
+ * S31.32 format like this, with all fractional bits as 1
+ */
+#define CHV_CSC_COEFF_MAX			0x00000007FFFFFFFF
+#define CHV_CSC_COEFF_SHIFT			32
+#define CHV_CSC_COEFF_INT_SHIFT			28
+#define CSC_COEFF_SIGN				(1 << 31)
+#define CHV_CSC_COEFF_FRACT_SHIFT		20
+#define CSC_MAX_VALS				9
+
 /* CHV CGM Block */
 #define CGM_GAMMA_EN				(1 << 2)
+#define CGM_CSC_EN				(1 << 1)
 #define CGM_DEGAMMA_EN				(1 << 0)
-- 
1.9.1

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

[PATCH 15/18] drm/i915: Initialize Gen8 pipe gamma correction

[Shashank Sharma]

From: Kausal Malladi <kausalmalladi@gmail.com>

This patch initializes gamma color correction proeprty
for Gen8 and higher platforms.

It does the following :
1. Load pipe Gamma color correction capabilities for BDW/SKL/BXT
2. Attach the color properties to CRTC

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

diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
index 5fa575b..bc77ab5 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.c
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -475,11 +475,39 @@ int get_chv_pipe_gamma_capabilities(struct drm_device *dev,
 	return 0;
 }
 
+int get_gen9_pipe_gamma_capabilities(struct drm_device *dev,
+		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
+{
+	struct drm_property_blob *blob = NULL;
+
+	/*
+	 * This function exposes best capability for DeGamma and Gamma
+	 * For BXT, the DeGamma LUT has 512 entries
+	 * and the best Gamma capability has 512 entries
+	 */
+	palette_caps->version = GEN9_PALETTE_STRUCT_VERSION;
+	palette_caps->num_samples_before_ctm =
+		GEN9_SPLITGAMMA_MAX_VALS;
+	palette_caps->num_samples_after_ctm =
+		GEN9_SPLITGAMMA_MAX_VALS;
+
+	blob = drm_property_create_blob(dev, sizeof(struct drm_palette_caps),
+			(const void *) palette_caps);
+
+	if (blob)
+		return blob->base.id;
+
+	return 0;
+}
+
 int get_pipe_gamma_capabilities(struct drm_device *dev,
 		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
 {
 	if (IS_CHERRYVIEW(dev))
 		return get_chv_pipe_gamma_capabilities(dev, palette_caps, crtc);
+	if (IS_BROADWELL(dev) || IS_GEN9(dev))
+		return get_gen9_pipe_gamma_capabilities(dev,
+				palette_caps, crtc);
 	return -EINVAL;
 }
 
@@ -491,7 +519,7 @@ void intel_attach_color_properties_to_crtc(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	int capabilities_blob_id;
 
-	if (IS_CHERRYVIEW(dev)) {
+	if (IS_CHERRYVIEW(dev) || IS_BROADWELL(dev) || IS_GEN9(dev)) {
 		crtc = obj_to_crtc(mode_obj);
 
 		palette_caps = kzalloc(sizeof(struct drm_palette_caps),
diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
index b2ee847..78de1a2 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.h
+++ b/drivers/gpu/drm/i915/intel_color_manager.h
@@ -35,6 +35,9 @@
 #define CHV_DEGAMMA_MAX_VALS			65
 #define CHV_10BIT_GAMMA_MAX_VALS		257
 
+#define GEN9_PALETTE_STRUCT_VERSION		1
+#define GEN9_SPLITGAMMA_MAX_VALS		512
+
 /* Gamma correction */
 #define CHV_GAMMA_DATA_STRUCT_VERSION		1
 #define CHV_10BIT_GAMMA_MAX_VALS		257
-- 
1.9.1

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

Re: [PATCH 15/18] drm/i915: Initialize Gen8 pipe gamma correction

[Matt Roper]

On Thu, Aug 06, 2015 at 10:08:24PM +0530, Shashank Sharma wrote:
> From: Kausal Malladi <kausalmalladi@gmail.com>
> 
> This patch initializes gamma color correction proeprty
                                                ^^^^^^^^
                                                typo
> for Gen8 and higher platforms.

I'd specifically say 'BDW and Gen9' here since we already have some gen8
support (CHV).

> 
> It does the following :
> 1. Load pipe Gamma color correction capabilities for BDW/SKL/BXT
> 2. Attach the color properties to CRTC
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_color_manager.c | 30 +++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_color_manager.h |  3 +++
>  2 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
> index 5fa575b..bc77ab5 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.c
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -475,11 +475,39 @@ int get_chv_pipe_gamma_capabilities(struct drm_device *dev,
>  	return 0;
>  }
>  
> +int get_gen9_pipe_gamma_capabilities(struct drm_device *dev,
> +		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)

Calling this 'gen9' seems a little confusing to me given that it's also
used for BDW, which is a gen8 platform.  The general pattern is that
functions get named after the first platform that works a specific way,
so I'd expect this to be called "get_bdw_pipe_gamma_capabilities."

> +{
> +	struct drm_property_blob *blob = NULL;
> +
> +	/*
> +	 * This function exposes best capability for DeGamma and Gamma
> +	 * For BXT, the DeGamma LUT has 512 entries
> +	 * and the best Gamma capability has 512 entries
> +	 */
> +	palette_caps->version = GEN9_PALETTE_STRUCT_VERSION;
> +	palette_caps->num_samples_before_ctm =
> +		GEN9_SPLITGAMMA_MAX_VALS;
> +	palette_caps->num_samples_after_ctm =
> +		GEN9_SPLITGAMMA_MAX_VALS;
> +
> +	blob = drm_property_create_blob(dev, sizeof(struct drm_palette_caps),
> +			(const void *) palette_caps);

We're pretty much doing the same thing we did for CHV, but just filling
in different values.  Could we just stick the number of samples in
INTEL_INFO(dev)->num_gamma_samples_{before/after}_ctm instead and then
have a single function that fills out your capability blob (or at least
the part of it that we have today) across all platforms?  Or is this
something that we think might actually start to vary across the
different pipes of a single platform in the future?

> +
> +	if (blob)
> +		return blob->base.id;
> +
> +	return 0;
> +}
> +
>  int get_pipe_gamma_capabilities(struct drm_device *dev,
>  		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
>  {
>  	if (IS_CHERRYVIEW(dev))
>  		return get_chv_pipe_gamma_capabilities(dev, palette_caps, crtc);
> +	if (IS_BROADWELL(dev) || IS_GEN9(dev))
> +		return get_gen9_pipe_gamma_capabilities(dev,
> +				palette_caps, crtc);
>  	return -EINVAL;
>  }
>  
> @@ -491,7 +519,7 @@ void intel_attach_color_properties_to_crtc(struct drm_device *dev,
>  	struct drm_crtc *crtc;
>  	int capabilities_blob_id;
>  
> -	if (IS_CHERRYVIEW(dev)) {
> +	if (IS_CHERRYVIEW(dev) || IS_BROADWELL(dev) || IS_GEN9(dev)) {

'IS_CHERRYVIEW(dev) || IS_BROADWELL(dev)' could be simplified to just
'IS_GEN8(dev)' right?


Matt


>  		crtc = obj_to_crtc(mode_obj);
>  
>  		palette_caps = kzalloc(sizeof(struct drm_palette_caps),
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
> index b2ee847..78de1a2 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.h
> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
> @@ -35,6 +35,9 @@
>  #define CHV_DEGAMMA_MAX_VALS			65
>  #define CHV_10BIT_GAMMA_MAX_VALS		257
>  
> +#define GEN9_PALETTE_STRUCT_VERSION		1
> +#define GEN9_SPLITGAMMA_MAX_VALS		512
> +
>  /* Gamma correction */
>  #define CHV_GAMMA_DATA_STRUCT_VERSION		1
>  #define CHV_10BIT_GAMMA_MAX_VALS		257
> -- 
> 1.9.1
> 

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

Re: [PATCH 15/18] drm/i915: Initialize Gen8 pipe gamma correction

[Sharma, Shashank]

Regards
Shashank

On 8/22/2015 4:11 AM, Matt Roper wrote:
> On Thu, Aug 06, 2015 at 10:08:24PM +0530, Shashank Sharma wrote:
>> From: Kausal Malladi <kausalmalladi@gmail.com>
>>
>> This patch initializes gamma color correction proeprty
>                                                  ^^^^^^^^
>                                                  typo
Oops !
>> for Gen8 and higher platforms.
>
> I'd specifically say 'BDW and Gen9' here since we already have some gen8
> support (CHV).
>
Agree. Will change it.
>>
>> It does the following :
>> 1. Load pipe Gamma color correction capabilities for BDW/SKL/BXT
>> 2. Attach the color properties to CRTC
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_color_manager.c | 30 +++++++++++++++++++++++++++++-
>>   drivers/gpu/drm/i915/intel_color_manager.h |  3 +++
>>   2 files changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
>> index 5fa575b..bc77ab5 100644
>> --- a/drivers/gpu/drm/i915/intel_color_manager.c
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
>> @@ -475,11 +475,39 @@ int get_chv_pipe_gamma_capabilities(struct drm_device *dev,
>>   	return 0;
>>   }
>>
>> +int get_gen9_pipe_gamma_capabilities(struct drm_device *dev,
>> +		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
>
> Calling this 'gen9' seems a little confusing to me given that it's also
> used for BDW, which is a gen8 platform.  The general pattern is that
> functions get named after the first platform that works a specific way,
> so I'd expect this to be called "get_bdw_pipe_gamma_capabilities."
>
Yes, its a mistake. I will fix this and will be more specific.
>> +{
>> +	struct drm_property_blob *blob = NULL;
>> +
>> +	/*
>> +	 * This function exposes best capability for DeGamma and Gamma
>> +	 * For BXT, the DeGamma LUT has 512 entries
>> +	 * and the best Gamma capability has 512 entries
>> +	 */
>> +	palette_caps->version = GEN9_PALETTE_STRUCT_VERSION;
>> +	palette_caps->num_samples_before_ctm =
>> +		GEN9_SPLITGAMMA_MAX_VALS;
>> +	palette_caps->num_samples_after_ctm =
>> +		GEN9_SPLITGAMMA_MAX_VALS;
>> +
>> +	blob = drm_property_create_blob(dev, sizeof(struct drm_palette_caps),
>> +			(const void *) palette_caps);
>
> We're pretty much doing the same thing we did for CHV, but just filling
> in different values.  Could we just stick the number of samples in
> INTEL_INFO(dev)->num_gamma_samples_{before/after}_ctm instead and then
> have a single function that fills out your capability blob (or at least
> the part of it that we have today) across all platforms?  Or is this
> something that we think might actually start to vary across the
> different pipes of a single platform in the future?
>
Thanks, that's pretty good suggestion. Will do that.
>> +
>> +	if (blob)
>> +		return blob->base.id;
>> +
>> +	return 0;
>> +}
>> +
>>   int get_pipe_gamma_capabilities(struct drm_device *dev,
>>   		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
>>   {
>>   	if (IS_CHERRYVIEW(dev))
>>   		return get_chv_pipe_gamma_capabilities(dev, palette_caps, crtc);
>> +	if (IS_BROADWELL(dev) || IS_GEN9(dev))
>> +		return get_gen9_pipe_gamma_capabilities(dev,
>> +				palette_caps, crtc);
>>   	return -EINVAL;
>>   }
>>
>> @@ -491,7 +519,7 @@ void intel_attach_color_properties_to_crtc(struct drm_device *dev,
>>   	struct drm_crtc *crtc;
>>   	int capabilities_blob_id;
>>
>> -	if (IS_CHERRYVIEW(dev)) {
>> +	if (IS_CHERRYVIEW(dev) || IS_BROADWELL(dev) || IS_GEN9(dev)) {
>
> 'IS_CHERRYVIEW(dev) || IS_BROADWELL(dev)' could be simplified to just
> 'IS_GEN8(dev)' right?
>
yep, will do it.
>
> Matt
>
>
>>   		crtc = obj_to_crtc(mode_obj);
>>
>>   		palette_caps = kzalloc(sizeof(struct drm_palette_caps),
>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
>> index b2ee847..78de1a2 100644
>> --- a/drivers/gpu/drm/i915/intel_color_manager.h
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
>> @@ -35,6 +35,9 @@
>>   #define CHV_DEGAMMA_MAX_VALS			65
>>   #define CHV_10BIT_GAMMA_MAX_VALS		257
>>
>> +#define GEN9_PALETTE_STRUCT_VERSION		1
>> +#define GEN9_SPLITGAMMA_MAX_VALS		512
>> +
>>   /* Gamma correction */
>>   #define CHV_GAMMA_DATA_STRUCT_VERSION		1
>>   #define CHV_10BIT_GAMMA_MAX_VALS		257
>> --
>> 1.9.1
>>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 16/18] drm/i915: Gen8 pipe level Gamma correction

[Shashank Sharma]

From: Kausal Malladi <kausalmalladi@gmail.com>

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

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

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

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 9ce259e..92233ce 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5591,7 +5591,9 @@ enum skl_disp_power_wells {
 
 #define _GAMMA_MODE_A		0x4a480
 #define _GAMMA_MODE_B		0x4ac80
-#define GAMMA_MODE(pipe) _PIPE(pipe, _GAMMA_MODE_A, _GAMMA_MODE_B)
+#define _GAMMA_MODE_C		0x4b480
+#define GAMMA_MODE(pipe) \
+	_PIPE3(pipe, _GAMMA_MODE_A, _GAMMA_MODE_B, _GAMMA_MODE_C)
 #define GAMMA_MODE_MODE_MASK	(3 << 0)
 #define GAMMA_MODE_MODE_8BIT	(0 << 0)
 #define GAMMA_MODE_MODE_10BIT	(1 << 0)
@@ -7934,6 +7936,14 @@ enum skl_disp_power_wells {
 #define PIPEA_CGM_CSC				(VLV_DISPLAY_BASE + 0x67900)
 #define PIPEB_CGM_CSC				(VLV_DISPLAY_BASE + 0x69900)
 #define PIPEC_CGM_CSC				(VLV_DISPLAY_BASE + 0x6B900)
+
+#define PAL_PREC_INDEX_A			0x4A400
+#define PAL_PREC_INDEX_B			0x4AC00
+#define PAL_PREC_INDEX_C			0x4B400
+#define PAL_PREC_DATA_A				0x4A404
+#define PAL_PREC_DATA_B				0x4AC04
+#define PAL_PREC_DATA_C				0x4B404
+
 #define _PIPE_CGM_CONTROL(pipe) \
 	(_PIPE3(pipe, PIPEA_CGM_CONTROL, PIPEB_CGM_CONTROL, PIPEC_CGM_CONTROL))
 #define _PIPE_GAMMA_BASE(pipe) \
@@ -7943,4 +7953,9 @@ enum skl_disp_power_wells {
 #define _PIPE_CSC_BASE(pipe) \
 	(_PIPE3(pipe, PIPEA_CGM_CSC, PIPEB_CGM_CSC, PIPEC_CGM_CSC))
 
+#define _PREC_PAL_INDEX(pipe) \
+	(_PIPE3(pipe, PAL_PREC_INDEX_A, PAL_PREC_INDEX_B, PAL_PREC_INDEX_C))
+#define _PREC_PAL_DATA(pipe) \
+	(_PIPE3(pipe, PAL_PREC_DATA_A, PAL_PREC_DATA_B, PAL_PREC_DATA_C))
+
 #endif /* _I915_REG_H_ */
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
index bc77ab5..a894f4c 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.c
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -120,6 +120,40 @@ int chv_set_csc(struct drm_device *dev, struct drm_property_blob *blob,
 	return 0;
 }
 
+u32 gen9_write_10bit_gamma_precision(u32 red, u32 green, u32 blue)
+{
+	u32 word;
+	u8 blue_int, green_int, red_int;
+	u16 blue_fract, green_fract, red_fract;
+
+	blue_int = _GAMMA_INT_PART(blue);
+	if (blue_int > GAMMA_INT_MAX)
+		blue = GEN9_MAX_GAMMA;
+	green_int = _GAMMA_INT_PART(green);
+	if (green_int > GAMMA_INT_MAX)
+		green = GEN9_MAX_GAMMA;
+	red_int = _GAMMA_INT_PART(red);
+	if (red_int > GAMMA_INT_MAX)
+		red = GEN9_MAX_GAMMA;
+
+	blue_fract = _GAMMA_FRACT_PART(blue);
+	green_fract = _GAMMA_FRACT_PART(green);
+	red_fract = _GAMMA_FRACT_PART(red);
+
+	blue_fract >>= GEN9_10BIT_GAMMA_MSB_SHIFT;
+	green_fract >>= GEN9_10BIT_GAMMA_MSB_SHIFT;
+	red_fract >>= GEN9_10BIT_GAMMA_MSB_SHIFT;
+
+	/* Red (29:20) Green (19:10) and Blue (9:0) */
+	word = red_fract;
+	word <<= GEN9_GAMMA_SHIFT;
+	word = word | green_fract;
+	word <<= GEN9_GAMMA_SHIFT;
+	word = word | blue_fract;
+
+	return word;
+}
+
 int chv_set_degamma(struct drm_device *dev, struct drm_property_blob *blob,
 		struct drm_crtc *crtc)
 {
@@ -225,6 +259,238 @@ int chv_set_degamma(struct drm_device *dev, struct drm_property_blob *blob,
 	return ret;
 }
 
+int gen9_set_gamma(struct drm_device *dev, struct drm_property_blob *blob,
+		   struct drm_crtc *crtc)
+{
+	u8 blue_int, green_int, red_int;
+	u16 blue_fract, green_fract, red_fract;
+	u16 blue_odd, green_odd, red_odd;
+	u16 blue_even, green_even, red_even;
+	int ret, count, num_samples, length;
+	enum pipe pipe;
+	u32 blue, green, red;
+	u32 mode, pal_prec_index, pal_prec_data;
+	u32 cgm_control_reg = 0;
+	u32 index, word;
+	struct drm_palette *gamma_data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_r32g32b32 *correction_values = NULL;
+
+	if (!blob) {
+		DRM_ERROR("Null Blob\n");
+		return -EINVAL;
+	}
+
+	gamma_data = (struct drm_palette *)blob->data;
+
+	if (gamma_data->version != GEN9_GAMMA_DATA_STRUCT_VERSION) {
+		DRM_ERROR("Invalid Gamma Data struct version\n");
+		return -EINVAL;
+	}
+
+	pipe = to_intel_crtc(crtc)->pipe;
+	num_samples = gamma_data->num_samples;
+	length = num_samples * sizeof(struct drm_r32g32b32);
+	mode = I915_READ(GAMMA_MODE(pipe));
+
+	pal_prec_index = _PREC_PAL_INDEX(pipe);
+	pal_prec_data = _PREC_PAL_DATA(pipe);
+
+	correction_values = (struct drm_r32g32b32 *)&gamma_data->lut;
+	index = I915_READ(pal_prec_index);
+	switch (num_samples) {
+
+	case GEN9_8BIT_GAMMA_MAX_VALS:
+
+		/* Legacy palette */
+		pal_prec_data = LGC_PALETTE(pipe);
+
+		count = 0;
+		while (count < GEN9_8BIT_GAMMA_MAX_VALS) {
+			blue = correction_values[count].b32;
+			green = correction_values[count].g32;
+			red = correction_values[count].r32;
+
+			blue_int = _GAMMA_INT_PART(blue);
+			if (blue_int > GAMMA_INT_MAX)
+				blue = GEN9_MAX_GAMMA;
+			green_int = _GAMMA_INT_PART(green);
+			if (green_int > GAMMA_INT_MAX)
+				green = GEN9_MAX_GAMMA;
+			red_int = _GAMMA_INT_PART(red);
+			if (red_int > GAMMA_INT_MAX)
+				red = GEN9_MAX_GAMMA;
+
+			blue_fract = _GAMMA_FRACT_PART(blue);
+			green_fract = _GAMMA_FRACT_PART(green);
+			red_fract = _GAMMA_FRACT_PART(red);
+
+			blue_fract >>= GEN9_8BIT_GAMMA_MSB_SHIFT;
+			green_fract >>= GEN9_8BIT_GAMMA_MSB_SHIFT;
+			red_fract >>= GEN9_8BIT_GAMMA_MSB_SHIFT;
+
+			/* Red (23:16) Green (15:8) and Blue (7:0) */
+			word = red_fract;
+			word <<= GEN9_8BIT_GAMMA_SHIFT;
+			word = word | green_fract;
+			word <<= GEN9_8BIT_GAMMA_SHIFT;
+			word = word | blue_fract;
+
+			I915_WRITE(pal_prec_data, word);
+			pal_prec_data += 4;
+
+			count++;
+		}
+
+		mode &= ~GAMMA_MODE_MODE_MASK;
+		I915_WRITE(GAMMA_MODE(pipe), mode | GAMMA_MODE_MODE_8BIT);
+		DRM_DEBUG_DRIVER("Gamma registers updated on %c\n",
+				pipe_name(pipe));
+		ret = 0;
+		break;
+
+	case GEN9_SPLITGAMMA_MAX_VALS:
+
+		index |= GEN9_INDEX_AUTO_INCREMENT | GEN9_INDEX_SPLIT_MODE;
+		I915_WRITE(pal_prec_index, index);
+
+		count = 0;
+		while (count < GEN9_SPLITGAMMA_MAX_VALS) {
+			blue = correction_values[count].b32;
+			green = correction_values[count].g32;
+			red = correction_values[count].r32;
+
+			word = gen9_write_10bit_gamma_precision(red,
+					green, blue);
+			I915_WRITE(pal_prec_data, word);
+			count++;
+		}
+
+		mode &= ~GAMMA_MODE_MODE_MASK;
+		I915_WRITE(GAMMA_MODE(pipe), mode | GAMMA_MODE_MODE_SPLIT);
+		DRM_DEBUG_DRIVER("Gamma registers updated on %c\n",
+				pipe_name(pipe));
+		ret = 0;
+		break;
+
+	case GEN9_12BIT_GAMMA_MAX_VALS:
+
+		index |= GEN9_INDEX_AUTO_INCREMENT;
+		index &= ~GEN9_INDEX_SPLIT_MODE;
+		I915_WRITE(pal_prec_index, index);
+
+		count = 0;
+		while (count < GEN9_12BIT_GAMMA_MAX_VALS) {
+			blue = correction_values[count].b32;
+			green = correction_values[count].g32;
+			red = correction_values[count].r32;
+
+			blue_int = _GAMMA_INT_PART(blue);
+			if (blue_int > GAMMA_INT_MAX)
+				blue = GEN9_MAX_GAMMA;
+			green_int = _GAMMA_INT_PART(green);
+			if (green_int > GAMMA_INT_MAX)
+				green = GEN9_MAX_GAMMA;
+			red_int = _GAMMA_INT_PART(red);
+			if (red_int > GAMMA_INT_MAX)
+				red = GEN9_MAX_GAMMA;
+
+			blue_fract = _GAMMA_FRACT_PART(blue);
+			green_fract = _GAMMA_FRACT_PART(green);
+			red_fract = _GAMMA_FRACT_PART(red);
+
+			/* Odd index */
+			if (count % 2 == 0) {
+				blue_odd = blue_fract >>
+					GEN9_12BIT_GAMMA_ODD_SHIFT;
+				green_odd = green_fract >>
+					GEN9_12BIT_GAMMA_ODD_SHIFT;
+				red_odd = red_fract >>
+					GEN9_12BIT_GAMMA_ODD_SHIFT;
+
+				word = red_odd;
+				word = word << GEN9_GAMMA_SHIFT;
+				word = word | green_odd;
+				word = word << GEN9_GAMMA_SHIFT;
+				word = word | blue_odd;
+			} else {
+				blue_even = blue_fract <<
+					GEN9_12BIT_GAMMA_EVEN_SHIFT1 >>
+					GEN9_12BIT_GAMMA_EVEN_SHIFT2;
+				green_even = green_fract <<
+					GEN9_12BIT_GAMMA_EVEN_SHIFT1 >>
+					GEN9_12BIT_GAMMA_EVEN_SHIFT2;
+				red_even = red_fract <<
+					GEN9_12BIT_GAMMA_EVEN_SHIFT1 >>
+					GEN9_12BIT_GAMMA_EVEN_SHIFT2;
+
+				word = red_even;
+				word = word << GEN9_GAMMA_SHIFT;
+				word = word | green_even;
+				word = word << GEN9_GAMMA_SHIFT;
+				word = word | blue_even;
+			}
+			I915_WRITE(pal_prec_data, word);
+			count++;
+		}
+
+		mode &= ~GAMMA_MODE_MODE_MASK;
+		I915_WRITE(GAMMA_MODE(pipe), mode | GAMMA_MODE_MODE_12BIT);
+		DRM_DEBUG_DRIVER("Gamma registers updated on %c\n",
+				pipe_name(pipe));
+		ret = 0;
+		break;
+
+	case GEN9_10BIT_GAMMA_MAX_VALS:
+
+		index |= GEN9_INDEX_AUTO_INCREMENT;
+		index &= ~GEN9_INDEX_SPLIT_MODE;
+		I915_WRITE(pal_prec_index, index);
+
+		count = 0;
+		while (count < GEN9_10BIT_GAMMA_MAX_VALS) {
+			blue = correction_values[count].b32;
+			green = correction_values[count].g32;
+			red = correction_values[count].r32;
+
+			word = gen9_write_10bit_gamma_precision(red,
+						green, blue);
+			I915_WRITE(pal_prec_data, word);
+			count++;
+		}
+
+		mode &= ~GAMMA_MODE_MODE_MASK;
+		I915_WRITE(GAMMA_MODE(pipe), mode | GAMMA_MODE_MODE_10BIT);
+		DRM_DEBUG_DRIVER("Gamma registers updated on %c\n",
+				pipe_name(pipe));
+		ret = 0;
+		break;
+
+	case 0:
+		/* Disable Gamma functionality on Pipe - CGM Block */
+		cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe));
+		cgm_control_reg &= ~CGM_GAMMA_EN;
+		I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg);
+
+		DRM_DEBUG_DRIVER("Gamma disabled on Pipe %c\n",
+				pipe_name(pipe));
+		return 0;
+
+	default:
+		DRM_ERROR("Invalid number of samples\n");
+		return -EINVAL;
+	}
+
+	/* Enable (CGM) Gamma on Pipe */
+	I915_WRITE(_PIPE_CGM_CONTROL(pipe),
+		I915_READ(_PIPE_CGM_CONTROL(pipe))
+		| CGM_GAMMA_EN);
+	DRM_DEBUG_DRIVER("CGM Gamma enabled on Pipe %c\n",
+		pipe_name(pipe));
+
+	return ret;
+}
+
 int chv_set_gamma(struct drm_device *dev, struct drm_property_blob *blob,
 		  struct drm_crtc *crtc)
 {
@@ -360,6 +626,8 @@ void intel_color_manager_crtc_commit(struct drm_device *dev,
 		/* Gamma correction is platform specific */
 		if (IS_CHERRYVIEW(dev))
 			ret = chv_set_gamma(dev, blob, crtc);
+		else if (IS_BROADWELL(dev) || IS_GEN9(dev))
+			ret = gen9_set_gamma(dev, blob, crtc);
 
 		if (ret)
 			DRM_ERROR("set Gamma correction failed\n");
@@ -390,6 +658,7 @@ void intel_color_manager_crtc_commit(struct drm_device *dev,
 		else
 			DRM_DEBUG_DRIVER("CSC correction success\n");
 	}
+
 }
 
 int intel_color_manager_set_pipe_csc(struct drm_device *dev,
diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
index 78de1a2..fa9d0b0 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.h
+++ b/drivers/gpu/drm/i915/intel_color_manager.h
@@ -37,6 +37,9 @@
 
 #define GEN9_PALETTE_STRUCT_VERSION		1
 #define GEN9_SPLITGAMMA_MAX_VALS		512
+#define GEN9_8BIT_GAMMA_MAX_VALS		256
+#define GEN9_10BIT_GAMMA_MAX_VALS		1024
+#define GEN9_12BIT_GAMMA_MAX_VALS		513
 
 /* Gamma correction */
 #define CHV_GAMMA_DATA_STRUCT_VERSION		1
@@ -52,6 +55,19 @@
 /* Max value for Gamma on CHV */
 #define CHV_MAX_GAMMA				0x10000
 
+/* Gen 9 */
+#define GEN9_GAMMA_DATA_STRUCT_VERSION		1
+#define GEN9_MAX_GAMMA				0x10000
+#define GEN9_10BIT_GAMMA_MSB_SHIFT		6
+#define GEN9_GAMMA_SHIFT			10
+#define GEN9_INDEX_AUTO_INCREMENT		(1 << 15)
+#define GEN9_INDEX_SPLIT_MODE			(1 << 31)
+#define GEN9_8BIT_GAMMA_MSB_SHIFT		8
+#define GEN9_8BIT_GAMMA_SHIFT			8
+#define GEN9_12BIT_GAMMA_ODD_SHIFT		6
+#define GEN9_12BIT_GAMMA_EVEN_SHIFT1		10
+#define GEN9_12BIT_GAMMA_EVEN_SHIFT2		6
+
 /* DeGamma correction */
 #define CHV_DEGAMMA_DATA_STRUCT_VERSION		1
 #define CHV_DEGAMMA_MSB_SHIFT			2
-- 
1.9.1

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

[PATCH 17/18] drm/i915: Add DeGamma correction for BDW/SKL/BXT

[Shashank Sharma]

From: Kausal Malladi <kausalmalladi@gmail.com>

BDW/SKL/BXT supports DeGamma color correction feature, which linearizes all
the non-linear color values. This will be applied before Color
Transformation.

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

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

diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
index a894f4c..9f9fb1a 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.c
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -154,6 +154,72 @@ u32 gen9_write_10bit_gamma_precision(u32 red, u32 green, u32 blue)
 	return word;
 }
 
+int gen9_set_degamma(struct drm_device *dev, struct drm_property_blob *blob,
+		struct drm_crtc *crtc)
+{
+	struct drm_palette *degamma_data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_r32g32b32 *correction_values = NULL;
+	u32 mode, pal_prec_index, pal_prec_data;
+	int count = 0;
+	u32 blue, green, red;
+	enum pipe pipe;
+	int num_samples, length;
+	u32 index, word;
+
+	if (!blob) {
+		DRM_ERROR("Null Blob\n");
+		return -EINVAL;
+	}
+
+	degamma_data = (struct drm_palette *)blob->data;
+
+	if (degamma_data->version != GEN9_DEGAMMA_DATA_STRUCT_VERSION) {
+		DRM_ERROR("Invalid DeGamma Data struct version\n");
+		return -EINVAL;
+	}
+
+	pipe = to_intel_crtc(crtc)->pipe;
+	num_samples = degamma_data->num_samples;
+	if (num_samples != GEN9_SPLITGAMMA_MAX_VALS) {
+		DRM_ERROR("Invalid number of samples\n");
+		return -EINVAL;
+	}
+
+	length = num_samples * sizeof(struct drm_r32g32b32);
+	mode = I915_READ(GAMMA_MODE(pipe));
+
+	pal_prec_index = _PREC_PAL_INDEX(pipe);
+	pal_prec_data = _PREC_PAL_DATA(pipe);
+
+	correction_values = (struct drm_r32g32b32 *)&degamma_data->lut;
+	index = I915_READ(pal_prec_index);
+	index |= GEN9_INDEX_AUTO_INCREMENT | GEN9_INDEX_SPLIT_MODE;
+	I915_WRITE(pal_prec_index, index);
+
+	while (count < num_samples) {
+		blue = correction_values[count].b32;
+		green = correction_values[count].g32;
+		red = correction_values[count].r32;
+
+		word = gen9_write_10bit_gamma_precision(red, green, blue);
+		I915_WRITE(pal_prec_data, word);
+		count++;
+	}
+
+	mode &= ~GAMMA_MODE_MODE_MASK;
+	I915_WRITE(GAMMA_MODE(pipe), mode | GAMMA_MODE_MODE_SPLIT);
+
+	/* Enable DeGamma on Pipe */
+	I915_WRITE(_PIPE_CGM_CONTROL(pipe),
+		I915_READ(_PIPE_CGM_CONTROL(pipe)) | CGM_DEGAMMA_EN);
+
+	DRM_DEBUG_DRIVER("DeGamma correction enabled on Pipe %c\n",
+			pipe_name(pipe));
+
+	return 0;
+}
+
 int chv_set_degamma(struct drm_device *dev, struct drm_property_blob *blob,
 		struct drm_crtc *crtc)
 {
@@ -640,6 +706,8 @@ void intel_color_manager_crtc_commit(struct drm_device *dev,
 		/* degamma correction */
 		if (IS_CHERRYVIEW(dev))
 			ret = chv_set_degamma(dev, blob, crtc);
+		else if (IS_BROADWELL(dev) || IS_GEN9(dev))
+			ret = gen9_set_degamma(dev, blob, crtc);
 
 		if (ret)
 			DRM_ERROR("set degamma correction failed\n");
diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
index fa9d0b0..ca89f25 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.h
+++ b/drivers/gpu/drm/i915/intel_color_manager.h
@@ -72,6 +72,8 @@
 #define CHV_DEGAMMA_DATA_STRUCT_VERSION		1
 #define CHV_DEGAMMA_MSB_SHIFT			2
 #define CHV_DEGAMMA_GREEN_SHIFT			16
+/* Gen 9 */
+#define GEN9_DEGAMMA_DATA_STRUCT_VERSION	1
 
 /* CSC correction */
 #define CHV_CSC_DATA_STRUCT_VERSION		1
-- 
1.9.1

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

[PATCH 18/18] drm/i915: Add CSC correction for BDW/SKL/BXT

[Shashank Sharma]

From: Kausal Malladi <kausalmalladi@gmail.com>

BDW/SKL/BXT support Color Space Conversion (CSC) using a 3x3 matrix
that needs to be programmed into respective CSC registers.

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

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

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 92233ce..31d7ff8 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7943,6 +7943,9 @@ enum skl_disp_power_wells {
 #define PAL_PREC_DATA_A				0x4A404
 #define PAL_PREC_DATA_B				0x4AC04
 #define PAL_PREC_DATA_C				0x4B404
+#define CSC_COEFF_A				0x49010
+#define CSC_COEFF_B				0x49110
+#define CSC_COEFF_C				0x49210
 
 #define _PIPE_CGM_CONTROL(pipe) \
 	(_PIPE3(pipe, PIPEA_CGM_CONTROL, PIPEB_CGM_CONTROL, PIPEC_CGM_CONTROL))
@@ -7957,5 +7960,7 @@ enum skl_disp_power_wells {
 	(_PIPE3(pipe, PAL_PREC_INDEX_A, PAL_PREC_INDEX_B, PAL_PREC_INDEX_C))
 #define _PREC_PAL_DATA(pipe) \
 	(_PIPE3(pipe, PAL_PREC_DATA_A, PAL_PREC_DATA_B, PAL_PREC_DATA_C))
+#define _PIPE_CSC_COEFF(pipe) \
+	(_PIPE3(pipe, CSC_COEFF_A, CSC_COEFF_B, CSC_COEFF_C))
 
 #endif /* _I915_REG_H_ */
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
index 9f9fb1a..fc08e67 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.c
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -27,6 +27,94 @@
 
 #include "intel_color_manager.h"
 
+s16 get_csc_s2_7_format(s64 csc_value)
+{
+	s32 csc_int_value;
+	u32 csc_fract_value;
+	s16 csc_s2_7_format;
+
+	if (csc_value >= 0) {
+		csc_value += GEN9_CSC_FRACT_ROUNDOFF;
+		if (csc_value > GEN9_CSC_COEFF_MAX)
+			csc_value = GEN9_CSC_COEFF_MAX;
+	} else {
+		csc_value = -csc_value;
+		csc_value += GEN9_CSC_FRACT_ROUNDOFF;
+		if (csc_value > GEN9_CSC_COEFF_MAX + 1)
+			csc_value = GEN9_CSC_COEFF_MAX + 1;
+		csc_value = -csc_value;
+	}
+
+	csc_int_value = csc_value >> GEN9_CSC_COEFF_SHIFT;
+	csc_int_value <<= GEN9_CSC_COEFF_INT_SHIFT;
+	if (csc_value < 0)
+		csc_int_value |= CSC_COEFF_SIGN;
+	csc_fract_value = csc_value;
+	csc_fract_value >>= GEN9_CSC_COEFF_FRACT_SHIFT;
+	csc_s2_7_format = csc_int_value | csc_fract_value;
+
+	return csc_s2_7_format;
+}
+
+int gen9_set_csc(struct drm_device *dev, struct drm_property_blob *blob,
+		struct drm_crtc *crtc)
+{
+	struct drm_ctm *csc_data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 reg, plane_ctl;
+	enum pipe pipe;
+	enum plane plane;
+	s32 word;
+	int i, j;
+
+	if (!blob) {
+		DRM_ERROR("NULL Blob\n");
+		return -EINVAL;
+	}
+
+	if (blob->length != sizeof(struct drm_ctm)) {
+		DRM_ERROR("Invalid length of data received\n");
+		return -EINVAL;
+	}
+
+	csc_data = (struct drm_ctm *)blob->data;
+	if (csc_data->version != GEN9_CSC_DATA_STRUCT_VERSION) {
+		DRM_ERROR("Invalid CSC Data struct version\n");
+		return -EINVAL;
+	}
+
+	pipe = to_intel_crtc(crtc)->pipe;
+	plane = to_intel_crtc(crtc)->plane;
+
+	plane_ctl = I915_READ(PLANE_CTL(pipe, plane));
+	plane_ctl |= PLANE_CTL_PIPE_CSC_ENABLE;
+
+	I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl);
+
+	reg = _PIPE_CSC_COEFF(pipe);
+
+	/* Write csc coeff to csc regs */
+	for (i = 0, j = 0; i < CSC_MAX_VALS; i++) {
+		word = get_csc_s2_7_format(csc_data->ctm_coeff[i]);
+		word = word << GEN9_CSC_SHIFT;
+		if (i % 3 != 2)
+			word = word |
+				get_csc_s2_7_format(csc_data->ctm_coeff[i]);
+
+		I915_WRITE(reg + j, word);
+		j = j + 4;
+	}
+
+	DRM_DEBUG_DRIVER("All CSC values written to registers\n");
+
+	/* Enable CSC functionality */
+	reg = _PIPE_CGM_CONTROL(pipe);
+	I915_WRITE(reg, I915_READ(reg) | CGM_CSC_EN);
+	DRM_DEBUG_DRIVER("CSC enabled on Pipe %c\n", pipe_name(pipe));
+
+	return 0;
+}
+
 s16 get_csc_s3_12_format(s64 csc_value)
 {
 	s32 csc_int_value;
@@ -720,6 +808,8 @@ void intel_color_manager_crtc_commit(struct drm_device *dev,
 		/* CSC correction */
 		if (IS_CHERRYVIEW(dev))
 			ret = chv_set_csc(dev, blob, crtc);
+		else if (IS_BROADWELL(dev) || IS_GEN9(dev))
+			ret = gen9_set_csc(dev, blob, crtc);
 
 		if (ret)
 			DRM_ERROR("set CSC correction failed\n");
diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
index ca89f25..83e9bc7 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.h
+++ b/drivers/gpu/drm/i915/intel_color_manager.h
@@ -93,6 +93,23 @@
 #define CSC_COEFF_SIGN				(1 << 31)
 #define CHV_CSC_COEFF_FRACT_SHIFT		20
 #define CSC_MAX_VALS				9
+/* Gen 9 */
+#define GEN9_CSC_DATA_STRUCT_VERSION		1
+/*
+ * Fractional part is 32 bit, and we need only 7 MSBs for programming
+ * into registers. ROUNDOFF is required to minimize loss of precision.
+ */
+#define GEN9_CSC_FRACT_ROUNDOFF			(1 << 24)
+/*
+ * CSC values are 64-bit values. For GEN9, the maximum CSC value that
+ * user can program is 3.99999..., which can be represented in fixed point
+ * S31.32 format like this, with all fractional bits as 1
+ */
+#define GEN9_CSC_COEFF_MAX			0x00000003FFFFFFFF
+#define GEN9_CSC_COEFF_SHIFT			32
+#define GEN9_CSC_COEFF_INT_SHIFT		29
+#define GEN9_CSC_COEFF_FRACT_SHIFT		25
+#define GEN9_CSC_SHIFT				16
 
 /* CHV CGM Block */
 #define CGM_GAMMA_EN				(1 << 2)
-- 
1.9.1

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

Re: [PATCH 18/18] drm/i915: Add CSC correction for BDW/SKL/BXT

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 7110
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                                  302/302              302/302
SNB                                  315/315              315/315
IVB                                  336/336              336/336
BYT                 -1              283/283              282/283
HSW                                  378/378              378/378
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*BYT  igt@gem_tiled_partial_pwrite_pread@reads      PASS(1)      FAIL(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 18/18] drm/i915: Add CSC correction for BDW/SKL/BXT

[Rob Bradford]

Hi Shashank, some feedback on the CSC code below.


On Thu, 2015-08-06 at 22:08 +0530, Shashank Sharma wrote:
> From: Kausal Malladi <kausalmalladi@gmail.com>
> 
> BDW/SKL/BXT support Color Space Conversion (CSC) using a 3x3 matrix
> that needs to be programmed into respective CSC registers.
> 
> This patch does the following:
> 1. Adds the core function to program CSC correction values for
>    BDW/SKL/BXT platform
> 2. Adds CSC correction macros/defines
> 

*snip* 

> +s16 get_csc_s2_7_format(s64 csc_value)
> +{
> +	s32 csc_int_value;
> +	u32 csc_fract_value;
> +	s16 csc_s2_7_format;
> +
> +	if (csc_value >= 0) {
> +		csc_value += GEN9_CSC_FRACT_ROUNDOFF;
> +		if (csc_value > GEN9_CSC_COEFF_MAX)
> +			csc_value = GEN9_CSC_COEFF_MAX;
> +	} else {
> +		csc_value = -csc_value;
> +		csc_value += GEN9_CSC_FRACT_ROUNDOFF;
> +		if (csc_value > GEN9_CSC_COEFF_MAX + 1)
> +			csc_value = GEN9_CSC_COEFF_MAX + 1;
> +		csc_value = -csc_value;
> +	}
> +
> +	csc_int_value = csc_value >> GEN9_CSC_COEFF_SHIFT;
> +	csc_int_value <<= GEN9_CSC_COEFF_INT_SHIFT;
> +	if (csc_value < 0)
> +		csc_int_value |= CSC_COEFF_SIGN;
> +	csc_fract_value = csc_value;
> +	csc_fract_value >>= GEN9_CSC_COEFF_FRACT_SHIFT;
> +	csc_s2_7_format = csc_int_value | csc_fract_value;
> +
> +	return csc_s2_7_format;
> +}

I'm afraid this isn't the right way to calculate the coefficients for
BDW. It doesn't use a fixed s2.7 format rather a limited floating point
(according to the BSpec.) 

I think it's perfectly reasonable to make the decision only to support
the values in that range but then you still need to modify the above
code to set the exponent part to 110b and also shift for the reserved
bits.

> +int gen9_set_csc(struct drm_device *dev, struct drm_property_blob
> *blob,
> +		struct drm_crtc *crtc)
> +{

*snip*

> +	/* Write csc coeff to csc regs */
> +	for (i = 0, j = 0; i < CSC_MAX_VALS; i++) {
> +		word = get_csc_s2_7_format(csc_data->ctm_coeff[i]);
> +		word = word << GEN9_CSC_SHIFT;
> +		if (i % 3 != 2)
> +			word = word |
> +				get_csc_s2_7_format(csc_data
> ->ctm_coeff[i]);
> +
> +		I915_WRITE(reg + j, word);
> +		j = j + 4;
> +	}

*snip*

I'm not sure the above loop is great style, vs explicitly describing
each of the register updates. But for the above code to come close to
working you need to increment i before ORing the second packed value.

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

Re: [PATCH 00/18] Color Management for DRM

[Rob Bradford]

On Thu, 2015-08-06 at 22:08 +0530, Shashank Sharma wrote:
> This patch set adds Color Manager implementation in DRM layer. Color
> Manager is
> an extension in DRM framework to support color
> correction/enhancement. Various
> Hardware platforms can support several color correction capabilities.
> Color Manager provides abstraction of these capabilities and allows a
> user space UI agent to correct/enhance the display using the DRM
> property interface.


I think this patch series is a bit too fragmented and so it's hard to
review because the code is spread over too many commits. Could you look
at consolidating them.

Some suggestions:

- merge the the data structures with adding the properties that use
them.
- merge 02 and 03
- tidy up the ordering so that you e.g. add CTM infrastructure and then
program it for BDW and then for BSW, and similarly for gamma and
degamma. 
- and conversely i think patch 05 tries to do too much, it sets up
infrastructure and implements it for CHV.
- (nit)try and improve the terminology (and their consistency) in the
patches and commit messages, CTM, CSC, deGamma, gamma.

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

Re: [PATCH 00/18] Color Management for DRM

[Sharma, Shashank]

Hey Rob,
My comments inline.

Regards
Shashank
On 9/8/2015 4:19 PM, Rob Bradford wrote:
> On Thu, 2015-08-06 at 22:08 +0530, Shashank Sharma wrote:
>> This patch set adds Color Manager implementation in DRM layer. Color
>> Manager is
>> an extension in DRM framework to support color
>> correction/enhancement. Various
>> Hardware platforms can support several color correction capabilities.
>> Color Manager provides abstraction of these capabilities and allows a
>> user space UI agent to correct/enhance the display using the DRM
>> property interface.
>
>
> I think this patch series is a bit too fragmented and so it's hard to
> review because the code is spread over too many commits. Could you look
> at consolidating them.
>
The whole idea and requirements we agreed on initially was to implement 
it first for CHV/BSW and then implement it for BDW+. Hence the 
sequencing is like that.
> Some suggestions:
>
> - merge the the data structures with adding the properties that use
> them.
> - merge 02 and 03
> - tidy up the ordering so that you e.g. add CTM infrastructure and then
> program it for BDW and then for BSW, and similarly for gamma and
> degamma.
I am afraid we are a bit late for this.
Matt has done 4 set of reviews in this order, and if I rearrange it, it 
would be too much to ask from him to do it all again.

If we consider this enabling all the features for one platform first and 
then extending it to others, this might not look that fragmented to you, 
try this out :)
> - and conversely i think patch 05 tries to do too much, it sets up
> infrastructure and implements it for CHV.
I think I can split this into two patches.
> - (nit)try and improve the terminology (and their consistency) in the
> patches and commit messages, CTM, CSC, deGamma, gamma.
Its pretty clear, actually, may be this would help.
On DRM layer its CTM, palette_befor_ctm and palette_after_ctm, which is 
as defined in the framework.
On i915 layer its CSC, degamma and gammma, which is specific to intel 
hardware. Does it make sense now ?
>
> Rob
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx