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@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.
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.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
