Re: [Intel-gfx] [v3] drm/i915: Fix Limited Range Color Handling

2018-01-30 Thread Shankar, Uma 


>-Original Message-
>From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com]
>Sent: Tuesday, January 30, 2018 8:53 PM
>To: Shankar, Uma <uma.shan...@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Lin, Johnson <johnson@intel.com>;
>Syrjala, Ville <ville.syrj...@intel.com>; Lankhorst, Maarten
><maarten.lankho...@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@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@intel.com>
>> Signed-off-by: Uma Shankar <uma.shan...@intel.com>
>> Reviewed-by: Ville Syrjä <ville.syrj...@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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [v3] drm/i915: Fix Limited Range Color Handling

2018-01-30 Thread Ville Syrjälä 
On Tue, Jan 30, 2018 at 08:49:22PM +0530, Uma Shankar wrote:
> From: Johnson Lin 
> 
> 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 
> Signed-off-by: Uma Shankar 
> Reviewed-by: Ville Syrjä 

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

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