Skip to content

drm/nouveau: Cleanup event/handler design

Peter Hurley dri-devel nouveau 1 version In review

Get series with b4
b4 am [email protected]

Versions newest first

  1. v1 Latest9 patches 2 repliesIn review

Patches in v1 No applies recorded yet

#Subject and filesSizeRepliesApplied
1/9drm/nouveau: Add priv field for event handlersdrivers/gpu/drm/nouveau/core/include/core/event.h drivers/gpu/drm/nouveau/nouveau_fence.c+8 -130 replies
2/9drm/nouveau: Move event index check from critical sectiondrivers/gpu/drm/nouveau/core/core/event.c+11 -80 replies
3/9drm/nouveau: Allocate local event handlersdrivers/gpu/drm/nouveau/core/core/event.c drivers/gpu/drm/nouveau/core/include/core/event.h drivers/gpu/drm/nouveau/nouveau_fence.c+38 -70 replies
4/9drm/nouveau: Allow asymmetric nouveau_event_get/_putdrivers/gpu/drm/nouveau/core/core/event.c drivers/gpu/drm/nouveau/core/include/core/event.h drivers/gpu/drm/nouveau/nouveau_drm.c+18 -140 replies
5/9drm/nouveau: Add install/remove semantics for event handlersdrivers/gpu/drm/nouveau/core/core/event.c drivers/gpu/drm/nouveau/core/engine/software/nv50.c drivers/gpu/drm/nouveau/core/engine/software/nvc0.c +4 more+140 -200 replies
6/9drm/nouveau: Convert event handler list to RCUdrivers/gpu/drm/nouveau/core/core/event.c drivers/gpu/drm/nouveau/core/engine/software/nv50.c drivers/gpu/drm/nouveau/core/engine/software/nvc0.c +3 more+16 -120 replies
7/9drm/nouveau: Fold nouveau_event_put_locked into callerdrivers/gpu/drm/nouveau/core/core/event.c+6 -130 replies
8/9drm/nouveau: Simplify event interfacedrivers/gpu/drm/nouveau/core/core/event.c drivers/gpu/drm/nouveau/core/engine/software/nv50.c drivers/gpu/drm/nouveau/core/engine/software/nvc0.c +5 more+44 -620 replies
9/9drm/nouveau: Simplify event handler interfacedrivers/gpu/drm/nouveau/core/core/event.c drivers/gpu/drm/nouveau/core/engine/software/nv50.c drivers/gpu/drm/nouveau/core/engine/software/nvc0.c +4 more+23 -230 replies

Reviews trailers per patch

No Reviewed-by, Acked-by or Tested-by trailers for v1 yet.

Read the v1 thread

Cover letter [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(-)
Signature · 1 line
1.8.1.2