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

[Noralf Trønnes]

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 at tronnes.org>
>>> You're mixing in the new simple fbdev helper helpers as a bonus, that's
>>> two things in one patch and makes it harder to review what's really going
>>> on.
>>>
>>> My gut feeling is that when we need a helper for the helper the original
>>> helper needs to be improved, but I think that's much easier to decide when
>>> it's a separate patch. Splitting things out would definitely make this
>>> patch much smaller and easier to understand and review.
>>>
>>> The only reason for the simple helpers that I could find is the
>>> drm_dev_ref/unref, we should be able to do that in one of the existing
>>> callbacks.
>> 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.