>-----Original Message----- >From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] >Sent: Friday, December 22, 2017 9:25 PM >To: Shankar, Uma <uma.shankar@xxxxxxxxx> >Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Lin, Johnson <johnson.lin@xxxxxxxxx>; >Syrjala, Ville <ville.syrjala@xxxxxxxxx>; Lankhorst, Maarten ><maarten.lankhorst@xxxxxxxxx> >Subject: Re: [PATCH] drm/i915: Fix Limited Range Color Handling > >On Fri, Dec 22, 2017 at 08:34:47PM +0530, Uma Shankar wrote: >> From: Johnson Lin <johnson.lin@xxxxxxxxx> >> >> 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@xxxxxxxxx> >> Signed-off-by: Uma Shankar <uma.shankar@xxxxxxxxx> >> --- >> 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. > Yes, will remove this. >> >> - 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. Ok, will change this. > >> 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. Yes, this should work. Will change this. > >> >> - 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. > Sure, this can be changed. Will update this. Thanks Ville for your comments. Regards, Uma Shankar > >> - result[i * 3 + i] |= CTM_COEFF_SIGN; >> + result[i] |= CTM_COEFF_SIGN; >> } >> } >> >> -- >> 1.7.9.5 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >-- >Ville Syrjälä >Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
