Regards
Shashank

On 8/22/2015 4:11 AM, Matt Roper wrote:
On Thu, Aug 06, 2015 at 10:08:18PM +0530, Shashank Sharma wrote:
From: Kausal Malladi <kausalmall...@gmail.com>

CHV/BSW platform supports two different pipe level gamma
correction modes, which are:
1. Legacy 8-bit mode
2. 10-bit CGM (Color Gamut Mapping) mode

This patch does the following:
1. Attaches Gamma property to CRTC
3. Adds the core Gamma correction function for CHV/BSW
4. Adds Gamma correction macros

Signed-off-by: Shashank Sharma <shashank.sha...@intel.com>
Signed-off-by: Kausal Malladi <kausalmall...@gmail.com>
---
  drivers/gpu/drm/i915/i915_reg.h            |  12 +++
  drivers/gpu/drm/i915/intel_color_manager.c | 146 +++++++++++++++++++++++++++++
  drivers/gpu/drm/i915/intel_color_manager.h |  20 ++++
  drivers/gpu/drm/i915/intel_display.c       |   2 +
  drivers/gpu/drm/i915/intel_drv.h           |   2 +
  5 files changed, 182 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 3a77678..4997ebd 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7921,4 +7921,16 @@ enum skl_disp_power_wells {
  #define GEN9_VEBOX_MOCS_0     0xcb00  /* Video MOCS base register*/
  #define GEN9_BLT_MOCS_0               0xcc00  /* Blitter MOCS base register*/

+/* Color Management */
+#define PIPEA_CGM_CONTROL                      (VLV_DISPLAY_BASE + 0x67A00)
+#define PIPEB_CGM_CONTROL                      (VLV_DISPLAY_BASE + 0x69A00)
+#define PIPEC_CGM_CONTROL                      (VLV_DISPLAY_BASE + 0x6BA00)
+#define PIPEA_CGM_GAMMA                                (VLV_DISPLAY_BASE + 
0x67000)
+#define PIPEB_CGM_GAMMA                                (VLV_DISPLAY_BASE + 
0x69000)
+#define PIPEC_CGM_GAMMA                                (VLV_DISPLAY_BASE + 
0x6B000)
+#define _PIPE_CGM_CONTROL(pipe) \
+       (_PIPE3(pipe, PIPEA_CGM_CONTROL, PIPEB_CGM_CONTROL, PIPEC_CGM_CONTROL))
+#define _PIPE_GAMMA_BASE(pipe) \
+       (_PIPE3(pipe, PIPEA_CGM_GAMMA, PIPEB_CGM_GAMMA, PIPEC_CGM_GAMMA))
+
  #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 9a6126c..f8c8d26 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.c
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -27,6 +27,149 @@

  #include "intel_color_manager.h"

+int chv_set_gamma(struct drm_device *dev, struct drm_property_blob *blob,
+                 struct drm_crtc *crtc)

It looks like this function is only called from this file, right?  We
can probably make it static in that case.

Yeah, this was the plan. We kept this non-static initially so that we can debug with KGDB when required, but then forgot to add static in the end :). Will do it for all platform specific core functions, for all properties.
+{
+       bool flag = false;
+       enum pipe pipe;
+       u8 red_int, blue_int, green_int;
+       u16 red_fract, green_fract, blue_fract;
+       u32 red, green, blue;
+       u32 cgm_control_reg = 0;
+       u32 cgm_gamma_reg = 0;
+       u32 count = 0, num_samples, word;
+       int ret = 0, length;
+       struct drm_r32g32b32 *correction_values = NULL;
+       struct drm_palette *gamma_data;
+       struct drm_i915_private *dev_priv = dev->dev_private;
+
+       if (!blob) {
+               DRM_ERROR("NULL Blob\n");
+               return -EINVAL;
+       }

The way the code stands right now, this should never be possible since
we don't even call this function if the blob is NULL, right?  In that
case we usually just handle this as

         if (WARN_ON(!blob))
                 return -EINVAL;

to make it a little more obvious that this is truly a "this can never
happen" type of assertion.

Also, see my comment near the bottom about whether this would be a more
intuitive way to disable the current gamma values.

Got it. Agree.

+
+       gamma_data = (struct drm_palette *)blob->data;
+
+       if (gamma_data->version != CHV_GAMMA_DATA_STRUCT_VERSION) {
+               DRM_ERROR("Invalid Gamma Data struct version\n");

A user can trigger this on-demand (and thus spam your kernel log), so
this should probably be a DRM_DEBUG_KMS rather than a DRM_ERROR.

We though we will keep this check only until the first revision of the structure is used, and once we have the next possible version, we will remove this. Do you think we can keep it by then ?
+               return -EINVAL;
+       }
+
+       pipe = to_intel_crtc(crtc)->pipe;
+       num_samples = gamma_data->num_samples;
+       length = num_samples * sizeof(struct drm_r32g32b32);
+
+       switch (num_samples) {
+       case 0:
+
+               /* Disable Gamma functionality on Pipe - CGM Block */
+               cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe));
+               cgm_control_reg &= ~CGM_GAMMA_EN;
+               I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg);
+
+               DRM_DEBUG_DRIVER("Gamma disabled on Pipe %c\n",
+                               pipe_name(pipe));
+               ret = 0;
+               break;
+
+       case CHV_8BIT_GAMMA_MAX_VALS:
+       case CHV_10BIT_GAMMA_MAX_VALS:
+
+               count = 0;
+               cgm_gamma_reg = _PIPE_GAMMA_BASE(pipe);
+               correction_values = (struct drm_r32g32b32 *)&gamma_data->lut;
+
+               while (count < num_samples) {
+                       blue = correction_values[count].b32;
+                       green = correction_values[count].g32;
+                       red = correction_values[count].r32;
+
+                       blue_int = _GAMMA_INT_PART(blue);
+                       if (blue_int > GAMMA_INT_MAX)
+                               blue = CHV_MAX_GAMMA;
+
+                       green_int = _GAMMA_INT_PART(green);
+                       if (green_int > GAMMA_INT_MAX)
+                               green = CHV_MAX_GAMMA;
+
+                       red_int = _GAMMA_INT_PART(red);
+                       if (red_int > GAMMA_INT_MAX)
+                               red = CHV_MAX_GAMMA;
+
+                       blue_fract = _GAMMA_FRACT_PART(blue);
+                       green_fract = _GAMMA_FRACT_PART(green);
+                       red_fract = _GAMMA_FRACT_PART(red);
+
+                       blue_fract >>= CHV_10BIT_GAMMA_MSB_SHIFT;
+                       green_fract >>= CHV_10BIT_GAMMA_MSB_SHIFT;
+                       red_fract >>= CHV_10BIT_GAMMA_MSB_SHIFT;
+
+                       /* Green (25:16) and Blue (9:0) to be written */
+                       word = green_fract;
+                       word <<= CHV_GAMMA_SHIFT_GREEN;
+                       word |= blue_fract;
+                       I915_WRITE(cgm_gamma_reg, word);
+                       cgm_gamma_reg += 4;
+
+                       /* Red (9:0) to be written */
+                       word = red_fract;
+                       I915_WRITE(cgm_gamma_reg, word);
+
+                       cgm_gamma_reg += 4;
+                       count++;
+
+                       /*
+                        * On CHV, the best supported Gamma capability is
+                        * CGM block, that takes 257 samples.
+                        * If user gives 256 samples (legacy mode), then
+                        * duplicate the 256th value to 257th.
+                        * "flag" is used to make sure it happens only once
+                        */
+                       if (num_samples == CHV_8BIT_GAMMA_MAX_VALS
+                                       && count == CHV_8BIT_GAMMA_MAX_VALS
+                                       && !flag) {
+                               count--;
+                               flag = true;
+                       }
+               }
+
+               /* Enable (CGM) Gamma on Pipe */
+               I915_WRITE(_PIPE_CGM_CONTROL(pipe),
+                       I915_READ(_PIPE_CGM_CONTROL(pipe))
+                               | CGM_GAMMA_EN);
+               DRM_DEBUG_DRIVER("CGM Gamma enabled on Pipe %c\n",
+                               pipe_name(pipe));
+               ret = 0;
+               break;
+
+       default:
+               DRM_ERROR("Invalid number of samples for Gamma LUT\n");
+               ret = -EINVAL;
+       }
+
+       return ret;
+}
+
+void intel_color_manager_crtc_commit(struct drm_device *dev,
+               struct drm_crtc_state *crtc_state)
+{
+       struct drm_property_blob *blob;
+       struct drm_crtc *crtc = crtc_state->crtc;
+       int ret = -EINVAL;
+
+       blob = crtc_state->palette_after_ctm_blob;
+       if (blob) {
+               /* Gamma correction is platform specific */
+               if (IS_CHERRYVIEW(dev))
+                       ret = chv_set_gamma(dev, blob, crtc);
+
+               if (ret)
+                       DRM_ERROR("set Gamma correction failed\n");
+               else
+                       DRM_DEBUG_DRIVER("Gamma correction success\n");
+       }

If I'm understanding the code properly, then it looks like I can't
disable gamma correction by just removing the current blob (i.e., by
updating the blob ID to be 0)?  Instead I have to actually create a new,
valid blob that has num_samples set to zero inside the blob in order to
disable the functionality.  That isn't the behavior I would have
intuitively expected, but that's probably more of a call for the guys
working on the userspace code that actually uses this functionality.
Just thought I'd make a note of it since it seemed a bit surprising to
me.

Actually, the way to disable gamma/degamma is to pass num_samples=0 in the blob. The same is agreed when we had this design discussion. Do you think it makes sense now ?
(btw, I think all the comments on this patch also apply to patches #11
and 14)

Ok, will take care of that.
Matt

+}
+
  int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
                struct drm_crtc_state *crtc_state,
                struct drm_mode_object *obj, uint32_t blob_id)
