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

[Daniel Vetter <daniel@ffwll.ch>]

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

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