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
