Re: [v3 0/7] Add Multi Segment Gamma Support

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Apr 18, 2019 at 04:11:58PM +0300, Ville Syrjälä wrote:
> On Thu, Apr 18, 2019 at 09:13:04AM +0200, Daniel Vetter wrote:
> > On Wed, Apr 17, 2019 at 02:57:31PM +0300, Ville Syrjälä wrote:
> > > On Wed, Apr 17, 2019 at 09:28:19AM +0200, Daniel Vetter wrote:
> > > > On Fri, Apr 12, 2019 at 03:50:56PM +0530, Uma Shankar wrote:
> > > > > This series adds support for programmable gamma modes and
> > > > > exposes a property interface for the same. Also added,
> > > > > support for multi segment gamma mode introduced in ICL+
> > > > >
> > > > > It creates GAMMA_MODE property interface. This is an enum
> > > > > property with values as blob_id's and exposes
> > > > > the various gamma modes supported and the lut ranges  Getting the
> > > > > blob id in userspace, user can get the mode supported and
> > > > > also the range of gamma mode supported with number of lut
> > > > > coefficients. It can then set one of the modes using this
> > > > > enum property.
> > > > >
> > > > > Lut values will be sent through already available GAMMA_LUT
> > > > > blob property.
> > > > >
> > > > > It also introduces a CLIENT CAP for advanced GAMMA_MODE.
> > > > >  This is for user to set the and use advance gamma mode and older
> > > > > userspace can continue using the legacy paths.
> > > > >
> > > > > v2: Used Ville's design and approach to define the interfaces.
> > > > > Addressed Matt Roper's review feedback and re-ordered the
> > > > > patches.
> > > > >
> > > > > v3: Converged to 1 property interface and introduced a Client cap
> > > > > as suggested by Ville. Fixed review comments received.
> > > > >
> > > > > Uma Shankar (5):
> > > > >   drm/i915/icl: Add register definitions for Multi Segmented gamma
> > > > >   drm/i915/icl: Add support for multi segmented gamma mode
> > > > >   drm/i915: Attach gamma mode property
> > > > >   drm: Add Client Cap for advance gamma mode
> > > > >   drm/i915: Enable advance gamma mode
> > > > >
> > > > > Ville Syrjälä (2):
> > > > >   drm: Add gamma mode property
> > > > >   drm/i915: Define color lut range structure
> > > >
> > > > Bunch of higher level comments after some internal discussions:
> > > >
> > > > - we need the userspace for this, can't design new uapi without involving
> > > >   the compositor folks for hdr.
> > > >
> > > > - single property doesn't work: Once userspace has set it, the old blob
> > > >   property with the list of all options is gone. We need one read-only
> > > >   property for the list of options, plus a 2nd property that userspace can
> > > >   set. This is a general rule for more complex properties, where the usual
> > > >   property metadata isn't enough to describe the possible options.
> > >
> > > I guess no one understood my blob_enum idea? It's an enum where each
> > > possible value is a blob. The only thing that changes is the current
> > > value (which can only point to one of the enumerated blobs).
> > 
> > Uh yes that's not clear at all, and if we do go with this, I guess we
> > should have a pile of core code to make sure it validates and is
> > consistent.
> > 
> > >> > - no caps for properties. Yes that gives us a theoretical problem, no in
> > > >   practice it doesn't matter, since people don't even care enough to make
> > > >   e.g. fbdev resetting work today for everything. Long form discussion,
> > > >   see here:
> > > >
> > > >   https://blog.ffwll.ch/2016/01/vt-switching-with-atomic-modeset.html
> > > >
> > > >   Nothing happened in this area ever since I typed this up, so I guess
> > > >   it's really not a real-world concern.
> > > >
> > > > - Simplest path forward would be if we accept different LUT sizes than the
> > > >   one advertised (we already do that for legacy gamma, and this is
> > > >   officially what we had in mind too), and the kernel automatically picks
> > > >   the best lut configuration. Will be somewhat awkard for the
> > > >   multi-segment lut, but would decouple the uapi discussion a bit.
> > >
> > > It'll be ridiculously wasteful. IIRC we need a LUT with 32768 entries,
> > > and then ~98% of those gets thrown away and never programmed to the
> > > hardware.
> > 
> > Yeah it's a few MB, not that awesome really ...
> > 
> > > > - Frankly the uapi proposed looks like fake generic - it tries to model
> > > >   all possibilities in a generic way, when really userspace needs to have
> > > >   special code for special pipelines.
> > >
> > > I think it can be used pretty easily. Userspace just has to decide
> > > whether it wants a straight up LUT or whether an interpolated curve
> > > is enough, and how much precision it needs. For x11 the logic would
> > > be simple enough: 1. look for straight up LUT with num_entries >= 1<<bpc,
> > > if that isn't found fall back to an interpolated curve with >= 1<<bpc
> > > precision, and finally just fall back to whatever gives the best
> > > results I suppose.
> > 
> > Hm, there's also a bunch more defines about mirroring and non-negative and
> > other stuff that I have no idea how userspace should use it. I do think
> > some "here's the possible configs for color management" thing is needed,
> > I'm not sure userspace can do much with all the details provided in the
> > current series.
> 
> The negative values would represent out of gamut colors, which
> can definitely happen with xvYCC. Looks like the v4l folks
> also considered this in their transfer func docs [1], which
> are specifying the formulas extended for <0.0 values.
> 
> Also based on my chat with Ilia on irc at some point I got the
> impression that nv hardware may have gamma LUTs which support
> negative values without this mirroring trick. Hence I wanted to
> make all the numbers signed rather than unsigned.
> 
> We could of course leave a bunch of these advanced things 
> undefined until an actual use case comes around. But I wanted to
> include it all in my initial proposal so that we could be more
> confident that we're not painting ourselves in a corner that
> would require yet another uapi to escape.

Hm good point. I just feel like with just one driver (at least in kms) it
might be a bit early to go all in on color manager v2.0. From a kms
ecosystem pov we're still pretty busy rolling out the first one. But if we
need it now, I guess we'll need it now ...

> [1] https://www.linuxtv.org/downloads/v4l-dvb-apis-old/ch02s06.html
> 
> -- 
> Ville Syrjälä
> Intel
> ---------------------------------------------------------------------
> Intel Finland Oy
> Registered Address: PL 281, 00181 Helsinki 
> Business Identity Code: 0357606 - 4 
> Domiciled in Helsinki 
> 
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.

Wrong mail address :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel