Re: [PATCH 09/12] drm/i915: Add pipe level Gamma correction for CHV/BSW

[Matt Roper <matthew.d.roper@intel.com>]

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@intel.com>
> Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
> ---
>  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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel