[Intel-gfx] [v3 0/7] Add Multi Segment Gamma Support
Daniel Vetter
daniel at ffwll.ch
Thu Apr 18 07:13:04 UTC 2019
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 at 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
More information about the Intel-gfx
mailing list