From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Sharma, Shashank" Subject: Re: [PATCH 3/4] drm/i915: CSC color correction Date: Wed, 10 Sep 2014 14:25:00 +0530 Message-ID: <541011E4.6080603@intel.com> References: <20140726015853.GA8878@intel.com> <1410243796-11172-1-git-send-email-shashank.sharma@intel.com> <1410243796-11172-4-git-send-email-shashank.sharma@intel.com> <20140909155152.2df6ff6d@bpaauwe-desk.fm.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id B38186E401 for ; Wed, 10 Sep 2014 01:55:36 -0700 (PDT) In-Reply-To: <20140909155152.2df6ff6d@bpaauwe-desk.fm.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Bob Paauwe Cc: intel-gfx@lists.freedesktop.org, daniel.vetter@intel.com List-Id: intel-gfx@lists.freedesktop.org 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 > wrote: > >> From: Shashank Sharma >> >> 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 >> --- >> 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) >