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

From: "Sharma, Shashank" <shashank.sharma@intel.com>
To: "Matheson, Annie J" <annie.j.matheson@intel.com>,
	Daniel Vetter <daniel@ffwll.ch>,
	"Lespiau, Damien" <damien.lespiau@intel.com>,
	"Bhattacharjee, Susanta" <susanta.bhattacharjee@intel.com>
Cc: "intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"Purushothaman, Vijay A" <vijay.a.purushothaman@intel.com>,
	"Barnes, Jesse" <jesse.barnes@intel.com>,
	"Vetter, Daniel" <daniel.vetter@intel.com>,
	"R, Dhanya p" <dhanya.p.r@intel.com>
Subject: Re: [PATCH v2 00/10] Color Manager Implementation
Date: Tue, 16 Jun 2015 03:12:09 +0000	[thread overview]
Message-ID: <FF3DDC77922A8A4BB08A3BC48A1EA8CB099422B7@BGSMSX101.gar.corp.intel.com> (raw)
In-Reply-To: <9B2DE66A66248547A306BC19587A3F785412EC2A@ORSMSX105.amr.corp.intel.com>

Hi Annie, 

I missed this comment (I was missing from to/cc list, so my filter did the trick :))
Overall, comments from Daniel looks good, and is pretty much aligned to what we are trying to do (1. Generic for all KMS drivers).

If you check the latest design, we are also planning to expose a read-only property, to userspace, to expose all the color capabilities. 
It's similar to what Danvet suggested,  we are just doing it for all color properties, instead of just for gamma. 
Userspace can read this blob property, and extract all the color capabilities of the platform (gamma, CSC, degamma) and decide which one to opt for.

I would again recommend all to go through the document (only the how to query / get / set section would be enough), and let us know if we need a change at this level.

Regards
Shashank
-----Original Message-----
From: Matheson, Annie J 
Sent: Tuesday, June 16, 2015 2:00 AM
To: Daniel Vetter; Lespiau, Damien; Sharma, Shashank; Bhattacharjee, Susanta
Cc: Malladi, Kausal; R, Dhanya p; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Purushothaman, Vijay A; Barnes, Jesse; Vetter, Daniel
Subject: RE: [Intel-gfx] [PATCH v2 00/10] Color Manager Implementation

+Susanta/Shashank

How does this review from Daniel sound to you guys? I know you've ask for the Display team to review the latest design doc before you start the external communication and there's been some discussion below...

Thanks. 

Annie Matheson
Intel Corporation
Phone: (503) 712-0586
Email: annie.j.matheson@intel.com

-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Sunday, June 14, 2015 11:53 PM
To: Lespiau, Damien
Cc: Malladi, Kausal; Matheson, Annie J; R, Dhanya p; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Purushothaman, Vijay A; Barnes, Jesse; Vetter, Daniel
Subject: Re: [Intel-gfx] [PATCH v2 00/10] Color Manager Implementation

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 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

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx