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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Regards
Shashank

On 10/13/2015 7:03 PM, Emil Velikov wrote:
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.

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux