From c9e42b72b44acc67ede9726e6434b12ba153d901 Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Tue, 28 Mar 2017 17:53:48 +0200 Subject: [PATCH] drm: Document kms locking a bit better The rules are getting real hard, better to dump my brain into text a bit. This is by far not complete, but I think I reasonable start at least. Some of the older kms structures would need a full doc review anyway ... Cc: Harry Wentland Reviewed-by: Harry Wentland Reviewed-by: Alex Deucher Cc: Maarten Lankhorst Signed-off-by: Daniel Vetter Link: http://patchwork.freedesktop.org/patch/msgid/20170328155349.5972-2-daniel.vetter@ffwll.ch --- include/drm/drm_connector.h | 16 +++- include/drm/drm_crtc.h | 14 +++- include/drm/drm_mode_config.h | 140 ++++++++++++++++++++++++++-------- include/drm/drm_plane.h | 16 +++- 4 files changed, 152 insertions(+), 34 deletions(-) diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 6d3f7ba1c797..941f57f311aa 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -656,7 +656,6 @@ struct drm_cmdline_mode { * @bad_edid_counter: track sinks that give us an EDID with invalid checksum * @edid_corrupt: indicates whether the last read EDID was corrupt * @debugfs_entry: debugfs directory for this connector - * @state: current atomic state for this connector * @has_tile: is this connector connected to a tiled monitor * @tile_group: tile group for the connected monitor * @tile_is_single_monitor: whether the tile is one monitor housing @@ -824,6 +823,21 @@ struct drm_connector { struct dentry *debugfs_entry; + /** + * @state: + * + * Current atomic state for this connector. + * + * This is protected by @drm_mode_config.connection_mutex. Note that + * nonblocking atomic commits access the current connector state without + * taking locks. Either by going through the &struct drm_atomic_state + * pointers, see for_each_connector_in_state(), + * for_each_oldnew_connector_in_state(), + * for_each_old_connector_in_state() and + * for_each_new_connector_in_state(). Or through careful ordering of + * atomic commit operations as implemented in the atomic helpers, see + * &struct drm_crtc_commit. + */ struct drm_connector_state *state; /* DisplayID bits */ diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 24dcb121bad4..c1a75a9c81c2 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -699,10 +699,12 @@ struct drm_crtc { /** * @mutex: * - * This provides a read lock for the overall crtc state (mode, dpms + * This provides a read lock for the overall CRTC state (mode, dpms * state, ...) and a write lock for everything which can be update - * without a full modeset (fb, cursor data, crtc properties ...). A full + * without a full modeset (fb, cursor data, CRTC properties ...). A full * modeset also need to grab &drm_mode_config.connection_mutex. + * + * For atomic drivers specifically this protects @state. */ struct drm_modeset_lock mutex; @@ -748,6 +750,14 @@ struct drm_crtc { * @state: * * Current atomic state for this CRTC. + * + * This is protected by @mutex. Note that nonblocking atomic commits + * access the current CRTC state without taking locks. Either by going + * through the &struct drm_atomic_state pointers, see + * for_each_crtc_in_state(), for_each_oldnew_crtc_in_state(), + * for_each_old_crtc_in_state() and for_each_new_crtc_in_state(). Or + * through careful ordering of atomic commit operations as implemented + * in the atomic helpers, see &struct drm_crtc_commit. */ struct drm_crtc_state *state; diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index 579070ff06ef..42981711189b 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -307,21 +307,6 @@ struct drm_mode_config_funcs { /** * struct drm_mode_config - Mode configuration control structure - * @mutex: mutex protecting KMS related lists and structures - * @connection_mutex: ww mutex protecting connector state and routing - * @acquire_ctx: global implicit acquire context used by atomic drivers for - * legacy IOCTLs - * @fb_lock: mutex to protect fb state and lists - * @num_fb: number of fbs available - * @fb_list: list of framebuffers available - * @num_encoder: number of encoders on this device - * @encoder_list: list of encoder objects - * @num_overlay_plane: number of overlay planes on this device - * @num_total_plane: number of universal (i.e. with primary/curso) planes on this device - * @plane_list: list of plane objects - * @num_crtc: number of CRTCs on this device - * @crtc_list: list of CRTC objects - * @property_list: list of property objects * @min_width: minimum pixel width on this device * @min_height: minimum pixel height on this device * @max_width: maximum pixel width on this device @@ -332,9 +317,6 @@ struct drm_mode_config_funcs { * @poll_running: track polling status for this device * @delayed_event: track delayed poll uevent deliver for this device * @output_poll_work: delayed work for polling in process context - * @property_blob_list: list of all the blob property objects - * @blob_lock: mutex for blob property allocation and management - * @*_property: core property tracking * @preferred_depth: preferred RBG pixel depth, used by fb helpers * @prefer_shadow: hint to userspace to prefer shadow-fb rendering * @cursor_width: hint to userspace for max cursor width @@ -346,9 +328,37 @@ struct drm_mode_config_funcs { * global restrictions are also here, e.g. dimension restrictions. */ struct drm_mode_config { - struct mutex mutex; /* protects configuration (mode lists etc.) */ - struct drm_modeset_lock connection_mutex; /* protects connector->encoder and encoder->crtc links */ - struct drm_modeset_acquire_ctx *acquire_ctx; /* for legacy _lock_all() / _unlock_all() */ + /** + * @mutex: + * + * This is the big scary modeset BKL which protects everything that + * isn't protect otherwise. Scope is unclear and fuzzy, try to remove + * anything from under it's protection and move it into more well-scoped + * locks. + * + * The one important thing this protects is the use of @acquire_ctx. + */ + struct mutex mutex; + + /** + * @connection_mutex: + * + * This protects connector state and the connector to encoder to CRTC + * routing chain. + * + * For atomic drivers specifically this protects &drm_connector.state. + */ + struct drm_modeset_lock connection_mutex; + + /** + * @acquire_ctx: + * + * Global implicit acquire context used by atomic drivers for legacy + * IOCTLs. Deprecated, since implicit locking contexts make it + * impossible to use driver-private &struct drm_modeset_lock. Users of + * this must hold @mutex. + */ + struct drm_modeset_acquire_ctx *acquire_ctx; /** * @idr_mutex: @@ -374,8 +384,11 @@ struct drm_mode_config { */ struct idr tile_idr; - struct mutex fb_lock; /* proctects global and per-file fb lists */ + /** @fb_lock: Mutex to protect fb the global @fb_list and @num_fb. */ + struct mutex fb_lock; + /** @num_fb: Number of entries on @fb_list. */ int num_fb; + /** @fb_list: List of all &struct drm_framebuffer. */ struct list_head fb_list; /** @@ -393,27 +406,80 @@ struct drm_mode_config { */ struct ida connector_ida; /** - * @connector_list: List of connector objects. Protected by - * @connector_list_lock. Only use drm_for_each_connector_iter() and + * @connector_list: + * + * List of connector objects linked with &drm_connector.head. Protected + * by @connector_list_lock. Only use drm_for_each_connector_iter() and * &struct drm_connector_list_iter to walk this list. */ struct list_head connector_list; + /** + * @num_encoder: + * + * Number of encoders on this device. This is invariant over the + * lifetime of a device and hence doesn't need any locks. + */ int num_encoder; + /** + * @encoder_list: + * + * List of encoder objects linked with &drm_encoder.head. This is + * invariant over the lifetime of a device and hence doesn't need any + * locks. + */ struct list_head encoder_list; - /* - * Track # of overlay planes separately from # of total planes. By - * default we only advertise overlay planes to userspace; if userspace - * sets the "universal plane" capability bit, we'll go ahead and - * expose all planes. + /** + * @num_overlay_plane: + * + * Number of overlay planes on this device, excluding primary and cursor + * planes. + * + * Track number of overlay planes separately from number of total + * planes. By default we only advertise overlay planes to userspace; if + * userspace sets the "universal plane" capability bit, we'll go ahead + * and expose all planes. This is invariant over the lifetime of a + * device and hence doesn't need any locks. */ int num_overlay_plane; + /** + * @num_total_plane: + * + * Number of universal (i.e. with primary/curso) planes on this device. + * This is invariant over the lifetime of a device and hence doesn't + * need any locks. + */ int num_total_plane; + /** + * @plane_list: + * + * List of plane objects linked with &drm_plane.head. This is invariant + * over the lifetime of a device and hence doesn't need any locks. + */ struct list_head plane_list; + /** + * @num_crtc: + * + * Number of CRTCs on this device linked with &drm_crtc.head. This is invariant over the lifetime + * of a device and hence doesn't need any locks. + */ int num_crtc; + /** + * @crtc_list: + * + * List of CRTC objects linked with &drm_crtc.head. This is invariant + * over the lifetime of a device and hence doesn't need any locks. + */ struct list_head crtc_list; + /** + * @property_list: + * + * List of property type objects linked with &drm_property.head. This is + * invariant over the lifetime of a device and hence doesn't need any + * locks. + */ struct list_head property_list; int min_width, min_height; @@ -427,10 +493,24 @@ struct drm_mode_config { bool delayed_event; struct delayed_work output_poll_work; + /** + * @blob_lock: + * + * Mutex for blob property allocation and management, protects + * @property_blob_list and &drm_file.blobs. + */ struct mutex blob_lock; - /* pointers to standard properties */ + /** + * @property_blob_list: + * + * List of all the blob property objects linked with + * &drm_property_blob.head. Protected by @blob_lock. + */ struct list_head property_blob_list; + + /* pointers to standard properties */ + /** * @edid_property: Default connector property to hold the EDID of the * currently connected sink, if any. diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 31da9f0c4ad2..7aedbf34f3a6 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -456,7 +456,6 @@ enum drm_plane_type { * @funcs: helper functions * @properties: property tracking for this plane * @type: type of plane (overlay, primary, cursor) - * @state: current atomic state for this plane * @zpos_property: zpos property for this plane * @rotation_property: rotation property for this plane * @helper_private: mid-layer private data @@ -473,6 +472,8 @@ struct drm_plane { * Protects modeset plane state, together with the &drm_crtc.mutex of * CRTC this plane is linked to (when active, getting activated or * getting disabled). + * + * For atomic drivers specifically this protects @state. */ struct drm_modeset_lock mutex; @@ -502,6 +503,19 @@ struct drm_plane { const struct drm_plane_helper_funcs *helper_private; + /** + * @state: + * + * Current atomic state for this plane. + * + * This is protected by @mutex. Note that nonblocking atomic commits + * access the current plane state without taking locks. Either by going + * through the &struct drm_atomic_state pointers, see + * for_each_plane_in_state(), for_each_oldnew_plane_in_state(), + * for_each_old_plane_in_state() and for_each_new_plane_in_state(). Or + * through careful ordering of atomic commit operations as implemented + * in the atomic helpers, see &struct drm_crtc_commit. + */ struct drm_plane_state *state; struct drm_property *zpos_property; -- 2.20.1