| Message ID | 1503940668-25883-3-git-send-email-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
Den 28.08.2017 23.41, skrev 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@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. The reason I did that is because I couldn't put it in the existing helpers since the framebuffer is removed after drm_fb_helper_fini() and I can't drop the ref on drm_device before the framebuffer is gone. I can split the patch in two: 1. Add drm_dev_is_unplugged() checks 2. Add drm_fb_helper_simple_init/fini() and drm_fb_helper_fb_destroy() Maybe there's a way to remove the framebuffer in drm_fb_helper_fini() so we can drop the ref there, I don't know. Here's how drivers destroy fbdev: static int amdgpu_fbdev_destroy(struct drm_device *dev, struct amdgpu_fbdev *rfbdev) { struct amdgpu_framebuffer *rfb = &rfbdev->rfb; drm_fb_helper_unregister_fbi(&rfbdev->helper); if (rfb->obj) { amdgpufb_destroy_pinned_object(rfb->obj); rfb->obj = NULL; } drm_fb_helper_fini(&rfbdev->helper); drm_framebuffer_unregister_private(&rfb->base); drm_framebuffer_cleanup(&rfb->base); return 0; } void armada_fbdev_fini(struct drm_device *dev) { struct armada_private *priv = dev->dev_private; struct drm_fb_helper *fbh = priv->fbdev; if (fbh) { drm_fb_helper_unregister_fbi(fbh); drm_fb_helper_fini(fbh); if (fbh->fb) fbh->fb->funcs->destroy(fbh->fb); priv->fbdev = NULL; } } static void ast_fbdev_destroy(struct drm_device *dev, struct ast_fbdev *afbdev) { struct ast_framebuffer *afb = &afbdev->afb; drm_fb_helper_unregister_fbi(&afbdev->helper); if (afb->obj) { drm_gem_object_unreference_unlocked(afb->obj); afb->obj = NULL; } drm_fb_helper_fini(&afbdev->helper); vfree(afbdev->sysram); drm_framebuffer_unregister_private(&afb->base); drm_framebuffer_cleanup(&afb->base); } static int bochs_fbdev_destroy(struct bochs_device *bochs) { struct bochs_framebuffer *gfb = &bochs->fb.gfb; DRM_DEBUG_DRIVER("\n"); drm_fb_helper_unregister_fbi(&bochs->fb.helper); if (gfb->obj) { drm_gem_object_unreference_unlocked(gfb->obj); gfb->obj = NULL; } drm_framebuffer_unregister_private(&gfb->base); drm_framebuffer_cleanup(&gfb->base); return 0; } static int cirrus_fbdev_destroy(struct drm_device *dev, struct cirrus_fbdev *gfbdev) { struct cirrus_framebuffer *gfb = &gfbdev->gfb; drm_fb_helper_unregister_fbi(&gfbdev->helper); if (gfb->obj) { drm_gem_object_unreference_unlocked(gfb->obj); gfb->obj = NULL; } vfree(gfbdev->sysram); drm_fb_helper_fini(&gfbdev->helper); drm_framebuffer_unregister_private(&gfb->base); drm_framebuffer_cleanup(&gfb->base); return 0; } void drm_fbdev_cma_fini(struct drm_fbdev_cma *fbdev_cma) { drm_fb_helper_unregister_fbi(&fbdev_cma->fb_helper); if (fbdev_cma->fb_helper.fbdev) drm_fbdev_cma_defio_fini(fbdev_cma->fb_helper.fbdev); if (fbdev_cma->fb) drm_framebuffer_remove(&fbdev_cma->fb->fb); drm_fb_helper_fini(&fbdev_cma->fb_helper); kfree(fbdev_cma); } static void exynos_drm_fbdev_destroy(struct drm_device *dev, struct drm_fb_helper *fb_helper) { struct exynos_drm_fbdev *exynos_fbd = to_exynos_fbdev(fb_helper); struct exynos_drm_gem *exynos_gem = exynos_fbd->exynos_gem; struct drm_framebuffer *fb; vunmap(exynos_gem->kvaddr); /* release drm framebuffer and real buffer */ if (fb_helper->fb && fb_helper->fb->funcs) { fb = fb_helper->fb; if (fb) drm_framebuffer_remove(fb); } drm_fb_helper_unregister_fbi(fb_helper); drm_fb_helper_fini(fb_helper); } static int psb_fbdev_destroy(struct drm_device *dev, struct psb_fbdev *fbdev) { struct psb_framebuffer *psbfb = &fbdev->pfb; drm_fb_helper_unregister_fbi(&fbdev->psb_fb_helper); drm_fb_helper_fini(&fbdev->psb_fb_helper); drm_framebuffer_unregister_private(&psbfb->base); drm_framebuffer_cleanup(&psbfb->base); if (psbfb->gtt) drm_gem_object_unreference_unlocked(&psbfb->gtt->gem); return 0; } static void hibmc_fbdev_destroy(struct hibmc_fbdev *fbdev) { struct hibmc_framebuffer *gfb = fbdev->fb; struct drm_fb_helper *fbh = &fbdev->helper; drm_fb_helper_unregister_fbi(fbh); drm_fb_helper_fini(fbh); if (gfb) drm_framebuffer_unreference(&gfb->fb); } static void intel_fbdev_destroy(struct intel_fbdev *ifbdev) { /* We rely on the object-free to release the VMA pinning for * the info->screen_base mmaping. Leaking the VMA is simpler than * trying to rectify all the possible error paths leading here. */ drm_fb_helper_unregister_fbi(&ifbdev->helper); drm_fb_helper_fini(&ifbdev->helper); if (ifbdev->vma) { mutex_lock(&ifbdev->helper.dev->struct_mutex); intel_unpin_fb_vma(ifbdev->vma); mutex_unlock(&ifbdev->helper.dev->struct_mutex); } if (ifbdev->fb) drm_framebuffer_remove(&ifbdev->fb->base); kfree(ifbdev); } static int mga_fbdev_destroy(struct drm_device *dev, struct mga_fbdev *mfbdev) { struct mga_framebuffer *mfb = &mfbdev->mfb; drm_fb_helper_unregister_fbi(&mfbdev->helper); if (mfb->obj) { drm_gem_object_unreference_unlocked(mfb->obj); mfb->obj = NULL; } drm_fb_helper_fini(&mfbdev->helper); vfree(mfbdev->sysram); drm_framebuffer_unregister_private(&mfb->base); drm_framebuffer_cleanup(&mfb->base); return 0; } void msm_fbdev_free(struct drm_device *dev) { struct msm_drm_private *priv = dev->dev_private; struct drm_fb_helper *helper = priv->fbdev; struct msm_fbdev *fbdev; DBG(); drm_fb_helper_unregister_fbi(helper); drm_fb_helper_fini(helper); fbdev = to_msm_fbdev(priv->fbdev); /* this will free the backing object */ if (fbdev->fb) { msm_gem_put_vaddr(fbdev->bo); drm_framebuffer_remove(fbdev->fb); } kfree(fbdev); priv->fbdev = NULL; } static int nouveau_fbcon_destroy(struct drm_device *dev, struct nouveau_fbdev *fbcon) { struct nouveau_framebuffer *nouveau_fb = nouveau_framebuffer(fbcon->helper.fb); drm_fb_helper_unregister_fbi(&fbcon->helper); drm_fb_helper_fini(&fbcon->helper); if (nouveau_fb->nvbo) { nouveau_bo_vma_del(nouveau_fb->nvbo, &nouveau_fb->vma); nouveau_bo_unmap(nouveau_fb->nvbo); nouveau_bo_unpin(nouveau_fb->nvbo); drm_framebuffer_unreference(&nouveau_fb->base); } return 0; } void omap_fbdev_free(struct drm_device *dev) { struct omap_drm_private *priv = dev->dev_private; struct drm_fb_helper *helper = priv->fbdev; struct omap_fbdev *fbdev; DBG(); drm_fb_helper_unregister_fbi(helper); drm_fb_helper_fini(helper); fbdev = to_omap_fbdev(priv->fbdev); /* release the ref taken in omap_fbdev_create() */ omap_gem_put_paddr(fbdev->bo); /* this will free the backing object */ if (fbdev->fb) drm_framebuffer_remove(fbdev->fb); kfree(fbdev); priv->fbdev = NULL; } static int qxl_fbdev_destroy(struct drm_device *dev, struct qxl_fbdev *qfbdev) { struct qxl_framebuffer *qfb = &qfbdev->qfb; drm_fb_helper_unregister_fbi(&qfbdev->helper); if (qfb->obj) { qxlfb_destroy_pinned_object(qfb->obj); qfb->obj = NULL; } drm_fb_helper_fini(&qfbdev->helper); vfree(qfbdev->shadow); drm_framebuffer_cleanup(&qfb->base); return 0; } static int radeon_fbdev_destroy(struct drm_device *dev, struct radeon_fbdev *rfbdev) { struct radeon_framebuffer *rfb = &rfbdev->rfb; drm_fb_helper_unregister_fbi(&rfbdev->helper); if (rfb->obj) { radeonfb_destroy_pinned_object(rfb->obj); rfb->obj = NULL; } drm_fb_helper_fini(&rfbdev->helper); drm_framebuffer_unregister_private(&rfb->base); drm_framebuffer_cleanup(&rfb->base); return 0; } void rockchip_drm_fbdev_fini(struct drm_device *dev) { struct rockchip_drm_private *private = dev->dev_private; struct drm_fb_helper *helper; helper = &private->fbdev_helper; drm_fb_helper_unregister_fbi(helper); if (helper->fb) drm_framebuffer_unreference(helper->fb); drm_fb_helper_fini(helper); } static void tegra_fbdev_exit(struct tegra_fbdev *fbdev) { drm_fb_helper_unregister_fbi(&fbdev->base); if (fbdev->fb) drm_framebuffer_remove(&fbdev->fb->base); drm_fb_helper_fini(&fbdev->base); tegra_fbdev_free(fbdev); } static void udl_fbdev_destroy(struct drm_device *dev, struct udl_fbdev *ufbdev) { drm_fb_helper_unregister_fbi(&ufbdev->helper); drm_fb_helper_fini(&ufbdev->helper); drm_framebuffer_unregister_private(&ufbdev->ufb.base); drm_framebuffer_cleanup(&ufbdev->ufb.base); drm_gem_object_unreference_unlocked(&ufbdev->ufb.obj->base); } static int virtio_gpu_fbdev_destroy(struct drm_device *dev, struct virtio_gpu_fbdev *vgfbdev) { struct virtio_gpu_framebuffer *vgfb = &vgfbdev->vgfb; drm_fb_helper_unregister_fbi(&vgfbdev->helper); if (vgfb->obj) vgfb->obj = NULL; drm_fb_helper_fini(&vgfbdev->helper); drm_framebuffer_cleanup(&vgfb->base); return 0; } Noralf. >> --- >> 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 >>
On Tue, Aug 29, 2017 at 06:17:25PM +0200, Noralf Trønnes wrote: > > Den 28.08.2017 23.41, skrev 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@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. > > The reason I did that is because I couldn't put it in the existing > helpers since the framebuffer is removed after drm_fb_helper_fini() and > I can't drop the ref on drm_device before the framebuffer is gone. Why is that impossible? I'd have assumed that we'd first drop all the drm_device refcounts before we do any of the cleanup steps. > I can split the patch in two: > 1. Add drm_dev_is_unplugged() checks > 2. Add drm_fb_helper_simple_init/fini() and drm_fb_helper_fb_destroy() > > Maybe there's a way to remove the framebuffer in drm_fb_helper_fini() > so we can drop the ref there, I don't know. No, we can't do that since the references that get dropped in the driver code is the driver code's private reference. The problem is that a bunch of drivers still embed the framebuffer for the fbdev emulation into a larger structure, which means the reference counting is obviously busted. But all reasonable modern drivers should do that, and if we have to convert some to do proper unplug behaviour - well that's expected anyway because the current ->unload hook doesn't work for unplugging. -Daniel > Here's how drivers destroy fbdev: > > > static int amdgpu_fbdev_destroy(struct drm_device *dev, struct amdgpu_fbdev > *rfbdev) > { > struct amdgpu_framebuffer *rfb = &rfbdev->rfb; > > drm_fb_helper_unregister_fbi(&rfbdev->helper); > > if (rfb->obj) { > amdgpufb_destroy_pinned_object(rfb->obj); > rfb->obj = NULL; > } > drm_fb_helper_fini(&rfbdev->helper); > drm_framebuffer_unregister_private(&rfb->base); > drm_framebuffer_cleanup(&rfb->base); > > return 0; > } > > void armada_fbdev_fini(struct drm_device *dev) > { > struct armada_private *priv = dev->dev_private; > struct drm_fb_helper *fbh = priv->fbdev; > > if (fbh) { > drm_fb_helper_unregister_fbi(fbh); > > drm_fb_helper_fini(fbh); > > if (fbh->fb) > fbh->fb->funcs->destroy(fbh->fb); > > priv->fbdev = NULL; > } > } > > static void ast_fbdev_destroy(struct drm_device *dev, > struct ast_fbdev *afbdev) > { > struct ast_framebuffer *afb = &afbdev->afb; > > drm_fb_helper_unregister_fbi(&afbdev->helper); > > if (afb->obj) { > drm_gem_object_unreference_unlocked(afb->obj); > afb->obj = NULL; > } > drm_fb_helper_fini(&afbdev->helper); > > vfree(afbdev->sysram); > drm_framebuffer_unregister_private(&afb->base); > drm_framebuffer_cleanup(&afb->base); > } > > static int bochs_fbdev_destroy(struct bochs_device *bochs) > { > struct bochs_framebuffer *gfb = &bochs->fb.gfb; > > DRM_DEBUG_DRIVER("\n"); > > drm_fb_helper_unregister_fbi(&bochs->fb.helper); > > if (gfb->obj) { > drm_gem_object_unreference_unlocked(gfb->obj); > gfb->obj = NULL; > } > > drm_framebuffer_unregister_private(&gfb->base); > drm_framebuffer_cleanup(&gfb->base); > > return 0; > } > > static int cirrus_fbdev_destroy(struct drm_device *dev, > struct cirrus_fbdev *gfbdev) > { > struct cirrus_framebuffer *gfb = &gfbdev->gfb; > > drm_fb_helper_unregister_fbi(&gfbdev->helper); > > if (gfb->obj) { > drm_gem_object_unreference_unlocked(gfb->obj); > gfb->obj = NULL; > } > > vfree(gfbdev->sysram); > drm_fb_helper_fini(&gfbdev->helper); > drm_framebuffer_unregister_private(&gfb->base); > drm_framebuffer_cleanup(&gfb->base); > > return 0; > } > > void drm_fbdev_cma_fini(struct drm_fbdev_cma *fbdev_cma) > { > drm_fb_helper_unregister_fbi(&fbdev_cma->fb_helper); > if (fbdev_cma->fb_helper.fbdev) > drm_fbdev_cma_defio_fini(fbdev_cma->fb_helper.fbdev); > > if (fbdev_cma->fb) > drm_framebuffer_remove(&fbdev_cma->fb->fb); > > drm_fb_helper_fini(&fbdev_cma->fb_helper); > kfree(fbdev_cma); > } > > static void exynos_drm_fbdev_destroy(struct drm_device *dev, > struct drm_fb_helper *fb_helper) > { > struct exynos_drm_fbdev *exynos_fbd = to_exynos_fbdev(fb_helper); > struct exynos_drm_gem *exynos_gem = exynos_fbd->exynos_gem; > struct drm_framebuffer *fb; > > vunmap(exynos_gem->kvaddr); > > /* release drm framebuffer and real buffer */ > if (fb_helper->fb && fb_helper->fb->funcs) { > fb = fb_helper->fb; > if (fb) > drm_framebuffer_remove(fb); > } > > drm_fb_helper_unregister_fbi(fb_helper); > > drm_fb_helper_fini(fb_helper); > } > > static int psb_fbdev_destroy(struct drm_device *dev, struct psb_fbdev > *fbdev) > { > struct psb_framebuffer *psbfb = &fbdev->pfb; > > drm_fb_helper_unregister_fbi(&fbdev->psb_fb_helper); > > drm_fb_helper_fini(&fbdev->psb_fb_helper); > drm_framebuffer_unregister_private(&psbfb->base); > drm_framebuffer_cleanup(&psbfb->base); > > if (psbfb->gtt) > drm_gem_object_unreference_unlocked(&psbfb->gtt->gem); > return 0; > } > > static void hibmc_fbdev_destroy(struct hibmc_fbdev *fbdev) > { > struct hibmc_framebuffer *gfb = fbdev->fb; > struct drm_fb_helper *fbh = &fbdev->helper; > > drm_fb_helper_unregister_fbi(fbh); > > drm_fb_helper_fini(fbh); > > if (gfb) > drm_framebuffer_unreference(&gfb->fb); > } > > static void intel_fbdev_destroy(struct intel_fbdev *ifbdev) > { > /* We rely on the object-free to release the VMA pinning for > * the info->screen_base mmaping. Leaking the VMA is simpler than > * trying to rectify all the possible error paths leading here. > */ > > drm_fb_helper_unregister_fbi(&ifbdev->helper); > > drm_fb_helper_fini(&ifbdev->helper); > > if (ifbdev->vma) { > mutex_lock(&ifbdev->helper.dev->struct_mutex); > intel_unpin_fb_vma(ifbdev->vma); > mutex_unlock(&ifbdev->helper.dev->struct_mutex); > } > > if (ifbdev->fb) > drm_framebuffer_remove(&ifbdev->fb->base); > > kfree(ifbdev); > } > > static int mga_fbdev_destroy(struct drm_device *dev, > struct mga_fbdev *mfbdev) > { > struct mga_framebuffer *mfb = &mfbdev->mfb; > > drm_fb_helper_unregister_fbi(&mfbdev->helper); > > if (mfb->obj) { > drm_gem_object_unreference_unlocked(mfb->obj); > mfb->obj = NULL; > } > drm_fb_helper_fini(&mfbdev->helper); > vfree(mfbdev->sysram); > drm_framebuffer_unregister_private(&mfb->base); > drm_framebuffer_cleanup(&mfb->base); > > return 0; > } > > void msm_fbdev_free(struct drm_device *dev) > { > struct msm_drm_private *priv = dev->dev_private; > struct drm_fb_helper *helper = priv->fbdev; > struct msm_fbdev *fbdev; > > DBG(); > > drm_fb_helper_unregister_fbi(helper); > > drm_fb_helper_fini(helper); > > fbdev = to_msm_fbdev(priv->fbdev); > > /* this will free the backing object */ > if (fbdev->fb) { > msm_gem_put_vaddr(fbdev->bo); > drm_framebuffer_remove(fbdev->fb); > } > > kfree(fbdev); > > priv->fbdev = NULL; > } > > static int > nouveau_fbcon_destroy(struct drm_device *dev, struct nouveau_fbdev *fbcon) > { > struct nouveau_framebuffer *nouveau_fb = > nouveau_framebuffer(fbcon->helper.fb); > > drm_fb_helper_unregister_fbi(&fbcon->helper); > drm_fb_helper_fini(&fbcon->helper); > > if (nouveau_fb->nvbo) { > nouveau_bo_vma_del(nouveau_fb->nvbo, &nouveau_fb->vma); > nouveau_bo_unmap(nouveau_fb->nvbo); > nouveau_bo_unpin(nouveau_fb->nvbo); > drm_framebuffer_unreference(&nouveau_fb->base); > } > > return 0; > } > > void omap_fbdev_free(struct drm_device *dev) > { > struct omap_drm_private *priv = dev->dev_private; > struct drm_fb_helper *helper = priv->fbdev; > struct omap_fbdev *fbdev; > > DBG(); > > drm_fb_helper_unregister_fbi(helper); > > drm_fb_helper_fini(helper); > > fbdev = to_omap_fbdev(priv->fbdev); > > /* release the ref taken in omap_fbdev_create() */ > omap_gem_put_paddr(fbdev->bo); > > /* this will free the backing object */ > if (fbdev->fb) > drm_framebuffer_remove(fbdev->fb); > > kfree(fbdev); > > priv->fbdev = NULL; > } > > static int qxl_fbdev_destroy(struct drm_device *dev, struct qxl_fbdev > *qfbdev) > { > struct qxl_framebuffer *qfb = &qfbdev->qfb; > > drm_fb_helper_unregister_fbi(&qfbdev->helper); > > if (qfb->obj) { > qxlfb_destroy_pinned_object(qfb->obj); > qfb->obj = NULL; > } > drm_fb_helper_fini(&qfbdev->helper); > vfree(qfbdev->shadow); > drm_framebuffer_cleanup(&qfb->base); > > return 0; > } > > static int radeon_fbdev_destroy(struct drm_device *dev, struct radeon_fbdev > *rfbdev) > { > struct radeon_framebuffer *rfb = &rfbdev->rfb; > > drm_fb_helper_unregister_fbi(&rfbdev->helper); > > if (rfb->obj) { > radeonfb_destroy_pinned_object(rfb->obj); > rfb->obj = NULL; > } > drm_fb_helper_fini(&rfbdev->helper); > drm_framebuffer_unregister_private(&rfb->base); > drm_framebuffer_cleanup(&rfb->base); > > return 0; > } > > void rockchip_drm_fbdev_fini(struct drm_device *dev) > { > struct rockchip_drm_private *private = dev->dev_private; > struct drm_fb_helper *helper; > > helper = &private->fbdev_helper; > > drm_fb_helper_unregister_fbi(helper); > > if (helper->fb) > drm_framebuffer_unreference(helper->fb); > > drm_fb_helper_fini(helper); > } > > static void tegra_fbdev_exit(struct tegra_fbdev *fbdev) > { > drm_fb_helper_unregister_fbi(&fbdev->base); > > if (fbdev->fb) > drm_framebuffer_remove(&fbdev->fb->base); > > drm_fb_helper_fini(&fbdev->base); > tegra_fbdev_free(fbdev); > } > > static void udl_fbdev_destroy(struct drm_device *dev, > struct udl_fbdev *ufbdev) > { > drm_fb_helper_unregister_fbi(&ufbdev->helper); > drm_fb_helper_fini(&ufbdev->helper); > drm_framebuffer_unregister_private(&ufbdev->ufb.base); > drm_framebuffer_cleanup(&ufbdev->ufb.base); > drm_gem_object_unreference_unlocked(&ufbdev->ufb.obj->base); > } > > static int virtio_gpu_fbdev_destroy(struct drm_device *dev, > struct virtio_gpu_fbdev *vgfbdev) > { > struct virtio_gpu_framebuffer *vgfb = &vgfbdev->vgfb; > > drm_fb_helper_unregister_fbi(&vgfbdev->helper); > > if (vgfb->obj) > vgfb->obj = NULL; > drm_fb_helper_fini(&vgfbdev->helper); > drm_framebuffer_cleanup(&vgfb->base); > > return 0; > } > > > Noralf. > > > > --- > > > 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 > > > >
Den 30.08.2017 09.29, skrev Daniel Vetter: > On Tue, Aug 29, 2017 at 06:17:25PM +0200, Noralf Trønnes wrote: >> Den 28.08.2017 23.41, skrev 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@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. >> The reason I did that is because I couldn't put it in the existing >> helpers since the framebuffer is removed after drm_fb_helper_fini() and >> I can't drop the ref on drm_device before the framebuffer is gone. > Why is that impossible? I'd have assumed that we'd first drop all the > drm_device refcounts before we do any of the cleanup steps. Ah, yes ofc that makes sense :$ I'm cleaning up fbdev in fb_ops.fb_destroy, but I can just drop the ref there... I've been several rounds with this and it feels like I have been stepping in every pothole there is. The upside is that in the end, the solution turns out to be very simple :-) I'll respin and drop drm_fb_helper_simple_init/fini() and drm_fb_helper_dev_unplug(). Noralf.
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); +} +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) {
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> --- drivers/gpu/drm/drm_fb_helper.c | 204 +++++++++++++++++++++++++++++++++++++++- include/drm/drm_fb_helper.h | 35 +++++++ 2 files changed, 237 insertions(+), 2 deletions(-)