[Intel-gfx] [PATCH 3/4] drm/i915: CSC color correction
Bob Paauwe
bob.j.paauwe at intel.com
Wed Sep 10 00:51:52 CEST 2014
On Tue, 9 Sep 2014 11:53:15 +0530
<shashank.sharma at intel.com> wrote:
> From: Shashank Sharma <shashank.sharma at intel.com>
>
> 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 <shashank.sharma at intel.com>
> ---
> 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)
More information about the Intel-gfx
mailing list