[Intel-gfx] [PATCH 00/11]: Color manager framework for I915 driver

[Daniel Vetter]

On Wed, Jul 23, 2014 at 11:34:54PM +0530, shashank.sharma at intel.com wrote:
> From: Shashank Sharma <shashank.sharma at 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 at 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