@@ -99,5 +242,8 @@ void intel_attach_color_properties_to_crtc(struct drm_device 
*dev,
                        drm_object_attach_property(mode_obj,
                                config->cm_crtc_palette_capabilities_property,
                                capabilities_blob_id);
+               if (config->cm_palette_after_ctm_property)
+                       drm_object_attach_property(mode_obj,
+                               config->cm_palette_after_ctm_property, 0);
        }
  }
diff --git a/drivers/gpu/drm/i915/intel_color_manager.h 
b/drivers/gpu/drm/i915/intel_color_manager.h
index 51aeb91..8bbac20 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.h
+++ b/drivers/gpu/drm/i915/intel_color_manager.h
@@ -28,6 +28,26 @@
  #include <drm/drm_crtc_helper.h>
  #include "i915_drv.h"

+#define _GAMMA_INT_PART(value)                 (value >> GAMMA_INT_SHIFT)
+#define _GAMMA_FRACT_PART(value)               (value >> GAMMA_FRACT_SHIFT)
+
  #define CHV_PALETTE_STRUCT_VERSION            1
  #define CHV_DEGAMMA_MAX_VALS                  65
  #define CHV_10BIT_GAMMA_MAX_VALS              257
+
+/* Gamma correction */
+#define CHV_GAMMA_DATA_STRUCT_VERSION          1
+#define CHV_10BIT_GAMMA_MAX_VALS               257
+#define CHV_8BIT_GAMMA_MAX_VALS                        256
+#define CHV_10BIT_GAMMA_MSB_SHIFT              6
+#define CHV_GAMMA_SHIFT_GREEN                  16
+/* Gamma values are u8.24 format */
+#define GAMMA_INT_SHIFT                                24
+#define GAMMA_FRACT_SHIFT                      8
+/* Max int value for Gamma is 1 */
+#define GAMMA_INT_MAX                          1
+/* Max value for Gamma on CHV */
+#define CHV_MAX_GAMMA                          0x10000
+
+/* CHV CGM Block */
+#define CGM_GAMMA_EN                           (1 << 2)
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 349a1c2..9d1a6c3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13641,6 +13641,8 @@ static void intel_begin_crtc_commit(struct drm_crtc 
*crtc,

        if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
                skl_detach_scalers(intel_crtc);
+
+       intel_color_manager_crtc_commit(dev, crtc->state);
  }

  static void intel_finish_crtc_commit(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 820ded7..de3e6e7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1444,5 +1444,7 @@ void intel_attach_color_properties_to_crtc(struct 
drm_device *dev,
  int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
                struct drm_crtc_state *crtc_state,
                struct drm_mode_object *obj, uint32_t blob_id);
+void intel_color_manager_crtc_commit(struct drm_device *dev,
+               struct drm_crtc_state *crtc_state);

  #endif /* __INTEL_DRV_H__ */
--
1.9.1


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

Reply via email to