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

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

On Tue, Jan 30, 2018 at 03:23:32PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >Sent: Friday, December 22, 2017 9:43 PM
> >To: Shankar, Uma <uma.shankar@intel.com>
> >Cc: intel-gfx@lists.freedesktop.org; Lin, Johnson <johnson.lin@intel.com>;
> >Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
> ><maarten.lankhorst@intel.com>
> >Subject: Re: [Intel-gfx] [PATCH] drm/i915: Fix Limited Range Color Handling
> >
> >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.
> >
> 
> Hi Ville,
> We will try this approach and update. Ideally crc's should match, will confirm the
> behavior on hardware.

I suspect it's not going to work on some older platforms because we use the
magic bit in the pipe/port register to do the range compression. The
effect of that bit may not be visible in the pipe crc. But starting from
hsw it should be ok.

> >>
> >>
> >> > -			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

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