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@xxxxxxxxxxx>
You're mixing in the new simple fbdev helper helpers as a bonus, that's
two things in one patch and makes it harder to review what's really going
on.
My gut feeling is that when we need a helper for the helper the original
helper needs to be improved, but I think that's much easier to decide when
it's a separate patch. Splitting things out would definitely make this
patch much smaller and easier to understand and review.
The only reason for the simple helpers that I could find is the
drm_dev_ref/unref, we should be able to do that in one of the existing
callbacks.
> ---
> drivers/gpu/drm/drm_fb_helper.c | 204 +++++++++++++++++++++++++++++++++++++++-
> include/drm/drm_fb_helper.h | 35 +++++++
> 2 files changed, 237 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 2e33467..fc898db 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -105,6 +105,158 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
> * mmap page writes.
> */
>
> +/**
> + * drm_fb_helper_simple_init - Simple fbdev emulation initialization
> + * @dev: drm device
> + * @fb_helper: driver-allocated fbdev helper structure to initialize
> + * @bpp_sel: bpp value to use for the framebuffer configuration
> + * @max_conn_count: max connector count
> + * @funcs: pointer to structure of functions associate with this helper
> + *
> + * Simple fbdev emulation initialization which calls the following functions:
> + * drm_fb_helper_prepare(), drm_fb_helper_init(),
> + * drm_fb_helper_single_add_all_connectors() and
> + * drm_fb_helper_initial_config().
> + *
> + * This function takes a ref on &drm_device and must be used together with
> + * drm_fb_helper_simple_fini() or drm_fb_helper_dev_unplug().
> + *
> + * fbdev deferred I/O users must use drm_fb_helper_defio_init().
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + */
> +int drm_fb_helper_simple_init(struct drm_device *dev,
> + struct drm_fb_helper *fb_helper, int bpp_sel,
> + int max_conn_count,
> + const struct drm_fb_helper_funcs *funcs)
> +{
> + int ret;
> +
> + drm_fb_helper_prepare(dev, fb_helper, funcs);
> +
> + ret = drm_fb_helper_init(dev, fb_helper, max_conn_count);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev->dev, "Failed to initialize fb helper.\n");
> + return ret;
> + }
> +
> + drm_dev_ref(dev);
> +
> + ret = drm_fb_helper_single_add_all_connectors(fb_helper);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev->dev, "Failed to add connectors.\n");
> + goto err_drm_fb_helper_fini;
> +
> + }
> +
> + ret = drm_fb_helper_initial_config(fb_helper, bpp_sel);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev->dev, "Failed to set initial hw config.\n");
> + goto err_drm_fb_helper_fini;
> + }
> +
> + return 0;
> +
> +err_drm_fb_helper_fini:
> + drm_fb_helper_fini(fb_helper);
> + drm_dev_unref(dev);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(drm_fb_helper_simple_init);
> +
> +static void drm_fb_helper_simple_fini_cleanup(struct drm_fb_helper *fb_helper)
> +{
> + struct fb_info *info = fb_helper->fbdev;
> + struct fb_ops *fbops = NULL;
> +
> + if (info && info->fbdefio) {
> + fb_deferred_io_cleanup(info);
> + kfree(info->fbdefio);
> + info->fbdefio = NULL;
> + fbops = info->fbops;
> + }
> +
> + drm_fb_helper_fini(fb_helper);
> + kfree(fbops);
> + if (fb_helper->fb)
> + drm_framebuffer_remove(fb_helper->fb);
> + drm_dev_unref(fb_helper->dev);
> +}
> +
> +/**
> + * drm_fb_helper_simple_fini - Simple fbdev cleanup
> + * @fb_helper: fbdev emulation structure, can be NULL
> + *
> + * Simple fbdev emulation cleanup. This function unregisters fbdev, cleans up
> + * deferred I/O if necessary, finalises @fb_helper and removes the framebuffer.
> + * The driver if responsible for freeing the @fb_helper structure.
> + *
> + * Don't use this function if you use drm_fb_helper_dev_unplug().
> + */
> +void drm_fb_helper_simple_fini(struct drm_fb_helper *fb_helper)
> +{
> + struct fb_info *info;
> +
> + if (!fb_helper)
> + return;
> +
> + info = fb_helper->fbdev;
> +
> + /* Has drm_fb_helper_dev_unplug() been used? */
> + if (info && info->dev)
> + drm_fb_helper_unregister_fbi(fb_helper);
> +
> + if (!(info && info->fbops && info->fbops->fb_destroy))
> + drm_fb_helper_simple_fini_cleanup(fb_helper);
> +}
> +EXPORT_SYMBOL_GPL(drm_fb_helper_simple_fini);
> +
> +/**
> + * drm_fb_helper_dev_unplug - unplug an fbdev device
> + * @fb_helper: driver-allocated fbdev helper, can be NULL
> + *
> + * This unplugs the fbdev emulation for a hotpluggable DRM device, which makes
> + * fbdev inaccessible to userspace operations. This essentially unregisters
> + * fbdev and can be called while there are still open users of @fb_helper.
> + * Entry-points from fbdev into drm core/helpers are protected by the fbdev
> + * &fb_info ref count and drm_dev_is_unplugged(). This means that the driver
> + * also has to call drm_dev_unplug() to complete the unplugging.
> + *
> + * Drivers must use drm_fb_helper_fb_destroy() as their &fb_ops.fb_destroy
> + * callback and call drm_mode_config_cleanup() and free @fb_helper in their
> + * &drm_driver->release callback.
> + *
> + * @fb_helper is finalized by this function unless there are open fbdev fd's
> + * in case this is postponed to the closing of the last fd. Finalizing includes
> + * dropping the ref taken on &drm_device in drm_fb_helper_simple_init().
> + */
> +void drm_fb_helper_dev_unplug(struct drm_fb_helper *fb_helper)
> +{
> + drm_fb_helper_unregister_fbi(fb_helper);
I don't see the value of this wrapper. If you want to explain how to best
unplug the fbdev emulation I thinkt that's better done in some dedicated
DOC section (referenced by drm_dev_unplug() maybe), than by providing a
wrapper that only gives you a different name.
_unplug is also a bit misleading, since _unplug is about yanking the
backing device. This just unregisters the the fbdev interface.
> +}
> +EXPORT_SYMBOL(drm_fb_helper_dev_unplug);
> +
> +/**
> + * drm_fb_helper_fb_destroy - implementation for &fb_ops.fb_destroy
> + * @info: fbdev registered by the helper
> + *
> + * This function does the same as drm_fb_helper_simple_fini() except
> + * unregistering fbdev which is already done.
> + *
> + * &fb_ops.fb_destroy is called during unregister_framebuffer() or the last
> + * fb_release() which ever comes last.
> + */
> +void drm_fb_helper_fb_destroy(struct fb_info *info)
> +{
> + struct drm_fb_helper *helper = info->par;
> +
> + DRM_DEBUG("\n");
> + drm_fb_helper_simple_fini_cleanup(helper);
> +}
> +EXPORT_SYMBOL(drm_fb_helper_fb_destroy);
> +
> #define drm_fb_helper_for_each_connector(fbh, i__) \
> for (({ lockdep_assert_held(&(fbh)->lock); }), \
> i__ = 0; i__ < (fbh)->connector_count; i__++)
> @@ -498,7 +650,7 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
> bool do_delayed;
> int ret;
>
> - if (!drm_fbdev_emulation)
> + if (!drm_fbdev_emulation || drm_dev_is_unplugged(fb_helper->dev))
> return -ENODEV;
>
> if (READ_ONCE(fb_helper->deferred_setup))
> @@ -563,6 +715,9 @@ static bool drm_fb_helper_force_kernel_mode(void)
> list_for_each_entry(helper, &kernel_fb_helper_list, kernel_fb_list) {
> struct drm_device *dev = helper->dev;
>
> + if (drm_dev_is_unplugged(dev))
> + continue;
> +
> if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> continue;
>
> @@ -735,6 +890,9 @@ static void drm_fb_helper_dirty_work(struct work_struct *work)
> struct drm_clip_rect clip_copy;
> unsigned long flags;
>
> + if (drm_dev_is_unplugged(helper->dev))
> + return;
> +
> spin_lock_irqsave(&helper->dirty_lock, flags);
> clip_copy = *clip;
> clip->x1 = clip->y1 = ~0;
> @@ -949,6 +1107,48 @@ void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper)
> }
> EXPORT_SYMBOL(drm_fb_helper_unlink_fbi);
>
> +/**
> + * drm_fb_helper_defio_init - fbdev deferred I/O initialization
> + * @fb_helper: driver-allocated fbdev helper
> + *
> + * This function allocates &fb_deferred_io, sets callback to
> + * drm_fb_helper_deferred_io(), delay to 50ms and calls fb_deferred_io_init().
> + *
> + * NOTE: A copy of &fb_ops is made and assigned to &info->fbops. This is done
> + * because fb_deferred_io_cleanup() clears &fbops->fb_mmap and would thereby
> + * affect other instances of that &fb_ops. This copy is freed by the helper
> + * during cleanup.
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + */
> +int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper)
> +{
> + struct fb_info *info = fb_helper->fbdev;
> + struct fb_deferred_io *fbdefio;
> + struct fb_ops *fbops;
> +
> + fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
> + fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
> + if (!fbdefio || !fbops) {
> + kfree(fbdefio);
> + kfree(fbops);
> + return -ENOMEM;
> + }
> +
> + info->fbdefio = fbdefio;
> + fbdefio->delay = msecs_to_jiffies(50);
> + fbdefio->deferred_io = drm_fb_helper_deferred_io;
> +
> + *fbops = *info->fbops;
> + info->fbops = fbops;
> +
> + fb_deferred_io_init(info);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_fb_helper_defio_init);
> +
> static void drm_fb_helper_dirty(struct fb_info *info, u32 x, u32 y,
> u32 width, u32 height)
> {
> @@ -2591,7 +2791,7 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
> {
> int err = 0;
>
> - if (!drm_fbdev_emulation)
> + if (!drm_fbdev_emulation || drm_dev_is_unplugged(fb_helper->dev))
> return 0;
>
> mutex_lock(&fb_helper->lock);
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 33fe959..1c5a648 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -242,6 +242,14 @@ struct drm_fb_helper {
> .fb_ioctl = drm_fb_helper_ioctl
>
> #ifdef CONFIG_DRM_FBDEV_EMULATION
> +int drm_fb_helper_simple_init(struct drm_device *dev,
> + struct drm_fb_helper *fb_helper, int bpp_sel,
> + int max_conn_count,
> + const struct drm_fb_helper_funcs *funcs);
> +void drm_fb_helper_simple_fini(struct drm_fb_helper *fb_helper);
> +void drm_fb_helper_dev_unplug(struct drm_fb_helper *fb_helper);
> +void drm_fb_helper_fb_destroy(struct fb_info *info);
> +
> void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper,
> const struct drm_fb_helper_funcs *funcs);
> int drm_fb_helper_init(struct drm_device *dev,
> @@ -265,6 +273,7 @@ void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
>
> void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper);
>
> +int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper);
> void drm_fb_helper_deferred_io(struct fb_info *info,
> struct list_head *pagelist);
>
> @@ -311,6 +320,27 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, struct drm_
> int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
> struct drm_connector *connector);
> #else
> +static inline int
> +drm_fb_helper_simple_init(struct drm_device *dev,
> + struct drm_fb_helper *fb_helper, int bpp_sel,
> + int max_conn_count,
> + const struct drm_fb_helper_funcs *funcs)
> +{
> + return 0;
> +}
> +
> +static inline void drm_fb_helper_simple_fini(struct drm_fb_helper *fb_helper)
> +{
> +}
> +
> +static inline void drm_fb_helper_dev_unplug(struct drm_fb_helper *fb_helper)
> +{
> +}
> +
> +static inline void drm_fb_helper_fb_destroy(struct fb_info *info)
> +{
> +}
> +
> static inline void drm_fb_helper_prepare(struct drm_device *dev,
> struct drm_fb_helper *helper,
> const struct drm_fb_helper_funcs *funcs)
> @@ -393,6 +423,11 @@ static inline void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper)
> {
> }
>
> +static inline int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper)
> +{
> + return 0;
> +}
> +
> static inline void drm_fb_helper_deferred_io(struct fb_info *info,
> struct list_head *pagelist)
> {
> --
> 2.7.4
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
