On 10 October 2015 at 06:26, Sharma, Shashank <shashank.sharma@xxxxxxxxx> wrote:
> On 10/10/2015 5:13 AM, Emil Velikov wrote:
>>
>> On 9 October 2015 at 20:29, Shashank Sharma <shashank.sharma@xxxxxxxxx>
>> wrote:
>>>
>>> CHV/BSW supports Color Space Conversion (CSC) using a 3x3 matrix
>>> that needs to be programmed into CGM (Color Gamut Mapping) registers.
>>>
>>> This patch does the following:
>>> 1. Attaches CSC property to CRTC
>>> 2. Adds the core function to program CSC correction values
>>> 3. Adds CSC correction macros
>>>
>>> Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxxxx>
>>> Signed-off-by: Kausal Malladi <kausalmalladi@xxxxxxxxx>
>>> Signed-off-by: Kumar, Kiran S <kiran.s.kumar@xxxxxxxxx>
>>> ---
>>> drivers/gpu/drm/i915/i915_reg.h | 8 +++
>>> drivers/gpu/drm/i915/intel_color_manager.c | 94
>>> ++++++++++++++++++++++++++++++
>>> drivers/gpu/drm/i915/intel_color_manager.h | 19 ++++++
>>> 3 files changed, 121 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>>> b/drivers/gpu/drm/i915/i915_reg.h
>>> index c32e35d..5825ab2 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -8056,4 +8056,12 @@ enum skl_disp_power_wells {
>>> #define _PIPE_DEGAMMA_BASE(pipe) \
>>> (_PIPE3(pipe, PIPEA_CGM_DEGAMMA, PIPEB_CGM_DEGAMMA,
>>> PIPEC_CGM_DEGAMMA))
>>>
>>> +#define PIPEA_CGM_CSC (VLV_DISPLAY_BASE +
>>> 0x67900)
>>> +#define PIPEB_CGM_CSC (VLV_DISPLAY_BASE +
>>> 0x69900)
>>> +#define PIPEC_CGM_CSC (VLV_DISPLAY_BASE +
>>> 0x6B900)
>>> +#define _PIPE_CSC_BASE(pipe) \
>>> + (_PIPE3(pipe, PIPEA_CGM_CSC, PIPEB_CGM_CSC, PIPEC_CGM_CSC))
>>> +
>>> +
>>> +
>>> #endif /* _I915_REG_H_ */
>>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c
>>> b/drivers/gpu/drm/i915/intel_color_manager.c
>>> index bbfe185..433e50a 100644
>>> --- a/drivers/gpu/drm/i915/intel_color_manager.c
>>> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
>>> @@ -27,6 +27,93 @@
>>>
>>> #include "intel_color_manager.h"
>>>
>>> +static s16 chv_prepare_csc_coeff(s64 csc_value)
>>> +{
>>> + s32 csc_int_value;
>>> + u32 csc_fract_value;
>>> + s16 csc_s3_12_format;
>>
>> The type of csc_s3_12_format and chv_prepare_csc_coeff() does not see
>> correct. Seem like the fix got merged into another patch :\
>>
> Can you please elaborate this comment, I dont get it.
>
You have two typos above s16 > s32 which you've fixed in the next
patch. That fix should belong here imho.
[snip]
>>> + while (count < CSC_MAX_VALS) {
>>> + temp = chv_prepare_csc_coeff(
>>> + csc_data->ctm_coeff[count]);
>>> + SET_BITS(word, GET_BITS(temp, 16, 16), 0, 16);
>>> +
>>> + /*
>>> + * Last value to be written in 1 register.
>>> + * Otherwise, each pair of CSC values go
>>> + * into 1 register
>>> + */
>>> + if (count != (CSC_MAX_VALS - 1)) {
>>> + count++;
>>> + temp = chv_prepare_csc_coeff(
>>> + csc_data->ctm_coeff[count]);
>>> + SET_BITS(word, GET_BITS(temp, 16, 16), 16, 16);
>>> + }
>>
>> This looks a bit odd. Use the same approach as in
>> bdw_write_12bit_gamma_precision() ?
>
> Again, can you please give little more details here ?
Take a look at the loop construct in bdw_write_12bit_gamma_precision()
- both of them are essentially doing the same thing.
Here you have
while(i < odd_number) {
foo()
if (if != odd_number-1) {
I++
foo()
}
}
while in the mentioned function
while (i < odd_number -1) {
foo()
foo()
i++
}
foo()
Normally you'd use one or the other. Esp. since this is a single
patchset :-) I'm leaning towards the latter as it's more obvious but
others may prefer the former approach.
Regards,
Emil
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
