diff mbox

[21/23] drm/i915: BDW: Pipe level Gamma correction

Message ID 1442425040-32185-22-git-send-email-shashank.sharma@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Sharma, Shashank Sept. 16, 2015, 5:37 p.m. UTC
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 platforms
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 | 270 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_color_manager.h |  15 ++
 3 files changed, 300 insertions(+), 2 deletions(-)

Comments

Rob Bradford Sept. 30, 2015, 2:31 p.m. UTC | #1
Hi Shashank, some feedback below that you would be great to get
addressed before your next version.

On Wed, 2015-09-16 at 23:07 +0530, Shashank Sharma wrote:
> 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 platforms
> 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 | 270
> +++++++++++++++++++++++++++++

*snip*

> +static u32 bdw_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 = BDW_MAX_GAMMA;
> +
> +	green_int = _GAMMA_INT_PART(green);
> +	if (green_int > GAMMA_INT_MAX)
> +		green = BDW_MAX_GAMMA;
> +
> +	red_int = _GAMMA_INT_PART(red);
> +	if (red_int > GAMMA_INT_MAX)
> +		red = BDW_MAX_GAMMA;
> +
> +	blue_fract = _GAMMA_FRACT_PART(blue);
> +	green_fract = _GAMMA_FRACT_PART(green);
> +	red_fract = _GAMMA_FRACT_PART(red);
> +
> +	blue_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
> +	green_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
> +	red_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
> +
> +	/* Red (29:20) Green (19:10) and Blue (9:0) */
> +	word = red_fract;
> +	word <<= BDW_GAMMA_SHIFT;
> +	word = word | green_fract;
> +	word <<= BDW_GAMMA_SHIFT;
> +	word = word | blue_fract;
> +
> +	return word;
> +}
> +

I think the above function, and perhaps others in this series have the
same flaw with respect to maximum colour value.

In our discussions we agreed that we would follow the "GL style" where
maximum colour (i.e. 255 in 8-bit) would be represented by 1.0f. 1.0f
when converted to fixed point 8.24 notation is 1 << 24.

I observed that with my test code that a linear ramp (where the last
entry is set to 1.0) gives me black for white. I tracked it down to
this function.

In order to map 1.0 to the maximum value for the table entry in the
hardware this function needs to be changed. One way to achieve this
would be change the test "blue_int > GAMMA_INT_MAX" to be "blue_int >=
GAMMA_INT_MAX" but since GAMMA_INT_MAX = 1 then it might be clearer to
say blue_int > 0.

BDW_MAX_GAMMA also looks wrong. I think it should be (1 << 24) - 1 not
the 0x10000 (see below). As well as correct clamping it is also
necessary to scale as Daniel suggested on IRC:

"
13:54 < danvet> robster, so we'd need to rescale in the kernel from 1.0
to 0.1111111111b ?
13:55 < danvet> well substract 0.0000000001b for all values > 0.5
probably since float math in the kernel is evil
13:56 < danvet> also this probably needs an igt - check that all black
with an all 1.0 gamma table is the same as 
                all white with a linear one
"

You won't see this with your test program (color-correction.c) as the
largest value you program in appears to be 0.ff

*snip*

> +/* Gen 9 */
> +#define BDW_MAX_GAMMA				0x10000
> 

*snip*

I look forward to testing against your next version.

Rob
Sharma, Shashank Sept. 30, 2015, 4:25 p.m. UTC | #2
Hey Rob, 

Thanks for the feedback, and the testing efforts. 
I will check into your suggestions and get back. 

Regards
Shashank
-----Original Message-----
From: Bradford, Robert 

Sent: Wednesday, September 30, 2015 8:02 PM
To: Sharma, Shashank; Roper, Matthew D; Bish, Jim; Smith, Gary K; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
Cc: Vetter, Daniel; Matheson, Annie J; Mukherjee, Indranil; Palleti, Avinash Reddy; kausalmalladi@gmail.com
Subject: Re: [PATCH 21/23] drm/i915: BDW: Pipe level Gamma correction

Hi Shashank, some feedback below that you would be great to get addressed before your next version.

On Wed, 2015-09-16 at 23:07 +0530, Shashank Sharma wrote:
> 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 platforms

> 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 | 270

> +++++++++++++++++++++++++++++


*snip*

> +static u32 bdw_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 = BDW_MAX_GAMMA;

> +

> +	green_int = _GAMMA_INT_PART(green);

> +	if (green_int > GAMMA_INT_MAX)

> +		green = BDW_MAX_GAMMA;

> +

> +	red_int = _GAMMA_INT_PART(red);

> +	if (red_int > GAMMA_INT_MAX)

> +		red = BDW_MAX_GAMMA;

> +

> +	blue_fract = _GAMMA_FRACT_PART(blue);

> +	green_fract = _GAMMA_FRACT_PART(green);

> +	red_fract = _GAMMA_FRACT_PART(red);

> +

> +	blue_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;

> +	green_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;

> +	red_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;

> +

> +	/* Red (29:20) Green (19:10) and Blue (9:0) */

> +	word = red_fract;

> +	word <<= BDW_GAMMA_SHIFT;

> +	word = word | green_fract;

> +	word <<= BDW_GAMMA_SHIFT;

> +	word = word | blue_fract;

> +

> +	return word;

> +}

> +


I think the above function, and perhaps others in this series have the same flaw with respect to maximum colour value.

In our discussions we agreed that we would follow the "GL style" where maximum colour (i.e. 255 in 8-bit) would be represented by 1.0f. 1.0f when converted to fixed point 8.24 notation is 1 << 24.

I observed that with my test code that a linear ramp (where the last entry is set to 1.0) gives me black for white. I tracked it down to this function.

In order to map 1.0 to the maximum value for the table entry in the hardware this function needs to be changed. One way to achieve this would be change the test "blue_int > GAMMA_INT_MAX" to be "blue_int >= GAMMA_INT_MAX" but since GAMMA_INT_MAX = 1 then it might be clearer to say blue_int > 0.

BDW_MAX_GAMMA also looks wrong. I think it should be (1 << 24) - 1 not the 0x10000 (see below). As well as correct clamping it is also necessary to scale as Daniel suggested on IRC:

"
13:54 < danvet> robster, so we'd need to rescale in the kernel from 1.0 to 0.1111111111b ?
13:55 < danvet> well substract 0.0000000001b for all values > 0.5 probably since float math in the kernel is evil
13:56 < danvet> also this probably needs an igt - check that all black with an all 1.0 gamma table is the same as 
                all white with a linear one "

You won't see this with your test program (color-correction.c) as the largest value you program in appears to be 0.ff

*snip*

> +/* Gen 9 */

> +#define BDW_MAX_GAMMA				0x10000

> 


*snip*

I look forward to testing against your next version.

Rob
Matheson, Annie J Sept. 30, 2015, 4:31 p.m. UTC | #3
Shashank,

Please let us know when the next patch series would be ready assuming you incorporate the feedback from Rob. 

I would like to hope we're done with feedback at this point. 

Thanks. 

Annie Matheson

