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.