On Tue, Sep 09, 2014 at 11:53:13AM +0530, shashank.sharma@xxxxxxxxx wrote: > From: Shashank Sharma <shashank.sharma@xxxxxxxxx> > > 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 <shashank.sharma@xxxxxxxxx> > --- > 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 += intel_bios.o \ > intel_modes.o \ > intel_overlay.o \ > intel_sideband.o \ > - intel_sprite.o > + intel_sprite.o \ > + intel_clrmgr.o > i915-$(CONFIG_ACPI) += intel_acpi.o intel_opregion.o > i915-$(CONFIG_DRM_I915_FBDEV) += 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 © 2014 Intel Corporation > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the "Software"), > + * to deal in the Software without restriction, including without limitation > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > + * 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, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > + * IN THE SOFTWARE. > + * > + * Authors: > + * ====== > + * Shashank Sharma <shashank.sharma@xxxxxxxxx> > + * Uma Shankar <uma.shankar@xxxxxxxxx> > + * Sonika Jindal <sonika.jindal@xxxxxxxxx> > + */ > + > +#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[] = { > + /* 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 = 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/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 © 2014 Intel Corporation > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the "Software"), > + * to deal in the Software without restriction, including without limitation > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > + * 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, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > + * IN THE SOFTWARE. > + * > + * Authors: > + * ===== > + * Shashank Sharma <shashank.sharma@xxxxxxxxx> > + * Uma Shankar <uma.shankar@xxxxxxxxx> > + * Sonika Jindal <sonika.jindal@xxxxxxxxx> > + */ > + > +#ifndef _I915_CLR_MNGR_H_ > +#define _I915_CLR_MNGR_H_ > + > +#include "drmP.h" > +#include "intel_drv.h" > +#include <linux/errno.h> > + > +/* 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 = 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 <drm/drm_plane_helper.h> > #include <drm/drm_rect.h> > #include <linux/dma_remapping.h> > +#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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx