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

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Sep 09, 2014 at 06:30:09PM -0700, Matt Roper wrote:
> On Tue, Sep 09, 2014 at 11:53:15AM +0530, shashank.sharma@intel.com wrote:
> > From: Shashank Sharma <shashank.sharma@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@intel.com>
> 
> 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
> (916) 356-2795
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch