* [RFC v2 1/8] drm: provide management functions for drm_file
2018-01-03 22:21 [RFC v2 0/8] drm: Add generic fbdev emulation Noralf Trønnes
@ 2018-01-03 22:21 ` Noralf Trønnes
2018-01-09 10:20 ` Daniel Vetter
2018-01-03 22:21 ` [RFC v2 2/8] drm/ioctl: Remove trailing whitespace Noralf Trønnes
` (7 subsequent siblings)
8 siblings, 1 reply; 26+ messages in thread
From: Noralf Trønnes @ 2018-01-03 22:21 UTC (permalink / raw)
To: dri-devel
Cc: daniel.vetter, intel-gfx, Noralf Trønnes, laurent.pinchart,
dh.herrmann
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]
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
drivers/gpu/drm/drm_file.c | 309 +++++++++++++++++++++++------------------
drivers/gpu/drm/drm_internal.h | 2 +
2 files changed, 179 insertions(+), 132 deletions(-)
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index b3c6e997ccdb..d208faade27e 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -101,6 +101,179 @@ 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);
+}
+
+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);
+}
+
+/**
+ * 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;
+
+ DRM_DEBUG("pid = %d, device = 0x%lx, open_count = %d\n",
+ task_pid_nr(current),
+ (long)old_encode_dev(file->minor->kdev->devt),
+ dev->open_count);
+
+ 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);
+}
+
static int drm_setup(struct drm_device * dev)
{
int ret;
@@ -196,7 +369,6 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor)
{
struct drm_device *dev = minor->dev;
struct drm_file *priv;
- int ret;
if (filp->f_flags & O_EXCL)
return -EBUSY; /* No exclusive opens */
@@ -207,50 +379,12 @@ 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;
+ priv = drm_file_alloc(minor);
+ if (IS_ERR(priv))
+ return PTR_ERR(priv);
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;
- }
mutex_lock(&dev->filelist_mutex);
list_add(&priv->lhead, &dev->filelist);
@@ -277,45 +411,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)
@@ -382,57 +477,7 @@ int drm_release(struct inode *inode, struct file *filp)
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),
- dev->open_count);
-
- if (drm_core_check_feature(dev, DRIVER_LEGACY))
- drm_legacy_lock_release(dev, filp);
-
- 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_priv);
if (!--dev->open_count) {
drm_lastclose(dev);
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index b72242e93ea4..40179c5fc6b8 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -26,6 +26,8 @@
/* drm_file.c */
extern struct mutex drm_global_mutex;
+struct drm_file *drm_file_alloc(struct drm_minor *minor);
+void drm_file_free(struct drm_file *file);
void drm_lastclose(struct drm_device *dev);
/* drm_pci.c */
--
2.14.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [RFC v2 1/8] drm: provide management functions for drm_file
2018-01-03 22:21 ` [RFC v2 1/8] drm: provide management functions for drm_file Noralf Trønnes
@ 2018-01-09 10:20 ` Daniel Vetter
2018-01-09 10:32 ` David Herrmann
0 siblings, 1 reply; 26+ messages in thread
From: Daniel Vetter @ 2018-01-09 10:20 UTC (permalink / raw)
To: Noralf Trønnes; +Cc: daniel.vetter, intel-gfx, dri-devel, laurent.pinchart
On Wed, Jan 03, 2018 at 11:21:03PM +0100, Noralf Trønnes wrote:
> 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]
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
> drivers/gpu/drm/drm_file.c | 309 +++++++++++++++++++++++------------------
> drivers/gpu/drm/drm_internal.h | 2 +
> 2 files changed, 179 insertions(+), 132 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index b3c6e997ccdb..d208faade27e 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -101,6 +101,179 @@ 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.
I'd remove this line, since it's only relevant to close the driver load
vs. file open race for drivers still using the deprecated ->load callback.
This only confuses and doesn't help anyone writing a new/modern driver.
> + *
> + * 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);
> +}
> +
> +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);
> +}
> +
> +/**
> + * 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.
Same here.
With the bikesheds address and maybe the EXPORT_SYMBOL for these from
patch 3 moved to here:
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> + *
> + * 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;
> +
> + DRM_DEBUG("pid = %d, device = 0x%lx, open_count = %d\n",
> + task_pid_nr(current),
> + (long)old_encode_dev(file->minor->kdev->devt),
> + dev->open_count);
> +
> + 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);
> +}
> +
> static int drm_setup(struct drm_device * dev)
> {
> int ret;
> @@ -196,7 +369,6 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor)
> {
> struct drm_device *dev = minor->dev;
> struct drm_file *priv;
> - int ret;
>
> if (filp->f_flags & O_EXCL)
> return -EBUSY; /* No exclusive opens */
> @@ -207,50 +379,12 @@ 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;
> + priv = drm_file_alloc(minor);
> + if (IS_ERR(priv))
> + return PTR_ERR(priv);
>
> 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;
> - }
>
> mutex_lock(&dev->filelist_mutex);
> list_add(&priv->lhead, &dev->filelist);
> @@ -277,45 +411,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)
> @@ -382,57 +477,7 @@ int drm_release(struct inode *inode, struct file *filp)
> 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),
> - dev->open_count);
> -
> - if (drm_core_check_feature(dev, DRIVER_LEGACY))
> - drm_legacy_lock_release(dev, filp);
> -
> - 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_priv);
>
> if (!--dev->open_count) {
> drm_lastclose(dev);
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index b72242e93ea4..40179c5fc6b8 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -26,6 +26,8 @@
>
> /* drm_file.c */
> extern struct mutex drm_global_mutex;
> +struct drm_file *drm_file_alloc(struct drm_minor *minor);
> +void drm_file_free(struct drm_file *file);
> void drm_lastclose(struct drm_device *dev);
>
> /* drm_pci.c */
> --
> 2.14.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [RFC v2 1/8] drm: provide management functions for drm_file
2018-01-09 10:20 ` Daniel Vetter
@ 2018-01-09 10:32 ` David Herrmann
0 siblings, 0 replies; 26+ messages in thread
From: David Herrmann @ 2018-01-09 10:32 UTC (permalink / raw)
To: Daniel Vetter
Cc: Daniel Vetter, Intel Graphics Development, dri-devel,
Laurent Pinchart
Hi
On Tue, Jan 9, 2018 at 11:20 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Jan 03, 2018 at 11:21:03PM +0100, Noralf Trønnes wrote:
>> 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]
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
For completeness: Still fine with me! Thanks for picking it up.
Thanks
David
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* [RFC v2 2/8] drm/ioctl: Remove trailing whitespace
2018-01-03 22:21 [RFC v2 0/8] drm: Add generic fbdev emulation Noralf Trønnes
2018-01-03 22:21 ` [RFC v2 1/8] drm: provide management functions for drm_file Noralf Trønnes
@ 2018-01-03 22:21 ` Noralf Trønnes
2018-01-09 10:18 ` Daniel Vetter
2018-01-09 14:49 ` Laurent Pinchart
2018-01-03 22:21 ` [RFC v2 3/8] drm: Export some ioctl functions Noralf Trønnes
` (6 subsequent siblings)
8 siblings, 2 replies; 26+ messages in thread
From: Noralf Trønnes @ 2018-01-03 22:21 UTC (permalink / raw)
To: dri-devel; +Cc: daniel.vetter, intel-gfx, laurent.pinchart
Remove a couple of trailing spaces.
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
drivers/gpu/drm/drm_ioctl.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 4aafe4802099..b1e96fb68ea8 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -509,7 +509,7 @@ int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
return -EACCES;
/* MASTER is only for master or control clients */
- if (unlikely((flags & DRM_MASTER) &&
+ if (unlikely((flags & DRM_MASTER) &&
!drm_is_current_master(file_priv) &&
!drm_is_control_client(file_priv)))
return -EACCES;
@@ -704,7 +704,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
*
* ##define DRM_IOCTL_MY_DRIVER_OPERATION \
* DRM_IOW(DRM_COMMAND_BASE, struct my_driver_operation)
- *
+ *
* DRM driver private IOCTL must be in the range from DRM_COMMAND_BASE to
* DRM_COMMAND_END. Finally you need an array of &struct drm_ioctl_desc to wire
* up the handlers and set the access rights::
--
2.14.2
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [RFC v2 2/8] drm/ioctl: Remove trailing whitespace
2018-01-03 22:21 ` [RFC v2 2/8] drm/ioctl: Remove trailing whitespace Noralf Trønnes
@ 2018-01-09 10:18 ` Daniel Vetter
2018-01-09 14:49 ` Laurent Pinchart
1 sibling, 0 replies; 26+ messages in thread
From: Daniel Vetter @ 2018-01-09 10:18 UTC (permalink / raw)
To: Noralf Trønnes
Cc: daniel.vetter, intel-gfx, dri-devel, laurent.pinchart,
dh.herrmann
On Wed, Jan 03, 2018 at 11:21:04PM +0100, Noralf Trønnes wrote:
> Remove a couple of trailing spaces.
>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/gpu/drm/drm_ioctl.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 4aafe4802099..b1e96fb68ea8 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -509,7 +509,7 @@ int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
> return -EACCES;
>
> /* MASTER is only for master or control clients */
> - if (unlikely((flags & DRM_MASTER) &&
> + if (unlikely((flags & DRM_MASTER) &&
> !drm_is_current_master(file_priv) &&
> !drm_is_control_client(file_priv)))
> return -EACCES;
> @@ -704,7 +704,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
> *
> * ##define DRM_IOCTL_MY_DRIVER_OPERATION \
> * DRM_IOW(DRM_COMMAND_BASE, struct my_driver_operation)
> - *
> + *
> * DRM driver private IOCTL must be in the range from DRM_COMMAND_BASE to
> * DRM_COMMAND_END. Finally you need an array of &struct drm_ioctl_desc to wire
> * up the handlers and set the access rights::
> --
> 2.14.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [RFC v2 2/8] drm/ioctl: Remove trailing whitespace
2018-01-03 22:21 ` [RFC v2 2/8] drm/ioctl: Remove trailing whitespace Noralf Trønnes
2018-01-09 10:18 ` Daniel Vetter
@ 2018-01-09 14:49 ` Laurent Pinchart
1 sibling, 0 replies; 26+ messages in thread
From: Laurent Pinchart @ 2018-01-09 14:49 UTC (permalink / raw)
To: Noralf Trønnes; +Cc: daniel.vetter, intel-gfx, dri-devel
Hi Noralf,
Thank you for the patch.
On Thursday, 4 January 2018 00:21:04 EET Noralf Trønnes wrote:
> Remove a couple of trailing spaces.
>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/gpu/drm/drm_ioctl.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 4aafe4802099..b1e96fb68ea8 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -509,7 +509,7 @@ int drm_ioctl_permit(u32 flags, struct drm_file
> *file_priv) return -EACCES;
>
> /* MASTER is only for master or control clients */
> - if (unlikely((flags & DRM_MASTER) &&
> + if (unlikely((flags & DRM_MASTER) &&
> !drm_is_current_master(file_priv) &&
> !drm_is_control_client(file_priv)))
> return -EACCES;
> @@ -704,7 +704,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
> *
> * ##define DRM_IOCTL_MY_DRIVER_OPERATION \
> * DRM_IOW(DRM_COMMAND_BASE, struct my_driver_operation)
> - *
> + *
> * DRM driver private IOCTL must be in the range from DRM_COMMAND_BASE to
> * DRM_COMMAND_END. Finally you need an array of &struct drm_ioctl_desc to
> wire * up the handlers and set the access rights::
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* [RFC v2 3/8] drm: Export some ioctl functions
2018-01-03 22:21 [RFC v2 0/8] drm: Add generic fbdev emulation Noralf Trønnes
2018-01-03 22:21 ` [RFC v2 1/8] drm: provide management functions for drm_file Noralf Trønnes
2018-01-03 22:21 ` [RFC v2 2/8] drm/ioctl: Remove trailing whitespace Noralf Trønnes
@ 2018-01-03 22:21 ` Noralf Trønnes
2018-01-09 10:16 ` Daniel Vetter
2018-01-09 14:48 ` Laurent Pinchart
2018-01-03 22:21 ` [RFC v2 4/8] drm/fb-helper: Ensure driver module is pinned in fb_open() Noralf Trønnes
` (5 subsequent siblings)
8 siblings, 2 replies; 26+ messages in thread
From: Noralf Trønnes @ 2018-01-03 22:21 UTC (permalink / raw)
To: dri-devel
Cc: daniel.vetter, intel-gfx, Noralf Trønnes, laurent.pinchart,
dh.herrmann
Export the following functions so in-kernel users can allocate
dumb buffers:
- drm_file_alloc
- drm_file_free
- drm_prime_handle_to_fd_ioctl
- drm_mode_addfb2
- drm_mode_create_dumb_ioctl
- drm_dropmaster_ioctl
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
drivers/gpu/drm/drm_auth.c | 1 +
drivers/gpu/drm/drm_crtc_internal.h | 4 ----
drivers/gpu/drm/drm_dumb_buffers.c | 1 +
drivers/gpu/drm/drm_file.c | 2 ++
drivers/gpu/drm/drm_framebuffer.c | 1 +
drivers/gpu/drm/drm_internal.h | 6 ------
drivers/gpu/drm/drm_ioctl.c | 1 +
drivers/gpu/drm/drm_prime.c | 1 +
include/drm/drm_auth.h | 3 +++
include/drm/drm_dumb_buffers.h | 10 ++++++++++
include/drm/drm_file.h | 2 ++
include/drm/drm_framebuffer.h | 3 +++
include/drm/drm_prime.h | 2 ++
13 files changed, 27 insertions(+), 10 deletions(-)
create mode 100644 include/drm/drm_dumb_buffers.h
diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index aad468d170a7..e35ed9ee0c5a 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -236,6 +236,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_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 9ebb8841778c..86422492ad00 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -63,8 +63,6 @@ int drm_mode_getresources(struct drm_device *dev,
/* drm_dumb_buffers.c */
/* IOCTLs */
-int drm_mode_create_dumb_ioctl(struct drm_device *dev,
- void *data, struct drm_file *file_priv);
int drm_mode_mmap_dumb_ioctl(struct drm_device *dev,
void *data, struct drm_file *file_priv);
int drm_mode_destroy_dumb_ioctl(struct drm_device *dev,
@@ -164,8 +162,6 @@ void drm_fb_release(struct drm_file *file_priv);
/* IOCTL */
int drm_mode_addfb(struct drm_device *dev,
void *data, struct drm_file *file_priv);
-int drm_mode_addfb2(struct drm_device *dev,
- void *data, struct drm_file *file_priv);
int drm_mode_rmfb(struct drm_device *dev,
void *data, struct drm_file *file_priv);
int drm_mode_getfb(struct drm_device *dev,
diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
index 39ac15ce4702..199b279f7650 100644
--- a/drivers/gpu/drm/drm_dumb_buffers.c
+++ b/drivers/gpu/drm/drm_dumb_buffers.c
@@ -90,6 +90,7 @@ int drm_mode_create_dumb_ioctl(struct drm_device *dev,
return dev->driver->dumb_create(file_priv, dev, args);
}
+EXPORT_SYMBOL(drm_mode_create_dumb_ioctl);
/**
* drm_mode_mmap_dumb_ioctl - create an mmap offset for a dumb backing storage buffer
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index d208faade27e..400d44437e93 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -180,6 +180,7 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
return ERR_PTR(ret);
}
+EXPORT_SYMBOL(drm_file_alloc);
static void drm_events_release(struct drm_file *file_priv)
{
@@ -273,6 +274,7 @@ void drm_file_free(struct drm_file *file)
put_pid(file->pid);
kfree(file);
}
+EXPORT_SYMBOL(drm_file_free);
static int drm_setup(struct drm_device * dev)
{
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 5a13ff29f4f0..0493977e6848 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -341,6 +341,7 @@ int drm_mode_addfb2(struct drm_device *dev,
return 0;
}
+EXPORT_SYMBOL(drm_mode_addfb2);
struct drm_mode_rmfb_work {
struct work_struct work;
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 40179c5fc6b8..7d62e412fbb8 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -26,8 +26,6 @@
/* drm_file.c */
extern struct mutex drm_global_mutex;
-struct drm_file *drm_file_alloc(struct drm_minor *minor);
-void drm_file_free(struct drm_file *file);
void drm_lastclose(struct drm_device *dev);
/* drm_pci.c */
@@ -37,8 +35,6 @@ void drm_pci_agp_destroy(struct drm_device *dev);
int drm_pci_set_busid(struct drm_device *dev, struct drm_master *master);
/* drm_prime.c */
-int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
- struct drm_file *file_priv);
int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_priv);
@@ -85,8 +81,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/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index b1e96fb68ea8..ea8f6ca8b449 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -31,6 +31,7 @@
#include <drm/drm_ioctl.h>
#include <drm/drmP.h>
#include <drm/drm_auth.h>
+#include <drm/drm_dumb_buffers.h>
#include "drm_legacy.h"
#include "drm_internal.h"
#include "drm_crtc_internal.h"
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 9a17725b0f7a..1bfc7c9d6127 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -774,6 +774,7 @@ int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
return dev->driver->prime_handle_to_fd(dev, file_priv,
args->handle, args->flags, &args->fd);
}
+EXPORT_SYMBOL(drm_prime_handle_to_fd_ioctl);
int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_priv)
diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h
index 86bff9841b54..fdc3fed50daf 100644
--- a/include/drm/drm_auth.h
+++ b/include/drm/drm_auth.h
@@ -103,4 +103,7 @@ bool drm_is_current_master(struct drm_file *fpriv);
struct drm_master *drm_master_create(struct drm_device *dev);
+int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_priv);
+
#endif
diff --git a/include/drm/drm_dumb_buffers.h b/include/drm/drm_dumb_buffers.h
new file mode 100644
index 000000000000..c1138c1c06ab
--- /dev/null
+++ b/include/drm/drm_dumb_buffers.h
@@ -0,0 +1,10 @@
+#ifndef _DRM_DUMB_BUFFERS_H_
+#define _DRM_DUMB_BUFFERS_H_
+
+struct drm_device;
+struct drm_file;
+
+int drm_mode_create_dumb_ioctl(struct drm_device *dev,
+ void *data, struct drm_file *file_priv);
+
+#endif
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index 0e0c868451a5..23d90744c3d9 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);
diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
index c50502c656e5..6c54db1c23ae 100644
--- a/include/drm/drm_framebuffer.h
+++ b/include/drm/drm_framebuffer.h
@@ -313,4 +313,7 @@ int drm_framebuffer_plane_width(int width,
int drm_framebuffer_plane_height(int height,
const struct drm_framebuffer *fb, int plane);
+int drm_mode_addfb2(struct drm_device *dev,
+ void *data, struct drm_file *file_priv);
+
#endif
diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h
index 9cd9e36f77b5..ca7bc50af2d7 100644
--- a/include/drm/drm_prime.h
+++ b/include/drm/drm_prime.h
@@ -85,5 +85,7 @@ int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages,
struct sg_table *drm_prime_pages_to_sg(struct page **pages, unsigned int nr_pages);
void drm_prime_gem_destroy(struct drm_gem_object *obj, struct sg_table *sg);
+int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_priv);
#endif /* __DRM_PRIME_H__ */
--
2.14.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [RFC v2 3/8] drm: Export some ioctl functions
2018-01-03 22:21 ` [RFC v2 3/8] drm: Export some ioctl functions Noralf Trønnes
@ 2018-01-09 10:16 ` Daniel Vetter
2018-01-09 10:31 ` David Herrmann
2018-01-09 14:48 ` Laurent Pinchart
1 sibling, 1 reply; 26+ messages in thread
From: Daniel Vetter @ 2018-01-09 10:16 UTC (permalink / raw)
To: Noralf Trønnes; +Cc: daniel.vetter, intel-gfx, dri-devel, laurent.pinchart
On Wed, Jan 03, 2018 at 11:21:05PM +0100, Noralf Trønnes wrote:
> Export the following functions so in-kernel users can allocate
> dumb buffers:
> - drm_file_alloc
> - drm_file_free
> - drm_prime_handle_to_fd_ioctl
> - drm_mode_addfb2
> - drm_mode_create_dumb_ioctl
> - drm_dropmaster_ioctl
>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
> drivers/gpu/drm/drm_auth.c | 1 +
> drivers/gpu/drm/drm_crtc_internal.h | 4 ----
> drivers/gpu/drm/drm_dumb_buffers.c | 1 +
> drivers/gpu/drm/drm_file.c | 2 ++
> drivers/gpu/drm/drm_framebuffer.c | 1 +
> drivers/gpu/drm/drm_internal.h | 6 ------
> drivers/gpu/drm/drm_ioctl.c | 1 +
> drivers/gpu/drm/drm_prime.c | 1 +
> include/drm/drm_auth.h | 3 +++
> include/drm/drm_dumb_buffers.h | 10 ++++++++++
> include/drm/drm_file.h | 2 ++
> include/drm/drm_framebuffer.h | 3 +++
> include/drm/drm_prime.h | 2 ++
> 13 files changed, 27 insertions(+), 10 deletions(-)
> create mode 100644 include/drm/drm_dumb_buffers.h
>
> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> index aad468d170a7..e35ed9ee0c5a 100644
> --- a/drivers/gpu/drm/drm_auth.c
> +++ b/drivers/gpu/drm/drm_auth.c
> @@ -236,6 +236,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_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> index 9ebb8841778c..86422492ad00 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -63,8 +63,6 @@ int drm_mode_getresources(struct drm_device *dev,
>
> /* drm_dumb_buffers.c */
> /* IOCTLs */
> -int drm_mode_create_dumb_ioctl(struct drm_device *dev,
> - void *data, struct drm_file *file_priv);
> int drm_mode_mmap_dumb_ioctl(struct drm_device *dev,
> void *data, struct drm_file *file_priv);
> int drm_mode_destroy_dumb_ioctl(struct drm_device *dev,
> @@ -164,8 +162,6 @@ void drm_fb_release(struct drm_file *file_priv);
> /* IOCTL */
> int drm_mode_addfb(struct drm_device *dev,
> void *data, struct drm_file *file_priv);
> -int drm_mode_addfb2(struct drm_device *dev,
> - void *data, struct drm_file *file_priv);
> int drm_mode_rmfb(struct drm_device *dev,
> void *data, struct drm_file *file_priv);
> int drm_mode_getfb(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
> index 39ac15ce4702..199b279f7650 100644
> --- a/drivers/gpu/drm/drm_dumb_buffers.c
> +++ b/drivers/gpu/drm/drm_dumb_buffers.c
> @@ -90,6 +90,7 @@ int drm_mode_create_dumb_ioctl(struct drm_device *dev,
>
> return dev->driver->dumb_create(file_priv, dev, args);
> }
> +EXPORT_SYMBOL(drm_mode_create_dumb_ioctl);
>
> /**
> * drm_mode_mmap_dumb_ioctl - create an mmap offset for a dumb backing storage buffer
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index d208faade27e..400d44437e93 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -180,6 +180,7 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
>
> return ERR_PTR(ret);
> }
> +EXPORT_SYMBOL(drm_file_alloc);
>
> static void drm_events_release(struct drm_file *file_priv)
> {
> @@ -273,6 +274,7 @@ void drm_file_free(struct drm_file *file)
> put_pid(file->pid);
> kfree(file);
> }
> +EXPORT_SYMBOL(drm_file_free);
I'd put the EXPORT_SYMBOL for the drm_file stuff into the first patch.
That way it's next to the kerneldoc and code all in the same patch.
>
> static int drm_setup(struct drm_device * dev)
> {
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 5a13ff29f4f0..0493977e6848 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -341,6 +341,7 @@ int drm_mode_addfb2(struct drm_device *dev,
>
> return 0;
> }
> +EXPORT_SYMBOL(drm_mode_addfb2);
>
> struct drm_mode_rmfb_work {
> struct work_struct work;
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index 40179c5fc6b8..7d62e412fbb8 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -26,8 +26,6 @@
>
> /* drm_file.c */
> extern struct mutex drm_global_mutex;
> -struct drm_file *drm_file_alloc(struct drm_minor *minor);
> -void drm_file_free(struct drm_file *file);
> void drm_lastclose(struct drm_device *dev);
>
> /* drm_pci.c */
> @@ -37,8 +35,6 @@ void drm_pci_agp_destroy(struct drm_device *dev);
> int drm_pci_set_busid(struct drm_device *dev, struct drm_master *master);
>
> /* drm_prime.c */
> -int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
> - struct drm_file *file_priv);
> int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file_priv);
>
> @@ -85,8 +81,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/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index b1e96fb68ea8..ea8f6ca8b449 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -31,6 +31,7 @@
> #include <drm/drm_ioctl.h>
> #include <drm/drmP.h>
> #include <drm/drm_auth.h>
> +#include <drm/drm_dumb_buffers.h>
> #include "drm_legacy.h"
> #include "drm_internal.h"
> #include "drm_crtc_internal.h"
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 9a17725b0f7a..1bfc7c9d6127 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -774,6 +774,7 @@ int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
> return dev->driver->prime_handle_to_fd(dev, file_priv,
> args->handle, args->flags, &args->fd);
> }
> +EXPORT_SYMBOL(drm_prime_handle_to_fd_ioctl);
>
> int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file_priv)
> diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h
> index 86bff9841b54..fdc3fed50daf 100644
> --- a/include/drm/drm_auth.h
> +++ b/include/drm/drm_auth.h
> @@ -103,4 +103,7 @@ bool drm_is_current_master(struct drm_file *fpriv);
>
> struct drm_master *drm_master_create(struct drm_device *dev);
>
> +int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_priv);
Hm, so this looks reasonable as an internal function except for the void
*data. I think for making ioctl calls available internally we should:
- have the _ioctl variant for the ioctl table, which takes void *data.
That one stays internal to drm.ko
- Move the real implementation into a function without the _ioctl suffix,
which takes the casted pointer (and has kerneldoc and all the neat
things).
- Also maybe let's drop the uapi version suffix internall too, so
drm_addfb instead of drm_addfb2.
- I wonder whether we should drop the drm_device *dev too while at it
(from the internal callback), it's kinda redundant.
All in the name of a neat&tidy internal api.
Besides this polish, looks all reasonable.
-Daniel
> +
> #endif
> diff --git a/include/drm/drm_dumb_buffers.h b/include/drm/drm_dumb_buffers.h
> new file mode 100644
> index 000000000000..c1138c1c06ab
> --- /dev/null
> +++ b/include/drm/drm_dumb_buffers.h
> @@ -0,0 +1,10 @@
> +#ifndef _DRM_DUMB_BUFFERS_H_
> +#define _DRM_DUMB_BUFFERS_H_
> +
> +struct drm_device;
> +struct drm_file;
> +
> +int drm_mode_create_dumb_ioctl(struct drm_device *dev,
> + void *data, struct drm_file *file_priv);
> +
> +#endif
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index 0e0c868451a5..23d90744c3d9 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);
> diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
> index c50502c656e5..6c54db1c23ae 100644
> --- a/include/drm/drm_framebuffer.h
> +++ b/include/drm/drm_framebuffer.h
> @@ -313,4 +313,7 @@ int drm_framebuffer_plane_width(int width,
> int drm_framebuffer_plane_height(int height,
> const struct drm_framebuffer *fb, int plane);
>
> +int drm_mode_addfb2(struct drm_device *dev,
> + void *data, struct drm_file *file_priv);
> +
> #endif
> diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h
> index 9cd9e36f77b5..ca7bc50af2d7 100644
> --- a/include/drm/drm_prime.h
> +++ b/include/drm/drm_prime.h
> @@ -85,5 +85,7 @@ int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages,
> struct sg_table *drm_prime_pages_to_sg(struct page **pages, unsigned int nr_pages);
> void drm_prime_gem_destroy(struct drm_gem_object *obj, struct sg_table *sg);
>
> +int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_priv);
>
> #endif /* __DRM_PRIME_H__ */
> --
> 2.14.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [RFC v2 3/8] drm: Export some ioctl functions
2018-01-09 10:16 ` Daniel Vetter
@ 2018-01-09 10:31 ` David Herrmann
0 siblings, 0 replies; 26+ messages in thread
From: David Herrmann @ 2018-01-09 10:31 UTC (permalink / raw)
To: Daniel Vetter
Cc: Daniel Vetter, Intel Graphics Development, dri-devel,
Noralf Trønnes, Laurent Pinchart
Hi
On Tue, Jan 9, 2018 at 11:16 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Jan 03, 2018 at 11:21:05PM +0100, Noralf Trønnes wrote:
>> Export the following functions so in-kernel users can allocate
>> dumb buffers:
>> - drm_file_alloc
>> - drm_file_free
>> - drm_prime_handle_to_fd_ioctl
>> - drm_mode_addfb2
>> - drm_mode_create_dumb_ioctl
>> - drm_dropmaster_ioctl
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>> drivers/gpu/drm/drm_auth.c | 1 +
>> drivers/gpu/drm/drm_crtc_internal.h | 4 ----
>> drivers/gpu/drm/drm_dumb_buffers.c | 1 +
>> drivers/gpu/drm/drm_file.c | 2 ++
>> drivers/gpu/drm/drm_framebuffer.c | 1 +
>> drivers/gpu/drm/drm_internal.h | 6 ------
>> drivers/gpu/drm/drm_ioctl.c | 1 +
>> drivers/gpu/drm/drm_prime.c | 1 +
>> include/drm/drm_auth.h | 3 +++
>> include/drm/drm_dumb_buffers.h | 10 ++++++++++
>> include/drm/drm_file.h | 2 ++
>> include/drm/drm_framebuffer.h | 3 +++
>> include/drm/drm_prime.h | 2 ++
>> 13 files changed, 27 insertions(+), 10 deletions(-)
>> create mode 100644 include/drm/drm_dumb_buffers.h
>>
>> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
>> index aad468d170a7..e35ed9ee0c5a 100644
>> --- a/drivers/gpu/drm/drm_auth.c
>> +++ b/drivers/gpu/drm/drm_auth.c
>> @@ -236,6 +236,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_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
>> index 9ebb8841778c..86422492ad00 100644
>> --- a/drivers/gpu/drm/drm_crtc_internal.h
>> +++ b/drivers/gpu/drm/drm_crtc_internal.h
>> @@ -63,8 +63,6 @@ int drm_mode_getresources(struct drm_device *dev,
>>
>> /* drm_dumb_buffers.c */
>> /* IOCTLs */
>> -int drm_mode_create_dumb_ioctl(struct drm_device *dev,
>> - void *data, struct drm_file *file_priv);
>> int drm_mode_mmap_dumb_ioctl(struct drm_device *dev,
>> void *data, struct drm_file *file_priv);
>> int drm_mode_destroy_dumb_ioctl(struct drm_device *dev,
>> @@ -164,8 +162,6 @@ void drm_fb_release(struct drm_file *file_priv);
>> /* IOCTL */
>> int drm_mode_addfb(struct drm_device *dev,
>> void *data, struct drm_file *file_priv);
>> -int drm_mode_addfb2(struct drm_device *dev,
>> - void *data, struct drm_file *file_priv);
>> int drm_mode_rmfb(struct drm_device *dev,
>> void *data, struct drm_file *file_priv);
>> int drm_mode_getfb(struct drm_device *dev,
>> diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
>> index 39ac15ce4702..199b279f7650 100644
>> --- a/drivers/gpu/drm/drm_dumb_buffers.c
>> +++ b/drivers/gpu/drm/drm_dumb_buffers.c
>> @@ -90,6 +90,7 @@ int drm_mode_create_dumb_ioctl(struct drm_device *dev,
>>
>> return dev->driver->dumb_create(file_priv, dev, args);
>> }
>> +EXPORT_SYMBOL(drm_mode_create_dumb_ioctl);
>>
>> /**
>> * drm_mode_mmap_dumb_ioctl - create an mmap offset for a dumb backing storage buffer
>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>> index d208faade27e..400d44437e93 100644
>> --- a/drivers/gpu/drm/drm_file.c
>> +++ b/drivers/gpu/drm/drm_file.c
>> @@ -180,6 +180,7 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
>>
>> return ERR_PTR(ret);
>> }
>> +EXPORT_SYMBOL(drm_file_alloc);
>>
>> static void drm_events_release(struct drm_file *file_priv)
>> {
>> @@ -273,6 +274,7 @@ void drm_file_free(struct drm_file *file)
>> put_pid(file->pid);
>> kfree(file);
>> }
>> +EXPORT_SYMBOL(drm_file_free);
>
> I'd put the EXPORT_SYMBOL for the drm_file stuff into the first patch.
> That way it's next to the kerneldoc and code all in the same patch.
I agree. Feel free to amend my patch there.
>>
>> static int drm_setup(struct drm_device * dev)
>> {
>> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
>> index 5a13ff29f4f0..0493977e6848 100644
>> --- a/drivers/gpu/drm/drm_framebuffer.c
>> +++ b/drivers/gpu/drm/drm_framebuffer.c
>> @@ -341,6 +341,7 @@ int drm_mode_addfb2(struct drm_device *dev,
>>
>> return 0;
>> }
>> +EXPORT_SYMBOL(drm_mode_addfb2);
>>
>> struct drm_mode_rmfb_work {
>> struct work_struct work;
>> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
>> index 40179c5fc6b8..7d62e412fbb8 100644
>> --- a/drivers/gpu/drm/drm_internal.h
>> +++ b/drivers/gpu/drm/drm_internal.h
>> @@ -26,8 +26,6 @@
>>
>> /* drm_file.c */
>> extern struct mutex drm_global_mutex;
>> -struct drm_file *drm_file_alloc(struct drm_minor *minor);
>> -void drm_file_free(struct drm_file *file);
>> void drm_lastclose(struct drm_device *dev);
>>
>> /* drm_pci.c */
>> @@ -37,8 +35,6 @@ void drm_pci_agp_destroy(struct drm_device *dev);
>> int drm_pci_set_busid(struct drm_device *dev, struct drm_master *master);
>>
>> /* drm_prime.c */
>> -int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
>> - struct drm_file *file_priv);
>> int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data,
>> struct drm_file *file_priv);
>>
>> @@ -85,8 +81,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/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
>> index b1e96fb68ea8..ea8f6ca8b449 100644
>> --- a/drivers/gpu/drm/drm_ioctl.c
>> +++ b/drivers/gpu/drm/drm_ioctl.c
>> @@ -31,6 +31,7 @@
>> #include <drm/drm_ioctl.h>
>> #include <drm/drmP.h>
>> #include <drm/drm_auth.h>
>> +#include <drm/drm_dumb_buffers.h>
>> #include "drm_legacy.h"
>> #include "drm_internal.h"
>> #include "drm_crtc_internal.h"
>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>> index 9a17725b0f7a..1bfc7c9d6127 100644
>> --- a/drivers/gpu/drm/drm_prime.c
>> +++ b/drivers/gpu/drm/drm_prime.c
>> @@ -774,6 +774,7 @@ int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
>> return dev->driver->prime_handle_to_fd(dev, file_priv,
>> args->handle, args->flags, &args->fd);
>> }
>> +EXPORT_SYMBOL(drm_prime_handle_to_fd_ioctl);
>>
>> int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data,
>> struct drm_file *file_priv)
>> diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h
>> index 86bff9841b54..fdc3fed50daf 100644
>> --- a/include/drm/drm_auth.h
>> +++ b/include/drm/drm_auth.h
>> @@ -103,4 +103,7 @@ bool drm_is_current_master(struct drm_file *fpriv);
>>
>> struct drm_master *drm_master_create(struct drm_device *dev);
>>
>> +int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
>> + struct drm_file *file_priv);
>
> Hm, so this looks reasonable as an internal function except for the void
> *data. I think for making ioctl calls available internally we should:
>
> - have the _ioctl variant for the ioctl table, which takes void *data.
> That one stays internal to drm.ko
> - Move the real implementation into a function without the _ioctl suffix,
> which takes the casted pointer (and has kerneldoc and all the neat
> things).
> - Also maybe let's drop the uapi version suffix internall too, so
> drm_addfb instead of drm_addfb2.
> - I wonder whether we should drop the drm_device *dev too while at it
> (from the internal callback), it's kinda redundant.
>
> All in the name of a neat&tidy internal api.
I agree to all points. Provide a sane exported API along the lines of:
int drm_master_drop(struct drm_file *file, struct
foobar_ioctl_params *params);
..and then make drm_dropmaster_ioctl() call this. It might even be
nicer to open-code the ioctl parameters to that function, rather than
re-using the ioctl-struct.
I think drm_file should be a superset of drm_device, so no need to
pass both (unless it does not have a back-pointer..).
Thanks
David
> Besides this polish, looks all reasonable.
> -Daniel
>
>> +
>> #endif
>> diff --git a/include/drm/drm_dumb_buffers.h b/include/drm/drm_dumb_buffers.h
>> new file mode 100644
>> index 000000000000..c1138c1c06ab
>> --- /dev/null
>> +++ b/include/drm/drm_dumb_buffers.h
>> @@ -0,0 +1,10 @@
>> +#ifndef _DRM_DUMB_BUFFERS_H_
>> +#define _DRM_DUMB_BUFFERS_H_
>> +
>> +struct drm_device;
>> +struct drm_file;
>> +
>> +int drm_mode_create_dumb_ioctl(struct drm_device *dev,
>> + void *data, struct drm_file *file_priv);
>> +
>> +#endif
>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>> index 0e0c868451a5..23d90744c3d9 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);
>> diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
>> index c50502c656e5..6c54db1c23ae 100644
>> --- a/include/drm/drm_framebuffer.h
>> +++ b/include/drm/drm_framebuffer.h
>> @@ -313,4 +313,7 @@ int drm_framebuffer_plane_width(int width,
>> int drm_framebuffer_plane_height(int height,
>> const struct drm_framebuffer *fb, int plane);
>>
>> +int drm_mode_addfb2(struct drm_device *dev,
>> + void *data, struct drm_file *file_priv);
>> +
>> #endif
>> diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h
>> index 9cd9e36f77b5..ca7bc50af2d7 100644
>> --- a/include/drm/drm_prime.h
>> +++ b/include/drm/drm_prime.h
>> @@ -85,5 +85,7 @@ int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages,
>> struct sg_table *drm_prime_pages_to_sg(struct page **pages, unsigned int nr_pages);
>> void drm_prime_gem_destroy(struct drm_gem_object *obj, struct sg_table *sg);
>>
>> +int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
>> + struct drm_file *file_priv);
>>
>> #endif /* __DRM_PRIME_H__ */
>> --
>> 2.14.2
>>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC v2 3/8] drm: Export some ioctl functions
2018-01-03 22:21 ` [RFC v2 3/8] drm: Export some ioctl functions Noralf Trønnes
2018-01-09 10:16 ` Daniel Vetter
@ 2018-01-09 14:48 ` Laurent Pinchart
1 sibling, 0 replies; 26+ messages in thread
From: Laurent Pinchart @ 2018-01-09 14:48 UTC (permalink / raw)
To: Noralf Trønnes; +Cc: daniel.vetter, intel-gfx, dri-devel
Hi Noralf,
Thank you for the patch.
On Thursday, 4 January 2018 00:21:05 EET Noralf Trønnes wrote:
> Export the following functions so in-kernel users can allocate
> dumb buffers:
> - drm_file_alloc
> - drm_file_free
> - drm_prime_handle_to_fd_ioctl
> - drm_mode_addfb2
> - drm_mode_create_dumb_ioctl
> - drm_dropmaster_ioctl
>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
I agree with Daniel's comments. Additionally please see below for one small
comment.
> ---
> drivers/gpu/drm/drm_auth.c | 1 +
> drivers/gpu/drm/drm_crtc_internal.h | 4 ----
> drivers/gpu/drm/drm_dumb_buffers.c | 1 +
> drivers/gpu/drm/drm_file.c | 2 ++
> drivers/gpu/drm/drm_framebuffer.c | 1 +
> drivers/gpu/drm/drm_internal.h | 6 ------
> drivers/gpu/drm/drm_ioctl.c | 1 +
> drivers/gpu/drm/drm_prime.c | 1 +
> include/drm/drm_auth.h | 3 +++
> include/drm/drm_dumb_buffers.h | 10 ++++++++++
> include/drm/drm_file.h | 2 ++
> include/drm/drm_framebuffer.h | 3 +++
> include/drm/drm_prime.h | 2 ++
> 13 files changed, 27 insertions(+), 10 deletions(-)
> create mode 100644 include/drm/drm_dumb_buffers.h
[snip]
> diff --git a/include/drm/drm_dumb_buffers.h b/include/drm/drm_dumb_buffers.h
> new file mode 100644
> index 000000000000..c1138c1c06ab
> --- /dev/null
> +++ b/include/drm/drm_dumb_buffers.h
> @@ -0,0 +1,10 @@
You should add an SPDX header to specify the file license.
> +#ifndef _DRM_DUMB_BUFFERS_H_
> +#define _DRM_DUMB_BUFFERS_H_
> +
> +struct drm_device;
> +struct drm_file;
> +
> +int drm_mode_create_dumb_ioctl(struct drm_device *dev,
> + void *data, struct drm_file *file_priv);
> +
> +#endif
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* [RFC v2 4/8] drm/fb-helper: Ensure driver module is pinned in fb_open()
2018-01-03 22:21 [RFC v2 0/8] drm: Add generic fbdev emulation Noralf Trønnes
` (2 preceding siblings ...)
2018-01-03 22:21 ` [RFC v2 3/8] drm: Export some ioctl functions Noralf Trønnes
@ 2018-01-03 22:21 ` Noralf Trønnes
2018-01-09 10:18 ` Daniel Vetter
2018-01-09 10:22 ` Daniel Vetter
2018-01-03 22:21 ` [RFC v2 5/8] drm/fb-helper: Don't restore if fbdev is not in use Noralf Trønnes
` (4 subsequent siblings)
8 siblings, 2 replies; 26+ messages in thread
From: Noralf Trønnes @ 2018-01-03 22:21 UTC (permalink / raw)
To: dri-devel
Cc: daniel.vetter, intel-gfx, Noralf Trønnes, laurent.pinchart,
dh.herrmann
If struct fb_ops is defined in a library like cma, fb_open() and fbcon
takes a ref on the library instead of the driver module. Use
fb_ops.fb_open/fb_release to ensure that the driver module is pinned.
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
drivers/gpu/drm/drm_fb_helper.c | 40 ++++++++++++++++++++++++++++++++++++++++
include/drm/drm_fb_helper.h | 13 +++++++++++++
2 files changed, 53 insertions(+)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 035784ddd133..2c6adf1d80c2 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1207,6 +1207,46 @@ void drm_fb_helper_cfb_imageblit(struct fb_info *info,
}
EXPORT_SYMBOL(drm_fb_helper_cfb_imageblit);
+/**
+ * drm_fb_helper_fb_open - implementation for &fb_ops.fb_open
+ * @info: fbdev registered by the helper
+ * @user: 1=userspace, 0=fbcon
+ *
+ * If &fb_ops is wrapped in a library, pin the driver module.
+ */
+int drm_fb_helper_fb_open(struct fb_info *info, int user)
+{
+ struct drm_fb_helper *fb_helper = info->par;
+ struct drm_device *dev = fb_helper->dev;
+
+ if (info->fbops->owner != dev->driver->fops->owner) {
+ if (!try_module_get(dev->driver->fops->owner))
+ return -ENODEV;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_fb_helper_fb_open);
+
+/**
+ * drm_fb_helper_fb_release - implementation for &fb_ops.fb_release
+ * @info: fbdev registered by the helper
+ * @user: 1=userspace, 0=fbcon
+ *
+ * If &fb_ops is wrapped in a library, unpin the driver module.
+ */
+int drm_fb_helper_fb_release(struct fb_info *info, int user)
+{
+ struct drm_fb_helper *fb_helper = info->par;
+ struct drm_device *dev = fb_helper->dev;
+
+ if (info->fbops->owner != dev->driver->fops->owner)
+ module_put(dev->driver->fops->owner);
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_fb_helper_fb_release);
+
/**
* drm_fb_helper_set_suspend - wrapper around fb_set_suspend
* @fb_helper: driver-allocated fbdev helper, can be NULL
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index b069433e7fc1..a593f01ff69e 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -297,6 +297,9 @@ void drm_fb_helper_cfb_copyarea(struct fb_info *info,
void drm_fb_helper_cfb_imageblit(struct fb_info *info,
const struct fb_image *image);
+int drm_fb_helper_fb_open(struct fb_info *info, int user);
+int drm_fb_helper_fb_release(struct fb_info *info, int user);
+
void drm_fb_helper_set_suspend(struct drm_fb_helper *fb_helper, bool suspend);
void drm_fb_helper_set_suspend_unlocked(struct drm_fb_helper *fb_helper,
bool suspend);
@@ -473,6 +476,16 @@ static inline void drm_fb_helper_cfb_imageblit(struct fb_info *info,
{
}
+static inline int drm_fb_helper_fb_open(struct fb_info *info, int user)
+{
+ return -ENODEV;
+}
+
+static inline int drm_fb_helper_fb_release(struct fb_info *info, int user)
+{
+ return -ENODEV;
+}
+
static inline void drm_fb_helper_set_suspend(struct drm_fb_helper *fb_helper,
bool suspend)
{
--
2.14.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [RFC v2 4/8] drm/fb-helper: Ensure driver module is pinned in fb_open()
2018-01-03 22:21 ` [RFC v2 4/8] drm/fb-helper: Ensure driver module is pinned in fb_open() Noralf Trønnes
@ 2018-01-09 10:18 ` Daniel Vetter
2018-01-09 10:22 ` Daniel Vetter
1 sibling, 0 replies; 26+ messages in thread
From: Daniel Vetter @ 2018-01-09 10:18 UTC (permalink / raw)
To: Noralf Trønnes
Cc: daniel.vetter, intel-gfx, dri-devel, laurent.pinchart,
dh.herrmann
On Wed, Jan 03, 2018 at 11:21:06PM +0100, Noralf Trønnes wrote:
> If struct fb_ops is defined in a library like cma, fb_open() and fbcon
> takes a ref on the library instead of the driver module. Use
> fb_ops.fb_open/fb_release to ensure that the driver module is pinned.
>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
... I guess the patch to roll it out to the various vfun tables and
macros comes later?
-Daniel
> ---
> drivers/gpu/drm/drm_fb_helper.c | 40 ++++++++++++++++++++++++++++++++++++++++
> include/drm/drm_fb_helper.h | 13 +++++++++++++
> 2 files changed, 53 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 035784ddd133..2c6adf1d80c2 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1207,6 +1207,46 @@ void drm_fb_helper_cfb_imageblit(struct fb_info *info,
> }
> EXPORT_SYMBOL(drm_fb_helper_cfb_imageblit);
>
> +/**
> + * drm_fb_helper_fb_open - implementation for &fb_ops.fb_open
> + * @info: fbdev registered by the helper
> + * @user: 1=userspace, 0=fbcon
> + *
> + * If &fb_ops is wrapped in a library, pin the driver module.
> + */
> +int drm_fb_helper_fb_open(struct fb_info *info, int user)
> +{
> + struct drm_fb_helper *fb_helper = info->par;
> + struct drm_device *dev = fb_helper->dev;
> +
> + if (info->fbops->owner != dev->driver->fops->owner) {
> + if (!try_module_get(dev->driver->fops->owner))
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_fb_helper_fb_open);
> +
> +/**
> + * drm_fb_helper_fb_release - implementation for &fb_ops.fb_release
> + * @info: fbdev registered by the helper
> + * @user: 1=userspace, 0=fbcon
> + *
> + * If &fb_ops is wrapped in a library, unpin the driver module.
> + */
> +int drm_fb_helper_fb_release(struct fb_info *info, int user)
> +{
> + struct drm_fb_helper *fb_helper = info->par;
> + struct drm_device *dev = fb_helper->dev;
> +
> + if (info->fbops->owner != dev->driver->fops->owner)
> + module_put(dev->driver->fops->owner);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_fb_helper_fb_release);
> +
> /**
> * drm_fb_helper_set_suspend - wrapper around fb_set_suspend
> * @fb_helper: driver-allocated fbdev helper, can be NULL
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index b069433e7fc1..a593f01ff69e 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -297,6 +297,9 @@ void drm_fb_helper_cfb_copyarea(struct fb_info *info,
> void drm_fb_helper_cfb_imageblit(struct fb_info *info,
> const struct fb_image *image);
>
> +int drm_fb_helper_fb_open(struct fb_info *info, int user);
> +int drm_fb_helper_fb_release(struct fb_info *info, int user);
> +
> void drm_fb_helper_set_suspend(struct drm_fb_helper *fb_helper, bool suspend);
> void drm_fb_helper_set_suspend_unlocked(struct drm_fb_helper *fb_helper,
> bool suspend);
> @@ -473,6 +476,16 @@ static inline void drm_fb_helper_cfb_imageblit(struct fb_info *info,
> {
> }
>
> +static inline int drm_fb_helper_fb_open(struct fb_info *info, int user)
> +{
> + return -ENODEV;
> +}
> +
> +static inline int drm_fb_helper_fb_release(struct fb_info *info, int user)
> +{
> + return -ENODEV;
> +}
> +
> static inline void drm_fb_helper_set_suspend(struct drm_fb_helper *fb_helper,
> bool suspend)
> {
> --
> 2.14.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [RFC v2 4/8] drm/fb-helper: Ensure driver module is pinned in fb_open()
2018-01-03 22:21 ` [RFC v2 4/8] drm/fb-helper: Ensure driver module is pinned in fb_open() Noralf Trønnes
2018-01-09 10:18 ` Daniel Vetter
@ 2018-01-09 10:22 ` Daniel Vetter
1 sibling, 0 replies; 26+ messages in thread
From: Daniel Vetter @ 2018-01-09 10:22 UTC (permalink / raw)
To: Noralf Trønnes
Cc: daniel.vetter, intel-gfx, dri-devel, laurent.pinchart,
dh.herrmann
On Wed, Jan 03, 2018 at 11:21:06PM +0100, Noralf Trønnes wrote:
> If struct fb_ops is defined in a library like cma, fb_open() and fbcon
> takes a ref on the library instead of the driver module. Use
> fb_ops.fb_open/fb_release to ensure that the driver module is pinned.
>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
> drivers/gpu/drm/drm_fb_helper.c | 40 ++++++++++++++++++++++++++++++++++++++++
> include/drm/drm_fb_helper.h | 13 +++++++++++++
> 2 files changed, 53 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 035784ddd133..2c6adf1d80c2 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1207,6 +1207,46 @@ void drm_fb_helper_cfb_imageblit(struct fb_info *info,
> }
> EXPORT_SYMBOL(drm_fb_helper_cfb_imageblit);
>
> +/**
> + * drm_fb_helper_fb_open - implementation for &fb_ops.fb_open
> + * @info: fbdev registered by the helper
> + * @user: 1=userspace, 0=fbcon
> + *
> + * If &fb_ops is wrapped in a library, pin the driver module.
> + */
> +int drm_fb_helper_fb_open(struct fb_info *info, int user)
> +{
> + struct drm_fb_helper *fb_helper = info->par;
> + struct drm_device *dev = fb_helper->dev;
> +
> + if (info->fbops->owner != dev->driver->fops->owner) {
Actually bikeshed potential here: I'd just unconditionally grab the module
reference, makes the code simpler. Or is there a reason not to?
-Daniel
> + if (!try_module_get(dev->driver->fops->owner))
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_fb_helper_fb_open);
> +
> +/**
> + * drm_fb_helper_fb_release - implementation for &fb_ops.fb_release
> + * @info: fbdev registered by the helper
> + * @user: 1=userspace, 0=fbcon
> + *
> + * If &fb_ops is wrapped in a library, unpin the driver module.
> + */
> +int drm_fb_helper_fb_release(struct fb_info *info, int user)
> +{
> + struct drm_fb_helper *fb_helper = info->par;
> + struct drm_device *dev = fb_helper->dev;
> +
> + if (info->fbops->owner != dev->driver->fops->owner)
> + module_put(dev->driver->fops->owner);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_fb_helper_fb_release);
> +
> /**
> * drm_fb_helper_set_suspend - wrapper around fb_set_suspend
> * @fb_helper: driver-allocated fbdev helper, can be NULL
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index b069433e7fc1..a593f01ff69e 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -297,6 +297,9 @@ void drm_fb_helper_cfb_copyarea(struct fb_info *info,
> void drm_fb_helper_cfb_imageblit(struct fb_info *info,
> const struct fb_image *image);
>
> +int drm_fb_helper_fb_open(struct fb_info *info, int user);
> +int drm_fb_helper_fb_release(struct fb_info *info, int user);
> +
> void drm_fb_helper_set_suspend(struct drm_fb_helper *fb_helper, bool suspend);
> void drm_fb_helper_set_suspend_unlocked(struct drm_fb_helper *fb_helper,
> bool suspend);
> @@ -473,6 +476,16 @@ static inline void drm_fb_helper_cfb_imageblit(struct fb_info *info,
> {
> }
>
> +static inline int drm_fb_helper_fb_open(struct fb_info *info, int user)
> +{
> + return -ENODEV;
> +}
> +
> +static inline int drm_fb_helper_fb_release(struct fb_info *info, int user)
> +{
> + return -ENODEV;
> +}
> +
> static inline void drm_fb_helper_set_suspend(struct drm_fb_helper *fb_helper,
> bool suspend)
> {
> --
> 2.14.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* [RFC v2 5/8] drm/fb-helper: Don't restore if fbdev is not in use
2018-01-03 22:21 [RFC v2 0/8] drm: Add generic fbdev emulation Noralf Trønnes
` (3 preceding siblings ...)
2018-01-03 22:21 ` [RFC v2 4/8] drm/fb-helper: Ensure driver module is pinned in fb_open() Noralf Trønnes
@ 2018-01-03 22:21 ` Noralf Trønnes
2018-01-09 10:28 ` Daniel Vetter
2018-01-03 22:21 ` [RFC v2 6/8] drm: Handle fbdev emulation in core Noralf Trønnes
` (3 subsequent siblings)
8 siblings, 1 reply; 26+ messages in thread
From: Noralf Trønnes @ 2018-01-03 22:21 UTC (permalink / raw)
To: dri-devel; +Cc: daniel.vetter, intel-gfx, laurent.pinchart
Keep track of fbdev users and only restore fbdev in
drm_fb_helper_restore_fbdev_mode_unlocked() when in use. This avoids
fbdev being restored in drm_driver.last_close when nothing uses it.
Additionally fbdev is turned off when the last user is closing.
fbcon is a user in this context.
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
drivers/gpu/drm/drm_fb_helper.c | 15 +++++++++++++++
include/drm/drm_fb_helper.h | 14 ++++++++++++++
2 files changed, 29 insertions(+)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 2c6adf1d80c2..f9dcc7a5761f 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -522,6 +522,9 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
if (READ_ONCE(fb_helper->deferred_setup))
return 0;
+ if (!atomic_read(&fb_helper->open_count))
+ return 0;
+
mutex_lock(&fb_helper->lock);
ret = restore_fbdev_mode(fb_helper);
@@ -781,6 +784,7 @@ void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper,
INIT_WORK(&helper->resume_work, drm_fb_helper_resume_worker);
INIT_WORK(&helper->dirty_work, drm_fb_helper_dirty_work);
helper->dirty_clip.x1 = helper->dirty_clip.y1 = ~0;
+ atomic_set(&helper->open_count, 1);
mutex_init(&helper->lock);
helper->funcs = funcs;
helper->dev = dev;
@@ -1212,6 +1216,7 @@ EXPORT_SYMBOL(drm_fb_helper_cfb_imageblit);
* @info: fbdev registered by the helper
* @user: 1=userspace, 0=fbcon
*
+ * Increase fbdev use count.
* If &fb_ops is wrapped in a library, pin the driver module.
*/
int drm_fb_helper_fb_open(struct fb_info *info, int user)
@@ -1224,6 +1229,8 @@ int drm_fb_helper_fb_open(struct fb_info *info, int user)
return -ENODEV;
}
+ atomic_inc(&fb_helper->open_count);
+
return 0;
}
EXPORT_SYMBOL(drm_fb_helper_fb_open);
@@ -1233,6 +1240,7 @@ EXPORT_SYMBOL(drm_fb_helper_fb_open);
* @info: fbdev registered by the helper
* @user: 1=userspace, 0=fbcon
*
+ * Decrease fbdev use count and turn off if there are no users left.
* If &fb_ops is wrapped in a library, unpin the driver module.
*/
int drm_fb_helper_fb_release(struct fb_info *info, int user)
@@ -1240,6 +1248,10 @@ int drm_fb_helper_fb_release(struct fb_info *info, int user)
struct drm_fb_helper *fb_helper = info->par;
struct drm_device *dev = fb_helper->dev;
+ if (atomic_dec_and_test(&fb_helper->open_count) &&
+ !drm_dev_is_unplugged(fb_helper->dev))
+ drm_fb_helper_blank(FB_BLANK_POWERDOWN, info);
+
if (info->fbops->owner != dev->driver->fops->owner)
module_put(dev->driver->fops->owner);
@@ -1936,6 +1948,9 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
if (ret < 0)
return ret;
+ if (fb_helper->fbdev->fbops->fb_open == drm_fb_helper_fb_open)
+ atomic_set(&fb_helper->open_count, 0);
+
strcpy(fb_helper->fb->comm, "[fbcon]");
return 0;
}
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index a593f01ff69e..16d8773b60e3 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -232,6 +232,20 @@ struct drm_fb_helper {
* See also: @deferred_setup
*/
int preferred_bpp;
+
+ /**
+ * @open_count:
+ *
+ * Keeps track of fbdev use to know when to not restore fbdev.
+ *
+ * Drivers that use drm_fb_helper_fb_open() as their \.fb_open
+ * callback will get an initial value of 0 and get restore based on
+ * actual use. Others will get an initial value of 1 which means that
+ * fbdev will always be restored. Drivers that call
+ * drm_fb_helper_fb_open() in their \.fb_open, thus needs to set the
+ * initial value to 0 themselves.
+ */
+ atomic_t open_count;
};
/**
--
2.14.2
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [RFC v2 5/8] drm/fb-helper: Don't restore if fbdev is not in use
2018-01-03 22:21 ` [RFC v2 5/8] drm/fb-helper: Don't restore if fbdev is not in use Noralf Trønnes
@ 2018-01-09 10:28 ` Daniel Vetter
0 siblings, 0 replies; 26+ messages in thread
From: Daniel Vetter @ 2018-01-09 10:28 UTC (permalink / raw)
To: Noralf Trønnes
Cc: daniel.vetter, intel-gfx, dri-devel, laurent.pinchart,
dh.herrmann
On Wed, Jan 03, 2018 at 11:21:07PM +0100, Noralf Trønnes wrote:
> Keep track of fbdev users and only restore fbdev in
> drm_fb_helper_restore_fbdev_mode_unlocked() when in use. This avoids
> fbdev being restored in drm_driver.last_close when nothing uses it.
> Additionally fbdev is turned off when the last user is closing.
> fbcon is a user in this context.
>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
> drivers/gpu/drm/drm_fb_helper.c | 15 +++++++++++++++
> include/drm/drm_fb_helper.h | 14 ++++++++++++++
> 2 files changed, 29 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 2c6adf1d80c2..f9dcc7a5761f 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -522,6 +522,9 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
> if (READ_ONCE(fb_helper->deferred_setup))
> return 0;
>
> + if (!atomic_read(&fb_helper->open_count))
> + return 0;
> +
> mutex_lock(&fb_helper->lock);
> ret = restore_fbdev_mode(fb_helper);
>
> @@ -781,6 +784,7 @@ void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper,
> INIT_WORK(&helper->resume_work, drm_fb_helper_resume_worker);
> INIT_WORK(&helper->dirty_work, drm_fb_helper_dirty_work);
> helper->dirty_clip.x1 = helper->dirty_clip.y1 = ~0;
> + atomic_set(&helper->open_count, 1);
> mutex_init(&helper->lock);
> helper->funcs = funcs;
> helper->dev = dev;
> @@ -1212,6 +1216,7 @@ EXPORT_SYMBOL(drm_fb_helper_cfb_imageblit);
> * @info: fbdev registered by the helper
> * @user: 1=userspace, 0=fbcon
> *
> + * Increase fbdev use count.
> * If &fb_ops is wrapped in a library, pin the driver module.
> */
> int drm_fb_helper_fb_open(struct fb_info *info, int user)
> @@ -1224,6 +1229,8 @@ int drm_fb_helper_fb_open(struct fb_info *info, int user)
> return -ENODEV;
> }
>
> + atomic_inc(&fb_helper->open_count);
> +
> return 0;
> }
> EXPORT_SYMBOL(drm_fb_helper_fb_open);
> @@ -1233,6 +1240,7 @@ EXPORT_SYMBOL(drm_fb_helper_fb_open);
> * @info: fbdev registered by the helper
> * @user: 1=userspace, 0=fbcon
> *
> + * Decrease fbdev use count and turn off if there are no users left.
> * If &fb_ops is wrapped in a library, unpin the driver module.
> */
> int drm_fb_helper_fb_release(struct fb_info *info, int user)
> @@ -1240,6 +1248,10 @@ int drm_fb_helper_fb_release(struct fb_info *info, int user)
> struct drm_fb_helper *fb_helper = info->par;
> struct drm_device *dev = fb_helper->dev;
>
> + if (atomic_dec_and_test(&fb_helper->open_count) &&
> + !drm_dev_is_unplugged(fb_helper->dev))
> + drm_fb_helper_blank(FB_BLANK_POWERDOWN, info);
> +
> if (info->fbops->owner != dev->driver->fops->owner)
> module_put(dev->driver->fops->owner);
>
> @@ -1936,6 +1948,9 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
> if (ret < 0)
> return ret;
>
> + if (fb_helper->fbdev->fbops->fb_open == drm_fb_helper_fb_open)
> + atomic_set(&fb_helper->open_count, 0);
> +
> strcpy(fb_helper->fb->comm, "[fbcon]");
> return 0;
> }
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index a593f01ff69e..16d8773b60e3 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -232,6 +232,20 @@ struct drm_fb_helper {
> * See also: @deferred_setup
> */
> int preferred_bpp;
> +
> + /**
> + * @open_count:
> + *
> + * Keeps track of fbdev use to know when to not restore fbdev.
> + *
> + * Drivers that use drm_fb_helper_fb_open() as their \.fb_open
> + * callback will get an initial value of 0 and get restore based on
> + * actual use. Others will get an initial value of 1 which means that
> + * fbdev will always be restored. Drivers that call
> + * drm_fb_helper_fb_open() in their \.fb_open, thus needs to set the
> + * initial value to 0 themselves.
> + */
> + atomic_t open_count;
Do we really need an atomic_t here? Especially the "disable stuff on last
fb_close" would still be race without more locking, and from a quick look
at the fbdev core code we're protected with fb_info->lock. I think an
unsigned is perfectly fine here. And if we really need atomics, then
refcount_t (for the safety checks that provides).
With that changed:
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
But before we merge this we definitely need to roll out fb_open/close to
all drivers. But I guess that's why the RFC in your series.
On that: There's some intersting runtime pm stuff for
nouveau/amd/radeon. I guess we'll need to keep that. Plus udl has some
defio fun :-/
-Daniel
> };
>
> /**
> --
> 2.14.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* [RFC v2 6/8] drm: Handle fbdev emulation in core
2018-01-03 22:21 [RFC v2 0/8] drm: Add generic fbdev emulation Noralf Trønnes
` (4 preceding siblings ...)
2018-01-03 22:21 ` [RFC v2 5/8] drm/fb-helper: Don't restore if fbdev is not in use Noralf Trønnes
@ 2018-01-03 22:21 ` Noralf Trønnes
2018-01-09 10:38 ` Daniel Vetter
2018-01-03 22:21 ` [RFC v2 7/8] drm/fb-helper: Add generic fbdev emulation Noralf Trønnes
` (2 subsequent siblings)
8 siblings, 1 reply; 26+ messages in thread
From: Noralf Trønnes @ 2018-01-03 22:21 UTC (permalink / raw)
To: dri-devel; +Cc: daniel.vetter, intel-gfx, laurent.pinchart
Prepare for generic fbdev emulation by letting DRM core work directly
with the fbdev compatibility layer. This is done by adding new fbdev
helper vtable callbacks for restore, hotplug_event, unregister and
release.
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
drivers/gpu/drm/drm_file.c | 12 +++++++++++-
drivers/gpu/drm/drm_mode_config.c | 10 ++++++++++
drivers/gpu/drm/drm_probe_helper.c | 4 ++++
include/drm/drm_fb_helper.h | 33 +++++++++++++++++++++++++++++++++
4 files changed, 58 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 400d44437e93..7ec09fb83135 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -35,6 +35,7 @@
#include <linux/slab.h>
#include <linux/module.h>
+#include <drm/drm_fb_helper.h>
#include <drm/drm_file.h>
#include <drm/drmP.h>
@@ -441,10 +442,19 @@ static void drm_legacy_dev_reinit(struct drm_device *dev)
void drm_lastclose(struct drm_device * dev)
{
+ struct drm_fb_helper *fb_helper = dev->fb_helper;
+ int ret;
+
DRM_DEBUG("\n");
- if (dev->driver->lastclose)
+ if (dev->driver->lastclose) {
dev->driver->lastclose(dev);
+ } else if (fb_helper && fb_helper->funcs && fb_helper->funcs->restore) {
+ ret = fb_helper->funcs->restore(fb_helper);
+ if (ret)
+ DRM_ERROR("Failed to restore fbdev: %d\n", ret);
+ }
+
DRM_DEBUG("driver lastclose completed\n");
if (drm_core_check_feature(dev, DRIVER_LEGACY))
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index bc5c46306b3d..260eb1730244 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -21,6 +21,7 @@
*/
#include <drm/drm_encoder.h>
+#include <drm/drm_fb_helper.h>
#include <drm/drm_mode_config.h>
#include <drm/drmP.h>
@@ -61,6 +62,11 @@ int drm_modeset_register_all(struct drm_device *dev)
void drm_modeset_unregister_all(struct drm_device *dev)
{
+ struct drm_fb_helper *fb_helper = dev->fb_helper;
+
+ if (fb_helper && fb_helper->funcs && fb_helper->funcs->unregister)
+ fb_helper->funcs->unregister(fb_helper);
+
drm_connector_unregister_all(dev);
drm_encoder_unregister_all(dev);
drm_crtc_unregister_all(dev);
@@ -408,6 +414,7 @@ EXPORT_SYMBOL(drm_mode_config_init);
*/
void drm_mode_config_cleanup(struct drm_device *dev)
{
+ struct drm_fb_helper *fb_helper = dev->fb_helper;
struct drm_connector *connector;
struct drm_connector_list_iter conn_iter;
struct drm_crtc *crtc, *ct;
@@ -417,6 +424,9 @@ void drm_mode_config_cleanup(struct drm_device *dev)
struct drm_property_blob *blob, *bt;
struct drm_plane *plane, *plt;
+ if (fb_helper && fb_helper->funcs && fb_helper->funcs->release)
+ fb_helper->funcs->release(fb_helper);
+
list_for_each_entry_safe(encoder, enct, &dev->mode_config.encoder_list,
head) {
encoder->funcs->destroy(encoder);
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 555fbe54d6e2..9d8b0ba54173 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -559,10 +559,14 @@ EXPORT_SYMBOL(drm_helper_probe_single_connector_modes);
*/
void drm_kms_helper_hotplug_event(struct drm_device *dev)
{
+ struct drm_fb_helper *fb_helper = dev->fb_helper;
+
/* 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 (fb_helper && fb_helper->funcs && fb_helper->funcs->hotplug_event)
+ fb_helper->funcs->hotplug_event(fb_helper);
}
EXPORT_SYMBOL(drm_kms_helper_hotplug_event);
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 16d8773b60e3..385f967c3552 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -125,6 +125,39 @@ struct drm_fb_helper_funcs {
struct drm_display_mode **modes,
struct drm_fb_offset *offsets,
bool *enabled, int width, int height);
+
+ /**
+ * @restore:
+ *
+ * Optional callback for restoring fbdev emulation.
+ * Called by drm_lastclose() if &drm_driver->lastclose is not set.
+ */
+ int (*restore)(struct drm_fb_helper *fb_helper);
+
+ /**
+ * @hotplug_event:
+ *
+ * Optional callback for hotplug events.
+ * Called by drm_kms_helper_hotplug_event() if
+ * &drm_mode_config_funcs->output_poll_changed is not set.
+ */
+ int (*hotplug_event)(struct drm_fb_helper *fb_helper);
+
+ /**
+ * @unregister:
+ *
+ * Optional callback for unregistrering fbdev emulation.
+ * Called by drm_dev_unregister().
+ */
+ void (*unregister)(struct drm_fb_helper *fb_helper);
+
+ /**
+ * @release:
+ *
+ * Optional callback for releasing fbdev emulation resources.
+ * Called by drm_mode_config_cleanup().
+ */
+ void (*release)(struct drm_fb_helper *fb_helper);
};
struct drm_fb_helper_connector {
--
2.14.2
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [RFC v2 6/8] drm: Handle fbdev emulation in core
2018-01-03 22:21 ` [RFC v2 6/8] drm: Handle fbdev emulation in core Noralf Trønnes
@ 2018-01-09 10:38 ` Daniel Vetter
2018-01-10 17:02 ` Noralf Trønnes
0 siblings, 1 reply; 26+ messages in thread
From: Daniel Vetter @ 2018-01-09 10:38 UTC (permalink / raw)
To: Noralf Trønnes; +Cc: daniel.vetter, intel-gfx, dri-devel, laurent.pinchart
On Wed, Jan 03, 2018 at 11:21:08PM +0100, Noralf Trønnes wrote:
> Prepare for generic fbdev emulation by letting DRM core work directly
> with the fbdev compatibility layer. This is done by adding new fbdev
> helper vtable callbacks for restore, hotplug_event, unregister and
> release.
>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
No clue whether my idea is any good, but inspired by the bootsplash
discussion, and the prospect that we might get other in-kernel drm/kms
clients I'm wondering whether we should make this a bit more generic and
tie it to drm_file?
The idea would be to have a list of internal drm clients by putting all
the internal drm_files onto a list (same way we do with the userspace
ones). Then we'd just walk that list and call ->hotplug, ->unregister and
->release at the right places.
->restore would be a bit different, I guess for that we'd only call the
->restore callback of the very first kernel-internal client.
With that we could then add/remove kernel-internal drm clients like normal
userspace clients, which should make integration of emergency consoles,
boot splashes and whatever else real easy. And I think it would be a lot
cleaner than leaking fb_helper knowledge into the drm core, which feels a
rather backwards.
Otoh that feels a bit overengineered (but at least it shouldn't be a lot
more code really). If the list is too much work we could start with 1
drm_file * pointer for internal stuff (but would still need locking, so
list_head is probably easier).
Thoughts?
-Daniel
> ---
> drivers/gpu/drm/drm_file.c | 12 +++++++++++-
> drivers/gpu/drm/drm_mode_config.c | 10 ++++++++++
> drivers/gpu/drm/drm_probe_helper.c | 4 ++++
> include/drm/drm_fb_helper.h | 33 +++++++++++++++++++++++++++++++++
> 4 files changed, 58 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index 400d44437e93..7ec09fb83135 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -35,6 +35,7 @@
> #include <linux/slab.h>
> #include <linux/module.h>
>
> +#include <drm/drm_fb_helper.h>
> #include <drm/drm_file.h>
> #include <drm/drmP.h>
>
> @@ -441,10 +442,19 @@ static void drm_legacy_dev_reinit(struct drm_device *dev)
>
> void drm_lastclose(struct drm_device * dev)
> {
> + struct drm_fb_helper *fb_helper = dev->fb_helper;
> + int ret;
> +
> DRM_DEBUG("\n");
>
> - if (dev->driver->lastclose)
> + if (dev->driver->lastclose) {
> dev->driver->lastclose(dev);
> + } else if (fb_helper && fb_helper->funcs && fb_helper->funcs->restore) {
> + ret = fb_helper->funcs->restore(fb_helper);
> + if (ret)
> + DRM_ERROR("Failed to restore fbdev: %d\n", ret);
> + }
> +
> DRM_DEBUG("driver lastclose completed\n");
>
> if (drm_core_check_feature(dev, DRIVER_LEGACY))
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index bc5c46306b3d..260eb1730244 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -21,6 +21,7 @@
> */
>
> #include <drm/drm_encoder.h>
> +#include <drm/drm_fb_helper.h>
> #include <drm/drm_mode_config.h>
> #include <drm/drmP.h>
>
> @@ -61,6 +62,11 @@ int drm_modeset_register_all(struct drm_device *dev)
>
> void drm_modeset_unregister_all(struct drm_device *dev)
> {
> + struct drm_fb_helper *fb_helper = dev->fb_helper;
> +
> + if (fb_helper && fb_helper->funcs && fb_helper->funcs->unregister)
> + fb_helper->funcs->unregister(fb_helper);
> +
> drm_connector_unregister_all(dev);
> drm_encoder_unregister_all(dev);
> drm_crtc_unregister_all(dev);
> @@ -408,6 +414,7 @@ EXPORT_SYMBOL(drm_mode_config_init);
> */
> void drm_mode_config_cleanup(struct drm_device *dev)
> {
> + struct drm_fb_helper *fb_helper = dev->fb_helper;
> struct drm_connector *connector;
> struct drm_connector_list_iter conn_iter;
> struct drm_crtc *crtc, *ct;
> @@ -417,6 +424,9 @@ void drm_mode_config_cleanup(struct drm_device *dev)
> struct drm_property_blob *blob, *bt;
> struct drm_plane *plane, *plt;
>
> + if (fb_helper && fb_helper->funcs && fb_helper->funcs->release)
> + fb_helper->funcs->release(fb_helper);
> +
> list_for_each_entry_safe(encoder, enct, &dev->mode_config.encoder_list,
> head) {
> encoder->funcs->destroy(encoder);
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 555fbe54d6e2..9d8b0ba54173 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -559,10 +559,14 @@ EXPORT_SYMBOL(drm_helper_probe_single_connector_modes);
> */
> void drm_kms_helper_hotplug_event(struct drm_device *dev)
> {
> + struct drm_fb_helper *fb_helper = dev->fb_helper;
> +
> /* 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 (fb_helper && fb_helper->funcs && fb_helper->funcs->hotplug_event)
> + fb_helper->funcs->hotplug_event(fb_helper);
> }
> EXPORT_SYMBOL(drm_kms_helper_hotplug_event);
>
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 16d8773b60e3..385f967c3552 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -125,6 +125,39 @@ struct drm_fb_helper_funcs {
> struct drm_display_mode **modes,
> struct drm_fb_offset *offsets,
> bool *enabled, int width, int height);
> +
> + /**
> + * @restore:
> + *
> + * Optional callback for restoring fbdev emulation.
> + * Called by drm_lastclose() if &drm_driver->lastclose is not set.
> + */
> + int (*restore)(struct drm_fb_helper *fb_helper);
> +
> + /**
> + * @hotplug_event:
> + *
> + * Optional callback for hotplug events.
> + * Called by drm_kms_helper_hotplug_event() if
> + * &drm_mode_config_funcs->output_poll_changed is not set.
> + */
> + int (*hotplug_event)(struct drm_fb_helper *fb_helper);
> +
> + /**
> + * @unregister:
> + *
> + * Optional callback for unregistrering fbdev emulation.
> + * Called by drm_dev_unregister().
> + */
> + void (*unregister)(struct drm_fb_helper *fb_helper);
> +
> + /**
> + * @release:
> + *
> + * Optional callback for releasing fbdev emulation resources.
> + * Called by drm_mode_config_cleanup().
> + */
> + void (*release)(struct drm_fb_helper *fb_helper);
> };
>
> struct drm_fb_helper_connector {
> --
> 2.14.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [RFC v2 6/8] drm: Handle fbdev emulation in core
2018-01-09 10:38 ` Daniel Vetter
@ 2018-01-10 17:02 ` Noralf Trønnes
2018-01-11 7:45 ` Daniel Vetter
0 siblings, 1 reply; 26+ messages in thread
From: Noralf Trønnes @ 2018-01-10 17:02 UTC (permalink / raw)
To: Daniel Vetter; +Cc: daniel.vetter, intel-gfx, dri-devel, laurent.pinchart
Den 09.01.2018 11.38, skrev Daniel Vetter:
> On Wed, Jan 03, 2018 at 11:21:08PM +0100, Noralf Trønnes wrote:
>> Prepare for generic fbdev emulation by letting DRM core work directly
>> with the fbdev compatibility layer. This is done by adding new fbdev
>> helper vtable callbacks for restore, hotplug_event, unregister and
>> release.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> No clue whether my idea is any good, but inspired by the bootsplash
> discussion, and the prospect that we might get other in-kernel drm/kms
> clients I'm wondering whether we should make this a bit more generic and
> tie it to drm_file?
>
> The idea would be to have a list of internal drm clients by putting all
> the internal drm_files onto a list (same way we do with the userspace
> ones). Then we'd just walk that list and call ->hotplug, ->unregister and
> ->release at the right places.
>
> ->restore would be a bit different, I guess for that we'd only call the
> ->restore callback of the very first kernel-internal client.
>
> With that we could then add/remove kernel-internal drm clients like normal
> userspace clients, which should make integration of emergency consoles,
> boot splashes and whatever else real easy. And I think it would be a lot
> cleaner than leaking fb_helper knowledge into the drm core, which feels a
> rather backwards.
>
> Otoh that feels a bit overengineered (but at least it shouldn't be a lot
> more code really). If the list is too much work we could start with 1
> drm_file * pointer for internal stuff (but would still need locking, so
> list_head is probably easier).
>
> Thoughts?
I prefer to have a proper in-kernel client API from the get go, instead
of fixing it up later. The reason I skipped spending time on it in this
RFC, was that I didn't know if I was on the right track at the right time
to get the necessary attention to make this dumb_buffer idea happen.
With a drm_file centric approach, are you thinking something like this:
struct drm_device {
+ struct list_head filelist_internal;
};
+struct drm_file_funcs {
+ int (*restore)(struct drm_file *file);
+ void (*hotplug)(struct drm_file *file);
+ void (*unregister)(struct drm_file *file);
+};
struct drm_file {
+ struct drm_device *dev;
+ const struct drm_file_funcs *funcs;
};
struct drm_file *drm_file_alloc(struct drm_minor *minor)
{
+ file->dev = dev;
}
struct drm_file* drm_internal_open(struct drm_device *dev,
const struct drm_file_funcs *funcs)
struct drm_file *file;
int ret;
if (!try_module_get(dev->driver->fops->owner))
return ERR_PTR(-ENODEV);
drm_dev_ref(dev);
file = drm_file_alloc(dev);
if (IS_ERR(file)) {
drm_dev_unref(dev);
module_put(dev->driver->fops->owner);
return file;
}
file->funcs = funcs;
mutex_lock(&dev->filelist_mutex);
list_add(&file->lhead, &dev->filelist_internal);
mutex_unlock(&dev->filelist_mutex);
return file;
}
void drm_internal_release(struct drm_file *file)
{
struct drm_device *dev = file->dev;
mutex_lock(&dev->filelist_mutex);
list_del(&file->lhead);
mutex_unlock(&dev->filelist_mutex);
drm_file_free(file);
drm_dev_unref(dev);
module_put(dev->driver->fops->owner);
}
void drm_lastclose(struct drm_device *dev)
{
+ mutex_lock(&dev->filelist_mutex);
+ list_for_each_entry(file, &dev->filelist_internal, lhead) {
+ if (file->funcs && file->funcs->restore &&
+ !file->funcs->restore(file))
+ break;
+ mutex_unlock(&dev->filelist_mutex);
}
void drm_kms_helper_hotplug_event(struct drm_device *dev)
{
+ mutex_lock(&dev->filelist_mutex);
+ list_for_each_entry(file, &dev->filelist_internal, lhead) {
+ if (file->funcs && file->funcs->hotplug)
+ file->funcs->hotplug(file);
+ mutex_unlock(&dev->filelist_mutex);
}
How is locking done when .unregister will call into drm_internal_release()?
void drm_dev_unregister(struct drm_device *dev)
{
+ list_for_each_entry(file, &dev->filelist_internal, lhead) {
+ if (file->funcs && file->funcs->unregister)
+ file->funcs->unregister(file);
}
There is also the case where .unregister can't release the drm_file
because userspace has mmap'ed the buffer (fbdev). The client holds a ref
on drm_device so cleanup is stalled but that requires a driver that can
handle that (ref the tinydrm unplug series which is awaiting a timeslot).
amdgpu iterates dev->filelist in amdgpu_gem_force_release() and
amdgpu_sched_process_priority_override(). I don't know if it needs to
handle the internal ones as well.
What's the rational for having separate lists for internal and userspace?
There is one thing missing and that is notification of new drm_devices.
The client iterates the available drm_devices on module init, but it
needs a way to know about any later drm_device registrations.
Noralf.
> -Daniel
>
>> ---
>> drivers/gpu/drm/drm_file.c | 12 +++++++++++-
>> drivers/gpu/drm/drm_mode_config.c | 10 ++++++++++
>> drivers/gpu/drm/drm_probe_helper.c | 4 ++++
>> include/drm/drm_fb_helper.h | 33 +++++++++++++++++++++++++++++++++
>> 4 files changed, 58 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>> index 400d44437e93..7ec09fb83135 100644
>> --- a/drivers/gpu/drm/drm_file.c
>> +++ b/drivers/gpu/drm/drm_file.c
>> @@ -35,6 +35,7 @@
>> #include <linux/slab.h>
>> #include <linux/module.h>
>>
>> +#include <drm/drm_fb_helper.h>
>> #include <drm/drm_file.h>
>> #include <drm/drmP.h>
>>
>> @@ -441,10 +442,19 @@ static void drm_legacy_dev_reinit(struct drm_device *dev)
>>
>> void drm_lastclose(struct drm_device * dev)
>> {
>> + struct drm_fb_helper *fb_helper = dev->fb_helper;
>> + int ret;
>> +
>> DRM_DEBUG("\n");
>>
>> - if (dev->driver->lastclose)
>> + if (dev->driver->lastclose) {
>> dev->driver->lastclose(dev);
>> + } else if (fb_helper && fb_helper->funcs && fb_helper->funcs->restore) {
>> + ret = fb_helper->funcs->restore(fb_helper);
>> + if (ret)
>> + DRM_ERROR("Failed to restore fbdev: %d\n", ret);
>> + }
>> +
>> DRM_DEBUG("driver lastclose completed\n");
>>
>> if (drm_core_check_feature(dev, DRIVER_LEGACY))
>> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
>> index bc5c46306b3d..260eb1730244 100644
>> --- a/drivers/gpu/drm/drm_mode_config.c
>> +++ b/drivers/gpu/drm/drm_mode_config.c
>> @@ -21,6 +21,7 @@
>> */
>>
>> #include <drm/drm_encoder.h>
>> +#include <drm/drm_fb_helper.h>
>> #include <drm/drm_mode_config.h>
>> #include <drm/drmP.h>
>>
>> @@ -61,6 +62,11 @@ int drm_modeset_register_all(struct drm_device *dev)
>>
>> void drm_modeset_unregister_all(struct drm_device *dev)
>> {
>> + struct drm_fb_helper *fb_helper = dev->fb_helper;
>> +
>> + if (fb_helper && fb_helper->funcs && fb_helper->funcs->unregister)
>> + fb_helper->funcs->unregister(fb_helper);
>> +
>> drm_connector_unregister_all(dev);
>> drm_encoder_unregister_all(dev);
>> drm_crtc_unregister_all(dev);
>> @@ -408,6 +414,7 @@ EXPORT_SYMBOL(drm_mode_config_init);
>> */
>> void drm_mode_config_cleanup(struct drm_device *dev)
>> {
>> + struct drm_fb_helper *fb_helper = dev->fb_helper;
>> struct drm_connector *connector;
>> struct drm_connector_list_iter conn_iter;
>> struct drm_crtc *crtc, *ct;
>> @@ -417,6 +424,9 @@ void drm_mode_config_cleanup(struct drm_device *dev)
>> struct drm_property_blob *blob, *bt;
>> struct drm_plane *plane, *plt;
>>
>> + if (fb_helper && fb_helper->funcs && fb_helper->funcs->release)
>> + fb_helper->funcs->release(fb_helper);
>> +
>> list_for_each_entry_safe(encoder, enct, &dev->mode_config.encoder_list,
>> head) {
>> encoder->funcs->destroy(encoder);
>> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
>> index 555fbe54d6e2..9d8b0ba54173 100644
>> --- a/drivers/gpu/drm/drm_probe_helper.c
>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>> @@ -559,10 +559,14 @@ EXPORT_SYMBOL(drm_helper_probe_single_connector_modes);
>> */
>> void drm_kms_helper_hotplug_event(struct drm_device *dev)
>> {
>> + struct drm_fb_helper *fb_helper = dev->fb_helper;
>> +
>> /* 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 (fb_helper && fb_helper->funcs && fb_helper->funcs->hotplug_event)
>> + fb_helper->funcs->hotplug_event(fb_helper);
>> }
>> EXPORT_SYMBOL(drm_kms_helper_hotplug_event);
>>
>> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
>> index 16d8773b60e3..385f967c3552 100644
>> --- a/include/drm/drm_fb_helper.h
>> +++ b/include/drm/drm_fb_helper.h
>> @@ -125,6 +125,39 @@ struct drm_fb_helper_funcs {
>> struct drm_display_mode **modes,
>> struct drm_fb_offset *offsets,
>> bool *enabled, int width, int height);
>> +
>> + /**
>> + * @restore:
>> + *
>> + * Optional callback for restoring fbdev emulation.
>> + * Called by drm_lastclose() if &drm_driver->lastclose is not set.
>> + */
>> + int (*restore)(struct drm_fb_helper *fb_helper);
>> +
>> + /**
>> + * @hotplug_event:
>> + *
>> + * Optional callback for hotplug events.
>> + * Called by drm_kms_helper_hotplug_event() if
>> + * &drm_mode_config_funcs->output_poll_changed is not set.
>> + */
>> + int (*hotplug_event)(struct drm_fb_helper *fb_helper);
>> +
>> + /**
>> + * @unregister:
>> + *
>> + * Optional callback for unregistrering fbdev emulation.
>> + * Called by drm_dev_unregister().
>> + */
>> + void (*unregister)(struct drm_fb_helper *fb_helper);
>> +
>> + /**
>> + * @release:
>> + *
>> + * Optional callback for releasing fbdev emulation resources.
>> + * Called by drm_mode_config_cleanup().
>> + */
>> + void (*release)(struct drm_fb_helper *fb_helper);
>> };
>>
>> struct drm_fb_helper_connector {
>> --
>> 2.14.2
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [RFC v2 6/8] drm: Handle fbdev emulation in core
2018-01-10 17:02 ` Noralf Trønnes
@ 2018-01-11 7:45 ` Daniel Vetter
2018-01-11 14:09 ` Noralf Trønnes
0 siblings, 1 reply; 26+ messages in thread
From: Daniel Vetter @ 2018-01-11 7:45 UTC (permalink / raw)
To: Noralf Trønnes
Cc: daniel.vetter, intel-gfx, dri-devel, laurent.pinchart,
dh.herrmann
On Wed, Jan 10, 2018 at 06:02:38PM +0100, Noralf Trønnes wrote:
>
> Den 09.01.2018 11.38, skrev Daniel Vetter:
> > On Wed, Jan 03, 2018 at 11:21:08PM +0100, Noralf Trønnes wrote:
> > > Prepare for generic fbdev emulation by letting DRM core work directly
> > > with the fbdev compatibility layer. This is done by adding new fbdev
> > > helper vtable callbacks for restore, hotplug_event, unregister and
> > > release.
> > >
> > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > No clue whether my idea is any good, but inspired by the bootsplash
> > discussion, and the prospect that we might get other in-kernel drm/kms
> > clients I'm wondering whether we should make this a bit more generic and
> > tie it to drm_file?
> >
> > The idea would be to have a list of internal drm clients by putting all
> > the internal drm_files onto a list (same way we do with the userspace
> > ones). Then we'd just walk that list and call ->hotplug, ->unregister and
> > ->release at the right places.
> >
> > ->restore would be a bit different, I guess for that we'd only call the
> > ->restore callback of the very first kernel-internal client.
> >
> > With that we could then add/remove kernel-internal drm clients like normal
> > userspace clients, which should make integration of emergency consoles,
> > boot splashes and whatever else real easy. And I think it would be a lot
> > cleaner than leaking fb_helper knowledge into the drm core, which feels a
> > rather backwards.
> >
> > Otoh that feels a bit overengineered (but at least it shouldn't be a lot
> > more code really). If the list is too much work we could start with 1
> > drm_file * pointer for internal stuff (but would still need locking, so
> > list_head is probably easier).
> >
> > Thoughts?
>
> I prefer to have a proper in-kernel client API from the get go, instead
> of fixing it up later. The reason I skipped spending time on it in this
> RFC, was that I didn't know if I was on the right track at the right time
> to get the necessary attention to make this dumb_buffer idea happen.
>
> With a drm_file centric approach, are you thinking something like this:
>
> struct drm_device {
> + struct list_head filelist_internal;
> };
>
> +struct drm_file_funcs {
> + int (*restore)(struct drm_file *file);
> + void (*hotplug)(struct drm_file *file);
> + void (*unregister)(struct drm_file *file);
I guess we still need a cleanup callback here? For the usual two-stage
driver unload sequence of 1. unregister 2. cleanup.
> +};
>
> struct drm_file {
> + struct drm_device *dev;
> + const struct drm_file_funcs *funcs;
> };
>
> struct drm_file *drm_file_alloc(struct drm_minor *minor)
> {
> + file->dev = dev;
> }
>
> struct drm_file* drm_internal_open(struct drm_device *dev,
> const struct drm_file_funcs *funcs)
> struct drm_file *file;
> int ret;
>
> if (!try_module_get(dev->driver->fops->owner))
> return ERR_PTR(-ENODEV);
>
> drm_dev_ref(dev);
>
> file = drm_file_alloc(dev);
> if (IS_ERR(file)) {
> drm_dev_unref(dev);
> module_put(dev->driver->fops->owner);
> return file;
> }
>
> file->funcs = funcs;
>
> mutex_lock(&dev->filelist_mutex);
> list_add(&file->lhead, &dev->filelist_internal);
> mutex_unlock(&dev->filelist_mutex);
>
> return file;
> }
>
> void drm_internal_release(struct drm_file *file)
> {
> struct drm_device *dev = file->dev;
>
> mutex_lock(&dev->filelist_mutex);
> list_del(&file->lhead);
> mutex_unlock(&dev->filelist_mutex);
>
> drm_file_free(file);
> drm_dev_unref(dev);
> module_put(dev->driver->fops->owner);
> }
>
> void drm_lastclose(struct drm_device *dev)
> {
>
> + mutex_lock(&dev->filelist_mutex);
> + list_for_each_entry(file, &dev->filelist_internal, lhead) {
> + if (file->funcs && file->funcs->restore &&
> + !file->funcs->restore(file))
> + break;
> + mutex_unlock(&dev->filelist_mutex);
> }
>
> void drm_kms_helper_hotplug_event(struct drm_device *dev)
> {
>
> + mutex_lock(&dev->filelist_mutex);
> + list_for_each_entry(file, &dev->filelist_internal, lhead) {
> + if (file->funcs && file->funcs->hotplug)
> + file->funcs->hotplug(file);
> + mutex_unlock(&dev->filelist_mutex);
> }
>
> How is locking done when .unregister will call into drm_internal_release()?
>
> void drm_dev_unregister(struct drm_device *dev)
> {
>
> + list_for_each_entry(file, &dev->filelist_internal, lhead) {
> + if (file->funcs && file->funcs->unregister)
> + file->funcs->unregister(file);
> }
>
> There is also the case where .unregister can't release the drm_file
> because userspace has mmap'ed the buffer (fbdev). The client holds a ref
> on drm_device so cleanup is stalled but that requires a driver that can
> handle that (ref the tinydrm unplug series which is awaiting a timeslot).
Yeah we need the usual split into 2 things, with unregister only taking
away the uapi (but keeping references still to the drm_device/module).
Once fb_close has been called and the drm_device can be freed, then we
need a separate cleanup callback.
This also means that locking is no issue for ->unregister. It's only an
issue for ->cleanup:
- list_for_each_entry_safe, since the ->cleanup callback will self-remove
- a comment like we already have for the fb list that since cleanup is
called only from 1 thread, at a time where we know nothing else can get
at the drm_device anymore (the last reference was dropped) it's safe to
iterate without locking.
> amdgpu iterates dev->filelist in amdgpu_gem_force_release() and
> amdgpu_sched_process_priority_override(). I don't know if it needs to
> handle the internal ones as well.
That's for the scheduler, not for kms. As long as the internal clients are
kms-only (I don't expect that to ever change, gpu command submission needs
non-generic userspace) this shouldn't be a problem.
> What's the rational for having separate lists for internal and userspace?
Hm I guess we could unify them. But probably more work since we need to
make sure that all the existing filelist walkers won't be confused by the
internal stuff. I figured it's easier to just split them. If we end up
having lots of duplicated loops we can reconsider I think.
> There is one thing missing and that is notification of new drm_devices.
> The client iterates the available drm_devices on module init, but it
> needs a way to know about any later drm_device registrations.
Oh, I didn't notice that part. That's the exact same nasty issue fbcon
with trying to keep up to date fbdev drivers. fbcon uses a notifier, and
the resulting locking loops are terrible.
But I think if we do a notifier _only_ for new drm_device registrations
(i.e. we call the notifier from drm_dev_register), and let the
unregister/cleanup all get handled through the new drm_file callbacks, it
should be all fine. The only tricky bit will be closing the race when
loading the fbdev emulation code (or anything else really).
The real downside of this is that it forces distros to manually load the
fbdev emulation module (right now the explicit driver call is forcing
that), which is annoying.
But over the past years we've cleaned up the separation between the fbdev
emulation and the other helpers, and we could simply move the fbdev
emulation into the core drm.ko module. Then you'd simply put a direct call
to register the fbdev instance into drm_dev_register, done. No locking
headache, no races, not manual module loading needed.
Right now I'm leaning towards making the fbdev stuff built-into drm.ko,
but need to ponder this some more first.
-Daniel
>
> Noralf.
>
> > -Daniel
> >
> > > ---
> > > drivers/gpu/drm/drm_file.c | 12 +++++++++++-
> > > drivers/gpu/drm/drm_mode_config.c | 10 ++++++++++
> > > drivers/gpu/drm/drm_probe_helper.c | 4 ++++
> > > include/drm/drm_fb_helper.h | 33 +++++++++++++++++++++++++++++++++
> > > 4 files changed, 58 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > > index 400d44437e93..7ec09fb83135 100644
> > > --- a/drivers/gpu/drm/drm_file.c
> > > +++ b/drivers/gpu/drm/drm_file.c
> > > @@ -35,6 +35,7 @@
> > > #include <linux/slab.h>
> > > #include <linux/module.h>
> > > +#include <drm/drm_fb_helper.h>
> > > #include <drm/drm_file.h>
> > > #include <drm/drmP.h>
> > > @@ -441,10 +442,19 @@ static void drm_legacy_dev_reinit(struct drm_device *dev)
> > > void drm_lastclose(struct drm_device * dev)
> > > {
> > > + struct drm_fb_helper *fb_helper = dev->fb_helper;
> > > + int ret;
> > > +
> > > DRM_DEBUG("\n");
> > > - if (dev->driver->lastclose)
> > > + if (dev->driver->lastclose) {
> > > dev->driver->lastclose(dev);
> > > + } else if (fb_helper && fb_helper->funcs && fb_helper->funcs->restore) {
> > > + ret = fb_helper->funcs->restore(fb_helper);
> > > + if (ret)
> > > + DRM_ERROR("Failed to restore fbdev: %d\n", ret);
> > > + }
> > > +
> > > DRM_DEBUG("driver lastclose completed\n");
> > > if (drm_core_check_feature(dev, DRIVER_LEGACY))
> > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > > index bc5c46306b3d..260eb1730244 100644
> > > --- a/drivers/gpu/drm/drm_mode_config.c
> > > +++ b/drivers/gpu/drm/drm_mode_config.c
> > > @@ -21,6 +21,7 @@
> > > */
> > > #include <drm/drm_encoder.h>
> > > +#include <drm/drm_fb_helper.h>
> > > #include <drm/drm_mode_config.h>
> > > #include <drm/drmP.h>
> > > @@ -61,6 +62,11 @@ int drm_modeset_register_all(struct drm_device *dev)
> > > void drm_modeset_unregister_all(struct drm_device *dev)
> > > {
> > > + struct drm_fb_helper *fb_helper = dev->fb_helper;
> > > +
> > > + if (fb_helper && fb_helper->funcs && fb_helper->funcs->unregister)
> > > + fb_helper->funcs->unregister(fb_helper);
> > > +
> > > drm_connector_unregister_all(dev);
> > > drm_encoder_unregister_all(dev);
> > > drm_crtc_unregister_all(dev);
> > > @@ -408,6 +414,7 @@ EXPORT_SYMBOL(drm_mode_config_init);
> > > */
> > > void drm_mode_config_cleanup(struct drm_device *dev)
> > > {
> > > + struct drm_fb_helper *fb_helper = dev->fb_helper;
> > > struct drm_connector *connector;
> > > struct drm_connector_list_iter conn_iter;
> > > struct drm_crtc *crtc, *ct;
> > > @@ -417,6 +424,9 @@ void drm_mode_config_cleanup(struct drm_device *dev)
> > > struct drm_property_blob *blob, *bt;
> > > struct drm_plane *plane, *plt;
> > > + if (fb_helper && fb_helper->funcs && fb_helper->funcs->release)
> > > + fb_helper->funcs->release(fb_helper);
> > > +
> > > list_for_each_entry_safe(encoder, enct, &dev->mode_config.encoder_list,
> > > head) {
> > > encoder->funcs->destroy(encoder);
> > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > > index 555fbe54d6e2..9d8b0ba54173 100644
> > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > @@ -559,10 +559,14 @@ EXPORT_SYMBOL(drm_helper_probe_single_connector_modes);
> > > */
> > > void drm_kms_helper_hotplug_event(struct drm_device *dev)
> > > {
> > > + struct drm_fb_helper *fb_helper = dev->fb_helper;
> > > +
> > > /* 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 (fb_helper && fb_helper->funcs && fb_helper->funcs->hotplug_event)
> > > + fb_helper->funcs->hotplug_event(fb_helper);
> > > }
> > > EXPORT_SYMBOL(drm_kms_helper_hotplug_event);
> > > diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> > > index 16d8773b60e3..385f967c3552 100644
> > > --- a/include/drm/drm_fb_helper.h
> > > +++ b/include/drm/drm_fb_helper.h
> > > @@ -125,6 +125,39 @@ struct drm_fb_helper_funcs {
> > > struct drm_display_mode **modes,
> > > struct drm_fb_offset *offsets,
> > > bool *enabled, int width, int height);
> > > +
> > > + /**
> > > + * @restore:
> > > + *
> > > + * Optional callback for restoring fbdev emulation.
> > > + * Called by drm_lastclose() if &drm_driver->lastclose is not set.
> > > + */
> > > + int (*restore)(struct drm_fb_helper *fb_helper);
> > > +
> > > + /**
> > > + * @hotplug_event:
> > > + *
> > > + * Optional callback for hotplug events.
> > > + * Called by drm_kms_helper_hotplug_event() if
> > > + * &drm_mode_config_funcs->output_poll_changed is not set.
> > > + */
> > > + int (*hotplug_event)(struct drm_fb_helper *fb_helper);
> > > +
> > > + /**
> > > + * @unregister:
> > > + *
> > > + * Optional callback for unregistrering fbdev emulation.
> > > + * Called by drm_dev_unregister().
> > > + */
> > > + void (*unregister)(struct drm_fb_helper *fb_helper);
> > > +
> > > + /**
> > > + * @release:
> > > + *
> > > + * Optional callback for releasing fbdev emulation resources.
> > > + * Called by drm_mode_config_cleanup().
> > > + */
> > > + void (*release)(struct drm_fb_helper *fb_helper);
> > > };
> > > struct drm_fb_helper_connector {
> > > --
> > > 2.14.2
> > >
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [RFC v2 6/8] drm: Handle fbdev emulation in core
2018-01-11 7:45 ` Daniel Vetter
@ 2018-01-11 14:09 ` Noralf Trønnes
2018-01-18 21:36 ` Daniel Vetter
0 siblings, 1 reply; 26+ messages in thread
From: Noralf Trønnes @ 2018-01-11 14:09 UTC (permalink / raw)
To: Daniel Vetter
Cc: daniel.vetter, intel-gfx, dri-devel, laurent.pinchart,
dh.herrmann
Den 11.01.2018 08.45, skrev Daniel Vetter:
> On Wed, Jan 10, 2018 at 06:02:38PM +0100, Noralf Trønnes wrote:
>> Den 09.01.2018 11.38, skrev Daniel Vetter:
>>> On Wed, Jan 03, 2018 at 11:21:08PM +0100, Noralf Trønnes wrote:
>>>> Prepare for generic fbdev emulation by letting DRM core work directly
>>>> with the fbdev compatibility layer. This is done by adding new fbdev
>>>> helper vtable callbacks for restore, hotplug_event, unregister and
>>>> release.
>>>>
>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>> No clue whether my idea is any good, but inspired by the bootsplash
>>> discussion, and the prospect that we might get other in-kernel drm/kms
>>> clients I'm wondering whether we should make this a bit more generic and
>>> tie it to drm_file?
>>>
>>> The idea would be to have a list of internal drm clients by putting all
>>> the internal drm_files onto a list (same way we do with the userspace
>>> ones). Then we'd just walk that list and call ->hotplug, ->unregister and
>>> ->release at the right places.
>>>
>>> ->restore would be a bit different, I guess for that we'd only call the
>>> ->restore callback of the very first kernel-internal client.
>>>
>>> With that we could then add/remove kernel-internal drm clients like normal
>>> userspace clients, which should make integration of emergency consoles,
>>> boot splashes and whatever else real easy. And I think it would be a lot
>>> cleaner than leaking fb_helper knowledge into the drm core, which feels a
>>> rather backwards.
>>>
>>> Otoh that feels a bit overengineered (but at least it shouldn't be a lot
>>> more code really). If the list is too much work we could start with 1
>>> drm_file * pointer for internal stuff (but would still need locking, so
>>> list_head is probably easier).
>>>
>>> Thoughts?
>> I prefer to have a proper in-kernel client API from the get go, instead
>> of fixing it up later. The reason I skipped spending time on it in this
>> RFC, was that I didn't know if I was on the right track at the right time
>> to get the necessary attention to make this dumb_buffer idea happen.
>>
>> With a drm_file centric approach, are you thinking something like this:
>>
>> struct drm_device {
>> + struct list_head filelist_internal;
>> };
>>
>> +struct drm_file_funcs {
>> + int (*restore)(struct drm_file *file);
>> + void (*hotplug)(struct drm_file *file);
>> + void (*unregister)(struct drm_file *file);
> I guess we still need a cleanup callback here? For the usual two-stage
> driver unload sequence of 1. unregister 2. cleanup.
There's no need for a cleanup callback in this scenario.
The client holds a ref on drm_device through drm_internal_open() and if
it can't release the drm_file on .unregister, there won't be any cleanup.
When the client is in a position to release drm_file (fb_close), it will
do so and the ref is dropped.
If for some reason we can't take a ref, then we need to use
drm_device.open_count to avoid drm_device going away in drm_dev_unplug().
Noralf.
>
>> +};
>>
>> struct drm_file {
>> + struct drm_device *dev;
>> + const struct drm_file_funcs *funcs;
>> };
>>
>> struct drm_file *drm_file_alloc(struct drm_minor *minor)
>> {
>> + file->dev = dev;
>> }
>>
>> struct drm_file* drm_internal_open(struct drm_device *dev,
>> const struct drm_file_funcs *funcs)
>> struct drm_file *file;
>> int ret;
>>
>> if (!try_module_get(dev->driver->fops->owner))
>> return ERR_PTR(-ENODEV);
>>
>> drm_dev_ref(dev);
>>
>> file = drm_file_alloc(dev);
>> if (IS_ERR(file)) {
>> drm_dev_unref(dev);
>> module_put(dev->driver->fops->owner);
>> return file;
>> }
>>
>> file->funcs = funcs;
>>
>> mutex_lock(&dev->filelist_mutex);
>> list_add(&file->lhead, &dev->filelist_internal);
>> mutex_unlock(&dev->filelist_mutex);
>>
>> return file;
>> }
>>
>> void drm_internal_release(struct drm_file *file)
>> {
>> struct drm_device *dev = file->dev;
>>
>> mutex_lock(&dev->filelist_mutex);
>> list_del(&file->lhead);
>> mutex_unlock(&dev->filelist_mutex);
>>
>> drm_file_free(file);
>> drm_dev_unref(dev);
>> module_put(dev->driver->fops->owner);
>> }
>>
>> void drm_lastclose(struct drm_device *dev)
>> {
>>
>> + mutex_lock(&dev->filelist_mutex);
>> + list_for_each_entry(file, &dev->filelist_internal, lhead) {
>> + if (file->funcs && file->funcs->restore &&
>> + !file->funcs->restore(file))
>> + break;
>> + mutex_unlock(&dev->filelist_mutex);
>> }
>>
>> void drm_kms_helper_hotplug_event(struct drm_device *dev)
>> {
>>
>> + mutex_lock(&dev->filelist_mutex);
>> + list_for_each_entry(file, &dev->filelist_internal, lhead) {
>> + if (file->funcs && file->funcs->hotplug)
>> + file->funcs->hotplug(file);
>> + mutex_unlock(&dev->filelist_mutex);
>> }
>>
>> How is locking done when .unregister will call into drm_internal_release()?
>>
>> void drm_dev_unregister(struct drm_device *dev)
>> {
>>
>> + list_for_each_entry(file, &dev->filelist_internal, lhead) {
>> + if (file->funcs && file->funcs->unregister)
>> + file->funcs->unregister(file);
>> }
>>
>> There is also the case where .unregister can't release the drm_file
>> because userspace has mmap'ed the buffer (fbdev). The client holds a ref
>> on drm_device so cleanup is stalled but that requires a driver that can
>> handle that (ref the tinydrm unplug series which is awaiting a timeslot).
> Yeah we need the usual split into 2 things, with unregister only taking
> away the uapi (but keeping references still to the drm_device/module).
> Once fb_close has been called and the drm_device can be freed, then we
> need a separate cleanup callback.
>
> This also means that locking is no issue for ->unregister. It's only an
> issue for ->cleanup:
> - list_for_each_entry_safe, since the ->cleanup callback will self-remove
> - a comment like we already have for the fb list that since cleanup is
> called only from 1 thread, at a time where we know nothing else can get
> at the drm_device anymore (the last reference was dropped) it's safe to
> iterate without locking.
>
>> amdgpu iterates dev->filelist in amdgpu_gem_force_release() and
>> amdgpu_sched_process_priority_override(). I don't know if it needs to
>> handle the internal ones as well.
> That's for the scheduler, not for kms. As long as the internal clients are
> kms-only (I don't expect that to ever change, gpu command submission needs
> non-generic userspace) this shouldn't be a problem.
>
>> What's the rational for having separate lists for internal and userspace?
> Hm I guess we could unify them. But probably more work since we need to
> make sure that all the existing filelist walkers won't be confused by the
> internal stuff. I figured it's easier to just split them. If we end up
> having lots of duplicated loops we can reconsider I think.
>
>> There is one thing missing and that is notification of new drm_devices.
>> The client iterates the available drm_devices on module init, but it
>> needs a way to know about any later drm_device registrations.
> Oh, I didn't notice that part. That's the exact same nasty issue fbcon
> with trying to keep up to date fbdev drivers. fbcon uses a notifier, and
> the resulting locking loops are terrible.
>
> But I think if we do a notifier _only_ for new drm_device registrations
> (i.e. we call the notifier from drm_dev_register), and let the
> unregister/cleanup all get handled through the new drm_file callbacks, it
> should be all fine. The only tricky bit will be closing the race when
> loading the fbdev emulation code (or anything else really).
>
> The real downside of this is that it forces distros to manually load the
> fbdev emulation module (right now the explicit driver call is forcing
> that), which is annoying.
>
> But over the past years we've cleaned up the separation between the fbdev
> emulation and the other helpers, and we could simply move the fbdev
> emulation into the core drm.ko module. Then you'd simply put a direct call
> to register the fbdev instance into drm_dev_register, done. No locking
> headache, no races, not manual module loading needed.
>
> Right now I'm leaning towards making the fbdev stuff built-into drm.ko,
> but need to ponder this some more first.
> -Daniel
>
>> Noralf.
>>
>>> -Daniel
>>>
>>>> ---
>>>> drivers/gpu/drm/drm_file.c | 12 +++++++++++-
>>>> drivers/gpu/drm/drm_mode_config.c | 10 ++++++++++
>>>> drivers/gpu/drm/drm_probe_helper.c | 4 ++++
>>>> include/drm/drm_fb_helper.h | 33 +++++++++++++++++++++++++++++++++
>>>> 4 files changed, 58 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>>> index 400d44437e93..7ec09fb83135 100644
>>>> --- a/drivers/gpu/drm/drm_file.c
>>>> +++ b/drivers/gpu/drm/drm_file.c
>>>> @@ -35,6 +35,7 @@
>>>> #include <linux/slab.h>
>>>> #include <linux/module.h>
>>>> +#include <drm/drm_fb_helper.h>
>>>> #include <drm/drm_file.h>
>>>> #include <drm/drmP.h>
>>>> @@ -441,10 +442,19 @@ static void drm_legacy_dev_reinit(struct drm_device *dev)
>>>> void drm_lastclose(struct drm_device * dev)
>>>> {
>>>> + struct drm_fb_helper *fb_helper = dev->fb_helper;
>>>> + int ret;
>>>> +
>>>> DRM_DEBUG("\n");
>>>> - if (dev->driver->lastclose)
>>>> + if (dev->driver->lastclose) {
>>>> dev->driver->lastclose(dev);
>>>> + } else if (fb_helper && fb_helper->funcs && fb_helper->funcs->restore) {
>>>> + ret = fb_helper->funcs->restore(fb_helper);
>>>> + if (ret)
>>>> + DRM_ERROR("Failed to restore fbdev: %d\n", ret);
>>>> + }
>>>> +
>>>> DRM_DEBUG("driver lastclose completed\n");
>>>> if (drm_core_check_feature(dev, DRIVER_LEGACY))
>>>> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
>>>> index bc5c46306b3d..260eb1730244 100644
>>>> --- a/drivers/gpu/drm/drm_mode_config.c
>>>> +++ b/drivers/gpu/drm/drm_mode_config.c
>>>> @@ -21,6 +21,7 @@
>>>> */
>>>> #include <drm/drm_encoder.h>
>>>> +#include <drm/drm_fb_helper.h>
>>>> #include <drm/drm_mode_config.h>
>>>> #include <drm/drmP.h>
>>>> @@ -61,6 +62,11 @@ int drm_modeset_register_all(struct drm_device *dev)
>>>> void drm_modeset_unregister_all(struct drm_device *dev)
>>>> {
>>>> + struct drm_fb_helper *fb_helper = dev->fb_helper;
>>>> +
>>>> + if (fb_helper && fb_helper->funcs && fb_helper->funcs->unregister)
>>>> + fb_helper->funcs->unregister(fb_helper);
>>>> +
>>>> drm_connector_unregister_all(dev);
>>>> drm_encoder_unregister_all(dev);
>>>> drm_crtc_unregister_all(dev);
>>>> @@ -408,6 +414,7 @@ EXPORT_SYMBOL(drm_mode_config_init);
>>>> */
>>>> void drm_mode_config_cleanup(struct drm_device *dev)
>>>> {
>>>> + struct drm_fb_helper *fb_helper = dev->fb_helper;
>>>> struct drm_connector *connector;
>>>> struct drm_connector_list_iter conn_iter;
>>>> struct drm_crtc *crtc, *ct;
>>>> @@ -417,6 +424,9 @@ void drm_mode_config_cleanup(struct drm_device *dev)
>>>> struct drm_property_blob *blob, *bt;
>>>> struct drm_plane *plane, *plt;
>>>> + if (fb_helper && fb_helper->funcs && fb_helper->funcs->release)
>>>> + fb_helper->funcs->release(fb_helper);
>>>> +
>>>> list_for_each_entry_safe(encoder, enct, &dev->mode_config.encoder_list,
>>>> head) {
>>>> encoder->funcs->destroy(encoder);
>>>> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
>>>> index 555fbe54d6e2..9d8b0ba54173 100644
>>>> --- a/drivers/gpu/drm/drm_probe_helper.c
>>>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>>>> @@ -559,10 +559,14 @@ EXPORT_SYMBOL(drm_helper_probe_single_connector_modes);
>>>> */
>>>> void drm_kms_helper_hotplug_event(struct drm_device *dev)
>>>> {
>>>> + struct drm_fb_helper *fb_helper = dev->fb_helper;
>>>> +
>>>> /* 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 (fb_helper && fb_helper->funcs && fb_helper->funcs->hotplug_event)
>>>> + fb_helper->funcs->hotplug_event(fb_helper);
>>>> }
>>>> EXPORT_SYMBOL(drm_kms_helper_hotplug_event);
>>>> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
>>>> index 16d8773b60e3..385f967c3552 100644
>>>> --- a/include/drm/drm_fb_helper.h
>>>> +++ b/include/drm/drm_fb_helper.h
>>>> @@ -125,6 +125,39 @@ struct drm_fb_helper_funcs {
>>>> struct drm_display_mode **modes,
>>>> struct drm_fb_offset *offsets,
>>>> bool *enabled, int width, int height);
>>>> +
>>>> + /**
>>>> + * @restore:
>>>> + *
>>>> + * Optional callback for restoring fbdev emulation.
>>>> + * Called by drm_lastclose() if &drm_driver->lastclose is not set.
>>>> + */
>>>> + int (*restore)(struct drm_fb_helper *fb_helper);
>>>> +
>>>> + /**
>>>> + * @hotplug_event:
>>>> + *
>>>> + * Optional callback for hotplug events.
>>>> + * Called by drm_kms_helper_hotplug_event() if
>>>> + * &drm_mode_config_funcs->output_poll_changed is not set.
>>>> + */
>>>> + int (*hotplug_event)(struct drm_fb_helper *fb_helper);
>>>> +
>>>> + /**
>>>> + * @unregister:
>>>> + *
>>>> + * Optional callback for unregistrering fbdev emulation.
>>>> + * Called by drm_dev_unregister().
>>>> + */
>>>> + void (*unregister)(struct drm_fb_helper *fb_helper);
>>>> +
>>>> + /**
>>>> + * @release:
>>>> + *
>>>> + * Optional callback for releasing fbdev emulation resources.
>>>> + * Called by drm_mode_config_cleanup().
>>>> + */
>>>> + void (*release)(struct drm_fb_helper *fb_helper);
>>>> };
>>>> struct drm_fb_helper_connector {
>>>> --
>>>> 2.14.2
>>>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [RFC v2 6/8] drm: Handle fbdev emulation in core
2018-01-11 14:09 ` Noralf Trønnes
@ 2018-01-18 21:36 ` Daniel Vetter
0 siblings, 0 replies; 26+ messages in thread
From: Daniel Vetter @ 2018-01-18 21:36 UTC (permalink / raw)
To: Noralf Trønnes
Cc: intel-gfx, David Herrmann, Laurent Pinchart, dri-devel
[oops, this was stuck in my draft folder, sry]
On Thu, Jan 11, 2018 at 3:09 PM, Noralf Trønnes <noralf@tronnes.org> wrote:
>
> Den 11.01.2018 08.45, skrev Daniel Vetter:
>>
>> On Wed, Jan 10, 2018 at 06:02:38PM +0100, Noralf Trønnes wrote:
>>>
>>> Den 09.01.2018 11.38, skrev Daniel Vetter:
>>>>
>>>> On Wed, Jan 03, 2018 at 11:21:08PM +0100, Noralf Trønnes wrote:
>>>>>
>>>>> Prepare for generic fbdev emulation by letting DRM core work directly
>>>>> with the fbdev compatibility layer. This is done by adding new fbdev
>>>>> helper vtable callbacks for restore, hotplug_event, unregister and
>>>>> release.
>>>>>
>>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>>
>>>> No clue whether my idea is any good, but inspired by the bootsplash
>>>> discussion, and the prospect that we might get other in-kernel drm/kms
>>>> clients I'm wondering whether we should make this a bit more generic and
>>>> tie it to drm_file?
>>>>
>>>> The idea would be to have a list of internal drm clients by putting all
>>>> the internal drm_files onto a list (same way we do with the userspace
>>>> ones). Then we'd just walk that list and call ->hotplug, ->unregister
>>>> and
>>>> ->release at the right places.
>>>>
>>>> ->restore would be a bit different, I guess for that we'd only call the
>>>> ->restore callback of the very first kernel-internal client.
>>>>
>>>> With that we could then add/remove kernel-internal drm clients like
>>>> normal
>>>> userspace clients, which should make integration of emergency consoles,
>>>> boot splashes and whatever else real easy. And I think it would be a lot
>>>> cleaner than leaking fb_helper knowledge into the drm core, which feels
>>>> a
>>>> rather backwards.
>>>>
>>>> Otoh that feels a bit overengineered (but at least it shouldn't be a lot
>>>> more code really). If the list is too much work we could start with 1
>>>> drm_file * pointer for internal stuff (but would still need locking, so
>>>> list_head is probably easier).
>>>>
>>>> Thoughts?
>>>
>>> I prefer to have a proper in-kernel client API from the get go, instead
>>> of fixing it up later. The reason I skipped spending time on it in this
>>> RFC, was that I didn't know if I was on the right track at the right time
>>> to get the necessary attention to make this dumb_buffer idea happen.
>>>
>>> With a drm_file centric approach, are you thinking something like this:
>>>
>>> struct drm_device {
>>> + struct list_head filelist_internal;
>>> };
>>>
>>> +struct drm_file_funcs {
>>> + int (*restore)(struct drm_file *file);
>>> + void (*hotplug)(struct drm_file *file);
>>> + void (*unregister)(struct drm_file *file);
>>
>> I guess we still need a cleanup callback here? For the usual two-stage
>> driver unload sequence of 1. unregister 2. cleanup.
>
>
> There's no need for a cleanup callback in this scenario.
> The client holds a ref on drm_device through drm_internal_open() and if
> it can't release the drm_file on .unregister, there won't be any cleanup.
> When the client is in a position to release drm_file (fb_close), it will
> do so and the ref is dropped.
But when will we clean up fbdev helper allocations and things like
that? If we do that through a ->release callback I think we can avoid
a few special cases.
Hm right, drm_file holds a full ref, so we won't ever enter that case.
That's annoying, since this means there's no clear place for us to
release stuff again .... Should we perhaps try to drop the references
in ->unregister, but keep the drm_file on the list for ->release time?
I'd assume that trying to guess when exactly it's safe to release all
the fbdev allocated resources is otherwise going to be somewhat
tricky.
Or maybe I'm just seeing monsters when there's actually no problem at all :-)
> If for some reason we can't take a ref, then we need to use
> drm_device.open_count to avoid drm_device going away in drm_dev_unplug().
open_count also holds a drm_get_dev reference (but implicitly).
-Daniel
>
> Noralf.
>
>
>>
>>> +};
>>>
>>> struct drm_file {
>>> + struct drm_device *dev;
>>> + const struct drm_file_funcs *funcs;
>>> };
>>>
>>> struct drm_file *drm_file_alloc(struct drm_minor *minor)
>>> {
>>> + file->dev = dev;
>>> }
>>>
>>> struct drm_file* drm_internal_open(struct drm_device *dev,
>>> const struct drm_file_funcs *funcs)
>>> struct drm_file *file;
>>> int ret;
>>>
>>> if (!try_module_get(dev->driver->fops->owner))
>>> return ERR_PTR(-ENODEV);
>>>
>>> drm_dev_ref(dev);
>>>
>>> file = drm_file_alloc(dev);
>>> if (IS_ERR(file)) {
>>> drm_dev_unref(dev);
>>> module_put(dev->driver->fops->owner);
>>> return file;
>>> }
>>>
>>> file->funcs = funcs;
>>>
>>> mutex_lock(&dev->filelist_mutex);
>>> list_add(&file->lhead, &dev->filelist_internal);
>>> mutex_unlock(&dev->filelist_mutex);
>>>
>>> return file;
>>> }
>>>
>>> void drm_internal_release(struct drm_file *file)
>>> {
>>> struct drm_device *dev = file->dev;
>>>
>>> mutex_lock(&dev->filelist_mutex);
>>> list_del(&file->lhead);
>>> mutex_unlock(&dev->filelist_mutex);
>>>
>>> drm_file_free(file);
>>> drm_dev_unref(dev);
>>> module_put(dev->driver->fops->owner);
>>> }
>>>
>>> void drm_lastclose(struct drm_device *dev)
>>> {
>>>
>>> + mutex_lock(&dev->filelist_mutex);
>>> + list_for_each_entry(file, &dev->filelist_internal, lhead) {
>>> + if (file->funcs && file->funcs->restore &&
>>> + !file->funcs->restore(file))
>>> + break;
>>> + mutex_unlock(&dev->filelist_mutex);
>>> }
>>>
>>> void drm_kms_helper_hotplug_event(struct drm_device *dev)
>>> {
>>>
>>> + mutex_lock(&dev->filelist_mutex);
>>> + list_for_each_entry(file, &dev->filelist_internal, lhead) {
>>> + if (file->funcs && file->funcs->hotplug)
>>> + file->funcs->hotplug(file);
>>> + mutex_unlock(&dev->filelist_mutex);
>>> }
>>>
>>> How is locking done when .unregister will call into
>>> drm_internal_release()?
>>>
>>> void drm_dev_unregister(struct drm_device *dev)
>>> {
>>>
>>> + list_for_each_entry(file, &dev->filelist_internal, lhead) {
>>> + if (file->funcs && file->funcs->unregister)
>>> + file->funcs->unregister(file);
>>> }
>>>
>>> There is also the case where .unregister can't release the drm_file
>>> because userspace has mmap'ed the buffer (fbdev). The client holds a ref
>>> on drm_device so cleanup is stalled but that requires a driver that can
>>> handle that (ref the tinydrm unplug series which is awaiting a timeslot).
>>
>> Yeah we need the usual split into 2 things, with unregister only taking
>> away the uapi (but keeping references still to the drm_device/module).
>> Once fb_close has been called and the drm_device can be freed, then we
>> need a separate cleanup callback.
>>
>> This also means that locking is no issue for ->unregister. It's only an
>> issue for ->cleanup:
>> - list_for_each_entry_safe, since the ->cleanup callback will self-remove
>> - a comment like we already have for the fb list that since cleanup is
>> called only from 1 thread, at a time where we know nothing else can get
>> at the drm_device anymore (the last reference was dropped) it's safe to
>> iterate without locking.
>>
>>> amdgpu iterates dev->filelist in amdgpu_gem_force_release() and
>>> amdgpu_sched_process_priority_override(). I don't know if it needs to
>>> handle the internal ones as well.
>>
>> That's for the scheduler, not for kms. As long as the internal clients are
>> kms-only (I don't expect that to ever change, gpu command submission needs
>> non-generic userspace) this shouldn't be a problem.
>>
>>> What's the rational for having separate lists for internal and userspace?
>>
>> Hm I guess we could unify them. But probably more work since we need to
>> make sure that all the existing filelist walkers won't be confused by the
>> internal stuff. I figured it's easier to just split them. If we end up
>> having lots of duplicated loops we can reconsider I think.
>>
>>> There is one thing missing and that is notification of new drm_devices.
>>> The client iterates the available drm_devices on module init, but it
>>> needs a way to know about any later drm_device registrations.
>>
>> Oh, I didn't notice that part. That's the exact same nasty issue fbcon
>> with trying to keep up to date fbdev drivers. fbcon uses a notifier, and
>> the resulting locking loops are terrible.
>>
>> But I think if we do a notifier _only_ for new drm_device registrations
>> (i.e. we call the notifier from drm_dev_register), and let the
>> unregister/cleanup all get handled through the new drm_file callbacks, it
>> should be all fine. The only tricky bit will be closing the race when
>> loading the fbdev emulation code (or anything else really).
>>
>> The real downside of this is that it forces distros to manually load the
>> fbdev emulation module (right now the explicit driver call is forcing
>> that), which is annoying.
>>
>> But over the past years we've cleaned up the separation between the fbdev
>> emulation and the other helpers, and we could simply move the fbdev
>> emulation into the core drm.ko module. Then you'd simply put a direct call
>> to register the fbdev instance into drm_dev_register, done. No locking
>> headache, no races, not manual module loading needed.
>>
>> Right now I'm leaning towards making the fbdev stuff built-into drm.ko,
>> but need to ponder this some more first.
>> -Daniel
>>
>>> Noralf.
>>>
>>>> -Daniel
>>>>
>>>>> ---
>>>>> drivers/gpu/drm/drm_file.c | 12 +++++++++++-
>>>>> drivers/gpu/drm/drm_mode_config.c | 10 ++++++++++
>>>>> drivers/gpu/drm/drm_probe_helper.c | 4 ++++
>>>>> include/drm/drm_fb_helper.h | 33
>>>>> +++++++++++++++++++++++++++++++++
>>>>> 4 files changed, 58 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>>>> index 400d44437e93..7ec09fb83135 100644
>>>>> --- a/drivers/gpu/drm/drm_file.c
>>>>> +++ b/drivers/gpu/drm/drm_file.c
>>>>> @@ -35,6 +35,7 @@
>>>>> #include <linux/slab.h>
>>>>> #include <linux/module.h>
>>>>> +#include <drm/drm_fb_helper.h>
>>>>> #include <drm/drm_file.h>
>>>>> #include <drm/drmP.h>
>>>>> @@ -441,10 +442,19 @@ static void drm_legacy_dev_reinit(struct
>>>>> drm_device *dev)
>>>>> void drm_lastclose(struct drm_device * dev)
>>>>> {
>>>>> + struct drm_fb_helper *fb_helper = dev->fb_helper;
>>>>> + int ret;
>>>>> +
>>>>> DRM_DEBUG("\n");
>>>>> - if (dev->driver->lastclose)
>>>>> + if (dev->driver->lastclose) {
>>>>> dev->driver->lastclose(dev);
>>>>> + } else if (fb_helper && fb_helper->funcs &&
>>>>> fb_helper->funcs->restore) {
>>>>> + ret = fb_helper->funcs->restore(fb_helper);
>>>>> + if (ret)
>>>>> + DRM_ERROR("Failed to restore fbdev: %d\n",
>>>>> ret);
>>>>> + }
>>>>> +
>>>>> DRM_DEBUG("driver lastclose completed\n");
>>>>> if (drm_core_check_feature(dev, DRIVER_LEGACY))
>>>>> diff --git a/drivers/gpu/drm/drm_mode_config.c
>>>>> b/drivers/gpu/drm/drm_mode_config.c
>>>>> index bc5c46306b3d..260eb1730244 100644
>>>>> --- a/drivers/gpu/drm/drm_mode_config.c
>>>>> +++ b/drivers/gpu/drm/drm_mode_config.c
>>>>> @@ -21,6 +21,7 @@
>>>>> */
>>>>> #include <drm/drm_encoder.h>
>>>>> +#include <drm/drm_fb_helper.h>
>>>>> #include <drm/drm_mode_config.h>
>>>>> #include <drm/drmP.h>
>>>>> @@ -61,6 +62,11 @@ int drm_modeset_register_all(struct drm_device *dev)
>>>>> void drm_modeset_unregister_all(struct drm_device *dev)
>>>>> {
>>>>> + struct drm_fb_helper *fb_helper = dev->fb_helper;
>>>>> +
>>>>> + if (fb_helper && fb_helper->funcs &&
>>>>> fb_helper->funcs->unregister)
>>>>> + fb_helper->funcs->unregister(fb_helper);
>>>>> +
>>>>> drm_connector_unregister_all(dev);
>>>>> drm_encoder_unregister_all(dev);
>>>>> drm_crtc_unregister_all(dev);
>>>>> @@ -408,6 +414,7 @@ EXPORT_SYMBOL(drm_mode_config_init);
>>>>> */
>>>>> void drm_mode_config_cleanup(struct drm_device *dev)
>>>>> {
>>>>> + struct drm_fb_helper *fb_helper = dev->fb_helper;
>>>>> struct drm_connector *connector;
>>>>> struct drm_connector_list_iter conn_iter;
>>>>> struct drm_crtc *crtc, *ct;
>>>>> @@ -417,6 +424,9 @@ void drm_mode_config_cleanup(struct drm_device
>>>>> *dev)
>>>>> struct drm_property_blob *blob, *bt;
>>>>> struct drm_plane *plane, *plt;
>>>>> + if (fb_helper && fb_helper->funcs && fb_helper->funcs->release)
>>>>> + fb_helper->funcs->release(fb_helper);
>>>>> +
>>>>> list_for_each_entry_safe(encoder, enct,
>>>>> &dev->mode_config.encoder_list,
>>>>> head) {
>>>>> encoder->funcs->destroy(encoder);
>>>>> diff --git a/drivers/gpu/drm/drm_probe_helper.c
>>>>> b/drivers/gpu/drm/drm_probe_helper.c
>>>>> index 555fbe54d6e2..9d8b0ba54173 100644
>>>>> --- a/drivers/gpu/drm/drm_probe_helper.c
>>>>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>>>>> @@ -559,10 +559,14 @@
>>>>> EXPORT_SYMBOL(drm_helper_probe_single_connector_modes);
>>>>> */
>>>>> void drm_kms_helper_hotplug_event(struct drm_device *dev)
>>>>> {
>>>>> + struct drm_fb_helper *fb_helper = dev->fb_helper;
>>>>> +
>>>>> /* 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 (fb_helper && fb_helper->funcs &&
>>>>> fb_helper->funcs->hotplug_event)
>>>>> + fb_helper->funcs->hotplug_event(fb_helper);
>>>>> }
>>>>> EXPORT_SYMBOL(drm_kms_helper_hotplug_event);
>>>>> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
>>>>> index 16d8773b60e3..385f967c3552 100644
>>>>> --- a/include/drm/drm_fb_helper.h
>>>>> +++ b/include/drm/drm_fb_helper.h
>>>>> @@ -125,6 +125,39 @@ struct drm_fb_helper_funcs {
>>>>> struct drm_display_mode **modes,
>>>>> struct drm_fb_offset *offsets,
>>>>> bool *enabled, int width, int height);
>>>>> +
>>>>> + /**
>>>>> + * @restore:
>>>>> + *
>>>>> + * Optional callback for restoring fbdev emulation.
>>>>> + * Called by drm_lastclose() if &drm_driver->lastclose is not
>>>>> set.
>>>>> + */
>>>>> + int (*restore)(struct drm_fb_helper *fb_helper);
>>>>> +
>>>>> + /**
>>>>> + * @hotplug_event:
>>>>> + *
>>>>> + * Optional callback for hotplug events.
>>>>> + * Called by drm_kms_helper_hotplug_event() if
>>>>> + * &drm_mode_config_funcs->output_poll_changed is not set.
>>>>> + */
>>>>> + int (*hotplug_event)(struct drm_fb_helper *fb_helper);
>>>>> +
>>>>> + /**
>>>>> + * @unregister:
>>>>> + *
>>>>> + * Optional callback for unregistrering fbdev emulation.
>>>>> + * Called by drm_dev_unregister().
>>>>> + */
>>>>> + void (*unregister)(struct drm_fb_helper *fb_helper);
>>>>> +
>>>>> + /**
>>>>> + * @release:
>>>>> + *
>>>>> + * Optional callback for releasing fbdev emulation resources.
>>>>> + * Called by drm_mode_config_cleanup().
>>>>> + */
>>>>> + void (*release)(struct drm_fb_helper *fb_helper);
>>>>> };
>>>>> struct drm_fb_helper_connector {
>>>>> --
>>>>> 2.14.2
>>>>>
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* [RFC v2 7/8] drm/fb-helper: Add generic fbdev emulation
2018-01-03 22:21 [RFC v2 0/8] drm: Add generic fbdev emulation Noralf Trønnes
` (5 preceding siblings ...)
2018-01-03 22:21 ` [RFC v2 6/8] drm: Handle fbdev emulation in core Noralf Trønnes
@ 2018-01-03 22:21 ` Noralf Trønnes
2018-01-09 10:46 ` Daniel Vetter
2018-01-03 22:21 ` [RFC v2 8/8] drm/vc4: Test " Noralf Trønnes
2018-01-03 22:42 ` ✓ Fi.CI.BAT: success for drm: Add generic fbdev emulation (rev2) Patchwork
8 siblings, 1 reply; 26+ messages in thread
From: Noralf Trønnes @ 2018-01-03 22:21 UTC (permalink / raw)
To: dri-devel
Cc: daniel.vetter, intel-gfx, Noralf Trønnes, laurent.pinchart,
dh.herrmann
Add generic fbdev emulation which uses a drm_file to get a dumb_buffer
and drm_framebuffer. The buffer is exported and vmap/mmap called on
the dma-buf.
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
drivers/gpu/drm/drm_fb_helper.c | 301 +++++++++++++++++++++++++++++++++++++++-
include/drm/drm_fb_helper.h | 33 +++++
2 files changed, 333 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index f9dcc7a5761f..270ff6dc8045 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -30,12 +30,15 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include <linux/console.h>
+#include <linux/dma-buf.h>
#include <linux/kernel.h>
#include <linux/sysrq.h>
#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_dumb_buffers.h>
#include <drm/drm_fb_helper.h>
#include <drm/drm_crtc_helper.h>
#include <drm/drm_atomic.h>
@@ -1951,7 +1954,9 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
if (fb_helper->fbdev->fbops->fb_open == drm_fb_helper_fb_open)
atomic_set(&fb_helper->open_count, 0);
- strcpy(fb_helper->fb->comm, "[fbcon]");
+ if (fb_helper->fb)
+ strcpy(fb_helper->fb->comm, "[fbcon]");
+
return 0;
}
@@ -2975,6 +2980,300 @@ void drm_fb_helper_output_poll_changed(struct drm_device *dev)
}
EXPORT_SYMBOL(drm_fb_helper_output_poll_changed);
+static struct fb_deferred_io drm_fb_helper_generic_defio = {
+ .delay = HZ / 20,
+ .deferred_io = drm_fb_helper_deferred_io,
+};
+
+static int drm_fb_helper_generic_alloc_buf(struct drm_fb_helper *fb_helper)
+{
+ struct drm_fb_helper_surface_size *sizes = &fb_helper->sizes;
+ struct drm_mode_create_dumb dumb_args = { 0 };
+ struct drm_prime_handle prime_args = { 0 };
+ struct drm_mode_fb_cmd2 fb_args = { 0 };
+ struct drm_device *dev = fb_helper->dev;
+ struct fb_info *fbi = fb_helper->fbdev;
+ struct drm_framebuffer *fb;
+ struct dma_buf *dma_buf;
+ struct drm_file *file;
+ void *vaddr;
+ int ret;
+
+ file = drm_file_alloc(dev->primary);
+ if (IS_ERR(file))
+ return PTR_ERR(file);
+
+ drm_dropmaster_ioctl(dev, NULL, file);
+
+ dumb_args.width = sizes->surface_width;
+ dumb_args.height = sizes->surface_height;
+ dumb_args.bpp = sizes->surface_bpp;
+ ret = drm_mode_create_dumb_ioctl(dev, &dumb_args, file);
+ if (ret)
+ goto err_free_file;
+
+ fb_args.width = dumb_args.width;
+ fb_args.height = dumb_args.height;
+ fb_args.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
+ sizes->surface_depth);
+ fb_args.handles[0] = dumb_args.handle;
+ fb_args.pitches[0] = dumb_args.pitch;
+ ret = drm_mode_addfb2(dev, &fb_args, file);
+ if (ret)
+ goto err_free_file;
+
+ fb = drm_framebuffer_lookup(dev, file, fb_args.fb_id);
+ if (!fb) {
+ ret = -ENOENT;
+ goto err_free_file;
+ }
+
+ /* drop the reference we picked up in framebuffer lookup */
+ drm_framebuffer_put(fb);
+
+ strcpy(fb->comm, "[fbcon]");
+
+ prime_args.handle = dumb_args.handle;
+ ret = drm_prime_handle_to_fd_ioctl(dev, &prime_args, file);
+ if (ret)
+ goto err_free_file;
+
+ dma_buf = dma_buf_get(prime_args.fd);
+ if (WARN_ON(IS_ERR(dma_buf))) {
+ ret = PTR_ERR(dma_buf);
+ goto err_free_file;
+ }
+
+ vaddr = dma_buf_vmap(dma_buf);
+ if (!vaddr) {
+ ret = -ENOMEM;
+ goto err_put_dmabuf;
+ }
+
+ if (fb->funcs->dirty) {
+ fbi->fbdefio = &drm_fb_helper_generic_defio;
+ fb_deferred_io_init(fbi);
+ }
+
+ fbi->screen_size = fb->height * fb->pitches[0];
+ fbi->fix.smem_len = fbi->screen_size;
+ fbi->screen_buffer = vaddr;
+
+ fb_helper->dma_buf = dma_buf;
+ fb_helper->file = file;
+
+ mutex_lock(&fb_helper->lock);
+ fb_helper->fb = fb;
+ drm_setup_crtcs_fb(fb_helper);
+ mutex_unlock(&fb_helper->lock);
+
+ /* First time setup */
+ if (!fbi->var.bits_per_pixel) {
+ struct fb_videomode mode;
+
+ drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->format->depth);
+ drm_fb_helper_fill_var(fbi, fb_helper, sizes->fb_width, sizes->fb_height);
+
+ /* Drop the mode added by register_framebuffer() */
+ fb_destroy_modelist(&fb_helper->fbdev->modelist);
+
+ fb_var_to_videomode(&mode, &fbi->var);
+ fb_add_videomode(&mode, &fbi->modelist);
+ }
+
+ return 0;
+
+err_put_dmabuf:
+ dma_buf_put(dma_buf);
+err_free_file:
+ drm_file_free(file);
+
+ return ret;
+}
+
+static void drm_fb_helper_generic_free_buf(struct drm_fb_helper *fb_helper)
+{
+ mutex_lock(&fb_helper->lock);
+ fb_helper->fb = NULL;
+ drm_setup_crtcs_fb(fb_helper);
+ mutex_unlock(&fb_helper->lock);
+
+ if (fb_helper->fbdev->fbdefio) {
+ cancel_delayed_work_sync(&fb_helper->fbdev->deferred_work);
+ cancel_work_sync(&fb_helper->dirty_work);
+ fb_deferred_io_cleanup(fb_helper->fbdev);
+ }
+
+ dma_buf_vunmap(fb_helper->dma_buf, fb_helper->fbdev->screen_buffer);
+ dma_buf_put(fb_helper->dma_buf);
+ drm_file_free(fb_helper->file);
+
+ fb_helper->fbdev->screen_buffer = NULL;
+ fb_helper->dma_buf = NULL;
+ fb_helper->file = NULL;
+}
+
+static int drm_fb_helper_generic_fb_open(struct fb_info *info, int user)
+{
+ struct drm_fb_helper *fb_helper = info->par;
+ int ret;
+
+ ret = drm_fb_helper_fb_open(info, user);
+ if (ret)
+ return ret;
+
+ if (!fb_helper->fbdev->screen_buffer) {
+ /*
+ * Exporting a buffer to get a virtual address results in
+ * dma-buf pinning the driver module. This means that we have
+ * to defer this to open/close in order to unload the driver
+ * module.
+ */
+ ret = drm_fb_helper_generic_alloc_buf(fb_helper);
+ if (ret) {
+ DRM_ERROR("fbdev: Failed to allocate buffer: %d\n", ret);
+ drm_fb_helper_fb_release(info, user);
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static int drm_fb_helper_generic_fb_release(struct fb_info *info, int user)
+{
+ struct drm_fb_helper *fb_helper = info->par;
+
+ drm_fb_helper_fb_release(info, user);
+
+ if (!atomic_read(&fb_helper->open_count))
+ drm_fb_helper_generic_free_buf(fb_helper);
+
+ return 0;
+}
+
+static int drm_fb_helper_generic_fb_mmap(struct fb_info *info,
+ struct vm_area_struct *vma)
+{
+ struct drm_fb_helper *fb_helper = info->par;
+
+ return dma_buf_mmap(fb_helper->dma_buf, vma, 0);
+}
+
+static struct fb_ops drm_fb_helper_generic_fbdev_ops = {
+ .owner = THIS_MODULE,
+ DRM_FB_HELPER_DEFAULT_OPS,
+ .fb_open = drm_fb_helper_generic_fb_open,
+ .fb_release = drm_fb_helper_generic_fb_release,
+ .fb_mmap = drm_fb_helper_generic_fb_mmap,
+ .fb_read = drm_fb_helper_sys_read,
+ .fb_write = drm_fb_helper_sys_write,
+ .fb_fillrect = drm_fb_helper_sys_fillrect,
+ .fb_copyarea = drm_fb_helper_sys_copyarea,
+ .fb_imageblit = drm_fb_helper_sys_imageblit,
+};
+
+static int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,
+ struct drm_fb_helper_surface_size *sizes)
+{
+ struct fb_ops *fbops;
+ struct fb_info *fbi;
+
+ DRM_DEBUG_KMS("surface width(%d), height(%d) and bpp(%d)\n",
+ sizes->surface_width, sizes->surface_height,
+ sizes->surface_bpp);
+
+ fb_helper->sizes = *sizes;
+
+ /*
+ * fb_deferred_io_cleanup() clears &fbops->fb_mmap so a per instance
+ * version is necessary. We do it for all users since we don't know
+ * yet if the fb has a dirty callback. This also gives us the
+ * opportunity to set the correct owner.
+ */
+ fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
+ if (!fbops)
+ return -ENOMEM;
+
+ *fbops = drm_fb_helper_generic_fbdev_ops;
+ fbops->owner = fb_helper->dev->driver->fops->owner;
+
+ fbi = drm_fb_helper_alloc_fbi(fb_helper);
+ if (IS_ERR(fbi)) {
+ kfree(fbops);
+ return PTR_ERR(fbi);
+ }
+
+ fbi->par = fb_helper;
+ fbi->fbops = fbops;
+ strcpy(fbi->fix.id, "generic");
+
+ /* The rest of the setup is deferred to fb_open */
+
+ atomic_set(&fb_helper->open_count, 0);
+
+ return 0;
+}
+
+static void drm_fb_helper_generic_release(struct drm_fb_helper *fb_helper)
+{
+ struct fb_ops *fbops = fb_helper->fbdev->fbops;
+
+ drm_fb_helper_fini(fb_helper);
+ kfree(fb_helper);
+ kfree(fbops);
+}
+
+static const struct drm_fb_helper_funcs drm_fb_helper_generic_funcs = {
+ .fb_probe = drm_fb_helper_generic_probe,
+ .restore = drm_fb_helper_restore_fbdev_mode_unlocked,
+ .hotplug_event = drm_fb_helper_hotplug_event,
+ .unregister = drm_fb_helper_unregister_fbi,
+ .release = drm_fb_helper_generic_release,
+};
+
+/**
+ * drm_fb_helper_generic_fbdev_setup() - Setup generic fbdev emulation
+ * @dev: DRM device
+ * @preferred_bpp: Preferred bits per pixel for the device.
+ * @dev->mode_config.preferred_depth is used if this is zero.
+ * @max_conn_count: Maximum number of connectors.
+ * @dev->mode_config.num_connector is used if this is zero.
+ *
+ * This function sets up generic fbdev emulation for drivers that supports
+ * dumb buffers which can be exported. The driver doesn't have to do anything
+ * else than to call this function, restore, hotplug events and teardown are
+ * all taken care of.
+ *
+ * Returns:
+ * Zero on success or negative error code on failure.
+ */
+int drm_fb_helper_generic_fbdev_setup(struct drm_device *dev,
+ unsigned int preferred_bpp,
+ unsigned int max_conn_count)
+{
+ struct drm_fb_helper *fb_helper;
+ int ret;
+
+ if (!drm_fbdev_emulation)
+ return 0;
+
+ fb_helper = kzalloc(sizeof(*fb_helper), GFP_KERNEL);
+ if (!fb_helper)
+ return -ENOMEM;
+
+ ret = drm_fb_helper_fbdev_setup(dev, fb_helper,
+ &drm_fb_helper_generic_funcs,
+ preferred_bpp, max_conn_count);
+ if (ret) {
+ kfree(fb_helper);
+ return ret;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_fb_helper_generic_fbdev_setup);
+
/* The Kconfig DRM_KMS_HELPER selects FRAMEBUFFER_CONSOLE (if !EXPERT)
* but the module doesn't depend on any fb console symbols. At least
* attempt to load fbcon to avoid leaving the system without a usable console.
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 385f967c3552..c6940ab6ffac 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -279,6 +279,28 @@ struct drm_fb_helper {
* initial value to 0 themselves.
*/
atomic_t open_count;
+
+ /**
+ * @file:
+ *
+ * Optional DRM file. Used by the generic fbdev code.
+ */
+ struct drm_file *file;
+
+ /**
+ * @dma_buf:
+ *
+ * Optional pointer to a DMA buffer object.
+ * Used by the generic fbdev code.
+ */
+ struct dma_buf *dma_buf;
+
+ /**
+ * @sizes:
+ *
+ * Optional surface sizes. Used by the generic fbdev code.
+ */
+ struct drm_fb_helper_surface_size sizes;
};
/**
@@ -380,6 +402,10 @@ void drm_fb_helper_fbdev_teardown(struct drm_device *dev);
void drm_fb_helper_lastclose(struct drm_device *dev);
void drm_fb_helper_output_poll_changed(struct drm_device *dev);
+
+int drm_fb_helper_generic_fbdev_setup(struct drm_device *dev,
+ unsigned int preferred_bpp,
+ unsigned int max_conn_count);
#else
static inline void drm_fb_helper_prepare(struct drm_device *dev,
struct drm_fb_helper *helper,
@@ -624,6 +650,13 @@ static inline void drm_fb_helper_output_poll_changed(struct drm_device *dev)
{
}
+static inline int
+drm_fb_helper_generic_fbdev_setup(struct drm_device *dev,
+ unsigned int preferred_bpp,
+ unsigned int max_conn_count)
+{
+ return 0;
+}
#endif
static inline int
--
2.14.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [RFC v2 7/8] drm/fb-helper: Add generic fbdev emulation
2018-01-03 22:21 ` [RFC v2 7/8] drm/fb-helper: Add generic fbdev emulation Noralf Trønnes
@ 2018-01-09 10:46 ` Daniel Vetter
0 siblings, 0 replies; 26+ messages in thread
From: Daniel Vetter @ 2018-01-09 10:46 UTC (permalink / raw)
To: Noralf Trønnes; +Cc: daniel.vetter, intel-gfx, dri-devel, laurent.pinchart
On Wed, Jan 03, 2018 at 11:21:09PM +0100, Noralf Trønnes wrote:
> Add generic fbdev emulation which uses a drm_file to get a dumb_buffer
> and drm_framebuffer. The buffer is exported and vmap/mmap called on
> the dma-buf.
>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
> drivers/gpu/drm/drm_fb_helper.c | 301 +++++++++++++++++++++++++++++++++++++++-
> include/drm/drm_fb_helper.h | 33 +++++
> 2 files changed, 333 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index f9dcc7a5761f..270ff6dc8045 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -30,12 +30,15 @@
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> #include <linux/console.h>
> +#include <linux/dma-buf.h>
> #include <linux/kernel.h>
> #include <linux/sysrq.h>
> #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_dumb_buffers.h>
> #include <drm/drm_fb_helper.h>
> #include <drm/drm_crtc_helper.h>
> #include <drm/drm_atomic.h>
> @@ -1951,7 +1954,9 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
> if (fb_helper->fbdev->fbops->fb_open == drm_fb_helper_fb_open)
> atomic_set(&fb_helper->open_count, 0);
>
> - strcpy(fb_helper->fb->comm, "[fbcon]");
> + if (fb_helper->fb)
> + strcpy(fb_helper->fb->comm, "[fbcon]");
> +
> return 0;
> }
>
> @@ -2975,6 +2980,300 @@ void drm_fb_helper_output_poll_changed(struct drm_device *dev)
> }
> EXPORT_SYMBOL(drm_fb_helper_output_poll_changed);
>
> +static struct fb_deferred_io drm_fb_helper_generic_defio = {
> + .delay = HZ / 20,
> + .deferred_io = drm_fb_helper_deferred_io,
> +};
> +
> +static int drm_fb_helper_generic_alloc_buf(struct drm_fb_helper *fb_helper)
> +{
> + struct drm_fb_helper_surface_size *sizes = &fb_helper->sizes;
> + struct drm_mode_create_dumb dumb_args = { 0 };
> + struct drm_prime_handle prime_args = { 0 };
> + struct drm_mode_fb_cmd2 fb_args = { 0 };
> + struct drm_device *dev = fb_helper->dev;
> + struct fb_info *fbi = fb_helper->fbdev;
> + struct drm_framebuffer *fb;
> + struct dma_buf *dma_buf;
> + struct drm_file *file;
> + void *vaddr;
> + int ret;
> +
> + file = drm_file_alloc(dev->primary);
> + if (IS_ERR(file))
> + return PTR_ERR(file);
> +
> + drm_dropmaster_ioctl(dev, NULL, file);
Hm .... why do we need this? Feels a bit like drm_file_alloc shouldn't do
the entire master dance for us ...
Otherwise this looks awesome, I really like it.
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-Daniel
> +
> + dumb_args.width = sizes->surface_width;
> + dumb_args.height = sizes->surface_height;
> + dumb_args.bpp = sizes->surface_bpp;
> + ret = drm_mode_create_dumb_ioctl(dev, &dumb_args, file);
> + if (ret)
> + goto err_free_file;
> +
> + fb_args.width = dumb_args.width;
> + fb_args.height = dumb_args.height;
> + fb_args.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
> + sizes->surface_depth);
> + fb_args.handles[0] = dumb_args.handle;
> + fb_args.pitches[0] = dumb_args.pitch;
> + ret = drm_mode_addfb2(dev, &fb_args, file);
> + if (ret)
> + goto err_free_file;
> +
> + fb = drm_framebuffer_lookup(dev, file, fb_args.fb_id);
> + if (!fb) {
> + ret = -ENOENT;
> + goto err_free_file;
> + }
> +
> + /* drop the reference we picked up in framebuffer lookup */
> + drm_framebuffer_put(fb);
> +
> + strcpy(fb->comm, "[fbcon]");
> +
> + prime_args.handle = dumb_args.handle;
> + ret = drm_prime_handle_to_fd_ioctl(dev, &prime_args, file);
> + if (ret)
> + goto err_free_file;
> +
> + dma_buf = dma_buf_get(prime_args.fd);
> + if (WARN_ON(IS_ERR(dma_buf))) {
> + ret = PTR_ERR(dma_buf);
> + goto err_free_file;
> + }
> +
> + vaddr = dma_buf_vmap(dma_buf);
> + if (!vaddr) {
> + ret = -ENOMEM;
> + goto err_put_dmabuf;
> + }
> +
> + if (fb->funcs->dirty) {
> + fbi->fbdefio = &drm_fb_helper_generic_defio;
> + fb_deferred_io_init(fbi);
> + }
> +
> + fbi->screen_size = fb->height * fb->pitches[0];
> + fbi->fix.smem_len = fbi->screen_size;
> + fbi->screen_buffer = vaddr;
> +
> + fb_helper->dma_buf = dma_buf;
> + fb_helper->file = file;
> +
> + mutex_lock(&fb_helper->lock);
> + fb_helper->fb = fb;
> + drm_setup_crtcs_fb(fb_helper);
> + mutex_unlock(&fb_helper->lock);
> +
> + /* First time setup */
> + if (!fbi->var.bits_per_pixel) {
> + struct fb_videomode mode;
> +
> + drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->format->depth);
> + drm_fb_helper_fill_var(fbi, fb_helper, sizes->fb_width, sizes->fb_height);
> +
> + /* Drop the mode added by register_framebuffer() */
> + fb_destroy_modelist(&fb_helper->fbdev->modelist);
> +
> + fb_var_to_videomode(&mode, &fbi->var);
> + fb_add_videomode(&mode, &fbi->modelist);
> + }
> +
> + return 0;
> +
> +err_put_dmabuf:
> + dma_buf_put(dma_buf);
> +err_free_file:
> + drm_file_free(file);
> +
> + return ret;
> +}
> +
> +static void drm_fb_helper_generic_free_buf(struct drm_fb_helper *fb_helper)
> +{
> + mutex_lock(&fb_helper->lock);
> + fb_helper->fb = NULL;
> + drm_setup_crtcs_fb(fb_helper);
> + mutex_unlock(&fb_helper->lock);
> +
> + if (fb_helper->fbdev->fbdefio) {
> + cancel_delayed_work_sync(&fb_helper->fbdev->deferred_work);
> + cancel_work_sync(&fb_helper->dirty_work);
> + fb_deferred_io_cleanup(fb_helper->fbdev);
> + }
> +
> + dma_buf_vunmap(fb_helper->dma_buf, fb_helper->fbdev->screen_buffer);
> + dma_buf_put(fb_helper->dma_buf);
> + drm_file_free(fb_helper->file);
> +
> + fb_helper->fbdev->screen_buffer = NULL;
> + fb_helper->dma_buf = NULL;
> + fb_helper->file = NULL;
> +}
> +
> +static int drm_fb_helper_generic_fb_open(struct fb_info *info, int user)
> +{
> + struct drm_fb_helper *fb_helper = info->par;
> + int ret;
> +
> + ret = drm_fb_helper_fb_open(info, user);
> + if (ret)
> + return ret;
> +
> + if (!fb_helper->fbdev->screen_buffer) {
> + /*
> + * Exporting a buffer to get a virtual address results in
> + * dma-buf pinning the driver module. This means that we have
> + * to defer this to open/close in order to unload the driver
> + * module.
> + */
> + ret = drm_fb_helper_generic_alloc_buf(fb_helper);
> + if (ret) {
> + DRM_ERROR("fbdev: Failed to allocate buffer: %d\n", ret);
> + drm_fb_helper_fb_release(info, user);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int drm_fb_helper_generic_fb_release(struct fb_info *info, int user)
> +{
> + struct drm_fb_helper *fb_helper = info->par;
> +
> + drm_fb_helper_fb_release(info, user);
> +
> + if (!atomic_read(&fb_helper->open_count))
> + drm_fb_helper_generic_free_buf(fb_helper);
> +
> + return 0;
> +}
> +
> +static int drm_fb_helper_generic_fb_mmap(struct fb_info *info,
> + struct vm_area_struct *vma)
> +{
> + struct drm_fb_helper *fb_helper = info->par;
> +
> + return dma_buf_mmap(fb_helper->dma_buf, vma, 0);
> +}
> +
> +static struct fb_ops drm_fb_helper_generic_fbdev_ops = {
> + .owner = THIS_MODULE,
> + DRM_FB_HELPER_DEFAULT_OPS,
> + .fb_open = drm_fb_helper_generic_fb_open,
> + .fb_release = drm_fb_helper_generic_fb_release,
> + .fb_mmap = drm_fb_helper_generic_fb_mmap,
> + .fb_read = drm_fb_helper_sys_read,
> + .fb_write = drm_fb_helper_sys_write,
> + .fb_fillrect = drm_fb_helper_sys_fillrect,
> + .fb_copyarea = drm_fb_helper_sys_copyarea,
> + .fb_imageblit = drm_fb_helper_sys_imageblit,
> +};
> +
> +static int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,
> + struct drm_fb_helper_surface_size *sizes)
> +{
> + struct fb_ops *fbops;
> + struct fb_info *fbi;
> +
> + DRM_DEBUG_KMS("surface width(%d), height(%d) and bpp(%d)\n",
> + sizes->surface_width, sizes->surface_height,
> + sizes->surface_bpp);
> +
> + fb_helper->sizes = *sizes;
> +
> + /*
> + * fb_deferred_io_cleanup() clears &fbops->fb_mmap so a per instance
> + * version is necessary. We do it for all users since we don't know
> + * yet if the fb has a dirty callback. This also gives us the
> + * opportunity to set the correct owner.
> + */
> + fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
> + if (!fbops)
> + return -ENOMEM;
> +
> + *fbops = drm_fb_helper_generic_fbdev_ops;
> + fbops->owner = fb_helper->dev->driver->fops->owner;
> +
> + fbi = drm_fb_helper_alloc_fbi(fb_helper);
> + if (IS_ERR(fbi)) {
> + kfree(fbops);
> + return PTR_ERR(fbi);
> + }
> +
> + fbi->par = fb_helper;
> + fbi->fbops = fbops;
> + strcpy(fbi->fix.id, "generic");
> +
> + /* The rest of the setup is deferred to fb_open */
> +
> + atomic_set(&fb_helper->open_count, 0);
> +
> + return 0;
> +}
> +
> +static void drm_fb_helper_generic_release(struct drm_fb_helper *fb_helper)
> +{
> + struct fb_ops *fbops = fb_helper->fbdev->fbops;
> +
> + drm_fb_helper_fini(fb_helper);
> + kfree(fb_helper);
> + kfree(fbops);
> +}
> +
> +static const struct drm_fb_helper_funcs drm_fb_helper_generic_funcs = {
> + .fb_probe = drm_fb_helper_generic_probe,
> + .restore = drm_fb_helper_restore_fbdev_mode_unlocked,
> + .hotplug_event = drm_fb_helper_hotplug_event,
> + .unregister = drm_fb_helper_unregister_fbi,
> + .release = drm_fb_helper_generic_release,
> +};
> +
> +/**
> + * drm_fb_helper_generic_fbdev_setup() - Setup generic fbdev emulation
> + * @dev: DRM device
> + * @preferred_bpp: Preferred bits per pixel for the device.
> + * @dev->mode_config.preferred_depth is used if this is zero.
> + * @max_conn_count: Maximum number of connectors.
> + * @dev->mode_config.num_connector is used if this is zero.
> + *
> + * This function sets up generic fbdev emulation for drivers that supports
> + * dumb buffers which can be exported. The driver doesn't have to do anything
> + * else than to call this function, restore, hotplug events and teardown are
> + * all taken care of.
> + *
> + * Returns:
> + * Zero on success or negative error code on failure.
> + */
> +int drm_fb_helper_generic_fbdev_setup(struct drm_device *dev,
> + unsigned int preferred_bpp,
> + unsigned int max_conn_count)
> +{
> + struct drm_fb_helper *fb_helper;
> + int ret;
> +
> + if (!drm_fbdev_emulation)
> + return 0;
> +
> + fb_helper = kzalloc(sizeof(*fb_helper), GFP_KERNEL);
> + if (!fb_helper)
> + return -ENOMEM;
> +
> + ret = drm_fb_helper_fbdev_setup(dev, fb_helper,
> + &drm_fb_helper_generic_funcs,
> + preferred_bpp, max_conn_count);
> + if (ret) {
> + kfree(fb_helper);
> + return ret;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_fb_helper_generic_fbdev_setup);
> +
> /* The Kconfig DRM_KMS_HELPER selects FRAMEBUFFER_CONSOLE (if !EXPERT)
> * but the module doesn't depend on any fb console symbols. At least
> * attempt to load fbcon to avoid leaving the system without a usable console.
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 385f967c3552..c6940ab6ffac 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -279,6 +279,28 @@ struct drm_fb_helper {
> * initial value to 0 themselves.
> */
> atomic_t open_count;
> +
> + /**
> + * @file:
> + *
> + * Optional DRM file. Used by the generic fbdev code.
> + */
> + struct drm_file *file;
> +
> + /**
> + * @dma_buf:
> + *
> + * Optional pointer to a DMA buffer object.
> + * Used by the generic fbdev code.
> + */
> + struct dma_buf *dma_buf;
> +
> + /**
> + * @sizes:
> + *
> + * Optional surface sizes. Used by the generic fbdev code.
> + */
> + struct drm_fb_helper_surface_size sizes;
> };
>
> /**
> @@ -380,6 +402,10 @@ void drm_fb_helper_fbdev_teardown(struct drm_device *dev);
>
> void drm_fb_helper_lastclose(struct drm_device *dev);
> void drm_fb_helper_output_poll_changed(struct drm_device *dev);
> +
> +int drm_fb_helper_generic_fbdev_setup(struct drm_device *dev,
> + unsigned int preferred_bpp,
> + unsigned int max_conn_count);
> #else
> static inline void drm_fb_helper_prepare(struct drm_device *dev,
> struct drm_fb_helper *helper,
> @@ -624,6 +650,13 @@ static inline void drm_fb_helper_output_poll_changed(struct drm_device *dev)
> {
> }
>
> +static inline int
> +drm_fb_helper_generic_fbdev_setup(struct drm_device *dev,
> + unsigned int preferred_bpp,
> + unsigned int max_conn_count)
> +{
> + return 0;
> +}
> #endif
>
> static inline int
> --
> 2.14.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* [RFC v2 8/8] drm/vc4: Test generic fbdev emulation
2018-01-03 22:21 [RFC v2 0/8] drm: Add generic fbdev emulation Noralf Trønnes
` (6 preceding siblings ...)
2018-01-03 22:21 ` [RFC v2 7/8] drm/fb-helper: Add generic fbdev emulation Noralf Trønnes
@ 2018-01-03 22:21 ` Noralf Trønnes
2018-01-03 22:42 ` ✓ Fi.CI.BAT: success for drm: Add generic fbdev emulation (rev2) Patchwork
8 siblings, 0 replies; 26+ messages in thread
From: Noralf Trønnes @ 2018-01-03 22:21 UTC (permalink / raw)
To: dri-devel
Cc: daniel.vetter, intel-gfx, Noralf Trønnes, laurent.pinchart,
dh.herrmann
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
drivers/gpu/drm/vc4/vc4_drv.c | 3 ---
drivers/gpu/drm/vc4/vc4_kms.c | 3 +--
2 files changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index ceb385fd69c5..ef8a2d3a6d1f 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -152,7 +152,6 @@ static struct drm_driver vc4_drm_driver = {
DRIVER_HAVE_IRQ |
DRIVER_RENDER |
DRIVER_PRIME),
- .lastclose = drm_fb_helper_lastclose,
.irq_handler = vc4_irq,
.irq_preinstall = vc4_irq_preinstall,
.irq_postinstall = vc4_irq_postinstall,
@@ -297,8 +296,6 @@ static void vc4_drm_unbind(struct device *dev)
drm_dev_unregister(drm);
- drm_fb_cma_fbdev_fini(drm);
-
drm_mode_config_cleanup(drm);
drm_dev_unref(drm);
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 4256f294c346..671c62f1b4d3 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -188,7 +188,6 @@ static struct drm_framebuffer *vc4_fb_create(struct drm_device *dev,
}
static const struct drm_mode_config_funcs vc4_mode_funcs = {
- .output_poll_changed = drm_fb_helper_output_poll_changed,
.atomic_check = drm_atomic_helper_check,
.atomic_commit = vc4_atomic_commit,
.fb_create = vc4_fb_create,
@@ -219,7 +218,7 @@ int vc4_kms_load(struct drm_device *dev)
drm_mode_config_reset(dev);
if (dev->mode_config.num_connector)
- drm_fb_cma_fbdev_init(dev, 32, 0);
+ drm_fb_helper_generic_fbdev_setup(dev, 32, 0);
drm_kms_helper_poll_init(dev);
--
2.14.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread* ✓ Fi.CI.BAT: success for drm: Add generic fbdev emulation (rev2)
2018-01-03 22:21 [RFC v2 0/8] drm: Add generic fbdev emulation Noralf Trønnes
` (7 preceding siblings ...)
2018-01-03 22:21 ` [RFC v2 8/8] drm/vc4: Test " Noralf Trønnes
@ 2018-01-03 22:42 ` Patchwork
8 siblings, 0 replies; 26+ messages in thread
From: Patchwork @ 2018-01-03 22:42 UTC (permalink / raw)
To: Noralf Trønnes; +Cc: intel-gfx
== Series Details ==
Series: drm: Add generic fbdev emulation (rev2)
URL : https://patchwork.freedesktop.org/series/35873/
State : success
== Summary ==
Series 35873v2 drm: Add generic fbdev emulation
https://patchwork.freedesktop.org/api/1.0/series/35873/revisions/2/mbox/
Test debugfs_test:
Subgroup read_all_entries:
dmesg-fail -> DMESG-WARN (fi-elk-e7500) fdo#103989
Test gem_exec_reloc:
Subgroup basic-write-cpu:
incomplete -> PASS (fi-byt-j1900)
Test kms_psr_sink_crc:
Subgroup psr_basic:
pass -> DMESG-WARN (fi-skl-6700hq) fdo#101144
fdo#103989 https://bugs.freedesktop.org/show_bug.cgi?id=103989
fdo#101144 https://bugs.freedesktop.org/show_bug.cgi?id=101144
fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:437s
fi-bdw-gvtdvm total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:452s
fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:386s
fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:501s
fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:277s
fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:497s
fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:500s
fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:482s
fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:473s
fi-elk-e7500 total:224 pass:168 dwarn:10 dfail:0 fail:0 skip:45
fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:529s
fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:406s
fi-hsw-4770r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:415s
fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:424s
fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:471s
fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:427s
fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:484s
fi-kbl-7560u total:288 pass:268 dwarn:1 dfail:0 fail:0 skip:19 time:513s
fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:469s
fi-kbl-r total:288 pass:260 dwarn:1 dfail:0 fail:0 skip:27 time:514s
fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:595s
fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:446s
fi-skl-6600u total:288 pass:260 dwarn:1 dfail:0 fail:0 skip:27 time:525s
fi-skl-6700hq total:288 pass:261 dwarn:1 dfail:0 fail:0 skip:26 time:541s
fi-skl-6700k2 total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:508s
fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:498s
fi-skl-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:447s
fi-snb-2520m total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:542s
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:410s
Blacklisted hosts:
fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:580s
fi-glk-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:481s
d26e7804b83cf7d3754323d02afa5ff04326d33d drm-tip: 2018y-01m-03d-17h-48m-30s UTC integration manifest
fc7421689c65 drm/vc4: Test generic fbdev emulation
896906b4c1de drm/fb-helper: Add generic fbdev emulation
dbd4dcf1cf44 drm: Handle fbdev emulation in core
ad91cd97f6bb drm/fb-helper: Don't restore if fbdev is not in use
e014540787a3 drm/fb-helper: Ensure driver module is pinned in fb_open()
50d1352cc8ca drm: Export some ioctl functions
f80d3012c620 drm/ioctl: Remove trailing whitespace
ffbe71a1ca18 drm: provide management functions for drm_file
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7604/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread