[Intel-gfx] [PATCH 1/4] drm/i915: Color manager framework for valleyview
Matt Roper
matthew.d.roper at intel.com
Wed Sep 10 03:29:04 CEST 2014
On Tue, Sep 09, 2014 at 11:53:13AM +0530, shashank.sharma at intel.com wrote:
> From: Shashank Sharma <shashank.sharma at 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 at 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 at intel.com>
> + * Uma Shankar <uma.shankar at intel.com>
> + * Sonika Jindal <sonika.jindal at 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 at intel.com>
> + * Uma Shankar <uma.shankar at intel.com>
> + * Sonika Jindal <sonika.jindal at 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
>
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
More information about the Intel-gfx
mailing list