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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel