From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matt Roper Subject: Re: [PATCH 1/4] drm/i915: Color manager framework for valleyview Date: Tue, 9 Sep 2014 18:29:04 -0700 Message-ID: <20140910012904.GD26681@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> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id C1CC189CE0 for ; Tue, 9 Sep 2014 18:27:23 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1410243796-11172-2-git-send-email-shashank.sharma@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: shashank.sharma@intel.com Cc: intel-gfx@lists.freedesktop.org, daniel.vetter@intel.com List-Id: intel-gfx@lists.freedesktop.org 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/Makefile > 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/i= ntel_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 obtaining= a > + * copy of this software and associated documentation files (the "Softwa= re"), > + * to deal in the Software without restriction, including without limita= tion > + * the rights to use, copy, modify, merge, publish, distribute, sublicen= se, > + * 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 of= the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRE= SS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILI= TY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SH= ALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR = OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISI= NG > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER D= EALINGS > + * 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. > + > +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. > + > +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/i= ntel_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 obtaining= a > + * copy of this software and associated documentation files (the "Softwa= re"), > + * to deal in the Software without restriction, including without limita= tion > + * the rights to use, copy, modify, merge, publish, distribute, sublicen= se, > + * 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 of= the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRE= SS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILI= TY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SH= ALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR = OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISI= NG > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER D= EALINGS > + * 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? > + > +/* 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. > + > +/* > +* 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 *dev) > intel_connector->unregister(intel_connector); > } > = > + intel_clrmgr_exit(dev); > + > drm_mode_config_cleanup(dev); > = > intel_cleanup_overlay(dev); > -- = > 1.9.1 > = -- = Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795