Re: [PATCH 1/4] drm/i915: Color manager framework for valleyview

[Matt Roper <matthew.d.roper@intel.com>]

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
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795