[Intel-gfx] [PATCH 1/6] drm: Create Color Management DRM properties

[Daniel Vetter]

On Wed, Dec 23, 2015 at 09:47:00AM +0000, Daniel Stone wrote:
> Hi,
> 
> On 21 December 2015 at 12:38, Daniel Vetter <daniel at ffwll.ch> wrote:
> > On Fri, Dec 18, 2015 at 04:53:28PM +0000, Daniel Stone wrote:
> >> > +struct drm_r32g32b32 {
> >> > +       /*
> >> > +        * Data is in U8.24 fixed point format.
> >> > +        * All platforms support values within [0, 1.0] range,
> >> > +        * for Red, Green and Blue colors.
> >> > +        */
> >> > +       __u32 r32;
> >> > +       __u32 g32;
> >> > +       __u32 b32;
> >> > +       __u32 reserved;
> >> > +};
> >>
> >> Wait, it's U8.24 (i.e. 0 -> 255.09999999403953517), but the supported
> >> range is [0..1.0]? What am I missing?
> >
> > Probably means:
> > - all platforms MUST support [0.0, 1.0] range, inclusive
> > - platforms CAN support larger values (which can make sense in degamma
> >   tables if your CTM will shrink things down again).
> 
> Ah, makes sense.
> 
> >> I also don't have a good answer on how to support non-CM-aware
> >> clients. Currently they'll just blindly carry around the correction
> >> factors from previous clients, which is _really_ bad: imagine you have
> >> Weston ported to use KMS/CM to correct perfectly, and then start
> >> Mutter/GNOME which still corrects perfectly, but using lcms and
> >> various client-side compensation rather than the CM engine. Your
> >> output will now be double-corrected, i.e. wrong, because Mutter won't
> >> know to reset the CM properties.
> >
> > Hm yeah, I think legacy entry points should remap to atomic ones.
> > Otherwise things are massively inconsistent (and we have a problem
> > figuring out when/which table applies in the driver). One problem with
> > that approach is that the legacy table has the assumption of a fixed
> > 256 entries (well we expose the size somewhere, but that's what everyone
> > uses). At least on some platforms, the full-blown table used by these
> > patches isn't even an integer multiple of that.
> 
> It's not even a legacy vs. atomic thing, this can happen in
> pure-atomic as well. Same as the render-compression plane property
> that I just replied to here as well.
> 
> - Weston starts and sets up palette/CTM properties
> - VT switch to Mutter, which isn't aware of new properties
> - Mutter atomically sets new plane state, containing framebuffer which
> is already colour-corrected for the target
> - result is now double-corrected as Mutter didn't know to unset the
> old properties
> 
> IOW, in the current model, any user of CM has to explicitly unset it
> before handover (not always actually possible), or any generic
> userspace must unset every property it sees and doesn't know about,
> hoping that setting to 0 will do the right thing.
> 
> Or we could add another client cap, which would prevent the CM
> properties from ever being exposed to userspace which doesn't know how
> to work with it. Passing the client caps through to
> plane_duplicate_state also means that we can unset the CM properties
> at this early point for unaware clients. This would mean that we
> wouldn't have to have code to explicitly unset it in, e.g., every
> legacy hook.

We don't have any support for unsetting anything really. Same argument you
make for CM here applies to rotation too. The only generic solution (if
you really care about this) would be for logind to once sample all atomic
state on boot-up, and force-restore that state when switching. Restoring
atomic state doesn't require userspace to understanding the semantics
really.

This kind of force-restoring is probably something we should implement in
the fbdev emulation, which would cover about 99% of all cases where
developers/users want to run different compositors. Or fbdev should simply
reset CM state, like it does for rotation already (that part is easy to
add, but indeed seems to be missing).

Trying to create an ad-hoc solution (using opt-in flags) to this problem
for every single feature seems pointless imo.

> >> About the only thing I can think of is to add a
> >> DRM_CLIENT_CAP_COLOR_MGMT, and modify drm_atomic_*_duplicate_state to
> >> take client caps (passed in from file_priv with the atomic ioctl, or
> >> explicitly set by other kernel code, according to its capabilities),
> >> and if the CM cap is not set, clear out the blobs when duplicating
> >> state.
> 
> (As here.)
> 
> >> All of this also needs to be backed up by a lot more extensive IGT,
> >> including disabling (from _every_ mode, not just 12-bit mode on BDW)
> >> via setting blob == NULL, testing the various depths (I still don't
> >> understand the difference between 8 and 10-bit on CHV), legacy gamma
> >> hooks when using CM, testing reset/dumb clients when the above
> >> duplicate_state issue is resolved, and especially the boundary cases
> >> in conversions (e.g. the sign wraparound on CHV). The documentation
> >> also _really_ needs fixing to be consistent with the code, and
> >> consistent with itself. When that's done, I think I'll be
> >> better-placed to do a second pass review, because after however many
> >> revisions, I still can't clearly see how it would be used from generic
> >> userspace (as opposed to an Intel-specific colour manager).
> >
> > One bit I'm not sure (and which isn't documented here) is that there's
> > supposed to be a read-only hint property telling new generic userspace
> > what tables are expected. This might mean you're not always using the most
> > awesome configuration, but it should at least work. That part of the
> > design seems to be undocumented.
> 
> Yeah, there is a single 'length' property per table, but given the
> dizzying array of options available, I'm not really sure if that's
> sufficient.

The idea is to just give a hint to generic userspace. Fancy userspace that
wants to exploit all the other options needs to have some baked-in
knowledge of the hardware it runs on. This is kinda similar to how you can
do dumb buffers, but for fancy ones userspace (at least currently) needs
to just know how to allocate stuff. Imo there's just plain limits to how
generic KMS can be. And it's not worth pushing beyond those.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch