[RFC 0/7] drm: Add shmem GEM library

[RFC 0/7] drm: Add shmem GEM library

[Noralf Trønnes]

This patchset adds a library for shmem backed GEM objects.
I'm putting out an rfc to find out if I'm on the right track with this.
The shmem library is very similar to the cma library, so I have tried
to pull out the common bits.

A couple of questions:
- What should the default cache mode be?
- Do I need locking around vmap/vunmap?
  See TODO in [RFC 5/7] drm: Add library for shmem backed GEM objects

Noralf.


Noralf Trønnes (7):
  drm/gem: Add drm_gem_dumb_map_offset()
  drm: Add GEM backed framebuffer library
  drm/fb-helper: Support shadow buffer with deferred io
  drm/fb-helper: Add simple init/fini functions
  drm: Add library for shmem backed GEM objects
  drm: Add kms library for shmem backed GEM
  drm/tinydrm: Switch from CMA to shmem buffers

 drivers/gpu/drm/Kconfig                     |  17 +
 drivers/gpu/drm/Makefile                    |   4 +-
 drivers/gpu/drm/drm_fb_gem_helper.c         | 248 +++++++++++
 drivers/gpu/drm/drm_fb_helper.c             | 229 +++++++++-
 drivers/gpu/drm/drm_fb_shmem_helper.c       | 168 ++++++++
 drivers/gpu/drm/drm_gem.c                   |  35 ++
 drivers/gpu/drm/drm_gem_shmem_helper.c      | 628 ++++++++++++++++++++++++++++
 drivers/gpu/drm/tinydrm/Kconfig             |   2 +-
 drivers/gpu/drm/tinydrm/core/tinydrm-core.c | 147 +++----
 drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c |   4 +-
 drivers/gpu/drm/tinydrm/mi0283qt.c          |   2 +-
 drivers/gpu/drm/tinydrm/mipi-dbi.c          |  41 +-
 include/drm/drm_fb_gem_helper.h             |  64 +++
 include/drm/drm_fb_helper.h                 |  50 +++
 include/drm/drm_fb_shmem_helper.h           |  18 +
 include/drm/drm_gem.h                       |   2 +
 include/drm/drm_gem_shmem_helper.h          | 131 ++++++
 include/drm/tinydrm/tinydrm.h               |  32 +-
 18 files changed, 1683 insertions(+), 139 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_fb_gem_helper.c
 create mode 100644 drivers/gpu/drm/drm_fb_shmem_helper.c
 create mode 100644 drivers/gpu/drm/drm_gem_shmem_helper.c
 create mode 100644 include/drm/drm_fb_gem_helper.h
 create mode 100644 include/drm/drm_fb_shmem_helper.h
 create mode 100644 include/drm/drm_gem_shmem_helper.h

--
2.7.4

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

[RFC 1/7] drm/gem: Add drm_gem_dumb_map_offset()

[Noralf Trønnes]

Add a common drm_driver.dumb_map_offset function for GEM backed drivers.

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

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 8dc1106..44ecbaa 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -311,6 +311,41 @@ drm_gem_handle_delete(struct drm_file *filp, u32 handle)
 EXPORT_SYMBOL(drm_gem_handle_delete);
 
 /**
+ * drm_gem_dumb_map_offset - return the fake mmap offset for a gem object
+ * @file: drm file-private structure containing the gem object
+ * @dev: corresponding drm_device
+ * @handle: gem object handle
+ * @offset: return location for the fake mmap offset
+ *
+ * This implements the &drm_driver.dumb_map_offset kms driver callback for
+ * drivers which use gem to manage their backing storage.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ */
+int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
+			    u32 handle, u64 *offset)
+{
+	struct drm_gem_object *obj;
+	int ret;
+
+	obj = drm_gem_object_lookup(file, handle);
+	if (!obj)
+		return -ENOENT;
+
+	ret = drm_gem_create_mmap_offset(obj);
+	if (ret)
+		goto out;
+
+	*offset = drm_vma_node_offset_addr(&obj->vma_node);
+out:
+	drm_gem_object_put_unlocked(obj);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(drm_gem_dumb_map_offset);
+
+/**
  * drm_gem_dumb_destroy - dumb fb callback helper for gem based drivers
  * @file: drm file-private structure to remove the dumb handle from
  * @dev: corresponding drm_device
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index 4a9d231..9c55c2a 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -302,6 +302,8 @@ void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages,
 		bool dirty, bool accessed);
 
 struct drm_gem_object *drm_gem_object_lookup(struct drm_file *filp, u32 handle);
+int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
+			    u32 handle, u64 *offset);
 int drm_gem_dumb_destroy(struct drm_file *file,
 			 struct drm_device *dev,
 			 uint32_t handle);
-- 
2.7.4

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

[RFC 2/7] drm: Add GEM backed framebuffer library

[Noralf Trønnes]

Add a library for drivers that can use a simple representation
of a GEM backed framebuffer.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/Makefile            |   2 +-
 drivers/gpu/drm/drm_fb_gem_helper.c | 248 ++++++++++++++++++++++++++++++++++++
 include/drm/drm_fb_gem_helper.h     |  64 ++++++++++
 3 files changed, 313 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/drm_fb_gem_helper.c
 create mode 100644 include/drm/drm_fb_gem_helper.h

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 24a066e..83d8b09 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -33,7 +33,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
 		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
 		drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
 		drm_simple_kms_helper.o drm_modeset_helper.o \
-		drm_scdc_helper.o
+		drm_scdc_helper.o drm_fb_gem_helper.o
 
 drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o
 drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
diff --git a/drivers/gpu/drm/drm_fb_gem_helper.c b/drivers/gpu/drm/drm_fb_gem_helper.c
new file mode 100644
index 0000000..9a0da09
--- /dev/null
+++ b/drivers/gpu/drm/drm_fb_gem_helper.c
@@ -0,0 +1,248 @@
+/*
+ * drm fb gem helper functions
+ *
+ * Copyright (C) 2017 Noralf Trønnes
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/dma-buf.h>
+#include <linux/dma-mapping.h>
+#include <linux/module.h>
+#include <linux/reservation.h>
+
+#include <drm/drmP.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_fb_helper.h>
+#include <drm/drm_fb_gem_helper.h>
+#include <drm/drm_gem.h>
+
+/**
+ * drm_fb_gem_get_obj() - Get GEM object for framebuffer
+ * @fb: The framebuffer
+ * @plane: Which plane
+ *
+ * Returns the GEM object for given framebuffer.
+ */
+struct drm_gem_object *drm_fb_gem_get_obj(struct drm_framebuffer *fb,
+					  unsigned int plane)
+{
+	struct drm_fb_gem *fb_gem = to_fb_gem(fb);
+
+	if (plane >= 4)
+		return NULL;
+
+	return fb_gem->obj[plane];
+}
+EXPORT_SYMBOL_GPL(drm_fb_gem_get_obj);
+
+/**
+ * drm_fb_gem_alloc - Allocate GEM backed framebuffer
+ * @dev: DRM device
+ * @mode_cmd: metadata from the userspace fb creation request
+ * @obj: GEM object nacking the framebuffer
+ * @num_planes: Number of planes
+ * @funcs: vtable to be used for the new framebuffer object
+ *
+ * Returns:
+ * Allocated struct drm_fb_gem * or error encoded pointer.
+ */
+struct drm_fb_gem *
+drm_fb_gem_alloc(struct drm_device *dev,
+		 const struct drm_mode_fb_cmd2 *mode_cmd,
+		 struct drm_gem_object **obj, unsigned int num_planes,
+		 const struct drm_framebuffer_funcs *funcs)
+{
+	struct drm_fb_gem *fb_gem;
+	int ret, i;
+
+	fb_gem = kzalloc(sizeof(*fb_gem), GFP_KERNEL);
+	if (!fb_gem)
+		return ERR_PTR(-ENOMEM);
+
+	drm_helper_mode_fill_fb_struct(dev, &fb_gem->base, mode_cmd);
+
+	for (i = 0; i < num_planes; i++)
+		fb_gem->obj[i] = obj[i];
+
+	ret = drm_framebuffer_init(dev, &fb_gem->base, funcs);
+	if (ret) {
+		dev_err(dev->dev, "Failed to initialize framebuffer: %d\n", ret);
+		kfree(fb_gem);
+		return ERR_PTR(ret);
+	}
+
+	return fb_gem;
+}
+EXPORT_SYMBOL(drm_fb_gem_alloc);
+
+/**
+ * drm_fb_gem_destroy - Free GEM backed framebuffer
+ * @fb: DRM framebuffer
+ *
+ * Frees a GEM backed framebuffer with it's backing buffer(s) and the structure
+ * itself. Drivers can use this as their &drm_framebuffer_funcs->destroy
+ * callback.
+ */
+void drm_fb_gem_destroy(struct drm_framebuffer *fb)
+{
+	struct drm_fb_gem *fb_gem = to_fb_gem(fb);
+	int i;
+
+	for (i = 0; i < 4; i++) {
+		if (fb_gem->obj[i])
+			drm_gem_object_put_unlocked(fb_gem->obj[i]);
+	}
+
+	drm_framebuffer_cleanup(fb);
+	kfree(fb_gem);
+}
+EXPORT_SYMBOL(drm_fb_gem_destroy);
+
+/**
+ * drm_fb_gem_create_handle - Create handle for GEM backed framebuffer
+ * @fb: DRM framebuffer
+ * @file: drm file
+ * @handle: handle created
+ *
+ * Drivers can use this as their &drm_framebuffer_funcs->create_handle
+ * callback.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ */
+int drm_fb_gem_create_handle(struct drm_framebuffer *fb, struct drm_file *file,
+			     unsigned int *handle)
+{
+	struct drm_fb_gem *fb_gem = to_fb_gem(fb);
+
+	return drm_gem_handle_create(file, fb_gem->obj[0], handle);
+}
+EXPORT_SYMBOL(drm_fb_gem_create_handle);
+
+/**
+ * drm_fb_gem_create_with_funcs() - helper function for the
+ *                                  &drm_mode_config_funcs.fb_create
+ *                                  callback
+ * @dev: DRM device
+ * @file: drm file for the ioctl call
+ * @mode_cmd: metadata from the userspace fb creation request
+ * @funcs: vtable to be used for the new framebuffer object
+ *
+ * This can be used to set &drm_framebuffer_funcs for drivers that need the
+ * &drm_framebuffer_funcs.dirty callback. Use drm_fb_gem_create() if you don't
+ * need to change &drm_framebuffer_funcs.
+ */
+struct drm_framebuffer *
+drm_fb_gem_create_with_funcs(struct drm_device *dev, struct drm_file *file,
+			     const struct drm_mode_fb_cmd2 *mode_cmd,
+			     const struct drm_framebuffer_funcs *funcs)
+{
+	const struct drm_format_info *info;
+	struct drm_gem_object *objs[4];
+	struct drm_fb_gem *fb_gem;
+	int ret, i;
+
+	info = drm_get_format_info(dev, mode_cmd);
+	if (!info)
+		return ERR_PTR(-EINVAL);
+
+	for (i = 0; i < info->num_planes; i++) {
+		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
+		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
+		unsigned int min_size;
+
+		objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]);
+		if (!objs[i]) {
+			dev_err(dev->dev, "Failed to lookup GEM object\n");
+			ret = -ENOENT;
+			goto err_gem_object_put;
+		}
+
+		min_size = (height - 1) * mode_cmd->pitches[i]
+			 + width * info->cpp[i]
+			 + mode_cmd->offsets[i];
+
+		if (objs[i]->size < min_size) {
+			drm_gem_object_put_unlocked(objs[i]);
+			ret = -EINVAL;
+			goto err_gem_object_put;
+		}
+	}
+
+	fb_gem = drm_fb_gem_alloc(dev, mode_cmd, objs, i, funcs);
+	if (IS_ERR(fb_gem)) {
+		ret = PTR_ERR(fb_gem);
+		goto err_gem_object_put;
+	}
+
+	return &fb_gem->base;
+
+err_gem_object_put:
+	for (i--; i >= 0; i--)
+		drm_gem_object_put_unlocked(objs[i]);
+
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(drm_fb_gem_create_with_funcs);
+
+static struct drm_framebuffer_funcs drm_fb_gem_fb_funcs = {
+	.destroy	= drm_fb_gem_destroy,
+	.create_handle	= drm_fb_gem_create_handle,
+};
+
+/**
+ * drm_fb_gem_create() - &drm_mode_config_funcs.fb_create callback function
+ * @dev: DRM device
+ * @file: drm file for the ioctl call
+ * @mode_cmd: metadata from the userspace fb creation request
+ *
+ * If your hardware has special alignment or pitch requirements these should be
+ * checked before calling this function. Use drm_fb_gem_create_with_funcs() if
+ * you need to set &drm_framebuffer_funcs.dirty.
+ */
+struct drm_framebuffer *
+drm_fb_gem_create(struct drm_device *dev, struct drm_file *file,
+		  const struct drm_mode_fb_cmd2 *mode_cmd)
+{
+	return drm_fb_gem_create_with_funcs(dev, file, mode_cmd,
+					    &drm_fb_gem_fb_funcs);
+}
+EXPORT_SYMBOL_GPL(drm_fb_gem_create);
+
+/**
+ * drm_fb_gem_prepare_fb() - Prepare gem framebuffer
+ * @plane: Which plane
+ * @state: Plane state attach fence to
+ *
+ * This should be set as the &struct drm_plane_helper_funcs.prepare_fb hook.
+ *
+ * This function checks if the plane FB has an dma-buf attached, extracts
+ * the exclusive fence and attaches it to plane state for the atomic helper
+ * to wait on.
+ *
+ * There is no need for cleanup_fb for gem based framebuffer drivers.
+ */
+int drm_fb_gem_prepare_fb(struct drm_plane *plane,
+			  struct drm_plane_state *state)
+{
+	struct dma_buf *dma_buf;
+	struct dma_fence *fence;
+
+	if ((plane->state->fb == state->fb) || !state->fb)
+		return 0;
+
+	dma_buf = drm_fb_gem_get_obj(state->fb, 0)->dma_buf;
+	if (dma_buf) {
+		fence = reservation_object_get_excl_rcu(dma_buf->resv);
+		drm_atomic_set_fence_for_plane(state, fence);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(drm_fb_gem_prepare_fb);
diff --git a/include/drm/drm_fb_gem_helper.h b/include/drm/drm_fb_gem_helper.h
new file mode 100644
index 0000000..405a1e1
--- /dev/null
+++ b/include/drm/drm_fb_gem_helper.h
@@ -0,0 +1,64 @@
+#ifndef __DRM_FB_GEM_HELPER_H__
+#define __DRM_FB_GEM_HELPER_H__
+
+#include <drm/drm_framebuffer.h>
+
+struct drm_gem_shmem_object;
+struct drm_mode_fb_cmd2;
+struct drm_plane;
+struct drm_plane_state;
+
+/**
+ * struct drm_fb_gem - GEM backed framebuffer
+ */
+struct drm_fb_gem {
+	/**
+	 * @base: Base DRM framebuffer
+	 */
+	struct drm_framebuffer base;
+	/**
+	 * @obj: GEM object array backing the framebuffer. One object per
+	 * plane.
+	 */
+	struct drm_gem_object *obj[4];
+};
+
+static inline struct drm_fb_gem *to_fb_gem(struct drm_framebuffer *fb)
+{
+	return container_of(fb, struct drm_fb_gem, base);
+}
+
+struct drm_gem_object *drm_fb_gem_get_obj(struct drm_framebuffer *fb,
+					  unsigned int plane);
+struct drm_fb_gem *
+drm_fb_gem_alloc(struct drm_device *dev,
+		 const struct drm_mode_fb_cmd2 *mode_cmd,
+		 struct drm_gem_object **obj, unsigned int num_planes,
+		 const struct drm_framebuffer_funcs *funcs);
+void drm_fb_gem_destroy(struct drm_framebuffer *fb);
+int drm_fb_gem_create_handle(struct drm_framebuffer *fb, struct drm_file *file,
+			     unsigned int *handle);
+
+struct drm_framebuffer *
+drm_fb_gem_create_with_funcs(struct drm_device *dev, struct drm_file *file,
+			     const struct drm_mode_fb_cmd2 *mode_cmd,
+			     const struct drm_framebuffer_funcs *funcs);
+struct drm_framebuffer *
+drm_fb_gem_create(struct drm_device *dev, struct drm_file *file,
+		  const struct drm_mode_fb_cmd2 *mode_cmd);
+
+
+int drm_fb_gem_prepare_fb(struct drm_plane *plane,
+			  struct drm_plane_state *state);
+
+
+
+
+#ifdef CONFIG_DEBUG_FS
+struct seq_file;
+
+int drm_fb_gem_debugfs_show(struct seq_file *m, void *arg);
+#endif
+
+#endif
+
-- 
2.7.4

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

[RFC 3/7] drm/fb-helper: Support shadow buffer with deferred io

[Noralf Trønnes]

This adds support for using a shadow buffer for fbdev that is
copied to the real buffer during dirty flushing.
shmem buffers doesn't work with the fbdev deferred io functions,
because it touches page->mapping and page->lru.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/drm_fb_helper.c | 21 +++++++++++++++++++--
 include/drm/drm_fb_helper.h     |  8 ++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 721511d..d8b2690 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -739,8 +739,25 @@ 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)
-		helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip_copy, 1);
+	if (clip_copy.x1 > clip_copy.x2 || clip_copy.y1 > clip_copy.y2)
+		return;
+
+	/* using shadow buffer? */
+	if (helper->defio_vaddr) {
+		unsigned int pitch = helper->fb->pitches[0];
+		u8 cpp = helper->fb->format->cpp[0];
+		unsigned int y;
+
+		for (y = clip_copy.y1; y < clip_copy.y2; y++) {
+			size_t offset = (y * pitch) + (clip_copy.x1 * cpp);
+
+			memcpy(helper->defio_vaddr + offset,
+			       helper->fbdev->screen_buffer + offset,
+			       (clip_copy.x2 - clip_copy.x1) * cpp);
+		}
+	}
+
+	helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip_copy, 1);
 }
 
 /**
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index ea170b9..159d5dd 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -203,6 +203,14 @@ struct drm_fb_helper {
 	struct drm_clip_rect dirty_clip;
 	spinlock_t dirty_lock;
 	struct work_struct dirty_work;
+	/**
+	 * @defio_vaddr:
+	 *
+	 * Destination address for shadowed and deferred framebuffer. If this
+	 * is set, the dirty area is copied from &fb_info->screen_buffer to
+	 * this address before calling &drm_framebuffer_funcs->dirty.
+	 */
+	void *defio_vaddr;
 	struct work_struct resume_work;
 
 	/**
-- 
2.7.4

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

[RFC 4/7] drm/fb-helper: Add simple init/fini functions

[Noralf Trønnes]

This adds some simple init/fini/fb_probe helpers for drivers
that don't need anything special in their fbdev emulation.

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

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index d8b2690..77f2943 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2515,3 +2515,211 @@ int __init drm_fb_helper_modinit(void)
 	return 0;
 }
 EXPORT_SYMBOL(drm_fb_helper_modinit);
+
+static int drm_fb_helper_defio_init(struct drm_fb_helper *helper)
+{
+	struct fb_info *fbi = helper->fbdev;
+	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;
+	}
+
+	*fbops = *fbi->fbops;
+	fbi->fbops = fbops;
+
+	fbdefio->delay = msecs_to_jiffies(50);
+	fbdefio->deferred_io = drm_fb_helper_deferred_io;
+	fbi->fbdefio = fbdefio;
+	fb_deferred_io_init(fbi);
+
+	return 0;
+}
+
+static void drm_fb_helper_defio_fini(struct drm_fb_helper *helper)
+{
+	if (!helper->fbdev || !helper->fbdev->fbdefio)
+		return;
+
+	fb_deferred_io_cleanup(helper->fbdev);
+	kfree(helper->fbdev->fbdefio);
+	kfree(helper->fbdev->fbops);
+}
+
+/**
+ * @drm_fb_helper_mode_cmd - Convert fbdev to fb description
+ * @mode_cmd: addfb description
+ * @sizes: fbdev description
+ *
+ * Returns: Size of framebuffer.
+ */
+size_t drm_fb_helper_mode_cmd(struct drm_mode_fb_cmd2 *mode_cmd,
+			      struct drm_fb_helper_surface_size *sizes)
+{
+	memset(mode_cmd, 0, sizeof(*mode_cmd));
+	mode_cmd->width = sizes->surface_width;
+	mode_cmd->height = sizes->surface_height;
+	mode_cmd->pitches[0] = sizes->surface_width *
+			       DIV_ROUND_UP(sizes->surface_bpp, 8);
+	mode_cmd->pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
+							sizes->surface_depth);
+
+	return mode_cmd->pitches[0] * mode_cmd->height;
+}
+
+/**
+ * drm_fb_helper_simple_fb_probe - simple &drm_fb_helper_funcs->fb_probe helper
+ * @helper: fbdev emulation structure
+ * @sizes: fbdev description
+ * @fb: Attached DRM framebuffer
+ * @fbops: fbdev operations
+ * @vaddr: Virtual address for &fb_info->screen_buffer
+ * @paddr: Address for &fb_fix_screeninfo->smem_start.
+ * @size: Framebuffer size
+ *
+ * Drivers can use this in their &drm_fb_helper_funcs->fb_probe function.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ */
+int drm_fb_helper_simple_fb_probe(struct drm_fb_helper *helper,
+				  struct drm_fb_helper_surface_size *sizes,
+				  struct drm_framebuffer *fb,
+				  struct fb_ops *fbops, void *vaddr,
+				  unsigned long paddr, size_t size)
+{
+	struct fb_info *fbi;
+	int ret;
+
+	DRM_DEBUG_KMS("surface width(%d), height(%d) and bpp(%d)\n",
+		      sizes->surface_width, sizes->surface_height,
+		      sizes->surface_bpp);
+
+	fbi = drm_fb_helper_alloc_fbi(helper);
+	if (IS_ERR(fbi))
+		return PTR_ERR(fbi);
+
+	helper->fb = fb;
+	fbi->par = helper;
+	fbi->fbops = fbops;
+
+	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);
+
+	fbi->screen_base = vaddr;
+	fbi->fix.smem_start = paddr;
+	fbi->screen_size = size;
+	fbi->fix.smem_len = size;
+
+	if (fb->funcs->dirty) {
+		ret = drm_fb_helper_defio_init(helper);
+		if (ret)
+			goto err_helper_fini;
+	}
+
+	return 0;
+
+err_helper_fini:
+	drm_fb_helper_fini(helper);
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_fb_helper_simple_fb_probe);
+
+/**
+ * drm_fb_helper_simple_init - Simple fbdev emulation init
+ * @dev: drm device
+ * @helper: driver-allocated fbdev helper structure to initialize
+ * @bpp_sel: bpp value to use for the framebuffer configuration
+ * @max_conn_count: max connector count
+ * @funcs: pointer to structure of functions associate with this helper
+ *
+ * Simple fbdev emulation initialization. This function calls:
+ * drm_fb_helper_prepare(), drm_fb_helper_init(),
+ * drm_fb_helper_single_add_all_connectors() and
+ * drm_fb_helper_initial_config().
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ */
+int drm_fb_helper_simple_init(struct drm_device *dev,
+			      struct drm_fb_helper *helper, int bpp_sel,
+			      int max_conn_count,
+			      const struct drm_fb_helper_funcs *funcs)
+{
+	int ret;
+
+	drm_fb_helper_prepare(dev, helper, funcs);
+
+	ret = drm_fb_helper_init(dev, helper, max_conn_count);
+	if (ret < 0) {
+		DRM_DEV_ERROR(dev->dev, "Failed to initialize fb helper.\n");
+		return ret;
+	}
+
+	ret = drm_fb_helper_single_add_all_connectors(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(helper, bpp_sel);
+	if (ret < 0) {
+		DRM_DEV_ERROR(dev->dev, "Failed to set initial hw config.\n");
+		goto err_drm_fb_helper_fini;
+	}
+
+	return 0;
+
+err_drm_fb_helper_fini:
+	drm_fb_helper_fini(helper);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(drm_fb_helper_simple_init);
+
+/**
+ * drm_fb_helper_simple_fini - Simple fbdev cleanup
+ * @helper: fbdev emulation structure
+ *
+ * Simple fbdev emulation cleanup. This function unregisters fbdev, cleans up
+ * deferred io if necessary, removes framebuffer, frees shadow buffer if used
+ * and finally cleans up @helper. The driver if responsible for freeing the
+ * @helper structure.
+ */
+void drm_fb_helper_simple_fini(struct drm_fb_helper *helper)
+{
+	if (!helper)
+		return;
+
+	drm_fb_helper_unregister_fbi(helper);
+
+	drm_fb_helper_defio_fini(helper);
+
+	if (helper->fb) {
+		drm_framebuffer_remove(helper->fb);
+		/*
+		 * for some reason if drm_fb_helper_set_par() is called we end
+		 * up with an extra ref taken on the framebuffer...
+		 */
+		if (drm_framebuffer_read_refcount(helper->fb))
+			drm_framebuffer_put(helper->fb);
+	}
+
+	if (helper->defio_vaddr)
+		kvfree(helper->fbdev->screen_buffer);
+
+	drm_fb_helper_fini(helper);
+}
+EXPORT_SYMBOL_GPL(drm_fb_helper_simple_fini);
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 159d5dd..f8db888 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -327,6 +327,19 @@ drm_pick_cmdline_mode(struct drm_fb_helper_connector *fb_helper_conn);
 int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, struct drm_connector *connector);
 int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
 				       struct drm_connector *connector);
+
+size_t drm_fb_helper_mode_cmd(struct drm_mode_fb_cmd2 *mode_cmd,
+			      struct drm_fb_helper_surface_size *sizes);
+int drm_fb_helper_simple_fb_probe(struct drm_fb_helper *helper,
+				  struct drm_fb_helper_surface_size *sizes,
+				  struct drm_framebuffer *fb,
+				  struct fb_ops *fbops, void *vaddr,
+				  unsigned long paddr, size_t size);
+int drm_fb_helper_simple_init(struct drm_device *dev,
+			      struct drm_fb_helper *helper, int bpp_sel,
+			      int max_conn_count,
+			      const struct drm_fb_helper_funcs *funcs);
+void drm_fb_helper_simple_fini(struct drm_fb_helper *helper);
 #else
 static inline void drm_fb_helper_prepare(struct drm_device *dev,
 					struct drm_fb_helper *helper,
@@ -524,6 +537,35 @@ drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
 	return 0;
 }
 
+static inline size_t
+drm_fb_helper_mode_cmd(struct drm_mode_fb_cmd2 *mode_cmd,
+		       struct drm_fb_helper_surface_size *sizes)
+{
+	return 0;
+}
+
+static inline int
+drm_fb_helper_simple_fb_probe(struct drm_fb_helper *helper,
+			      struct drm_fb_helper_surface_size *sizes,
+			      struct drm_framebuffer *fb,
+			      struct fb_ops *fbops, void *vaddr,
+			      unsigned long paddr, size_t size)
+{
+	return 0;
+}
+
+static inline int
+drm_fb_helper_simple_init(struct drm_device *dev, struct drm_fb_helper *helper,
+			  int bpp_sel, int max_conn_count,
+			  const struct drm_fb_helper_funcs *funcs)
+{
+	return 0;
+}
+
+static inline void drm_fb_helper_simple_fini(struct drm_fb_helper *helper)
+{
+}
+
 #endif
 
 static inline int
-- 
2.7.4

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

[RFC 5/7] drm: Add library for shmem backed GEM objects

[Noralf Trønnes]

This adds a library for shmem backed GEM objects with the
necessary drm_driver callbacks.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/Kconfig                |   6 +
 drivers/gpu/drm/Makefile               |   1 +
 drivers/gpu/drm/drm_gem_shmem_helper.c | 628 +++++++++++++++++++++++++++++++++
 include/drm/drm_gem_shmem_helper.h     | 131 +++++++
 4 files changed, 766 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_gem_shmem_helper.c
 create mode 100644 include/drm/drm_gem_shmem_helper.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 83cb2a8..8e709c2 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -145,6 +145,12 @@ config DRM_KMS_CMA_HELPER
 	help
 	  Choose this if you need the KMS CMA helper functions
 
+config DRM_GEM_SHMEM_HELPER
+	bool
+	depends on DRM
+	help
+	  Choose this if you need the GEM SHMEM helper functions
+
 config DRM_VM
 	bool
 	depends on DRM && MMU
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 83d8b09..598c247 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -23,6 +23,7 @@ drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
 drm-$(CONFIG_DRM_VM) += drm_vm.o
 drm-$(CONFIG_COMPAT) += drm_ioc32.o
 drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
+drm-$(CONFIG_DRM_GEM_SHMEM_HELPER) += drm_gem_shmem_helper.o
 drm-$(CONFIG_PCI) += ati_pcigart.o
 drm-$(CONFIG_DRM_PANEL) += drm_panel.o
 drm-$(CONFIG_OF) += drm_of.o
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
new file mode 100644
index 0000000..137badc
--- /dev/null
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -0,0 +1,628 @@
+/*
+ * drm gem shmem (shared memory) helper functions
+ *
+ * Copyright (C) 2017 Noralf Trønnes
+ *
+ * Based on drm_gem_cma_helper.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/mm.h>
+#include <linux/slab.h>
+#include <linux/export.h>
+#include <linux/dma-buf.h>
+#include <linux/shmem_fs.h>
+
+#include <drm/drmP.h>
+#include <drm/drm_gem_shmem_helper.h>
+#include <drm/drm_vma_manager.h>
+
+/**
+ * DOC: shmem helpers
+ *
+ * The Contiguous Memory Allocator reserves a pool of memory at early boot
+ * that is used to service requests for large blocks of contiguous memory.
+ *
+ * The DRM GEM/SHMEM helpers use this allocator as a means to provide buffer
+ * objects that are physically contiguous in memory. This is useful for
+ * display drivers that are unable to map scattered buffers via an IOMMU.
+ */
+
+static struct drm_gem_shmem_object *
+__drm_gem_shmem_create(struct drm_device *drm, size_t size)
+{
+	struct drm_gem_shmem_object *obj;
+	struct drm_gem_object *gem_obj;
+	int ret;
+
+	if (drm->driver->gem_create_object)
+		gem_obj = drm->driver->gem_create_object(drm, size);
+	else
+		gem_obj = kzalloc(sizeof(*obj), GFP_KERNEL);
+	if (!gem_obj)
+		return ERR_PTR(-ENOMEM);
+
+	obj = to_drm_gem_shmem_obj(gem_obj);
+
+	if (!drm->driver->gem_create_object)
+		obj->cache_mode = DRM_GEM_SHMEM_BO_WRITECOMBINED;
+
+	ret = drm_gem_object_init(drm, gem_obj, size);
+	if (ret)
+		goto error;
+
+	ret = drm_gem_create_mmap_offset(gem_obj);
+	if (ret) {
+		drm_gem_object_release(gem_obj);
+		goto error;
+	}
+
+	return obj;
+
+error:
+	kfree(obj);
+
+	return ERR_PTR(ret);
+}
+
+/**
+ * drm_gem_shmem_create - allocate an object with the given size
+ * @drm: DRM device
+ * @size: size of the object to allocate
+ *
+ * This function creates a shmem GEM object and uses drm_gem_get_pages() to get
+ * the backing pages.
+ *
+ * Returns:
+ * A struct drm_gem_shmem_object * on success or an ERR_PTR()-encoded negative
+ * error code on failure.
+ */
+struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *drm,
+						  size_t size)
+{
+	struct drm_gem_shmem_object *obj;
+	int ret;
+
+	size = round_up(size, PAGE_SIZE);
+
+	obj = __drm_gem_shmem_create(drm, size);
+	if (IS_ERR(obj))
+		return obj;
+
+	obj->pages = drm_gem_get_pages(&obj->base);
+	if (IS_ERR(obj->pages)) {
+		dev_err(drm->dev, "failed to allocate buffer with size %zu\n",
+			size);
+		ret = PTR_ERR(obj->pages);
+		goto error;
+	}
+
+	return obj;
+
+error:
+	drm_gem_object_put_unlocked(&obj->base);
+
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(drm_gem_shmem_create);
+
+static struct drm_gem_shmem_object *
+drm_gem_shmem_create_with_handle(struct drm_file *file_priv,
+				 struct drm_device *drm, size_t size,
+				 uint32_t *handle)
+{
+	struct drm_gem_shmem_object *obj;
+	struct drm_gem_object *gem_obj;
+	int ret;
+
+	obj = drm_gem_shmem_create(drm, size);
+	if (IS_ERR(obj))
+		return obj;
+
+	gem_obj = &obj->base;
+
+	/*
+	 * allocate a id of idr table where the obj is registered
+	 * and handle has the id what user can see.
+	 */
+	ret = drm_gem_handle_create(file_priv, gem_obj, handle);
+	/* drop reference from allocate - handle holds it now. */
+	drm_gem_object_put_unlocked(gem_obj);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return obj;
+}
+
+/**
+ * drm_gem_shmem_free_object - free resources associated with a shmem GEM object
+ * @gem_obj: GEM object to free
+ *
+ * This function frees the backing memory of the shmem GEM object, cleans up
+ * the GEM object state and frees the memory used to store the object itself.
+ * Drivers using the shmem helpers should set this as their
+ * &drm_driver.gem_free_object callback.
+ */
+void drm_gem_shmem_free_object(struct drm_gem_object *gem_obj)
+{
+	struct drm_gem_shmem_object *obj = to_drm_gem_shmem_obj(gem_obj);
+
+	drm_gem_shmem_vunmap(obj);
+
+	if (gem_obj->import_attach) {
+		drm_prime_gem_destroy(gem_obj, obj->sgt);
+		kvfree(obj->pages);
+	} else {
+		drm_gem_put_pages(&obj->base, obj->pages, false, false);
+	}
+
+	drm_gem_object_release(gem_obj);
+
+	kfree(obj);
+}
+EXPORT_SYMBOL_GPL(drm_gem_shmem_free_object);
+
+/*
+ * drm_gem_shmem_vmap - vmap a shmem GEM object
+ * @obj: shmem GEM object
+ *
+ * This function makes sure that a virtual address exists for
+ * the shmem GEM object.
+ *
+ * Returns:
+ * Zero on success, negative error code on failure.
+ */
+int drm_gem_shmem_vmap(struct drm_gem_shmem_object *obj)
+{
+	struct drm_gem_object *gem_obj = &obj->base;
+
+	if (obj->vaddr)
+		return 0;
+
+	/* TODO: locking? */
+
+	if (gem_obj->import_attach) {
+		obj->vaddr = dma_buf_vmap(gem_obj->import_attach->dmabuf);
+	} else {
+		pgprot_t prot;
+
+		switch (obj->cache_mode) {
+		case DRM_GEM_SHMEM_BO_WRITECOMBINED:
+			prot = pgprot_writecombine(PAGE_KERNEL);
+			break;
+
+		case DRM_GEM_SHMEM_BO_UNCACHED:
+			prot = pgprot_noncached(PAGE_KERNEL);
+			break;
+
+		case DRM_GEM_SHMEM_BO_CACHED:
+			prot = PAGE_KERNEL;
+			break;
+		}
+
+		obj->vaddr = vmap(obj->pages, gem_obj->size >> PAGE_SHIFT,
+				  VM_MAP, prot);
+	}
+
+	if (!obj->vaddr)
+		return -ENOMEM;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_gem_shmem_vmap);
+
+/*
+ * drm_gem_shmem_vunmap - vunmap a shmem GEM object
+ * @obj: shmem GEM object
+ *
+ * This function makes sure that the virtual address is removed for
+ * the shmem GEM object.
+ */
+void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *obj)
+{
+	struct drm_gem_object *gem_obj = &obj->base;
+
+	if (!obj->vaddr)
+		return;
+
+	if (gem_obj->import_attach)
+		dma_buf_vunmap(gem_obj->import_attach->dmabuf, obj->vaddr);
+	else
+		vunmap(obj->vaddr);
+
+	obj->vaddr = NULL;
+}
+EXPORT_SYMBOL(drm_gem_shmem_vunmap);
+
+/**
+ * drm_gem_shmem_dumb_create - create a dumb shmem buffer object
+ * @file_priv: DRM file structure to create the dumb buffer for
+ * @drm: DRM device
+ * @args: IOCTL data
+ *
+ * This function computes the pitch of the dumb buffer and rounds it up to an
+ * integer number of bytes per pixel. Drivers for hardware that doesn't have
+ * any additional restrictions on the pitch can directly use this function as
+ * their &drm_driver.dumb_create callback.
+ *
+ * For hardware with additional restrictions, drivers can adjust the fields
+ * set up by userspace before calling into this function.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ */
+int drm_gem_shmem_dumb_create(struct drm_file *file_priv,
+			      struct drm_device *drm,
+			      struct drm_mode_create_dumb *args)
+{
+	struct drm_gem_shmem_object *obj;
+
+	if (!args->pitch || !args->size) {
+		args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
+		args->size = args->pitch * args->height;
+	} else {
+		/* ensure sane minimum values */
+		args->pitch = max(args->pitch,
+				  DIV_ROUND_UP(args->width * args->bpp, 8));
+		args->size = max_t(typeof(args->size), args->size,
+				   args->pitch * args->height);
+	}
+
+	obj = drm_gem_shmem_create_with_handle(file_priv, drm, args->size,
+					       &args->handle);
+	return PTR_ERR_OR_ZERO(obj);
+}
+EXPORT_SYMBOL_GPL(drm_gem_shmem_dumb_create);
+
+static int drm_gem_shmem_fault(struct vm_fault *vmf)
+{
+	struct vm_area_struct *vma = vmf->vma;
+	struct drm_gem_shmem_object *obj = to_drm_gem_shmem_obj(vma->vm_private_data);
+	/* We don't use vmf->pgoff since that has the fake offset */
+	unsigned long vaddr = vmf->address;
+	struct page *page;
+
+	page = shmem_read_mapping_page(file_inode(obj->base.filp)->i_mapping,
+				       (vaddr - vma->vm_start) >> PAGE_SHIFT);
+	if (!IS_ERR(page)) {
+		vmf->page = page;
+		return 0;
+	} else switch (PTR_ERR(page)) {
+		case -ENOSPC:
+		case -ENOMEM:
+			return VM_FAULT_OOM;
+		case -EBUSY:
+			return VM_FAULT_RETRY;
+		case -EFAULT:
+		case -EINVAL:
+			return VM_FAULT_SIGBUS;
+		default:
+			WARN_ON_ONCE(PTR_ERR(page));
+			return VM_FAULT_SIGBUS;
+	}
+}
+
+const struct vm_operations_struct drm_gem_shmem_vm_ops = {
+	.fault = drm_gem_shmem_fault,
+	.open = drm_gem_vm_open,
+	.close = drm_gem_vm_close,
+};
+EXPORT_SYMBOL_GPL(drm_gem_shmem_vm_ops);
+
+static int drm_gem_shmem_mmap_obj(struct drm_gem_shmem_object *obj,
+				  struct vm_area_struct *vma)
+{
+	/* VM_PFNMAP was set by drm_gem_mmap() */
+	vma->vm_flags &= ~VM_PFNMAP;
+	vma->vm_flags |= VM_MIXEDMAP;
+
+	switch (obj->cache_mode) {
+	case DRM_GEM_SHMEM_BO_WRITECOMBINED:
+		vma->vm_page_prot =
+			pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
+		break;
+
+	case DRM_GEM_SHMEM_BO_UNCACHED:
+		vma->vm_page_prot =
+			pgprot_noncached(vm_get_page_prot(vma->vm_flags));
+		break;
+
+	case DRM_GEM_SHMEM_BO_CACHED:
+		/*
+		 * Shunt off cached objs to shmem file so they have their own
+		 * address_space (so unmap_mapping_range does what we want,
+		 * in particular in the case of mmap'd dmabufs)
+		 */
+		fput(vma->vm_file);
+		get_file(obj->base.filp);
+		vma->vm_pgoff = 0;
+		vma->vm_file  = obj->base.filp;
+
+		vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
+		break;
+	}
+
+	return 0;
+}
+
+/**
+ * drm_gem_shmem_mmap - memory-map a shmem GEM object
+ * @filp: file object
+ * @vma: VMA for the area to be mapped
+ *
+ * This function implements an augmented version of the GEM DRM file mmap
+ * operation for shmem objects. Drivers which employ the shmem helpers should
+ * use this function as their ->mmap() handler in the DRM device file's
+ * file_operations structure.
+ *
+ * Instead of directly referencing this function, drivers should use the
+ * DEFINE_DRM_GEM_SHMEM_FOPS().macro.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ */
+int drm_gem_shmem_mmap(struct file *filp, struct vm_area_struct *vma)
+{
+
+	struct drm_gem_shmem_object *obj;
+	struct drm_gem_object *gem_obj;
+	int ret;
+
+	ret = drm_gem_mmap(filp, vma);
+	if (ret)
+		return ret;
+
+	gem_obj = vma->vm_private_data;
+	obj = to_drm_gem_shmem_obj(gem_obj);
+
+	return drm_gem_shmem_mmap_obj(obj, vma);
+}
+EXPORT_SYMBOL_GPL(drm_gem_shmem_mmap);
+
+#ifndef CONFIG_MMU
+/**
+ * drm_gem_shmem_get_unmapped_area - propose address for mapping in noMMU cases
+ * @filp: file object
+ * @addr: memory address
+ * @len: buffer size
+ * @pgoff: page offset
+ * @flags: memory flags
+ *
+ * This function is used in noMMU platforms to propose address mapping
+ * for a given buffer.
+ * It's intended to be used as a direct handler for the struct
+ * &file_operations.get_unmapped_area operation.
+ *
+ * Returns:
+ * mapping address on success or a negative error code on failure.
+ */
+unsigned long drm_gem_shmem_get_unmapped_area(struct file *filp,
+					      unsigned long addr,
+					      unsigned long len,
+					      unsigned long pgoff,
+					      unsigned long flags)
+{
+	struct drm_file *priv = filp->private_data;
+	struct drm_device *dev = priv->minor->dev;
+	struct drm_gem_object *gem_obj = NULL;
+	struct drm_vma_offset_node *node;
+	struct drm_gem_shmem_object *obj;
+
+	if (drm_device_is_unplugged(dev))
+		return -ENODEV;
+
+	drm_vma_offset_lock_lookup(dev->vma_offset_manager);
+	node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
+						  pgoff,
+						  len >> PAGE_SHIFT);
+	if (likely(node)) {
+		gem_obj = container_of(node, struct drm_gem_object, vma_node);
+		/*
+		 * When the object is being freed, after it hits 0-refcnt it
+		 * proceeds to tear down the object. In the process it will
+		 * attempt to remove the VMA offset and so acquire this
+		 * mgr->vm_lock.  Therefore if we find an object with a 0-refcnt
+		 * that matches our range, we know it is in the process of being
+		 * destroyed and will be freed as soon as we release the lock -
+		 * so we have to check for the 0-refcnted object and treat it as
+		 * invalid.
+		 */
+		if (!kref_get_unless_zero(&gem_obj->refcount))
+			gem_obj = NULL;
+	}
+
+	drm_vma_offset_unlock_lookup(dev->vma_offset_manager);
+
+	if (!gem_obj)
+		return -EINVAL;
+
+	if (!drm_vma_node_is_allowed(node, priv)) {
+		drm_gem_object_put_unlocked(gem_obj);
+		return -EACCES;
+	}
+
+	obj = to_drm_gem_shmem_obj(gem_obj);
+
+	drm_gem_object_put_unlocked(gem_obj);
+
+	/* TODO: should we call drm_gem_shmem_vmap() here? */
+
+	return obj->vaddr ? (unsigned long)obj->vaddr : -EINVAL;
+}
+EXPORT_SYMBOL_GPL(drm_gem_shmem_get_unmapped_area);
+#endif
+
+#ifdef CONFIG_DEBUG_FS
+static const char *cache_mode_str[] = {
+	[DRM_GEM_SHMEM_BO_UNCACHED] = "uncached",
+	[DRM_GEM_SHMEM_BO_CACHED] = "cached",
+	[DRM_GEM_SHMEM_BO_WRITECOMBINED] = "writecombined",
+};
+
+/**
+ * drm_gem_shmem_describe - describe a shmem GEM object for debugfs
+ * @obj: shmem GEM object
+ * @m: debugfs file handle
+ *
+ * This function can be used to dump a human-readable representation of the
+ * shmem GEM object into a synthetic file.
+ */
+void drm_gem_shmem_describe(struct drm_gem_shmem_object *obj,
+			    struct seq_file *m)
+{
+	struct drm_gem_object *gem_obj = &obj->base;
+	uint64_t off;
+
+	off = drm_vma_node_start(&gem_obj->vma_node);
+	seq_printf(m, "name=%d refcount=%d off=%08llx vaddr=%p size=%zu mode=%s",
+		   gem_obj->name, kref_read(&gem_obj->refcount), off,
+		   obj->vaddr, gem_obj->size, cache_mode_str[obj->cache_mode]);
+
+	seq_printf(m, "\n");
+}
+EXPORT_SYMBOL_GPL(drm_gem_shmem_describe);
+#endif
+
+/**
+ * drm_gem_shmem_prime_get_sg_table - provide a scatter/gather table of pinned
+ *                                    pages for a SHMEM GEM object
+ * @obj: GEM object
+ *
+ * This function exports a scatter/gather table suitable for PRIME usage by
+ * calling the standard DMA mapping API. Drivers using the shmem helpers should
+ * set this as their &drm_driver.gem_prime_get_sg_table callback.
+ *
+ * Returns:
+ * A pointer to the scatter/gather table of pinned pages or NULL on failure.
+ */
+struct sg_table *
+drm_gem_shmem_prime_get_sg_table(struct drm_gem_object *gem_obj)
+{
+	struct drm_gem_shmem_object *obj = to_drm_gem_shmem_obj(gem_obj);
+
+	return drm_prime_pages_to_sg(obj->pages, gem_obj->size >> PAGE_SHIFT);
+}
+EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_get_sg_table);
+
+/**
+ * drm_gem_shmem_prime_import_sg_table - produce a shmem GEM object from
+ *                 another driver's scatter/gather table of pinned pages
+ * @dev: device to import into
+ * @attach: DMA-BUF attachment
+ * @sgt: scatter/gather table of pinned pages
+ *
+ * This function imports a scatter/gather table exported via DMA-BUF by
+ * another driver. Drivers that use the shmem helpers should set this as their
+ * &drm_driver.gem_prime_import_sg_table callback.
+ *
+ * Returns:
+ * A pointer to a newly created GEM object or an ERR_PTR-encoded negative
+ * error code on failure.
+ */
+struct drm_gem_object *
+drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
+				    struct dma_buf_attachment *attach,
+				    struct sg_table *sgt)
+{
+	int npages = attach->dmabuf->size >> PAGE_SHIFT;
+	struct drm_gem_shmem_object *obj;
+	int ret;
+
+	obj = __drm_gem_shmem_create(dev, attach->dmabuf->size);
+	if (IS_ERR(obj))
+		return ERR_CAST(obj);
+
+	obj->pages = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
+	if (!obj->pages) {
+		ret = -ENOMEM;
+		goto err_free_gem;
+	}
+
+	ret = drm_prime_sg_to_page_addr_arrays(sgt, obj->pages, NULL, npages);
+	if (ret < 0)
+		goto err_free_array;
+
+	obj->sgt = sgt;
+
+	DRM_DEBUG_PRIME("size = %zu\n", attach->dmabuf->size);
+
+	return &obj->base;
+
+err_free_array:
+	kvfree(obj->pages);
+err_free_gem:
+	drm_gem_object_put_unlocked(&obj->base);
+
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_sg_table);
+
+/**
+ * drm_gem_shmem_prime_mmap - memory-map an exported shmem GEM object
+ * @obj: GEM object
+ * @vma: VMA for the area to be mapped
+ *
+ * This function maps a buffer imported via DRM PRIME into a userspace
+ * process's address space. Drivers that use the shmem helpers should set this
+ * as their &drm_driver.gem_prime_mmap callback.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ */
+int drm_gem_shmem_prime_mmap(struct drm_gem_object *gem_obj,
+			     struct vm_area_struct *vma)
+{
+	struct drm_gem_shmem_object *obj = to_drm_gem_shmem_obj(gem_obj);
+	int ret;
+
+	ret = drm_gem_mmap_obj(gem_obj, gem_obj->size, vma);
+	if (ret < 0)
+		return ret;
+
+	return drm_gem_shmem_mmap_obj(obj, vma);
+}
+EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_mmap);
+
+/**
+ * drm_gem_shmem_prime_vmap - map a shmem GEM object into the kernel's virtual
+ *                            address space
+ * @obj: GEM object
+ *
+ * This function maps a buffer exported via DRM PRIME into the kernel's
+ * virtual address space. Drivers using the SHMEM helpers should set this as
+ * their DRM driver's &drm_driver.gem_prime_vmap callback.
+ *
+ * Returns:
+ * The kernel virtual address of the SHMEM GEM object's backing store.
+ */
+void *drm_gem_shmem_prime_vmap(struct drm_gem_object *gem_obj)
+{
+	struct drm_gem_shmem_object *obj = to_drm_gem_shmem_obj(gem_obj);
+
+	drm_gem_shmem_vmap(obj);
+
+	return obj->vaddr;
+}
+EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_vmap);
+
+/**
+ * drm_gem_shmem_prime_vunmap - unmap a shmem GEM object from the kernel's
+ *                              virtual address space
+ * @obj: GEM object
+ * @vaddr: kernel virtual address where the shmem GEM object was mapped
+ *
+ * This function removes a buffer exported via DRM PRIME from the kernel's
+ * virtual address space. Drivers using the SHMEM helpers should set this as
+ * their &drm_driver.gem_prime_vunmap callback.
+ */
+void drm_gem_shmem_prime_vunmap(struct drm_gem_object *gem_obj, void *vaddr)
+{
+	struct drm_gem_shmem_object *obj = to_drm_gem_shmem_obj(gem_obj);
+
+	drm_gem_shmem_vunmap(obj);
+}
+EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_vunmap);
diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
new file mode 100644
index 0000000..5cbd990
--- /dev/null
+++ b/include/drm/drm_gem_shmem_helper.h
@@ -0,0 +1,131 @@
+#ifndef __DRM_GEM_SHMEM_HELPER_H__
+#define __DRM_GEM_SHMEM_HELPER_H__
+
+#include <drm/drmP.h>
+#include <drm/drm_gem.h>
+
+enum drm_gem_shmem_cache_mode {
+	DRM_GEM_SHMEM_BO_UNCACHED,
+	DRM_GEM_SHMEM_BO_CACHED,
+	DRM_GEM_SHMEM_BO_WRITECOMBINED,
+};
+
+/**
+ * struct drm_gem_shmem_object - GEM object backed by shmem
+ * @base: base GEM object
+ * @pages: page table
+ * @cache_mode: Cache mode
+ * @sgt: scatter/gather table for imported PRIME buffers
+ * @vaddr: kernel virtual address of the backing memory
+ */
+struct drm_gem_shmem_object {
+	struct drm_gem_object base;
+	struct page **pages;
+	enum drm_gem_shmem_cache_mode cache_mode;
+	struct sg_table *sgt;
+	void *vaddr;
+};
+
+static inline struct drm_gem_shmem_object *
+to_drm_gem_shmem_obj(struct drm_gem_object *gem_obj)
+{
+	return container_of(gem_obj, struct drm_gem_shmem_object, base);
+}
+
+#ifndef CONFIG_MMU
+#define DRM_GEM_SHMEM_UNMAPPED_AREA_FOPS \
+	.get_unmapped_area	= drm_gem_shmem_get_unmapped_area,
+#else
+#define DRM_GEM_SHMEM_UNMAPPED_AREA_FOPS
+#endif
+
+/**
+ * DEFINE_DRM_GEM_SHMEM_FOPS() - macro to generate file operations for shmem
+ *                               drivers
+ * @name: name for the generated structure
+ *
+ * This macro autogenerates a suitable &struct file_operations for shmem based
+ * drivers, which can be assigned to &drm_driver.fops. Note that this structure
+ * cannot be shared between drivers, because it contains a reference to the
+ * current module using THIS_MODULE.
+ *
+ * Note that the declaration is already marked as static - if you need a
+ * non-static version of this you're probably doing it wrong and will break the
+ * THIS_MODULE reference by accident.
+ */
+#define DEFINE_DRM_GEM_SHMEM_FOPS(name) \
+	static const struct file_operations name = {\
+		.owner		= THIS_MODULE,\
+		.open		= drm_open,\
+		.release	= drm_release,\
+		.unlocked_ioctl	= drm_ioctl,\
+		.compat_ioctl	= drm_compat_ioctl,\
+		.poll		= drm_poll,\
+		.read		= drm_read,\
+		.llseek		= noop_llseek,\
+		.mmap		= drm_gem_shmem_mmap,\
+		DRM_GEM_SHMEM_UNMAPPED_AREA_FOPS \
+	}
+
+struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *drm,
+						  size_t size);
+void drm_gem_shmem_free_object(struct drm_gem_object *gem_obj);
+
+int drm_gem_shmem_vmap(struct drm_gem_shmem_object *obj);
+void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *obj);
+
+int drm_gem_shmem_dumb_create(struct drm_file *file_priv,
+			      struct drm_device *drm,
+			      struct drm_mode_create_dumb *args);
+
+int drm_gem_shmem_mmap(struct file *filp, struct vm_area_struct *vma);
+
+extern const struct vm_operations_struct drm_gem_shmem_vm_ops;
+
+#ifndef CONFIG_MMU
+unsigned long drm_gem_shmem_get_unmapped_area(struct file *filp,
+					      unsigned long addr,
+					      unsigned long len,
+					      unsigned long pgoff,
+					      unsigned long flags);
+#endif
+
+#ifdef CONFIG_DEBUG_FS
+void drm_gem_shmem_describe(struct drm_gem_shmem_object *obj,
+			    struct seq_file *m);
+#endif
+
+struct sg_table *
+drm_gem_shmem_prime_get_sg_table(struct drm_gem_object *gem_obj);
+struct drm_gem_object *
+drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
+				    struct dma_buf_attachment *attach,
+				    struct sg_table *sgt);
+int drm_gem_shmem_prime_mmap(struct drm_gem_object *gem_obj,
+			     struct vm_area_struct *vma);
+void *drm_gem_shmem_prime_vmap(struct drm_gem_object *gem_obj);
+void drm_gem_shmem_prime_vunmap(struct drm_gem_object *gem_obj, void *vaddr);
+
+/**
+ * DRM_GEM_SHMEM_DRIVER_OPS - default shmem gem operations
+ *
+ * This macro provides a shortcut for setting the shmem GEM operations in
+ * the &drm_driver structure.
+ */
+#define DRM_GEM_SHMEM_DRIVER_OPS \
+	.gem_free_object	= drm_gem_shmem_free_object, \
+	.gem_vm_ops		= &drm_gem_shmem_vm_ops, \
+	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd, \
+	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle, \
+	.gem_prime_import	= drm_gem_prime_import, \
+	.gem_prime_export	= drm_gem_prime_export, \
+	.gem_prime_get_sg_table	= drm_gem_shmem_prime_get_sg_table, \
+	.gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table, \
+	.gem_prime_vmap		= drm_gem_shmem_prime_vmap, \
+	.gem_prime_vunmap	= drm_gem_shmem_prime_vunmap, \
+	.gem_prime_mmap		= drm_gem_shmem_prime_mmap, \
+	.dumb_create		= drm_gem_shmem_dumb_create, \
+	.dumb_map_offset	= drm_gem_dumb_map_offset, \
+	.dumb_destroy		= drm_gem_dumb_destroy
+
+#endif /* __DRM_GEM_SHMEM_HELPER_H__ */
-- 
2.7.4

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

[RFC 6/7] drm: Add kms library for shmem backed GEM

[Noralf Trønnes]

This adds kms helpers for the shmem gem library.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/Kconfig               |  11 +++
 drivers/gpu/drm/Makefile              |   1 +
 drivers/gpu/drm/drm_fb_shmem_helper.c | 168 ++++++++++++++++++++++++++++++++++
 include/drm/drm_fb_shmem_helper.h     |  18 ++++
 4 files changed, 198 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_fb_shmem_helper.c
 create mode 100644 include/drm/drm_fb_shmem_helper.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 8e709c2..4c9010d 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -151,6 +151,17 @@ config DRM_GEM_SHMEM_HELPER
 	help
 	  Choose this if you need the GEM SHMEM helper functions
 
+config DRM_KMS_SHMEM_HELPER
+	bool
+	depends on DRM
+	select DRM_GEM_SHMEM_HELPER
+	select DRM_KMS_FB_HELPER
+	select FB_SYS_FILLRECT
+	select FB_SYS_COPYAREA
+	select FB_SYS_IMAGEBLIT
+	help
+	  Choose this if you need the KMS SHMEM helper functions
+
 config DRM_VM
 	bool
 	depends on DRM && MMU
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 598c247..706037c 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -40,6 +40,7 @@ drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o
 drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
 drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
 drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
+drm_kms_helper-$(CONFIG_DRM_KMS_SHMEM_HELPER) += drm_fb_shmem_helper.o
 drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
 
 obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
diff --git a/drivers/gpu/drm/drm_fb_shmem_helper.c b/drivers/gpu/drm/drm_fb_shmem_helper.c
new file mode 100644
index 0000000..ddcf24f
--- /dev/null
+++ b/drivers/gpu/drm/drm_fb_shmem_helper.c
@@ -0,0 +1,168 @@
+/*
+ * drm kms/fb shmem (shared memory) helper functions
+ *
+ * Copyright (C) 2017 Noralf Trønnes
+ *
+ * Based on drm_fb_cma_helper.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/module.h>
+
+#include <drm/drmP.h>
+#include <drm/drm_fb_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
+#include <drm/drm_fb_gem_helper.h>
+#include <drm/drm_fb_shmem_helper.h>
+
+#ifdef CONFIG_DEBUG_FS
+static void drm_fb_shmem_describe(struct drm_framebuffer *fb, struct seq_file *m)
+{
+	struct drm_fb_gem *fb_gem = to_fb_gem(fb);
+	struct drm_gem_shmem_object *obj;
+	int i;
+
+	seq_printf(m, "[FB:%d] %dx%d@%4.4s\n", fb->base.id, fb->width, fb->height,
+			(char *)&fb->format->format);
+
+	for (i = 0; i < fb->format->num_planes; i++) {
+		obj = to_drm_gem_shmem_obj(fb_gem->obj[i]);
+		seq_printf(m, "   %d: offset=%d pitch=%d, obj: ",
+				i, fb->offsets[i], fb->pitches[i]);
+		drm_gem_shmem_describe(obj, m);
+	}
+}
+
+/**
+ * drm_fb_shmem_debugfs_show() - Helper to list shmem framebuffer objects
+ *                               in debugfs.
+ * @m: output file
+ * @arg: private data for the callback
+ */
+int drm_fb_shmem_debugfs_show(struct seq_file *m, void *arg)
+{
+	struct drm_info_node *node = (struct drm_info_node *)m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct drm_framebuffer *fb;
+
+	mutex_lock(&dev->mode_config.fb_lock);
+	drm_for_each_fb(fb, dev)
+		drm_fb_shmem_describe(fb, m);
+	mutex_unlock(&dev->mode_config.fb_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(drm_fb_shmem_debugfs_show);
+#endif
+
+static int drm_fb_shmem_mmap(struct fb_info *fbi, struct vm_area_struct *vma)
+{
+	struct drm_fb_helper *helper = fbi->par;
+	struct drm_fb_gem *fb_gem = to_fb_gem(helper->fb);
+
+	return drm_gem_shmem_prime_mmap(fb_gem->obj[0], vma);
+}
+
+static int drm_fb_helper_fb_open(struct fb_info *fbi, int user)
+{
+	struct drm_fb_helper *helper = fbi->par;
+
+	if (!try_module_get(helper->dev->driver->fops->owner))
+		return -ENODEV;
+
+	return 0;
+}
+
+static int drm_fb_helper_fb_release(struct fb_info *fbi, int user)
+{
+	struct drm_fb_helper *helper = fbi->par;
+
+	module_put(helper->dev->driver->fops->owner);
+
+	return 0;
+}
+
+static struct fb_ops drm_fb_helper_fb_ops = {
+	.owner		= THIS_MODULE,
+	DRM_FB_HELPER_DEFAULT_OPS,
+	.fb_open	= drm_fb_helper_fb_open,
+	.fb_release	= drm_fb_helper_fb_release,
+	.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,
+	.fb_mmap	= drm_fb_shmem_mmap,
+};
+
+/**
+ * drm_fb_shmem_fbdev_probe -
+ * @helper: fbdev emulation structure
+ * @sizes: fbdev description
+ * @fb_funcs: Framebuffer helper functions
+ *
+ * Drivers can use this in their &drm_fb_helper_funcs->fb_probe function.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ */
+int drm_fb_shmem_fbdev_probe(struct drm_fb_helper *helper,
+			     struct drm_fb_helper_surface_size *sizes,
+			     const struct drm_framebuffer_funcs *fb_funcs)
+{
+	struct drm_device *dev = helper->dev;
+	struct drm_mode_fb_cmd2 mode_cmd;
+	struct drm_gem_shmem_object *obj;
+	struct drm_gem_object *gem;
+	struct drm_fb_gem *fb_gem;
+	void *shadow = NULL;
+	size_t size;
+	int ret;
+
+	size = drm_fb_helper_mode_cmd(&mode_cmd, sizes);
+
+	obj = drm_gem_shmem_create(dev, size);
+	if (IS_ERR(obj))
+		return PTR_ERR(obj);
+
+	gem = &obj->base;
+	fb_gem = drm_fb_gem_alloc(dev, &mode_cmd, &gem, 1, fb_funcs);
+	if (IS_ERR(fb_gem)) {
+		dev_err(dev->dev, "Failed to allocate DRM framebuffer.\n");
+		drm_gem_object_put_unlocked(&obj->base);
+		return PTR_ERR(fb_gem);
+	}
+
+	ret = drm_gem_shmem_vmap(obj);
+	if (ret)
+		goto error;
+
+	if (fb_funcs->dirty) {
+		shadow = vzalloc(size);
+		if (!shadow) {
+			ret = -ENOMEM;
+			goto error;
+		}
+		helper->defio_vaddr = obj->vaddr;
+	}
+
+	ret = drm_fb_helper_simple_fb_probe(helper, sizes, &fb_gem->base,
+					    &drm_fb_helper_fb_ops,
+					    shadow ? shadow : obj->vaddr, 0,
+					    size);
+	if (ret < 0)
+		goto error;
+
+	return 0;
+
+error:
+	vfree(shadow);
+	drm_framebuffer_remove(&fb_gem->base);
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_fb_shmem_fbdev_probe);
diff --git a/include/drm/drm_fb_shmem_helper.h b/include/drm/drm_fb_shmem_helper.h
new file mode 100644
index 0000000..5c57131
--- /dev/null
+++ b/include/drm/drm_fb_shmem_helper.h
@@ -0,0 +1,18 @@
+#ifndef __DRM_FB_SHMEM_HELPER_H__
+#define __DRM_FB_SHMEM_HELPER_H__
+
+struct drm_fb_helper_surface_size;
+struct drm_framebuffer_funcs;
+struct drm_fb_helper;
+
+int drm_fb_shmem_fbdev_probe(struct drm_fb_helper *helper,
+			     struct drm_fb_helper_surface_size *sizes,
+			     const struct drm_framebuffer_funcs *fb_funcs);
+
+#ifdef CONFIG_DEBUG_FS
+struct seq_file;
+
+int drm_fb_shmem_debugfs_show(struct seq_file *m, void *arg);
+#endif
+
+#endif
-- 
2.7.4

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

[RFC 7/7] drm/tinydrm: Switch from CMA to shmem buffers

[Noralf Trønnes]

This move makes tinydrm useful for more drivers. tinydrm doesn't
need continous memory, but at the time it was convinient to use
the CMA library. The spi core can do dma on vmalloc addresses
making this possible.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/tinydrm/Kconfig             |   2 +-
 drivers/gpu/drm/tinydrm/core/tinydrm-core.c | 147 ++++++++++++----------------
 drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c |   4 +-
 drivers/gpu/drm/tinydrm/mi0283qt.c          |   2 +-
 drivers/gpu/drm/tinydrm/mipi-dbi.c          |  41 +++-----
 include/drm/tinydrm/tinydrm.h               |  32 ++----
 6 files changed, 92 insertions(+), 136 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/Kconfig b/drivers/gpu/drm/tinydrm/Kconfig
index 3504c53..da9b8b8 100644
--- a/drivers/gpu/drm/tinydrm/Kconfig
+++ b/drivers/gpu/drm/tinydrm/Kconfig
@@ -2,7 +2,7 @@ menuconfig DRM_TINYDRM
 	tristate "Support for simple displays"
 	depends on DRM
 	select DRM_KMS_HELPER
-	select DRM_KMS_CMA_HELPER
+	select DRM_KMS_SHMEM_HELPER
 	select BACKLIGHT_LCD_SUPPORT
 	select BACKLIGHT_CLASS_DEVICE
 	help
diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
index 551709e..fb2cc1f 100644
--- a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
@@ -10,6 +10,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_fb_helper.h>
 #include <drm/tinydrm/tinydrm.h>
 #include <linux/device.h>
 #include <linux/dma-buf.h>
@@ -21,7 +22,7 @@
  *
  * It is based on &drm_simple_display_pipe coupled with a &drm_connector which
  * has only one fixed &drm_display_mode. The framebuffers are backed by the
- * cma helper and have support for framebuffer flushing (dirty).
+ * shmem helper and have support for framebuffer flushing (dirty).
  * fbdev support is also included.
  *
  */
@@ -47,80 +48,33 @@ void tinydrm_lastclose(struct drm_device *drm)
 	struct tinydrm_device *tdev = drm->dev_private;
 
 	DRM_DEBUG_KMS("\n");
-	drm_fbdev_cma_restore_mode(tdev->fbdev_cma);
+	if (tdev->fbdev)
+		drm_fb_helper_restore_fbdev_mode_unlocked(tdev->fbdev);
 }
 EXPORT_SYMBOL(tinydrm_lastclose);
 
 /**
- * tinydrm_gem_cma_prime_import_sg_table - Produce a CMA GEM object from
- *     another driver's scatter/gather table of pinned pages
- * @drm: DRM device to import into
- * @attach: DMA-BUF attachment
- * @sgt: Scatter/gather table of pinned pages
- *
- * This function imports a scatter/gather table exported via DMA-BUF by
- * another driver using drm_gem_cma_prime_import_sg_table(). It sets the
- * kernel virtual address on the CMA object. Drivers should use this as their
- * &drm_driver->gem_prime_import_sg_table callback if they need the virtual
- * address. tinydrm_gem_cma_free_object() should be used in combination with
- * this function.
+ * tinydrm_gem_create_object - Create shmem GEM object
+ * @drm: DRM device
+ * @size: Size
  *
- * Returns:
- * A pointer to a newly created GEM object or an ERR_PTR-encoded negative
- * error code on failure.
+ * This function set cache mode to cached. Drivers should use this as their
+ * &drm_driver->gem_create_object callback.
  */
-struct drm_gem_object *
-tinydrm_gem_cma_prime_import_sg_table(struct drm_device *drm,
-				      struct dma_buf_attachment *attach,
-				      struct sg_table *sgt)
+struct drm_gem_object *tinydrm_gem_create_object(struct drm_device *drm,
+						 size_t size)
 {
-	struct drm_gem_cma_object *cma_obj;
-	struct drm_gem_object *obj;
-	void *vaddr;
+	struct drm_gem_shmem_object *obj;
 
-	vaddr = dma_buf_vmap(attach->dmabuf);
-	if (!vaddr) {
-		DRM_ERROR("Failed to vmap PRIME buffer\n");
+	obj = kzalloc(sizeof(*obj), GFP_KERNEL);
+	if (!obj)
 		return ERR_PTR(-ENOMEM);
-	}
-
-	obj = drm_gem_cma_prime_import_sg_table(drm, attach, sgt);
-	if (IS_ERR(obj)) {
-		dma_buf_vunmap(attach->dmabuf, vaddr);
-		return obj;
-	}
 
-	cma_obj = to_drm_gem_cma_obj(obj);
-	cma_obj->vaddr = vaddr;
+	obj->cache_mode = DRM_GEM_SHMEM_BO_CACHED;
 
-	return obj;
+	return &obj->base;
 }
-EXPORT_SYMBOL(tinydrm_gem_cma_prime_import_sg_table);
-
-/**
- * tinydrm_gem_cma_free_object - Free resources associated with a CMA GEM
- *                               object
- * @gem_obj: GEM object to free
- *
- * This function frees the backing memory of the CMA GEM object, cleans up the
- * GEM object state and frees the memory used to store the object itself using
- * drm_gem_cma_free_object(). It also handles PRIME buffers which has the kernel
- * virtual address set by tinydrm_gem_cma_prime_import_sg_table(). Drivers
- * can use this as their &drm_driver->gem_free_object callback.
- */
-void tinydrm_gem_cma_free_object(struct drm_gem_object *gem_obj)
-{
-	if (gem_obj->import_attach) {
-		struct drm_gem_cma_object *cma_obj;
-
-		cma_obj = to_drm_gem_cma_obj(gem_obj);
-		dma_buf_vunmap(gem_obj->import_attach->dmabuf, cma_obj->vaddr);
-		cma_obj->vaddr = NULL;
-	}
-
-	drm_gem_cma_free_object(gem_obj);
-}
-EXPORT_SYMBOL_GPL(tinydrm_gem_cma_free_object);
+EXPORT_SYMBOL(tinydrm_gem_create_object);
 
 static struct drm_framebuffer *
 tinydrm_fb_create(struct drm_device *drm, struct drm_file *file_priv,
@@ -128,7 +82,7 @@ tinydrm_fb_create(struct drm_device *drm, struct drm_file *file_priv,
 {
 	struct tinydrm_device *tdev = drm->dev_private;
 
-	return drm_fb_cma_create_with_funcs(drm, file_priv, mode_cmd,
+	return drm_fb_gem_create_with_funcs(drm, file_priv, mode_cmd,
 					    tdev->fb_funcs);
 }
 
@@ -210,38 +164,64 @@ int devm_tinydrm_init(struct device *parent, struct tinydrm_device *tdev,
 }
 EXPORT_SYMBOL(devm_tinydrm_init);
 
-static int tinydrm_register(struct tinydrm_device *tdev)
+static int tinydrm_fbdev_probe(struct drm_fb_helper *helper,
+			       struct drm_fb_helper_surface_size *sizes)
+{
+	struct tinydrm_device *tdev = helper->dev->dev_private;
+
+	return drm_fb_shmem_fbdev_probe(helper, sizes, tdev->fb_funcs);
+}
+
+static const struct drm_fb_helper_funcs tinydrm_fb_helper_funcs = {
+	.fb_probe = tinydrm_fbdev_probe,
+};
+
+static int tinydrm_fbdev_init(struct tinydrm_device *tdev)
 {
 	struct drm_device *drm = tdev->drm;
 	int bpp = drm->mode_config.preferred_depth;
-	struct drm_fbdev_cma *fbdev;
+	int ret;
+
+	tdev->fbdev = kzalloc(sizeof(*tdev->fbdev), GFP_KERNEL);
+	if (!tdev->fbdev)
+		return -ENOMEM;
+
+	ret = drm_fb_helper_simple_init(drm, tdev->fbdev, bpp ? bpp : 32,
+					drm->mode_config.num_connector,
+					&tinydrm_fb_helper_funcs);
+	if (ret) {
+		kfree(tdev->fbdev);
+		tdev->fbdev = NULL;
+		return ret;
+	}
+
+	return 0;
+}
+
+static int tinydrm_register(struct tinydrm_device *tdev)
+{
 	int ret;
 
 	ret = drm_dev_register(tdev->drm, 0);
 	if (ret)
 		return ret;
 
-	fbdev = drm_fbdev_cma_init_with_funcs(drm, bpp ? bpp : 32,
-					      drm->mode_config.num_connector,
-					      tdev->fb_funcs);
-	if (IS_ERR(fbdev))
-		DRM_ERROR("Failed to initialize fbdev: %ld\n", PTR_ERR(fbdev));
-	else
-		tdev->fbdev_cma = fbdev;
+	if (tinydrm_fbdev_init(tdev))
+		DRM_WARN("Failed to initialize fbdev\n");
 
-	return 0;
+	return ret;
 }
 
 static void tinydrm_unregister(struct tinydrm_device *tdev)
 {
-	struct drm_fbdev_cma *fbdev_cma = tdev->fbdev_cma;
+	struct drm_fb_helper *fbdev = tdev->fbdev;
 
-	drm_atomic_helper_shutdown(tdev->drm);
 	/* don't restore fbdev in lastclose, keep pipeline disabled */
-	tdev->fbdev_cma = NULL;
+	tdev->fbdev = NULL;
+	drm_atomic_helper_shutdown(tdev->drm);
+	drm_fb_helper_simple_fini(fbdev);
 	drm_dev_unregister(tdev->drm);
-	if (fbdev_cma)
-		drm_fbdev_cma_fini(fbdev_cma);
+	kfree(fbdev);
 }
 
 static void devm_tinydrm_register_release(void *data)
@@ -311,10 +291,12 @@ int tinydrm_suspend(struct tinydrm_device *tdev)
 		return -EINVAL;
 	}
 
-	drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 1);
+	if (tdev->fbdev)
+		drm_fb_helper_set_suspend_unlocked(tdev->fbdev, 1);
 	state = drm_atomic_helper_suspend(tdev->drm);
 	if (IS_ERR(state)) {
-		drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 0);
+		if (tdev->fbdev)
+			drm_fb_helper_set_suspend_unlocked(tdev->fbdev, 0);
 		return PTR_ERR(state);
 	}
 
@@ -352,7 +334,8 @@ int tinydrm_resume(struct tinydrm_device *tdev)
 		return ret;
 	}
 
-	drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 0);
+	if (tdev->fbdev)
+		drm_fb_helper_set_suspend_unlocked(tdev->fbdev, 0);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
index ec43fb7..a30518c 100644
--- a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
@@ -145,7 +145,7 @@ EXPORT_SYMBOL(tinydrm_display_pipe_update);
  * @pipe: Simple display pipe
  * @plane_state: Plane state
  *
- * This function uses drm_fb_cma_prepare_fb() to check if the plane FB has an
+ * This function uses drm_fb_shmem_prepare_fb() to check if the plane FB has a
  * dma-buf attached, extracts the exclusive fence and attaches it to plane
  * state for the atomic helper to wait on. Drivers can use this as their
  * &drm_simple_display_pipe_funcs->prepare_fb callback.
@@ -153,7 +153,7 @@ EXPORT_SYMBOL(tinydrm_display_pipe_update);
 int tinydrm_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
 				    struct drm_plane_state *plane_state)
 {
-	return drm_fb_cma_prepare_fb(&pipe->plane, plane_state);
+	return drm_fb_gem_prepare_fb(&pipe->plane, plane_state);
 }
 EXPORT_SYMBOL(tinydrm_display_pipe_prepare_fb);
 
diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
index 482ff1c3..8d11cd0 100644
--- a/drivers/gpu/drm/tinydrm/mi0283qt.c
+++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
@@ -132,7 +132,7 @@ static const struct drm_display_mode mi0283qt_mode = {
 	TINYDRM_MODE(320, 240, 58, 43),
 };
 
-DEFINE_DRM_GEM_CMA_FOPS(mi0283qt_fops);
+DEFINE_DRM_GEM_SHMEM_FOPS(mi0283qt_fops);
 
 static struct drm_driver mi0283qt_driver = {
 	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME |
diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
index c83eeb7..a4d8c93 100644
--- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
+++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
@@ -156,10 +156,11 @@ EXPORT_SYMBOL(mipi_dbi_command_buf);
 static int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
 				struct drm_clip_rect *clip, bool swap)
 {
-	struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
-	struct dma_buf_attachment *import_attach = cma_obj->base.import_attach;
+	struct drm_gem_object *gem_obj = drm_fb_gem_get_obj(fb, 0);
+	struct drm_gem_shmem_object *obj = to_drm_gem_shmem_obj(gem_obj);
+	struct dma_buf_attachment *import_attach = gem_obj->import_attach;
 	struct drm_format_name_buf format_name;
-	void *src = cma_obj->vaddr;
+	void *src = obj->vaddr;
 	int ret = 0;
 
 	if (import_attach) {
@@ -198,7 +199,8 @@ static int mipi_dbi_fb_dirty(struct drm_framebuffer *fb,
 			     struct drm_clip_rect *clips,
 			     unsigned int num_clips)
 {
-	struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
+	struct drm_gem_object *gem_obj = drm_fb_gem_get_obj(fb, 0);
+	struct drm_gem_shmem_object *obj = to_drm_gem_shmem_obj(gem_obj);
 	struct tinydrm_device *tdev = fb->dev->dev_private;
 	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
 	bool swap = mipi->swap_bytes;
@@ -216,6 +218,10 @@ static int mipi_dbi_fb_dirty(struct drm_framebuffer *fb,
 	if (tdev->pipe.plane.fb != fb)
 		goto out_unlock;
 
+	ret = drm_gem_shmem_vmap(obj);
+	if (ret)
+		goto out_unlock;
+
 	full = tinydrm_merge_clips(&clip, clips, num_clips, flags,
 				   fb->width, fb->height);
 
@@ -229,7 +235,7 @@ static int mipi_dbi_fb_dirty(struct drm_framebuffer *fb,
 		if (ret)
 			goto out_unlock;
 	} else {
-		tr = cma_obj->vaddr;
+		tr = obj->vaddr;
 	}
 
 	mipi_dbi_command(mipi, MIPI_DCS_SET_COLUMN_ADDRESS,
@@ -253,8 +259,8 @@ static int mipi_dbi_fb_dirty(struct drm_framebuffer *fb,
 }
 
 static const struct drm_framebuffer_funcs mipi_dbi_fb_funcs = {
-	.destroy	= drm_fb_cma_destroy,
-	.create_handle	= drm_fb_cma_create_handle,
+	.destroy	= drm_fb_gem_destroy,
+	.create_handle	= drm_fb_gem_create_handle,
 	.dirty		= mipi_dbi_fb_dirty,
 };
 
@@ -807,31 +813,12 @@ int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi,
 {
 	size_t tx_size = tinydrm_spi_max_transfer_size(spi, 0);
 	struct device *dev = &spi->dev;
-	int ret;
 
 	if (tx_size < 16) {
 		DRM_ERROR("SPI transmit buffer too small: %zu\n", tx_size);
 		return -EINVAL;
 	}
 
-	/*
-	 * Even though it's not the SPI device that does DMA (the master does),
-	 * the dma mask is necessary for the dma_alloc_wc() in
-	 * drm_gem_cma_create(). The dma_addr returned will be a physical
-	 * adddress which might be different from the bus address, but this is
-	 * not a problem since the address will not be used.
-	 * The virtual address is used in the transfer and the SPI core
-	 * re-maps it on the SPI master device using the DMA streaming API
-	 * (spi_map_buf()).
-	 */
-	if (!dev->coherent_dma_mask) {
-		ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
-		if (ret) {
-			dev_warn(dev, "Failed to set dma mask %d\n", ret);
-			return ret;
-		}
-	}
-
 	mipi->spi = spi;
 	mipi->read_commands = mipi_dbi_dcs_read_commands;
 
@@ -966,7 +953,7 @@ static const struct file_operations mipi_dbi_debugfs_command_fops = {
 };
 
 static const struct drm_info_list mipi_dbi_debugfs_list[] = {
-	{ "fb",   drm_fb_cma_debugfs_show, 0 },
+	{ "fb",   drm_fb_shmem_debugfs_show, 0 },
 };
 
 /**
diff --git a/include/drm/tinydrm/tinydrm.h b/include/drm/tinydrm/tinydrm.h
index 00b800d..93d7a03 100644
--- a/include/drm/tinydrm/tinydrm.h
+++ b/include/drm/tinydrm/tinydrm.h
@@ -10,8 +10,9 @@
 #ifndef __LINUX_TINYDRM_H
 #define __LINUX_TINYDRM_H
 
-#include <drm/drm_gem_cma_helper.h>
-#include <drm/drm_fb_cma_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
+#include <drm/drm_fb_gem_helper.h>
+#include <drm/drm_fb_shmem_helper.h>
 #include <drm/drm_simple_kms_helper.h>
 
 /**
@@ -19,7 +20,7 @@
  * @drm: DRM device
  * @pipe: Display pipe structure
  * @dirty_lock: Serializes framebuffer flushing
- * @fbdev_cma: CMA fbdev structure
+ * @fbdev: fbdev helper
  * @suspend_state: Atomic state when suspended
  * @fb_funcs: Framebuffer functions used when creating framebuffers
  */
@@ -27,7 +28,7 @@ struct tinydrm_device {
 	struct drm_device *drm;
 	struct drm_simple_display_pipe pipe;
 	struct mutex dirty_lock;
-	struct drm_fbdev_cma *fbdev_cma;
+	struct drm_fb_helper *fbdev;
 	struct drm_atomic_state *suspend_state;
 	const struct drm_framebuffer_funcs *fb_funcs;
 };
@@ -45,20 +46,8 @@ pipe_to_tinydrm(struct drm_simple_display_pipe *pipe)
  * the &drm_driver structure.
  */
 #define TINYDRM_GEM_DRIVER_OPS \
-	.gem_free_object	= tinydrm_gem_cma_free_object, \
-	.gem_vm_ops		= &drm_gem_cma_vm_ops, \
-	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd, \
-	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle, \
-	.gem_prime_import	= drm_gem_prime_import, \
-	.gem_prime_export	= drm_gem_prime_export, \
-	.gem_prime_get_sg_table	= drm_gem_cma_prime_get_sg_table, \
-	.gem_prime_import_sg_table = tinydrm_gem_cma_prime_import_sg_table, \
-	.gem_prime_vmap		= drm_gem_cma_prime_vmap, \
-	.gem_prime_vunmap	= drm_gem_cma_prime_vunmap, \
-	.gem_prime_mmap		= drm_gem_cma_prime_mmap, \
-	.dumb_create		= drm_gem_cma_dumb_create, \
-	.dumb_map_offset	= drm_gem_cma_dumb_map_offset, \
-	.dumb_destroy		= drm_gem_dumb_destroy
+	.gem_create_object = tinydrm_gem_create_object, \
+	DRM_GEM_SHMEM_DRIVER_OPS
 
 /**
  * TINYDRM_MODE - tinydrm display mode
@@ -84,11 +73,8 @@ pipe_to_tinydrm(struct drm_simple_display_pipe *pipe)
 	.clock = 1 /* pass validation */
 
 void tinydrm_lastclose(struct drm_device *drm);
-void tinydrm_gem_cma_free_object(struct drm_gem_object *gem_obj);
-struct drm_gem_object *
-tinydrm_gem_cma_prime_import_sg_table(struct drm_device *drm,
-				      struct dma_buf_attachment *attach,
-				      struct sg_table *sgt);
+struct drm_gem_object *tinydrm_gem_create_object(struct drm_device *drm,
+						 size_t size);
 int devm_tinydrm_init(struct device *parent, struct tinydrm_device *tdev,
 		      const struct drm_framebuffer_funcs *fb_funcs,
 		      struct drm_driver *driver);
-- 
2.7.4

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

Re: [RFC 2/7] drm: Add GEM backed framebuffer library

[Noralf Trønnes]

Den 12.07.2017 15.46, skrev Noralf Trønnes:
> Add a library for drivers that can use a simple representation
> of a GEM backed framebuffer.
>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---

This patch adds a gem backed drm_framebuffer like this:

struct drm_fb_gem {
     /**
      * @base: Base DRM framebuffer
      */
     struct drm_framebuffer base;
     /**
      * @obj: GEM object array backing the framebuffer. One object per
      * plane.
      */
     struct drm_gem_object *obj[4];
};

Now I wonder if it would be better to extend drm_framebuffer instead:

  struct drm_framebuffer {
+    /**
+     * @obj: GEM objects backing the framebuffer, one per plane (optional).
+     */
+    struct drm_gem_object *obj[4];
  };

The reason for this, is that I have looked through all drivers that
subclass drm_framebuffer and found this:
- 11 drivers just adds drm_gem_object
- 2 drivers adds drm_gem_object and some more
- 6 drivers adds drm_gem_object indirectly through their subclassed
   drm_gem_object
- 3 drivers doesn't add drm_gem_object


Full details below:

Adds only drm_gem_object
------------------------

struct amdgpu_framebuffer {
     struct drm_framebuffer base;
     struct drm_gem_object *obj;
};

struct ast_framebuffer {
     struct drm_framebuffer base;
     struct drm_gem_object *obj;
};

struct bochs_framebuffer {
     struct drm_framebuffer base;
     struct drm_gem_object *obj;
};

struct cirrus_framebuffer {
     struct drm_framebuffer        base;
     struct drm_gem_object *obj;
};

struct drm_fb_cma {
     struct drm_framebuffer        fb;
     struct drm_gem_cma_object    *obj[4];
};

struct hibmc_framebuffer {
     struct drm_framebuffer fb;
     struct drm_gem_object *obj;
};

struct mga_framebuffer {
     struct drm_framebuffer base;
     struct drm_gem_object *obj;
};

struct mtk_drm_fb {
     struct drm_framebuffer    base;
     /* For now we only support a single plane */
     struct drm_gem_object    *gem_obj;
};

struct qxl_framebuffer {
     struct drm_framebuffer base;
     struct drm_gem_object *obj;
};

struct radeon_framebuffer {
     struct drm_framebuffer base;
     struct drm_gem_object *obj;
};

struct rockchip_drm_fb {
     struct drm_framebuffer fb;
     struct drm_gem_object *obj[ROCKCHIP_MAX_FB_BUFFER];
};
#define ROCKCHIP_MAX_FB_BUFFER    3


Adds drm_gem_object and some more
---------------------------------

struct msm_framebuffer {
     struct drm_framebuffer base;
     const struct msm_format *format;
     struct drm_gem_object *planes[MAX_PLANE];
};
#define MAX_PLANE    4

struct virtio_gpu_framebuffer {
     struct drm_framebuffer base;
     struct drm_gem_object *obj;
     int x1, y1, x2, y2; /* dirty rect */
     spinlock_t dirty_lock;
     uint32_t hw_res_handle;
};


Indirectly adds a drm_gem_object
--------------------------------

struct armada_framebuffer {
     struct drm_framebuffer    fb;
     struct armada_gem_object *obj;
     uint8_t            fmt;
     uint8_t            mod;
};

struct exynos_drm_fb {
     struct drm_framebuffer    fb;
     struct exynos_drm_gem    *exynos_gem[MAX_FB_BUFFER];
     dma_addr_t            dma_addr[MAX_FB_BUFFER];
};
#define MAX_FB_BUFFER    4

struct intel_framebuffer {
     struct drm_framebuffer base;
     struct drm_i915_gem_object *obj;
     struct intel_rotation_info rot_info;

     /* for each plane in the normal GTT view */
     struct {
         unsigned int x, y;
     } normal[2];
     /* for each plane in the rotated GTT view */
     struct {
         unsigned int x, y;
         unsigned int pitch; /* pixels */
     } rotated[2];
};

struct nouveau_framebuffer {
     struct drm_framebuffer base;
     struct nouveau_bo *nvbo;
     struct nvkm_vma vma;
     u32 r_handle;
     u32 r_format;
     u32 r_pitch;
     struct nvif_object h_base[4];
     struct nvif_object h_core;
};

struct tegra_fb {
     struct drm_framebuffer base;
     struct tegra_bo **planes;
     unsigned int num_planes;
};

struct udl_framebuffer {
     struct drm_framebuffer base;
     struct udl_gem_object *obj;
     bool active_16; /* active on the 16-bit channel */
};


Does not add drm_gem_object
---------------------------

struct omap_framebuffer {
     struct drm_framebuffer base;
     int pin_count;
     const struct drm_format_info *format;
     enum omap_color_mode dss_format;
     struct plane planes[2];
     /* lock for pinning (pin_count and planes.paddr) */
     struct mutex lock;
};

struct psb_framebuffer {
     struct drm_framebuffer base;
     struct address_space *addr_space;
     struct fb_info *fbdev;
     struct gtt_range *gtt;
};

struct vmw_framebuffer {
     struct drm_framebuffer base;
     int (*pin)(struct vmw_framebuffer *fb);
     int (*unpin)(struct vmw_framebuffer *fb);
     bool dmabuf;
     struct ttm_base_object *user_obj;
     uint32_t user_handle;
};


Noralf.


>   drivers/gpu/drm/Makefile            |   2 +-
>   drivers/gpu/drm/drm_fb_gem_helper.c | 248 ++++++++++++++++++++++++++++++++++++
>   include/drm/drm_fb_gem_helper.h     |  64 ++++++++++
>   3 files changed, 313 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/gpu/drm/drm_fb_gem_helper.c
>   create mode 100644 include/drm/drm_fb_gem_helper.h
>
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 24a066e..83d8b09 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -33,7 +33,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
>   		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
>   		drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
>   		drm_simple_kms_helper.o drm_modeset_helper.o \
> -		drm_scdc_helper.o
> +		drm_scdc_helper.o drm_fb_gem_helper.o
>   
>   drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o
>   drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> diff --git a/drivers/gpu/drm/drm_fb_gem_helper.c b/drivers/gpu/drm/drm_fb_gem_helper.c
> new file mode 100644
> index 0000000..9a0da09
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_fb_gem_helper.c
> @@ -0,0 +1,248 @@
> +/*
> + * drm fb gem helper functions
> + *
> + * Copyright (C) 2017 Noralf Trønnes
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/dma-buf.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/module.h>
> +#include <linux/reservation.h>
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_fb_helper.h>
> +#include <drm/drm_fb_gem_helper.h>
> +#include <drm/drm_gem.h>
> +
> +/**
> + * drm_fb_gem_get_obj() - Get GEM object for framebuffer
> + * @fb: The framebuffer
> + * @plane: Which plane
> + *
> + * Returns the GEM object for given framebuffer.
> + */
> +struct drm_gem_object *drm_fb_gem_get_obj(struct drm_framebuffer *fb,
> +					  unsigned int plane)
> +{
> +	struct drm_fb_gem *fb_gem = to_fb_gem(fb);
> +
> +	if (plane >= 4)
> +		return NULL;
> +
> +	return fb_gem->obj[plane];
> +}
> +EXPORT_SYMBOL_GPL(drm_fb_gem_get_obj);
> +
> +/**
> + * drm_fb_gem_alloc - Allocate GEM backed framebuffer
> + * @dev: DRM device
> + * @mode_cmd: metadata from the userspace fb creation request
> + * @obj: GEM object nacking the framebuffer
> + * @num_planes: Number of planes
> + * @funcs: vtable to be used for the new framebuffer object
> + *
> + * Returns:
> + * Allocated struct drm_fb_gem * or error encoded pointer.
> + */
> +struct drm_fb_gem *
> +drm_fb_gem_alloc(struct drm_device *dev,
> +		 const struct drm_mode_fb_cmd2 *mode_cmd,
> +		 struct drm_gem_object **obj, unsigned int num_planes,
> +		 const struct drm_framebuffer_funcs *funcs)
> +{
> +	struct drm_fb_gem *fb_gem;
> +	int ret, i;
> +
> +	fb_gem = kzalloc(sizeof(*fb_gem), GFP_KERNEL);
> +	if (!fb_gem)
> +		return ERR_PTR(-ENOMEM);
> +
> +	drm_helper_mode_fill_fb_struct(dev, &fb_gem->base, mode_cmd);
> +
> +	for (i = 0; i < num_planes; i++)
> +		fb_gem->obj[i] = obj[i];
> +
> +	ret = drm_framebuffer_init(dev, &fb_gem->base, funcs);
> +	if (ret) {
> +		dev_err(dev->dev, "Failed to initialize framebuffer: %d\n", ret);
> +		kfree(fb_gem);
> +		return ERR_PTR(ret);
> +	}
> +
> +	return fb_gem;
> +}
> +EXPORT_SYMBOL(drm_fb_gem_alloc);
> +
> +/**
> + * drm_fb_gem_destroy - Free GEM backed framebuffer
> + * @fb: DRM framebuffer
> + *
> + * Frees a GEM backed framebuffer with it's backing buffer(s) and the structure
> + * itself. Drivers can use this as their &drm_framebuffer_funcs->destroy
> + * callback.
> + */
> +void drm_fb_gem_destroy(struct drm_framebuffer *fb)
> +{
> +	struct drm_fb_gem *fb_gem = to_fb_gem(fb);
> +	int i;
> +
> +	for (i = 0; i < 4; i++) {
> +		if (fb_gem->obj[i])
> +			drm_gem_object_put_unlocked(fb_gem->obj[i]);
> +	}
> +
> +	drm_framebuffer_cleanup(fb);
> +	kfree(fb_gem);
> +}
> +EXPORT_SYMBOL(drm_fb_gem_destroy);
> +
> +/**
> + * drm_fb_gem_create_handle - Create handle for GEM backed framebuffer
> + * @fb: DRM framebuffer
> + * @file: drm file
> + * @handle: handle created
> + *
> + * Drivers can use this as their &drm_framebuffer_funcs->create_handle
> + * callback.
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + */
> +int drm_fb_gem_create_handle(struct drm_framebuffer *fb, struct drm_file *file,
> +			     unsigned int *handle)
> +{
> +	struct drm_fb_gem *fb_gem = to_fb_gem(fb);
> +
> +	return drm_gem_handle_create(file, fb_gem->obj[0], handle);
> +}
> +EXPORT_SYMBOL(drm_fb_gem_create_handle);
> +
> +/**
> + * drm_fb_gem_create_with_funcs() - helper function for the
> + *                                  &drm_mode_config_funcs.fb_create
> + *                                  callback
> + * @dev: DRM device
> + * @file: drm file for the ioctl call
> + * @mode_cmd: metadata from the userspace fb creation request
> + * @funcs: vtable to be used for the new framebuffer object
> + *
> + * This can be used to set &drm_framebuffer_funcs for drivers that need the
> + * &drm_framebuffer_funcs.dirty callback. Use drm_fb_gem_create() if you don't
> + * need to change &drm_framebuffer_funcs.
> + */
> +struct drm_framebuffer *
> +drm_fb_gem_create_with_funcs(struct drm_device *dev, struct drm_file *file,
> +			     const struct drm_mode_fb_cmd2 *mode_cmd,
> +			     const struct drm_framebuffer_funcs *funcs)
> +{
> +	const struct drm_format_info *info;
> +	struct drm_gem_object *objs[4];
> +	struct drm_fb_gem *fb_gem;
> +	int ret, i;
> +
> +	info = drm_get_format_info(dev, mode_cmd);
> +	if (!info)
> +		return ERR_PTR(-EINVAL);
> +
> +	for (i = 0; i < info->num_planes; i++) {
> +		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
> +		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
> +		unsigned int min_size;
> +
> +		objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]);
> +		if (!objs[i]) {
> +			dev_err(dev->dev, "Failed to lookup GEM object\n");
> +			ret = -ENOENT;
> +			goto err_gem_object_put;
> +		}
> +
> +		min_size = (height - 1) * mode_cmd->pitches[i]
> +			 + width * info->cpp[i]
> +			 + mode_cmd->offsets[i];
> +
> +		if (objs[i]->size < min_size) {
> +			drm_gem_object_put_unlocked(objs[i]);
> +			ret = -EINVAL;
> +			goto err_gem_object_put;
> +		}
> +	}
> +
> +	fb_gem = drm_fb_gem_alloc(dev, mode_cmd, objs, i, funcs);
> +	if (IS_ERR(fb_gem)) {
> +		ret = PTR_ERR(fb_gem);
> +		goto err_gem_object_put;
> +	}
> +
> +	return &fb_gem->base;
> +
> +err_gem_object_put:
> +	for (i--; i >= 0; i--)
> +		drm_gem_object_put_unlocked(objs[i]);
> +
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(drm_fb_gem_create_with_funcs);
> +
> +static struct drm_framebuffer_funcs drm_fb_gem_fb_funcs = {
> +	.destroy	= drm_fb_gem_destroy,
> +	.create_handle	= drm_fb_gem_create_handle,
> +};
> +
> +/**
> + * drm_fb_gem_create() - &drm_mode_config_funcs.fb_create callback function
> + * @dev: DRM device
> + * @file: drm file for the ioctl call
> + * @mode_cmd: metadata from the userspace fb creation request
> + *
> + * If your hardware has special alignment or pitch requirements these should be
> + * checked before calling this function. Use drm_fb_gem_create_with_funcs() if
> + * you need to set &drm_framebuffer_funcs.dirty.
> + */
> +struct drm_framebuffer *
> +drm_fb_gem_create(struct drm_device *dev, struct drm_file *file,
> +		  const struct drm_mode_fb_cmd2 *mode_cmd)
> +{
> +	return drm_fb_gem_create_with_funcs(dev, file, mode_cmd,
> +					    &drm_fb_gem_fb_funcs);
> +}
> +EXPORT_SYMBOL_GPL(drm_fb_gem_create);
> +
> +/**
> + * drm_fb_gem_prepare_fb() - Prepare gem framebuffer
> + * @plane: Which plane
> + * @state: Plane state attach fence to
> + *
> + * This should be set as the &struct drm_plane_helper_funcs.prepare_fb hook.
> + *
> + * This function checks if the plane FB has an dma-buf attached, extracts
> + * the exclusive fence and attaches it to plane state for the atomic helper
> + * to wait on.
> + *
> + * There is no need for cleanup_fb for gem based framebuffer drivers.
> + */
> +int drm_fb_gem_prepare_fb(struct drm_plane *plane,
> +			  struct drm_plane_state *state)
> +{
> +	struct dma_buf *dma_buf;
> +	struct dma_fence *fence;
> +
> +	if ((plane->state->fb == state->fb) || !state->fb)
> +		return 0;
> +
> +	dma_buf = drm_fb_gem_get_obj(state->fb, 0)->dma_buf;
> +	if (dma_buf) {
> +		fence = reservation_object_get_excl_rcu(dma_buf->resv);
> +		drm_atomic_set_fence_for_plane(state, fence);
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(drm_fb_gem_prepare_fb);
> diff --git a/include/drm/drm_fb_gem_helper.h b/include/drm/drm_fb_gem_helper.h
> new file mode 100644
> index 0000000..405a1e1
> --- /dev/null
> +++ b/include/drm/drm_fb_gem_helper.h
> @@ -0,0 +1,64 @@
> +#ifndef __DRM_FB_GEM_HELPER_H__
> +#define __DRM_FB_GEM_HELPER_H__
> +
> +#include <drm/drm_framebuffer.h>
> +
> +struct drm_gem_shmem_object;
> +struct drm_mode_fb_cmd2;
> +struct drm_plane;
> +struct drm_plane_state;
> +
> +/**
> + * struct drm_fb_gem - GEM backed framebuffer
> + */
> +struct drm_fb_gem {
> +	/**
> +	 * @base: Base DRM framebuffer
> +	 */
> +	struct drm_framebuffer base;
> +	/**
> +	 * @obj: GEM object array backing the framebuffer. One object per
> +	 * plane.
> +	 */
> +	struct drm_gem_object *obj[4];
> +};
> +
> +static inline struct drm_fb_gem *to_fb_gem(struct drm_framebuffer *fb)
> +{
> +	return container_of(fb, struct drm_fb_gem, base);
> +}
> +
> +struct drm_gem_object *drm_fb_gem_get_obj(struct drm_framebuffer *fb,
> +					  unsigned int plane);
> +struct drm_fb_gem *
> +drm_fb_gem_alloc(struct drm_device *dev,
> +		 const struct drm_mode_fb_cmd2 *mode_cmd,
> +		 struct drm_gem_object **obj, unsigned int num_planes,
> +		 const struct drm_framebuffer_funcs *funcs);
> +void drm_fb_gem_destroy(struct drm_framebuffer *fb);
> +int drm_fb_gem_create_handle(struct drm_framebuffer *fb, struct drm_file *file,
> +			     unsigned int *handle);
> +
> +struct drm_framebuffer *
> +drm_fb_gem_create_with_funcs(struct drm_device *dev, struct drm_file *file,
> +			     const struct drm_mode_fb_cmd2 *mode_cmd,
> +			     const struct drm_framebuffer_funcs *funcs);
> +struct drm_framebuffer *
> +drm_fb_gem_create(struct drm_device *dev, struct drm_file *file,
> +		  const struct drm_mode_fb_cmd2 *mode_cmd);
> +
> +
> +int drm_fb_gem_prepare_fb(struct drm_plane *plane,
> +			  struct drm_plane_state *state);
> +
> +
> +
> +
> +#ifdef CONFIG_DEBUG_FS
> +struct seq_file;
> +
> +int drm_fb_gem_debugfs_show(struct seq_file *m, void *arg);
> +#endif
> +
> +#endif
> +

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

Re: [RFC 1/7] drm/gem: Add drm_gem_dumb_map_offset()

[Noralf Trønnes]

Den 12.07.2017 15.45, skrev Noralf Trønnes:
> Add a common drm_driver.dumb_map_offset function for GEM backed drivers.
>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>   drivers/gpu/drm/drm_gem.c | 35 +++++++++++++++++++++++++++++++++++
>   include/drm/drm_gem.h     |  2 ++
>   2 files changed, 37 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 8dc1106..44ecbaa 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -311,6 +311,41 @@ drm_gem_handle_delete(struct drm_file *filp, u32 handle)
>   EXPORT_SYMBOL(drm_gem_handle_delete);
>   
>   /**
> + * drm_gem_dumb_map_offset - return the fake mmap offset for a gem object
> + * @file: drm file-private structure containing the gem object
> + * @dev: corresponding drm_device
> + * @handle: gem object handle
> + * @offset: return location for the fake mmap offset
> + *
> + * This implements the &drm_driver.dumb_map_offset kms driver callback for
> + * drivers which use gem to manage their backing storage.
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + */
> +int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
> +			    u32 handle, u64 *offset)
> +{
> +	struct drm_gem_object *obj;
> +	int ret;
> +
> +	obj = drm_gem_object_lookup(file, handle);
> +	if (!obj)
> +		return -ENOENT;
> +
> +	ret = drm_gem_create_mmap_offset(obj);

There are 3 drm_driver->dumb_map_offset implementations that
don't call drm_gem_create_mmap_offset():
- drm_gem_cma_dumb_map_offset()
- exynos_drm_gem_dumb_map_offset()
- tegra_bo_dumb_map_offset()

They do it during object creation.

exynos have this commit:
drm/exynos: create a fake mmap offset with gem creation
48cf53f4343ae12ddc1c60dbe116161ecf7a2885

I looked at the discussion but didn't understand the rationale:
https://lists.freedesktop.org/archives/dri-devel/2015-August/088541.html

I see that it's ok to call drm_gem_create_mmap_offset() multiple times,
so it's not really a problem for this function, but it would be nice to
know why for my shmem gem library which is based on the cma library.

Noralf.


> +	if (ret)
> +		goto out;
> +
> +	*offset = drm_vma_node_offset_addr(&obj->vma_node);
> +out:
> +	drm_gem_object_put_unlocked(obj);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_dumb_map_offset);
> +
> +/**
>    * drm_gem_dumb_destroy - dumb fb callback helper for gem based drivers
>    * @file: drm file-private structure to remove the dumb handle from
>    * @dev: corresponding drm_device
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index 4a9d231..9c55c2a 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -302,6 +302,8 @@ void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages,
>   		bool dirty, bool accessed);
>   
>   struct drm_gem_object *drm_gem_object_lookup(struct drm_file *filp, u32 handle);
> +int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
> +			    u32 handle, u64 *offset);
>   int drm_gem_dumb_destroy(struct drm_file *file,
>   			 struct drm_device *dev,
>   			 uint32_t handle);

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

Re: [RFC 2/7] drm: Add GEM backed framebuffer library

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 978 bytes --]

Noralf Trønnes <noralf@tronnes.org> writes:

> Den 12.07.2017 15.46, skrev Noralf Trønnes:
>> Add a library for drivers that can use a simple representation
>> of a GEM backed framebuffer.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>
> This patch adds a gem backed drm_framebuffer like this:
>
> struct drm_fb_gem {
>      /**
>       * @base: Base DRM framebuffer
>       */
>      struct drm_framebuffer base;
>      /**
>       * @obj: GEM object array backing the framebuffer. One object per
>       * plane.
>       */
>      struct drm_gem_object *obj[4];
> };
>
> Now I wonder if it would be better to extend drm_framebuffer instead:
>
>   struct drm_framebuffer {
> +    /**
> +     * @obj: GEM objects backing the framebuffer, one per plane (optional).
> +     */
> +    struct drm_gem_object *obj[4];
>   };

FWIW, I would love to see this tried.  I think we would end up with some
nice cleanups if we did so.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [RFC 1/7] drm/gem: Add drm_gem_dumb_map_offset()

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 475 bytes --]

Noralf Trønnes <noralf@tronnes.org> writes:

> Add a common drm_driver.dumb_map_offset function for GEM backed drivers.
>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

Instead of just introducing the new code, could we replace CMA's code
with calls to this at the same time?

I suspect that if we had CMA subclass from drm_fb_gem (or we move the
gem objects to the base class), we could remove a lot of its code that
you're copying in patch 2, as well.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [RFC 1/7] drm/gem: Add drm_gem_dumb_map_offset()

[Noralf Trønnes]

Den 19.07.2017 23.01, skrev Eric Anholt:
> Noralf Trønnes <noralf@tronnes.org> writes:
>
>> Add a common drm_driver.dumb_map_offset function for GEM backed drivers.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> Instead of just introducing the new code, could we replace CMA's code
> with calls to this at the same time?

I have gone through all the drm_driver->dumb_map_offset
implementations and found 23 drivers that could use it:
- 18 drivers use drm_gem_cma_dumb_map_offset()
- exynos_drm_gem_dumb_map_offset()
- mtk_drm_gem_dumb_map_offset()
- psb_gem_dumb_map_gtt()
- rockchip_gem_dumb_map_offset()
- tegra_bo_dumb_map_offset()

vgem_gem_dumb_map() can't use it because of this check:
     if (!obj->filp) {
         ret = -EINVAL;
         goto unref;
     }

armada_gem_dumb_map_offset() can't use it because of this check:
     /* Don't allow imported objects to be mapped */
     if (obj->obj.import_attach) {
         ret = -EINVAL;
         goto err_unref;
     }

I see that drivers must implement all 3 .dumb_* callbacks:

  * To support dumb objects drivers must implement the 
&drm_driver.dumb_create,
  * &drm_driver.dumb_destroy and &drm_driver.dumb_map_offset operations. See
  * there for further details.

I'm a fan of defaults, is there any reason we can't do this:

  int drm_mode_mmap_dumb_ioctl(struct drm_device *dev,
                   void *data, struct drm_file *file_priv)
  {
      struct drm_mode_map_dumb *args = data;

      /* call driver ioctl to get mmap offset */
-     if (!dev->driver->dumb_map_offset)
+    if (!dev->driver->dumb_create)
         return -ENOSYS;

-     return dev->driver->dumb_map_offset(file_priv, dev, args->handle, 
&args->offset);
+     if (dev->driver->dumb_map_offset)
+        return dev->driver->dumb_map_offset(file_priv, dev, 
args->handle, &args->offset);
+    else
+        return drm_gem_dumb_map_offset(file_priv, dev, args->handle, 
&args->offset);
  }

  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_destroy)
+    if (!dev->driver->dumb_create)
          return -ENOSYS;

-    return dev->driver->dumb_destroy(file_priv, dev, args->handle);
+     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);
  }

There are 36 drivers that use drm_gem_dumb_destroy() directly.
vgem violates the docs and doesn't set .dumb_destroy.

> I suspect that if we had CMA subclass from drm_fb_gem (or we move the
> gem objects to the base class), we could remove a lot of its code that
> you're copying in patch 2, as well.

Yes, that was the intention.

Noralf.

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

Re: [RFC 1/7] drm/gem: Add drm_gem_dumb_map_offset()

[Daniel Vetter]

On Thu, Jul 20, 2017 at 12:13:07AM +0200, Noralf Trønnes wrote:
> 
> Den 19.07.2017 23.01, skrev Eric Anholt:
> > Noralf Trønnes <noralf@tronnes.org> writes:
> > 
> > > Add a common drm_driver.dumb_map_offset function for GEM backed drivers.
> > > 
> > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > Instead of just introducing the new code, could we replace CMA's code
> > with calls to this at the same time?
> 
> I have gone through all the drm_driver->dumb_map_offset
> implementations and found 23 drivers that could use it:
> - 18 drivers use drm_gem_cma_dumb_map_offset()
> - exynos_drm_gem_dumb_map_offset()
> - mtk_drm_gem_dumb_map_offset()
> - psb_gem_dumb_map_gtt()
> - rockchip_gem_dumb_map_offset()
> - tegra_bo_dumb_map_offset()
> 
> vgem_gem_dumb_map() can't use it because of this check:
>     if (!obj->filp) {
>         ret = -EINVAL;
>         goto unref;
>     }
> 
> armada_gem_dumb_map_offset() can't use it because of this check:
>     /* Don't allow imported objects to be mapped */
>     if (obj->obj.import_attach) {
>         ret = -EINVAL;
>         goto err_unref;
>     }
> 
> I see that drivers must implement all 3 .dumb_* callbacks:
> 
>  * To support dumb objects drivers must implement the
> &drm_driver.dumb_create,
>  * &drm_driver.dumb_destroy and &drm_driver.dumb_map_offset operations. See
>  * there for further details.
> 
> I'm a fan of defaults, is there any reason we can't do this:

So in general we try not to set defaults for the main driver entry hooks,
to avoid the helper functions leaking into core and becoming mandatory.
So e.g. ->atomic_commit should never have a default of
drm_atomic_helper_commit.

Same thought applied here for the dumb buffers - the idea is that drivers
using any kind of buffer manager scheme could have dumb buffers, including
maybe not having a buffer manager at all (and you get some cookie to
direct vram allocations or whatever). But everyone ended up with gem, just
with different kinds of backing storage implementations (cma, shmem or
ttm).

I think it makes sense to make these the defaults and rip out the default
assignment from all drivers.
> 
>  int drm_mode_mmap_dumb_ioctl(struct drm_device *dev,
>                   void *data, struct drm_file *file_priv)
>  {
>      struct drm_mode_map_dumb *args = data;
> 
>      /* call driver ioctl to get mmap offset */
> -     if (!dev->driver->dumb_map_offset)
> +    if (!dev->driver->dumb_create)
>         return -ENOSYS;
> 
> -     return dev->driver->dumb_map_offset(file_priv, dev, args->handle,
> &args->offset);
> +     if (dev->driver->dumb_map_offset)
> +        return dev->driver->dumb_map_offset(file_priv, dev, args->handle,
> &args->offset);
> +    else
> +        return drm_gem_dumb_map_offset(file_priv, dev, args->handle,
> &args->offset);
>  }
> 
>  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_destroy)
> +    if (!dev->driver->dumb_create)
>          return -ENOSYS;
> 
> -    return dev->driver->dumb_destroy(file_priv, dev, args->handle);
> +     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);
>  }
> 
> There are 36 drivers that use drm_gem_dumb_destroy() directly.
> vgem violates the docs and doesn't set .dumb_destroy.

Interesting, suprising it doesn't leak like mad.
 
> > I suspect that if we had CMA subclass from drm_fb_gem (or we move the
> > gem objects to the base class), we could remove a lot of its code that
> > you're copying in patch 2, as well.
> 
> Yes, that was the intention.

I guess we'd need to see more of the grand plan ...
-Daniel
-- 
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: [RFC 1/7] drm/gem: Add drm_gem_dumb_map_offset()

[Daniel Vetter]

On Tue, Jul 18, 2017 at 11:06:56PM +0200, Noralf Trønnes wrote:
> 
> Den 12.07.2017 15.45, skrev Noralf Trønnes:
> > Add a common drm_driver.dumb_map_offset function for GEM backed drivers.
> > 
> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > ---
> >   drivers/gpu/drm/drm_gem.c | 35 +++++++++++++++++++++++++++++++++++
> >   include/drm/drm_gem.h     |  2 ++
> >   2 files changed, 37 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index 8dc1106..44ecbaa 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -311,6 +311,41 @@ drm_gem_handle_delete(struct drm_file *filp, u32 handle)
> >   EXPORT_SYMBOL(drm_gem_handle_delete);
> >   /**
> > + * drm_gem_dumb_map_offset - return the fake mmap offset for a gem object
> > + * @file: drm file-private structure containing the gem object
> > + * @dev: corresponding drm_device
> > + * @handle: gem object handle
> > + * @offset: return location for the fake mmap offset
> > + *
> > + * This implements the &drm_driver.dumb_map_offset kms driver callback for
> > + * drivers which use gem to manage their backing storage.
> > + *
> > + * Returns:
> > + * 0 on success or a negative error code on failure.
> > + */
> > +int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
> > +			    u32 handle, u64 *offset)
> > +{
> > +	struct drm_gem_object *obj;
> > +	int ret;
> > +
> > +	obj = drm_gem_object_lookup(file, handle);
> > +	if (!obj)
> > +		return -ENOENT;
> > +
> > +	ret = drm_gem_create_mmap_offset(obj);
> 
> There are 3 drm_driver->dumb_map_offset implementations that
> don't call drm_gem_create_mmap_offset():
> - drm_gem_cma_dumb_map_offset()
> - exynos_drm_gem_dumb_map_offset()
> - tegra_bo_dumb_map_offset()
> 
> They do it during object creation.
> 
> exynos have this commit:
> drm/exynos: create a fake mmap offset with gem creation
> 48cf53f4343ae12ddc1c60dbe116161ecf7a2885
> 
> I looked at the discussion but didn't understand the rationale:
> https://lists.freedesktop.org/archives/dri-devel/2015-August/088541.html

This discussion indeed doesn't make sense - Inki says the patch doesn't
make sense and then still goes ahead and merges it.

> I see that it's ok to call drm_gem_create_mmap_offset() multiple times,
> so it's not really a problem for this function, but it would be nice to
> know why for my shmem gem library which is based on the cma library.

I don't think there's any reasonable reason for this. Lazy creating of
offsets is perfectly fine imo, we might as well bake that in as the
standard. And for some drivers it's required (you'll run out of mmap
space), but modern userspace should be all using mmap64.

Only thing to make sure is that the driver-specific mmap also creates the
offset when needed, when we remove it from gem_init functions.
-Daniel

> 
> Noralf.
> 
> 
> > +	if (ret)
> > +		goto out;
> > +
> > +	*offset = drm_vma_node_offset_addr(&obj->vma_node);
> > +out:
> > +	drm_gem_object_put_unlocked(obj);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(drm_gem_dumb_map_offset);
> > +
> > +/**
> >    * drm_gem_dumb_destroy - dumb fb callback helper for gem based drivers
> >    * @file: drm file-private structure to remove the dumb handle from
> >    * @dev: corresponding drm_device
> > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> > index 4a9d231..9c55c2a 100644
> > --- a/include/drm/drm_gem.h
> > +++ b/include/drm/drm_gem.h
> > @@ -302,6 +302,8 @@ void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages,
> >   		bool dirty, bool accessed);
> >   struct drm_gem_object *drm_gem_object_lookup(struct drm_file *filp, u32 handle);
> > +int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
> > +			    u32 handle, u64 *offset);
> >   int drm_gem_dumb_destroy(struct drm_file *file,
> >   			 struct drm_device *dev,
> >   			 uint32_t handle);
> 
> _______________________________________________
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 2/7] drm: Add GEM backed framebuffer library

[Daniel Vetter]

On Tue, Jul 18, 2017 at 05:42:28PM +0200, Noralf Trønnes wrote:
> 
> Den 12.07.2017 15.46, skrev Noralf Trønnes:
> > Add a library for drivers that can use a simple representation
> > of a GEM backed framebuffer.
> > 
> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > ---
> 
> This patch adds a gem backed drm_framebuffer like this:
> 
> struct drm_fb_gem {
>     /**
>      * @base: Base DRM framebuffer
>      */
>     struct drm_framebuffer base;
>     /**
>      * @obj: GEM object array backing the framebuffer. One object per
>      * plane.
>      */
>     struct drm_gem_object *obj[4];
> };
> 
> Now I wonder if it would be better to extend drm_framebuffer instead:
> 
>  struct drm_framebuffer {
> +    /**
> +     * @obj: GEM objects backing the framebuffer, one per plane (optional).
> +     */
> +    struct drm_gem_object *obj[4];
>  };

I think we can directly embedd the gem obj pointers into the
drm_framebuffer. Again the idea that there would be anything else than gem
kinda didn't pan out, except for vmwgfx.

We could then have a gem version of drm_helper_mode_fill_fb_struct(),
which also does the gem bo lookups.

> The reason for this, is that I have looked through all drivers that
> subclass drm_framebuffer and found this:
> - 11 drivers just adds drm_gem_object

Usually that's because they don't support yuv, so only need one plane.

> - 2 drivers adds drm_gem_object and some more
> - 6 drivers adds drm_gem_object indirectly through their subclassed
>   drm_gem_object
> - 3 drivers doesn't add drm_gem_object

I think for easier conversion we can leave all the driver-specific stuff
intact, and just provide helpers that work on the core bits.

I guess this would also help a lot with unifying the cma fb helpers by
essentially making them fully generic gem fb helpers?
-Daniel

> 
> 
> Full details below:
> 
> Adds only drm_gem_object
> ------------------------
> 
> struct amdgpu_framebuffer {
>     struct drm_framebuffer base;
>     struct drm_gem_object *obj;
> };
> 
> struct ast_framebuffer {
>     struct drm_framebuffer base;
>     struct drm_gem_object *obj;
> };
> 
> struct bochs_framebuffer {
>     struct drm_framebuffer base;
>     struct drm_gem_object *obj;
> };
> 
> struct cirrus_framebuffer {
>     struct drm_framebuffer        base;
>     struct drm_gem_object *obj;
> };
> 
> struct drm_fb_cma {
>     struct drm_framebuffer        fb;
>     struct drm_gem_cma_object    *obj[4];
> };
> 
> struct hibmc_framebuffer {
>     struct drm_framebuffer fb;
>     struct drm_gem_object *obj;
> };
> 
> struct mga_framebuffer {
>     struct drm_framebuffer base;
>     struct drm_gem_object *obj;
> };
> 
> struct mtk_drm_fb {
>     struct drm_framebuffer    base;
>     /* For now we only support a single plane */
>     struct drm_gem_object    *gem_obj;
> };
> 
> struct qxl_framebuffer {
>     struct drm_framebuffer base;
>     struct drm_gem_object *obj;
> };
> 
> struct radeon_framebuffer {
>     struct drm_framebuffer base;
>     struct drm_gem_object *obj;
> };
> 
> struct rockchip_drm_fb {
>     struct drm_framebuffer fb;
>     struct drm_gem_object *obj[ROCKCHIP_MAX_FB_BUFFER];
> };
> #define ROCKCHIP_MAX_FB_BUFFER    3
> 
> 
> Adds drm_gem_object and some more
> ---------------------------------
> 
> struct msm_framebuffer {
>     struct drm_framebuffer base;
>     const struct msm_format *format;
>     struct drm_gem_object *planes[MAX_PLANE];
> };
> #define MAX_PLANE    4
> 
> struct virtio_gpu_framebuffer {
>     struct drm_framebuffer base;
>     struct drm_gem_object *obj;
>     int x1, y1, x2, y2; /* dirty rect */
>     spinlock_t dirty_lock;
>     uint32_t hw_res_handle;
> };
> 
> 
> Indirectly adds a drm_gem_object
> --------------------------------
> 
> struct armada_framebuffer {
>     struct drm_framebuffer    fb;
>     struct armada_gem_object *obj;
>     uint8_t            fmt;
>     uint8_t            mod;
> };
> 
> struct exynos_drm_fb {
>     struct drm_framebuffer    fb;
>     struct exynos_drm_gem    *exynos_gem[MAX_FB_BUFFER];
>     dma_addr_t            dma_addr[MAX_FB_BUFFER];
> };
> #define MAX_FB_BUFFER    4
> 
> struct intel_framebuffer {
>     struct drm_framebuffer base;
>     struct drm_i915_gem_object *obj;
>     struct intel_rotation_info rot_info;
> 
>     /* for each plane in the normal GTT view */
>     struct {
>         unsigned int x, y;
>     } normal[2];
>     /* for each plane in the rotated GTT view */
>     struct {
>         unsigned int x, y;
>         unsigned int pitch; /* pixels */
>     } rotated[2];
> };
> 
> struct nouveau_framebuffer {
>     struct drm_framebuffer base;
>     struct nouveau_bo *nvbo;
>     struct nvkm_vma vma;
>     u32 r_handle;
>     u32 r_format;
>     u32 r_pitch;
>     struct nvif_object h_base[4];
>     struct nvif_object h_core;
> };
> 
> struct tegra_fb {
>     struct drm_framebuffer base;
>     struct tegra_bo **planes;
>     unsigned int num_planes;
> };
> 
> struct udl_framebuffer {
>     struct drm_framebuffer base;
>     struct udl_gem_object *obj;
>     bool active_16; /* active on the 16-bit channel */
> };
> 
> 
> Does not add drm_gem_object
> ---------------------------
> 
> struct omap_framebuffer {
>     struct drm_framebuffer base;
>     int pin_count;
>     const struct drm_format_info *format;
>     enum omap_color_mode dss_format;
>     struct plane planes[2];
>     /* lock for pinning (pin_count and planes.paddr) */
>     struct mutex lock;
> };
> 
> struct psb_framebuffer {
>     struct drm_framebuffer base;
>     struct address_space *addr_space;
>     struct fb_info *fbdev;
>     struct gtt_range *gtt;
> };
> 
> struct vmw_framebuffer {
>     struct drm_framebuffer base;
>     int (*pin)(struct vmw_framebuffer *fb);
>     int (*unpin)(struct vmw_framebuffer *fb);
>     bool dmabuf;
>     struct ttm_base_object *user_obj;
>     uint32_t user_handle;
> };
> 
> 
> Noralf.
> 
> 
> >   drivers/gpu/drm/Makefile            |   2 +-
> >   drivers/gpu/drm/drm_fb_gem_helper.c | 248 ++++++++++++++++++++++++++++++++++++
> >   include/drm/drm_fb_gem_helper.h     |  64 ++++++++++
> >   3 files changed, 313 insertions(+), 1 deletion(-)
> >   create mode 100644 drivers/gpu/drm/drm_fb_gem_helper.c
> >   create mode 100644 include/drm/drm_fb_gem_helper.h
> > 
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index 24a066e..83d8b09 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -33,7 +33,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
> >   		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
> >   		drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
> >   		drm_simple_kms_helper.o drm_modeset_helper.o \
> > -		drm_scdc_helper.o
> > +		drm_scdc_helper.o drm_fb_gem_helper.o
> >   drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o
> >   drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> > diff --git a/drivers/gpu/drm/drm_fb_gem_helper.c b/drivers/gpu/drm/drm_fb_gem_helper.c
> > new file mode 100644
> > index 0000000..9a0da09
> > --- /dev/null
> > +++ b/drivers/gpu/drm/drm_fb_gem_helper.c
> > @@ -0,0 +1,248 @@
> > +/*
> > + * drm fb gem helper functions
> > + *
> > + * Copyright (C) 2017 Noralf Trønnes
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + */
> > +
> > +#include <linux/dma-buf.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/module.h>
> > +#include <linux/reservation.h>
> > +
> > +#include <drm/drmP.h>
> > +#include <drm/drm_atomic.h>
> > +#include <drm/drm_crtc.h>
> > +#include <drm/drm_crtc_helper.h>
> > +#include <drm/drm_fb_helper.h>
> > +#include <drm/drm_fb_gem_helper.h>
> > +#include <drm/drm_gem.h>
> > +
> > +/**
> > + * drm_fb_gem_get_obj() - Get GEM object for framebuffer
> > + * @fb: The framebuffer
> > + * @plane: Which plane
> > + *
> > + * Returns the GEM object for given framebuffer.
> > + */
> > +struct drm_gem_object *drm_fb_gem_get_obj(struct drm_framebuffer *fb,
> > +					  unsigned int plane)
> > +{
> > +	struct drm_fb_gem *fb_gem = to_fb_gem(fb);
> > +
> > +	if (plane >= 4)
> > +		return NULL;
> > +
> > +	return fb_gem->obj[plane];
> > +}
> > +EXPORT_SYMBOL_GPL(drm_fb_gem_get_obj);
> > +
> > +/**
> > + * drm_fb_gem_alloc - Allocate GEM backed framebuffer
> > + * @dev: DRM device
> > + * @mode_cmd: metadata from the userspace fb creation request
> > + * @obj: GEM object nacking the framebuffer
> > + * @num_planes: Number of planes
> > + * @funcs: vtable to be used for the new framebuffer object
> > + *
> > + * Returns:
> > + * Allocated struct drm_fb_gem * or error encoded pointer.
> > + */
> > +struct drm_fb_gem *
> > +drm_fb_gem_alloc(struct drm_device *dev,
> > +		 const struct drm_mode_fb_cmd2 *mode_cmd,
> > +		 struct drm_gem_object **obj, unsigned int num_planes,
> > +		 const struct drm_framebuffer_funcs *funcs)
> > +{
> > +	struct drm_fb_gem *fb_gem;
> > +	int ret, i;
> > +
> > +	fb_gem = kzalloc(sizeof(*fb_gem), GFP_KERNEL);
> > +	if (!fb_gem)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	drm_helper_mode_fill_fb_struct(dev, &fb_gem->base, mode_cmd);
> > +
> > +	for (i = 0; i < num_planes; i++)
> > +		fb_gem->obj[i] = obj[i];
> > +
> > +	ret = drm_framebuffer_init(dev, &fb_gem->base, funcs);
> > +	if (ret) {
> > +		dev_err(dev->dev, "Failed to initialize framebuffer: %d\n", ret);
> > +		kfree(fb_gem);
> > +		return ERR_PTR(ret);
> > +	}
> > +
> > +	return fb_gem;
> > +}
> > +EXPORT_SYMBOL(drm_fb_gem_alloc);
> > +
> > +/**
> > + * drm_fb_gem_destroy - Free GEM backed framebuffer
> > + * @fb: DRM framebuffer
> > + *
> > + * Frees a GEM backed framebuffer with it's backing buffer(s) and the structure
> > + * itself. Drivers can use this as their &drm_framebuffer_funcs->destroy
> > + * callback.
> > + */
> > +void drm_fb_gem_destroy(struct drm_framebuffer *fb)
> > +{
> > +	struct drm_fb_gem *fb_gem = to_fb_gem(fb);
> > +	int i;
> > +
> > +	for (i = 0; i < 4; i++) {
> > +		if (fb_gem->obj[i])
> > +			drm_gem_object_put_unlocked(fb_gem->obj[i]);
> > +	}
> > +
> > +	drm_framebuffer_cleanup(fb);
> > +	kfree(fb_gem);
> > +}
> > +EXPORT_SYMBOL(drm_fb_gem_destroy);
> > +
> > +/**
> > + * drm_fb_gem_create_handle - Create handle for GEM backed framebuffer
> > + * @fb: DRM framebuffer
> > + * @file: drm file
> > + * @handle: handle created
> > + *
> > + * Drivers can use this as their &drm_framebuffer_funcs->create_handle
> > + * callback.
> > + *
> > + * Returns:
> > + * 0 on success or a negative error code on failure.
> > + */
> > +int drm_fb_gem_create_handle(struct drm_framebuffer *fb, struct drm_file *file,
> > +			     unsigned int *handle)
> > +{
> > +	struct drm_fb_gem *fb_gem = to_fb_gem(fb);
> > +
> > +	return drm_gem_handle_create(file, fb_gem->obj[0], handle);
> > +}
> > +EXPORT_SYMBOL(drm_fb_gem_create_handle);
> > +
> > +/**
> > + * drm_fb_gem_create_with_funcs() - helper function for the
> > + *                                  &drm_mode_config_funcs.fb_create
> > + *                                  callback
> > + * @dev: DRM device
> > + * @file: drm file for the ioctl call
> > + * @mode_cmd: metadata from the userspace fb creation request
> > + * @funcs: vtable to be used for the new framebuffer object
> > + *
> > + * This can be used to set &drm_framebuffer_funcs for drivers that need the
> > + * &drm_framebuffer_funcs.dirty callback. Use drm_fb_gem_create() if you don't
> > + * need to change &drm_framebuffer_funcs.
> > + */
> > +struct drm_framebuffer *
> > +drm_fb_gem_create_with_funcs(struct drm_device *dev, struct drm_file *file,
> > +			     const struct drm_mode_fb_cmd2 *mode_cmd,
> > +			     const struct drm_framebuffer_funcs *funcs)
> > +{
> > +	const struct drm_format_info *info;
> > +	struct drm_gem_object *objs[4];
> > +	struct drm_fb_gem *fb_gem;
> > +	int ret, i;
> > +
> > +	info = drm_get_format_info(dev, mode_cmd);
> > +	if (!info)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	for (i = 0; i < info->num_planes; i++) {
> > +		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
> > +		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
> > +		unsigned int min_size;
> > +
> > +		objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]);
> > +		if (!objs[i]) {
> > +			dev_err(dev->dev, "Failed to lookup GEM object\n");
> > +			ret = -ENOENT;
> > +			goto err_gem_object_put;
> > +		}
> > +
> > +		min_size = (height - 1) * mode_cmd->pitches[i]
> > +			 + width * info->cpp[i]
> > +			 + mode_cmd->offsets[i];
> > +
> > +		if (objs[i]->size < min_size) {
> > +			drm_gem_object_put_unlocked(objs[i]);
> > +			ret = -EINVAL;
> > +			goto err_gem_object_put;
> > +		}
> > +	}
> > +
> > +	fb_gem = drm_fb_gem_alloc(dev, mode_cmd, objs, i, funcs);
> > +	if (IS_ERR(fb_gem)) {
> > +		ret = PTR_ERR(fb_gem);
> > +		goto err_gem_object_put;
> > +	}
> > +
> > +	return &fb_gem->base;
> > +
> > +err_gem_object_put:
> > +	for (i--; i >= 0; i--)
> > +		drm_gem_object_put_unlocked(objs[i]);
> > +
> > +	return ERR_PTR(ret);
> > +}
> > +EXPORT_SYMBOL_GPL(drm_fb_gem_create_with_funcs);
> > +
> > +static struct drm_framebuffer_funcs drm_fb_gem_fb_funcs = {
> > +	.destroy	= drm_fb_gem_destroy,
> > +	.create_handle	= drm_fb_gem_create_handle,
> > +};
> > +
> > +/**
> > + * drm_fb_gem_create() - &drm_mode_config_funcs.fb_create callback function
> > + * @dev: DRM device
> > + * @file: drm file for the ioctl call
> > + * @mode_cmd: metadata from the userspace fb creation request
> > + *
> > + * If your hardware has special alignment or pitch requirements these should be
> > + * checked before calling this function. Use drm_fb_gem_create_with_funcs() if
> > + * you need to set &drm_framebuffer_funcs.dirty.
> > + */
> > +struct drm_framebuffer *
> > +drm_fb_gem_create(struct drm_device *dev, struct drm_file *file,
> > +		  const struct drm_mode_fb_cmd2 *mode_cmd)
> > +{
> > +	return drm_fb_gem_create_with_funcs(dev, file, mode_cmd,
> > +					    &drm_fb_gem_fb_funcs);
> > +}
> > +EXPORT_SYMBOL_GPL(drm_fb_gem_create);
> > +
> > +/**
> > + * drm_fb_gem_prepare_fb() - Prepare gem framebuffer
> > + * @plane: Which plane
> > + * @state: Plane state attach fence to
> > + *
> > + * This should be set as the &struct drm_plane_helper_funcs.prepare_fb hook.
> > + *
> > + * This function checks if the plane FB has an dma-buf attached, extracts
> > + * the exclusive fence and attaches it to plane state for the atomic helper
> > + * to wait on.
> > + *
> > + * There is no need for cleanup_fb for gem based framebuffer drivers.
> > + */
> > +int drm_fb_gem_prepare_fb(struct drm_plane *plane,
> > +			  struct drm_plane_state *state)
> > +{
> > +	struct dma_buf *dma_buf;
> > +	struct dma_fence *fence;
> > +
> > +	if ((plane->state->fb == state->fb) || !state->fb)
> > +		return 0;
> > +
> > +	dma_buf = drm_fb_gem_get_obj(state->fb, 0)->dma_buf;
> > +	if (dma_buf) {
> > +		fence = reservation_object_get_excl_rcu(dma_buf->resv);
> > +		drm_atomic_set_fence_for_plane(state, fence);
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(drm_fb_gem_prepare_fb);
> > diff --git a/include/drm/drm_fb_gem_helper.h b/include/drm/drm_fb_gem_helper.h
> > new file mode 100644
> > index 0000000..405a1e1
> > --- /dev/null
> > +++ b/include/drm/drm_fb_gem_helper.h
> > @@ -0,0 +1,64 @@
> > +#ifndef __DRM_FB_GEM_HELPER_H__
> > +#define __DRM_FB_GEM_HELPER_H__
> > +
> > +#include <drm/drm_framebuffer.h>
> > +
> > +struct drm_gem_shmem_object;
> > +struct drm_mode_fb_cmd2;
> > +struct drm_plane;
> > +struct drm_plane_state;
> > +
> > +/**
> > + * struct drm_fb_gem - GEM backed framebuffer
> > + */
> > +struct drm_fb_gem {
> > +	/**
> > +	 * @base: Base DRM framebuffer
> > +	 */
> > +	struct drm_framebuffer base;
> > +	/**
> > +	 * @obj: GEM object array backing the framebuffer. One object per
> > +	 * plane.
> > +	 */
> > +	struct drm_gem_object *obj[4];
> > +};
> > +
> > +static inline struct drm_fb_gem *to_fb_gem(struct drm_framebuffer *fb)
> > +{
> > +	return container_of(fb, struct drm_fb_gem, base);
> > +}
> > +
> > +struct drm_gem_object *drm_fb_gem_get_obj(struct drm_framebuffer *fb,
> > +					  unsigned int plane);
> > +struct drm_fb_gem *
> > +drm_fb_gem_alloc(struct drm_device *dev,
> > +		 const struct drm_mode_fb_cmd2 *mode_cmd,
> > +		 struct drm_gem_object **obj, unsigned int num_planes,
> > +		 const struct drm_framebuffer_funcs *funcs);
> > +void drm_fb_gem_destroy(struct drm_framebuffer *fb);
> > +int drm_fb_gem_create_handle(struct drm_framebuffer *fb, struct drm_file *file,
> > +			     unsigned int *handle);
> > +
> > +struct drm_framebuffer *
> > +drm_fb_gem_create_with_funcs(struct drm_device *dev, struct drm_file *file,
> > +			     const struct drm_mode_fb_cmd2 *mode_cmd,
> > +			     const struct drm_framebuffer_funcs *funcs);
> > +struct drm_framebuffer *
> > +drm_fb_gem_create(struct drm_device *dev, struct drm_file *file,
> > +		  const struct drm_mode_fb_cmd2 *mode_cmd);
> > +
> > +
> > +int drm_fb_gem_prepare_fb(struct drm_plane *plane,
> > +			  struct drm_plane_state *state);
> > +
> > +
> > +
> > +
> > +#ifdef CONFIG_DEBUG_FS
> > +struct seq_file;
> > +
> > +int drm_fb_gem_debugfs_show(struct seq_file *m, void *arg);
> > +#endif
> > +
> > +#endif
> > +
> 
> _______________________________________________
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 2/7] drm: Add GEM backed framebuffer library

[Noralf Trønnes]

Den 20.07.2017 10.10, skrev Daniel Vetter:
> On Tue, Jul 18, 2017 at 05:42:28PM +0200, Noralf Trønnes wrote:
>> Den 12.07.2017 15.46, skrev Noralf Trønnes:
>>> Add a library for drivers that can use a simple representation
>>> of a GEM backed framebuffer.
>>>
>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>> ---
>> This patch adds a gem backed drm_framebuffer like this:
>>
>> struct drm_fb_gem {
>>      /**
>>       * @base: Base DRM framebuffer
>>       */
>>      struct drm_framebuffer base;
>>      /**
>>       * @obj: GEM object array backing the framebuffer. One object per
>>       * plane.
>>       */
>>      struct drm_gem_object *obj[4];
>> };
>>
>> Now I wonder if it would be better to extend drm_framebuffer instead:
>>
>>   struct drm_framebuffer {
>> +    /**
>> +     * @obj: GEM objects backing the framebuffer, one per plane (optional).
>> +     */
>> +    struct drm_gem_object *obj[4];
>>   };
> I think we can directly embedd the gem obj pointers into the
> drm_framebuffer. Again the idea that there would be anything else than gem
> kinda didn't pan out, except for vmwgfx.
>
> We could then have a gem version of drm_helper_mode_fill_fb_struct(),
> which also does the gem bo lookups.

How about this, I've copied from drm_fb_cma_create_with_funcs():

/**
  * drm_helper_mode_fill_fb_gem - fill out framebuffer metadata and 
lookup GEM
  * @dev: DRM device
  * @file_priv: DRM file to lookup buffers from
  * @fb: drm_framebuffer object to fill out
  * @mode_cmd: metadata from the userspace fb creation request
  *
  * This helper can be used in a drivers fb_create callback to pre-fill 
the fb's
  * metadata fields and lookup the GEM buffer object(s).
  *
  * Returns:
  * 0 on success or a negative error code on failure.
  */
int drm_helper_mode_fill_fb_gem(struct drm_device *dev,
                 struct drm_file *file_priv,
                 struct drm_framebuffer *fb,
                 const struct drm_mode_fb_cmd2 *mode_cmd)
{
     int i, ret;

     drm_helper_mode_fill_fb_struct(dev, fb, mode_cmd);
     if (!fb->format)
         return -EINVAL;

     for (i = 0; i < fb->format->num_planes; i++) {
         unsigned int width = mode_cmd->width /
                      (i ? fb->format->hsub : 1);
         unsigned int height = mode_cmd->height /
                       (i ? fb->format->vsub : 1);
         unsigned int min_size;

         fb->objs[i] = drm_gem_object_lookup(file_priv,
                             mode_cmd->handles[i]);
         if (!fb->objs[i]) {
             ret = -ENOENT;
             goto err_gem_object_put;
         }

         min_size = (height - 1) * mode_cmd->pitches[i] +
                width * fb->format->cpp[i] + mode_cmd->offsets[i];

         if (fb->objs[i]->size < min_size) {
             drm_gem_object_put_unlocked(fb->objs[i]);
             fb->objs[i] = NULL;
             ret = -EINVAL;
             goto err_gem_object_put;
         }
     }

err_gem_object_put:
     for (i--; i >= 0; i--) {
         drm_gem_object_put_unlocked(fb->objs[i]);
         fb->objs[i] = NULL;
     }

     return ret;
}
EXPORT_SYMBOL(drm_helper_mode_fill_fb_gem);

>> The reason for this, is that I have looked through all drivers that
>> subclass drm_framebuffer and found this:
>> - 11 drivers just adds drm_gem_object
> Usually that's because they don't support yuv, so only need one plane.
>
>> - 2 drivers adds drm_gem_object and some more
>> - 6 drivers adds drm_gem_object indirectly through their subclassed
>>    drm_gem_object
>> - 3 drivers doesn't add drm_gem_object
> I think for easier conversion we can leave all the driver-specific stuff
> intact, and just provide helpers that work on the core bits.
>
> I guess this would also help a lot with unifying the cma fb helpers by
> essentially making them fully generic gem fb helpers?

Yes both cma and shmem library can now use these helpers. But pushing
the helpers all the way out to the cma drivers will be too much for me.

Any chance the fbdev helper could get a struct drm_file?
It would be nice to use drm_driver.dumb_create and
drm_mode_config_funcs.fb_create to get a
framebuffer for fbdev.

Noralf.

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

Re: [RFC 1/7] drm/gem: Add drm_gem_dumb_map_offset()

[Noralf Trønnes]

Den 20.07.2017 10.00, skrev Daniel Vetter:
> On Thu, Jul 20, 2017 at 12:13:07AM +0200, Noralf Trønnes wrote:
>> Den 19.07.2017 23.01, skrev Eric Anholt:
>>> Noralf Trønnes <noralf@tronnes.org> writes:
>>>
>>>> Add a common drm_driver.dumb_map_offset function for GEM backed drivers.
>>>>
>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>> Instead of just introducing the new code, could we replace CMA's code
>>> with calls to this at the same time?
>> I have gone through all the drm_driver->dumb_map_offset
>> implementations and found 23 drivers that could use it:
>> - 18 drivers use drm_gem_cma_dumb_map_offset()
>> - exynos_drm_gem_dumb_map_offset()
>> - mtk_drm_gem_dumb_map_offset()
>> - psb_gem_dumb_map_gtt()
>> - rockchip_gem_dumb_map_offset()
>> - tegra_bo_dumb_map_offset()
>>
>> vgem_gem_dumb_map() can't use it because of this check:
>>      if (!obj->filp) {
>>          ret = -EINVAL;
>>          goto unref;
>>      }
>>
>> armada_gem_dumb_map_offset() can't use it because of this check:
>>      /* Don't allow imported objects to be mapped */
>>      if (obj->obj.import_attach) {
>>          ret = -EINVAL;
>>          goto err_unref;
>>      }
>>
>> I see that drivers must implement all 3 .dumb_* callbacks:
>>
>>   * To support dumb objects drivers must implement the
>> &drm_driver.dumb_create,
>>   * &drm_driver.dumb_destroy and &drm_driver.dumb_map_offset operations. See
>>   * there for further details.
>>
>> I'm a fan of defaults, is there any reason we can't do this:
> So in general we try not to set defaults for the main driver entry hooks,
> to avoid the helper functions leaking into core and becoming mandatory.
> So e.g. ->atomic_commit should never have a default of
> drm_atomic_helper_commit.
>
> Same thought applied here for the dumb buffers - the idea is that drivers
> using any kind of buffer manager scheme could have dumb buffers, including
> maybe not having a buffer manager at all (and you get some cookie to
> direct vram allocations or whatever). But everyone ended up with gem, just
> with different kinds of backing storage implementations (cma, shmem or
> ttm).
>
> I think it makes sense to make these the defaults and rip out the default
> assignment from all drivers.
>>   int drm_mode_mmap_dumb_ioctl(struct drm_device *dev,
>>                    void *data, struct drm_file *file_priv)
>>   {
>>       struct drm_mode_map_dumb *args = data;
>>
>>       /* call driver ioctl to get mmap offset */
>> -     if (!dev->driver->dumb_map_offset)
>> +    if (!dev->driver->dumb_create)
>>          return -ENOSYS;
>>
>> -     return dev->driver->dumb_map_offset(file_priv, dev, args->handle,
>> &args->offset);
>> +     if (dev->driver->dumb_map_offset)
>> +        return dev->driver->dumb_map_offset(file_priv, dev, args->handle,
>> &args->offset);
>> +    else
>> +        return drm_gem_dumb_map_offset(file_priv, dev, args->handle,
>> &args->offset);
>>   }
>>
>>   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_destroy)
>> +    if (!dev->driver->dumb_create)
>>           return -ENOSYS;
>>
>> -    return dev->driver->dumb_destroy(file_priv, dev, args->handle);
>> +     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);
>>   }
>>
>> There are 36 drivers that use drm_gem_dumb_destroy() directly.
>> vgem violates the docs and doesn't set .dumb_destroy.
> Interesting, suprising it doesn't leak like mad.
>   
>>> I suspect that if we had CMA subclass from drm_fb_gem (or we move the
>>> gem objects to the base class), we could remove a lot of its code that
>>> you're copying in patch 2, as well.
>> Yes, that was the intention.
> I guess we'd need to see more of the grand plan ...

Currently it looks like 4 patchsets:

1. Add drm_gem_dumb_map_offset() and implement defaults as discussed above.

2. Add drm_gem_object pointers to drm_framebuffer and create matching
    helpers for:
      drm_framebuffer_funcs->create_handle
      drm_framebuffer_funcs->destroy
      drm_mode_config_funcs->fb_create
      Should I put the functions in drm_framebuffer_helper.[h,c] ?
    Use these helpers in the cma library

3. Add drm_fb_helper_simple_init() and drm_fb_helper_simple_fini()
    Use them in drivers where applicable.

4. Add shmem library
    Convert tinydrm to shmem.

Noralf.

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

Re: [RFC 1/7] drm/gem: Add drm_gem_dumb_map_offset()

[Daniel Vetter]

On Fri, Jul 21, 2017 at 08:41:50PM +0200, Noralf Trønnes wrote:
> 
> Den 20.07.2017 10.00, skrev Daniel Vetter:
> > On Thu, Jul 20, 2017 at 12:13:07AM +0200, Noralf Trønnes wrote:
> > > Den 19.07.2017 23.01, skrev Eric Anholt:
> > > > Noralf Trønnes <noralf@tronnes.org> writes:
> > > > 
> > > > > Add a common drm_driver.dumb_map_offset function for GEM backed drivers.
> > > > > 
> > > > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > > > Instead of just introducing the new code, could we replace CMA's code
> > > > with calls to this at the same time?
> > > I have gone through all the drm_driver->dumb_map_offset
> > > implementations and found 23 drivers that could use it:
> > > - 18 drivers use drm_gem_cma_dumb_map_offset()
> > > - exynos_drm_gem_dumb_map_offset()
> > > - mtk_drm_gem_dumb_map_offset()
> > > - psb_gem_dumb_map_gtt()
> > > - rockchip_gem_dumb_map_offset()
> > > - tegra_bo_dumb_map_offset()
> > > 
> > > vgem_gem_dumb_map() can't use it because of this check:
> > >      if (!obj->filp) {
> > >          ret = -EINVAL;
> > >          goto unref;
> > >      }
> > > 
> > > armada_gem_dumb_map_offset() can't use it because of this check:
> > >      /* Don't allow imported objects to be mapped */
> > >      if (obj->obj.import_attach) {
> > >          ret = -EINVAL;
> > >          goto err_unref;
> > >      }
> > > 
> > > I see that drivers must implement all 3 .dumb_* callbacks:
> > > 
> > >   * To support dumb objects drivers must implement the
> > > &drm_driver.dumb_create,
> > >   * &drm_driver.dumb_destroy and &drm_driver.dumb_map_offset operations. See
> > >   * there for further details.
> > > 
> > > I'm a fan of defaults, is there any reason we can't do this:
> > So in general we try not to set defaults for the main driver entry hooks,
> > to avoid the helper functions leaking into core and becoming mandatory.
> > So e.g. ->atomic_commit should never have a default of
> > drm_atomic_helper_commit.
> > 
> > Same thought applied here for the dumb buffers - the idea is that drivers
> > using any kind of buffer manager scheme could have dumb buffers, including
> > maybe not having a buffer manager at all (and you get some cookie to
> > direct vram allocations or whatever). But everyone ended up with gem, just
> > with different kinds of backing storage implementations (cma, shmem or
> > ttm).
> > 
> > I think it makes sense to make these the defaults and rip out the default
> > assignment from all drivers.
> > >   int drm_mode_mmap_dumb_ioctl(struct drm_device *dev,
> > >                    void *data, struct drm_file *file_priv)
> > >   {
> > >       struct drm_mode_map_dumb *args = data;
> > > 
> > >       /* call driver ioctl to get mmap offset */
> > > -     if (!dev->driver->dumb_map_offset)
> > > +    if (!dev->driver->dumb_create)
> > >          return -ENOSYS;
> > > 
> > > -     return dev->driver->dumb_map_offset(file_priv, dev, args->handle,
> > > &args->offset);
> > > +     if (dev->driver->dumb_map_offset)
> > > +        return dev->driver->dumb_map_offset(file_priv, dev, args->handle,
> > > &args->offset);
> > > +    else
> > > +        return drm_gem_dumb_map_offset(file_priv, dev, args->handle,
> > > &args->offset);
> > >   }
> > > 
> > >   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_destroy)
> > > +    if (!dev->driver->dumb_create)
> > >           return -ENOSYS;
> > > 
> > > -    return dev->driver->dumb_destroy(file_priv, dev, args->handle);
> > > +     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);
> > >   }
> > > 
> > > There are 36 drivers that use drm_gem_dumb_destroy() directly.
> > > vgem violates the docs and doesn't set .dumb_destroy.
> > Interesting, suprising it doesn't leak like mad.
> > > > I suspect that if we had CMA subclass from drm_fb_gem (or we move the
> > > > gem objects to the base class), we could remove a lot of its code that
> > > > you're copying in patch 2, as well.
> > > Yes, that was the intention.
> > I guess we'd need to see more of the grand plan ...
> 
> Currently it looks like 4 patchsets:
> 
> 1. Add drm_gem_dumb_map_offset() and implement defaults as discussed above.
> 
> 2. Add drm_gem_object pointers to drm_framebuffer and create matching
>    helpers for:
>      drm_framebuffer_funcs->create_handle
>      drm_framebuffer_funcs->destroy
>      drm_mode_config_funcs->fb_create
>      Should I put the functions in drm_framebuffer_helper.[h,c] ?

I'd call them drm_gem_framebuffer_helper.[hc], just to highlight the
gem<->kms connection a bit more.

>    Use these helpers in the cma library
> 
> 3. Add drm_fb_helper_simple_init() and drm_fb_helper_simple_fini()
>    Use them in drivers where applicable.
> 
> 4. Add shmem library
>    Convert tinydrm to shmem.

Sounds like a good plan. I'll try to scrape away some review bandwidth to
look at your patches.
-Daniel
-- 
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: [RFC 1/7] drm/gem: Add drm_gem_dumb_map_offset()

[Noralf Trønnes]

Den 24.07.2017 10.28, skrev Daniel Vetter:
> On Fri, Jul 21, 2017 at 08:41:50PM +0200, Noralf Trønnes wrote:
>> Den 20.07.2017 10.00, skrev Daniel Vetter:
>>> On Thu, Jul 20, 2017 at 12:13:07AM +0200, Noralf Trønnes wrote:
>>>> Den 19.07.2017 23.01, skrev Eric Anholt:
>>>>> Noralf Trønnes <noralf@tronnes.org> writes:
>>>>>
>>>>>> Add a common drm_driver.dumb_map_offset function for GEM backed drivers.
>>>>>>
>>>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>>> Instead of just introducing the new code, could we replace CMA's code
>>>>> with calls to this at the same time?
>>>> I have gone through all the drm_driver->dumb_map_offset
>>>> implementations and found 23 drivers that could use it:
>>>> - 18 drivers use drm_gem_cma_dumb_map_offset()
>>>> - exynos_drm_gem_dumb_map_offset()
>>>> - mtk_drm_gem_dumb_map_offset()
>>>> - psb_gem_dumb_map_gtt()
>>>> - rockchip_gem_dumb_map_offset()
>>>> - tegra_bo_dumb_map_offset()
>>>>
>>>> vgem_gem_dumb_map() can't use it because of this check:
>>>>       if (!obj->filp) {
>>>>           ret = -EINVAL;
>>>>           goto unref;
>>>>       }
>>>>
>>>> armada_gem_dumb_map_offset() can't use it because of this check:
>>>>       /* Don't allow imported objects to be mapped */
>>>>       if (obj->obj.import_attach) {
>>>>           ret = -EINVAL;
>>>>           goto err_unref;
>>>>       }
>>>>
>>>> I see that drivers must implement all 3 .dumb_* callbacks:
>>>>
>>>>    * To support dumb objects drivers must implement the
>>>> &drm_driver.dumb_create,
>>>>    * &drm_driver.dumb_destroy and &drm_driver.dumb_map_offset operations. See
>>>>    * there for further details.
>>>>
>>>> I'm a fan of defaults, is there any reason we can't do this:
>>> So in general we try not to set defaults for the main driver entry hooks,
>>> to avoid the helper functions leaking into core and becoming mandatory.
>>> So e.g. ->atomic_commit should never have a default of
>>> drm_atomic_helper_commit.
>>>
>>> Same thought applied here for the dumb buffers - the idea is that drivers
>>> using any kind of buffer manager scheme could have dumb buffers, including
>>> maybe not having a buffer manager at all (and you get some cookie to
>>> direct vram allocations or whatever). But everyone ended up with gem, just
>>> with different kinds of backing storage implementations (cma, shmem or
>>> ttm).
>>>
>>> I think it makes sense to make these the defaults and rip out the default
>>> assignment from all drivers.
>>>>    int drm_mode_mmap_dumb_ioctl(struct drm_device *dev,
>>>>                     void *data, struct drm_file *file_priv)
>>>>    {
>>>>        struct drm_mode_map_dumb *args = data;
>>>>
>>>>        /* call driver ioctl to get mmap offset */
>>>> -     if (!dev->driver->dumb_map_offset)
>>>> +    if (!dev->driver->dumb_create)
>>>>           return -ENOSYS;
>>>>
>>>> -     return dev->driver->dumb_map_offset(file_priv, dev, args->handle,
>>>> &args->offset);
>>>> +     if (dev->driver->dumb_map_offset)
>>>> +        return dev->driver->dumb_map_offset(file_priv, dev, args->handle,
>>>> &args->offset);
>>>> +    else
>>>> +        return drm_gem_dumb_map_offset(file_priv, dev, args->handle,
>>>> &args->offset);
>>>>    }
>>>>
>>>>    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_destroy)
>>>> +    if (!dev->driver->dumb_create)
>>>>            return -ENOSYS;
>>>>
>>>> -    return dev->driver->dumb_destroy(file_priv, dev, args->handle);
>>>> +     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);
>>>>    }
>>>>
>>>> There are 36 drivers that use drm_gem_dumb_destroy() directly.
>>>> vgem violates the docs and doesn't set .dumb_destroy.
>>> Interesting, suprising it doesn't leak like mad.
>>>>> I suspect that if we had CMA subclass from drm_fb_gem (or we move the
>>>>> gem objects to the base class), we could remove a lot of its code that
>>>>> you're copying in patch 2, as well.
>>>> Yes, that was the intention.
>>> I guess we'd need to see more of the grand plan ...
>> Currently it looks like 4 patchsets:
>>
>> 1. Add drm_gem_dumb_map_offset() and implement defaults as discussed above.
>>
>> 2. Add drm_gem_object pointers to drm_framebuffer and create matching
>>     helpers for:
>>       drm_framebuffer_funcs->create_handle
>>       drm_framebuffer_funcs->destroy
>>       drm_mode_config_funcs->fb_create
>>       Should I put the functions in drm_framebuffer_helper.[h,c] ?
> I'd call them drm_gem_framebuffer_helper.[hc], just to highlight the
> gem<->kms connection a bit more.
>
>>     Use these helpers in the cma library
>>
>> 3. Add drm_fb_helper_simple_init() and drm_fb_helper_simple_fini()
>>     Use them in drivers where applicable.
>>
>> 4. Add shmem library
>>     Convert tinydrm to shmem.
> Sounds like a good plan. I'll try to scrape away some review bandwidth to
> look at your patches.

Thanks, this fanned out a bit more than first expected.

Noralf.

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

Re: [RFC 2/7] drm: Add GEM backed framebuffer library

[Daniel Vetter]

On Fri, Jul 21, 2017 at 08:39:17PM +0200, Noralf Trønnes wrote:
>
> Den 20.07.2017 10.10, skrev Daniel Vetter:
> > On Tue, Jul 18, 2017 at 05:42:28PM +0200, Noralf Trønnes wrote:
> > > Den 12.07.2017 15.46, skrev Noralf Trønnes:
> > > > Add a library for drivers that can use a simple representation
> > > > of a GEM backed framebuffer.
> > > >
> > > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > > > ---
> > > This patch adds a gem backed drm_framebuffer like this:
> > >
> > > struct drm_fb_gem {
> > >      /**
> > >       * @base: Base DRM framebuffer
> > >       */
> > >      struct drm_framebuffer base;
> > >      /**
> > >       * @obj: GEM object array backing the framebuffer. One object per
> > >       * plane.
> > >       */
> > >      struct drm_gem_object *obj[4];
> > > };
> > >
> > > Now I wonder if it would be better to extend drm_framebuffer instead:
> > >
> > >   struct drm_framebuffer {
> > > +    /**
> > > +     * @obj: GEM objects backing the framebuffer, one per plane (optional).
> > > +     */
> > > +    struct drm_gem_object *obj[4];
> > >   };
> > I think we can directly embedd the gem obj pointers into the
> > drm_framebuffer. Again the idea that there would be anything else than gem
> > kinda didn't pan out, except for vmwgfx.
> >
> > We could then have a gem version of drm_helper_mode_fill_fb_struct(),
> > which also does the gem bo lookups.
>
> How about this, I've copied from drm_fb_cma_create_with_funcs():

Hm, we might as well call it drm_fb_gem_create_with_funcs, to stay in line
with the naming scheme we have.

> /**
>  * drm_helper_mode_fill_fb_gem - fill out framebuffer metadata and lookup
> GEM
>  * @dev: DRM device
>  * @file_priv: DRM file to lookup buffers from
>  * @fb: drm_framebuffer object to fill out
>  * @mode_cmd: metadata from the userspace fb creation request
>  *
>  * This helper can be used in a drivers fb_create callback to pre-fill the
> fb's
>  * metadata fields and lookup the GEM buffer object(s).
>  *
>  * Returns:
>  * 0 on success or a negative error code on failure.
>  */
> int drm_helper_mode_fill_fb_gem(struct drm_device *dev,
>                 struct drm_file *file_priv,
>                 struct drm_framebuffer *fb,
>                 const struct drm_mode_fb_cmd2 *mode_cmd)
> {
>     int i, ret;
>
>     drm_helper_mode_fill_fb_struct(dev, fb, mode_cmd);
>     if (!fb->format)
>         return -EINVAL;
>
>     for (i = 0; i < fb->format->num_planes; i++) {
>         unsigned int width = mode_cmd->width /
>                      (i ? fb->format->hsub : 1);
>         unsigned int height = mode_cmd->height /
>                       (i ? fb->format->vsub : 1);
>         unsigned int min_size;
>
>         fb->objs[i] = drm_gem_object_lookup(file_priv,
>                             mode_cmd->handles[i]);
>         if (!fb->objs[i]) {
>             ret = -ENOENT;
>             goto err_gem_object_put;
>         }
>
>         min_size = (height - 1) * mode_cmd->pitches[i] +
>                width * fb->format->cpp[i] + mode_cmd->offsets[i];
>
>         if (fb->objs[i]->size < min_size) {
>             drm_gem_object_put_unlocked(fb->objs[i]);
>             fb->objs[i] = NULL;
>             ret = -EINVAL;
>             goto err_gem_object_put;
>         }
>     }
>
> err_gem_object_put:
>     for (i--; i >= 0; i--) {
>         drm_gem_object_put_unlocked(fb->objs[i]);
>         fb->objs[i] = NULL;
>     }
>
>     return ret;
> }
> EXPORT_SYMBOL(drm_helper_mode_fill_fb_gem);
>
> > > The reason for this, is that I have looked through all drivers that
> > > subclass drm_framebuffer and found this:
> > > - 11 drivers just adds drm_gem_object
> > Usually that's because they don't support yuv, so only need one plane.
> >
> > > - 2 drivers adds drm_gem_object and some more
> > > - 6 drivers adds drm_gem_object indirectly through their subclassed
> > >    drm_gem_object
> > > - 3 drivers doesn't add drm_gem_object
> > I think for easier conversion we can leave all the driver-specific stuff
> > intact, and just provide helpers that work on the core bits.
> >
> > I guess this would also help a lot with unifying the cma fb helpers by
> > essentially making them fully generic gem fb helpers?
>
> Yes both cma and shmem library can now use these helpers. But pushing
> the helpers all the way out to the cma drivers will be too much for me.

No worries, we can do this one by one across different people.

> Any chance the fbdev helper could get a struct drm_file?
> It would be nice to use drm_driver.dumb_create and
> drm_mode_config_funcs.fb_create to get a
> framebuffer for fbdev.

David Herrmann had a patch series to allow the kernel to create a drm_file
without a real struct file attached behind it. I think we landed the prep
work, but not any of the work to move the fbdev helper over to it. I'll
ping him whether he still has some of those patches somewhere.
-Daniel
-- 
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