[3/4] drm/i915: CSC color correction

Commit Message

Sharma, Shashank Sept. 9, 2014, 6:23 a.m. UTC

From: Shashank Sharma <[e-mail]>

This patch adds support for CSC correction color property.
It does the following:
1. Creates a new DRM property for CSC correction. Adds this into
   mode_config.
2. Attaches this CSC property to calling CRTC. Creates a blob
   to store the correction values, and attaches the blob to CRTC.
3. Adds function intel_clrmgr_set_csc: This is a wrapper function
   which checks the platform type, and calls the valleyview
   specific set_csc function. As different platforms may have different
   methods of setting CSC, wrapper function is required to route to proper
   core CSC set function. In future, the support for other platfroms can be
   plugged-in here. Adding this function as .set_property CSC color property.
4. Adds function vlv_set_csc: core function to program CSC coefficients as per
   vlv specs, and then enable CSC.

Signed-off-by: Shashank Sharma <[e-mail]>
---
 drivers/gpu/drm/drm_crtc.c          |   4 +-
 drivers/gpu/drm/i915/i915_reg.h     |  11 ++
 drivers/gpu/drm/i915/intel_clrmgr.c | 208 +++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_clrmgr.h |  16 +++
 drivers/gpu/drm/i915/intel_drv.h    |   1 +
 include/drm/drm_crtc.h              |   7 ++
 6 files changed, 244 insertions(+), 3 deletions(-)

Comments

Paauwe, Bob J Sept. 9, 2014, 10:51 p.m. UTC | #1

On Tue, 9 Sep 2014 11:53:15 +0530
<[e-mail]> wrote:

> From: Shashank Sharma <[e-mail]>
> 
> This patch adds support for CSC correction color property.
> It does the following:
> 1. Creates a new DRM property for CSC correction. Adds this into
>    mode_config.
> 2. Attaches this CSC property to calling CRTC. Creates a blob
>    to store the correction values, and attaches the blob to CRTC.
> 3. Adds function intel_clrmgr_set_csc: This is a wrapper function
>    which checks the platform type, and calls the valleyview
>    specific set_csc function. As different platforms may have different
>    methods of setting CSC, wrapper function is required to route to proper
>    core CSC set function. In future, the support for other platfroms can be
>    plugged-in here. Adding this function as .set_property CSC color property.
> 4. Adds function vlv_set_csc: core function to program CSC coefficients as per
>    vlv specs, and then enable CSC.
> 
> Signed-off-by: Shashank Sharma <[e-mail]>
> ---
>  drivers/gpu/drm/drm_crtc.c          |   4 +-
>  drivers/gpu/drm/i915/i915_reg.h     |  11 ++
>  drivers/gpu/drm/i915/intel_clrmgr.c | 208 +++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_clrmgr.h |  16 +++
>  drivers/gpu/drm/i915/intel_drv.h    |   1 +
>  include/drm/drm_crtc.h              |   7 ++
>  6 files changed, 244 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 272b66f..be9d110 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -3917,7 +3917,7 @@ done:
>  	return ret;
>  }
>  
> -static struct drm_property_blob *drm_property_create_blob(struct drm_device *dev, int length,
> +struct drm_property_blob *drm_property_create_blob(struct drm_device *dev, int length,
>  							  void *data)
>  {
>  	struct drm_property_blob *blob;
> @@ -3944,7 +3944,7 @@ static struct drm_property_blob *drm_property_create_blob(struct drm_device *dev
>  	return blob;
>  }
>  
> -static void drm_property_destroy_blob(struct drm_device *dev,
> +void drm_property_destroy_blob(struct drm_device *dev,
>  			       struct drm_property_blob *blob)
>  {
>  	drm_mode_object_put(dev, &blob->base);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 20673cc..e3010b3 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6183,6 +6183,17 @@ enum punit_power_well {
>  #define PIPE_CSC_POSTOFF_ME(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_ME, _PIPE_B_CSC_POSTOFF_ME)
>  #define PIPE_CSC_POSTOFF_LO(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_LO, _PIPE_B_CSC_POSTOFF_LO)
>  
> +/* VLV color correction registers */
> +/* CSC */
> +#define PIPECONF_CSC_ENABLE	(1 << 15)
> +#define _PIPEACSC		(dev_priv->info.display_mmio_offset + \
> +								0x600b0)
> +#define _PIPEBCSC		(dev_priv->info.display_mmio_offset + \
> +								0x610b0)
> +#define PIPECSC(pipe)		(_PIPEACSC + (pipe *  CSC_OFFSET))
> +#define CSC_OFFSET			(_PIPEBCSC - _PIPEACSC)
> +#define PIPECSC(pipe)		(_PIPEACSC + (pipe *  CSC_OFFSET))
> +
>  /* VLV MIPI registers */
>  
>  #define _MIPIA_PORT_CTRL			(VLV_DISPLAY_BASE + 0x61190)
> diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c b/drivers/gpu/drm/i915/intel_clrmgr.c
> index ac0a890..36c64c1 100644
> --- a/drivers/gpu/drm/i915/intel_clrmgr.c
> +++ b/drivers/gpu/drm/i915/intel_clrmgr.c
> @@ -32,6 +32,138 @@
>  #include "i915_reg.h"
>  #include "intel_clrmgr.h"
>  
> +/*
> +* Names to register with color properties.
> +* Sequence must be the same as the order
> +* of enum clrmgr_tweaks
> +*/
> +const char *clrmgr_property_names[] = {
> +	/* csc */
> +	"csc-correction",
> +	/* add a new prop name here */
> +};
> +
> +
> +/*
> +* Sizes for color properties. This can differ
> +* platform by platform, hence 'vlv' prefix
> +* The sequence must be same as the order of
> +* enum clrmgr_tweaks
> +*/
> +u32 vlv_color_property_sizes[] = {
> +	VLV_CSC_MATRIX_MAX_VALS,
> +	/* Add new property size here */
> +};
> +
> +/* Default CSC values to create property with */
> +uint64_t vlv_csc_default[VLV_CSC_MATRIX_MAX_VALS] = {
> +	0x400, 0, 0, 0, 0x400, 0, 0, 0, 0x400
> +};
> +
> +/*
> +* vlv_set_csc
> +* Valleyview specific csc correction method.
> +* Programs the 6 csc registers with 3x3 correction matrix
> +* values.
> +* inputs:
> +* - intel_crtc*
> +* - color manager registered property for csc correction
> +* - data: pointer to correction values to be applied

The comment isn't matching the function. data is not a pointer, it's a
single 64bit value.  Also, the boolean enable isn't described in the
comment block.

> +*/
> +/* Enable color space conversion on PIPE */
> +bool vlv_set_csc(struct intel_crtc *intel_crtc,
> +	struct drm_property *prop, uint64_t values, bool enable)
> +{
> +	u32 count = 0;
> +	u32 c0, c1, c2;
> +	u32 pipeconf, csc_reg, data_size;
> +	uint64_t *blob_data;
> +	uint64_t *data = NULL;
> +	struct drm_device *dev = intel_crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_property_blob *blob = intel_crtc->csc_blob;
> +
> +	/* Sanity */
> +	if (!blob || (blob->length != VLV_CSC_MATRIX_MAX_VALS)) {
> +		DRM_ERROR("Invalid/NULL CSC blob\n");
> +		return false;
> +	}
> +	blob_data = (uint64_t *)blob->data;
> +
> +	/* No need of values to disable property */
> +	if (!enable)
> +		goto configure;
> +
> +	/* Enabling property needs correction values */
> +	data_size = VLV_CSC_MATRIX_MAX_VALS;
> +	data = kmalloc(sizeof(uint64_t) * (data_size), GFP_KERNEL);
> +	if (!data) {
> +		DRM_ERROR("Out of memory\n");
> +		return false;
> +	}
> +
> +	if (copy_from_user((void *)data, (const void __user *)values,
> +			data_size * sizeof(uint64_t))) {
> +		DRM_ERROR("Failed to copy all data\n");
> +		kfree(data);
> +		return false;
> +	}

I don't think this should be doing a copy_from_user.  It should be the
drm_property code that handles the IOCTL that does that.

The whole handling of the blob property looks suspect to me.  I believe
that the DRM code currently doesn't allow blob type properties to be
set. So you should first have a patch that adds that capability to the
DRM property code.  But I'm not sure that's even the right way to
handle this. 

> +
> +	DRM_DEBUG_DRIVER("Setting CSC on pipe = %d\n", intel_crtc->pipe);
> +	csc_reg = PIPECSC(intel_crtc->pipe);
> +
> +	/* Read CSC matrix, one row at a time */
> +	while (count < VLV_CSC_MATRIX_MAX_VALS) {
> +		c0 = data[count] & VLV_CSC_VALUE_MASK;
> +		*blob_data++ = c0;
> +		c1 = data[count] & VLV_CSC_VALUE_MASK;
> +		*blob_data++ = c1;
> +		c2 = data[count] & VLV_CSC_VALUE_MASK;
> +		*blob_data++ = c2;

You aren't incrementing count after each assignment above, that means
that c0, c1, and c2 are all getting set to the same value. That doesn't
seem right.

> +
> +		/* C0 is LSB 12bits, C1 is MSB 16-27 */
> +		I915_WRITE(csc_reg, (c1 << VLV_CSC_COEFF_SHIFT) | c0);
> +		csc_reg += 4;
> +
> +		/* C2 is LSB 12 bits */
> +		I915_WRITE(csc_reg, c2);
> +		csc_reg += 4;
> +	}
> +
> +configure:
> +
> +	/* Enable / Disable csc correction */
> +	pipeconf = I915_READ(PIPECONF(intel_crtc->pipe));
> +	enable ? (pipeconf |= PIPECONF_CSC_ENABLE) :
> +		(pipeconf &= ~PIPECONF_CSC_ENABLE);
> +	I915_WRITE(PIPECONF(intel_crtc->pipe), pipeconf);
> +	POSTING_READ(PIPECONF(intel_crtc->pipe));
> +	DRM_DEBUG_DRIVER("CSC successfully %s pipe %C\n",
> +		enable ? "enabled" : "disabled", pipe_name(intel_crtc->pipe));
> +
> +	kfree(data);
> +	return true;
> +}
> +
> +bool intel_clrmgr_set_csc(struct drm_crtc *crtc,
> +	struct drm_property *prop, uint64_t values)
> +{
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct drm_device *dev = intel_crtc->base.dev;
> +	bool enable;
> +
> +	/* First value is enable/disable control, others are data */
> +	enable = *((uint64_t *)values);
> +	values += (sizeof(uint64_t));

It looks like you're trying to pass in a user space pointer to some
type of csc structure.  As above, I'm not sure that's the right way to
approach this. Ideally, I think we want to use the standard drm
property types or you're going to have to define a new drm property
type that could be universally used.

> +
> +	if (IS_VALLEYVIEW(dev))
> +		return vlv_set_csc(intel_crtc, prop, values, enable);
> +
> +	/* Todo: Support other gen devices */
> +	DRM_ERROR("Color correction is supported only on VLV for now\n");
> +	return false;
> +}
> +
>  void
>  intel_attach_plane_color_correction(struct intel_plane *intel_plane)
>  {
> @@ -41,18 +173,92 @@ intel_attach_plane_color_correction(struct intel_plane *intel_plane)
>  void
>  intel_attach_pipe_color_correction(struct intel_crtc *intel_crtc)
>  {
> +	struct drm_device *dev = intel_crtc->base.dev;
> +	struct drm_mode_object *obj = &intel_crtc->base.base;
> +	struct drm_property *property = NULL;
> +
>  	DRM_DEBUG_DRIVER("\n");
> +	mutex_lock(&dev->mode_config.mutex);
> +
> +	/* color property = csc */
> +	property = dev->mode_config.csc_property;
> +	if (!property) {
> +		DRM_ERROR("No such property to attach\n");
> +		goto release_mutex;
> +	}
> +
> +	/* create blob for correction values */
> +	intel_crtc->csc_blob = drm_property_create_blob(dev,
> +		vlv_color_property_sizes[csc], (void *)vlv_csc_default);
> +	if (!intel_crtc->csc_blob) {
> +		DRM_ERROR("Failed to create property blob\n");
> +		goto release_mutex;
> +	}
> +
> +	/* Attach blob with property */
> +	if (drm_object_property_set_value(obj, property,
> +			intel_crtc->csc_blob->base.id)) {
> +		DRM_ERROR("Failed to attach property blob, destroying\n");
> +		drm_property_destroy_blob(dev, intel_crtc->csc_blob);
> +		goto release_mutex;
> +	}
> +
> +	DRM_DEBUG_DRIVER("Successfully attached CSC property\n");
> +
> +release_mutex:
> +	mutex_unlock(&dev->mode_config.mutex);
>  }
>  
>  int intel_clrmgr_create_color_properties(struct drm_device *dev)
>  {
> +	int ret = 0;
> +	struct drm_property *property;
> +
>  	DRM_DEBUG_DRIVER("\n");
> -	return 0;
> +	mutex_lock(&dev->mode_config.mutex);
> +
> +	/* CSC correction color property, blob type, size 0 */
> +	property = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> +		clrmgr_property_names[csc], 0);
> +	if (!property) {
> +		DRM_ERROR("Failed to create property(CSC)\n");
> +		ret++;
> +	} else {
> +		dev->mode_config.csc_property = property;
> +		DRM_DEBUG_DRIVER("Created property: CSC\n");
> +	}
> +
> +	mutex_unlock(&dev->mode_config.mutex);
> +	return ret;
>  }
>  
>  void intel_clrmgr_destroy_color_properties(struct drm_device *dev)
>  {
> +	struct drm_crtc *crtc;
> +	struct intel_crtc *intel_crtc;
> +
>  	DRM_DEBUG_DRIVER("\n");
> +
> +	mutex_lock(&dev->mode_config.mutex);
> +
> +	/* CSC correction color property */
> +	if (dev->mode_config.csc_property) {
> +		drm_property_destroy(dev, dev->mode_config.csc_property);
> +		dev->mode_config.csc_property = NULL;
> +		DRM_DEBUG_DRIVER("Destroyed property: CSC\n");
> +	}
> +
> +	/* Destroy property blob from each CRTC */
> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> +		intel_crtc = to_intel_crtc(crtc);
> +		if (intel_crtc->csc_blob) {
> +			drm_property_destroy_blob(dev, intel_crtc->csc_blob);
> +			intel_crtc->csc_blob = NULL;
> +		}
> +	}
> +
> +	mutex_unlock(&dev->mode_config.mutex);
> +	DRM_DEBUG_DRIVER("Successfully destroyed all color properties\n");
>  }
>  
>  void intel_clrmgr_init(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_clrmgr.h b/drivers/gpu/drm/i915/intel_clrmgr.h
> index 8cff487..5725d6b 100644
> --- a/drivers/gpu/drm/i915/intel_clrmgr.h
> +++ b/drivers/gpu/drm/i915/intel_clrmgr.h
> @@ -39,6 +39,13 @@
>  #define CLRMGR_PROP_NAME_MAX				128
>  #define CLRMGR_PROP_RANGE_MAX				0xFFFFFFFFFFFFFFFF
>  
> +/* CSC */
> + /* CSC / Wide gamut */
> +#define VLV_CSC_MATRIX_MAX_VALS		9
> +#define VLV_CSC_VALUE_MASK			0xFFF
> +#define VLV_CSC_COEFF_SHIFT			16
> +
> +
>  /* Properties */
>  enum clrmgr_tweaks {
>  	csc = 0,
> @@ -50,6 +57,15 @@ enum clrmgr_tweaks {
>  };
>  
>  /*
> +* intel_clrmgr_set_csc
> +* CSC correction method is different across various
> +* gen devices. This wrapper function calls the respective
> +* platform specific function to set CSC
> +*/
> +bool intel_clrmgr_set_csc(struct drm_crtc *crtc,
> +	struct drm_property *prop, uint64_t data);
> +
> +/*
>  * intel_attach_plane_color_correction:
>  * Attach plane level color correction DRM properties to
>  * corresponding plane objects.
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 7ba5785..a10b9bb 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -437,6 +437,7 @@ struct intel_crtc {
>  
>  	int scanline_offset;
>  	struct intel_mmio_flip mmio_flip;
> +	struct drm_property_blob *csc_blob;
>  };
>  
>  struct intel_plane_wm_parameters {
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 31344bf..487ce44 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -851,6 +851,9 @@ struct drm_mode_config {
>  	struct drm_property *aspect_ratio_property;
>  	struct drm_property *dirty_info_property;
>  
> +	/* Color correction properties */
> +	struct drm_property *csc_property;
> +
>  	/* dumb ioctl parameters */
>  	uint32_t preferred_depth, prefer_shadow;
>  
> @@ -981,6 +984,10 @@ extern int drm_mode_connector_set_path_property(struct drm_connector *connector,
>  						char *path);
>  extern int drm_mode_connector_update_edid_property(struct drm_connector *connector,
>  						struct edid *edid);
> +extern struct drm_property_blob *drm_property_create_blob(struct drm_device *dev,
> +						int length, void *data);
> +extern void drm_property_destroy_blob(struct drm_device *dev,
> +			       struct drm_property_blob *blob);
>  
>  static inline bool drm_property_type_is(struct drm_property *property,
>  		uint32_t type)

Matt Roper Sept. 10, 2014, 1:30 a.m. UTC | #2

On Tue, Sep 09, 2014 at 11:53:15AM +0530, [e-mail] wrote:
> From: Shashank Sharma <[e-mail]>
> 
> This patch adds support for CSC correction color property.
> It does the following:
> 1. Creates a new DRM property for CSC correction. Adds this into
>    mode_config.
> 2. Attaches this CSC property to calling CRTC. Creates a blob
>    to store the correction values, and attaches the blob to CRTC.
> 3. Adds function intel_clrmgr_set_csc: This is a wrapper function
>    which checks the platform type, and calls the valleyview
>    specific set_csc function. As different platforms may have different
>    methods of setting CSC, wrapper function is required to route to proper
>    core CSC set function. In future, the support for other platfroms can be
>    plugged-in here. Adding this function as .set_property CSC color property.
> 4. Adds function vlv_set_csc: core function to program CSC coefficients as per
>    vlv specs, and then enable CSC.
> 
> Signed-off-by: Shashank Sharma <[e-mail]>

I haven't read up on the hardware programming side of this code to give
any comments there, but I got a bit lost trying to follow your blob
upload handling here.  Like Bob noted, it kind of looks like you're
trying to add userspace blob property upload functionality that would
really belong in the DRM core.  However, in the intermediate/long term
there probably isn't really a need for this kind of blob upload support
because the atomic propertysets will provide the functionality you need;
once we have atomic support, I think it would be better to just make
each of these values an independent property and upload all of the
values together as part of a single property set.  But I realize you're
specifically trying to add add this support in a pre-atomic world which
makes things more challenging.  Atomic is definitely coming, but I think
the timeframe is kind of uncertain still, so it's really going to be up
to the upstream maintainers on how to proceed.  Maybe Daniel can give
you some direction?


Matt

> ---
>  drivers/gpu/drm/drm_crtc.c          |   4 +-
>  drivers/gpu/drm/i915/i915_reg.h     |  11 ++
>  drivers/gpu/drm/i915/intel_clrmgr.c | 208 +++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_clrmgr.h |  16 +++
>  drivers/gpu/drm/i915/intel_drv.h    |   1 +
>  include/drm/drm_crtc.h              |   7 ++
>  6 files changed, 244 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 272b66f..be9d110 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -3917,7 +3917,7 @@ done:
>  	return ret;
>  }
>  
> -static struct drm_property_blob *drm_property_create_blob(struct drm_device *dev, int length,
> +struct drm_property_blob *drm_property_create_blob(struct drm_device *dev, int length,
>  							  void *data)
>  {
>  	struct drm_property_blob *blob;
> @@ -3944,7 +3944,7 @@ static struct drm_property_blob *drm_property_create_blob(struct drm_device *dev
>  	return blob;
>  }
>  
> -static void drm_property_destroy_blob(struct drm_device *dev,
> +void drm_property_destroy_blob(struct drm_device *dev,
>  			       struct drm_property_blob *blob)
>  {
>  	drm_mode_object_put(dev, &blob->base);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 20673cc..e3010b3 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6183,6 +6183,17 @@ enum punit_power_well {
>  #define PIPE_CSC_POSTOFF_ME(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_ME, _PIPE_B_CSC_POSTOFF_ME)
>  #define PIPE_CSC_POSTOFF_LO(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_LO, _PIPE_B_CSC_POSTOFF_LO)
>  
> +/* VLV color correction registers */
> +/* CSC */
> +#define PIPECONF_CSC_ENABLE	(1 << 15)
> +#define _PIPEACSC		(dev_priv->info.display_mmio_offset + \
> +								0x600b0)
> +#define _PIPEBCSC		(dev_priv->info.display_mmio_offset + \
> +								0x610b0)
> +#define PIPECSC(pipe)		(_PIPEACSC + (pipe *  CSC_OFFSET))
> +#define CSC_OFFSET			(_PIPEBCSC - _PIPEACSC)
> +#define PIPECSC(pipe)		(_PIPEACSC + (pipe *  CSC_OFFSET))
> +
>  /* VLV MIPI registers */
>  
>  #define _MIPIA_PORT_CTRL			(VLV_DISPLAY_BASE + 0x61190)
> diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c b/drivers/gpu/drm/i915/intel_clrmgr.c
> index ac0a890..36c64c1 100644
> --- a/drivers/gpu/drm/i915/intel_clrmgr.c
> +++ b/drivers/gpu/drm/i915/intel_clrmgr.c
> @@ -32,6 +32,138 @@
>  #include "i915_reg.h"
>  #include "intel_clrmgr.h"
>  
> +/*
> +* Names to register with color properties.
> +* Sequence must be the same as the order
> +* of enum clrmgr_tweaks
> +*/
> +const char *clrmgr_property_names[] = {
> +	/* csc */
> +	"csc-correction",
> +	/* add a new prop name here */
> +};
> +
> +
> +/*
> +* Sizes for color properties. This can differ
> +* platform by platform, hence 'vlv' prefix
> +* The sequence must be same as the order of
> +* enum clrmgr_tweaks
> +*/
> +u32 vlv_color_property_sizes[] = {
> +	VLV_CSC_MATRIX_MAX_VALS,
> +	/* Add new property size here */
> +};

As with the property names, I'm not sure whether having an array here
gives us much clarity.  I think it's fine to just pass
VLV_CSC_MATRIX_MAX_VALS directly to drm_property_create_blob() in the
code below.


> +
> +/* Default CSC values to create property with */
> +uint64_t vlv_csc_default[VLV_CSC_MATRIX_MAX_VALS] = {
> +	0x400, 0, 0, 0, 0x400, 0, 0, 0, 0x400
> +};
> +
> +/*
> +* vlv_set_csc
> +* Valleyview specific csc correction method.
> +* Programs the 6 csc registers with 3x3 correction matrix
> +* values.
> +* inputs:
> +* - intel_crtc*
> +* - color manager registered property for csc correction
> +* - data: pointer to correction values to be applied
> +*/
> +/* Enable color space conversion on PIPE */
> +bool vlv_set_csc(struct intel_crtc *intel_crtc,
> +	struct drm_property *prop, uint64_t values, bool enable)
> +{
> +	u32 count = 0;
> +	u32 c0, c1, c2;
> +	u32 pipeconf, csc_reg, data_size;
> +	uint64_t *blob_data;
> +	uint64_t *data = NULL;
> +	struct drm_device *dev = intel_crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_property_blob *blob = intel_crtc->csc_blob;
> +
> +	/* Sanity */
> +	if (!blob || (blob->length != VLV_CSC_MATRIX_MAX_VALS)) {
> +		DRM_ERROR("Invalid/NULL CSC blob\n");
> +		return false;
> +	}
> +	blob_data = (uint64_t *)blob->data;
> +
> +	/* No need of values to disable property */
> +	if (!enable)
> +		goto configure;
> +
> +	/* Enabling property needs correction values */
> +	data_size = VLV_CSC_MATRIX_MAX_VALS;
> +	data = kmalloc(sizeof(uint64_t) * (data_size), GFP_KERNEL);
> +	if (!data) {
> +		DRM_ERROR("Out of memory\n");
> +		return false;
> +	}
> +
> +	if (copy_from_user((void *)data, (const void __user *)values,
> +			data_size * sizeof(uint64_t))) {
> +		DRM_ERROR("Failed to copy all data\n");
> +		kfree(data);
> +		return false;
> +	}
> +
> +	DRM_DEBUG_DRIVER("Setting CSC on pipe = %d\n", intel_crtc->pipe);
> +	csc_reg = PIPECSC(intel_crtc->pipe);
> +
> +	/* Read CSC matrix, one row at a time */
> +	while (count < VLV_CSC_MATRIX_MAX_VALS) {
> +		c0 = data[count] & VLV_CSC_VALUE_MASK;
> +		*blob_data++ = c0;
> +		c1 = data[count] & VLV_CSC_VALUE_MASK;
> +		*blob_data++ = c1;
> +		c2 = data[count] & VLV_CSC_VALUE_MASK;
> +		*blob_data++ = c2;
> +
> +		/* C0 is LSB 12bits, C1 is MSB 16-27 */
> +		I915_WRITE(csc_reg, (c1 << VLV_CSC_COEFF_SHIFT) | c0);
> +		csc_reg += 4;
> +
> +		/* C2 is LSB 12 bits */
> +		I915_WRITE(csc_reg, c2);
> +		csc_reg += 4;
> +	}
> +
> +configure:
> +
> +	/* Enable / Disable csc correction */
> +	pipeconf = I915_READ(PIPECONF(intel_crtc->pipe));
> +	enable ? (pipeconf |= PIPECONF_CSC_ENABLE) :
> +		(pipeconf &= ~PIPECONF_CSC_ENABLE);
> +	I915_WRITE(PIPECONF(intel_crtc->pipe), pipeconf);
> +	POSTING_READ(PIPECONF(intel_crtc->pipe));
> +	DRM_DEBUG_DRIVER("CSC successfully %s pipe %C\n",
> +		enable ? "enabled" : "disabled", pipe_name(intel_crtc->pipe));
> +
> +	kfree(data);
> +	return true;
> +}
> +
> +bool intel_clrmgr_set_csc(struct drm_crtc *crtc,
> +	struct drm_property *prop, uint64_t values)
> +{
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct drm_device *dev = intel_crtc->base.dev;
> +	bool enable;
> +
> +	/* First value is enable/disable control, others are data */
> +	enable = *((uint64_t *)values);
> +	values += (sizeof(uint64_t));
> +
> +	if (IS_VALLEYVIEW(dev))
> +		return vlv_set_csc(intel_crtc, prop, values, enable);
> +
> +	/* Todo: Support other gen devices */
> +	DRM_ERROR("Color correction is supported only on VLV for now\n");
> +	return false;
> +}
> +
>  void
>  intel_attach_plane_color_correction(struct intel_plane *intel_plane)
>  {
> @@ -41,18 +173,92 @@ intel_attach_plane_color_correction(struct intel_plane *intel_plane)
>  void
>  intel_attach_pipe_color_correction(struct intel_crtc *intel_crtc)
>  {
> +	struct drm_device *dev = intel_crtc->base.dev;
> +	struct drm_mode_object *obj = &intel_crtc->base.base;
> +	struct drm_property *property = NULL;
> +
>  	DRM_DEBUG_DRIVER("\n");
> +	mutex_lock(&dev->mode_config.mutex);
> +
> +	/* color property = csc */
> +	property = dev->mode_config.csc_property;
> +	if (!property) {
> +		DRM_ERROR("No such property to attach\n");
> +		goto release_mutex;
> +	}
> +
> +	/* create blob for correction values */
> +	intel_crtc->csc_blob = drm_property_create_blob(dev,
> +		vlv_color_property_sizes[csc], (void *)vlv_csc_default);
> +	if (!intel_crtc->csc_blob) {
> +		DRM_ERROR("Failed to create property blob\n");
> +		goto release_mutex;
> +	}
> +
> +	/* Attach blob with property */
> +	if (drm_object_property_set_value(obj, property,
> +			intel_crtc->csc_blob->base.id)) {
> +		DRM_ERROR("Failed to attach property blob, destroying\n");
> +		drm_property_destroy_blob(dev, intel_crtc->csc_blob);
> +		goto release_mutex;
> +	}
> +
> +	DRM_DEBUG_DRIVER("Successfully attached CSC property\n");
> +
> +release_mutex:
> +	mutex_unlock(&dev->mode_config.mutex);
>  }
>  
>  int intel_clrmgr_create_color_properties(struct drm_device *dev)
>  {
> +	int ret = 0;
> +	struct drm_property *property;
> +
>  	DRM_DEBUG_DRIVER("\n");
> -	return 0;
> +	mutex_lock(&dev->mode_config.mutex);
> +
> +	/* CSC correction color property, blob type, size 0 */
> +	property = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> +		clrmgr_property_names[csc], 0);
> +	if (!property) {
> +		DRM_ERROR("Failed to create property(CSC)\n");
> +		ret++;
> +	} else {
> +		dev->mode_config.csc_property = property;
> +		DRM_DEBUG_DRIVER("Created property: CSC\n");
> +	}
> +
> +	mutex_unlock(&dev->mode_config.mutex);
> +	return ret;
>  }
>  
>  void intel_clrmgr_destroy_color_properties(struct drm_device *dev)
>  {
> +	struct drm_crtc *crtc;
> +	struct intel_crtc *intel_crtc;
> +
>  	DRM_DEBUG_DRIVER("\n");
> +
> +	mutex_lock(&dev->mode_config.mutex);
> +
> +	/* CSC correction color property */
> +	if (dev->mode_config.csc_property) {
> +		drm_property_destroy(dev, dev->mode_config.csc_property);
> +		dev->mode_config.csc_property = NULL;
> +		DRM_DEBUG_DRIVER("Destroyed property: CSC\n");
> +	}
> +
> +	/* Destroy property blob from each CRTC */
> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> +		intel_crtc = to_intel_crtc(crtc);
> +		if (intel_crtc->csc_blob) {
> +			drm_property_destroy_blob(dev, intel_crtc->csc_blob);
> +			intel_crtc->csc_blob = NULL;
> +		}
> +	}
> +
> +	mutex_unlock(&dev->mode_config.mutex);
> +	DRM_DEBUG_DRIVER("Successfully destroyed all color properties\n");
>  }
>  
>  void intel_clrmgr_init(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_clrmgr.h b/drivers/gpu/drm/i915/intel_clrmgr.h
> index 8cff487..5725d6b 100644
> --- a/drivers/gpu/drm/i915/intel_clrmgr.h
> +++ b/drivers/gpu/drm/i915/intel_clrmgr.h
> @@ -39,6 +39,13 @@
>  #define CLRMGR_PROP_NAME_MAX				128
>  #define CLRMGR_PROP_RANGE_MAX				0xFFFFFFFFFFFFFFFF
>  
> +/* CSC */
> + /* CSC / Wide gamut */
> +#define VLV_CSC_MATRIX_MAX_VALS		9
> +#define VLV_CSC_VALUE_MASK			0xFFF
> +#define VLV_CSC_COEFF_SHIFT			16
> +
> +
>  /* Properties */
>  enum clrmgr_tweaks {
>  	csc = 0,
> @@ -50,6 +57,15 @@ enum clrmgr_tweaks {
>  };
>  
>  /*
> +* intel_clrmgr_set_csc
> +* CSC correction method is different across various
> +* gen devices. This wrapper function calls the respective
> +* platform specific function to set CSC
> +*/
> +bool intel_clrmgr_set_csc(struct drm_crtc *crtc,
> +	struct drm_property *prop, uint64_t data);
> +
> +/*
>  * intel_attach_plane_color_correction:
>  * Attach plane level color correction DRM properties to
>  * corresponding plane objects.
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 7ba5785..a10b9bb 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -437,6 +437,7 @@ struct intel_crtc {
>  
>  	int scanline_offset;
>  	struct intel_mmio_flip mmio_flip;
> +	struct drm_property_blob *csc_blob;
>  };
>  
>  struct intel_plane_wm_parameters {
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 31344bf..487ce44 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -851,6 +851,9 @@ struct drm_mode_config {
>  	struct drm_property *aspect_ratio_property;
>  	struct drm_property *dirty_info_property;
>  
> +	/* Color correction properties */
> +	struct drm_property *csc_property;
> +
>  	/* dumb ioctl parameters */
>  	uint32_t preferred_depth, prefer_shadow;
>  
> @@ -981,6 +984,10 @@ extern int drm_mode_connector_set_path_property(struct drm_connector *connector,
>  						char *path);
>  extern int drm_mode_connector_update_edid_property(struct drm_connector *connector,
>  						struct edid *edid);
> +extern struct drm_property_blob *drm_property_create_blob(struct drm_device *dev,
> +						int length, void *data);
> +extern void drm_property_destroy_blob(struct drm_device *dev,
> +			       struct drm_property_blob *blob);
>  
>  static inline bool drm_property_type_is(struct drm_property *property,
>  		uint32_t type)
> -- 
> 1.9.1
>

Daniel Vetter Sept. 10, 2014, 6:40 a.m. UTC | #3

On Tue, Sep 09, 2014 at 06:30:09PM -0700, Matt Roper wrote:
> On Tue, Sep 09, 2014 at 11:53:15AM +0530, [e-mail] wrote:
> > From: Shashank Sharma <[e-mail]>
> > 
> > This patch adds support for CSC correction color property.
> > It does the following:
> > 1. Creates a new DRM property for CSC correction. Adds this into
> >    mode_config.
> > 2. Attaches this CSC property to calling CRTC. Creates a blob
> >    to store the correction values, and attaches the blob to CRTC.
> > 3. Adds function intel_clrmgr_set_csc: This is a wrapper function
> >    which checks the platform type, and calls the valleyview
> >    specific set_csc function. As different platforms may have different
> >    methods of setting CSC, wrapper function is required to route to proper
> >    core CSC set function. In future, the support for other platfroms can be
> >    plugged-in here. Adding this function as .set_property CSC color property.
> > 4. Adds function vlv_set_csc: core function to program CSC coefficients as per
> >    vlv specs, and then enable CSC.
> > 
> > Signed-off-by: Shashank Sharma <[e-mail]>
> 
> I haven't read up on the hardware programming side of this code to give
> any comments there, but I got a bit lost trying to follow your blob
> upload handling here.  Like Bob noted, it kind of looks like you're
> trying to add userspace blob property upload functionality that would
> really belong in the DRM core.  However, in the intermediate/long term
> there probably isn't really a need for this kind of blob upload support
> because the atomic propertysets will provide the functionality you need;
> once we have atomic support, I think it would be better to just make
> each of these values an independent property and upload all of the
> values together as part of a single property set.  But I realize you're
> specifically trying to add add this support in a pre-atomic world which
> makes things more challenging.  Atomic is definitely coming, but I think
> the timeframe is kind of uncertain still, so it's really going to be up
> to the upstream maintainers on how to proceed.  Maybe Daniel can give
> you some direction?

I've thought the csc stuff here is just a matrix of register values, and
highly intel specific at that. So might as well keep it as a blob property
for now until either the specific layout changes or some standard for
generic csc emerges.

Overall I still think there's a bit too much indirection - I don't see why
the color manager needs to sit in a separate file with separate
registration functions. Doing it like that rips apart the per-crtc
setup/teardown quite a lot and isn't how properties are handled anywhere
else.
-Daniel

> 
> 
> Matt
> 
> > ---
> >  drivers/gpu/drm/drm_crtc.c          |   4 +-
> >  drivers/gpu/drm/i915/i915_reg.h     |  11 ++
> >  drivers/gpu/drm/i915/intel_clrmgr.c | 208 +++++++++++++++++++++++++++++++++++-
> >  drivers/gpu/drm/i915/intel_clrmgr.h |  16 +++
> >  drivers/gpu/drm/i915/intel_drv.h    |   1 +
> >  include/drm/drm_crtc.h              |   7 ++
> >  6 files changed, 244 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index 272b66f..be9d110 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -3917,7 +3917,7 @@ done:
> >  	return ret;
> >  }
> >  
> > -static struct drm_property_blob *drm_property_create_blob(struct drm_device *dev, int length,
> > +struct drm_property_blob *drm_property_create_blob(struct drm_device *dev, int length,
> >  							  void *data)
> >  {
> >  	struct drm_property_blob *blob;
> > @@ -3944,7 +3944,7 @@ static struct drm_property_blob *drm_property_create_blob(struct drm_device *dev
> >  	return blob;
> >  }
> >  
> > -static void drm_property_destroy_blob(struct drm_device *dev,
> > +void drm_property_destroy_blob(struct drm_device *dev,
> >  			       struct drm_property_blob *blob)
> >  {
> >  	drm_mode_object_put(dev, &blob->base);
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 20673cc..e3010b3 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6183,6 +6183,17 @@ enum punit_power_well {
> >  #define PIPE_CSC_POSTOFF_ME(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_ME, _PIPE_B_CSC_POSTOFF_ME)
> >  #define PIPE_CSC_POSTOFF_LO(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_LO, _PIPE_B_CSC_POSTOFF_LO)
> >  
> > +/* VLV color correction registers */
> > +/* CSC */
> > +#define PIPECONF_CSC_ENABLE	(1 << 15)
> > +#define _PIPEACSC		(dev_priv->info.display_mmio_offset + \
> > +								0x600b0)
> > +#define _PIPEBCSC		(dev_priv->info.display_mmio_offset + \
> > +								0x610b0)
> > +#define PIPECSC(pipe)		(_PIPEACSC + (pipe *  CSC_OFFSET))
> > +#define CSC_OFFSET			(_PIPEBCSC - _PIPEACSC)
> > +#define PIPECSC(pipe)		(_PIPEACSC + (pipe *  CSC_OFFSET))
> > +
> >  /* VLV MIPI registers */
> >  
> >  #define _MIPIA_PORT_CTRL			(VLV_DISPLAY_BASE + 0x61190)
> > diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c b/drivers/gpu/drm/i915/intel_clrmgr.c
> > index ac0a890..36c64c1 100644
> > --- a/drivers/gpu/drm/i915/intel_clrmgr.c
> > +++ b/drivers/gpu/drm/i915/intel_clrmgr.c
> > @@ -32,6 +32,138 @@
> >  #include "i915_reg.h"
> >  #include "intel_clrmgr.h"
> >  
> > +/*
> > +* Names to register with color properties.
> > +* Sequence must be the same as the order
> > +* of enum clrmgr_tweaks
> > +*/
> > +const char *clrmgr_property_names[] = {
> > +	/* csc */
> > +	"csc-correction",
> > +	/* add a new prop name here */
> > +};
> > +
> > +
> > +/*
> > +* Sizes for color properties. This can differ
> > +* platform by platform, hence 'vlv' prefix
> > +* The sequence must be same as the order of
> > +* enum clrmgr_tweaks
> > +*/
> > +u32 vlv_color_property_sizes[] = {
> > +	VLV_CSC_MATRIX_MAX_VALS,
> > +	/* Add new property size here */
> > +};
> 
> As with the property names, I'm not sure whether having an array here
> gives us much clarity.  I think it's fine to just pass
> VLV_CSC_MATRIX_MAX_VALS directly to drm_property_create_blob() in the
> code below.
> 
> 
> > +
> > +/* Default CSC values to create property with */
> > +uint64_t vlv_csc_default[VLV_CSC_MATRIX_MAX_VALS] = {
> > +	0x400, 0, 0, 0, 0x400, 0, 0, 0, 0x400
> > +};
> > +
> > +/*
> > +* vlv_set_csc
> > +* Valleyview specific csc correction method.
> > +* Programs the 6 csc registers with 3x3 correction matrix
> > +* values.
> > +* inputs:
> > +* - intel_crtc*
> > +* - color manager registered property for csc correction
> > +* - data: pointer to correction values to be applied
> > +*/
> > +/* Enable color space conversion on PIPE */
> > +bool vlv_set_csc(struct intel_crtc *intel_crtc,
> > +	struct drm_property *prop, uint64_t values, bool enable)
> > +{
> > +	u32 count = 0;
> > +	u32 c0, c1, c2;
> > +	u32 pipeconf, csc_reg, data_size;
> > +	uint64_t *blob_data;
> > +	uint64_t *data = NULL;
> > +	struct drm_device *dev = intel_crtc->base.dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct drm_property_blob *blob = intel_crtc->csc_blob;
> > +
> > +	/* Sanity */
> > +	if (!blob || (blob->length != VLV_CSC_MATRIX_MAX_VALS)) {
> > +		DRM_ERROR("Invalid/NULL CSC blob\n");
> > +		return false;
> > +	}
> > +	blob_data = (uint64_t *)blob->data;
> > +
> > +	/* No need of values to disable property */
> > +	if (!enable)
> > +		goto configure;
> > +
> > +	/* Enabling property needs correction values */
> > +	data_size = VLV_CSC_MATRIX_MAX_VALS;
> > +	data = kmalloc(sizeof(uint64_t) * (data_size), GFP_KERNEL);
> > +	if (!data) {
> > +		DRM_ERROR("Out of memory\n");
> > +		return false;
> > +	}
> > +
> > +	if (copy_from_user((void *)data, (const void __user *)values,
> > +			data_size * sizeof(uint64_t))) {
> > +		DRM_ERROR("Failed to copy all data\n");
> > +		kfree(data);
> > +		return false;
> > +	}
> > +
> > +	DRM_DEBUG_DRIVER("Setting CSC on pipe = %d\n", intel_crtc->pipe);
> > +	csc_reg = PIPECSC(intel_crtc->pipe);
> > +
> > +	/* Read CSC matrix, one row at a time */
> > +	while (count < VLV_CSC_MATRIX_MAX_VALS) {
> > +		c0 = data[count] & VLV_CSC_VALUE_MASK;
> > +		*blob_data++ = c0;
> > +		c1 = data[count] & VLV_CSC_VALUE_MASK;
> > +		*blob_data++ = c1;
> > +		c2 = data[count] & VLV_CSC_VALUE_MASK;
> > +		*blob_data++ = c2;
> > +
> > +		/* C0 is LSB 12bits, C1 is MSB 16-27 */
> > +		I915_WRITE(csc_reg, (c1 << VLV_CSC_COEFF_SHIFT) | c0);
> > +		csc_reg += 4;
> > +
> > +		/* C2 is LSB 12 bits */
> > +		I915_WRITE(csc_reg, c2);
> > +		csc_reg += 4;
> > +	}
> > +
> > +configure:
> > +
> > +	/* Enable / Disable csc correction */
> > +	pipeconf = I915_READ(PIPECONF(intel_crtc->pipe));
> > +	enable ? (pipeconf |= PIPECONF_CSC_ENABLE) :
> > +		(pipeconf &= ~PIPECONF_CSC_ENABLE);
> > +	I915_WRITE(PIPECONF(intel_crtc->pipe), pipeconf);
> > +	POSTING_READ(PIPECONF(intel_crtc->pipe));
> > +	DRM_DEBUG_DRIVER("CSC successfully %s pipe %C\n",
> > +		enable ? "enabled" : "disabled", pipe_name(intel_crtc->pipe));
> > +
> > +	kfree(data);
> > +	return true;
> > +}
> > +
> > +bool intel_clrmgr_set_csc(struct drm_crtc *crtc,
> > +	struct drm_property *prop, uint64_t values)
> > +{
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +	struct drm_device *dev = intel_crtc->base.dev;
> > +	bool enable;
> > +
> > +	/* First value is enable/disable control, others are data */
> > +	enable = *((uint64_t *)values);
> > +	values += (sizeof(uint64_t));
> > +
> > +	if (IS_VALLEYVIEW(dev))
> > +		return vlv_set_csc(intel_crtc, prop, values, enable);
> > +
> > +	/* Todo: Support other gen devices */
> > +	DRM_ERROR("Color correction is supported only on VLV for now\n");
> > +	return false;
> > +}
> > +
> >  void
> >  intel_attach_plane_color_correction(struct intel_plane *intel_plane)
> >  {
> > @@ -41,18 +173,92 @@ intel_attach_plane_color_correction(struct intel_plane *intel_plane)
> >  void
> >  intel_attach_pipe_color_correction(struct intel_crtc *intel_crtc)
> >  {
> > +	struct drm_device *dev = intel_crtc->base.dev;
> > +	struct drm_mode_object *obj = &intel_crtc->base.base;
> > +	struct drm_property *property = NULL;
> > +
> >  	DRM_DEBUG_DRIVER("\n");
> > +	mutex_lock(&dev->mode_config.mutex);
> > +
> > +	/* color property = csc */
> > +	property = dev->mode_config.csc_property;
> > +	if (!property) {
> > +		DRM_ERROR("No such property to attach\n");
> > +		goto release_mutex;
> > +	}
> > +
> > +	/* create blob for correction values */
> > +	intel_crtc->csc_blob = drm_property_create_blob(dev,
> > +		vlv_color_property_sizes[csc], (void *)vlv_csc_default);
> > +	if (!intel_crtc->csc_blob) {
> > +		DRM_ERROR("Failed to create property blob\n");
> > +		goto release_mutex;
> > +	}
> > +
> > +	/* Attach blob with property */
> > +	if (drm_object_property_set_value(obj, property,
> > +			intel_crtc->csc_blob->base.id)) {
> > +		DRM_ERROR("Failed to attach property blob, destroying\n");
> > +		drm_property_destroy_blob(dev, intel_crtc->csc_blob);
> > +		goto release_mutex;
> > +	}
> > +
> > +	DRM_DEBUG_DRIVER("Successfully attached CSC property\n");
> > +
> > +release_mutex:
> > +	mutex_unlock(&dev->mode_config.mutex);
> >  }
> >  
> >  int intel_clrmgr_create_color_properties(struct drm_device *dev)
> >  {
> > +	int ret = 0;
> > +	struct drm_property *property;
> > +
> >  	DRM_DEBUG_DRIVER("\n");
> > -	return 0;
> > +	mutex_lock(&dev->mode_config.mutex);
> > +
> > +	/* CSC correction color property, blob type, size 0 */
> > +	property = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> > +		clrmgr_property_names[csc], 0);
> > +	if (!property) {
> > +		DRM_ERROR("Failed to create property(CSC)\n");
> > +		ret++;
> > +	} else {
> > +		dev->mode_config.csc_property = property;
> > +		DRM_DEBUG_DRIVER("Created property: CSC\n");
> > +	}
> > +
> > +	mutex_unlock(&dev->mode_config.mutex);
> > +	return ret;
> >  }
> >  
> >  void intel_clrmgr_destroy_color_properties(struct drm_device *dev)
> >  {
> > +	struct drm_crtc *crtc;
> > +	struct intel_crtc *intel_crtc;
> > +
> >  	DRM_DEBUG_DRIVER("\n");
> > +
> > +	mutex_lock(&dev->mode_config.mutex);
> > +
> > +	/* CSC correction color property */
> > +	if (dev->mode_config.csc_property) {
> > +		drm_property_destroy(dev, dev->mode_config.csc_property);
> > +		dev->mode_config.csc_property = NULL;
> > +		DRM_DEBUG_DRIVER("Destroyed property: CSC\n");
> > +	}
> > +
> > +	/* Destroy property blob from each CRTC */
> > +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> > +		intel_crtc = to_intel_crtc(crtc);
> > +		if (intel_crtc->csc_blob) {
> > +			drm_property_destroy_blob(dev, intel_crtc->csc_blob);
> > +			intel_crtc->csc_blob = NULL;
> > +		}
> > +	}
> > +
> > +	mutex_unlock(&dev->mode_config.mutex);
> > +	DRM_DEBUG_DRIVER("Successfully destroyed all color properties\n");
> >  }
> >  
> >  void intel_clrmgr_init(struct drm_device *dev)
> > diff --git a/drivers/gpu/drm/i915/intel_clrmgr.h b/drivers/gpu/drm/i915/intel_clrmgr.h
> > index 8cff487..5725d6b 100644
> > --- a/drivers/gpu/drm/i915/intel_clrmgr.h
> > +++ b/drivers/gpu/drm/i915/intel_clrmgr.h
> > @@ -39,6 +39,13 @@
> >  #define CLRMGR_PROP_NAME_MAX				128
> >  #define CLRMGR_PROP_RANGE_MAX				0xFFFFFFFFFFFFFFFF
> >  
> > +/* CSC */
> > + /* CSC / Wide gamut */
> > +#define VLV_CSC_MATRIX_MAX_VALS		9
> > +#define VLV_CSC_VALUE_MASK			0xFFF
> > +#define VLV_CSC_COEFF_SHIFT			16
> > +
> > +
> >  /* Properties */
> >  enum clrmgr_tweaks {
> >  	csc = 0,
> > @@ -50,6 +57,15 @@ enum clrmgr_tweaks {
> >  };
> >  
> >  /*
> > +* intel_clrmgr_set_csc
> > +* CSC correction method is different across various
> > +* gen devices. This wrapper function calls the respective
> > +* platform specific function to set CSC
> > +*/
> > +bool intel_clrmgr_set_csc(struct drm_crtc *crtc,
> > +	struct drm_property *prop, uint64_t data);
> > +
> > +/*
> >  * intel_attach_plane_color_correction:
> >  * Attach plane level color correction DRM properties to
> >  * corresponding plane objects.
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 7ba5785..a10b9bb 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -437,6 +437,7 @@ struct intel_crtc {
> >  
> >  	int scanline_offset;
> >  	struct intel_mmio_flip mmio_flip;
> > +	struct drm_property_blob *csc_blob;
> >  };
> >  
> >  struct intel_plane_wm_parameters {
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index 31344bf..487ce44 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -851,6 +851,9 @@ struct drm_mode_config {
> >  	struct drm_property *aspect_ratio_property;
> >  	struct drm_property *dirty_info_property;
> >  
> > +	/* Color correction properties */
> > +	struct drm_property *csc_property;
> > +
> >  	/* dumb ioctl parameters */
> >  	uint32_t preferred_depth, prefer_shadow;
> >  
> > @@ -981,6 +984,10 @@ extern int drm_mode_connector_set_path_property(struct drm_connector *connector,
> >  						char *path);
> >  extern int drm_mode_connector_update_edid_property(struct drm_connector *connector,
> >  						struct edid *edid);
> > +extern struct drm_property_blob *drm_property_create_blob(struct drm_device *dev,
> > +						int length, void *data);
> > +extern void drm_property_destroy_blob(struct drm_device *dev,
> > +			       struct drm_property_blob *blob);
> >  
> >  static inline bool drm_property_type_is(struct drm_property *property,
> >  		uint32_t type)
> > -- 
> > 1.9.1
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> [phone]
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Sharma, Shashank Sept. 10, 2014, 8:55 a.m. UTC | #4

Thanks for your time and review.
Please find my comments inline.

Regards
Shashank

On 9/10/2014 4:21 AM, Bob Paauwe wrote:
> On Tue, 9 Sep 2014 11:53:15 +0530
> <[e-mail]> wrote:
>
>> From: Shashank Sharma <[e-mail]>
>>
>> This patch adds support for CSC correction color property.
>> It does the following:
>> 1. Creates a new DRM property for CSC correction. Adds this into
>>     mode_config.
>> 2. Attaches this CSC property to calling CRTC. Creates a blob
>>     to store the correction values, and attaches the blob to CRTC.
>> 3. Adds function intel_clrmgr_set_csc: This is a wrapper function
>>     which checks the platform type, and calls the valleyview
>>     specific set_csc function. As different platforms may have different
>>     methods of setting CSC, wrapper function is required to route to proper
>>     core CSC set function. In future, the support for other platfroms can be
>>     plugged-in here. Adding this function as .set_property CSC color property.
>> 4. Adds function vlv_set_csc: core function to program CSC coefficients as per
>>     vlv specs, and then enable CSC.
>>
>> Signed-off-by: Shashank Sharma <[e-mail]>
>> ---
>>   drivers/gpu/drm/drm_crtc.c          |   4 +-
>>   drivers/gpu/drm/i915/i915_reg.h     |  11 ++
>>   drivers/gpu/drm/i915/intel_clrmgr.c | 208 +++++++++++++++++++++++++++++++++++-
>>   drivers/gpu/drm/i915/intel_clrmgr.h |  16 +++
>>   drivers/gpu/drm/i915/intel_drv.h    |   1 +
>>   include/drm/drm_crtc.h              |   7 ++
>>   6 files changed, 244 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index 272b66f..be9d110 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -3917,7 +3917,7 @@ done:
>>   	return ret;
>>   }
>>
>> -static struct drm_property_blob *drm_property_create_blob(struct drm_device *dev, int length,
>> +struct drm_property_blob *drm_property_create_blob(struct drm_device *dev, int length,
>>   							  void *data)
>>   {
>>   	struct drm_property_blob *blob;
>> @@ -3944,7 +3944,7 @@ static struct drm_property_blob *drm_property_create_blob(struct drm_device *dev
>>   	return blob;
>>   }
>>
>> -static void drm_property_destroy_blob(struct drm_device *dev,
>> +void drm_property_destroy_blob(struct drm_device *dev,
>>   			       struct drm_property_blob *blob)
>>   {
>>   	drm_mode_object_put(dev, &blob->base);
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 20673cc..e3010b3 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -6183,6 +6183,17 @@ enum punit_power_well {
>>   #define PIPE_CSC_POSTOFF_ME(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_ME, _PIPE_B_CSC_POSTOFF_ME)
>>   #define PIPE_CSC_POSTOFF_LO(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_LO, _PIPE_B_CSC_POSTOFF_LO)
>>
>> +/* VLV color correction registers */
>> +/* CSC */
>> +#define PIPECONF_CSC_ENABLE	(1 << 15)
>> +#define _PIPEACSC		(dev_priv->info.display_mmio_offset + \
>> +								0x600b0)
>> +#define _PIPEBCSC		(dev_priv->info.display_mmio_offset + \
>> +								0x610b0)
>> +#define PIPECSC(pipe)		(_PIPEACSC + (pipe *  CSC_OFFSET))
>> +#define CSC_OFFSET			(_PIPEBCSC - _PIPEACSC)
>> +#define PIPECSC(pipe)		(_PIPEACSC + (pipe *  CSC_OFFSET))
>> +
>>   /* VLV MIPI registers */
>>
>>   #define _MIPIA_PORT_CTRL			(VLV_DISPLAY_BASE + 0x61190)
>> diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c b/drivers/gpu/drm/i915/intel_clrmgr.c
>> index ac0a890..36c64c1 100644
>> --- a/drivers/gpu/drm/i915/intel_clrmgr.c
>> +++ b/drivers/gpu/drm/i915/intel_clrmgr.c
>> @@ -32,6 +32,138 @@
>>   #include "i915_reg.h"
>>   #include "intel_clrmgr.h"
>>
>> +/*
>> +* Names to register with color properties.
>> +* Sequence must be the same as the order
>> +* of enum clrmgr_tweaks
>> +*/
>> +const char *clrmgr_property_names[] = {
>> +	/* csc */
>> +	"csc-correction",
>> +	/* add a new prop name here */
>> +};
>> +
>> +
>> +/*
>> +* Sizes for color properties. This can differ
>> +* platform by platform, hence 'vlv' prefix
>> +* The sequence must be same as the order of
>> +* enum clrmgr_tweaks
>> +*/
>> +u32 vlv_color_property_sizes[] = {
>> +	VLV_CSC_MATRIX_MAX_VALS,
>> +	/* Add new property size here */
>> +};
>> +
>> +/* Default CSC values to create property with */
>> +uint64_t vlv_csc_default[VLV_CSC_MATRIX_MAX_VALS] = {
>> +	0x400, 0, 0, 0, 0x400, 0, 0, 0, 0x400
>> +};
>> +
>> +/*
>> +* vlv_set_csc
>> +* Valleyview specific csc correction method.
>> +* Programs the 6 csc registers with 3x3 correction matrix
>> +* values.
>> +* inputs:
>> +* - intel_crtc*
>> +* - color manager registered property for csc correction
>> +* - data: pointer to correction values to be applied
>
> The comment isn't matching the function. data is not a pointer, it's a
> single 64bit value.  Also, the boolean enable isn't described in the
> comment block.
>
Yes,Thanks for pointing out. Missed updating comments after design 
change. I will fix this.

>> +*/
>> +/* Enable color space conversion on PIPE */
>> +bool vlv_set_csc(struct intel_crtc *intel_crtc,
>> +	struct drm_property *prop, uint64_t values, bool enable)
>> +{
>> +	u32 count = 0;
>> +	u32 c0, c1, c2;
>> +	u32 pipeconf, csc_reg, data_size;
>> +	uint64_t *blob_data;
>> +	uint64_t *data = NULL;
>> +	struct drm_device *dev = intel_crtc->base.dev;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	struct drm_property_blob *blob = intel_crtc->csc_blob;
>> +
>> +	/* Sanity */
>> +	if (!blob || (blob->length != VLV_CSC_MATRIX_MAX_VALS)) {
>> +		DRM_ERROR("Invalid/NULL CSC blob\n");
>> +		return false;
>> +	}
>> +	blob_data = (uint64_t *)blob->data;
>> +
>> +	/* No need of values to disable property */
>> +	if (!enable)
>> +		goto configure;
>> +
>> +	/* Enabling property needs correction values */
>> +	data_size = VLV_CSC_MATRIX_MAX_VALS;
>> +	data = kmalloc(sizeof(uint64_t) * (data_size), GFP_KERNEL);
>> +	if (!data) {
>> +		DRM_ERROR("Out of memory\n");
>> +		return false;
>> +	}
>> +
>> +	if (copy_from_user((void *)data, (const void __user *)values,
>> +			data_size * sizeof(uint64_t))) {
>> +		DRM_ERROR("Failed to copy all data\n");
>> +		kfree(data);
>> +		return false;
>> +	}
>
> I don't think this should be doing a copy_from_user.  It should be the
> drm_property code that handles the IOCTL that does that.
>
> The whole handling of the blob property looks suspect to me.  I believe
> that the DRM code currently doesn't allow blob type properties to be
> set. So you should first have a patch that adds that capability to the
> DRM property code.  But I'm not sure that's even the right way to
> handle this.
>
Humm. This adds another dilemma of how to do this.
I believe blob property type is the only one which suits the CSC 
property, (also gamma correction, with 129 correction values)

Even if we create a set_blob, that will be again doing a 
copy_from_user() only, won't it ?

I can add a patch to do a set_blob in the next patch set. Please have a 
look and let me know if that's what you expect.
>> +
>> +	DRM_DEBUG_DRIVER("Setting CSC on pipe = %d\n", intel_crtc->pipe);
>> +	csc_reg = PIPECSC(intel_crtc->pipe);
>> +
>> +	/* Read CSC matrix, one row at a time */
>> +	while (count < VLV_CSC_MATRIX_MAX_VALS) {
>> +		c0 = data[count] & VLV_CSC_VALUE_MASK;
>> +		*blob_data++ = c0;
>> +		c1 = data[count] & VLV_CSC_VALUE_MASK;
>> +		*blob_data++ = c1;
>> +		c2 = data[count] & VLV_CSC_VALUE_MASK;
>> +		*blob_data++ = c2;
>
> You aren't incrementing count after each assignment above, that means
> that c0, c1, and c2 are all getting set to the same value. That doesn't
> seem right.
>

I am sorry, this is bad. While splitting the patches, I messed here, and 
looks like the unit CSC values were getting applied properly, so dint 
catch this during ULT.
Thanks for pointing this out, will fix this.
>> +
>> +		/* C0 is LSB 12bits, C1 is MSB 16-27 */
>> +		I915_WRITE(csc_reg, (c1 << VLV_CSC_COEFF_SHIFT) | c0);
>> +		csc_reg += 4;
>> +
>> +		/* C2 is LSB 12 bits */
>> +		I915_WRITE(csc_reg, c2);
>> +		csc_reg += 4;
>> +	}
>> +
>> +configure:
>> +
>> +	/* Enable / Disable csc correction */
>> +	pipeconf = I915_READ(PIPECONF(intel_crtc->pipe));
>> +	enable ? (pipeconf |= PIPECONF_CSC_ENABLE) :
>> +		(pipeconf &= ~PIPECONF_CSC_ENABLE);
>> +	I915_WRITE(PIPECONF(intel_crtc->pipe), pipeconf);
>> +	POSTING_READ(PIPECONF(intel_crtc->pipe));
>> +	DRM_DEBUG_DRIVER("CSC successfully %s pipe %C\n",
>> +		enable ? "enabled" : "disabled", pipe_name(intel_crtc->pipe));
>> +
>> +	kfree(data);
>> +	return true;
>> +}
>> +
>> +bool intel_clrmgr_set_csc(struct drm_crtc *crtc,
>> +	struct drm_property *prop, uint64_t values)
>> +{
>> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> +	struct drm_device *dev = intel_crtc->base.dev;
>> +	bool enable;
>> +
>> +	/* First value is enable/disable control, others are data */
>> +	enable = *((uint64_t *)values);
>> +	values += (sizeof(uint64_t));
>
> It looks like you're trying to pass in a user space pointer to some
> type of csc structure.  As above, I'm not sure that's the right way to
> approach this. Ideally, I think we want to use the standard drm
> property types or you're going to have to define a new drm property
> type that could be universally used.
>
Actually, I need to first decide, if we need to enable / disable the 
property. Once we are sure that user space wants to enable it, then I 
need correction coefficients. So I decided to have first byte as 
enable/disable control, and others as correction values.

I dont see any other way of doing it, while coming from drm_property 
interface.
Do you think that we can create a custom/new property type for this 
reason ?
>> +
>> +	if (IS_VALLEYVIEW(dev))
>> +		return vlv_set_csc(intel_crtc, prop, values, enable);
>> +
>> +	/* Todo: Support other gen devices */
>> +	DRM_ERROR("Color correction is supported only on VLV for now\n");
>> +	return false;
>> +}
>> +
>>   void
>>   intel_attach_plane_color_correction(struct intel_plane *intel_plane)
>>   {
>> @@ -41,18 +173,92 @@ intel_attach_plane_color_correction(struct intel_plane *intel_plane)
>>   void
>>   intel_attach_pipe_color_correction(struct intel_crtc *intel_crtc)
>>   {
>> +	struct drm_device *dev = intel_crtc->base.dev;
>> +	struct drm_mode_object *obj = &intel_crtc->base.base;
>> +	struct drm_property *property = NULL;
>> +
>>   	DRM_DEBUG_DRIVER("\n");
>> +	mutex_lock(&dev->mode_config.mutex);
>> +
>> +	/* color property = csc */
>> +	property = dev->mode_config.csc_property;
>> +	if (!property) {
>> +		DRM_ERROR("No such property to attach\n");
>> +		goto release_mutex;
>> +	}
>> +
>> +	/* create blob for correction values */
>> +	intel_crtc->csc_blob = drm_property_create_blob(dev,
>> +		vlv_color_property_sizes[csc], (void *)vlv_csc_default);
>> +	if (!intel_crtc->csc_blob) {
>> +		DRM_ERROR("Failed to create property blob\n");
>> +		goto release_mutex;
>> +	}
>> +
>> +	/* Attach blob with property */
>> +	if (drm_object_property_set_value(obj, property,
>> +			intel_crtc->csc_blob->base.id)) {
>> +		DRM_ERROR("Failed to attach property blob, destroying\n");
>> +		drm_property_destroy_blob(dev, intel_crtc->csc_blob);
>> +		goto release_mutex;
>> +	}
>> +
>> +	DRM_DEBUG_DRIVER("Successfully attached CSC property\n");
>> +
>> +release_mutex:
>> +	mutex_unlock(&dev->mode_config.mutex);
>>   }
>>
>>   int intel_clrmgr_create_color_properties(struct drm_device *dev)
>>   {
>> +	int ret = 0;
>> +	struct drm_property *property;
>> +
>>   	DRM_DEBUG_DRIVER("\n");
>> -	return 0;
>> +	mutex_lock(&dev->mode_config.mutex);
>> +
>> +	/* CSC correction color property, blob type, size 0 */
>> +	property = drm_property_create(dev, DRM_MODE_PROP_BLOB,
>> +		clrmgr_property_names[csc], 0);
>> +	if (!property) {
>> +		DRM_ERROR("Failed to create property(CSC)\n");
>> +		ret++;
>> +	} else {
>> +		dev->mode_config.csc_property = property;
>> +		DRM_DEBUG_DRIVER("Created property: CSC\n");
>> +	}
>> +
>> +	mutex_unlock(&dev->mode_config.mutex);
>> +	return ret;
>>   }
>>
>>   void intel_clrmgr_destroy_color_properties(struct drm_device *dev)
>>   {
>> +	struct drm_crtc *crtc;
>> +	struct intel_crtc *intel_crtc;
>> +
>>   	DRM_DEBUG_DRIVER("\n");
>> +
>> +	mutex_lock(&dev->mode_config.mutex);
>> +
>> +	/* CSC correction color property */
>> +	if (dev->mode_config.csc_property) {
>> +		drm_property_destroy(dev, dev->mode_config.csc_property);
>> +		dev->mode_config.csc_property = NULL;
>> +		DRM_DEBUG_DRIVER("Destroyed property: CSC\n");
>> +	}
>> +
>> +	/* Destroy property blob from each CRTC */
>> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
>> +		intel_crtc = to_intel_crtc(crtc);
>> +		if (intel_crtc->csc_blob) {
>> +			drm_property_destroy_blob(dev, intel_crtc->csc_blob);
>> +			intel_crtc->csc_blob = NULL;
>> +		}
>> +	}
>> +
>> +	mutex_unlock(&dev->mode_config.mutex);
>> +	DRM_DEBUG_DRIVER("Successfully destroyed all color properties\n");
>>   }
>>
>>   void intel_clrmgr_init(struct drm_device *dev)
>> diff --git a/drivers/gpu/drm/i915/intel_clrmgr.h b/drivers/gpu/drm/i915/intel_clrmgr.h
>> index 8cff487..5725d6b 100644
>> --- a/drivers/gpu/drm/i915/intel_clrmgr.h
>> +++ b/drivers/gpu/drm/i915/intel_clrmgr.h
>> @@ -39,6 +39,13 @@
>>   #define CLRMGR_PROP_NAME_MAX				128
>>   #define CLRMGR_PROP_RANGE_MAX				0xFFFFFFFFFFFFFFFF
>>
>> +/* CSC */
>> + /* CSC / Wide gamut */
>> +#define VLV_CSC_MATRIX_MAX_VALS		9
>> +#define VLV_CSC_VALUE_MASK			0xFFF
>> +#define VLV_CSC_COEFF_SHIFT			16
>> +
>> +
>>   /* Properties */
>>   enum clrmgr_tweaks {
>>   	csc = 0,
>> @@ -50,6 +57,15 @@ enum clrmgr_tweaks {
>>   };
>>
>>   /*
>> +* intel_clrmgr_set_csc
>> +* CSC correction method is different across various
>> +* gen devices. This wrapper function calls the respective
>> +* platform specific function to set CSC
>> +*/
>> +bool intel_clrmgr_set_csc(struct drm_crtc *crtc,
>> +	struct drm_property *prop, uint64_t data);
>> +
>> +/*
>>   * intel_attach_plane_color_correction:
>>   * Attach plane level color correction DRM properties to
>>   * corresponding plane objects.
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 7ba5785..a10b9bb 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -437,6 +437,7 @@ struct intel_crtc {
>>
>>   	int scanline_offset;
>>   	struct intel_mmio_flip mmio_flip;
>> +	struct drm_property_blob *csc_blob;
>>   };
>>
>>   struct intel_plane_wm_parameters {
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index 31344bf..487ce44 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -851,6 +851,9 @@ struct drm_mode_config {
>>   	struct drm_property *aspect_ratio_property;
>>   	struct drm_property *dirty_info_property;
>>
>> +	/* Color correction properties */
>> +	struct drm_property *csc_property;
>> +
>>   	/* dumb ioctl parameters */
>>   	uint32_t preferred_depth, prefer_shadow;
>>
>> @@ -981,6 +984,10 @@ extern int drm_mode_connector_set_path_property(struct drm_connector *connector,
>>   						char *path);
>>   extern int drm_mode_connector_update_edid_property(struct drm_connector *connector,
>>   						struct edid *edid);
>> +extern struct drm_property_blob *drm_property_create_blob(struct drm_device *dev,
>> +						int length, void *data);
>> +extern void drm_property_destroy_blob(struct drm_device *dev,
>> +			       struct drm_property_blob *blob);
>>
>>   static inline bool drm_property_type_is(struct drm_property *property,
>>   		uint32_t type)
>

Sharma, Shashank Sept. 10, 2014, 12:05 p.m. UTC | #5

Hi Daniel,

 > Overall I still think there's a bit too much indirection - I don't 
see why
 > the color manager needs to sit in a separate file with separate
 > registration functions. Doing it like that rips apart the per-crtc
 > setup/teardown quite a lot and isn't how properties are handled anywhere
 > else.

I dont see any issue creating a new file, for 2 reasons:
- I dont see any value in overpopulating the 11K lines file intel-
   display.c any further.
- All these properties are doing color correction, and hence there is
   no harm in keeping them in a functionally separate file.
- How does it tear down per CRTC setup creating a separate file :) ?
   I can see all standard DRM connector property getting created in
   drm_crtc.c, other properties like pfit, force-audio etc are getting
   created in intel_modes.c. They are already spread across functional
   files, so I find it best to create in color correction file.

Regards
Shashank
On 9/10/2014 12:10 PM, Daniel Vetter wrote:
> On Tue, Sep 09, 2014 at 06:30:09PM -0700, Matt Roper wrote:
>> On Tue, Sep 09, 2014 at 11:53:15AM +0530, [e-mail] wrote:
>>> From: Shashank Sharma <[e-mail]>
>>>
>>> This patch adds support for CSC correction color property.
>>> It does the following:
>>> 1. Creates a new DRM property for CSC correction. Adds this into
>>>     mode_config.
>>> 2. Attaches this CSC property to calling CRTC. Creates a blob
>>>     to store the correction values, and attaches the blob to CRTC.
>>> 3. Adds function intel_clrmgr_set_csc: This is a wrapper function
>>>     which checks the platform type, and calls the valleyview
>>>     specific set_csc function. As different platforms may have different
>>>     methods of setting CSC, wrapper function is required to route to proper
>>>     core CSC set function. In future, the support for other platfroms can be
>>>     plugged-in here. Adding this function as .set_property CSC color property.
>>> 4. Adds function vlv_set_csc: core function to program CSC coefficients as per
>>>     vlv specs, and then enable CSC.
>>>
>>> Signed-off-by: Shashank Sharma <[e-mail]>
>>
>> I haven't read up on the hardware programming side of this code to give
>> any comments there, but I got a bit lost trying to follow your blob
>> upload handling here.  Like Bob noted, it kind of looks like you're
>> trying to add userspace blob property upload functionality that would
>> really belong in the DRM core.  However, in the intermediate/long term
>> there probably isn't really a need for this kind of blob upload support
>> because the atomic propertysets will provide the functionality you need;
>> once we have atomic support, I think it would be better to just make
>> each of these values an independent property and upload all of the
>> values together as part of a single property set.  But I realize you're
>> specifically trying to add add this support in a pre-atomic world which
>> makes things more challenging.  Atomic is definitely coming, but I think
>> the timeframe is kind of uncertain still, so it's really going to be up
>> to the upstream maintainers on how to proceed.  Maybe Daniel can give
>> you some direction?
>
> I've thought the csc stuff here is just a matrix of register values, and
> highly intel specific at that. So might as well keep it as a blob property
> for now until either the specific layout changes or some standard for
> generic csc emerges.
>
> Overall I still think there's a bit too much indirection - I don't see why
> the color manager needs to sit in a separate file with separate
> registration functions. Doing it like that rips apart the per-crtc
> setup/teardown quite a lot and isn't how properties are handled anywhere
> else.
> -Daniel
>
>>
>>
>> Matt
>>
>>> ---
>>>   drivers/gpu/drm/drm_crtc.c          |   4 +-
>>>   drivers/gpu/drm/i915/i915_reg.h     |  11 ++
>>>   drivers/gpu/drm/i915/intel_clrmgr.c | 208 +++++++++++++++++++++++++++++++++++-
>>>   drivers/gpu/drm/i915/intel_clrmgr.h |  16 +++
>>>   drivers/gpu/drm/i915/intel_drv.h    |   1 +
>>>   include/drm/drm_crtc.h              |   7 ++
>>>   6 files changed, 244 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>>> index 272b66f..be9d110 100644
>>> --- a/drivers/gpu/drm/drm_crtc.c
>>> +++ b/drivers/gpu/drm/drm_crtc.c
>>> @@ -3917,7 +3917,7 @@ done:
>>>   	return ret;
>>>   }
>>>
>>> -static struct drm_property_blob *drm_property_create_blob(struct drm_device *dev, int length,
>>> +struct drm_property_blob *drm_property_create_blob(struct drm_device *dev, int length,
>>>   							  void *data)
>>>   {
>>>   	struct drm_property_blob *blob;
>>> @@ -3944,7 +3944,7 @@ static struct drm_property_blob *drm_property_create_blob(struct drm_device *dev
>>>   	return blob;
>>>   }
>>>
>>> -static void drm_property_destroy_blob(struct drm_device *dev,
>>> +void drm_property_destroy_blob(struct drm_device *dev,
>>>   			       struct drm_property_blob *blob)
>>>   {
>>>   	drm_mode_object_put(dev, &blob->base);
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>> index 20673cc..e3010b3 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -6183,6 +6183,17 @@ enum punit_power_well {
>>>   #define PIPE_CSC_POSTOFF_ME(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_ME, _PIPE_B_CSC_POSTOFF_ME)
>>>   #define PIPE_CSC_POSTOFF_LO(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_LO, _PIPE_B_CSC_POSTOFF_LO)
>>>
>>> +/* VLV color correction registers */
>>> +/* CSC */
>>> +#define PIPECONF_CSC_ENABLE	(1 << 15)
>>> +#define _PIPEACSC		(dev_priv->info.display_mmio_offset + \
>>> +								0x600b0)
>>> +#define _PIPEBCSC		(dev_priv->info.display_mmio_offset + \
>>> +								0x610b0)
>>> +#define PIPECSC(pipe)		(_PIPEACSC + (pipe *  CSC_OFFSET))
>>> +#define CSC_OFFSET			(_PIPEBCSC - _PIPEACSC)
>>> +#define PIPECSC(pipe)		(_PIPEACSC + (pipe *  CSC_OFFSET))
>>> +
>>>   /* VLV MIPI registers */
>>>
>>>   #define _MIPIA_PORT_CTRL			(VLV_DISPLAY_BASE + 0x61190)
>>> diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c b/drivers/gpu/drm/i915/intel_clrmgr.c
>>> index ac0a890..36c64c1 100644
>>> --- a/drivers/gpu/drm/i915/intel_clrmgr.c
>>> +++ b/drivers/gpu/drm/i915/intel_clrmgr.c
>>> @@ -32,6 +32,138 @@
>>>   #include "i915_reg.h"
>>>   #include "intel_clrmgr.h"
>>>
>>> +/*
>>> +* Names to register with color properties.
>>> +* Sequence must be the same as the order
>>> +* of enum clrmgr_tweaks
>>> +*/
>>> +const char *clrmgr_property_names[] = {
>>> +	/* csc */
>>> +	"csc-correction",
>>> +	/* add a new prop name here */
>>> +};
>>> +
>>> +
>>> +/*
>>> +* Sizes for color properties. This can differ
>>> +* platform by platform, hence 'vlv' prefix
>>> +* The sequence must be same as the order of
>>> +* enum clrmgr_tweaks
>>> +*/
>>> +u32 vlv_color_property_sizes[] = {
>>> +	VLV_CSC_MATRIX_MAX_VALS,
>>> +	/* Add new property size here */
>>> +};
>>
>> As with the property names, I'm not sure whether having an array here
>> gives us much clarity.  I think it's fine to just pass
>> VLV_CSC_MATRIX_MAX_VALS directly to drm_property_create_blob() in the
>> code below.
>>
>>
>>> +
>>> +/* Default CSC values to create property with */
>>> +uint64_t vlv_csc_default[VLV_CSC_MATRIX_MAX_VALS] = {
>>> +	0x400, 0, 0, 0, 0x400, 0, 0, 0, 0x400
>>> +};
>>> +
>>> +/*
>>> +* vlv_set_csc
>>> +* Valleyview specific csc correction method.
>>> +* Programs the 6 csc registers with 3x3 correction matrix
>>> +* values.
>>> +* inputs:
>>> +* - intel_crtc*
>>> +* - color manager registered property for csc correction
>>> +* - data: pointer to correction values to be applied
>>> +*/
>>> +/* Enable color space conversion on PIPE */
>>> +bool vlv_set_csc(struct intel_crtc *intel_crtc,
>>> +	struct drm_property *prop, uint64_t values, bool enable)
>>> +{
>>> +	u32 count = 0;
>>> +	u32 c0, c1, c2;
>>> +	u32 pipeconf, csc_reg, data_size;
>>> +	uint64_t *blob_data;
>>> +	uint64_t *data = NULL;
>>> +	struct drm_device *dev = intel_crtc->base.dev;
>>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>> +	struct drm_property_blob *blob = intel_crtc->csc_blob;
>>> +
>>> +	/* Sanity */
>>> +	if (!blob || (blob->length != VLV_CSC_MATRIX_MAX_VALS)) {
>>> +		DRM_ERROR("Invalid/NULL CSC blob\n");
>>> +		return false;
>>> +	}
>>> +	blob_data = (uint64_t *)blob->data;
>>> +
>>> +	/* No need of values to disable property */
>>> +	if (!enable)
>>> +		goto configure;
>>> +
>>> +	/* Enabling property needs correction values */
>>> +	data_size = VLV_CSC_MATRIX_MAX_VALS;
>>> +	data = kmalloc(sizeof(uint64_t) * (data_size), GFP_KERNEL);
>>> +	if (!data) {
>>> +		DRM_ERROR("Out of memory\n");
>>> +		return false;
>>> +	}
>>> +
>>> +	if (copy_from_user((void *)data, (const void __user *)values,
>>> +			data_size * sizeof(uint64_t))) {
>>> +		DRM_ERROR("Failed to copy all data\n");
>>> +		kfree(data);
>>> +		return false;
>>> +	}
>>> +
>>> +	DRM_DEBUG_DRIVER("Setting CSC on pipe = %d\n", intel_crtc->pipe);
>>> +	csc_reg = PIPECSC(intel_crtc->pipe);
>>> +
>>> +	/* Read CSC matrix, one row at a time */
>>> +	while (count < VLV_CSC_MATRIX_MAX_VALS) {
>>> +		c0 = data[count] & VLV_CSC_VALUE_MASK;
>>> +		*blob_data++ = c0;
>>> +		c1 = data[count] & VLV_CSC_VALUE_MASK;
>>> +		*blob_data++ = c1;
>>> +		c2 = data[count] & VLV_CSC_VALUE_MASK;
>>> +		*blob_data++ = c2;
>>> +
>>> +		/* C0 is LSB 12bits, C1 is MSB 16-27 */
>>> +		I915_WRITE(csc_reg, (c1 << VLV_CSC_COEFF_SHIFT) | c0);
>>> +		csc_reg += 4;
>>> +
>>> +		/* C2 is LSB 12 bits */
>>> +		I915_WRITE(csc_reg, c2);
>>> +		csc_reg += 4;
>>> +	}
>>> +
>>> +configure:
>>> +
>>> +	/* Enable / Disable csc correction */
>>> +	pipeconf = I915_READ(PIPECONF(intel_crtc->pipe));
>>> +	enable ? (pipeconf |= PIPECONF_CSC_ENABLE) :
>>> +		(pipeconf &= ~PIPECONF_CSC_ENABLE);
>>> +	I915_WRITE(PIPECONF(intel_crtc->pipe), pipeconf);
>>> +	POSTING_READ(PIPECONF(intel_crtc->pipe));
>>> +	DRM_DEBUG_DRIVER("CSC successfully %s pipe %C\n",
>>> +		enable ? "enabled" : "disabled", pipe_name(intel_crtc->pipe));
>>> +
>>> +	kfree(data);
>>> +	return true;
>>> +}
>>> +
>>> +bool intel_clrmgr_set_csc(struct drm_crtc *crtc,
>>> +	struct drm_property *prop, uint64_t values)
>>> +{
>>> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>> +	struct drm_device *dev = intel_crtc->base.dev;
>>> +	bool enable;
>>> +
>>> +	/* First value is enable/disable control, others are data */
>>> +	enable = *((uint64_t *)values);
>>> +	values += (sizeof(uint64_t));
>>> +
>>> +	if (IS_VALLEYVIEW(dev))
>>> +		return vlv_set_csc(intel_crtc, prop, values, enable);
>>> +
>>> +	/* Todo: Support other gen devices */
>>> +	DRM_ERROR("Color correction is supported only on VLV for now\n");
>>> +	return false;
>>> +}
>>> +
>>>   void
>>>   intel_attach_plane_color_correction(struct intel_plane *intel_plane)
>>>   {
>>> @@ -41,18 +173,92 @@ intel_attach_plane_color_correction(struct intel_plane *intel_plane)
>>>   void
>>>   intel_attach_pipe_color_correction(struct intel_crtc *intel_crtc)
>>>   {
>>> +	struct drm_device *dev = intel_crtc->base.dev;
>>> +	struct drm_mode_object *obj = &intel_crtc->base.base;
>>> +	struct drm_property *property = NULL;
>>> +
>>>   	DRM_DEBUG_DRIVER("\n");
>>> +	mutex_lock(&dev->mode_config.mutex);
>>> +
>>> +	/* color property = csc */
>>> +	property = dev->mode_config.csc_property;
>>> +	if (!property) {
>>> +		DRM_ERROR("No such property to attach\n");
>>> +		goto release_mutex;
>>> +	}
>>> +
>>> +	/* create blob for correction values */
>>> +	intel_crtc->csc_blob = drm_property_create_blob(dev,
>>> +		vlv_color_property_sizes[csc], (void *)vlv_csc_default);
>>> +	if (!intel_crtc->csc_blob) {
>>> +		DRM_ERROR("Failed to create property blob\n");
>>> +		goto release_mutex;
>>> +	}
>>> +
>>> +	/* Attach blob with property */
>>> +	if (drm_object_property_set_value(obj, property,
>>> +			intel_crtc->csc_blob->base.id)) {
>>> +		DRM_ERROR("Failed to attach property blob, destroying\n");
>>> +		drm_property_destroy_blob(dev, intel_crtc->csc_blob);
>>> +		goto release_mutex;
>>> +	}
>>> +
>>> +	DRM_DEBUG_DRIVER("Successfully attached CSC property\n");
>>> +
>>> +release_mutex:
>>> +	mutex_unlock(&dev->mode_config.mutex);
>>>   }
>>>
>>>   int intel_clrmgr_create_color_properties(struct drm_device *dev)
>>>   {
>>> +	int ret = 0;
>>> +	struct drm_property *property;
>>> +
>>>   	DRM_DEBUG_DRIVER("\n");
>>> -	return 0;
>>> +	mutex_lock(&dev->mode_config.mutex);
>>> +
>>> +	/* CSC correction color property, blob type, size 0 */
>>> +	property = drm_property_create(dev, DRM_MODE_PROP_BLOB,
>>> +		clrmgr_property_names[csc], 0);
>>> +	if (!property) {
>>> +		DRM_ERROR("Failed to create property(CSC)\n");
>>> +		ret++;
>>> +	} else {
>>> +		dev->mode_config.csc_property = property;
>>> +		DRM_DEBUG_DRIVER("Created property: CSC\n");
>>> +	}
>>> +
>>> +	mutex_unlock(&dev->mode_config.mutex);
>>> +	return ret;
>>>   }
>>>
>>>   void intel_clrmgr_destroy_color_properties(struct drm_device *dev)
>>>   {
>>> +	struct drm_crtc *crtc;
>>> +	struct intel_crtc *intel_crtc;
>>> +
>>>   	DRM_DEBUG_DRIVER("\n");
>>> +
>>> +	mutex_lock(&dev->mode_config.mutex);
>>> +
>>> +	/* CSC correction color property */
>>> +	if (dev->mode_config.csc_property) {
>>> +		drm_property_destroy(dev, dev->mode_config.csc_property);
>>> +		dev->mode_config.csc_property = NULL;
>>> +		DRM_DEBUG_DRIVER("Destroyed property: CSC\n");
>>> +	}
>>> +
>>> +	/* Destroy property blob from each CRTC */
>>> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
>>> +		intel_crtc = to_intel_crtc(crtc);
>>> +		if (intel_crtc->csc_blob) {
>>> +			drm_property_destroy_blob(dev, intel_crtc->csc_blob);
>>> +			intel_crtc->csc_blob = NULL;
>>> +		}
>>> +	}
>>> +
>>> +	mutex_unlock(&dev->mode_config.mutex);
>>> +	DRM_DEBUG_DRIVER("Successfully destroyed all color properties\n");
>>>   }
>>>
>>>   void intel_clrmgr_init(struct drm_device *dev)
>>> diff --git a/drivers/gpu/drm/i915/intel_clrmgr.h b/drivers/gpu/drm/i915/intel_clrmgr.h
>>> index 8cff487..5725d6b 100644
>>> --- a/drivers/gpu/drm/i915/intel_clrmgr.h
>>> +++ b/drivers/gpu/drm/i915/intel_clrmgr.h
>>> @@ -39,6 +39,13 @@
>>>   #define CLRMGR_PROP_NAME_MAX				128
>>>   #define CLRMGR_PROP_RANGE_MAX				0xFFFFFFFFFFFFFFFF
>>>
>>> +/* CSC */
>>> + /* CSC / Wide gamut */
>>> +#define VLV_CSC_MATRIX_MAX_VALS		9
>>> +#define VLV_CSC_VALUE_MASK			0xFFF
>>> +#define VLV_CSC_COEFF_SHIFT			16
>>> +
>>> +
>>>   /* Properties */
>>>   enum clrmgr_tweaks {
>>>   	csc = 0,
>>> @@ -50,6 +57,15 @@ enum clrmgr_tweaks {
>>>   };
>>>
>>>   /*
>>> +* intel_clrmgr_set_csc
>>> +* CSC correction method is different across various
>>> +* gen devices. This wrapper function calls the respective
>>> +* platform specific function to set CSC
>>> +*/
>>> +bool intel_clrmgr_set_csc(struct drm_crtc *crtc,
>>> +	struct drm_property *prop, uint64_t data);
>>> +
>>> +/*
>>>   * intel_attach_plane_color_correction:
>>>   * Attach plane level color correction DRM properties to
>>>   * corresponding plane objects.
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>> index 7ba5785..a10b9bb 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -437,6 +437,7 @@ struct intel_crtc {
>>>
>>>   	int scanline_offset;
>>>   	struct intel_mmio_flip mmio_flip;
>>> +	struct drm_property_blob *csc_blob;
>>>   };
>>>
>>>   struct intel_plane_wm_parameters {
>>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>>> index 31344bf..487ce44 100644
>>> --- a/include/drm/drm_crtc.h
>>> +++ b/include/drm/drm_crtc.h
>>> @@ -851,6 +851,9 @@ struct drm_mode_config {
>>>   	struct drm_property *aspect_ratio_property;
>>>   	struct drm_property *dirty_info_property;
>>>
>>> +	/* Color correction properties */
>>> +	struct drm_property *csc_property;
>>> +
>>>   	/* dumb ioctl parameters */
>>>   	uint32_t preferred_depth, prefer_shadow;
>>>
>>> @@ -981,6 +984,10 @@ extern int drm_mode_connector_set_path_property(struct drm_connector *connector,
>>>   						char *path);
>>>   extern int drm_mode_connector_update_edid_property(struct drm_connector *connector,
>>>   						struct edid *edid);
>>> +extern struct drm_property_blob *drm_property_create_blob(struct drm_device *dev,
>>> +						int length, void *data);
>>> +extern void drm_property_destroy_blob(struct drm_device *dev,
>>> +			       struct drm_property_blob *blob);
>>>
>>>   static inline bool drm_property_type_is(struct drm_property *property,
>>>   		uint32_t type)
>>> --
>>> 1.9.1
>>>
>>
>> --
>> Matt Roper
>> Graphics Software Engineer
>> IoTG Platform Enabling & Development
>> Intel Corporation
>> [phone]
>> _______________________________________________
>> Intel-gfx mailing list
>> [e-mail]
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

Daniel Vetter Sept. 10, 2014, 12:13 p.m. UTC | #6

On Wed, Sep 10, 2014 at 2:05 PM, Sharma, Shashank
<[e-mail]> wrote:
> - How does it tear down per CRTC setup creating a separate file :) ?
>   I can see all standard DRM connector property getting created in
>   drm_crtc.c, other properties like pfit, force-audio etc are getting
>   created in intel_modes.c. They are already spread across functional
>   files, so I find it best to create in color correction file.

I don't mean creating/destroying the properties, but
attaching/detaching them. I agree with Matt that that should happen
directly in the crtc setup/teardown functions.
-Daniel

Paauwe, Bob J Sept. 10, 2014, 4:03 p.m. UTC | #7

On Wed, 10 Sep 2014 14:25:00 +0530
"Sharma, Shashank" <[e-mail]> wrote:

> Thanks for your time and review.

Thanks for working on this.  This is a feature that the IOTG group is
interested in.

> Please find my comments inline.
> 
> Regards
> Shashank
> 
> On 9/10/2014 4:21 AM, Bob Paauwe wrote:
> > On Tue, 9 Sep 2014 11:53:15 +0530
> > <[e-mail]> wrote:
> >
> >> From: Shashank Sharma <[e-mail]>
> >>
> >> This patch adds support for CSC correction color property.
> >> It does the following:
> >> 1. Creates a new DRM property for CSC correction. Adds this into
> >>     mode_config.
> >> 2. Attaches this CSC property to calling CRTC. Creates a blob
> >>     to store the correction values, and attaches the blob to CRTC.
> >> 3. Adds function intel_clrmgr_set_csc: This is a wrapper function
> >>     which checks the platform type, and calls the valleyview
> >>     specific set_csc function. As different platforms may have different
> >>     methods of setting CSC, wrapper function is required to route to proper
> >>     core CSC set function. In future, the support for other platfroms can be
> >>     plugged-in here. Adding this function as .set_property CSC color property.
> >> 4. Adds function vlv_set_csc: core function to program CSC coefficients as per
> >>     vlv specs, and then enable CSC.
> >>
> >> Signed-off-by: Shashank Sharma <[e-mail]>
> >> ---
> >>   drivers/gpu/drm/drm_crtc.c          |   4 +-
> >>   drivers/gpu/drm/i915/i915_reg.h     |  11 ++
> >>   drivers/gpu/drm/i915/intel_clrmgr.c | 208 +++++++++++++++++++++++++++++++++++-
> >>   drivers/gpu/drm/i915/intel_clrmgr.h |  16 +++
> >>   drivers/gpu/drm/i915/intel_drv.h    |   1 +
> >>   include/drm/drm_crtc.h              |   7 ++
> >>   6 files changed, 244 insertions(+), 3 deletions(-)
> >>

> >> +*/
> >> +/* Enable color space conversion on PIPE */
> >> +bool vlv_set_csc(struct intel_crtc *intel_crtc,
> >> +	struct drm_property *prop, uint64_t values, bool enable)
> >> +{
> >> +	u32 count = 0;
> >> +	u32 c0, c1, c2;
> >> +	u32 pipeconf, csc_reg, data_size;
> >> +	uint64_t *blob_data;
> >> +	uint64_t *data = NULL;
> >> +	struct drm_device *dev = intel_crtc->base.dev;
> >> +	struct drm_i915_private *dev_priv = dev->dev_private;
> >> +	struct drm_property_blob *blob = intel_crtc->csc_blob;
> >> +
> >> +	/* Sanity */
> >> +	if (!blob || (blob->length != VLV_CSC_MATRIX_MAX_VALS)) {
> >> +		DRM_ERROR("Invalid/NULL CSC blob\n");
> >> +		return false;
> >> +	}
> >> +	blob_data = (uint64_t *)blob->data;
> >> +
> >> +	/* No need of values to disable property */
> >> +	if (!enable)
> >> +		goto configure;
> >> +
> >> +	/* Enabling property needs correction values */
> >> +	data_size = VLV_CSC_MATRIX_MAX_VALS;
> >> +	data = kmalloc(sizeof(uint64_t) * (data_size), GFP_KERNEL);
> >> +	if (!data) {
> >> +		DRM_ERROR("Out of memory\n");
> >> +		return false;
> >> +	}
> >> +
> >> +	if (copy_from_user((void *)data, (const void __user *)values,
> >> +			data_size * sizeof(uint64_t))) {
> >> +		DRM_ERROR("Failed to copy all data\n");
> >> +		kfree(data);
> >> +		return false;
> >> +	}
> >
> > I don't think this should be doing a copy_from_user.  It should be the
> > drm_property code that handles the IOCTL that does that.
> >
> > The whole handling of the blob property looks suspect to me.  I believe
> > that the DRM code currently doesn't allow blob type properties to be
> > set. So you should first have a patch that adds that capability to the
> > DRM property code.  But I'm not sure that's even the right way to
> > handle this.
> >
> Humm. This adds another dilemma of how to do this.
> I believe blob property type is the only one which suits the CSC 
> property, (also gamma correction, with 129 correction values)
> 
> Even if we create a set_blob, that will be again doing a 
> copy_from_user() only, won't it ?
> 
> I can add a patch to do a set_blob in the next patch set. Please have a 
> look and let me know if that's what you expect.

Yeah, any set_blob function will need to do the copy_from_user but I
think it would be better if this was in the core drm code.  I thought I
saw a patch a while back that added writable property blobs but I don't
see it in the code so maybe it never got in.  Might be worth searching
the archive for that.

> >> +
> >> +	DRM_DEBUG_DRIVER("Setting CSC on pipe = %d\n", intel_crtc->pipe);
> >> +	csc_reg = PIPECSC(intel_crtc->pipe);
> >> +
> >> +	/* Read CSC matrix, one row at a time */
> >> +	while (count < VLV_CSC_MATRIX_MAX_VALS) {
> >> +		c0 = data[count] & VLV_CSC_VALUE_MASK;
> >> +		*blob_data++ = c0;
> >> +		c1 = data[count] & VLV_CSC_VALUE_MASK;
> >> +		*blob_data++ = c1;
> >> +		c2 = data[count] & VLV_CSC_VALUE_MASK;
> >> +		*blob_data++ = c2;
> >
> > You aren't incrementing count after each assignment above, that means
> > that c0, c1, and c2 are all getting set to the same value. That doesn't
> > seem right.
> >
> 
> I am sorry, this is bad. While splitting the patches, I messed here, and 
> looks like the unit CSC values were getting applied properly, so dint 
> catch this during ULT.
> Thanks for pointing this out, will fix this.
> >> +
> >> +		/* C0 is LSB 12bits, C1 is MSB 16-27 */
> >> +		I915_WRITE(csc_reg, (c1 << VLV_CSC_COEFF_SHIFT) | c0);
> >> +		csc_reg += 4;
> >> +
> >> +		/* C2 is LSB 12 bits */
> >> +		I915_WRITE(csc_reg, c2);
> >> +		csc_reg += 4;
> >> +	}
> >> +
> >> +configure:
> >> +
> >> +	/* Enable / Disable csc correction */
> >> +	pipeconf = I915_READ(PIPECONF(intel_crtc->pipe));
> >> +	enable ? (pipeconf |= PIPECONF_CSC_ENABLE) :
> >> +		(pipeconf &= ~PIPECONF_CSC_ENABLE);
> >> +	I915_WRITE(PIPECONF(intel_crtc->pipe), pipeconf);
> >> +	POSTING_READ(PIPECONF(intel_crtc->pipe));
> >> +	DRM_DEBUG_DRIVER("CSC successfully %s pipe %C\n",
> >> +		enable ? "enabled" : "disabled", pipe_name(intel_crtc->pipe));
> >> +
> >> +	kfree(data);
> >> +	return true;
> >> +}
> >> +
> >> +bool intel_clrmgr_set_csc(struct drm_crtc *crtc,
> >> +	struct drm_property *prop, uint64_t values)
> >> +{
> >> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >> +	struct drm_device *dev = intel_crtc->base.dev;
> >> +	bool enable;
> >> +
> >> +	/* First value is enable/disable control, others are data */
> >> +	enable = *((uint64_t *)values);
> >> +	values += (sizeof(uint64_t));
> >
> > It looks like you're trying to pass in a user space pointer to some
> > type of csc structure.  As above, I'm not sure that's the right way to
> > approach this. Ideally, I think we want to use the standard drm
> > property types or you're going to have to define a new drm property
> > type that could be universally used.
> >
> Actually, I need to first decide, if we need to enable / disable the 
> property. Once we are sure that user space wants to enable it, then I 
> need correction coefficients. So I decided to have first byte as 
> enable/disable control, and others as correction values.
> 
> I dont see any other way of doing it, while coming from drm_property 
> interface.
> Do you think that we can create a custom/new property type for this 
> reason ?

Since Daniel is OK with a blob type property for this, you might
consider defining a structure for it and then casting the blob pointer
to that data structure.  It would make it clear what you expect the
blob to look like. 

You could also separate the enable/disable from the actual values, you
kind of already do that in the code anyway.  One property that holds
the values and when the set handler takes the data and programs the
registers.  A second property to enable/disable CSC.  All it does is
program the PIPECONF_CSC_ENABLE bit.

Matt Roper Sept. 10, 2014, 10:17 p.m. UTC | #8

On Tue, Sep 09, 2014 at 11:53:15AM +0530, [e-mail] wrote:
> From: Shashank Sharma <[e-mail]>
> 
> This patch adds support for CSC correction color property.
> It does the following:
> 1. Creates a new DRM property for CSC correction. Adds this into
>    mode_config.
...

One thing I forgot to ask in my response yesterday...does it make sense
to create this property in dev->mode_config rather than in dev_priv?
Some properties (like the plane rotation property that went in a little
while ago) do seem like a good match for dev->mode_config because
they're generic concepts that a lot of different drivers will want to
reuse (and in some cases the common core/helper library code might want
to make use of them as well, as we do in restore_fbdev_mode()).  But
it's unclear to me whether non-Intel hardware handles color correction
similarly enough that the CSC property you're creating here is something
other drivers will ever use, or whether it will always be an
Intel-specific thing.  My understanding is that the values here are very
Intel-specific, so there's never going to be an opportunity for
core/helper library code to make use of the property, right?  If this
does turn out to be an Intel-only property, then it's probably better to
move it to dev_priv (where we have a couple of our connector properties
today).

If you do decide that there's potential for reuse by other drivers and
that you want to keep this in dev->mode_config you'll want to make sure
you Cc the dri-devel mailing list on the patches that touch the DRM core
files.


Matt

Daniel Vetter Sept. 11, 2014, 7:53 a.m. UTC | #9

On Wed, Sep 10, 2014 at 03:17:34PM -0700, Matt Roper wrote:
> On Tue, Sep 09, 2014 at 11:53:15AM +0530, [e-mail] wrote:
> > From: Shashank Sharma <[e-mail]>
> > 
> > This patch adds support for CSC correction color property.
> > It does the following:
> > 1. Creates a new DRM property for CSC correction. Adds this into
> >    mode_config.
> ...
> 
> One thing I forgot to ask in my response yesterday...does it make sense
> to create this property in dev->mode_config rather than in dev_priv?
> Some properties (like the plane rotation property that went in a little
> while ago) do seem like a good match for dev->mode_config because
> they're generic concepts that a lot of different drivers will want to
> reuse (and in some cases the common core/helper library code might want
> to make use of them as well, as we do in restore_fbdev_mode()).  But
> it's unclear to me whether non-Intel hardware handles color correction
> similarly enough that the CSC property you're creating here is something
> other drivers will ever use, or whether it will always be an
> Intel-specific thing.  My understanding is that the values here are very
> Intel-specific, so there's never going to be an opportunity for
> core/helper library code to make use of the property, right?  If this
> does turn out to be an Intel-only property, then it's probably better to
> move it to dev_priv (where we have a couple of our connector properties
> today).
> 
> If you do decide that there's potential for reuse by other drivers and
> that you want to keep this in dev->mode_config you'll want to make sure
> you Cc the dri-devel mailing list on the patches that touch the DRM core
> files.

The csc matrix looks platform-specific enough (I expect it'll be different
on big-core, but haven't checked tbh) that a generic property imo doesn't
make sense. But others from the color manager task certainly qualify,
which is another reason not to smash them all together into a tightly knit
boundle.
-Daniel

Patch

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 272b66f..be9d110 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3917,7 +3917,7 @@  done:
 	return ret;
 }
 
-static struct drm_property_blob *drm_property_create_blob(struct drm_device *dev, int length,
+struct drm_property_blob *drm_property_create_blob(struct drm_device *dev, int length,
 							  void *data)
 {
 	struct drm_property_blob *blob;
@@ -3944,7 +3944,7 @@  static struct drm_property_blob *drm_property_create_blob(struct drm_device *dev
 	return blob;
 }
 
-static void drm_property_destroy_blob(struct drm_device *dev,
+void drm_property_destroy_blob(struct drm_device *dev,
 			       struct drm_property_blob *blob)
 {
 	drm_mode_object_put(dev, &blob->base);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 20673cc..e3010b3 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6183,6 +6183,17 @@  enum punit_power_well {
 #define PIPE_CSC_POSTOFF_ME(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_ME, _PIPE_B_CSC_POSTOFF_ME)
 #define PIPE_CSC_POSTOFF_LO(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_LO, _PIPE_B_CSC_POSTOFF_LO)
 
+/* VLV color correction registers */
+/* CSC */
+#define PIPECONF_CSC_ENABLE	(1 << 15)
+#define _PIPEACSC		(dev_priv->info.display_mmio_offset + \
+								0x600b0)
+#define _PIPEBCSC		(dev_priv->info.display_mmio_offset + \
+								0x610b0)
+#define PIPECSC(pipe)		(_PIPEACSC + (pipe *  CSC_OFFSET))
+#define CSC_OFFSET			(_PIPEBCSC - _PIPEACSC)
+#define PIPECSC(pipe)		(_PIPEACSC + (pipe *  CSC_OFFSET))
+
 /* VLV MIPI registers */
 
 #define _MIPIA_PORT_CTRL			(VLV_DISPLAY_BASE + 0x61190)
diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c b/drivers/gpu/drm/i915/intel_clrmgr.c
index ac0a890..36c64c1 100644
--- a/drivers/gpu/drm/i915/intel_clrmgr.c
+++ b/drivers/gpu/drm/i915/intel_clrmgr.c
@@ -32,6 +32,138 @@ 
 #include "i915_reg.h"
 #include "intel_clrmgr.h"
 
+/*
+* Names to register with color properties.
+* Sequence must be the same as the order
+* of enum clrmgr_tweaks
+*/
+const char *clrmgr_property_names[] = {
+	/* csc */
+	"csc-correction",
+	/* add a new prop name here */
+};
+
+
+/*
+* Sizes for color properties. This can differ
+* platform by platform, hence 'vlv' prefix
+* The sequence must be same as the order of
+* enum clrmgr_tweaks
+*/
+u32 vlv_color_property_sizes[] = {
+	VLV_CSC_MATRIX_MAX_VALS,
+	/* Add new property size here */
+};
+
+/* Default CSC values to create property with */
+uint64_t vlv_csc_default[VLV_CSC_MATRIX_MAX_VALS] = {
+	0x400, 0, 0, 0, 0x400, 0, 0, 0, 0x400
+};
+
+/*
+* vlv_set_csc
+* Valleyview specific csc correction method.
+* Programs the 6 csc registers with 3x3 correction matrix
+* values.
+* inputs:
+* - intel_crtc*
+* - color manager registered property for csc correction
+* - data: pointer to correction values to be applied
+*/
+/* Enable color space conversion on PIPE */
+bool vlv_set_csc(struct intel_crtc *intel_crtc,
+	struct drm_property *prop, uint64_t values, bool enable)
+{
+	u32 count = 0;
+	u32 c0, c1, c2;
+	u32 pipeconf, csc_reg, data_size;
+	uint64_t *blob_data;
+	uint64_t *data = NULL;
+	struct drm_device *dev = intel_crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_property_blob *blob = intel_crtc->csc_blob;
+
+	/* Sanity */
+	if (!blob || (blob->length != VLV_CSC_MATRIX_MAX_VALS)) {
+		DRM_ERROR("Invalid/NULL CSC blob\n");
+		return false;
+	}
+	blob_data = (uint64_t *)blob->data;
+
+	/* No need of values to disable property */
+	if (!enable)
+		goto configure;
+
+	/* Enabling property needs correction values */
+	data_size = VLV_CSC_MATRIX_MAX_VALS;
+	data = kmalloc(sizeof(uint64_t) * (data_size), GFP_KERNEL);
+	if (!data) {
+		DRM_ERROR("Out of memory\n");
+		return false;
+	}
+
+	if (copy_from_user((void *)data, (const void __user *)values,
+			data_size * sizeof(uint64_t))) {
+		DRM_ERROR("Failed to copy all data\n");
+		kfree(data);
+		return false;
+	}
+
+	DRM_DEBUG_DRIVER("Setting CSC on pipe = %d\n", intel_crtc->pipe);
+	csc_reg = PIPECSC(intel_crtc->pipe);
+
+	/* Read CSC matrix, one row at a time */
+	while (count < VLV_CSC_MATRIX_MAX_VALS) {
+		c0 = data[count] & VLV_CSC_VALUE_MASK;
+		*blob_data++ = c0;
+		c1 = data[count] & VLV_CSC_VALUE_MASK;
+		*blob_data++ = c1;
+		c2 = data[count] & VLV_CSC_VALUE_MASK;
+		*blob_data++ = c2;
+
+		/* C0 is LSB 12bits, C1 is MSB 16-27 */
+		I915_WRITE(csc_reg, (c1 << VLV_CSC_COEFF_SHIFT) | c0);
+		csc_reg += 4;
+
+		/* C2 is LSB 12 bits */
+		I915_WRITE(csc_reg, c2);
+		csc_reg += 4;
+	}
+
+configure:
+
+	/* Enable / Disable csc correction */
+	pipeconf = I915_READ(PIPECONF(intel_crtc->pipe));
+	enable ? (pipeconf |= PIPECONF_CSC_ENABLE) :
+		(pipeconf &= ~PIPECONF_CSC_ENABLE);
+	I915_WRITE(PIPECONF(intel_crtc->pipe), pipeconf);
+	POSTING_READ(PIPECONF(intel_crtc->pipe));
+	DRM_DEBUG_DRIVER("CSC successfully %s pipe %C\n",
+		enable ? "enabled" : "disabled", pipe_name(intel_crtc->pipe));
+
+	kfree(data);
+	return true;
+}
+
+bool intel_clrmgr_set_csc(struct drm_crtc *crtc,
+	struct drm_property *prop, uint64_t values)
+{
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_device *dev = intel_crtc->base.dev;
+	bool enable;
+
+	/* First value is enable/disable control, others are data */
+	enable = *((uint64_t *)values);
+	values += (sizeof(uint64_t));
+
+	if (IS_VALLEYVIEW(dev))
+		return vlv_set_csc(intel_crtc, prop, values, enable);
+
+	/* Todo: Support other gen devices */
+	DRM_ERROR("Color correction is supported only on VLV for now\n");
+	return false;
+}
+
 void
 intel_attach_plane_color_correction(struct intel_plane *intel_plane)
 {
@@ -41,18 +173,92 @@  intel_attach_plane_color_correction(struct intel_plane *intel_plane)
 void
 intel_attach_pipe_color_correction(struct intel_crtc *intel_crtc)
 {
+	struct drm_device *dev = intel_crtc->base.dev;
+	struct drm_mode_object *obj = &intel_crtc->base.base;
+	struct drm_property *property = NULL;
+
 	DRM_DEBUG_DRIVER("\n");
+	mutex_lock(&dev->mode_config.mutex);
+
+	/* color property = csc */
+	property = dev->mode_config.csc_property;
+	if (!property) {
+		DRM_ERROR("No such property to attach\n");
+		goto release_mutex;
+	}
+
+	/* create blob for correction values */
+	intel_crtc->csc_blob = drm_property_create_blob(dev,
+		vlv_color_property_sizes[csc], (void *)vlv_csc_default);
+	if (!intel_crtc->csc_blob) {
+		DRM_ERROR("Failed to create property blob\n");
+		goto release_mutex;
+	}
+
+	/* Attach blob with property */
+	if (drm_object_property_set_value(obj, property,
+			intel_crtc->csc_blob->base.id)) {
+		DRM_ERROR("Failed to attach property blob, destroying\n");
+		drm_property_destroy_blob(dev, intel_crtc->csc_blob);
+		goto release_mutex;
+	}
+
+	DRM_DEBUG_DRIVER("Successfully attached CSC property\n");
+
+release_mutex:
+	mutex_unlock(&dev->mode_config.mutex);
 }
 
 int intel_clrmgr_create_color_properties(struct drm_device *dev)
 {
+	int ret = 0;
+	struct drm_property *property;
+
 	DRM_DEBUG_DRIVER("\n");
-	return 0;
+	mutex_lock(&dev->mode_config.mutex);
+
+	/* CSC correction color property, blob type, size 0 */
+	property = drm_property_create(dev, DRM_MODE_PROP_BLOB,
+		clrmgr_property_names[csc], 0);
+	if (!property) {
+		DRM_ERROR("Failed to create property(CSC)\n");
+		ret++;
+	} else {
+		dev->mode_config.csc_property = property;
+		DRM_DEBUG_DRIVER("Created property: CSC\n");
+	}
+
+	mutex_unlock(&dev->mode_config.mutex);
+	return ret;
 }
 
 void intel_clrmgr_destroy_color_properties(struct drm_device *dev)
 {
+	struct drm_crtc *crtc;
+	struct intel_crtc *intel_crtc;
+
 	DRM_DEBUG_DRIVER("\n");
+
+	mutex_lock(&dev->mode_config.mutex);
+
+	/* CSC correction color property */
+	if (dev->mode_config.csc_property) {
+		drm_property_destroy(dev, dev->mode_config.csc_property);
+		dev->mode_config.csc_property = NULL;
+		DRM_DEBUG_DRIVER("Destroyed property: CSC\n");
+	}
+
+	/* Destroy property blob from each CRTC */
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		intel_crtc = to_intel_crtc(crtc);
+		if (intel_crtc->csc_blob) {
+			drm_property_destroy_blob(dev, intel_crtc->csc_blob);
+			intel_crtc->csc_blob = NULL;
+		}
+	}
+
+	mutex_unlock(&dev->mode_config.mutex);
+	DRM_DEBUG_DRIVER("Successfully destroyed all color properties\n");
 }
 
 void intel_clrmgr_init(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_clrmgr.h b/drivers/gpu/drm/i915/intel_clrmgr.h
index 8cff487..5725d6b 100644
--- a/drivers/gpu/drm/i915/intel_clrmgr.h
+++ b/drivers/gpu/drm/i915/intel_clrmgr.h
@@ -39,6 +39,13 @@ 
 #define CLRMGR_PROP_NAME_MAX				128
 #define CLRMGR_PROP_RANGE_MAX				0xFFFFFFFFFFFFFFFF
 
+/* CSC */
+ /* CSC / Wide gamut */
+#define VLV_CSC_MATRIX_MAX_VALS		9
+#define VLV_CSC_VALUE_MASK			0xFFF
+#define VLV_CSC_COEFF_SHIFT			16
+
+
 /* Properties */
 enum clrmgr_tweaks {
 	csc = 0,
@@ -50,6 +57,15 @@  enum clrmgr_tweaks {
 };
 
 /*
+* intel_clrmgr_set_csc
+* CSC correction method is different across various
+* gen devices. This wrapper function calls the respective
+* platform specific function to set CSC
+*/
+bool intel_clrmgr_set_csc(struct drm_crtc *crtc,
+	struct drm_property *prop, uint64_t data);
+
+/*
 * intel_attach_plane_color_correction:
 * Attach plane level color correction DRM properties to
 * corresponding plane objects.
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 7ba5785..a10b9bb 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -437,6 +437,7 @@  struct intel_crtc {
 
 	int scanline_offset;
 	struct intel_mmio_flip mmio_flip;
+	struct drm_property_blob *csc_blob;
 };
 
 struct intel_plane_wm_parameters {
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 31344bf..487ce44 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -851,6 +851,9 @@  struct drm_mode_config {
 	struct drm_property *aspect_ratio_property;
 	struct drm_property *dirty_info_property;
 
+	/* Color correction properties */
+	struct drm_property *csc_property;
+
 	/* dumb ioctl parameters */
 	uint32_t preferred_depth, prefer_shadow;
 
@@ -981,6 +984,10 @@  extern int drm_mode_connector_set_path_property(struct drm_connector *connector,
 						char *path);
 extern int drm_mode_connector_update_edid_property(struct drm_connector *connector,
 						struct edid *edid);
+extern struct drm_property_blob *drm_property_create_blob(struct drm_device *dev,
+						int length, void *data);
+extern void drm_property_destroy_blob(struct drm_device *dev,
+			       struct drm_property_blob *blob);
 
 static inline bool drm_property_type_is(struct drm_property *property,
 		uint32_t type)