Re: [Intel-gfx] [PATCH 5/6] drm/i915/bdw+: Implement color management

[Daniel Stone]

Hi,

On 17 December 2015 at 18:57, Lionel Landwerlin
 wrote:
> @@ -289,22 +289,30 @@ static const struct intel_device_info 
> intel_haswell_m_info = {
>  static const struct intel_device_info intel_broadwell_d_info = {
> HSW_FEATURES,
> .gen = 8,
> +   .num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS,
> +   .num_samples_before_ctm = BDW_DEGAMMA_MAX_VALS,
>  };

Nitpick I know, but please invert before and after for the sake of my sanity.

> +static void bdw_write_8bit_gamma_legacy(struct drm_device *dev,
> +   struct drm_r32g32b32 *correction_values, i915_reg_t palette)
> +{
> +   u16 blue_fract, green_fract, red_fract;
> +   u32 blue, green, red;
> +   u32 count = 0;
> +   u32 word = 0;
> +   struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +   while (count < BDW_8BIT_GAMMA_MAX_VALS) {
> +   blue = correction_values[count].b32;
> +   green = correction_values[count].g32;
> +   red = correction_values[count].r32;
> +
> +   /*
> +   * Maximum possible gamma correction value supported
> +   * for BDW is 0x, so clamp the values accordingly
> +   */

Eh? 0x != (1 << 24) - 1.

> +   if (blue >= BDW_MAX_GAMMA)
> +   blue = BDW_MAX_GAMMA;
> +   if (green >= BDW_MAX_GAMMA)
> +   green = BDW_MAX_GAMMA;
> +   if (red >= BDW_MAX_GAMMA)
> +   red = BDW_MAX_GAMMA;

> rather than >=.

> +   /*
> +   * Gamma correction values are sent in 8.24 format
> +   * with 8 int and 24 fraction bits. BDW 10 bit gamma
> +   * unit expects correction registers to be programmed in
> +   * 0.10 format, with 0 int and 16 fraction bits. So take

'0.10 format, with 0 int and 16 fraction bits' ...

> +   * MSB 10 bit values(bits 23-14) from the fraction part and
> +   * prepare the correction registers.
> +   */

The comment here is helpful, but colour me confused: we just discard
the 8 integer bits anyway? Why do they exist? Every time I think I've
worked out which of [0,1.0] and [0,8.0) is the correct range, I
realise I'm wrong.

> +static void bdw_write_12bit_gamma_precision(struct drm_device *dev,
> +   struct drm_r32g32b32 *correction_values, i915_reg_t pal_prec_data,
> +   enum pipe pipe)

Why does this take a pipe where all the others don't, and also not
take a num_coeff, also unlike the others?

> +   /* Program first 512 values in precision palette */
> +   while (count < BDW_12BIT_GAMMA_MAX_VALS - 1) {

Um, this is a for loop.

> +   /*
> +   * Maximum possible gamma correction value supported
> +   * for BDW is 0x, so clamp the values accordingly
> +   */

Again, not 0x.

> +   /*
> +   * Framework's general gamma format is 8.24 (8 int 16 fraction)

8 int 16?!

> +   * BDW Platform's supported gamma format is 16 bit correction
> +   * values in 0.16 format. So extract higher 16 fraction bits
> +   * from 8.24 gamma correction values.

Isn't this the 12-bit mode? Colour me stupid, but what makes this
12-bit rather than 16-bit? Anyway.

> +   */
> +   red_fract = GET_BITS(red, 8, 16);
> +   green_fract = GET_BITS(green, 8, 16);
> +   blue_fract = GET_BITS(blue, 8, 16);

Okay, at least the range is consistent here - still [0,1.0], with the
integer component totally ignored.

> +   gamma_data = (struct drm_palette *)blob->data;
> +   pipe = to_intel_crtc(crtc)->pipe;
> +   num_samples = blob->length / sizeof(struct drm_r32g32b32);
> +
> +   pal_prec_index = _MMIO(_PREC_PAL_INDEX(pipe));
> +   pal_prec_data = _MMIO(_PREC_PAL_DATA(pipe));
> +   correction_values = (struct drm_r32g32b32 *)_data->lut;
> +
> +   switch (num_samples) {
> +   case GAMMA_DISABLE_VALS:
> +   /* Disable Gamma functionality on Pipe */
> +   DRM_DEBUG_DRIVER("Disabling gamma on Pipe %c\n",
> +   pipe_name(pipe));
> +   if ((mode & GAMMA_MODE_MODE_MASK) == GAMMA_MODE_MODE_12BIT)
> +   bdw_reset_gamma(dev_priv, pipe);
> +   state->palette_after_ctm_blob = NULL;
> +   word = GAMMA_MODE_MODE_8BIT;
> +   break;

Right, so we program the gamma unit to 8-bit mode (sensible!), and
write a linear palette (sensible!) ... but only if we were previously
in 12-bit gamma mode. What? So, if I start with 10-bit gamma and then
disable the gamma unit, my gamma won't be disabled, but will be an
8-bit interpretation of the previous 10-bit gamma ramp? Ouch. Also
broken when going from 8-bit -> disabled. Surely just make that check
unconditional.

(Again, this should 

[Intel-gfx] [PATCH 5/6] drm/i915/bdw+: Implement color management

[Lionel Landwerlin]

From: Shashank Sharma 

Implement degamma, csc and gamma steps on BDW+.

Signed-off-by: Shashank Sharma 
Signed-off-by: Kausal Malladi 
---
 drivers/gpu/drm/i915/i915_drv.c|  14 +
 drivers/gpu/drm/i915/i915_reg.h|  43 ++-
 drivers/gpu/drm/i915/intel_color_manager.c | 451 +
 drivers/gpu/drm/i915/intel_color_manager.h |  20 ++
 4 files changed, 525 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 4396300..395e5ad 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -289,22 +289,30 @@ static const struct intel_device_info 
intel_haswell_m_info = {
 static const struct intel_device_info intel_broadwell_d_info = {
HSW_FEATURES,
.gen = 8,
+   .num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS,
+   .num_samples_before_ctm = BDW_DEGAMMA_MAX_VALS,
 };
 
 static const struct intel_device_info intel_broadwell_m_info = {
HSW_FEATURES,
.gen = 8, .is_mobile = 1,
+   .num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS,
+   .num_samples_before_ctm = BDW_DEGAMMA_MAX_VALS,
 };
 
 static const struct intel_device_info intel_broadwell_gt3d_info = {
HSW_FEATURES,
.gen = 8,
+   .num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS,
+   .num_samples_before_ctm = BDW_DEGAMMA_MAX_VALS,
.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
 };
 
 static const struct intel_device_info intel_broadwell_gt3m_info = {
HSW_FEATURES,
.gen = 8, .is_mobile = 1,
+   .num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS,
+   .num_samples_before_ctm = BDW_DEGAMMA_MAX_VALS,
.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
 };
 
@@ -324,12 +332,16 @@ static const struct intel_device_info intel_skylake_info 
= {
HSW_FEATURES,
.is_skylake = 1,
.gen = 9,
+   .num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS,
+   .num_samples_before_ctm = BDW_DEGAMMA_MAX_VALS,
 };
 
 static const struct intel_device_info intel_skylake_gt3_info = {
HSW_FEATURES,
.is_skylake = 1,
.gen = 9,
+   .num_samples_before_ctm = BDW_DEGAMMA_MAX_VALS,
+   .num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS,
.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
 };
 
@@ -339,6 +351,8 @@ static const struct intel_device_info intel_broxton_info = {
.gen = 9,
.need_gfx_hws = 1, .has_hotplug = 1,
.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
+   .num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS,
+   .num_samples_before_ctm = BDW_DEGAMMA_MAX_VALS,
.num_pipes = 3,
.has_ddi = 1,
.has_fpga_dbg = 1,
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 36bb320..c4da842 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5748,11 +5748,19 @@ enum skl_disp_power_wells {
 /* legacy palette */
 #define _LGC_PALETTE_A   0x4a000
 #define _LGC_PALETTE_B   0x4a800
-#define LGC_PALETTE(pipe, i) _MMIO(_PIPE(pipe, _LGC_PALETTE_A, _LGC_PALETTE_B) 
+ (i) * 4)
+#define _LGC_PALETTE_C   0x4b000
+#define LGC_PALETTE(pipe, i) _MMIO(_PIPE3(pipe, \
+ _LGC_PALETTE_A, \
+ _LGC_PALETTE_B, \
+ _LGC_PALETTE_C) + (i) * 4)
 
 #define _GAMMA_MODE_A  0x4a480
 #define _GAMMA_MODE_B  0x4ac80
-#define GAMMA_MODE(pipe) _MMIO_PIPE(pipe, _GAMMA_MODE_A, _GAMMA_MODE_B)
+#define _GAMMA_MODE_C  0x4b480
+#define GAMMA_MODE(pipe) _MMIO(_PIPE3(pipe, \
+ _GAMMA_MODE_A, \
+ _GAMMA_MODE_B, \
+ _GAMMA_MODE_C))
 #define GAMMA_MODE_MODE_MASK   (3 << 0)
 #define GAMMA_MODE_MODE_8BIT   (0 << 0)
 #define GAMMA_MODE_MODE_10BIT  (1 << 0)
@@ -8178,6 +8186,35 @@ enum skl_disp_power_wells {
 #define _PIPE_CSC_BASE(pipe) \
(_PIPE3(pipe, PIPEA_CGM_CSC, PIPEB_CGM_CSC, PIPEC_CGM_CSC))
 
-
+/* BDW gamma correction */
+#define PAL_PREC_INDEX_A   0x4A400
+#define PAL_PREC_INDEX_B   0x4AC00
+#define PAL_PREC_INDEX_C   0x4B400
+#define PAL_PREC_DATA_A0x4A404
+#define PAL_PREC_DATA_B0x4AC04
+#define PAL_PREC_DATA_C0x4B404
+#define PAL_PREC_GC_MAX_A  0x4A410
+#define PAL_PREC_GC_MAX_B  0x4AC10
+#define PAL_PREC_GC_MAX_C  0x4B410
+#define PAL_PREC_EXT_GC_MAX_A  0x4A420
+#define PAL_PREC_EXT_GC_MAX_B  0x4AC20
+#define PAL_PREC_EXT_GC_MAX_C  0x4B420
+
+#define