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@xxxxxxxxx wrote:
From: Shashank Sharma <shashank.sharma@xxxxxxxxx>
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@xxxxxxxxx>
---
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@xxxxxxxxx>
+ * Uma Shankar <uma.shankar@xxxxxxxxx>
+ * Sonika Jindal <sonika.jindal@xxxxxxxxx>
+ */
+
+#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@xxxxxxxxx>
+ * Uma Shankar <uma.shankar@xxxxxxxxx>
+ * Sonika Jindal <sonika.jindal@xxxxxxxxx>
+ */
+
+#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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
