[PATCH 22/22] drm/i915: BDW: Pipe level CSC correction

2015-10-13 Thread Sharma, Shashank 
Regards
Shashank

On 10/13/2015 7:15 PM, Emil Velikov wrote:
> On 10 October 2015 at 06:34, Sharma, Shashank  
> wrote:
>> On 10/10/2015 5:24 AM, Emil Velikov wrote:
>>>
>>> Hi Shashank,
>>>
>>> On 9 October 2015 at 20:29, Shashank Sharma 
>>> wrote:

 BDW/SKL/BXT support Color Space Conversion (CSC) using a 3x3 matrix
 that needs to be programmed into respective CSC registers.

 This patch does the following:
 1. Adds the core function to program CSC correction values for
  BDW/SKL/BXT platform
 2. Adds CSC correction macros/defines

 Signed-off-by: Shashank Sharma 
 Signed-off-by: Kausal Malladi 
 Signed-off-by: Kumar, Kiran S 
 ---
drivers/gpu/drm/i915/i915_reg.h|   7 ++
drivers/gpu/drm/i915/intel_color_manager.c | 114
 -
drivers/gpu/drm/i915/intel_color_manager.h |  12 ++-
3 files changed, 129 insertions(+), 4 deletions(-)

 diff --git a/drivers/gpu/drm/i915/i915_reg.h
 b/drivers/gpu/drm/i915/i915_reg.h
 index ed50f75..0e9d252 100644
 --- a/drivers/gpu/drm/i915/i915_reg.h
 +++ b/drivers/gpu/drm/i915/i915_reg.h
 @@ -8085,4 +8085,11 @@ enum skl_disp_power_wells {
   (_PIPE3(pipe, PAL_PREC_GCMAX_A, PAL_PREC_GCMAX_B,
 PAL_PREC_GCMAX_C))


 +/* BDW CSC correction */
 +#define CSC_COEFF_A0x49010
 +#define CSC_COEFF_B0x49110
 +#define CSC_COEFF_C0x49210
 +#define _PIPE_CSC_COEFF(pipe) \
 +   (_PIPE3(pipe, CSC_COEFF_A, CSC_COEFF_B, CSC_COEFF_C))
 +
#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 e659382..0a6c00c 100644
 --- a/drivers/gpu/drm/i915/intel_color_manager.c
 +++ b/drivers/gpu/drm/i915/intel_color_manager.c
 @@ -330,11 +330,119 @@ static int bdw_set_degamma(struct drm_device *dev,
   return 0;
}

 -static s16 chv_prepare_csc_coeff(s64 csc_value)
>>>
>>> As mentioned previously, this should be part of the respective patch.
>>>
>> Agree. Looks like diff is messing up a bit. Will take care of this.
>>
 +static uint32_t bdw_prepare_csc_coeff(int64_t coeff)
 +{
 +   uint32_t reg_val, ls_bit_pos, exponent_bits, sign_bit = 0;
 +   int32_t mantissa;
 +   uint64_t abs_coeff;
 +
 +   coeff = min_t(int64_t, coeff, BDW_CSC_COEFF_MAX_VAL);
 +   coeff = max_t(int64_t, coeff, BDW_CSC_COEFF_MIN_VAL);
 +
 +   abs_coeff = abs(coeff);
 +   if (abs_coeff < (BDW_CSC_COEFF_UNITY_VAL >> 3)) {
 +   /* abs_coeff < 0.125 */
 +   exponent_bits = 3;
 +   ls_bit_pos = 19;
 +   } else if (abs_coeff >= (BDW_CSC_COEFF_UNITY_VAL >> 3) &&
 +   abs_coeff < (BDW_CSC_COEFF_UNITY_VAL >> 2)) {
 +   /* abs_coeff >= 0.125 && val < 0.25 */
 +   exponent_bits = 2;
 +   ls_bit_pos = 20;
 +   } else if (abs_coeff >= (BDW_CSC_COEFF_UNITY_VAL >> 2)
 +   && abs_coeff < (BDW_CSC_COEFF_UNITY_VAL >> 1)) {
 +   /* abs_coeff >= 0.25 && val < 0.5 */
 +   exponent_bits = 1;
 +   ls_bit_pos = 21;
 +   } else if (abs_coeff >= (BDW_CSC_COEFF_UNITY_VAL >> 1)
 +   && abs_coeff < BDW_CSC_COEFF_UNITY_VAL) {
 +   /* abs_coeff >= 0.5 && val < 1.0 */
 +   exponent_bits = 0;
 +   ls_bit_pos = 22;
 +   } else if (abs_coeff >= BDW_CSC_COEFF_UNITY_VAL &&
 +   abs_coeff < (BDW_CSC_COEFF_UNITY_VAL << 1)) {
 +   /* abs_coeff >= 1.0 && val < 2.0 */
 +   exponent_bits = 7;
 +   ls_bit_pos = 23;
 +   } else {
 +   /* abs_coeff >= 2.0 && val < 4.0 */
 +   exponent_bits = 6;
 +   ls_bit_pos = 24;
 +   }
 +
 +   mantissa = GET_BITS_ROUNDOFF(abs_coeff, ls_bit_pos,
 CSC_MAX_VALS);
 +   if (coeff < 0) {
 +   sign_bit = 1;
 +   mantissa = -mantissa;
 +   mantissa &= ((1 << CSC_MAX_VALS) - 1);
>>>
>>> I think there is a macro for this already ?
>>>
>> Thats for GAMMA_MAX, not for CSC_MAX. Or you mean the whole (1 <<
>> CSC_MAX_VALS -1) to be replaced with GET/SET bits ?
> What I mean is - the above looks exactly like the GET_BIT_MASK (which
> you introduced). Perhaps you can use it ?
>
Yes, Agree. but in later code review phase we realized that we dont even 
need this masking for mantissa. New patch set doesnt have this , so 
we dont need this.
> Regards,
> Emil
>

[PATCH 22/22] drm/i915: BDW: Pipe level CSC correction

2015-10-13 Thread Emil Velikov 
On 10 October 2015 at 06:34, Sharma, Shashank  
wrote:
> On 10/10/2015 5:24 AM, Emil Velikov wrote:
>>
>> Hi Shashank,
>>
>> On 9 October 2015 at 20:29, Shashank Sharma 
>> wrote:
>>>
>>> BDW/SKL/BXT support Color Space Conversion (CSC) using a 3x3 matrix
>>> that needs to be programmed into respective CSC registers.
>>>
>>> This patch does the following:
>>> 1. Adds the core function to program CSC correction values for
>>> BDW/SKL/BXT platform
>>> 2. Adds CSC correction macros/defines
>>>
>>> Signed-off-by: Shashank Sharma 
>>> Signed-off-by: Kausal Malladi 
>>> Signed-off-by: Kumar, Kiran S 
>>> ---
>>>   drivers/gpu/drm/i915/i915_reg.h|   7 ++
>>>   drivers/gpu/drm/i915/intel_color_manager.c | 114
>>> -
>>>   drivers/gpu/drm/i915/intel_color_manager.h |  12 ++-
>>>   3 files changed, 129 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>>> b/drivers/gpu/drm/i915/i915_reg.h
>>> index ed50f75..0e9d252 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -8085,4 +8085,11 @@ enum skl_disp_power_wells {
>>>  (_PIPE3(pipe, PAL_PREC_GCMAX_A, PAL_PREC_GCMAX_B,
>>> PAL_PREC_GCMAX_C))
>>>
>>>
>>> +/* BDW CSC correction */
>>> +#define CSC_COEFF_A0x49010
>>> +#define CSC_COEFF_B0x49110
>>> +#define CSC_COEFF_C0x49210
>>> +#define _PIPE_CSC_COEFF(pipe) \
>>> +   (_PIPE3(pipe, CSC_COEFF_A, CSC_COEFF_B, CSC_COEFF_C))
>>> +
>>>   #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 e659382..0a6c00c 100644
>>> --- a/drivers/gpu/drm/i915/intel_color_manager.c
>>> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
>>> @@ -330,11 +330,119 @@ static int bdw_set_degamma(struct drm_device *dev,
>>>  return 0;
>>>   }
>>>
>>> -static s16 chv_prepare_csc_coeff(s64 csc_value)
>>
>> As mentioned previously, this should be part of the respective patch.
>>
> Agree. Looks like diff is messing up a bit. Will take care of this.
>
>>> +static uint32_t bdw_prepare_csc_coeff(int64_t coeff)
>>> +{
>>> +   uint32_t reg_val, ls_bit_pos, exponent_bits, sign_bit = 0;
>>> +   int32_t mantissa;
>>> +   uint64_t abs_coeff;
>>> +
>>> +   coeff = min_t(int64_t, coeff, BDW_CSC_COEFF_MAX_VAL);
>>> +   coeff = max_t(int64_t, coeff, BDW_CSC_COEFF_MIN_VAL);
>>> +
>>> +   abs_coeff = abs(coeff);
>>> +   if (abs_coeff < (BDW_CSC_COEFF_UNITY_VAL >> 3)) {
>>> +   /* abs_coeff < 0.125 */
>>> +   exponent_bits = 3;
>>> +   ls_bit_pos = 19;
>>> +   } else if (abs_coeff >= (BDW_CSC_COEFF_UNITY_VAL >> 3) &&
>>> +   abs_coeff < (BDW_CSC_COEFF_UNITY_VAL >> 2)) {
>>> +   /* abs_coeff >= 0.125 && val < 0.25 */
>>> +   exponent_bits = 2;
>>> +   ls_bit_pos = 20;
>>> +   } else if (abs_coeff >= (BDW_CSC_COEFF_UNITY_VAL >> 2)
>>> +   && abs_coeff < (BDW_CSC_COEFF_UNITY_VAL >> 1)) {
>>> +   /* abs_coeff >= 0.25 && val < 0.5 */
>>> +   exponent_bits = 1;
>>> +   ls_bit_pos = 21;
>>> +   } else if (abs_coeff >= (BDW_CSC_COEFF_UNITY_VAL >> 1)
>>> +   && abs_coeff < BDW_CSC_COEFF_UNITY_VAL) {
>>> +   /* abs_coeff >= 0.5 && val < 1.0 */
>>> +   exponent_bits = 0;
>>> +   ls_bit_pos = 22;
>>> +   } else if (abs_coeff >= BDW_CSC_COEFF_UNITY_VAL &&
>>> +   abs_coeff < (BDW_CSC_COEFF_UNITY_VAL << 1)) {
>>> +   /* abs_coeff >= 1.0 && val < 2.0 */
>>> +   exponent_bits = 7;
>>> +   ls_bit_pos = 23;
>>> +   } else {
>>> +   /* abs_coeff >= 2.0 && val < 4.0 */
>>> +   exponent_bits = 6;
>>> +   ls_bit_pos = 24;
>>> +   }
>>> +
>>> +   mantissa = GET_BITS_ROUNDOFF(abs_coeff, ls_bit_pos,
>>> CSC_MAX_VALS);
>>> +   if (coeff < 0) {
>>> +   sign_bit = 1;
>>> +   mantissa = -mantissa;
>>> +   mantissa &= ((1 << CSC_MAX_VALS) - 1);
>>
>> I think there is a macro for this already ?
>>
> Thats for GAMMA_MAX, not for CSC_MAX. Or you mean the whole (1 <<
> CSC_MAX_VALS -1) to be replaced with GET/SET bits ?
What I mean is - the above looks exactly like the GET_BIT_MASK (which
you introduced). Perhaps you can use it ?

Regards,
Emil

[PATCH 22/22] drm/i915: BDW: Pipe level CSC correction

2015-10-12 Thread Rob Bradford 
On Sat, 2015-10-10 at 00:59 +0530, Shashank Sharma wrote:
> BDW/SKL/BXT support Color Space Conversion (CSC) using a 3x3 matrix
> that needs to be programmed into respective CSC registers.
> 
> This patch does the following:
> 1. Adds the core function to program CSC correction values for
>BDW/SKL/BXT platform
> 2. Adds CSC correction macros/defines
> 
> Signed-off-by: Shashank Sharma 
> Signed-off-by: Kausal Malladi 
> Signed-off-by: Kumar, Kiran S 
> ---
>  drivers/gpu/drm/i915/i915_reg.h|   7 ++
>  drivers/gpu/drm/i915/intel_color_manager.c | 114
> -
>  drivers/gpu/drm/i915/intel_color_manager.h |  12 ++-
>  3 files changed, 129 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index ed50f75..0e9d252 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -8085,4 +8085,11 @@ enum skl_disp_power_wells {
>   (_PIPE3(pipe, PAL_PREC_GCMAX_A, PAL_PREC_GCMAX_B,
> PAL_PREC_GCMAX_C))
>  
>  
> +/* BDW CSC correction */
> +#define CSC_COEFF_A  0x49010
> +#define CSC_COEFF_B  0x49110
> +#define CSC_COEFF_C  0x49210
> +#define _PIPE_CSC_COEFF(pipe) \
> + (_PIPE3(pipe, CSC_COEFF_A, CSC_COEFF_B, CSC_COEFF_C))
> +
>  #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 e659382..0a6c00c 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.c
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -330,11 +330,119 @@ static int bdw_set_degamma(struct drm_device
> *dev,
>   return 0;
>  }
>  
> -static s16 chv_prepare_csc_coeff(s64 csc_value)
> +static uint32_t bdw_prepare_csc_coeff(int64_t coeff)
> +{
> + uint32_t reg_val, ls_bit_pos, exponent_bits, sign_bit = 0;
> + int32_t mantissa;
> + uint64_t abs_coeff;
> +
> + coeff = min_t(int64_t, coeff, BDW_CSC_COEFF_MAX_VAL);
> + coeff = max_t(int64_t, coeff, BDW_CSC_COEFF_MIN_VAL);
> +
> + abs_coeff = abs(coeff);
> + if (abs_coeff < (BDW_CSC_COEFF_UNITY_VAL >> 3)) {
> + /* abs_coeff < 0.125 */
> + exponent_bits = 3;
> + ls_bit_pos = 19;
> + } else if (abs_coeff >= (BDW_CSC_COEFF_UNITY_VAL >> 3) &&
> + abs_coeff < (BDW_CSC_COEFF_UNITY_VAL >> 2)) {
> + /* abs_coeff >= 0.125 && val < 0.25 */
> + exponent_bits = 2;
> + ls_bit_pos = 20;
> + } else if (abs_coeff >= (BDW_CSC_COEFF_UNITY_VAL >> 2)
> + && abs_coeff < (BDW_CSC_COEFF_UNITY_VAL >> 1)) {
> + /* abs_coeff >= 0.25 && val < 0.5 */
> + exponent_bits = 1;
> + ls_bit_pos = 21;
> + } else if (abs_coeff >= (BDW_CSC_COEFF_UNITY_VAL >> 1)
> + && abs_coeff < BDW_CSC_COEFF_UNITY_VAL) {
> + /* abs_coeff >= 0.5 && val < 1.0 */
> + exponent_bits = 0;
> + ls_bit_pos = 22;
> + } else if (abs_coeff >= BDW_CSC_COEFF_UNITY_VAL &&
> + abs_coeff < (BDW_CSC_COEFF_UNITY_VAL << 1)) {
> + /* abs_coeff >= 1.0 && val < 2.0 */
> + exponent_bits = 7;
> + ls_bit_pos = 23;
> + } else {
> + /* abs_coeff >= 2.0 && val < 4.0 */
> + exponent_bits = 6;
> + ls_bit_pos = 24;
> + }
> +
> + mantissa = GET_BITS_ROUNDOFF(abs_coeff, ls_bit_pos,
> CSC_MAX_VALS);

Is GET_BITS_ROUNDOFF safe for negative values?

> + if (coeff < 0) {
> + sign_bit = 1;
> + mantissa = -mantissa;
> + mantissa &= ((1 << CSC_MAX_VALS) - 1);

Oops. It looks like you are reusing the macro/define for the number of
coefficients for the size of the mantissa. Also you defined a macro for
this very purpose in your other patch.

> + }
> +
> + reg_val = 0;
> + SET_BITS(reg_val, exponent_bits, 12, 3);
> + SET_BITS(reg_val, mantissa, 3, 9);
> + SET_BITS(reg_val, sign_bit, 15, 1);
> + DRM_DEBUG_DRIVER("CSC: reg_val=0x%x\n", reg_val);

This debug message is superfluous as you can easily read register
values out with i-g-t.

> + return reg_val;
> +}
> +
> +int bdw_set_csc(struct drm_device *dev, struct drm_property_blob
> *blob,
> + struct drm_crtc *crtc)
> +{
> + enum pipe pipe;
> + enum plane plane;
> + int i, j, temp;
> + int word = 0;
> + u32 reg, plane_ctl, mode;
> + struct drm_ctm *csc_data;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + if (WARN_ON(!blob))
> + return -EINVAL;
> +
> + if (blob->length != sizeof(struct drm_ctm)) {
> + DRM_ERROR("Invalid length of data received\n");
> + return -EINVAL;
> + }
> +
> + csc_data = (struct drm_ctm *)blob->data;
> + pipe = to_intel_crtc(crtc)->pipe;
> + plane = to_intel_crtc(crtc)->plane;
> +
> + plane_ctl = I915_READ(PLANE_CTL(pipe,

[PATCH 22/22] drm/i915: BDW: Pipe level CSC correction

2015-10-10 Thread Sharma, Shashank 
Regards
Shashank

On 10/10/2015 5:24 AM, Emil Velikov wrote:
> Hi Shashank,
>
> On 9 October 2015 at 20:29, Shashank Sharma  
> wrote:
>> BDW/SKL/BXT support Color Space Conversion (CSC) using a 3x3 matrix
>> that needs to be programmed into respective CSC registers.
>>
>> This patch does the following:
>> 1. Adds the core function to program CSC correction values for
>> BDW/SKL/BXT platform
>> 2. Adds CSC correction macros/defines
>>
>> Signed-off-by: Shashank Sharma 
>> Signed-off-by: Kausal Malladi 
>> Signed-off-by: Kumar, Kiran S 
>> ---
>>   drivers/gpu/drm/i915/i915_reg.h|   7 ++
>>   drivers/gpu/drm/i915/intel_color_manager.c | 114 
>> -
>>   drivers/gpu/drm/i915/intel_color_manager.h |  12 ++-
>>   3 files changed, 129 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h 
>> b/drivers/gpu/drm/i915/i915_reg.h
>> index ed50f75..0e9d252 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -8085,4 +8085,11 @@ enum skl_disp_power_wells {
>>  (_PIPE3(pipe, PAL_PREC_GCMAX_A, PAL_PREC_GCMAX_B, PAL_PREC_GCMAX_C))
>>
>>
>> +/* BDW CSC correction */
>> +#define CSC_COEFF_A0x49010
>> +#define CSC_COEFF_B0x49110
>> +#define CSC_COEFF_C0x49210
>> +#define _PIPE_CSC_COEFF(pipe) \
>> +   (_PIPE3(pipe, CSC_COEFF_A, CSC_COEFF_B, CSC_COEFF_C))
>> +
>>   #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 e659382..0a6c00c 100644
>> --- a/drivers/gpu/drm/i915/intel_color_manager.c
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
>> @@ -330,11 +330,119 @@ static int bdw_set_degamma(struct drm_device *dev,
>>  return 0;
>>   }
>>
>> -static s16 chv_prepare_csc_coeff(s64 csc_value)
> As mentioned previously, this should be part of the respective patch.
>
Agree. Looks like diff is messing up a bit. Will take care of this.
>> +static uint32_t bdw_prepare_csc_coeff(int64_t coeff)
>> +{
>> +   uint32_t reg_val, ls_bit_pos, exponent_bits, sign_bit = 0;
>> +   int32_t mantissa;
>> +   uint64_t abs_coeff;
>> +
>> +   coeff = min_t(int64_t, coeff, BDW_CSC_COEFF_MAX_VAL);
>> +   coeff = max_t(int64_t, coeff, BDW_CSC_COEFF_MIN_VAL);
>> +
>> +   abs_coeff = abs(coeff);
>> +   if (abs_coeff < (BDW_CSC_COEFF_UNITY_VAL >> 3)) {
>> +   /* abs_coeff < 0.125 */
>> +   exponent_bits = 3;
>> +   ls_bit_pos = 19;
>> +   } else if (abs_coeff >= (BDW_CSC_COEFF_UNITY_VAL >> 3) &&
>> +   abs_coeff < (BDW_CSC_COEFF_UNITY_VAL >> 2)) {
>> +   /* abs_coeff >= 0.125 && val < 0.25 */
>> +   exponent_bits = 2;
>> +   ls_bit_pos = 20;
>> +   } else if (abs_coeff >= (BDW_CSC_COEFF_UNITY_VAL >> 2)
>> +   && abs_coeff < (BDW_CSC_COEFF_UNITY_VAL >> 1)) {
>> +   /* abs_coeff >= 0.25 && val < 0.5 */
>> +   exponent_bits = 1;
>> +   ls_bit_pos = 21;
>> +   } else if (abs_coeff >= (BDW_CSC_COEFF_UNITY_VAL >> 1)
>> +   && abs_coeff < BDW_CSC_COEFF_UNITY_VAL) {
>> +   /* abs_coeff >= 0.5 && val < 1.0 */
>> +   exponent_bits = 0;
>> +   ls_bit_pos = 22;
>> +   } else if (abs_coeff >= BDW_CSC_COEFF_UNITY_VAL &&
>> +   abs_coeff < (BDW_CSC_COEFF_UNITY_VAL << 1)) {
>> +   /* abs_coeff >= 1.0 && val < 2.0 */
>> +   exponent_bits = 7;
>> +   ls_bit_pos = 23;
>> +   } else {
>> +   /* abs_coeff >= 2.0 && val < 4.0 */
>> +   exponent_bits = 6;
>> +   ls_bit_pos = 24;
>> +   }
>> +
>> +   mantissa = GET_BITS_ROUNDOFF(abs_coeff, ls_bit_pos, CSC_MAX_VALS);
>> +   if (coeff < 0) {
>> +   sign_bit = 1;
>> +   mantissa = -mantissa;
>> +   mantissa &= ((1 << CSC_MAX_VALS) - 1);
> I think there is a macro for this already ?
>
Thats for GAMMA_MAX, not for CSC_MAX. Or you mean the whole (1 << 
CSC_MAX_VALS -1) to be replaced with GET/SET bits ?
>> +   }
>> +
>> +   reg_val = 0;
>> +   SET_BITS(reg_val, exponent_bits, 12, 3);
>> +   SET_BITS(reg_val, mantissa, 3, 9);
>> +   SET_BITS(reg_val, sign_bit, 15, 1);
>> +   DRM_DEBUG_DRIVER("CSC: reg_val=0x%x\n", reg_val);
>> +   return reg_val;
>> +}
>> +
>> +int bdw_set_csc(struct drm_device *dev, struct drm_property_blob *blob,
>> +   struct drm_crtc *crtc)
>> +{
> The function should be static ?
>
Agree.
> Regards,
> Emil
>

[PATCH 22/22] drm/i915: BDW: Pipe level CSC correction

2015-10-10 Thread Shashank Sharma 
BDW/SKL/BXT support Color Space Conversion (CSC) using a 3x3 matrix
that needs to be programmed into respective CSC registers.

This patch does the following:
1. Adds the core function to program CSC correction values for
   BDW/SKL/BXT platform
2. Adds CSC correction macros/defines

Signed-off-by: Shashank Sharma 
Signed-off-by: Kausal Malladi 
Signed-off-by: Kumar, Kiran S 
---
 drivers/gpu/drm/i915/i915_reg.h|   7 ++
 drivers/gpu/drm/i915/intel_color_manager.c | 114 -
 drivers/gpu/drm/i915/intel_color_manager.h |  12 ++-
 3 files changed, 129 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index ed50f75..0e9d252 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -8085,4 +8085,11 @@ enum skl_disp_power_wells {
(_PIPE3(pipe, PAL_PREC_GCMAX_A, PAL_PREC_GCMAX_B, PAL_PREC_GCMAX_C))


+/* BDW CSC correction */
+#define CSC_COEFF_A0x49010
+#define CSC_COEFF_B0x49110
+#define CSC_COEFF_C0x49210
+#define _PIPE_CSC_COEFF(pipe) \
+   (_PIPE3(pipe, CSC_COEFF_A, CSC_COEFF_B, CSC_COEFF_C))
+
 #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 e659382..0a6c00c 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.c
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -330,11 +330,119 @@ static int bdw_set_degamma(struct drm_device *dev,
return 0;
 }

-static s16 chv_prepare_csc_coeff(s64 csc_value)
+static uint32_t bdw_prepare_csc_coeff(int64_t coeff)
+{
+   uint32_t reg_val, ls_bit_pos, exponent_bits, sign_bit = 0;
+   int32_t mantissa;
+   uint64_t abs_coeff;
+
+   coeff = min_t(int64_t, coeff, BDW_CSC_COEFF_MAX_VAL);
+   coeff = max_t(int64_t, coeff, BDW_CSC_COEFF_MIN_VAL);
+
+   abs_coeff = abs(coeff);
+   if (abs_coeff < (BDW_CSC_COEFF_UNITY_VAL >> 3)) {
+   /* abs_coeff < 0.125 */
+   exponent_bits = 3;
+   ls_bit_pos = 19;
+   } else if (abs_coeff >= (BDW_CSC_COEFF_UNITY_VAL >> 3) &&
+   abs_coeff < (BDW_CSC_COEFF_UNITY_VAL >> 2)) {
+   /* abs_coeff >= 0.125 && val < 0.25 */
+   exponent_bits = 2;
+   ls_bit_pos = 20;
+   } else if (abs_coeff >= (BDW_CSC_COEFF_UNITY_VAL >> 2)
+   && abs_coeff < (BDW_CSC_COEFF_UNITY_VAL >> 1)) {
+   /* abs_coeff >= 0.25 && val < 0.5 */
+   exponent_bits = 1;
+   ls_bit_pos = 21;
+   } else if (abs_coeff >= (BDW_CSC_COEFF_UNITY_VAL >> 1)
+   && abs_coeff < BDW_CSC_COEFF_UNITY_VAL) {
+   /* abs_coeff >= 0.5 && val < 1.0 */
+   exponent_bits = 0;
+   ls_bit_pos = 22;
+   } else if (abs_coeff >= BDW_CSC_COEFF_UNITY_VAL &&
+   abs_coeff < (BDW_CSC_COEFF_UNITY_VAL << 1)) {
+   /* abs_coeff >= 1.0 && val < 2.0 */
+   exponent_bits = 7;
+   ls_bit_pos = 23;
+   } else {
+   /* abs_coeff >= 2.0 && val < 4.0 */
+   exponent_bits = 6;
+   ls_bit_pos = 24;
+   }
+
+   mantissa = GET_BITS_ROUNDOFF(abs_coeff, ls_bit_pos, CSC_MAX_VALS);
+   if (coeff < 0) {
+   sign_bit = 1;
+   mantissa = -mantissa;
+   mantissa &= ((1 << CSC_MAX_VALS) - 1);
+   }
+
+   reg_val = 0;
+   SET_BITS(reg_val, exponent_bits, 12, 3);
+   SET_BITS(reg_val, mantissa, 3, 9);
+   SET_BITS(reg_val, sign_bit, 15, 1);
+   DRM_DEBUG_DRIVER("CSC: reg_val=0x%x\n", reg_val);
+   return reg_val;
+}
+
+int bdw_set_csc(struct drm_device *dev, struct drm_property_blob *blob,
+   struct drm_crtc *crtc)
+{
+   enum pipe pipe;
+   enum plane plane;
+   int i, j, temp;
+   int word = 0;
+   u32 reg, plane_ctl, mode;
+   struct drm_ctm *csc_data;
+   struct drm_i915_private *dev_priv = dev->dev_private;
+
+   if (WARN_ON(!blob))
+   return -EINVAL;
+
+   if (blob->length != sizeof(struct drm_ctm)) {
+   DRM_ERROR("Invalid length of data received\n");
+   return -EINVAL;
+   }
+
+   csc_data = (struct drm_ctm *)blob->data;
+   pipe = to_intel_crtc(crtc)->pipe;
+   plane = to_intel_crtc(crtc)->plane;
+
+   plane_ctl = I915_READ(PLANE_CTL(pipe, plane));
+   plane_ctl |= PLANE_CTL_PIPE_CSC_ENABLE;
+   I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl);
+   reg = _PIPE_CSC_COEFF(pipe);
+
+   /* Write csc coeff to csc regs */
+   for (i = 0, j = 0; i < CSC_MAX_VALS; i++) {
+   if ((i % 3) == 0) {
+   temp = bdw_prepare_csc_coeff(csc_data->ctm_coeff[i]);
+   SET_BITS(word, temp, 16, 16);
+   i++;
+   temp =

[PATCH 22/22] drm/i915: BDW: Pipe level CSC correction

2015-10-10 Thread Emil Velikov 
Hi Shashank,

On 9 October 2015 at 20:29, Shashank Sharma  
wrote:
> BDW/SKL/BXT support Color Space Conversion (CSC) using a 3x3 matrix
> that needs to be programmed into respective CSC registers.
>
> This patch does the following:
> 1. Adds the core function to program CSC correction values for
>BDW/SKL/BXT platform
> 2. Adds CSC correction macros/defines
>
> Signed-off-by: Shashank Sharma 
> Signed-off-by: Kausal Malladi 
> Signed-off-by: Kumar, Kiran S 
> ---
>  drivers/gpu/drm/i915/i915_reg.h|   7 ++
>  drivers/gpu/drm/i915/intel_color_manager.c | 114 
> -
>  drivers/gpu/drm/i915/intel_color_manager.h |  12 ++-
>  3 files changed, 129 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index ed50f75..0e9d252 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -8085,4 +8085,11 @@ enum skl_disp_power_wells {
> (_PIPE3(pipe, PAL_PREC_GCMAX_A, PAL_PREC_GCMAX_B, PAL_PREC_GCMAX_C))
>
>
> +/* BDW CSC correction */
> +#define CSC_COEFF_A0x49010
> +#define CSC_COEFF_B0x49110
> +#define CSC_COEFF_C0x49210
> +#define _PIPE_CSC_COEFF(pipe) \
> +   (_PIPE3(pipe, CSC_COEFF_A, CSC_COEFF_B, CSC_COEFF_C))
> +
>  #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 e659382..0a6c00c 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.c
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -330,11 +330,119 @@ static int bdw_set_degamma(struct drm_device *dev,
> return 0;
>  }
>
> -static s16 chv_prepare_csc_coeff(s64 csc_value)
As mentioned previously, this should be part of the respective patch.

> +static uint32_t bdw_prepare_csc_coeff(int64_t coeff)
> +{
> +   uint32_t reg_val, ls_bit_pos, exponent_bits, sign_bit = 0;
> +   int32_t mantissa;
> +   uint64_t abs_coeff;
> +
> +   coeff = min_t(int64_t, coeff, BDW_CSC_COEFF_MAX_VAL);
> +   coeff = max_t(int64_t, coeff, BDW_CSC_COEFF_MIN_VAL);
> +
> +   abs_coeff = abs(coeff);
> +   if (abs_coeff < (BDW_CSC_COEFF_UNITY_VAL >> 3)) {
> +   /* abs_coeff < 0.125 */
> +   exponent_bits = 3;
> +   ls_bit_pos = 19;
> +   } else if (abs_coeff >= (BDW_CSC_COEFF_UNITY_VAL >> 3) &&
> +   abs_coeff < (BDW_CSC_COEFF_UNITY_VAL >> 2)) {
> +   /* abs_coeff >= 0.125 && val < 0.25 */
> +   exponent_bits = 2;
> +   ls_bit_pos = 20;
> +   } else if (abs_coeff >= (BDW_CSC_COEFF_UNITY_VAL >> 2)
> +   && abs_coeff < (BDW_CSC_COEFF_UNITY_VAL >> 1)) {
> +   /* abs_coeff >= 0.25 && val < 0.5 */
> +   exponent_bits = 1;
> +   ls_bit_pos = 21;
> +   } else if (abs_coeff >= (BDW_CSC_COEFF_UNITY_VAL >> 1)
> +   && abs_coeff < BDW_CSC_COEFF_UNITY_VAL) {
> +   /* abs_coeff >= 0.5 && val < 1.0 */
> +   exponent_bits = 0;
> +   ls_bit_pos = 22;
> +   } else if (abs_coeff >= BDW_CSC_COEFF_UNITY_VAL &&
> +   abs_coeff < (BDW_CSC_COEFF_UNITY_VAL << 1)) {
> +   /* abs_coeff >= 1.0 && val < 2.0 */
> +   exponent_bits = 7;
> +   ls_bit_pos = 23;
> +   } else {
> +   /* abs_coeff >= 2.0 && val < 4.0 */
> +   exponent_bits = 6;
> +   ls_bit_pos = 24;
> +   }
> +
> +   mantissa = GET_BITS_ROUNDOFF(abs_coeff, ls_bit_pos, CSC_MAX_VALS);
> +   if (coeff < 0) {
> +   sign_bit = 1;
> +   mantissa = -mantissa;
> +   mantissa &= ((1 << CSC_MAX_VALS) - 1);
I think there is a macro for this already ?

> +   }
> +
> +   reg_val = 0;
> +   SET_BITS(reg_val, exponent_bits, 12, 3);
> +   SET_BITS(reg_val, mantissa, 3, 9);
> +   SET_BITS(reg_val, sign_bit, 15, 1);
> +   DRM_DEBUG_DRIVER("CSC: reg_val=0x%x\n", reg_val);
> +   return reg_val;
> +}
> +
> +int bdw_set_csc(struct drm_device *dev, struct drm_property_blob *blob,
> +   struct drm_crtc *crtc)
> +{
The function should be static ?

Regards,
Emil