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

[Daniel Stone]

[Paging danvet to the bottom paragraphs re client-cap ...]

Hi Lionel,
I've still got quite a few concerns about the implementation as it
stands. Some are minor quibbles (e.g. can't unset blob IDs), some are
larger design issues, some are rehashed comments from previous series,
and some are new now I've looked at it afresh.

On 17 December 2015 at 18:57, Lionel Landwerlin
<lionel.g.landwerlin at intel.com> wrote:
> DRM color manager supports these color properties:
> 1. "ctm": Color transformation matrix property, where a
>    color transformation matrix of 9 correction values gets
>    applied as correction.
> 2. "palette_before_ctm": for corrections which get applied
>    beore color transformation matrix correction.
> 3. "palette_after_ctm": for corrections which get applied
>    after color transformation matrix correction.

These all appear to be pretty common, at least for the platforms I've
checked. So, sounds good. You might want to document that before and
after palettes are often referred to as degamma and gamma,
respectively, though.

>  /**
> + * drm_atomic_crtc_set_blob - find and set a blob
> + * @state_blob: reference pointer to the color blob in the crtc_state
> + * @blob_id: blob_id coming from set_property() call
> + *
> + * Set a color correction blob (originating from a set blob property) on the
> + * desired CRTC state. This function will take reference of the blob property
> + * in the CRTC state, finds the blob based on blob_id (which comes from
> + * set_property call) and set the blob at the proper place.
> + *
> + * RETURNS:
> + * Zero on success, error code on failure.
> + */
> +static int drm_atomic_crtc_set_blob(struct drm_device *dev,
> +       struct drm_property_blob **state_blob, uint32_t blob_id)
> +{
> +       struct drm_property_blob *blob;
> +
> +       blob = drm_property_lookup_blob(dev, blob_id);
> +       if (!blob) {
> +               DRM_DEBUG_KMS("Invalid Blob ID\n");
> +               return -EINVAL;
> +       }

This needs to handle blob_id == 0, to unset it. Initialising 'blob' to
NULL and wrapping the lookup_blob check in if (blob_id != 0) should do
it. While you're at it, a more helpful error message (e.g. actually
listing the blob ID) might be in order. And IGT.

Making crtc_state->MODE_ID use this helper, and kms_atomic not
breaking, would be a fairly good indication that you've got it right.
;)

> @@ -305,11 +305,15 @@ struct drm_plane_helper_funcs;
>   * @mode_changed: crtc_state->mode or crtc_state->enable has been changed
>   * @active_changed: crtc_state->active has been toggled.
>   * @connectors_changed: connectors to this crtc have been updated
> + * @color_correction_changed: color correction blob in this crtc got updated
>   * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
>   * @last_vblank_count: for helpers and drivers to capture the vblank of the
>   *     update to ensure framebuffer cleanup isn't done too early
>   * @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings
>   * @mode: current mode timings
> + * @palette_before_ctm_blob: blob for color corrections to be applied after CTM
> + * @palette_after_ctm_blob: blob for color corrections to be applied before CTM
> + * @ctm_blob: blob for CTM color correction

For instance, documenting (de)gamma terminology here might be nice.

> @@ -2019,6 +2029,11 @@ struct drm_mode_config_funcs {
>   * @property_blob_list: list of all the blob property objects
>   * @blob_lock: mutex for blob property allocation and management
>   * @*_property: core property tracking
> + * @cm_palette_before_ctm_property: color corrections before CTM block
> + * @cm_palette_after_ctm_property: color corrections after CTM block
> + * @cm_ctm_property: color transformation matrix correction
> + * @cm_coeff_before_ctm_property: query no of correction coeffi before CTM
> + * @cm_coeff_after_ctm_property: query no of correction coeffi after CTM

'coeffi'? These also aren't sufficient either (see my descent into
madness in the BDW patch), I don't think.

> +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?

> +struct drm_palette {
> +       /*
> +        * Starting of palette LUT in R32G32B32 format.
> +        * Each of RGB value is in U8.24 fixed point format.
> +        */
> +       struct drm_r32g32b32 lut[0];
> +};

Why the indirection; can we just expose the member directly?

> +struct drm_ctm {
> +       /*
> +        * Each value is in S31.32 format.
> +        * This is 3x3 matrix in row major format.
> +        * Integer part will be clipped to nearest
> +        * max/min boundary as supported by the HW platform.
> +        */
> +       __s64 ctm_coeff[9];
> +};

Perhaps ditto, but this seems more valid. As a total bikeshed,
'ctm_coeff' is redundant in a structure named drm_ctm anyway. Having
these namespaced into something consistent (e.g. drm_color_ or
whatever) might be nice too.

All in all, I have to admit I'm fairly baffled as to how I'd actually
use this from generic userspace. Not being able to divine the correct
list length (see BDW) is a showstopper. Having repeated conflicting
comments on the range (8.24 or 0.24?) is greatly confusing. Sign
wraparound on CHV is confusing, as is the '8-bit' vs. '10-bit' modes
which appear to do exactly the same thing (but with one more entry).
Not supporting blob == NULL to disable is pretty much a non-starter.

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.

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.

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

Sorry this probably isn't quite the Christmas present you'd hoped for ...

Cheers,
Daniel