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

[Daniel Stone]

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.

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

Cheers,
Daniel