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@intel.com

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@intel.com Signed-off-by: Kausal Malladi Kausal.Malladi@intel.com

---

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@intel.com
• • Kausal Malladi Kausal.Malladi@intel.com
• */

+#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@intel.com
• • Kausal Malladi Kausal.Malladi@intel.com
• */

+#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#idp5956312... (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