[Intel-gfx] [PATCH 3/4] drm/i915: CSC color correction

[Bob Paauwe]

On Wed, 10 Sep 2014 14:25:00 +0530
"Sharma, Shashank" <shashank.sharma at intel.com> 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
> > <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(-)
> >>

> >> +*/
> >> +/* 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.