Re: [PATCH v2 07/10] drm/i915: Add pipe level Gamma correction for CHV/BSW

[Damien Lespiau <damien.lespiau@intel.com>]

(Note I haven't actually looked at the CHV specific details just yet,
that'll be for another pass).

On Thu, Jun 04, 2015 at 07:12:38PM +0530, Kausal Malladi wrote:
> +int chv_set_gamma(struct drm_device *dev, uint32_t blob_id,
> +		struct drm_crtc *crtc)
> +{
> +	struct drm_gamma *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;
> +	struct rgb_pixel correct_rgb;
> +	u32 count = 0;
> +	struct rgb_pixel *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");

Not DRM_ERROR. We do give -EINVAL hint for people developping user
space, but with DRM_DEBUG_KMS().

> +		return -EINVAL;
> +	}
> +
> +	gamma_data = (struct drm_gamma *)blob->data;
> +	pipe = to_intel_crtc(crtc)->pipe;
> +	num_samples = gamma_data->num_samples;
> +	length = num_samples * sizeof(struct rgb_pixel);
> +
> +	if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_UNKNOWN) {
> +
> +		/* 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 (gamma_data->gamma_precision == I915_GAMMA_PRECISION_LEGACY) {
> +
> +		if (num_samples != CHV_8BIT_GAMMA_MAX_VALS) {
> +			DRM_ERROR("Incorrect number of samples received\n");

Not DRM_ERROR. We do give -EINVAL hint for people developping user
space, but with DRM_DEBUG_KMS().

> +			return -EINVAL;
> +		}

This means the current code doesn't allow us to load LUTs with 256
values (like today). That's not what we want.

Have you looked at the interactions between this property and the legacy
gamma ioctl()?

> +
> +		/* First, 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);
> +
> +		/* Enable (Legacy) Gamma on Pipe */
> +		palette = _PIPE_GAMMA_BASE(pipe);
> +
> +		count = 0;
> +		correction_values = (struct rgb_pixel *)&gamma_data->values;
> +		while (count < num_samples) {
> +			correct_rgb = correction_values[count];
> +			blue = correction_values[count].blue;
> +			green = correction_values[count].green;
> +			red = correction_values[count].red;
> +
> +			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++;
> +		}
> +		reg = PIPECONF(pipe);
> +		val = I915_READ(reg) | PIPECONF_GAMMA;
> +		I915_WRITE(reg, val);
> +
> +		DRM_DEBUG_DRIVER("Gamma LUT loaded successfully for Pipe %c\n",
> +			pipe_name(pipe));
> +		ret = 0;
> +	} else if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_10BIT) {
> +
> +		if (num_samples != CHV_10BIT_GAMMA_MAX_VALS) {
> +			DRM_ERROR("Incorrect number of samples received\n");

Not DRM_ERROR. We do give -EINVAL hint for people developping user
space, but with DRM_DEBUG_KMS().

> +			return -EINVAL;
> +		}
> +
> +		/* Enable (CGM) Gamma on Pipe */
> +		cgm_gamma_reg = _PIPE_GAMMA_BASE(pipe);
> +
> +		count = 0;
> +		correction_values = (struct rgb_pixel *)&gamma_data->values;
> +		while (count < num_samples) {
> +			correct_rgb = correction_values[count];
> +			blue = correction_values[count].blue;
> +			green = correction_values[count].green;
> +			red = correction_values[count].red;
> +
> +			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++;
> +		}
> +
> +		I915_WRITE(_PIPE_CGM_CONTROL(pipe),
> +			I915_READ(_PIPE_CGM_CONTROL(pipe))
> +			| CGM_GAMMA_EN);
> +
> +		DRM_DEBUG_DRIVER("Gamma LUT loaded successfully for Pipe %c\n",
> +			pipe_name(pipe));
> +
> +		ret = 0;
> +	} else {
> +		DRM_ERROR("Invalid gamma_level received\n");

Not DRM_ERROR. We do give -EINVAL hint for people developping user
space, but with DRM_DEBUG_KMS().

> +		return -EFAULT;
> +	}
> +
> +	ret = drm_mode_crtc_update_color_property(&blob, length,
> +			(void *) gamma_data, &crtc->base,
> +			config->gamma_property);
> +
> +	if (ret) {
> +		DRM_ERROR("Error updating Gamma blob\n");

Not DRM_ERROR. We do give -EINVAL hint for people developping user
space, but with DRM_DEBUG_KMS().

> +		crtc->gamma_blob_id = INVALID_BLOB_ID;
> +		return -EFAULT;
> +	}
> +
> +	/* Save blob ID for future use */
> +	crtc->gamma_blob_id = blob->base.id;
> +	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);
> +
> +	DRM_DEBUG_DRIVER("\n");

Too verbose.

