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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxxxxxxx
> > > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux