On Wed, Jul 23, 2014 at 11:34:54PM +0530, shashank.sha...@intel.com wrote: > From: Shashank Sharma <shashank.sha...@intel.com> > > This patchset adds color-manager, a new framework in I915 driver which > adds color correction and tweak capabilities in the driver. > > Color manager creates a DRM propery based interface for each color > correction, and based on the property type, registers it with each > CRTC/plane available. > > The current implementation is for valleyview family. > Valleyview supports following color correction properties: > 1. CSC correction (wide gamut): This is a pipe level correction. > There are total 9 correction coefficients in form of a 3x3 matrix, > which are to be programmed on 6 correction registers. CSC correction > > 2. Gamma correction: This is also pipe level correction > There are total 256 palette registers, which can be programmed with > 128 correction values, in 10.6 (10bit) format. The expected color > Correction can be applied using 129, 64 bit correction values. > First 128 correction values are to program palette, 129th value is for > GCMAX register value. > correction format in a 64 bit value is: > | <16 higher bits>| <16bit R value>|<16 bit G value>|<16 bit B value>| > > 3. Contrast: This is sprite plane level correction > Expected correction value is 9 bit value > Driver expects values in this format: > |bits 64:32 | bits 31:9 | 8:0 contrast correction value| > > 4. Brightness: This is also a sprite level correction > Expected correction value is 8 bit value > Driver expects values in this format: > |bits 64:32 | bits 31:8 | 7:0 9 bit brightness correction value| > > 5. Hue and saturation: This is also a sprite level correction > Expected correction value is 32 bit value > Driver expects values in this format: > |bits 64:32| bits 31:0 hs correction value| > > Patches: > 1. First three patches create the basic framework. > 2. Next 4 add functions to do color correction per property. > 3. Next 2 add interface to set property. > 4. last 2 patches plug-in init and exit in modeset sequences. > > Shashank Sharma (11): > drm/i915: Color manager framework for valleyview > drm/i915: Register pipe level color properties > drm/i915: Register plane level color properties
So you're now using drm properties instead of a private ioctl, which is good. But there's still this entire indirection through the color manager and I don't understand it at all and what it serves us. From what I can see it only makes the code more complex. The basic drm approach for properties is - Create the property once per device. If we standardize it, then it should be created in drm_device->mode_config, otherwise at a suitable place in our driver private structure. - Link up that property with the right kms objects in the relevant init functions. That needs a pointer on each object to store that association. - Match for these properties in the set_prop callback. Please explain why is not sufficient with this basic&straightforward approach and what the color manager adds in value to this. Thus far I think we can just rip out the color manager without loss of functionality. > drm/i915: Add color manager CSC correction > drm/i915: Add color manager gamma correction We already have gamma support in core drm, so we shouldn't duplicate things. There's a few things we need to do though: - Convert gamma to be a property from the special-purpose ioctl it is currently. - Add an enum property with all the gamma table formats support. Default would be the 256 entry 8 bit table. - Add new table layout for vlv. - Implement the new layout and switch between high-res and legacy gamma table appropriately. Note that the gamma table should be flexible enough to also support the high-res gamma tables on big cores. Or any other gpu fwiw. > drm/i915: Add contrast and brightness correction > drm/i915: Add hue and saturation correction These 4 properties should imo all be drm core properties registered in drm_device->mode_config, with a setup function for all four. Also the commit message still talk about the old design where you split up a 64 bit value. That's highly confusing. And all the indirection needs to be ripped out (see my above comment about the color manager). Finally we already have a few sprite properties exposed as ioctls on vlv. I'd like those to be converted to real properties as part of this work if possible. For a suitable merge stragety I think it would be good to split up this work into different parts: - Convert existing ioctl properties. - Gamma table improvements. - Color adjustements (brightness, contrast, hue, saturation). - CSC. Cheers, Daniel > drm/i915: Add CRTC set property functions > drm/i915: Add set plane property functions > drm/i915: Plug-in color manager init > drm/i915: Plug-in color manager exit > > drivers/gpu/drm/i915/Makefile | 3 +- > drivers/gpu/drm/i915/i915_reg.h | 22 + > drivers/gpu/drm/i915/intel_clrmgr.c | 795 > +++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_clrmgr.h | 282 +++++++++++++ > drivers/gpu/drm/i915/intel_display.c | 50 +++ > drivers/gpu/drm/i915/intel_drv.h | 6 + > drivers/gpu/drm/i915/intel_sprite.c | 45 ++ > 7 files changed, 1202 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/i915/intel_clrmgr.c > create mode 100644 drivers/gpu/drm/i915/intel_clrmgr.h > > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- 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
