thr3ads.net - Nouveau - [Nouveau] [PATCH 0/9] drm/nouveau: Cleanup event/handler design [Aug 2013]

If this information is useful, please help other people find it:
Share via:

Peter Hurley

2013-Aug-27 20:12 UTC

[Nouveau] [PATCH 0/9] drm/nouveau: Cleanup event/handler design

This series was originally motivated by a deadlock, introduced in
commit 1d7c71a3e2f77336df536855b0efd2dc5bdeb41b
'drm/nouveau/disp: port vblank handling to event interface',
due to inverted lock order between nouveau_drm_vblank_enable()
and nouveau_drm_vblank_handler() (the complete
lockdep report is included in the patch 4/5 changelog).

Because this series fixes the vblank event deadlock, it is
a competing solution to Maarten Lankhorst's 'drm/nouveau:
fix vblank deadlock'.

This series fixes the vblank event deadlock by converting the
event handler list to RCU. This solution allows the event trigger
to be lockless, and thus avoiding the lock inversion.

Typical hurdles to RCU conversion include: 1) ensuring the
object lifetime exceeds the lockless access; 2) preventing
premature object reuse; and 3) verifying that stale object
use not present logical errors.

Because object reuse is not safe in RCU-based operations,
the nouveau_event_get/_put interface is migrated from
add/remove semantics to enable/disable semantics with a separate
install/remove step (which also serves to document the
handler lifetime). This also corrects an unsafe interface design
where handlers can mistakenly be reused while in use.

The nouveau driver currently supports 4 events -- gpio, uevent, cevent,
and vblank. Every event is created by and stored within its
respective subdev/engine object -- gpio in the GPIO subdev, uevent
and cevent in the FIFO engine, and vblank in the DISP engine.
Thus event lifetime extends until the subdev is destructed during
devobj teardown.

Event handler lifetime varies and is detailed in the table below
(apologies for the wide-format):

Event  Handler function                   Container                     Until
-----  ----------------                   ---------------              
------------------
gpio   nouveau_connector_hotplug          struct nouveau_connector     
nouveau_connector_destroy
uevent nouveau_fence_wait_uevent_handler  local stack object           
nouveau_fence_wait_uevent returns
cevent none                               n/a                           n/a
vblank nouveau_drm_vblank_handler         struct nouveau_drm           
nouveau_drm_remove
vblank nv50_software_vblsem_release       struct nouveau_software_chan 
_nouveau_engctx_dtor
                                                                          (call
stack originates with
                                                                          
nouveau_abi16_chan_free ioctl)
vblank nvc0_software_vblsem_release       struct nouveau_software_chan  same as
above


RCU lifetime considerations for handlers:

Event  Handler                            Lifetime
-----  ----------------                   ---------------------------------
gpio   nouveau_connector_hotplug          kfree_rcu(nv_connector)
uevent nouveau_fence_wait_uevent_handler  explicit use of
nouveau_event_hander_create/_destroy
cevent none                               n/a
vblank nouveau_drm_vblank_handler         synchronize_rcu() in
nouveau_drm_unload
vblank nv50_software_vblsem_release       synchronize_rcu() in container dtor
vblank nvc0_software_vblsem_release       synchronize_rcu() in container dtor

synchronize_rcu() is used for nouveau_object-based containers for which
kfree_rcu() is not suitable/possible.

Stale event handler execution is not a concern for the existing handlers
because either: 1) the handler is responsible for disabling itself (via
returning NVKM_EVENT_DROP), or 2) the existing handler can already be stale,
as is the case with nouveau_connector_hotplug, which only schedules a work
item, and nouveau_drm_vblank_handler, which the drm core expects may be stale.


Peter Hurley (9):
  drm/nouveau: Add priv field for event handlers
  drm/nouveau: Move event index check from critical section
  drm/nouveau: Allocate local event handlers
  drm/nouveau: Allow asymmetric nouveau_event_get/_put
  drm/nouveau: Add install/remove semantics for event handlers
  drm/nouveau: Convert event handler list to RCU
  drm/nouveau: Fold nouveau_event_put_locked into caller
  drm/nouveau: Simplify event interface
  drm/nouveau: Simplify event handler interface

 drivers/gpu/drm/nouveau/core/core/event.c          | 121 +++++++++++++++++----
 .../gpu/drm/nouveau/core/engine/software/nv50.c    |  32 ++++--
 .../gpu/drm/nouveau/core/engine/software/nvc0.c    |  32 ++++--
 drivers/gpu/drm/nouveau/core/include/core/event.h  |  27 ++++-
 .../gpu/drm/nouveau/core/include/engine/software.h |   2 +-
 drivers/gpu/drm/nouveau/nouveau_connector.c        |  16 ++-
 drivers/gpu/drm/nouveau/nouveau_display.c          |  16 +--
 drivers/gpu/drm/nouveau/nouveau_drm.c              |  35 +++---
 drivers/gpu/drm/nouveau/nouveau_fence.c            |  27 ++---
 9 files changed, 220 insertions(+), 88 deletions(-)

-- 
1.8.1.2

Peter Hurley

2013-Aug-27 20:12 UTC

head link

[Nouveau] [PATCH 1/9] drm/nouveau: Add priv field for event handlers

Provide private field for event handlers exclusive use.
Convert nouveau_fence_wait_uevent() and
nouveau_fence_wait_uevent_handler(); drop struct nouveau_fence_uevent.

Signed-off-by: Peter Hurley <peter at hurleysoftware.com>
---
 drivers/gpu/drm/nouveau/core/include/core/event.h |  1 +
 drivers/gpu/drm/nouveau/nouveau_fence.c           | 20 +++++++-------------
 2 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/core/include/core/event.h
b/drivers/gpu/drm/nouveau/core/include/core/event.h
index 9e09440..ad4d8c2 100644
--- a/drivers/gpu/drm/nouveau/core/include/core/event.h
+++ b/drivers/gpu/drm/nouveau/core/include/core/event.h
@@ -7,6 +7,7 @@
 
 struct nouveau_eventh {
 	struct list_head head;
+	void *priv;
 	int (*func)(struct nouveau_eventh *, int index);
 };
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c
b/drivers/gpu/drm/nouveau/nouveau_fence.c
index be31499..c2e3167 100644
--- a/drivers/gpu/drm/nouveau/nouveau_fence.c
+++ b/drivers/gpu/drm/nouveau/nouveau_fence.c
@@ -165,17 +165,11 @@ nouveau_fence_done(struct nouveau_fence *fence)
 	return !fence->channel;
 }
 
-struct nouveau_fence_uevent {
-	struct nouveau_eventh handler;
-	struct nouveau_fence_priv *priv;
-};
-
 static int
-nouveau_fence_wait_uevent_handler(struct nouveau_eventh *event, int index)
+nouveau_fence_wait_uevent_handler(struct nouveau_eventh *handler, int index)
 {
-	struct nouveau_fence_uevent *uevent -		container_of(event, struct
nouveau_fence_uevent, handler);
-	wake_up_all(&uevent->priv->waiting);
+	struct nouveau_fence_priv *priv = handler->priv;
+	wake_up_all(&priv->waiting);
 	return NVKM_EVENT_KEEP;
 }
 
@@ -186,13 +180,13 @@ nouveau_fence_wait_uevent(struct nouveau_fence *fence,
bool intr)
 	struct nouveau_channel *chan = fence->channel;
 	struct nouveau_fifo *pfifo = nouveau_fifo(chan->drm->device);
 	struct nouveau_fence_priv *priv = chan->drm->fence;
-	struct nouveau_fence_uevent uevent = {
-		.handler.func = nouveau_fence_wait_uevent_handler,
+	struct nouveau_eventh handler = {
+		.func = nouveau_fence_wait_uevent_handler,
 		.priv = priv,
 	};
 	int ret = 0;
 
-	nouveau_event_get(pfifo->uevent, 0, &uevent.handler);
+	nouveau_event_get(pfifo->uevent, 0, &handler);
 
 	if (fence->timeout) {
 		unsigned long timeout = fence->timeout - jiffies;
@@ -224,7 +218,7 @@ nouveau_fence_wait_uevent(struct nouveau_fence *fence, bool
intr)
 		}
 	}
 
-	nouveau_event_put(pfifo->uevent, 0, &uevent.handler);
+	nouveau_event_put(pfifo->uevent, 0, &handler);
 	if (unlikely(ret < 0))
 		return ret;
 
-- 
1.8.1.2

Peter Hurley

2013-Aug-27 20:12 UTC

head link

[Nouveau] [PATCH 2/9] drm/nouveau: Move event index check from critical section

The index_nr field is constant for the lifetime of the event, so
serialized access is unnecessary.

Signed-off-by: Peter Hurley <peter at hurleysoftware.com>
---
 drivers/gpu/drm/nouveau/core/core/event.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/core/core/event.c
b/drivers/gpu/drm/nouveau/core/core/event.c
index 7eb81c1..e69c463 100644
--- a/drivers/gpu/drm/nouveau/core/core/event.c
+++ b/drivers/gpu/drm/nouveau/core/core/event.c
@@ -40,9 +40,11 @@ nouveau_event_put(struct nouveau_event *event, int index,
 {
 	unsigned long flags;
 
+	if (index >= event->index_nr)
+		return;
+
 	spin_lock_irqsave(&event->lock, flags);
-	if (index < event->index_nr)
-		nouveau_event_put_locked(event, index, handler);
+	nouveau_event_put_locked(event, index, handler);
 	spin_unlock_irqrestore(&event->lock, flags);
 }
 
@@ -52,13 +54,14 @@ nouveau_event_get(struct nouveau_event *event, int index,
 {
 	unsigned long flags;
 
+	if (index >= event->index_nr)
+		return;
+
 	spin_lock_irqsave(&event->lock, flags);
-	if (index < event->index_nr) {
-		list_add(&handler->head, &event->index[index].list);
-		if (!event->index[index].refs++) {
-			if (event->enable)
-				event->enable(event, index);
-		}
+	list_add(&handler->head, &event->index[index].list);
+	if (!event->index[index].refs++) {
+		if (event->enable)
+			event->enable(event, index);
 	}
 	spin_unlock_irqrestore(&event->lock, flags);
 }
-- 
1.8.1.2

Peter Hurley

2013-Aug-27 20:12 UTC

head link

[Nouveau] [PATCH 3/9] drm/nouveau: Allocate local event handlers

Prepare for transition to RCU-based event handler list;
since RCU list traversal may have stale pointers, local
storage may go out of scope before handler completes.

Introduce nouveau_event_handler_create/_destroy which provides
suitable semantics for multiple, temporary event handlers.

Signed-off-by: Peter Hurley <peter at hurleysoftware.com>
---
 drivers/gpu/drm/nouveau/core/core/event.c         | 24 +++++++++++++++++++++++
 drivers/gpu/drm/nouveau/core/include/core/event.h |  6 ++++++
 drivers/gpu/drm/nouveau/nouveau_fence.c           | 15 +++++++-------
 3 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/core/core/event.c
b/drivers/gpu/drm/nouveau/core/core/event.c
index e69c463..1a8d685 100644
--- a/drivers/gpu/drm/nouveau/core/core/event.c
+++ b/drivers/gpu/drm/nouveau/core/core/event.c
@@ -23,6 +23,30 @@
 #include <core/os.h>
 #include <core/event.h>
 
+int
+nouveau_event_handler_create(struct nouveau_event *event, int index,
+			     int (*func)(struct nouveau_eventh*, int),
+			     void *priv, struct nouveau_eventh **phandler)
+{
+	struct nouveau_eventh *handler;
+
+	handler = *phandler = kzalloc(sizeof(*handler), GFP_KERNEL);
+	if (!handler)
+		return -ENOMEM;
+	handler->func = func;
+	handler->priv = priv;
+	nouveau_event_get(event, index, handler);
+	return 0;
+}
+
+void
+nouveau_event_handler_destroy(struct nouveau_event *event, int index,
+			      struct nouveau_eventh *handler)
+{
+	nouveau_event_put(event, index, handler);
+	kfree(handler);
+}
+
 static void
 nouveau_event_put_locked(struct nouveau_event *event, int index,
 			 struct nouveau_eventh *handler)
diff --git a/drivers/gpu/drm/nouveau/core/include/core/event.h
b/drivers/gpu/drm/nouveau/core/include/core/event.h
index ad4d8c2..bdf1a0a 100644
--- a/drivers/gpu/drm/nouveau/core/include/core/event.h
+++ b/drivers/gpu/drm/nouveau/core/include/core/event.h
@@ -34,4 +34,10 @@ void nouveau_event_get(struct nouveau_event *, int index,
 void nouveau_event_put(struct nouveau_event *, int index,
 		       struct nouveau_eventh *);
 
+int nouveau_event_handler_create(struct nouveau_event *, int index,
+				 int (*func)(struct nouveau_eventh*, int),
+				 void *priv, struct nouveau_eventh **);
+void nouveau_event_handler_destroy(struct nouveau_event *, int index,
+				   struct nouveau_eventh *);
+
 #endif
diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c
b/drivers/gpu/drm/nouveau/nouveau_fence.c
index c2e3167..6dde483 100644
--- a/drivers/gpu/drm/nouveau/nouveau_fence.c
+++ b/drivers/gpu/drm/nouveau/nouveau_fence.c
@@ -180,13 +180,14 @@ nouveau_fence_wait_uevent(struct nouveau_fence *fence,
bool intr)
 	struct nouveau_channel *chan = fence->channel;
 	struct nouveau_fifo *pfifo = nouveau_fifo(chan->drm->device);
 	struct nouveau_fence_priv *priv = chan->drm->fence;
-	struct nouveau_eventh handler = {
-		.func = nouveau_fence_wait_uevent_handler,
-		.priv = priv,
-	};
-	int ret = 0;
+	struct nouveau_eventh *handler;
+	int ret;
 
-	nouveau_event_get(pfifo->uevent, 0, &handler);
+	ret = nouveau_event_handler_create(pfifo->uevent, 0,
+					   nouveau_fence_wait_uevent_handler,
+					   priv, &handler);
+	if (ret)
+		return ret;
 
 	if (fence->timeout) {
 		unsigned long timeout = fence->timeout - jiffies;
@@ -218,7 +219,7 @@ nouveau_fence_wait_uevent(struct nouveau_fence *fence, bool
intr)
 		}
 	}
 
-	nouveau_event_put(pfifo->uevent, 0, &handler);
+	nouveau_event_handler_destroy(pfifo->uevent, 0, handler);
 	if (unlikely(ret < 0))
 		return ret;
 
-- 
1.8.1.2

Peter Hurley

2013-Aug-27 20:12 UTC

head link

[Nouveau] [PATCH 4/9] drm/nouveau: Allow asymmetric nouveau_event_get/_put

Most nouveau event handlers have storage in 'static' containers
(structures with lifetimes nearly equivalent to the drm_device),
but are dangerously reused via nouveau_event_get/_put. For
example, if nouveau_event_get is called more than once for a
given handler, the event handler list will be corrupted.

Migrate nouveau_event_get/_put from add/remove semantics to
enable/disable semantics.

Signed-off-by: Peter Hurley <peter at hurleysoftware.com>
---
 drivers/gpu/drm/nouveau/core/core/event.c         | 20 ++++++++++++--------
 drivers/gpu/drm/nouveau/core/include/core/event.h |  4 ++++
 drivers/gpu/drm/nouveau/nouveau_drm.c             |  8 ++------
 3 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/core/core/event.c
b/drivers/gpu/drm/nouveau/core/core/event.c
index 1a8d685..0a65ede 100644
--- a/drivers/gpu/drm/nouveau/core/core/event.c
+++ b/drivers/gpu/drm/nouveau/core/core/event.c
@@ -51,11 +51,13 @@ static void
 nouveau_event_put_locked(struct nouveau_event *event, int index,
 			 struct nouveau_eventh *handler)
 {
-	if (!--event->index[index].refs) {
-		if (event->disable)
-			event->disable(event, index);
+	if (__test_and_clear_bit(NVKM_EVENT_ENABLE, &handler->flags)) {
+		if (!--event->index[index].refs) {
+			if (event->disable)
+				event->disable(event, index);
+		}
+		list_del(&handler->head);
 	}
-	list_del(&handler->head);
 }
 
 void
@@ -82,10 +84,12 @@ nouveau_event_get(struct nouveau_event *event, int index,
 		return;
 
 	spin_lock_irqsave(&event->lock, flags);
-	list_add(&handler->head, &event->index[index].list);
-	if (!event->index[index].refs++) {
-		if (event->enable)
-			event->enable(event, index);
+	if (!__test_and_set_bit(NVKM_EVENT_ENABLE, &handler->flags)) {
+		list_add(&handler->head, &event->index[index].list);
+		if (!event->index[index].refs++) {
+			if (event->enable)
+				event->enable(event, index);
+		}
 	}
 	spin_unlock_irqrestore(&event->lock, flags);
 }
diff --git a/drivers/gpu/drm/nouveau/core/include/core/event.h
b/drivers/gpu/drm/nouveau/core/include/core/event.h
index bdf1a0a..3e704d5 100644
--- a/drivers/gpu/drm/nouveau/core/include/core/event.h
+++ b/drivers/gpu/drm/nouveau/core/include/core/event.h
@@ -5,8 +5,12 @@
 #define NVKM_EVENT_DROP 0
 #define NVKM_EVENT_KEEP 1
 
+/* nouveau_eventh.flags bit #s */
+#define NVKM_EVENT_ENABLE 0
+
 struct nouveau_eventh {
 	struct list_head head;
+	unsigned long flags;
 	void *priv;
 	int (*func)(struct nouveau_eventh *, int index);
 };
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index b29d04b..ccea2c4 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -88,7 +88,6 @@ nouveau_drm_vblank_enable(struct drm_device *dev, int head)
 
 	if (WARN_ON_ONCE(head > ARRAY_SIZE(drm->vblank)))
 		return -EIO;
-	WARN_ON_ONCE(drm->vblank[head].func);
 	drm->vblank[head].func = nouveau_drm_vblank_handler;
 	nouveau_event_get(pdisp->vblank, head, &drm->vblank[head]);
 	return 0;
@@ -99,11 +98,8 @@ nouveau_drm_vblank_disable(struct drm_device *dev, int head)
 {
 	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nouveau_disp *pdisp = nouveau_disp(drm->device);
-	if (drm->vblank[head].func)
-		nouveau_event_put(pdisp->vblank, head, &drm->vblank[head]);
-	else
-		WARN_ON_ONCE(1);
-	drm->vblank[head].func = NULL;
+
+	nouveau_event_put(pdisp->vblank, head, &drm->vblank[head]);
 }
 
 static u64
-- 
1.8.1.2

Peter Hurley

2013-Aug-27 20:12 UTC

head link

[Nouveau] [PATCH 5/9] drm/nouveau: Add install/remove semantics for event handlers

Complete migration of nouveau_event_get/_put from add/remove
semantics to enable/disable semantics.

Introduce nouveau_event_handler_install/_remove interface to
add/remove non-local-scope event handlers (ie., those stored in
parent containers). This change in semantics makes explicit the
handler lifetime, and distinguishes "one-of" event handlers
(such as gpio) from "many temporary" event handlers (such as uevent).

Signed-off-by: Peter Hurley <peter at hurleysoftware.com>
---
 drivers/gpu/drm/nouveau/core/core/event.c          | 63 +++++++++++++++++++---
 .../gpu/drm/nouveau/core/engine/software/nv50.c    | 31 +++++++++--
 .../gpu/drm/nouveau/core/engine/software/nvc0.c    | 31 +++++++++--
 drivers/gpu/drm/nouveau/core/include/core/event.h  |  6 +++
 .../gpu/drm/nouveau/core/include/engine/software.h |  2 +-
 drivers/gpu/drm/nouveau/nouveau_connector.c        | 10 +++-
 drivers/gpu/drm/nouveau/nouveau_drm.c              | 17 +++++-
 7 files changed, 140 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/core/core/event.c
b/drivers/gpu/drm/nouveau/core/core/event.c
index 0a65ede..4cd1ebe 100644
--- a/drivers/gpu/drm/nouveau/core/core/event.c
+++ b/drivers/gpu/drm/nouveau/core/core/event.c
@@ -23,19 +23,60 @@
 #include <core/os.h>
 #include <core/event.h>
 
+void
+nouveau_event_handler_install(struct nouveau_event *event, int index,
+			      int (*func)(struct nouveau_eventh*, int),
+			      void *priv, struct nouveau_eventh *handler)
+{
+	unsigned long flags;
+
+	if (index >= event->index_nr)
+		return;
+
+	handler->func = func;
+	handler->priv = priv;
+
+	spin_lock_irqsave(&event->lock, flags);
+	list_add(&handler->head, &event->index[index].list);
+	spin_unlock_irqrestore(&event->lock, flags);
+}
+
+void
+nouveau_event_handler_remove(struct nouveau_event *event, int index,
+			     struct nouveau_eventh *handler)
+{
+	unsigned long flags;
+
+	if (index >= event->index_nr)
+		return;
+
+	spin_lock_irqsave(&event->lock, flags);
+	list_del(&handler->head);
+	spin_unlock_irqrestore(&event->lock, flags);
+}
+
 int
 nouveau_event_handler_create(struct nouveau_event *event, int index,
 			     int (*func)(struct nouveau_eventh*, int),
 			     void *priv, struct nouveau_eventh **phandler)
 {
 	struct nouveau_eventh *handler;
+	unsigned long flags;
 
 	handler = *phandler = kzalloc(sizeof(*handler), GFP_KERNEL);
 	if (!handler)
 		return -ENOMEM;
 	handler->func = func;
 	handler->priv = priv;
-	nouveau_event_get(event, index, handler);
+	__set_bit(NVKM_EVENT_ENABLE, &handler->flags);
+
+	spin_lock_irqsave(&event->lock, flags);
+	list_add(&handler->head, &event->index[index].list);
+	if (!event->index[index].refs++) {
+		if (event->enable)
+			event->enable(event, index);
+	}
+	spin_unlock_irqrestore(&event->lock, flags);
 	return 0;
 }
 
@@ -43,7 +84,18 @@ void
 nouveau_event_handler_destroy(struct nouveau_event *event, int index,
 			      struct nouveau_eventh *handler)
 {
-	nouveau_event_put(event, index, handler);
+	unsigned long flags;
+
+	if (index >= event->index_nr)
+		return;
+
+	spin_lock_irqsave(&event->lock, flags);
+	if (!--event->index[index].refs) {
+		if (event->disable)
+			event->disable(event, index);
+	}
+	list_del(&handler->head);
+	spin_unlock_irqrestore(&event->lock, flags);
 	kfree(handler);
 }
 
@@ -56,7 +108,6 @@ nouveau_event_put_locked(struct nouveau_event *event, int
index,
 			if (event->disable)
 				event->disable(event, index);
 		}
-		list_del(&handler->head);
 	}
 }
 
@@ -85,7 +136,6 @@ nouveau_event_get(struct nouveau_event *event, int index,
 
 	spin_lock_irqsave(&event->lock, flags);
 	if (!__test_and_set_bit(NVKM_EVENT_ENABLE, &handler->flags)) {
-		list_add(&handler->head, &event->index[index].list);
 		if (!event->index[index].refs++) {
 			if (event->enable)
 				event->enable(event, index);
@@ -105,8 +155,9 @@ nouveau_event_trigger(struct nouveau_event *event, int
index)
 
 	spin_lock_irqsave(&event->lock, flags);
 	list_for_each_entry_safe(handler, temp, &event->index[index].list,
head) {
-		if (handler->func(handler, index) == NVKM_EVENT_DROP) {
-			nouveau_event_put_locked(event, index, handler);
+		if (test_bit(NVKM_EVENT_ENABLE, &handler->flags)) {
+			if (handler->func(handler, index) == NVKM_EVENT_DROP)
+				nouveau_event_put_locked(event, index, handler);
 		}
 	}
 	spin_unlock_irqrestore(&event->lock, flags);
diff --git a/drivers/gpu/drm/nouveau/core/engine/software/nv50.c
b/drivers/gpu/drm/nouveau/core/engine/software/nv50.c
index c48e749..dfce1f5 100644
--- a/drivers/gpu/drm/nouveau/core/engine/software/nv50.c
+++ b/drivers/gpu/drm/nouveau/core/engine/software/nv50.c
@@ -97,7 +97,7 @@ nv50_software_mthd_vblsem_release(struct nouveau_object
*object, u32 mthd,
 	if (crtc > 1)
 		return -EINVAL;
 
-	nouveau_event_get(disp->vblank, crtc, &chan->base.vblank.event);
+	nouveau_event_get(disp->vblank, crtc,
&chan->base.vblank.event[crtc]);
 	return 0;
 }
 
@@ -135,7 +135,7 @@ static int
 nv50_software_vblsem_release(struct nouveau_eventh *event, int head)
 {
 	struct nouveau_software_chan *chan -		container_of(event, struct
nouveau_software_chan, vblank.event);
+		container_of(event, struct nouveau_software_chan, vblank.event[head]);
 	struct nv50_software_priv *priv = (void *)nv_object(chan)->engine;
 	struct nouveau_bar *bar = nouveau_bar(priv);
 
@@ -161,7 +161,8 @@ nv50_software_context_ctor(struct nouveau_object *parent,
 			   struct nouveau_object **pobject)
 {
 	struct nv50_software_chan *chan;
-	int ret;
+	struct nouveau_disp *disp = nouveau_disp(engine);
+	int ret, i;
 
 	ret = nouveau_software_context_create(parent, engine, oclass, &chan);
 	*pobject = nv_object(chan);
@@ -169,16 +170,36 @@ nv50_software_context_ctor(struct nouveau_object *parent,
 		return ret;
 
 	chan->base.vblank.channel = nv_gpuobj(parent->parent)->addr >>
12;
-	chan->base.vblank.event.func = nv50_software_vblsem_release;
+	for (i = 0; i < ARRAY_SIZE(chan->base.vblank.event); i++) {
+		nouveau_event_handler_install(disp->vblank, i,
+					      nv50_software_vblsem_release,
+					      NULL,
+					      &chan->base.vblank.event[i]);
+	}
 	return 0;
 }
 
+void
+nv50_software_context_dtor(struct nouveau_object *object)
+{
+	struct nv50_software_chan *chan = (void *)object;
+	struct nv50_software_priv *priv = (void *)nv_object(chan)->engine;
+	struct nouveau_disp *disp = nouveau_disp(priv);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(chan->base.vblank.event); i++) {
+		nouveau_event_handler_remove(disp->vblank, i,
+					     &chan->base.vblank.event[i]);
+	}
+	_nouveau_software_context_dtor(object);
+}
+
 static struct nouveau_oclass
 nv50_software_cclass = {
 	.handle = NV_ENGCTX(SW, 0x50),
 	.ofuncs = &(struct nouveau_ofuncs) {
 		.ctor = nv50_software_context_ctor,
-		.dtor = _nouveau_software_context_dtor,
+		.dtor = nv50_software_context_dtor,
 		.init = _nouveau_software_context_init,
 		.fini = _nouveau_software_context_fini,
 	},
diff --git a/drivers/gpu/drm/nouveau/core/engine/software/nvc0.c
b/drivers/gpu/drm/nouveau/core/engine/software/nvc0.c
index d698e71..d1f52c0 100644
--- a/drivers/gpu/drm/nouveau/core/engine/software/nvc0.c
+++ b/drivers/gpu/drm/nouveau/core/engine/software/nvc0.c
@@ -80,7 +80,7 @@ nvc0_software_mthd_vblsem_release(struct nouveau_object
*object, u32 mthd,
 	if ((nv_device(object)->card_type < NV_E0 && crtc > 1) ||
crtc > 3)
 		return -EINVAL;
 
-	nouveau_event_get(disp->vblank, crtc, &chan->base.vblank.event);
+	nouveau_event_get(disp->vblank, crtc,
&chan->base.vblank.event[crtc]);
 	return 0;
 }
 
@@ -147,7 +147,7 @@ static int
 nvc0_software_vblsem_release(struct nouveau_eventh *event, int head)
 {
 	struct nouveau_software_chan *chan -		container_of(event, struct
nouveau_software_chan, vblank.event);
+		container_of(event, struct nouveau_software_chan, vblank.event[head]);
 	struct nvc0_software_priv *priv = (void *)nv_object(chan)->engine;
 	struct nouveau_bar *bar = nouveau_bar(priv);
 
@@ -167,7 +167,8 @@ nvc0_software_context_ctor(struct nouveau_object *parent,
 			   struct nouveau_object **pobject)
 {
 	struct nvc0_software_chan *chan;
-	int ret;
+	struct nouveau_disp *disp = nouveau_disp(engine);
+	int ret, i;
 
 	ret = nouveau_software_context_create(parent, engine, oclass, &chan);
 	*pobject = nv_object(chan);
@@ -175,16 +176,36 @@ nvc0_software_context_ctor(struct nouveau_object *parent,
 		return ret;
 
 	chan->base.vblank.channel = nv_gpuobj(parent->parent)->addr >>
12;
-	chan->base.vblank.event.func = nvc0_software_vblsem_release;
+	for (i = 0; i < ARRAY_SIZE(chan->base.vblank.event); i++) {
+		nouveau_event_handler_install(disp->vblank, i,
+					      nvc0_software_vblsem_release,
+					      NULL,
+					      &chan->base.vblank.event[i]);
+	}
 	return 0;
 }
 
+void
+nvc0_software_context_dtor(struct nouveau_object *object)
+{
+	struct nvc0_software_chan *chan = (void *)object;
+	struct nvc0_software_priv *priv = (void *)nv_object(chan)->engine;
+	struct nouveau_disp *disp = nouveau_disp(priv);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(chan->base.vblank.event); i++) {
+		nouveau_event_handler_remove(disp->vblank, i,
+					     &chan->base.vblank.event[i]);
+	}
+	_nouveau_software_context_dtor(object);
+}
+
 static struct nouveau_oclass
 nvc0_software_cclass = {
 	.handle = NV_ENGCTX(SW, 0xc0),
 	.ofuncs = &(struct nouveau_ofuncs) {
 		.ctor = nvc0_software_context_ctor,
-		.dtor = _nouveau_software_context_dtor,
+		.dtor = nvc0_software_context_dtor,
 		.init = _nouveau_software_context_init,
 		.fini = _nouveau_software_context_fini,
 	},
diff --git a/drivers/gpu/drm/nouveau/core/include/core/event.h
b/drivers/gpu/drm/nouveau/core/include/core/event.h
index 3e704d5..45e8a0f 100644
--- a/drivers/gpu/drm/nouveau/core/include/core/event.h
+++ b/drivers/gpu/drm/nouveau/core/include/core/event.h
@@ -44,4 +44,10 @@ int nouveau_event_handler_create(struct nouveau_event *, int
index,
 void nouveau_event_handler_destroy(struct nouveau_event *, int index,
 				   struct nouveau_eventh *);
 
+void nouveau_event_handler_install(struct nouveau_event *, int index,
+				   int (*func)(struct nouveau_eventh*, int),
+				   void *priv, struct nouveau_eventh *);
+void nouveau_event_handler_remove(struct nouveau_event *, int index,
+				  struct nouveau_eventh *);
+
 #endif
diff --git a/drivers/gpu/drm/nouveau/core/include/engine/software.h
b/drivers/gpu/drm/nouveau/core/include/engine/software.h
index 4579948..bc26ea8 100644
--- a/drivers/gpu/drm/nouveau/core/include/engine/software.h
+++ b/drivers/gpu/drm/nouveau/core/include/engine/software.h
@@ -9,7 +9,7 @@ struct nouveau_software_chan {
 	struct nouveau_engctx base;
 
 	struct {
-		struct nouveau_eventh event;
+		struct nouveau_eventh event[4];
 		u32 channel;
 		u32 ctxdma;
 		u64 offset;
diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c
b/drivers/gpu/drm/nouveau/nouveau_connector.c
index 4da776f..13b38ed 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -98,6 +98,11 @@ static void
 nouveau_connector_destroy(struct drm_connector *connector)
 {
 	struct nouveau_connector *nv_connector = nouveau_connector(connector);
+	struct nouveau_drm *drm = nouveau_drm(connector->dev);
+	struct nouveau_gpio *gpio = nouveau_gpio(drm->device);
+
+	nouveau_event_handler_remove(gpio->events, nv_connector->hpd.line,
+				     &nv_connector->hpd_func);
 	kfree(nv_connector->edid);
 	drm_sysfs_connector_remove(connector);
 	drm_connector_cleanup(connector);
@@ -990,7 +995,10 @@ nouveau_connector_create(struct drm_device *dev, int index)
 
 		ret = gpio->find(gpio, 0, hpd[ffs((entry & 0x07033000) >> 12)],
 				 DCB_GPIO_UNUSED, &nv_connector->hpd);
-		nv_connector->hpd_func.func = nouveau_connector_hotplug;
+		nouveau_event_handler_install(gpio->events,
+					      nv_connector->hpd.line,
+					      nouveau_connector_hotplug, NULL,
+					      &nv_connector->hpd_func);
 		if (ret)
 			nv_connector->hpd.func = DCB_GPIO_UNUSED;
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index ccea2c4..4187cad 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -88,7 +88,6 @@ nouveau_drm_vblank_enable(struct drm_device *dev, int head)
 
 	if (WARN_ON_ONCE(head > ARRAY_SIZE(drm->vblank)))
 		return -EIO;
-	drm->vblank[head].func = nouveau_drm_vblank_handler;
 	nouveau_event_get(pdisp->vblank, head, &drm->vblank[head]);
 	return 0;
 }
@@ -298,7 +297,8 @@ nouveau_drm_load(struct drm_device *dev, unsigned long
flags)
 	struct pci_dev *pdev = dev->pdev;
 	struct nouveau_device *device;
 	struct nouveau_drm *drm;
-	int ret;
+	struct nouveau_disp *disp;
+	int ret, i;
 
 	ret = nouveau_cli_create(pdev, "DRM", sizeof(*drm),
(void**)&drm);
 	if (ret)
@@ -352,6 +352,13 @@ nouveau_drm_load(struct drm_device *dev, unsigned long
flags)
 	if (nv_device(drm->device)->chipset == 0xc1)
 		nv_mask(device, 0x00088080, 0x00000800, 0x00000000);
 
+	disp = nouveau_disp(device);
+	for (i = 0; i < ARRAY_SIZE(drm->vblank); i++) {
+		nouveau_event_handler_install(disp->vblank, i,
+					      nouveau_drm_vblank_handler,
+					      NULL, &drm->vblank[i]);
+	}
+
 	nouveau_vga_init(drm);
 	nouveau_agp_init(drm);
 
@@ -404,6 +411,8 @@ static int
 nouveau_drm_unload(struct drm_device *dev)
 {
 	struct nouveau_drm *drm = nouveau_drm(dev);
+	struct nouveau_disp *disp = nouveau_disp(drm->device);
+	int i;
 
 	nouveau_fbcon_fini(dev);
 	nouveau_accel_fini(drm);
@@ -420,6 +429,10 @@ nouveau_drm_unload(struct drm_device *dev)
 	nouveau_agp_fini(drm);
 	nouveau_vga_fini(drm);
 
+	for (i = 0; i < ARRAY_SIZE(drm->vblank); i++)
+		nouveau_event_handler_remove(disp->vblank, i,
+					     &drm->vblank[i]);
+
 	nouveau_cli_destroy(&drm->client);
 	return 0;
 }
-- 
1.8.1.2

Peter Hurley

2013-Aug-27 20:12 UTC

head link

[Nouveau] [PATCH 6/9] drm/nouveau: Convert event handler list to RCU

Lockdep report [1] correctly identifies a potential deadlock between
nouveau_drm_vblank_enable() and nouveau_drm_vblank_handler() due
to inverted lock order of event->lock with drm core's
dev->vblank_time_lock.

Fix vblank event deadlock by converting event handler list to RCU.
List updates remain serialized by event->lock.
List traversal & handler execution is lockless which prevents
inverted lock order problems with the drm core.
Safe deletion of the event handler is accomplished with
synchronize_rcu() for those handlers stored in nouveau_object-based
containers (nouveau_drm & nv50_/nvc0_software_context); otherwise,
with kfree_rcu (for nouveau_connector & uevent temporary handlers).

[1]
=====================================================[ INFO: possible circular
locking dependency detected ]
3.10.0-0+tip-xeon+lockdep #0+tip Not tainted
-------------------------------------------------------
swapper/7/0 is trying to acquire lock:
 (&(&dev->vblank_time_lock)->rlock){-.....}, at:
[<ffffffffa0086269>] drm_handle_vblank+0x69/0x400 [drm]

but task is already holding lock:
 (&(&event->lock)->rlock){-.....}, at: [<ffffffffa0101dbd>]
nouveau_event_trigger+0x4d/0xd0 [nouveau]

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #1 (&(&event->lock)->rlock){-.....}:
       [<ffffffff810b6d62>] lock_acquire+0x92/0x1f0
       [<ffffffff8175e926>] _raw_spin_lock_irqsave+0x46/0x60
       [<ffffffffa0101cf7>] nouveau_event_get+0x27/0xa0 [nouveau]
       [<ffffffffa01807bd>] nouveau_drm_vblank_enable+0x8d/0xf0 [nouveau]
       [<ffffffffa00856d8>] drm_vblank_get+0xf8/0x2c0 [drm]
       [<ffffffffa00867f4>] drm_wait_vblank+0x84/0x6f0 [drm]
       [<ffffffffa0081719>] drm_ioctl+0x559/0x690 [drm]
       [<ffffffff811bed77>] do_vfs_ioctl+0x97/0x590
       [<ffffffff811bf301>] SyS_ioctl+0x91/0xb0
       [<ffffffff817677c2>] system_call_fastpath+0x16/0x1b

-> #0 (&(&dev->vblank_time_lock)->rlock){-.....}:
       [<ffffffff810b65c3>] __lock_acquire+0x1c43/0x1d30
       [<ffffffff810b6d62>] lock_acquire+0x92/0x1f0
       [<ffffffff8175e926>] _raw_spin_lock_irqsave+0x46/0x60
       [<ffffffffa0086269>] drm_handle_vblank+0x69/0x400 [drm]
       [<ffffffffa0180847>] nouveau_drm_vblank_handler+0x27/0x30 [nouveau]
       [<ffffffffa0101e00>] nouveau_event_trigger+0x90/0xd0 [nouveau]
       [<ffffffffa012dafd>] nv50_disp_intr+0xdd/0x230 [nouveau]
       [<ffffffffa0120451>] nouveau_mc_intr+0xa1/0x100 [nouveau]
       [<ffffffff810f3c7c>] handle_irq_event_percpu+0x6c/0x3d0
       [<ffffffff810f4028>] handle_irq_event+0x48/0x70
       [<ffffffff810f71ca>] handle_fasteoi_irq+0x5a/0x100
       [<ffffffff81004512>] handle_irq+0x22/0x40
       [<ffffffff817691fa>] do_IRQ+0x5a/0xd0
       [<ffffffff8175f76f>] ret_from_intr+0x0/0x13
       [<ffffffff8100b876>] arch_cpu_idle+0x26/0x30
       [<ffffffff810a5b4e>] cpu_startup_entry+0xce/0x410
       [<ffffffff81743d4e>] start_secondary+0x255/0x25c

other info that might help us debug this:

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(&(&event->lock)->rlock);
                              
lock(&(&dev->vblank_time_lock)->rlock);
                               lock(&(&event->lock)->rlock);
  lock(&(&dev->vblank_time_lock)->rlock);

 *** DEADLOCK ***

1 lock held by swapper/7/0:
 #0:  (&(&event->lock)->rlock){-.....}, at:
[<ffffffffa0101dbd>] nouveau_event_trigger+0x4d/0xd0 [nouveau]

stack backtrace:
CPU: 7 PID: 0 Comm: swapper/7 Not tainted 3.10.0-0+tip-xeon+lockdep #0+tip
Hardware name: Dell Inc. Precision WorkStation T5400  /0RW203, BIOS A11
04/30/2012
 ffffffff821ccf60 ffff8802bfdc3b08 ffffffff81755f39 ffff8802bfdc3b58
 ffffffff8174f526 0000000000000002 ffff8802bfdc3be8 ffff8802b330a7f8
 ffff8802b330a820 ffff8802b330a7f8 0000000000000000 0000000000000001
Call Trace:
 <IRQ>  [<ffffffff81755f39>] dump_stack+0x19/0x1b
 [<ffffffff8174f526>] print_circular_bug+0x1fb/0x20c
 [<ffffffff810b65c3>] __lock_acquire+0x1c43/0x1d30
 [<ffffffff810b1f8d>] ? trace_hardirqs_off+0xd/0x10
 [<ffffffff8102b1a8>] ? flat_send_IPI_mask+0x88/0xa0
 [<ffffffff810b6d62>] lock_acquire+0x92/0x1f0
 [<ffffffffa0086269>] ? drm_handle_vblank+0x69/0x400 [drm]
 [<ffffffff8175e926>] _raw_spin_lock_irqsave+0x46/0x60
 [<ffffffffa0086269>] ? drm_handle_vblank+0x69/0x400 [drm]
 [<ffffffffa0086269>] drm_handle_vblank+0x69/0x400 [drm]
 [<ffffffffa0180847>] nouveau_drm_vblank_handler+0x27/0x30 [nouveau]
 [<ffffffffa0101e00>] nouveau_event_trigger+0x90/0xd0 [nouveau]
 [<ffffffffa012dafd>] nv50_disp_intr+0xdd/0x230 [nouveau]
 [<ffffffff8175f182>] ? _raw_spin_unlock_irqrestore+0x42/0x80
 [<ffffffff8107800c>] ? __hrtimer_start_range_ns+0x16c/0x530
 [<ffffffffa0120451>] nouveau_mc_intr+0xa1/0x100 [nouveau]
 [<ffffffff810f3c7c>] handle_irq_event_percpu+0x6c/0x3d0
 [<ffffffff810f4028>] handle_irq_event+0x48/0x70
 [<ffffffff810f718e>] ? handle_fasteoi_irq+0x1e/0x100
 [<ffffffff810f71ca>] handle_fasteoi_irq+0x5a/0x100
 [<ffffffff81004512>] handle_irq+0x22/0x40
 [<ffffffff817691fa>] do_IRQ+0x5a/0xd0
 [<ffffffff8175f76f>] common_interrupt+0x6f/0x6f
 <EOI>  [<ffffffff8100ad3d>] ? default_idle+0x1d/0x290
 [<ffffffff8100ad3b>] ? default_idle+0x1b/0x290
 [<ffffffff8100b876>] arch_cpu_idle+0x26/0x30
 [<ffffffff810a5b4e>] cpu_startup_entry+0xce/0x410
 [<ffffffff810ae0dc>] ? clockevents_register_device+0xdc/0x140
 [<ffffffff81743d4e>] start_secondary+0x255/0x25c

Reported-by: Dave Jones <davej at redhat.com>
Signed-off-by: Peter Hurley <peter at hurleysoftware.com>
---
 drivers/gpu/drm/nouveau/core/core/event.c          | 22 +++++++++++-----------
 .../gpu/drm/nouveau/core/engine/software/nv50.c    |  1 +
 .../gpu/drm/nouveau/core/engine/software/nvc0.c    |  1 +
 drivers/gpu/drm/nouveau/core/include/core/event.h  |  1 +
 drivers/gpu/drm/nouveau/nouveau_connector.c        |  2 +-
 drivers/gpu/drm/nouveau/nouveau_drm.c              |  1 +
 6 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/core/core/event.c
b/drivers/gpu/drm/nouveau/core/core/event.c
index 4cd1ebe..ce0a0ef 100644
--- a/drivers/gpu/drm/nouveau/core/core/event.c
+++ b/drivers/gpu/drm/nouveau/core/core/event.c
@@ -22,6 +22,7 @@
 
 #include <core/os.h>
 #include <core/event.h>
+#include <linux/rculist.h>
 
 void
 nouveau_event_handler_install(struct nouveau_event *event, int index,
@@ -37,7 +38,7 @@ nouveau_event_handler_install(struct nouveau_event *event, int
index,
 	handler->priv = priv;
 
 	spin_lock_irqsave(&event->lock, flags);
-	list_add(&handler->head, &event->index[index].list);
+	list_add_rcu(&handler->head, &event->index[index].list);
 	spin_unlock_irqrestore(&event->lock, flags);
 }
 
@@ -51,7 +52,7 @@ nouveau_event_handler_remove(struct nouveau_event *event, int
index,
 		return;
 
 	spin_lock_irqsave(&event->lock, flags);
-	list_del(&handler->head);
+	list_del_rcu(&handler->head);
 	spin_unlock_irqrestore(&event->lock, flags);
 }
 
@@ -71,7 +72,7 @@ nouveau_event_handler_create(struct nouveau_event *event, int
index,
 	__set_bit(NVKM_EVENT_ENABLE, &handler->flags);
 
 	spin_lock_irqsave(&event->lock, flags);
-	list_add(&handler->head, &event->index[index].list);
+	list_add_rcu(&handler->head, &event->index[index].list);
 	if (!event->index[index].refs++) {
 		if (event->enable)
 			event->enable(event, index);
@@ -94,9 +95,9 @@ nouveau_event_handler_destroy(struct nouveau_event *event, int
index,
 		if (event->disable)
 			event->disable(event, index);
 	}
-	list_del(&handler->head);
+	list_del_rcu(&handler->head);
 	spin_unlock_irqrestore(&event->lock, flags);
-	kfree(handler);
+	kfree_rcu(handler, rcu);
 }
 
 static void
@@ -147,20 +148,19 @@ nouveau_event_get(struct nouveau_event *event, int index,
 void
 nouveau_event_trigger(struct nouveau_event *event, int index)
 {
-	struct nouveau_eventh *handler, *temp;
-	unsigned long flags;
+	struct nouveau_eventh *handler;
 
 	if (index >= event->index_nr)
 		return;
 
-	spin_lock_irqsave(&event->lock, flags);
-	list_for_each_entry_safe(handler, temp, &event->index[index].list,
head) {
+	rcu_read_lock();
+	list_for_each_entry_rcu(handler, &event->index[index].list, head) {
 		if (test_bit(NVKM_EVENT_ENABLE, &handler->flags)) {
 			if (handler->func(handler, index) == NVKM_EVENT_DROP)
-				nouveau_event_put_locked(event, index, handler);
+				nouveau_event_put(event, index, handler);
 		}
 	}
-	spin_unlock_irqrestore(&event->lock, flags);
+	rcu_read_unlock();
 }
 
 void
diff --git a/drivers/gpu/drm/nouveau/core/engine/software/nv50.c
b/drivers/gpu/drm/nouveau/core/engine/software/nv50.c
index dfce1f5..87aeee1 100644
--- a/drivers/gpu/drm/nouveau/core/engine/software/nv50.c
+++ b/drivers/gpu/drm/nouveau/core/engine/software/nv50.c
@@ -191,6 +191,7 @@ nv50_software_context_dtor(struct nouveau_object *object)
 		nouveau_event_handler_remove(disp->vblank, i,
 					     &chan->base.vblank.event[i]);
 	}
+	synchronize_rcu();
 	_nouveau_software_context_dtor(object);
 }
 
diff --git a/drivers/gpu/drm/nouveau/core/engine/software/nvc0.c
b/drivers/gpu/drm/nouveau/core/engine/software/nvc0.c
index d1f52c0..e87ba42 100644
--- a/drivers/gpu/drm/nouveau/core/engine/software/nvc0.c
+++ b/drivers/gpu/drm/nouveau/core/engine/software/nvc0.c
@@ -197,6 +197,7 @@ nvc0_software_context_dtor(struct nouveau_object *object)
 		nouveau_event_handler_remove(disp->vblank, i,
 					     &chan->base.vblank.event[i]);
 	}
+	synchronize_rcu();
 	_nouveau_software_context_dtor(object);
 }
 
diff --git a/drivers/gpu/drm/nouveau/core/include/core/event.h
b/drivers/gpu/drm/nouveau/core/include/core/event.h
index 45e8a0f..f01b173 100644
--- a/drivers/gpu/drm/nouveau/core/include/core/event.h
+++ b/drivers/gpu/drm/nouveau/core/include/core/event.h
@@ -13,6 +13,7 @@ struct nouveau_eventh {
 	unsigned long flags;
 	void *priv;
 	int (*func)(struct nouveau_eventh *, int index);
+	struct rcu_head rcu;
 };
 
 struct nouveau_event {
diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c
b/drivers/gpu/drm/nouveau/nouveau_connector.c
index 13b38ed..14fce8a 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -106,7 +106,7 @@ nouveau_connector_destroy(struct drm_connector *connector)
 	kfree(nv_connector->edid);
 	drm_sysfs_connector_remove(connector);
 	drm_connector_cleanup(connector);
-	kfree(connector);
+	kfree_rcu(nv_connector, hpd_func.rcu);
 }
 
 static struct nouveau_i2c_port *
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 4187cad..544ca19 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -432,6 +432,7 @@ nouveau_drm_unload(struct drm_device *dev)
 	for (i = 0; i < ARRAY_SIZE(drm->vblank); i++)
 		nouveau_event_handler_remove(disp->vblank, i,
 					     &drm->vblank[i]);
+	synchronize_rcu();
 
 	nouveau_cli_destroy(&drm->client);
 	return 0;
-- 
1.8.1.2

Peter Hurley

2013-Aug-27 20:13 UTC

head link

[Nouveau] [PATCH 7/9] drm/nouveau: Fold nouveau_event_put_locked into caller

nouveau_event_put_locked() only has 1 call site; fold into caller.

Signed-off-by: Peter Hurley <peter at hurleysoftware.com>
---
 drivers/gpu/drm/nouveau/core/core/event.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/core/core/event.c
b/drivers/gpu/drm/nouveau/core/core/event.c
index ce0a0ef..45bcb37 100644
--- a/drivers/gpu/drm/nouveau/core/core/event.c
+++ b/drivers/gpu/drm/nouveau/core/core/event.c
@@ -100,18 +100,6 @@ nouveau_event_handler_destroy(struct nouveau_event *event,
int index,
 	kfree_rcu(handler, rcu);
 }
 
-static void
-nouveau_event_put_locked(struct nouveau_event *event, int index,
-			 struct nouveau_eventh *handler)
-{
-	if (__test_and_clear_bit(NVKM_EVENT_ENABLE, &handler->flags)) {
-		if (!--event->index[index].refs) {
-			if (event->disable)
-				event->disable(event, index);
-		}
-	}
-}
-
 void
 nouveau_event_put(struct nouveau_event *event, int index,
 		  struct nouveau_eventh *handler)
@@ -122,7 +110,12 @@ nouveau_event_put(struct nouveau_event *event, int index,
 		return;
 
 	spin_lock_irqsave(&event->lock, flags);
-	nouveau_event_put_locked(event, index, handler);
+	if (__test_and_clear_bit(NVKM_EVENT_ENABLE, &handler->flags)) {
+		if (!--event->index[index].refs) {
+			if (event->disable)
+				event->disable(event, index);
+		}
+	}
 	spin_unlock_irqrestore(&event->lock, flags);
 }
 
-- 
1.8.1.2

Peter Hurley

2013-Aug-27 20:13 UTC

head link

[Nouveau] [PATCH 8/9] drm/nouveau: Simplify event interface

Store event back-pointer and index within struct event_handler;
remove superfluous parameters when event_handler is supplied.

Signed-off-by: Peter Hurley <peter at hurleysoftware.com>
---
 drivers/gpu/drm/nouveau/core/core/event.c          | 36 +++++++++++++---------
 .../gpu/drm/nouveau/core/engine/software/nv50.c    | 11 ++-----
 .../gpu/drm/nouveau/core/engine/software/nvc0.c    | 11 ++-----
 drivers/gpu/drm/nouveau/core/include/core/event.h  | 15 +++++----
 drivers/gpu/drm/nouveau/nouveau_connector.c        |  5 +--
 drivers/gpu/drm/nouveau/nouveau_display.c          | 16 +++-------
 drivers/gpu/drm/nouveau/nouveau_drm.c              | 10 ++----
 drivers/gpu/drm/nouveau/nouveau_fence.c            |  2 +-
 8 files changed, 44 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/core/core/event.c
b/drivers/gpu/drm/nouveau/core/core/event.c
index 45bcb37..b7d8ae1 100644
--- a/drivers/gpu/drm/nouveau/core/core/event.c
+++ b/drivers/gpu/drm/nouveau/core/core/event.c
@@ -34,6 +34,9 @@ nouveau_event_handler_install(struct nouveau_event *event, int
index,
 	if (index >= event->index_nr)
 		return;
 
+	handler->event = event;
+	handler->index = index;
+
 	handler->func = func;
 	handler->priv = priv;
 
@@ -43,12 +46,12 @@ nouveau_event_handler_install(struct nouveau_event *event,
int index,
 }
 
 void
-nouveau_event_handler_remove(struct nouveau_event *event, int index,
-			     struct nouveau_eventh *handler)
+nouveau_event_handler_remove(struct nouveau_eventh *handler)
 {
+	struct nouveau_event *event = handler->event;
 	unsigned long flags;
 
-	if (index >= event->index_nr)
+	if (!event)
 		return;
 
 	spin_lock_irqsave(&event->lock, flags);
@@ -67,6 +70,10 @@ nouveau_event_handler_create(struct nouveau_event *event, int
index,
 	handler = *phandler = kzalloc(sizeof(*handler), GFP_KERNEL);
 	if (!handler)
 		return -ENOMEM;
+
+	handler->event = event;
+	handler->index = index;
+
 	handler->func = func;
 	handler->priv = priv;
 	__set_bit(NVKM_EVENT_ENABLE, &handler->flags);
@@ -82,13 +89,12 @@ nouveau_event_handler_create(struct nouveau_event *event,
int index,
 }
 
 void
-nouveau_event_handler_destroy(struct nouveau_event *event, int index,
-			      struct nouveau_eventh *handler)
+nouveau_event_handler_destroy(struct nouveau_eventh *handler)
 {
+	struct nouveau_event *event = handler->event;
+	int index = handler->index;
 	unsigned long flags;
 
-	if (index >= event->index_nr)
-		return;
 
 	spin_lock_irqsave(&event->lock, flags);
 	if (!--event->index[index].refs) {
@@ -101,12 +107,13 @@ nouveau_event_handler_destroy(struct nouveau_event *event,
int index,
 }
 
 void
-nouveau_event_put(struct nouveau_event *event, int index,
-		  struct nouveau_eventh *handler)
+nouveau_event_put(struct nouveau_eventh *handler)
 {
+	struct nouveau_event *event = handler->event;
+	int index = handler->index;
 	unsigned long flags;
 
-	if (index >= event->index_nr)
+	if (!event)
 		return;
 
 	spin_lock_irqsave(&event->lock, flags);
@@ -120,12 +127,13 @@ nouveau_event_put(struct nouveau_event *event, int index,
 }
 
 void
-nouveau_event_get(struct nouveau_event *event, int index,
-		  struct nouveau_eventh *handler)
+nouveau_event_get(struct nouveau_eventh *handler)
 {
+	struct nouveau_event *event = handler->event;
+	int index = handler->index;
 	unsigned long flags;
 
-	if (index >= event->index_nr)
+	if (!event)
 		return;
 
 	spin_lock_irqsave(&event->lock, flags);
@@ -150,7 +158,7 @@ nouveau_event_trigger(struct nouveau_event *event, int
index)
 	list_for_each_entry_rcu(handler, &event->index[index].list, head) {
 		if (test_bit(NVKM_EVENT_ENABLE, &handler->flags)) {
 			if (handler->func(handler, index) == NVKM_EVENT_DROP)
-				nouveau_event_put(event, index, handler);
+				nouveau_event_put(handler);
 		}
 	}
 	rcu_read_unlock();
diff --git a/drivers/gpu/drm/nouveau/core/engine/software/nv50.c
b/drivers/gpu/drm/nouveau/core/engine/software/nv50.c
index 87aeee1..e969f0c 100644
--- a/drivers/gpu/drm/nouveau/core/engine/software/nv50.c
+++ b/drivers/gpu/drm/nouveau/core/engine/software/nv50.c
@@ -92,12 +92,11 @@ nv50_software_mthd_vblsem_release(struct nouveau_object
*object, u32 mthd,
 				  void *args, u32 size)
 {
 	struct nv50_software_chan *chan = (void *)nv_engctx(object->parent);
-	struct nouveau_disp *disp = nouveau_disp(object);
 	u32 crtc = *(u32 *)args;
 	if (crtc > 1)
 		return -EINVAL;
 
-	nouveau_event_get(disp->vblank, crtc,
&chan->base.vblank.event[crtc]);
+	nouveau_event_get(&chan->base.vblank.event[crtc]);
 	return 0;
 }
 
@@ -183,14 +182,10 @@ void
 nv50_software_context_dtor(struct nouveau_object *object)
 {
 	struct nv50_software_chan *chan = (void *)object;
-	struct nv50_software_priv *priv = (void *)nv_object(chan)->engine;
-	struct nouveau_disp *disp = nouveau_disp(priv);
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(chan->base.vblank.event); i++) {
-		nouveau_event_handler_remove(disp->vblank, i,
-					     &chan->base.vblank.event[i]);
-	}
+	for (i = 0; i < ARRAY_SIZE(chan->base.vblank.event); i++)
+		nouveau_event_handler_remove(&chan->base.vblank.event[i]);
 	synchronize_rcu();
 	_nouveau_software_context_dtor(object);
 }
diff --git a/drivers/gpu/drm/nouveau/core/engine/software/nvc0.c
b/drivers/gpu/drm/nouveau/core/engine/software/nvc0.c
index e87ba42..d06658a 100644
--- a/drivers/gpu/drm/nouveau/core/engine/software/nvc0.c
+++ b/drivers/gpu/drm/nouveau/core/engine/software/nvc0.c
@@ -74,13 +74,12 @@ nvc0_software_mthd_vblsem_release(struct nouveau_object
*object, u32 mthd,
 				  void *args, u32 size)
 {
 	struct nvc0_software_chan *chan = (void *)nv_engctx(object->parent);
-	struct nouveau_disp *disp = nouveau_disp(object);
 	u32 crtc = *(u32 *)args;
 
 	if ((nv_device(object)->card_type < NV_E0 && crtc > 1) ||
crtc > 3)
 		return -EINVAL;
 
-	nouveau_event_get(disp->vblank, crtc,
&chan->base.vblank.event[crtc]);
+	nouveau_event_get(&chan->base.vblank.event[crtc]);
 	return 0;
 }
 
@@ -189,14 +188,10 @@ void
 nvc0_software_context_dtor(struct nouveau_object *object)
 {
 	struct nvc0_software_chan *chan = (void *)object;
-	struct nvc0_software_priv *priv = (void *)nv_object(chan)->engine;
-	struct nouveau_disp *disp = nouveau_disp(priv);
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(chan->base.vblank.event); i++) {
-		nouveau_event_handler_remove(disp->vblank, i,
-					     &chan->base.vblank.event[i]);
-	}
+	for (i = 0; i < ARRAY_SIZE(chan->base.vblank.event); i++)
+		nouveau_event_handler_remove(&chan->base.vblank.event[i]);
 	synchronize_rcu();
 	_nouveau_software_context_dtor(object);
 }
diff --git a/drivers/gpu/drm/nouveau/core/include/core/event.h
b/drivers/gpu/drm/nouveau/core/include/core/event.h
index f01b173..e839d70 100644
--- a/drivers/gpu/drm/nouveau/core/include/core/event.h
+++ b/drivers/gpu/drm/nouveau/core/include/core/event.h
@@ -9,11 +9,14 @@
 #define NVKM_EVENT_ENABLE 0
 
 struct nouveau_eventh {
+	struct nouveau_event *event;
+
 	struct list_head head;
 	unsigned long flags;
 	void *priv;
 	int (*func)(struct nouveau_eventh *, int index);
 	struct rcu_head rcu;
+	int index;
 };
 
 struct nouveau_event {
@@ -34,21 +37,17 @@ int  nouveau_event_create(int index_nr, struct nouveau_event
**);
 void nouveau_event_destroy(struct nouveau_event **);
 void nouveau_event_trigger(struct nouveau_event *, int index);
 
-void nouveau_event_get(struct nouveau_event *, int index,
-		       struct nouveau_eventh *);
-void nouveau_event_put(struct nouveau_event *, int index,
-		       struct nouveau_eventh *);
+void nouveau_event_get(struct nouveau_eventh *);
+void nouveau_event_put(struct nouveau_eventh *);
 
 int nouveau_event_handler_create(struct nouveau_event *, int index,
 				 int (*func)(struct nouveau_eventh*, int),
 				 void *priv, struct nouveau_eventh **);
-void nouveau_event_handler_destroy(struct nouveau_event *, int index,
-				   struct nouveau_eventh *);
+void nouveau_event_handler_destroy(struct nouveau_eventh *);
 
 void nouveau_event_handler_install(struct nouveau_event *, int index,
 				   int (*func)(struct nouveau_eventh*, int),
 				   void *priv, struct nouveau_eventh *);
-void nouveau_event_handler_remove(struct nouveau_event *, int index,
-				  struct nouveau_eventh *);
+void nouveau_event_handler_remove(struct nouveau_eventh *);
 
 #endif
diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c
b/drivers/gpu/drm/nouveau/nouveau_connector.c
index 14fce8a..85494d2 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -98,11 +98,8 @@ static void
 nouveau_connector_destroy(struct drm_connector *connector)
 {
 	struct nouveau_connector *nv_connector = nouveau_connector(connector);
-	struct nouveau_drm *drm = nouveau_drm(connector->dev);
-	struct nouveau_gpio *gpio = nouveau_gpio(drm->device);
 
-	nouveau_event_handler_remove(gpio->events, nv_connector->hpd.line,
-				     &nv_connector->hpd_func);
+	nouveau_event_handler_remove(&nv_connector->hpd_func);
 	kfree(nv_connector->edid);
 	drm_sysfs_connector_remove(connector);
 	drm_connector_cleanup(connector);
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c
b/drivers/gpu/drm/nouveau/nouveau_display.c
index 78637af..e9b1132 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -222,9 +222,7 @@ static struct nouveau_drm_prop_enum_list dither_depth[] = {
 int
 nouveau_display_init(struct drm_device *dev)
 {
-	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nouveau_display *disp = nouveau_display(dev);
-	struct nouveau_gpio *gpio = nouveau_gpio(drm->device);
 	struct drm_connector *connector;
 	int ret;
 
@@ -238,10 +236,8 @@ nouveau_display_init(struct drm_device *dev)
 	/* enable hotplug interrupts */
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
{
 		struct nouveau_connector *conn = nouveau_connector(connector);
-		if (gpio && conn->hpd.func != DCB_GPIO_UNUSED) {
-			nouveau_event_get(gpio->events, conn->hpd.line,
-					 &conn->hpd_func);
-		}
+		if (conn->hpd.func != DCB_GPIO_UNUSED)
+			nouveau_event_get(&conn->hpd_func);
 	}
 
 	return ret;
@@ -250,18 +246,14 @@ nouveau_display_init(struct drm_device *dev)
 void
 nouveau_display_fini(struct drm_device *dev)
 {
-	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nouveau_display *disp = nouveau_display(dev);
-	struct nouveau_gpio *gpio = nouveau_gpio(drm->device);
 	struct drm_connector *connector;
 
 	/* disable hotplug interrupts */
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
{
 		struct nouveau_connector *conn = nouveau_connector(connector);
-		if (gpio && conn->hpd.func != DCB_GPIO_UNUSED) {
-			nouveau_event_put(gpio->events, conn->hpd.line,
-					 &conn->hpd_func);
-		}
+		if (conn->hpd.func != DCB_GPIO_UNUSED)
+			nouveau_event_put(&conn->hpd_func);
 	}
 
 	drm_kms_helper_poll_disable(dev);
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 544ca19..845dd57 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -84,11 +84,10 @@ static int
 nouveau_drm_vblank_enable(struct drm_device *dev, int head)
 {
 	struct nouveau_drm *drm = nouveau_drm(dev);
-	struct nouveau_disp *pdisp = nouveau_disp(drm->device);
 
 	if (WARN_ON_ONCE(head > ARRAY_SIZE(drm->vblank)))
 		return -EIO;
-	nouveau_event_get(pdisp->vblank, head, &drm->vblank[head]);
+	nouveau_event_get(&drm->vblank[head]);
 	return 0;
 }
 
@@ -96,9 +95,8 @@ static void
 nouveau_drm_vblank_disable(struct drm_device *dev, int head)
 {
 	struct nouveau_drm *drm = nouveau_drm(dev);
-	struct nouveau_disp *pdisp = nouveau_disp(drm->device);
 
-	nouveau_event_put(pdisp->vblank, head, &drm->vblank[head]);
+	nouveau_event_put(&drm->vblank[head]);
 }
 
 static u64
@@ -411,7 +409,6 @@ static int
 nouveau_drm_unload(struct drm_device *dev)
 {
 	struct nouveau_drm *drm = nouveau_drm(dev);
-	struct nouveau_disp *disp = nouveau_disp(drm->device);
 	int i;
 
 	nouveau_fbcon_fini(dev);
@@ -430,8 +427,7 @@ nouveau_drm_unload(struct drm_device *dev)
 	nouveau_vga_fini(drm);
 
 	for (i = 0; i < ARRAY_SIZE(drm->vblank); i++)
-		nouveau_event_handler_remove(disp->vblank, i,
-					     &drm->vblank[i]);
+		nouveau_event_handler_remove(&drm->vblank[i]);
 	synchronize_rcu();
 
 	nouveau_cli_destroy(&drm->client);
diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c
b/drivers/gpu/drm/nouveau/nouveau_fence.c
index 6dde483..0ae280a 100644
--- a/drivers/gpu/drm/nouveau/nouveau_fence.c
+++ b/drivers/gpu/drm/nouveau/nouveau_fence.c
@@ -219,7 +219,7 @@ nouveau_fence_wait_uevent(struct nouveau_fence *fence, bool
intr)
 		}
 	}
 
-	nouveau_event_handler_destroy(pfifo->uevent, 0, handler);
+	nouveau_event_handler_destroy(handler);
 	if (unlikely(ret < 0))
 		return ret;
 
-- 
1.8.1.2

Peter Hurley

2013-Aug-27 20:13 UTC

head link

[Nouveau] [PATCH 9/9] drm/nouveau: Simplify event handler interface

Remove index parameter; access index via handler->index instead.
Dissociate handler from related container; use handler->priv to
access container.

Signed-off-by: Peter Hurley <peter at hurleysoftware.com>
---
 drivers/gpu/drm/nouveau/core/core/event.c           | 6 +++---
 drivers/gpu/drm/nouveau/core/engine/software/nv50.c | 7 +++----
 drivers/gpu/drm/nouveau/core/engine/software/nvc0.c | 7 +++----
 drivers/gpu/drm/nouveau/core/include/core/event.h   | 6 +++---
 drivers/gpu/drm/nouveau/nouveau_connector.c         | 9 +++++----
 drivers/gpu/drm/nouveau/nouveau_drm.c               | 9 +++++----
 drivers/gpu/drm/nouveau/nouveau_fence.c             | 2 +-
 7 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/core/core/event.c
b/drivers/gpu/drm/nouveau/core/core/event.c
index b7d8ae1..1240fef 100644
--- a/drivers/gpu/drm/nouveau/core/core/event.c
+++ b/drivers/gpu/drm/nouveau/core/core/event.c
@@ -26,7 +26,7 @@
 
 void
 nouveau_event_handler_install(struct nouveau_event *event, int index,
-			      int (*func)(struct nouveau_eventh*, int),
+			      int (*func)(struct nouveau_eventh*),
 			      void *priv, struct nouveau_eventh *handler)
 {
 	unsigned long flags;
@@ -61,7 +61,7 @@ nouveau_event_handler_remove(struct nouveau_eventh *handler)
 
 int
 nouveau_event_handler_create(struct nouveau_event *event, int index,
-			     int (*func)(struct nouveau_eventh*, int),
+			     int (*func)(struct nouveau_eventh*),
 			     void *priv, struct nouveau_eventh **phandler)
 {
 	struct nouveau_eventh *handler;
@@ -157,7 +157,7 @@ nouveau_event_trigger(struct nouveau_event *event, int
index)
 	rcu_read_lock();
 	list_for_each_entry_rcu(handler, &event->index[index].list, head) {
 		if (test_bit(NVKM_EVENT_ENABLE, &handler->flags)) {
-			if (handler->func(handler, index) == NVKM_EVENT_DROP)
+			if (handler->func(handler) == NVKM_EVENT_DROP)
 				nouveau_event_put(handler);
 		}
 	}
diff --git a/drivers/gpu/drm/nouveau/core/engine/software/nv50.c
b/drivers/gpu/drm/nouveau/core/engine/software/nv50.c
index e969f0c..bf6f23b 100644
--- a/drivers/gpu/drm/nouveau/core/engine/software/nv50.c
+++ b/drivers/gpu/drm/nouveau/core/engine/software/nv50.c
@@ -131,10 +131,9 @@ nv50_software_sclass[] = {
 
******************************************************************************/
 
 static int
-nv50_software_vblsem_release(struct nouveau_eventh *event, int head)
+nv50_software_vblsem_release(struct nouveau_eventh *handler)
 {
-	struct nouveau_software_chan *chan -		container_of(event, struct
nouveau_software_chan, vblank.event[head]);
+	struct nouveau_software_chan *chan = handler->priv;
 	struct nv50_software_priv *priv = (void *)nv_object(chan)->engine;
 	struct nouveau_bar *bar = nouveau_bar(priv);
 
@@ -172,7 +171,7 @@ nv50_software_context_ctor(struct nouveau_object *parent,
 	for (i = 0; i < ARRAY_SIZE(chan->base.vblank.event); i++) {
 		nouveau_event_handler_install(disp->vblank, i,
 					      nv50_software_vblsem_release,
-					      NULL,
+					      chan,
 					      &chan->base.vblank.event[i]);
 	}
 	return 0;
diff --git a/drivers/gpu/drm/nouveau/core/engine/software/nvc0.c
b/drivers/gpu/drm/nouveau/core/engine/software/nvc0.c
index d06658a..1a2a7a8 100644
--- a/drivers/gpu/drm/nouveau/core/engine/software/nvc0.c
+++ b/drivers/gpu/drm/nouveau/core/engine/software/nvc0.c
@@ -143,10 +143,9 @@ nvc0_software_sclass[] = {
 
******************************************************************************/
 
 static int
-nvc0_software_vblsem_release(struct nouveau_eventh *event, int head)
+nvc0_software_vblsem_release(struct nouveau_eventh *handler)
 {
-	struct nouveau_software_chan *chan -		container_of(event, struct
nouveau_software_chan, vblank.event[head]);
+	struct nouveau_software_chan *chan = handler->priv;
 	struct nvc0_software_priv *priv = (void *)nv_object(chan)->engine;
 	struct nouveau_bar *bar = nouveau_bar(priv);
 
@@ -178,7 +177,7 @@ nvc0_software_context_ctor(struct nouveau_object *parent,
 	for (i = 0; i < ARRAY_SIZE(chan->base.vblank.event); i++) {
 		nouveau_event_handler_install(disp->vblank, i,
 					      nvc0_software_vblsem_release,
-					      NULL,
+					      chan,
 					      &chan->base.vblank.event[i]);
 	}
 	return 0;
diff --git a/drivers/gpu/drm/nouveau/core/include/core/event.h
b/drivers/gpu/drm/nouveau/core/include/core/event.h
index e839d70..d062176 100644
--- a/drivers/gpu/drm/nouveau/core/include/core/event.h
+++ b/drivers/gpu/drm/nouveau/core/include/core/event.h
@@ -14,7 +14,7 @@ struct nouveau_eventh {
 	struct list_head head;
 	unsigned long flags;
 	void *priv;
-	int (*func)(struct nouveau_eventh *, int index);
+	int (*func)(struct nouveau_eventh *);
 	struct rcu_head rcu;
 	int index;
 };
@@ -41,12 +41,12 @@ void nouveau_event_get(struct nouveau_eventh *);
 void nouveau_event_put(struct nouveau_eventh *);
 
 int nouveau_event_handler_create(struct nouveau_event *, int index,
-				 int (*func)(struct nouveau_eventh*, int),
+				 int (*func)(struct nouveau_eventh*),
 				 void *priv, struct nouveau_eventh **);
 void nouveau_event_handler_destroy(struct nouveau_eventh *);
 
 void nouveau_event_handler_install(struct nouveau_event *, int index,
-				   int (*func)(struct nouveau_eventh*, int),
+				   int (*func)(struct nouveau_eventh*),
 				   void *priv, struct nouveau_eventh *);
 void nouveau_event_handler_remove(struct nouveau_eventh *);
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c
b/drivers/gpu/drm/nouveau/nouveau_connector.c
index 85494d2..bfe28a0 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -917,10 +917,10 @@ nouveau_connector_hotplug_work(struct work_struct *work)
 }
 
 static int
-nouveau_connector_hotplug(struct nouveau_eventh *event, int index)
+nouveau_connector_hotplug(struct nouveau_eventh *handler)
 {
-	struct nouveau_connector *nv_connector -		container_of(event, struct
nouveau_connector, hpd_func);
+	struct nouveau_connector *nv_connector = handler->priv;
+
 	schedule_work(&nv_connector->hpd_work);
 	return NVKM_EVENT_KEEP;
 }
@@ -994,7 +994,8 @@ nouveau_connector_create(struct drm_device *dev, int index)
 				 DCB_GPIO_UNUSED, &nv_connector->hpd);
 		nouveau_event_handler_install(gpio->events,
 					      nv_connector->hpd.line,
-					      nouveau_connector_hotplug, NULL,
+					      nouveau_connector_hotplug,
+					      nv_connector,
 					      &nv_connector->hpd_func);
 		if (ret)
 			nv_connector->hpd.func = DCB_GPIO_UNUSED;
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 845dd57..2e6a226 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -72,10 +72,11 @@ module_param_named(modeset, nouveau_modeset, int, 0400);
 static struct drm_driver driver;
 
 static int
-nouveau_drm_vblank_handler(struct nouveau_eventh *event, int head)
+nouveau_drm_vblank_handler(struct nouveau_eventh *handler)
 {
-	struct nouveau_drm *drm -		container_of(event, struct nouveau_drm,
vblank[head]);
+	struct nouveau_drm *drm = handler->priv;
+	int head = handler->index;
+
 	drm_handle_vblank(drm->dev, head);
 	return NVKM_EVENT_KEEP;
 }
@@ -354,7 +355,7 @@ nouveau_drm_load(struct drm_device *dev, unsigned long
flags)
 	for (i = 0; i < ARRAY_SIZE(drm->vblank); i++) {
 		nouveau_event_handler_install(disp->vblank, i,
 					      nouveau_drm_vblank_handler,
-					      NULL, &drm->vblank[i]);
+					      drm, &drm->vblank[i]);
 	}
 
 	nouveau_vga_init(drm);
diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c
b/drivers/gpu/drm/nouveau/nouveau_fence.c
index 0ae280a..f9bff26 100644
--- a/drivers/gpu/drm/nouveau/nouveau_fence.c
+++ b/drivers/gpu/drm/nouveau/nouveau_fence.c
@@ -166,7 +166,7 @@ nouveau_fence_done(struct nouveau_fence *fence)
 }
 
 static int
-nouveau_fence_wait_uevent_handler(struct nouveau_eventh *handler, int index)
+nouveau_fence_wait_uevent_handler(struct nouveau_eventh *handler)
 {
 	struct nouveau_fence_priv *priv = handler->priv;
 	wake_up_all(&priv->waiting);
-- 
1.8.1.2

Ben Skeggs

2013-Aug-28 07:15 UTC

head link

[Nouveau] [PATCH 0/9] drm/nouveau: Cleanup event/handler design

On Wed, Aug 28, 2013 at 6:12 AM, Peter Hurley <peter at
hurleysoftware.com> wrote:> This series was originally motivated by a deadlock, introduced in
> commit 1d7c71a3e2f77336df536855b0efd2dc5bdeb41b
> 'drm/nouveau/disp: port vblank handling to event interface',
> due to inverted lock order between nouveau_drm_vblank_enable()
> and nouveau_drm_vblank_handler() (the complete
> lockdep report is included in the patch 4/5 changelog).Hey Peter,

Thanks for the patch series.  I've only taken a cursory glance through
the patches thus far, as they're definitely too intrusive for -fixes
at this point.  I will take a proper look through the series within
the next week or so, I have some work pending which may possibly make
some of these changes unnecessary, though from what I can tell,
there's other good bits here we'll want.

In a previous mail on the locking issue, you mentioned the drm core
doing the "wrong thing" here too, did you have any further thoughts on
correcting that issue too?

Ben.
>
> Because this series fixes the vblank event deadlock, it is
> a competing solution to Maarten Lankhorst's 'drm/nouveau:
> fix vblank deadlock'.
>
> This series fixes the vblank event deadlock by converting the
> event handler list to RCU. This solution allows the event trigger
> to be lockless, and thus avoiding the lock inversion.
>
> Typical hurdles to RCU conversion include: 1) ensuring the
> object lifetime exceeds the lockless access; 2) preventing
> premature object reuse; and 3) verifying that stale object
> use not present logical errors.
>
> Because object reuse is not safe in RCU-based operations,
> the nouveau_event_get/_put interface is migrated from
> add/remove semantics to enable/disable semantics with a separate
> install/remove step (which also serves to document the
> handler lifetime). This also corrects an unsafe interface design
> where handlers can mistakenly be reused while in use.
>
> The nouveau driver currently supports 4 events -- gpio, uevent, cevent,
> and vblank. Every event is created by and stored within its
> respective subdev/engine object -- gpio in the GPIO subdev, uevent
> and cevent in the FIFO engine, and vblank in the DISP engine.
> Thus event lifetime extends until the subdev is destructed during
> devobj teardown.
>
> Event handler lifetime varies and is detailed in the table below
> (apologies for the wide-format):
>
> Event  Handler function                   Container                    
Until
> -----  ----------------                   ---------------              
------------------
> gpio   nouveau_connector_hotplug          struct nouveau_connector     
nouveau_connector_destroy
> uevent nouveau_fence_wait_uevent_handler  local stack object           
nouveau_fence_wait_uevent returns
> cevent none                               n/a                           n/a
> vblank nouveau_drm_vblank_handler         struct nouveau_drm           
nouveau_drm_remove
> vblank nv50_software_vblsem_release       struct nouveau_software_chan 
_nouveau_engctx_dtor
>                                                                          
(call stack originates with
>                                                                           
nouveau_abi16_chan_free ioctl)
> vblank nvc0_software_vblsem_release       struct nouveau_software_chan 
same as above
>
>
> RCU lifetime considerations for handlers:
>
> Event  Handler                            Lifetime
> -----  ----------------                   ---------------------------------
> gpio   nouveau_connector_hotplug          kfree_rcu(nv_connector)
> uevent nouveau_fence_wait_uevent_handler  explicit use of
nouveau_event_hander_create/_destroy
> cevent none                               n/a
> vblank nouveau_drm_vblank_handler         synchronize_rcu() in
nouveau_drm_unload
> vblank nv50_software_vblsem_release       synchronize_rcu() in container
dtor
> vblank nvc0_software_vblsem_release       synchronize_rcu() in container
dtor
>
> synchronize_rcu() is used for nouveau_object-based containers for which
> kfree_rcu() is not suitable/possible.
>
> Stale event handler execution is not a concern for the existing handlers
> because either: 1) the handler is responsible for disabling itself (via
> returning NVKM_EVENT_DROP), or 2) the existing handler can already be
stale,
> as is the case with nouveau_connector_hotplug, which only schedules a work
> item, and nouveau_drm_vblank_handler, which the drm core expects may be
stale.
>
>
> Peter Hurley (9):
>   drm/nouveau: Add priv field for event handlers
>   drm/nouveau: Move event index check from critical section
>   drm/nouveau: Allocate local event handlers
>   drm/nouveau: Allow asymmetric nouveau_event_get/_put
>   drm/nouveau: Add install/remove semantics for event handlers
>   drm/nouveau: Convert event handler list to RCU
>   drm/nouveau: Fold nouveau_event_put_locked into caller
>   drm/nouveau: Simplify event interface
>   drm/nouveau: Simplify event handler interface
>
>  drivers/gpu/drm/nouveau/core/core/event.c          | 121
+++++++++++++++++----
>  .../gpu/drm/nouveau/core/engine/software/nv50.c    |  32 ++++--
>  .../gpu/drm/nouveau/core/engine/software/nvc0.c    |  32 ++++--
>  drivers/gpu/drm/nouveau/core/include/core/event.h  |  27 ++++-
>  .../gpu/drm/nouveau/core/include/engine/software.h |   2 +-
>  drivers/gpu/drm/nouveau/nouveau_connector.c        |  16 ++-
>  drivers/gpu/drm/nouveau/nouveau_display.c          |  16 +--
>  drivers/gpu/drm/nouveau/nouveau_drm.c              |  35 +++---
>  drivers/gpu/drm/nouveau/nouveau_fence.c            |  27 ++---
>  9 files changed, 220 insertions(+), 88 deletions(-)
>
> --
> 1.8.1.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Peter Hurley

2013-Aug-30 01:23 UTC

head link

[Nouveau] [PATCH 0/9] drm/nouveau: Cleanup event/handler design

On 08/28/2013 03:15 AM, Ben Skeggs wrote:> On Wed, Aug 28, 2013 at 6:12 AM, Peter Hurley <peter at
hurleysoftware.com> wrote:
>> This series was originally motivated by a deadlock, introduced in
>> commit 1d7c71a3e2f77336df536855b0efd2dc5bdeb41b
>> 'drm/nouveau/disp: port vblank handling to event interface',
>> due to inverted lock order between nouveau_drm_vblank_enable()
>> and nouveau_drm_vblank_handler() (the complete
>> lockdep report is included in the patch 4/5 changelog).
> Hey Peter,
>
> Thanks for the patch series.  I've only taken a cursory glance through
> the patches thus far, as they're definitely too intrusive for -fixes
> at this point.  I will take a proper look through the series within
> the next week or so, I have some work pending which may possibly make
> some of these changes unnecessary, though from what I can tell,
> there's other good bits here we'll want.
>
> In a previous mail on the locking issue, you mentioned the drm core
> doing the "wrong thing" here too, did you have any further
thoughts on
> correcting that issue too?
I'm working on the premise that drm_handle_vblank() can be lockless;
that would minimize the api disruption. I still have a fair bit of
analysis to do, but some early observations:

1) The vbl_time_lock spinlock is unnecessary -- drm_handle_vblank()
    could be protected with vbl_lock, since everywhere else
    vbl_time_lock is held, vbl_lock is already held.

2) The _vblank_count[crtc] doesn't need to be atomic. All the code
    paths that update _vblank_count[] are already spinlocked. Even
    if the code weren't spinlocked, the atomic_inc/_adds aren't
    necessary; only the memory barriers and read/write order of
    the vblank count/timestamp tuple is relevant.

3) Careful enabling of drm_handle_vblank() is sufficient to
    solve the concurrency between drm_handle_vblank() and
    drm_vblank_get() without needing a spinlock for exclusion.
    drm_handle_vblank() would need to account for the possibility of
    having missed a vblank interrupt between the reading of the
    hw vblank counter and the enabling drm_handle_vblank().

4) I'm still thinking about how/whether to exclude
    drm_handle_vblank() and vblank_disable_and_save(). One thought
    is to replace the timeout timer with delayed work instead so
    that vblank_disable_and_save() could wait for
    drm_handle_vblank() to finish after disabling it.
    [I'd also need to verify that the drm drivers other than
    intel which use drm_vblank_off() do so from non-atomic context.]

I realize that I'm mostly referring to the hw counter/timestamp
flavor of vblank handling; that's primarily because it requires a
more rigorous handling and has race conditions that don't apply
to the software-only counter.

Regards,
Peter Hurley

Reasonably Related Threads

Search for more maybe matching threads

Nouveau - Aug 2013 - [PATCH 0/9] drm/nouveau: Cleanup event/handler design

[Nouveau] [PATCH 0/9] drm/nouveau: Cleanup event/handler design

[Nouveau] [PATCH 1/9] drm/nouveau: Add priv field for event handlers

[Nouveau] [PATCH 2/9] drm/nouveau: Move event index check from critical section

[Nouveau] [PATCH 3/9] drm/nouveau: Allocate local event handlers

[Nouveau] [PATCH 4/9] drm/nouveau: Allow asymmetric nouveau_event_get/_put

[Nouveau] [PATCH 5/9] drm/nouveau: Add install/remove semantics for event handlers

[Nouveau] [PATCH 6/9] drm/nouveau: Convert event handler list to RCU

[Nouveau] [PATCH 7/9] drm/nouveau: Fold nouveau_event_put_locked into caller

[Nouveau] [PATCH 8/9] drm/nouveau: Simplify event interface

[Nouveau] [PATCH 9/9] drm/nouveau: Simplify event handler interface

[Nouveau] [PATCH 0/9] drm/nouveau: Cleanup event/handler design

[Nouveau] [PATCH 0/9] drm/nouveau: Cleanup event/handler design

Reasonably Related Threads