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

["Sharma, Shashank" <shashank.sharma@intel.com>]

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
> <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>
>> ---
>>   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)
>