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

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

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