[Intel-gfx] [PATCH 00/11]: Color manager framework for I915 driver
Sharma, Shashank
shashank.sharma at intel.com
Thu Jul 24 06:08:41 CEST 2014
Hi Daniel,
Thanks for your time and comments.
The current design is exactly same as we discussed previously in the mail chain, color manager is just the framework which does this job:
1. Create a DRM property for requesting object.
2. Attach the DRM property with the object.
There is no other job done here in the framework, no parsing and nothing else.
The color manager data structures also just add and array of DRM properties for an object (CRTC/PIPE) and total no of DRM properties.
So there is nothing which is not required.
Typical sequence of how it works:
1. intel CRTC init calls color-manager init for that CRTC, gets a color pointer, which has space to save DRM properties.
2. intel CRTC init calls attach color properties, which will register the DRM property, add into the color pointer, and return.
3. A CRTC set property checks if this is color property, calls color-manager-set-property.
4. Color manager set-property calls core set property function, which takes care of calling platform specific set_propety function.
5. Color manager exit gets call from CRTC/plane destroy, and frees the DRM property per CRTC/plane, plus color pointer.
Can you please point out, which of the above steps is not falling in line for you?
The current gamma implementation is in 8bit gamma format, which can be only used to initialize the lut, it doesn't do the gamma correction.
The actual correction is done in 10bit gamma correction mode, which is more accurate and can support fraction level gamma correction.
Please find the detail explanation per comment, inline.
Regards
Shashank
-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel Vetter
Sent: Thursday, July 24, 2014 12:04 AM
To: Sharma, Shashank
Cc: intel-gfx at lists.freedesktop.org; ville.syrjala at linux.intel.com; Lespiau, Damien; Vetter, Daniel; Kumar, Shobhit; M, Satheeshakrishna; =indranil.mukherjee at intel.com
Subject: Re: [Intel-gfx] [PATCH 00/11]: Color manager framework for I915 driver
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.
Shashank: This is exactly what we are doing in color manager, no extra work at all.
Please note that CSC and Gamma can be only supported on PIPE, so has to be a CRTC property.
Hue, Brightness, Contrast are only applicable for sprite planes, so it has to be Plane property.
All we have done is, create an array of such properties, segregate it in PIPE and Plane basis, and register with every pipe/plane during init time.
The reason why this is in a new file is:
1. We don't want to over populate the 12k lines file intel_display.c
2. This file has a huge scope to expand, as we are going to support all the gen devices, plugging in their SOC specific color correction. For example, the
Gamma correction method is different for VLV and other devices. We will sooner add another function for those devices, and just plug-in that
under a if check, say if (device_X), call this function. So we want all color corrections in a separate place, which makes sense to us.
> 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.
Shashank: As I mentioned, there are many reasons to do this.
1. Existing gamma correction is for 8 bit gamma correction only, which can be utilized to initialize the LUT, not to apply gamma correction.
For accurate gamma correction, we need 10bit/12 bit gamma corrections, based on availability of platform.
2. Existing gamma interface expects 256 8bit correction values per channel, whereas 10bit and 12 bit gamma correction need 16 bits (10 integer and 6 fraction bits) or more.
The programming method of palette registers is also totally different, which expects values in an even:odd register configuration , writing 2 registers per gamma values.
So these two are totally different in all possible ways, and IMHO we should deprecate the previous IOCTL interface, or should just use it for LUT initialization. It doesn't do gamma
correction.
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.
Shashank: The current color manager implementation takes care of that already. Right now, in gen_6_color_correction_properties, we are giving a table which is suitable for VLV based HW.
We can always add a GEN_X_color_correction_properties, with different no of gamma palette entries (1024), and pick that based on GEN basis. This can't be don't in the existing IOCTL.
> 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).
Shashank: On VLV, 2 of these properties (CSC and GAMMA) can be supported on PIPE level, and rest 3 (HS, Contrast and Brightness) are only on Sprite level, they can't be applied on the PIPE level.
So all of them can't be classified the same. Hence, we are having CSC and Gamma on CRTC and, and other three on Plane.
Plus, this might not be true for all SOCs, and they can show different levels of color properties. In the current design, all you have to do is list all your PIPE leve properties at one place, and that will be registered with CRTC
and plane level properties at other place, which will automatically be registered as Plane property.
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.
Shashank: Can you please elaborate what do you mean by real property ?
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.
Shashank: This already is taken care in the current architecture.
- 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
More information about the Intel-gfx
mailing list