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

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@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