Hi,
I want to start out by saying that this patchset is low priority for me and if no one has interest or time to review this, that is just fine. I was in the flow and just typed it out.
This patchset adds a way for fbdev emulation code to create a framebuffer that is backed by a dumb buffer. drm_fb_helper gets a drm_file to hang the objects on, drm_framebuffer_create_dumb() creates the framebuffer and drm_fb_helper_fini() destroys it. I have verified that all cma drivers supports dumb buffers, so converting the library should be fine for all.
A patch by David Herrmann from a year ago made this easy. It was the last piece in his work to make it possible to create a drm_file for in-kernel use, but it never got merged.
I've cc'ed intel-gfx since that will give CI runs of the core patches if I understood Daniel right.
Noralf.
David Herrmann (1): drm: provide management functions for drm_file
Noralf Trønnes (7): drm/framebuffer: Add drm_framebuffer_create_dumb() drm/auth: Export drm_dropmaster_ioctl() drm/fb-helper: Allocate a drm_file drm/fb-cma-helper: Use drm_framebuffer_create_dumb() drm/fb-cma-helper: Drop unnecessary fbdev buffer offset drm/tinydrm: Use drm_fbdev_cma_init() drm/fb-cma-helper: Remove drm_fbdev_cma_init_with_funcs()
drivers/gpu/drm/drm_auth.c | 1 + drivers/gpu/drm/drm_fb_cma_helper.c | 111 ++-------- drivers/gpu/drm/drm_fb_helper.c | 22 +- drivers/gpu/drm/drm_file.c | 323 ++++++++++++++++------------ drivers/gpu/drm/drm_framebuffer.c | 61 ++++++ drivers/gpu/drm/drm_internal.h | 2 - drivers/gpu/drm/tinydrm/core/tinydrm-core.c | 5 +- include/drm/drm_auth.h | 2 + include/drm/drm_fb_helper.h | 9 + include/drm/drm_file.h | 2 + include/drm/drm_framebuffer.h | 4 + 11 files changed, 305 insertions(+), 237 deletions(-)
From: David Herrmann dh.herrmann@gmail.com
Rather than doing drm_file allocation/destruction right in the fops, lets provide separate helpers. This decouples drm_file management from the still-mandatory drm-fops. It prepares for use of drm_file without the fops, both by possible separate fops implementations and APIs (not that I am aware of any such plans), and more importantly from in-kernel use where no real file is available.
Signed-off-by: David Herrmann dh.herrmann@gmail.com Rebased, kept drm_events_release() and exported functions. Signed-off-by: Noralf Trønnes noralf@tronnes.org --- drivers/gpu/drm/drm_file.c | 323 ++++++++++++++++++++++++++------------------- include/drm/drm_file.h | 2 + 2 files changed, 186 insertions(+), 139 deletions(-)
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c index b3c6e99..eeea374 100644 --- a/drivers/gpu/drm/drm_file.c +++ b/drivers/gpu/drm/drm_file.c @@ -101,6 +101,176 @@ DEFINE_MUTEX(drm_global_mutex);
static int drm_open_helper(struct file *filp, struct drm_minor *minor);
+/** + * drm_file_alloc - allocate file context + * @minor: minor to allocate on + * + * This allocates a new DRM file context. It is not linked into any context and + * can be used by the caller freely. Note that the context keeps a pointer to + * @minor, so it must be freed before @minor is. + * + * The legacy paths might require the drm_global_mutex to be held. + * + * RETURNS: + * Pointer to newly allocated context, ERR_PTR on failure. + */ +struct drm_file *drm_file_alloc(struct drm_minor *minor) +{ + struct drm_device *dev = minor->dev; + struct drm_file *file; + int ret; + + file = kzalloc(sizeof(*file), GFP_KERNEL); + if (!file) + return ERR_PTR(-ENOMEM); + + file->pid = get_pid(task_pid(current)); + file->minor = minor; + + /* for compatibility root is always authenticated */ + file->authenticated = capable(CAP_SYS_ADMIN); + file->lock_count = 0; + + INIT_LIST_HEAD(&file->lhead); + INIT_LIST_HEAD(&file->fbs); + mutex_init(&file->fbs_lock); + INIT_LIST_HEAD(&file->blobs); + INIT_LIST_HEAD(&file->pending_event_list); + INIT_LIST_HEAD(&file->event_list); + init_waitqueue_head(&file->event_wait); + file->event_space = 4096; /* set aside 4k for event buffer */ + + mutex_init(&file->event_read_lock); + + if (drm_core_check_feature(dev, DRIVER_GEM)) + drm_gem_open(dev, file); + + if (drm_core_check_feature(dev, DRIVER_SYNCOBJ)) + drm_syncobj_open(file); + + if (drm_core_check_feature(dev, DRIVER_PRIME)) + drm_prime_init_file_private(&file->prime); + + if (dev->driver->open) { + ret = dev->driver->open(dev, file); + if (ret < 0) + goto out_prime_destroy; + } + + if (drm_is_primary_client(file)) { + ret = drm_master_open(file); + if (ret) + goto out_close; + } + + return file; + +out_close: + if (dev->driver->postclose) + dev->driver->postclose(dev, file); +out_prime_destroy: + if (drm_core_check_feature(dev, DRIVER_PRIME)) + drm_prime_destroy_file_private(&file->prime); + if (drm_core_check_feature(dev, DRIVER_SYNCOBJ)) + drm_syncobj_release(file); + if (drm_core_check_feature(dev, DRIVER_GEM)) + drm_gem_release(dev, file); + put_pid(file->pid); + kfree(file); + + return ERR_PTR(ret); +} +EXPORT_SYMBOL(drm_file_alloc); + +static void drm_events_release(struct drm_file *file) +{ + struct drm_device *dev = file->minor->dev; + struct drm_pending_event *e, *et; + unsigned long flags; + + spin_lock_irqsave(&dev->event_lock, flags); + + /* Unlink pending events */ + list_for_each_entry_safe(e, et, &file->pending_event_list, + pending_link) { + list_del(&e->pending_link); + e->file_priv = NULL; + } + + /* Remove unconsumed events */ + list_for_each_entry_safe(e, et, &file->event_list, link) { + list_del(&e->link); + kfree(e); + } + + spin_unlock_irqrestore(&dev->event_lock, flags); +} + +/** + * drm_file_free - free file context + * @file: context to free, or NULL + * + * This destroys and deallocates a DRM file context previously allocated via + * drm_file_alloc(). The caller must make sure to unlink it from any contexts + * before calling this. + * + * The legacy paths might require the drm_global_mutex to be held. + * + * If NULL is passed, this is a no-op. + * + * RETURNS: + * 0 on success, or error code on failure. + */ +void drm_file_free(struct drm_file *file) +{ + struct drm_device *dev; + + if (!file) + return; + + dev = file->minor->dev; + + if (drm_core_check_feature(dev, DRIVER_LEGACY) && + dev->driver->preclose) + dev->driver->preclose(dev, file); + + if (drm_core_check_feature(dev, DRIVER_LEGACY)) + drm_legacy_lock_release(dev, file->filp); + + if (drm_core_check_feature(dev, DRIVER_HAVE_DMA)) + drm_legacy_reclaim_buffers(dev, file); + + drm_events_release(file); + + if (drm_core_check_feature(dev, DRIVER_MODESET)) { + drm_fb_release(file); + drm_property_destroy_user_blobs(dev, file); + } + + if (drm_core_check_feature(dev, DRIVER_SYNCOBJ)) + drm_syncobj_release(file); + + if (drm_core_check_feature(dev, DRIVER_GEM)) + drm_gem_release(dev, file); + + drm_legacy_ctxbitmap_flush(dev, file); + + if (drm_is_primary_client(file)) + drm_master_release(file); + + if (dev->driver->postclose) + dev->driver->postclose(dev, file); + + if (drm_core_check_feature(dev, DRIVER_PRIME)) + drm_prime_destroy_file_private(&file->prime); + + WARN_ON(!list_empty(&file->event_list)); + + put_pid(file->pid); + kfree(file); +} +EXPORT_SYMBOL(drm_file_free); + static int drm_setup(struct drm_device * dev) { int ret; @@ -195,8 +365,7 @@ static int drm_cpu_valid(void) static int drm_open_helper(struct file *filp, struct drm_minor *minor) { struct drm_device *dev = minor->dev; - struct drm_file *priv; - int ret; + struct drm_file *file;
if (filp->f_flags & O_EXCL) return -EBUSY; /* No exclusive opens */ @@ -207,53 +376,15 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor)
DRM_DEBUG("pid = %d, minor = %d\n", task_pid_nr(current), minor->index);
- priv = kzalloc(sizeof(*priv), GFP_KERNEL); - if (!priv) - return -ENOMEM; + file = drm_file_alloc(minor); + if (IS_ERR(file)) + return PTR_ERR(file);
- filp->private_data = priv; - priv->filp = filp; - priv->pid = get_pid(task_pid(current)); - priv->minor = minor; - - /* for compatibility root is always authenticated */ - priv->authenticated = capable(CAP_SYS_ADMIN); - priv->lock_count = 0; - - INIT_LIST_HEAD(&priv->lhead); - INIT_LIST_HEAD(&priv->fbs); - mutex_init(&priv->fbs_lock); - INIT_LIST_HEAD(&priv->blobs); - INIT_LIST_HEAD(&priv->pending_event_list); - INIT_LIST_HEAD(&priv->event_list); - init_waitqueue_head(&priv->event_wait); - priv->event_space = 4096; /* set aside 4k for event buffer */ - - mutex_init(&priv->event_read_lock); - - if (drm_core_check_feature(dev, DRIVER_GEM)) - drm_gem_open(dev, priv); - - if (drm_core_check_feature(dev, DRIVER_SYNCOBJ)) - drm_syncobj_open(priv); - - if (drm_core_check_feature(dev, DRIVER_PRIME)) - drm_prime_init_file_private(&priv->prime); - - if (dev->driver->open) { - ret = dev->driver->open(dev, priv); - if (ret < 0) - goto out_prime_destroy; - } - - if (drm_is_primary_client(priv)) { - ret = drm_master_open(priv); - if (ret) - goto out_close; - } + filp->private_data = file; + file->filp = filp;
mutex_lock(&dev->filelist_mutex); - list_add(&priv->lhead, &dev->filelist); + list_add(&file->lhead, &dev->filelist); mutex_unlock(&dev->filelist_mutex);
#ifdef __alpha__ @@ -277,45 +408,6 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor) #endif
return 0; - -out_close: - if (dev->driver->postclose) - dev->driver->postclose(dev, priv); -out_prime_destroy: - if (drm_core_check_feature(dev, DRIVER_PRIME)) - drm_prime_destroy_file_private(&priv->prime); - if (drm_core_check_feature(dev, DRIVER_SYNCOBJ)) - drm_syncobj_release(priv); - if (drm_core_check_feature(dev, DRIVER_GEM)) - drm_gem_release(dev, priv); - put_pid(priv->pid); - kfree(priv); - filp->private_data = NULL; - return ret; -} - -static void drm_events_release(struct drm_file *file_priv) -{ - struct drm_device *dev = file_priv->minor->dev; - struct drm_pending_event *e, *et; - unsigned long flags; - - spin_lock_irqsave(&dev->event_lock, flags); - - /* Unlink pending events */ - list_for_each_entry_safe(e, et, &file_priv->pending_event_list, - pending_link) { - list_del(&e->pending_link); - e->file_priv = NULL; - } - - /* Remove unconsumed events */ - list_for_each_entry_safe(e, et, &file_priv->event_list, link) { - list_del(&e->link); - kfree(e); - } - - spin_unlock_irqrestore(&dev->event_lock, flags); }
static void drm_legacy_dev_reinit(struct drm_device *dev) @@ -370,69 +462,22 @@ void drm_lastclose(struct drm_device * dev) */ int drm_release(struct inode *inode, struct file *filp) { - struct drm_file *file_priv = filp->private_data; - struct drm_minor *minor = file_priv->minor; + struct drm_file *file = filp->private_data; + struct drm_minor *minor = file->minor; struct drm_device *dev = minor->dev;
mutex_lock(&drm_global_mutex);
- DRM_DEBUG("open_count = %d\n", dev->open_count); - - mutex_lock(&dev->filelist_mutex); - list_del(&file_priv->lhead); - mutex_unlock(&dev->filelist_mutex); - - if (drm_core_check_feature(dev, DRIVER_LEGACY) && - dev->driver->preclose) - dev->driver->preclose(dev, file_priv); - - /* ======================================================== - * Begin inline drm_release - */ - DRM_DEBUG("pid = %d, device = 0x%lx, open_count = %d\n", task_pid_nr(current), - (long)old_encode_dev(file_priv->minor->kdev->devt), + (long)old_encode_dev(file->minor->kdev->devt), dev->open_count);
- if (drm_core_check_feature(dev, DRIVER_LEGACY)) - drm_legacy_lock_release(dev, filp); + mutex_lock(&dev->filelist_mutex); + list_del(&file->lhead); + mutex_unlock(&dev->filelist_mutex);
- if (drm_core_check_feature(dev, DRIVER_HAVE_DMA)) - drm_legacy_reclaim_buffers(dev, file_priv); - - drm_events_release(file_priv); - - if (drm_core_check_feature(dev, DRIVER_MODESET)) { - drm_fb_release(file_priv); - drm_property_destroy_user_blobs(dev, file_priv); - } - - if (drm_core_check_feature(dev, DRIVER_SYNCOBJ)) - drm_syncobj_release(file_priv); - - if (drm_core_check_feature(dev, DRIVER_GEM)) - drm_gem_release(dev, file_priv); - - drm_legacy_ctxbitmap_flush(dev, file_priv); - - if (drm_is_primary_client(file_priv)) - drm_master_release(file_priv); - - if (dev->driver->postclose) - dev->driver->postclose(dev, file_priv); - - if (drm_core_check_feature(dev, DRIVER_PRIME)) - drm_prime_destroy_file_private(&file_priv->prime); - - WARN_ON(!list_empty(&file_priv->event_list)); - - put_pid(file_priv->pid); - kfree(file_priv); - - /* ======================================================== - * End inline drm_release - */ + drm_file_free(file);
if (!--dev->open_count) { drm_lastclose(dev); diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h index 0e0c868..23d9074 100644 --- a/include/drm/drm_file.h +++ b/include/drm/drm_file.h @@ -360,6 +360,8 @@ static inline bool drm_is_control_client(const struct drm_file *file_priv) return file_priv->minor->type == DRM_MINOR_CONTROL; }
+struct drm_file *drm_file_alloc(struct drm_minor *minor); +void drm_file_free(struct drm_file *file); int drm_open(struct inode *inode, struct file *filp); ssize_t drm_read(struct file *filp, char __user *buffer, size_t count, loff_t *offset);
drm_framebuffer_create_dumb() uses the dumb buffer API to create a buffer that is attached to a framebuffer. Useful for fbdev emulation.
Signed-off-by: Noralf Trønnes noralf@tronnes.org --- drivers/gpu/drm/drm_framebuffer.c | 61 +++++++++++++++++++++++++++++++++++++++ include/drm/drm_framebuffer.h | 4 +++ 2 files changed, 65 insertions(+)
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index af27984..07e5199 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -955,3 +955,64 @@ int drm_framebuffer_plane_height(int height, return fb_plane_height(height, fb->format, plane); } EXPORT_SYMBOL(drm_framebuffer_plane_height); + +/** + * drm_framebuffer_create_dumb - Create dumb framebuffer + * @file_priv: DRM file + * @width: Framebuffer width + * @height: Framebuffer height + * @format: Framebuffer FOURCC format + * + * This function creates a framebuffer backed by a dumb buffer for in-kernel + * use. fbdev emulation code can use this to create a framebuffer. + * + * Returns: + * Pointer to a drm_framebuffer on success or an error pointer on failure. + */ +struct drm_framebuffer * +drm_framebuffer_create_dumb(struct drm_file *file_priv, unsigned int width, + unsigned int height, u32 format) +{ + struct drm_device *dev = file_priv->minor->dev; + struct drm_mode_create_dumb dumb_args = { 0 }; + struct drm_mode_fb_cmd2 fb_args = { 0 }; + struct drm_framebuffer *fb; + int ret; + + dumb_args.width = width; + dumb_args.height = height; + dumb_args.bpp = drm_format_plane_cpp(format, 0) * 8; + + ret = drm_mode_create_dumb_ioctl(dev, &dumb_args, file_priv); + if (ret) + return ERR_PTR(ret); + + fb_args.width = width; + fb_args.height = height; + fb_args.pixel_format = format; + fb_args.handles[0] = dumb_args.handle; + fb_args.pitches[0] = dumb_args.pitch; + + ret = drm_mode_addfb2(dev, &fb_args, file_priv); + if (ret) + goto err_destroy_dumb; + + fb = drm_framebuffer_lookup(dev, fb_args.fb_id); + if (!fb) { + ret = -ENOENT; + goto err_rmfb; + } + + /* drop the reference we picked up in framebuffer lookup */ + drm_framebuffer_put(fb); + + return fb; + +err_rmfb: + drm_mode_rmfb(dev, &fb_args, file_priv); +err_destroy_dumb: + drm_mode_destroy_dumb_ioctl(dev, &dumb_args, file_priv); + + return ERR_PTR(ret); +} +EXPORT_SYMBOL(drm_framebuffer_create_dumb); diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h index b6996dd..212c66a 100644 --- a/include/drm/drm_framebuffer.h +++ b/include/drm/drm_framebuffer.h @@ -306,4 +306,8 @@ int drm_framebuffer_plane_width(int width, int drm_framebuffer_plane_height(int height, const struct drm_framebuffer *fb, int plane);
+struct drm_framebuffer * +drm_framebuffer_create_dumb(struct drm_file *file_priv, unsigned int width, + unsigned int height, u32 format); + #endif
Export drm_dropmaster_ioctl() so in-kernel drm_file_alloc() users can drop master.
Signed-off-by: Noralf Trønnes noralf@tronnes.org --- drivers/gpu/drm/drm_auth.c | 1 + drivers/gpu/drm/drm_internal.h | 2 -- include/drm/drm_auth.h | 2 ++ 3 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c index 7ff6973..8417130 100644 --- a/drivers/gpu/drm/drm_auth.c +++ b/drivers/gpu/drm/drm_auth.c @@ -221,6 +221,7 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data, mutex_unlock(&dev->master_mutex); return ret; } +EXPORT_SYMBOL(drm_dropmaster_ioctl);
int drm_master_open(struct drm_file *file_priv) { diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index 4e906b8..f690ccd 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -78,8 +78,6 @@ int drm_authmagic(struct drm_device *dev, void *data, struct drm_file *file_priv); int drm_setmaster_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); -int drm_dropmaster_ioctl(struct drm_device *dev, void *data, - struct drm_file *file_priv); int drm_master_open(struct drm_file *file_priv); void drm_master_release(struct drm_file *file_priv);
diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h index 81a40c2..b72c137 100644 --- a/include/drm/drm_auth.h +++ b/include/drm/drm_auth.h @@ -81,5 +81,7 @@ struct drm_master { struct drm_master *drm_master_get(struct drm_master *master); void drm_master_put(struct drm_master **master); bool drm_is_current_master(struct drm_file *fpriv); +int drm_dropmaster_ioctl(struct drm_device *dev, void *data, + struct drm_file *file_priv);
#endif
Allocate a drm_file so drivers can use drm_framebuffer_create_dumb() to create a buffer. The framebuffer is reaped automtically in drm_fb_helper_fini().
Signed-off-by: Noralf Trønnes noralf@tronnes.org --- drivers/gpu/drm/drm_fb_helper.c | 22 ++++++++++++++++++++-- include/drm/drm_fb_helper.h | 9 +++++++++ 2 files changed, 29 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 6a31d13..a04bdf9 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -35,6 +35,7 @@ #include <linux/slab.h> #include <linux/module.h> #include <drm/drmP.h> +#include <drm/drm_auth.h> #include <drm/drm_crtc.h> #include <drm/drm_fb_helper.h> #include <drm/drm_crtc_helper.h> @@ -902,6 +903,7 @@ EXPORT_SYMBOL(drm_fb_helper_unregister_fbi); * * This cleans up all remaining resources associated with @fb_helper. Must be * called after drm_fb_helper_unlink_fbi() was called. + * drm_framebuffer_create_dumb() created framebuffers will be destroyed. */ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper) { @@ -932,6 +934,7 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper) mutex_destroy(&fb_helper->lock); drm_fb_helper_crtc_free(fb_helper);
+ drm_file_free(fb_helper->file); } EXPORT_SYMBOL(drm_fb_helper_fini);
@@ -2465,6 +2468,16 @@ __drm_fb_helper_initial_config_and_unlock(struct drm_fb_helper *fb_helper, unsigned int width, height; int ret;
+ fb_helper->file = drm_file_alloc(dev->primary); + if (IS_ERR(fb_helper->file)) { + ret = PTR_ERR(fb_helper->file); + fb_helper->file = NULL; + mutex_unlock(&fb_helper->lock); + return ret; + } + + drm_dropmaster_ioctl(dev, NULL, fb_helper->file); + width = dev->mode_config.max_width; height = dev->mode_config.max_height;
@@ -2478,7 +2491,7 @@ __drm_fb_helper_initial_config_and_unlock(struct drm_fb_helper *fb_helper, } mutex_unlock(&fb_helper->lock);
- return ret; + goto err_free_file; } drm_setup_crtcs_fb(fb_helper);
@@ -2494,7 +2507,7 @@ __drm_fb_helper_initial_config_and_unlock(struct drm_fb_helper *fb_helper,
ret = register_framebuffer(info); if (ret < 0) - return ret; + goto err_free_file;
dev_info(dev->dev, "fb%d: %s frame buffer device\n", info->node, info->fix.id); @@ -2507,6 +2520,11 @@ __drm_fb_helper_initial_config_and_unlock(struct drm_fb_helper *fb_helper, mutex_unlock(&kernel_fb_helper_lock);
return 0; + +err_free_file: + drm_file_free(fb_helper->file); + + return ret; }
/** diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index 33fe959..89757ff 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -154,6 +154,15 @@ struct drm_fb_helper_connector { struct drm_fb_helper { struct drm_framebuffer *fb; struct drm_device *dev; + + /** + * @file: + * + * DRM file that can be used with drm_framebuffer_create_dumb() to + * create a framebuffer to back fbdev. + */ + struct drm_file *file; + int crtc_count; struct drm_fb_helper_crtc *crtc_info; int connector_count;
FYI, we noticed the following commit:
commit: a583bc678d0a5e54844d3380e242dd9bb5fb0b00 ("drm/fb-helper: Allocate a drm_file") url: https://github.com/0day-ci/linux/commits/Noralf-Tr-nnes/drm-fb-helper-Use-dr... base: git://people.freedesktop.org/~airlied/linux.git drm-next
in testcase: boot
on test machine: qemu-system-x86_64 -enable-kvm -smp 2 -m 512M
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
+------------------------------------------------+------------+------------+ | | 6b0c88fff7 | a583bc678d | +------------------------------------------------+------------+------------+ | boot_successes | 66 | 0 | | boot_failures | 0 | 77 | | WARNING:at_kernel/workqueue.c:#flush_workqueue | 0 | 32 | | kernel_BUG_at_lib/list_debug.c | 0 | 66 | | invalid_opcode:#[##] | 0 | 66 | | Kernel_panic-not_syncing:Fatal_exception | 0 | 77 | | BUG:unable_to_handle_kernel | 0 | 11 | | Oops:#[##] | 0 | 11 | +------------------------------------------------+------------+------------+
[ 14.785867] WARNING: CPU: 0 PID: 5 at kernel/workqueue.c:2606 flush_workqueue+0x881/0x8b0 [ 14.798652] list_del corruption. prev->next should be ffff88001c459300, but was (null) [ 14.798673] ------------[ cut here ]------------ [ 14.798674] kernel BUG at lib/list_debug.c:56! [ 14.798677] invalid opcode: 0000 [#1] PREEMPT [ 14.798679] CPU: 0 PID: 24 Comm: kworker/u2:1 Not tainted 4.13.0-rc5-01112-ga583bc6 #1 [ 14.798680] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.9.3-20161025_171302-gandalf 04/01/2014 [ 14.798686] task: ffff88001d31f0c0 task.stack: ffffc900000c0000 [ 14.798690] RIP: 0010:__list_del_entry_valid+0x155/0x160 [ 14.798691] RSP: 0000:ffffc900000c3e60 EFLAGS: 00010086 [ 14.798693] RAX: 0000000000000054 RBX: 0000000000000003 RCX: 0000000000000000 [ 14.798693] RDX: 0000000000000002 RSI: 0000000000000000 RDI: 0000000000000046 [ 14.798694] RBP: ffffc900000c3e88 R08: 0000000000000000 R09: 0000000000000000 [ 14.798695] R10: ffff88001d31f110 R11: 000000006c756e28 R12: ffff88001c459300 [ 14.798696] R13: ffff88001c491cc0 R14: ffff88001d1bc030 R15: 0000000000000000 [ 14.798698] FS: 0000000000000000(0000) GS:ffffffff8422f000(0000) knlGS:0000000000000000 [ 14.798699] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 14.798700] CR2: 00000000000000a8 CR3: 000000000420e000 CR4: 00000000000006f0 [ 14.798702] Call Trace: [ 14.798707] worker_thread+0x1c5/0x810 [ 14.798709] kthread+0x187/0x190 [ 14.798711] ? process_one_work+0x7f0/0x7f0 [ 14.798712] ? __kthread_create_on_node+0x260/0x260 [ 14.798715] ret_from_fork+0x25/0x30 [ 14.798716] Code: 06 7e ff 0f 0b 4c 89 f2 4c 89 e6 48 c7 c7 c0 d9 db 83 e8 9f 06 7e ff 0f 0b 49 8b 16 4c 89 e6 48 c7 c7 f8 d9 db 83 e8 8b 06 7e ff <0f> 0b 90 90 90 90 90 90 90 90 90 55 48 89 f8 45 31 db 41 ba 01 [ 14.798741] RIP: __list_del_entry_valid+0x155/0x160 RSP: ffffc900000c3e60 [ 14.798746] ---[ end trace 22b1dffd04582160 ]---
To reproduce:
git clone https://github.com/intel/lkp-tests.git cd lkp-tests bin/lkp qemu -k <bzImage> job-script # job-script is attached in this email
Thanks, Xiaolong
Use drm_framebuffer_create_dumb() to create a framebuffer for fbdev. No need for error cleanup in drm_fbdev_cma_create(), since the one in drm_fbdev_cma_init_with_funcs() will do fine.
Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Noralf Trønnes noralf@tronnes.org --- drivers/gpu/drm/drm_fb_cma_helper.c | 44 +++++++++++++------------------------ 1 file changed, 15 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index f2ee883..f0a2863 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -312,7 +312,6 @@ static int drm_fbdev_cma_create(struct drm_fb_helper *helper, struct drm_fb_helper_surface_size *sizes) { - struct drm_fbdev_cma *fbdev_cma = to_fbdev_cma(helper); struct drm_device *dev = helper->dev; struct drm_gem_cma_object *obj; struct drm_framebuffer *fb; @@ -320,6 +319,7 @@ drm_fbdev_cma_create(struct drm_fb_helper *helper, unsigned long offset; struct fb_info *fbi; size_t size; + u32 format; int ret;
DRM_DEBUG_KMS("surface width(%d), height(%d) and bpp(%d)\n", @@ -327,26 +327,23 @@ drm_fbdev_cma_create(struct drm_fb_helper *helper, sizes->surface_bpp);
bytes_per_pixel = DIV_ROUND_UP(sizes->surface_bpp, 8); - size = sizes->surface_width * sizes->surface_height * bytes_per_pixel; - obj = drm_gem_cma_create(dev, size); - if (IS_ERR(obj)) - return -ENOMEM;
- fbi = drm_fb_helper_alloc_fbi(helper); - if (IS_ERR(fbi)) { - ret = PTR_ERR(fbi); - goto err_gem_free_object; - } - - fb = drm_gem_fbdev_fb_create(dev, sizes, 0, &obj->base, - fbdev_cma->fb_funcs); + format = drm_mode_legacy_fb_format(sizes->surface_bpp, + sizes->surface_depth); + fb = drm_framebuffer_create_dumb(helper->file, sizes->surface_width, + sizes->surface_height, format); if (IS_ERR(fb)) { - dev_err(dev->dev, "Failed to allocate DRM framebuffer.\n"); - ret = PTR_ERR(fb); - goto err_fb_info_destroy; + DRM_DEV_ERROR(dev->dev, "Failed to create DRM framebuffer.\n"); + return PTR_ERR(fb); }
helper->fb = fb; + obj = to_drm_gem_cma_obj(fb->obj[0]); + size = fb->obj[0]->size; + + fbi = drm_fb_helper_alloc_fbi(helper); + if (IS_ERR(fbi)) + return PTR_ERR(fbi);
fbi->par = helper; fbi->flags = FBINFO_FLAG_DEFAULT; @@ -364,21 +361,13 @@ drm_fbdev_cma_create(struct drm_fb_helper *helper, fbi->screen_size = size; fbi->fix.smem_len = size;
- if (fbdev_cma->fb_funcs->dirty) { + if (fb->funcs->dirty) { ret = drm_fbdev_cma_defio_init(fbi, obj); if (ret) - goto err_cma_destroy; + return ret; }
return 0; - -err_cma_destroy: - drm_framebuffer_remove(fb); -err_fb_info_destroy: - drm_fb_helper_fini(helper); -err_gem_free_object: - drm_gem_object_put_unlocked(&obj->base); - return ret; }
static const struct drm_fb_helper_funcs drm_fb_cma_helper_funcs = { @@ -475,9 +464,6 @@ void drm_fbdev_cma_fini(struct drm_fbdev_cma *fbdev_cma) if (fbdev_cma->fb_helper.fbdev) drm_fbdev_cma_defio_fini(fbdev_cma->fb_helper.fbdev);
- if (fbdev_cma->fb_helper.fb) - drm_framebuffer_remove(fbdev_cma->fb_helper.fb); - drm_fb_helper_fini(&fbdev_cma->fb_helper); kfree(fbdev_cma); }
Drop the unnecessary fbdev buffer offset, it can only be zero since drm_fb_helper_fill_var() sets xoffset and yoffset to zero;
Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Noralf Trønnes noralf@tronnes.org --- drivers/gpu/drm/drm_fb_cma_helper.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index f0a2863..49623a4 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -316,7 +316,6 @@ drm_fbdev_cma_create(struct drm_fb_helper *helper, struct drm_gem_cma_object *obj; struct drm_framebuffer *fb; unsigned int bytes_per_pixel; - unsigned long offset; struct fb_info *fbi; size_t size; u32 format; @@ -352,12 +351,9 @@ drm_fbdev_cma_create(struct drm_fb_helper *helper, drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->format->depth); drm_fb_helper_fill_var(fbi, helper, sizes->fb_width, sizes->fb_height);
- offset = fbi->var.xoffset * bytes_per_pixel; - offset += fbi->var.yoffset * fb->pitches[0]; - dev->mode_config.fb_base = (resource_size_t)obj->paddr; - fbi->screen_base = obj->vaddr + offset; - fbi->fix.smem_start = (unsigned long)(obj->paddr + offset); + fbi->screen_base = obj->vaddr; + fbi->fix.smem_start = (unsigned long)obj->paddr; fbi->screen_size = size; fbi->fix.smem_len = size;
Since the cma library now uses drm_framebuffer_create_dumb(), drm_fbdev_cma_init() will provide a framebuffer with the dirty callback set.
Signed-off-by: Noralf Trønnes noralf@tronnes.org --- drivers/gpu/drm/tinydrm/core/tinydrm-core.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c index 551709e..6ebe970 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c @@ -221,9 +221,8 @@ static int tinydrm_register(struct tinydrm_device *tdev) if (ret) return ret;
- fbdev = drm_fbdev_cma_init_with_funcs(drm, bpp ? bpp : 32, - drm->mode_config.num_connector, - tdev->fb_funcs); + fbdev = drm_fbdev_cma_init(drm, bpp ? bpp : 32, + drm->mode_config.num_connector); if (IS_ERR(fbdev)) DRM_ERROR("Failed to initialize fbdev: %ld\n", PTR_ERR(fbdev)); else
No users left so remove drm_fbdev_cma_init_with_funcs(). Also remove example fbdev code in docs, there's nothing special now.
Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Noralf Trønnes noralf@tronnes.org --- drivers/gpu/drm/drm_fb_cma_helper.c | 59 ++----------------------------------- 1 file changed, 3 insertions(+), 56 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index 49623a4..d8904a8 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -29,7 +29,6 @@
struct drm_fbdev_cma { struct drm_fb_helper fb_helper; - const struct drm_framebuffer_funcs *fb_funcs; };
/** @@ -46,33 +45,6 @@ struct drm_fbdev_cma { * If the &drm_framebuffer_funcs.dirty callback is set, fb_deferred_io will be * set up automatically. &drm_framebuffer_funcs.dirty is called by * drm_fb_helper_deferred_io() in process context (&struct delayed_work). - * - * Example fbdev deferred io code:: - * - * static int driver_fb_dirty(struct drm_framebuffer *fb, - * struct drm_file *file_priv, - * unsigned flags, unsigned color, - * struct drm_clip_rect *clips, - * unsigned num_clips) - * { - * struct drm_gem_cma_object *cma = drm_fb_cma_get_gem_obj(fb, 0); - * ... push changes ... - * return 0; - * } - * - * static struct drm_framebuffer_funcs driver_fb_funcs = { - * .destroy = drm_fb_cma_destroy, - * .create_handle = drm_fb_cma_create_handle, - * .dirty = driver_fb_dirty, - * }; - * - * Initialize:: - * - * fbdev = drm_fbdev_cma_init_with_funcs(dev, 16, - * dev->mode_config.num_crtc, - * dev->mode_config.num_connector, - * &driver_fb_funcs); - * */
static inline struct drm_fbdev_cma *to_fbdev_cma(struct drm_fb_helper *helper) @@ -371,17 +343,15 @@ static const struct drm_fb_helper_funcs drm_fb_cma_helper_funcs = { };
/** - * drm_fbdev_cma_init_with_funcs() - Allocate and initializes a drm_fbdev_cma struct + * drm_fbdev_cma_init() - Allocate and initializes a drm_fbdev_cma struct * @dev: DRM device * @preferred_bpp: Preferred bits per pixel for the device * @max_conn_count: Maximum number of connectors - * @funcs: fb helper functions, in particular a custom dirty() callback * * Returns a newly allocated drm_fbdev_cma struct or a ERR_PTR. */ -struct drm_fbdev_cma *drm_fbdev_cma_init_with_funcs(struct drm_device *dev, - unsigned int preferred_bpp, unsigned int max_conn_count, - const struct drm_framebuffer_funcs *funcs) +struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev, + unsigned int preferred_bpp, unsigned int max_conn_count) { struct drm_fbdev_cma *fbdev_cma; struct drm_fb_helper *helper; @@ -392,7 +362,6 @@ struct drm_fbdev_cma *drm_fbdev_cma_init_with_funcs(struct drm_device *dev, dev_err(dev->dev, "Failed to allocate drm fbdev.\n"); return ERR_PTR(-ENOMEM); } - fbdev_cma->fb_funcs = funcs;
helper = &fbdev_cma->fb_helper;
@@ -426,28 +395,6 @@ struct drm_fbdev_cma *drm_fbdev_cma_init_with_funcs(struct drm_device *dev,
return ERR_PTR(ret); } -EXPORT_SYMBOL_GPL(drm_fbdev_cma_init_with_funcs); - -static const struct drm_framebuffer_funcs drm_fb_cma_funcs = { - .destroy = drm_gem_fb_destroy, - .create_handle = drm_gem_fb_create_handle, -}; - -/** - * drm_fbdev_cma_init() - Allocate and initializes a drm_fbdev_cma struct - * @dev: DRM device - * @preferred_bpp: Preferred bits per pixel for the device - * @max_conn_count: Maximum number of connectors - * - * Returns a newly allocated drm_fbdev_cma struct or a ERR_PTR. - */ -struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev, - unsigned int preferred_bpp, unsigned int max_conn_count) -{ - return drm_fbdev_cma_init_with_funcs(dev, preferred_bpp, - max_conn_count, - &drm_fb_cma_funcs); -} EXPORT_SYMBOL_GPL(drm_fbdev_cma_init);
/**
Hi Noralf,
Thank you for the patches.
On Monday, 11 September 2017 17:31:54 EEST Noralf Trønnes wrote:
Hi,
I want to start out by saying that this patchset is low priority for me and if no one has interest or time to review this, that is just fine. I was in the flow and just typed it out.
This patchset adds a way for fbdev emulation code to create a framebuffer that is backed by a dumb buffer. drm_fb_helper gets a drm_file to hang the objects on, drm_framebuffer_create_dumb() creates the framebuffer and drm_fb_helper_fini() destroys it. I have verified that all cma drivers supports dumb buffers, so converting the library should be fine for all.
Stupid question, what does this give us ? The series makes the call stack more complex (up to a point where I'm getting trouble just following it), what's the advantage that offsets that ?
With the exception of vmwgfx that does weird things I won't even try to understand, all drivers seem to use the drm_file object passed to the .dumb_create() operation just to register the GEM object handle. I wonder whether a better solution to use .dumb_create() for framebuffer emulation wouldn't be to move the GEM object handle registration from the .dumb_create() implementation to its caller in the core.
A patch by David Herrmann from a year ago made this easy. It was the last piece in his work to make it possible to create a drm_file for in-kernel use, but it never got merged.
I've cc'ed intel-gfx since that will give CI runs of the core patches if I understood Daniel right.
Noralf.
David Herrmann (1): drm: provide management functions for drm_file
Noralf Trønnes (7): drm/framebuffer: Add drm_framebuffer_create_dumb() drm/auth: Export drm_dropmaster_ioctl() drm/fb-helper: Allocate a drm_file drm/fb-cma-helper: Use drm_framebuffer_create_dumb() drm/fb-cma-helper: Drop unnecessary fbdev buffer offset drm/tinydrm: Use drm_fbdev_cma_init() drm/fb-cma-helper: Remove drm_fbdev_cma_init_with_funcs()
drivers/gpu/drm/drm_auth.c | 1 + drivers/gpu/drm/drm_fb_cma_helper.c | 111 ++-------- drivers/gpu/drm/drm_fb_helper.c | 22 +- drivers/gpu/drm/drm_file.c | 323 +++++++++++++++---------- drivers/gpu/drm/drm_framebuffer.c | 61 ++++++ drivers/gpu/drm/drm_internal.h | 2 - drivers/gpu/drm/tinydrm/core/tinydrm-core.c | 5 +- include/drm/drm_auth.h | 2 + include/drm/drm_fb_helper.h | 9 + include/drm/drm_file.h | 2 + include/drm/drm_framebuffer.h | 4 + 11 files changed, 305 insertions(+), 237 deletions(-)
Den 13.09.2017 07.09, skrev Laurent Pinchart:
Hi Noralf,
Thank you for the patches.
On Monday, 11 September 2017 17:31:54 EEST Noralf Trønnes wrote:
Hi,
I want to start out by saying that this patchset is low priority for me and if no one has interest or time to review this, that is just fine. I was in the flow and just typed it out.
This patchset adds a way for fbdev emulation code to create a framebuffer that is backed by a dumb buffer. drm_fb_helper gets a drm_file to hang the objects on, drm_framebuffer_create_dumb() creates the framebuffer and drm_fb_helper_fini() destroys it. I have verified that all cma drivers supports dumb buffers, so converting the library should be fine for all.
Stupid question, what does this give us ? The series makes the call stack more complex (up to a point where I'm getting trouble just following it), what's the advantage that offsets that ?
The short answer is that it avoids the need for special fbdev _with_funcs() functions in libraries like cma for framebuffers with the dirty callback set.
I'm suprised that you think this more complex, I find it more coherent when fbdev userspace is doing things more like DRM userspace, like hanging the objects on a drm_file and cleaning up the objects when done.
The longer and more diffuse answer is that it's annoying me that many drivers carry the burden of fbdev emulation just to get a console! And an in-kernel drm console, as David Herrmann described it a year ago, would allocate a framebuffer using something like what I have done here. But maybe it's better to do this the other way around: first get the drm console and then let fbdev use the same mechanisms. Or maybe at that point, just forget about fbdev and not spend any more time on it :-)
Sidenote: Do you if any DRM drivers is capable of showing kernel oops'es on the console?
With the exception of vmwgfx that does weird things I won't even try to understand, all drivers seem to use the drm_file object passed to the .dumb_create() operation just to register the GEM object handle. I wonder whether a better solution to use .dumb_create() for framebuffer emulation wouldn't be to move the GEM object handle registration from the .dumb_create() implementation to its caller in the core.
Can you elaborate what you mean by this?
Noralf.
A patch by David Herrmann from a year ago made this easy. It was the last piece in his work to make it possible to create a drm_file for in-kernel use, but it never got merged.
I've cc'ed intel-gfx since that will give CI runs of the core patches if I understood Daniel right.
Noralf.
David Herrmann (1): drm: provide management functions for drm_file
Noralf Trønnes (7): drm/framebuffer: Add drm_framebuffer_create_dumb() drm/auth: Export drm_dropmaster_ioctl() drm/fb-helper: Allocate a drm_file drm/fb-cma-helper: Use drm_framebuffer_create_dumb() drm/fb-cma-helper: Drop unnecessary fbdev buffer offset drm/tinydrm: Use drm_fbdev_cma_init() drm/fb-cma-helper: Remove drm_fbdev_cma_init_with_funcs()
drivers/gpu/drm/drm_auth.c | 1 + drivers/gpu/drm/drm_fb_cma_helper.c | 111 ++-------- drivers/gpu/drm/drm_fb_helper.c | 22 +- drivers/gpu/drm/drm_file.c | 323 +++++++++++++++---------- drivers/gpu/drm/drm_framebuffer.c | 61 ++++++ drivers/gpu/drm/drm_internal.h | 2 - drivers/gpu/drm/tinydrm/core/tinydrm-core.c | 5 +- include/drm/drm_auth.h | 2 + include/drm/drm_fb_helper.h | 9 + include/drm/drm_file.h | 2 + include/drm/drm_framebuffer.h | 4 + 11 files changed, 305 insertions(+), 237 deletions(-)
Hi Noralf,
On Wednesday, 13 September 2017 18:19:22 EEST Noralf Trønnes wrote:
Den 13.09.2017 07.09, skrev Laurent Pinchart:
On Monday, 11 September 2017 17:31:54 EEST Noralf Trønnes wrote:
Hi,
I want to start out by saying that this patchset is low priority for me and if no one has interest or time to review this, that is just fine. I was in the flow and just typed it out.
This patchset adds a way for fbdev emulation code to create a framebuffer that is backed by a dumb buffer. drm_fb_helper gets a drm_file to hang the objects on, drm_framebuffer_create_dumb() creates the framebuffer and drm_fb_helper_fini() destroys it. I have verified that all cma drivers supports dumb buffers, so converting the library should be fine for all.
Stupid question, what does this give us ? The series makes the call stack more complex (up to a point where I'm getting trouble just following it), what's the advantage that offsets that ?
The short answer is that it avoids the need for special fbdev _with_funcs() functions in libraries like cma for framebuffers with the dirty callback set.
I'm suprised that you think this more complex, I find it more coherent when fbdev userspace is doing things more like DRM userspace, like hanging the objects on a drm_file and cleaning up the objects when done.
Maybe moving to a new code base gave me the wrong feeling that the result is more complex. The current implementation is certainly not simple.
The longer and more diffuse answer is that it's annoying me that many drivers carry the burden of fbdev emulation just to get a console!
I totally agree with that. Ideally I'd like to remove 100% of fbdev-related code from drivers. This includes
- initialization and cleanup of fbdev helpers - fbdev restore in last_close() - forwarding of hotplug events to fbdev compatibility layer
In practice we'll likely need to keep one initialization function (or a drm_driver flag) to let drivers opt-in to fbdev compatibility, but the rest should go away. Or maybe we could even enable fbdev compatibility in all drivers unconditionally.
And an in-kernel drm console, as David Herrmann described it a year ago, would allocate a framebuffer using something like what I have done here. But maybe it's better to do this the other way around: first get the drm console and then let fbdev use the same mechanisms. Or maybe at that point, just forget about fbdev and not spend any more time on it :-)
When I mention I'd like fbdev to go away completely people complain that I like breaking things :-)
Sidenote: Do you if any DRM drivers is capable of showing kernel oops'es on the console?
The last time my kernel oopses the i915 driver certainly didn't show me the oops.
With the exception of vmwgfx that does weird things I won't even try to understand, all drivers seem to use the drm_file object passed to the .dumb_create() operation just to register the GEM object handle. I wonder whether a better solution to use .dumb_create() for framebuffer emulation wouldn't be to move the GEM object handle registration from the .dumb_create() implementation to its caller in the core.
Can you elaborate what you mean by this?
Again vmwgfx aside (and that's not a detail, we'd still have to handle that driver), the reason why the .dumb_create() operation needs a drm_file pointer is to register the GEM object handle for the dumb buffer. We could perform that registration in the caller of .dumb_create(), in which case we wouldn't need to pass the drm_file to the operation.
A patch by David Herrmann from a year ago made this easy. It was the last piece in his work to make it possible to create a drm_file for in-kernel use, but it never got merged.
I've cc'ed intel-gfx since that will give CI runs of the core patches if I understood Daniel right.
Noralf.
David Herrmann (1): drm: provide management functions for drm_file
Noralf Trønnes (7): drm/framebuffer: Add drm_framebuffer_create_dumb() drm/auth: Export drm_dropmaster_ioctl() drm/fb-helper: Allocate a drm_file drm/fb-cma-helper: Use drm_framebuffer_create_dumb() drm/fb-cma-helper: Drop unnecessary fbdev buffer offset drm/tinydrm: Use drm_fbdev_cma_init() drm/fb-cma-helper: Remove drm_fbdev_cma_init_with_funcs()
drivers/gpu/drm/drm_auth.c | 1 + drivers/gpu/drm/drm_fb_cma_helper.c | 111 ++-------- drivers/gpu/drm/drm_fb_helper.c | 22 +- drivers/gpu/drm/drm_file.c | 323 ++++++++++++-------- drivers/gpu/drm/drm_framebuffer.c | 61 ++++++ drivers/gpu/drm/drm_internal.h | 2 - drivers/gpu/drm/tinydrm/core/tinydrm-core.c | 5 +- include/drm/drm_auth.h | 2 + include/drm/drm_fb_helper.h | 9 + include/drm/drm_file.h | 2 + include/drm/drm_framebuffer.h | 4 + 11 files changed, 305 insertions(+), 237 deletions(-)
Den 15.09.2017 00.29, skrev Laurent Pinchart:
Hi Noralf,
On Wednesday, 13 September 2017 18:19:22 EEST Noralf Trønnes wrote:
Den 13.09.2017 07.09, skrev Laurent Pinchart:
On Monday, 11 September 2017 17:31:54 EEST Noralf Trønnes wrote:
Hi,
I want to start out by saying that this patchset is low priority for me and if no one has interest or time to review this, that is just fine. I was in the flow and just typed it out.
This patchset adds a way for fbdev emulation code to create a framebuffer that is backed by a dumb buffer. drm_fb_helper gets a drm_file to hang the objects on, drm_framebuffer_create_dumb() creates the framebuffer and drm_fb_helper_fini() destroys it. I have verified that all cma drivers supports dumb buffers, so converting the library should be fine for all.
Stupid question, what does this give us ? The series makes the call stack more complex (up to a point where I'm getting trouble just following it), what's the advantage that offsets that ?
The short answer is that it avoids the need for special fbdev _with_funcs() functions in libraries like cma for framebuffers with the dirty callback set.
I'm suprised that you think this more complex, I find it more coherent when fbdev userspace is doing things more like DRM userspace, like hanging the objects on a drm_file and cleaning up the objects when done.
Maybe moving to a new code base gave me the wrong feeling that the result is more complex. The current implementation is certainly not simple.
The longer and more diffuse answer is that it's annoying me that many drivers carry the burden of fbdev emulation just to get a console!
I totally agree with that. Ideally I'd like to remove 100% of fbdev-related code from drivers. This includes
- initialization and cleanup of fbdev helpers
- fbdev restore in last_close()
- forwarding of hotplug events to fbdev compatibility layer
In practice we'll likely need to keep one initialization function (or a drm_driver flag) to let drivers opt-in to fbdev compatibility, but the rest should go away. Or maybe we could even enable fbdev compatibility in all drivers unconditionally.
Interesting idea, maybe something like this:
struct drm_device { + struct drm_fb_helper *fbdev; };
void drm_lastclose(struct drm_device * dev) { DRM_DEBUG("\n");
if (dev->driver->lastclose) dev->driver->lastclose(dev); + else if (dev->fbdev) + drm_fb_helper_restore_fbdev_mode_unlocked(dev->fbdev); DRM_DEBUG("driver lastclose completed\n");
if (drm_core_check_feature(dev, DRIVER_LEGACY)) drm_legacy_dev_reinit(dev); }
void drm_kms_helper_hotplug_event(struct drm_device *dev) { /* send a uevent + call fbdev */ drm_sysfs_hotplug_event(dev); if (dev->mode_config.funcs->output_poll_changed) dev->mode_config.funcs->output_poll_changed(dev); + else if (dev->fbdev) + drm_fb_helper_hotplug_event(dev->fbdev); }
void drm_dev_unregister(struct drm_device *dev) { struct drm_map_list *r_list, *list_temp;
+ if (dev->fbdev) + drm_fb_helper_unregister_fbi(dev->fbdev); + drm_lastclose(dev); ... }
static void drm_dev_release(struct kref *ref) { struct drm_device *dev = container_of(ref, struct drm_device, ref);
+ drm_fb_helper_simple_fini(dev); + if (dev->driver->release) { dev->driver->release(dev); } else { drm_dev_fini(dev); kfree(dev); } }
int driver_probe() { ret = drm_fb_helper_simple_init(dev, ...); if (ret) DRM_WARN("Oh well fbdev init failed, but don't let that stop us\n"); }
I've slightly adjusted some code I have in the pipeline:
/** * drm_fb_helper_simple_init - Simple fbdev emulation init * @dev: DRM device * @bpp_sel: bpp value to use for the framebuffer configuration * @max_conn: max connector count * @funcs: pointer to structure of functions associated with this helper * * Simple fbdev emulation initialization. This function allocates a * &drm_fb_helper structure and calls: * drm_fb_helper_prepare(), drm_fb_helper_init(), * drm_fb_helper_single_add_all_connectors() and * drm_fb_helper_initial_config(). * * fbdev deferred I/O users should use drm_fb_helper_defio_init(). * * Returns: * Zero on success, negative error code on failure. */ int drm_fb_helper_simple_init(struct drm_device *dev, int bpp_sel, int max_conn, const struct drm_fb_helper_funcs *funcs) { struct drm_fb_helper *fb_helper; int ret;
fb_helper = kzalloc(sizeof(*fb_helper), GFP_KERNEL); if (!fb_helper) return -ENOMEM;
drm_fb_helper_prepare(dev, fb_helper, funcs);
ret = drm_fb_helper_init(dev, fb_helper, max_conn); if (ret < 0) { DRM_DEV_ERROR(dev->dev, "Failed to initialize fb helper.\n"); goto err_drm_fb_helper_free; }
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; }
dev->fbdev = fb_helper;
return 0;
err_drm_fb_helper_fini: drm_fb_helper_fini(fb_helper); err_drm_fb_helper_free: kfree(fb_helper);
return ret; } EXPORT_SYMBOL_GPL(drm_fb_helper_simple_init);
/** * drm_fb_helper_simple_fini - Simple fbdev cleanup * @dev: DRM device * * Simple fbdev emulation cleanup. This function unregisters fbdev if it is not * done, cleans up deferred IO if necessary, removes framebuffer, finalizes * @fb_helper and frees the structure. */ void drm_fb_helper_simple_fini(struct drm_device *dev) { struct drm_fb_helper *fb_helper = dev->fbdev; struct fb_ops *fbops = NULL; struct fb_info *info;
if (!fb_helper) return;
info = fb_helper->fbdev;
/* Make sure it hasn't been unregistered already */ if (info && info->dev) drm_fb_helper_unregister_fbi(fb_helper);
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);
kfree(fb_helper); } EXPORT_SYMBOL_GPL(drm_fb_helper_simple_fini);
<snip>
Den 15.09.2017 18.37, skrev Noralf Trønnes:
Den 15.09.2017 00.29, skrev Laurent Pinchart:
Hi Noralf,
On Wednesday, 13 September 2017 18:19:22 EEST Noralf Trønnes wrote:
Den 13.09.2017 07.09, skrev Laurent Pinchart:
On Monday, 11 September 2017 17:31:54 EEST Noralf Trønnes wrote:
Hi,
I want to start out by saying that this patchset is low priority for me and if no one has interest or time to review this, that is just fine. I was in the flow and just typed it out.
This patchset adds a way for fbdev emulation code to create a framebuffer that is backed by a dumb buffer. drm_fb_helper gets a drm_file to hang the objects on, drm_framebuffer_create_dumb() creates the framebuffer and drm_fb_helper_fini() destroys it. I have verified that all cma drivers supports dumb buffers, so converting the library should be fine for all.
Stupid question, what does this give us ? The series makes the call stack more complex (up to a point where I'm getting trouble just following it), what's the advantage that offsets that ?
The short answer is that it avoids the need for special fbdev _with_funcs() functions in libraries like cma for framebuffers with the dirty callback set.
I'm suprised that you think this more complex, I find it more coherent when fbdev userspace is doing things more like DRM userspace, like hanging the objects on a drm_file and cleaning up the objects when done.
Maybe moving to a new code base gave me the wrong feeling that the result is more complex. The current implementation is certainly not simple.
The longer and more diffuse answer is that it's annoying me that many drivers carry the burden of fbdev emulation just to get a console!
I totally agree with that. Ideally I'd like to remove 100% of fbdev-related code from drivers. This includes
- initialization and cleanup of fbdev helpers
- fbdev restore in last_close()
- forwarding of hotplug events to fbdev compatibility layer
In practice we'll likely need to keep one initialization function (or a drm_driver flag) to let drivers opt-in to fbdev compatibility, but the rest should go away. Or maybe we could even enable fbdev compatibility in all drivers unconditionally.
Interesting idea, maybe something like this:
I just remembered that Daniel said something about the helpers shouldn't bleed into the core, which my suggestion does :-/
struct drm_device {
- struct drm_fb_helper *fbdev;
};
void drm_lastclose(struct drm_device * dev) { DRM_DEBUG("\n");
if (dev->driver->lastclose) dev->driver->lastclose(dev); + else if (dev->fbdev) + drm_fb_helper_restore_fbdev_mode_unlocked(dev->fbdev); DRM_DEBUG("driver lastclose completed\n");
if (drm_core_check_feature(dev, DRIVER_LEGACY)) drm_legacy_dev_reinit(dev); }
void drm_kms_helper_hotplug_event(struct drm_device *dev) { /* send a uevent + call fbdev */ drm_sysfs_hotplug_event(dev); if (dev->mode_config.funcs->output_poll_changed) dev->mode_config.funcs->output_poll_changed(dev); + else if (dev->fbdev) + drm_fb_helper_hotplug_event(dev->fbdev); }
void drm_dev_unregister(struct drm_device *dev) { struct drm_map_list *r_list, *list_temp;
+ if (dev->fbdev) + drm_fb_helper_unregister_fbi(dev->fbdev);
drm_lastclose(dev); ... }
static void drm_dev_release(struct kref *ref) { struct drm_device *dev = container_of(ref, struct drm_device, ref);
+ drm_fb_helper_simple_fini(dev);
if (dev->driver->release) { dev->driver->release(dev); } else { drm_dev_fini(dev); kfree(dev); } }
int driver_probe() { ret = drm_fb_helper_simple_init(dev, ...); if (ret) DRM_WARN("Oh well fbdev init failed, but don't let that stop us\n"); }
I've slightly adjusted some code I have in the pipeline:
/** * drm_fb_helper_simple_init - Simple fbdev emulation init * @dev: DRM device * @bpp_sel: bpp value to use for the framebuffer configuration * @max_conn: max connector count * @funcs: pointer to structure of functions associated with this helper * * Simple fbdev emulation initialization. This function allocates a * &drm_fb_helper structure and calls: * drm_fb_helper_prepare(), drm_fb_helper_init(), * drm_fb_helper_single_add_all_connectors() and * drm_fb_helper_initial_config(). * * fbdev deferred I/O users should use drm_fb_helper_defio_init(). * * Returns: * Zero on success, negative error code on failure. */ int drm_fb_helper_simple_init(struct drm_device *dev, int bpp_sel, int max_conn, const struct drm_fb_helper_funcs *funcs) { struct drm_fb_helper *fb_helper; int ret;
fb_helper = kzalloc(sizeof(*fb_helper), GFP_KERNEL); if (!fb_helper) return -ENOMEM;
drm_fb_helper_prepare(dev, fb_helper, funcs);
ret = drm_fb_helper_init(dev, fb_helper, max_conn); if (ret < 0) { DRM_DEV_ERROR(dev->dev, "Failed to initialize fb helper.\n"); goto err_drm_fb_helper_free; }
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; }
dev->fbdev = fb_helper;
return 0;
err_drm_fb_helper_fini: drm_fb_helper_fini(fb_helper); err_drm_fb_helper_free: kfree(fb_helper);
return ret; } EXPORT_SYMBOL_GPL(drm_fb_helper_simple_init);
/** * drm_fb_helper_simple_fini - Simple fbdev cleanup * @dev: DRM device * * Simple fbdev emulation cleanup. This function unregisters fbdev if it is not * done, cleans up deferred IO if necessary, removes framebuffer, finalizes * @fb_helper and frees the structure. */ void drm_fb_helper_simple_fini(struct drm_device *dev) { struct drm_fb_helper *fb_helper = dev->fbdev; struct fb_ops *fbops = NULL; struct fb_info *info;
if (!fb_helper) return;
info = fb_helper->fbdev;
/* Make sure it hasn't been unregistered already */ if (info && info->dev) drm_fb_helper_unregister_fbi(fb_helper);
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);
kfree(fb_helper); } EXPORT_SYMBOL_GPL(drm_fb_helper_simple_fini);
<snip>
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Den 16.09.2017 14.37, skrev Noralf Trønnes:
Den 15.09.2017 18.37, skrev Noralf Trønnes:
Den 15.09.2017 00.29, skrev Laurent Pinchart:
Hi Noralf,
On Wednesday, 13 September 2017 18:19:22 EEST Noralf Trønnes wrote:
Den 13.09.2017 07.09, skrev Laurent Pinchart:
On Monday, 11 September 2017 17:31:54 EEST Noralf Trønnes wrote:
Hi,
I want to start out by saying that this patchset is low priority for me and if no one has interest or time to review this, that is just fine. I was in the flow and just typed it out.
This patchset adds a way for fbdev emulation code to create a framebuffer that is backed by a dumb buffer. drm_fb_helper gets a drm_file to hang the objects on, drm_framebuffer_create_dumb() creates the framebuffer and drm_fb_helper_fini() destroys it. I have verified that all cma drivers supports dumb buffers, so converting the library should be fine for all.
Stupid question, what does this give us ? The series makes the call stack more complex (up to a point where I'm getting trouble just following it), what's the advantage that offsets that ?
The short answer is that it avoids the need for special fbdev _with_funcs() functions in libraries like cma for framebuffers with the dirty callback set.
I'm suprised that you think this more complex, I find it more coherent when fbdev userspace is doing things more like DRM userspace, like hanging the objects on a drm_file and cleaning up the objects when done.
Maybe moving to a new code base gave me the wrong feeling that the result is more complex. The current implementation is certainly not simple.
The longer and more diffuse answer is that it's annoying me that many drivers carry the burden of fbdev emulation just to get a console!
I totally agree with that. Ideally I'd like to remove 100% of fbdev-related code from drivers. This includes
- initialization and cleanup of fbdev helpers
- fbdev restore in last_close()
- forwarding of hotplug events to fbdev compatibility layer
In practice we'll likely need to keep one initialization function (or a drm_driver flag) to let drivers opt-in to fbdev compatibility, but the rest should go away. Or maybe we could even enable fbdev compatibility in all drivers unconditionally.
Interesting idea, maybe something like this:
I just remembered that Daniel said something about the helpers shouldn't bleed into the core, which my suggestion does :-/
struct drm_device {
- struct drm_fb_helper *fbdev;
};
However, other helpers are allowed pointers in core structures, so if we can do that we can at least add helpers that can be used directly as callbacks:
/** * drm_fb_helper_lastclose - DRM driver lastclose helper for fbdev * @dev: DRM device * * This function can be used as the &drm_driver->lastclose callback for drivers * that only need to call drm_fb_helper_restore_fbdev_mode_unlocked(). * * Note: &drm_device->fbdev needs to be set. */ void drm_fb_helper_lastclose(struct drm_device *dev) { drm_fb_helper_restore_fbdev_mode_unlocked(dev->fbdev); } EXPORT_SYMBOL(drm_fb_helper_lastclose);
/** * drm_fb_helper_output_poll_changed - DRM mode config .output_poll_changed * helper * @dev: DRM device * * This function can be used as the * &drm_mode_config_funcs.output_poll_changed callback for drivers that only * need to call drm_fb_helper_hotplug_event(). * * Note: &drm_device->fbdev needs to be set. */ void drm_fb_helper_output_poll_changed(struct drm_device *dev) { drm_fb_helper_hotplug_event(dev->fbdev); } EXPORT_SYMBOL(drm_fb_helper_output_poll_changed);
void drm_lastclose(struct drm_device * dev) { DRM_DEBUG("\n");
if (dev->driver->lastclose) dev->driver->lastclose(dev); + else if (dev->fbdev)
- drm_fb_helper_restore_fbdev_mode_unlocked(dev->fbdev);
DRM_DEBUG("driver lastclose completed\n");
if (drm_core_check_feature(dev, DRIVER_LEGACY)) drm_legacy_dev_reinit(dev); }
void drm_kms_helper_hotplug_event(struct drm_device *dev) { /* send a uevent + call fbdev */ drm_sysfs_hotplug_event(dev); if (dev->mode_config.funcs->output_poll_changed) dev->mode_config.funcs->output_poll_changed(dev); + else if (dev->fbdev) + drm_fb_helper_hotplug_event(dev->fbdev); }
void drm_dev_unregister(struct drm_device *dev) { struct drm_map_list *r_list, *list_temp;
+ if (dev->fbdev) + drm_fb_helper_unregister_fbi(dev->fbdev);
drm_lastclose(dev); ... }
static void drm_dev_release(struct kref *ref) { struct drm_device *dev = container_of(ref, struct drm_device, ref);
+ drm_fb_helper_simple_fini(dev);
if (dev->driver->release) { dev->driver->release(dev); } else { drm_dev_fini(dev); kfree(dev); } }
int driver_probe() { ret = drm_fb_helper_simple_init(dev, ...); if (ret) DRM_WARN("Oh well fbdev init failed, but don't let that stop us\n"); }
I've slightly adjusted some code I have in the pipeline:
/** * drm_fb_helper_simple_init - Simple fbdev emulation init * @dev: DRM device * @bpp_sel: bpp value to use for the framebuffer configuration * @max_conn: max connector count * @funcs: pointer to structure of functions associated with this helper * * Simple fbdev emulation initialization. This function allocates a * &drm_fb_helper structure and calls: * drm_fb_helper_prepare(), drm_fb_helper_init(), * drm_fb_helper_single_add_all_connectors() and * drm_fb_helper_initial_config(). * * fbdev deferred I/O users should use drm_fb_helper_defio_init(). * * Returns: * Zero on success, negative error code on failure. */ int drm_fb_helper_simple_init(struct drm_device *dev, int bpp_sel, int max_conn, const struct drm_fb_helper_funcs *funcs) { struct drm_fb_helper *fb_helper; int ret;
fb_helper = kzalloc(sizeof(*fb_helper), GFP_KERNEL); if (!fb_helper) return -ENOMEM;
drm_fb_helper_prepare(dev, fb_helper, funcs);
ret = drm_fb_helper_init(dev, fb_helper, max_conn); if (ret < 0) { DRM_DEV_ERROR(dev->dev, "Failed to initialize fb helper.\n"); goto err_drm_fb_helper_free; }
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; }
dev->fbdev = fb_helper;
return 0;
err_drm_fb_helper_fini: drm_fb_helper_fini(fb_helper); err_drm_fb_helper_free: kfree(fb_helper);
return ret; } EXPORT_SYMBOL_GPL(drm_fb_helper_simple_init);
/** * drm_fb_helper_simple_fini - Simple fbdev cleanup * @dev: DRM device * * Simple fbdev emulation cleanup. This function unregisters fbdev if it is not * done, cleans up deferred IO if necessary, removes framebuffer, finalizes * @fb_helper and frees the structure. */ void drm_fb_helper_simple_fini(struct drm_device *dev) { struct drm_fb_helper *fb_helper = dev->fbdev; struct fb_ops *fbops = NULL; struct fb_info *info;
if (!fb_helper) return;
info = fb_helper->fbdev;
/* Make sure it hasn't been unregistered already */ if (info && info->dev) drm_fb_helper_unregister_fbi(fb_helper);
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);
kfree(fb_helper); } EXPORT_SYMBOL_GPL(drm_fb_helper_simple_fini);
<snip>
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
kernel test robot -
Laurent Pinchart -
Noralf Trønnes