> On Sep 30, 2015, at 9:25 AM, Sharma, Shashank <shashank.sharma@intel.com> wrote:
> 
> Hey Rob, 
> 
> Thanks for the feedback, and the testing efforts. 
> I will check into your suggestions and get back. 
> 
> Regards
> Shashank
> -----Original Message-----
> From: Bradford, Robert 
> Sent: Wednesday, September 30, 2015 8:02 PM
> To: Sharma, Shashank; Roper, Matthew D; Bish, Jim; Smith, Gary K; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
> Cc: Vetter, Daniel; Matheson, Annie J; Mukherjee, Indranil; Palleti, Avinash Reddy; kausalmalladi@gmail.com
> Subject: Re: [PATCH 21/23] drm/i915: BDW: Pipe level Gamma correction
> 
> Hi Shashank, some feedback below that you would be great to get addressed before your next version.
> 
>> On Wed, 2015-09-16 at 23:07 +0530, Shashank Sharma wrote:
>> 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 platforms
>> 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 | 270
>> +++++++++++++++++++++++++++++
> 
> *snip*
> 
>> +static u32 bdw_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 = BDW_MAX_GAMMA;
>> +
>> +    green_int = _GAMMA_INT_PART(green);
>> +    if (green_int > GAMMA_INT_MAX)
>> +        green = BDW_MAX_GAMMA;
>> +
>> +    red_int = _GAMMA_INT_PART(red);
>> +    if (red_int > GAMMA_INT_MAX)
>> +        red = BDW_MAX_GAMMA;
>> +
>> +    blue_fract = _GAMMA_FRACT_PART(blue);
>> +    green_fract = _GAMMA_FRACT_PART(green);
>> +    red_fract = _GAMMA_FRACT_PART(red);
>> +
>> +    blue_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
>> +    green_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
>> +    red_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
>> +
>> +    /* Red (29:20) Green (19:10) and Blue (9:0) */
>> +    word = red_fract;
>> +    word <<= BDW_GAMMA_SHIFT;
>> +    word = word | green_fract;
>> +    word <<= BDW_GAMMA_SHIFT;
>> +    word = word | blue_fract;
>> +
>> +    return word;
>> +}
>> +
> 
> I think the above function, and perhaps others in this series have the same flaw with respect to maximum colour value.
> 
> In our discussions we agreed that we would follow the "GL style" where maximum colour (i.e. 255 in 8-bit) would be represented by 1.0f. 1.0f when converted to fixed point 8.24 notation is 1 << 24.
> 
> I observed that with my test code that a linear ramp (where the last entry is set to 1.0) gives me black for white. I tracked it down to this function.
> 
> In order to map 1.0 to the maximum value for the table entry in the hardware this function needs to be changed. One way to achieve this would be change the test "blue_int > GAMMA_INT_MAX" to be "blue_int >= GAMMA_INT_MAX" but since GAMMA_INT_MAX = 1 then it might be clearer to say blue_int > 0.
> 
> BDW_MAX_GAMMA also looks wrong. I think it should be (1 << 24) - 1 not the 0x10000 (see below). As well as correct clamping it is also necessary to scale as Daniel suggested on IRC:
> 
> "
> 13:54 < danvet> robster, so we'd need to rescale in the kernel from 1.0 to 0.1111111111b ?
> 13:55 < danvet> well substract 0.0000000001b for all values > 0.5 probably since float math in the kernel is evil
> 13:56 < danvet> also this probably needs an igt - check that all black with an all 1.0 gamma table is the same as 
>                all white with a linear one "
> 
> You won't see this with your test program (color-correction.c) as the largest value you program in appears to be 0.ff
> 
> *snip*
> 
>> +/* Gen 9 */
>> +#define BDW_MAX_GAMMA                0x10000
> 
> *snip*
> 
> I look forward to testing against your next version.
> 
> Rob
Ville Syrjälä Sept. 30, 2015, 4:44 p.m. UTC | #4
On Wed, Sep 30, 2015 at 03:31:34PM +0100, Rob Bradford wrote:
> Hi Shashank, some feedback below that you would be great to get
> addressed before your next version.
> 
> On Wed, 2015-09-16 at 23:07 +0530, Shashank Sharma wrote:
> > 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 platforms
> > 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 | 270
> > +++++++++++++++++++++++++++++
> 
> *snip*
> 
> > +static u32 bdw_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 = BDW_MAX_GAMMA;
> > +
> > +	green_int = _GAMMA_INT_PART(green);
> > +	if (green_int > GAMMA_INT_MAX)
> > +		green = BDW_MAX_GAMMA;
> > +
> > +	red_int = _GAMMA_INT_PART(red);
> > +	if (red_int > GAMMA_INT_MAX)
> > +		red = BDW_MAX_GAMMA;
> > +
> > +	blue_fract = _GAMMA_FRACT_PART(blue);
> > +	green_fract = _GAMMA_FRACT_PART(green);
> > +	red_fract = _GAMMA_FRACT_PART(red);
> > +
> > +	blue_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
> > +	green_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
> > +	red_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
> > +
> > +	/* Red (29:20) Green (19:10) and Blue (9:0) */
> > +	word = red_fract;
> > +	word <<= BDW_GAMMA_SHIFT;
> > +	word = word | green_fract;
> > +	word <<= BDW_GAMMA_SHIFT;
> > +	word = word | blue_fract;
> > +
> > +	return word;
> > +}
> > +
> 
> I think the above function, and perhaps others in this series have the
> same flaw with respect to maximum colour value.
> 
> In our discussions we agreed that we would follow the "GL style" where
> maximum colour (i.e. 255 in 8-bit) would be represented by 1.0f. 1.0f
> when converted to fixed point 8.24 notation is 1 << 24.
> 
> I observed that with my test code that a linear ramp (where the last
> entry is set to 1.0) gives me black for white. I tracked it down to
> this function.
> 
> In order to map 1.0 to the maximum value for the table entry in the
> hardware this function needs to be changed. One way to achieve this
> would be change the test "blue_int > GAMMA_INT_MAX" to be "blue_int >=
> GAMMA_INT_MAX" but since GAMMA_INT_MAX = 1 then it might be clearer to
> say blue_int > 0.
> 
> BDW_MAX_GAMMA also looks wrong. I think it should be (1 << 24) - 1 not
> the 0x10000 (see below). As well as correct clamping it is also
> necessary to scale as Daniel suggested on IRC:
> 
> "
> 13:54 < danvet> robster, so we'd need to rescale in the kernel from 1.0
> to 0.1111111111b ?
> 13:55 < danvet> well substract 0.0000000001b for all values > 0.5
> probably since float math in the kernel is evil

