+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@xxxxxxxxx -----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@xxxxxxxx] 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@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; Purushothaman, Vijay A; Barnes, Jesse; Vetter, Daniel Subject: Re: [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@xxxxxxxxx> > > > > 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@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
