| Message ID | 1410243796-11172-2-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 9 Sep 2014 11:53:13 +0530 <shashank.sharma@intel.com> wrote: > From: Shashank Sharma <shashank.sharma@intel.com> > > 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. I'd suggest maybe dropping the name "color manager" and "clrmgr" in favor of "color properties" and maybe "color props" as a shorter form. > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > --- > 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@intel.com> > + * Uma Shankar <uma.shankar@intel.com> > + * Sonika Jindal <sonika.jindal@intel.com> > + */ > + > +#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 need the comments for each of the names. The names themselves are descriptive. I haven't looked at the rest of the series yet, but in embedded we've considered hue and saturation separate properties. It looks like you drop this array in the second patch. You should simply not introduce it at all. Oh an then you re-introduce this array back in patch 3 but only the first part of 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"); > +} I don't really see a need for the above functions. I think just creating the properties in the init function will simplify the code a bit. Same for the exit and destroy functions. > + > +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"); > +} > + > +void intel_clrmgr_exit(struct drm_device *dev) > +{ > + /* Remove color properties */ > + intel_clrmgr_destroy_color_properties(dev); > + DRM_DEBUG_DRIVER("Destroyed color properties\n"); > +} I believe the standard practice is to introduce functioning code and not subbed functions. It makes it easier to review if the function is fleshed out when it's first introduced. > 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@intel.com> > + * Uma Shankar <uma.shankar@intel.com> > + * Sonika Jindal <sonika.jindal@intel.com> > + */ > + > +#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 > + > +/* Properties */ > +enum clrmgr_tweaks { > + csc = 0, > + gamma, > + contrast, > + brightness, > + hue_saturation, > + clrmgr_tweak_invalid > +}; > + > +/* > +* 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);
On Tue, Sep 09, 2014 at 11:53:13AM +0530, shashank.sharma@intel.com wrote: > From: Shashank Sharma <shashank.sharma@intel.com> > > 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@intel.com> > --- > 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@intel.com> > + * Uma Shankar <uma.shankar@intel.com> > + * Sonika Jindal <sonika.jindal@intel.com> > + */ > + > +#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@intel.com> > + * Uma Shankar <uma.shankar@intel.com> > + * Sonika Jindal <sonika.jindal@intel.com> > + */ > + > +#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 >
Hello Bob, Thanks for your time and review comments. Please find my comments inline. Regards Shashank On 9/10/2014 4:21 AM, Bob Paauwe wrote: > On Tue, 9 Sep 2014 11:53:13 +0530 > <shashank.sharma@intel.com> wrote: > >> From: Shashank Sharma <shashank.sharma@intel.com> >> >> 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. > > I'd suggest maybe dropping the name "color manager" and "clrmgr" in > favor of "color properties" and maybe "color props" as a shorter form. > I will try to comply with this suggestion. >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >> --- >> 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@intel.com> >> + * Uma Shankar <uma.shankar@intel.com> >> + * Sonika Jindal <sonika.jindal@intel.com> >> + */ >> + >> +#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 need the comments for each of the names. The names > themselves are descriptive. > > I haven't looked at the rest of the series yet, but in embedded we've > considered hue and saturation separate properties. > Actually they are, but in VLV hardware they are getting programmed/adjusted via the same register (single), so I am force to combine them together. > It looks like you drop this array in the second patch. You should > simply not introduce it at all. Oh an then you re-introduce this array > back in patch 3 but only the first part of it. > Actually my plan was to add an empty array, and then add names, sizes along with the corresponding property specific patches. That made a mess. >> + >> +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"); >> +} > > I don't really see a need for the above functions. I think just > creating the properties in the init function will simplify the code > a bit. Same for the exit and destroy functions. > Yes, I agree. >> + >> +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"); >> +} >> + >> +void intel_clrmgr_exit(struct drm_device *dev) >> +{ >> + /* Remove color properties */ >> + intel_clrmgr_destroy_color_properties(dev); >> + DRM_DEBUG_DRIVER("Destroyed color properties\n"); >> +} > > I believe the standard practice is to introduce functioning code and > not subbed functions. It makes it easier to review if the function is > fleshed out when it's first introduced. > Yes, actually my idea was to add empty functions, and populate them along with the corresponding property patches. For example CSC patch will come and populate create_properties, destroy_properties, attach_pipe_properties etc. But looks like this is not making it very clear. I will change this. > >> 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@intel.com> >> + * Uma Shankar <uma.shankar@intel.com> >> + * Sonika Jindal <sonika.jindal@intel.com> >> + */ >> + >> +#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 >> + >> +/* Properties */ >> +enum clrmgr_tweaks { >> + csc = 0, >> + gamma, >> + contrast, >> + brightness, >> + hue_saturation, >> + clrmgr_tweak_invalid >> +}; >> + >> +/* >> +* 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); >
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 <shashank.sharma@intel.com> >> >> 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@intel.com> >> --- >> 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@intel.com> >> + * Uma Shankar <uma.shankar@intel.com> >> + * Sonika Jindal <sonika.jindal@intel.com> >> + */ >> + >> +#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. > > 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 = 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 © 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@intel.com> >> + * Uma Shankar <uma.shankar@intel.com> >> + * Sonika Jindal <sonika.jindal@intel.com> >> + */ >> + >> +#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? > Yes, these were added for plane properties (contrast, brightness, hue and sat). I missed while cleaning up. >> + >> +/* 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. > > 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 = 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 <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 >> >
On Wed, 10 Sep 2014 14:10:01 +0530 "Sharma, Shashank" <shashank.sharma@intel.com> wrote: > Hello Bob, > > Thanks for your time and review comments. > Please find my comments inline. > > Regards > Shashank > On 9/10/2014 4:21 AM, Bob Paauwe wrote: > > On Tue, 9 Sep 2014 11:53:13 +0530 > > <shashank.sharma@intel.com> wrote: > > > >> From: Shashank Sharma <shashank.sharma@intel.com> > >> > >> 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. > > > > I'd suggest maybe dropping the name "color manager" and "clrmgr" in > > favor of "color properties" and maybe "color props" as a shorter form. > > > I will try to comply with this suggestion. > >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > >> --- > >> 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@intel.com> > >> + * Uma Shankar <uma.shankar@intel.com> > >> + * Sonika Jindal <sonika.jindal@intel.com> > >> + */ > >> + > >> +#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 need the comments for each of the names. The names > > themselves are descriptive. > > > > I haven't looked at the rest of the series yet, but in embedded we've > > considered hue and saturation separate properties. > > > Actually they are, but in VLV hardware they are getting > programmed/adjusted via the same register (single), so I am force to > combine them together. But the interface to them should be generic. You should be able to program them separately by masking out only what needs to change. At least that's what we've been doing in the embedded driver for VLV. I'm not real familiar with the this part of the code but here's what we have in our driver today. sat_hue_reg is the contents of the saturation / hue register To change the saturation: (sat_hue_reg & 0xffffc00) | (saturation_val & 0x3ff); to change the hue: (sat_hue_reg & 0xf800ffff) | ((hue_val & 0x7ff) << 16) Just looking at our code, we may have bug in the hue calculation, we limit the max hue to 0x3ff, but then do the masking with 0x7ff. One of those may be wrong and I'm not sure which. You probably have something similar to this in your user-space code today if you're passing in the actual value for the register. In general, I don't think we want user-space to have to know the format of the saturation/hue register.
On Wed, Sep 10, 2014 at 04:50:56PM +0530, Sharma, Shashank wrote: ... > >>+ > >>+/* 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. > > > > > 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 = 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 :) ? Okay, I think I see what you were going for, but I'm still not convinced that pulling these values out into separate arrays is more clear at the moment. You still have to make sure your arrays stay in sync and if there's any control flow (e.g., some properties are only relevant on certain platforms), then that gets more complicated as well. I'd suggest not using the arrays for now while we only have a handful of properties. Once we have a large list of properties in the driver maybe we can revisit whether there's a nicer way to stick them in a table and simplify the code. Matt
On Wed, Sep 10, 2014 at 02:17:02PM -0700, Matt Roper wrote: > On Wed, Sep 10, 2014 at 04:50:56PM +0530, Sharma, Shashank wrote: > ... > > >>+ > > >>+/* 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. > > > > > > > > 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 = 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 :) ? > > Okay, I think I see what you were going for, but I'm still not convinced > that pulling these values out into separate arrays is more clear at the > moment. You still have to make sure your arrays stay in sync and if > there's any control flow (e.g., some properties are only relevant on > certain platforms), then that gets more complicated as well. Yeah, aiming for too much clever structure in your code usually means you get to toss it all away again for the next generation. Some of the properties here also look generic enough that we want to make them universally known, i.e. put them into dev->mode_config so that all drivers can use them (contrast, brightness, hue_saturation and gamma imo fall into this cathegory). > I'd suggest not using the arrays for now while we only have a handful of > properties. Once we have a large list of properties in the driver maybe > we can revisit whether there's a nicer way to stick them in a table and > simplify the code. Yeah, refactoring after the fact is what I recommend when creating completely new stuff like the color management support here. It's a different story if there's already existing support infrastructure around, where it's generally a good idea to reuse them (and refactor first to make that possible, if it doesn't fit perfectly). -Daniel
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@intel.com> + * Uma Shankar <uma.shankar@intel.com> + * Sonika Jindal <sonika.jindal@intel.com> + */ + +#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 */ +}; + +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"); +} + +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@intel.com> + * Uma Shankar <uma.shankar@intel.com> + * Sonika Jindal <sonika.jindal@intel.com> + */ + +#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 + +/* Properties */ +enum clrmgr_tweaks { + csc = 0, + gamma, + contrast, + brightness, + hue_saturation, + clrmgr_tweak_invalid +}; + +/* +* 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);