Re: [PATCH] drm/i915: Fix Limited Range Color Handling

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

On Fri, Dec 22, 2017 at 05:54:31PM +0200, Ville Syrjälä wrote:
> On Fri, Dec 22, 2017 at 08:34:47PM +0530, Uma Shankar wrote:
> > From: Johnson Lin <johnson.lin@intel.com>
> > 
> > Some panels support limited range output (16-235) compared
> > to full range RGB values (0-255). Also userspace can control
> > the RGB range using "Broadcast RGB" property. Currently the
> > code to handle full range to limited range is broken. This
> > patch fixes the same by properly scaling down all the full
> > range co-efficients with limited range scaling factor.
> > 
> > Signed-off-by: Johnson Lin <johnson.lin@intel.com>
> > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_color.c |   16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> > index aa66e95..777ce26 100644
> > --- a/drivers/gpu/drm/i915/intel_color.c
> > +++ b/drivers/gpu/drm/i915/intel_color.c
> > @@ -94,16 +94,24 @@ static void ctm_mult_by_limited(uint64_t *result, int64_t *input)
> >  	for (i = 0; i < 9; i++)
> >  		result[i] = 0;
> 
> This can go.
> 
> >  
> > -	for (i = 0; i < 3; i++) {
> > -		int64_t user_coeff = input[i * 3 + i];
> > +	for (i = 0; i < 9; i++) {
> > +		int64_t user_coeff = input[i];
> 
> That's not a two's complement number so should probably be u64.
> 
> >  		uint64_t limited_coeff = CTM_COEFF_LIMITED_RANGE >> 2;
> >  		uint64_t abs_coeff = clamp_val(CTM_COEFF_ABS(user_coeff),
> >  					       0,
> >  					       CTM_COEFF_4_0 - 1) >> 2;
> 
> Seems to me that we should be able to drop the >>2 from the limited_coeff
> since it's always < 1.0, make both of these u32 and then use
> 'mul_u32_u32() >> 30' to get the answer.
> 
> >  
> > -		result[i * 3 + i] = (limited_coeff * abs_coeff) >> 27;
> > +		/*
> > +		 * By scaling every co-efficient with limited range (235-16)
> > +		 * vs full range (0-255) the final o/p will be scaled down to
> > +		 * fit in the limited range supported by the panel.
> > +		 * Need to shift the multiplication result by 28 as the floating
> > +		 * count expected is of 32bits, multiplication here is done in
> > +		 * U2.30 so result need to be right shifted by 60-32 = 28
> > +		 */
> > +		result[i] = (limited_coeff * abs_coeff) >> 28;
> >  		if (CTM_COEFF_NEGATIVE(user_coeff))
> 
> And this can be replaced with just
> 
> result[i] |= user_coeff & CTM_COEFF_SIGN;
> 
> to eliminate the silly branch.

I wonder if we could also get some igts for this limited range stuff.
Maybe something like:
1. grab crc from black/white screen with limited range
2. grab cdc from almost black/white screen with full range
3. compare the two

Repeat with an identity csc matrix set.

Not sure if we can get the crcs to match in these two cases though.
Hard to say without trying it out.

> 
> 
> > -			result[i * 3 + i] |= CTM_COEFF_SIGN;
> > +			result[i] |= CTM_COEFF_SIGN;
> >  	}
> >  }
> >  
> > -- 
> > 1.7.9.5
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx