>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com]
>Sent: Friday, December 22, 2017 9:25 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] [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....@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....@intel.com>
>> Signed-off-by: Uma Shankar <uma.shan...@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.
>
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@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
