[PATCH] drm/i915: Localise the fbdev console lock frobbing

[PATCH] drm/i915: Localise the fbdev console lock frobbing

[Chris Wilson]

Rather than take and release the console_lock() around a non-existent
DRM_I915_FBDEV, move the lock acquisation into the callee where it will
be compiled out by the config option entirely. This includes moving the
deferred fb_set_suspend() dance and encapsulating it entirely within
intel_fbdev.c.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c    |  5 ++---
 drivers/gpu/drm/i915/i915_drv.c    | 26 +-------------------------
 drivers/gpu/drm/i915/i915_drv.h    |  8 --------
 drivers/gpu/drm/i915/intel_fbdev.c | 33 +++++++++++++++++++++++++++++++++
 4 files changed, 36 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 49149406fcd8..d0b3411fc13c 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1339,8 +1339,6 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	if (ret)
 		goto cleanup_irq;
 
-	INIT_WORK(&dev_priv->console_resume_work, intel_console_resume);
-
 	intel_modeset_gem_init(dev);
 
 	/* Always safe in the mode setting case. */
@@ -1835,6 +1833,8 @@ int i915_driver_unload(struct drm_device *dev)
 		return ret;
 	}
 
+	flush_scheduled_work();
+
 	i915_perf_unregister(dev);
 	intel_fini_runtime_pm(dev_priv);
 
@@ -1859,7 +1859,6 @@ int i915_driver_unload(struct drm_device *dev)
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		intel_fbdev_fini(dev);
 		intel_modeset_cleanup(dev);
-		cancel_work_sync(&dev_priv->console_resume_work);
 
 		/*
 		 * free the memory space allocated for the child device
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 35b119072c11..9adafc7c5819 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -558,9 +558,7 @@ static int i915_drm_freeze(struct drm_device *dev)
 	intel_uncore_forcewake_reset(dev, false);
 	intel_opregion_fini(dev);
 
-	console_lock();
 	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED);
-	console_unlock();
 
 	dev_priv->suspend_count++;
 
@@ -599,18 +597,6 @@ int i915_suspend(struct drm_device *dev, pm_message_t state)
 	return 0;
 }
 
-void intel_console_resume(struct work_struct *work)
-{
-	struct drm_i915_private *dev_priv =
-		container_of(work, struct drm_i915_private,
-			     console_resume_work);
-	struct drm_device *dev = dev_priv->dev;
-
-	console_lock();
-	intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING);
-	console_unlock();
-}
-
 static int i915_drm_thaw_early(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -683,17 +669,7 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
 
 	intel_opregion_init(dev);
 
-	/*
-	 * 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.
-	 */
-	if (console_trylock()) {
-		intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING);
-		console_unlock();
-	} else {
-		schedule_work(&dev_priv->console_resume_work);
-	}
+	intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING);
 
 	mutex_lock(&dev_priv->modeset_restore_lock);
 	dev_priv->modeset_restore = MODESET_DONE;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 517a78e27bbb..e38dd39d84ad 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1603,12 +1603,6 @@ struct drm_i915_private {
 	struct intel_fbdev *fbdev;
 #endif
 
-	/*
-	 * The console may be contended at resume, but we don't
-	 * want it to block on it.
-	 */
-	struct work_struct console_resume_work;
-
 	struct drm_property *broadcast_rgb_property;
 	struct drm_property *force_audio_property;
 
@@ -2310,8 +2304,6 @@ extern unsigned long i915_gfx_val(struct drm_i915_private *dev_priv);
 extern void i915_update_gfx_val(struct drm_i915_private *dev_priv);
 int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on);
 
-extern void intel_console_resume(struct work_struct *work);
-
 /* i915_irq.c */
 void i915_queue_hangcheck(struct drm_device *dev);
 __printf(3, 4)
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 5d879d185fe1..4eecb34a0e32 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -26,6 +26,7 @@
 
 #include <linux/module.h>
 #include <linux/kernel.h>
+#include <linux/console.h>
 #include <linux/errno.h>
 #include <linux/string.h>
 #include <linux/mm.h>
@@ -678,6 +679,19 @@ void intel_fbdev_fini(struct drm_device *dev)
 	dev_priv->fbdev = NULL;
 }
 
+struct set_suspend_work {
+	struct work_struct work;
+	struct drm_device *dev;
+	int state;
+};
+
+static void intel_fbdev_set_suspend_worker(struct work_struct *work)
+{
+	struct set_suspend_work *ss = container_of(work, typeof(*ss), work);
+	intel_fbdev_set_suspend(ss->dev, ss->state);
+	kfree(ss);
+}
+
 void intel_fbdev_set_suspend(struct drm_device *dev, int state)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -689,6 +703,24 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state)
 
 	info = ifbdev->helper.fbdev;
 
+	/*
+	 * 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.
+	 */
+	if (!console_trylock()) {
+		struct set_suspend_work *ss;
+
+		ss = kmalloc(sizeof(*ss), GFP_KERNEL);
+		if (ss) {
+			ss->dev = dev;
+			ss->state = state;
+			INIT_WORK(&ss->work, intel_fbdev_set_suspend_worker);
+			schedule_work(&ss->work);
+		}
+		return;
+	}
+
 	/* On resume from hibernation: If the object is shmemfs backed, it has
 	 * been restored from swap. If the object is stolen however, it will be
 	 * full of whatever garbage was left in there.
@@ -697,6 +729,7 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state)
 		memset_io(info->screen_base, 0, info->screen_size);
 
 	fb_set_suspend(info, state);
+	console_unlock();
 }
 
 void intel_fbdev_output_poll_changed(struct drm_device *dev)
-- 
2.1.0.rc1

Re: [PATCH] drm/i915: Localise the fbdev console lock frobbing

[Daniel Vetter]

On Tue, Aug 12, 2014 at 03:15:17PM +0100, Chris Wilson wrote:
> Rather than take and release the console_lock() around a non-existent
> DRM_I915_FBDEV, move the lock acquisation into the callee where it will
> be compiled out by the config option entirely. This includes moving the
> deferred fb_set_suspend() dance and encapsulating it entirely within
> intel_fbdev.c.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_dma.c    |  5 ++---
>  drivers/gpu/drm/i915/i915_drv.c    | 26 +-------------------------
>  drivers/gpu/drm/i915/i915_drv.h    |  8 --------
>  drivers/gpu/drm/i915/intel_fbdev.c | 33 +++++++++++++++++++++++++++++++++
>  4 files changed, 36 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 49149406fcd8..d0b3411fc13c 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1339,8 +1339,6 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  	if (ret)
>  		goto cleanup_irq;
>  
> -	INIT_WORK(&dev_priv->console_resume_work, intel_console_resume);
> -
>  	intel_modeset_gem_init(dev);
>  
>  	/* Always safe in the mode setting case. */
> @@ -1835,6 +1833,8 @@ int i915_driver_unload(struct drm_device *dev)
>  		return ret;
>  	}
>  
> +	flush_scheduled_work();

Tbh I prefer the explicit work flushing and keeping it in dev_priv. We can
shovel it into the I915_FBDEV #ifdef that's already there though.

