| Message ID | 1450378678-24296-2-git-send-email-lionel.g.landwerlin@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Shashank, [auto build test WARNING on drm-intel/for-linux-next] [also build test WARNING on next-20151217] [cannot apply to v4.4-rc5] url: https://github.com/0day-ci/linux/commits/Lionel-Landwerlin/Color-Management-for-DRM-framework-v10/20151218-025917 base: git://anongit.freedesktop.org/drm-intel for-linux-next reproduce: make htmldocs All warnings (new ones prefixed by >>): drivers/gpu/drm/i915/i915_irq.c:2647: warning: No description found for parameter 'wedged' drivers/gpu/drm/i915/i915_irq.c:2647: warning: No description found for parameter 'fmt' >> drivers/gpu/drm/drm_atomic.c:405: warning: No description found for parameter 'dev' include/drm/drm_crtc.h:324: warning: No description found for parameter 'mode_blob' include/drm/drm_crtc.h:751: warning: No description found for parameter 'tile_blob_ptr' include/drm/drm_crtc.h:790: warning: No description found for parameter 'rotation' include/drm/drm_crtc.h:886: warning: No description found for parameter 'mutex' include/drm/drm_crtc.h:886: warning: No description found for parameter 'helper_private' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tile_idr' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'delayed_event' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'edid_property' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'dpms_property' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'path_property' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tile_property' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'plane_type_property' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'rotation_property' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'prop_src_x' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'prop_src_y' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'prop_src_w' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'prop_src_h' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'prop_crtc_x' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'prop_crtc_y' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'prop_crtc_w' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'prop_crtc_h' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'prop_fb_id' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'prop_crtc_id' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'prop_active' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'prop_mode_id' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'dvi_i_subconnector_property' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'dvi_i_select_subconnector_property' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tv_subconnector_property' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tv_select_subconnector_property' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tv_mode_property' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tv_left_margin_property' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tv_right_margin_property' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tv_top_margin_property' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tv_bottom_margin_property' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tv_brightness_property' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tv_contrast_property' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tv_flicker_reduction_property' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tv_overscan_property' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tv_saturation_property' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'tv_hue_property' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'scaling_mode_property' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'aspect_ratio_property' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'dirty_info_property' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'suggested_x_property' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'suggested_y_property' include/drm/drm_crtc.h:1185: warning: No description found for parameter 'allow_fb_modifiers' include/drm/drm_fb_helper.h:148: warning: No description found for parameter 'connector_info' include/drm/drm_dp_helper.h:749: warning: No description found for parameter 'i2c_nack_count' include/drm/drm_dp_helper.h:749: warning: No description found for parameter 'i2c_defer_count' drivers/gpu/drm/drm_dp_mst_topology.c:2237: warning: No description found for parameter 'connector' include/drm/drm_dp_mst_helper.h:98: warning: No description found for parameter 'cached_edid' include/drm/drm_dp_mst_helper.h:98: warning: No description found for parameter 'has_audio' include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'max_dpcd_transaction_bytes' include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'sink_count' include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'total_slots' include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'avail_slots' include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'total_pbn' include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'qlock' include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'tx_msg_downq' include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'tx_msg_upq' include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'tx_down_in_progress' include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'tx_up_in_progress' include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'payload_lock' include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'proposed_vcpis' include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'payloads' include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'payload_mask' include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'vcpi_mask' include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'tx_waitq' include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'work' include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'tx_work' include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'destroy_connector_list' include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'destroy_connector_lock' include/drm/drm_dp_mst_helper.h:473: warning: No description found for parameter 'destroy_connector_work' drivers/gpu/drm/drm_dp_mst_topology.c:2237: warning: No description found for parameter 'connector' drivers/gpu/drm/drm_irq.c:173: warning: No description found for parameter 'flags' include/drm/drmP.h:168: warning: No description found for parameter 'fmt' include/drm/drmP.h:184: warning: No description found for parameter 'fmt' include/drm/drmP.h:202: warning: No description found for parameter 'fmt' include/drm/drmP.h:247: warning: No description found for parameter 'dev' include/drm/drmP.h:247: warning: No description found for parameter 'data' include/drm/drmP.h:247: warning: No description found for parameter 'file_priv' include/drm/drmP.h:280: warning: No description found for parameter 'ioctl' include/drm/drmP.h:280: warning: No description found for parameter '_func' include/drm/drmP.h:280: warning: No description found for parameter '_flags' include/drm/drmP.h:360: warning: cannot understand function prototype: 'struct drm_lock_data ' include/drm/drmP.h:413: warning: cannot understand function prototype: 'struct drm_driver ' include/drm/drmP.h:663: warning: cannot understand function prototype: 'struct drm_info_list ' include/drm/drmP.h:673: warning: cannot understand function prototype: 'struct drm_info_node ' include/drm/drmP.h:683: warning: cannot understand function prototype: 'struct drm_minor ' include/drm/drmP.h:731: warning: cannot understand function prototype: 'struct drm_device ' drivers/gpu/drm/i915/intel_runtime_pm.c:2180: warning: No description found for parameter 'resume' drivers/gpu/drm/i915/intel_runtime_pm.c:2180: warning: No description found for parameter 'resume' drivers/gpu/drm/i915/i915_irq.c:2647: warning: No description found for parameter 'wedged' drivers/gpu/drm/i915/i915_irq.c:2647: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_irq.c:2647: warning: No description found for parameter 'wedged' drivers/gpu/drm/i915/i915_irq.c:2647: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_irq.c:2647: warning: No description found for parameter 'wedged' drivers/gpu/drm/i915/i915_irq.c:2647: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_irq.c:2647: warning: No description found for parameter 'wedged' drivers/gpu/drm/i915/i915_irq.c:2647: warning: No description found for parameter 'fmt' vim +/dev +405 drivers/gpu/drm/drm_atomic.c 389 390 /** 391 * drm_atomic_crtc_set_blob - find and set a blob 392 * @state_blob: reference pointer to the color blob in the crtc_state 393 * @blob_id: blob_id coming from set_property() call 394 * 395 * Set a color correction blob (originating from a set blob property) on the 396 * desired CRTC state. This function will take reference of the blob property 397 * in the CRTC state, finds the blob based on blob_id (which comes from 398 * set_property call) and set the blob at the proper place. 399 * 400 * RETURNS: 401 * Zero on success, error code on failure. 402 */ 403 static int drm_atomic_crtc_set_blob(struct drm_device *dev, 404 struct drm_property_blob **state_blob, uint32_t blob_id) > 405 { 406 struct drm_property_blob *blob; 407 408 blob = drm_property_lookup_blob(dev, blob_id); 409 if (!blob) { 410 DRM_DEBUG_KMS("Invalid Blob ID\n"); 411 return -EINVAL; 412 } 413 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
[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@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
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@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
On Thu, Dec 17, 2015 at 06:57:53PM +0000, Lionel Landwerlin wrote: > From: Shashank Sharma <shashank.sharma@intel.com> > > Color Management is an extension to DRM framework to allow hardware color > correction and enhancement capabilities. > > 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 color correction capabilities may differ per platform, supporting > various different no. of correction coefficients. So DRM color manager > support few properties using which a user space can query the platform's > capability, and prepare color correction accordingly. > These query properties are: > 1. cm_coeff_after_ctm_property > 2. cm_coeff_before_ctm_property > (CTM is fix to 9 coefficients across industry) > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com> > --- > drivers/gpu/drm/drm_atomic.c | 67 +++++++++++++++++++++++++++++++++++-- > drivers/gpu/drm/drm_atomic_helper.c | 9 +++++ > drivers/gpu/drm/drm_crtc.c | 32 ++++++++++++++++++ > include/drm/drm_crtc.h | 24 +++++++++++++ > include/uapi/drm/drm.h | 30 +++++++++++++++++ > 5 files changed, 160 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 6a21e5c..ebdb98d 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -388,6 +388,38 @@ int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state, > EXPORT_SYMBOL(drm_atomic_set_mode_prop_for_crtc); > > /** > + * 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) I think we need to at least split this into a ctm and gamma table variant to be able to have a size check for the passed-in blob: - ctm must exactly match the size we expect - gamma table must be an integer multiple of the drm_r32g32b32 struct. Also we should probably have a little helper to compute the size of the gamma lut in entries (just the size/sizeof(drm_r32g32b32) division really). The other bits that's missing here is a drm core function to attach these properties to planes/crtc. We don't want every driver to roll their own implementation, since it'll lead to fun divergence between different implementations. Also, that main function could be used to place the ABI rules someplace nice (like how generic userspace is supposed to interact with color manager props). -Daniel > +{ > + struct drm_property_blob *blob; > + > + blob = drm_property_lookup_blob(dev, blob_id); > + if (!blob) { > + DRM_DEBUG_KMS("Invalid Blob ID\n"); > + return -EINVAL; > + } > + > + if (*state_blob) > + drm_property_unreference_blob(*state_blob); > + > + /* Attach the blob to be committed in state */ > + *state_blob = blob; > + return 0; > +} > + > +/** > * drm_atomic_crtc_set_property - set property on CRTC > * @crtc: the drm CRTC to set a property on > * @state: the state object to update with the new property value > @@ -419,8 +451,31 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, > ret = drm_atomic_set_mode_prop_for_crtc(state, mode); > drm_property_unreference_blob(mode); > return ret; > - } > - else if (crtc->funcs->atomic_set_property) > + } else if (property == config->cm_palette_after_ctm_property) { > + ret = drm_atomic_crtc_set_blob(dev, > + &state->palette_after_ctm_blob, val); > + if (ret) > + DRM_DEBUG_KMS("Failed to load blob palette_after_ctm\n"); > + else > + state->color_correction_changed = true; > + return ret; > + } else if (property == config->cm_palette_before_ctm_property) { > + ret = drm_atomic_crtc_set_blob(dev, > + &state->palette_before_ctm_blob, val); > + if (ret) > + DRM_DEBUG_KMS("Failed to load blob palette_before_ctm\n"); > + else > + state->color_correction_changed = true; > + return ret; > + } else if (property == config->cm_ctm_property) { > + ret = drm_atomic_crtc_set_blob(dev, > + &state->ctm_blob, val); > + if (ret) > + DRM_DEBUG_KMS("Failed to load blob ctm\n"); > + else > + state->color_correction_changed = true; > + return ret; > + } else if (crtc->funcs->atomic_set_property) > return crtc->funcs->atomic_set_property(crtc, state, property, val); > else > return -EINVAL; > @@ -456,6 +511,14 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc, > *val = state->active; > else if (property == config->prop_mode_id) > *val = (state->mode_blob) ? state->mode_blob->base.id : 0; > + else if (property == config->cm_palette_after_ctm_property) > + *val = (state->palette_after_ctm_blob) ? > + state->palette_after_ctm_blob->base.id : 0; > + else if (property == config->cm_palette_before_ctm_property) > + *val = (state->palette_before_ctm_blob) ? > + state->palette_before_ctm_blob->base.id : 0; > + else if (property == config->cm_ctm_property) > + *val = (state->ctm_blob) ? state->ctm_blob->base.id : 0; > else if (crtc->funcs->atomic_get_property) > return crtc->funcs->atomic_get_property(crtc, state, property, val); > else > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 268d37f..bd325da 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -2448,6 +2448,12 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc, > > if (state->mode_blob) > drm_property_reference_blob(state->mode_blob); > + if (state->ctm_blob) > + drm_property_reference_blob(state->ctm_blob); > + if (state->palette_after_ctm_blob) > + drm_property_reference_blob(state->palette_after_ctm_blob); > + if (state->palette_before_ctm_blob) > + drm_property_reference_blob(state->palette_before_ctm_blob); > state->mode_changed = false; > state->active_changed = false; > state->planes_changed = false; > @@ -2492,6 +2498,9 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc, > struct drm_crtc_state *state) > { > drm_property_unreference_blob(state->mode_blob); > + drm_property_unreference_blob(state->ctm_blob); > + drm_property_unreference_blob(state->palette_after_ctm_blob); > + drm_property_unreference_blob(state->palette_before_ctm_blob); > } > EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state); > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 62fa95f..2e02d0f 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -1542,6 +1542,38 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) > return -ENOMEM; > dev->mode_config.prop_mode_id = prop; > > + /* Color Management properties */ > + prop = drm_property_create(dev, > + DRM_MODE_PROP_BLOB, "PALETTE_AFTER_CTM", 0); > + if (!prop) > + return -ENOMEM; > + dev->mode_config.cm_palette_after_ctm_property = prop; > + > + prop = drm_property_create(dev, > + DRM_MODE_PROP_BLOB, "PALETTE_BEFORE_CTM", 0); > + if (!prop) > + return -ENOMEM; > + dev->mode_config.cm_palette_before_ctm_property = prop; > + > + prop = drm_property_create(dev, > + DRM_MODE_PROP_BLOB, "CTM", 0); > + if (!prop) > + return -ENOMEM; > + dev->mode_config.cm_ctm_property = prop; > + > + /* DRM properties to query color capabilities */ > + prop = drm_property_create(dev, DRM_MODE_PROP_IMMUTABLE, > + "COEFFICIENTS_BEFORE_CTM", 0); > + if (!prop) > + return -ENOMEM; > + dev->mode_config.cm_coeff_before_ctm_property = prop; > + > + prop = drm_property_create(dev, DRM_MODE_PROP_IMMUTABLE, > + "COEFFICIENTS_AFTER_CTM", 0); > + if (!prop) > + return -ENOMEM; > + dev->mode_config.cm_coeff_after_ctm_property = prop; > + > return 0; > } > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 3b040b3..8326765 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -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 > * @event: optional pointer to a DRM event to signal upon completion of the > * state update > * @state: backpointer to global drm_atomic_state > @@ -331,6 +335,7 @@ struct drm_crtc_state { > bool mode_changed : 1; > bool active_changed : 1; > bool connectors_changed : 1; > + bool color_correction_changed : 1; > > /* attached planes bitmask: > * WARNING: transitional helpers do not maintain plane_mask so > @@ -350,6 +355,11 @@ struct drm_crtc_state { > /* blob property to expose current mode to atomic userspace */ > struct drm_property_blob *mode_blob; > > + /* Color management blobs */ > + struct drm_property_blob *palette_before_ctm_blob; > + struct drm_property_blob *palette_after_ctm_blob; > + struct drm_property_blob *ctm_blob; > + > struct drm_pending_vblank_event *event; > > struct drm_atomic_state *state; > @@ -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 > * @preferred_depth: preferred RBG pixel depth, used by fb helpers > * @prefer_shadow: hint to userspace to prefer shadow-fb rendering > * @async_page_flip: does this device support async flips on the primary plane? > @@ -2124,6 +2139,15 @@ struct drm_mode_config { > struct drm_property *suggested_x_property; > struct drm_property *suggested_y_property; > > + /* Color correction properties */ > + struct drm_property *cm_palette_before_ctm_property; > + struct drm_property *cm_palette_after_ctm_property; > + struct drm_property *cm_ctm_property; > + > + /* Color correction query */ > + struct drm_property *cm_coeff_before_ctm_property; > + struct drm_property *cm_coeff_after_ctm_property; > + > /* dumb ioctl parameters */ > uint32_t preferred_depth, prefer_shadow; > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > index b4e92eb..24e4520 100644 > --- a/include/uapi/drm/drm.h > +++ b/include/uapi/drm/drm.h > @@ -830,6 +830,36 @@ struct drm_event_vblank { > __u32 reserved; > }; > > +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; > +}; > + > +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]; > +}; > + > +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]; > +}; > + > /* typedef area */ > #ifndef __KERNEL__ > typedef struct drm_clip_rect drm_clip_rect_t; > -- > 2.6.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi, On 21 December 2015 at 12:38, Daniel Vetter <daniel@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
On Wed, Dec 23, 2015 at 09:47:00AM +0000, Daniel Stone wrote: > Hi, > > On 21 December 2015 at 12:38, Daniel Vetter <daniel@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
Hi, On 5 January 2016 at 10:23, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Dec 23, 2015 at 09:47:00AM +0000, Daniel Stone wrote: >> 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. Sure, but on the other hand, rotation has been around ~forever, and is implemented in multiple places. Colour management is a lot less obvious. > 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. Fair enough, I guess it could be more difficult, so how about a new flag to the atomic ioctl which requests state be _reset_ to scratch rather than duplicated? That way, clients could be really sure they weren't going to get screwed by rotation / colour management / render compression / whatever. >> 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. I get the point, but am really struggling to make sense of the options presented, and how a halfway-sensible generic userspace would work. Exposing length/depth tuples might not be the worst idea; it also sounds like there's more to dig out in terms of index interpolation as well. But these are much smaller objections than having unwitting clients inherit the CM state ... Cheers, Daniel
On Mon, Jan 11, 2016 at 08:37:09PM +0000, Daniel Stone wrote: > Hi, > > On 5 January 2016 at 10:23, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Wed, Dec 23, 2015 at 09:47:00AM +0000, Daniel Stone wrote: > >> 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. > > Sure, but on the other hand, rotation has been around ~forever, and is > implemented in multiple places. Colour management is a lot less > obvious. > > > 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. > > Fair enough, I guess it could be more difficult, so how about a new > flag to the atomic ioctl which requests state be _reset_ to scratch > rather than duplicated? That way, clients could be really sure they > weren't going to get screwed by rotation / colour management / render > compression / whatever. One question is what exactly is scratch? Eg. I have BYT tablet here which has the display mounted upside down so in theory the reset state should be 180 degree rotated. I have a patch hiding in some branch to make the fbdev helper take over the rotation from the BIOS. I suppose I should also dig into the VBT to see if there's some rotation indicators there for the cases when you boot with HDMI plugged in. But I digress.
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 6a21e5c..ebdb98d 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -388,6 +388,38 @@ int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state, EXPORT_SYMBOL(drm_atomic_set_mode_prop_for_crtc); /** + * 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; + } + + if (*state_blob) + drm_property_unreference_blob(*state_blob); + + /* Attach the blob to be committed in state */ + *state_blob = blob; + return 0; +} + +/** * drm_atomic_crtc_set_property - set property on CRTC * @crtc: the drm CRTC to set a property on * @state: the state object to update with the new property value @@ -419,8 +451,31 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, ret = drm_atomic_set_mode_prop_for_crtc(state, mode); drm_property_unreference_blob(mode); return ret; - } - else if (crtc->funcs->atomic_set_property) + } else if (property == config->cm_palette_after_ctm_property) { + ret = drm_atomic_crtc_set_blob(dev, + &state->palette_after_ctm_blob, val); + if (ret) + DRM_DEBUG_KMS("Failed to load blob palette_after_ctm\n"); + else + state->color_correction_changed = true; + return ret; + } else if (property == config->cm_palette_before_ctm_property) { + ret = drm_atomic_crtc_set_blob(dev, + &state->palette_before_ctm_blob, val); + if (ret) + DRM_DEBUG_KMS("Failed to load blob palette_before_ctm\n"); + else + state->color_correction_changed = true; + return ret; + } else if (property == config->cm_ctm_property) { + ret = drm_atomic_crtc_set_blob(dev, + &state->ctm_blob, val); + if (ret) + DRM_DEBUG_KMS("Failed to load blob ctm\n"); + else + state->color_correction_changed = true; + return ret; + } else if (crtc->funcs->atomic_set_property) return crtc->funcs->atomic_set_property(crtc, state, property, val); else return -EINVAL; @@ -456,6 +511,14 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc, *val = state->active; else if (property == config->prop_mode_id) *val = (state->mode_blob) ? state->mode_blob->base.id : 0; + else if (property == config->cm_palette_after_ctm_property) + *val = (state->palette_after_ctm_blob) ? + state->palette_after_ctm_blob->base.id : 0; + else if (property == config->cm_palette_before_ctm_property) + *val = (state->palette_before_ctm_blob) ? + state->palette_before_ctm_blob->base.id : 0; + else if (property == config->cm_ctm_property) + *val = (state->ctm_blob) ? state->ctm_blob->base.id : 0; else if (crtc->funcs->atomic_get_property) return crtc->funcs->atomic_get_property(crtc, state, property, val); else diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 268d37f..bd325da 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2448,6 +2448,12 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc, if (state->mode_blob) drm_property_reference_blob(state->mode_blob); + if (state->ctm_blob) + drm_property_reference_blob(state->ctm_blob); + if (state->palette_after_ctm_blob) + drm_property_reference_blob(state->palette_after_ctm_blob); + if (state->palette_before_ctm_blob) + drm_property_reference_blob(state->palette_before_ctm_blob); state->mode_changed = false; state->active_changed = false; state->planes_changed = false; @@ -2492,6 +2498,9 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc, struct drm_crtc_state *state) { drm_property_unreference_blob(state->mode_blob); + drm_property_unreference_blob(state->ctm_blob); + drm_property_unreference_blob(state->palette_after_ctm_blob); + drm_property_unreference_blob(state->palette_before_ctm_blob); } EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state); diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 62fa95f..2e02d0f 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1542,6 +1542,38 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.prop_mode_id = prop; + /* Color Management properties */ + prop = drm_property_create(dev, + DRM_MODE_PROP_BLOB, "PALETTE_AFTER_CTM", 0); + if (!prop) + return -ENOMEM; + dev->mode_config.cm_palette_after_ctm_property = prop; + + prop = drm_property_create(dev, + DRM_MODE_PROP_BLOB, "PALETTE_BEFORE_CTM", 0); + if (!prop) + return -ENOMEM; + dev->mode_config.cm_palette_before_ctm_property = prop; + + prop = drm_property_create(dev, + DRM_MODE_PROP_BLOB, "CTM", 0); + if (!prop) + return -ENOMEM; + dev->mode_config.cm_ctm_property = prop; + + /* DRM properties to query color capabilities */ + prop = drm_property_create(dev, DRM_MODE_PROP_IMMUTABLE, + "COEFFICIENTS_BEFORE_CTM", 0); + if (!prop) + return -ENOMEM; + dev->mode_config.cm_coeff_before_ctm_property = prop; + + prop = drm_property_create(dev, DRM_MODE_PROP_IMMUTABLE, + "COEFFICIENTS_AFTER_CTM", 0); + if (!prop) + return -ENOMEM; + dev->mode_config.cm_coeff_after_ctm_property = prop; + return 0; } diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 3b040b3..8326765 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -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 * @event: optional pointer to a DRM event to signal upon completion of the * state update * @state: backpointer to global drm_atomic_state @@ -331,6 +335,7 @@ struct drm_crtc_state { bool mode_changed : 1; bool active_changed : 1; bool connectors_changed : 1; + bool color_correction_changed : 1; /* attached planes bitmask: * WARNING: transitional helpers do not maintain plane_mask so @@ -350,6 +355,11 @@ struct drm_crtc_state { /* blob property to expose current mode to atomic userspace */ struct drm_property_blob *mode_blob; + /* Color management blobs */ + struct drm_property_blob *palette_before_ctm_blob; + struct drm_property_blob *palette_after_ctm_blob; + struct drm_property_blob *ctm_blob; + struct drm_pending_vblank_event *event; struct drm_atomic_state *state; @@ -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 * @preferred_depth: preferred RBG pixel depth, used by fb helpers * @prefer_shadow: hint to userspace to prefer shadow-fb rendering * @async_page_flip: does this device support async flips on the primary plane? @@ -2124,6 +2139,15 @@ struct drm_mode_config { struct drm_property *suggested_x_property; struct drm_property *suggested_y_property; + /* Color correction properties */ + struct drm_property *cm_palette_before_ctm_property; + struct drm_property *cm_palette_after_ctm_property; + struct drm_property *cm_ctm_property; + + /* Color correction query */ + struct drm_property *cm_coeff_before_ctm_property; + struct drm_property *cm_coeff_after_ctm_property; + /* dumb ioctl parameters */ uint32_t preferred_depth, prefer_shadow; diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index b4e92eb..24e4520 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -830,6 +830,36 @@ struct drm_event_vblank { __u32 reserved; }; +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; +}; + +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]; +}; + +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]; +}; + /* typedef area */ #ifndef __KERNEL__ typedef struct drm_clip_rect drm_clip_rect_t;