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

From: Damien Lespiau <damien.lespiau@intel.com>
To: Kausal Malladi <Kausal.Malladi@intel.com>
Cc: annie.j.matheson@intel.com, dhanya.p.r@intel.com,
	intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	vijay.a.purushothaman@intel.com, jesse.barnes@intel.com,
	daniel.vetter@intel.com
Subject: Re: [PATCH v2 00/10] Color Manager Implementation
Date: Tue, 9 Jun 2015 13:50:48 +0100	[thread overview]
Message-ID: <20150609125047.GF1671@strange.amr.corp.intel.com> (raw)
In-Reply-To: <1433425361-17864-1-git-send-email-Kausal.Malladi@intel.com>

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.

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