Re: [PATCH v2 00/10] Color Manager Implementation

[Hans Verkuil <hverkuil@xs4all.nl>]

On 06/15/2015 08:53 AM, Daniel Vetter wrote:
> On Tue, Jun 09, 2015 at 01:50:48PM +0100, Damien Lespiau wrote:
>> On Thu, Jun 04, 2015 at 07:12:31PM +0530, Kausal Malladi wrote:
>>> From: Kausal Malladi <Kausal.Malladi@intel.com>
>>>
>>> This patch set adds color manager implementation in drm/i915 layer.
>>> Color Manager is an extension in i915 driver to support color 
>>> correction/enhancement. Various Intel platforms support several
>>> color correction capabilities. Color Manager provides abstraction
>>> of these properties and allows a user space UI agent to 
>>> correct/enhance the display.
>>
>> So I did a first rough pass on the API itself. The big question that
>> isn't solved at the moment is: do we want to try to do generic KMS
>> properties for pre-LUT + matrix + post-LUT or not. "Generic" has 3 levels:
>>
>>   1/ Generic for all KMS drivers
>>   2/ Generic for i915 supported platfoms
>>   3/ Specific to each platform
>>
>> At this point, I'm quite tempted to say we should give 1/ a shot. We
>> should be able to have pre-LUT + matrix + post-LUT on CRTC objects and
>> guarantee that, when the drivers expose such properties, user space can
>> at least give 8 bits LUT + 3x3 matrix + 8 bits LUT.
>>
>> It may be possible to use the "try" version of the atomic ioctl to
>> explore the space of possibilities from a generic user space to use
>> bigger LUTs as well. A HAL layer (which is already there in some but not
>> all OSes) would still be able to use those generic properties to load
>> "precision optimized" LUTs with some knowledge of the hardware.
> 
> Yeah, imo 1/ should be doable. For the matrix we should be able to be
> fully generic with a 16.16 format. For gamma one option would be to have

I know I am late replying, apologies for that.

I've been working on CSC support for V4L2 as well (still work in progress)
and I would like to at least end up with the same low-level fixed point
format as DRM so we can share matrix/vector calculations.

Based on my experiences I have concerns about the 16.16 format: the precision
is quite low which can be a problem when such values are used in matrix
multiplications.

In addition, while the precision may be sufficient for 8 bit color component
values, I'm pretty sure it will be insufficient when dealing with 12 or 16 bit
color components.

In earlier versions of my CSC code I used a 12.20 format, but in the latest I
switched to 32.32. This fits nicely in a u64 and it's easy to extract the
integer and fractional parts.

If this is going to be a generic and future proof API, then my suggestion
would be to increase the precision of the underlying data type.

Regards,

	Hans

> an enum property listing all the supported gamma table formats, of which
> 8bit 256 entry (the current standard) would be a one. This enum space
> would need to be drm-wide ofc. Then the gamma blob would just contain the
> table. This way we can allow funky stuff like the 1025th entry for 1.0+
> values some intel tables have, and similar things.
> 
> Wrt pre-post and plan/crtc I guess we'd just add the properties to all the
> objects where they're possible on a given platform and then the driver
> must check if there's constraints (e.g. post-lut gamma only on 1 plane or
> the crtc or similar stuff).
> 
> Also there's the legacy gamma ioctl. That should forward to the crtc gamma
> (and there probably pick post lut and pre-lut only if there's no post
> lut). For names I'd suggest
> 
> "pre-gamma-type", "pre-gamma-data", "post-gamma-type" and
> "post-gamma-data" but I don't care terrible much about them.
> -Daniel
> 
>>
>> Option 3/ is, IMHO, a no-go, we should really try hard to limit the work
>> we need to do per-platform, which means defining a common format for the
>> values we give to the kernel. As stated in various places, 16.16 seems
>> the format of choice, even for the LUTs as we have wide gamut support in
>> some of the LUTs where we can map values > 1.0 to other values > 1.0.
>>
>> Another thing, the documentation of the interface needs to be a bit more
>> crisp. For instance, we don't currently define the order in which the
>> CSC and LUT transforms of this patch set are applied: is this a de-gamma
>> LUT to do the CSC in linear space? but then that means the display is
>> linear, oops. So it must be a post-CSC lut, but then we don't de-gamma
>> sRGB (not technically a single gamma power curve for sRGB, but details,
>> details) before applying a linear transform. So with this interface, we
>> have to enforce the fbs are linear, losing dynamic range. I'm sure later
>> patches would expose more properties, but as a stand-alone patch set, it
>> would seem we can't do anything useful?
>>
>> -- 
>> Damien
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx