diff mbox

[v4] drm/i915: Fix Limited Range Color Handling

Message ID 1517327489-26128-1-git-send-email-uma.shankar@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Shankar, Uma Jan. 30, 2018, 3:51 p.m. UTC
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

v4: Fixed some missing data type corrections.

Signed-off-by: Johnson Lin <johnson.lin@intel.com>
Signed-off-by: Uma Shankar <uma.shankar@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_color.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

Comments

Ville Syrjälä Feb. 22, 2018, 4:15 p.m. UTC | #1
On Tue, Jan 30, 2018 at 09:21:29PM +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
> 
> v4: Fixed some missing data type corrections.
> 
> Signed-off-by: Johnson Lin <johnson.lin@intel.com>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

OK, so maybe we can't get the test done or maybe we can. Either way
I've gone and pushed this to dinq. Thanks for the patch.

> ---
>  drivers/gpu/drm/i915/intel_color.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 333a23b..0b505d4 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -84,26 +84,25 @@ 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];
> +		u32 limited_coeff = CTM_COEFF_LIMITED_RANGE;
> +		u32 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;
>  	}
>  }
>  
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 333a23b..0b505d4 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -84,26 +84,25 @@  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];
+		u32 limited_coeff = CTM_COEFF_LIMITED_RANGE;
+		u32 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;
 	}
 }