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@xxxxxxxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Signed-off-by: Peter Wu <peter@xxxxxxxxxxxxx>
> ---
> Tested on top of v4.7-rc5, the deadlock is gone.
Hm, how did you trigger this deadlock?
Thanks,
Lukas
> ---
> 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
>
> _______________________________________________
> Nouveau mailing list
> Nouveau@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/nouveau
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
