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 at lists.freedesktop.org; intel-gfx at lists.freedesktop.org; Vetter, Daniel; Mukherjee, Indranil; Palleti, Avinash Reddy; kausalmalladi at 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 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