[Intel-gfx] [PATCH 21/23] drm/i915: BDW: Pipe level Gamma correction

[Matheson, Annie J]

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 at 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 at lists.freedesktop.org; intel-gfx at lists.freedesktop.org
> Cc: Vetter, Daniel; Matheson, Annie J; Mukherjee, Indranil; Palleti, Avinash Reddy; kausalmalladi at 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 at intel.com>
>> Signed-off-by: Kausal Malladi <kausalmalladi at 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