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@xxxxxxxxx>
> > Signed-off-by: Kausal Malladi <kausalmalladi@xxxxxxxxx>
> > ---
> > 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.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
