[PATCH v2 01/10] drm/i915: Initialize Color Manager
On Tue, Jun 09, 2015 at 11:34:44AM +0100, Damien Lespiau wrote: > "CSC" is quite specific to how Intel calls the unit doing the 3x3 matrix > transform on colors, maybe we could be more generic there? > "color-matrix"? This also depends if we want to try to create somewhat > generic properties or not (if not, maybe we should prefix the properties > with "i915-") Actually, given the list of current properties "color-matrix" would look out of place (but seems like a good property name if you're doing a bit of CSS). People seem to go with either: - "COLOR_MATRIX", please don't :( - "color_matrix" - "color matrix" - "Color matrix" Oh well, naming rat hole... -- DAmien
[PATCH v2 01/10] drm/i915: Initialize Color Manager
On Thu, Jun 04, 2015 at 07:12:32PM +0530, Kausal Malladi wrote: > From: Kausal Malladi > > 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 > Signed-off-by: Kausal Malladi > --- > 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 > + FWIW, I'd put this in the "# modesetting core code" section. The objects are somewhat sorted by big categories, having a category saying that "intel_color_manager.c" is for "Color Management" is, well, not that useful :) > 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 000..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 > + * Kausal Malladi > + */ > + > +#include "intel_color_manager.h" > + > +void intel_color_manager_init(struct drm_device *dev) This function create "generic" properties (more on that in the cover letter) in drm_mode_config. It looks like it should be part of the DRM core, not i915? > +{ > + struct drm_mode_config *config = >mode_config; > + > + /* Create Gamma and CSC properties */ > + config->gamma_property = drm_property_create(dev, > + DRM_MODE_PROP_BLOB, "gamma_property", 0); You are creating properties with the names "gamma_property", "csc_property". While we do have (and that's quite unfortunate) a variety of naming schemes for properties none of them include the _property suffix. Just "gamma" and "csc" would be more appropriate. See the current list of properties: https://www.kernel.org/doc/htmldocs/drm/drm-kms-properties.html#idp13803344 Speaking of which, you also need to add the property to the documentation (Documentation/DocBook/drm.xml): > + if (!config->gamma_property) > + DRM_ERROR("Gamma property creation failed\n"); > + else > + DRM_DEBUG_DRIVER("Created Gamma property\n"); That's really too verbose on the logs, I don't think you need any of them. It's easy enough to check if the property was created, it's part of the API. > + > + config->csc_property = drm_property_create(dev, > + DRM_MODE_PROP_BLOB, "csc_property", 0); "CSC" is quite specific to how Intel calls the unit doing the 3x3 matrix transform on colors, maybe we could be more generic there? "color-matrix"? This also depends if we want to try to
[PATCH v2 01/10] drm/i915: Initialize Color Manager
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 >> >> 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 >> Signed-off-by: Kausal Malladi >> --- >> 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 000..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 >> + * Kausal Malladi >> + */ >> + >> +#include "intel_color_manager.h" >> + >> +void intel_color_manager_init(struct drm_device *dev) >> +{ >> +struct drm_mode_config *config = >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 000..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
[PATCH v2 01/10] drm/i915: Initialize Color Manager
On Thu, Jun 04, 2015 at 07:12:32PM +0530, Kausal Malladi wrote: > From: Kausal Malladi > > 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 > Signed-off-by: Kausal Malladi > --- > 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 000..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 > + * Kausal Malladi > + */ > + > +#include "intel_color_manager.h" > + > +void intel_color_manager_init(struct drm_device *dev) > +{ > + struct drm_mode_config *config = >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 000..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 >