> +
>  	i915_perf_unregister(dev);
>  	intel_fini_runtime_pm(dev_priv);
>  
> @@ -1859,7 +1859,6 @@ int i915_driver_unload(struct drm_device *dev)
>  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>  		intel_fbdev_fini(dev);
>  		intel_modeset_cleanup(dev);
> -		cancel_work_sync(&dev_priv->console_resume_work);
>  
>  		/*
>  		 * free the memory space allocated for the child device
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 35b119072c11..9adafc7c5819 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -558,9 +558,7 @@ static int i915_drm_freeze(struct drm_device *dev)
>  	intel_uncore_forcewake_reset(dev, false);
>  	intel_opregion_fini(dev);
>  
> -	console_lock();
>  	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED);

This one here must be synchronous.

> -	console_unlock();
>  
>  	dev_priv->suspend_count++;
>  
> @@ -599,18 +597,6 @@ int i915_suspend(struct drm_device *dev, pm_message_t state)
>  	return 0;
>  }
>  
> -void intel_console_resume(struct work_struct *work)
> -{
> -	struct drm_i915_private *dev_priv =
> -		container_of(work, struct drm_i915_private,
> -			     console_resume_work);
> -	struct drm_device *dev = dev_priv->dev;
> -
> -	console_lock();
> -	intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING);
> -	console_unlock();
> -}
> -
>  static int i915_drm_thaw_early(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -683,17 +669,7 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
>  
>  	intel_opregion_init(dev);
>  
> -	/*
> -	 * 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.
> -	 */
> -	if (console_trylock()) {
> -		intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING);
> -		console_unlock();
> -	} else {
> -		schedule_work(&dev_priv->console_resume_work);
> -	}
> +	intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING);
>  
>  	mutex_lock(&dev_priv->modeset_restore_lock);
>  	dev_priv->modeset_restore = MODESET_DONE;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 517a78e27bbb..e38dd39d84ad 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1603,12 +1603,6 @@ struct drm_i915_private {
>  	struct intel_fbdev *fbdev;
>  #endif
>  
> -	/*
> -	 * The console may be contended at resume, but we don't
> -	 * want it to block on it.
> -	 */
> -	struct work_struct console_resume_work;
> -
>  	struct drm_property *broadcast_rgb_property;
>  	struct drm_property *force_audio_property;
>  
> @@ -2310,8 +2304,6 @@ extern unsigned long i915_gfx_val(struct drm_i915_private *dev_priv);
>  extern void i915_update_gfx_val(struct drm_i915_private *dev_priv);
>  int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on);
>  
> -extern void intel_console_resume(struct work_struct *work);
> -
>  /* i915_irq.c */
>  void i915_queue_hangcheck(struct drm_device *dev);
>  __printf(3, 4)
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 5d879d185fe1..4eecb34a0e32 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -26,6 +26,7 @@
>  
>  #include <linux/module.h>
>  #include <linux/kernel.h>
> +#include <linux/console.h>
>  #include <linux/errno.h>
>  #include <linux/string.h>
>  #include <linux/mm.h>
> @@ -678,6 +679,19 @@ void intel_fbdev_fini(struct drm_device *dev)
>  	dev_priv->fbdev = NULL;
>  }
>  
> +struct set_suspend_work {
> +	struct work_struct work;
> +	struct drm_device *dev;
> +	int state;
> +};
> +
> +static void intel_fbdev_set_suspend_worker(struct work_struct *work)
> +{
> +	struct set_suspend_work *ss = container_of(work, typeof(*ss), work);
> +	intel_fbdev_set_suspend(ss->dev, ss->state);

This still trylocks so ends up busy-looping through launching more work
items. Probably better also do this synchronously.
-Daniel

> +	kfree(ss);
> +}
> +
>  void intel_fbdev_set_suspend(struct drm_device *dev, int state)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -689,6 +703,24 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state)
>  
>  	info = ifbdev->helper.fbdev;
>  
> +	/*
> +	 * 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.
> +	 */
> +	if (!console_trylock()) {
> +		struct set_suspend_work *ss;
> +
> +		ss = kmalloc(sizeof(*ss), GFP_KERNEL);
> +		if (ss) {
> +			ss->dev = dev;
> +			ss->state = state;
> +			INIT_WORK(&ss->work, intel_fbdev_set_suspend_worker);
> +			schedule_work(&ss->work);
> +		}
> +		return;
> +	}
> +
>  	/* On resume from hibernation: If the object is shmemfs backed, it has
>  	 * been restored from swap. If the object is stolen however, it will be
>  	 * full of whatever garbage was left in there.
> @@ -697,6 +729,7 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state)
>  		memset_io(info->screen_base, 0, info->screen_size);
>  
>  	fb_set_suspend(info, state);
> +	console_unlock();
>  }
>  
>  void intel_fbdev_output_poll_changed(struct drm_device *dev)
> -- 
> 2.1.0.rc1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Localise the fbdev console lock frobbing

[Chris Wilson]

On Wed, Aug 13, 2014 at 01:26:40PM +0200, Daniel Vetter wrote:
> On Tue, Aug 12, 2014 at 03:15:17PM +0100, Chris Wilson wrote:
> > Rather than take and release the console_lock() around a non-existent
> > DRM_I915_FBDEV, move the lock acquisation into the callee where it will
> > be compiled out by the config option entirely. This includes moving the
> > deferred fb_set_suspend() dance and encapsulating it entirely within
> > intel_fbdev.c.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c    |  5 ++---
> >  drivers/gpu/drm/i915/i915_drv.c    | 26 +-------------------------
> >  drivers/gpu/drm/i915/i915_drv.h    |  8 --------
> >  drivers/gpu/drm/i915/intel_fbdev.c | 33 +++++++++++++++++++++++++++++++++
> >  4 files changed, 36 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index 49149406fcd8..d0b3411fc13c 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1339,8 +1339,6 @@ static int i915_load_modeset_init(struct drm_device *dev)
> >  	if (ret)
> >  		goto cleanup_irq;
> >  
> > -	INIT_WORK(&dev_priv->console_resume_work, intel_console_resume);
> > -
> >  	intel_modeset_gem_init(dev);
> >  
> >  	/* Always safe in the mode setting case. */
> > @@ -1835,6 +1833,8 @@ int i915_driver_unload(struct drm_device *dev)
> >  		return ret;
> >  	}
> >  
> > +	flush_scheduled_work();
> 
> Tbh I prefer the explicit work flushing and keeping it in dev_priv. We can
> shovel it into the I915_FBDEV #ifdef that's already there though.
> 
> > +
> >  	i915_perf_unregister(dev);
> >  	intel_fini_runtime_pm(dev_priv);
> >  
> > @@ -1859,7 +1859,6 @@ int i915_driver_unload(struct drm_device *dev)
> >  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> >  		intel_fbdev_fini(dev);
> >  		intel_modeset_cleanup(dev);
> > -		cancel_work_sync(&dev_priv->console_resume_work);
> >  
> >  		/*
> >  		 * free the memory space allocated for the child device
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 35b119072c11..9adafc7c5819 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -558,9 +558,7 @@ static int i915_drm_freeze(struct drm_device *dev)
> >  	intel_uncore_forcewake_reset(dev, false);
> >  	intel_opregion_fini(dev);
> >  
> > -	console_lock();
> >  	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED);
> 
> This one here must be synchronous.

Right, but notice that we synchronize the work afterwards. I thought if
we were careful enough to call the waiter that waited for additional
work items to be completed, it would be sufficient.

> > +static void intel_fbdev_set_suspend_worker(struct work_struct *work)
> > +{
> > +	struct set_suspend_work *ss = container_of(work, typeof(*ss), work);
> > +	intel_fbdev_set_suspend(ss->dev, ss->state);
> 
> This still trylocks so ends up busy-looping through launching more work
> items. Probably better also do this synchronously.

Considered that, but I decided that trying to keep the locking
straightforward was better.

I'll look at putting the work item back onto the dev_priv so that we can
cleanly flush it.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Localise the fbdev console lock frobbing

[Daniel Vetter]

On Wed, Aug 13, 2014 at 12:32:33PM +0100, Chris Wilson wrote:
> On Wed, Aug 13, 2014 at 01:26:40PM +0200, Daniel Vetter wrote:
> > On Tue, Aug 12, 2014 at 03:15:17PM +0100, Chris Wilson wrote:
> > > Rather than take and release the console_lock() around a non-existent
> > > DRM_I915_FBDEV, move the lock acquisation into the callee where it will
> > > be compiled out by the config option entirely. This includes moving the
> > > deferred fb_set_suspend() dance and encapsulating it entirely within
> > > intel_fbdev.c.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> > >  drivers/gpu/drm/i915/i915_dma.c    |  5 ++---
> > >  drivers/gpu/drm/i915/i915_drv.c    | 26 +-------------------------
> > >  drivers/gpu/drm/i915/i915_drv.h    |  8 --------
> > >  drivers/gpu/drm/i915/intel_fbdev.c | 33 +++++++++++++++++++++++++++++++++
> > >  4 files changed, 36 insertions(+), 36 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > index 49149406fcd8..d0b3411fc13c 100644
> > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > @@ -1339,8 +1339,6 @@ static int i915_load_modeset_init(struct drm_device *dev)
> > >  	if (ret)
> > >  		goto cleanup_irq;
> > >  
> > > -	INIT_WORK(&dev_priv->console_resume_work, intel_console_resume);
> > > -
> > >  	intel_modeset_gem_init(dev);
> > >  
> > >  	/* Always safe in the mode setting case. */
> > > @@ -1835,6 +1833,8 @@ int i915_driver_unload(struct drm_device *dev)
> > >  		return ret;
> > >  	}
> > >  
> > > +	flush_scheduled_work();
> > 
> > Tbh I prefer the explicit work flushing and keeping it in dev_priv. We can
> > shovel it into the I915_FBDEV #ifdef that's already there though.
> > 
> > > +
> > >  	i915_perf_unregister(dev);
> > >  	intel_fini_runtime_pm(dev_priv);
> > >  
> > > @@ -1859,7 +1859,6 @@ int i915_driver_unload(struct drm_device *dev)
> > >  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> > >  		intel_fbdev_fini(dev);
> > >  		intel_modeset_cleanup(dev);
> > > -		cancel_work_sync(&dev_priv->console_resume_work);
> > >  
> > >  		/*
> > >  		 * free the memory space allocated for the child device
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > index 35b119072c11..9adafc7c5819 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -558,9 +558,7 @@ static int i915_drm_freeze(struct drm_device *dev)
> > >  	intel_uncore_forcewake_reset(dev, false);
> > >  	intel_opregion_fini(dev);
> > >  
> > > -	console_lock();
> > >  	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED);
> > 
> > This one here must be synchronous.
> 
> Right, but notice that we synchronize the work afterwards. I thought if
> we were careful enough to call the waiter that waited for additional
> work items to be completed, it would be sufficient.

Given that the work creates new work when it can't do it's job I don't
think that flush is sufficient. Iirc that only flush out pending stuff,
not newly queued work.

> > > +static void intel_fbdev_set_suspend_worker(struct work_struct *work)
> > > +{
> > > +	struct set_suspend_work *ss = container_of(work, typeof(*ss), work);
> > > +	intel_fbdev_set_suspend(ss->dev, ss->state);
> > 
> > This still trylocks so ends up busy-looping through launching more work
> > items. Probably better also do this synchronously.
> 
> Considered that, but I decided that trying to keep the locking
> straightforward was better.

Open-coding console_lock(); fb_set_suspend(); console_unlock(); doesn't
really look onerous to me.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Localise the fbdev console lock frobbing

[Chris Wilson]

On Wed, Aug 13, 2014 at 01:39:22PM +0200, Daniel Vetter wrote:
> On Wed, Aug 13, 2014 at 12:32:33PM +0100, Chris Wilson wrote:
> > On Wed, Aug 13, 2014 at 01:26:40PM +0200, Daniel Vetter wrote:
> > > This one here must be synchronous.
> > 
> > Right, but notice that we synchronize the work afterwards. I thought if
> > we were careful enough to call the waiter that waited for additional
> > work items to be completed, it would be sufficient.
> 
> Given that the work creates new work when it can't do it's job I don't
> think that flush is sufficient. Iirc that only flush out pending stuff,
> not newly queued work.

I had a fancy while(flush_work()) ; all planned out. But that is
effectively a busy-wait during suspend and the console_lock() is
contended, so that got binned. :(
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Localise the fbdev console lock frobbing

[Chris Wilson]

On Wed, Aug 13, 2014 at 01:39:22PM +0200, Daniel Vetter wrote:
> On Wed, Aug 13, 2014 at 12:32:33PM +0100, Chris Wilson wrote:
> > On Wed, Aug 13, 2014 at 01:26:40PM +0200, Daniel Vetter wrote:
> > > On Tue, Aug 12, 2014 at 03:15:17PM +0100, Chris Wilson wrote:
> > > > +static void intel_fbdev_set_suspend_worker(struct work_struct *work)
> > > > +{
> > > > +	struct set_suspend_work *ss = container_of(work, typeof(*ss), work);
> > > > +	intel_fbdev_set_suspend(ss->dev, ss->state);
> > > 
> > > This still trylocks so ends up busy-looping through launching more work
> > > items. Probably better also do this synchronously.
> > 
> > Considered that, but I decided that trying to keep the locking
> > straightforward was better.
> 
> Open-coding console_lock(); fb_set_suspend(); console_unlock(); doesn't
> really look onerous to me.

Apart from intel_fbdev_set_suspend does a little bit more on resume.
It's simple enough to add an extra parameter to tell
intel_fbdev_set_suspend to wait.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: Localise the fbdev console lock frobbing

[Chris Wilson]

Rather than take and release the console_lock() around a non-existent
DRM_I915_FBDEV, move the lock acquisation into the callee where it will
be compiled out by the config option entirely. This includes moving the
deferred fb_set_suspend() dance and encapsulating it entirely within
intel_fbdev.c.

v2: Use an integral work item so that we can explicitly flush the work
upon suspend/unload.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c    |  3 ---
 drivers/gpu/drm/i915/i915_drv.c    | 28 ++-----------------------
 drivers/gpu/drm/i915/i915_drv.h    |  9 +-------
 drivers/gpu/drm/i915/intel_drv.h   |  4 ++--
 drivers/gpu/drm/i915/intel_fbdev.c | 42 +++++++++++++++++++++++++++++++++++++-
 5 files changed, 46 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 860da759ae34..fbab9370cb5c 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1339,8 +1339,6 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	if (ret)
 		goto cleanup_irq;
 
-	INIT_WORK(&dev_priv->console_resume_work, intel_console_resume);
-
 	intel_modeset_gem_init(dev);
 
 	/* Always safe in the mode setting case. */
@@ -1857,7 +1855,6 @@ int i915_driver_unload(struct drm_device *dev)
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		intel_fbdev_fini(dev);
 		intel_modeset_cleanup(dev);
-		cancel_work_sync(&dev_priv->console_resume_work);
 
 		/*
 		 * free the memory space allocated for the child device
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 35b119072c11..7c7bb94d7654 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -558,9 +558,7 @@ static int i915_drm_freeze(struct drm_device *dev)
 	intel_uncore_forcewake_reset(dev, false);
 	intel_opregion_fini(dev);
 
-	console_lock();
-	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED);
-	console_unlock();
+	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED, true);
 
 	dev_priv->suspend_count++;
 
@@ -599,18 +597,6 @@ int i915_suspend(struct drm_device *dev, pm_message_t state)
 	return 0;
 }
 
-void intel_console_resume(struct work_struct *work)
-{
-	struct drm_i915_private *dev_priv =
-		container_of(work, struct drm_i915_private,
-			     console_resume_work);
-	struct drm_device *dev = dev_priv->dev;
-
-	console_lock();
-	intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING);
-	console_unlock();
-}
-
 static int i915_drm_thaw_early(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -683,17 +669,7 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
 
 	intel_opregion_init(dev);
 
-	/*
-	 * 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.
-	 */
-	if (console_trylock()) {
-		intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING);
-		console_unlock();
-	} else {
-		schedule_work(&dev_priv->console_resume_work);
-	}
+	intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING, false);
 
 	mutex_lock(&dev_priv->modeset_restore_lock);
 	dev_priv->modeset_restore = MODESET_DONE;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 19aa07b8d5dd..00f343116aad 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1601,14 +1601,9 @@ struct drm_i915_private {
 #ifdef CONFIG_DRM_I915_FBDEV
 	/* list of fbdev register on this device */
 	struct intel_fbdev *fbdev;
+	struct work_struct fbdev_suspend_work;
 #endif
 
-	/*
-	 * The console may be contended at resume, but we don't
-	 * want it to block on it.
-	 */
-	struct work_struct console_resume_work;
-
 	struct drm_property *broadcast_rgb_property;
 	struct drm_property *force_audio_property;
 
@@ -2308,8 +2303,6 @@ extern unsigned long i915_gfx_val(struct drm_i915_private *dev_priv);
 extern void i915_update_gfx_val(struct drm_i915_private *dev_priv);
 int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on);
 
-extern void intel_console_resume(struct work_struct *work);
-
 /* i915_irq.c */
 void i915_queue_hangcheck(struct drm_device *dev);
 __printf(3, 4)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index feb16c730d91..ffc9288ce3f3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -960,7 +960,7 @@ void intel_dvo_init(struct drm_device *dev);
 extern int intel_fbdev_init(struct drm_device *dev);
 extern void intel_fbdev_initial_config(struct drm_device *dev);
 extern void intel_fbdev_fini(struct drm_device *dev);
-extern void intel_fbdev_set_suspend(struct drm_device *dev, int state);
+extern void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous);
 extern void intel_fbdev_output_poll_changed(struct drm_device *dev);
 extern void intel_fbdev_restore_mode(struct drm_device *dev);
 #else
@@ -977,7 +977,7 @@ static inline void intel_fbdev_fini(struct drm_device *dev)
 {
 }
 
-static inline void intel_fbdev_set_suspend(struct drm_device *dev, int state)
+static inline void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous)
 {
 }
 
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 5d879d185fe1..5352b170b7bb 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -26,6 +26,7 @@
 
 #include <linux/module.h>
 #include <linux/kernel.h>
+#include <linux/console.h>
 #include <linux/errno.h>
 #include <linux/string.h>
 #include <linux/mm.h>
@@ -627,6 +628,15 @@ out:
 	return false;
 }
 
+static void intel_fbdev_suspend_worker(struct work_struct *work)
+{
+	intel_fbdev_set_suspend(container_of(work,
+					     struct drm_i915_private,
+					     fbdev_suspend_work)->dev,
+				FBINFO_STATE_RUNNING,
+				true);
+}
+
 int intel_fbdev_init(struct drm_device *dev)
 {
 	struct intel_fbdev *ifbdev;
@@ -653,6 +663,8 @@ int intel_fbdev_init(struct drm_device *dev)
 	}
 
 	dev_priv->fbdev = ifbdev;
+	INIT_WORK(&dev_priv->fbdev_suspend_work, intel_fbdev_suspend_worker);
+
 	drm_fb_helper_single_add_all_connectors(&ifbdev->helper);
 
 	return 0;
@@ -678,7 +690,7 @@ void intel_fbdev_fini(struct drm_device *dev)
 	dev_priv->fbdev = NULL;
 }
 
-void intel_fbdev_set_suspend(struct drm_device *dev, int state)
+void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_fbdev *ifbdev = dev_priv->fbdev;
@@ -689,6 +701,33 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state)
 
 	info = ifbdev->helper.fbdev;
 
+	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)
+			flush_work(&dev_priv->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.
+		 */
+		BUG_ON(state != FBINFO_STATE_RUNNING);
+		if (!console_trylock()) {
+			/* Don't block our own workqueue as this can
+			 * be run in parallel with other i915.ko tasks.
+			 */
+			schedule_work(&dev_priv->fbdev_suspend_work);
+			return;
+		}
+	}
+
 	/* On resume from hibernation: If the object is shmemfs backed, it has
 	 * been restored from swap. If the object is stolen however, it will be
 	 * full of whatever garbage was left in there.
@@ -697,6 +736,7 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state)
 		memset_io(info->screen_base, 0, info->screen_size);
 
 	fb_set_suspend(info, state);
+	console_unlock();
 }
 
 void intel_fbdev_output_poll_changed(struct drm_device *dev)
-- 
2.1.0.rc1

Re: [PATCH] drm/i915: Localise the fbdev console lock frobbing

[Daniel Vetter]

On Wed, Aug 13, 2014 at 01:09:46PM +0100, Chris Wilson wrote:
> Rather than take and release the console_lock() around a non-existent
> DRM_I915_FBDEV, move the lock acquisation into the callee where it will
> be compiled out by the config option entirely. This includes moving the
> deferred fb_set_suspend() dance and encapsulating it entirely within
> intel_fbdev.c.
> 
> v2: Use an integral work item so that we can explicitly flush the work
> upon suspend/unload.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_dma.c    |  3 ---
>  drivers/gpu/drm/i915/i915_drv.c    | 28 ++-----------------------
>  drivers/gpu/drm/i915/i915_drv.h    |  9 +-------
>  drivers/gpu/drm/i915/intel_drv.h   |  4 ++--
>  drivers/gpu/drm/i915/intel_fbdev.c | 42 +++++++++++++++++++++++++++++++++++++-
>  5 files changed, 46 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 860da759ae34..fbab9370cb5c 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1339,8 +1339,6 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  	if (ret)
>  		goto cleanup_irq;
>  
> -	INIT_WORK(&dev_priv->console_resume_work, intel_console_resume);
> -
>  	intel_modeset_gem_init(dev);
>  
>  	/* Always safe in the mode setting case. */
> @@ -1857,7 +1855,6 @@ int i915_driver_unload(struct drm_device *dev)
>  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>  		intel_fbdev_fini(dev);
>  		intel_modeset_cleanup(dev);
> -		cancel_work_sync(&dev_priv->console_resume_work);

This one here seems to have been lost - shouldn't we move this to
fbdev_fini instead? Otherwise this looks good imo, so I'll fix that up
while merging if you're ok.
-Daniel

>  
>  		/*
>  		 * free the memory space allocated for the child device
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 35b119072c11..7c7bb94d7654 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -558,9 +558,7 @@ static int i915_drm_freeze(struct drm_device *dev)
>  	intel_uncore_forcewake_reset(dev, false);
>  	intel_opregion_fini(dev);
>  
> -	console_lock();
> -	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED);
> -	console_unlock();
> +	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED, true);
>  
>  	dev_priv->suspend_count++;
>  
> @@ -599,18 +597,6 @@ int i915_suspend(struct drm_device *dev, pm_message_t state)
>  	return 0;
>  }
>  
> -void intel_console_resume(struct work_struct *work)
> -{
> -	struct drm_i915_private *dev_priv =
> -		container_of(work, struct drm_i915_private,
> -			     console_resume_work);
> -	struct drm_device *dev = dev_priv->dev;
> -
> -	console_lock();
> -	intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING);
> -	console_unlock();
> -}
> -
>  static int i915_drm_thaw_early(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -683,17 +669,7 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
>  
>  	intel_opregion_init(dev);
>  
> -	/*
> -	 * 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.
> -	 */
> -	if (console_trylock()) {
> -		intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING);
> -		console_unlock();
> -	} else {
> -		schedule_work(&dev_priv->console_resume_work);
> -	}
> +	intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING, false);
>  
>  	mutex_lock(&dev_priv->modeset_restore_lock);
>  	dev_priv->modeset_restore = MODESET_DONE;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 19aa07b8d5dd..00f343116aad 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1601,14 +1601,9 @@ struct drm_i915_private {
>  #ifdef CONFIG_DRM_I915_FBDEV
>  	/* list of fbdev register on this device */
>  	struct intel_fbdev *fbdev;
> +	struct work_struct fbdev_suspend_work;
>  #endif
>  
> -	/*
> -	 * The console may be contended at resume, but we don't
> -	 * want it to block on it.
> -	 */
> -	struct work_struct console_resume_work;
> -
>  	struct drm_property *broadcast_rgb_property;
>  	struct drm_property *force_audio_property;
>  
> @@ -2308,8 +2303,6 @@ extern unsigned long i915_gfx_val(struct drm_i915_private *dev_priv);
>  extern void i915_update_gfx_val(struct drm_i915_private *dev_priv);
>  int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on);
>  
> -extern void intel_console_resume(struct work_struct *work);
> -
>  /* i915_irq.c */
>  void i915_queue_hangcheck(struct drm_device *dev);
>  __printf(3, 4)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index feb16c730d91..ffc9288ce3f3 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -960,7 +960,7 @@ void intel_dvo_init(struct drm_device *dev);
>  extern int intel_fbdev_init(struct drm_device *dev);
>  extern void intel_fbdev_initial_config(struct drm_device *dev);
>  extern void intel_fbdev_fini(struct drm_device *dev);
> -extern void intel_fbdev_set_suspend(struct drm_device *dev, int state);
> +extern void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous);
>  extern void intel_fbdev_output_poll_changed(struct drm_device *dev);
>  extern void intel_fbdev_restore_mode(struct drm_device *dev);
>  #else
> @@ -977,7 +977,7 @@ static inline void intel_fbdev_fini(struct drm_device *dev)
>  {
>  }
>  
> -static inline void intel_fbdev_set_suspend(struct drm_device *dev, int state)
> +static inline void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous)
>  {
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 5d879d185fe1..5352b170b7bb 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -26,6 +26,7 @@
>  
>  #include <linux/module.h>
>  #include <linux/kernel.h>
> +#include <linux/console.h>
>  #include <linux/errno.h>
>  #include <linux/string.h>
>  #include <linux/mm.h>
> @@ -627,6 +628,15 @@ out:
>  	return false;
>  }
>  
> +static void intel_fbdev_suspend_worker(struct work_struct *work)
> +{
> +	intel_fbdev_set_suspend(container_of(work,
> +					     struct drm_i915_private,
> +					     fbdev_suspend_work)->dev,
> +				FBINFO_STATE_RUNNING,
> +				true);
> +}
> +
>  int intel_fbdev_init(struct drm_device *dev)
>  {
>  	struct intel_fbdev *ifbdev;
> @@ -653,6 +663,8 @@ int intel_fbdev_init(struct drm_device *dev)
>  	}
>  
>  	dev_priv->fbdev = ifbdev;
> +	INIT_WORK(&dev_priv->fbdev_suspend_work, intel_fbdev_suspend_worker);
> +
>  	drm_fb_helper_single_add_all_connectors(&ifbdev->helper);
>  
>  	return 0;
> @@ -678,7 +690,7 @@ void intel_fbdev_fini(struct drm_device *dev)
>  	dev_priv->fbdev = NULL;
>  }
>  
> -void intel_fbdev_set_suspend(struct drm_device *dev, int state)
> +void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_fbdev *ifbdev = dev_priv->fbdev;
> @@ -689,6 +701,33 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state)
>  
>  	info = ifbdev->helper.fbdev;
>  
> +	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)
> +			flush_work(&dev_priv->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.
> +		 */
> +		BUG_ON(state != FBINFO_STATE_RUNNING);
> +		if (!console_trylock()) {
> +			/* Don't block our own workqueue as this can
> +			 * be run in parallel with other i915.ko tasks.
> +			 */
> +			schedule_work(&dev_priv->fbdev_suspend_work);
> +			return;
> +		}
> +	}
> +
>  	/* On resume from hibernation: If the object is shmemfs backed, it has
>  	 * been restored from swap. If the object is stolen however, it will be
>  	 * full of whatever garbage was left in there.
> @@ -697,6 +736,7 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state)
>  		memset_io(info->screen_base, 0, info->screen_size);
>  
>  	fb_set_suspend(info, state);
> +	console_unlock();
>  }
>  
>  void intel_fbdev_output_poll_changed(struct drm_device *dev)
> -- 
> 2.1.0.rc1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Localise the fbdev console lock frobbing

[Chris Wilson]

On Wed, Aug 13, 2014 at 02:46:53PM +0200, Daniel Vetter wrote:
> On Wed, Aug 13, 2014 at 01:09:46PM +0100, Chris Wilson wrote:
> > Rather than take and release the console_lock() around a non-existent
> > DRM_I915_FBDEV, move the lock acquisation into the callee where it will
> > be compiled out by the config option entirely. This includes moving the
> > deferred fb_set_suspend() dance and encapsulating it entirely within
> > intel_fbdev.c.
> > 
> > v2: Use an integral work item so that we can explicitly flush the work
> > upon suspend/unload.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c    |  3 ---
> >  drivers/gpu/drm/i915/i915_drv.c    | 28 ++-----------------------
> >  drivers/gpu/drm/i915/i915_drv.h    |  9 +-------
> >  drivers/gpu/drm/i915/intel_drv.h   |  4 ++--
> >  drivers/gpu/drm/i915/intel_fbdev.c | 42 +++++++++++++++++++++++++++++++++++++-
> >  5 files changed, 46 insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index 860da759ae34..fbab9370cb5c 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1339,8 +1339,6 @@ static int i915_load_modeset_init(struct drm_device *dev)
> >  	if (ret)
> >  		goto cleanup_irq;
> >  
> > -	INIT_WORK(&dev_priv->console_resume_work, intel_console_resume);
> > -
> >  	intel_modeset_gem_init(dev);
> >  
> >  	/* Always safe in the mode setting case. */
> > @@ -1857,7 +1855,6 @@ int i915_driver_unload(struct drm_device *dev)
> >  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> >  		intel_fbdev_fini(dev);
> >  		intel_modeset_cleanup(dev);
> > -		cancel_work_sync(&dev_priv->console_resume_work);
> 
> This one here seems to have been lost - shouldn't we move this to
> fbdev_fini instead? Otherwise this looks good imo, so I'll fix that up
> while merging if you're ok.

Hmm, I counted on us calling suspend before intel_fbdev_fini() in
unload. Is this not the case? Otherwise I think we should be suspending
the console.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Localise the fbdev console lock frobbing

[Daniel Vetter]

On Wed, Aug 13, 2014 at 01:58:30PM +0100, Chris Wilson wrote:
> On Wed, Aug 13, 2014 at 02:46:53PM +0200, Daniel Vetter wrote:
> > On Wed, Aug 13, 2014 at 01:09:46PM +0100, Chris Wilson wrote:
> > > Rather than take and release the console_lock() around a non-existent
> > > DRM_I915_FBDEV, move the lock acquisation into the callee where it will
> > > be compiled out by the config option entirely. This includes moving the
> > > deferred fb_set_suspend() dance and encapsulating it entirely within
> > > intel_fbdev.c.
> > > 
> > > v2: Use an integral work item so that we can explicitly flush the work
> > > upon suspend/unload.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> > >  drivers/gpu/drm/i915/i915_dma.c    |  3 ---
> > >  drivers/gpu/drm/i915/i915_drv.c    | 28 ++-----------------------
> > >  drivers/gpu/drm/i915/i915_drv.h    |  9 +-------
> > >  drivers/gpu/drm/i915/intel_drv.h   |  4 ++--
> > >  drivers/gpu/drm/i915/intel_fbdev.c | 42 +++++++++++++++++++++++++++++++++++++-
> > >  5 files changed, 46 insertions(+), 40 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > index 860da759ae34..fbab9370cb5c 100644
> > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > @@ -1339,8 +1339,6 @@ static int i915_load_modeset_init(struct drm_device *dev)
> > >  	if (ret)
> > >  		goto cleanup_irq;
> > >  
> > > -	INIT_WORK(&dev_priv->console_resume_work, intel_console_resume);
> > > -
> > >  	intel_modeset_gem_init(dev);
> > >  
> > >  	/* Always safe in the mode setting case. */
> > > @@ -1857,7 +1855,6 @@ int i915_driver_unload(struct drm_device *dev)
> > >  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> > >  		intel_fbdev_fini(dev);
> > >  		intel_modeset_cleanup(dev);
> > > -		cancel_work_sync(&dev_priv->console_resume_work);
> > 
> > This one here seems to have been lost - shouldn't we move this to
> > fbdev_fini instead? Otherwise this looks good imo, so I'll fix that up
> > while merging if you're ok.
> 
> Hmm, I counted on us calling suspend before intel_fbdev_fini() in
> unload. Is this not the case? Otherwise I think we should be suspending
> the console.

We don't do any of that. Unregistering the fbdev emulation in fbdev_fini
should be enough though to stop anyone from actually using the gpu, and
we're careful to unregister other console drivers like vgacon to make sure
no one else can grow stupid ideas.

So I think the only concern really is that the work item completes before
the text section disappears, and maybe (on multi-gpu systems) that we
don't keep all fbdev devices disabled forever after a racy module unload.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Localise the fbdev console lock frobbing

[Chris Wilson]

On Wed, Aug 13, 2014 at 03:04:02PM +0200, Daniel Vetter wrote:
> On Wed, Aug 13, 2014 at 01:58:30PM +0100, Chris Wilson wrote:
> > On Wed, Aug 13, 2014 at 02:46:53PM +0200, Daniel Vetter wrote:
> > > On Wed, Aug 13, 2014 at 01:09:46PM +0100, Chris Wilson wrote:
> > > > Rather than take and release the console_lock() around a non-existent
> > > > DRM_I915_FBDEV, move the lock acquisation into the callee where it will
> > > > be compiled out by the config option entirely. This includes moving the
> > > > deferred fb_set_suspend() dance and encapsulating it entirely within
> > > > intel_fbdev.c.
> > > > 
> > > > v2: Use an integral work item so that we can explicitly flush the work
> > > > upon suspend/unload.
> > > > 
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_dma.c    |  3 ---
> > > >  drivers/gpu/drm/i915/i915_drv.c    | 28 ++-----------------------
> > > >  drivers/gpu/drm/i915/i915_drv.h    |  9 +-------
> > > >  drivers/gpu/drm/i915/intel_drv.h   |  4 ++--
> > > >  drivers/gpu/drm/i915/intel_fbdev.c | 42 +++++++++++++++++++++++++++++++++++++-
> > > >  5 files changed, 46 insertions(+), 40 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > > index 860da759ae34..fbab9370cb5c 100644
> > > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > > @@ -1339,8 +1339,6 @@ static int i915_load_modeset_init(struct drm_device *dev)
> > > >  	if (ret)
> > > >  		goto cleanup_irq;
> > > >  
> > > > -	INIT_WORK(&dev_priv->console_resume_work, intel_console_resume);
> > > > -
> > > >  	intel_modeset_gem_init(dev);
> > > >  
> > > >  	/* Always safe in the mode setting case. */
> > > > @@ -1857,7 +1855,6 @@ int i915_driver_unload(struct drm_device *dev)
> > > >  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> > > >  		intel_fbdev_fini(dev);
> > > >  		intel_modeset_cleanup(dev);
> > > > -		cancel_work_sync(&dev_priv->console_resume_work);
> > > 
> > > This one here seems to have been lost - shouldn't we move this to
> > > fbdev_fini instead? Otherwise this looks good imo, so I'll fix that up
> > > while merging if you're ok.
> > 
> > Hmm, I counted on us calling suspend before intel_fbdev_fini() in
> > unload. Is this not the case? Otherwise I think we should be suspending
> > the console.
> 
> We don't do any of that. Unregistering the fbdev emulation in fbdev_fini
> should be enough though to stop anyone from actually using the gpu, and
> we're careful to unregister other console drivers like vgacon to make sure
> no one else can grow stupid ideas.
> 
> So I think the only concern really is that the work item completes before
> the text section disappears, and maybe (on multi-gpu systems) that we
> don't keep all fbdev devices disabled forever after a racy module unload.

Ok, I followed the unload->fbdev_fini->unregister_console and concur
that all you want is the flush_work() in fbdev_fini.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Localise the fbdev console lock frobbing

[Daniel Vetter]

On Wed, Aug 13, 2014 at 02:09:22PM +0100, Chris Wilson wrote:
> On Wed, Aug 13, 2014 at 03:04:02PM +0200, Daniel Vetter wrote:
> > On Wed, Aug 13, 2014 at 01:58:30PM +0100, Chris Wilson wrote:
> > > On Wed, Aug 13, 2014 at 02:46:53PM +0200, Daniel Vetter wrote:
> > > > On Wed, Aug 13, 2014 at 01:09:46PM +0100, Chris Wilson wrote:
> > > > > Rather than take and release the console_lock() around a non-existent
> > > > > DRM_I915_FBDEV, move the lock acquisation into the callee where it will
> > > > > be compiled out by the config option entirely. This includes moving the
> > > > > deferred fb_set_suspend() dance and encapsulating it entirely within
> > > > > intel_fbdev.c.
> > > > > 
> > > > > v2: Use an integral work item so that we can explicitly flush the work
> > > > > upon suspend/unload.
> > > > > 
> > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_dma.c    |  3 ---
> > > > >  drivers/gpu/drm/i915/i915_drv.c    | 28 ++-----------------------
> > > > >  drivers/gpu/drm/i915/i915_drv.h    |  9 +-------
> > > > >  drivers/gpu/drm/i915/intel_drv.h   |  4 ++--
> > > > >  drivers/gpu/drm/i915/intel_fbdev.c | 42 +++++++++++++++++++++++++++++++++++++-
> > > > >  5 files changed, 46 insertions(+), 40 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > > > index 860da759ae34..fbab9370cb5c 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > > > @@ -1339,8 +1339,6 @@ static int i915_load_modeset_init(struct drm_device *dev)
> > > > >  	if (ret)
> > > > >  		goto cleanup_irq;
> > > > >  
> > > > > -	INIT_WORK(&dev_priv->console_resume_work, intel_console_resume);
> > > > > -
> > > > >  	intel_modeset_gem_init(dev);
> > > > >  
> > > > >  	/* Always safe in the mode setting case. */
> > > > > @@ -1857,7 +1855,6 @@ int i915_driver_unload(struct drm_device *dev)
> > > > >  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> > > > >  		intel_fbdev_fini(dev);
> > > > >  		intel_modeset_cleanup(dev);
> > > > > -		cancel_work_sync(&dev_priv->console_resume_work);
> > > > 
> > > > This one here seems to have been lost - shouldn't we move this to
> > > > fbdev_fini instead? Otherwise this looks good imo, so I'll fix that up
> > > > while merging if you're ok.
> > > 
> > > Hmm, I counted on us calling suspend before intel_fbdev_fini() in
> > > unload. Is this not the case? Otherwise I think we should be suspending
> > > the console.
> > 
> > We don't do any of that. Unregistering the fbdev emulation in fbdev_fini
> > should be enough though to stop anyone from actually using the gpu, and
> > we're careful to unregister other console drivers like vgacon to make sure
> > no one else can grow stupid ideas.
> > 
> > So I think the only concern really is that the work item completes before
> > the text section disappears, and maybe (on multi-gpu systems) that we
> > don't keep all fbdev devices disabled forever after a racy module unload.
> 
> Ok, I followed the unload->fbdev_fini->unregister_console and concur
> that all you want is the flush_work() in fbdev_fini.

Done&merged, thanks a lot for the patch - looks much better than my #ifdef
hacks ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Localise the fbdev console lock frobbing

[Chris Wilson]

On Wed, Aug 13, 2014 at 03:40:24PM +0200, Daniel Vetter wrote:
> On Wed, Aug 13, 2014 at 02:09:22PM +0100, Chris Wilson wrote:
> > On Wed, Aug 13, 2014 at 03:04:02PM +0200, Daniel Vetter wrote:
> > > On Wed, Aug 13, 2014 at 01:58:30PM +0100, Chris Wilson wrote:
> > > > On Wed, Aug 13, 2014 at 02:46:53PM +0200, Daniel Vetter wrote:
> > > > > On Wed, Aug 13, 2014 at 01:09:46PM +0100, Chris Wilson wrote:
> > > > > > Rather than take and release the console_lock() around a non-existent
> > > > > > DRM_I915_FBDEV, move the lock acquisation into the callee where it will
> > > > > > be compiled out by the config option entirely. This includes moving the
> > > > > > deferred fb_set_suspend() dance and encapsulating it entirely within
> > > > > > intel_fbdev.c.
> > > > > > 
> > > > > > v2: Use an integral work item so that we can explicitly flush the work
> > > > > > upon suspend/unload.
> > > > > > 
> > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/i915_dma.c    |  3 ---
> > > > > >  drivers/gpu/drm/i915/i915_drv.c    | 28 ++-----------------------
> > > > > >  drivers/gpu/drm/i915/i915_drv.h    |  9 +-------
> > > > > >  drivers/gpu/drm/i915/intel_drv.h   |  4 ++--
> > > > > >  drivers/gpu/drm/i915/intel_fbdev.c | 42 +++++++++++++++++++++++++++++++++++++-
> > > > > >  5 files changed, 46 insertions(+), 40 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > > > > index 860da759ae34..fbab9370cb5c 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > > > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > > > > @@ -1339,8 +1339,6 @@ static int i915_load_modeset_init(struct drm_device *dev)
> > > > > >  	if (ret)
> > > > > >  		goto cleanup_irq;
> > > > > >  
> > > > > > -	INIT_WORK(&dev_priv->console_resume_work, intel_console_resume);
> > > > > > -
> > > > > >  	intel_modeset_gem_init(dev);
> > > > > >  
> > > > > >  	/* Always safe in the mode setting case. */
> > > > > > @@ -1857,7 +1855,6 @@ int i915_driver_unload(struct drm_device *dev)
> > > > > >  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> > > > > >  		intel_fbdev_fini(dev);
> > > > > >  		intel_modeset_cleanup(dev);
> > > > > > -		cancel_work_sync(&dev_priv->console_resume_work);
> > > > > 
> > > > > This one here seems to have been lost - shouldn't we move this to
> > > > > fbdev_fini instead? Otherwise this looks good imo, so I'll fix that up
> > > > > while merging if you're ok.
> > > > 
> > > > Hmm, I counted on us calling suspend before intel_fbdev_fini() in
> > > > unload. Is this not the case? Otherwise I think we should be suspending
> > > > the console.
> > > 
> > > We don't do any of that. Unregistering the fbdev emulation in fbdev_fini
> > > should be enough though to stop anyone from actually using the gpu, and
> > > we're careful to unregister other console drivers like vgacon to make sure
> > > no one else can grow stupid ideas.
> > > 
> > > So I think the only concern really is that the work item completes before
> > > the text section disappears, and maybe (on multi-gpu systems) that we
> > > don't keep all fbdev devices disabled forever after a racy module unload.
> > 
> > Ok, I followed the unload->fbdev_fini->unregister_console and concur
> > that all you want is the flush_work() in fbdev_fini.
> 
> Done&merged, thanks a lot for the patch - looks much better than my #ifdef
> hacks ;-)

I disaggree with the conversion of the BUG_ON though, a WARN there is
going to screw up unpredictably (well, a hard hang without any output
is the predictable outcome). I'd like to have asserts for things that
could and should be statically analyzed...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Localise the fbdev console lock frobbing

[Daniel Vetter]

On Thu, Aug 14, 2014 at 8:54 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> I disaggree with the conversion of the BUG_ON though, a WARN there is
> going to screw up unpredictably (well, a hard hang without any output
> is the predictable outcome). I'd like to have asserts for things that
> could and should be statically analyzed...

Well I've put a zero-tolerance rule for BUG_ON into place with the
only exception if the kernel will die anyway in the next few lines.
Which means I trade in a limping (and potentially dangerous) kernel
for the ability to be able to read the backtrace somewhere. I agree
that any such extreme policy will end up looking stupid in some cases,
but I've just decided that I wasted too much time on chasing lookups
which would have been trivial to debug with a WARN_ON instead of a
BUG_ON.

Until I've wasted too much time with WARN_ON instead of BUG_ON I'll
let it stick.  And it's supported by my patch scripts, so small chance
I'll miss one. Ofc I'll never change it without a notice in the commit
message, so people can always blame me for it.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

BUG_ON vs WARN_ON (was: Re: [PATCH] drm/i915: Localise the fbdev console lock frobbing)

[Jani Nikula]

On Thu, 14 Aug 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Aug 14, 2014 at 8:54 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> I disaggree with the conversion of the BUG_ON though, a WARN there is
>> going to screw up unpredictably (well, a hard hang without any output
>> is the predictable outcome). I'd like to have asserts for things that
>> could and should be statically analyzed...
>
> Well I've put a zero-tolerance rule for BUG_ON into place with the
> only exception if the kernel will die anyway in the next few lines.
> Which means I trade in a limping (and potentially dangerous) kernel
> for the ability to be able to read the backtrace somewhere. I agree
> that any such extreme policy will end up looking stupid in some cases,
> but I've just decided that I wasted too much time on chasing lookups
> which would have been trivial to debug with a WARN_ON instead of a
> BUG_ON.
>
> Until I've wasted too much time with WARN_ON instead of BUG_ON I'll
> let it stick.  And it's supported by my patch scripts, so small chance
> I'll miss one. Ofc I'll never change it without a notice in the commit
> message, so people can always blame me for it.

In other words, WARN_ON is the new BUG_ON. But what's the new WARN_ON?
Now we're conflating two things (limp home mode and crashing) into
one. When I see WARN_ON in code, it's no longer clear to me whether this
is a condition that we're supposed to survive or not. For example, does
the code below a WARN_ON need to properly handle errors due to the
condition? To me, BUG_ON is a code reading aide that sets the absolute
precondition for the following code. Something that absolutely must be
fixed if someone hits it, while WARN_ON can sometimes be ignored, or
even replaced with DRM_DEBUG.

If we have zero-tolerance for BUG_ON, and replace all of those with
WARN_ON, we'll need to start being *very* selective about adding WARN_ON
as well.


BR,
Jani.


> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: BUG_ON vs WARN_ON (was: Re: [PATCH] drm/i915: Localise the fbdev console lock frobbing)

[Daniel Vetter]

On Thu, Aug 14, 2014 at 01:07:06PM +0300, Jani Nikula wrote:
> On Thu, 14 Aug 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Thu, Aug 14, 2014 at 8:54 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >> I disaggree with the conversion of the BUG_ON though, a WARN there is
> >> going to screw up unpredictably (well, a hard hang without any output
> >> is the predictable outcome). I'd like to have asserts for things that
> >> could and should be statically analyzed...
> >
> > Well I've put a zero-tolerance rule for BUG_ON into place with the
> > only exception if the kernel will die anyway in the next few lines.
> > Which means I trade in a limping (and potentially dangerous) kernel
> > for the ability to be able to read the backtrace somewhere. I agree
> > that any such extreme policy will end up looking stupid in some cases,
> > but I've just decided that I wasted too much time on chasing lookups
> > which would have been trivial to debug with a WARN_ON instead of a
> > BUG_ON.
> >
> > Until I've wasted too much time with WARN_ON instead of BUG_ON I'll
> > let it stick.  And it's supported by my patch scripts, so small chance
> > I'll miss one. Ofc I'll never change it without a notice in the commit
> > message, so people can always blame me for it.
> 
> In other words, WARN_ON is the new BUG_ON. But what's the new WARN_ON?
> Now we're conflating two things (limp home mode and crashing) into
> one. When I see WARN_ON in code, it's no longer clear to me whether this
> is a condition that we're supposed to survive or not. For example, does
> the code below a WARN_ON need to properly handle errors due to the
> condition? To me, BUG_ON is a code reading aide that sets the absolute
> precondition for the following code. Something that absolutely must be
> fixed if someone hits it, while WARN_ON can sometimes be ignored, or
> even replaced with DRM_DEBUG.

WARN_ON is what userspace hackers usually put into asserts - pre/post
conditions and invariants and stuff like that worth checking but not part
of the main logic. Occasionally it makes sense to have special logic in a
WARN_ON (e.g. when a refcount overflows it's better to leak it), but
usually not worth it.

A WARN_ON should never be on a level with DRM_DEBUG, but perhaps a
DRM_ERROR is more adequate if the backtrace is useless.

> If we have zero-tolerance for BUG_ON, and replace all of those with
> WARN_ON, we'll need to start being *very* selective about adding WARN_ON
> as well.

My rule of thumb for letting a BUG_ON survive is if I can see the kernel
Oopsing in the diff context, it stays. I don't see why that suddenly means
we have to sprinkle less of them, or of WARN_ONs.

So maybe you need to elaborate on your concern here a bit?

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch