Re: [PATCH v2 01/10] drm/i915: Initialize Color Manager

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thanks for your time and review Matt.
Please find my comments inline

Regards
Shashank
On 6/6/2015 6:30 AM, Matt Roper wrote:
On Thu, Jun 04, 2015 at 07:12:32PM +0530, Kausal Malladi wrote:
From: Kausal Malladi <Kausal.Malladi@xxxxxxxxx>

Color Manager is an extension in i915 driver to handle color correction
and enhancements across various Intel platforms.

This patch initializes color manager framework by :
1. Adding two new files, intel_color_manager(.c/.h)
2. Introducing new pointers in DRM mode_config structure to
    carry CSC and Gamma color correction properties.
3. Creating these DRM properties in Color Manager initialization
    sequence.

v2: Addressing Sonika's review comment.
1. Made intel_color_manager_init void
2. Moved init after NUM_PIPES check

Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxxxx>
Signed-off-by: Kausal Malladi <Kausal.Malladi@xxxxxxxxx>
---
  drivers/gpu/drm/i915/Makefile              |  3 ++
  drivers/gpu/drm/i915/intel_color_manager.c | 48 ++++++++++++++++++++++++++++++
  drivers/gpu/drm/i915/intel_color_manager.h | 32 ++++++++++++++++++++
  drivers/gpu/drm/i915/intel_display.c       |  3 ++
  include/drm/drm_crtc.h                     |  4 +++
  5 files changed, 90 insertions(+)
  create mode 100644 drivers/gpu/drm/i915/intel_color_manager.c
  create mode 100644 drivers/gpu/drm/i915/intel_color_manager.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index b7ddf48..c62d048 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -89,6 +89,9 @@ i915-y += i915_vgpu.o
  # legacy horrors
  i915-y += i915_dma.o

+# Color Management
+i915-y += intel_color_manager.o
+
  obj-$(CONFIG_DRM_I915)  += i915.o

  CFLAGS_i915_trace_points.o := -I$(src)
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
new file mode 100644
index 0000000..f7e2380
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -0,0 +1,48 @@
+/*
+ * Copyright © 2015 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>
+ * Kausal Malladi <Kausal.Malladi@xxxxxxxxx>
+ */
+
+#include "intel_color_manager.h"
+
+void intel_color_manager_init(struct drm_device *dev)
+{
+	struct drm_mode_config *config = &dev->mode_config;
+
+	/* Create Gamma and CSC properties */
+	config->gamma_property = drm_property_create(dev,
+		DRM_MODE_PROP_BLOB, "gamma_property", 0);
+	if (!config->gamma_property)
+		DRM_ERROR("Gamma property creation failed\n");
+	else
+		DRM_DEBUG_DRIVER("Created Gamma property\n");
+
+	config->csc_property = drm_property_create(dev,
+			DRM_MODE_PROP_BLOB, "csc_property", 0);
+	if (!config->csc_property)
+		DRM_ERROR("CSC property creation failed\n");
+	else
+		DRM_DEBUG_DRIVER("Created CSC property\n");
+}
diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
new file mode 100644
index 0000000..154bf16
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_color_manager.h
@@ -0,0 +1,32 @@
+/*
+ * Copyright © 2015 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>
+ * Kausal Malladi <Kausal.Malladi@xxxxxxxxx>
+ */
+
+#include <drm/drmP.h>
+#include <drm/drm_crtc_helper.h>
+
+/* Generic Function prototypes */
+void intel_color_manager_init(struct drm_device *dev);

While I personally don't mind creating a new header for prototypes, most
of our other KMS-related stuff just gets thrown in intel_drv.h under a
comment like "/* intel_foobar.c */" so this is a little inconsistent.
Maybe we should keep these prototypes there as well since that's the
file most developers are going to look for these in out of habit?

Yes sure. Actually there are a lot of macros coming up in the next set of patches, which are only specific to color management (CSC gamma hue, saturation, brightness and contrast), which creates a necessity of a new header file, so we though why don't we put the prototypes also here. But if you think that we should move it, we can very well do that.

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 067b1de..2322dee 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -44,6 +44,7 @@
  #include <drm/drm_plane_helper.h>
  #include <drm/drm_rect.h>
  #include <linux/dma_remapping.h>
+#include "intel_color_manager.h"

  /* Primary plane formats for gen <= 3 */
  static const uint32_t i8xx_primary_formats[] = {
@@ -14673,6 +14674,8 @@ void intel_modeset_init(struct drm_device *dev)
  	if (INTEL_INFO(dev)->num_pipes == 0)
  		return;

+	intel_color_manager_init(dev);
+
  	intel_init_display(dev);
  	intel_init_audio(dev);

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 3b4d8a4..2a75d7d 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1176,6 +1176,10 @@ struct drm_mode_config {
  	struct drm_property *suggested_x_property;
  	struct drm_property *suggested_y_property;

+	/* Color Management Properties */
+	struct drm_property *gamma_property;
+	struct drm_property *csc_property;
+

I notice you're adding these to a core DRM structure; is the expectation
that other drivers will want to re-use these properties for their own
color correction functionality?

If the answer is yes, you'll definitely need to add documentation for
the property to Documentation/DocBook/drm.tmpl which will get compiled
into documentation like you see at
http://people.freedesktop.org/~danvet/drm/drm-kms-properties.html#idp59563120
(actually you'll want to add that documentation even if it's an
i915-specific property, but it's especially important for anything we
expect to be reusable by other drivers).

If the answer is no, and you expect this to be i915-specific for the
foreseeable future, then stashing the property in dev_priv might be a
better choice to avoid growing the driver-independent structures.

Well, actually the intention is to have a generic core property which can encourage other drivers also, to use this interface. We will update the documentation accordingly.

Matt

  	/* dumb ioctl parameters */
  	uint32_t preferred_depth, prefer_shadow;

--
2.4.2


_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux