Re: [Nouveau] [PATCH] drm/nouveau: fix vblank deadlock

On 08/12/2013 07:50 AM, Maarten Lankhorst wrote:

This fixes a deadlock inversion when vblank is enabled/disabled by drm. &dev->vblank_time_lock is always taken when the vblank state is toggled, which caused a deadlock when &event->lock was also taken during event_get/put.

Solve the race by requiring that lock to change enable/disable state, and always keeping vblank on the event list. Core drm ignores unwanted vblanks, so extra calls to drm_handle_vblank are harmless.

I don't feel this is the appropriate solution to the lock inversion between vblank_time_lock and event->lock.

Preferably drm core should correct the interface layer bug; ie., calling into a sub-driver holding a lock _and_ requiring the sub-driver to call a drm helper function which claims the same lock is bad design. The console lock suffers from the same design flaw and is a constant problem.

Alternatively, the event trigger could be lockless; ie., the event list could be an RCU list instead. In this way, the event->lock does not need to be claimed, and thus no lock inversion is possible. The main drawback here is that currently the event->lock enforces non-overlapping lifetimes between the event handler and the event. Untangling object lifetimes in nouveau is a non-trivial exercise.

Regards, Peter Hurley

Signed-off-by: Maarten Lankhorst maarten.lankhorst@canonical.com

diff --git a/drivers/gpu/drm/nouveau/core/core/event.c b/drivers/gpu/drm/nouveau/core/core/event.c index 7eb81c1..78bff7c 100644 --- a/drivers/gpu/drm/nouveau/core/core/event.c +++ b/drivers/gpu/drm/nouveau/core/core/event.c @@ -23,43 +23,64 @@ #include <core/os.h> #include <core/event.h>

-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);
  

• }
• list_del(&handler->head);

-}

• void nouveau_event_put(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);

• if (index < event->index_nr)

• nouveau_event_put_locked(event, index, handler);
  

• list_del(&handler->head);

• if (event->toggle_lock)

• spin_lock(event->toggle_lock);
  

• nouveau_event_disable_locked(event, index, 1);

• if (event->toggle_lock)

• spin_unlock(event->toggle_lock);
  

• spin_unlock_irqrestore(&event->lock, flags); }

  void

+nouveau_event_enable_locked(struct nouveau_event *event, int index) +{

• if (index >= event->index_nr)

• return;
  

• if (!event->index[index].refs++ && event->enable)

• event->enable(event, index);
  

+}

+void +nouveau_event_disable_locked(struct nouveau_event *event, int index, int refs) +{

• if (index >= event->index_nr)

• return;
  

• event->index[index].refs -= refs;
• if (!event->index[index].refs && event->disable)

• event->disable(event, index);
  

+}

+void nouveau_event_get(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);

• 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->toggle_lock)

• spin_lock(event->toggle_lock);
  

• nouveau_event_enable_locked(event, index);
• if (event->toggle_lock)

• spin_unlock(event->toggle_lock);
  

  spin_unlock_irqrestore(&event->lock, flags); }

@@ -68,6 +89,7 @@ nouveau_event_trigger(struct nouveau_event *event, int index) { struct nouveau_eventh *handler, *temp; unsigned long flags;

• int refs = 0;

  if (index >= event->index_nr) return;

@@ -75,9 +97,17 @@ 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);
  

• 	list_del(&handler->head);
  

• 	refs++;
  

  } }
• if (refs) {

• if (event->toggle_lock)
  

• 	spin_lock(event->toggle_lock);
  

• nouveau_event_disable_locked(event, index, refs);
  

• if (event->toggle_lock)
  

• 	spin_unlock(event->toggle_lock);
  

• } 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 9e09440..b1a6c91 100644 --- a/drivers/gpu/drm/nouveau/core/include/core/event.h +++ b/drivers/gpu/drm/nouveau/core/include/core/event.h @@ -12,6 +12,7 @@ struct nouveau_eventh {

struct nouveau_event { spinlock_t lock;

• spinlock_t *toggle_lock;

  void *priv; void (*enable)(struct nouveau_event *, int index);

@@ -33,4 +34,8 @@ void nouveau_event_get(struct nouveau_event *, int index, void nouveau_event_put(struct nouveau_event *, int index, struct nouveau_eventh *);

+void nouveau_event_enable_locked(struct nouveau_event *event, int index); +void nouveau_event_disable_locked(struct nouveau_event *event, int index,

• 		  int refs);
  

• #endif

diff --git a/drivers/gpu/drm/nouveau/nouveau_crtc.h b/drivers/gpu/drm/nouveau/nouveau_crtc.h index d1e5890..398baa3 100644 --- a/drivers/gpu/drm/nouveau/nouveau_crtc.h +++ b/drivers/gpu/drm/nouveau/nouveau_crtc.h @@ -29,6 +29,7 @@

struct nouveau_crtc { struct drm_crtc base;

• struct nouveau_eventh vblank;

  int index;

diff --git a/drivers/gpu/drm/nouveau/nouveau_display.h b/drivers/gpu/drm/nouveau/nouveau_display.h index 1ea3e47..4ba8cb5 100644 --- a/drivers/gpu/drm/nouveau/nouveau_display.h +++ b/drivers/gpu/drm/nouveau/nouveau_display.h @@ -72,6 +72,7 @@ int nouveau_display_dumb_destroy(struct drm_file *, struct drm_device *, u32 handle);

void nouveau_hdmi_mode_set(struct drm_encoder *, struct drm_display_mode *); +int nouveau_drm_vblank_handler(struct nouveau_eventh *event, int head);

#ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT extern int nouveau_backlight_init(struct drm_device *); diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c index 4bf8dc3..bd301f4 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -46,6 +46,7 @@ #include "nouveau_pm.h" #include "nouveau_acpi.h" #include "nouveau_bios.h" +#include "nouveau_crtc.h" #include "nouveau_ioctl.h" #include "nouveau_abi16.h" #include "nouveau_fbcon.h" @@ -71,12 +72,13 @@ module_param_named(modeset, nouveau_modeset, int, 0400);

static struct drm_driver driver;

-static int +int nouveau_drm_vblank_handler(struct nouveau_eventh *event, int head) {

• struct nouveau_drm *drm =

• container_of(event, struct nouveau_drm, vblank[head]);
  

• drm_handle_vblank(drm->dev, head);

• struct nouveau_crtc *nv_crtc =

• container_of(event, struct nouveau_crtc, vblank);
  

• drm_handle_vblank(nv_crtc->base.dev, head); return NVKM_EVENT_KEEP; }

@@ -86,11 +88,9 @@ 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)))

• if (head >= pdisp->vblank->index_nr) 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]);

• nouveau_event_enable_locked(pdisp->vblank, head); return 0; }

@@ -99,11 +99,9 @@ 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;

• if (head < pdisp->vblank->index_nr)

• nouveau_event_disable_locked(pdisp->vblank, head, 1);
  

  }

  static u64

diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.h b/drivers/gpu/drm/nouveau/nouveau_drm.h index 41ff7e0..0d0ba3b 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.h +++ b/drivers/gpu/drm/nouveau/nouveau_drm.h @@ -125,7 +125,6 @@ struct nouveau_drm { struct nvbios vbios; struct nouveau_display *display; struct backlight_device *backlight;

• struct nouveau_eventh vblank[4];

  /* power management */ struct nouveau_pm *pm;

diff --git a/drivers/gpu/drm/nouveau/nv50_display.c b/drivers/gpu/drm/nouveau/nv50_display.c index 007731d..738d7a2 100644 --- a/drivers/gpu/drm/nouveau/nv50_display.c +++ b/drivers/gpu/drm/nouveau/nv50_display.c @@ -39,6 +39,7 @@ #include <core/client.h> #include <core/gpuobj.h> #include <core/class.h> +#include <engine/disp.h>

#include <subdev/timer.h> #include <subdev/bar.h> @@ -1280,15 +1281,21 @@ nv50_crtc_gamma_set(struct drm_crtc *crtc, u16 *r, u16 *g, u16 *b, static void nv50_crtc_destroy(struct drm_crtc *crtc) {

• struct nouveau_event *event = nouveau_disp(nouveau_dev(crtc->dev))->vblank; struct nouveau_crtc *nv_crtc = nouveau_crtc(crtc); struct nv50_disp *disp = nv50_disp(crtc->dev); struct nv50_head *head = nv50_head(crtc);

• unsigned long flags;

  nv50_dmac_destroy(disp->core, &head->ovly.base); nv50_pioc_destroy(disp->core, &head->oimm.base); nv50_dmac_destroy(disp->core, &head->sync.base); nv50_pioc_destroy(disp->core, &head->curs.base);

• spin_lock_irqsave(&event->lock, flags);

• list_del(&nv_crtc->vblank.head);

• spin_unlock_irqrestore(&event->lock, flags);

• /*XXX: this shouldn't be necessary, but the core doesn't call

  • disconnect() during the cleanup paths
    

  */

@@ -1344,10 +1351,12 @@ nv50_cursor_set_offset(struct nouveau_crtc *nv_crtc, uint32_t offset) static int nv50_crtc_create(struct drm_device *dev, struct nouveau_object *core, int index) {

• struct nouveau_event *event = nouveau_disp(nouveau_dev(dev))->vblank; struct nv50_disp *disp = nv50_disp(dev); struct nv50_head *head; struct drm_crtc *crtc; int ret, i;

• unsigned long flags;

  head = kzalloc(sizeof(*head), GFP_KERNEL); if (!head)

@@ -1372,6 +1381,12 @@ nv50_crtc_create(struct drm_device *dev, struct nouveau_object *core, int index) drm_crtc_helper_add(crtc, &nv50_crtc_hfunc); drm_mode_crtc_set_gamma_size(crtc, 256);

• spin_lock_irqsave(&event->lock, flags);
• event->toggle_lock = &dev->vblank_time_lock;
• head->base.vblank.func = nouveau_drm_vblank_handler;
• list_add(&head->base.vblank.head, &event->index[index].list);
• spin_unlock_irqrestore(&event->lock, flags);
• ret = nouveau_bo_new(dev, 8192, 0x100, TTM_PL_FLAG_VRAM, 0, 0x0000, NULL, NULL, &head->base.lut.nvbo); if (!ret) {

---

Nouveau mailing list Nouveau@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/nouveau