* [PATCH] drm: rework delayed connector cleanup in connector_iter
@ 2017-12-13 10:45 ` Daniel Vetter
0 siblings, 0 replies; 17+ messages in thread
From: Daniel Vetter @ 2017-12-13 10:45 UTC (permalink / raw)
To: Intel Graphics Development
Cc: DRI Development, Daniel Vetter, Ben Widawsky, Dave Airlie,
Chris Wilson, Sean Paul, stable, Daniel Vetter, Jani Nikula,
Gustavo Padovan, David Airlie, Javier Martinez Canillas,
Shuah Khan, Guillaume Tucker, Mark Brown, Kevin Hilman,
Matt Hart, Thierry Escande, Tomeu Vizoso, Enric Balletbo i Serra
PROBE_DEFER also uses system_wq to reprobe drivers, which means when
that again fails, and we try to flush the overall system_wq (to get
all the delayed connectore cleanup work_struct completed), we
deadlock.
Fix this by using just a single cleanup work, so that we can only
flush that one and don't block on anything else. That means a free
list plus locking, a standard pattern.
Fixes: a703c55004e1 ("drm: safely free connectors from connector_iter")
Fixes: 613051dac40d ("drm: locking&new iterators for connector_list")
Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Dave Airlie <airlied@gmail.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: <stable@vger.kernel.org> # v4.11+: 613051dac40d ("drm: locking&new iterators for connector_list"
Cc: <stable@vger.kernel.org> # v4.11+
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: David Airlie <airlied@linux.ie>
Cc: Javier Martinez Canillas <javier@dowhile0.org>
Cc: Shuah Khan <shuahkh@osg.samsung.com>
Cc: Guillaume Tucker <guillaume.tucker@collabora.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Kevin Hilman <khilman@baylibre.com>
Cc: Matt Hart <matthew.hart@linaro.org>
Cc: Thierry Escande <thierry.escande@collabora.co.uk>
Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
drivers/gpu/drm/drm_connector.c | 46 ++++++++++++++++++++++++++-----------
drivers/gpu/drm/drm_crtc_internal.h | 1 +
drivers/gpu/drm/drm_mode_config.c | 5 +++-
include/drm/drm_connector.h | 4 ++--
include/drm/drm_mode_config.h | 17 +++++++++++++-
5 files changed, 55 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 0b7e0974e6da..2f43dc217936 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -153,14 +153,23 @@ static void drm_connector_free(struct kref *kref)
connector->funcs->destroy(connector);
}
-static void drm_connector_free_work_fn(struct work_struct *work)
+void drm_connector_free_work_fn(struct work_struct *work)
{
- struct drm_connector *connector =
- container_of(work, struct drm_connector, free_work);
- struct drm_device *dev = connector->dev;
+ struct drm_connector *connector;
+ struct drm_device *dev =
+ container_of(work, struct drm_device, mode_config.connector_free_work);
+ struct drm_mode_config *config = &dev->mode_config;
+ unsigned long flags;
+ LIST_HEAD(list_head);
- drm_mode_object_unregister(dev, &connector->base);
- connector->funcs->destroy(connector);
+ spin_lock_irqsave(&config->connector_list_lock, flags);
+ list_splice(&config->connector_free_list, &list_head);
+ spin_unlock_irqrestore(&config->connector_list_lock, flags);
+
+ list_for_each_entry(connector, &list_head, free_head) {
+ drm_mode_object_unregister(dev, &connector->base);
+ connector->funcs->destroy(connector);
+ }
}
/**
@@ -192,8 +201,6 @@ int drm_connector_init(struct drm_device *dev,
if (ret)
return ret;
- INIT_WORK(&connector->free_work, drm_connector_free_work_fn);
-
connector->base.properties = &connector->properties;
connector->dev = dev;
connector->funcs = funcs;
@@ -550,10 +557,15 @@ EXPORT_SYMBOL(drm_connector_list_iter_begin);
* actually release the connector when dropping our final reference.
*/
static void
-drm_connector_put_safe(struct drm_connector *conn)
+__drm_connector_put_safe(struct drm_connector *conn)
{
+ struct drm_mode_config *config = &conn->dev->mode_config;
+
+ lockdep_assert_held(&config->connector_list_lock);
+ list_add(&conn->free_head, &config->connector_free_list);
+
if (refcount_dec_and_test(&conn->base.refcount.refcount))
- schedule_work(&conn->free_work);
+ schedule_work(&config->connector_free_work);
}
/**
@@ -585,10 +597,10 @@ drm_connector_list_iter_next(struct drm_connector_list_iter *iter)
/* loop until it's not a zombie connector */
} while (!kref_get_unless_zero(&iter->conn->base.refcount));
- spin_unlock_irqrestore(&config->connector_list_lock, flags);
if (old_conn)
- drm_connector_put_safe(old_conn);
+ __drm_connector_put_safe(old_conn);
+ spin_unlock_irqrestore(&config->connector_list_lock, flags);
return iter->conn;
}
@@ -605,9 +617,15 @@ EXPORT_SYMBOL(drm_connector_list_iter_next);
*/
void drm_connector_list_iter_end(struct drm_connector_list_iter *iter)
{
+ struct drm_mode_config *config = &iter->dev->mode_config;
+ unsigned long flags;
+
iter->dev = NULL;
- if (iter->conn)
- drm_connector_put_safe(iter->conn);
+ if (iter->conn) {
+ spin_lock_irqsave(&config->connector_list_lock, flags);
+ __drm_connector_put_safe(iter->conn);
+ spin_unlock_irqrestore(&config->connector_list_lock, flags);
+ }
lock_release(&connector_list_iter_dep_map, 0, _RET_IP_);
}
EXPORT_SYMBOL(drm_connector_list_iter_end);
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 9ebb8841778c..af00f42ba269 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -142,6 +142,7 @@ int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
uint64_t value);
int drm_connector_create_standard_properties(struct drm_device *dev);
const char *drm_get_connector_force_name(enum drm_connector_force force);
+void drm_connector_free_work_fn(struct work_struct *work);
/* IOCTL */
int drm_mode_connector_property_set_ioctl(struct drm_device *dev,
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index 6ffe952142e6..025fc23cf7c6 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -373,6 +373,7 @@ void drm_mode_config_init(struct drm_device *dev)
INIT_LIST_HEAD(&dev->mode_config.fb_list);
INIT_LIST_HEAD(&dev->mode_config.crtc_list);
INIT_LIST_HEAD(&dev->mode_config.connector_list);
+ INIT_LIST_HEAD(&dev->mode_config.connector_free_list);
INIT_LIST_HEAD(&dev->mode_config.encoder_list);
INIT_LIST_HEAD(&dev->mode_config.property_list);
INIT_LIST_HEAD(&dev->mode_config.property_blob_list);
@@ -382,6 +383,8 @@ void drm_mode_config_init(struct drm_device *dev)
ida_init(&dev->mode_config.connector_ida);
spin_lock_init(&dev->mode_config.connector_list_lock);
+ INIT_WORK(&dev->mode_config.connector_free_work, drm_connector_free_work_fn);
+
drm_mode_create_standard_properties(dev);
/* Just to be sure */
@@ -432,7 +435,7 @@ void drm_mode_config_cleanup(struct drm_device *dev)
}
drm_connector_list_iter_end(&conn_iter);
/* connector_iter drops references in a work item. */
- flush_scheduled_work();
+ flush_work(&dev->mode_config.connector_free_work);
if (WARN_ON(!list_empty(&dev->mode_config.connector_list))) {
drm_connector_list_iter_begin(dev, &conn_iter);
drm_for_each_connector_iter(connector, &conn_iter)
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index ee3155391243..50bc21050fbf 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -966,12 +966,12 @@ struct drm_connector {
uint16_t tile_h_size, tile_v_size;
/**
- * @free_work:
+ * @free_head:
*
* Work used only by &drm_connector_iter to be able to clean up a
* connector from any context.
*/
- struct work_struct free_work;
+ struct list_head free_head;
};
#define obj_to_connector(x) container_of(x, struct drm_connector, base)
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index e5f3b43014e1..5e62e52a2c58 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -402,7 +402,7 @@ struct drm_mode_config {
/**
* @connector_list_lock: Protects @num_connector and
- * @connector_list.
+ * @connector_list and @connector_free_list.
*/
spinlock_t connector_list_lock;
/**
@@ -422,6 +422,21 @@ struct drm_mode_config {
* &struct drm_connector_list_iter to walk this list.
*/
struct list_head connector_list;
+ /**
+ * @connector_free_list:
+ *
+ * List of connector objects linked with &drm_connector.free_head.
+ * Protected by @connector_list_lock. Used by
+ * drm_for_each_connector_iter() and
+ * &struct drm_connector_list_iter to savely free connectors using
+ * @connector_free_work.
+ */
+ struct list_head connector_free_list;
+ /**
+ * @connector_free_work: Work to clean up @connector_free_list.
+ */
+ struct work_struct connector_free_work;
+
/**
* @num_encoder:
*
--
2.15.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH] drm: rework delayed connector cleanup in connector_iter
@ 2017-12-13 10:45 ` Daniel Vetter
0 siblings, 0 replies; 17+ messages in thread
From: Daniel Vetter @ 2017-12-13 10:45 UTC (permalink / raw)
To: Intel Graphics Development
Cc: DRI Development, Daniel Vetter, Ben Widawsky, Dave Airlie,
Chris Wilson, Sean Paul, stable, Daniel Vetter, Jani Nikula,
Gustavo Padovan, David Airlie, Javier Martinez Canillas,
Shuah Khan, Guillaume Tucker, Mark Brown, Kevin Hilman,
Matt Hart, Thierry Escande, Tomeu Vizoso
PROBE_DEFER also uses system_wq to reprobe drivers, which means when
that again fails, and we try to flush the overall system_wq (to get
all the delayed connectore cleanup work_struct completed), we
deadlock.
Fix this by using just a single cleanup work, so that we can only
flush that one and don't block on anything else. That means a free
list plus locking, a standard pattern.
Fixes: a703c55004e1 ("drm: safely free connectors from connector_iter")
Fixes: 613051dac40d ("drm: locking&new iterators for connector_list")
Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Dave Airlie <airlied@gmail.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: <stable@vger.kernel.org> # v4.11+: 613051dac40d ("drm: locking&new iterators for connector_list"
Cc: <stable@vger.kernel.org> # v4.11+
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: David Airlie <airlied@linux.ie>
Cc: Javier Martinez Canillas <javier@dowhile0.org>
Cc: Shuah Khan <shuahkh@osg.samsung.com>
Cc: Guillaume Tucker <guillaume.tucker@collabora.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Kevin Hilman <khilman@baylibre.com>
Cc: Matt Hart <matthew.hart@linaro.org>
Cc: Thierry Escande <thierry.escande@collabora.co.uk>
Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
drivers/gpu/drm/drm_connector.c | 46 ++++++++++++++++++++++++++-----------
drivers/gpu/drm/drm_crtc_internal.h | 1 +
drivers/gpu/drm/drm_mode_config.c | 5 +++-
include/drm/drm_connector.h | 4 ++--
include/drm/drm_mode_config.h | 17 +++++++++++++-
5 files changed, 55 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 0b7e0974e6da..2f43dc217936 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -153,14 +153,23 @@ static void drm_connector_free(struct kref *kref)
connector->funcs->destroy(connector);
}
-static void drm_connector_free_work_fn(struct work_struct *work)
+void drm_connector_free_work_fn(struct work_struct *work)
{
- struct drm_connector *connector =
- container_of(work, struct drm_connector, free_work);
- struct drm_device *dev = connector->dev;
+ struct drm_connector *connector;
+ struct drm_device *dev =
+ container_of(work, struct drm_device, mode_config.connector_free_work);
+ struct drm_mode_config *config = &dev->mode_config;
+ unsigned long flags;
+ LIST_HEAD(list_head);
- drm_mode_object_unregister(dev, &connector->base);
- connector->funcs->destroy(connector);
+ spin_lock_irqsave(&config->connector_list_lock, flags);
+ list_splice(&config->connector_free_list, &list_head);
+ spin_unlock_irqrestore(&config->connector_list_lock, flags);
+
+ list_for_each_entry(connector, &list_head, free_head) {
+ drm_mode_object_unregister(dev, &connector->base);
+ connector->funcs->destroy(connector);
+ }
}
/**
@@ -192,8 +201,6 @@ int drm_connector_init(struct drm_device *dev,
if (ret)
return ret;
- INIT_WORK(&connector->free_work, drm_connector_free_work_fn);
-
connector->base.properties = &connector->properties;
connector->dev = dev;
connector->funcs = funcs;
@@ -550,10 +557,15 @@ EXPORT_SYMBOL(drm_connector_list_iter_begin);
* actually release the connector when dropping our final reference.
*/
static void
-drm_connector_put_safe(struct drm_connector *conn)
+__drm_connector_put_safe(struct drm_connector *conn)
{
+ struct drm_mode_config *config = &conn->dev->mode_config;
+
+ lockdep_assert_held(&config->connector_list_lock);
+ list_add(&conn->free_head, &config->connector_free_list);
+
if (refcount_dec_and_test(&conn->base.refcount.refcount))
- schedule_work(&conn->free_work);
+ schedule_work(&config->connector_free_work);
}
/**
@@ -585,10 +597,10 @@ drm_connector_list_iter_next(struct drm_connector_list_iter *iter)
/* loop until it's not a zombie connector */
} while (!kref_get_unless_zero(&iter->conn->base.refcount));
- spin_unlock_irqrestore(&config->connector_list_lock, flags);
if (old_conn)
- drm_connector_put_safe(old_conn);
+ __drm_connector_put_safe(old_conn);
+ spin_unlock_irqrestore(&config->connector_list_lock, flags);
return iter->conn;
}
@@ -605,9 +617,15 @@ EXPORT_SYMBOL(drm_connector_list_iter_next);
*/
void drm_connector_list_iter_end(struct drm_connector_list_iter *iter)
{
+ struct drm_mode_config *config = &iter->dev->mode_config;
+ unsigned long flags;
+
iter->dev = NULL;
- if (iter->conn)
- drm_connector_put_safe(iter->conn);
+ if (iter->conn) {
+ spin_lock_irqsave(&config->connector_list_lock, flags);
+ __drm_connector_put_safe(iter->conn);
+ spin_unlock_irqrestore(&config->connector_list_lock, flags);
+ }
lock_release(&connector_list_iter_dep_map, 0, _RET_IP_);
}
EXPORT_SYMBOL(drm_connector_list_iter_end);
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 9ebb8841778c..af00f42ba269 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -142,6 +142,7 @@ int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
uint64_t value);
int drm_connector_create_standard_properties(struct drm_device *dev);
const char *drm_get_connector_force_name(enum drm_connector_force force);
+void drm_connector_free_work_fn(struct work_struct *work);
/* IOCTL */
int drm_mode_connector_property_set_ioctl(struct drm_device *dev,
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index 6ffe952142e6..025fc23cf7c6 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -373,6 +373,7 @@ void drm_mode_config_init(struct drm_device *dev)
INIT_LIST_HEAD(&dev->mode_config.fb_list);
INIT_LIST_HEAD(&dev->mode_config.crtc_list);
INIT_LIST_HEAD(&dev->mode_config.connector_list);
+ INIT_LIST_HEAD(&dev->mode_config.connector_free_list);
INIT_LIST_HEAD(&dev->mode_config.encoder_list);
INIT_LIST_HEAD(&dev->mode_config.property_list);
INIT_LIST_HEAD(&dev->mode_config.property_blob_list);
@@ -382,6 +383,8 @@ void drm_mode_config_init(struct drm_device *dev)
ida_init(&dev->mode_config.connector_ida);
spin_lock_init(&dev->mode_config.connector_list_lock);
+ INIT_WORK(&dev->mode_config.connector_free_work, drm_connector_free_work_fn);
+
drm_mode_create_standard_properties(dev);
/* Just to be sure */
@@ -432,7 +435,7 @@ void drm_mode_config_cleanup(struct drm_device *dev)
}
drm_connector_list_iter_end(&conn_iter);
/* connector_iter drops references in a work item. */
- flush_scheduled_work();
+ flush_work(&dev->mode_config.connector_free_work);
if (WARN_ON(!list_empty(&dev->mode_config.connector_list))) {
drm_connector_list_iter_begin(dev, &conn_iter);
drm_for_each_connector_iter(connector, &conn_iter)
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index ee3155391243..50bc21050fbf 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -966,12 +966,12 @@ struct drm_connector {
uint16_t tile_h_size, tile_v_size;
/**
- * @free_work:
+ * @free_head:
*
* Work used only by &drm_connector_iter to be able to clean up a
* connector from any context.
*/
- struct work_struct free_work;
+ struct list_head free_head;
};
#define obj_to_connector(x) container_of(x, struct drm_connector, base)
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index e5f3b43014e1..5e62e52a2c58 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -402,7 +402,7 @@ struct drm_mode_config {
/**
* @connector_list_lock: Protects @num_connector and
- * @connector_list.
+ * @connector_list and @connector_free_list.
*/
spinlock_t connector_list_lock;
/**
@@ -422,6 +422,21 @@ struct drm_mode_config {
* &struct drm_connector_list_iter to walk this list.
*/
struct list_head connector_list;
+ /**
+ * @connector_free_list:
+ *
+ * List of connector objects linked with &drm_connector.free_head.
+ * Protected by @connector_list_lock. Used by
+ * drm_for_each_connector_iter() and
+ * &struct drm_connector_list_iter to savely free connectors using
+ * @connector_free_work.
+ */
+ struct list_head connector_free_list;
+ /**
+ * @connector_free_work: Work to clean up @connector_free_list.
+ */
+ struct work_struct connector_free_work;
+
/**
* @num_encoder:
*
--
2.15.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] drm: rework delayed connector cleanup in connector_iter
2017-12-13 10:45 ` Daniel Vetter
@ 2017-12-13 11:04 ` Chris Wilson
-1 siblings, 0 replies; 17+ messages in thread
From: Chris Wilson @ 2017-12-13 11:04 UTC (permalink / raw)
To: Daniel Vetter, Intel Graphics Development
Cc: DRI Development, Daniel Vetter, Ben Widawsky, Dave Airlie,
Sean Paul, stable, Daniel Vetter, Jani Nikula, Gustavo Padovan,
David Airlie, Javier Martinez Canillas, Shuah Khan,
Guillaume Tucker, Mark Brown, Kevin Hilman, Matt Hart,
Thierry Escande, Tomeu Vizoso, Enric Balletbo i Serra
Quoting Daniel Vetter (2017-12-13 10:45:53)
> PROBE_DEFER also uses system_wq to reprobe drivers, which means when
> that again fails, and we try to flush the overall system_wq (to get
> all the delayed connectore cleanup work_struct completed), we
> deadlock.
>
> Fix this by using just a single cleanup work, so that we can only
> flush that one and don't block on anything else. That means a free
> list plus locking, a standard pattern.
>
> Fixes: a703c55004e1 ("drm: safely free connectors from connector_iter")
> Fixes: 613051dac40d ("drm: locking&new iterators for connector_list")
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Dave Airlie <airlied@gmail.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: <stable@vger.kernel.org> # v4.11+: 613051dac40d ("drm: locking&new iterators for connector_list"
> Cc: <stable@vger.kernel.org> # v4.11+
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Javier Martinez Canillas <javier@dowhile0.org>
> Cc: Shuah Khan <shuahkh@osg.samsung.com>
> Cc: Guillaume Tucker <guillaume.tucker@collabora.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Kevin Hilman <khilman@baylibre.com>
> Cc: Matt Hart <matthew.hart@linaro.org>
> Cc: Thierry Escande <thierry.escande@collabora.co.uk>
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
> drivers/gpu/drm/drm_connector.c | 46 ++++++++++++++++++++++++++-----------
> drivers/gpu/drm/drm_crtc_internal.h | 1 +
> drivers/gpu/drm/drm_mode_config.c | 5 +++-
> include/drm/drm_connector.h | 4 ++--
> include/drm/drm_mode_config.h | 17 +++++++++++++-
> 5 files changed, 55 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 0b7e0974e6da..2f43dc217936 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -153,14 +153,23 @@ static void drm_connector_free(struct kref *kref)
> connector->funcs->destroy(connector);
> }
>
> -static void drm_connector_free_work_fn(struct work_struct *work)
> +void drm_connector_free_work_fn(struct work_struct *work)
> {
> - struct drm_connector *connector =
> - container_of(work, struct drm_connector, free_work);
> - struct drm_device *dev = connector->dev;
> + struct drm_connector *connector;
> + struct drm_device *dev =
> + container_of(work, struct drm_device, mode_config.connector_free_work);
> + struct drm_mode_config *config = &dev->mode_config;
> + unsigned long flags;
> + LIST_HEAD(list_head);
>
> - drm_mode_object_unregister(dev, &connector->base);
> - connector->funcs->destroy(connector);
> + spin_lock_irqsave(&config->connector_list_lock, flags);
> + list_splice(&config->connector_free_list, &list_head);
> + spin_unlock_irqrestore(&config->connector_list_lock, flags);
> +
> + list_for_each_entry(connector, &list_head, free_head) {
> + drm_mode_object_unregister(dev, &connector->base);
> + connector->funcs->destroy(connector);
> + }
> }
>
> /**
> @@ -192,8 +201,6 @@ int drm_connector_init(struct drm_device *dev,
> if (ret)
> return ret;
>
> - INIT_WORK(&connector->free_work, drm_connector_free_work_fn);
> -
> connector->base.properties = &connector->properties;
> connector->dev = dev;
> connector->funcs = funcs;
> @@ -550,10 +557,15 @@ EXPORT_SYMBOL(drm_connector_list_iter_begin);
> * actually release the connector when dropping our final reference.
> */
> static void
> -drm_connector_put_safe(struct drm_connector *conn)
> +__drm_connector_put_safe(struct drm_connector *conn)
> {
> + struct drm_mode_config *config = &conn->dev->mode_config;
> +
> + lockdep_assert_held(&config->connector_list_lock);
> + list_add(&conn->free_head, &config->connector_free_list);
Adding to the free_list before refcount==0?
May I suggest,
if (!refcount_dec_and_test())
return;
if (llist_add(&conn->free_link, &config->connector_free_list))
schedule_work(&config->connector_free_work);
llist being much more friendly for freelists:
void drm_connector_free_work_fn(struct work_struct *work)
{
struct drm_device *dev =
container_of(work, struct drm_device, mode_config.connector_free_work);
struct drm_mode_config *config = &dev->mode_config;
struct drm_connector *connector, *n;
struct llist_node *freed;
freed = llist_del_all(config->connector_free_list);
llist_for_each_entry_safe(connector, n, freed, free_link) {
drm_mode_object_unregister(dev, &connector->base);
connector->funcs->destroy(connector);
}
}
-Chris
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm: rework delayed connector cleanup in connector_iter
@ 2017-12-13 11:04 ` Chris Wilson
0 siblings, 0 replies; 17+ messages in thread
From: Chris Wilson @ 2017-12-13 11:04 UTC (permalink / raw)
To: Intel Graphics Development
Cc: DRI Development, Daniel Vetter, Ben Widawsky, Dave Airlie,
Sean Paul, stable, Daniel Vetter, Jani Nikula, Gustavo Padovan,
David Airlie, Javier Martinez Canillas, Shuah Khan,
Guillaume Tucker, Mark Brown, Kevin Hilman, Matt Hart,
Thierry Escande, Tomeu Vizoso, Enric
Quoting Daniel Vetter (2017-12-13 10:45:53)
> PROBE_DEFER also uses system_wq to reprobe drivers, which means when
> that again fails, and we try to flush the overall system_wq (to get
> all the delayed connectore cleanup work_struct completed), we
> deadlock.
>
> Fix this by using just a single cleanup work, so that we can only
> flush that one and don't block on anything else. That means a free
> list plus locking, a standard pattern.
>
> Fixes: a703c55004e1 ("drm: safely free connectors from connector_iter")
> Fixes: 613051dac40d ("drm: locking&new iterators for connector_list")
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Dave Airlie <airlied@gmail.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: <stable@vger.kernel.org> # v4.11+: 613051dac40d ("drm: locking&new iterators for connector_list"
> Cc: <stable@vger.kernel.org> # v4.11+
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Javier Martinez Canillas <javier@dowhile0.org>
> Cc: Shuah Khan <shuahkh@osg.samsung.com>
> Cc: Guillaume Tucker <guillaume.tucker@collabora.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Kevin Hilman <khilman@baylibre.com>
> Cc: Matt Hart <matthew.hart@linaro.org>
> Cc: Thierry Escande <thierry.escande@collabora.co.uk>
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
> drivers/gpu/drm/drm_connector.c | 46 ++++++++++++++++++++++++++-----------
> drivers/gpu/drm/drm_crtc_internal.h | 1 +
> drivers/gpu/drm/drm_mode_config.c | 5 +++-
> include/drm/drm_connector.h | 4 ++--
> include/drm/drm_mode_config.h | 17 +++++++++++++-
> 5 files changed, 55 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 0b7e0974e6da..2f43dc217936 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -153,14 +153,23 @@ static void drm_connector_free(struct kref *kref)
> connector->funcs->destroy(connector);
> }
>
> -static void drm_connector_free_work_fn(struct work_struct *work)
> +void drm_connector_free_work_fn(struct work_struct *work)
> {
> - struct drm_connector *connector =
> - container_of(work, struct drm_connector, free_work);
> - struct drm_device *dev = connector->dev;
> + struct drm_connector *connector;
> + struct drm_device *dev =
> + container_of(work, struct drm_device, mode_config.connector_free_work);
> + struct drm_mode_config *config = &dev->mode_config;
> + unsigned long flags;
> + LIST_HEAD(list_head);
>
> - drm_mode_object_unregister(dev, &connector->base);
> - connector->funcs->destroy(connector);
> + spin_lock_irqsave(&config->connector_list_lock, flags);
> + list_splice(&config->connector_free_list, &list_head);
> + spin_unlock_irqrestore(&config->connector_list_lock, flags);
> +
> + list_for_each_entry(connector, &list_head, free_head) {
> + drm_mode_object_unregister(dev, &connector->base);
> + connector->funcs->destroy(connector);
> + }
> }
>
> /**
> @@ -192,8 +201,6 @@ int drm_connector_init(struct drm_device *dev,
> if (ret)
> return ret;
>
> - INIT_WORK(&connector->free_work, drm_connector_free_work_fn);
> -
> connector->base.properties = &connector->properties;
> connector->dev = dev;
> connector->funcs = funcs;
> @@ -550,10 +557,15 @@ EXPORT_SYMBOL(drm_connector_list_iter_begin);
> * actually release the connector when dropping our final reference.
> */
> static void
> -drm_connector_put_safe(struct drm_connector *conn)
> +__drm_connector_put_safe(struct drm_connector *conn)
> {
> + struct drm_mode_config *config = &conn->dev->mode_config;
> +
> + lockdep_assert_held(&config->connector_list_lock);
> + list_add(&conn->free_head, &config->connector_free_list);
Adding to the free_list before refcount==0?
May I suggest,
if (!refcount_dec_and_test())
return;
if (llist_add(&conn->free_link, &config->connector_free_list))
schedule_work(&config->connector_free_work);
llist being much more friendly for freelists:
void drm_connector_free_work_fn(struct work_struct *work)
{
struct drm_device *dev =
container_of(work, struct drm_device, mode_config.connector_free_work);
struct drm_mode_config *config = &dev->mode_config;
struct drm_connector *connector, *n;
struct llist_node *freed;
freed = llist_del_all(config->connector_free_list);
llist_for_each_entry_safe(connector, n, freed, free_link) {
drm_mode_object_unregister(dev, &connector->base);
connector->funcs->destroy(connector);
}
}
-Chris
^ permalink raw reply [flat|nested] 17+ messages in thread
* ✗ Fi.CI.BAT: failure for drm: rework delayed connector cleanup in connector_iter
2017-12-13 10:45 ` Daniel Vetter
(?)
(?)
@ 2017-12-13 11:48 ` Patchwork
-1 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2017-12-13 11:48 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
== Series Details ==
Series: drm: rework delayed connector cleanup in connector_iter
URL : https://patchwork.freedesktop.org/series/35282/
State : failure
== Summary ==
Series 35282v1 drm: rework delayed connector cleanup in connector_iter
https://patchwork.freedesktop.org/api/1.0/series/35282/revisions/1/mbox/
Test core_auth:
Subgroup basic-auth:
pass -> DMESG-WARN (fi-blb-e6850)
pass -> DMESG-WARN (fi-pnv-d510)
Test core_prop_blob:
Subgroup basic:
pass -> DMESG-WARN (fi-blb-e6850)
pass -> DMESG-WARN (fi-pnv-d510)
Test debugfs_test:
Subgroup read_all_entries:
pass -> DMESG-WARN (fi-gdg-551)
pass -> DMESG-WARN (fi-blb-e6850)
pass -> DMESG-WARN (fi-pnv-d510)
pass -> DMESG-WARN (fi-ilk-650)
pass -> DMESG-WARN (fi-snb-2520m)
pass -> DMESG-WARN (fi-ivb-3520m)
pass -> DMESG-WARN (fi-hsw-4770)
pass -> DMESG-WARN (fi-bdw-gvtdvm) fdo#103938
pass -> DMESG-WARN (fi-skl-6260u)
pass -> DMESG-WARN (fi-skl-6600u)
pass -> DMESG-WARN (fi-skl-6700hq)
pass -> DMESG-WARN (fi-skl-6770hq)
pass -> DMESG-WARN (fi-skl-gvtdvm)
pass -> DMESG-WARN (fi-bxt-dsi)
pass -> DMESG-WARN (fi-bxt-j4205)
pass -> DMESG-WARN (fi-kbl-7500u) fdo#103285
pass -> DMESG-WARN (fi-kbl-7560u)
pass -> DMESG-WARN (fi-kbl-7567u)
pass -> DMESG-WARN (fi-kbl-r)
pass -> DMESG-WARN (fi-glk-1)
Test drv_getparams_basic:
Subgroup basic-eu-total:
pass -> DMESG-WARN (fi-blb-e6850)
pass -> DMESG-WARN (fi-pnv-d510)
Subgroup basic-subslice-total:
pass -> DMESG-WARN (fi-blb-e6850)
pass -> DMESG-WARN (fi-pnv-d510)
Test drv_hangman:
Subgroup error-state-basic:
pass -> DMESG-WARN (fi-blb-e6850)
pass -> DMESG-WARN (fi-pnv-d510)
Test gem_basic:
Subgroup bad-close:
pass -> DMESG-WARN (fi-blb-e6850)
pass -> DMESG-WARN (fi-pnv-d510)
Subgroup create-close:
pass -> DMESG-WARN (fi-blb-e6850)
pass -> DMESG-WARN (fi-pnv-d510)
Subgroup create-fd-close:
pass -> DMESG-WARN (fi-blb-e6850)
pass -> DMESG-WARN (fi-pnv-d510)
Test gem_busy:
Subgroup basic-busy-default:
pass -> DMESG-WARN (fi-blb-e6850)
pass -> DMESG-WARN (fi-pnv-d510)
Subgroup basic-hang-default:
pass -> DMESG-WARN (fi-blb-e6850)
pass -> DMESG-WARN (fi-pnv-d510)
Test gem_close_race:
Subgroup basic-process:
pass -> DMESG-WARN (fi-blb-e6850)
pass -> DMESG-WARN (fi-pnv-d510)
Subgroup basic-threads:
pass -> DMESG-WARN (fi-blb-e6850)
pass -> DMESG-WARN (fi-pnv-d510)
Test gem_cpu_reloc:
Subgroup basic:
pass -> DMESG-WARN (fi-blb-e6850)
pass -> DMESG-WARN (fi-pnv-d510)
Test gem_cs_tlb:
Subgroup basic-default:
pass -> DMESG-WARN (fi-blb-e6850)
pass -> DMESG-WARN (fi-pnv-d510)
Test gem_exec_basic:
Subgroup basic-default:
pass -> DMESG-WARN (fi-blb-e6850)
pass -> DMESG-WARN (fi-pnv-d510)
Subgroup basic-render:
pass -> DMESG-WARN (fi-blb-e6850)
pass -> DMESG-WARN (fi-pnv-d510)
Subgroup gtt-default:
pass -> DMESG-WARN (fi-blb-e6850)
pass -> DMESG-WARN (fi-pnv-d510)
Subgroup gtt-render:
pass -> DMESG-WARN (fi-blb-e6850)
pass -> DMESG-WARN (fi-pnv-d510)
Subgroup readonly-default:
pass -> DMESG-WARN (fi-blb-e6850)
pass -> DMESG-WARN (fi-pnv-d510)
Subgroup readonly-render:
pass -> DMESG-WARN (fi-blb-e6850)
pass -> DMESG-WARN (fi-pnv-d510)
Test gem_exec_create:
Subgroup basic:
pass -> DMESG-WARN (fi-blb-e6850)
pass -> DMESG-WARN (fi-pnv-d510)
Test gem_exec_fence:
WARNING: Long output truncated
fi-skl-6700k failed to collect. IGT log at Patchwork_7482/fi-skl-6700k/igt.log
0f6e2aba6ac5889ede348f37caf12fb30a366fc5 drm-tip: 2017y-12m-13d-10h-46m-31s UTC integration manifest
96210dc8b32d drm: rework delayed connector cleanup in connector_iter
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7482/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm: rework delayed connector cleanup in connector_iter
2017-12-13 10:45 ` Daniel Vetter
` (2 preceding siblings ...)
(?)
@ 2017-12-13 12:27 ` Marek Szyprowski
-1 siblings, 0 replies; 17+ messages in thread
From: Marek Szyprowski @ 2017-12-13 12:27 UTC (permalink / raw)
To: Daniel Vetter, Intel Graphics Development
Cc: Ben Widawsky, Tomeu Vizoso, David Airlie, Mark Brown, Shuah Khan,
DRI Development, Thierry Escande, Guillaume Tucker, stable,
Enric Balletbo i Serra, Daniel Vetter, Javier Martinez Canillas,
Kevin Hilman, Matt Hart
Hi Daniel,
On 2017-12-13 11:45, Daniel Vetter wrote:
> PROBE_DEFER also uses system_wq to reprobe drivers, which means when
> that again fails, and we try to flush the overall system_wq (to get
> all the delayed connectore cleanup work_struct completed), we
> deadlock.
>
> Fix this by using just a single cleanup work, so that we can only
> flush that one and don't block on anything else. That means a free
> list plus locking, a standard pattern.
>
> Fixes: a703c55004e1 ("drm: safely free connectors from connector_iter")
> Fixes: 613051dac40d ("drm: locking&new iterators for connector_list")
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Dave Airlie <airlied@gmail.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: <stable@vger.kernel.org> # v4.11+: 613051dac40d ("drm: locking&new iterators for connector_list"
> Cc: <stable@vger.kernel.org> # v4.11+
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Javier Martinez Canillas <javier@dowhile0.org>
> Cc: Shuah Khan <shuahkh@osg.samsung.com>
> Cc: Guillaume Tucker <guillaume.tucker@collabora.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Kevin Hilman <khilman@baylibre.com>
> Cc: Matt Hart <matthew.hart@linaro.org>
> Cc: Thierry Escande <thierry.escande@collabora.co.uk>
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Sadly, this patch doesn't work in my test environment (Samsung Snow
Chromebook with modified DTS), which reproduces the problem discussed
inhttps://www.spinics.net/lists/arm-kernel/msg622332.html
[ 4.925178] exynos-drm exynos-drm: failed to bind 14530000.hdmi (ops
hdmi_component_ops): -19
[ 4.936167] Unable to handle kernel paging request at virtual address
fffffcac
[ 4.942004] pgd = ad8213ad
[ 4.944654] [fffffcac] *pgd=6fffd861, *pte=00000000, *ppte=00000000
[ 4.950944] Internal error: Oops: 37 [#1] PREEMPT SMP ARM
[ 4.956284] Modules linked in:
[ 4.959324] CPU: 1 PID: 80 Comm: kworker/1:3 Not tainted
4.15.0-rc3-next-20171212-00004-g879e6206b6fb-dirty #56
[ 4.969391] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[ 4.975472] Workqueue: events drm_connector_free_work_fn
[ 4.980765] PC is at drm_mode_object_unregister+0x1c/0x48
[ 4.986146] LR is at __mutex_lock+0x3ec/0xa0c
[ 4.990483] pc : [<c050e744>] lr : [<c0a0465c>] psr: 60000113
[ 4.996731] sp : ed27dec0 ip : ed27c000 fp : ed25042c
[ 5.001940] r10: 00000000 r9 : c1008c6c r8 : c10ae6d4
[ 5.007148] r7 : ed2503ec r6 : ed250000 r5 : fffffcac r4 : ed250330
[ 5.013659] r3 : 00000000 r2 : 00000001 r1 : ed27de50 r0 : 00000000
[ 5.020170] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM
Segment none
[ 5.027286] Control: 10c5387d Table: 4000406a DAC: 00000051
[ 5.033016] Process kworker/1:3 (pid: 80, stack limit = 0x37cd389f)
[ 5.039265] Stack: (0xed27dec0 to 0xed27e000)
[ 5.043607] dec0: fffffc98 ed27ded0 ed250000 c050d374 ed242f78
ed242f78 00000001 ed22e200
[ 5.051766] dee0: eefc91c0 eefcc500 c10ad21d c0143438 00000001
00000000 c0143384 c0143534
[ 5.059925] df00: 00000001 00000000 c0143384 00000000 edd0ce00
c181658c c1214a78 00000000
[ 5.068084] df20: c0d7c514 00000000 c120f048 ed22e200 ed22e218
eefc91f8 c0143ae8 c10ae166
[ 5.076243] df40: 00000008 ed22e218 c1005d00 c0143a2c eefcc5f5
ed22e200 eefc91c0 c0143c4c
[ 5.084402] df60: c0a09894 ed709100 00000000 ed709100 00000000
ed25e000 ed709180 ed22e200
[ 5.092561] df80: edfdfe68 c0143a3c 00000000 c014a1c4 ed25e000
c014a074 00000000 00000000
[ 5.100720] dfa0: 00000000 00000000 00000000 c01010b4 00000000
00000000 00000000 00000000
[ 5.108879] dfc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[ 5.117039] dfe0: 00000000 00000000 00000000 00000000 00000013
00000000 00000000 00000000
[ 5.125202] [<c050e744>] (drm_mode_object_unregister) from
[<c050d374>] (drm_connector_free_work_fn+0x80/0xa8)
[ 5.135185] [<c050d374>] (drm_connector_free_work_fn) from
[<c0143438>] (process_one_work+0x1dc/0x7a8)
[ 5.144472] [<c0143438>] (process_one_work) from [<c0143a2c>]
(process_scheduled_works+0x28/0x38)
[ 5.153324] [<c0143a2c>] (process_scheduled_works) from [<c0143c4c>]
(worker_thread+0x210/0x4dc)
[ 5.162092] [<c0143c4c>] (worker_thread) from [<c014a1c4>]
(kthread+0x150/0x19c)
[ 5.169469] [<c014a1c4>] (kthread) from [<c01010b4>]
(ret_from_fork+0x14/0x20)
[ 5.176669] Exception stack(0xed27dfb0 to 0xed27dff8)
[ 5.181704] dfa0: 00000000
00000000 00000000 00000000
[ 5.189864] dfc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[ 5.198023] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
[ 5.204622] Code: e1a06000 e3a01000 e1a00004 eb13d968 (e5951000)
[ 5.210731] ---[ end trace 55b8b9b30581db4f ]---
I will try to provide a bit more details soon.
> ---
> drivers/gpu/drm/drm_connector.c | 46 ++++++++++++++++++++++++++-----------
> drivers/gpu/drm/drm_crtc_internal.h | 1 +
> drivers/gpu/drm/drm_mode_config.c | 5 +++-
> include/drm/drm_connector.h | 4 ++--
> include/drm/drm_mode_config.h | 17 +++++++++++++-
> 5 files changed, 55 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 0b7e0974e6da..2f43dc217936 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -153,14 +153,23 @@ static void drm_connector_free(struct kref *kref)
> connector->funcs->destroy(connector);
> }
>
> -static void drm_connector_free_work_fn(struct work_struct *work)
> +void drm_connector_free_work_fn(struct work_struct *work)
> {
> - struct drm_connector *connector =
> - container_of(work, struct drm_connector, free_work);
> - struct drm_device *dev = connector->dev;
> + struct drm_connector *connector;
> + struct drm_device *dev =
> + container_of(work, struct drm_device, mode_config.connector_free_work);
> + struct drm_mode_config *config = &dev->mode_config;
> + unsigned long flags;
> + LIST_HEAD(list_head);
>
> - drm_mode_object_unregister(dev, &connector->base);
> - connector->funcs->destroy(connector);
> + spin_lock_irqsave(&config->connector_list_lock, flags);
> + list_splice(&config->connector_free_list, &list_head);
> + spin_unlock_irqrestore(&config->connector_list_lock, flags);
> +
> + list_for_each_entry(connector, &list_head, free_head) {
> + drm_mode_object_unregister(dev, &connector->base);
> + connector->funcs->destroy(connector);
> + }
> }
>
> /**
> @@ -192,8 +201,6 @@ int drm_connector_init(struct drm_device *dev,
> if (ret)
> return ret;
>
> - INIT_WORK(&connector->free_work, drm_connector_free_work_fn);
> -
> connector->base.properties = &connector->properties;
> connector->dev = dev;
> connector->funcs = funcs;
> @@ -550,10 +557,15 @@ EXPORT_SYMBOL(drm_connector_list_iter_begin);
> * actually release the connector when dropping our final reference.
> */
> static void
> -drm_connector_put_safe(struct drm_connector *conn)
> +__drm_connector_put_safe(struct drm_connector *conn)
> {
> + struct drm_mode_config *config = &conn->dev->mode_config;
> +
> + lockdep_assert_held(&config->connector_list_lock);
> + list_add(&conn->free_head, &config->connector_free_list);
> +
> if (refcount_dec_and_test(&conn->base.refcount.refcount))
> - schedule_work(&conn->free_work);
> + schedule_work(&config->connector_free_work);
> }
>
> /**
> @@ -585,10 +597,10 @@ drm_connector_list_iter_next(struct drm_connector_list_iter *iter)
>
> /* loop until it's not a zombie connector */
> } while (!kref_get_unless_zero(&iter->conn->base.refcount));
> - spin_unlock_irqrestore(&config->connector_list_lock, flags);
>
> if (old_conn)
> - drm_connector_put_safe(old_conn);
> + __drm_connector_put_safe(old_conn);
> + spin_unlock_irqrestore(&config->connector_list_lock, flags);
>
> return iter->conn;
> }
> @@ -605,9 +617,15 @@ EXPORT_SYMBOL(drm_connector_list_iter_next);
> */
> void drm_connector_list_iter_end(struct drm_connector_list_iter *iter)
> {
> + struct drm_mode_config *config = &iter->dev->mode_config;
> + unsigned long flags;
> +
> iter->dev = NULL;
> - if (iter->conn)
> - drm_connector_put_safe(iter->conn);
> + if (iter->conn) {
> + spin_lock_irqsave(&config->connector_list_lock, flags);
> + __drm_connector_put_safe(iter->conn);
> + spin_unlock_irqrestore(&config->connector_list_lock, flags);
> + }
> lock_release(&connector_list_iter_dep_map, 0, _RET_IP_);
> }
> EXPORT_SYMBOL(drm_connector_list_iter_end);
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> index 9ebb8841778c..af00f42ba269 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -142,6 +142,7 @@ int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
> uint64_t value);
> int drm_connector_create_standard_properties(struct drm_device *dev);
> const char *drm_get_connector_force_name(enum drm_connector_force force);
> +void drm_connector_free_work_fn(struct work_struct *work);
>
> /* IOCTL */
> int drm_mode_connector_property_set_ioctl(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index 6ffe952142e6..025fc23cf7c6 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -373,6 +373,7 @@ void drm_mode_config_init(struct drm_device *dev)
> INIT_LIST_HEAD(&dev->mode_config.fb_list);
> INIT_LIST_HEAD(&dev->mode_config.crtc_list);
> INIT_LIST_HEAD(&dev->mode_config.connector_list);
> + INIT_LIST_HEAD(&dev->mode_config.connector_free_list);
> INIT_LIST_HEAD(&dev->mode_config.encoder_list);
> INIT_LIST_HEAD(&dev->mode_config.property_list);
> INIT_LIST_HEAD(&dev->mode_config.property_blob_list);
> @@ -382,6 +383,8 @@ void drm_mode_config_init(struct drm_device *dev)
> ida_init(&dev->mode_config.connector_ida);
> spin_lock_init(&dev->mode_config.connector_list_lock);
>
> + INIT_WORK(&dev->mode_config.connector_free_work, drm_connector_free_work_fn);
> +
> drm_mode_create_standard_properties(dev);
>
> /* Just to be sure */
> @@ -432,7 +435,7 @@ void drm_mode_config_cleanup(struct drm_device *dev)
> }
> drm_connector_list_iter_end(&conn_iter);
> /* connector_iter drops references in a work item. */
> - flush_scheduled_work();
> + flush_work(&dev->mode_config.connector_free_work);
> if (WARN_ON(!list_empty(&dev->mode_config.connector_list))) {
> drm_connector_list_iter_begin(dev, &conn_iter);
> drm_for_each_connector_iter(connector, &conn_iter)
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index ee3155391243..50bc21050fbf 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -966,12 +966,12 @@ struct drm_connector {
> uint16_t tile_h_size, tile_v_size;
>
> /**
> - * @free_work:
> + * @free_head:
> *
> * Work used only by &drm_connector_iter to be able to clean up a
> * connector from any context.
> */
> - struct work_struct free_work;
> + struct list_head free_head;
> };
>
> #define obj_to_connector(x) container_of(x, struct drm_connector, base)
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index e5f3b43014e1..5e62e52a2c58 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -402,7 +402,7 @@ struct drm_mode_config {
>
> /**
> * @connector_list_lock: Protects @num_connector and
> - * @connector_list.
> + * @connector_list and @connector_free_list.
> */
> spinlock_t connector_list_lock;
> /**
> @@ -422,6 +422,21 @@ struct drm_mode_config {
> * &struct drm_connector_list_iter to walk this list.
> */
> struct list_head connector_list;
> + /**
> + * @connector_free_list:
> + *
> + * List of connector objects linked with &drm_connector.free_head.
> + * Protected by @connector_list_lock. Used by
> + * drm_for_each_connector_iter() and
> + * &struct drm_connector_list_iter to savely free connectors using
> + * @connector_free_work.
> + */
> + struct list_head connector_free_list;
> + /**
> + * @connector_free_work: Work to clean up @connector_free_list.
> + */
> + struct work_struct connector_free_work;
> +
> /**
> * @num_encoder:
> *
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] drm: rework delayed connector cleanup in connector_iter
2017-12-13 10:45 ` Daniel Vetter
@ 2017-12-13 12:49 ` Daniel Vetter
-1 siblings, 0 replies; 17+ messages in thread
From: Daniel Vetter @ 2017-12-13 12:49 UTC (permalink / raw)
To: Intel Graphics Development
Cc: DRI Development, Daniel Vetter, Ben Widawsky, Dave Airlie,
Chris Wilson, Sean Paul, stable, Daniel Vetter, Jani Nikula,
Gustavo Padovan, David Airlie, Javier Martinez Canillas,
Shuah Khan, Guillaume Tucker, Mark Brown, Kevin Hilman,
Matt Hart, Thierry Escande, Tomeu Vizoso, Enric Balletbo i Serra
PROBE_DEFER also uses system_wq to reprobe drivers, which means when
that again fails, and we try to flush the overall system_wq (to get
all the delayed connectore cleanup work_struct completed), we
deadlock.
Fix this by using just a single cleanup work, so that we can only
flush that one and don't block on anything else. That means a free
list plus locking, a standard pattern.
v2:
- Correctly free connectors only on last ref. Oops (Chris).
- use llist_head/node (Chris).
Fixes: a703c55004e1 ("drm: safely free connectors from connector_iter")
Fixes: 613051dac40d ("drm: locking&new iterators for connector_list")
Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Dave Airlie <airlied@gmail.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: <stable@vger.kernel.org> # v4.11+: 613051dac40d ("drm: locking&new iterators for connector_list"
Cc: <stable@vger.kernel.org> # v4.11+
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: David Airlie <airlied@linux.ie>
Cc: Javier Martinez Canillas <javier@dowhile0.org>
Cc: Shuah Khan <shuahkh@osg.samsung.com>
Cc: Guillaume Tucker <guillaume.tucker@collabora.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Kevin Hilman <khilman@baylibre.com>
Cc: Matt Hart <matthew.hart@linaro.org>
Cc: Thierry Escande <thierry.escande@collabora.co.uk>
Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
drivers/gpu/drm/drm_connector.c | 50 ++++++++++++++++++++++++++-----------
drivers/gpu/drm/drm_crtc_internal.h | 1 +
drivers/gpu/drm/drm_mode_config.c | 4 ++-
include/drm/drm_connector.h | 10 +++++---
include/drm/drm_mode_config.h | 18 ++++++++++++-
5 files changed, 62 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 0b7e0974e6da..3f53f127e1f2 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -153,14 +153,23 @@ static void drm_connector_free(struct kref *kref)
connector->funcs->destroy(connector);
}
-static void drm_connector_free_work_fn(struct work_struct *work)
+void drm_connector_free_work_fn(struct work_struct *work)
{
- struct drm_connector *connector =
- container_of(work, struct drm_connector, free_work);
- struct drm_device *dev = connector->dev;
+ struct drm_connector *connector, *n;
+ struct drm_device *dev =
+ container_of(work, struct drm_device, mode_config.connector_free_work);
+ struct drm_mode_config *config = &dev->mode_config;
+ unsigned long flags;
+ struct llist_node *freed;
- drm_mode_object_unregister(dev, &connector->base);
- connector->funcs->destroy(connector);
+ spin_lock_irqsave(&config->connector_list_lock, flags);
+ freed = llist_del_all(&config->connector_free_list);
+ spin_unlock_irqrestore(&config->connector_list_lock, flags);
+
+ llist_for_each_entry_safe(connector, n, freed, free_node) {
+ drm_mode_object_unregister(dev, &connector->base);
+ connector->funcs->destroy(connector);
+ }
}
/**
@@ -192,8 +201,6 @@ int drm_connector_init(struct drm_device *dev,
if (ret)
return ret;
- INIT_WORK(&connector->free_work, drm_connector_free_work_fn);
-
connector->base.properties = &connector->properties;
connector->dev = dev;
connector->funcs = funcs;
@@ -550,10 +557,17 @@ EXPORT_SYMBOL(drm_connector_list_iter_begin);
* actually release the connector when dropping our final reference.
*/
static void
-drm_connector_put_safe(struct drm_connector *conn)
+__drm_connector_put_safe(struct drm_connector *conn)
{
- if (refcount_dec_and_test(&conn->base.refcount.refcount))
- schedule_work(&conn->free_work);
+ struct drm_mode_config *config = &conn->dev->mode_config;
+
+ lockdep_assert_held(&config->connector_list_lock);
+
+ if (!refcount_dec_and_test(&conn->base.refcount.refcount))
+ return;
+
+ llist_add(&conn->free_node, &config->connector_free_list);
+ schedule_work(&config->connector_free_work);
}
/**
@@ -585,10 +599,10 @@ drm_connector_list_iter_next(struct drm_connector_list_iter *iter)
/* loop until it's not a zombie connector */
} while (!kref_get_unless_zero(&iter->conn->base.refcount));
- spin_unlock_irqrestore(&config->connector_list_lock, flags);
if (old_conn)
- drm_connector_put_safe(old_conn);
+ __drm_connector_put_safe(old_conn);
+ spin_unlock_irqrestore(&config->connector_list_lock, flags);
return iter->conn;
}
@@ -605,9 +619,15 @@ EXPORT_SYMBOL(drm_connector_list_iter_next);
*/
void drm_connector_list_iter_end(struct drm_connector_list_iter *iter)
{
+ struct drm_mode_config *config = &iter->dev->mode_config;
+ unsigned long flags;
+
iter->dev = NULL;
- if (iter->conn)
- drm_connector_put_safe(iter->conn);
+ if (iter->conn) {
+ spin_lock_irqsave(&config->connector_list_lock, flags);
+ __drm_connector_put_safe(iter->conn);
+ spin_unlock_irqrestore(&config->connector_list_lock, flags);
+ }
lock_release(&connector_list_iter_dep_map, 0, _RET_IP_);
}
EXPORT_SYMBOL(drm_connector_list_iter_end);
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 9ebb8841778c..af00f42ba269 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -142,6 +142,7 @@ int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
uint64_t value);
int drm_connector_create_standard_properties(struct drm_device *dev);
const char *drm_get_connector_force_name(enum drm_connector_force force);
+void drm_connector_free_work_fn(struct work_struct *work);
/* IOCTL */
int drm_mode_connector_property_set_ioctl(struct drm_device *dev,
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index 6ffe952142e6..7681269abe41 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -382,6 +382,8 @@ void drm_mode_config_init(struct drm_device *dev)
ida_init(&dev->mode_config.connector_ida);
spin_lock_init(&dev->mode_config.connector_list_lock);
+ INIT_WORK(&dev->mode_config.connector_free_work, drm_connector_free_work_fn);
+
drm_mode_create_standard_properties(dev);
/* Just to be sure */
@@ -432,7 +434,7 @@ void drm_mode_config_cleanup(struct drm_device *dev)
}
drm_connector_list_iter_end(&conn_iter);
/* connector_iter drops references in a work item. */
- flush_scheduled_work();
+ flush_work(&dev->mode_config.connector_free_work);
if (WARN_ON(!list_empty(&dev->mode_config.connector_list))) {
drm_connector_list_iter_begin(dev, &conn_iter);
drm_for_each_connector_iter(connector, &conn_iter)
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index ee3155391243..ed38df4ac204 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -24,6 +24,7 @@
#define __DRM_CONNECTOR_H__
#include <linux/list.h>
+#include <linux/llist.h>
#include <linux/ctype.h>
#include <linux/hdmi.h>
#include <drm/drm_mode_object.h>
@@ -966,12 +967,13 @@ struct drm_connector {
uint16_t tile_h_size, tile_v_size;
/**
- * @free_work:
+ * @free_node:
*
- * Work used only by &drm_connector_iter to be able to clean up a
- * connector from any context.
+ * List used only by &drm_connector_iter to be able to clean up a
+ * connector from any context, in conjunction with
+ * &drm_mode_config.connector_free_work.
*/
- struct work_struct free_work;
+ struct llist_node free_node;
};
#define obj_to_connector(x) container_of(x, struct drm_connector, base)
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index e5f3b43014e1..2cb6f02df64a 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -27,6 +27,7 @@
#include <linux/types.h>
#include <linux/idr.h>
#include <linux/workqueue.h>
+#include <linux/llist.h>
#include <drm/drm_modeset_lock.h>
@@ -402,7 +403,7 @@ struct drm_mode_config {
/**
* @connector_list_lock: Protects @num_connector and
- * @connector_list.
+ * @connector_list and @connector_free_list.
*/
spinlock_t connector_list_lock;
/**
@@ -422,6 +423,21 @@ struct drm_mode_config {
* &struct drm_connector_list_iter to walk this list.
*/
struct list_head connector_list;
+ /**
+ * @connector_free_list:
+ *
+ * List of connector objects linked with &drm_connector.free_head.
+ * Protected by @connector_list_lock. Used by
+ * drm_for_each_connector_iter() and
+ * &struct drm_connector_list_iter to savely free connectors using
+ * @connector_free_work.
+ */
+ struct llist_head connector_free_list;
+ /**
+ * @connector_free_work: Work to clean up @connector_free_list.
+ */
+ struct work_struct connector_free_work;
+
/**
* @num_encoder:
*
--
2.15.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH] drm: rework delayed connector cleanup in connector_iter
@ 2017-12-13 12:49 ` Daniel Vetter
0 siblings, 0 replies; 17+ messages in thread
From: Daniel Vetter @ 2017-12-13 12:49 UTC (permalink / raw)
To: Intel Graphics Development
Cc: Ben Widawsky, Tomeu Vizoso, David Airlie, Daniel Vetter,
Mark Brown, Shuah Khan, DRI Development, Thierry Escande, stable,
Enric Balletbo i Serra, Daniel Vetter, Javier Martinez Canillas,
Kevin Hilman, Matt Hart
PROBE_DEFER also uses system_wq to reprobe drivers, which means when
that again fails, and we try to flush the overall system_wq (to get
all the delayed connectore cleanup work_struct completed), we
deadlock.
Fix this by using just a single cleanup work, so that we can only
flush that one and don't block on anything else. That means a free
list plus locking, a standard pattern.
v2:
- Correctly free connectors only on last ref. Oops (Chris).
- use llist_head/node (Chris).
Fixes: a703c55004e1 ("drm: safely free connectors from connector_iter")
Fixes: 613051dac40d ("drm: locking&new iterators for connector_list")
Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Dave Airlie <airlied@gmail.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: <stable@vger.kernel.org> # v4.11+: 613051dac40d ("drm: locking&new iterators for connector_list"
Cc: <stable@vger.kernel.org> # v4.11+
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: David Airlie <airlied@linux.ie>
Cc: Javier Martinez Canillas <javier@dowhile0.org>
Cc: Shuah Khan <shuahkh@osg.samsung.com>
Cc: Guillaume Tucker <guillaume.tucker@collabora.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Kevin Hilman <khilman@baylibre.com>
Cc: Matt Hart <matthew.hart@linaro.org>
Cc: Thierry Escande <thierry.escande@collabora.co.uk>
Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
drivers/gpu/drm/drm_connector.c | 50 ++++++++++++++++++++++++++-----------
drivers/gpu/drm/drm_crtc_internal.h | 1 +
drivers/gpu/drm/drm_mode_config.c | 4 ++-
include/drm/drm_connector.h | 10 +++++---
include/drm/drm_mode_config.h | 18 ++++++++++++-
5 files changed, 62 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 0b7e0974e6da..3f53f127e1f2 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -153,14 +153,23 @@ static void drm_connector_free(struct kref *kref)
connector->funcs->destroy(connector);
}
-static void drm_connector_free_work_fn(struct work_struct *work)
+void drm_connector_free_work_fn(struct work_struct *work)
{
- struct drm_connector *connector =
- container_of(work, struct drm_connector, free_work);
- struct drm_device *dev = connector->dev;
+ struct drm_connector *connector, *n;
+ struct drm_device *dev =
+ container_of(work, struct drm_device, mode_config.connector_free_work);
+ struct drm_mode_config *config = &dev->mode_config;
+ unsigned long flags;
+ struct llist_node *freed;
- drm_mode_object_unregister(dev, &connector->base);
- connector->funcs->destroy(connector);
+ spin_lock_irqsave(&config->connector_list_lock, flags);
+ freed = llist_del_all(&config->connector_free_list);
+ spin_unlock_irqrestore(&config->connector_list_lock, flags);
+
+ llist_for_each_entry_safe(connector, n, freed, free_node) {
+ drm_mode_object_unregister(dev, &connector->base);
+ connector->funcs->destroy(connector);
+ }
}
/**
@@ -192,8 +201,6 @@ int drm_connector_init(struct drm_device *dev,
if (ret)
return ret;
- INIT_WORK(&connector->free_work, drm_connector_free_work_fn);
-
connector->base.properties = &connector->properties;
connector->dev = dev;
connector->funcs = funcs;
@@ -550,10 +557,17 @@ EXPORT_SYMBOL(drm_connector_list_iter_begin);
* actually release the connector when dropping our final reference.
*/
static void
-drm_connector_put_safe(struct drm_connector *conn)
+__drm_connector_put_safe(struct drm_connector *conn)
{
- if (refcount_dec_and_test(&conn->base.refcount.refcount))
- schedule_work(&conn->free_work);
+ struct drm_mode_config *config = &conn->dev->mode_config;
+
+ lockdep_assert_held(&config->connector_list_lock);
+
+ if (!refcount_dec_and_test(&conn->base.refcount.refcount))
+ return;
+
+ llist_add(&conn->free_node, &config->connector_free_list);
+ schedule_work(&config->connector_free_work);
}
/**
@@ -585,10 +599,10 @@ drm_connector_list_iter_next(struct drm_connector_list_iter *iter)
/* loop until it's not a zombie connector */
} while (!kref_get_unless_zero(&iter->conn->base.refcount));
- spin_unlock_irqrestore(&config->connector_list_lock, flags);
if (old_conn)
- drm_connector_put_safe(old_conn);
+ __drm_connector_put_safe(old_conn);
+ spin_unlock_irqrestore(&config->connector_list_lock, flags);
return iter->conn;
}
@@ -605,9 +619,15 @@ EXPORT_SYMBOL(drm_connector_list_iter_next);
*/
void drm_connector_list_iter_end(struct drm_connector_list_iter *iter)
{
+ struct drm_mode_config *config = &iter->dev->mode_config;
+ unsigned long flags;
+
iter->dev = NULL;
- if (iter->conn)
- drm_connector_put_safe(iter->conn);
+ if (iter->conn) {
+ spin_lock_irqsave(&config->connector_list_lock, flags);
+ __drm_connector_put_safe(iter->conn);
+ spin_unlock_irqrestore(&config->connector_list_lock, flags);
+ }
lock_release(&connector_list_iter_dep_map, 0, _RET_IP_);
}
EXPORT_SYMBOL(drm_connector_list_iter_end);
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 9ebb8841778c..af00f42ba269 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -142,6 +142,7 @@ int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
uint64_t value);
int drm_connector_create_standard_properties(struct drm_device *dev);
const char *drm_get_connector_force_name(enum drm_connector_force force);
+void drm_connector_free_work_fn(struct work_struct *work);
/* IOCTL */
int drm_mode_connector_property_set_ioctl(struct drm_device *dev,
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index 6ffe952142e6..7681269abe41 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -382,6 +382,8 @@ void drm_mode_config_init(struct drm_device *dev)
ida_init(&dev->mode_config.connector_ida);
spin_lock_init(&dev->mode_config.connector_list_lock);
+ INIT_WORK(&dev->mode_config.connector_free_work, drm_connector_free_work_fn);
+
drm_mode_create_standard_properties(dev);
/* Just to be sure */
@@ -432,7 +434,7 @@ void drm_mode_config_cleanup(struct drm_device *dev)
}
drm_connector_list_iter_end(&conn_iter);
/* connector_iter drops references in a work item. */
- flush_scheduled_work();
+ flush_work(&dev->mode_config.connector_free_work);
if (WARN_ON(!list_empty(&dev->mode_config.connector_list))) {
drm_connector_list_iter_begin(dev, &conn_iter);
drm_for_each_connector_iter(connector, &conn_iter)
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index ee3155391243..ed38df4ac204 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -24,6 +24,7 @@
#define __DRM_CONNECTOR_H__
#include <linux/list.h>
+#include <linux/llist.h>
#include <linux/ctype.h>
#include <linux/hdmi.h>
#include <drm/drm_mode_object.h>
@@ -966,12 +967,13 @@ struct drm_connector {
uint16_t tile_h_size, tile_v_size;
/**
- * @free_work:
+ * @free_node:
*
- * Work used only by &drm_connector_iter to be able to clean up a
- * connector from any context.
+ * List used only by &drm_connector_iter to be able to clean up a
+ * connector from any context, in conjunction with
+ * &drm_mode_config.connector_free_work.
*/
- struct work_struct free_work;
+ struct llist_node free_node;
};
#define obj_to_connector(x) container_of(x, struct drm_connector, base)
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index e5f3b43014e1..2cb6f02df64a 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -27,6 +27,7 @@
#include <linux/types.h>
#include <linux/idr.h>
#include <linux/workqueue.h>
+#include <linux/llist.h>
#include <drm/drm_modeset_lock.h>
@@ -402,7 +403,7 @@ struct drm_mode_config {
/**
* @connector_list_lock: Protects @num_connector and
- * @connector_list.
+ * @connector_list and @connector_free_list.
*/
spinlock_t connector_list_lock;
/**
@@ -422,6 +423,21 @@ struct drm_mode_config {
* &struct drm_connector_list_iter to walk this list.
*/
struct list_head connector_list;
+ /**
+ * @connector_free_list:
+ *
+ * List of connector objects linked with &drm_connector.free_head.
+ * Protected by @connector_list_lock. Used by
+ * drm_for_each_connector_iter() and
+ * &struct drm_connector_list_iter to savely free connectors using
+ * @connector_free_work.
+ */
+ struct llist_head connector_free_list;
+ /**
+ * @connector_free_work: Work to clean up @connector_free_list.
+ */
+ struct work_struct connector_free_work;
+
/**
* @num_encoder:
*
--
2.15.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] drm: rework delayed connector cleanup in connector_iter
2017-12-13 12:49 ` Daniel Vetter
(?)
@ 2017-12-13 13:01 ` Marek Szyprowski
-1 siblings, 0 replies; 17+ messages in thread
From: Marek Szyprowski @ 2017-12-13 13:01 UTC (permalink / raw)
To: Daniel Vetter, Intel Graphics Development
Cc: Ben Widawsky, Tomeu Vizoso, David Airlie, Mark Brown, Shuah Khan,
DRI Development, Thierry Escande, Guillaume Tucker, stable,
Enric Balletbo i Serra, Daniel Vetter, Javier Martinez Canillas,
Kevin Hilman, Matt Hart
Hi Daniel,
On 2017-12-13 13:49, Daniel Vetter wrote:
> PROBE_DEFER also uses system_wq to reprobe drivers, which means when
> that again fails, and we try to flush the overall system_wq (to get
> all the delayed connectore cleanup work_struct completed), we
> deadlock.
>
> Fix this by using just a single cleanup work, so that we can only
> flush that one and don't block on anything else. That means a free
> list plus locking, a standard pattern.
>
> v2:
> - Correctly free connectors only on last ref. Oops (Chris).
> - use llist_head/node (Chris).
>
> Fixes: a703c55004e1 ("drm: safely free connectors from connector_iter")
> Fixes: 613051dac40d ("drm: locking&new iterators for connector_list")
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Dave Airlie <airlied@gmail.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: <stable@vger.kernel.org> # v4.11+: 613051dac40d ("drm: locking&new iterators for connector_list"
> Cc: <stable@vger.kernel.org> # v4.11+
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Javier Martinez Canillas <javier@dowhile0.org>
> Cc: Shuah Khan <shuahkh@osg.samsung.com>
> Cc: Guillaume Tucker <guillaume.tucker@collabora.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Kevin Hilman <khilman@baylibre.com>
> Cc: Matt Hart <matthew.hart@linaro.org>
> Cc: Thierry Escande <thierry.escande@collabora.co.uk>
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
This one works fine and fixes deadlock in my test environment.
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> drivers/gpu/drm/drm_connector.c | 50 ++++++++++++++++++++++++++-----------
> drivers/gpu/drm/drm_crtc_internal.h | 1 +
> drivers/gpu/drm/drm_mode_config.c | 4 ++-
> include/drm/drm_connector.h | 10 +++++---
> include/drm/drm_mode_config.h | 18 ++++++++++++-
> 5 files changed, 62 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 0b7e0974e6da..3f53f127e1f2 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -153,14 +153,23 @@ static void drm_connector_free(struct kref *kref)
> connector->funcs->destroy(connector);
> }
>
> -static void drm_connector_free_work_fn(struct work_struct *work)
> +void drm_connector_free_work_fn(struct work_struct *work)
> {
> - struct drm_connector *connector =
> - container_of(work, struct drm_connector, free_work);
> - struct drm_device *dev = connector->dev;
> + struct drm_connector *connector, *n;
> + struct drm_device *dev =
> + container_of(work, struct drm_device, mode_config.connector_free_work);
> + struct drm_mode_config *config = &dev->mode_config;
> + unsigned long flags;
> + struct llist_node *freed;
>
> - drm_mode_object_unregister(dev, &connector->base);
> - connector->funcs->destroy(connector);
> + spin_lock_irqsave(&config->connector_list_lock, flags);
> + freed = llist_del_all(&config->connector_free_list);
> + spin_unlock_irqrestore(&config->connector_list_lock, flags);
> +
> + llist_for_each_entry_safe(connector, n, freed, free_node) {
> + drm_mode_object_unregister(dev, &connector->base);
> + connector->funcs->destroy(connector);
> + }
> }
>
> /**
> @@ -192,8 +201,6 @@ int drm_connector_init(struct drm_device *dev,
> if (ret)
> return ret;
>
> - INIT_WORK(&connector->free_work, drm_connector_free_work_fn);
> -
> connector->base.properties = &connector->properties;
> connector->dev = dev;
> connector->funcs = funcs;
> @@ -550,10 +557,17 @@ EXPORT_SYMBOL(drm_connector_list_iter_begin);
> * actually release the connector when dropping our final reference.
> */
> static void
> -drm_connector_put_safe(struct drm_connector *conn)
> +__drm_connector_put_safe(struct drm_connector *conn)
> {
> - if (refcount_dec_and_test(&conn->base.refcount.refcount))
> - schedule_work(&conn->free_work);
> + struct drm_mode_config *config = &conn->dev->mode_config;
> +
> + lockdep_assert_held(&config->connector_list_lock);
> +
> + if (!refcount_dec_and_test(&conn->base.refcount.refcount))
> + return;
> +
> + llist_add(&conn->free_node, &config->connector_free_list);
> + schedule_work(&config->connector_free_work);
> }
>
> /**
> @@ -585,10 +599,10 @@ drm_connector_list_iter_next(struct drm_connector_list_iter *iter)
>
> /* loop until it's not a zombie connector */
> } while (!kref_get_unless_zero(&iter->conn->base.refcount));
> - spin_unlock_irqrestore(&config->connector_list_lock, flags);
>
> if (old_conn)
> - drm_connector_put_safe(old_conn);
> + __drm_connector_put_safe(old_conn);
> + spin_unlock_irqrestore(&config->connector_list_lock, flags);
>
> return iter->conn;
> }
> @@ -605,9 +619,15 @@ EXPORT_SYMBOL(drm_connector_list_iter_next);
> */
> void drm_connector_list_iter_end(struct drm_connector_list_iter *iter)
> {
> + struct drm_mode_config *config = &iter->dev->mode_config;
> + unsigned long flags;
> +
> iter->dev = NULL;
> - if (iter->conn)
> - drm_connector_put_safe(iter->conn);
> + if (iter->conn) {
> + spin_lock_irqsave(&config->connector_list_lock, flags);
> + __drm_connector_put_safe(iter->conn);
> + spin_unlock_irqrestore(&config->connector_list_lock, flags);
> + }
> lock_release(&connector_list_iter_dep_map, 0, _RET_IP_);
> }
> EXPORT_SYMBOL(drm_connector_list_iter_end);
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> index 9ebb8841778c..af00f42ba269 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -142,6 +142,7 @@ int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
> uint64_t value);
> int drm_connector_create_standard_properties(struct drm_device *dev);
> const char *drm_get_connector_force_name(enum drm_connector_force force);
> +void drm_connector_free_work_fn(struct work_struct *work);
>
> /* IOCTL */
> int drm_mode_connector_property_set_ioctl(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index 6ffe952142e6..7681269abe41 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -382,6 +382,8 @@ void drm_mode_config_init(struct drm_device *dev)
> ida_init(&dev->mode_config.connector_ida);
> spin_lock_init(&dev->mode_config.connector_list_lock);
>
> + INIT_WORK(&dev->mode_config.connector_free_work, drm_connector_free_work_fn);
> +
> drm_mode_create_standard_properties(dev);
>
> /* Just to be sure */
> @@ -432,7 +434,7 @@ void drm_mode_config_cleanup(struct drm_device *dev)
> }
> drm_connector_list_iter_end(&conn_iter);
> /* connector_iter drops references in a work item. */
> - flush_scheduled_work();
> + flush_work(&dev->mode_config.connector_free_work);
> if (WARN_ON(!list_empty(&dev->mode_config.connector_list))) {
> drm_connector_list_iter_begin(dev, &conn_iter);
> drm_for_each_connector_iter(connector, &conn_iter)
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index ee3155391243..ed38df4ac204 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -24,6 +24,7 @@
> #define __DRM_CONNECTOR_H__
>
> #include <linux/list.h>
> +#include <linux/llist.h>
> #include <linux/ctype.h>
> #include <linux/hdmi.h>
> #include <drm/drm_mode_object.h>
> @@ -966,12 +967,13 @@ struct drm_connector {
> uint16_t tile_h_size, tile_v_size;
>
> /**
> - * @free_work:
> + * @free_node:
> *
> - * Work used only by &drm_connector_iter to be able to clean up a
> - * connector from any context.
> + * List used only by &drm_connector_iter to be able to clean up a
> + * connector from any context, in conjunction with
> + * &drm_mode_config.connector_free_work.
> */
> - struct work_struct free_work;
> + struct llist_node free_node;
> };
>
> #define obj_to_connector(x) container_of(x, struct drm_connector, base)
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index e5f3b43014e1..2cb6f02df64a 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -27,6 +27,7 @@
> #include <linux/types.h>
> #include <linux/idr.h>
> #include <linux/workqueue.h>
> +#include <linux/llist.h>
>
> #include <drm/drm_modeset_lock.h>
>
> @@ -402,7 +403,7 @@ struct drm_mode_config {
>
> /**
> * @connector_list_lock: Protects @num_connector and
> - * @connector_list.
> + * @connector_list and @connector_free_list.
> */
> spinlock_t connector_list_lock;
> /**
> @@ -422,6 +423,21 @@ struct drm_mode_config {
> * &struct drm_connector_list_iter to walk this list.
> */
> struct list_head connector_list;
> + /**
> + * @connector_free_list:
> + *
> + * List of connector objects linked with &drm_connector.free_head.
> + * Protected by @connector_list_lock. Used by
> + * drm_for_each_connector_iter() and
> + * &struct drm_connector_list_iter to savely free connectors using
> + * @connector_free_work.
> + */
> + struct llist_head connector_free_list;
> + /**
> + * @connector_free_work: Work to clean up @connector_free_list.
> + */
> + struct work_struct connector_free_work;
> +
> /**
> * @num_encoder:
> *
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm: rework delayed connector cleanup in connector_iter
2017-12-13 12:49 ` Daniel Vetter
@ 2017-12-13 13:05 ` Chris Wilson
-1 siblings, 0 replies; 17+ messages in thread
From: Chris Wilson @ 2017-12-13 13:05 UTC (permalink / raw)
To: Daniel Vetter, Intel Graphics Development
Cc: DRI Development, Daniel Vetter, Ben Widawsky, Dave Airlie,
Sean Paul, stable, Daniel Vetter, Jani Nikula, Gustavo Padovan,
David Airlie, Javier Martinez Canillas, Shuah Khan,
Guillaume Tucker, Mark Brown, Kevin Hilman, Matt Hart,
Thierry Escande, Tomeu Vizoso, Enric Balletbo i Serra
Quoting Daniel Vetter (2017-12-13 12:49:36)
> PROBE_DEFER also uses system_wq to reprobe drivers, which means when
> that again fails, and we try to flush the overall system_wq (to get
> all the delayed connectore cleanup work_struct completed), we
> deadlock.
>
> Fix this by using just a single cleanup work, so that we can only
> flush that one and don't block on anything else. That means a free
> list plus locking, a standard pattern.
>
> v2:
> - Correctly free connectors only on last ref. Oops (Chris).
> - use llist_head/node (Chris).
>
> Fixes: a703c55004e1 ("drm: safely free connectors from connector_iter")
> Fixes: 613051dac40d ("drm: locking&new iterators for connector_list")
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Dave Airlie <airlied@gmail.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: <stable@vger.kernel.org> # v4.11+: 613051dac40d ("drm: locking&new iterators for connector_list"
> Cc: <stable@vger.kernel.org> # v4.11+
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Javier Martinez Canillas <javier@dowhile0.org>
> Cc: Shuah Khan <shuahkh@osg.samsung.com>
> Cc: Guillaume Tucker <guillaume.tucker@collabora.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Kevin Hilman <khilman@baylibre.com>
> Cc: Matt Hart <matthew.hart@linaro.org>
> Cc: Thierry Escande <thierry.escande@collabora.co.uk>
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
> drivers/gpu/drm/drm_connector.c | 50 ++++++++++++++++++++++++++-----------
> drivers/gpu/drm/drm_crtc_internal.h | 1 +
> drivers/gpu/drm/drm_mode_config.c | 4 ++-
> include/drm/drm_connector.h | 10 +++++---
> include/drm/drm_mode_config.h | 18 ++++++++++++-
> 5 files changed, 62 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 0b7e0974e6da..3f53f127e1f2 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -153,14 +153,23 @@ static void drm_connector_free(struct kref *kref)
> connector->funcs->destroy(connector);
> }
>
> -static void drm_connector_free_work_fn(struct work_struct *work)
> +void drm_connector_free_work_fn(struct work_struct *work)
> {
> - struct drm_connector *connector =
> - container_of(work, struct drm_connector, free_work);
> - struct drm_device *dev = connector->dev;
> + struct drm_connector *connector, *n;
> + struct drm_device *dev =
> + container_of(work, struct drm_device, mode_config.connector_free_work);
> + struct drm_mode_config *config = &dev->mode_config;
> + unsigned long flags;
> + struct llist_node *freed;
>
> - drm_mode_object_unregister(dev, &connector->base);
> - connector->funcs->destroy(connector);
> + spin_lock_irqsave(&config->connector_list_lock, flags);
> + freed = llist_del_all(&config->connector_free_list);
> + spin_unlock_irqrestore(&config->connector_list_lock, flags);
My understanding is that the spinlock here is only used to guard the
free_list. (It's not protecting the final refcount.) In which case it is
redundant as llist_del_all/llist_add are a safe lockless combination.
That just makes the patch bigger than has to be, but it looks correct.
> +__drm_connector_put_safe(struct drm_connector *conn)
> {
> - if (refcount_dec_and_test(&conn->base.refcount.refcount))
> - schedule_work(&conn->free_work);
> + struct drm_mode_config *config = &conn->dev->mode_config;
> +
> + lockdep_assert_held(&config->connector_list_lock);
> +
> + if (!refcount_dec_and_test(&conn->base.refcount.refcount))
> + return;
> +
> + llist_add(&conn->free_node, &config->connector_free_list);
> + schedule_work(&config->connector_free_work);
(Didn't like the if (llist_add) nano-optimisation? :)
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> index 9ebb8841778c..af00f42ba269 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -142,6 +142,7 @@ int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
> uint64_t value);
> int drm_connector_create_standard_properties(struct drm_device *dev);
> const char *drm_get_connector_force_name(enum drm_connector_force force);
> +void drm_connector_free_work_fn(struct work_struct *work);
>
> /* IOCTL */
> int drm_mode_connector_property_set_ioctl(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index 6ffe952142e6..7681269abe41 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -382,6 +382,8 @@ void drm_mode_config_init(struct drm_device *dev)
> ida_init(&dev->mode_config.connector_ida);
> spin_lock_init(&dev->mode_config.connector_list_lock);
>
> + INIT_WORK(&dev->mode_config.connector_free_work, drm_connector_free_work_fn);
A init_llist_head(&dev->mode_config.connector_free_list) wouldn't go
amiss here. So perhaps push the connectors init into its own exported
function from drm_connector.c as opposed to exposing the free_fn.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm: rework delayed connector cleanup in connector_iter
@ 2017-12-13 13:05 ` Chris Wilson
0 siblings, 0 replies; 17+ messages in thread
From: Chris Wilson @ 2017-12-13 13:05 UTC (permalink / raw)
To: Intel Graphics Development
Cc: DRI Development, Daniel Vetter, Ben Widawsky, Dave Airlie,
Sean Paul, stable, Daniel Vetter, Jani Nikula, Gustavo Padovan,
David Airlie, Javier Martinez Canillas, Shuah Khan,
Guillaume Tucker, Mark Brown, Kevin Hilman, Matt Hart,
Thierry Escande, Tomeu Vizoso, Enric
Quoting Daniel Vetter (2017-12-13 12:49:36)
> PROBE_DEFER also uses system_wq to reprobe drivers, which means when
> that again fails, and we try to flush the overall system_wq (to get
> all the delayed connectore cleanup work_struct completed), we
> deadlock.
>
> Fix this by using just a single cleanup work, so that we can only
> flush that one and don't block on anything else. That means a free
> list plus locking, a standard pattern.
>
> v2:
> - Correctly free connectors only on last ref. Oops (Chris).
> - use llist_head/node (Chris).
>
> Fixes: a703c55004e1 ("drm: safely free connectors from connector_iter")
> Fixes: 613051dac40d ("drm: locking&new iterators for connector_list")
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Dave Airlie <airlied@gmail.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: <stable@vger.kernel.org> # v4.11+: 613051dac40d ("drm: locking&new iterators for connector_list"
> Cc: <stable@vger.kernel.org> # v4.11+
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Javier Martinez Canillas <javier@dowhile0.org>
> Cc: Shuah Khan <shuahkh@osg.samsung.com>
> Cc: Guillaume Tucker <guillaume.tucker@collabora.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Kevin Hilman <khilman@baylibre.com>
> Cc: Matt Hart <matthew.hart@linaro.org>
> Cc: Thierry Escande <thierry.escande@collabora.co.uk>
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
> drivers/gpu/drm/drm_connector.c | 50 ++++++++++++++++++++++++++-----------
> drivers/gpu/drm/drm_crtc_internal.h | 1 +
> drivers/gpu/drm/drm_mode_config.c | 4 ++-
> include/drm/drm_connector.h | 10 +++++---
> include/drm/drm_mode_config.h | 18 ++++++++++++-
> 5 files changed, 62 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 0b7e0974e6da..3f53f127e1f2 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -153,14 +153,23 @@ static void drm_connector_free(struct kref *kref)
> connector->funcs->destroy(connector);
> }
>
> -static void drm_connector_free_work_fn(struct work_struct *work)
> +void drm_connector_free_work_fn(struct work_struct *work)
> {
> - struct drm_connector *connector =
> - container_of(work, struct drm_connector, free_work);
> - struct drm_device *dev = connector->dev;
> + struct drm_connector *connector, *n;
> + struct drm_device *dev =
> + container_of(work, struct drm_device, mode_config.connector_free_work);
> + struct drm_mode_config *config = &dev->mode_config;
> + unsigned long flags;
> + struct llist_node *freed;
>
> - drm_mode_object_unregister(dev, &connector->base);
> - connector->funcs->destroy(connector);
> + spin_lock_irqsave(&config->connector_list_lock, flags);
> + freed = llist_del_all(&config->connector_free_list);
> + spin_unlock_irqrestore(&config->connector_list_lock, flags);
My understanding is that the spinlock here is only used to guard the
free_list. (It's not protecting the final refcount.) In which case it is
redundant as llist_del_all/llist_add are a safe lockless combination.
That just makes the patch bigger than has to be, but it looks correct.
> +__drm_connector_put_safe(struct drm_connector *conn)
> {
> - if (refcount_dec_and_test(&conn->base.refcount.refcount))
> - schedule_work(&conn->free_work);
> + struct drm_mode_config *config = &conn->dev->mode_config;
> +
> + lockdep_assert_held(&config->connector_list_lock);
> +
> + if (!refcount_dec_and_test(&conn->base.refcount.refcount))
> + return;
> +
> + llist_add(&conn->free_node, &config->connector_free_list);
> + schedule_work(&config->connector_free_work);
(Didn't like the if (llist_add) nano-optimisation? :)
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> index 9ebb8841778c..af00f42ba269 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -142,6 +142,7 @@ int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
> uint64_t value);
> int drm_connector_create_standard_properties(struct drm_device *dev);
> const char *drm_get_connector_force_name(enum drm_connector_force force);
> +void drm_connector_free_work_fn(struct work_struct *work);
>
> /* IOCTL */
> int drm_mode_connector_property_set_ioctl(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index 6ffe952142e6..7681269abe41 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -382,6 +382,8 @@ void drm_mode_config_init(struct drm_device *dev)
> ida_init(&dev->mode_config.connector_ida);
> spin_lock_init(&dev->mode_config.connector_list_lock);
>
> + INIT_WORK(&dev->mode_config.connector_free_work, drm_connector_free_work_fn);
A init_llist_head(&dev->mode_config.connector_free_list) wouldn't go
amiss here. So perhaps push the connectors init into its own exported
function from drm_connector.c as opposed to exposing the free_fn.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm: rework delayed connector cleanup in connector_iter
2017-12-13 13:05 ` Chris Wilson
@ 2017-12-13 13:35 ` Daniel Vetter
-1 siblings, 0 replies; 17+ messages in thread
From: Daniel Vetter @ 2017-12-13 13:35 UTC (permalink / raw)
To: Chris Wilson
Cc: Daniel Vetter, Intel Graphics Development, DRI Development,
Ben Widawsky, Dave Airlie, Sean Paul, stable, Daniel Vetter,
Jani Nikula, Gustavo Padovan, David Airlie,
Javier Martinez Canillas, Shuah Khan, Guillaume Tucker,
Mark Brown, Kevin Hilman, Matt Hart, Thierry Escande,
Tomeu Vizoso, Enric Balletbo i Serra
On Wed, Dec 13, 2017 at 01:05:49PM +0000, Chris Wilson wrote:
> Quoting Daniel Vetter (2017-12-13 12:49:36)
> > PROBE_DEFER also uses system_wq to reprobe drivers, which means when
> > that again fails, and we try to flush the overall system_wq (to get
> > all the delayed connectore cleanup work_struct completed), we
> > deadlock.
> >
> > Fix this by using just a single cleanup work, so that we can only
> > flush that one and don't block on anything else. That means a free
> > list plus locking, a standard pattern.
> >
> > v2:
> > - Correctly free connectors only on last ref. Oops (Chris).
> > - use llist_head/node (Chris).
> >
> > Fixes: a703c55004e1 ("drm: safely free connectors from connector_iter")
> > Fixes: 613051dac40d ("drm: locking&new iterators for connector_list")
> > Cc: Ben Widawsky <ben@bwidawsk.net>
> > Cc: Dave Airlie <airlied@gmail.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Sean Paul <seanpaul@chromium.org>
> > Cc: <stable@vger.kernel.org> # v4.11+: 613051dac40d ("drm: locking&new iterators for connector_list"
> > Cc: <stable@vger.kernel.org> # v4.11+
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Gustavo Padovan <gustavo@padovan.org>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Javier Martinez Canillas <javier@dowhile0.org>
> > Cc: Shuah Khan <shuahkh@osg.samsung.com>
> > Cc: Guillaume Tucker <guillaume.tucker@collabora.com>
> > Cc: Mark Brown <broonie@kernel.org>
> > Cc: Kevin Hilman <khilman@baylibre.com>
> > Cc: Matt Hart <matthew.hart@linaro.org>
> > Cc: Thierry Escande <thierry.escande@collabora.co.uk>
> > Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> > Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> > drivers/gpu/drm/drm_connector.c | 50 ++++++++++++++++++++++++++-----------
> > drivers/gpu/drm/drm_crtc_internal.h | 1 +
> > drivers/gpu/drm/drm_mode_config.c | 4 ++-
> > include/drm/drm_connector.h | 10 +++++---
> > include/drm/drm_mode_config.h | 18 ++++++++++++-
> > 5 files changed, 62 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > index 0b7e0974e6da..3f53f127e1f2 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -153,14 +153,23 @@ static void drm_connector_free(struct kref *kref)
> > connector->funcs->destroy(connector);
> > }
> >
> > -static void drm_connector_free_work_fn(struct work_struct *work)
> > +void drm_connector_free_work_fn(struct work_struct *work)
> > {
> > - struct drm_connector *connector =
> > - container_of(work, struct drm_connector, free_work);
> > - struct drm_device *dev = connector->dev;
> > + struct drm_connector *connector, *n;
> > + struct drm_device *dev =
> > + container_of(work, struct drm_device, mode_config.connector_free_work);
> > + struct drm_mode_config *config = &dev->mode_config;
> > + unsigned long flags;
> > + struct llist_node *freed;
> >
> > - drm_mode_object_unregister(dev, &connector->base);
> > - connector->funcs->destroy(connector);
> > + spin_lock_irqsave(&config->connector_list_lock, flags);
> > + freed = llist_del_all(&config->connector_free_list);
> > + spin_unlock_irqrestore(&config->connector_list_lock, flags);
>
> My understanding is that the spinlock here is only used to guard the
> free_list. (It's not protecting the final refcount.) In which case it is
> redundant as llist_del_all/llist_add are a safe lockless combination.
>
> That just makes the patch bigger than has to be, but it looks correct.
>
> > +__drm_connector_put_safe(struct drm_connector *conn)
> > {
> > - if (refcount_dec_and_test(&conn->base.refcount.refcount))
> > - schedule_work(&conn->free_work);
> > + struct drm_mode_config *config = &conn->dev->mode_config;
> > +
> > + lockdep_assert_held(&config->connector_list_lock);
> > +
> > + if (!refcount_dec_and_test(&conn->base.refcount.refcount))
> > + return;
> > +
> > + llist_add(&conn->free_node, &config->connector_free_list);
> > + schedule_work(&config->connector_free_work);
>
> (Didn't like the if (llist_add) nano-optimisation? :)
I thought that one might race, since the schedule_work is what provides the
crucial barrier here. But then I was kinda too lazy to read all the llist
guarantees already and just figured I'll keep the spin_lock stuck around
everything.
But yeah it's all neatly lockless, now I'm tempted to redo it all. If CI
spots something I'll include it in the respin for sure.
> > diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> > index 9ebb8841778c..af00f42ba269 100644
> > --- a/drivers/gpu/drm/drm_crtc_internal.h
> > +++ b/drivers/gpu/drm/drm_crtc_internal.h
> > @@ -142,6 +142,7 @@ int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
> > uint64_t value);
> > int drm_connector_create_standard_properties(struct drm_device *dev);
> > const char *drm_get_connector_force_name(enum drm_connector_force force);
> > +void drm_connector_free_work_fn(struct work_struct *work);
> >
> > /* IOCTL */
> > int drm_mode_connector_property_set_ioctl(struct drm_device *dev,
> > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > index 6ffe952142e6..7681269abe41 100644
> > --- a/drivers/gpu/drm/drm_mode_config.c
> > +++ b/drivers/gpu/drm/drm_mode_config.c
> > @@ -382,6 +382,8 @@ void drm_mode_config_init(struct drm_device *dev)
> > ida_init(&dev->mode_config.connector_ida);
> > spin_lock_init(&dev->mode_config.connector_list_lock);
> >
> > + INIT_WORK(&dev->mode_config.connector_free_work, drm_connector_free_work_fn);
>
> A init_llist_head(&dev->mode_config.connector_free_list) wouldn't go
> amiss here. So perhaps push the connectors init into its own exported
> function from drm_connector.c as opposed to exposing the free_fn.
Imo it doesn't matter much how we go about drm.ko internals. But I'll
stick the init_llist_head in there when applying, somehow I dind't find it
(why is every kernel data type slightly different in this).
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm: rework delayed connector cleanup in connector_iter
@ 2017-12-13 13:35 ` Daniel Vetter
0 siblings, 0 replies; 17+ messages in thread
From: Daniel Vetter @ 2017-12-13 13:35 UTC (permalink / raw)
To: Chris Wilson
Cc: Daniel Vetter, Intel Graphics Development, DRI Development,
Ben Widawsky, Dave Airlie, Sean Paul, stable, Daniel Vetter,
Jani Nikula, Gustavo Padovan, David Airlie,
Javier Martinez Canillas, Shuah Khan, Guillaume Tucker,
Mark Brown, Kevin Hilman, Matt Hart, Thierry Escande,
Tomeu Vizoso
On Wed, Dec 13, 2017 at 01:05:49PM +0000, Chris Wilson wrote:
> Quoting Daniel Vetter (2017-12-13 12:49:36)
> > PROBE_DEFER also uses system_wq to reprobe drivers, which means when
> > that again fails, and we try to flush the overall system_wq (to get
> > all the delayed connectore cleanup work_struct completed), we
> > deadlock.
> >
> > Fix this by using just a single cleanup work, so that we can only
> > flush that one and don't block on anything else. That means a free
> > list plus locking, a standard pattern.
> >
> > v2:
> > - Correctly free connectors only on last ref. Oops (Chris).
> > - use llist_head/node (Chris).
> >
> > Fixes: a703c55004e1 ("drm: safely free connectors from connector_iter")
> > Fixes: 613051dac40d ("drm: locking&new iterators for connector_list")
> > Cc: Ben Widawsky <ben@bwidawsk.net>
> > Cc: Dave Airlie <airlied@gmail.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Sean Paul <seanpaul@chromium.org>
> > Cc: <stable@vger.kernel.org> # v4.11+: 613051dac40d ("drm: locking&new iterators for connector_list"
> > Cc: <stable@vger.kernel.org> # v4.11+
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Gustavo Padovan <gustavo@padovan.org>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Javier Martinez Canillas <javier@dowhile0.org>
> > Cc: Shuah Khan <shuahkh@osg.samsung.com>
> > Cc: Guillaume Tucker <guillaume.tucker@collabora.com>
> > Cc: Mark Brown <broonie@kernel.org>
> > Cc: Kevin Hilman <khilman@baylibre.com>
> > Cc: Matt Hart <matthew.hart@linaro.org>
> > Cc: Thierry Escande <thierry.escande@collabora.co.uk>
> > Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> > Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> > drivers/gpu/drm/drm_connector.c | 50 ++++++++++++++++++++++++++-----------
> > drivers/gpu/drm/drm_crtc_internal.h | 1 +
> > drivers/gpu/drm/drm_mode_config.c | 4 ++-
> > include/drm/drm_connector.h | 10 +++++---
> > include/drm/drm_mode_config.h | 18 ++++++++++++-
> > 5 files changed, 62 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > index 0b7e0974e6da..3f53f127e1f2 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -153,14 +153,23 @@ static void drm_connector_free(struct kref *kref)
> > connector->funcs->destroy(connector);
> > }
> >
> > -static void drm_connector_free_work_fn(struct work_struct *work)
> > +void drm_connector_free_work_fn(struct work_struct *work)
> > {
> > - struct drm_connector *connector =
> > - container_of(work, struct drm_connector, free_work);
> > - struct drm_device *dev = connector->dev;
> > + struct drm_connector *connector, *n;
> > + struct drm_device *dev =
> > + container_of(work, struct drm_device, mode_config.connector_free_work);
> > + struct drm_mode_config *config = &dev->mode_config;
> > + unsigned long flags;
> > + struct llist_node *freed;
> >
> > - drm_mode_object_unregister(dev, &connector->base);
> > - connector->funcs->destroy(connector);
> > + spin_lock_irqsave(&config->connector_list_lock, flags);
> > + freed = llist_del_all(&config->connector_free_list);
> > + spin_unlock_irqrestore(&config->connector_list_lock, flags);
>
> My understanding is that the spinlock here is only used to guard the
> free_list. (It's not protecting the final refcount.) In which case it is
> redundant as llist_del_all/llist_add are a safe lockless combination.
>
> That just makes the patch bigger than has to be, but it looks correct.
>
> > +__drm_connector_put_safe(struct drm_connector *conn)
> > {
> > - if (refcount_dec_and_test(&conn->base.refcount.refcount))
> > - schedule_work(&conn->free_work);
> > + struct drm_mode_config *config = &conn->dev->mode_config;
> > +
> > + lockdep_assert_held(&config->connector_list_lock);
> > +
> > + if (!refcount_dec_and_test(&conn->base.refcount.refcount))
> > + return;
> > +
> > + llist_add(&conn->free_node, &config->connector_free_list);
> > + schedule_work(&config->connector_free_work);
>
> (Didn't like the if (llist_add) nano-optimisation? :)
I thought that one might race, since the schedule_work is what provides the
crucial barrier here. But then I was kinda too lazy to read all the llist
guarantees already and just figured I'll keep the spin_lock stuck around
everything.
But yeah it's all neatly lockless, now I'm tempted to redo it all. If CI
spots something I'll include it in the respin for sure.
> > diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> > index 9ebb8841778c..af00f42ba269 100644
> > --- a/drivers/gpu/drm/drm_crtc_internal.h
> > +++ b/drivers/gpu/drm/drm_crtc_internal.h
> > @@ -142,6 +142,7 @@ int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
> > uint64_t value);
> > int drm_connector_create_standard_properties(struct drm_device *dev);
> > const char *drm_get_connector_force_name(enum drm_connector_force force);
> > +void drm_connector_free_work_fn(struct work_struct *work);
> >
> > /* IOCTL */
> > int drm_mode_connector_property_set_ioctl(struct drm_device *dev,
> > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > index 6ffe952142e6..7681269abe41 100644
> > --- a/drivers/gpu/drm/drm_mode_config.c
> > +++ b/drivers/gpu/drm/drm_mode_config.c
> > @@ -382,6 +382,8 @@ void drm_mode_config_init(struct drm_device *dev)
> > ida_init(&dev->mode_config.connector_ida);
> > spin_lock_init(&dev->mode_config.connector_list_lock);
> >
> > + INIT_WORK(&dev->mode_config.connector_free_work, drm_connector_free_work_fn);
>
> A init_llist_head(&dev->mode_config.connector_free_list) wouldn't go
> amiss here. So perhaps push the connectors init into its own exported
> function from drm_connector.c as opposed to exposing the free_fn.
Imo it doesn't matter much how we go about drm.ko internals. But I'll
stick the init_llist_head in there when applying, somehow I dind't find it
(why is every kernel data type slightly different in this).
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 17+ messages in thread
* ✓ Fi.CI.BAT: success for drm: rework delayed connector cleanup in connector_iter (rev2)
2017-12-13 10:45 ` Daniel Vetter
` (4 preceding siblings ...)
(?)
@ 2017-12-13 14:48 ` Patchwork
-1 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2017-12-13 14:48 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
== Series Details ==
Series: drm: rework delayed connector cleanup in connector_iter (rev2)
URL : https://patchwork.freedesktop.org/series/35282/
State : success
== Summary ==
Series 35282v2 drm: rework delayed connector cleanup in connector_iter
https://patchwork.freedesktop.org/api/1.0/series/35282/revisions/2/mbox/
Test debugfs_test:
Subgroup read_all_entries:
pass -> DMESG-WARN (fi-elk-e7500) fdo#103989 +2
Test gem_exec_suspend:
Subgroup basic-s4-devices:
dmesg-warn -> PASS (fi-kbl-7500u) fdo#104062
fdo#103989 https://bugs.freedesktop.org/show_bug.cgi?id=103989
fdo#104062 https://bugs.freedesktop.org/show_bug.cgi?id=104062
fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:439s
fi-bdw-gvtdvm total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:441s
fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:381s
fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:508s
fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:278s
fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:505s
fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:513s
fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:484s
fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:469s
fi-elk-e7500 total:224 pass:164 dwarn:14 dfail:0 fail:0 skip:45
fi-gdg-551 total:288 pass:178 dwarn:1 dfail:0 fail:1 skip:108 time:266s
fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:407s
fi-hsw-4770r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:419s
fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:401s
fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:479s
fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:434s
fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:486s
fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:529s
fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:470s
fi-kbl-r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:532s
fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:592s
fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:452s
fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:537s
fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:563s
fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:504s
fi-snb-2520m total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:548s
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:414s
Blacklisted hosts:
fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:590s
fi-cnl-y total:216 pass:195 dwarn:0 dfail:0 fail:0 skip:20
fi-glk-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:487s
fi-glk-1 failed to collect. IGT log at Patchwork_7483/fi-glk-1/igt.log
fi-skl-gvtdvm failed to collect. IGT log at Patchwork_7483/fi-skl-gvtdvm/igt.log
f97f7de6c725c07feea8de61aadc1d0574301bdb drm-tip: 2017y-12m-13d-13h-46m-37s UTC integration manifest
b2a87a543978 drm: rework delayed connector cleanup in connector_iter
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7483/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread
* ✗ Fi.CI.IGT: warning for drm: rework delayed connector cleanup in connector_iter (rev2)
2017-12-13 10:45 ` Daniel Vetter
` (5 preceding siblings ...)
(?)
@ 2017-12-13 17:30 ` Patchwork
-1 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2017-12-13 17:30 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
== Series Details ==
Series: drm: rework delayed connector cleanup in connector_iter (rev2)
URL : https://patchwork.freedesktop.org/series/35282/
State : warning
== Summary ==
Warning: bzip CI_DRM_3507/shard-glkb6/results33.json.bz2 wasn't in correct JSON format
Test kms_setmode:
Subgroup basic:
pass -> FAIL (shard-hsw) fdo#99912
Test kms_cursor_crc:
Subgroup cursor-256x256-suspend:
pass -> SKIP (shard-snb) fdo#103375 +2
Subgroup cursor-256x256-offscreen:
pass -> SKIP (shard-hsw)
Test kms_frontbuffer_tracking:
Subgroup fbc-1p-offscren-pri-shrfb-draw-blt:
pass -> FAIL (shard-snb) fdo#101623 +1
Subgroup fbc-1p-primscrn-pri-shrfb-draw-render:
pass -> FAIL (shard-hsw) fdo#103167
Test kms_flip:
Subgroup blt-wf_vblank-vs-dpms:
pass -> SKIP (shard-hsw) fdo#102614
Subgroup flip-vs-dpms-interruptible:
pass -> SKIP (shard-hsw)
Test kms_pipe_crc_basic:
Subgroup hang-read-crc-pipe-a:
pass -> SKIP (shard-hsw)
Test kms_busy:
Subgroup extended-modeset-hang-oldfb-render-b:
pass -> SKIP (shard-hsw)
Test kms_draw_crc:
Subgroup draw-method-xrgb2101010-blt-xtiled:
pass -> SKIP (shard-hsw)
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
shard-hsw total:2712 pass:1529 dwarn:1 dfail:0 fail:11 skip:1171 time:9152s
shard-snb total:2636 pass:1277 dwarn:1 dfail:0 fail:13 skip:1345 time:7954s
Blacklisted hosts:
shard-apl total:2694 pass:1675 dwarn:1 dfail:0 fail:21 skip:996 time:13516s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7483/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm: rework delayed connector cleanup in connector_iter
2017-12-13 13:35 ` Daniel Vetter
@ 2017-12-13 21:59 ` Daniel Vetter
-1 siblings, 0 replies; 17+ messages in thread
From: Daniel Vetter @ 2017-12-13 21:59 UTC (permalink / raw)
To: Chris Wilson
Cc: Daniel Vetter, Intel Graphics Development, DRI Development,
Ben Widawsky, Dave Airlie, Sean Paul, stable, Daniel Vetter,
Jani Nikula, Gustavo Padovan, David Airlie,
Javier Martinez Canillas, Shuah Khan, Guillaume Tucker,
Mark Brown, Kevin Hilman, Matt Hart, Thierry Escande,
Tomeu Vizoso, Enric Balletbo i Serra
On Wed, Dec 13, 2017 at 02:35:16PM +0100, Daniel Vetter wrote:
> On Wed, Dec 13, 2017 at 01:05:49PM +0000, Chris Wilson wrote:
> > Quoting Daniel Vetter (2017-12-13 12:49:36)
> > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > > index 6ffe952142e6..7681269abe41 100644
> > > --- a/drivers/gpu/drm/drm_mode_config.c
> > > +++ b/drivers/gpu/drm/drm_mode_config.c
> > > @@ -382,6 +382,8 @@ void drm_mode_config_init(struct drm_device *dev)
> > > ida_init(&dev->mode_config.connector_ida);
> > > spin_lock_init(&dev->mode_config.connector_list_lock);
> > >
> > > + INIT_WORK(&dev->mode_config.connector_free_work, drm_connector_free_work_fn);
> >
> > A init_llist_head(&dev->mode_config.connector_free_list) wouldn't go
> > amiss here. So perhaps push the connectors init into its own exported
> > function from drm_connector.c as opposed to exposing the free_fn.
>
> Imo it doesn't matter much how we go about drm.ko internals. But I'll
> stick the init_llist_head in there when applying, somehow I dind't find it
> (why is every kernel data type slightly different in this).
>
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
And applied with init_llist_head added.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm: rework delayed connector cleanup in connector_iter
@ 2017-12-13 21:59 ` Daniel Vetter
0 siblings, 0 replies; 17+ messages in thread
From: Daniel Vetter @ 2017-12-13 21:59 UTC (permalink / raw)
To: Chris Wilson
Cc: Daniel Vetter, Intel Graphics Development, DRI Development,
Ben Widawsky, Dave Airlie, Sean Paul, stable, Daniel Vetter,
Jani Nikula, Gustavo Padovan, David Airlie,
Javier Martinez Canillas, Shuah Khan, Guillaume Tucker,
Mark Brown, Kevin Hilman, Matt Hart, Thierry Escande,
Tomeu Vizoso
On Wed, Dec 13, 2017 at 02:35:16PM +0100, Daniel Vetter wrote:
> On Wed, Dec 13, 2017 at 01:05:49PM +0000, Chris Wilson wrote:
> > Quoting Daniel Vetter (2017-12-13 12:49:36)
> > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > > index 6ffe952142e6..7681269abe41 100644
> > > --- a/drivers/gpu/drm/drm_mode_config.c
> > > +++ b/drivers/gpu/drm/drm_mode_config.c
> > > @@ -382,6 +382,8 @@ void drm_mode_config_init(struct drm_device *dev)
> > > ida_init(&dev->mode_config.connector_ida);
> > > spin_lock_init(&dev->mode_config.connector_list_lock);
> > >
> > > + INIT_WORK(&dev->mode_config.connector_free_work, drm_connector_free_work_fn);
> >
> > A init_llist_head(&dev->mode_config.connector_free_list) wouldn't go
> > amiss here. So perhaps push the connectors init into its own exported
> > function from drm_connector.c as opposed to exposing the free_fn.
>
> Imo it doesn't matter much how we go about drm.ko internals. But I'll
> stick the init_llist_head in there when applying, somehow I dind't find it
> (why is every kernel data type slightly different in this).
>
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
And applied with init_llist_head added.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2017-12-13 21:59 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20171213104607epcas4p420c1a7a83a19c1924318207c0bb2e8e2@epcas4p4.samsung.com>
2017-12-13 10:45 ` [PATCH] drm: rework delayed connector cleanup in connector_iter Daniel Vetter
2017-12-13 10:45 ` Daniel Vetter
2017-12-13 11:04 ` Chris Wilson
2017-12-13 11:04 ` Chris Wilson
2017-12-13 11:48 ` ✗ Fi.CI.BAT: failure for " Patchwork
2017-12-13 12:27 ` [PATCH] " Marek Szyprowski
2017-12-13 12:49 ` Daniel Vetter
2017-12-13 12:49 ` Daniel Vetter
2017-12-13 13:01 ` Marek Szyprowski
2017-12-13 13:05 ` Chris Wilson
2017-12-13 13:05 ` Chris Wilson
2017-12-13 13:35 ` Daniel Vetter
2017-12-13 13:35 ` Daniel Vetter
2017-12-13 21:59 ` Daniel Vetter
2017-12-13 21:59 ` Daniel Vetter
2017-12-13 14:48 ` ✓ Fi.CI.BAT: success for drm: rework delayed connector cleanup in connector_iter (rev2) Patchwork
2017-12-13 17:30 ` ✗ Fi.CI.IGT: warning " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.