[Intel-gfx] [PATCH 1/4] drm/i915: Color manager framework for valleyview
Sharma, Shashank
shashank.sharma at intel.com
Wed Sep 10 13:20:56 CEST 2014
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 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.
>
>
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 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?
>
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
>>
>
More information about the Intel-gfx
mailing list