From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Sharma, Shashank" Subject: Re: [PATCH 1/4] drm/i915: Color manager framework for valleyview Date: Wed, 10 Sep 2014 16:50:56 +0530 Message-ID: <54103418.7050305@intel.com> References: <20140726015853.GA8878@intel.com> <1410243796-11172-1-git-send-email-shashank.sharma@intel.com> <1410243796-11172-2-git-send-email-shashank.sharma@intel.com> <20140910012904.GD26681@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id E400789DE3 for ; Wed, 10 Sep 2014 04:20:59 -0700 (PDT) In-Reply-To: <20140910012904.GD26681@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Matt Roper Cc: intel-gfx@lists.freedesktop.org, daniel.vetter@intel.com List-Id: intel-gfx@lists.freedesktop.org Matt, Thanks for your time and comments. Please find mine inline. Regards Shashank On 9/10/2014 6:59 AM, Matt Roper wrote: > On Tue, Sep 09, 2014 at 11:53:13AM +0530, shashank.sharma@intel.com wrote: >> From: Shashank Sharma >> >> Color manager is a framework which adds drm properties for >> color correction in I915 driver. This framework creates DRM >> properties for each color correction feature, and attaches >> it to appropriate CRTC/plane based on the property type. >> This allows userspace to fine tune the display as per the panel. >> >> This is the first patch of the series, what this patch does is: >> 1. Create 2 new files >> intel_clrmgr.c >> intel_clrmgr.h >> 2. Add color manager init function, This function will create >> DRM properties for color correction. >> 3. Add color manager exit function. This function will destroy >> registered DRM color properties. >> 4. Adds a enum for currently listed color correction properties: >> they are: >> CSC correction (wide gamut), Gamma correction, Contrast, >> Brightness, Hue and Saturation >> This enum will be further used to index color properties >> from array of elements. >> 5. Add names for vlv color properties. >> Signed-off-by: Shashank Sharma >> --- >> drivers/gpu/drm/i915/Makefile | 3 +- >> drivers/gpu/drm/i915/intel_clrmgr.c | 80 ++++++++++++++++++++++++++++= ++++++++ >> drivers/gpu/drm/i915/intel_clrmgr.h | 70 ++++++++++++++++++++++++++++= +++ >> drivers/gpu/drm/i915/intel_display.c | 5 +++ >> 4 files changed, 157 insertions(+), 1 deletion(-) >> create mode 100644 drivers/gpu/drm/i915/intel_clrmgr.c >> create mode 100644 drivers/gpu/drm/i915/intel_clrmgr.h >> >> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefi= le >> index c1dd485..6361c9b 100644 >> --- a/drivers/gpu/drm/i915/Makefile >> +++ b/drivers/gpu/drm/i915/Makefile >> @@ -46,7 +46,8 @@ i915-y +=3D intel_bios.o \ >> intel_modes.o \ >> intel_overlay.o \ >> intel_sideband.o \ >> - intel_sprite.o >> + intel_sprite.o \ >> + intel_clrmgr.o >> i915-$(CONFIG_ACPI) +=3D intel_acpi.o intel_opregion.o >> i915-$(CONFIG_DRM_I915_FBDEV) +=3D intel_fbdev.o >> >> diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c b/drivers/gpu/drm/i915/= intel_clrmgr.c >> new file mode 100644 >> index 0000000..0def917 >> --- /dev/null >> +++ b/drivers/gpu/drm/i915/intel_clrmgr.c >> @@ -0,0 +1,80 @@ >> +/* >> + * Copyright =A9 2014 Intel Corporation >> + * >> + * Permission is hereby granted, free of charge, to any person obtainin= g a >> + * copy of this software and associated documentation files (the "Softw= are"), >> + * to deal in the Software without restriction, including without limit= ation >> + * the rights to use, copy, modify, merge, publish, distribute, sublice= nse, >> + * and/or sell copies of the Software, and to permit persons to whom the >> + * Software is furnished to do so, subject to the following conditions: >> + * >> + * The above copyright notice and this permission notice (including the= next >> + * paragraph) shall be included in all copies or substantial portions o= f the >> + * Software. >> + * >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPR= ESS OR >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABIL= ITY, >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT S= HALL >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR= OTHER >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARIS= ING >> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER = DEALINGS >> + * IN THE SOFTWARE. >> + * >> + * Authors: >> + * =3D=3D=3D=3D=3D=3D >> + * Shashank Sharma >> + * Uma Shankar >> + * Sonika Jindal >> + */ >> + >> +#include "i915_drm.h" >> +#include "i915_drv.h" >> +#include "i915_reg.h" >> +#include "intel_clrmgr.h" >> + >> +/* Names to register with color properties */ >> +const char *clrmgr_property_names[] =3D { >> + /* csc */ >> + "csc-correction", >> + /* gamma */ >> + "gamma-correction", >> + /* contrast */ >> + "contrast", >> + /* brightness */ >> + "brightness", >> + /* hue_saturation */ >> + "hue_saturation" >> + /* add a new prop name here */ >> +}; > > I don't think you really need this array. A bunch of calls to > drm_property_create() with string literals for the property names seems > plenty clear to me. > > Ok, got it. >> + >> +int intel_clrmgr_create_color_properties(struct drm_device *dev) >> +{ >> + DRM_DEBUG_DRIVER("\n"); >> + return 0; >> +} >> + >> +void intel_clrmgr_destroy_color_properties(struct drm_device *dev) >> +{ >> + DRM_DEBUG_DRIVER("\n"); >> +} >> + >> +void intel_clrmgr_init(struct drm_device *dev) >> +{ >> + int ret; >> + >> + /* Create color properties */ >> + ret =3D intel_clrmgr_create_color_properties(dev); >> + if (ret) { >> + DRM_ERROR("Unable to create %d propert%s\n", >> + ret, ret > 1 ? "ies" : "y"); >> + return; >> + } >> + DRM_DEBUG_DRIVER("Successfully created color properties\n"); >> +} > > As I noted in reply to your cover letter, I don't think this function > really adds any value. If we rename > intel_clrmgr_create_color_properties() to something more generic and > move it to be called from intel_modeset_init(), then it will be suitable > for more than just the color management properties you're adding here. > Same goes for the corresponding cleanup function. > Same points as discussed in cover-letter. >> + >> +void intel_clrmgr_exit(struct drm_device *dev) >> +{ >> + /* Remove color properties */ >> + intel_clrmgr_destroy_color_properties(dev); >> + DRM_DEBUG_DRIVER("Destroyed color properties\n"); >> +} >> diff --git a/drivers/gpu/drm/i915/intel_clrmgr.h b/drivers/gpu/drm/i915/= intel_clrmgr.h >> new file mode 100644 >> index 0000000..1b7e906 >> --- /dev/null >> +++ b/drivers/gpu/drm/i915/intel_clrmgr.h >> @@ -0,0 +1,70 @@ >> +/* >> + * Copyright =A9 2014 Intel Corporation >> + * >> + * Permission is hereby granted, free of charge, to any person obtainin= g a >> + * copy of this software and associated documentation files (the "Softw= are"), >> + * to deal in the Software without restriction, including without limit= ation >> + * the rights to use, copy, modify, merge, publish, distribute, sublice= nse, >> + * and/or sell copies of the Software, and to permit persons to whom the >> + * Software is furnished to do so, subject to the following conditions: >> + * >> + * The above copyright notice and this permission notice (including the= next >> + * paragraph) shall be included in all copies or substantial portions o= f the >> + * Software. >> + * >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPR= ESS OR >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABIL= ITY, >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT S= HALL >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR= OTHER >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARIS= ING >> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER = DEALINGS >> + * IN THE SOFTWARE. >> + * >> + * Authors: >> + * =3D=3D=3D=3D=3D >> + * Shashank Sharma >> + * Uma Shankar >> + * Sonika Jindal >> + */ >> + >> +#ifndef _I915_CLR_MNGR_H_ >> +#define _I915_CLR_MNGR_H_ >> + >> +#include "drmP.h" >> +#include "intel_drv.h" >> +#include >> + >> +/* Framework defs */ >> +#define CLRMGR_PROP_MAX 10 >> +#define CLRMGR_PROP_NAME_MAX 128 >> +#define CLRMGR_PROP_RANGE_MAX 0xFFFFFFFFFFFFFFFF > > These are never used as far as I can see. I think you can drop them? > Yes, these were added for plane properties (contrast, brightness, hue = and sat). I missed while cleaning up. >> + >> +/* Properties */ >> +enum clrmgr_tweaks { >> + csc =3D 0, >> + gamma, >> + contrast, >> + brightness, >> + hue_saturation, >> + clrmgr_tweak_invalid >> +}; > > These are just enums into your property name array, right? I'm not > sure if we need these either. > > Actually my plan was like this: One enum(like this), which assigns color property id to each property. Arrays, arranged in order of this enum for sizes, name and init_values = of these properties. So it would be easy to access, easy to plug in new = property, and less hard coding. +/* Properties */ enum clrmgr_tweaks { csc =3D 0, gamma, contrast, brightness, hue_saturation, clrmgr_tweak_invalid }; array color_prop_sizes{size_csc, size_gamma, size_cont, size_bright}; array color_prop_name{"csc", "gamma", "cont", "bright"}; array init_values{{9 init values for CSC} {129 init values for gamma} {1 = default value of contrast} {1 default val for brightness}} Does it look that bad :) ? >> + >> +/* >> +* intel_clrmgr_init: >> +* Create drm properties for color correction >> +* Allocate memory to store current color correction >> +* input: struct drm_device * >> +*/ >> +void intel_clrmgr_init(struct drm_device *dev); >> + >> +/* >> +* intel_clrmgr_exit >> +* Destroy color correction DRM properties >> +* Free allocated memory for color correction storage >> +* Should be called from CRTC/Plane .destroy function >> +* input: >> +* struct drm_device * >> +*/ >> +void intel_clrmgr_exit(struct drm_device *dev); >> + >> +#endif >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915= /intel_display.c >> index e0beaad..df2dcbd 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -42,6 +42,7 @@ >> #include >> #include >> #include >> +#include "intel_clrmgr.h" >> >> /* Primary plane formats supported by all gen */ >> #define COMMON_PRIMARY_FORMATS \ >> @@ -12816,6 +12817,8 @@ void intel_modeset_init(struct drm_device *dev) >> INTEL_INFO(dev)->num_pipes, >> INTEL_INFO(dev)->num_pipes > 1 ? "s" : ""); >> >> + intel_clrmgr_init(dev); >> + >> for_each_pipe(dev_priv, pipe) { >> intel_crtc_init(dev, pipe); >> for_each_sprite(pipe, sprite) { >> @@ -13349,6 +13352,8 @@ void intel_modeset_cleanup(struct drm_device *de= v) >> intel_connector->unregister(intel_connector); >> } >> >> + intel_clrmgr_exit(dev); >> + >> drm_mode_config_cleanup(dev); >> >> intel_cleanup_overlay(dev); >> -- >> 1.9.1 >> >