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
[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
[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
[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
[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
[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
[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
[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
[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
[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
[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
[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