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