| Message ID | 1517325562-20523-1-git-send-email-uma.shankar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 30, 2018 at 08:49:22PM +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. > > v2: Fixed Ville's review comments. > > v3: Changed input to const and used correct data types as > suggested by Ville > > Signed-off-by: Johnson Lin <johnson.lin@intel.com> > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > Reviewed-by: Ville Syrjä <ville.syrjala@linux.intel.com> Name fail. > --- > drivers/gpu/drm/i915/intel_color.c | 31 ++++++++++++++++--------------- > 1 file changed, 16 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c > index 333a23b..3b708a9 100644 > --- a/drivers/gpu/drm/i915/intel_color.c > +++ b/drivers/gpu/drm/i915/intel_color.c > @@ -84,26 +84,27 @@ static bool crtc_state_is_legacy_gamma(struct drm_crtc_state *state) > > /* > * When using limited range, multiply the matrix given by userspace by > - * the matrix that we would use for the limited range. We do the > - * multiplication in U2.30 format. > + * the matrix that we would use for the limited range. > */ > -static void ctm_mult_by_limited(uint64_t *result, int64_t *input) > +static void ctm_mult_by_limited(u64 *result, const u64 *input) > { > int i; > > - for (i = 0; i < 9; i++) > - result[i] = 0; > + for (i = 0; i < 9; i++) { > + u64 user_coeff = input[i]; > + uint32_t limited_coeff = CTM_COEFF_LIMITED_RANGE; You forgot s/uint32_t/u32/ here... > + uint32_t abs_coeff = clamp_val( and here. And aving a newline here seems weird. > + CTM_COEFF_ABS(user_coeff), > + 0, > + CTM_COEFF_4_0 - 1) >> 2; > > - for (i = 0; i < 3; i++) { > - int64_t user_coeff = input[i * 3 + i]; > - 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; > - > - result[i * 3 + i] = (limited_coeff * abs_coeff) >> 27; > - if (CTM_COEFF_NEGATIVE(user_coeff)) > - result[i * 3 + i] |= CTM_COEFF_SIGN; > + /* > + * By scaling every co-efficient with limited range (16-235) > + * vs full range (0-255) the final o/p will be scaled down to > + * fit in the limited range supported by the panel. > + */ > + result[i] = mul_u32_u32(limited_coeff, abs_coeff) >> 30; > + result[i] |= user_coeff & CTM_COEFF_SIGN; > } > } > > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>-----Original Message----- >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >Sent: Tuesday, January 30, 2018 8:53 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] [v3] drm/i915: Fix Limited Range Color Handling > >On Tue, Jan 30, 2018 at 08:49:22PM +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. >> >> v2: Fixed Ville's review comments. >> >> v3: Changed input to const and used correct data types as >> suggested by Ville >> >> Signed-off-by: Johnson Lin <johnson.lin@intel.com> >> Signed-off-by: Uma Shankar <uma.shankar@intel.com> >> Reviewed-by: Ville Syrjä <ville.syrjala@linux.intel.com> > >Name fail. > Have updated this. >> --- >> drivers/gpu/drm/i915/intel_color.c | 31 >> ++++++++++++++++--------------- >> 1 file changed, 16 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_color.c >> b/drivers/gpu/drm/i915/intel_color.c >> index 333a23b..3b708a9 100644 >> --- a/drivers/gpu/drm/i915/intel_color.c >> +++ b/drivers/gpu/drm/i915/intel_color.c >> @@ -84,26 +84,27 @@ static bool crtc_state_is_legacy_gamma(struct >> drm_crtc_state *state) >> >> /* >> * When using limited range, multiply the matrix given by userspace >> by >> - * the matrix that we would use for the limited range. We do the >> - * multiplication in U2.30 format. >> + * the matrix that we would use for the limited range. >> */ >> -static void ctm_mult_by_limited(uint64_t *result, int64_t *input) >> +static void ctm_mult_by_limited(u64 *result, const u64 *input) >> { >> int i; >> >> - for (i = 0; i < 9; i++) >> - result[i] = 0; >> + for (i = 0; i < 9; i++) { >> + u64 user_coeff = input[i]; >> + uint32_t limited_coeff = CTM_COEFF_LIMITED_RANGE; > >You forgot s/uint32_t/u32/ here... This file is using this extensively, have changed this function though. I guess it will need a bigger cleanup. > >> + uint32_t abs_coeff = clamp_val( > >and here. And aving a newline here seems weird. Have updated this and re-sent a new patch. Thanks & Regards, Uma Shankar > >> + CTM_COEFF_ABS(user_coeff), >> + 0, >> + CTM_COEFF_4_0 - 1) >> 2; >> >> - for (i = 0; i < 3; i++) { >> - int64_t user_coeff = input[i * 3 + i]; >> - 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; >> - >> - result[i * 3 + i] = (limited_coeff * abs_coeff) >> 27; >> - if (CTM_COEFF_NEGATIVE(user_coeff)) >> - result[i * 3 + i] |= CTM_COEFF_SIGN; >> + /* >> + * By scaling every co-efficient with limited range (16-235) >> + * vs full range (0-255) the final o/p will be scaled down to >> + * fit in the limited range supported by the panel. >> + */ >> + result[i] = mul_u32_u32(limited_coeff, abs_coeff) >> 30; >> + result[i] |= user_coeff & CTM_COEFF_SIGN; >> } >> } >> >> -- >> 1.9.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >-- >Ville Syrjälä >Intel OTC
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c index 333a23b..3b708a9 100644 --- a/drivers/gpu/drm/i915/intel_color.c +++ b/drivers/gpu/drm/i915/intel_color.c @@ -84,26 +84,27 @@ static bool crtc_state_is_legacy_gamma(struct drm_crtc_state *state) /* * When using limited range, multiply the matrix given by userspace by - * the matrix that we would use for the limited range. We do the - * multiplication in U2.30 format. + * the matrix that we would use for the limited range. */ -static void ctm_mult_by_limited(uint64_t *result, int64_t *input) +static void ctm_mult_by_limited(u64 *result, const u64 *input) { int i; - for (i = 0; i < 9; i++) - result[i] = 0; + for (i = 0; i < 9; i++) { + u64 user_coeff = input[i]; + uint32_t limited_coeff = CTM_COEFF_LIMITED_RANGE; + uint32_t abs_coeff = clamp_val( + CTM_COEFF_ABS(user_coeff), + 0, + CTM_COEFF_4_0 - 1) >> 2; - for (i = 0; i < 3; i++) { - int64_t user_coeff = input[i * 3 + i]; - 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; - - result[i * 3 + i] = (limited_coeff * abs_coeff) >> 27; - if (CTM_COEFF_NEGATIVE(user_coeff)) - result[i * 3 + i] |= CTM_COEFF_SIGN; + /* + * By scaling every co-efficient with limited range (16-235) + * vs full range (0-255) the final o/p will be scaled down to + * fit in the limited range supported by the panel. + */ + result[i] = mul_u32_u32(limited_coeff, abs_coeff) >> 30; + result[i] |= user_coeff & CTM_COEFF_SIGN; } }