Or just clamp to '(1<<bits)-1'. I think the end result is the same
actually.
Sharma, Shashank Sept. 30, 2015, 5:15 p.m. UTC | #5
Hi Annie, 
This might add a day or two, but considering this is a long weekend in India, you can expect the patches by mid next week. 

Regards
Shashank
-----Original Message-----
From: Matheson, Annie J 
Sent: Wednesday, September 30, 2015 10:01 PM
To: Sharma, Shashank
Cc: Bradford, Robert; Roper, Matthew D; Bish, Jim; Smith, Gary K; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Vetter, Daniel; Mukherjee, Indranil; Palleti, Avinash Reddy; kausalmalladi@gmail.com
Subject: Re: [PATCH 21/23] drm/i915: BDW: Pipe level Gamma correction

Shashank,

Please let us know when the next patch series would be ready assuming you incorporate the feedback from Rob. 

I would like to hope we're done with feedback at this point. 

Thanks. 

Annie Matheson

> On Sep 30, 2015, at 9:25 AM, Sharma, Shashank <shashank.sharma@intel.com> wrote:
> 
> Hey Rob, 
> 
> Thanks for the feedback, and the testing efforts. 
> I will check into your suggestions and get back. 
> 
> Regards
> Shashank
> -----Original Message-----
> From: Bradford, Robert 
> Sent: Wednesday, September 30, 2015 8:02 PM
> To: Sharma, Shashank; Roper, Matthew D; Bish, Jim; Smith, Gary K; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
> Cc: Vetter, Daniel; Matheson, Annie J; Mukherjee, Indranil; Palleti, Avinash Reddy; kausalmalladi@gmail.com
> Subject: Re: [PATCH 21/23] drm/i915: BDW: Pipe level Gamma correction
> 
> Hi Shashank, some feedback below that you would be great to get addressed before your next version.
> 
>> On Wed, 2015-09-16 at 23:07 +0530, Shashank Sharma wrote:
>> 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 platforms
>> 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 | 270
>> +++++++++++++++++++++++++++++
> 
> *snip*
> 
>> +static u32 bdw_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 = BDW_MAX_GAMMA;
>> +
>> +    green_int = _GAMMA_INT_PART(green);
>> +    if (green_int > GAMMA_INT_MAX)
>> +        green = BDW_MAX_GAMMA;
>> +
>> +    red_int = _GAMMA_INT_PART(red);
>> +    if (red_int > GAMMA_INT_MAX)
>> +        red = BDW_MAX_GAMMA;
>> +
>> +    blue_fract = _GAMMA_FRACT_PART(blue);
>> +    green_fract = _GAMMA_FRACT_PART(green);
>> +    red_fract = _GAMMA_FRACT_PART(red);
>> +
>> +    blue_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
>> +    green_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
>> +    red_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
>> +
>> +    /* Red (29:20) Green (19:10) and Blue (9:0) */
>> +    word = red_fract;
>> +    word <<= BDW_GAMMA_SHIFT;
>> +    word = word | green_fract;
>> +    word <<= BDW_GAMMA_SHIFT;
>> +    word = word | blue_fract;
>> +
>> +    return word;
>> +}
>> +
> 
> I think the above function, and perhaps others in this series have the same flaw with respect to maximum colour value.
> 
> In our discussions we agreed that we would follow the "GL style" where maximum colour (i.e. 255 in 8-bit) would be represented by 1.0f. 1.0f when converted to fixed point 8.24 notation is 1 << 24.
> 
> I observed that with my test code that a linear ramp (where the last entry is set to 1.0) gives me black for white. I tracked it down to this function.
> 
> In order to map 1.0 to the maximum value for the table entry in the hardware this function needs to be changed. One way to achieve this would be change the test "blue_int > GAMMA_INT_MAX" to be "blue_int >= GAMMA_INT_MAX" but since GAMMA_INT_MAX = 1 then it might be clearer to say blue_int > 0.
> 
> BDW_MAX_GAMMA also looks wrong. I think it should be (1 << 24) - 1 not the 0x10000 (see below). As well as correct clamping it is also necessary to scale as Daniel suggested on IRC:
> 
> "
> 13:54 < danvet> robster, so we'd need to rescale in the kernel from 1.0 to 0.1111111111b ?
> 13:55 < danvet> well substract 0.0000000001b for all values > 0.5 probably since float math in the kernel is evil
> 13:56 < danvet> also this probably needs an igt - check that all black with an all 1.0 gamma table is the same as 
>                all white with a linear one "
> 
> You won't see this with your test program (color-correction.c) as the largest value you program in appears to be 0.ff
> 
> *snip*
> 
>> +/* Gen 9 */
>> +#define BDW_MAX_GAMMA                0x10000
> 
> *snip*
> 
> I look forward to testing against your next version.
> 
> Rob
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 5c7759d..88f4e41 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5634,7 +5634,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)
@@ -8001,6 +8003,17 @@  enum skl_disp_power_wells {
 #define _PIPE_CSC_BASE(pipe) \
 	(_PIPE3(pipe, PIPEA_CGM_CSC, PIPEB_CGM_CSC, PIPEC_CGM_CSC))
 
-
+/* BDW gamma correction */
+#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 _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 d33a2be..d935fd8 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.c
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -264,6 +264,274 @@  static int chv_set_degamma(struct drm_device *dev,
 	return ret;
 }
 
+static u32 bdw_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 = BDW_MAX_GAMMA;
+
+	green_int = _GAMMA_INT_PART(green);
+	if (green_int > GAMMA_INT_MAX)
+		green = BDW_MAX_GAMMA;
+
+	red_int = _GAMMA_INT_PART(red);
+	if (red_int > GAMMA_INT_MAX)
+		red = BDW_MAX_GAMMA;
+
+	blue_fract = _GAMMA_FRACT_PART(blue);
+	green_fract = _GAMMA_FRACT_PART(green);
+	red_fract = _GAMMA_FRACT_PART(red);
+
+	blue_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
+	green_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
+	red_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
+
+	/* Red (29:20) Green (19:10) and Blue (9:0) */
+	word = red_fract;
+	word <<= BDW_GAMMA_SHIFT;
+	word = word | green_fract;
+	word <<= BDW_GAMMA_SHIFT;
+	word = word | blue_fract;
+
+	return word;
+}
+
+/* Apply unity gamma for gamma reset */
+static void bdw_reset_gamma(struct drm_i915_private *dev_priv,
+			enum pipe pipe)
+{
+	u16 count = 0;
+	u32 val;
+	u32 pal_prec_data = LGC_PALETTE(pipe);
+
+	DRM_DEBUG_DRIVER("\n");
+
+	/* Reset the palette for unit gamma */
+	while (count < BDW_8BIT_GAMMA_MAX_VALS) {
+		/* Red (23:16) Green (15:8) and Blue (7:0) */
+		val = (count << 16) | (count << 8) | count;
+		I915_WRITE(pal_prec_data, val);
+		pal_prec_data += 4;
+		count++;
+	}
+}
+
+static int bdw_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;
+
+	enum pipe pipe;
+	int count, num_samples;
+	u32 blue, green, red;
+	u32 mode, pal_prec_index, pal_prec_data;
+	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 != GAMMA_DATA_STRUCT_VERSION) {
+		DRM_DEBUG_KMS("Invalid Gamma Data struct version\n");
+		return -EINVAL;
+	}
+
+	pipe = to_intel_crtc(crtc)->pipe;
+	num_samples = gamma_data->num_samples;
+
+	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 0:
+
+		/* Disable Gamma functionality on Pipe */
+		DRM_DEBUG_DRIVER("Disabling gamma on Pipe %c\n",
+				pipe_name(pipe));
+		bdw_reset_gamma(dev_priv, pipe);
+		word = GAMMA_MODE_MODE_8BIT;
+		break;
+
+	case BDW_8BIT_GAMMA_MAX_VALS:
+
+		/* Legacy palette */
+		pal_prec_data = LGC_PALETTE(pipe);
+
+		count = 0;
+		while (count < BDW_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 = BDW_MAX_GAMMA;
+			green_int = _GAMMA_INT_PART(green);
+			if (green_int > GAMMA_INT_MAX)
+				green = BDW_MAX_GAMMA;
+			red_int = _GAMMA_INT_PART(red);
+			if (red_int > GAMMA_INT_MAX)
+				red = BDW_MAX_GAMMA;
+
+			blue_fract = _GAMMA_FRACT_PART(blue);
+			green_fract = _GAMMA_FRACT_PART(green);
+			red_fract = _GAMMA_FRACT_PART(red);
+
+			blue_fract >>= BDW_8BIT_GAMMA_MSB_SHIFT;
+			green_fract >>= BDW_8BIT_GAMMA_MSB_SHIFT;
+			red_fract >>= BDW_8BIT_GAMMA_MSB_SHIFT;
+
+			/* Red (23:16) Green (15:8) and Blue (7:0) */
+			word = red_fract;
+			word <<= BDW_8BIT_GAMMA_SHIFT;
+			word = word | green_fract;
+			word <<= BDW_8BIT_GAMMA_SHIFT;
+			word = word | blue_fract;
+
+			I915_WRITE(pal_prec_data, word);
+			pal_prec_data += 4;
+
+			count++;
+		}
+
+		word = GAMMA_MODE_MODE_8BIT;
+		break;
+
+	case BDW_SPLITGAMMA_MAX_VALS:
+
+		index |= BDW_INDEX_AUTO_INCREMENT | BDW_INDEX_SPLIT_MODE;
+		I915_WRITE(pal_prec_index, index);
+
+		count = 0;
+		while (count < BDW_SPLITGAMMA_MAX_VALS) {
+			blue = correction_values[count].b32;
+			green = correction_values[count].g32;
+			red = correction_values[count].r32;
+
+			word = bdw_write_10bit_gamma_precision(red,
+					green, blue);
+			I915_WRITE(pal_prec_data, word);
+			count++;
+		}
+
+		word = GAMMA_MODE_MODE_SPLIT;
+		break;
+
+	case BDW_12BIT_GAMMA_MAX_VALS:
+
+		index |= BDW_INDEX_AUTO_INCREMENT;
+		index &= ~BDW_INDEX_SPLIT_MODE;
+		I915_WRITE(pal_prec_index, index);
+
+		count = 0;
+		while (count < BDW_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 = BDW_MAX_GAMMA;
+			green_int = _GAMMA_INT_PART(green);
+			if (green_int > GAMMA_INT_MAX)
+				green = BDW_MAX_GAMMA;
+			red_int = _GAMMA_INT_PART(red);
+			if (red_int > GAMMA_INT_MAX)
+				red = BDW_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 >>
+					BDW_12BIT_GAMMA_ODD_SHIFT;
+				green_odd = green_fract >>
+					BDW_12BIT_GAMMA_ODD_SHIFT;
+				red_odd = red_fract >>
+					BDW_12BIT_GAMMA_ODD_SHIFT;
+
+				word = red_odd;
+				word = word << BDW_GAMMA_SHIFT;
+				word = word | green_odd;
+				word = word << BDW_GAMMA_SHIFT;
+				word = word | blue_odd;
+			} else {
+				blue_even = blue_fract <<
+					BDW_12BIT_GAMMA_EVEN_SHIFT1 >>
+					BDW_12BIT_GAMMA_EVEN_SHIFT2;
+				green_even = green_fract <<
+					BDW_12BIT_GAMMA_EVEN_SHIFT1 >>
+					BDW_12BIT_GAMMA_EVEN_SHIFT2;
+				red_even = red_fract <<
+					BDW_12BIT_GAMMA_EVEN_SHIFT1 >>
+					BDW_12BIT_GAMMA_EVEN_SHIFT2;
+
+				word = red_even;
+				word = word << BDW_GAMMA_SHIFT;
+				word = word | green_even;
+				word = word << BDW_GAMMA_SHIFT;
+				word = word | blue_even;
+			}
+			I915_WRITE(pal_prec_data, word);
+			count++;
+		}
+
+		word = GAMMA_MODE_MODE_12BIT;
+		break;
+
+	case BDW_10BIT_GAMMA_MAX_VALS:
+
+		index |= BDW_INDEX_AUTO_INCREMENT;
+		index &= ~BDW_INDEX_SPLIT_MODE;
+		I915_WRITE(pal_prec_index, index);
+
+		count = 0;
+		while (count < BDW_10BIT_GAMMA_MAX_VALS) {
+			blue = correction_values[count].b32;
+			green = correction_values[count].g32;
+			red = correction_values[count].r32;
+
+			word = bdw_write_10bit_gamma_precision(red,
+						green, blue);
+			I915_WRITE(pal_prec_data, word);
+			count++;
+		}
+
+		word = GAMMA_MODE_MODE_10BIT;
+		break;
+
+	default:
+		DRM_ERROR("Invalid number of samples\n");
+		return -EINVAL;
+	}
+
+	/* Set gamma mode on pipe control reg */
+	mode = I915_READ(GAMMA_MODE(pipe));
+	mode &= ~GAMMA_MODE_MODE_MASK;
+	I915_WRITE(GAMMA_MODE(pipe), mode | word);
+	DRM_DEBUG_DRIVER("Gamma applied on pipe %c\n",
+			pipe_name(pipe));
+	return 0;
+}
+
 static int chv_set_gamma(struct drm_device *dev, struct drm_property_blob *blob,
 		  struct drm_crtc *crtc)
 {
@@ -418,6 +686,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 = bdw_set_gamma(dev, blob, crtc);
 
 		if (ret)
 			DRM_ERROR("set Gamma correction failed\n");
diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
index 3687989..17fcf3d 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.h
+++ b/drivers/gpu/drm/i915/intel_color_manager.h
@@ -43,6 +43,9 @@ 
 #define CHV_GAMMA_SHIFT_GREEN			16
 
 #define BDW_SPLITGAMMA_MAX_VALS		512
+#define BDW_8BIT_GAMMA_MAX_VALS               256
+#define BDW_10BIT_GAMMA_MAX_VALS              1024
+#define BDW_12BIT_GAMMA_MAX_VALS              513
 
 /* Gamma values are u8.24 format */
 #define GAMMA_INT_SHIFT			24
@@ -52,6 +55,18 @@ 
 /* Max value for Gamma on CHV */
 #define CHV_MAX_GAMMA				0x10000
 
+/* Gen 9 */
+#define BDW_MAX_GAMMA				0x10000
+#define BDW_10BIT_GAMMA_MSB_SHIFT		6
+#define BDW_GAMMA_SHIFT			10
+#define BDW_INDEX_AUTO_INCREMENT		(1 << 15)
+#define BDW_INDEX_SPLIT_MODE			(1 << 31)
+#define BDW_8BIT_GAMMA_MSB_SHIFT		8
+#define BDW_8BIT_GAMMA_SHIFT			8
+#define BDW_12BIT_GAMMA_ODD_SHIFT		6
+#define BDW_12BIT_GAMMA_EVEN_SHIFT1		10
+#define BDW_12BIT_GAMMA_EVEN_SHIFT2		6
+
 /* DeGamma correction */
 #define DEGAMMA_DATA_STRUCT_VERSION        1
 #define CHV_DEGAMMA_MSB_SHIFT                  2