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 17:35:34 +0530 Message-ID: <54103E8E.2070400@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> <20140910013009.GF26681@intel.com> <20140910064009.GO15520@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 300BD6E43D for ; Wed, 10 Sep 2014 05:05:38 -0700 (PDT) In-Reply-To: <20140910064009.GO15520@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter , Matt Roper Cc: daniel.vetter@intel.com, intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org Hi Daniel, > 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. I dont see any issue creating a new file, for 2 reasons: - I dont see any value in overpopulating the 11K lines file intel- display.c any further. - All these properties are doing color correction, and hence there is no harm in keeping them in a functionally separate file. - How does it tear down per CRTC setup creating a separate file :) ? I can see all standard DRM connector property getting created in drm_crtc.c, other properties like pfit, force-audio etc are getting created in intel_modes.c. They are already spread across functional files, so I find it best to create in color correction file. Regards Shashank On 9/10/2014 12:10 PM, Daniel Vetter wrote: > 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 >>> >>> 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 >> >> 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 >