mbox series

[v3,0/7] Add Multi Segment Gamma Support

Message ID 1555064463-18479-1-git-send-email-uma.shankar@intel.com (mailing list archive)
Headers show
Series Add Multi Segment Gamma Support | expand

Message

Uma Shankar April 12, 2019, 10:20 a.m. UTC
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

 drivers/gpu/drm/drm_atomic_uapi.c    |   8 +
 drivers/gpu/drm/drm_color_mgmt.c     |  77 ++++
 drivers/gpu/drm/drm_ioctl.c          |   5 +
 drivers/gpu/drm/i915/i915_reg.h      |  17 +
 drivers/gpu/drm/i915/intel_color.c   | 735 ++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_display.c |   3 +
 include/drm/drm_atomic.h             |   1 +
 include/drm/drm_color_mgmt.h         |   8 +
 include/drm/drm_crtc.h               |  17 +
 include/drm/drm_file.h               |   8 +
 include/drm/drm_mode_config.h        |   6 +
 include/uapi/drm/drm.h               |   2 +
 include/uapi/drm/drm_mode.h          |  38 ++
 13 files changed, 918 insertions(+), 7 deletions(-)

Comments

Daniel Vetter April 17, 2019, 7:28 a.m. UTC | #1
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.

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

- 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. To me this feels like the pixel
  modifier discussion all over, where we had multi-year discussions on
  trying to describe everything in generic terms or just have fairly
  opaque enumeration of special cases. Both approaches have been tried.
  For this I'm leaning towards the opaque color pipeline description for
  the more fancy stuff.

  Either way, settling on the right uapi will take some time, and will
  need a pile of people to be involved.

Cheers, Daniel

> 
>  drivers/gpu/drm/drm_atomic_uapi.c    |   8 +
>  drivers/gpu/drm/drm_color_mgmt.c     |  77 ++++
>  drivers/gpu/drm/drm_ioctl.c          |   5 +
>  drivers/gpu/drm/i915/i915_reg.h      |  17 +
>  drivers/gpu/drm/i915/intel_color.c   | 735 ++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_display.c |   3 +
>  include/drm/drm_atomic.h             |   1 +
>  include/drm/drm_color_mgmt.h         |   8 +
>  include/drm/drm_crtc.h               |  17 +
>  include/drm/drm_file.h               |   8 +
>  include/drm/drm_mode_config.h        |   6 +
>  include/uapi/drm/drm.h               |   2 +
>  include/uapi/drm/drm_mode.h          |  38 ++
>  13 files changed, 918 insertions(+), 7 deletions(-)
> 
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Ville Syrjälä April 17, 2019, 11:57 a.m. UTC | #2
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).

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

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

> To me this feels like the pixel
>   modifier discussion all over, where we had multi-year discussions on
>   trying to describe everything in generic terms or just have fairly
>   opaque enumeration of special cases. Both approaches have been tried.
>   For this I'm leaning towards the opaque color pipeline description for
>   the more fancy stuff.
> 
>   Either way, settling on the right uapi will take some time, and will
>   need a pile of people to be involved.
> 
> Cheers, Daniel
> 
> > 
> >  drivers/gpu/drm/drm_atomic_uapi.c    |   8 +
> >  drivers/gpu/drm/drm_color_mgmt.c     |  77 ++++
> >  drivers/gpu/drm/drm_ioctl.c          |   5 +
> >  drivers/gpu/drm/i915/i915_reg.h      |  17 +
> >  drivers/gpu/drm/i915/intel_color.c   | 735 ++++++++++++++++++++++++++++++++++-
> >  drivers/gpu/drm/i915/intel_display.c |   3 +
> >  include/drm/drm_atomic.h             |   1 +
> >  include/drm/drm_color_mgmt.h         |   8 +
> >  include/drm/drm_crtc.h               |  17 +
> >  include/drm/drm_file.h               |   8 +
> >  include/drm/drm_mode_config.h        |   6 +
> >  include/uapi/drm/drm.h               |   2 +
> >  include/uapi/drm/drm_mode.h          |  38 ++
> >  13 files changed, 918 insertions(+), 7 deletions(-)
> > 
> > -- 
> > 1.9.1
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Daniel Vetter April 18, 2019, 7:13 a.m. UTC | #3
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.
-Daniel

-Daniel

> > To me this feels like the pixel
> >   modifier discussion all over, where we had multi-year discussions on
> >   trying to describe everything in generic terms or just have fairly
> >   opaque enumeration of special cases. Both approaches have been tried.
> >   For this I'm leaning towards the opaque color pipeline description for
> >   the more fancy stuff.
> >
> >   Either way, settling on the right uapi will take some time, and will
> >   need a pile of people to be involved.
> >
> > Cheers, Daniel
> >
> > >
> > >  drivers/gpu/drm/drm_atomic_uapi.c    |   8 +
> > >  drivers/gpu/drm/drm_color_mgmt.c     |  77 ++++
> > >  drivers/gpu/drm/drm_ioctl.c          |   5 +
> > >  drivers/gpu/drm/i915/i915_reg.h      |  17 +
> > >  drivers/gpu/drm/i915/intel_color.c   | 735 ++++++++++++++++++++++++++++++++++-
> > >  drivers/gpu/drm/i915/intel_display.c |   3 +
> > >  include/drm/drm_atomic.h             |   1 +
> > >  include/drm/drm_color_mgmt.h         |   8 +
> > >  include/drm/drm_crtc.h               |  17 +
> > >  include/drm/drm_file.h               |   8 +
> > >  include/drm/drm_mode_config.h        |   6 +
> > >  include/uapi/drm/drm.h               |   2 +
> > >  include/uapi/drm/drm_mode.h          |  38 ++
> > >  13 files changed, 918 insertions(+), 7 deletions(-)
> > >
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>
> --
> 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.
>

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Ville Syrjälä April 18, 2019, 1:11 p.m. UTC | #4
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.

[1] https://www.linuxtv.org/downloads/v4l-dvb-apis-old/ch02s06.html
Daniel Vetter April 23, 2019, 6:52 a.m. UTC | #5
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