On Wed, Aug 28, 2013 at 6:12 AM, Peter Hurley <peter@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