> +
> +	if (IS_CHERRYVIEW(dev))
> +		return chv_set_gamma(dev, blob_id, crtc);
> +
> +	return -EINVAL;
> +}
> +
>  void intel_color_manager_attach(struct drm_device *dev,
>  		struct drm_mode_object *mode_obj)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
> index a55ce23..0acf8e9 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.h
> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
> @@ -27,8 +27,70 @@
>  
>  #include <drm/drmP.h>
>  #include <drm/drm_crtc_helper.h>
> +#include "i915_drv.h"
> +
> +/* Color Management macros for Gamma */
> +#define I915_GAMMA_FLAG_DEGAMMA			(1 << 0)
> +#define I915_PIPE_GAMMA				(1 << 0)
> +#define I915_PLANE_GAMMA			(1 << 1)
> +#define I915_GAMMA_PRECISION_UNKNOWN		0
> +#define I915_GAMMA_PRECISION_CURRENT		0xFFFFFFFF
> +#define I915_GAMMA_PRECISION_LEGACY		(1 << 0)
> +#define I915_GAMMA_PRECISION_10BIT		(1 << 1)
> +#define I915_GAMMA_PRECISION_12BIT		(1 << 2)
> +#define I915_GAMMA_PRECISION_14BIT		(1 << 3)
> +#define I915_GAMMA_PRECISION_16BIT		(1 << 4)
> +
> +#define CHV_MAX_PIPES				3

we have the number of pipes in the device info in dev_priv.

> +#define CHV_DISPLAY_BASE			0x180000

We also have the display base ther.

> +#define INVALID_BLOB_ID				9999

This looks weird. If really needed, it should be part of the generic
blob interface.

> +struct rgb_pixel {
> +	u16 red;
> +	u16 green;
> +	u16 blue;
> +};

That shouldn't be exposed in this header. If this is how userspace will
give us values, how about including it in the interface itself? Can't be
as generic as "rgb_pixel" though, struct drm_lut_entry could be a
contender.

> +
> +/* CHV CGM Block */
> +/* Bit 2 to be enabled in CGM block for CHV */
> +#define CGM_GAMMA_EN				4

This should be part of the corresponding register defines (we group
defines per-register)

> +
> +/* Gamma */
> +#define CHV_GAMMA_VALS				257

This define is unused in this patch and duplicates with
CHV_8BIT_GAMMA_MAX_VALS.

> +#define CHV_10BIT_GAMMA_MAX_INDEX		256
> +#define CHV_8BIT_GAMMA_MAX_INDEX		255
> +#define CHV_10BIT_GAMMA_MSB_SHIFT		6
> +#define CHV_GAMMA_EVEN_MASK			0xFF

Not used in this patch.

> +#define CHV_GAMMA_SHIFT_BLUE			0
> +#define CHV_GAMMA_SHIFT_GREEN			16
> +#define CHV_GAMMA_SHIFT_RED			0
> +#define CHV_GAMMA_ODD_SHIFT			8

Not used in this patch.

> +#define CHV_CLRMGR_GAMMA_GCMAX_SHIFT		17

Not used in this patch.

> +#define CHV_GAMMA_GCMAX_MASK			0x1FFFF

Not used in this patch.

> +#define CHV_GAMMA_GCMAX_MAX			0x400

Not used in this patch.

> +#define CHV_10BIT_GAMMA_MAX_VALS		(CHV_10BIT_GAMMA_MAX_INDEX + 1)
> +#define CHV_8BIT_GAMMA_MAX_VALS			(CHV_8BIT_GAMMA_MAX_INDEX + 1)
> +#define CHV_8BIT_GAMMA_MSB_SHIFT		8
> +#define CHV_8BIT_GAMMA_SHIFT_GREEN_REG		8
> +#define CHV_8BIT_GAMMA_SHIFT_RED_REG		16
> +
> +/* CGM Registers */
> +#define CGM_OFFSET				0x2000
> +#define GAMMA_OFFSET				0x2000
> +#define PIPEA_CGM_CONTROL			(CHV_DISPLAY_BASE + 0x67A00)
> +#define PIPEA_CGM_GAMMA_MIN			(CHV_DISPLAY_BASE + 0x67000)
> +#define _PIPE_CGM_CONTROL(pipe) \
> +	(PIPEA_CGM_CONTROL + (pipe * CGM_OFFSET))
> +#define _PIPE_GAMMA_BASE(pipe) \
> +	(PIPEA_CGM_GAMMA_MIN + (pipe * GAMMA_OFFSET))

Use the _PIPE() macro. The '_' is, by convention, for symbols considered
private, so the PIPEA_* defines should have it, not the define used in
code.

>  
>  /* Generic Function prototypes */
>  void intel_color_manager_init(struct drm_device *dev);
>  void intel_color_manager_attach(struct drm_device *dev,
>  		struct drm_mode_object *mode_obj);
> +extern int intel_color_manager_set_gamma(struct drm_device *dev,
> +		struct drm_mode_object *obj, uint32_t blob_id);

extern or not? one must choose (we go without extern these days).

> +
> +/* Platform specific function prototypes */
> +extern int chv_set_gamma(struct drm_device *dev,
> +		uint32_t blob_id, struct drm_crtc *crtc);
> -- 
> 2.4.2
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel