Re: [PATCH v4 2/3] drm/rockchip: Add optional support for CRTC gamma LUT

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Oct 10, 2019 at 12:23:05PM -0400, Ilia Mirkin wrote:
> On Thu, Oct 10, 2019 at 12:01 PM Sean Paul <sean@poorly.run> wrote:
> > > > > +static int vop_crtc_atomic_check(struct drm_crtc *crtc,
> > > > > +                              struct drm_crtc_state *crtc_state)
> > > > > +{
> > > > > +     struct vop *vop = to_vop(crtc);
> > > > > +
> > > > > +     if (vop->lut_regs && crtc_state->color_mgmt_changed &&
> > > > > +         crtc_state->gamma_lut) {
> > > > > +             unsigned int len;
> > > > > +
> > > > > +             len = drm_color_lut_size(crtc_state->gamma_lut);
> > > > > +             if (len != crtc->gamma_size) {
> > > > > +                     DRM_DEBUG_KMS("Invalid LUT size; got %d, expected %d\n",
> > > > > +                                   len, crtc->gamma_size);
> > > > > +                     return -EINVAL;
> > > > > +             }
> > > >
> > > > Overflow is avoided in drm_mode_gamma_set_ioctl(), so I don't think you need
> > > > this function.
> > > >
> > >
> > > But that only applies to the legacy path. Isn't this needed to ensure
> > > a gamma blob
> > > has the right size?
> >
> > Yeah, good point, we check the element size in the atomic path, but not the max
> > size. I haven't looked at enough color lut stuff to have an opinion whether this
> > check would be useful in a helper function or not, something to consider, I
> > suppose.
> 
> Some implementations support multiple sizes (e.g. 256 and 1024) but
> not anything in between. It would be difficult to expose this
> generically, I would imagine.
> The 256 size is kind of special, since
> basically all legacy usage assumes that 256 is the one true quantity
> of LUT entries...

What we do currently in i915 is:
crtc->gamma_size = 256
GAMMA_LUT_SIZE = platform specific (256, 129, 257, 2^10, or 2^18+1 (lol))
DEGAMMA_LUT_SIZE = platform specific (0, 33, 65, or 2^10)

i915 will accept:
- gamma lut of size 256, iff ctm==NULL and degamma==NULL (the so
  called "legacy gamma" mode)
- (de)gamma_lut of size (DE)GAMMA_LUT_SIZE if it passes the
  checks done by drm_color_lut_check()

Ie. just one or two gamma modes per platform is exposed. And that's
about all we can do with the current uapi even though our hardware
supports many more modes.

The resulting precision, interpolation vs. truncation behaviour,
and handling of out of gamut values are all totally unspecified
and userspace just has to make a guess.

We also cheat with the 2^10 sized LUTs a bit due to the hw sharing
the same LUT for gamma and degamma, and so if you enable both at
the same time we throw away every second entry and each stage
only gets a 2^9 entry LUT in the end.

Oh and for the 2^18+1 monstrosity we cheat even more and
throw away ~99.8% of the entries :(


This here was my idea for extending the uapi so that we
could expose the full hw capabilities and let userspace
decide which mode suits it best without having to guess
what it'll get:
https://github.com/vsyrjala/linux/commits/gamma_mode_prop

Maybe in a few years I'll find time to get back to it...

-- 
Ville Syrjälä
Intel