[Intel-gfx] [PATCH 15/22] drm/i915: CHV: Pipe level CSC correction

[Sharma, Shashank]

Regards
Shashank

On 10/13/2015 7:03 PM, Emil Velikov wrote:
> On 10 October 2015 at 06:26, Sharma, Shashank <shashank.sharma at intel.com> wrote:
>> On 10/10/2015 5:13 AM, Emil Velikov wrote:
>>>
>>> On 9 October 2015 at 20:29, Shashank Sharma <shashank.sharma at intel.com>
>>> 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 at intel.com>
>>>> Signed-off-by: Kausal Malladi <kausalmalladi at gmail.com>
>>>> Signed-off-by: Kumar, Kiran S <kiran.s.kumar at intel.com>
>>>> ---
>>>>    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.
>
Yes, I got that later, current patch set contains this fix.
> [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.
>
Yes, I got this one also, later :). New patch set has an implementation 
similar to this, please have a look.

> Regards,
> Emil
>