diff mbox

[13/22] drm/i915: CHV: Pipe level Gamma correction

Message ID 1444418952-5671-14-git-send-email-shashank.sharma@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Sharma, Shashank Oct. 9, 2015, 7:29 p.m. UTC
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 | 114 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_color_manager.h |  13 ++++
 3 files changed, 139 insertions(+)

Comments

Emil Velikov Oct. 9, 2015, 11:07 p.m. UTC | #1
Hi Shashank,

On 9 October 2015 at 20:29, Shashank Sharma <shashank.sharma@intel.com> wrote:
> 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 | 114 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_color_manager.h |  13 ++++
>  3 files changed, 139 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 60e6a73..885ac8a 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -8038,4 +8038,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 e466748..cf381b8 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.c
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -27,6 +27,112 @@
>
>  #include "intel_color_manager.h"
>
> +static int chv_set_gamma(struct drm_device *dev, struct drm_property_blob *blob,
> +               struct drm_crtc *crtc)
> +{
> +       bool flag = false;
> +       enum pipe pipe;
> +       u16 red_fract, green_fract, blue_fract;
> +       u32 red, green, blue, num_samples;
> +       u32 word = 0;
> +       u32 count = 0;
> +       u32 cgm_gamma_reg = 0;
> +       u32 cgm_control_reg = 0;
> +       int ret = 0, length;
> +       struct drm_r32g32b32 *correction_values = NULL;
You can drop the useless initialization of correction_values. Same
goes for patches 19 and 21.

> +       struct drm_palette *gamma_data;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +       if (WARN_ON(!blob))
> +               return -EINVAL;
> +
> +       gamma_data = (struct drm_palette *)blob->data;
> +       pipe = to_intel_crtc(crtc)->pipe;
> +       num_samples = gamma_data->num_samples;
> +       length = num_samples * sizeof(struct drm_r32g32b32);
Calculation can overflow.

> +
> +       switch (num_samples) {
> +       case GAMMA_DISABLE_VALS:
> +
> +               /* 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));
Indentation looks wrong here.

> +               ret = 0;
Drop the variable and return 0, at the bottom of the function ?

> +               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) {
Using for(i = 0;....) loop seems the more common approach ?

[snip]
> +                       /*
> +                       * 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;
> +                       }
There is little point in going over this if statement 256 odd times.
Split it out of the loop ?

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

> +               ret = 0;
Drop the variable ?

> +               break;
> +
> +       default:
> +               DRM_ERROR("Invalid number of samples for Gamma LUT\n");
> +               ret = -EINVAL;
return -EINVAL;

> +       }
> +
> +       return ret;
return 0;


Regards,
Emil
Sharma, Shashank Oct. 10, 2015, 5:09 a.m. UTC | #2
Regards
Shashank

On 10/10/2015 4:37 AM, Emil Velikov wrote:
> Hi Shashank,
>
> On 9 October 2015 at 20:29, Shashank Sharma <shashank.sharma@intel.com> wrote:
>> 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 | 114 +++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_color_manager.h |  13 ++++
>>   3 files changed, 139 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 60e6a73..885ac8a 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -8038,4 +8038,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 e466748..cf381b8 100644
>> --- a/drivers/gpu/drm/i915/intel_color_manager.c
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
>> @@ -27,6 +27,112 @@
>>
>>   #include "intel_color_manager.h"
>>
>> +static int chv_set_gamma(struct drm_device *dev, struct drm_property_blob *blob,
>> +               struct drm_crtc *crtc)
>> +{
>> +       bool flag = false;
>> +       enum pipe pipe;
>> +       u16 red_fract, green_fract, blue_fract;
>> +       u32 red, green, blue, num_samples;
>> +       u32 word = 0;
>> +       u32 count = 0;
>> +       u32 cgm_gamma_reg = 0;
>> +       u32 cgm_control_reg = 0;
>> +       int ret = 0, length;
>> +       struct drm_r32g32b32 *correction_values = NULL;
> You can drop the useless initialization of correction_values. Same
> goes for patches 19 and 21.
Agree, thanks for pointing it out.
>
>> +       struct drm_palette *gamma_data;
>> +       struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +       if (WARN_ON(!blob))
>> +               return -EINVAL;
>> +
>> +       gamma_data = (struct drm_palette *)blob->data;
>> +       pipe = to_intel_crtc(crtc)->pipe;
>> +       num_samples = gamma_data->num_samples;
>> +       length = num_samples * sizeof(struct drm_r32g32b32);
> Calculation can overflow.
good catch, will take care of this.
>
>> +
>> +       switch (num_samples) {
>> +       case GAMMA_DISABLE_VALS:
>> +
>> +               /* 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));
> Indentation looks wrong here.
>
Oops, bloody SI :).
>> +               ret = 0;
> Drop the variable and return 0, at the bottom of the function ?
>
Let me check this out.
>> +               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) {
> Using for(i = 0;....) loop seems the more common approach ?
Nah, we are good with while. The whole color management series prefers 
while (and me too.... :))
>
> [snip]
>> +                       /*
>> +                       * 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;
>> +                       }
> There is little point in going over this if statement 256 odd times.
> Split it out of the loop ?
Makes sense for sure, let me try this out.
>
>> +               }
>> +
>> +               /* 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));
> Indentation.
Guilty.
>
>> +               ret = 0;
> Drop the variable ?
Agree
>
>> +               break;
>> +
>> +       default:
>> +               DRM_ERROR("Invalid number of samples for Gamma LUT\n");
>> +               ret = -EINVAL;
> return -EINVAL;
Agree
>
>> +       }
>> +
>> +       return ret;
> return 0;
Agree
>
>
> Regards,
> Emil
>
Emil Velikov Oct. 13, 2015, 1:08 p.m. UTC | #3
On 10 October 2015 at 06:09, Sharma, Shashank <shashank.sharma@intel.com> wrote:
> On 10/10/2015 4:37 AM, Emil Velikov wrote:
>>
>> Hi Shashank,
>>
>> On 9 October 2015 at 20:29, Shashank Sharma <shashank.sharma@intel.com>
>> wrote:
>>>
>>> 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>
>>> ---
[snip]
>>> +       length = num_samples * sizeof(struct drm_r32g32b32);
>>
>> Calculation can overflow.
>
> good catch, will take care of this.
Actually we do not at all here as the variable is unused. Same applies
for patch 21/22.

[snip]
>>> +               while (count < num_samples) {
>>
>> Using for(i = 0;....) loop seems the more common approach ?
>
> Nah, we are good with while. The whole color management series prefers while
> (and me too.... :))
Hmm... so you'd prefer your approach/coding style over the one already
in i915? Feels a bit strange but as long as others are happy fine go
with it.

Regards,
Emil
Sharma, Shashank Oct. 13, 2015, 1:40 p.m. UTC | #4
Regards
Shashank

On 10/13/2015 6:38 PM, Emil Velikov wrote:
> On 10 October 2015 at 06:09, Sharma, Shashank <shashank.sharma@intel.com> wrote:
>> On 10/10/2015 4:37 AM, Emil Velikov wrote:
>>>
>>> Hi Shashank,
>>>
>>> On 9 October 2015 at 20:29, Shashank Sharma <shashank.sharma@intel.com>
>>> wrote:
>>>>
>>>> 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>
>>>> ---
> [snip]
>>>> +       length = num_samples * sizeof(struct drm_r32g32b32);
>>>
>>> Calculation can overflow.
>>
>> good catch, will take care of this.
> Actually we do not at all here as the variable is unused. Same applies
> for patch 21/22.
>
Yep, Rob mentioned that in his comment, I have removed this in the 
latest patch set I have sent.
> [snip]
>>>> +               while (count < num_samples) {
>>>
>>> Using for(i = 0;....) loop seems the more common approach ?
>>
>> Nah, we are good with while. The whole color management series prefers while
>> (and me too.... :))
> Hmm... so you'd prefer your approach/coding style over the one already
> in i915? Feels a bit strange but as long as others are happy fine go
> with it.
>
I am not sure if I915 follows a general rule of using for(...) over 
while(), coz I see many instances of using a while in i915_gem, 
i915_drv, i915_irq etc, so it should be good. I would see if someone 
else can suggest another good reason.
> Regards,
> Emil
>
Emil Velikov Oct. 13, 2015, 1:59 p.m. UTC | #5
On 13 October 2015 at 14:40, Sharma, Shashank <shashank.sharma@intel.com> wrote:

> I am not sure if I915 follows a general rule of using for(...) over while(),
> coz I see many instances of using a while in i915_gem, i915_drv, i915_irq
> etc, so it should be good. I would see if someone else can suggest another
> good reason.
I'm not saying "you should never use while loops", but more of "people
use for loops when accessing arrays of information". I cannot see any
cases of the latter in i915. Perhaps I'm looking at some old code ?

The point I'm trying to make here is that people are unlikely to make
comments about such trivial nitpicks (bikeshedding), but new
contributions should stick with the existing approach, rather than
going for "I like XXX" :)

Take any and all of my with a grain of salt, just in case.

Regards,
Emil
Sharma, Shashank Oct. 13, 2015, 2:04 p.m. UTC | #6
Regards
Shashank

On 10/13/2015 7:29 PM, Emil Velikov wrote:
> On 13 October 2015 at 14:40, Sharma, Shashank <shashank.sharma@intel.com> wrote:
>
>> I am not sure if I915 follows a general rule of using for(...) over while(),
>> coz I see many instances of using a while in i915_gem, i915_drv, i915_irq
>> etc, so it should be good. I would see if someone else can suggest another
>> good reason.
> I'm not saying "you should never use while loops", but more of "people
> use for loops when accessing arrays of information". I cannot see any
> cases of the latter in i915. Perhaps I'm looking at some old code ?
>
> The point I'm trying to make here is that people are unlikely to make
> comments about such trivial nitpicks (bikeshedding), but new
> contributions should stick with the existing approach, rather than
> going for "I like XXX" :)
>
> Take any and all of my with a grain of salt, just in case.
>
Its a very well written and logical comment, but I like this last line 
the most :) :)
> Regards,
> Emil
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 60e6a73..885ac8a 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -8038,4 +8038,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 e466748..cf381b8 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.c
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -27,6 +27,112 @@ 
 
 #include "intel_color_manager.h"
 
+static int chv_set_gamma(struct drm_device *dev, struct drm_property_blob *blob,
+		struct drm_crtc *crtc)
+{
+	bool flag = false;
+	enum pipe pipe;
+	u16 red_fract, green_fract, blue_fract;
+	u32 red, green, blue, num_samples;
+	u32 word = 0;
+	u32 count = 0;
+	u32 cgm_gamma_reg = 0;
+	u32 cgm_control_reg = 0;
+	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 (WARN_ON(!blob))
+		return -EINVAL;
+
+	gamma_data = (struct drm_palette *)blob->data;
+	pipe = to_intel_crtc(crtc)->pipe;
+	num_samples = gamma_data->num_samples;
+	length = num_samples * sizeof(struct drm_r32g32b32);
+
+	switch (num_samples) {
+	case GAMMA_DISABLE_VALS:
+
+		/* 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;
+
+			if (blue > CHV_MAX_GAMMA)
+				blue = CHV_MAX_GAMMA;
+
+			if (green > CHV_MAX_GAMMA)
+				green = CHV_MAX_GAMMA;
+
+			if (red > CHV_MAX_GAMMA)
+				red = CHV_MAX_GAMMA;
+
+			/* get MSB 10 bits from fraction part (14:23) */
+			blue_fract = GET_BITS(blue, 14, 10);
+			green_fract = GET_BITS(green, 14, 10);
+			red_fract = GET_BITS(red, 14, 10);
+
+			/* Green (25:16) and Blue (9:0) to be written */
+			SET_BITS(word, green_fract, 16, 10);
+			SET_BITS(word, blue_fract, 0, 10);
+			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_attach_color_properties_to_crtc(struct drm_device *dev,
 		struct drm_crtc *crtc)
 {
@@ -61,4 +167,12 @@  void intel_attach_color_properties_to_crtc(struct drm_device *dev,
 				INTEL_INFO(dev)->num_samples_before_ctm);
 		DRM_DEBUG_DRIVER("Degamma query property initialized\n");
 	}
+
+	/* Gamma correction */
+	if (config->cm_palette_after_ctm_property) {
+		drm_object_attach_property(mode_obj,
+			config->cm_palette_after_ctm_property, 0);
+		DRM_DEBUG_DRIVER("gamma property attached to CRTC\n");
+	}
+
 }
diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
index 14a1309..de706d9 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.h
+++ b/drivers/gpu/drm/i915/intel_color_manager.h
@@ -52,3 +52,16 @@ 
 /* CHV */
 #define CHV_10BIT_GAMMA_MAX_VALS		257
 #define CHV_DEGAMMA_MAX_VALS                   65
+
+/* No of coeff for disabling gamma is 0 */
+#define GAMMA_DISABLE_VALS			0
+
+/* Gamma on CHV */
+#define CHV_10BIT_GAMMA_MAX_VALS               257
+#define CHV_8BIT_GAMMA_MAX_VALS                256
+#define CHV_10BIT_GAMMA_MSB_SHIFT              6
+#define CHV_GAMMA_SHIFT_GREEN                  16
+#define CHV_MAX_GAMMA                          ((1 << 24) - 1)
+
+/* CHV CGM Block */
+#define CGM_GAMMA_EN                           (1 << 2)