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

[Daniel Vetter]

On Fri, Dec 18, 2015 at 04:53:28PM +0000, Daniel Stone wrote:
> [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.

Yeah there's a question about how to best document new atomic extensions.
Traditionally we haven't documented the properties themselves (hence the
*_property), and I think that makes sense since it's a very large pile.
Two options left:
- Extensively document all the properties for a new feature in the drm
  core function to attach the props to a plane/crtc.

- Document them in the drm_*_state structures. There we can even use the
  new per-member kerneldoc layout to go into great details.

Probably best to do both and link between the two places.
> 
> > +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).

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

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.

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

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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch