[Nouveau] [PATCH] drm/nouveau/fbcon: fix deadlock with FBIOPUT_CON2FBMAP
Daniel Vetter
daniel at ffwll.ch
Wed Jul 13 09:54:49 UTC 2016
On Tue, Jul 12, 2016 at 06:49:34PM +0200, Peter Wu wrote:
> The FBIOPUT_CON2FBMAP ioctl takes a console_lock(). When this is called
> while nouveau was runtime suspended, a deadlock would occur due to
> nouveau_fbcon_set_suspend also trying to obtain console_lock().
>
> Fix this by delaying the drm_fb_helper_set_suspend call. Based on the
> i915 code (which was done for performance reasons though).
>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Signed-off-by: Peter Wu <peter at lekensteyn.nl>
> ---
> Tested on top of v4.7-rc5, the deadlock is gone.
If we bother with this, it should imo be moved into the drm_fb_helper.c
function drm_fb_helper_set_suspend(). But this also smells like some kind
of bad duct-tape. I think Lukas is working on some other rpm vs. fbdev
deadlocks, maybe we could fix them all with one proper fix? I've made some
comments on Lukas' last patch series.
Besides this, when fixing a deadlock pls provide more details about the
precise callchain and the locks involved in the deadlock. If you
discovered this using lockdep, then just add the entire lockdep splat to
the commit message. Otherwise there's lots of guesswork involved here.
-Daniel
> ---
> drivers/gpu/drm/nouveau/nouveau_drm.c | 4 +--
> drivers/gpu/drm/nouveau/nouveau_drv.h | 1 +
> drivers/gpu/drm/nouveau/nouveau_fbcon.c | 54 ++++++++++++++++++++++++++++-----
> drivers/gpu/drm/nouveau/nouveau_fbcon.h | 2 +-
> 4 files changed, 50 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 11f8dd9..f9a2c10 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -552,7 +552,7 @@ nouveau_do_suspend(struct drm_device *dev, bool runtime)
>
> if (dev->mode_config.num_crtc) {
> NV_INFO(drm, "suspending console...\n");
> - nouveau_fbcon_set_suspend(dev, 1);
> + nouveau_fbcon_set_suspend(dev, FBINFO_STATE_SUSPENDED, true);
> NV_INFO(drm, "suspending display...\n");
> ret = nouveau_display_suspend(dev, runtime);
> if (ret)
> @@ -635,7 +635,7 @@ nouveau_do_resume(struct drm_device *dev, bool runtime)
> NV_INFO(drm, "resuming display...\n");
> nouveau_display_resume(dev, runtime);
> NV_INFO(drm, "resuming console...\n");
> - nouveau_fbcon_set_suspend(dev, 0);
> + nouveau_fbcon_set_suspend(dev, FBINFO_STATE_RUNNING, false);
> }
>
> return 0;
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h
> index 822a021..a743d19 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drv.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
> @@ -147,6 +147,7 @@ struct nouveau_drm {
> struct nouveau_channel *channel;
> struct nvkm_gpuobj *notify;
> struct nouveau_fbdev *fbcon;
> + struct work_struct fbdev_suspend_work;
> struct nvif_object nvsw;
> struct nvif_object ntfy;
> struct nvif_notify flip;
> diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> index d1f248f..089156a 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> @@ -492,19 +492,53 @@ static const struct drm_fb_helper_funcs nouveau_fbcon_helper_funcs = {
> .fb_probe = nouveau_fbcon_create,
> };
>
> +static void nouveau_fbcon_suspend_worker(struct work_struct *work)
> +{
> + nouveau_fbcon_set_suspend(container_of(work,
> + struct nouveau_drm,
> + fbdev_suspend_work)->dev,
> + FBINFO_STATE_RUNNING,
> + true);
> +}
> +
> void
> -nouveau_fbcon_set_suspend(struct drm_device *dev, int state)
> +nouveau_fbcon_set_suspend(struct drm_device *dev, int state, bool synchronous)
> {
> struct nouveau_drm *drm = nouveau_drm(dev);
> - if (drm->fbcon) {
> - console_lock();
> - if (state == FBINFO_STATE_RUNNING)
> - nouveau_fbcon_accel_restore(dev);
> - drm_fb_helper_set_suspend(&drm->fbcon->helper, state);
> + if (!drm->fbcon)
> + return;
> +
> + if (synchronous) {
> + /* Flush any pending work to turn the console on, and then
> + * wait to turn it off. It must be synchronous as we are
> + * about to suspend or unload the driver.
> + *
> + * Note that from within the work-handler, we cannot flush
> + * ourselves, so only flush outstanding work upon suspend!
> + */
> if (state != FBINFO_STATE_RUNNING)
> - nouveau_fbcon_accel_save_disable(dev);
> - console_unlock();
> + flush_work(&drm->fbdev_suspend_work);
> + console_lock();
> + } else {
> + /*
> + * The console lock can be pretty contented on resume due
> + * to all the printk activity. Try to keep it out of the hot
> + * path of resume if possible. This also prevents a deadlock
> + * with FBIOPUT_CON2FBMAP.
> + */
> + WARN_ON(state != FBINFO_STATE_RUNNING);
> + if (!console_trylock()) {
> + schedule_work(&drm->fbdev_suspend_work);
> + return;
> + }
> }
> +
> + if (state == FBINFO_STATE_RUNNING)
> + nouveau_fbcon_accel_restore(dev);
> + drm_fb_helper_set_suspend(&drm->fbcon->helper, state);
> + if (state != FBINFO_STATE_RUNNING)
> + nouveau_fbcon_accel_save_disable(dev);
> + console_unlock();
> }
>
> int
> @@ -526,6 +560,8 @@ nouveau_fbcon_init(struct drm_device *dev)
> fbcon->dev = dev;
> drm->fbcon = fbcon;
>
> + INIT_WORK(&drm->fbdev_suspend_work, nouveau_fbcon_suspend_worker);
> +
> drm_fb_helper_prepare(dev, &fbcon->helper, &nouveau_fbcon_helper_funcs);
>
> ret = drm_fb_helper_init(dev, &fbcon->helper,
> @@ -571,6 +607,8 @@ nouveau_fbcon_fini(struct drm_device *dev)
> if (!drm->fbcon)
> return;
>
> + flush_work(&drm->fbdev_suspend_work);
> +
> nouveau_fbcon_accel_fini(dev);
> nouveau_fbcon_destroy(dev, drm->fbcon);
> kfree(drm->fbcon);
> diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.h b/drivers/gpu/drm/nouveau/nouveau_fbcon.h
> index ca77ad0..34b2504 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_fbcon.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.h
> @@ -66,7 +66,7 @@ void nouveau_fbcon_gpu_lockup(struct fb_info *info);
>
> int nouveau_fbcon_init(struct drm_device *dev);
> void nouveau_fbcon_fini(struct drm_device *dev);
> -void nouveau_fbcon_set_suspend(struct drm_device *dev, int state);
> +void nouveau_fbcon_set_suspend(struct drm_device *dev, int state, bool synchronous);
> void nouveau_fbcon_accel_save_disable(struct drm_device *dev);
> void nouveau_fbcon_accel_restore(struct drm_device *dev);
>
> --
> 2.8.3
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Nouveau
mailing list