Nouveau - Aug 2013 - [PATCH] drm/nouveau: fix vblank deadlock

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.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst at 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) {

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 at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau
>

Hey,

Op 19-08-13 20:12, Peter Hurley schreef:
> 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.
If only it was so easy..

nouveau is doing a non-standard thing with core/engine/software, specifically
the release method.
The real fix is reverting the commit that changes this to a nouveau_event type
of thing, and reinstates
the old locking. This is not going to happen though, so the second best fix is
just telling it to lock the
other lock too when changing enable state.

~Maarten