[PATCH 00/10] RFC: hot-unplug safe connector list locking

[PATCH 00/10] RFC: hot-unplug safe connector list locking

[Daniel Vetter]

Hi all,

This is a bit nightmare fuel, but I think it's the best we can aim for:
- locking doesn't leak to callers/drivers, no headaches with locking inversions
- it allows us (once fbdev is fixed) to drop the modeset_lock_all from mst
  unplug, cleaning up our locking hirarchy quite a bit

As a teaser of what this enables I've thrown three patches on top to drop a bit
of now superflous locking.

Todo:
- survive nightmares when reviewing this
- make sure all bits in i915 are covered (mostly depends upon Maarten's atomic
  iterator cleanup, which is progressing well)
- apply same love to amdgpu/radeon to handle hot-unplugging there too

Some of the other approaches I've discussed with Dave on irc, but didn't
implemented here:
- Go with a reallocating (rcu-protected) array instead of connector_list. Has
  the upside of catching every offender, but the downside of forcing churn onto
  every driver which doesn't care.
- Rework locking to again protect the connector_list with mode_config.mutex. Imo
  too hairy since that leaks the locking context out of every
  drm_connector_unreference (since it's no longer a pure leaf lock protecting
  things). Which tends to be real painful.
- Hope it works - we have the oopses already to prove otherwise :(

I think this implementation here strikes the best balance between invasiveness
and keeping locking concerns separated as much as possible. The one downside is
the implementation of the hot-unplug safe connector_list iterator.

Comments, screams of agony, testing and review highly welcome.

Cheers, Daniel

Daniel Vetter (10):
  drm/amd-kfd: Clean up inline handling
  drm: Don't compute obj counts expensively in get_resources
  drm: Add explicit iter struct to drm_for_each_connector
  drm/i915: Use use the drm_for_each_connector in i915_debugfs.c
  drm/i915: Roll out drm_for_each_connector in intel_hotplug.c
  drm: Drop cargo-culted modeset_lock_all from encoder/plane
    init/cleanup
  drm: Revamp connector_list protection
  drm: Drop mode_config.mutex from connector_register_all
  drm: Drop mode_config.mutex from get_resources ioctl
  drm: Drop mode_config.mutex from _reset()

 .../gpu/drm/amd/amdkfd/kfd_device_queue_manager.h  |   4 +-
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h              |   3 -
 drivers/gpu/drm/drm_atomic.c                       |   3 +-
 drivers/gpu/drm/drm_atomic_helper.c                |  12 ++-
 drivers/gpu/drm/drm_crtc.c                         | 109 ++++++++++++---------
 drivers/gpu/drm/drm_crtc_helper.c                  |  21 ++--
 drivers/gpu/drm/drm_edid.c                         |   3 +-
 drivers/gpu/drm/drm_fb_helper.c                    |   3 +-
 drivers/gpu/drm/drm_plane_helper.c                 |   3 +-
 drivers/gpu/drm/drm_probe_helper.c                 |   9 +-
 drivers/gpu/drm/exynos/exynos_drm_drv.c            |   6 +-
 drivers/gpu/drm/i915/i915_debugfs.c                |  22 ++---
 drivers/gpu/drm/i915/intel_display.c               |   6 +-
 drivers/gpu/drm/i915/intel_dp_mst.c                |   2 +-
 drivers/gpu/drm/i915/intel_hotplug.c               |  16 +--
 drivers/gpu/drm/vc4/vc4_crtc.c                     |   3 +-
 include/drm/drm_crtc.h                             |  79 +++++++++++----
 17 files changed, 189 insertions(+), 115 deletions(-)

-- 
2.8.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 01/10] drm/amd-kfd: Clean up inline handling

[Daniel Vetter]

- inline functions need to be static inline, otherwise gcc can opt to
  not inline and the linker gets unhappy.
- no forward decls for inline functions, just include the right headers.

Cc: Oded Gabbay <oded.gabbay@gmail.com>
Cc: Ben Goz <ben.goz@amd.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h | 4 ++--
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h                 | 3 ---
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
index ec4036a09f3e..a625b9137da2 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
@@ -187,12 +187,12 @@ int init_pipelines(struct device_queue_manager *dqm,
 unsigned int get_first_pipe(struct device_queue_manager *dqm);
 unsigned int get_pipes_num(struct device_queue_manager *dqm);
 
-extern inline unsigned int get_sh_mem_bases_32(struct kfd_process_device *pdd)
+static inline unsigned int get_sh_mem_bases_32(struct kfd_process_device *pdd)
 {
 	return (pdd->lds_base >> 16) & 0xFF;
 }
 
-extern inline unsigned int
+static inline unsigned int
 get_sh_mem_bases_nybble_64(struct kfd_process_device *pdd)
 {
 	return (pdd->lds_base >> 60) & 0x0E;
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index d0d5f4baf72d..80113c335966 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -617,10 +617,7 @@ int kgd2kfd_resume(struct kfd_dev *kfd);
 int kfd_init_apertures(struct kfd_process *process);
 
 /* Queue Context Management */
-inline uint32_t lower_32(uint64_t x);
-inline uint32_t upper_32(uint64_t x);
 struct cik_sdma_rlc_registers *get_sdma_mqd(void *mqd);
-inline uint32_t get_sdma_base_addr(struct cik_sdma_rlc_registers *m);
 
 int init_queue(struct queue **q, struct queue_properties properties);
 void uninit_queue(struct queue *q);
-- 
2.8.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 02/10] drm: Don't compute obj counts expensively in get_resources

[Daniel Vetter]

Looping when we keep track of this is silly. Only thing we have to
be careful is with sampling the connector count. To avoid inconsisten
results due to gcc re-computing this, use READ_ONCE.

And to avoid surprising userspace, make sure we don't copy more
connectors than planned, and report the actual number of connectors
copied. That way any racing hot-add/remove will be handled.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 28c109ff7330..4f18299dc630 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1842,10 +1842,10 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	struct drm_crtc *crtc;
 	struct drm_encoder *encoder;
 	int ret = 0;
-	int connector_count = 0;
-	int crtc_count = 0;
+	int connector_count = READ_ONCE(dev->mode_config.num_connector);
+	int crtc_count = dev->mode_config.num_crtc;
 	int fb_count = 0;
-	int encoder_count = 0;
+	int encoder_count = dev->mode_config.num_encoder;
 	int copied = 0;
 	uint32_t __user *fb_id;
 	uint32_t __user *crtc_id;
@@ -1883,15 +1883,6 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	/* mode_config.mutex protects the connector list against e.g. DP MST
 	 * connector hot-adding. CRTC/Plane lists are invariant. */
 	mutex_lock(&dev->mode_config.mutex);
-	drm_for_each_crtc(crtc, dev)
-		crtc_count++;
-
-	drm_for_each_connector(connector, dev)
-		connector_count++;
-
-	drm_for_each_encoder(encoder, dev)
-		encoder_count++;
-
 	card_res->max_height = dev->mode_config.max_height;
 	card_res->min_height = dev->mode_config.min_height;
 	card_res->max_width = dev->mode_config.max_width;
@@ -1938,6 +1929,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 			}
 			copied++;
 		}
+		connector_count = copied;
 	}
 	card_res->count_connectors = connector_count;
 
-- 
2.8.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 03/10] drm: Add explicit iter struct to drm_for_each_connector

[Daniel Vetter]

There's various proposals floating around for how to exactly fix
the connector_list locking issue. Most of them need a more fancy
iterator, with a bit more state. Prep for that by creating a
dummy struct drm_connector_iter and add is as a paramter everywhere.

To appease gcc have a dummy member and assign that - avoids a dreaded
unused variable warning.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_atomic.c            |  3 ++-
 drivers/gpu/drm/drm_atomic_helper.c     | 12 ++++++++----
 drivers/gpu/drm/drm_crtc.c              | 12 ++++++++----
 drivers/gpu/drm/drm_crtc_helper.c       | 21 +++++++++++++--------
 drivers/gpu/drm/drm_edid.c              |  3 ++-
 drivers/gpu/drm/drm_fb_helper.c         |  3 ++-
 drivers/gpu/drm/drm_plane_helper.c      |  3 ++-
 drivers/gpu/drm/drm_probe_helper.c      |  9 ++++++---
 drivers/gpu/drm/exynos/exynos_drm_drv.c |  6 ++++--
 drivers/gpu/drm/i915/intel_display.c    |  6 ++++--
 drivers/gpu/drm/vc4/vc4_crtc.c          |  3 ++-
 include/drm/drm_crtc.h                  |  9 +++++++--
 12 files changed, 60 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index dac0875e669c..24d473fbf6ae 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1224,6 +1224,7 @@ drm_atomic_add_affected_connectors(struct drm_atomic_state *state,
 	struct drm_mode_config *config = &state->dev->mode_config;
 	struct drm_connector *connector;
 	struct drm_connector_state *conn_state;
+	struct drm_connector_iter iter;
 	int ret;
 
 	ret = drm_modeset_lock(&config->connection_mutex, state->acquire_ctx);
@@ -1237,7 +1238,7 @@ drm_atomic_add_affected_connectors(struct drm_atomic_state *state,
 	 * Changed connectors are already in @state, so only need to look at the
 	 * current configuration.
 	 */
-	drm_for_each_connector(connector, state->dev) {
+	drm_for_each_connector(connector, state->dev, iter) {
 		if (connector->state->crtc != crtc)
 			continue;
 
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index de7fddce3cef..02eee8164a55 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -93,6 +93,7 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
 	struct drm_connector_state *conn_state;
 	struct drm_connector *connector;
 	struct drm_encoder *encoder;
+	struct drm_connector_iter iter;
 	unsigned encoder_mask = 0;
 	int i, ret;
 
@@ -142,7 +143,7 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
 	 * and the crtc is disabled if no encoder is left. This preserves
 	 * compatibility with the legacy set_config behavior.
 	 */
-	drm_for_each_connector(connector, state->dev) {
+	drm_for_each_connector(connector, state->dev, iter) {
 		struct drm_crtc_state *crtc_state;
 
 		if (drm_atomic_get_existing_connector_state(state, connector))
@@ -2390,6 +2391,7 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
 {
 	struct drm_atomic_state *state;
 	struct drm_connector *conn;
+	struct drm_connector_iter iter;
 	int err;
 
 	state = drm_atomic_state_alloc(dev);
@@ -2398,7 +2400,7 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
 
 	state->acquire_ctx = ctx;
 
-	drm_for_each_connector(conn, dev) {
+	drm_for_each_connector(conn, dev, iter) {
 		struct drm_crtc *crtc = conn->state->crtc;
 		struct drm_crtc_state *crtc_state;
 
@@ -2811,6 +2813,7 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
 	struct drm_crtc_state *crtc_state;
 	struct drm_crtc *crtc;
 	struct drm_connector *tmp_connector;
+	struct drm_connector_iter iter;
 	int ret;
 	bool active = false;
 	int old_mode = connector->dpms;
@@ -2838,7 +2841,7 @@ retry:
 
 	WARN_ON(!drm_modeset_is_locked(&config->connection_mutex));
 
-	drm_for_each_connector(tmp_connector, connector->dev) {
+	drm_for_each_connector(tmp_connector, connector->dev, iter) {
 		if (tmp_connector->state->crtc != crtc)
 			continue;
 
@@ -3225,6 +3228,7 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev,
 	struct drm_connector *conn;
 	struct drm_plane *plane;
 	struct drm_crtc *crtc;
+	struct drm_connector_iter iter;
 	int err = 0;
 
 	state = drm_atomic_state_alloc(dev);
@@ -3253,7 +3257,7 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev,
 		}
 	}
 
-	drm_for_each_connector(conn, dev) {
+	drm_for_each_connector(conn, dev, iter) {
 		struct drm_connector_state *conn_state;
 
 		conn_state = drm_atomic_get_connector_state(state, conn);
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 4f18299dc630..2a927488ec26 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1068,11 +1068,12 @@ EXPORT_SYMBOL(drm_connector_unregister);
 int drm_connector_register_all(struct drm_device *dev)
 {
 	struct drm_connector *connector;
+	struct drm_connector_iter iter;
 	int ret;
 
 	mutex_lock(&dev->mode_config.mutex);
 
-	drm_for_each_connector(connector, dev) {
+	drm_for_each_connector(connector, dev, iter) {
 		ret = drm_connector_register(connector);
 		if (ret)
 			goto err;
@@ -1841,6 +1842,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	struct drm_connector *connector;
 	struct drm_crtc *crtc;
 	struct drm_encoder *encoder;
+	struct drm_connector_iter iter;
 	int ret = 0;
 	int connector_count = READ_ONCE(dev->mode_config.num_connector);
 	int crtc_count = dev->mode_config.num_crtc;
@@ -1921,7 +1923,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	if (card_res->count_connectors >= connector_count) {
 		copied = 0;
 		connector_id = (uint32_t __user *)(unsigned long)card_res->connector_id_ptr;
-		drm_for_each_connector(connector, dev) {
+		drm_for_each_connector(connector, dev, iter) {
 			if (put_user(connector->base.id,
 				     connector_id + copied)) {
 				ret = -EFAULT;
@@ -2188,11 +2190,12 @@ static struct drm_crtc *drm_encoder_get_crtc(struct drm_encoder *encoder)
 {
 	struct drm_connector *connector;
 	struct drm_device *dev = encoder->dev;
+	struct drm_connector_iter iter;
 	bool uses_atomic = false;
 
 	/* For atomic drivers only state objects are synchronously updated and
 	 * protected by modeset locks, so check those first. */
-	drm_for_each_connector(connector, dev) {
+	drm_for_each_connector(connector, dev, iter) {
 		if (!connector->state)
 			continue;
 
@@ -5379,6 +5382,7 @@ void drm_mode_config_reset(struct drm_device *dev)
 	struct drm_plane *plane;
 	struct drm_encoder *encoder;
 	struct drm_connector *connector;
+	struct drm_connector_iter iter;
 
 	drm_for_each_plane(plane, dev)
 		if (plane->funcs->reset)
@@ -5393,7 +5397,7 @@ void drm_mode_config_reset(struct drm_device *dev)
 			encoder->funcs->reset(encoder);
 
 	mutex_lock(&dev->mode_config.mutex);
-	drm_for_each_connector(connector, dev)
+	drm_for_each_connector(connector, dev, iter)
 		if (connector->funcs->reset)
 			connector->funcs->reset(connector);
 	mutex_unlock(&dev->mode_config.mutex);
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 604d3ef72ffa..ef3ccbec8a5b 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -118,6 +118,7 @@ bool drm_helper_encoder_in_use(struct drm_encoder *encoder)
 {
 	struct drm_connector *connector;
 	struct drm_device *dev = encoder->dev;
+	struct drm_connector_iter iter;
 
 	/*
 	 * We can expect this mutex to be locked if we are not panicking.
@@ -128,7 +129,7 @@ bool drm_helper_encoder_in_use(struct drm_encoder *encoder)
 		WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 	}
 
-	drm_for_each_connector(connector, dev)
+	drm_for_each_connector(connector, dev, iter)
 		if (connector->encoder == encoder)
 			return true;
 	return false;
@@ -462,13 +463,14 @@ drm_crtc_helper_disable(struct drm_crtc *crtc)
 	struct drm_device *dev = crtc->dev;
 	struct drm_connector *connector;
 	struct drm_encoder *encoder;
+	struct drm_connector_iter iter;
 
 	/* Decouple all encoders and their attached connectors from this crtc */
 	drm_for_each_encoder(encoder, dev) {
 		if (encoder->crtc != crtc)
 			continue;
 
-		drm_for_each_connector(connector, dev) {
+		drm_for_each_connector(connector, dev, iter) {
 			if (connector->encoder != encoder)
 				continue;
 
@@ -539,6 +541,7 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
 	int count = 0, ro, fail = 0;
 	const struct drm_crtc_helper_funcs *crtc_funcs;
 	struct drm_mode_set save_set;
+	struct drm_connector_iter iter;
 	int ret;
 	int i;
 
@@ -600,7 +603,7 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
 	}
 
 	count = 0;
-	drm_for_each_connector(connector, dev) {
+	drm_for_each_connector(connector, dev, iter) {
 		save_connector_encoders[count++] = connector->encoder;
 	}
 
@@ -645,7 +648,7 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
 
 	/* a) traverse passed in connector list and get encoders for them */
 	count = 0;
-	drm_for_each_connector(connector, dev) {
+	drm_for_each_connector(connector, dev, iter) {
 		const struct drm_connector_helper_funcs *connector_funcs =
 			connector->helper_private;
 		new_encoder = connector->encoder;
@@ -685,7 +688,7 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
 	}
 
 	count = 0;
-	drm_for_each_connector(connector, dev) {
+	drm_for_each_connector(connector, dev, iter) {
 		if (!connector->encoder)
 			continue;
 
@@ -773,7 +776,7 @@ fail:
 	}
 
 	count = 0;
-	drm_for_each_connector(connector, dev) {
+	drm_for_each_connector(connector, dev, iter) {
 		connector->encoder = save_connector_encoders[count++];
 	}
 
@@ -803,8 +806,9 @@ static int drm_helper_choose_encoder_dpms(struct drm_encoder *encoder)
 	int dpms = DRM_MODE_DPMS_OFF;
 	struct drm_connector *connector;
 	struct drm_device *dev = encoder->dev;
+	struct drm_connector_iter iter;
 
-	drm_for_each_connector(connector, dev)
+	drm_for_each_connector(connector, dev, iter)
 		if (connector->encoder == encoder)
 			if (connector->dpms < dpms)
 				dpms = connector->dpms;
@@ -840,8 +844,9 @@ static int drm_helper_choose_crtc_dpms(struct drm_crtc *crtc)
 	int dpms = DRM_MODE_DPMS_OFF;
 	struct drm_connector *connector;
 	struct drm_device *dev = crtc->dev;
+	struct drm_connector_iter iter;
 
-	drm_for_each_connector(connector, dev)
+	drm_for_each_connector(connector, dev, iter)
 		if (connector->encoder && connector->encoder->crtc == crtc)
 			if (connector->dpms < dpms)
 				dpms = connector->dpms;
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 553390fae803..bc07e4817471 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3600,11 +3600,12 @@ struct drm_connector *drm_select_eld(struct drm_encoder *encoder)
 {
 	struct drm_connector *connector;
 	struct drm_device *dev = encoder->dev;
+	struct drm_connector_iter iter;
 
 	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
 	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
-	drm_for_each_connector(connector, dev)
+	drm_for_each_connector(connector, dev, iter)
 		if (connector->encoder == encoder && connector->eld[0])
 			return connector;
 
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 0a06f9120b5a..966ea19a02b8 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -113,13 +113,14 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
 {
 	struct drm_device *dev = fb_helper->dev;
 	struct drm_connector *connector;
+	struct drm_connector_iter iter;
 	int i, ret;
 
 	if (!drm_fbdev_emulation)
 		return 0;
 
 	mutex_lock(&dev->mode_config.mutex);
-	drm_for_each_connector(connector, dev) {
+	drm_for_each_connector(connector, dev, iter) {
 		ret = drm_fb_helper_add_one_connector(fb_helper, connector);
 
 		if (ret)
diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index 16c4a7bd7465..f334c70b1044 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -86,6 +86,7 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_connector *connector;
+	struct drm_connector_iter iter;
 	int count = 0;
 
 	/*
@@ -95,7 +96,7 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
 	 */
 	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
-	drm_for_each_connector(connector, dev) {
+	drm_for_each_connector(connector, dev, iter) {
 		if (connector->encoder && connector->encoder->crtc == crtc) {
 			if (connector_list != NULL && count < num_connectors)
 				*(connector_list++) = connector;
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index a0df377d7d1c..f113ed87fefb 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -129,13 +129,14 @@ void drm_kms_helper_poll_enable_locked(struct drm_device *dev)
 {
 	bool poll = false;
 	struct drm_connector *connector;
+	struct drm_connector_iter iter;
 
 	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
 
 	if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll)
 		return;
 
-	drm_for_each_connector(connector, dev) {
+	drm_for_each_connector(connector, dev, iter) {
 		if (connector->polled & (DRM_CONNECTOR_POLL_CONNECT |
 					 DRM_CONNECTOR_POLL_DISCONNECT))
 			poll = true;
@@ -368,6 +369,7 @@ static void output_poll_execute(struct work_struct *work)
 	struct delayed_work *delayed_work = to_delayed_work(work);
 	struct drm_device *dev = container_of(delayed_work, struct drm_device, mode_config.output_poll_work);
 	struct drm_connector *connector;
+	struct drm_connector_iter iter;
 	enum drm_connector_status old_status;
 	bool repoll = false, changed;
 
@@ -379,7 +381,7 @@ static void output_poll_execute(struct work_struct *work)
 		goto out;
 
 	mutex_lock(&dev->mode_config.mutex);
-	drm_for_each_connector(connector, dev) {
+	drm_for_each_connector(connector, dev, iter) {
 
 		/* Ignore forced connectors. */
 		if (connector->force)
@@ -545,13 +547,14 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev)
 {
 	struct drm_connector *connector;
 	enum drm_connector_status old_status;
+	struct drm_connector_iter iter;
 	bool changed = false;
 
 	if (!dev->mode_config.poll_enabled)
 		return false;
 
 	mutex_lock(&dev->mode_config.mutex);
-	drm_for_each_connector(connector, dev) {
+	drm_for_each_connector(connector, dev, iter) {
 
 		/* Only handle HPD capable connectors. */
 		if (!(connector->polled & DRM_CONNECTOR_POLL_HPD))
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 13d28d4229e2..4f4fa0e17e82 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -439,12 +439,13 @@ static int exynos_drm_suspend(struct device *dev)
 {
 	struct drm_device *drm_dev = dev_get_drvdata(dev);
 	struct drm_connector *connector;
+	struct drm_connector_iter iter;
 
 	if (pm_runtime_suspended(dev) || !drm_dev)
 		return 0;
 
 	drm_modeset_lock_all(drm_dev);
-	drm_for_each_connector(connector, drm_dev) {
+	drm_for_each_connector(connector, drm_dev, iter) {
 		int old_dpms = connector->dpms;
 
 		if (connector->funcs->dpms)
@@ -462,12 +463,13 @@ static int exynos_drm_resume(struct device *dev)
 {
 	struct drm_device *drm_dev = dev_get_drvdata(dev);
 	struct drm_connector *connector;
+	struct drm_connector_iter iter;
 
 	if (pm_runtime_suspended(dev) || !drm_dev)
 		return 0;
 
 	drm_modeset_lock_all(drm_dev);
-	drm_for_each_connector(connector, drm_dev) {
+	drm_for_each_connector(connector, drm_dev, iter) {
 		if (connector->funcs->dpms) {
 			int dpms = connector->dpms;
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0b2cd669ac05..dc33bf278cc2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12412,6 +12412,7 @@ static bool check_digital_port_conflicts(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
 	struct drm_connector *connector;
+	struct drm_connector_iter iter;
 	unsigned int used_ports = 0;
 
 	/*
@@ -12419,7 +12420,7 @@ static bool check_digital_port_conflicts(struct drm_atomic_state *state)
 	 * list to detect the problem on ddi platforms
 	 * where there's just one encoder per digital port.
 	 */
-	drm_for_each_connector(connector, dev) {
+	drm_for_each_connector(connector, dev, iter) {
 		struct drm_connector_state *connector_state;
 		struct intel_encoder *encoder;
 
@@ -12997,8 +12998,9 @@ static void
 verify_connector_state(struct drm_device *dev, struct drm_crtc *crtc)
 {
 	struct drm_connector *connector;
+	struct drm_connector_iter iter;
 
-	drm_for_each_connector(connector, dev) {
+	drm_for_each_connector(connector, dev, iter) {
 		struct drm_encoder *encoder = connector->encoder;
 		struct drm_connector_state *state = connector->state;
 
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index c82d468d178b..72f2962e9d70 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -218,8 +218,9 @@ static u32 vc4_get_fifo_full_level(u32 format)
 static int vc4_get_clock_select(struct drm_crtc *crtc)
 {
 	struct drm_connector *connector;
+	struct drm_connector_iter iter;
 
-	drm_for_each_connector(connector, crtc->dev) {
+	drm_for_each_connector(connector, crtc->dev, iter) {
 		if (connector->state->crtc == crtc) {
 			struct drm_encoder *encoder = connector->encoder;
 			struct vc4_encoder *vc4_encoder =
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index efef0614703a..ba574b9c1ec4 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -2849,8 +2849,13 @@ assert_drm_connector_list_read_locked(struct drm_mode_config *mode_config)
 		!drm_modeset_is_locked(&mode_config->connection_mutex));
 }
 
-#define drm_for_each_connector(connector, dev) \
-	for (assert_drm_connector_list_read_locked(&(dev)->mode_config),	\
+struct drm_connector_iter {
+	struct drm_mode_config *mode_config;
+};
+
+#define drm_for_each_connector(connector, dev, iter) \
+	for ((iter).mode_config = NULL,						\
+	     assert_drm_connector_list_read_locked(&(dev)->mode_config),	\
 	     connector = list_first_entry(&(dev)->mode_config.connector_list,	\
 					  struct drm_connector, head);		\
 	     &connector->head != (&(dev)->mode_config.connector_list);		\
-- 
2.8.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 04/10] drm/i915: Use use the drm_for_each_connector in i915_debugfs.c

[Daniel Vetter]

I noticed that we don't have any locking. drm_for_each_connector will
soon encapsulate late, so just roll that macro out as prep work.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 5b7526697838..ea0e6cf2dc07 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4364,14 +4364,12 @@ static ssize_t i915_displayport_test_active_write(struct file *file,
 	int status = 0;
 	struct drm_device *dev;
 	struct drm_connector *connector;
-	struct list_head *connector_list;
+	struct drm_connector_iter iter;
 	struct intel_dp *intel_dp;
 	int val = 0;
 
 	dev = ((struct seq_file *)file->private_data)->private;
 
-	connector_list = &dev->mode_config.connector_list;
-
 	if (len == 0)
 		return 0;
 
@@ -4387,8 +4385,7 @@ static ssize_t i915_displayport_test_active_write(struct file *file,
 	input_buffer[len] = '\0';
 	DRM_DEBUG_DRIVER("Copied %d bytes from user\n", (unsigned int)len);
 
-	list_for_each_entry(connector, connector_list, head) {
-
+	drm_for_each_connector(connector, dev, iter) {
 		if (connector->connector_type !=
 		    DRM_MODE_CONNECTOR_DisplayPort)
 			continue;
@@ -4422,11 +4419,10 @@ static int i915_displayport_test_active_show(struct seq_file *m, void *data)
 {
 	struct drm_device *dev = m->private;
 	struct drm_connector *connector;
-	struct list_head *connector_list = &dev->mode_config.connector_list;
 	struct intel_dp *intel_dp;
+	struct drm_connector_iter iter;
 
-	list_for_each_entry(connector, connector_list, head) {
-
+	drm_for_each_connector(connector, dev, iter) {
 		if (connector->connector_type !=
 		    DRM_MODE_CONNECTOR_DisplayPort)
 			continue;
@@ -4466,11 +4462,10 @@ static int i915_displayport_test_data_show(struct seq_file *m, void *data)
 {
 	struct drm_device *dev = m->private;
 	struct drm_connector *connector;
-	struct list_head *connector_list = &dev->mode_config.connector_list;
+	struct drm_connector_iter iter;
 	struct intel_dp *intel_dp;
 
-	list_for_each_entry(connector, connector_list, head) {
-
+	drm_for_each_connector(connector, dev, iter) {
 		if (connector->connector_type !=
 		    DRM_MODE_CONNECTOR_DisplayPort)
 			continue;
@@ -4505,11 +4500,10 @@ static int i915_displayport_test_type_show(struct seq_file *m, void *data)
 {
 	struct drm_device *dev = m->private;
 	struct drm_connector *connector;
-	struct list_head *connector_list = &dev->mode_config.connector_list;
 	struct intel_dp *intel_dp;
+	struct drm_connector_iter iter;
 
-	list_for_each_entry(connector, connector_list, head) {
-
+	drm_for_each_connector(connector, dev, iter) {
 		if (connector->connector_type !=
 		    DRM_MODE_CONNECTOR_DisplayPort)
 			continue;
-- 
2.8.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 05/10] drm/i915: Roll out drm_for_each_connector in intel_hotplug.c

[Daniel Vetter]

Just prep work to for the revamped connector_list locking.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_hotplug.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
index 38eeca7a6e72..e537c03ef6ee 100644
--- a/drivers/gpu/drm/i915/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/intel_hotplug.c
@@ -145,16 +145,16 @@ static bool intel_hpd_irq_storm_detect(struct drm_i915_private *dev_priv,
 static void intel_hpd_irq_storm_disable(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
-	struct drm_mode_config *mode_config = &dev->mode_config;
 	struct intel_connector *intel_connector;
 	struct intel_encoder *intel_encoder;
 	struct drm_connector *connector;
 	enum hpd_pin pin;
 	bool hpd_disabled = false;
+	struct drm_connector_iter iter;
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	list_for_each_entry(connector, &mode_config->connector_list, head) {
+	drm_for_each_connector(connector, dev, iter) {
 		if (connector->polled != DRM_CONNECTOR_POLL_HPD)
 			continue;
 
@@ -192,7 +192,7 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work)
 		container_of(work, typeof(*dev_priv),
 			     hotplug.reenable_work.work);
 	struct drm_device *dev = dev_priv->dev;
-	struct drm_mode_config *mode_config = &dev->mode_config;
+	struct drm_connector_iter iter;
 	int i;
 
 	intel_runtime_pm_get(dev_priv);
@@ -206,7 +206,7 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work)
 
 		dev_priv->hotplug.stats[i].state = HPD_ENABLED;
 
-		list_for_each_entry(connector, &mode_config->connector_list, head) {
+		drm_for_each_connector(connector, dev, iter) {
 			struct intel_connector *intel_connector = to_intel_connector(connector);
 
 			if (intel_connector->encoder->hpd_pin == i) {
@@ -309,6 +309,7 @@ static void i915_hotplug_work_func(struct work_struct *work)
 	struct drm_connector *connector;
 	bool changed = false;
 	u32 hpd_event_bits;
+	struct drm_connector_iter iter;
 
 	mutex_lock(&mode_config->mutex);
 	DRM_DEBUG_KMS("running encoder hotplug functions\n");
@@ -323,7 +324,7 @@ static void i915_hotplug_work_func(struct work_struct *work)
 
 	spin_unlock_irq(&dev_priv->irq_lock);
 
-	list_for_each_entry(connector, &mode_config->connector_list, head) {
+	drm_for_each_connector(connector, dev, iter) {
 		intel_connector = to_intel_connector(connector);
 		if (!intel_connector->encoder)
 			continue;
@@ -456,15 +457,16 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
 void intel_hpd_init(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
-	struct drm_mode_config *mode_config = &dev->mode_config;
 	struct drm_connector *connector;
+	struct drm_connector_iter iter;
 	int i;
 
 	for_each_hpd_pin(i) {
 		dev_priv->hotplug.stats[i].count = 0;
 		dev_priv->hotplug.stats[i].state = HPD_ENABLED;
 	}
-	list_for_each_entry(connector, &mode_config->connector_list, head) {
+
+	drm_for_each_connector(connector, dev, iter) {
 		struct intel_connector *intel_connector = to_intel_connector(connector);
 		connector->polled = intel_connector->polled;
 
-- 
2.8.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 06/10] drm: Drop cargo-culted modeset_lock_all from encoder/plane init/cleanup

[Daniel Vetter]

We can't hotplug encoders/planes, there's no point in that locking. It
was also inconsistent because lacking from plane_init.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 2a927488ec26..6a8f91e8821b 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1131,11 +1131,9 @@ int drm_encoder_init(struct drm_device *dev,
 {
 	int ret;
 
-	drm_modeset_lock_all(dev);
-
 	ret = drm_mode_object_get(dev, &encoder->base, DRM_MODE_OBJECT_ENCODER);
 	if (ret)
-		goto out_unlock;
+		return ret;
 
 	encoder->dev = dev;
 	encoder->encoder_type = encoder_type;
@@ -1163,9 +1161,6 @@ out_put:
 	if (ret)
 		drm_mode_object_unregister(dev, &encoder->base);
 
-out_unlock:
-	drm_modeset_unlock_all(dev);
-
 	return ret;
 }
 EXPORT_SYMBOL(drm_encoder_init);
@@ -1185,12 +1180,10 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
 	 * the indices on the drm_encoder after us in the encoder_list.
 	 */
 
-	drm_modeset_lock_all(dev);
 	drm_mode_object_unregister(dev, &encoder->base);
 	kfree(encoder->name);
 	list_del(&encoder->head);
 	dev->mode_config.num_encoder--;
-	drm_modeset_unlock_all(dev);
 
 	memset(encoder, 0, sizeof(*encoder));
 }
@@ -1341,7 +1334,6 @@ void drm_plane_cleanup(struct drm_plane *plane)
 {
 	struct drm_device *dev = plane->dev;
 
-	drm_modeset_lock_all(dev);
 	kfree(plane->format_types);
 	drm_mode_object_unregister(dev, &plane->base);
 
@@ -1356,7 +1348,6 @@ void drm_plane_cleanup(struct drm_plane *plane)
 	dev->mode_config.num_total_plane--;
 	if (plane->type == DRM_PLANE_TYPE_OVERLAY)
 		dev->mode_config.num_overlay_plane--;
-	drm_modeset_unlock_all(dev);
 
 	WARN_ON(plane->state && !plane->funcs->atomic_destroy_state);
 	if (plane->state && plane->funcs->atomic_destroy_state)
-- 
2.8.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 07/10] drm: Revamp connector_list protection

[Daniel Vetter]

This is a pretty good horror show, but I think it's the best tradeoff:
- Thanks to srcu and delayed freeing the locking doesn't leak out to
  callers, hence no added headaches with locking inversions.
- For core and drivers which hot-remove connectors all the connector
  list walking details are hidden in a macro.
- Other drivers which don't care about hot-removing can simply keep on
  using the normal list walking macros.

The new design is:
- New dev->mode_config.connector_list_lock to protect the
  connector_list, and the connector_ida (since that's also
  unprotected), plus num_connectors. This is a pure leaf lock, nothing
  is allowed to nest within, nothing outside of connector init/cleanup
  will ever need it.
- Protect connector_list itself with srcu. This allows sleeping and
  anything else. The only thing which is not allowed is calling
  synchronize_srcu, or grabbing any locks or waiting on
  waitqueues/completions/whatever which might call that. To make this
  100% safe we opt to not have any calls to synchronize_srcu.
- Protect against use-after-free of connectors using call_srcu, to
  delay the kfree enough.
- To protect against zombie connectors which are in progress of final
  destruction use kref_get_unless_zero. This is safe since srcu
  prevents against use-after-free, and that's the only guarantee we
  need.

For this demo I only bothered converting i915, and there also not
everything - I left the connector loops in the modeset code unchanged
since those will all be converted over to
drm_for_each_connector_in_state to make them work correctly for
nonblocking atomic commits. Only loops outside of atomic commits
should be converted to drm_for_each_connector.

Note that the i915 DP MST code still uses drm_modeset_lock_all(). But
that's not because of drm core connector lifetime issues, but because
the fbdev helper reuses core locks to mange its own lists and data
structures. Thierry Reding has a patch series to gift fbdev its own
lock, which will remedy this.

v2: Totally rewrite everything to pack it up in a neat
iterator macro, with init/check/next extracted into helpers.

v3: Tiny rebase conflict.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_crtc.c          | 57 +++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_dp_mst.c |  2 +-
 include/drm/drm_crtc.h              | 82 +++++++++++++++++++++++++++----------
 3 files changed, 109 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 6a8f91e8821b..dc22c0bfbe99 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -44,6 +44,9 @@
 #include "drm_crtc_internal.h"
 #include "drm_internal.h"
 
+DEFINE_SRCU(drm_connector_list_srcu);
+EXPORT_SYMBOL(drm_connector_list_srcu);
+
 static struct drm_framebuffer *
 internal_framebuffer_create(struct drm_device *dev,
 			    const struct drm_mode_fb_cmd2 *r,
@@ -825,7 +828,7 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector)
 		      mode->interlace ?  " interlaced" : "");
 }
 
-static void drm_connector_free(struct kref *kref)
+static void drm_connector_kref_release(struct kref *kref)
 {
 	struct drm_connector *connector =
 		container_of(kref, struct drm_connector, base.refcount);
@@ -858,11 +861,10 @@ int drm_connector_init(struct drm_device *dev,
 	struct ida *connector_ida =
 		&drm_connector_enum_list[connector_type].ida;
 
-	drm_modeset_lock_all(dev);
-
+	mutex_lock(&dev->mode_config.connector_list_lock);
 	ret = drm_mode_object_get_reg(dev, &connector->base,
 				      DRM_MODE_OBJECT_CONNECTOR,
-				      false, drm_connector_free);
+				      false, drm_connector_kref_release);
 	if (ret)
 		goto out_unlock;
 
@@ -899,11 +901,6 @@ int drm_connector_init(struct drm_device *dev,
 
 	drm_connector_get_cmdline_mode(connector);
 
-	/* We should add connectors at the end to avoid upsetting the connector
-	 * index too much. */
-	list_add_tail(&connector->head, &config->connector_list);
-	config->num_connector++;
-
 	if (connector_type != DRM_MODE_CONNECTOR_VIRTUAL)
 		drm_object_attach_property(&connector->base,
 					      config->edid_property,
@@ -917,6 +914,11 @@ int drm_connector_init(struct drm_device *dev,
 	}
 
 	connector->debugfs_entry = NULL;
+
+	/* We must add the connector to the list last. */
+	list_add_tail_rcu(&connector->head, &config->connector_list);
+	config->num_connector++;
+
 out_put_type_id:
 	if (ret)
 		ida_remove(connector_ida, connector->connector_type_id);
@@ -928,7 +930,7 @@ out_put:
 		drm_mode_object_unregister(dev, &connector->base);
 
 out_unlock:
-	drm_modeset_unlock_all(dev);
+	mutex_unlock(&dev->mode_config.connector_list_lock);
 
 	return ret;
 }
@@ -951,6 +953,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
 	if (WARN_ON(connector->registered))
 		drm_connector_unregister(connector);
 
+	mutex_lock(&dev->mode_config.connector_list_lock);
 	if (connector->tile_group) {
 		drm_mode_put_tile_group(dev, connector->tile_group);
 		connector->tile_group = NULL;
@@ -981,9 +984,42 @@ void drm_connector_cleanup(struct drm_connector *connector)
 						       connector->state);
 
 	memset(connector, 0, sizeof(*connector));
+	mutex_unlock(&dev->mode_config.connector_list_lock);
 }
 EXPORT_SYMBOL(drm_connector_cleanup);
 
+static void drm_connector_free_cb(struct rcu_head *head)
+{
+	struct drm_connector *connector = container_of(head,
+						       struct drm_connector,
+						       free_head);
+
+	kfree(connector);
+}
+
+/**
+ * drm_connector_free - frees a connector structure
+ * @connector: connector to free
+ *
+ * This frees @connector using call_srcu() and kfree(). If the driver subclasses
+ * the connector, then the embedded struct &drm_connector must be the first
+ * element, and @connector must have been allocated using kmalloc() or one of
+ * the functions wrapping that.
+ *
+ * Delayed freeing is required to avoid use-after-free when walking the
+ * connector list, which is srcu protected. Hence drivers must use this function
+ * for connectors which can be removed while the driver is loaded. Currently
+ * that's only true for DP MST connectors. For any other connectors it is valid
+ * to call kfree directly.
+ */
+void drm_connector_free(struct drm_connector *connector)
+{
+	call_srcu(&drm_connector_list_srcu,
+		  &connector->free_head,
+		  drm_connector_free_cb);
+}
+EXPORT_SYMBOL(drm_connector_free);
+
 /**
  * drm_connector_register - register a connector
  * @connector: the connector to register
@@ -5554,6 +5590,7 @@ void drm_mode_config_init(struct drm_device *dev)
 	mutex_init(&dev->mode_config.idr_mutex);
 	mutex_init(&dev->mode_config.fb_lock);
 	mutex_init(&dev->mode_config.blob_lock);
+	mutex_init(&dev->mode_config.connector_list_lock);
 	INIT_LIST_HEAD(&dev->mode_config.fb_list);
 	INIT_LIST_HEAD(&dev->mode_config.crtc_list);
 	INIT_LIST_HEAD(&dev->mode_config.connector_list);
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 9646816604be..11bbbb194f2d 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -327,7 +327,7 @@ intel_dp_mst_connector_destroy(struct drm_connector *connector)
 		kfree(intel_connector->edid);
 
 	drm_connector_cleanup(connector);
-	kfree(connector);
+	drm_connector_free(connector);
 }
 
 static const struct drm_connector_funcs intel_dp_mst_connector_funcs = {
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index ba574b9c1ec4..8f4380ac113e 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -32,6 +32,7 @@
 #include <linux/fb.h>
 #include <linux/hdmi.h>
 #include <linux/media-bus-format.h>
+#include <linux/srcu.h>
 #include <uapi/drm/drm_mode.h>
 #include <uapi/drm/drm_fourcc.h>
 #include <drm/drm_modeset_lock.h>
@@ -1186,6 +1187,7 @@ struct drm_encoder {
  * @kdev: kernel device for sysfs attributes
  * @attr: sysfs attributes
  * @head: list management
+ * @free_head: rcu delayed freeing management
  * @base: base KMS object
  * @name: human readable name, can be overwritten by the driver
  * @connector_id: compacted connector id useful indexing arrays
@@ -1241,6 +1243,7 @@ struct drm_connector {
 	struct device *kdev;
 	struct device_attribute *attr;
 	struct list_head head;
+	struct rcu_head free_head;
 
 	struct drm_mode_object base;
 
@@ -2309,7 +2312,7 @@ struct drm_mode_config {
 	 * @tile_idr:
 	 *
 	 * Use this idr for allocating new IDs for tiled sinks like use in some
-	 * high-res DP MST screens.
+	 * high-res DP MST screens. Protected by @idr_mutex.
 	 */
 	struct idr tile_idr;
 
@@ -2320,6 +2323,14 @@ struct drm_mode_config {
 	int num_connector;
 	struct ida connector_ida;
 	struct list_head connector_list;
+
+	/**
+	 * @connector_list_lock:
+	 *
+	 * Protects @connector_list, @connector_ida and * @num_conncetors.
+	 */
+	struct mutex connector_list_lock;
+
 	int num_encoder;
 	struct list_head encoder_list;
 
@@ -2426,6 +2437,8 @@ struct drm_mode_config {
 	struct drm_mode_config_helper_funcs *helper_private;
 };
 
+extern struct srcu_struct drm_connector_list_srcu;
+
 /**
  * drm_for_each_plane_mask - iterate over planes specified by bitmask
  * @plane: the loop cursor
@@ -2505,6 +2518,7 @@ int drm_connector_register(struct drm_connector *connector);
 void drm_connector_unregister(struct drm_connector *connector);
 
 extern void drm_connector_cleanup(struct drm_connector *connector);
+extern void drm_connector_free(struct drm_connector *connector);
 static inline unsigned drm_connector_index(struct drm_connector *connector)
 {
 	return connector->connector_id;
@@ -2835,31 +2849,57 @@ static inline void drm_connector_unreference(struct drm_connector *connector)
 #define drm_for_each_crtc(crtc, dev) \
 	list_for_each_entry(crtc, &(dev)->mode_config.crtc_list, head)
 
+struct drm_connector_iter {
+	struct drm_mode_config *mode_config;
+	struct drm_connector *connector;
+	int srcu_ret;
+};
+
 static inline void
-assert_drm_connector_list_read_locked(struct drm_mode_config *mode_config)
+__drm_connector_iter_init(struct drm_mode_config *mode_config,
+			  struct drm_connector_iter *iter)
 {
-	/*
-	 * The connector hotadd/remove code currently grabs both locks when
-	 * updating lists. Hence readers need only hold either of them to be
-	 * safe and the check amounts to
-	 *
-	 * WARN_ON(not_holding(A) && not_holding(B)).
-	 */
-	WARN_ON(!mutex_is_locked(&mode_config->mutex) &&
-		!drm_modeset_is_locked(&mode_config->connection_mutex));
+	iter->mode_config = mode_config;
+	iter->srcu_ret = srcu_read_lock(&drm_connector_list_srcu);
+	iter->connector = list_entry_rcu(mode_config->connector_list.next,
+					 struct drm_connector, head);
 }
 
-struct drm_connector_iter {
-	struct drm_mode_config *mode_config;
-};
+static inline struct drm_connector *
+__drm_connector_iter_check(struct drm_connector_iter *iter)
+{
+retry:
+	if (&iter->connector->head == &iter->mode_config->connector_list) {
+		/* last iteration, clean up */
+		srcu_read_unlock(&drm_connector_list_srcu, iter->srcu_ret);
+		return NULL;
+	}
+
+	/* srcu protects against iter->connector disappearing */
+	if (!kref_get_unless_zero(&iter->connector->base.refcount)) {
+		iter->connector = list_entry_rcu(iter->connector->head.next,
+						 struct drm_connector, head);
+		goto retry;
+	}
+
+	return iter->connector;
+}
+
+static inline void
+__drm_connector_iter_next(struct drm_connector_iter *iter)
+{
+	/* _next() is only called when _check() succeeded on iter->connector,
+	 * and _check() only succeeds if kref_get_unless_zero() succeeded.
+	 * Therefore dropping the reference here is the correct place. */
+	drm_connector_unreference(iter->connector);
+	iter->connector = list_entry_rcu(iter->connector->head.next,
+					 struct drm_connector, head);
+}
 
-#define drm_for_each_connector(connector, dev, iter) \
-	for ((iter).mode_config = NULL,						\
-	     assert_drm_connector_list_read_locked(&(dev)->mode_config),	\
-	     connector = list_first_entry(&(dev)->mode_config.connector_list,	\
-					  struct drm_connector, head);		\
-	     &connector->head != (&(dev)->mode_config.connector_list);		\
-	     connector = list_next_entry(connector, head))
+#define drm_for_each_connector(connector, dev, iter) 			\
+	for (__drm_connector_iter_init(&(dev)->mode_config, &(iter));	\
+	     (connector = __drm_connector_iter_check(&(iter)));		\
+	     __drm_connector_iter_next(&(iter)))
 
 #define drm_for_each_encoder(encoder, dev) \
 	list_for_each_entry(encoder, &(dev)->mode_config.encoder_list, head)
-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 08/10] drm: Drop mode_config.mutex from connector_register_all

[Daniel Vetter]

With the reworked connector_list locking we don't need this
any more. Also, we can remove the FIXME comment from the unregister
function, too, by using drm_for_each_connector.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index dc22c0bfbe99..d104717cab6b 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1107,20 +1107,15 @@ int drm_connector_register_all(struct drm_device *dev)
 	struct drm_connector_iter iter;
 	int ret;
 
-	mutex_lock(&dev->mode_config.mutex);
-
 	drm_for_each_connector(connector, dev, iter) {
 		ret = drm_connector_register(connector);
 		if (ret)
 			goto err;
 	}
 
-	mutex_unlock(&dev->mode_config.mutex);
-
 	return 0;
 
 err:
-	mutex_unlock(&dev->mode_config.mutex);
 	drm_connector_unregister_all(dev);
 	return ret;
 }
@@ -1139,9 +1134,9 @@ EXPORT_SYMBOL(drm_connector_register_all);
 void drm_connector_unregister_all(struct drm_device *dev)
 {
 	struct drm_connector *connector;
+	struct drm_connector_iter iter;
 
-	/* FIXME: taking the mode config mutex ends up in a clash with sysfs */
-	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
+	drm_for_each_connector(connector, dev, iter)
 		drm_connector_unregister(connector);
 }
 EXPORT_SYMBOL(drm_connector_unregister_all);
-- 
2.8.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 09/10] drm: Drop mode_config.mutex from get_resources ioctl

[Daniel Vetter]

The only thing this protected is the connector_list, which is
now protected differently.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index d104717cab6b..0a678cfd9920 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1906,7 +1906,6 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 
 	/* mode_config.mutex protects the connector list against e.g. DP MST
 	 * connector hot-adding. CRTC/Plane lists are invariant. */
-	mutex_lock(&dev->mode_config.mutex);
 	card_res->max_height = dev->mode_config.max_height;
 	card_res->min_height = dev->mode_config.min_height;
 	card_res->max_width = dev->mode_config.max_width;
@@ -1958,7 +1957,6 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	card_res->count_connectors = connector_count;
 
 out:
-	mutex_unlock(&dev->mode_config.mutex);
 	return ret;
 }
 
-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 10/10] drm: Drop mode_config.mutex from _reset()

[Daniel Vetter]

Again, we now have a safe way to iterate over the connector list
and can remove the locking.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 0a678cfd9920..1064a41ed38b 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -5416,11 +5416,9 @@ void drm_mode_config_reset(struct drm_device *dev)
 		if (encoder->funcs->reset)
 			encoder->funcs->reset(encoder);
 
-	mutex_lock(&dev->mode_config.mutex);
 	drm_for_each_connector(connector, dev, iter)
 		if (connector->funcs->reset)
 			connector->funcs->reset(connector);
-	mutex_unlock(&dev->mode_config.mutex);
 }
 EXPORT_SYMBOL(drm_mode_config_reset);
 
-- 
2.8.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 09/10] drm: Drop mode_config.mutex from get_resources ioctl

[Chris Wilson]

On Tue, Jun 21, 2016 at 11:10:34AM +0200, Daniel Vetter wrote:
> The only thing this protected is the connector_list, which is
> now protected differently.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_crtc.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index d104717cab6b..0a678cfd9920 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1906,7 +1906,6 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
>  
>  	/* mode_config.mutex protects the connector list against e.g. DP MST
>  	 * connector hot-adding. CRTC/Plane lists are invariant. */
> -	mutex_lock(&dev->mode_config.mutex);

And the comment needs removal.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/10] drm: Don't compute obj counts expensively in get_resources

[Chris Wilson]

On Tue, Jun 21, 2016 at 11:10:27AM +0200, Daniel Vetter wrote:
> And to avoid surprising userspace, make sure we don't copy more
> connectors than planned, and report the actual number of connectors
> copied. That way any racing hot-add/remove will be handled.

> @@ -1938,6 +1929,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
>  			}
>  			copied++;
>  		}
> +		connector_count = copied;

You forgot to actually make sure we don't copy more conectors than
planned.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Ro.CI.BAT: warning for RFC: hot-unplug safe connector list locking

[Patchwork]

== Series Details ==

Series: RFC: hot-unplug safe connector list locking
URL   : https://patchwork.freedesktop.org/series/8974/
State : warning

== Summary ==

Series 8974v1 RFC: hot-unplug safe connector list locking
http://patchwork.freedesktop.org/api/1.0/series/8974/revisions/1/mbox

Test gem_ctx_basic:
                pass       -> DMESG-WARN (ro-hsw-i3-4010u)
Test gem_ctx_create:
        Subgroup basic:
                pass       -> DMESG-WARN (ro-hsw-i3-4010u)
        Subgroup basic-files:
                pass       -> DMESG-WARN (ro-hsw-i3-4010u)
Test gem_ctx_switch:
        Subgroup basic-default:
                pass       -> DMESG-WARN (ro-hsw-i3-4010u)
Test gem_storedw_loop:
        Subgroup basic-blt:
                pass       -> DMESG-WARN (ro-hsw-i3-4010u)
        Subgroup basic-bsd:
                pass       -> DMESG-WARN (ro-hsw-i3-4010u)
        Subgroup basic-vebox:
                pass       -> DMESG-WARN (ro-hsw-i3-4010u)
Test gem_sync:
        Subgroup basic-all:
                pass       -> DMESG-WARN (ro-hsw-i3-4010u)
        Subgroup basic-each:
                pass       -> DMESG-WARN (ro-hsw-i3-4010u)
Test gem_tiled_fence_blits:
        Subgroup basic:
                pass       -> DMESG-WARN (ro-hsw-i3-4010u)
Test gem_tiled_pread_basic:
                pass       -> DMESG-WARN (ro-hsw-i3-4010u)
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (ro-snb-i7-2620M)
Test kms_force_connector_basic:
        Subgroup force-connector-state:
                pass       -> DMESG-WARN (ro-ivb2-i7-3770)
                pass       -> DMESG-WARN (ro-ilk1-i5-650)
                pass       -> DMESG-WARN (ro-snb-i7-2620M)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                pass       -> SKIP       (ro-bdw-i7-5557U)
Test kms_sink_crc_basic:
                pass       -> DMESG-WARN (ro-bdw-i7-5600u)

fi-skl-i5-6260u  total:225  pass:182  dwarn:18  dfail:0   fail:2   skip:23 
fi-skl-i7-6700k  total:225  pass:186  dwarn:0   dfail:0   fail:2   skip:37 
ro-bdw-i7-5557U  total:225  pass:198  dwarn:0   dfail:0   fail:0   skip:27 
ro-bdw-i7-5600u  total:225  pass:184  dwarn:1   dfail:0   fail:0   skip:40 
ro-byt-n2820     total:225  pass:173  dwarn:0   dfail:0   fail:3   skip:49 
ro-hsw-i3-4010u  total:225  pass:179  dwarn:11  dfail:0   fail:0   skip:35 
ro-hsw-i7-4770r  total:225  pass:190  dwarn:0   dfail:0   fail:0   skip:35 
ro-ilk-i7-620lm  total:225  pass:150  dwarn:0   dfail:0   fail:1   skip:74 
ro-ilk1-i5-650   total:220  pass:149  dwarn:1   dfail:0   fail:1   skip:69 
ro-ivb-i7-3770   total:225  pass:181  dwarn:0   dfail:0   fail:0   skip:44 
ro-ivb2-i7-3770  total:225  pass:184  dwarn:1   dfail:0   fail:0   skip:40 
ro-skl3-i5-6260u total:225  pass:201  dwarn:1   dfail:0   fail:0   skip:23 
ro-snb-i7-2620M  total:225  pass:173  dwarn:1   dfail:0   fail:1   skip:50 
fi-hsw-i7-4770k failed to connect after reboot
fi-snb-i7-2600 failed to connect after reboot
ro-bdw-i5-5250u failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1251/

79c69c3 drm-intel-nightly: 2016y-06m-21d-08h-53m-58s UTC integration manifest
00fc9d2 drm: Drop mode_config.mutex from _reset()
5a86f0a drm: Drop mode_config.mutex from get_resources ioctl
c390398 drm: Drop mode_config.mutex from connector_register_all
0c61ea6 drm: Revamp connector_list protection
a85943f drm: Drop cargo-culted modeset_lock_all from encoder/plane init/cleanup
c71630b drm/i915: Roll out drm_for_each_connector in intel_hotplug.c
8ae4924 drm/i915: Use use the drm_for_each_connector in i915_debugfs.c
39fab56 drm: Add explicit iter struct to drm_for_each_connector
9449105 drm: Don't compute obj counts expensively in get_resources
ad7b8e5 drm/amd-kfd: Clean up inline handling

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/10] drm: Don't compute obj counts expensively in get_resources

[Daniel Vetter]

On Tue, Jun 21, 2016 at 10:48:08AM +0100, Chris Wilson wrote:
> On Tue, Jun 21, 2016 at 11:10:27AM +0200, Daniel Vetter wrote:
> > And to avoid surprising userspace, make sure we don't copy more
> > connectors than planned, and report the actual number of connectors
> > copied. That way any racing hot-add/remove will be handled.
> 
> > @@ -1938,6 +1929,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
> >  			}
> >  			copied++;
> >  		}
> > +		connector_count = copied;
> 
> You forgot to actually make sure we don't copy more conectors than
> planned.

Indeed, must have lost that hunk somewhere. At least I'm sure I've typed
it ;-) Will resend.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm: Don't compute obj counts expensively in get_resources

[Daniel Vetter]

Looping when we keep track of this is silly. Only thing we have to
be careful is with sampling the connector count. To avoid inconsisten
results due to gcc re-computing this, use READ_ONCE.

And to avoid surprising userspace, make sure we don't copy more
connectors than planned, and report the actual number of connectors
copied. That way any racing hot-add/remove will be handled.

v2: Actually try to not blow up, somehow I lost the hunk that checks
we don't copy too much. Noticed by Chris.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 28c109ff7330..59c5261a309c 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1842,10 +1842,10 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	struct drm_crtc *crtc;
 	struct drm_encoder *encoder;
 	int ret = 0;
-	int connector_count = 0;
-	int crtc_count = 0;
+	int connector_count = READ_ONCE(dev->mode_config.num_connector);
+	int crtc_count = dev->mode_config.num_crtc;
 	int fb_count = 0;
-	int encoder_count = 0;
+	int encoder_count = dev->mode_config.num_encoder;
 	int copied = 0;
 	uint32_t __user *fb_id;
 	uint32_t __user *crtc_id;
@@ -1883,15 +1883,6 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	/* mode_config.mutex protects the connector list against e.g. DP MST
 	 * connector hot-adding. CRTC/Plane lists are invariant. */
 	mutex_lock(&dev->mode_config.mutex);
-	drm_for_each_crtc(crtc, dev)
-		crtc_count++;
-
-	drm_for_each_connector(connector, dev)
-		connector_count++;
-
-	drm_for_each_encoder(encoder, dev)
-		encoder_count++;
-
 	card_res->max_height = dev->mode_config.max_height;
 	card_res->min_height = dev->mode_config.min_height;
 	card_res->max_width = dev->mode_config.max_width;
@@ -1931,6 +1922,9 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 		copied = 0;
 		connector_id = (uint32_t __user *)(unsigned long)card_res->connector_id_ptr;
 		drm_for_each_connector(connector, dev) {
+			if (copied >= connector_count)
+				break;
+
 			if (put_user(connector->base.id,
 				     connector_id + copied)) {
 				ret = -EFAULT;
@@ -1938,6 +1932,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 			}
 			copied++;
 		}
+		connector_count = copied;
 	}
 	card_res->count_connectors = connector_count;
 
-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Don't compute obj counts expensively in get_resources

[Chris Wilson]

On Tue, Jun 21, 2016 at 02:29:48PM +0200, Daniel Vetter wrote:
> Looping when we keep track of this is silly. Only thing we have to
> be careful is with sampling the connector count. To avoid inconsisten
> results due to gcc re-computing this, use READ_ONCE.
> 
> And to avoid surprising userspace, make sure we don't copy more
> connectors than planned, and report the actual number of connectors
> copied. That way any racing hot-add/remove will be handled.
> 
> v2: Actually try to not blow up, somehow I lost the hunk that checks
> we don't copy too much. Noticed by Chris.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

The defacto API is call once with count_connectors=0 then with again
with the previous value of count_connectors. On the second pass, the
user is expecting no more then count_connectors to be copied, and is not
expecting to be told about new ones. The expected ABI looks unchanged.

> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 28c109ff7330..59c5261a309c 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1842,10 +1842,10 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
>  	struct drm_crtc *crtc;
>  	struct drm_encoder *encoder;
>  	int ret = 0;
> -	int connector_count = 0;
> -	int crtc_count = 0;
> +	int connector_count = READ_ONCE(dev->mode_config.num_connector);

Ok, since in the future num_connector is not guarded by mode_conf.mutex,
moving the read underneath that lock doesn't obviate the need for
READ_ONCE. Still it reduces the window and how far one has too look back
if it were set just before the connector loop.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm: Drop mode_config.mutex from get_resources ioctl

[Daniel Vetter]

The only thing this protected is the connector_list, which is
now protected differently.

v2: Also remove comment (Chris).

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 13c3bacc6b51..f8931b82865a 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1904,9 +1904,6 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	card_res->count_fbs = fb_count;
 	mutex_unlock(&file_priv->fbs_lock);
 
-	/* mode_config.mutex protects the connector list against e.g. DP MST
-	 * connector hot-adding. CRTC/Plane lists are invariant. */
-	mutex_lock(&dev->mode_config.mutex);
 	card_res->max_height = dev->mode_config.max_height;
 	card_res->min_height = dev->mode_config.min_height;
 	card_res->max_width = dev->mode_config.max_width;
@@ -1961,7 +1958,6 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	card_res->count_connectors = connector_count;
 
 out:
-	mutex_unlock(&dev->mode_config.mutex);
 	return ret;
 }
 
-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/10] drm/amd-kfd: Clean up inline handling

[Oded Gabbay]

On Tue, Jun 21, 2016 at 12:10 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> - inline functions need to be static inline, otherwise gcc can opt to
>   not inline and the linker gets unhappy.
> - no forward decls for inline functions, just include the right headers.
>
> Cc: Oded Gabbay <oded.gabbay@gmail.com>
> Cc: Ben Goz <ben.goz@amd.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h | 4 ++--
>  drivers/gpu/drm/amd/amdkfd/kfd_priv.h                 | 3 ---
>  2 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
> index ec4036a09f3e..a625b9137da2 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
> @@ -187,12 +187,12 @@ int init_pipelines(struct device_queue_manager *dqm,
>  unsigned int get_first_pipe(struct device_queue_manager *dqm);
>  unsigned int get_pipes_num(struct device_queue_manager *dqm);
>
> -extern inline unsigned int get_sh_mem_bases_32(struct kfd_process_device *pdd)
> +static inline unsigned int get_sh_mem_bases_32(struct kfd_process_device *pdd)
>  {
>         return (pdd->lds_base >> 16) & 0xFF;
>  }
>
> -extern inline unsigned int
> +static inline unsigned int
>  get_sh_mem_bases_nybble_64(struct kfd_process_device *pdd)
>  {
>         return (pdd->lds_base >> 60) & 0x0E;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index d0d5f4baf72d..80113c335966 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -617,10 +617,7 @@ int kgd2kfd_resume(struct kfd_dev *kfd);
>  int kfd_init_apertures(struct kfd_process *process);
>
>  /* Queue Context Management */
> -inline uint32_t lower_32(uint64_t x);
> -inline uint32_t upper_32(uint64_t x);
>  struct cik_sdma_rlc_registers *get_sdma_mqd(void *mqd);
> -inline uint32_t get_sdma_base_addr(struct cik_sdma_rlc_registers *m);
>
>  int init_queue(struct queue **q, struct queue_properties properties);
>  void uninit_queue(struct queue *q);
> --
> 2.8.1
>

Hi Daniel,
Minor comment, please change the commit message title to "drm/amdkfd:
..." (without the "-" between amd and kfd), to make this patch
consistent with all amdkfd patches.

With that change, this patch is:
Reviewed-by: Oded Gabbay <oded.gabbay@gmail.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/10] drm/amd-kfd: Clean up inline handling

[Daniel Vetter]

On Tue, Jun 21, 2016 at 10:11:13PM +0300, Oded Gabbay wrote:
> On Tue, Jun 21, 2016 at 12:10 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > - inline functions need to be static inline, otherwise gcc can opt to
> >   not inline and the linker gets unhappy.
> > - no forward decls for inline functions, just include the right headers.
> >
> > Cc: Oded Gabbay <oded.gabbay@gmail.com>
> > Cc: Ben Goz <ben.goz@amd.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h | 4 ++--
> >  drivers/gpu/drm/amd/amdkfd/kfd_priv.h                 | 3 ---
> >  2 files changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
> > index ec4036a09f3e..a625b9137da2 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
> > @@ -187,12 +187,12 @@ int init_pipelines(struct device_queue_manager *dqm,
> >  unsigned int get_first_pipe(struct device_queue_manager *dqm);
> >  unsigned int get_pipes_num(struct device_queue_manager *dqm);
> >
> > -extern inline unsigned int get_sh_mem_bases_32(struct kfd_process_device *pdd)
> > +static inline unsigned int get_sh_mem_bases_32(struct kfd_process_device *pdd)
> >  {
> >         return (pdd->lds_base >> 16) & 0xFF;
> >  }
> >
> > -extern inline unsigned int
> > +static inline unsigned int
> >  get_sh_mem_bases_nybble_64(struct kfd_process_device *pdd)
> >  {
> >         return (pdd->lds_base >> 60) & 0x0E;
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> > index d0d5f4baf72d..80113c335966 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> > @@ -617,10 +617,7 @@ int kgd2kfd_resume(struct kfd_dev *kfd);
> >  int kfd_init_apertures(struct kfd_process *process);
> >
> >  /* Queue Context Management */
> > -inline uint32_t lower_32(uint64_t x);
> > -inline uint32_t upper_32(uint64_t x);
> >  struct cik_sdma_rlc_registers *get_sdma_mqd(void *mqd);
> > -inline uint32_t get_sdma_base_addr(struct cik_sdma_rlc_registers *m);
> >
> >  int init_queue(struct queue **q, struct queue_properties properties);
> >  void uninit_queue(struct queue *q);
> > --
> > 2.8.1
> >
> 
> Hi Daniel,
> Minor comment, please change the commit message title to "drm/amdkfd:
> ..." (without the "-" between amd and kfd), to make this patch
> consistent with all amdkfd patches.
> 
> With that change, this patch is:
> Reviewed-by: Oded Gabbay <oded.gabbay@gmail.com>

Fixed up an applied to drm-misc, thanks for the review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/10] drm/amd-kfd: Clean up inline handling

[Daniel Vetter]

On Tue, Jun 21, 2016 at 10:11:13PM +0300, Oded Gabbay wrote:
> On Tue, Jun 21, 2016 at 12:10 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > - inline functions need to be static inline, otherwise gcc can opt to
> >   not inline and the linker gets unhappy.
> > - no forward decls for inline functions, just include the right headers.
> >
> > Cc: Oded Gabbay <oded.gabbay@gmail.com>
> > Cc: Ben Goz <ben.goz@amd.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h | 4 ++--
> >  drivers/gpu/drm/amd/amdkfd/kfd_priv.h                 | 3 ---
> >  2 files changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
> > index ec4036a09f3e..a625b9137da2 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
> > @@ -187,12 +187,12 @@ int init_pipelines(struct device_queue_manager *dqm,
> >  unsigned int get_first_pipe(struct device_queue_manager *dqm);
> >  unsigned int get_pipes_num(struct device_queue_manager *dqm);
> >
> > -extern inline unsigned int get_sh_mem_bases_32(struct kfd_process_device *pdd)
> > +static inline unsigned int get_sh_mem_bases_32(struct kfd_process_device *pdd)
> >  {
> >         return (pdd->lds_base >> 16) & 0xFF;
> >  }
> >
> > -extern inline unsigned int
> > +static inline unsigned int
> >  get_sh_mem_bases_nybble_64(struct kfd_process_device *pdd)
> >  {
> >         return (pdd->lds_base >> 60) & 0x0E;
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> > index d0d5f4baf72d..80113c335966 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> > @@ -617,10 +617,7 @@ int kgd2kfd_resume(struct kfd_dev *kfd);
> >  int kfd_init_apertures(struct kfd_process *process);
> >
> >  /* Queue Context Management */
> > -inline uint32_t lower_32(uint64_t x);
> > -inline uint32_t upper_32(uint64_t x);
> >  struct cik_sdma_rlc_registers *get_sdma_mqd(void *mqd);
> > -inline uint32_t get_sdma_base_addr(struct cik_sdma_rlc_registers *m);
> >
> >  int init_queue(struct queue **q, struct queue_properties properties);
> >  void uninit_queue(struct queue *q);
> > --
> > 2.8.1
> >
> 
> Hi Daniel,
> Minor comment, please change the commit message title to "drm/amdkfd:
> ..." (without the "-" between amd and kfd), to make this patch
> consistent with all amdkfd patches.
> 
> With that change, this patch is:
> Reviewed-by: Oded Gabbay <oded.gabbay@gmail.com>

Fixed up and applied, thanks for the review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 07/10] drm: Revamp connector_list protection

[Daniel Vetter]

On Tue, Jun 21, 2016 at 11:10:32AM +0200, Daniel Vetter wrote:
> This is a pretty good horror show, but I think it's the best tradeoff:
> - Thanks to srcu and delayed freeing the locking doesn't leak out to
>   callers, hence no added headaches with locking inversions.
> - For core and drivers which hot-remove connectors all the connector
>   list walking details are hidden in a macro.
> - Other drivers which don't care about hot-removing can simply keep on
>   using the normal list walking macros.
> 
> The new design is:
> - New dev->mode_config.connector_list_lock to protect the
>   connector_list, and the connector_ida (since that's also
>   unprotected), plus num_connectors. This is a pure leaf lock, nothing
>   is allowed to nest within, nothing outside of connector init/cleanup
>   will ever need it.
> - Protect connector_list itself with srcu. This allows sleeping and
>   anything else. The only thing which is not allowed is calling
>   synchronize_srcu, or grabbing any locks or waiting on
>   waitqueues/completions/whatever which might call that. To make this
>   100% safe we opt to not have any calls to synchronize_srcu.
> - Protect against use-after-free of connectors using call_srcu, to
>   delay the kfree enough.
> - To protect against zombie connectors which are in progress of final
>   destruction use kref_get_unless_zero. This is safe since srcu
>   prevents against use-after-free, and that's the only guarantee we
>   need.
> 
> For this demo I only bothered converting i915, and there also not
> everything - I left the connector loops in the modeset code unchanged
> since those will all be converted over to
> drm_for_each_connector_in_state to make them work correctly for
> nonblocking atomic commits. Only loops outside of atomic commits
> should be converted to drm_for_each_connector.
> 
> Note that the i915 DP MST code still uses drm_modeset_lock_all(). But
> that's not because of drm core connector lifetime issues, but because
> the fbdev helper reuses core locks to mange its own lists and data
> structures. Thierry Reding has a patch series to gift fbdev its own
> lock, which will remedy this.
> 
> v2: Totally rewrite everything to pack it up in a neat
> iterator macro, with init/check/next extracted into helpers.
> 
> v3: Tiny rebase conflict.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Ok, CI pointed out a flaw in this: break (or worse goto) means that the
unreference and the read_unlock_srcu isn't done at the end, which means
leaks and lockdep complaints. break can be fixed, but goto is impossible
to fix with plain C ...

Not sure what do to now, since I'd really like to avoid leaking the
connector_list locking all over the place. It means lots of churn, and
also issues (if we go without srcu) with locking inversions.
-Daniel

> ---
>  drivers/gpu/drm/drm_crtc.c          | 57 +++++++++++++++++++++-----
>  drivers/gpu/drm/i915/intel_dp_mst.c |  2 +-
>  include/drm/drm_crtc.h              | 82 +++++++++++++++++++++++++++----------
>  3 files changed, 109 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 6a8f91e8821b..dc22c0bfbe99 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -44,6 +44,9 @@
>  #include "drm_crtc_internal.h"
>  #include "drm_internal.h"
>  
> +DEFINE_SRCU(drm_connector_list_srcu);
> +EXPORT_SYMBOL(drm_connector_list_srcu);
> +
>  static struct drm_framebuffer *
>  internal_framebuffer_create(struct drm_device *dev,
>  			    const struct drm_mode_fb_cmd2 *r,
> @@ -825,7 +828,7 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector)
>  		      mode->interlace ?  " interlaced" : "");
>  }
>  
> -static void drm_connector_free(struct kref *kref)
> +static void drm_connector_kref_release(struct kref *kref)
>  {
>  	struct drm_connector *connector =
>  		container_of(kref, struct drm_connector, base.refcount);
> @@ -858,11 +861,10 @@ int drm_connector_init(struct drm_device *dev,
>  	struct ida *connector_ida =
>  		&drm_connector_enum_list[connector_type].ida;
>  
> -	drm_modeset_lock_all(dev);
> -
> +	mutex_lock(&dev->mode_config.connector_list_lock);
>  	ret = drm_mode_object_get_reg(dev, &connector->base,
>  				      DRM_MODE_OBJECT_CONNECTOR,
> -				      false, drm_connector_free);
> +				      false, drm_connector_kref_release);
>  	if (ret)
>  		goto out_unlock;
>  
> @@ -899,11 +901,6 @@ int drm_connector_init(struct drm_device *dev,
>  
>  	drm_connector_get_cmdline_mode(connector);
>  
> -	/* We should add connectors at the end to avoid upsetting the connector
> -	 * index too much. */
> -	list_add_tail(&connector->head, &config->connector_list);
> -	config->num_connector++;
> -
>  	if (connector_type != DRM_MODE_CONNECTOR_VIRTUAL)
>  		drm_object_attach_property(&connector->base,
>  					      config->edid_property,
> @@ -917,6 +914,11 @@ int drm_connector_init(struct drm_device *dev,
>  	}
>  
>  	connector->debugfs_entry = NULL;
> +
> +	/* We must add the connector to the list last. */
> +	list_add_tail_rcu(&connector->head, &config->connector_list);
> +	config->num_connector++;
> +
>  out_put_type_id:
>  	if (ret)
>  		ida_remove(connector_ida, connector->connector_type_id);
> @@ -928,7 +930,7 @@ out_put:
>  		drm_mode_object_unregister(dev, &connector->base);
>  
>  out_unlock:
> -	drm_modeset_unlock_all(dev);
> +	mutex_unlock(&dev->mode_config.connector_list_lock);
>  
>  	return ret;
>  }
> @@ -951,6 +953,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
>  	if (WARN_ON(connector->registered))
>  		drm_connector_unregister(connector);
>  
> +	mutex_lock(&dev->mode_config.connector_list_lock);
>  	if (connector->tile_group) {
>  		drm_mode_put_tile_group(dev, connector->tile_group);
>  		connector->tile_group = NULL;
> @@ -981,9 +984,42 @@ void drm_connector_cleanup(struct drm_connector *connector)
>  						       connector->state);
>  
>  	memset(connector, 0, sizeof(*connector));
> +	mutex_unlock(&dev->mode_config.connector_list_lock);
>  }
>  EXPORT_SYMBOL(drm_connector_cleanup);
>  
> +static void drm_connector_free_cb(struct rcu_head *head)
> +{
> +	struct drm_connector *connector = container_of(head,
> +						       struct drm_connector,
> +						       free_head);
> +
> +	kfree(connector);
> +}
> +
> +/**
> + * drm_connector_free - frees a connector structure
> + * @connector: connector to free
> + *
> + * This frees @connector using call_srcu() and kfree(). If the driver subclasses
> + * the connector, then the embedded struct &drm_connector must be the first
> + * element, and @connector must have been allocated using kmalloc() or one of
> + * the functions wrapping that.
> + *
> + * Delayed freeing is required to avoid use-after-free when walking the
> + * connector list, which is srcu protected. Hence drivers must use this function
> + * for connectors which can be removed while the driver is loaded. Currently
> + * that's only true for DP MST connectors. For any other connectors it is valid
> + * to call kfree directly.
> + */
> +void drm_connector_free(struct drm_connector *connector)
> +{
> +	call_srcu(&drm_connector_list_srcu,
> +		  &connector->free_head,
> +		  drm_connector_free_cb);
> +}
> +EXPORT_SYMBOL(drm_connector_free);
> +
>  /**
>   * drm_connector_register - register a connector
>   * @connector: the connector to register
> @@ -5554,6 +5590,7 @@ void drm_mode_config_init(struct drm_device *dev)
>  	mutex_init(&dev->mode_config.idr_mutex);
>  	mutex_init(&dev->mode_config.fb_lock);
>  	mutex_init(&dev->mode_config.blob_lock);
> +	mutex_init(&dev->mode_config.connector_list_lock);
>  	INIT_LIST_HEAD(&dev->mode_config.fb_list);
>  	INIT_LIST_HEAD(&dev->mode_config.crtc_list);
>  	INIT_LIST_HEAD(&dev->mode_config.connector_list);
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 9646816604be..11bbbb194f2d 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -327,7 +327,7 @@ intel_dp_mst_connector_destroy(struct drm_connector *connector)
>  		kfree(intel_connector->edid);
>  
>  	drm_connector_cleanup(connector);
> -	kfree(connector);
> +	drm_connector_free(connector);
>  }
>  
>  static const struct drm_connector_funcs intel_dp_mst_connector_funcs = {
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index ba574b9c1ec4..8f4380ac113e 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -32,6 +32,7 @@
>  #include <linux/fb.h>
>  #include <linux/hdmi.h>
>  #include <linux/media-bus-format.h>
> +#include <linux/srcu.h>
>  #include <uapi/drm/drm_mode.h>
>  #include <uapi/drm/drm_fourcc.h>
>  #include <drm/drm_modeset_lock.h>
> @@ -1186,6 +1187,7 @@ struct drm_encoder {
>   * @kdev: kernel device for sysfs attributes
>   * @attr: sysfs attributes
>   * @head: list management
> + * @free_head: rcu delayed freeing management
>   * @base: base KMS object
>   * @name: human readable name, can be overwritten by the driver
>   * @connector_id: compacted connector id useful indexing arrays
> @@ -1241,6 +1243,7 @@ struct drm_connector {
>  	struct device *kdev;
>  	struct device_attribute *attr;
>  	struct list_head head;
> +	struct rcu_head free_head;
>  
>  	struct drm_mode_object base;
>  
> @@ -2309,7 +2312,7 @@ struct drm_mode_config {
>  	 * @tile_idr:
>  	 *
>  	 * Use this idr for allocating new IDs for tiled sinks like use in some
> -	 * high-res DP MST screens.
> +	 * high-res DP MST screens. Protected by @idr_mutex.
>  	 */
>  	struct idr tile_idr;
>  
> @@ -2320,6 +2323,14 @@ struct drm_mode_config {
>  	int num_connector;
>  	struct ida connector_ida;
>  	struct list_head connector_list;
> +
> +	/**
> +	 * @connector_list_lock:
> +	 *
> +	 * Protects @connector_list, @connector_ida and * @num_conncetors.
> +	 */
> +	struct mutex connector_list_lock;
> +
>  	int num_encoder;
>  	struct list_head encoder_list;
>  
> @@ -2426,6 +2437,8 @@ struct drm_mode_config {
>  	struct drm_mode_config_helper_funcs *helper_private;
>  };
>  
> +extern struct srcu_struct drm_connector_list_srcu;
> +
>  /**
>   * drm_for_each_plane_mask - iterate over planes specified by bitmask
>   * @plane: the loop cursor
> @@ -2505,6 +2518,7 @@ int drm_connector_register(struct drm_connector *connector);
>  void drm_connector_unregister(struct drm_connector *connector);
>  
>  extern void drm_connector_cleanup(struct drm_connector *connector);
> +extern void drm_connector_free(struct drm_connector *connector);
>  static inline unsigned drm_connector_index(struct drm_connector *connector)
>  {
>  	return connector->connector_id;
> @@ -2835,31 +2849,57 @@ static inline void drm_connector_unreference(struct drm_connector *connector)
>  #define drm_for_each_crtc(crtc, dev) \
>  	list_for_each_entry(crtc, &(dev)->mode_config.crtc_list, head)
>  
> +struct drm_connector_iter {
> +	struct drm_mode_config *mode_config;
> +	struct drm_connector *connector;
> +	int srcu_ret;
> +};
> +
>  static inline void
> -assert_drm_connector_list_read_locked(struct drm_mode_config *mode_config)
> +__drm_connector_iter_init(struct drm_mode_config *mode_config,
> +			  struct drm_connector_iter *iter)
>  {
> -	/*
> -	 * The connector hotadd/remove code currently grabs both locks when
> -	 * updating lists. Hence readers need only hold either of them to be
> -	 * safe and the check amounts to
> -	 *
> -	 * WARN_ON(not_holding(A) && not_holding(B)).
> -	 */
> -	WARN_ON(!mutex_is_locked(&mode_config->mutex) &&
> -		!drm_modeset_is_locked(&mode_config->connection_mutex));
> +	iter->mode_config = mode_config;
> +	iter->srcu_ret = srcu_read_lock(&drm_connector_list_srcu);
> +	iter->connector = list_entry_rcu(mode_config->connector_list.next,
> +					 struct drm_connector, head);
>  }
>  
> -struct drm_connector_iter {
> -	struct drm_mode_config *mode_config;
> -};
> +static inline struct drm_connector *
> +__drm_connector_iter_check(struct drm_connector_iter *iter)
> +{
> +retry:
> +	if (&iter->connector->head == &iter->mode_config->connector_list) {
> +		/* last iteration, clean up */
> +		srcu_read_unlock(&drm_connector_list_srcu, iter->srcu_ret);
> +		return NULL;
> +	}
> +
> +	/* srcu protects against iter->connector disappearing */
> +	if (!kref_get_unless_zero(&iter->connector->base.refcount)) {
> +		iter->connector = list_entry_rcu(iter->connector->head.next,
> +						 struct drm_connector, head);
> +		goto retry;
> +	}
> +
> +	return iter->connector;
> +}
> +
> +static inline void
> +__drm_connector_iter_next(struct drm_connector_iter *iter)
> +{
> +	/* _next() is only called when _check() succeeded on iter->connector,
> +	 * and _check() only succeeds if kref_get_unless_zero() succeeded.
> +	 * Therefore dropping the reference here is the correct place. */
> +	drm_connector_unreference(iter->connector);
> +	iter->connector = list_entry_rcu(iter->connector->head.next,
> +					 struct drm_connector, head);
> +}
>  
> -#define drm_for_each_connector(connector, dev, iter) \
> -	for ((iter).mode_config = NULL,						\
> -	     assert_drm_connector_list_read_locked(&(dev)->mode_config),	\
> -	     connector = list_first_entry(&(dev)->mode_config.connector_list,	\
> -					  struct drm_connector, head);		\
> -	     &connector->head != (&(dev)->mode_config.connector_list);		\
> -	     connector = list_next_entry(connector, head))
> +#define drm_for_each_connector(connector, dev, iter) 			\
> +	for (__drm_connector_iter_init(&(dev)->mode_config, &(iter));	\
> +	     (connector = __drm_connector_iter_check(&(iter)));		\
> +	     __drm_connector_iter_next(&(iter)))
>  
>  #define drm_for_each_encoder(encoder, dev) \
>  	list_for_each_entry(encoder, &(dev)->mode_config.encoder_list, head)
> -- 
> 2.8.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx