Re: [Intel-gfx] [PATCH 1/4] drm/i915: Color manager framework for valleyview
On Wed, Sep 10, 2014 at 02:17:02PM -0700, Matt Roper wrote:
On Wed, Sep 10, 2014 at 04:50:56PM +0530, Sharma, Shashank wrote:
...
+
+/* 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 :) ?
Okay, I think I see what you were going for, but I'm still not convinced
that pulling these values out into separate arrays is more clear at the
moment. You still have to make sure your arrays stay in sync and if
there's any control flow (e.g., some properties are only relevant on
certain platforms), then that gets more complicated as well.
Yeah, aiming for too much clever structure in your code usually means you
get to toss it all away again for the next generation. Some of the
properties here also look generic enough that we want to make them
universally known, i.e. put them into dev-mode_config so that all drivers
can use them (contrast, brightness, hue_saturation and gamma imo fall into
this cathegory).
I'd suggest not using the arrays for now while we only have a handful of
properties. Once we have a large list of properties in the driver maybe
we can revisit whether there's a nicer way to stick them in a table and
simplify the code.
Yeah, refactoring after the fact is what I recommend when creating
completely new stuff like the color management support here. It's a
different story if there's already existing support infrastructure around,
where it's generally a good idea to reuse them (and refactor first to make
that possible, if it doesn't fit perfectly).
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm/i915: Color manager framework for valleyview
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.sha...@intel.com wrote:
From: Shashank Sharma shashank.sha...@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.sha...@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 000..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.sha...@intel.com
+ * Uma Shankar uma.shan...@intel.com
+ * Sonika Jindal sonika.jin...@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
Re: [Intel-gfx] [PATCH 1/4] drm/i915: Color manager framework for valleyview
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.sha...@intel.com wrote:
From: Shashank Sharma shashank.sha...@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.sha...@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 000..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.sha...@intel.com
+ * Uma Shankar uma.shan...@intel.com
+ * Sonika Jindal sonika.jin...@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
Re: [Intel-gfx] [PATCH 1/4] drm/i915: Color manager framework for valleyview
On Wed, 10 Sep 2014 14:10:01 +0530
Sharma, Shashank shashank.sha...@intel.com wrote:
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.sha...@intel.com wrote:
From: Shashank Sharma shashank.sha...@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.sha...@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 000..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.sha...@intel.com
+ * Uma Shankar uma.shan...@intel.com
+ * Sonika Jindal sonika.jin...@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.
But the interface to them
Re: [Intel-gfx] [PATCH 1/4] drm/i915: Color manager framework for valleyview
On Wed, Sep 10, 2014 at 04:50:56PM +0530, Sharma, Shashank wrote:
...
+
+/* 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 :) ?
Okay, I think I see what you were going for, but I'm still not convinced
that pulling these values out into separate arrays is more clear at the
moment. You still have to make sure your arrays stay in sync and if
there's any control flow (e.g., some properties are only relevant on
certain platforms), then that gets more complicated as well.
I'd suggest not using the arrays for now while we only have a handful of
properties. Once we have a large list of properties in the driver maybe
we can revisit whether there's a nicer way to stick them in a table and
simplify the code.
Matt
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling Development
Intel Corporation
(916) 356-2795
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/4] drm/i915: Color manager framework for valleyview
From: Shashank Sharma shashank.sha...@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.sha...@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 000..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.sha...@intel.com
+ * Uma Shankar uma.shan...@intel.com
+ * Sonika Jindal sonika.jin...@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 */
+};
+
+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);
+}
+
+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 000..1b7e906
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_clrmgr.h
@@ -0,0 +1,70 @@
+/*
+ * Copyright © 2014 Intel
Re: [Intel-gfx] [PATCH 1/4] drm/i915: Color manager framework for valleyview
On Tue, 9 Sep 2014 11:53:13 +0530
shashank.sha...@intel.com wrote:
From: Shashank Sharma shashank.sha...@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.
Signed-off-by: Shashank Sharma shashank.sha...@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 000..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.sha...@intel.com
+ * Uma Shankar uma.shan...@intel.com
+ * Sonika Jindal sonika.jin...@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.
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.
+
+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
Re: [Intel-gfx] [PATCH 1/4] drm/i915: Color manager framework for valleyview
On Tue, Sep 09, 2014 at 11:53:13AM +0530, shashank.sha...@intel.com wrote:
From: Shashank Sharma shashank.sha...@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.sha...@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 000..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.sha...@intel.com
+ * Uma Shankar uma.shan...@intel.com
+ * Sonika Jindal sonika.jin...@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.
+
+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
