[Intel-gfx] [PATCH 21/22] drm/i915: BDW: Pipe level degamma correction

[Rob Bradford]

On Sat, 2015-10-10 at 00:59 +0530, Shashank Sharma wrote:
> BDW/SKL/BXT supports Degamma color correction feature, which
> linearizes the non-linearity due to gamma encoded color values.
> This will be applied before Color Transformation.
> 
> This patch does the following:
> 1. Adds the core function to program DeGamma correction values for
>    BDW/SKL/BXT platform
> 2. Adds DeGamma correction macros/defines
> 
> Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
> Signed-off-by: Kausal Malladi <kausalmalladi at gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_color_manager.c | 59
> ++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c
> b/drivers/gpu/drm/i915/intel_color_manager.c
> index 74f8fc3..e659382 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.c
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -273,6 +273,63 @@ static int bdw_set_gamma(struct drm_device *dev,
> struct drm_property_blob *blob,
>  	return 0;
>  }
>  
> +static int bdw_set_degamma(struct drm_device *dev,
> +	struct drm_property_blob *blob, struct drm_crtc *crtc)
> +{
> +	enum pipe pipe;
> +	int num_samples, length;
> +	u32 index, mode;
> +	u32 pal_prec_index, pal_prec_data;
> +	struct drm_palette *degamma_data;
> +	struct drm_crtc_state *state = crtc->state;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_r32g32b32 *correction_values = NULL;
> +
> +	if (WARN_ON(!blob))
> +		return -EINVAL;
> +
> +	degamma_data = (struct drm_palette *)blob->data;
> +	pipe = to_intel_crtc(crtc)->pipe;
> +	num_samples = degamma_data->num_samples;
> +
> +	if (num_samples == GAMMA_DISABLE_VALS) {
> +		/* Disable degamma on Pipe */
> +		mode = I915_READ(GAMMA_MODE(pipe)) &
> ~GAMMA_MODE_MODE_MASK;
> +		I915_WRITE(GAMMA_MODE(pipe), mode |
> GAMMA_MODE_MODE_8BIT);

When you turn off gamma you call bdw_reset_gamma() should you do the
same for degamma?

> +
> +		state->palette_before_ctm_blob = NULL;
> +		DRM_DEBUG_DRIVER("Disabling degamma on Pipe %c\n",
> +		pipe_name(pipe));
> +		return 0;
> +	}
> +
> +	if (num_samples != BDW_SPLITGAMMA_MAX_VALS) {
> +		DRM_ERROR("Invalid number of samples\n");
> +		return -EINVAL;
> +	}
> +
> +	length = num_samples * sizeof(struct drm_r32g32b32);

Uh, you calculate this length and don't use it anywhere? Was your
intention to check the blob length? And the length check should be in
the generic DRM code anyway...

I think it was suggested in the past that the number of samples could
be derived from the length of the data allowing the removal of the
struct member.

> +	mode = I915_READ(GAMMA_MODE(pipe));

Move this closer to where you use it?

> +	pal_prec_index = _PREC_PAL_INDEX(pipe);
> +	pal_prec_data = _PREC_PAL_DATA(pipe);
> +
> +	correction_values = (struct drm_r32g32b32 *)&degamma_data
> ->lut;

Why do you need this cast?

> +	index = I915_READ(pal_prec_index);
> +	index |= BDW_INDEX_AUTO_INCREMENT | BDW_INDEX_SPLIT_MODE;
> +	I915_WRITE(pal_prec_index, index);
> +
> +	bdw_write_10bit_gamma_precision(dev, correction_values,
> +		pal_prec_data, BDW_SPLITGAMMA_MAX_VALS);
> +
> +	/* Enable DeGamma on Pipe */
> +	mode &= ~GAMMA_MODE_MODE_MASK;
> +	I915_WRITE(GAMMA_MODE(pipe), mode | GAMMA_MODE_MODE_SPLIT);
> +	DRM_DEBUG_DRIVER("DeGamma correction enabled on Pipe %c\n",

Choose your capitalisation DeGamma or degamma. Pick one and use it
consistently to make it easier to grep through the code.

It also looks like you should check if the gamma mode is not something
other than split / off. Otherwise strange things could happen.
Similarly in the gamma code you shouldn't be able to program something
other than split if you have a degamma mode set.

> +	pipe_name(pipe));
> +
> +	return 0;
> +}
> +
>  static s16 chv_prepare_csc_coeff(s64 csc_value)
>  {
>  	s32 csc_int_value;
> @@ -579,6 +636,8 @@ void intel_color_manager_crtc_commit(struct
> drm_device *dev,
>  		/* Degamma correction */
>  		if (IS_CHERRYVIEW(dev))
>  			ret = chv_set_degamma(dev, blob, crtc);
> +		else if (IS_BROADWELL(dev) || IS_GEN9(dev))
> +			ret = bdw_set_degamma(dev, blob, crtc);
>  
>  		if (ret)
>  			DRM_ERROR("set degamma correction
> failed\n");

Rob