[PATCH v2 00/12] drm: Add generic fbdev emulation

[PATCH v2 00/12] drm: Add generic fbdev emulation

[Noralf Trønnes]

This patchset adds generic fbdev emulation for drivers that supports GEM
based dumb buffers which support .gem_prime_vmap and gem_prime_mmap. An
API is begun to support in-kernel clients in general.

Notable changes since version 1:

- Rework client unregister code. I've used reference counting to manage
  the fact that both the client itself and the driver through
  drm_dev_unregister() can release the client. The client is now released
  using drm_client_put() instead of drm_client_free().

- fbdev: Use a shadow buffer for framebuffers that have a dirty
  callback. This makes the fbdev client truly generic and useable for all
  drivers. There's a blitting penalty, but this is generic emulation after
  all. The reason for needing a shadow buffer is that deferred I/O only
  works with kmalloc/vmalloc buffers and not with shmem buffers
  (page->lru/mapping).

- Let tinydrm use the full fbdev client

Noralf.

Changes since version 1:
- Make it possible to embed struct drm_client_dev and drop the private
  pointer
- Use kref reference counting to control client release since both the
  client and the driver can release.
- Add comment about using dma-buf as a possibility with some rework
- Move buffer NULL check to drm_client_framebuffer_delete()
- Move client name to struct drm_client_dev
- Move up drm_dev_get/put calls to make them more visible
- Move drm_client_dev.list definition to later patch that makes use of it

- Embed drm_client at the beginning of drm_fb_helper to avoid a fragile
  transitional kfree hack in drm_client_release()
- Set owner in drm_fbdev_fb_ops
- Add kerneldoc to drm_fb_helper_generic_probe()

- Remove unused functions
- Change name drm_client_funcs.lastclose -> .restore
- Change name drm_client_funcs.remove -> .unregister
- Rework unregister code

- tinydrm: Use drm_fbdev_generic_setup() and remove
  drm_fb_cma_fbdev_init_with_funcs()

David Herrmann (1):
  drm: provide management functions for drm_file

Noralf Trønnes (11):
  drm/file: Don't set master on in-kernel clients
  drm: Make ioctls available for in-kernel clients
  drm: Begin an API for in-kernel clients
  drm/fb-helper: Add generic fbdev emulation .fb_probe function
  drm/pl111: Set .gem_prime_vmap and .gem_prime_mmap
  drm/cma-helper: Use the generic fbdev emulation
  drm/client: Add client callbacks
  drm/debugfs: Add internal client debugfs file
  drm/fb-helper: Finish the generic fbdev emulation
  drm/tinydrm: Use drm_fbdev_generic_setup()
  drm/cma-helper: Remove drm_fb_cma_fbdev_init_with_funcs()

 Documentation/gpu/drm-client.rst            |  12 +
 Documentation/gpu/index.rst                 |   1 +
 drivers/gpu/drm/Makefile                    |   2 +-
 drivers/gpu/drm/drm_client.c                | 435 ++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_crtc_internal.h         |  19 +-
 drivers/gpu/drm/drm_debugfs.c               |   7 +
 drivers/gpu/drm/drm_drv.c                   |   8 +
 drivers/gpu/drm/drm_dumb_buffers.c          |  33 ++-
 drivers/gpu/drm/drm_fb_cma_helper.c         | 380 +++---------------------
 drivers/gpu/drm/drm_fb_helper.c             | 330 ++++++++++++++++++++-
 drivers/gpu/drm/drm_file.c                  | 304 ++++++++++---------
 drivers/gpu/drm/drm_framebuffer.c           |  42 ++-
 drivers/gpu/drm/drm_internal.h              |   2 +
 drivers/gpu/drm/drm_ioctl.c                 |   4 +-
 drivers/gpu/drm/drm_probe_helper.c          |   3 +
 drivers/gpu/drm/pl111/pl111_drv.c           |   2 +
 drivers/gpu/drm/tinydrm/core/tinydrm-core.c |   3 +-
 drivers/gpu/drm/tinydrm/ili9225.c           |   1 -
 drivers/gpu/drm/tinydrm/mi0283qt.c          |   1 -
 drivers/gpu/drm/tinydrm/st7586.c            |   1 -
 drivers/gpu/drm/tinydrm/st7735r.c           |   1 -
 include/drm/drm_client.h                    | 156 ++++++++++
 include/drm/drm_device.h                    |  21 ++
 include/drm/drm_fb_cma_helper.h             |   6 -
 include/drm/drm_fb_helper.h                 |  38 +++
 25 files changed, 1298 insertions(+), 514 deletions(-)
 create mode 100644 Documentation/gpu/drm-client.rst
 create mode 100644 drivers/gpu/drm/drm_client.c
 create mode 100644 include/drm/drm_client.h

-- 
2.15.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 01/12] drm: provide management functions for drm_file

[Noralf Trønnes]

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>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_file.c     | 305 +++++++++++++++++++++++------------------
 drivers/gpu/drm/drm_internal.h |   2 +
 2 files changed, 175 insertions(+), 132 deletions(-)

diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index e394799979a6..d4588d33f91c 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -101,6 +101,175 @@ 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.
+ *
+ * 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.
+ *
+ * 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 +365,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 +375,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 +407,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 +473,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.15.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 02/12] drm/file: Don't set master on in-kernel clients

[Noralf Trønnes]

It only makes sense for userspace clients.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_file.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index d4588d33f91c..55505378df47 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -155,17 +155,8 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
 			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);
@@ -365,6 +356,7 @@ 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 */
@@ -379,6 +371,14 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor)
 	if (IS_ERR(priv))
 		return PTR_ERR(priv);
 
+	if (drm_is_primary_client(priv)) {
+		ret = drm_master_open(priv);
+		if (ret) {
+			drm_file_free(priv);
+			return ret;
+		}
+	}
+
 	filp->private_data = priv;
 	priv->filp = filp;
 
-- 
2.15.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 03/12] drm: Make ioctls available for in-kernel clients

[Noralf Trønnes]

Make ioctl wrappers for functions that will be used by the in-kernel API.
The following functions are touched:
- drm_mode_create_dumb_ioctl()
- drm_mode_destroy_dumb_ioctl()
- drm_mode_addfb()
- drm_mode_rmfb()

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc_internal.h | 19 +++++++++++++----
 drivers/gpu/drm/drm_dumb_buffers.c  | 33 +++++++++++++++++++----------
 drivers/gpu/drm/drm_framebuffer.c   | 42 ++++++++++++++++++++++++-------------
 drivers/gpu/drm/drm_ioctl.c         |  4 ++--
 4 files changed, 66 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 5d307b23a4e6..c762614af453 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -62,6 +62,12 @@ int drm_mode_getresources(struct drm_device *dev,
 
 
 /* drm_dumb_buffers.c */
+int drm_mode_create_dumb(struct drm_device *dev,
+			 struct drm_mode_create_dumb *args,
+			 struct drm_file *file_priv);
+int drm_mode_destroy_dumb(struct drm_device *dev, u32 handle,
+			  struct drm_file *file_priv);
+
 /* IOCTLs */
 int drm_mode_create_dumb_ioctl(struct drm_device *dev,
 			       void *data, struct drm_file *file_priv);
@@ -163,14 +169,19 @@ int drm_framebuffer_check_src_coords(uint32_t src_x, uint32_t src_y,
 				     const struct drm_framebuffer *fb);
 void drm_fb_release(struct drm_file *file_priv);
 
+int drm_mode_addfb(struct drm_device *dev, struct drm_mode_fb_cmd *or,
+		   struct drm_file *file_priv);
+int drm_mode_rmfb(struct drm_device *dev, u32 fb_id,
+		  struct drm_file *file_priv);
+
 
 /* IOCTL */
-int drm_mode_addfb(struct drm_device *dev,
-		   void *data, struct drm_file *file_priv);
+int drm_mode_addfb_ioctl(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_rmfb_ioctl(struct drm_device *dev,
+			void *data, struct drm_file *file_priv);
 int drm_mode_getfb(struct drm_device *dev,
 		   void *data, struct drm_file *file_priv);
 int drm_mode_dirtyfb_ioctl(struct drm_device *dev,
diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
index 39ac15ce4702..eed9687b8698 100644
--- a/drivers/gpu/drm/drm_dumb_buffers.c
+++ b/drivers/gpu/drm/drm_dumb_buffers.c
@@ -53,10 +53,10 @@
  * a hardware-specific ioctl to allocate suitable buffer objects.
  */
 
-int drm_mode_create_dumb_ioctl(struct drm_device *dev,
-			       void *data, struct drm_file *file_priv)
+int drm_mode_create_dumb(struct drm_device *dev,
+			 struct drm_mode_create_dumb *args,
+			 struct drm_file *file_priv)
 {
-	struct drm_mode_create_dumb *args = data;
 	u32 cpp, stride, size;
 
 	if (!dev->driver->dumb_create)
@@ -91,6 +91,12 @@ int drm_mode_create_dumb_ioctl(struct drm_device *dev,
 	return dev->driver->dumb_create(file_priv, dev, args);
 }
 
+int drm_mode_create_dumb_ioctl(struct drm_device *dev,
+			       void *data, struct drm_file *file_priv)
+{
+	return drm_mode_create_dumb(dev, data, file_priv);
+}
+
 /**
  * drm_mode_mmap_dumb_ioctl - create an mmap offset for a dumb backing storage buffer
  * @dev: DRM device
@@ -122,17 +128,22 @@ int drm_mode_mmap_dumb_ioctl(struct drm_device *dev,
 					       &args->offset);
 }
 
+int drm_mode_destroy_dumb(struct drm_device *dev, u32 handle,
+			  struct drm_file *file_priv)
+{
+	if (!dev->driver->dumb_create)
+		return -ENOSYS;
+
+	if (dev->driver->dumb_destroy)
+		return dev->driver->dumb_destroy(file_priv, dev, handle);
+	else
+		return drm_gem_dumb_destroy(file_priv, dev, handle);
+}
+
 int drm_mode_destroy_dumb_ioctl(struct drm_device *dev,
 				void *data, struct drm_file *file_priv)
 {
 	struct drm_mode_destroy_dumb *args = data;
 
-	if (!dev->driver->dumb_create)
-		return -ENOSYS;
-
-	if (dev->driver->dumb_destroy)
-		return dev->driver->dumb_destroy(file_priv, dev, args->handle);
-	else
-		return drm_gem_dumb_destroy(file_priv, dev, args->handle);
+	return drm_mode_destroy_dumb(dev, args->handle, file_priv);
 }
-
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index bfedceff87bb..44759aeed1e7 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -95,21 +95,20 @@ int drm_framebuffer_check_src_coords(uint32_t src_x, uint32_t src_y,
 /**
  * drm_mode_addfb - add an FB to the graphics configuration
  * @dev: drm device for the ioctl
- * @data: data pointer for the ioctl
- * @file_priv: drm file for the ioctl call
+ * @or: pointer to request structure
+ * @file_priv: drm file
  *
  * Add a new FB to the specified CRTC, given a user request. This is the
  * original addfb ioctl which only supported RGB formats.
  *
- * Called by the user via ioctl.
+ * Called by the user via ioctl, or by an in-kernel client.
  *
  * Returns:
  * Zero on success, negative errno on failure.
  */
-int drm_mode_addfb(struct drm_device *dev,
-		   void *data, struct drm_file *file_priv)
+int drm_mode_addfb(struct drm_device *dev, struct drm_mode_fb_cmd *or,
+		   struct drm_file *file_priv)
 {
-	struct drm_mode_fb_cmd *or = data;
 	struct drm_mode_fb_cmd2 r = {};
 	int ret;
 
@@ -134,6 +133,12 @@ int drm_mode_addfb(struct drm_device *dev,
 	return 0;
 }
 
+int drm_mode_addfb_ioctl(struct drm_device *dev,
+			 void *data, struct drm_file *file_priv)
+{
+	return drm_mode_addfb(dev, data, file_priv);
+}
+
 static int fb_plane_width(int width,
 			  const struct drm_format_info *format, int plane)
 {
@@ -367,29 +372,28 @@ static void drm_mode_rmfb_work_fn(struct work_struct *w)
 
 /**
  * drm_mode_rmfb - remove an FB from the configuration
- * @dev: drm device for the ioctl
- * @data: data pointer for the ioctl
- * @file_priv: drm file for the ioctl call
+ * @dev: drm device
+ * @fb_id: id of framebuffer to remove
+ * @file_priv: drm file
  *
- * Remove the FB specified by the user.
+ * Remove the specified FB.
  *
- * Called by the user via ioctl.
+ * Called by the user via ioctl, or by an in-kernel client.
  *
  * Returns:
  * Zero on success, negative errno on failure.
  */
-int drm_mode_rmfb(struct drm_device *dev,
-		   void *data, struct drm_file *file_priv)
+int drm_mode_rmfb(struct drm_device *dev, u32 fb_id,
+		  struct drm_file *file_priv)
 {
 	struct drm_framebuffer *fb = NULL;
 	struct drm_framebuffer *fbl = NULL;
-	uint32_t *id = data;
 	int found = 0;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
 
-	fb = drm_framebuffer_lookup(dev, file_priv, *id);
+	fb = drm_framebuffer_lookup(dev, file_priv, fb_id);
 	if (!fb)
 		return -ENOENT;
 
@@ -435,6 +439,14 @@ int drm_mode_rmfb(struct drm_device *dev,
 	return -ENOENT;
 }
 
+int drm_mode_rmfb_ioctl(struct drm_device *dev,
+			void *data, struct drm_file *file_priv)
+{
+	uint32_t *fb_id = data;
+
+	return drm_mode_rmfb(dev, *fb_id, file_priv);
+}
+
 /**
  * drm_mode_getfb - get FB info
  * @dev: drm device for the ioctl
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 0d4cfb232576..9f659e1a19c5 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -637,9 +637,9 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_SETPROPERTY, drm_mode_connector_property_set_ioctl, DRM_MASTER|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPROPBLOB, drm_mode_getblob_ioctl, DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETFB, drm_mode_getfb, DRM_UNLOCKED),
-	DRM_IOCTL_DEF(DRM_IOCTL_MODE_ADDFB, drm_mode_addfb, DRM_UNLOCKED),
+	DRM_IOCTL_DEF(DRM_IOCTL_MODE_ADDFB, drm_mode_addfb_ioctl, DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_ADDFB2, drm_mode_addfb2, DRM_UNLOCKED),
-	DRM_IOCTL_DEF(DRM_IOCTL_MODE_RMFB, drm_mode_rmfb, DRM_UNLOCKED),
+	DRM_IOCTL_DEF(DRM_IOCTL_MODE_RMFB, drm_mode_rmfb_ioctl, DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_PAGE_FLIP, drm_mode_page_flip_ioctl, DRM_MASTER|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_DIRTYFB, drm_mode_dirtyfb_ioctl, DRM_MASTER|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_DUMB, drm_mode_create_dumb_ioctl, DRM_UNLOCKED),
-- 
2.15.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 04/12] drm: Begin an API for in-kernel clients

[Noralf Trønnes]

This the beginning of an API for in-kernel clients.
First out is a way to get a framebuffer backed by a dumb buffer.

Only GEM drivers are supported.
The original idea of using an exported dma-buf was dropped because it
also creates an anonomous file descriptor which doesn't work when the
buffer is created from a kernel thread. The easy way out is to use
drm_driver.gem_prime_vmap to get the virtual address, which requires a
GEM object. This excludes the vmwgfx driver which is the only non-GEM
driver apart from the legacy ones. A solution for vmwgfx will have to be
worked out later if it wants to support the client API which it probably
will when we have a bootsplash client.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---

Changes since version 1:
- Make it possible to embed struct drm_client_dev and drop the private
  pointer
- Use kref reference counting to control client release since both the
  client and the driver can release.
- Add comment about using dma-buf as a possibility with some rework
- Move buffer NULL check to drm_client_framebuffer_delete()
- Move client name to struct drm_client_dev
- Move up drm_dev_get/put calls to make them more visible
- Move drm_client_dev.list definition to later patch that makes use of it


 Documentation/gpu/drm-client.rst |  12 ++
 Documentation/gpu/index.rst      |   1 +
 drivers/gpu/drm/Makefile         |   2 +-
 drivers/gpu/drm/drm_client.c     | 316 +++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_drv.c        |   1 +
 include/drm/drm_client.h         |  87 +++++++++++
 include/drm/drm_device.h         |   7 +
 7 files changed, 425 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/gpu/drm-client.rst
 create mode 100644 drivers/gpu/drm/drm_client.c
 create mode 100644 include/drm/drm_client.h

diff --git a/Documentation/gpu/drm-client.rst b/Documentation/gpu/drm-client.rst
new file mode 100644
index 000000000000..7e672063e7eb
--- /dev/null
+++ b/Documentation/gpu/drm-client.rst
@@ -0,0 +1,12 @@
+=================
+Kernel clients
+=================
+
+.. kernel-doc:: drivers/gpu/drm/drm_client.c
+   :doc: overview
+
+.. kernel-doc:: include/drm/drm_client.h
+   :internal:
+
+.. kernel-doc:: drivers/gpu/drm/drm_client.c
+   :export:
diff --git a/Documentation/gpu/index.rst b/Documentation/gpu/index.rst
index 00288f34c5a6..1fcf8e851e15 100644
--- a/Documentation/gpu/index.rst
+++ b/Documentation/gpu/index.rst
@@ -10,6 +10,7 @@ Linux GPU Driver Developer's Guide
    drm-kms
    drm-kms-helpers
    drm-uapi
+   drm-client
    drivers
    vga-switcheroo
    vgaarbiter
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index ef9f3dab287f..8c8045147416 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -18,7 +18,7 @@ drm-y       :=	drm_auth.o drm_bufs.o drm_cache.o \
 		drm_encoder.o drm_mode_object.o drm_property.o \
 		drm_plane.o drm_color_mgmt.o drm_print.o \
 		drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \
-		drm_syncobj.o drm_lease.o
+		drm_syncobj.o drm_lease.o drm_client.o
 
 drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
 drm-$(CONFIG_DRM_VM) += drm_vm.o
diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
new file mode 100644
index 000000000000..98dda40f5416
--- /dev/null
+++ b/drivers/gpu/drm/drm_client.c
@@ -0,0 +1,316 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2018 Noralf Trønnes
+ */
+
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/seq_file.h>
+#include <linux/slab.h>
+
+#include <drm/drm_client.h>
+#include <drm/drm_debugfs.h>
+#include <drm/drm_device.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_file.h>
+#include <drm/drm_fourcc.h>
+#include <drm/drm_gem.h>
+#include <drm/drm_mode.h>
+#include <drm/drm_print.h>
+#include <drm/drmP.h>
+
+#include "drm_crtc_internal.h"
+#include "drm_internal.h"
+
+/**
+ * DOC: overview
+ *
+ * This library provides support for clients running in the kernel like fbdev and bootsplash.
+ * Currently it's only partially implemented, just enough to support fbdev.
+ *
+ * GEM drivers which provide a GEM based dumb buffer with a virtual address are supported.
+ */
+
+static int drm_client_open(struct drm_client_dev *client)
+{
+	struct drm_device *dev = client->dev;
+	struct drm_file *file;
+
+	file = drm_file_alloc(dev->primary);
+	if (IS_ERR(file))
+		return PTR_ERR(file);
+
+	mutex_lock(&dev->filelist_mutex);
+	list_add(&file->lhead, &dev->filelist_internal);
+	mutex_unlock(&dev->filelist_mutex);
+
+	client->file = file;
+
+	return 0;
+}
+
+/**
+ * drm_client_close - Close the client DRM file
+ * @client: DRM client
+ *
+ * Close the &drm_file that was opened by drm_client_new(). It is called
+ * automatically if the &drm_client_funcs.release callback is not set.
+ * This function should only be called from the release callback.
+ */
+void drm_client_close(struct drm_client_dev *client)
+{
+	struct drm_device *dev = client->dev;
+
+	mutex_lock(&dev->filelist_mutex);
+	list_del(&client->file->lhead);
+	mutex_unlock(&dev->filelist_mutex);
+
+	drm_file_free(client->file);
+}
+EXPORT_SYMBOL(drm_client_close);
+
+/**
+ * drm_client_new - Create a DRM client
+ * @dev: DRM device
+ * @client: DRM client
+ * @name: Client name
+ *
+ * Use drm_client_put() to free the client.
+ *
+ * Returns:
+ * Zero on success or negative error code on failure.
+ */
+int drm_client_new(struct drm_device *dev, struct drm_client_dev *client,
+		   const char *name)
+{
+	int ret;
+
+	if (!drm_core_check_feature(dev, DRIVER_MODESET) ||
+	    !dev->driver->dumb_create || !dev->driver->gem_prime_vmap)
+		return -ENOTSUPP;
+
+	client->dev = dev;
+	client->name = name;
+	kref_init(&client->ref);
+
+	ret = drm_client_open(client);
+	if (ret)
+		return ret;
+
+	drm_dev_get(dev);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_client_new);
+
+static void drm_client_release(struct kref *ref)
+{
+	struct drm_client_dev *client = container_of(ref, struct drm_client_dev, ref);
+	struct drm_device *dev = client->dev;
+
+	DRM_DEV_DEBUG_KMS(dev->dev, "%s\n", client->name);
+
+	drm_client_close(client);
+	kfree(client);
+
+	drm_dev_put(dev);
+}
+
+/**
+ * drm_client_get - Take a reference of a DRM client
+ * @client: DRM client (can be NULL)
+ *
+ * This increases the ref-count of @client by one. You *must* already own a
+ * reference when calling this. Use drm_client_put() to drop this reference
+ * again.
+ */
+void drm_client_get(struct drm_client_dev *client)
+{
+	if (client)
+		kref_get(&client->ref);
+}
+EXPORT_SYMBOL(drm_client_get);
+
+/**
+ * drm_client_put - Drop reference of a DRM client
+ * @client: DRM client (can be NULL)
+ *
+ * This decreases the ref-count of @client by one. The client resources are
+ * released if the ref-count drops to zero.
+ */
+void drm_client_put(struct drm_client_dev *client)
+{
+	if (client)
+		kref_put(&client->ref, drm_client_release);
+}
+EXPORT_SYMBOL(drm_client_put);
+
+static void drm_client_buffer_delete(struct drm_client_buffer *buffer)
+{
+	struct drm_device *dev;
+
+	dev = buffer->client->dev;
+	if (buffer->vaddr && dev->driver->gem_prime_vunmap)
+		dev->driver->gem_prime_vunmap(buffer->gem, buffer->vaddr);
+
+	if (buffer->gem)
+		drm_gem_object_put_unlocked(buffer->gem);
+
+	drm_mode_destroy_dumb(dev, buffer->handle, buffer->client->file);
+	kfree(buffer);
+}
+
+static struct drm_client_buffer *
+drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u32 format)
+{
+	struct drm_mode_create_dumb dumb_args = { };
+	struct drm_device *dev = client->dev;
+	struct drm_client_buffer *buffer;
+	struct drm_gem_object *obj;
+	void *vaddr;
+	int ret;
+
+	buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
+	if (!buffer)
+		return ERR_PTR(-ENOMEM);
+
+	buffer->client = client;
+
+	dumb_args.width = width;
+	dumb_args.height = height;
+	dumb_args.bpp = drm_format_plane_cpp(format, 0) * 8;
+	ret = drm_mode_create_dumb(dev, &dumb_args, client->file);
+	if (ret)
+		goto err_free;
+
+	buffer->handle = dumb_args.handle;
+	buffer->pitch = dumb_args.pitch;
+
+	obj = drm_gem_object_lookup(client->file, dumb_args.handle);
+	if (!obj)  {
+		ret = -ENOENT;
+		goto err_delete;
+	}
+
+	buffer->gem = obj;
+
+	/*
+	 * FIXME: The dependency on GEM here isn't required, we could
+	 * convert the driver handle to a dma-buf instead and use the
+	 * backend-agnostic dma-buf vmap support instead. This would
+	 * require that the handle2fd prime ioctl is reworked to pull the
+	 * fd_install step out of the driver backend hooks, to make that
+	 * final step optional for internal users.
+	 */
+	vaddr = dev->driver->gem_prime_vmap(obj);
+	if (!vaddr) {
+		ret = -ENOMEM;
+		goto err_delete;
+	}
+
+	buffer->vaddr = vaddr;
+
+	return buffer;
+
+err_delete:
+	drm_client_buffer_delete(buffer);
+err_free:
+	kfree(buffer);
+
+	return ERR_PTR(ret);
+}
+
+static void drm_client_buffer_rmfb(struct drm_client_buffer *buffer)
+{
+	int ret;
+
+	if (!buffer->fb)
+		return;
+
+	ret = drm_mode_rmfb(buffer->client->dev, buffer->fb->base.id, buffer->client->file);
+	if (ret)
+		DRM_DEV_ERROR(buffer->client->dev->dev,
+			      "Error removing FB:%u (%d)\n", buffer->fb->base.id, ret);
+
+	buffer->fb = NULL;
+}
+
+static int drm_client_buffer_addfb(struct drm_client_buffer *buffer,
+				   u32 width, u32 height, u32 format)
+{
+	struct drm_client_dev *client = buffer->client;
+	struct drm_mode_fb_cmd fb_req = { };
+	const struct drm_format_info *info;
+	int ret;
+
+	info = drm_format_info(format);
+	fb_req.bpp = info->cpp[0] * 8;
+	fb_req.depth = info->depth;
+	fb_req.width = width;
+	fb_req.height = height;
+	fb_req.handle = buffer->handle;
+	fb_req.pitch = buffer->pitch;
+
+	ret = drm_mode_addfb(client->dev, &fb_req, client->file);
+	if (ret)
+		return ret;
+
+	buffer->fb = drm_framebuffer_lookup(client->dev, buffer->client->file, fb_req.fb_id);
+	if (WARN_ON(!buffer->fb))
+		return -ENOENT;
+
+	/* drop the reference we picked up in framebuffer lookup */
+	drm_framebuffer_put(buffer->fb);
+
+	strscpy(buffer->fb->comm, client->name, TASK_COMM_LEN);
+
+	return 0;
+}
+
+/**
+ * drm_client_framebuffer_create - Create a client framebuffer
+ * @client: DRM client
+ * @width: Framebuffer width
+ * @height: Framebuffer height
+ * @format: Buffer format
+ *
+ * This function creates a &drm_client_buffer which consists of a
+ * &drm_framebuffer backed by a dumb buffer.
+ * Call drm_client_framebuffer_delete() to free the buffer.
+ *
+ * Returns:
+ * Pointer to a client buffer or an error pointer on failure.
+ */
+struct drm_client_buffer *
+drm_client_framebuffer_create(struct drm_client_dev *client, u32 width, u32 height, u32 format)
+{
+	struct drm_client_buffer *buffer;
+	int ret;
+
+	buffer = drm_client_buffer_create(client, width, height, format);
+	if (IS_ERR(buffer))
+		return buffer;
+
+	ret = drm_client_buffer_addfb(buffer, width, height, format);
+	if (ret) {
+		drm_client_buffer_delete(buffer);
+		return ERR_PTR(ret);
+	}
+
+	return buffer;
+}
+EXPORT_SYMBOL(drm_client_framebuffer_create);
+
+/**
+ * drm_client_framebuffer_delete - Delete a client framebuffer
+ * @buffer: DRM client buffer (can be NULL)
+ */
+void drm_client_framebuffer_delete(struct drm_client_buffer *buffer)
+{
+	if (!buffer)
+		return;
+
+	drm_client_buffer_rmfb(buffer);
+	drm_client_buffer_delete(buffer);
+}
+EXPORT_SYMBOL(drm_client_framebuffer_delete);
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index f6910ebe4d0e..67ac793a7108 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -505,6 +505,7 @@ int drm_dev_init(struct drm_device *dev,
 	dev->driver = driver;
 
 	INIT_LIST_HEAD(&dev->filelist);
+	INIT_LIST_HEAD(&dev->filelist_internal);
 	INIT_LIST_HEAD(&dev->ctxlist);
 	INIT_LIST_HEAD(&dev->vmalist);
 	INIT_LIST_HEAD(&dev->maplist);
diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
new file mode 100644
index 000000000000..12ac2615b17d
--- /dev/null
+++ b/include/drm/drm_client.h
@@ -0,0 +1,87 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _DRM_CLIENT_H_
+#define _DRM_CLIENT_H_
+
+#include <linux/kref.h>
+#include <linux/types.h>
+
+struct drm_device;
+struct drm_framebuffer;
+struct drm_gem_object;
+struct drm_minor;
+
+/**
+ * struct drm_client_dev - DRM client instance
+ */
+struct drm_client_dev {
+	/**
+	 * @dev: DRM device
+	 */
+	struct drm_device *dev;
+
+	/**
+	 * @name: Name of the client.
+	 */
+	const char *name;
+
+	/**
+	 * @ref:
+	 *
+	 * Reference counting. The client resources are released when this drops
+	 * to zero.
+	 */
+	struct kref ref;
+
+	/**
+	 * @file: DRM file
+	 */
+	struct drm_file *file;
+};
+
+void drm_client_close(struct drm_client_dev *client);
+int drm_client_new(struct drm_device *dev, struct drm_client_dev *client,
+		   const char *name);
+void drm_client_get(struct drm_client_dev *client);
+void drm_client_put(struct drm_client_dev *client);
+
+/**
+ * struct drm_client_buffer - DRM client buffer
+ */
+struct drm_client_buffer {
+	/**
+	 * @client: DRM client
+	 */
+	struct drm_client_dev *client;
+
+	/**
+	 * @handle: Buffer handle
+	 */
+	u32 handle;
+
+	/**
+	 * @pitch: Buffer pitch
+	 */
+	u32 pitch;
+
+	/**
+	 * @gem: GEM object backing this buffer
+	 */
+	struct drm_gem_object *gem;
+
+	/**
+	 * @vaddr: Virtual address for the buffer
+	 */
+	void *vaddr;
+
+	/**
+	 * @fb: DRM framebuffer
+	 */
+	struct drm_framebuffer *fb;
+};
+
+struct drm_client_buffer *
+drm_client_framebuffer_create(struct drm_client_dev *client, u32 width, u32 height, u32 format);
+void drm_client_framebuffer_delete(struct drm_client_buffer *buffer);
+
+#endif
diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
index 858ba19a3e29..9e29976d4e98 100644
--- a/include/drm/drm_device.h
+++ b/include/drm/drm_device.h
@@ -74,6 +74,13 @@ struct drm_device {
 	struct mutex filelist_mutex;
 	struct list_head filelist;
 
+	/**
+	 * @filelist_internal:
+	 *
+	 * List of open DRM files for in-kernel clients. Protected by @filelist_mutex.
+	 */
+	struct list_head filelist_internal;
+
 	/** \name Memory management */
 	/*@{ */
 	struct list_head maplist;	/**< Linked list of regions */
-- 
2.15.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 05/12] drm/fb-helper: Add generic fbdev emulation .fb_probe function

[Noralf Trønnes]

This is the first step in getting generic fbdev emulation.
A drm_fb_helper_funcs.fb_probe function is added which uses the
DRM client API to get a framebuffer backed by a dumb buffer.

A transitional hack for tinydrm is needed in order to switch over all
CMA helper drivers in a later patch. This hack will be removed when
tinydrm moves to vmalloc buffers.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---

Changes since version 1:
- Embed drm_client at the beginning of drm_fb_helper to avoid a fragile
  transitional kfree hack in drm_client_release()
- Set owner in drm_fbdev_fb_ops
- Add kerneldoc to drm_fb_helper_generic_probe()


 drivers/gpu/drm/drm_fb_helper.c | 209 +++++++++++++++++++++++++++++++++++++++-
 include/drm/drm_fb_helper.h     |  31 ++++++
 2 files changed, 239 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 2ee1eaa66188..be6bab889f1b 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -30,6 +30,7 @@
 #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>
@@ -745,6 +746,24 @@ static void drm_fb_helper_resume_worker(struct work_struct *work)
 	console_unlock();
 }
 
+static void drm_fb_helper_dirty_blit_real(struct drm_fb_helper *fb_helper,
+					  struct drm_clip_rect *clip)
+{
+	struct drm_framebuffer *fb = fb_helper->fb;
+	unsigned int cpp = drm_format_plane_cpp(fb->format->format, 0);
+	size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp;
+	void *src = fb_helper->fbdev->screen_buffer + offset;
+	void *dst = fb_helper->buffer->vaddr + offset;
+	size_t len = (clip->x2 - clip->x1) * cpp;
+	unsigned int y;
+
+	for (y = clip->y1; y < clip->y2; y++) {
+		memcpy(dst, src, len);
+		src += fb->pitches[0];
+		dst += fb->pitches[0];
+	}
+}
+
 static void drm_fb_helper_dirty_work(struct work_struct *work)
 {
 	struct drm_fb_helper *helper = container_of(work, struct drm_fb_helper,
@@ -760,8 +779,11 @@ static void drm_fb_helper_dirty_work(struct work_struct *work)
 	spin_unlock_irqrestore(&helper->dirty_lock, flags);
 
 	/* call dirty callback only when it has been really touched */
-	if (clip_copy.x1 < clip_copy.x2 && clip_copy.y1 < clip_copy.y2)
+	if (clip_copy.x1 < clip_copy.x2 && clip_copy.y1 < clip_copy.y2) {
+		if (helper->buffer) /* Generic fbdev uses a shadow buffer */
+			drm_fb_helper_dirty_blit_real(helper, &clip_copy);
 		helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip_copy, 1);
+	}
 }
 
 /**
@@ -2928,6 +2950,191 @@ void drm_fb_helper_output_poll_changed(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_fb_helper_output_poll_changed);
 
+/* @user: 1=userspace, 0=fbcon */
+static int drm_fbdev_fb_open(struct fb_info *info, int user)
+{
+	struct drm_fb_helper *fb_helper = info->par;
+
+	if (!try_module_get(fb_helper->dev->driver->fops->owner))
+		return -ENODEV;
+
+	return 0;
+}
+
+static int drm_fbdev_fb_release(struct fb_info *info, int user)
+{
+	struct drm_fb_helper *fb_helper = info->par;
+
+	module_put(fb_helper->dev->driver->fops->owner);
+
+	return 0;
+}
+
+/*
+ * FIXME:
+ * Merge this into drm_fbdev_generic_client_release() when all CMA drivers have
+ * been moved over to using drm_fbdev_generic_setup().
+ */
+static void drm_fbdev_generic_client_release_do(struct drm_client_dev *client)
+{
+	struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
+	struct fb_info *fbi = fb_helper->fbdev;
+	bool defio = fbi->fbdefio;
+	struct fb_ops *fbops;
+	void *shadow;
+
+	if (defio) {
+		fb_deferred_io_cleanup(fbi);
+		fbops = fbi->fbops;
+		shadow = fbi->screen_buffer;
+	}
+
+	drm_fb_helper_fini(fb_helper);
+
+	if (defio) {
+		kfree(fbops);
+		vfree(shadow);
+	}
+
+	drm_client_framebuffer_delete(fb_helper->buffer);
+}
+
+/*
+ * fb_ops.fb_destroy is called by the last put_fb_info() call at the end of
+ * unregister_framebuffer() or fb_release().
+ */
+static void drm_fbdev_fb_destroy(struct fb_info *info)
+{
+	struct drm_fb_helper *fb_helper = info->par;
+
+	/*
+	 * FIXME:
+	 * Remove this when all CMA drivers have been moved over to using
+	 * drm_fbdev_generic_setup().
+	 */
+	drm_fbdev_generic_client_release_do(&fb_helper->client);
+
+	drm_client_put(&fb_helper->client);
+}
+
+static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
+{
+	struct drm_fb_helper *fb_helper = info->par;
+
+	if (fb_helper->dev->driver->gem_prime_mmap)
+		return fb_helper->dev->driver->gem_prime_mmap(fb_helper->buffer->gem, vma);
+	else
+		return -ENODEV;
+}
+
+static struct fb_ops drm_fbdev_fb_ops = {
+	.owner		= THIS_MODULE,
+	DRM_FB_HELPER_DEFAULT_OPS,
+	.fb_open	= drm_fbdev_fb_open,
+	.fb_release	= drm_fbdev_fb_release,
+	.fb_destroy	= drm_fbdev_fb_destroy,
+	.fb_mmap	= drm_fbdev_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 struct fb_deferred_io drm_fbdev_defio = {
+	.delay		= HZ / 20,
+	.deferred_io	= drm_fb_helper_deferred_io,
+};
+
+/**
+ * drm_fb_helper_generic_probe - Generic fbdev emulation probe helper
+ * @fb_helper: fbdev helper structure
+ * @sizes: describes fbdev size and scanout surface size
+ *
+ * This function uses the client API to crate a framebuffer backed by a dumb buffer.
+ *
+ * The _sys_ versions are used for &fb_ops.fb_read, fb_write, fb_fillrect,
+ * fb_copyarea, fb_imageblit.
+ *
+ * Returns:
+ * Zero on success or negative error code on failure.
+ */
+int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,
+				struct drm_fb_helper_surface_size *sizes)
+{
+	struct drm_client_dev *client = &fb_helper->client;
+	struct drm_client_buffer *buffer;
+	struct drm_framebuffer *fb;
+	struct fb_info *fbi;
+	u32 format;
+	int ret;
+
+	DRM_DEBUG_KMS("surface width(%d), height(%d) and bpp(%d)\n",
+		      sizes->surface_width, sizes->surface_height,
+		      sizes->surface_bpp);
+
+	format = drm_mode_legacy_fb_format(sizes->surface_bpp, sizes->surface_depth);
+	buffer = drm_client_framebuffer_create(client, sizes->surface_width,
+					       sizes->surface_height, format);
+	if (IS_ERR(buffer))
+		return PTR_ERR(buffer);
+
+	fb_helper->buffer = buffer;
+	fb_helper->fb = buffer->fb;
+	fb = buffer->fb;
+
+	fbi = drm_fb_helper_alloc_fbi(fb_helper);
+	if (IS_ERR(fbi)) {
+		ret = PTR_ERR(fbi);
+		goto err_free_buffer;
+	}
+
+	fbi->par = fb_helper;
+	fbi->fbops = &drm_fbdev_fb_ops;
+	fbi->screen_size = fb->height * fb->pitches[0];
+	fbi->fix.smem_len = fbi->screen_size;
+	fbi->screen_buffer = buffer->vaddr;
+	strcpy(fbi->fix.id, "DRM emulated");
+
+	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);
+
+	if (fb->funcs->dirty) {
+		struct fb_ops *fbops;
+		void *shadow;
+
+		/*
+		 * fb_deferred_io_cleanup() clears &fbops->fb_mmap so a per
+		 * instance version is necessary.
+		 */
+		fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
+		shadow = vzalloc(fbi->screen_size);
+		if (!fbops || !shadow) {
+			kfree(fbops);
+			vfree(shadow);
+			ret = -ENOMEM;
+			goto err_fb_info_destroy;
+		}
+
+		*fbops = *fbi->fbops;
+		fbi->fbops = fbops;
+		fbi->screen_buffer = shadow;
+		fbi->fbdefio = &drm_fbdev_defio;
+
+		fb_deferred_io_init(fbi);
+	}
+
+	return 0;
+
+err_fb_info_destroy:
+	drm_fb_helper_fini(fb_helper);
+err_free_buffer:
+	drm_client_framebuffer_delete(buffer);
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_fb_helper_generic_probe);
+
 /* 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 b069433e7fc1..c134bbcfd2e9 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -32,6 +32,7 @@
 
 struct drm_fb_helper;
 
+#include <drm/drm_client.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_device.h>
 #include <linux/kgdb.h>
@@ -154,6 +155,20 @@ struct drm_fb_helper_connector {
  * operations.
  */
 struct drm_fb_helper {
+	/**
+	 * @client:
+	 *
+	 * DRM client used by the generic fbdev emulation.
+	 */
+	struct drm_client_dev client;
+
+	/**
+	 * @buffer:
+	 *
+	 * Framebuffer used by the generic fbdev emulation.
+	 */
+	struct drm_client_buffer *buffer;
+
 	struct drm_framebuffer *fb;
 	struct drm_device *dev;
 	int crtc_count;
@@ -234,6 +249,12 @@ struct drm_fb_helper {
 	int preferred_bpp;
 };
 
+static inline struct drm_fb_helper *
+drm_fb_helper_from_client(struct drm_client_dev *client)
+{
+	return container_of(client, struct drm_fb_helper, client);
+}
+
 /**
  * define DRM_FB_HELPER_DEFAULT_OPS - helper define for drm drivers
  *
@@ -330,6 +351,9 @@ 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_probe(struct drm_fb_helper *fb_helper,
+				struct drm_fb_helper_surface_size *sizes);
 #else
 static inline void drm_fb_helper_prepare(struct drm_device *dev,
 					struct drm_fb_helper *helper,
@@ -564,6 +588,13 @@ static inline void drm_fb_helper_output_poll_changed(struct drm_device *dev)
 {
 }
 
+static inline int
+drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,
+			    struct drm_fb_helper_surface_size *sizes)
+{
+	return 0;
+}
+
 #endif
 
 static inline int
-- 
2.15.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 06/12] drm/pl111: Set .gem_prime_vmap and .gem_prime_mmap

[Noralf Trønnes]

These are needed for pl111 to use the generic fbdev emulation.

Cc: Eric Anholt <eric@anholt.net>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Reviewed-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/pl111/pl111_drv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/pl111/pl111_drv.c b/drivers/gpu/drm/pl111/pl111_drv.c
index 454ff0804642..78854b52676c 100644
--- a/drivers/gpu/drm/pl111/pl111_drv.c
+++ b/drivers/gpu/drm/pl111/pl111_drv.c
@@ -249,6 +249,8 @@ static struct drm_driver pl111_drm_driver = {
 	.gem_prime_import_sg_table = pl111_gem_import_sg_table,
 	.gem_prime_export = drm_gem_prime_export,
 	.gem_prime_get_sg_table	= drm_gem_cma_prime_get_sg_table,
+	.gem_prime_mmap = drm_gem_cma_prime_mmap,
+	.gem_prime_vmap = drm_gem_cma_prime_vmap,
 
 #if defined(CONFIG_DEBUG_FS)
 	.debugfs_init = pl111_debugfs_init,
-- 
2.15.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 07/12] drm/cma-helper: Use the generic fbdev emulation

[Noralf Trønnes]

This switches the CMA helper drivers that use its fbdev emulation over
to the generic fbdev emulation. It's the first phase of using generic
fbdev. A later phase will use DRM client callbacks for the
lastclose/hotplug/remove callbacks.

There are currently 2 fbdev init/fini functions:
- drm_fb_cma_fbdev_init/drm_fb_cma_fbdev_fini
- drm_fbdev_cma_init/drm_fbdev_cma_fini

This is because the work on generic fbdev came up during a fbdev
refactoring and thus wasn't completed. No point in completing that
refactoring when drivers will soon move to drm_fb_helper_generic_probe().

tinydrm uses drm_fb_cma_fbdev_init_with_funcs().

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_fb_cma_helper.c | 361 +++++-------------------------------
 include/drm/drm_fb_cma_helper.h     |   3 -
 2 files changed, 43 insertions(+), 321 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index 186d00adfb5f..dbd1933160d1 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -18,6 +18,7 @@
  */
 
 #include <drm/drmP.h>
+#include <drm/drm_client.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_gem_cma_helper.h>
@@ -26,11 +27,8 @@
 #include <drm/drm_print.h>
 #include <linux/module.h>
 
-#define DEFAULT_FBDEFIO_DELAY_MS 50
-
 struct drm_fbdev_cma {
 	struct drm_fb_helper	fb_helper;
-	const struct drm_framebuffer_funcs *fb_funcs;
 };
 
 /**
@@ -44,36 +42,6 @@ struct drm_fbdev_cma {
  *
  * An fbdev framebuffer backed by cma is also available by calling
  * drm_fb_cma_fbdev_init(). drm_fb_cma_fbdev_fini() tears it down.
- * If the &drm_framebuffer_funcs.dirty callback is set, fb_deferred_io will be
- * set up automatically. &drm_framebuffer_funcs.dirty is called by
- * drm_fb_helper_deferred_io() in process context (&struct delayed_work).
- *
- * Example fbdev deferred io code::
- *
- *     static int driver_fb_dirty(struct drm_framebuffer *fb,
- *                                struct drm_file *file_priv,
- *                                unsigned flags, unsigned color,
- *                                struct drm_clip_rect *clips,
- *                                unsigned num_clips)
- *     {
- *         struct drm_gem_cma_object *cma = drm_fb_cma_get_gem_obj(fb, 0);
- *         ... push changes ...
- *         return 0;
- *     }
- *
- *     static struct drm_framebuffer_funcs driver_fb_funcs = {
- *         .destroy       = drm_gem_fb_destroy,
- *         .create_handle = drm_gem_fb_create_handle,
- *         .dirty         = driver_fb_dirty,
- *     };
- *
- * Initialize::
- *
- *     fbdev = drm_fb_cma_fbdev_init_with_funcs(dev, 16,
- *                                           dev->mode_config.num_crtc,
- *                                           dev->mode_config.num_connector,
- *                                           &driver_fb_funcs);
- *
  */
 
 static inline struct drm_fbdev_cma *to_fbdev_cma(struct drm_fb_helper *helper)
@@ -131,153 +99,6 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
 }
 EXPORT_SYMBOL_GPL(drm_fb_cma_get_gem_addr);
 
-static int drm_fb_cma_mmap(struct fb_info *info, struct vm_area_struct *vma)
-{
-	return dma_mmap_writecombine(info->device, vma, info->screen_base,
-				     info->fix.smem_start, info->fix.smem_len);
-}
-
-static struct fb_ops drm_fbdev_cma_ops = {
-	.owner		= THIS_MODULE,
-	DRM_FB_HELPER_DEFAULT_OPS,
-	.fb_fillrect	= drm_fb_helper_sys_fillrect,
-	.fb_copyarea	= drm_fb_helper_sys_copyarea,
-	.fb_imageblit	= drm_fb_helper_sys_imageblit,
-	.fb_mmap	= drm_fb_cma_mmap,
-};
-
-static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info,
-					  struct vm_area_struct *vma)
-{
-	fb_deferred_io_mmap(info, vma);
-	vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
-
-	return 0;
-}
-
-static int drm_fbdev_cma_defio_init(struct fb_info *fbi,
-				    struct drm_gem_cma_object *cma_obj)
-{
-	struct fb_deferred_io *fbdefio;
-	struct fb_ops *fbops;
-
-	/*
-	 * Per device structures are needed because:
-	 * fbops: fb_deferred_io_cleanup() clears fbops.fb_mmap
-	 * fbdefio: individual delays
-	 */
-	fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
-	fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
-	if (!fbdefio || !fbops) {
-		kfree(fbdefio);
-		kfree(fbops);
-		return -ENOMEM;
-	}
-
-	/* can't be offset from vaddr since dirty() uses cma_obj */
-	fbi->screen_buffer = cma_obj->vaddr;
-	/* fb_deferred_io_fault() needs a physical address */
-	fbi->fix.smem_start = page_to_phys(virt_to_page(fbi->screen_buffer));
-
-	*fbops = *fbi->fbops;
-	fbi->fbops = fbops;
-
-	fbdefio->delay = msecs_to_jiffies(DEFAULT_FBDEFIO_DELAY_MS);
-	fbdefio->deferred_io = drm_fb_helper_deferred_io;
-	fbi->fbdefio = fbdefio;
-	fb_deferred_io_init(fbi);
-	fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap;
-
-	return 0;
-}
-
-static void drm_fbdev_cma_defio_fini(struct fb_info *fbi)
-{
-	if (!fbi->fbdefio)
-		return;
-
-	fb_deferred_io_cleanup(fbi);
-	kfree(fbi->fbdefio);
-	kfree(fbi->fbops);
-}
-
-static int
-drm_fbdev_cma_create(struct drm_fb_helper *helper,
-	struct drm_fb_helper_surface_size *sizes)
-{
-	struct drm_fbdev_cma *fbdev_cma = to_fbdev_cma(helper);
-	struct drm_device *dev = helper->dev;
-	struct drm_gem_cma_object *obj;
-	struct drm_framebuffer *fb;
-	unsigned int bytes_per_pixel;
-	unsigned long offset;
-	struct fb_info *fbi;
-	size_t size;
-	int ret;
-
-	DRM_DEBUG_KMS("surface width(%d), height(%d) and bpp(%d)\n",
-			sizes->surface_width, sizes->surface_height,
-			sizes->surface_bpp);
-
-	bytes_per_pixel = DIV_ROUND_UP(sizes->surface_bpp, 8);
-	size = sizes->surface_width * sizes->surface_height * bytes_per_pixel;
-	obj = drm_gem_cma_create(dev, size);
-	if (IS_ERR(obj))
-		return -ENOMEM;
-
-	fbi = drm_fb_helper_alloc_fbi(helper);
-	if (IS_ERR(fbi)) {
-		ret = PTR_ERR(fbi);
-		goto err_gem_free_object;
-	}
-
-	fb = drm_gem_fbdev_fb_create(dev, sizes, 0, &obj->base,
-				     fbdev_cma->fb_funcs);
-	if (IS_ERR(fb)) {
-		dev_err(dev->dev, "Failed to allocate DRM framebuffer.\n");
-		ret = PTR_ERR(fb);
-		goto err_fb_info_destroy;
-	}
-
-	helper->fb = fb;
-
-	fbi->par = helper;
-	fbi->flags = FBINFO_FLAG_DEFAULT;
-	fbi->fbops = &drm_fbdev_cma_ops;
-
-	drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->format->depth);
-	drm_fb_helper_fill_var(fbi, helper, sizes->fb_width, sizes->fb_height);
-
-	offset = fbi->var.xoffset * bytes_per_pixel;
-	offset += fbi->var.yoffset * fb->pitches[0];
-
-	dev->mode_config.fb_base = (resource_size_t)obj->paddr;
-	fbi->screen_base = obj->vaddr + offset;
-	fbi->fix.smem_start = (unsigned long)(obj->paddr + offset);
-	fbi->screen_size = size;
-	fbi->fix.smem_len = size;
-
-	if (fb->funcs->dirty) {
-		ret = drm_fbdev_cma_defio_init(fbi, obj);
-		if (ret)
-			goto err_cma_destroy;
-	}
-
-	return 0;
-
-err_cma_destroy:
-	drm_framebuffer_remove(fb);
-err_fb_info_destroy:
-	drm_fb_helper_fini(helper);
-err_gem_free_object:
-	drm_gem_object_put_unlocked(&obj->base);
-	return ret;
-}
-
-static const struct drm_fb_helper_funcs drm_fb_cma_helper_funcs = {
-	.fb_probe = drm_fbdev_cma_create,
-};
-
 /**
  * drm_fb_cma_fbdev_init_with_funcs() - Allocate and initialize fbdev emulation
  * @dev: DRM device
@@ -295,53 +116,7 @@ int drm_fb_cma_fbdev_init_with_funcs(struct drm_device *dev,
 	unsigned int preferred_bpp, unsigned int max_conn_count,
 	const struct drm_framebuffer_funcs *funcs)
 {
-	struct drm_fbdev_cma *fbdev_cma;
-	struct drm_fb_helper *fb_helper;
-	int ret;
-
-	if (!preferred_bpp)
-		preferred_bpp = dev->mode_config.preferred_depth;
-	if (!preferred_bpp)
-		preferred_bpp = 32;
-
-	if (!max_conn_count)
-		max_conn_count = dev->mode_config.num_connector;
-
-	fbdev_cma = kzalloc(sizeof(*fbdev_cma), GFP_KERNEL);
-	if (!fbdev_cma)
-		return -ENOMEM;
-
-	fbdev_cma->fb_funcs = funcs;
-	fb_helper = &fbdev_cma->fb_helper;
-
-	drm_fb_helper_prepare(dev, fb_helper, &drm_fb_cma_helper_funcs);
-
-	ret = drm_fb_helper_init(dev, fb_helper, max_conn_count);
-	if (ret < 0) {
-		DRM_DEV_ERROR(dev->dev, "Failed to initialize fbdev helper.\n");
-		goto err_free;
-	}
-
-	ret = drm_fb_helper_single_add_all_connectors(fb_helper);
-	if (ret < 0) {
-		DRM_DEV_ERROR(dev->dev, "Failed to add connectors.\n");
-		goto err_drm_fb_helper_fini;
-	}
-
-	ret = drm_fb_helper_initial_config(fb_helper, preferred_bpp);
-	if (ret < 0) {
-		DRM_DEV_ERROR(dev->dev, "Failed to set fbdev configuration.\n");
-		goto err_drm_fb_helper_fini;
-	}
-
-	return 0;
-
-err_drm_fb_helper_fini:
-	drm_fb_helper_fini(fb_helper);
-err_free:
-	kfree(fbdev_cma);
-
-	return ret;
+	return drm_fb_cma_fbdev_init(dev, preferred_bpp, max_conn_count);
 }
 EXPORT_SYMBOL_GPL(drm_fb_cma_fbdev_init_with_funcs);
 
@@ -359,8 +134,14 @@ EXPORT_SYMBOL_GPL(drm_fb_cma_fbdev_init_with_funcs);
 int drm_fb_cma_fbdev_init(struct drm_device *dev, unsigned int preferred_bpp,
 			  unsigned int max_conn_count)
 {
-	return drm_fb_cma_fbdev_init_with_funcs(dev, preferred_bpp,
-						max_conn_count, NULL);
+	struct drm_fbdev_cma *fbdev_cma;
+
+	/* dev->fb_helper will indirectly point to fbdev_cma after this call */
+	fbdev_cma = drm_fbdev_cma_init(dev, preferred_bpp, max_conn_count);
+	if (IS_ERR(fbdev_cma))
+		return PTR_ERR(fbdev_cma);
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(drm_fb_cma_fbdev_init);
 
@@ -370,87 +151,13 @@ EXPORT_SYMBOL_GPL(drm_fb_cma_fbdev_init);
  */
 void drm_fb_cma_fbdev_fini(struct drm_device *dev)
 {
-	struct drm_fb_helper *fb_helper = dev->fb_helper;
-
-	if (!fb_helper)
-		return;
-
-	/* Unregister if it hasn't been done already */
-	if (fb_helper->fbdev && fb_helper->fbdev->dev)
-		drm_fb_helper_unregister_fbi(fb_helper);
-
-	if (fb_helper->fbdev)
-		drm_fbdev_cma_defio_fini(fb_helper->fbdev);
-
-	if (fb_helper->fb)
-		drm_framebuffer_remove(fb_helper->fb);
-
-	drm_fb_helper_fini(fb_helper);
-	kfree(to_fbdev_cma(fb_helper));
+	if (dev->fb_helper)
+		drm_fbdev_cma_fini(to_fbdev_cma(dev->fb_helper));
 }
 EXPORT_SYMBOL_GPL(drm_fb_cma_fbdev_fini);
 
-/**
- * drm_fbdev_cma_init_with_funcs() - Allocate and initializes a drm_fbdev_cma struct
- * @dev: DRM device
- * @preferred_bpp: Preferred bits per pixel for the device
- * @max_conn_count: Maximum number of connectors
- * @funcs: fb helper functions, in particular a custom dirty() callback
- *
- * Returns a newly allocated drm_fbdev_cma struct or a ERR_PTR.
- */
-struct drm_fbdev_cma *drm_fbdev_cma_init_with_funcs(struct drm_device *dev,
-	unsigned int preferred_bpp, unsigned int max_conn_count,
-	const struct drm_framebuffer_funcs *funcs)
-{
-	struct drm_fbdev_cma *fbdev_cma;
-	struct drm_fb_helper *helper;
-	int ret;
-
-	fbdev_cma = kzalloc(sizeof(*fbdev_cma), GFP_KERNEL);
-	if (!fbdev_cma) {
-		dev_err(dev->dev, "Failed to allocate drm fbdev.\n");
-		return ERR_PTR(-ENOMEM);
-	}
-	fbdev_cma->fb_funcs = funcs;
-
-	helper = &fbdev_cma->fb_helper;
-
-	drm_fb_helper_prepare(dev, helper, &drm_fb_cma_helper_funcs);
-
-	ret = drm_fb_helper_init(dev, helper, max_conn_count);
-	if (ret < 0) {
-		dev_err(dev->dev, "Failed to initialize drm fb helper.\n");
-		goto err_free;
-	}
-
-	ret = drm_fb_helper_single_add_all_connectors(helper);
-	if (ret < 0) {
-		dev_err(dev->dev, "Failed to add connectors.\n");
-		goto err_drm_fb_helper_fini;
-
-	}
-
-	ret = drm_fb_helper_initial_config(helper, preferred_bpp);
-	if (ret < 0) {
-		dev_err(dev->dev, "Failed to set initial hw configuration.\n");
-		goto err_drm_fb_helper_fini;
-	}
-
-	return fbdev_cma;
-
-err_drm_fb_helper_fini:
-	drm_fb_helper_fini(helper);
-err_free:
-	kfree(fbdev_cma);
-
-	return ERR_PTR(ret);
-}
-EXPORT_SYMBOL_GPL(drm_fbdev_cma_init_with_funcs);
-
-static const struct drm_framebuffer_funcs drm_fb_cma_funcs = {
-	.destroy	= drm_gem_fb_destroy,
-	.create_handle	= drm_gem_fb_create_handle,
+static const struct drm_fb_helper_funcs drm_fb_cma_helper_funcs = {
+	.fb_probe = drm_fb_helper_generic_probe,
 };
 
 /**
@@ -464,9 +171,34 @@ static const struct drm_framebuffer_funcs drm_fb_cma_funcs = {
 struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev,
 	unsigned int preferred_bpp, unsigned int max_conn_count)
 {
-	return drm_fbdev_cma_init_with_funcs(dev, preferred_bpp,
-					     max_conn_count,
-					     &drm_fb_cma_funcs);
+	struct drm_fbdev_cma *fbdev_cma;
+	struct drm_fb_helper *fb_helper;
+	struct drm_client_dev *client;
+	int ret;
+
+	fbdev_cma = kzalloc(sizeof(*fbdev_cma), GFP_KERNEL);
+	if (!fbdev_cma)
+		return ERR_PTR(-ENOMEM);
+
+	fb_helper = &fbdev_cma->fb_helper;
+
+	ret = drm_client_new(dev, &fb_helper->client, "fbdev");
+	if (ret)
+		goto err_free;
+
+	ret = drm_fb_helper_fbdev_setup(dev, fb_helper, &drm_fb_cma_helper_funcs,
+					preferred_bpp, max_conn_count);
+	if (ret)
+		goto err_client_put;
+
+	return fbdev_cma;
+
+err_client_put:
+	drm_client_put(client);
+err_free:
+	kfree(fbdev_cma);
+
+	return ERR_PTR(ret);
 }
 EXPORT_SYMBOL_GPL(drm_fbdev_cma_init);
 
@@ -477,14 +209,7 @@ EXPORT_SYMBOL_GPL(drm_fbdev_cma_init);
 void drm_fbdev_cma_fini(struct drm_fbdev_cma *fbdev_cma)
 {
 	drm_fb_helper_unregister_fbi(&fbdev_cma->fb_helper);
-	if (fbdev_cma->fb_helper.fbdev)
-		drm_fbdev_cma_defio_fini(fbdev_cma->fb_helper.fbdev);
-
-	if (fbdev_cma->fb_helper.fb)
-		drm_framebuffer_remove(fbdev_cma->fb_helper.fb);
-
-	drm_fb_helper_fini(&fbdev_cma->fb_helper);
-	kfree(fbdev_cma);
+	/* All resources have now been freed by drm_fbdev_fb_destroy() */
 }
 EXPORT_SYMBOL_GPL(drm_fbdev_cma_fini);
 
diff --git a/include/drm/drm_fb_cma_helper.h b/include/drm/drm_fb_cma_helper.h
index d532f88a8d55..a0546c3451f9 100644
--- a/include/drm/drm_fb_cma_helper.h
+++ b/include/drm/drm_fb_cma_helper.h
@@ -23,9 +23,6 @@ int drm_fb_cma_fbdev_init(struct drm_device *dev, unsigned int preferred_bpp,
 			  unsigned int max_conn_count);
 void drm_fb_cma_fbdev_fini(struct drm_device *dev);
 
-struct drm_fbdev_cma *drm_fbdev_cma_init_with_funcs(struct drm_device *dev,
-	unsigned int preferred_bpp, unsigned int max_conn_count,
-	const struct drm_framebuffer_funcs *funcs);
 struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev,
 	unsigned int preferred_bpp, unsigned int max_conn_count);
 void drm_fbdev_cma_fini(struct drm_fbdev_cma *fbdev_cma);
-- 
2.15.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 08/12] drm/client: Add client callbacks

[Noralf Trønnes]

Add client callbacks and hook them up.
Add a list of clients per drm_device.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---

Changes since version 1:
- Remove unused functions
- Change name drm_client_funcs.lastclose -> .restore
- Change name drm_client_funcs.remove -> .unregister
- Rework unregister code


 drivers/gpu/drm/drm_client.c        | 97 +++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/drm_drv.c           |  7 +++
 drivers/gpu/drm/drm_fb_cma_helper.c |  2 +-
 drivers/gpu/drm/drm_file.c          |  3 ++
 drivers/gpu/drm/drm_probe_helper.c  |  3 ++
 include/drm/drm_client.h            | 69 +++++++++++++++++++++++++-
 include/drm/drm_device.h            | 14 ++++++
 7 files changed, 190 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
index 98dda40f5416..f1dc04d641cc 100644
--- a/drivers/gpu/drm/drm_client.c
+++ b/drivers/gpu/drm/drm_client.c
@@ -74,6 +74,7 @@ EXPORT_SYMBOL(drm_client_close);
  * @dev: DRM device
  * @client: DRM client
  * @name: Client name
+ * @funcs: DRM client functions (optional)
  *
  * Use drm_client_put() to free the client.
  *
@@ -81,8 +82,9 @@ EXPORT_SYMBOL(drm_client_close);
  * Zero on success or negative error code on failure.
  */
 int drm_client_new(struct drm_device *dev, struct drm_client_dev *client,
-		   const char *name)
+		   const char *name, const struct drm_client_funcs *funcs)
 {
+	bool registered;
 	int ret;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET) ||
@@ -91,12 +93,23 @@ int drm_client_new(struct drm_device *dev, struct drm_client_dev *client,
 
 	client->dev = dev;
 	client->name = name;
+	client->funcs = funcs;
 	kref_init(&client->ref);
 
 	ret = drm_client_open(client);
 	if (ret)
 		return ret;
 
+	mutex_lock(&dev->clientlist_mutex);
+	registered = dev->registered;
+	if (registered)
+		list_add(&client->list, &dev->clientlist);
+	mutex_unlock(&dev->clientlist_mutex);
+	if (!registered) {
+		drm_client_close(client);
+		return -ENODEV;
+	}
+
 	drm_dev_get(dev);
 
 	return 0;
@@ -110,8 +123,17 @@ static void drm_client_release(struct kref *ref)
 
 	DRM_DEV_DEBUG_KMS(dev->dev, "%s\n", client->name);
 
-	drm_client_close(client);
-	kfree(client);
+	mutex_lock(&dev->clientlist_mutex);
+	if (!list_empty(&client->list))
+		list_del(&client->list);
+	mutex_unlock(&dev->clientlist_mutex);
+
+	if (client->funcs && client->funcs->release) {
+		client->funcs->release(client);
+	} else {
+		drm_client_close(client);
+		kfree(client);
+	}
 
 	drm_dev_put(dev);
 }
@@ -145,6 +167,75 @@ void drm_client_put(struct drm_client_dev *client)
 }
 EXPORT_SYMBOL(drm_client_put);
 
+void drm_client_dev_unregister(struct drm_device *dev)
+{
+	struct drm_client_dev *client, *iter;
+
+	if (!drm_core_check_feature(dev, DRIVER_MODESET))
+		return;
+
+	do {
+		client = NULL;
+		mutex_lock(&dev->clientlist_mutex);
+		list_for_each_entry(iter, &dev->clientlist, list) {
+			list_del_init(&iter->list);
+			if (iter->funcs && iter->funcs->unregister) {
+				/* Make sure a release has not begun */
+				if (kref_get_unless_zero(&iter->ref)) {
+					client = iter;
+					break;
+				}
+			}
+		}
+		mutex_unlock(&dev->clientlist_mutex);
+		if (client) {
+			client->funcs->unregister(client);
+			drm_client_put(client);
+		}
+	} while (client);
+}
+
+void drm_client_dev_hotplug(struct drm_device *dev)
+{
+	struct drm_client_dev *client;
+	int ret;
+
+	if (!drm_core_check_feature(dev, DRIVER_MODESET))
+		return;
+
+	mutex_lock(&dev->clientlist_mutex);
+	list_for_each_entry(client, &dev->clientlist, list) {
+		if (!client->funcs || !client->funcs->hotplug)
+			continue;
+
+		ret = client->funcs->hotplug(client);
+		DRM_DEV_DEBUG_KMS(dev->dev, "%s: ret=%d\n", client->name, ret);
+	}
+	mutex_unlock(&dev->clientlist_mutex);
+}
+EXPORT_SYMBOL(drm_client_dev_hotplug);
+
+void drm_client_dev_restore(struct drm_device *dev)
+{
+	struct drm_client_dev *client;
+	int ret;
+
+	if (!drm_core_check_feature(dev, DRIVER_MODESET))
+		return;
+
+	mutex_lock(&dev->clientlist_mutex);
+	list_for_each_entry(client, &dev->clientlist, list) {
+		if (!client->funcs || !client->funcs->restore)
+			continue;
+
+		ret = client->funcs->restore(client);
+		DRM_DEV_DEBUG_KMS(dev->dev, "%s: ret=%d\n", client->name, ret);
+		if (!ret) /* The first one to return zero gets the privilege to restore */
+			break;
+	}
+	mutex_unlock(&dev->clientlist_mutex);
+}
+
 static void drm_client_buffer_delete(struct drm_client_buffer *buffer)
 {
 	struct drm_device *dev;
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 67ac793a7108..00172a3da62c 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -34,6 +34,7 @@
 #include <linux/slab.h>
 #include <linux/srcu.h>
 
+#include <drm/drm_client.h>
 #include <drm/drm_drv.h>
 #include <drm/drmP.h>
 
@@ -506,6 +507,7 @@ int drm_dev_init(struct drm_device *dev,
 
 	INIT_LIST_HEAD(&dev->filelist);
 	INIT_LIST_HEAD(&dev->filelist_internal);
+	INIT_LIST_HEAD(&dev->clientlist);
 	INIT_LIST_HEAD(&dev->ctxlist);
 	INIT_LIST_HEAD(&dev->vmalist);
 	INIT_LIST_HEAD(&dev->maplist);
@@ -515,6 +517,7 @@ int drm_dev_init(struct drm_device *dev,
 	spin_lock_init(&dev->event_lock);
 	mutex_init(&dev->struct_mutex);
 	mutex_init(&dev->filelist_mutex);
+	mutex_init(&dev->clientlist_mutex);
 	mutex_init(&dev->ctxlist_mutex);
 	mutex_init(&dev->master_mutex);
 
@@ -570,6 +573,7 @@ int drm_dev_init(struct drm_device *dev,
 err_free:
 	mutex_destroy(&dev->master_mutex);
 	mutex_destroy(&dev->ctxlist_mutex);
+	mutex_destroy(&dev->clientlist_mutex);
 	mutex_destroy(&dev->filelist_mutex);
 	mutex_destroy(&dev->struct_mutex);
 	return ret;
@@ -604,6 +608,7 @@ void drm_dev_fini(struct drm_device *dev)
 
 	mutex_destroy(&dev->master_mutex);
 	mutex_destroy(&dev->ctxlist_mutex);
+	mutex_destroy(&dev->clientlist_mutex);
 	mutex_destroy(&dev->filelist_mutex);
 	mutex_destroy(&dev->struct_mutex);
 	kfree(dev->unique);
@@ -859,6 +864,8 @@ void drm_dev_unregister(struct drm_device *dev)
 
 	dev->registered = false;
 
+	drm_client_dev_unregister(dev);
+
 	if (drm_core_check_feature(dev, DRIVER_MODESET))
 		drm_modeset_unregister_all(dev);
 
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index dbd1933160d1..31831a7a6a99 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -182,7 +182,7 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev,
 
 	fb_helper = &fbdev_cma->fb_helper;
 
-	ret = drm_client_new(dev, &fb_helper->client, "fbdev");
+	ret = drm_client_new(dev, &fb_helper->client, "fbdev", NULL);
 	if (ret)
 		goto err_free;
 
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 55505378df47..06e409007495 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_client.h>
 #include <drm/drm_file.h>
 #include <drm/drmP.h>
 
@@ -443,6 +444,8 @@ void drm_lastclose(struct drm_device * dev)
 
 	if (drm_core_check_feature(dev, DRIVER_LEGACY))
 		drm_legacy_dev_reinit(dev);
+
+	drm_client_dev_restore(dev);
 }
 
 /**
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 527743394150..26be57e28a9d 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -33,6 +33,7 @@
 #include <linux/moduleparam.h>
 
 #include <drm/drmP.h>
+#include <drm/drm_client.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_crtc_helper.h>
@@ -563,6 +564,8 @@ void drm_kms_helper_hotplug_event(struct drm_device *dev)
 	drm_sysfs_hotplug_event(dev);
 	if (dev->mode_config.funcs->output_poll_changed)
 		dev->mode_config.funcs->output_poll_changed(dev);
+
+	drm_client_dev_hotplug(dev);
 }
 EXPORT_SYMBOL(drm_kms_helper_hotplug_event);
 
diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
index 12ac2615b17d..80fe21c86f69 100644
--- a/include/drm/drm_client.h
+++ b/include/drm/drm_client.h
@@ -6,11 +6,61 @@
 #include <linux/kref.h>
 #include <linux/types.h>
 
+struct drm_client_dev;
 struct drm_device;
 struct drm_framebuffer;
 struct drm_gem_object;
 struct drm_minor;
 
+/**
+ * struct drm_client_funcs - DRM client callbacks
+ */
+struct drm_client_funcs {
+	/**
+	 * @release:
+	 *
+	 * Called when the reference count is dropped to zero. Users of this
+	 * callback is responsible for calling drm_client_close() and freeing
+	 * the client structure.
+	 *
+	 * This callback is optional.
+	 */
+	void (*release)(struct drm_client_dev *client);
+
+	/**
+	 * @unregister:
+	 *
+	 * Called when &drm_device is unregistered. The client should respond by
+	 * releasing it's resources using drm_client_put(). If it can't do so
+	 * due to resoruces being tied up, like fbdev with open file handles,
+	 * it should release it's resources as soon as possible.
+	 *
+	 * This callback is optional.
+	 */
+	void (*unregister)(struct drm_client_dev *client);
+
+	/**
+	 * @restore:
+	 *
+	 * Called on drm_lastclose(). The first client instance in the list that
+	 * returns zero gets the privilege to restore and no more clients are
+	 * called. This callback is not called after @unregister has been called.
+	 *
+	 * This callback is optional.
+	 */
+	int (*restore)(struct drm_client_dev *client);
+
+	/**
+	 * @hotplug:
+	 *
+	 * Called on drm_kms_helper_hotplug_event().
+	 * This callback is not called after @unregister has been called.
+	 *
+	 * This callback is optional.
+	 */
+	int (*hotplug)(struct drm_client_dev *client);
+};
+
 /**
  * struct drm_client_dev - DRM client instance
  */
@@ -33,6 +83,19 @@ struct drm_client_dev {
 	 */
 	struct kref ref;
 
+	/**
+	 * @list:
+	 *
+	 * List of all clients of a DRM device, linked into
+	 * &drm_device.clientlist. Protected by &drm_device.clientlist_mutex.
+	 */
+	struct list_head list;
+
+	/**
+	 * @funcs: DRM client functions (optional)
+	 */
+	const struct drm_client_funcs *funcs;
+
 	/**
 	 * @file: DRM file
 	 */
@@ -41,10 +104,14 @@ struct drm_client_dev {
 
 void drm_client_close(struct drm_client_dev *client);
 int drm_client_new(struct drm_device *dev, struct drm_client_dev *client,
-		   const char *name);
+		   const char *name, const struct drm_client_funcs *funcs);
 void drm_client_get(struct drm_client_dev *client);
 void drm_client_put(struct drm_client_dev *client);
 
+void drm_client_dev_unregister(struct drm_device *dev);
+void drm_client_dev_hotplug(struct drm_device *dev);
+void drm_client_dev_restore(struct drm_device *dev);
+
 /**
  * struct drm_client_buffer - DRM client buffer
  */
diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
index 9e29976d4e98..f9c6e0e3aec7 100644
--- a/include/drm/drm_device.h
+++ b/include/drm/drm_device.h
@@ -81,6 +81,20 @@ struct drm_device {
 	 */
 	struct list_head filelist_internal;
 
+	/**
+	 * @clientlist_mutex:
+	 *
+	 * Protects @clientlist access.
+	 */
+	struct mutex clientlist_mutex;
+
+	/**
+	 * @clientlist:
+	 *
+	 * List of in-kernel clients. Protected by @clientlist_mutex.
+	 */
+	struct list_head clientlist;
+
 	/** \name Memory management */
 	/*@{ */
 	struct list_head maplist;	/**< Linked list of regions */
-- 
2.15.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 09/12] drm/debugfs: Add internal client debugfs file

[Noralf Trønnes]

Print the names of the internal clients currently attached.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/drm_client.c  | 28 ++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_debugfs.c |  7 +++++++
 include/drm/drm_client.h      |  2 ++
 3 files changed, 37 insertions(+)

diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
index f1dc04d641cc..3ebb8fa34655 100644
--- a/drivers/gpu/drm/drm_client.c
+++ b/drivers/gpu/drm/drm_client.c
@@ -405,3 +405,31 @@ void drm_client_framebuffer_delete(struct drm_client_buffer *buffer)
 	drm_client_buffer_delete(buffer);
 }
 EXPORT_SYMBOL(drm_client_framebuffer_delete);
+
+#ifdef CONFIG_DEBUG_FS
+static int drm_client_debugfs_internal_clients(struct seq_file *m, void *data)
+{
+	struct drm_info_node *node = m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct drm_printer p = drm_seq_file_printer(m);
+	struct drm_client_dev *client;
+
+	mutex_lock(&dev->clientlist_mutex);
+	list_for_each_entry(client, &dev->clientlist, list)
+		drm_printf(&p, "%s\n", client->name);
+	mutex_unlock(&dev->clientlist_mutex);
+
+	return 0;
+}
+
+static const struct drm_info_list drm_client_debugfs_list[] = {
+	{ "internal_clients", drm_client_debugfs_internal_clients, 0 },
+};
+
+int drm_client_debugfs_init(struct drm_minor *minor)
+{
+	return drm_debugfs_create_files(drm_client_debugfs_list,
+					ARRAY_SIZE(drm_client_debugfs_list),
+					minor->debugfs_root, minor);
+}
+#endif
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index b2482818fee8..50a20bfc07ea 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -28,6 +28,7 @@
 #include <linux/slab.h>
 #include <linux/export.h>
 
+#include <drm/drm_client.h>
 #include <drm/drm_debugfs.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_atomic.h>
@@ -164,6 +165,12 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
 			DRM_ERROR("Failed to create framebuffer debugfs file\n");
 			return ret;
 		}
+
+		ret = drm_client_debugfs_init(minor);
+		if (ret) {
+			DRM_ERROR("Failed to create client debugfs file\n");
+			return ret;
+		}
 	}
 
 	if (dev->driver->debugfs_init) {
diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
index 80fe21c86f69..c3a87d6c30fc 100644
--- a/include/drm/drm_client.h
+++ b/include/drm/drm_client.h
@@ -151,4 +151,6 @@ struct drm_client_buffer *
 drm_client_framebuffer_create(struct drm_client_dev *client, u32 width, u32 height, u32 format);
 void drm_client_framebuffer_delete(struct drm_client_buffer *buffer);
 
+int drm_client_debugfs_init(struct drm_minor *minor);
+
 #endif
-- 
2.15.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 10/12] drm/fb-helper: Finish the generic fbdev emulation

[Noralf Trønnes]

This adds a drm_fbdev_generic_setup() function that sets up generic
fbdev emulation with client callbacks for lastclose, hotplug and
remove/unregister.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/drm_fb_helper.c | 123 +++++++++++++++++++++++++++++++++++++++-
 include/drm/drm_fb_helper.h     |   7 +++
 2 files changed, 129 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index be6bab889f1b..de98d6217864 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -67,6 +67,9 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
  * helper functions used by many drivers to implement the kernel mode setting
  * interfaces.
  *
+ * Drivers that support a dumb buffer with a virtual address and mmap support,
+ * should try out the generic fbdev emulation using drm_fbdev_generic_setup().
+ *
  * Setup fbdev emulation by calling drm_fb_helper_fbdev_setup() and tear it
  * down by calling drm_fb_helper_fbdev_teardown().
  *
@@ -3012,7 +3015,8 @@ static void drm_fbdev_fb_destroy(struct fb_info *info)
 	 * Remove this when all CMA drivers have been moved over to using
 	 * drm_fbdev_generic_setup().
 	 */
-	drm_fbdev_generic_client_release_do(&fb_helper->client);
+	if (!fb_helper->client.funcs)
+		drm_fbdev_generic_client_release_do(&fb_helper->client);
 
 	drm_client_put(&fb_helper->client);
 }
@@ -3135,6 +3139,123 @@ int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,
 }
 EXPORT_SYMBOL(drm_fb_helper_generic_probe);
 
+static const struct drm_fb_helper_funcs drm_fb_helper_generic_funcs = {
+	.fb_probe = drm_fb_helper_generic_probe,
+};
+
+static void drm_fbdev_generic_client_release(struct drm_client_dev *client)
+{
+	struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
+
+	/* Check if drm_fb_helper_fbdev_setup() has been run */
+	if (fb_helper->dev)
+		drm_fbdev_generic_client_release_do(client);
+	drm_client_close(client);
+	kfree(fb_helper);
+}
+
+static void drm_fbdev_client_unregister(struct drm_client_dev *client)
+{
+	struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
+
+	if (fb_helper->fbdev)
+		drm_fb_helper_unregister_fbi(fb_helper);
+	else
+		drm_client_put(client);
+}
+
+static int drm_fbdev_client_restore(struct drm_client_dev *client)
+{
+	struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
+
+	drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
+
+	return 0;
+}
+
+static int drm_fbdev_client_hotplug(struct drm_client_dev *client)
+{
+	struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
+	struct drm_device *dev = client->dev;
+	int ret;
+
+	/* If drm_fb_helper_fbdev_setup() failed, we only try once */
+	if (!fb_helper->dev && fb_helper->funcs)
+		return 0;
+
+	if (dev->fb_helper)
+		return drm_fb_helper_hotplug_event(dev->fb_helper);
+
+	if (!dev->mode_config.num_connector)
+		return 0;
+
+	ret = drm_fb_helper_fbdev_setup(dev, fb_helper, &drm_fb_helper_generic_funcs,
+					fb_helper->preferred_bpp, 0);
+	if (ret) {
+		fb_helper->dev = NULL;
+		fb_helper->fbdev = NULL;
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct drm_client_funcs drm_fbdev_client_funcs = {
+	.release	= drm_fbdev_generic_client_release,
+	.unregister	= drm_fbdev_client_unregister,
+	.restore	= drm_fbdev_client_restore,
+	.hotplug	= drm_fbdev_client_hotplug,
+};
+
+/**
+ * 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.
+ *
+ * This function sets up generic fbdev emulation for drivers that supports
+ * dumb buffers with a virtual address and that can be mmap'ed.
+ *
+ * Restore, hotplug events and teardown are all taken care of. Drivers that do
+ * suspend/resume need to call drm_fb_helper_set_suspend_unlocked() themselves.
+ * Simple drivers might use drm_mode_config_helper_suspend().
+ *
+ * Drivers that set the dirty callback on their framebuffer will get a shadow
+ * fbdev buffer that is blitted onto the real buffer. This is done in order to
+ * make deferred I/O work with all kinds of buffers.
+ *
+ * This function is safe to call even when there are no connectors present.
+ * Setup will be retried on the next hotplug event.
+ *
+ * Returns:
+ * Zero on success or negative error code on failure.
+ */
+int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
+{
+	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_client_new(dev, &fb_helper->client, "fbdev", &drm_fbdev_client_funcs);
+	if (ret) {
+		kfree(fb_helper);
+		return ret;
+	}
+
+	fb_helper->preferred_bpp = preferred_bpp;
+
+	drm_fbdev_client_hotplug(&fb_helper->client);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_fbdev_generic_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 c134bbcfd2e9..5db08c8f1d25 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -354,6 +354,7 @@ void drm_fb_helper_output_poll_changed(struct drm_device *dev);
 
 int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,
 				struct drm_fb_helper_surface_size *sizes);
+int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp);
 #else
 static inline void drm_fb_helper_prepare(struct drm_device *dev,
 					struct drm_fb_helper *helper,
@@ -595,6 +596,12 @@ drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,
 	return 0;
 }
 
+static inline int
+drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
+{
+	return 0;
+}
+
 #endif
 
 static inline int
-- 
2.15.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 11/12] drm/tinydrm: Use drm_fbdev_generic_setup()

[Noralf Trønnes]

Make full use of the generic fbdev client.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/tinydrm/core/tinydrm-core.c | 3 +--
 drivers/gpu/drm/tinydrm/ili9225.c           | 1 -
 drivers/gpu/drm/tinydrm/mi0283qt.c          | 1 -
 drivers/gpu/drm/tinydrm/st7586.c            | 1 -
 drivers/gpu/drm/tinydrm/st7735r.c           | 1 -
 5 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
index 24a33bf862fa..19c7f70adfa5 100644
--- a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
@@ -204,7 +204,7 @@ static int tinydrm_register(struct tinydrm_device *tdev)
 	if (ret)
 		return ret;
 
-	ret = drm_fb_cma_fbdev_init_with_funcs(drm, 0, 0, tdev->fb_funcs);
+	ret = drm_fbdev_generic_setup(drm, 0);
 	if (ret)
 		DRM_ERROR("Failed to initialize fbdev: %d\n", ret);
 
@@ -214,7 +214,6 @@ static int tinydrm_register(struct tinydrm_device *tdev)
 static void tinydrm_unregister(struct tinydrm_device *tdev)
 {
 	drm_atomic_helper_shutdown(tdev->drm);
-	drm_fb_cma_fbdev_fini(tdev->drm);
 	drm_dev_unregister(tdev->drm);
 }
 
diff --git a/drivers/gpu/drm/tinydrm/ili9225.c b/drivers/gpu/drm/tinydrm/ili9225.c
index 841c69aba059..455fefe012f5 100644
--- a/drivers/gpu/drm/tinydrm/ili9225.c
+++ b/drivers/gpu/drm/tinydrm/ili9225.c
@@ -368,7 +368,6 @@ static struct drm_driver ili9225_driver = {
 				  DRIVER_ATOMIC,
 	.fops			= &ili9225_fops,
 	TINYDRM_GEM_DRIVER_OPS,
-	.lastclose		= drm_fb_helper_lastclose,
 	.name			= "ili9225",
 	.desc			= "Ilitek ILI9225",
 	.date			= "20171106",
diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
index 015d03f2acba..d7bb4c5e6657 100644
--- a/drivers/gpu/drm/tinydrm/mi0283qt.c
+++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
@@ -154,7 +154,6 @@ static struct drm_driver mi0283qt_driver = {
 				  DRIVER_ATOMIC,
 	.fops			= &mi0283qt_fops,
 	TINYDRM_GEM_DRIVER_OPS,
-	.lastclose		= drm_fb_helper_lastclose,
 	.debugfs_init		= mipi_dbi_debugfs_init,
 	.name			= "mi0283qt",
 	.desc			= "Multi-Inno MI0283QT",
diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c
index 5c29e3803ecb..2fcbc3067d71 100644
--- a/drivers/gpu/drm/tinydrm/st7586.c
+++ b/drivers/gpu/drm/tinydrm/st7586.c
@@ -304,7 +304,6 @@ static struct drm_driver st7586_driver = {
 				  DRIVER_ATOMIC,
 	.fops			= &st7586_fops,
 	TINYDRM_GEM_DRIVER_OPS,
-	.lastclose		= drm_fb_helper_lastclose,
 	.debugfs_init		= mipi_dbi_debugfs_init,
 	.name			= "st7586",
 	.desc			= "Sitronix ST7586",
diff --git a/drivers/gpu/drm/tinydrm/st7735r.c b/drivers/gpu/drm/tinydrm/st7735r.c
index 6c7b15c9da4f..3081bc57c116 100644
--- a/drivers/gpu/drm/tinydrm/st7735r.c
+++ b/drivers/gpu/drm/tinydrm/st7735r.c
@@ -120,7 +120,6 @@ static struct drm_driver st7735r_driver = {
 				  DRIVER_ATOMIC,
 	.fops			= &st7735r_fops,
 	TINYDRM_GEM_DRIVER_OPS,
-	.lastclose		= drm_fb_helper_lastclose,
 	.debugfs_init		= mipi_dbi_debugfs_init,
 	.name			= "st7735r",
 	.desc			= "Sitronix ST7735R",
-- 
2.15.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 12/12] drm/cma-helper: Remove drm_fb_cma_fbdev_init_with_funcs()

[Noralf Trønnes]

Remove drm_fb_cma_fbdev_init_with_funcs(), its only user tinydrm has
moved to drm_fbdev_generic_setup().

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/drm_fb_cma_helper.c | 21 ---------------------
 include/drm/drm_fb_cma_helper.h     |  3 ---
 2 files changed, 24 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index 31831a7a6a99..63e5a0106211 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -99,27 +99,6 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
 }
 EXPORT_SYMBOL_GPL(drm_fb_cma_get_gem_addr);
 
-/**
- * drm_fb_cma_fbdev_init_with_funcs() - Allocate and initialize 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.
- * @funcs: Framebuffer functions, in particular a custom dirty() callback.
- *         Can be NULL.
- *
- * Returns:
- * Zero on success or negative error code on failure.
- */
-int drm_fb_cma_fbdev_init_with_funcs(struct drm_device *dev,
-	unsigned int preferred_bpp, unsigned int max_conn_count,
-	const struct drm_framebuffer_funcs *funcs)
-{
-	return drm_fb_cma_fbdev_init(dev, preferred_bpp, max_conn_count);
-}
-EXPORT_SYMBOL_GPL(drm_fb_cma_fbdev_init_with_funcs);
-
 /**
  * drm_fb_cma_fbdev_init() - Allocate and initialize fbdev emulation
  * @dev: DRM device
diff --git a/include/drm/drm_fb_cma_helper.h b/include/drm/drm_fb_cma_helper.h
index a0546c3451f9..96e26e3b9a0c 100644
--- a/include/drm/drm_fb_cma_helper.h
+++ b/include/drm/drm_fb_cma_helper.h
@@ -16,9 +16,6 @@ struct drm_mode_fb_cmd2;
 struct drm_plane;
 struct drm_plane_state;
 
-int drm_fb_cma_fbdev_init_with_funcs(struct drm_device *dev,
-	unsigned int preferred_bpp, unsigned int max_conn_count,
-	const struct drm_framebuffer_funcs *funcs);
 int drm_fb_cma_fbdev_init(struct drm_device *dev, unsigned int preferred_bpp,
 			  unsigned int max_conn_count);
 void drm_fb_cma_fbdev_fini(struct drm_device *dev);
-- 
2.15.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v2 09/12] drm/debugfs: Add internal client debugfs file

[Daniel Vetter]

On Mon, Jun 18, 2018 at 04:17:36PM +0200, Noralf Trønnes wrote:
> Print the names of the internal clients currently attached.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_client.c  | 28 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_debugfs.c |  7 +++++++
>  include/drm/drm_client.h      |  2 ++
>  3 files changed, 37 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> index f1dc04d641cc..3ebb8fa34655 100644
> --- a/drivers/gpu/drm/drm_client.c
> +++ b/drivers/gpu/drm/drm_client.c
> @@ -405,3 +405,31 @@ void drm_client_framebuffer_delete(struct drm_client_buffer *buffer)
>  	drm_client_buffer_delete(buffer);
>  }
>  EXPORT_SYMBOL(drm_client_framebuffer_delete);
> +
> +#ifdef CONFIG_DEBUG_FS
> +static int drm_client_debugfs_internal_clients(struct seq_file *m, void *data)
> +{
> +	struct drm_info_node *node = m->private;
> +	struct drm_device *dev = node->minor->dev;
> +	struct drm_printer p = drm_seq_file_printer(m);
> +	struct drm_client_dev *client;
> +
> +	mutex_lock(&dev->clientlist_mutex);
> +	list_for_each_entry(client, &dev->clientlist, list)
> +		drm_printf(&p, "%s\n", client->name);
> +	mutex_unlock(&dev->clientlist_mutex);
> +
> +	return 0;
> +}
> +
> +static const struct drm_info_list drm_client_debugfs_list[] = {
> +	{ "internal_clients", drm_client_debugfs_internal_clients, 0 },
> +};
> +
> +int drm_client_debugfs_init(struct drm_minor *minor)
> +{
> +	return drm_debugfs_create_files(drm_client_debugfs_list,
> +					ARRAY_SIZE(drm_client_debugfs_list),
> +					minor->debugfs_root, minor);
> +}
> +#endif
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index b2482818fee8..50a20bfc07ea 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -28,6 +28,7 @@
>  #include <linux/slab.h>
>  #include <linux/export.h>
>  
> +#include <drm/drm_client.h>
>  #include <drm/drm_debugfs.h>
>  #include <drm/drm_edid.h>
>  #include <drm/drm_atomic.h>
> @@ -164,6 +165,12 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>  			DRM_ERROR("Failed to create framebuffer debugfs file\n");
>  			return ret;
>  		}
> +
> +		ret = drm_client_debugfs_init(minor);
> +		if (ret) {
> +			DRM_ERROR("Failed to create client debugfs file\n");
> +			return ret;
> +		}
>  	}
>  
>  	if (dev->driver->debugfs_init) {
> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
> index 80fe21c86f69..c3a87d6c30fc 100644
> --- a/include/drm/drm_client.h
> +++ b/include/drm/drm_client.h
> @@ -151,4 +151,6 @@ struct drm_client_buffer *
>  drm_client_framebuffer_create(struct drm_client_dev *client, u32 width, u32 height, u32 format);
>  void drm_client_framebuffer_delete(struct drm_client_buffer *buffer);
>  
> +int drm_client_debugfs_init(struct drm_minor *minor);
> +
>  #endif
> -- 
> 2.15.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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

Re: [PATCH v2 00/12] drm: Add generic fbdev emulation

[Daniel Vetter]

On Mon, Jun 18, 2018 at 04:17:27PM +0200, Noralf Trønnes wrote:
> This patchset adds generic fbdev emulation for drivers that supports GEM
> based dumb buffers which support .gem_prime_vmap and gem_prime_mmap. An
> API is begun to support in-kernel clients in general.
> 
> Notable changes since version 1:
> 
> - Rework client unregister code. I've used reference counting to manage
>   the fact that both the client itself and the driver through
>   drm_dev_unregister() can release the client. The client is now released
>   using drm_client_put() instead of drm_client_free().

I started reviewing the reworked client register/unregister stuff, and it
looks good, except this kref stuff here for clients. I don't understand
why you need this - as long as removal from dev->clientlist is properly
protected by the mutex, I don't see how both the client and the device can
release the client at the same time. Of course this means that both the
device-trigger unregister and the client-triggered unregister must first
grab the mutex, remove the client from the list, and only if that was done
succesfully, clean up the client. If the client is already removed from
the list (i.e. list_empty() is true) then you need to bail out to avoid
double-freeing.

I don't think there's a need to use a kref here. And kref has the tricky
issue that you tempt everyone into constructing references loops between
drm_device and drm_client (which require lots of jumping through hoops in
your v1 to make sure you can break those reference loops).

> - fbdev: Use a shadow buffer for framebuffers that have a dirty
>   callback. This makes the fbdev client truly generic and useable for all
>   drivers. There's a blitting penalty, but this is generic emulation after
>   all. The reason for needing a shadow buffer is that deferred I/O only
>   works with kmalloc/vmalloc buffers and not with shmem buffers
>   (page->lru/mapping).

Yeah I think that's the only feasible option. Everyone who cares more
about fbdev performance can keep their driver-specific code. And for other
drm_client users this shouldn't be a problem, since they know how to use
dirty and flipping between multiple buffers to drive kms as it was
designed. The issue really only exists for fbdev's assumption of a direct
mmap of a dumb framebuffer, encoded into the uapi.

> - Let tinydrm use the full fbdev client

\o/

Cheers, Daniel
> 
> Noralf.
> 
> Changes since version 1:
> - Make it possible to embed struct drm_client_dev and drop the private
>   pointer
> - Use kref reference counting to control client release since both the
>   client and the driver can release.
> - Add comment about using dma-buf as a possibility with some rework
> - Move buffer NULL check to drm_client_framebuffer_delete()
> - Move client name to struct drm_client_dev
> - Move up drm_dev_get/put calls to make them more visible
> - Move drm_client_dev.list definition to later patch that makes use of it
> 
> - Embed drm_client at the beginning of drm_fb_helper to avoid a fragile
>   transitional kfree hack in drm_client_release()
> - Set owner in drm_fbdev_fb_ops
> - Add kerneldoc to drm_fb_helper_generic_probe()
> 
> - Remove unused functions
> - Change name drm_client_funcs.lastclose -> .restore
> - Change name drm_client_funcs.remove -> .unregister
> - Rework unregister code
> 
> - tinydrm: Use drm_fbdev_generic_setup() and remove
>   drm_fb_cma_fbdev_init_with_funcs()
> 
> David Herrmann (1):
>   drm: provide management functions for drm_file
> 
> Noralf Trønnes (11):
>   drm/file: Don't set master on in-kernel clients
>   drm: Make ioctls available for in-kernel clients
>   drm: Begin an API for in-kernel clients
>   drm/fb-helper: Add generic fbdev emulation .fb_probe function
>   drm/pl111: Set .gem_prime_vmap and .gem_prime_mmap
>   drm/cma-helper: Use the generic fbdev emulation
>   drm/client: Add client callbacks
>   drm/debugfs: Add internal client debugfs file
>   drm/fb-helper: Finish the generic fbdev emulation
>   drm/tinydrm: Use drm_fbdev_generic_setup()
>   drm/cma-helper: Remove drm_fb_cma_fbdev_init_with_funcs()
> 
>  Documentation/gpu/drm-client.rst            |  12 +
>  Documentation/gpu/index.rst                 |   1 +
>  drivers/gpu/drm/Makefile                    |   2 +-
>  drivers/gpu/drm/drm_client.c                | 435 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_crtc_internal.h         |  19 +-
>  drivers/gpu/drm/drm_debugfs.c               |   7 +
>  drivers/gpu/drm/drm_drv.c                   |   8 +
>  drivers/gpu/drm/drm_dumb_buffers.c          |  33 ++-
>  drivers/gpu/drm/drm_fb_cma_helper.c         | 380 +++---------------------
>  drivers/gpu/drm/drm_fb_helper.c             | 330 ++++++++++++++++++++-
>  drivers/gpu/drm/drm_file.c                  | 304 ++++++++++---------
>  drivers/gpu/drm/drm_framebuffer.c           |  42 ++-
>  drivers/gpu/drm/drm_internal.h              |   2 +
>  drivers/gpu/drm/drm_ioctl.c                 |   4 +-
>  drivers/gpu/drm/drm_probe_helper.c          |   3 +
>  drivers/gpu/drm/pl111/pl111_drv.c           |   2 +
>  drivers/gpu/drm/tinydrm/core/tinydrm-core.c |   3 +-
>  drivers/gpu/drm/tinydrm/ili9225.c           |   1 -
>  drivers/gpu/drm/tinydrm/mi0283qt.c          |   1 -
>  drivers/gpu/drm/tinydrm/st7586.c            |   1 -
>  drivers/gpu/drm/tinydrm/st7735r.c           |   1 -
>  include/drm/drm_client.h                    | 156 ++++++++++
>  include/drm/drm_device.h                    |  21 ++
>  include/drm/drm_fb_cma_helper.h             |   6 -
>  include/drm/drm_fb_helper.h                 |  38 +++
>  25 files changed, 1298 insertions(+), 514 deletions(-)
>  create mode 100644 Documentation/gpu/drm-client.rst
>  create mode 100644 drivers/gpu/drm/drm_client.c
>  create mode 100644 include/drm/drm_client.h
> 
> -- 
> 2.15.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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

Re: [Intel-gfx] [PATCH v2 00/12] drm: Add generic fbdev emulation

[Noralf Trønnes]

Den 20.06.2018 11.34, skrev Daniel Vetter:
> On Mon, Jun 18, 2018 at 04:17:27PM +0200, Noralf Trønnes wrote:
>> This patchset adds generic fbdev emulation for drivers that supports GEM
>> based dumb buffers which support .gem_prime_vmap and gem_prime_mmap. An
>> API is begun to support in-kernel clients in general.
>>
>> Notable changes since version 1:
>>
>> - Rework client unregister code. I've used reference counting to manage
>>    the fact that both the client itself and the driver through
>>    drm_dev_unregister() can release the client. The client is now released
>>    using drm_client_put() instead of drm_client_free().
> I started reviewing the reworked client register/unregister stuff, and it
> looks good, except this kref stuff here for clients. I don't understand
> why you need this - as long as removal from dev->clientlist is properly
> protected by the mutex, I don't see how both the client and the device can
> release the client at the same time. Of course this means that both the
> device-trigger unregister and the client-triggered unregister must first
> grab the mutex, remove the client from the list, and only if that was done
> succesfully, clean up the client. If the client is already removed from
> the list (i.e. list_empty() is true) then you need to bail out to avoid
> double-freeing.

I just suck at this :/

Use case:
Unloading client module at the same time as the device is unplugged.

The client module calls drm_client_release():

void drm_client_release(struct drm_client_dev *client)
{
     struct drm_device *dev = client->dev;

     mutex_lock(&dev->clientlist_mutex);
     list_del(&client->list);
     drm_client_close(client);
     mutex_unlock(&dev->clientlist_mutex);
     drm_dev_put(dev);
}


drm_device_unregister() calls drm_client_dev_unregister():

void drm_client_dev_unregister(struct drm_device *dev)
{
     struct drm_client_dev *client;

     mutex_lock(&dev->clientlist_mutex);
     list_for_each_entry(client, &dev->clientlist, list) {
         if (client->funcs && client->funcs->unregister)
             client->funcs->unregister(client);
         else
             drm_client_release(client);
     }
     mutex_unlock(&dev->clientlist_mutex);
}


How do I do this without deadlocking and without operating on a
drm_client_dev structure that has been freed in the other codepath?


Noralf.

> I don't think there's a need to use a kref here. And kref has the tricky
> issue that you tempt everyone into constructing references loops between
> drm_device and drm_client (which require lots of jumping through hoops in
> your v1 to make sure you can break those reference loops).
>
>> - fbdev: Use a shadow buffer for framebuffers that have a dirty
>>    callback. This makes the fbdev client truly generic and useable for all
>>    drivers. There's a blitting penalty, but this is generic emulation after
>>    all. The reason for needing a shadow buffer is that deferred I/O only
>>    works with kmalloc/vmalloc buffers and not with shmem buffers
>>    (page->lru/mapping).
> Yeah I think that's the only feasible option. Everyone who cares more
> about fbdev performance can keep their driver-specific code. And for other
> drm_client users this shouldn't be a problem, since they know how to use
> dirty and flipping between multiple buffers to drive kms as it was
> designed. The issue really only exists for fbdev's assumption of a direct
> mmap of a dumb framebuffer, encoded into the uapi.
>
>> - Let tinydrm use the full fbdev client
> \o/
>
> Cheers, Daniel
>> Noralf.
>>
>> Changes since version 1:
>> - Make it possible to embed struct drm_client_dev and drop the private
>>    pointer
>> - Use kref reference counting to control client release since both the
>>    client and the driver can release.
>> - Add comment about using dma-buf as a possibility with some rework
>> - Move buffer NULL check to drm_client_framebuffer_delete()
>> - Move client name to struct drm_client_dev
>> - Move up drm_dev_get/put calls to make them more visible
>> - Move drm_client_dev.list definition to later patch that makes use of it
>>
>> - Embed drm_client at the beginning of drm_fb_helper to avoid a fragile
>>    transitional kfree hack in drm_client_release()
>> - Set owner in drm_fbdev_fb_ops
>> - Add kerneldoc to drm_fb_helper_generic_probe()
>>
>> - Remove unused functions
>> - Change name drm_client_funcs.lastclose -> .restore
>> - Change name drm_client_funcs.remove -> .unregister
>> - Rework unregister code
>>
>> - tinydrm: Use drm_fbdev_generic_setup() and remove
>>    drm_fb_cma_fbdev_init_with_funcs()
>>
>> David Herrmann (1):
>>    drm: provide management functions for drm_file
>>
>> Noralf Trønnes (11):
>>    drm/file: Don't set master on in-kernel clients
>>    drm: Make ioctls available for in-kernel clients
>>    drm: Begin an API for in-kernel clients
>>    drm/fb-helper: Add generic fbdev emulation .fb_probe function
>>    drm/pl111: Set .gem_prime_vmap and .gem_prime_mmap
>>    drm/cma-helper: Use the generic fbdev emulation
>>    drm/client: Add client callbacks
>>    drm/debugfs: Add internal client debugfs file
>>    drm/fb-helper: Finish the generic fbdev emulation
>>    drm/tinydrm: Use drm_fbdev_generic_setup()
>>    drm/cma-helper: Remove drm_fb_cma_fbdev_init_with_funcs()
>>
>>   Documentation/gpu/drm-client.rst            |  12 +
>>   Documentation/gpu/index.rst                 |   1 +
>>   drivers/gpu/drm/Makefile                    |   2 +-
>>   drivers/gpu/drm/drm_client.c                | 435 ++++++++++++++++++++++++++++
>>   drivers/gpu/drm/drm_crtc_internal.h         |  19 +-
>>   drivers/gpu/drm/drm_debugfs.c               |   7 +
>>   drivers/gpu/drm/drm_drv.c                   |   8 +
>>   drivers/gpu/drm/drm_dumb_buffers.c          |  33 ++-
>>   drivers/gpu/drm/drm_fb_cma_helper.c         | 380 +++---------------------
>>   drivers/gpu/drm/drm_fb_helper.c             | 330 ++++++++++++++++++++-
>>   drivers/gpu/drm/drm_file.c                  | 304 ++++++++++---------
>>   drivers/gpu/drm/drm_framebuffer.c           |  42 ++-
>>   drivers/gpu/drm/drm_internal.h              |   2 +
>>   drivers/gpu/drm/drm_ioctl.c                 |   4 +-
>>   drivers/gpu/drm/drm_probe_helper.c          |   3 +
>>   drivers/gpu/drm/pl111/pl111_drv.c           |   2 +
>>   drivers/gpu/drm/tinydrm/core/tinydrm-core.c |   3 +-
>>   drivers/gpu/drm/tinydrm/ili9225.c           |   1 -
>>   drivers/gpu/drm/tinydrm/mi0283qt.c          |   1 -
>>   drivers/gpu/drm/tinydrm/st7586.c            |   1 -
>>   drivers/gpu/drm/tinydrm/st7735r.c           |   1 -
>>   include/drm/drm_client.h                    | 156 ++++++++++
>>   include/drm/drm_device.h                    |  21 ++
>>   include/drm/drm_fb_cma_helper.h             |   6 -
>>   include/drm/drm_fb_helper.h                 |  38 +++
>>   25 files changed, 1298 insertions(+), 514 deletions(-)
>>   create mode 100644 Documentation/gpu/drm-client.rst
>>   create mode 100644 drivers/gpu/drm/drm_client.c
>>   create mode 100644 include/drm/drm_client.h
>>
>> -- 
>> 2.15.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v2 00/12] drm: Add generic fbdev emulation

[Noralf Trønnes]

Den 20.06.2018 15.52, skrev Noralf Trønnes:
>
> Den 20.06.2018 11.34, skrev Daniel Vetter:
>> On Mon, Jun 18, 2018 at 04:17:27PM +0200, Noralf Trønnes wrote:
>>> This patchset adds generic fbdev emulation for drivers that supports 
>>> GEM
>>> based dumb buffers which support .gem_prime_vmap and gem_prime_mmap. An
>>> API is begun to support in-kernel clients in general.
>>>
>>> Notable changes since version 1:
>>>
>>> - Rework client unregister code. I've used reference counting to manage
>>>    the fact that both the client itself and the driver through
>>>    drm_dev_unregister() can release the client. The client is now 
>>> released
>>>    using drm_client_put() instead of drm_client_free().
>> I started reviewing the reworked client register/unregister stuff, 
>> and it
>> looks good, except this kref stuff here for clients. I don't understand
>> why you need this - as long as removal from dev->clientlist is properly
>> protected by the mutex, I don't see how both the client and the 
>> device can
>> release the client at the same time. Of course this means that both the
>> device-trigger unregister and the client-triggered unregister must first
>> grab the mutex, remove the client from the list, and only if that was 
>> done
>> succesfully, clean up the client. If the client is already removed from
>> the list (i.e. list_empty() is true) then you need to bail out to avoid
>> double-freeing.
>
> I just suck at this :/
>
> Use case:
> Unloading client module at the same time as the device is unplugged.
>
> The client module calls drm_client_release():
>
> void drm_client_release(struct drm_client_dev *client)
> {
>     struct drm_device *dev = client->dev;
>
>     mutex_lock(&dev->clientlist_mutex);
>     list_del(&client->list);
>     drm_client_close(client);
>     mutex_unlock(&dev->clientlist_mutex);
>     drm_dev_put(dev);
> }
>
>
> drm_device_unregister() calls drm_client_dev_unregister():
>
> void drm_client_dev_unregister(struct drm_device *dev)
> {
>     struct drm_client_dev *client;
>
>     mutex_lock(&dev->clientlist_mutex);
>     list_for_each_entry(client, &dev->clientlist, list) {
>         if (client->funcs && client->funcs->unregister)
>             client->funcs->unregister(client);
>         else
>             drm_client_release(client);
>     }
>     mutex_unlock(&dev->clientlist_mutex);
> }
>
>
> How do I do this without deadlocking and without operating on a
> drm_client_dev structure that has been freed in the other codepath?
>

There's one more quirk that I forgot:
If fbdev can't release the client on .unregister due to open fd's, the
list entry should be removed but releasing resources is deferred to
the last fd being closed.

Noralf.

>
> Noralf.
>
>> I don't think there's a need to use a kref here. And kref has the tricky
>> issue that you tempt everyone into constructing references loops between
>> drm_device and drm_client (which require lots of jumping through 
>> hoops in
>> your v1 to make sure you can break those reference loops).
>>
>>> - fbdev: Use a shadow buffer for framebuffers that have a dirty
>>>    callback. This makes the fbdev client truly generic and useable 
>>> for all
>>>    drivers. There's a blitting penalty, but this is generic 
>>> emulation after
>>>    all. The reason for needing a shadow buffer is that deferred I/O 
>>> only
>>>    works with kmalloc/vmalloc buffers and not with shmem buffers
>>>    (page->lru/mapping).
>> Yeah I think that's the only feasible option. Everyone who cares more
>> about fbdev performance can keep their driver-specific code. And for 
>> other
>> drm_client users this shouldn't be a problem, since they know how to use
>> dirty and flipping between multiple buffers to drive kms as it was
>> designed. The issue really only exists for fbdev's assumption of a 
>> direct
>> mmap of a dumb framebuffer, encoded into the uapi.
>>
>>> - Let tinydrm use the full fbdev client
>> \o/
>>
>> Cheers, Daniel
>>> Noralf.
>>>
>>> Changes since version 1:
>>> - Make it possible to embed struct drm_client_dev and drop the private
>>>    pointer
>>> - Use kref reference counting to control client release since both the
>>>    client and the driver can release.
>>> - Add comment about using dma-buf as a possibility with some rework
>>> - Move buffer NULL check to drm_client_framebuffer_delete()
>>> - Move client name to struct drm_client_dev
>>> - Move up drm_dev_get/put calls to make them more visible
>>> - Move drm_client_dev.list definition to later patch that makes use 
>>> of it
>>>
>>> - Embed drm_client at the beginning of drm_fb_helper to avoid a fragile
>>>    transitional kfree hack in drm_client_release()
>>> - Set owner in drm_fbdev_fb_ops
>>> - Add kerneldoc to drm_fb_helper_generic_probe()
>>>
>>> - Remove unused functions
>>> - Change name drm_client_funcs.lastclose -> .restore
>>> - Change name drm_client_funcs.remove -> .unregister
>>> - Rework unregister code
>>>
>>> - tinydrm: Use drm_fbdev_generic_setup() and remove
>>>    drm_fb_cma_fbdev_init_with_funcs()
>>>
>>> David Herrmann (1):
>>>    drm: provide management functions for drm_file
>>>
>>> Noralf Trønnes (11):
>>>    drm/file: Don't set master on in-kernel clients
>>>    drm: Make ioctls available for in-kernel clients
>>>    drm: Begin an API for in-kernel clients
>>>    drm/fb-helper: Add generic fbdev emulation .fb_probe function
>>>    drm/pl111: Set .gem_prime_vmap and .gem_prime_mmap
>>>    drm/cma-helper: Use the generic fbdev emulation
>>>    drm/client: Add client callbacks
>>>    drm/debugfs: Add internal client debugfs file
>>>    drm/fb-helper: Finish the generic fbdev emulation
>>>    drm/tinydrm: Use drm_fbdev_generic_setup()
>>>    drm/cma-helper: Remove drm_fb_cma_fbdev_init_with_funcs()
>>>
>>>   Documentation/gpu/drm-client.rst            |  12 +
>>>   Documentation/gpu/index.rst                 |   1 +
>>>   drivers/gpu/drm/Makefile                    |   2 +-
>>>   drivers/gpu/drm/drm_client.c                | 435 
>>> ++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/drm_crtc_internal.h         |  19 +-
>>>   drivers/gpu/drm/drm_debugfs.c               |   7 +
>>>   drivers/gpu/drm/drm_drv.c                   |   8 +
>>>   drivers/gpu/drm/drm_dumb_buffers.c          |  33 ++-
>>>   drivers/gpu/drm/drm_fb_cma_helper.c         | 380 
>>> +++---------------------
>>>   drivers/gpu/drm/drm_fb_helper.c             | 330 
>>> ++++++++++++++++++++-
>>>   drivers/gpu/drm/drm_file.c                  | 304 ++++++++++---------
>>>   drivers/gpu/drm/drm_framebuffer.c           |  42 ++-
>>>   drivers/gpu/drm/drm_internal.h              |   2 +
>>>   drivers/gpu/drm/drm_ioctl.c                 |   4 +-
>>>   drivers/gpu/drm/drm_probe_helper.c          |   3 +
>>>   drivers/gpu/drm/pl111/pl111_drv.c           |   2 +
>>>   drivers/gpu/drm/tinydrm/core/tinydrm-core.c |   3 +-
>>>   drivers/gpu/drm/tinydrm/ili9225.c           |   1 -
>>>   drivers/gpu/drm/tinydrm/mi0283qt.c          |   1 -
>>>   drivers/gpu/drm/tinydrm/st7586.c            |   1 -
>>>   drivers/gpu/drm/tinydrm/st7735r.c           |   1 -
>>>   include/drm/drm_client.h                    | 156 ++++++++++
>>>   include/drm/drm_device.h                    |  21 ++
>>>   include/drm/drm_fb_cma_helper.h             |   6 -
>>>   include/drm/drm_fb_helper.h                 |  38 +++
>>>   25 files changed, 1298 insertions(+), 514 deletions(-)
>>>   create mode 100644 Documentation/gpu/drm-client.rst
>>>   create mode 100644 drivers/gpu/drm/drm_client.c
>>>   create mode 100644 include/drm/drm_client.h
>>>
>>> -- 
>>> 2.15.1
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 00/12] drm: Add generic fbdev emulation

[Daniel Vetter]

On Wed, Jun 20, 2018 at 05:28:10PM +0200, Noralf Trønnes wrote:
> 
> Den 20.06.2018 15.52, skrev Noralf Trønnes:
> > 
> > Den 20.06.2018 11.34, skrev Daniel Vetter:
> > > On Mon, Jun 18, 2018 at 04:17:27PM +0200, Noralf Trønnes wrote:
> > > > This patchset adds generic fbdev emulation for drivers that
> > > > supports GEM
> > > > based dumb buffers which support .gem_prime_vmap and gem_prime_mmap. An
> > > > API is begun to support in-kernel clients in general.
> > > > 
> > > > Notable changes since version 1:
> > > > 
> > > > - Rework client unregister code. I've used reference counting to manage
> > > >    the fact that both the client itself and the driver through
> > > >    drm_dev_unregister() can release the client. The client is
> > > > now released
> > > >    using drm_client_put() instead of drm_client_free().
> > > I started reviewing the reworked client register/unregister stuff,
> > > and it
> > > looks good, except this kref stuff here for clients. I don't understand
> > > why you need this - as long as removal from dev->clientlist is properly
> > > protected by the mutex, I don't see how both the client and the
> > > device can
> > > release the client at the same time. Of course this means that both the
> > > device-trigger unregister and the client-triggered unregister must first
> > > grab the mutex, remove the client from the list, and only if that
> > > was done
> > > succesfully, clean up the client. If the client is already removed from
> > > the list (i.e. list_empty() is true) then you need to bail out to avoid
> > > double-freeing.
> > 
> > I just suck at this :/
> > 
> > Use case:
> > Unloading client module at the same time as the device is unplugged.

Do we really want to be able to unload client modules? Atm you can't
unload the drm fbdev emulation either while a driver is still using it.
Dropping that requirement would make things even simpler (you'd just need
to add an owner field to drm_client and a try_module_get when registering
it, bailing out if that fails).

What's the use-case you have in mind that requires that you can unload a
client driver module? Does that even work with the shuffling we've done on
the register side of things?

> > The client module calls drm_client_release():
> > 
> > void drm_client_release(struct drm_client_dev *client)
> > {
> >     struct drm_device *dev = client->dev;

Not sure (without reading your patches again) why we have drm_client_close
here and ->unregister/drm_client_release below. But the trick is roughly
to do
	client = NULL;

	mutex_lock();
	client = find_it();
	if (client)
		list_del();
	mute_unlock();

	if (!client)
		continue; /* or early return, whatever makes sense */

	drm_client_unregister(client);

This way if you race there's:
- only one thread will win, since the removal from the list is locked
- no deadlocks, because the actual cleanup is done outside of the locks

The problem is applying this trick to each situation, since you need to
make sure that you get them all. Since you want to be able to unregister
from 2 different lists, with each their own locks, you need to nest the
above pattern in clever ways. In the client unregister function:

	mutex_lock(fbdev_clients_lock);
	client = list_first(fbdev_clients_list);
	if (client)
		list_del();

	mutex_lock(client->dev);
	if (!list_empty(client->dev_list))
		list_del();
	else
		/* someone else raced us, give up */
		client = NULL;
	mutex_unlock(client->dev);
	mutex_unlock(fbdev_clients_lock);

	if (!client)
		continue; /* or early return, whatever makes sense */

	drm_client_unregister(client);

This way you know that as long as you hold the fbdevs_clients_lock client
can't disappear, so you can look at client->dev (which also won't
disappear, because the client can't disappear), which allows you to take
the per-device client look to check whether you've race with removing.

On the per-device client remove function we can't just do the same trick,
because that would be a locking inversion. Instead we need careful
ordering:


	mutex_lock(client->dev);
	if (!list_empty(client->dev_list))
		list_del();
	else
		/* someone else raced us, give up */
		client = NULL;
	mutex_unlock(client->dev);

	if (!client)
		continue; /* or early return, whatever makes sense */

	/* we've won the race and must do the cleanup, but first we need
	 * to stop use-after-free */

	mutex_lock(fbdev_clients_lock);
	if (!list_empty(client->fbdev_list))
		list_del();
	else
		/* we raced and the other thread did the list removal
		 * already, but will have backed off by now */
	mutex_unlock(fbdev_clients_lock);

	/* no one can get at the client structure anymore, it's safe to
	 * clean it up */

	drm_client_unregister(client);

Lots of complexity for a feature we didn't have yet and that I don't think
we need really, but it is doable :-)

> >     mutex_lock(&dev->clientlist_mutex);
> >     list_del(&client->list);
> >     drm_client_close(client);
> >     mutex_unlock(&dev->clientlist_mutex);
> >     drm_dev_put(dev);
> > }
> > 
> > 
> > drm_device_unregister() calls drm_client_dev_unregister():
> > 
> > void drm_client_dev_unregister(struct drm_device *dev)
> > {
> >     struct drm_client_dev *client;
> > 
> >     mutex_lock(&dev->clientlist_mutex);
> >     list_for_each_entry(client, &dev->clientlist, list) {
> >         if (client->funcs && client->funcs->unregister)
> >             client->funcs->unregister(client);
> >         else
> >             drm_client_release(client);
> >     }
> >     mutex_unlock(&dev->clientlist_mutex);
> > }
> > 
> > 
> > How do I do this without deadlocking and without operating on a
> > drm_client_dev structure that has been freed in the other codepath?
> > 
> 
> There's one more quirk that I forgot:
> If fbdev can't release the client on .unregister due to open fd's, the
> list entry should be removed but releasing resources is deferred to
> the last fd being closed.

For fbdev I think kref'ing it makes sense. But probably better to do that
in the structure that contains the drm_client, since I think this is very
much an fbdev problem, not a general drm_client problem.

Cheers, Daniel

> 
> Noralf.
> 
> > 
> > Noralf.
> > 
> > > I don't think there's a need to use a kref here. And kref has the tricky
> > > issue that you tempt everyone into constructing references loops between
> > > drm_device and drm_client (which require lots of jumping through
> > > hoops in
> > > your v1 to make sure you can break those reference loops).
> > > 
> > > > - fbdev: Use a shadow buffer for framebuffers that have a dirty
> > > >    callback. This makes the fbdev client truly generic and
> > > > useable for all
> > > >    drivers. There's a blitting penalty, but this is generic
> > > > emulation after
> > > >    all. The reason for needing a shadow buffer is that deferred
> > > > I/O only
> > > >    works with kmalloc/vmalloc buffers and not with shmem buffers
> > > >    (page->lru/mapping).
> > > Yeah I think that's the only feasible option. Everyone who cares more
> > > about fbdev performance can keep their driver-specific code. And for
> > > other
> > > drm_client users this shouldn't be a problem, since they know how to use
> > > dirty and flipping between multiple buffers to drive kms as it was
> > > designed. The issue really only exists for fbdev's assumption of a
> > > direct
> > > mmap of a dumb framebuffer, encoded into the uapi.
> > > 
> > > > - Let tinydrm use the full fbdev client
> > > \o/
> > > 
> > > Cheers, Daniel
> > > > Noralf.
> > > > 
> > > > Changes since version 1:
> > > > - Make it possible to embed struct drm_client_dev and drop the private
> > > >    pointer
> > > > - Use kref reference counting to control client release since both the
> > > >    client and the driver can release.
> > > > - Add comment about using dma-buf as a possibility with some rework
> > > > - Move buffer NULL check to drm_client_framebuffer_delete()
> > > > - Move client name to struct drm_client_dev
> > > > - Move up drm_dev_get/put calls to make them more visible
> > > > - Move drm_client_dev.list definition to later patch that makes
> > > > use of it
> > > > 
> > > > - Embed drm_client at the beginning of drm_fb_helper to avoid a fragile
> > > >    transitional kfree hack in drm_client_release()
> > > > - Set owner in drm_fbdev_fb_ops
> > > > - Add kerneldoc to drm_fb_helper_generic_probe()
> > > > 
> > > > - Remove unused functions
> > > > - Change name drm_client_funcs.lastclose -> .restore
> > > > - Change name drm_client_funcs.remove -> .unregister
> > > > - Rework unregister code
> > > > 
> > > > - tinydrm: Use drm_fbdev_generic_setup() and remove
> > > >    drm_fb_cma_fbdev_init_with_funcs()
> > > > 
> > > > David Herrmann (1):
> > > >    drm: provide management functions for drm_file
> > > > 
> > > > Noralf Trønnes (11):
> > > >    drm/file: Don't set master on in-kernel clients
> > > >    drm: Make ioctls available for in-kernel clients
> > > >    drm: Begin an API for in-kernel clients
> > > >    drm/fb-helper: Add generic fbdev emulation .fb_probe function
> > > >    drm/pl111: Set .gem_prime_vmap and .gem_prime_mmap
> > > >    drm/cma-helper: Use the generic fbdev emulation
> > > >    drm/client: Add client callbacks
> > > >    drm/debugfs: Add internal client debugfs file
> > > >    drm/fb-helper: Finish the generic fbdev emulation
> > > >    drm/tinydrm: Use drm_fbdev_generic_setup()
> > > >    drm/cma-helper: Remove drm_fb_cma_fbdev_init_with_funcs()
> > > > 
> > > >   Documentation/gpu/drm-client.rst            |  12 +
> > > >   Documentation/gpu/index.rst                 |   1 +
> > > >   drivers/gpu/drm/Makefile                    |   2 +-
> > > >   drivers/gpu/drm/drm_client.c                | 435
> > > > ++++++++++++++++++++++++++++
> > > >   drivers/gpu/drm/drm_crtc_internal.h         |  19 +-
> > > >   drivers/gpu/drm/drm_debugfs.c               |   7 +
> > > >   drivers/gpu/drm/drm_drv.c                   |   8 +
> > > >   drivers/gpu/drm/drm_dumb_buffers.c          |  33 ++-
> > > >   drivers/gpu/drm/drm_fb_cma_helper.c         | 380
> > > > +++---------------------
> > > >   drivers/gpu/drm/drm_fb_helper.c             | 330
> > > > ++++++++++++++++++++-
> > > >   drivers/gpu/drm/drm_file.c                  | 304 ++++++++++---------
> > > >   drivers/gpu/drm/drm_framebuffer.c           |  42 ++-
> > > >   drivers/gpu/drm/drm_internal.h              |   2 +
> > > >   drivers/gpu/drm/drm_ioctl.c                 |   4 +-
> > > >   drivers/gpu/drm/drm_probe_helper.c          |   3 +
> > > >   drivers/gpu/drm/pl111/pl111_drv.c           |   2 +
> > > >   drivers/gpu/drm/tinydrm/core/tinydrm-core.c |   3 +-
> > > >   drivers/gpu/drm/tinydrm/ili9225.c           |   1 -
> > > >   drivers/gpu/drm/tinydrm/mi0283qt.c          |   1 -
> > > >   drivers/gpu/drm/tinydrm/st7586.c            |   1 -
> > > >   drivers/gpu/drm/tinydrm/st7735r.c           |   1 -
> > > >   include/drm/drm_client.h                    | 156 ++++++++++
> > > >   include/drm/drm_device.h                    |  21 ++
> > > >   include/drm/drm_fb_cma_helper.h             |   6 -
> > > >   include/drm/drm_fb_helper.h                 |  38 +++
> > > >   25 files changed, 1298 insertions(+), 514 deletions(-)
> > > >   create mode 100644 Documentation/gpu/drm-client.rst
> > > >   create mode 100644 drivers/gpu/drm/drm_client.c
> > > >   create mode 100644 include/drm/drm_client.h
> > > > 
> > > > -- 
> > > > 2.15.1
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> 

-- 
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

Re: [PATCH v2 00/12] drm: Add generic fbdev emulation

[Noralf Trønnes]

Den 21.06.2018 09.14, skrev Daniel Vetter:
> On Wed, Jun 20, 2018 at 05:28:10PM +0200, Noralf Trønnes wrote:
>> Den 20.06.2018 15.52, skrev Noralf Trønnes:
>>> Den 20.06.2018 11.34, skrev Daniel Vetter:
>>>> On Mon, Jun 18, 2018 at 04:17:27PM +0200, Noralf Trønnes wrote:
>>>>> This patchset adds generic fbdev emulation for drivers that
>>>>> supports GEM
>>>>> based dumb buffers which support .gem_prime_vmap and gem_prime_mmap. An
>>>>> API is begun to support in-kernel clients in general.
>>>>>
>>>>> Notable changes since version 1:
>>>>>
>>>>> - Rework client unregister code. I've used reference counting to manage
>>>>>     the fact that both the client itself and the driver through
>>>>>     drm_dev_unregister() can release the client. The client is
>>>>> now released
>>>>>     using drm_client_put() instead of drm_client_free().
>>>> I started reviewing the reworked client register/unregister stuff,
>>>> and it
>>>> looks good, except this kref stuff here for clients. I don't understand
>>>> why you need this - as long as removal from dev->clientlist is properly
>>>> protected by the mutex, I don't see how both the client and the
>>>> device can
>>>> release the client at the same time. Of course this means that both the
>>>> device-trigger unregister and the client-triggered unregister must first
>>>> grab the mutex, remove the client from the list, and only if that
>>>> was done
>>>> succesfully, clean up the client. If the client is already removed from
>>>> the list (i.e. list_empty() is true) then you need to bail out to avoid
>>>> double-freeing.
>>> I just suck at this :/
>>>
>>> Use case:
>>> Unloading client module at the same time as the device is unplugged.
> Do we really want to be able to unload client modules? Atm you can't
> unload the drm fbdev emulation either while a driver is still using it.
> Dropping that requirement would make things even simpler (you'd just need
> to add an owner field to drm_client and a try_module_get when registering
> it, bailing out if that fails).
>
> What's the use-case you have in mind that requires that you can unload a
> client driver module? Does that even work with the shuffling we've done on
> the register side of things?

When I first started on this client API, the client could unload itself
and I had a sysfs file that would remove clients for a particular
drm_device. This would mean that unloading a driver would require clients
to be removed first by writing to the sysfs file.
Then I started to look at the possibility that the driver could remove
clients automatically on driver unload. I have wrecked my brain trying to
make it race free, but it gets very complicated as you have shown in your
example. So I think we'll just avoid this complexity altogether.

So, now the question is, who gets to remove clients. The client or the 
driver?
The common pattern is that clients can come and go on their own choosing.
It's what I would expect, that the client can be unloaded.
The reason I looked into auto unloading when the driver where going away,
was so developers shouldn't have to do the extra step to unload the driver.

Now I see a third case, and that's plug/unplug USB devices. If the driver
can't remove the client, we can end up with lots hanging drm_device and
drm_client_dev instances that have to be removed manually, which is
really not an option.

So I think we remove clients on driver/device removal. This means that to
unload a client, the driver(s) would have to be unloaded first.

I'll add an owner field to drm_client_funcs as you suggested and work out
the details.

Thanks for helping me get this as simple and straightforward as possible.

Noralf.

>>> The client module calls drm_client_release():
>>>
>>> void drm_client_release(struct drm_client_dev *client)
>>> {
>>>      struct drm_device *dev = client->dev;
> Not sure (without reading your patches again) why we have drm_client_close
> here and ->unregister/drm_client_release below. But the trick is roughly
> to do
> 	client = NULL;
>
> 	mutex_lock();
> 	client = find_it();
> 	if (client)
> 		list_del();
> 	mute_unlock();
>
> 	if (!client)
> 		continue; /* or early return, whatever makes sense */
>
> 	drm_client_unregister(client);
>
> This way if you race there's:
> - only one thread will win, since the removal from the list is locked
> - no deadlocks, because the actual cleanup is done outside of the locks
>
> The problem is applying this trick to each situation, since you need to
> make sure that you get them all. Since you want to be able to unregister
> from 2 different lists, with each their own locks, you need to nest the
> above pattern in clever ways. In the client unregister function:
>
> 	mutex_lock(fbdev_clients_lock);
> 	client = list_first(fbdev_clients_list);
> 	if (client)
> 		list_del();
>
> 	mutex_lock(client->dev);
> 	if (!list_empty(client->dev_list))
> 		list_del();
> 	else
> 		/* someone else raced us, give up */
> 		client = NULL;
> 	mutex_unlock(client->dev);
> 	mutex_unlock(fbdev_clients_lock);
>
> 	if (!client)
> 		continue; /* or early return, whatever makes sense */
>
> 	drm_client_unregister(client);
>
> This way you know that as long as you hold the fbdevs_clients_lock client
> can't disappear, so you can look at client->dev (which also won't
> disappear, because the client can't disappear), which allows you to take
> the per-device client look to check whether you've race with removing.
>
> On the per-device client remove function we can't just do the same trick,
> because that would be a locking inversion. Instead we need careful
> ordering:
>
>
> 	mutex_lock(client->dev);
> 	if (!list_empty(client->dev_list))
> 		list_del();
> 	else
> 		/* someone else raced us, give up */
> 		client = NULL;
> 	mutex_unlock(client->dev);
>
> 	if (!client)
> 		continue; /* or early return, whatever makes sense */
>
> 	/* we've won the race and must do the cleanup, but first we need
> 	 * to stop use-after-free */
>
> 	mutex_lock(fbdev_clients_lock);
> 	if (!list_empty(client->fbdev_list))
> 		list_del();
> 	else
> 		/* we raced and the other thread did the list removal
> 		 * already, but will have backed off by now */
> 	mutex_unlock(fbdev_clients_lock);
>
> 	/* no one can get at the client structure anymore, it's safe to
> 	 * clean it up */
>
> 	drm_client_unregister(client);
>
> Lots of complexity for a feature we didn't have yet and that I don't think
> we need really, but it is doable :-)
>
>>>      mutex_lock(&dev->clientlist_mutex);
>>>      list_del(&client->list);
>>>      drm_client_close(client);
>>>      mutex_unlock(&dev->clientlist_mutex);
>>>      drm_dev_put(dev);
>>> }
>>>
>>>
>>> drm_device_unregister() calls drm_client_dev_unregister():
>>>
>>> void drm_client_dev_unregister(struct drm_device *dev)
>>> {
>>>      struct drm_client_dev *client;
>>>
>>>      mutex_lock(&dev->clientlist_mutex);
>>>      list_for_each_entry(client, &dev->clientlist, list) {
>>>          if (client->funcs && client->funcs->unregister)
>>>              client->funcs->unregister(client);
>>>          else
>>>              drm_client_release(client);
>>>      }
>>>      mutex_unlock(&dev->clientlist_mutex);
>>> }
>>>
>>>
>>> How do I do this without deadlocking and without operating on a
>>> drm_client_dev structure that has been freed in the other codepath?
>>>
>> There's one more quirk that I forgot:
>> If fbdev can't release the client on .unregister due to open fd's, the
>> list entry should be removed but releasing resources is deferred to
>> the last fd being closed.
> For fbdev I think kref'ing it makes sense. But probably better to do that
> in the structure that contains the drm_client, since I think this is very
> much an fbdev problem, not a general drm_client problem.
>
> Cheers, Daniel
>
>> Noralf.
>>
>>> Noralf.
>>>
>>>> I don't think there's a need to use a kref here. And kref has the tricky
>>>> issue that you tempt everyone into constructing references loops between
>>>> drm_device and drm_client (which require lots of jumping through
>>>> hoops in
>>>> your v1 to make sure you can break those reference loops).
>>>>
>>>>> - fbdev: Use a shadow buffer for framebuffers that have a dirty
>>>>>     callback. This makes the fbdev client truly generic and
>>>>> useable for all
>>>>>     drivers. There's a blitting penalty, but this is generic
>>>>> emulation after
>>>>>     all. The reason for needing a shadow buffer is that deferred
>>>>> I/O only
>>>>>     works with kmalloc/vmalloc buffers and not with shmem buffers
>>>>>     (page->lru/mapping).
>>>> Yeah I think that's the only feasible option. Everyone who cares more
>>>> about fbdev performance can keep their driver-specific code. And for
>>>> other
>>>> drm_client users this shouldn't be a problem, since they know how to use
>>>> dirty and flipping between multiple buffers to drive kms as it was
>>>> designed. The issue really only exists for fbdev's assumption of a
>>>> direct
>>>> mmap of a dumb framebuffer, encoded into the uapi.
>>>>
>>>>> - Let tinydrm use the full fbdev client
>>>> \o/
>>>>
>>>> Cheers, Daniel
>>>>> Noralf.
>>>>>
>>>>> Changes since version 1:
>>>>> - Make it possible to embed struct drm_client_dev and drop the private
>>>>>     pointer
>>>>> - Use kref reference counting to control client release since both the
>>>>>     client and the driver can release.
>>>>> - Add comment about using dma-buf as a possibility with some rework
>>>>> - Move buffer NULL check to drm_client_framebuffer_delete()
>>>>> - Move client name to struct drm_client_dev
>>>>> - Move up drm_dev_get/put calls to make them more visible
>>>>> - Move drm_client_dev.list definition to later patch that makes
>>>>> use of it
>>>>>
>>>>> - Embed drm_client at the beginning of drm_fb_helper to avoid a fragile
>>>>>     transitional kfree hack in drm_client_release()
>>>>> - Set owner in drm_fbdev_fb_ops
>>>>> - Add kerneldoc to drm_fb_helper_generic_probe()
>>>>>
>>>>> - Remove unused functions
>>>>> - Change name drm_client_funcs.lastclose -> .restore
>>>>> - Change name drm_client_funcs.remove -> .unregister
>>>>> - Rework unregister code
>>>>>
>>>>> - tinydrm: Use drm_fbdev_generic_setup() and remove
>>>>>     drm_fb_cma_fbdev_init_with_funcs()
>>>>>
>>>>> David Herrmann (1):
>>>>>     drm: provide management functions for drm_file
>>>>>
>>>>> Noralf Trønnes (11):
>>>>>     drm/file: Don't set master on in-kernel clients
>>>>>     drm: Make ioctls available for in-kernel clients
>>>>>     drm: Begin an API for in-kernel clients
>>>>>     drm/fb-helper: Add generic fbdev emulation .fb_probe function
>>>>>     drm/pl111: Set .gem_prime_vmap and .gem_prime_mmap
>>>>>     drm/cma-helper: Use the generic fbdev emulation
>>>>>     drm/client: Add client callbacks
>>>>>     drm/debugfs: Add internal client debugfs file
>>>>>     drm/fb-helper: Finish the generic fbdev emulation
>>>>>     drm/tinydrm: Use drm_fbdev_generic_setup()
>>>>>     drm/cma-helper: Remove drm_fb_cma_fbdev_init_with_funcs()
>>>>>
>>>>>    Documentation/gpu/drm-client.rst            |  12 +
>>>>>    Documentation/gpu/index.rst                 |   1 +
>>>>>    drivers/gpu/drm/Makefile                    |   2 +-
>>>>>    drivers/gpu/drm/drm_client.c                | 435
>>>>> ++++++++++++++++++++++++++++
>>>>>    drivers/gpu/drm/drm_crtc_internal.h         |  19 +-
>>>>>    drivers/gpu/drm/drm_debugfs.c               |   7 +
>>>>>    drivers/gpu/drm/drm_drv.c                   |   8 +
>>>>>    drivers/gpu/drm/drm_dumb_buffers.c          |  33 ++-
>>>>>    drivers/gpu/drm/drm_fb_cma_helper.c         | 380
>>>>> +++---------------------
>>>>>    drivers/gpu/drm/drm_fb_helper.c             | 330
>>>>> ++++++++++++++++++++-
>>>>>    drivers/gpu/drm/drm_file.c                  | 304 ++++++++++---------
>>>>>    drivers/gpu/drm/drm_framebuffer.c           |  42 ++-
>>>>>    drivers/gpu/drm/drm_internal.h              |   2 +
>>>>>    drivers/gpu/drm/drm_ioctl.c                 |   4 +-
>>>>>    drivers/gpu/drm/drm_probe_helper.c          |   3 +
>>>>>    drivers/gpu/drm/pl111/pl111_drv.c           |   2 +
>>>>>    drivers/gpu/drm/tinydrm/core/tinydrm-core.c |   3 +-
>>>>>    drivers/gpu/drm/tinydrm/ili9225.c           |   1 -
>>>>>    drivers/gpu/drm/tinydrm/mi0283qt.c          |   1 -
>>>>>    drivers/gpu/drm/tinydrm/st7586.c            |   1 -
>>>>>    drivers/gpu/drm/tinydrm/st7735r.c           |   1 -
>>>>>    include/drm/drm_client.h                    | 156 ++++++++++
>>>>>    include/drm/drm_device.h                    |  21 ++
>>>>>    include/drm/drm_fb_cma_helper.h             |   6 -
>>>>>    include/drm/drm_fb_helper.h                 |  38 +++
>>>>>    25 files changed, 1298 insertions(+), 514 deletions(-)
>>>>>    create mode 100644 Documentation/gpu/drm-client.rst
>>>>>    create mode 100644 drivers/gpu/drm/drm_client.c
>>>>>    create mode 100644 include/drm/drm_client.h
>>>>>
>>>>> -- 
>>>>> 2.15.1
>>>>>
>>>>> _______________________________________________
>>>>> Intel-gfx mailing list
>>>>> Intel-gfx@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 00/12] drm: Add generic fbdev emulation

[Noralf Trønnes]

Den 18.06.2018 16.17, skrev Noralf Trønnes:
> This patchset adds generic fbdev emulation for drivers that supports GEM
> based dumb buffers which support .gem_prime_vmap and gem_prime_mmap. An
> API is begun to support in-kernel clients in general.
>
> Notable changes since version 1:
>
> - Rework client unregister code. I've used reference counting to manage
>    the fact that both the client itself and the driver through
>    drm_dev_unregister() can release the client. The client is now released
>    using drm_client_put() instead of drm_client_free().
>
> - fbdev: Use a shadow buffer for framebuffers that have a dirty
>    callback. This makes the fbdev client truly generic and useable for all
>    drivers. There's a blitting penalty, but this is generic emulation after
>    all. The reason for needing a shadow buffer is that deferred I/O only
>    works with kmalloc/vmalloc buffers and not with shmem buffers
>    (page->lru/mapping).
>
> - Let tinydrm use the full fbdev client
>
> Noralf.
>
> Changes since version 1:
> - Make it possible to embed struct drm_client_dev and drop the private
>    pointer
> - Use kref reference counting to control client release since both the
>    client and the driver can release.
> - Add comment about using dma-buf as a possibility with some rework
> - Move buffer NULL check to drm_client_framebuffer_delete()
> - Move client name to struct drm_client_dev
> - Move up drm_dev_get/put calls to make them more visible
> - Move drm_client_dev.list definition to later patch that makes use of it
>
> - Embed drm_client at the beginning of drm_fb_helper to avoid a fragile
>    transitional kfree hack in drm_client_release()
> - Set owner in drm_fbdev_fb_ops
> - Add kerneldoc to drm_fb_helper_generic_probe()
>
> - Remove unused functions
> - Change name drm_client_funcs.lastclose -> .restore
> - Change name drm_client_funcs.remove -> .unregister
> - Rework unregister code
>
> - tinydrm: Use drm_fbdev_generic_setup() and remove
>    drm_fb_cma_fbdev_init_with_funcs()
>
> David Herrmann (1):
>    drm: provide management functions for drm_file
>
> Noralf Trønnes (11):
>    drm/file: Don't set master on in-kernel clients
>    drm: Make ioctls available for in-kernel clients

Patches 1-3 applied to drm-misc-next. Thanks for reviewing!

Noralf.

>    drm: Begin an API for in-kernel clients
>    drm/fb-helper: Add generic fbdev emulation .fb_probe function
>    drm/pl111: Set .gem_prime_vmap and .gem_prime_mmap
>    drm/cma-helper: Use the generic fbdev emulation
>    drm/client: Add client callbacks
>    drm/debugfs: Add internal client debugfs file
>    drm/fb-helper: Finish the generic fbdev emulation
>    drm/tinydrm: Use drm_fbdev_generic_setup()
>    drm/cma-helper: Remove drm_fb_cma_fbdev_init_with_funcs()
>
>   Documentation/gpu/drm-client.rst            |  12 +
>   Documentation/gpu/index.rst                 |   1 +
>   drivers/gpu/drm/Makefile                    |   2 +-
>   drivers/gpu/drm/drm_client.c                | 435 ++++++++++++++++++++++++++++
>   drivers/gpu/drm/drm_crtc_internal.h         |  19 +-
>   drivers/gpu/drm/drm_debugfs.c               |   7 +
>   drivers/gpu/drm/drm_drv.c                   |   8 +
>   drivers/gpu/drm/drm_dumb_buffers.c          |  33 ++-
>   drivers/gpu/drm/drm_fb_cma_helper.c         | 380 +++---------------------
>   drivers/gpu/drm/drm_fb_helper.c             | 330 ++++++++++++++++++++-
>   drivers/gpu/drm/drm_file.c                  | 304 ++++++++++---------
>   drivers/gpu/drm/drm_framebuffer.c           |  42 ++-
>   drivers/gpu/drm/drm_internal.h              |   2 +
>   drivers/gpu/drm/drm_ioctl.c                 |   4 +-
>   drivers/gpu/drm/drm_probe_helper.c          |   3 +
>   drivers/gpu/drm/pl111/pl111_drv.c           |   2 +
>   drivers/gpu/drm/tinydrm/core/tinydrm-core.c |   3 +-
>   drivers/gpu/drm/tinydrm/ili9225.c           |   1 -
>   drivers/gpu/drm/tinydrm/mi0283qt.c          |   1 -
>   drivers/gpu/drm/tinydrm/st7586.c            |   1 -
>   drivers/gpu/drm/tinydrm/st7735r.c           |   1 -
>   include/drm/drm_client.h                    | 156 ++++++++++
>   include/drm/drm_device.h                    |  21 ++
>   include/drm/drm_fb_cma_helper.h             |   6 -
>   include/drm/drm_fb_helper.h                 |  38 +++
>   25 files changed, 1298 insertions(+), 514 deletions(-)
>   create mode 100644 Documentation/gpu/drm-client.rst
>   create mode 100644 drivers/gpu/drm/drm_client.c
>   create mode 100644 include/drm/drm_client.h
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 00/12] drm: Add generic fbdev emulation

[Noralf Trønnes]

Den 21.06.2018 19.19, skrev Noralf Trønnes:
>
> Den 21.06.2018 09.14, skrev Daniel Vetter:
>> On Wed, Jun 20, 2018 at 05:28:10PM +0200, Noralf Trønnes wrote:
>>> Den 20.06.2018 15.52, skrev Noralf Trønnes:
>>>> Den 20.06.2018 11.34, skrev Daniel Vetter:
>>>>> On Mon, Jun 18, 2018 at 04:17:27PM +0200, Noralf Trønnes wrote:
>>>>>> This patchset adds generic fbdev emulation for drivers that
>>>>>> supports GEM
>>>>>> based dumb buffers which support .gem_prime_vmap and 
>>>>>> gem_prime_mmap. An
>>>>>> API is begun to support in-kernel clients in general.
>>>>>>
>>>>>> Notable changes since version 1:
>>>>>>
>>>>>> - Rework client unregister code. I've used reference counting to 
>>>>>> manage
>>>>>>     the fact that both the client itself and the driver through
>>>>>>     drm_dev_unregister() can release the client. The client is
>>>>>> now released
>>>>>>     using drm_client_put() instead of drm_client_free().
>>>>> I started reviewing the reworked client register/unregister stuff,
>>>>> and it
>>>>> looks good, except this kref stuff here for clients. I don't 
>>>>> understand
>>>>> why you need this - as long as removal from dev->clientlist is 
>>>>> properly
>>>>> protected by the mutex, I don't see how both the client and the
>>>>> device can
>>>>> release the client at the same time. Of course this means that 
>>>>> both the
>>>>> device-trigger unregister and the client-triggered unregister must 
>>>>> first
>>>>> grab the mutex, remove the client from the list, and only if that
>>>>> was done
>>>>> succesfully, clean up the client. If the client is already removed 
>>>>> from
>>>>> the list (i.e. list_empty() is true) then you need to bail out to 
>>>>> avoid
>>>>> double-freeing.
>>>> I just suck at this :/
>>>>
>>>> Use case:
>>>> Unloading client module at the same time as the device is unplugged.
>> Do we really want to be able to unload client modules? Atm you can't
>> unload the drm fbdev emulation either while a driver is still using it.
>> Dropping that requirement would make things even simpler (you'd just 
>> need
>> to add an owner field to drm_client and a try_module_get when 
>> registering
>> it, bailing out if that fails).
>>
>> What's the use-case you have in mind that requires that you can unload a
>> client driver module? Does that even work with the shuffling we've 
>> done on
>> the register side of things?
>
> When I first started on this client API, the client could unload itself
> and I had a sysfs file that would remove clients for a particular
> drm_device. This would mean that unloading a driver would require clients
> to be removed first by writing to the sysfs file.
> Then I started to look at the possibility that the driver could remove
> clients automatically on driver unload. I have wrecked my brain trying to
> make it race free, but it gets very complicated as you have shown in your
> example. So I think we'll just avoid this complexity altogether.
>
> So, now the question is, who gets to remove clients. The client or the 
> driver?
> The common pattern is that clients can come and go on their own choosing.
> It's what I would expect, that the client can be unloaded.
> The reason I looked into auto unloading when the driver where going away,
> was so developers shouldn't have to do the extra step to unload the 
> driver.
>
> Now I see a third case, and that's plug/unplug USB devices. If the driver
> can't remove the client, we can end up with lots hanging drm_device and
> drm_client_dev instances that have to be removed manually, which is
> really not an option.
>
> So I think we remove clients on driver/device removal. This means that to
> unload a client, the driver(s) would have to be unloaded first.
>
> I'll add an owner field to drm_client_funcs as you suggested and work out
> the details.

Currently drm_client_dev->funcs is optional, but should it be mandatory
now that drm_client_funcs gets an owner field?

Noralf.

>
> Thanks for helping me get this as simple and straightforward as possible.
>
> Noralf.
>
>>>> The client module calls drm_client_release():
>>>>
>>>> void drm_client_release(struct drm_client_dev *client)
>>>> {
>>>>      struct drm_device *dev = client->dev;
>> Not sure (without reading your patches again) why we have 
>> drm_client_close
>> here and ->unregister/drm_client_release below. But the trick is roughly
>> to do
>>     client = NULL;
>>
>>     mutex_lock();
>>     client = find_it();
>>     if (client)
>>         list_del();
>>     mute_unlock();
>>
>>     if (!client)
>>         continue; /* or early return, whatever makes sense */
>>
>>     drm_client_unregister(client);
>>
>> This way if you race there's:
>> - only one thread will win, since the removal from the list is locked
>> - no deadlocks, because the actual cleanup is done outside of the locks
>>
>> The problem is applying this trick to each situation, since you need to
>> make sure that you get them all. Since you want to be able to unregister
>> from 2 different lists, with each their own locks, you need to nest the
>> above pattern in clever ways. In the client unregister function:
>>
>>     mutex_lock(fbdev_clients_lock);
>>     client = list_first(fbdev_clients_list);
>>     if (client)
>>         list_del();
>>
>>     mutex_lock(client->dev);
>>     if (!list_empty(client->dev_list))
>>         list_del();
>>     else
>>         /* someone else raced us, give up */
>>         client = NULL;
>>     mutex_unlock(client->dev);
>>     mutex_unlock(fbdev_clients_lock);
>>
>>     if (!client)
>>         continue; /* or early return, whatever makes sense */
>>
>>     drm_client_unregister(client);
>>
>> This way you know that as long as you hold the fbdevs_clients_lock 
>> client
>> can't disappear, so you can look at client->dev (which also won't
>> disappear, because the client can't disappear), which allows you to take
>> the per-device client look to check whether you've race with removing.
>>
>> On the per-device client remove function we can't just do the same 
>> trick,
>> because that would be a locking inversion. Instead we need careful
>> ordering:
>>
>>
>>     mutex_lock(client->dev);
>>     if (!list_empty(client->dev_list))
>>         list_del();
>>     else
>>         /* someone else raced us, give up */
>>         client = NULL;
>>     mutex_unlock(client->dev);
>>
>>     if (!client)
>>         continue; /* or early return, whatever makes sense */
>>
>>     /* we've won the race and must do the cleanup, but first we need
>>      * to stop use-after-free */
>>
>>     mutex_lock(fbdev_clients_lock);
>>     if (!list_empty(client->fbdev_list))
>>         list_del();
>>     else
>>         /* we raced and the other thread did the list removal
>>          * already, but will have backed off by now */
>>     mutex_unlock(fbdev_clients_lock);
>>
>>     /* no one can get at the client structure anymore, it's safe to
>>      * clean it up */
>>
>>     drm_client_unregister(client);
>>
>> Lots of complexity for a feature we didn't have yet and that I don't 
>> think
>> we need really, but it is doable :-)
>>
>>>> mutex_lock(&dev->clientlist_mutex);
>>>>      list_del(&client->list);
>>>>      drm_client_close(client);
>>>>      mutex_unlock(&dev->clientlist_mutex);
>>>>      drm_dev_put(dev);
>>>> }
>>>>
>>>>
>>>> drm_device_unregister() calls drm_client_dev_unregister():
>>>>
>>>> void drm_client_dev_unregister(struct drm_device *dev)
>>>> {
>>>>      struct drm_client_dev *client;
>>>>
>>>>      mutex_lock(&dev->clientlist_mutex);
>>>>      list_for_each_entry(client, &dev->clientlist, list) {
>>>>          if (client->funcs && client->funcs->unregister)
>>>>              client->funcs->unregister(client);
>>>>          else
>>>>              drm_client_release(client);
>>>>      }
>>>>      mutex_unlock(&dev->clientlist_mutex);
>>>> }
>>>>
>>>>
>>>> How do I do this without deadlocking and without operating on a
>>>> drm_client_dev structure that has been freed in the other codepath?
>>>>
>>> There's one more quirk that I forgot:
>>> If fbdev can't release the client on .unregister due to open fd's, the
>>> list entry should be removed but releasing resources is deferred to
>>> the last fd being closed.
>> For fbdev I think kref'ing it makes sense. But probably better to do 
>> that
>> in the structure that contains the drm_client, since I think this is 
>> very
>> much an fbdev problem, not a general drm_client problem.
>>
>> Cheers, Daniel
>>
>>> Noralf.
>>>
>>>> Noralf.
>>>>
>>>>> I don't think there's a need to use a kref here. And kref has the 
>>>>> tricky
>>>>> issue that you tempt everyone into constructing references loops 
>>>>> between
>>>>> drm_device and drm_client (which require lots of jumping through
>>>>> hoops in
>>>>> your v1 to make sure you can break those reference loops).
>>>>>
>>>>>> - fbdev: Use a shadow buffer for framebuffers that have a dirty
>>>>>>     callback. This makes the fbdev client truly generic and
>>>>>> useable for all
>>>>>>     drivers. There's a blitting penalty, but this is generic
>>>>>> emulation after
>>>>>>     all. The reason for needing a shadow buffer is that deferred
>>>>>> I/O only
>>>>>>     works with kmalloc/vmalloc buffers and not with shmem buffers
>>>>>>     (page->lru/mapping).
>>>>> Yeah I think that's the only feasible option. Everyone who cares more
>>>>> about fbdev performance can keep their driver-specific code. And for
>>>>> other
>>>>> drm_client users this shouldn't be a problem, since they know how 
>>>>> to use
>>>>> dirty and flipping between multiple buffers to drive kms as it was
>>>>> designed. The issue really only exists for fbdev's assumption of a
>>>>> direct
>>>>> mmap of a dumb framebuffer, encoded into the uapi.
>>>>>
>>>>>> - Let tinydrm use the full fbdev client
>>>>> \o/
>>>>>
>>>>> Cheers, Daniel
>>>>>> Noralf.
>>>>>>
>>>>>> Changes since version 1:
>>>>>> - Make it possible to embed struct drm_client_dev and drop the 
>>>>>> private
>>>>>>     pointer
>>>>>> - Use kref reference counting to control client release since 
>>>>>> both the
>>>>>>     client and the driver can release.
>>>>>> - Add comment about using dma-buf as a possibility with some rework
>>>>>> - Move buffer NULL check to drm_client_framebuffer_delete()
>>>>>> - Move client name to struct drm_client_dev
>>>>>> - Move up drm_dev_get/put calls to make them more visible
>>>>>> - Move drm_client_dev.list definition to later patch that makes
>>>>>> use of it
>>>>>>
>>>>>> - Embed drm_client at the beginning of drm_fb_helper to avoid a 
>>>>>> fragile
>>>>>>     transitional kfree hack in drm_client_release()
>>>>>> - Set owner in drm_fbdev_fb_ops
>>>>>> - Add kerneldoc to drm_fb_helper_generic_probe()
>>>>>>
>>>>>> - Remove unused functions
>>>>>> - Change name drm_client_funcs.lastclose -> .restore
>>>>>> - Change name drm_client_funcs.remove -> .unregister
>>>>>> - Rework unregister code
>>>>>>
>>>>>> - tinydrm: Use drm_fbdev_generic_setup() and remove
>>>>>>     drm_fb_cma_fbdev_init_with_funcs()
>>>>>>
>>>>>> David Herrmann (1):
>>>>>>     drm: provide management functions for drm_file
>>>>>>
>>>>>> Noralf Trønnes (11):
>>>>>>     drm/file: Don't set master on in-kernel clients
>>>>>>     drm: Make ioctls available for in-kernel clients
>>>>>>     drm: Begin an API for in-kernel clients
>>>>>>     drm/fb-helper: Add generic fbdev emulation .fb_probe function
>>>>>>     drm/pl111: Set .gem_prime_vmap and .gem_prime_mmap
>>>>>>     drm/cma-helper: Use the generic fbdev emulation
>>>>>>     drm/client: Add client callbacks
>>>>>>     drm/debugfs: Add internal client debugfs file
>>>>>>     drm/fb-helper: Finish the generic fbdev emulation
>>>>>>     drm/tinydrm: Use drm_fbdev_generic_setup()
>>>>>>     drm/cma-helper: Remove drm_fb_cma_fbdev_init_with_funcs()
>>>>>>
>>>>>>    Documentation/gpu/drm-client.rst            |  12 +
>>>>>>    Documentation/gpu/index.rst                 |   1 +
>>>>>>    drivers/gpu/drm/Makefile                    |   2 +-
>>>>>>    drivers/gpu/drm/drm_client.c                | 435
>>>>>> ++++++++++++++++++++++++++++
>>>>>>    drivers/gpu/drm/drm_crtc_internal.h         |  19 +-
>>>>>>    drivers/gpu/drm/drm_debugfs.c               |   7 +
>>>>>>    drivers/gpu/drm/drm_drv.c                   |   8 +
>>>>>>    drivers/gpu/drm/drm_dumb_buffers.c          |  33 ++-
>>>>>>    drivers/gpu/drm/drm_fb_cma_helper.c         | 380
>>>>>> +++---------------------
>>>>>>    drivers/gpu/drm/drm_fb_helper.c             | 330
>>>>>> ++++++++++++++++++++-
>>>>>>    drivers/gpu/drm/drm_file.c                  | 304 
>>>>>> ++++++++++---------
>>>>>>    drivers/gpu/drm/drm_framebuffer.c           |  42 ++-
>>>>>>    drivers/gpu/drm/drm_internal.h              |   2 +
>>>>>>    drivers/gpu/drm/drm_ioctl.c                 |   4 +-
>>>>>>    drivers/gpu/drm/drm_probe_helper.c          |   3 +
>>>>>>    drivers/gpu/drm/pl111/pl111_drv.c           |   2 +
>>>>>>    drivers/gpu/drm/tinydrm/core/tinydrm-core.c |   3 +-
>>>>>>    drivers/gpu/drm/tinydrm/ili9225.c           |   1 -
>>>>>>    drivers/gpu/drm/tinydrm/mi0283qt.c          |   1 -
>>>>>>    drivers/gpu/drm/tinydrm/st7586.c            |   1 -
>>>>>>    drivers/gpu/drm/tinydrm/st7735r.c           |   1 -
>>>>>>    include/drm/drm_client.h                    | 156 ++++++++++
>>>>>>    include/drm/drm_device.h                    |  21 ++
>>>>>>    include/drm/drm_fb_cma_helper.h             |   6 -
>>>>>>    include/drm/drm_fb_helper.h                 |  38 +++
>>>>>>    25 files changed, 1298 insertions(+), 514 deletions(-)
>>>>>>    create mode 100644 Documentation/gpu/drm-client.rst
>>>>>>    create mode 100644 drivers/gpu/drm/drm_client.c
>>>>>>    create mode 100644 include/drm/drm_client.h
>>>>>>
>>>>>> -- 
>>>>>> 2.15.1
>>>>>>
>>>>>> _______________________________________________
>>>>>> Intel-gfx mailing list
>>>>>> Intel-gfx@lists.freedesktop.org
>>>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 00/12] drm: Add generic fbdev emulation

[Daniel Vetter]

On Tue, Jun 26, 2018 at 3:41 PM, Noralf Trønnes <noralf@tronnes.org> wrote:
>
> Den 21.06.2018 19.19, skrev Noralf Trønnes:
>>
>>
>> Den 21.06.2018 09.14, skrev Daniel Vetter:
>>>
>>> On Wed, Jun 20, 2018 at 05:28:10PM +0200, Noralf Trønnes wrote:
>>>>
>>>> Den 20.06.2018 15.52, skrev Noralf Trønnes:
>>>>>
>>>>> Den 20.06.2018 11.34, skrev Daniel Vetter:
>>>>>>
>>>>>> On Mon, Jun 18, 2018 at 04:17:27PM +0200, Noralf Trønnes wrote:
>>>>>>>
>>>>>>> This patchset adds generic fbdev emulation for drivers that
>>>>>>> supports GEM
>>>>>>> based dumb buffers which support .gem_prime_vmap and gem_prime_mmap.
>>>>>>> An
>>>>>>> API is begun to support in-kernel clients in general.
>>>>>>>
>>>>>>> Notable changes since version 1:
>>>>>>>
>>>>>>> - Rework client unregister code. I've used reference counting to
>>>>>>> manage
>>>>>>>     the fact that both the client itself and the driver through
>>>>>>>     drm_dev_unregister() can release the client. The client is
>>>>>>> now released
>>>>>>>     using drm_client_put() instead of drm_client_free().
>>>>>>
>>>>>> I started reviewing the reworked client register/unregister stuff,
>>>>>> and it
>>>>>> looks good, except this kref stuff here for clients. I don't
>>>>>> understand
>>>>>> why you need this - as long as removal from dev->clientlist is
>>>>>> properly
>>>>>> protected by the mutex, I don't see how both the client and the
>>>>>> device can
>>>>>> release the client at the same time. Of course this means that both
>>>>>> the
>>>>>> device-trigger unregister and the client-triggered unregister must
>>>>>> first
>>>>>> grab the mutex, remove the client from the list, and only if that
>>>>>> was done
>>>>>> succesfully, clean up the client. If the client is already removed
>>>>>> from
>>>>>> the list (i.e. list_empty() is true) then you need to bail out to
>>>>>> avoid
>>>>>> double-freeing.
>>>>>
>>>>> I just suck at this :/
>>>>>
>>>>> Use case:
>>>>> Unloading client module at the same time as the device is unplugged.
>>>
>>> Do we really want to be able to unload client modules? Atm you can't
>>> unload the drm fbdev emulation either while a driver is still using it.
>>> Dropping that requirement would make things even simpler (you'd just need
>>> to add an owner field to drm_client and a try_module_get when registering
>>> it, bailing out if that fails).
>>>
>>> What's the use-case you have in mind that requires that you can unload a
>>> client driver module? Does that even work with the shuffling we've done
>>> on
>>> the register side of things?
>>
>>
>> When I first started on this client API, the client could unload itself
>> and I had a sysfs file that would remove clients for a particular
>> drm_device. This would mean that unloading a driver would require clients
>> to be removed first by writing to the sysfs file.
>> Then I started to look at the possibility that the driver could remove
>> clients automatically on driver unload. I have wrecked my brain trying to
>> make it race free, but it gets very complicated as you have shown in your
>> example. So I think we'll just avoid this complexity altogether.
>>
>> So, now the question is, who gets to remove clients. The client or the
>> driver?
>> The common pattern is that clients can come and go on their own choosing.
>> It's what I would expect, that the client can be unloaded.
>> The reason I looked into auto unloading when the driver where going away,
>> was so developers shouldn't have to do the extra step to unload the
>> driver.
>>
>> Now I see a third case, and that's plug/unplug USB devices. If the driver
>> can't remove the client, we can end up with lots hanging drm_device and
>> drm_client_dev instances that have to be removed manually, which is
>> really not an option.
>>
>> So I think we remove clients on driver/device removal. This means that to
>> unload a client, the driver(s) would have to be unloaded first.
>>
>> I'll add an owner field to drm_client_funcs as you suggested and work out
>> the details.
>
>
> Currently drm_client_dev->funcs is optional, but should it be mandatory
> now that drm_client_funcs gets an owner field?

I think because of the unregister callback it makes little sense to
have clients without a function table. But you can also make the
try_module_get conditional on drm_client->funcs, if that makes more
sense to you. No opinion on my side really.
-Daniel

>
> Noralf.
>
>
>>
>> Thanks for helping me get this as simple and straightforward as possible.
>>
>> Noralf.
>>
>>>>> The client module calls drm_client_release():
>>>>>
>>>>> void drm_client_release(struct drm_client_dev *client)
>>>>> {
>>>>>      struct drm_device *dev = client->dev;
>>>
>>> Not sure (without reading your patches again) why we have
>>> drm_client_close
>>> here and ->unregister/drm_client_release below. But the trick is roughly
>>> to do
>>>     client = NULL;
>>>
>>>     mutex_lock();
>>>     client = find_it();
>>>     if (client)
>>>         list_del();
>>>     mute_unlock();
>>>
>>>     if (!client)
>>>         continue; /* or early return, whatever makes sense */
>>>
>>>     drm_client_unregister(client);
>>>
>>> This way if you race there's:
>>> - only one thread will win, since the removal from the list is locked
>>> - no deadlocks, because the actual cleanup is done outside of the locks
>>>
>>> The problem is applying this trick to each situation, since you need to
>>> make sure that you get them all. Since you want to be able to unregister
>>> from 2 different lists, with each their own locks, you need to nest the
>>> above pattern in clever ways. In the client unregister function:
>>>
>>>     mutex_lock(fbdev_clients_lock);
>>>     client = list_first(fbdev_clients_list);
>>>     if (client)
>>>         list_del();
>>>
>>>     mutex_lock(client->dev);
>>>     if (!list_empty(client->dev_list))
>>>         list_del();
>>>     else
>>>         /* someone else raced us, give up */
>>>         client = NULL;
>>>     mutex_unlock(client->dev);
>>>     mutex_unlock(fbdev_clients_lock);
>>>
>>>     if (!client)
>>>         continue; /* or early return, whatever makes sense */
>>>
>>>     drm_client_unregister(client);
>>>
>>> This way you know that as long as you hold the fbdevs_clients_lock client
>>> can't disappear, so you can look at client->dev (which also won't
>>> disappear, because the client can't disappear), which allows you to take
>>> the per-device client look to check whether you've race with removing.
>>>
>>> On the per-device client remove function we can't just do the same trick,
>>> because that would be a locking inversion. Instead we need careful
>>> ordering:
>>>
>>>
>>>     mutex_lock(client->dev);
>>>     if (!list_empty(client->dev_list))
>>>         list_del();
>>>     else
>>>         /* someone else raced us, give up */
>>>         client = NULL;
>>>     mutex_unlock(client->dev);
>>>
>>>     if (!client)
>>>         continue; /* or early return, whatever makes sense */
>>>
>>>     /* we've won the race and must do the cleanup, but first we need
>>>      * to stop use-after-free */
>>>
>>>     mutex_lock(fbdev_clients_lock);
>>>     if (!list_empty(client->fbdev_list))
>>>         list_del();
>>>     else
>>>         /* we raced and the other thread did the list removal
>>>          * already, but will have backed off by now */
>>>     mutex_unlock(fbdev_clients_lock);
>>>
>>>     /* no one can get at the client structure anymore, it's safe to
>>>      * clean it up */
>>>
>>>     drm_client_unregister(client);
>>>
>>> Lots of complexity for a feature we didn't have yet and that I don't
>>> think
>>> we need really, but it is doable :-)
>>>
>>>>> mutex_lock(&dev->clientlist_mutex);
>>>>>      list_del(&client->list);
>>>>>      drm_client_close(client);
>>>>>      mutex_unlock(&dev->clientlist_mutex);
>>>>>      drm_dev_put(dev);
>>>>> }
>>>>>
>>>>>
>>>>> drm_device_unregister() calls drm_client_dev_unregister():
>>>>>
>>>>> void drm_client_dev_unregister(struct drm_device *dev)
>>>>> {
>>>>>      struct drm_client_dev *client;
>>>>>
>>>>>      mutex_lock(&dev->clientlist_mutex);
>>>>>      list_for_each_entry(client, &dev->clientlist, list) {
>>>>>          if (client->funcs && client->funcs->unregister)
>>>>>              client->funcs->unregister(client);
>>>>>          else
>>>>>              drm_client_release(client);
>>>>>      }
>>>>>      mutex_unlock(&dev->clientlist_mutex);
>>>>> }
>>>>>
>>>>>
>>>>> How do I do this without deadlocking and without operating on a
>>>>> drm_client_dev structure that has been freed in the other codepath?
>>>>>
>>>> There's one more quirk that I forgot:
>>>> If fbdev can't release the client on .unregister due to open fd's, the
>>>> list entry should be removed but releasing resources is deferred to
>>>> the last fd being closed.
>>>
>>> For fbdev I think kref'ing it makes sense. But probably better to do that
>>> in the structure that contains the drm_client, since I think this is very
>>> much an fbdev problem, not a general drm_client problem.
>>>
>>> Cheers, Daniel
>>>
>>>> Noralf.
>>>>
>>>>> Noralf.
>>>>>
>>>>>> I don't think there's a need to use a kref here. And kref has the
>>>>>> tricky
>>>>>> issue that you tempt everyone into constructing references loops
>>>>>> between
>>>>>> drm_device and drm_client (which require lots of jumping through
>>>>>> hoops in
>>>>>> your v1 to make sure you can break those reference loops).
>>>>>>
>>>>>>> - fbdev: Use a shadow buffer for framebuffers that have a dirty
>>>>>>>     callback. This makes the fbdev client truly generic and
>>>>>>> useable for all
>>>>>>>     drivers. There's a blitting penalty, but this is generic
>>>>>>> emulation after
>>>>>>>     all. The reason for needing a shadow buffer is that deferred
>>>>>>> I/O only
>>>>>>>     works with kmalloc/vmalloc buffers and not with shmem buffers
>>>>>>>     (page->lru/mapping).
>>>>>>
>>>>>> Yeah I think that's the only feasible option. Everyone who cares more
>>>>>> about fbdev performance can keep their driver-specific code. And for
>>>>>> other
>>>>>> drm_client users this shouldn't be a problem, since they know how to
>>>>>> use
>>>>>> dirty and flipping between multiple buffers to drive kms as it was
>>>>>> designed. The issue really only exists for fbdev's assumption of a
>>>>>> direct
>>>>>> mmap of a dumb framebuffer, encoded into the uapi.
>>>>>>
>>>>>>> - Let tinydrm use the full fbdev client
>>>>>>
>>>>>> \o/
>>>>>>
>>>>>> Cheers, Daniel
>>>>>>>
>>>>>>> Noralf.
>>>>>>>
>>>>>>> Changes since version 1:
>>>>>>> - Make it possible to embed struct drm_client_dev and drop the
>>>>>>> private
>>>>>>>     pointer
>>>>>>> - Use kref reference counting to control client release since both
>>>>>>> the
>>>>>>>     client and the driver can release.
>>>>>>> - Add comment about using dma-buf as a possibility with some rework
>>>>>>> - Move buffer NULL check to drm_client_framebuffer_delete()
>>>>>>> - Move client name to struct drm_client_dev
>>>>>>> - Move up drm_dev_get/put calls to make them more visible
>>>>>>> - Move drm_client_dev.list definition to later patch that makes
>>>>>>> use of it
>>>>>>>
>>>>>>> - Embed drm_client at the beginning of drm_fb_helper to avoid a
>>>>>>> fragile
>>>>>>>     transitional kfree hack in drm_client_release()
>>>>>>> - Set owner in drm_fbdev_fb_ops
>>>>>>> - Add kerneldoc to drm_fb_helper_generic_probe()
>>>>>>>
>>>>>>> - Remove unused functions
>>>>>>> - Change name drm_client_funcs.lastclose -> .restore
>>>>>>> - Change name drm_client_funcs.remove -> .unregister
>>>>>>> - Rework unregister code
>>>>>>>
>>>>>>> - tinydrm: Use drm_fbdev_generic_setup() and remove
>>>>>>>     drm_fb_cma_fbdev_init_with_funcs()
>>>>>>>
>>>>>>> David Herrmann (1):
>>>>>>>     drm: provide management functions for drm_file
>>>>>>>
>>>>>>> Noralf Trønnes (11):
>>>>>>>     drm/file: Don't set master on in-kernel clients
>>>>>>>     drm: Make ioctls available for in-kernel clients
>>>>>>>     drm: Begin an API for in-kernel clients
>>>>>>>     drm/fb-helper: Add generic fbdev emulation .fb_probe function
>>>>>>>     drm/pl111: Set .gem_prime_vmap and .gem_prime_mmap
>>>>>>>     drm/cma-helper: Use the generic fbdev emulation
>>>>>>>     drm/client: Add client callbacks
>>>>>>>     drm/debugfs: Add internal client debugfs file
>>>>>>>     drm/fb-helper: Finish the generic fbdev emulation
>>>>>>>     drm/tinydrm: Use drm_fbdev_generic_setup()
>>>>>>>     drm/cma-helper: Remove drm_fb_cma_fbdev_init_with_funcs()
>>>>>>>
>>>>>>>    Documentation/gpu/drm-client.rst            |  12 +
>>>>>>>    Documentation/gpu/index.rst                 |   1 +
>>>>>>>    drivers/gpu/drm/Makefile                    |   2 +-
>>>>>>>    drivers/gpu/drm/drm_client.c                | 435
>>>>>>> ++++++++++++++++++++++++++++
>>>>>>>    drivers/gpu/drm/drm_crtc_internal.h         |  19 +-
>>>>>>>    drivers/gpu/drm/drm_debugfs.c               |   7 +
>>>>>>>    drivers/gpu/drm/drm_drv.c                   |   8 +
>>>>>>>    drivers/gpu/drm/drm_dumb_buffers.c          |  33 ++-
>>>>>>>    drivers/gpu/drm/drm_fb_cma_helper.c         | 380
>>>>>>> +++---------------------
>>>>>>>    drivers/gpu/drm/drm_fb_helper.c             | 330
>>>>>>> ++++++++++++++++++++-
>>>>>>>    drivers/gpu/drm/drm_file.c                  | 304
>>>>>>> ++++++++++---------
>>>>>>>    drivers/gpu/drm/drm_framebuffer.c           |  42 ++-
>>>>>>>    drivers/gpu/drm/drm_internal.h              |   2 +
>>>>>>>    drivers/gpu/drm/drm_ioctl.c                 |   4 +-
>>>>>>>    drivers/gpu/drm/drm_probe_helper.c          |   3 +
>>>>>>>    drivers/gpu/drm/pl111/pl111_drv.c           |   2 +
>>>>>>>    drivers/gpu/drm/tinydrm/core/tinydrm-core.c |   3 +-
>>>>>>>    drivers/gpu/drm/tinydrm/ili9225.c           |   1 -
>>>>>>>    drivers/gpu/drm/tinydrm/mi0283qt.c          |   1 -
>>>>>>>    drivers/gpu/drm/tinydrm/st7586.c            |   1 -
>>>>>>>    drivers/gpu/drm/tinydrm/st7735r.c           |   1 -
>>>>>>>    include/drm/drm_client.h                    | 156 ++++++++++
>>>>>>>    include/drm/drm_device.h                    |  21 ++
>>>>>>>    include/drm/drm_fb_cma_helper.h             |   6 -
>>>>>>>    include/drm/drm_fb_helper.h                 |  38 +++
>>>>>>>    25 files changed, 1298 insertions(+), 514 deletions(-)
>>>>>>>    create mode 100644 Documentation/gpu/drm-client.rst
>>>>>>>    create mode 100644 drivers/gpu/drm/drm_client.c
>>>>>>>    create mode 100644 include/drm/drm_client.h
>>>>>>>
>>>>>>> --
>>>>>>> 2.15.1
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> Intel-gfx mailing list
>>>>>>> Intel-gfx@lists.freedesktop.org
>>>>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>>>
>>>>> _______________________________________________
>>>>> dri-devel mailing list
>>>>> dri-devel@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>



-- 
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