On Fri, Jul 03, 2015 at 09:01:44AM +0530, Kausal Malladi wrote: > CHV/BSW platform supports various Gamma correction modes, which are: > 1. Legacy 8-bit mode > 2. 10-bit CGM (Color Gamut Mapping) mode > > This patch does the following: > 1. Adds the core function to program Gamma correction values for CHV/BSW > platform > 2. Adds Gamma correction macros/defines > > Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxxxx> > Signed-off-by: Kausal Malladi <Kausal.Malladi@xxxxxxxxx> > --- > drivers/gpu/drm/i915/i915_reg.h | 10 ++ > drivers/gpu/drm/i915/intel_atomic.c | 6 ++ > drivers/gpu/drm/i915/intel_color_manager.c | 154 +++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_color_manager.h | 12 +++ > drivers/gpu/drm/i915/intel_drv.h | 2 + > 5 files changed, 184 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 313b1f9..36672e7 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -7900,4 +7900,14 @@ enum skl_disp_power_wells { > #define _PALETTE_A (dev_priv->info.display_mmio_offset + 0xa000) > #define _PALETTE_B (dev_priv->info.display_mmio_offset + 0xa800) > > +/* Color Management */ > +#define PIPEA_CGM_CONTROL (VLV_DISPLAY_BASE + 0x67A00) > +#define PIPEA_CGM_GAMMA_MIN (VLV_DISPLAY_BASE + 0x67000) > +#define CGM_OFFSET 0x2000 > +#define GAMMA_OFFSET 0x2000 > +#define _PIPE_CGM_CONTROL(pipe) \ > + (PIPEA_CGM_CONTROL + (pipe * CGM_OFFSET)) > +#define _PIPE_GAMMA_BASE(pipe) \ > + (PIPEA_CGM_GAMMA_MIN + (pipe * GAMMA_OFFSET)) > + > #endif /* _I915_REG_H_ */ > diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c > index d2674a6..21f0ac2 100644 > --- a/drivers/gpu/drm/i915/intel_atomic.c > +++ b/drivers/gpu/drm/i915/intel_atomic.c > @@ -473,6 +473,12 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc, > struct drm_property *property, > uint64_t val) > { > + struct drm_device *dev = crtc->dev; > + struct drm_mode_config *config = &dev->mode_config; > + > + if (property == config->prop_palette_after_ctm) > + return intel_color_manager_set_gamma(dev, &crtc->base, val); > + I think we discussed this on a previous iteration of the patch series, but .atomic_set_property() isn't supposed to actually update the hardware at all itself (as you're doing here); it's only supposed to update the 'state' parameter that was passed here. Further down the atomic pipeline the driver will decide whether it really wants to program any of the state or not and then the CRTC's atomic commit function should program the registers according to whatever value is set in the state object. > DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name); > return -EINVAL; > } > diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c > index 71b4c05..84cc3e47 100644 > --- a/drivers/gpu/drm/i915/intel_color_manager.c > +++ b/drivers/gpu/drm/i915/intel_color_manager.c > @@ -27,6 +27,160 @@ > > #include "intel_color_manager.h" > > +int chv_set_gamma(struct drm_device *dev, uint32_t blob_id, > + struct drm_crtc *crtc) > +{ > + struct drm_palette *gamma_data; > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct drm_property_blob *blob; > + struct drm_mode_config *config = &dev->mode_config; > + u32 cgm_control_reg = 0; > + u32 cgm_gamma_reg = 0; > + u32 reg, val; > + enum pipe pipe; > + u16 red, green, blue; > + u32 count = 0; > + struct drm_r32g32b32 *correction_values = NULL; > + u32 num_samples; > + u32 word; > + u32 palette; > + int ret = 0, length; > + > + blob = drm_property_lookup_blob(dev, blob_id); > + if (!blob) { > + DRM_ERROR("Invalid Blob ID\n"); > + return -EINVAL; > + } > + > + gamma_data = (struct drm_palette *)blob->data; Do we need to validate that the blob we receive is of the expected size or does something in the DRM core's blob handling take care of that for us? We don't want userspace to be able to trigger a panic by passing a smaller than expected blob here. > + > + if (gamma_data->version != CHV_GAMMA_DATA_STRUCT_VERSION) { > + DRM_ERROR("Invalid Gamma Data struct version\n"); > + return -EINVAL; > + } > + > + pipe = to_intel_crtc(crtc)->pipe; > + num_samples = gamma_data->palette_num_samples; > + length = num_samples * sizeof(struct drm_r32g32b32); > + > + if (num_samples == 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; > + } else if (num_samples == CHV_8BIT_GAMMA_MAX_VALS) { > + > + /* Disable CGM Gamma, if already set */ > + cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe)); > + cgm_control_reg &= ~CGM_GAMMA_EN; > + I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg); > + > + /* Write LUT values to registers */ > + palette = _PIPE_GAMMA_BASE(pipe); > + count = 0; > + correction_values = > + (struct drm_r32g32b32 *)&gamma_data->palette_lut; > + while (count < num_samples) { > + blue = correction_values[count].b32; > + green = correction_values[count].g32; > + red = correction_values[count].r32; > + > + blue = blue >> CHV_8BIT_GAMMA_MSB_SHIFT; > + green = green >> CHV_8BIT_GAMMA_MSB_SHIFT; > + red = red >> CHV_8BIT_GAMMA_MSB_SHIFT; > + > + /* Red (23:16), Green (15:8), Blue (7:0) */ > + word = (red << CHV_8BIT_GAMMA_SHIFT_RED_REG) | > + (green << > + CHV_8BIT_GAMMA_SHIFT_GREEN_REG) | > + blue; > + I915_WRITE(palette, word); > + > + palette += 4; > + count++; > + } > + DRM_DEBUG_DRIVER("Gamma LUT loaded successfully for Pipe %c\n", > + pipe_name(pipe)); > + > + /* Enable Gamma on Pipe */ > + reg = PIPECONF(pipe); > + val = I915_READ(reg) | PIPECONF_GAMMA; > + I915_WRITE(reg, val); > + DRM_DEBUG_DRIVER("Legacy Gamma enabled on Pipe %c\n", > + pipe_name(pipe)); > + > + ret = 0; > + } else if (num_samples == CHV_10BIT_GAMMA_MAX_VALS) { > + cgm_gamma_reg = _PIPE_GAMMA_BASE(pipe); > + > + count = 0; > + correction_values = > + (struct drm_r32g32b32 *)&gamma_data->palette_lut; > + while (count < num_samples) { > + blue = correction_values[count].b32; > + green = correction_values[count].g32; > + red = correction_values[count].r32; > + > + blue = blue >> CHV_10BIT_GAMMA_MSB_SHIFT; > + green = green >> CHV_10BIT_GAMMA_MSB_SHIFT; > + red = red >> CHV_10BIT_GAMMA_MSB_SHIFT; > + > + /* Green (25:16) and Blue (9:0) to be written */ > + word = (green << CHV_GAMMA_SHIFT_GREEN) | blue; > + I915_WRITE(cgm_gamma_reg, word); > + cgm_gamma_reg += 4; > + > + /* Red (9:0) to be written */ > + word = red; > + I915_WRITE(cgm_gamma_reg, word); > + > + cgm_gamma_reg += 4; > + count++; > + } > + DRM_DEBUG_DRIVER("Gamma LUT loaded successfully for Pipe %c\n", > + pipe_name(pipe)); > + > + /* 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; > + } else { > + DRM_ERROR("Invalid number of samples for Gamma LUT\n"); > + return -EINVAL; > + } > + > + ret = drm_property_replace_global_blob(dev, &blob, length, > + (void *) gamma_data, &crtc->base, > + config->prop_palette_after_ctm); > + > + if (ret) { > + DRM_ERROR("Error updating Gamma blob\n"); > + return -EFAULT; > + } > + > + return ret; > +} > + > +int intel_color_manager_set_gamma(struct drm_device *dev, > + struct drm_mode_object *obj, uint32_t blob_id) > +{ > + struct drm_crtc *crtc = obj_to_crtc(obj); > + > + if (IS_CHERRYVIEW(dev)) > + return chv_set_gamma(dev, blob_id, crtc); > + > + return -EINVAL; As noted earlier, it seems confusing to me to have userspace see a CRTC property for functionality that isn't supported at all on the platform, but if we're going to reject invalid platforms here, we should have at least a DRM_DEBUG message to give some clues as to why the -EINVAL was returned. > +} > + > int get_chv_pipe_capabilities(struct drm_device *dev, > struct drm_color_caps *color_caps, struct drm_crtc *crtc) > { > diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h > index 32262ac..d83567a 100644 > --- a/drivers/gpu/drm/i915/intel_color_manager.h > +++ b/drivers/gpu/drm/i915/intel_color_manager.h > @@ -31,6 +31,8 @@ > #define CHV_PALETTE_STRUCT_VERSION 1 > #define CHV_CTM_STRUCT_VERSION 1 > #define CHV_PLATFORM_STRUCT_VERSION 1 > +#define CHV_GAMMA_DATA_STRUCT_VERSION 1 > + > #define CHV_MAX_PALETTE_CAPS_BEFORE_CTM 1 > #define CHV_MAX_PALETTE_CAPS_AFTER_CTM 2 > #define CHV_DEGAMMA_PRECISION 14 > @@ -43,6 +45,16 @@ > #define CHV_10BIT_GAMMA_MAX_VALS 257 > #define CHV_8BIT_GAMMA_PRECISION 8 > #define CHV_8BIT_GAMMA_MAX_VALS 256 > +#define CHV_8BIT_GAMMA_MSB_SHIFT 8 > +#define CHV_8BIT_GAMMA_SHIFT_RED_REG 16 > +#define CHV_8BIT_GAMMA_SHIFT_GREEN_REG 8 > +#define CHV_10BIT_GAMMA_MSB_SHIFT 6 > +#define CHV_GAMMA_SHIFT_GREEN 16 > + > #define CHV_CSC_COEFF_MAX_PRECISION 12 > #define CHV_CSC_COEFF_MAX_INT 7 > #define CHV_CSC_COEFF_MIN_INT -7 > + > +/* CHV CGM Block */ > +/* Bit 2 to be enabled in CGM block for CHV */ > +#define CGM_GAMMA_EN 4 > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 053ceb0..a7aaadf 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1453,5 +1453,7 @@ extern const struct drm_plane_helper_funcs intel_plane_helper_funcs; > /* intel_color_manager.c */ > void intel_color_manager_attach(struct drm_device *dev, > struct drm_mode_object *mode_obj); > +int intel_color_manager_set_gamma(struct drm_device *dev, > + struct drm_mode_object *obj, uint32_t blob_id); > > #endif /* __INTEL_DRV_H__ */ > -- > 2.4.5 > -- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx