[PATCH 2/6] drm/fb-helper: Support device unplug

[Daniel Vetter]

On Mon, Aug 28, 2017 at 07:17:44PM +0200, Noralf Trønnes wrote:
> Support device unplug by introducing a new initialization function:
> drm_fb_helper_simple_init() which together with
> drm_fb_helper_dev_unplug() and drm_fb_helper_fb_destroy() handles open
> fbdev fd's after device unplug. There's also a
> drm_fb_helper_simple_fini() for drivers who's device won't be removed.
> 
> It turns out that fbdev has the necessary ref counting, so
> unregister_framebuffer() together with fb_ops->destroy handles unplug
> with open fd's. The ref counting doesn't apply to the fb_info structure
> itself, but to use of the fbdev framebuffer.
> 
> Analysis of entry points after unregister_framebuffer():
> - fbcon has been unbound (through notifier)
> - sysfs files removed
> 
> First we look at possible operations on open fbdev file handles:
> 
> static const struct file_operations fb_fops = {
> 	.read =		fb_read,
> 	.write =	fb_write,
> 	.unlocked_ioctl = fb_ioctl,
> 	.compat_ioctl = fb_compat_ioctl,
> 	.mmap =		fb_mmap,
> 	.open =		fb_open,
> 	.release =	fb_release,
> 	.get_unmapped_area = get_fb_unmapped_area,
> 	.fsync =	fb_deferred_io_fsync,
> };
> 
> Not possible after unregister:
> fb_open() -> fb_ops.fb_open
> 
> Protected by file_fb_info() (-ENODEV):
> fb_read() -> fb_ops.fb_read : drm_fb_helper_sys_read()
> fb_write() -> fb_ops.fb_write : drm_fb_helper_sys_write()
> fb_ioctl() -> fb_ops.fb_ioctl : drm_fb_helper_ioctl()
> fb_compat_ioctl() -> fb_ops.fb_compat_ioctl
> fb_mmap() -> fb_ops.fb_mmap
> 
> Safe:
> fb_release() -> fb_ops.fb_release
> get_fb_unmapped_area() : info->screen_base
> fb_deferred_io_fsync() : if (info->fbdefio) &info->deferred_work
> 
> Active mmap's will need the backing buffer to be present.
> If deferred IO is used, mmap writes will via a worker generate calls to
> drm_fb_helper_deferred_io() which in turn via a worker calls into
> drm_fb_helper_dirty_work().
> 
> Secondly we look at the remaining struct fb_ops operations:
> 
> Called by fbcon:
> - fb_ops.fb_fillrect : drm_fb_helper_{sys,cfb}_fillrect()
> - fb_ops.fb_copyarea : drm_fb_helper_{sys,cfb}_copyarea()
> - fb_ops.fb_imageblit : drm_fb_helper_{sys,cfb}_imageblit()
> 
> Called in fb_set_var() which is called from ioctl, sysfs and fbcon:
> - fb_ops.fb_check_var : drm_fb_helper_check_var()
> - fb_ops.fb_set_par : drm_fb_helper_set_par()
> drm_fb_helper_set_par() is also called from drm_fb_helper_hotplug_event().
> 
> Called in fb_set_cmap() which is called from fb_set_var(), ioctl
> and fbcon:
> - fb_ops.fb_setcolreg
> - fb_ops.fb_setcmap : drm_fb_helper_setcmap()
> 
> Called in fb_blank() which is called from ioctl and fbcon:
> - fb_ops.fb_blank : drm_fb_helper_blank()
> 
> Called in fb_pan_display() which is called from fb_set_var(), ioctl,
> sysfs and fbcon:
> - fb_ops.fb_pan_display : drm_fb_helper_pan_display()
> 
> Called by fbcon:
> - fb_ops.fb_cursor
> 
> Called in fb_read(), fb_write(), and fb_get_buffer_offset() which is
> called by fbcon:
> - fb_ops.fb_sync
> 
> Called by fb_set_var():
> - fb_ops.fb_get_caps
> 
> Called by fbcon:
> - fb_ops.fb_debug_enter : drm_fb_helper_debug_enter()
> - fb_ops.fb_debug_leave : drm_fb_helper_debug_leave()
> 
> Destroy is safe
> - fb_ops.fb_destroy
> 
> Finally we look at other call paths:
> 
> drm_fb_helper_set_suspend{_unlocked}() and
> drm_fb_helper_resume_worker():
> Calls into fb_set_suspend(), but that's fine since it just uses the
> fbdev notifier.
> 
> drm_fb_helper_restore_fbdev_mode_unlocked():
> Called from drm_driver.last_close, possibly after drm_fb_helper_fini().
> 
> drm_fb_helper_force_kernel_mode():
> Triggered by sysrq, possibly after unplug, but before
> drm_fb_helper_fini().
> 
> drm_fb_helper_hotplug_event():
> Called by drm_kms_helper_hotplug_event(). I don't know if this can be
> called after drm_dev_unregister(), so add a check to be safe.
> 
> Based on this analysis the following functions get a
> drm_dev_is_unplugged() check:
> - drm_fb_helper_restore_fbdev_mode_unlocked()
> - drm_fb_helper_force_kernel_mode()
> - drm_fb_helper_hotplug_event()
> - drm_fb_helper_dirty_work()
> 
> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>

You're mixing in the new simple fbdev helper helpers as a bonus, that's
two things in one patch and makes it harder to review what's really going
on.

My gut feeling is that when we need a helper for the helper the original
helper needs to be improved, but I think that's much easier to decide when
it's a separate patch. Splitting things out would definitely make this
patch much smaller and easier to understand and review.

The only reason for the simple helpers that I could find is the
drm_dev_ref/unref, we should be able to do that in one of the existing
callbacks.

> ---
>  drivers/gpu/drm/drm_fb_helper.c | 204 +++++++++++++++++++++++++++++++++++++++-
>  include/drm/drm_fb_helper.h     |  35 +++++++
>  2 files changed, 237 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 2e33467..fc898db 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -105,6 +105,158 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
>   * mmap page writes.
>   */
>  
> +/**
> + * drm_fb_helper_simple_init - Simple fbdev emulation initialization
> + * @dev: drm device
> + * @fb_helper: driver-allocated fbdev helper structure to initialize
> + * @bpp_sel: bpp value to use for the framebuffer configuration
> + * @max_conn_count: max connector count
> + * @funcs: pointer to structure of functions associate with this helper
> + *
> + * Simple fbdev emulation initialization which calls the following functions:
> + * drm_fb_helper_prepare(), drm_fb_helper_init(),
> + * drm_fb_helper_single_add_all_connectors() and
> + * drm_fb_helper_initial_config().
> + *
> + * This function takes a ref on &drm_device and must be used together with
> + * drm_fb_helper_simple_fini() or drm_fb_helper_dev_unplug().
> + *
> + * fbdev deferred I/O users must use drm_fb_helper_defio_init().
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + */
> +int drm_fb_helper_simple_init(struct drm_device *dev,
> +			      struct drm_fb_helper *fb_helper, int bpp_sel,
> +			      int max_conn_count,
> +			      const struct drm_fb_helper_funcs *funcs)
> +{
> +	int ret;
> +
> +	drm_fb_helper_prepare(dev, fb_helper, funcs);
> +
> +	ret = drm_fb_helper_init(dev, fb_helper, max_conn_count);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(dev->dev, "Failed to initialize fb helper.\n");
> +		return ret;
> +	}
> +
> +	drm_dev_ref(dev);
> +
> +	ret = drm_fb_helper_single_add_all_connectors(fb_helper);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(dev->dev, "Failed to add connectors.\n");
> +		goto err_drm_fb_helper_fini;
> +
> +	}
> +
> +	ret = drm_fb_helper_initial_config(fb_helper, bpp_sel);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(dev->dev, "Failed to set initial hw config.\n");
> +		goto err_drm_fb_helper_fini;
> +	}
> +
> +	return 0;
> +
> +err_drm_fb_helper_fini:
> +	drm_fb_helper_fini(fb_helper);
> +	drm_dev_unref(dev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(drm_fb_helper_simple_init);
> +
> +static void drm_fb_helper_simple_fini_cleanup(struct drm_fb_helper *fb_helper)
> +{
> +	struct fb_info *info = fb_helper->fbdev;
> +	struct fb_ops *fbops = NULL;
> +
> +	if (info && info->fbdefio) {
> +		fb_deferred_io_cleanup(info);
> +		kfree(info->fbdefio);
> +		info->fbdefio = NULL;
> +		fbops = info->fbops;
> +	}
> +
> +	drm_fb_helper_fini(fb_helper);
> +	kfree(fbops);
> +	if (fb_helper->fb)
> +		drm_framebuffer_remove(fb_helper->fb);
> +	drm_dev_unref(fb_helper->dev);
> +}
> +
> +/**
> + * drm_fb_helper_simple_fini - Simple fbdev cleanup
> + * @fb_helper: fbdev emulation structure, can be NULL
> + *
> + * Simple fbdev emulation cleanup. This function unregisters fbdev, cleans up
> + * deferred I/O if necessary, finalises @fb_helper and removes the framebuffer.
> + * The driver if responsible for freeing the @fb_helper structure.
> + *
> + * Don't use this function if you use drm_fb_helper_dev_unplug().
> + */
> +void drm_fb_helper_simple_fini(struct drm_fb_helper *fb_helper)
> +{
> +	struct fb_info *info;
> +
> +	if (!fb_helper)
> +		return;
> +
> +	info = fb_helper->fbdev;
> +
> +	/* Has drm_fb_helper_dev_unplug() been used? */
> +	if (info && info->dev)
> +		drm_fb_helper_unregister_fbi(fb_helper);
> +
> +	if (!(info && info->fbops && info->fbops->fb_destroy))
> +		drm_fb_helper_simple_fini_cleanup(fb_helper);
> +}
> +EXPORT_SYMBOL_GPL(drm_fb_helper_simple_fini);
> +
> +/**
> + * drm_fb_helper_dev_unplug - unplug an fbdev device
> + * @fb_helper: driver-allocated fbdev helper, can be NULL
> + *
> + * This unplugs the fbdev emulation for a hotpluggable DRM device, which makes
> + * fbdev inaccessible to userspace operations. This essentially unregisters
> + * fbdev and can be called while there are still open users of @fb_helper.
> + * Entry-points from fbdev into drm core/helpers are protected by the fbdev
> + * &fb_info ref count and drm_dev_is_unplugged(). This means that the driver
> + * also has to call drm_dev_unplug() to complete the unplugging.
> + *
> + * Drivers must use drm_fb_helper_fb_destroy() as their &fb_ops.fb_destroy
> + * callback and call drm_mode_config_cleanup() and free @fb_helper in their
> + * &drm_driver->release callback.
> + *
> + * @fb_helper is finalized by this function unless there are open fbdev fd's
> + * in case this is postponed to the closing of the last fd. Finalizing includes
> + * dropping the ref taken on &drm_device in drm_fb_helper_simple_init().
> + */
> +void drm_fb_helper_dev_unplug(struct drm_fb_helper *fb_helper)
> +{
> +	drm_fb_helper_unregister_fbi(fb_helper);

I don't see the value of this wrapper. If you want to explain how to best
unplug the fbdev emulation I thinkt that's better done in some dedicated
DOC section (referenced by drm_dev_unplug() maybe), than by providing a
wrapper that only gives you a different name.

_unplug is also a bit misleading, since _unplug is about yanking the
backing device. This just unregisters the the fbdev interface.

> +}
> +EXPORT_SYMBOL(drm_fb_helper_dev_unplug);
> +
> +/**
> + * drm_fb_helper_fb_destroy - implementation for &fb_ops.fb_destroy
> + * @info: fbdev registered by the helper
> + *
> + * This function does the same as drm_fb_helper_simple_fini() except
> + * unregistering fbdev which is already done.
> + *
> + * &fb_ops.fb_destroy is called during unregister_framebuffer() or the last
> + * fb_release() which ever comes last.
> + */
> +void drm_fb_helper_fb_destroy(struct fb_info *info)
> +{
> +	struct drm_fb_helper *helper = info->par;
> +
> +	DRM_DEBUG("\n");
> +	drm_fb_helper_simple_fini_cleanup(helper);
> +}
> +EXPORT_SYMBOL(drm_fb_helper_fb_destroy);
> +
>  #define drm_fb_helper_for_each_connector(fbh, i__) \
>  	for (({ lockdep_assert_held(&(fbh)->lock); }), \
>  	     i__ = 0; i__ < (fbh)->connector_count; i__++)
> @@ -498,7 +650,7 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
>  	bool do_delayed;
>  	int ret;
>  
> -	if (!drm_fbdev_emulation)
> +	if (!drm_fbdev_emulation || drm_dev_is_unplugged(fb_helper->dev))
>  		return -ENODEV;
>  
>  	if (READ_ONCE(fb_helper->deferred_setup))
> @@ -563,6 +715,9 @@ static bool drm_fb_helper_force_kernel_mode(void)
>  	list_for_each_entry(helper, &kernel_fb_helper_list, kernel_fb_list) {
>  		struct drm_device *dev = helper->dev;
>  
> +		if (drm_dev_is_unplugged(dev))
> +			continue;
> +
>  		if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
>  			continue;
>  
> @@ -735,6 +890,9 @@ static void drm_fb_helper_dirty_work(struct work_struct *work)
>  	struct drm_clip_rect clip_copy;
>  	unsigned long flags;
>  
> +	if (drm_dev_is_unplugged(helper->dev))
> +		return;
> +
>  	spin_lock_irqsave(&helper->dirty_lock, flags);
>  	clip_copy = *clip;
>  	clip->x1 = clip->y1 = ~0;
> @@ -949,6 +1107,48 @@ void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper)
>  }
>  EXPORT_SYMBOL(drm_fb_helper_unlink_fbi);
>  
> +/**
> + * drm_fb_helper_defio_init - fbdev deferred I/O initialization
> + * @fb_helper: driver-allocated fbdev helper
> + *
> + * This function allocates &fb_deferred_io, sets callback to
> + * drm_fb_helper_deferred_io(), delay to 50ms and calls fb_deferred_io_init().
> + *
> + * NOTE: A copy of &fb_ops is made and assigned to &info->fbops. This is done
> + * because fb_deferred_io_cleanup() clears &fbops->fb_mmap and would thereby
> + * affect other instances of that &fb_ops. This copy is freed by the helper
> + * during cleanup.
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + */
> +int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper)
> +{
> +	struct fb_info *info = fb_helper->fbdev;
> +	struct fb_deferred_io *fbdefio;
> +	struct fb_ops *fbops;
> +
> +	fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
> +	fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
> +	if (!fbdefio || !fbops) {
> +		kfree(fbdefio);
> +		kfree(fbops);
> +		return -ENOMEM;
> +	}
> +
> +	info->fbdefio = fbdefio;
> +	fbdefio->delay = msecs_to_jiffies(50);
> +	fbdefio->deferred_io = drm_fb_helper_deferred_io;
> +
> +	*fbops = *info->fbops;
> +	info->fbops = fbops;
> +
> +	fb_deferred_io_init(info);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_fb_helper_defio_init);
> +
>  static void drm_fb_helper_dirty(struct fb_info *info, u32 x, u32 y,
>  				u32 width, u32 height)
>  {
> @@ -2591,7 +2791,7 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
>  {
>  	int err = 0;
>  
> -	if (!drm_fbdev_emulation)
> +	if (!drm_fbdev_emulation || drm_dev_is_unplugged(fb_helper->dev))
>  		return 0;
>  
>  	mutex_lock(&fb_helper->lock);
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 33fe959..1c5a648 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -242,6 +242,14 @@ struct drm_fb_helper {
>  	.fb_ioctl	= drm_fb_helper_ioctl
>  
>  #ifdef CONFIG_DRM_FBDEV_EMULATION
> +int drm_fb_helper_simple_init(struct drm_device *dev,
> +			      struct drm_fb_helper *fb_helper, int bpp_sel,
> +			      int max_conn_count,
> +			      const struct drm_fb_helper_funcs *funcs);
> +void drm_fb_helper_simple_fini(struct drm_fb_helper *fb_helper);
> +void drm_fb_helper_dev_unplug(struct drm_fb_helper *fb_helper);
> +void drm_fb_helper_fb_destroy(struct fb_info *info);
> +
>  void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper,
>  			   const struct drm_fb_helper_funcs *funcs);
>  int drm_fb_helper_init(struct drm_device *dev,
> @@ -265,6 +273,7 @@ void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
>  
>  void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper);
>  
> +int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper);
>  void drm_fb_helper_deferred_io(struct fb_info *info,
>  			       struct list_head *pagelist);
>  
> @@ -311,6 +320,27 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, struct drm_
>  int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
>  				       struct drm_connector *connector);
>  #else
> +static inline int
> +drm_fb_helper_simple_init(struct drm_device *dev,
> +			  struct drm_fb_helper *fb_helper, int bpp_sel,
> +			  int max_conn_count,
> +			  const struct drm_fb_helper_funcs *funcs)
> +{
> +	return 0;
> +}
> +
> +static inline void drm_fb_helper_simple_fini(struct drm_fb_helper *fb_helper)
> +{
> +}
> +
> +static inline void drm_fb_helper_dev_unplug(struct drm_fb_helper *fb_helper)
> +{
> +}
> +
> +static inline void drm_fb_helper_fb_destroy(struct fb_info *info)
> +{
> +}
> +
>  static inline void drm_fb_helper_prepare(struct drm_device *dev,
>  					struct drm_fb_helper *helper,
>  					const struct drm_fb_helper_funcs *funcs)
> @@ -393,6 +423,11 @@ static inline void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper)
>  {
>  }
>  
> +static inline int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper)
> +{
> +	return 0;
> +}
> +
>  static inline void drm_fb_helper_deferred_io(struct fb_info *info,
>  					     struct list_head *pagelist)
>  {
> -- 
> 2.7.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch