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

["Sharma, Shashank" <shashank.sharma@intel.com>]

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
>>
>