Re: [PATCH 15/22] drm/i915: CHV: Pipe level CSC correction

["Sharma, Shashank" <shashank.sharma@intel.com>]

Regards
Shashank

On 10/10/2015 5:13 AM, Emil Velikov wrote:
> On 9 October 2015 at 20:29, Shashank Sharma <shashank.sharma@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@intel.com>
>> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
>> Signed-off-by: Kumar, Kiran S <kiran.s.kumar@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.
> [snip]
>> +static int chv_set_csc(struct drm_device *dev, struct drm_property_blob *blob,
>> +               struct drm_crtc *crtc)
>> +{
>> +       struct drm_ctm *csc_data;
>> +       struct drm_i915_private *dev_priv = dev->dev_private;
>> +       u32 reg;
>> +       enum pipe pipe;
>> +       s32 word = 0, temp;
>> +       int count = 0;
>> +
>> +       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;
>> +
>> +       /* Disable CSC functionality */
>> +       reg = _PIPE_CGM_CONTROL(pipe);
>> +       I915_WRITE(reg, I915_READ(reg) & (~CGM_CSC_EN));
>> +
>> +       DRM_DEBUG_DRIVER("Disabled CSC Functionality on Pipe %c\n",
>> +                       pipe_name(pipe));
>> +
>> +       reg = _PIPE_CSC_BASE(pipe);
>> +       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 ?
>
> Regards,
> Emil
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel