Re: [PATCH 09/18] drm/i915: Pipe level Gamma correction for CHV/BSW

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

On Thu, Aug 06, 2015 at 10:08:18PM +0530, Shashank Sharma wrote:
> From: Kausal Malladi <kausalmalladi@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.sharma@intel.com>
> Signed-off-by: Kausal Malladi <kausalmalladi@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.

> +{
> +	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.


> +
> +	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.

> +		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.

(btw, I think all the comments on this patch also apply to patches #11
and 14)

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
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx