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
>
