[PATCH 0/8] drm/vram-helper: Lock GEM BOs while they are mapped

[PATCH 0/8] drm/vram-helper: Lock GEM BOs while they are mapped

[Thomas Zimmermann]

GEM VRAM helpers used to pin the BO in their implementation of vmap, so
that they could not be relocated. In a recent discussion, [1] it became
clear that this is incorrect and that vmap should rather repend on the
reservation lock to prevent relocation. This patchset addresses the issue.
Besides the vram helpers, this affects ast, vboxvideo and the generic
fbdev emulation.

Patch 1 adds a few more rules to vmap internfaces. With VRAM, it is
necessary to keep the BO evictable, which requires soem care when mapping
the memory. Patch 2 changes ast's cursor code accordingly.

Patch 3 adds vram helpers that acquires the reservation lock and vmap the
memory buffer. Same for vunmap in reverse. Patches 4 and 5 convert ast
and vboxvideo to the new helper.

Patch 6 removes pinning and locking from VRAM helper's vmap and vunmap.
The affected users in ast and fbdev emulation acquire the reservation
locks of the GEM objects before vmapping BOs. VRAM helpers don't support
to export the buffer, so there are no other users of these functions.

The the pinning and locking removed, vmap can be simplified as done in
patches 7 and 8.

Tested on ast with GEM VRAM and also on mgag200 to verify that the fbdev
change does not interfere with GEM SHMEM.

Thomas Zimmermann (8):
  drm/gem: Write down some rules for vmap usage
  drm/ast: Only map cursor BOs during updates
  drm/vram-helper: Provide drm_gem_vram_vmap_unlocked()
  drm/ast: Use drm_gem_vram_vmap_unlocked() in ast_cursor_show()
  drm/vboxvideo: Use drm_gem_vram_vmap_unlocked() in cursor update
  drm/vram-helper: Remove pinning and locking from drm_gem_vram_vmap()
  drm/vram-helper: Remove vmap reference counting
  drm/vram-helper: Simplify vmap implementation

 drivers/gpu/drm/ast/ast_cursor.c      |  63 +++++++++-------
 drivers/gpu/drm/ast/ast_drv.h         |   2 -
 drivers/gpu/drm/drm_client.c          |  31 ++++++++
 drivers/gpu/drm/drm_fb_helper.c       |  10 ++-
 drivers/gpu/drm/drm_gem_vram_helper.c | 101 +++++++++++++-------------
 drivers/gpu/drm/drm_prime.c           |   6 ++
 drivers/gpu/drm/vboxvideo/vbox_mode.c |   7 +-
 include/drm/drm_client.h              |   2 +
 include/drm/drm_gem.h                 |  16 ++++
 include/drm/drm_gem_vram_helper.h     |  21 ++----
 10 files changed, 159 insertions(+), 100 deletions(-)

--
2.29.2

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

[PATCH 1/8] drm/gem: Write down some rules for vmap usage

[Thomas Zimmermann]

Mapping a GEM object's buffer into kernel address space prevents the
buffer from being evicted from VRAM, which in turn may result in
out-of-memory errors. It's therefore required to only vmap GEM BOs for
short periods of time; unless the GEM implementation provides additional
guarantees.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_prime.c |  6 ++++++
 include/drm/drm_gem.h       | 16 ++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 7db55fce35d8..9c9ece9833e0 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -669,6 +669,12 @@ EXPORT_SYMBOL(drm_gem_unmap_dma_buf);
  * callback. Calls into &drm_gem_object_funcs.vmap for device specific handling.
  * The kernel virtual address is returned in map.
  *
+ * To prevent the GEM object from being relocated, callers must hold the GEM
+ * object's reservation lock from when calling this function until releasing the
+ * mapping. Holding onto a mapping and the associated reservation lock for an
+ * unbound time may result in out-of-memory errors. Calls to drm_gem_dmabuf_vmap()
+ * should therefore be accompanied by a call to drm_gem_dmabuf_vunmap().
+ *
  * Returns 0 on success or a negative errno code otherwise.
  */
 int drm_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct dma_buf_map *map)
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index 5e6daa1c982f..7c34cd5ec261 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -137,7 +137,21 @@ struct drm_gem_object_funcs {
 	 * Returns a virtual address for the buffer. Used by the
 	 * drm_gem_dmabuf_vmap() helper.
 	 *
+	 * Notes to implementors:
+	 *
+	 * - Implementations must expect pairs of @vmap and @vunmap to be
+	 *   called frequently and should optimize for this case.
+	 *
+	 * - Implemenations may expect the caller to hold the GEM object's
+	 *   reservation lock to protect against concurrent calls and relocation
+	 *   of the GEM object.
+	 *
+	 * - Implementations may provide additional guarantees (e.g., working
+	 *   without holding the reservation lock).
+	 *
 	 * This callback is optional.
+	 *
+	 * See also drm_gem_dmabuf_vmap()
 	 */
 	int (*vmap)(struct drm_gem_object *obj, struct dma_buf_map *map);
 
@@ -148,6 +162,8 @@ struct drm_gem_object_funcs {
 	 * drm_gem_dmabuf_vunmap() helper.
 	 *
 	 * This callback is optional.
+	 *
+	 * See also @vmap.
 	 */
 	void (*vunmap)(struct drm_gem_object *obj, struct dma_buf_map *map);
 
-- 
2.29.2

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

[PATCH 2/8] drm/ast: Only map cursor BOs during updates

[Thomas Zimmermann]

The HW cursor's BO used to be mapped permanently into the kernel's
address space. GEM's vmap operation will be protected by locks, and
we don't want to lock the BO's for an undefinate period of time.

Change the cursor code to map the HW BOs only during updates. The
vmap operation in VRAM helpers is cheap, as a once estabished mapping
is being reused until the BO actually moves. As the HW cursor BOs are
pinned, they never move at all.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_cursor.c | 59 +++++++++++++++++---------------
 drivers/gpu/drm/ast/ast_drv.h    |  2 --
 2 files changed, 31 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_cursor.c b/drivers/gpu/drm/ast/ast_cursor.c
index 742d43a7edf4..c5892827b220 100644
--- a/drivers/gpu/drm/ast/ast_cursor.c
+++ b/drivers/gpu/drm/ast/ast_cursor.c
@@ -39,7 +39,6 @@ static void ast_cursor_fini(struct ast_private *ast)
 
 	for (i = 0; i < ARRAY_SIZE(ast->cursor.gbo); ++i) {
 		gbo = ast->cursor.gbo[i];
-		drm_gem_vram_vunmap(gbo, &ast->cursor.map[i]);
 		drm_gem_vram_unpin(gbo);
 		drm_gem_vram_put(gbo);
 	}
@@ -53,14 +52,13 @@ static void ast_cursor_release(struct drm_device *dev, void *ptr)
 }
 
 /*
- * Allocate cursor BOs and pins them at the end of VRAM.
+ * Allocate cursor BOs and pin them at the end of VRAM.
  */
 int ast_cursor_init(struct ast_private *ast)
 {
 	struct drm_device *dev = &ast->base;
 	size_t size, i;
 	struct drm_gem_vram_object *gbo;
-	struct dma_buf_map map;
 	int ret;
 
 	size = roundup(AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE, PAGE_SIZE);
@@ -77,15 +75,7 @@ int ast_cursor_init(struct ast_private *ast)
 			drm_gem_vram_put(gbo);
 			goto err_drm_gem_vram_put;
 		}
-		ret = drm_gem_vram_vmap(gbo, &map);
-		if (ret) {
-			drm_gem_vram_unpin(gbo);
-			drm_gem_vram_put(gbo);
-			goto err_drm_gem_vram_put;
-		}
-
 		ast->cursor.gbo[i] = gbo;
-		ast->cursor.map[i] = map;
 	}
 
 	return drmm_add_action_or_reset(dev, ast_cursor_release, NULL);
@@ -94,7 +84,6 @@ int ast_cursor_init(struct ast_private *ast)
 	while (i) {
 		--i;
 		gbo = ast->cursor.gbo[i];
-		drm_gem_vram_vunmap(gbo, &ast->cursor.map[i]);
 		drm_gem_vram_unpin(gbo);
 		drm_gem_vram_put(gbo);
 	}
@@ -168,38 +157,43 @@ static void update_cursor_image(u8 __iomem *dst, const u8 *src, int width, int h
 int ast_cursor_blit(struct ast_private *ast, struct drm_framebuffer *fb)
 {
 	struct drm_device *dev = &ast->base;
-	struct drm_gem_vram_object *gbo;
-	struct dma_buf_map map;
-	int ret;
-	void *src;
+	struct drm_gem_vram_object *dst_gbo = ast->cursor.gbo[ast->cursor.next_index];
+	struct drm_gem_vram_object *src_gbo = drm_gem_vram_of_gem(fb->obj[0]);
+	struct dma_buf_map src_map, dst_map;
 	void __iomem *dst;
+	void *src;
+	int ret;
 
 	if (drm_WARN_ON_ONCE(dev, fb->width > AST_MAX_HWC_WIDTH) ||
 	    drm_WARN_ON_ONCE(dev, fb->height > AST_MAX_HWC_HEIGHT))
 		return -EINVAL;
 
-	gbo = drm_gem_vram_of_gem(fb->obj[0]);
-
-	ret = drm_gem_vram_pin(gbo, 0);
+	ret = drm_gem_vram_pin(src_gbo, 0);
 	if (ret)
 		return ret;
-	ret = drm_gem_vram_vmap(gbo, &map);
+	ret = drm_gem_vram_vmap(src_gbo, &src_map);
 	if (ret)
-		goto err_drm_gem_vram_unpin;
-	src = map.vaddr; /* TODO: Use mapping abstraction properly */
+		goto err_drm_gem_vram_unpin_src;
+	src = src_map.vaddr; /* TODO: Use mapping abstraction properly */
 
-	dst = ast->cursor.map[ast->cursor.next_index].vaddr_iomem;
+	ret = drm_gem_vram_vmap(dst_gbo, &dst_map);
+	if (ret)
+		goto err_drm_gem_vram_vunmap_src;
+	dst = dst_map.vaddr_iomem; /* TODO: Use mapping abstraction properly */
 
 	/* do data transfer to cursor BO */
 	update_cursor_image(dst, src, fb->width, fb->height);
 
-	drm_gem_vram_vunmap(gbo, &map);
-	drm_gem_vram_unpin(gbo);
+	drm_gem_vram_vunmap(dst_gbo, &dst_map);
+	drm_gem_vram_vunmap(src_gbo, &src_map);
+	drm_gem_vram_unpin(src_gbo);
 
 	return 0;
 
-err_drm_gem_vram_unpin:
-	drm_gem_vram_unpin(gbo);
+err_drm_gem_vram_vunmap_src:
+	drm_gem_vram_vunmap(src_gbo, &src_map);
+err_drm_gem_vram_unpin_src:
+	drm_gem_vram_unpin(src_gbo);
 	return ret;
 }
 
@@ -251,17 +245,26 @@ static void ast_cursor_set_location(struct ast_private *ast, u16 x, u16 y,
 void ast_cursor_show(struct ast_private *ast, int x, int y,
 		     unsigned int offset_x, unsigned int offset_y)
 {
+	struct drm_device *dev = &ast->base;
+	struct drm_gem_vram_object *gbo = ast->cursor.gbo[ast->cursor.next_index];
+	struct dma_buf_map map;
 	u8 x_offset, y_offset;
 	u8 __iomem *dst;
 	u8 __iomem *sig;
 	u8 jreg;
+	int ret;
 
-	dst = ast->cursor.map[ast->cursor.next_index].vaddr;
+	ret = drm_gem_vram_vmap(gbo, &map);
+	if (drm_WARN_ONCE(dev, ret, "drm_gem_vram_vmap() failed, ret=%d\n", ret))
+		return;
+	dst = map.vaddr_iomem; /* TODO: Use mapping abstraction properly */
 
 	sig = dst + AST_HWC_SIZE;
 	writel(x, sig + AST_HWC_SIGNATURE_X);
 	writel(y, sig + AST_HWC_SIGNATURE_Y);
 
+	drm_gem_vram_vunmap(gbo, &map);
+
 	if (x < 0) {
 		x_offset = (-x) + offset_x;
 		x = 0;
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index ccaff81924ee..f871fc36c2f7 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -28,7 +28,6 @@
 #ifndef __AST_DRV_H__
 #define __AST_DRV_H__
 
-#include <linux/dma-buf-map.h>
 #include <linux/i2c.h>
 #include <linux/i2c-algo-bit.h>
 #include <linux/io.h>
@@ -133,7 +132,6 @@ struct ast_private {
 
 	struct {
 		struct drm_gem_vram_object *gbo[AST_DEFAULT_HWC_NUM];
-		struct dma_buf_map map[AST_DEFAULT_HWC_NUM];
 		unsigned int next_index;
 	} cursor;
 
-- 
2.29.2

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

[PATCH 3/8] drm/vram-helper: Provide drm_gem_vram_vmap_unlocked()

[Thomas Zimmermann]

The new interface drm_gem_vram_vmap_unlocked() acquires a GEM object's
reservation lock and vmaps the buffer into the kernel address space. In
contract to the existing vmap implementation, it does not pin the buffer.
The function will be helpful for code that needs to vmap and vunmap
single GEM objects.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_vram_helper.c | 52 +++++++++++++++++++++++++++
 include/drm/drm_gem_vram_helper.h     |  4 +++
 2 files changed, 56 insertions(+)

diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index 02ca22e90290..af54cb52423b 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -486,6 +486,58 @@ void drm_gem_vram_vunmap(struct drm_gem_vram_object *gbo, struct dma_buf_map *ma
 }
 EXPORT_SYMBOL(drm_gem_vram_vunmap);
 
+/**
+ * drm_gem_vram_vmap_unlocked() - Locks a GEM VRAM object and maps it into
+ *                                kernel address space
+ * @gbo: The GEM VRAM object to map
+ * @map: Returns the kernel virtual address of the VRAM GEM object's backing
+ *       store.
+ * @ctx: The locking context.
+ *
+ * This vmap function locks a GEM VRAM object and maps its buffer into kernel
+ * address space. Call drm_gem_vram_vunmap_unlocked() with the returned address
+ * to unmap and unlock the GEM VRAM object.
+ *
+ * Returns:
+ * 0 on success, or a negative error code otherwise.
+ */
+int drm_gem_vram_vmap_unlocked(struct drm_gem_vram_object *gbo, struct dma_buf_map *map,
+			       struct ww_acquire_ctx *ctx)
+{
+	int ret;
+
+	ret = ttm_bo_reserve(&gbo->bo, true, false, ctx);
+	if (ret)
+		return ret;
+
+	ret = drm_gem_vram_kmap_locked(gbo, map);
+	if (ret)
+		goto err_ttm_bo_unreserve;
+
+	return 0;
+
+err_ttm_bo_unreserve:
+	ttm_bo_unreserve(&gbo->bo);
+	return ret;
+}
+EXPORT_SYMBOL(drm_gem_vram_vmap_unlocked);
+
+/**
+ * drm_gem_vram_vunmap_unlocked() - Unmaps and unlocks a GEM VRAM object
+ * @gbo: The GEM VRAM object to unmap
+ * @map: Kernel virtual address where the VRAM GEM object was mapped
+ *
+ * A call to drm_gem_vram_vunmap_unlocked() unmaps and unlocks a GEM VRAM
+ * buffer object. See the documentation for drm_gem_vram_vmap_unlocked()
+ * for more information.
+ */
+void drm_gem_vram_vunmap_unlocked(struct drm_gem_vram_object *gbo, struct dma_buf_map *map)
+{
+	drm_gem_vram_kunmap_locked(gbo, map);
+	ttm_bo_unreserve(&gbo->bo);
+}
+EXPORT_SYMBOL(drm_gem_vram_vunmap_unlocked);
+
 /**
  * drm_gem_vram_fill_create_dumb() - \
 	Helper for implementing &struct drm_driver.dumb_create
diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
index a4bac02249c2..2d22888b5754 100644
--- a/include/drm/drm_gem_vram_helper.h
+++ b/include/drm/drm_gem_vram_helper.h
@@ -19,6 +19,7 @@ struct drm_plane_state;
 struct drm_simple_display_pipe;
 struct filp;
 struct vm_area_struct;
+struct ww_acquire_ctx;
 
 #define DRM_GEM_VRAM_PL_FLAG_SYSTEM	(1 << 0)
 #define DRM_GEM_VRAM_PL_FLAG_VRAM	(1 << 1)
@@ -99,6 +100,9 @@ int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, unsigned long pl_flag);
 int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo);
 int drm_gem_vram_vmap(struct drm_gem_vram_object *gbo, struct dma_buf_map *map);
 void drm_gem_vram_vunmap(struct drm_gem_vram_object *gbo, struct dma_buf_map *map);
+int drm_gem_vram_vmap_unlocked(struct drm_gem_vram_object *gbo, struct dma_buf_map *map,
+			       struct ww_acquire_ctx *ctx);
+void drm_gem_vram_vunmap_unlocked(struct drm_gem_vram_object *gbo, struct dma_buf_map *map);
 
 int drm_gem_vram_fill_create_dumb(struct drm_file *file,
 				  struct drm_device *dev,
-- 
2.29.2

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

[PATCH 4/8] drm/ast: Use drm_gem_vram_vmap_unlocked() in ast_cursor_show()

[Thomas Zimmermann]

This change avoids to pin the source BO when showing the cursor, and
instead acquires the BO's reservation lock. Prevents concurrent access
during the buffer update.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_cursor.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_cursor.c b/drivers/gpu/drm/ast/ast_cursor.c
index c5892827b220..7a56838fb703 100644
--- a/drivers/gpu/drm/ast/ast_cursor.c
+++ b/drivers/gpu/drm/ast/ast_cursor.c
@@ -254,8 +254,8 @@ void ast_cursor_show(struct ast_private *ast, int x, int y,
 	u8 jreg;
 	int ret;
 
-	ret = drm_gem_vram_vmap(gbo, &map);
-	if (drm_WARN_ONCE(dev, ret, "drm_gem_vram_vmap() failed, ret=%d\n", ret))
+	ret = drm_gem_vram_vmap_unlocked(gbo, &map, NULL);
+	if (drm_WARN_ONCE(dev, ret, "drm_gem_vram_vmap_unlocked() failed, ret=%d\n", ret))
 		return;
 	dst = map.vaddr_iomem; /* TODO: Use mapping abstraction properly */
 
@@ -263,7 +263,7 @@ void ast_cursor_show(struct ast_private *ast, int x, int y,
 	writel(x, sig + AST_HWC_SIGNATURE_X);
 	writel(y, sig + AST_HWC_SIGNATURE_Y);
 
-	drm_gem_vram_vunmap(gbo, &map);
+	drm_gem_vram_vunmap_unlocked(gbo, &map);
 
 	if (x < 0) {
 		x_offset = (-x) + offset_x;
-- 
2.29.2

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

[PATCH 5/8] drm/vboxvideo: Use drm_gem_vram_vmap_unlocked() in cursor update

[Thomas Zimmermann]

This change avoids to pin the source BO and instead acquires the BO's
reservations lock. Prevents concurrent access during the buffer update.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/vboxvideo/vbox_mode.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/vboxvideo/vbox_mode.c b/drivers/gpu/drm/vboxvideo/vbox_mode.c
index dbc0dd53c69e..cd3001fa209f 100644
--- a/drivers/gpu/drm/vboxvideo/vbox_mode.c
+++ b/drivers/gpu/drm/vboxvideo/vbox_mode.c
@@ -401,11 +401,8 @@ static void vbox_cursor_atomic_update(struct drm_plane *plane,
 
 	vbox_crtc->cursor_enabled = true;
 
-	ret = drm_gem_vram_vmap(gbo, &map);
+	ret = drm_gem_vram_vmap_unlocked(gbo, &map, NULL);
 	if (ret) {
-		/*
-		 * BUG: we should have pinned the BO in prepare_fb().
-		 */
 		mutex_unlock(&vbox->hw_mutex);
 		DRM_WARN("Could not map cursor bo, skipping update\n");
 		return;
@@ -421,7 +418,7 @@ static void vbox_cursor_atomic_update(struct drm_plane *plane,
 	data_size = width * height * 4 + mask_size;
 
 	copy_cursor_image(src, vbox->cursor_data, width, height, mask_size);
-	drm_gem_vram_vunmap(gbo, &map);
+	drm_gem_vram_vunmap_unlocked(gbo, &map);
 
 	flags = VBOX_MOUSE_POINTER_VISIBLE | VBOX_MOUSE_POINTER_SHAPE |
 		VBOX_MOUSE_POINTER_ALPHA;
-- 
2.29.2

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

[PATCH 6/8] drm/vram-helper: Remove pinning and locking from drm_gem_vram_vmap()

[Thomas Zimmermann]

BO pinning was never meant to be part of a GEM object's vmap operation.
Remove it from the related code in VRAM helpers. Pinning requires a
locking operation in the vmap code, which we also remove. Callers of
drm_gem_vram_vmap()/_vunmap() are now responsible for acquiring the GEM
object's reservation lock around their vmap/vunmap blocks.

The two remaining callers are fb helpers and ast cursor updates. The
fbdev helpers now call an explicit lock/unlock in their damage worker
to prevent the BO from being evicted.

The ast cursor code acquires the reservation locks of the cursor's HW BO
and the cursor plane's framebuffer BO. Once acquired, it vmaps both BOs
and updates the HW BO from the plane's BO.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_cursor.c      | 18 +++++++++------
 drivers/gpu/drm/drm_client.c          | 31 +++++++++++++++++++++++++
 drivers/gpu/drm/drm_fb_helper.c       | 10 ++++++--
 drivers/gpu/drm/drm_gem_vram_helper.c | 33 +++------------------------
 include/drm/drm_client.h              |  2 ++
 5 files changed, 55 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_cursor.c b/drivers/gpu/drm/ast/ast_cursor.c
index 7a56838fb703..9273276d278a 100644
--- a/drivers/gpu/drm/ast/ast_cursor.c
+++ b/drivers/gpu/drm/ast/ast_cursor.c
@@ -159,6 +159,8 @@ int ast_cursor_blit(struct ast_private *ast, struct drm_framebuffer *fb)
 	struct drm_device *dev = &ast->base;
 	struct drm_gem_vram_object *dst_gbo = ast->cursor.gbo[ast->cursor.next_index];
 	struct drm_gem_vram_object *src_gbo = drm_gem_vram_of_gem(fb->obj[0]);
+	struct drm_gem_object *objs[] = {&src_gbo->bo.base, &dst_gbo->bo.base};
+	struct ww_acquire_ctx ctx;
 	struct dma_buf_map src_map, dst_map;
 	void __iomem *dst;
 	void *src;
@@ -168,17 +170,18 @@ int ast_cursor_blit(struct ast_private *ast, struct drm_framebuffer *fb)
 	    drm_WARN_ON_ONCE(dev, fb->height > AST_MAX_HWC_HEIGHT))
 		return -EINVAL;
 
-	ret = drm_gem_vram_pin(src_gbo, 0);
+	ret = drm_gem_lock_reservations(objs, ARRAY_SIZE(objs), &ctx);
 	if (ret)
 		return ret;
+
 	ret = drm_gem_vram_vmap(src_gbo, &src_map);
 	if (ret)
-		goto err_drm_gem_vram_unpin_src;
+		goto err_drm_gem_unlock_reservations;
 	src = src_map.vaddr; /* TODO: Use mapping abstraction properly */
 
 	ret = drm_gem_vram_vmap(dst_gbo, &dst_map);
 	if (ret)
-		goto err_drm_gem_vram_vunmap_src;
+		goto err_drm_gem_vram_vunmap;
 	dst = dst_map.vaddr_iomem; /* TODO: Use mapping abstraction properly */
 
 	/* do data transfer to cursor BO */
@@ -186,14 +189,15 @@ int ast_cursor_blit(struct ast_private *ast, struct drm_framebuffer *fb)
 
 	drm_gem_vram_vunmap(dst_gbo, &dst_map);
 	drm_gem_vram_vunmap(src_gbo, &src_map);
-	drm_gem_vram_unpin(src_gbo);
+
+	drm_gem_unlock_reservations(objs, ARRAY_SIZE(objs), &ctx);
 
 	return 0;
 
-err_drm_gem_vram_vunmap_src:
+err_drm_gem_vram_vunmap:
 	drm_gem_vram_vunmap(src_gbo, &src_map);
-err_drm_gem_vram_unpin_src:
-	drm_gem_vram_unpin(src_gbo);
+err_drm_gem_unlock_reservations:
+	drm_gem_unlock_reservations(objs, ARRAY_SIZE(objs), &ctx);
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
index ce45e380f4a2..82453ca0b3ec 100644
--- a/drivers/gpu/drm/drm_client.c
+++ b/drivers/gpu/drm/drm_client.c
@@ -288,6 +288,37 @@ drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u
 	return ERR_PTR(ret);
 }
 
+/**
+ * drm_client_buffer_lock - Locks the DRM client buffer
+ * @buffer: DRM client buffer
+ *
+ * This function locks the client buffer by acquiring the buffer
+ * object's reservation lock.
+ *
+ * Unlock the buffer with drm_client_buffer_unlock().
+ *
+ * Returns:
+ *	0 on success, or a negative errno code otherwise.
+ */
+int
+drm_client_buffer_lock(struct drm_client_buffer *buffer)
+{
+	return dma_resv_lock(buffer->gem->resv, NULL);
+}
+EXPORT_SYMBOL(drm_client_buffer_lock);
+
+/**
+ * drm_client_buffer_unlock - Unlock DRM client buffer
+ * @buffer: DRM client buffer
+ *
+ * Unlocks a client buffer. See drm_client_buffer_lock().
+ */
+void drm_client_buffer_unlock(struct drm_client_buffer *buffer)
+{
+	dma_resv_unlock(buffer->gem->resv);
+}
+EXPORT_SYMBOL(drm_client_buffer_unlock);
+
 /**
  * drm_client_buffer_vmap - Map DRM client buffer into address space
  * @buffer: DRM client buffer
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 4b8119510687..97856d9194de 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -411,16 +411,22 @@ static int drm_fb_helper_damage_blit(struct drm_fb_helper *fb_helper,
 	 */
 	mutex_lock(&fb_helper->lock);
 
+	ret = drm_client_buffer_lock(buffer);
+	if (ret)
+		goto out_mutex_unlock;
+
 	ret = drm_client_buffer_vmap(buffer, &map);
 	if (ret)
-		goto out;
+		goto out_drm_client_buffer_unlock;
 
 	dst = map;
 	drm_fb_helper_damage_blit_real(fb_helper, clip, &dst);
 
 	drm_client_buffer_vunmap(buffer);
 
-out:
+out_drm_client_buffer_unlock:
+	drm_client_buffer_unlock(buffer);
+out_mutex_unlock:
 	mutex_unlock(&fb_helper->lock);
 
 	return ret;
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index af54cb52423b..ac0fa9da1c83 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -438,28 +438,9 @@ static void drm_gem_vram_kunmap_locked(struct drm_gem_vram_object *gbo,
  */
 int drm_gem_vram_vmap(struct drm_gem_vram_object *gbo, struct dma_buf_map *map)
 {
-	int ret;
+	dma_resv_assert_held(gbo->bo.base.resv);
 
-	ret = ttm_bo_reserve(&gbo->bo, true, false, NULL);
-	if (ret)
-		return ret;
-
-	ret = drm_gem_vram_pin_locked(gbo, 0);
-	if (ret)
-		goto err_ttm_bo_unreserve;
-	ret = drm_gem_vram_kmap_locked(gbo, map);
-	if (ret)
-		goto err_drm_gem_vram_unpin_locked;
-
-	ttm_bo_unreserve(&gbo->bo);
-
-	return 0;
-
-err_drm_gem_vram_unpin_locked:
-	drm_gem_vram_unpin_locked(gbo);
-err_ttm_bo_unreserve:
-	ttm_bo_unreserve(&gbo->bo);
-	return ret;
+	return drm_gem_vram_kmap_locked(gbo, map);
 }
 EXPORT_SYMBOL(drm_gem_vram_vmap);
 
@@ -473,16 +454,8 @@ EXPORT_SYMBOL(drm_gem_vram_vmap);
  */
 void drm_gem_vram_vunmap(struct drm_gem_vram_object *gbo, struct dma_buf_map *map)
 {
-	int ret;
-
-	ret = ttm_bo_reserve(&gbo->bo, false, false, NULL);
-	if (WARN_ONCE(ret, "ttm_bo_reserve_failed(): ret=%d\n", ret))
-		return;
-
+	dma_resv_assert_held(gbo->bo.base.resv);
 	drm_gem_vram_kunmap_locked(gbo, map);
-	drm_gem_vram_unpin_locked(gbo);
-
-	ttm_bo_unreserve(&gbo->bo);
 }
 EXPORT_SYMBOL(drm_gem_vram_vunmap);
 
diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
index f07f2fb02e75..1cf811471fc4 100644
--- a/include/drm/drm_client.h
+++ b/include/drm/drm_client.h
@@ -156,6 +156,8 @@ struct drm_client_buffer *
 drm_client_framebuffer_create(struct drm_client_dev *client, u32 width, u32 height, u32 format);
 void drm_client_framebuffer_delete(struct drm_client_buffer *buffer);
 int drm_client_framebuffer_flush(struct drm_client_buffer *buffer, struct drm_rect *rect);
+int drm_client_buffer_lock(struct drm_client_buffer *buffer);
+void drm_client_buffer_unlock(struct drm_client_buffer *buffer);
 int drm_client_buffer_vmap(struct drm_client_buffer *buffer, struct dma_buf_map *map);
 void drm_client_buffer_vunmap(struct drm_client_buffer *buffer);
 
-- 
2.29.2

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

[PATCH 7/8] drm/vram-helper: Remove vmap reference counting

[Thomas Zimmermann]

Overlapping or nested mappings of the same BO are not allowed by the
semantics of the GEM vmap/vunmap operations. Concurent access to the
GEM object is prevented by reservation locks.

So we don't need the reference counter in the GEM VRAM object. Remove
it.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_vram_helper.c | 19 ++++---------------
 include/drm/drm_gem_vram_helper.h     | 17 +++--------------
 2 files changed, 7 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index ac0fa9da1c83..a44718dd66cb 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -113,7 +113,6 @@ static void drm_gem_vram_cleanup(struct drm_gem_vram_object *gbo)
 	 * up; only release the GEM object.
 	 */
 
-	WARN_ON(gbo->vmap_use_count);
 	WARN_ON(dma_buf_map_is_set(&gbo->map));
 
 	drm_gem_object_release(&gbo->bo.base);
@@ -384,15 +383,10 @@ static int drm_gem_vram_kmap_locked(struct drm_gem_vram_object *gbo,
 {
 	int ret;
 
-	if (gbo->vmap_use_count > 0)
-		goto out;
-
 	ret = ttm_bo_vmap(&gbo->bo, &gbo->map);
 	if (ret)
 		return ret;
 
-out:
-	++gbo->vmap_use_count;
 	*map = gbo->map;
 
 	return 0;
@@ -403,15 +397,9 @@ static void drm_gem_vram_kunmap_locked(struct drm_gem_vram_object *gbo,
 {
 	struct drm_device *dev = gbo->bo.base.dev;
 
-	if (drm_WARN_ON_ONCE(dev, !gbo->vmap_use_count))
-		return;
-
 	if (drm_WARN_ON_ONCE(dev, !dma_buf_map_is_equal(&gbo->map, map)))
 		return; /* BUG: map not mapped from this BO */
 
-	if (--gbo->vmap_use_count > 0)
-		return;
-
 	/*
 	 * Permanently mapping and unmapping buffers adds overhead from
 	 * updating the page tables and creates debugging output. Therefore,
@@ -596,12 +584,13 @@ static void drm_gem_vram_bo_driver_move_notify(struct drm_gem_vram_object *gbo,
 					       struct ttm_resource *new_mem)
 {
 	struct ttm_buffer_object *bo = &gbo->bo;
-	struct drm_device *dev = bo->base.dev;
+	struct dma_buf_map *map = &gbo->map;
 
-	if (drm_WARN_ON_ONCE(dev, gbo->vmap_use_count))
+	if (dma_buf_map_is_null(map))
 		return;
 
-	ttm_bo_vunmap(bo, &gbo->map);
+	ttm_bo_vunmap(bo, map);
+	dma_buf_map_clear(map);
 }
 
 static int drm_gem_vram_bo_driver_move(struct drm_gem_vram_object *gbo,
diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
index 2d22888b5754..b679349bbfb2 100644
--- a/include/drm/drm_gem_vram_helper.h
+++ b/include/drm/drm_gem_vram_helper.h
@@ -42,25 +42,14 @@ struct ww_acquire_ctx;
  * dedicated memory. The buffer object can be evicted to system memory if
  * video memory becomes scarce.
  *
- * GEM VRAM objects perform reference counting for pin and mapping
- * operations. So a buffer object that has been pinned N times with
- * drm_gem_vram_pin() must be unpinned N times with
- * drm_gem_vram_unpin(). The same applies to pairs of
- * drm_gem_vram_kmap() and drm_gem_vram_kunmap(), as well as pairs of
- * drm_gem_vram_vmap() and drm_gem_vram_vunmap().
+ * GEM VRAM objects perform reference counting for pin operations. So a
+ * buffer object that has been pinned N times with drm_gem_vram_pin() must
+ * be unpinned N times with drm_gem_vram_unpin().
  */
 struct drm_gem_vram_object {
 	struct ttm_buffer_object bo;
 	struct dma_buf_map map;
 
-	/**
-	 * @vmap_use_count:
-	 *
-	 * Reference count on the virtual address.
-	 * The address are un-mapped when the count reaches zero.
-	 */
-	unsigned int vmap_use_count;
-
 	/* Supported placements are %TTM_PL_VRAM and %TTM_PL_SYSTEM */
 	struct ttm_placement placement;
 	struct ttm_place placements[2];
-- 
2.29.2

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

[PATCH 8/8] drm/vram-helper: Simplify vmap implementation

[Thomas Zimmermann]

After removing the pinning operations, the vmap/vunmap code as been
reduced to what used to be an internal helper. Inline the helper to
simplify the implementation.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_vram_helper.c | 57 +++++++++++----------------
 1 file changed, 23 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index a44718dd66cb..10d89c01990f 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -378,36 +378,6 @@ int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo)
 }
 EXPORT_SYMBOL(drm_gem_vram_unpin);
 
-static int drm_gem_vram_kmap_locked(struct drm_gem_vram_object *gbo,
-				    struct dma_buf_map *map)
-{
-	int ret;
-
-	ret = ttm_bo_vmap(&gbo->bo, &gbo->map);
-	if (ret)
-		return ret;
-
-	*map = gbo->map;
-
-	return 0;
-}
-
-static void drm_gem_vram_kunmap_locked(struct drm_gem_vram_object *gbo,
-				       struct dma_buf_map *map)
-{
-	struct drm_device *dev = gbo->bo.base.dev;
-
-	if (drm_WARN_ON_ONCE(dev, !dma_buf_map_is_equal(&gbo->map, map)))
-		return; /* BUG: map not mapped from this BO */
-
-	/*
-	 * Permanently mapping and unmapping buffers adds overhead from
-	 * updating the page tables and creates debugging output. Therefore,
-	 * we delay the actual unmap operation until the BO gets evicted
-	 * from memory. See drm_gem_vram_bo_driver_move_notify().
-	 */
-}
-
 /**
  * drm_gem_vram_vmap() - Pins and maps a GEM VRAM object into kernel address
  *                       space
@@ -426,9 +396,17 @@ static void drm_gem_vram_kunmap_locked(struct drm_gem_vram_object *gbo,
  */
 int drm_gem_vram_vmap(struct drm_gem_vram_object *gbo, struct dma_buf_map *map)
 {
+	int ret;
+
 	dma_resv_assert_held(gbo->bo.base.resv);
 
-	return drm_gem_vram_kmap_locked(gbo, map);
+	ret = ttm_bo_vmap(&gbo->bo, &gbo->map);
+	if (ret)
+		return ret;
+
+	*map = gbo->map;
+
+	return 0;
 }
 EXPORT_SYMBOL(drm_gem_vram_vmap);
 
@@ -442,8 +420,19 @@ EXPORT_SYMBOL(drm_gem_vram_vmap);
  */
 void drm_gem_vram_vunmap(struct drm_gem_vram_object *gbo, struct dma_buf_map *map)
 {
+	struct drm_device *dev = gbo->bo.base.dev;
+
 	dma_resv_assert_held(gbo->bo.base.resv);
-	drm_gem_vram_kunmap_locked(gbo, map);
+
+	if (drm_WARN_ON_ONCE(dev, !dma_buf_map_is_equal(&gbo->map, map)))
+		return; /* BUG: map not mapped from this BO */
+
+	/*
+	 * Permanently mapping and unmapping buffers adds overhead from
+	 * updating the page tables and creates debugging output. Therefore,
+	 * we delay the actual unmap operation until the BO gets evicted
+	 * from memory. See drm_gem_vram_bo_driver_move_notify().
+	 */
 }
 EXPORT_SYMBOL(drm_gem_vram_vunmap);
 
@@ -471,7 +460,7 @@ int drm_gem_vram_vmap_unlocked(struct drm_gem_vram_object *gbo, struct dma_buf_m
 	if (ret)
 		return ret;
 
-	ret = drm_gem_vram_kmap_locked(gbo, map);
+	ret = drm_gem_vram_vmap(gbo, map);
 	if (ret)
 		goto err_ttm_bo_unreserve;
 
@@ -494,7 +483,7 @@ EXPORT_SYMBOL(drm_gem_vram_vmap_unlocked);
  */
 void drm_gem_vram_vunmap_unlocked(struct drm_gem_vram_object *gbo, struct dma_buf_map *map)
 {
-	drm_gem_vram_kunmap_locked(gbo, map);
+	drm_gem_vram_vunmap(gbo, map);
 	ttm_bo_unreserve(&gbo->bo);
 }
 EXPORT_SYMBOL(drm_gem_vram_vunmap_unlocked);
-- 
2.29.2

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

Re: [PATCH 0/8] drm/vram-helper: Lock GEM BOs while they are mapped

[Christian König]

Am 30.11.20 um 13:04 schrieb Thomas Zimmermann:
> GEM VRAM helpers used to pin the BO in their implementation of vmap, so
> that they could not be relocated. In a recent discussion, [1] it became
> clear that this is incorrect and that vmap should rather repend on the
> reservation lock to prevent relocation. This patchset addresses the issue.
> Besides the vram helpers, this affects ast, vboxvideo and the generic
> fbdev emulation.
>
> Patch 1 adds a few more rules to vmap internfaces. With VRAM, it is
> necessary to keep the BO evictable, which requires soem care when mapping
> the memory. Patch 2 changes ast's cursor code accordingly.
>
> Patch 3 adds vram helpers that acquires the reservation lock and vmap the
> memory buffer. Same for vunmap in reverse. Patches 4 and 5 convert ast
> and vboxvideo to the new helper.
>
> Patch 6 removes pinning and locking from VRAM helper's vmap and vunmap.
> The affected users in ast and fbdev emulation acquire the reservation
> locks of the GEM objects before vmapping BOs. VRAM helpers don't support
> to export the buffer, so there are no other users of these functions.
>
> The the pinning and locking removed, vmap can be simplified as done in
> patches 7 and 8.
>
> Tested on ast with GEM VRAM and also on mgag200 to verify that the fbdev
> change does not interfere with GEM SHMEM.

Acked-by: Christian König <christian.koenig@amd.com> for the series.

>
> Thomas Zimmermann (8):
>    drm/gem: Write down some rules for vmap usage
>    drm/ast: Only map cursor BOs during updates
>    drm/vram-helper: Provide drm_gem_vram_vmap_unlocked()
>    drm/ast: Use drm_gem_vram_vmap_unlocked() in ast_cursor_show()
>    drm/vboxvideo: Use drm_gem_vram_vmap_unlocked() in cursor update
>    drm/vram-helper: Remove pinning and locking from drm_gem_vram_vmap()
>    drm/vram-helper: Remove vmap reference counting
>    drm/vram-helper: Simplify vmap implementation
>
>   drivers/gpu/drm/ast/ast_cursor.c      |  63 +++++++++-------
>   drivers/gpu/drm/ast/ast_drv.h         |   2 -
>   drivers/gpu/drm/drm_client.c          |  31 ++++++++
>   drivers/gpu/drm/drm_fb_helper.c       |  10 ++-
>   drivers/gpu/drm/drm_gem_vram_helper.c | 101 +++++++++++++-------------
>   drivers/gpu/drm/drm_prime.c           |   6 ++
>   drivers/gpu/drm/vboxvideo/vbox_mode.c |   7 +-
>   include/drm/drm_client.h              |   2 +
>   include/drm/drm_gem.h                 |  16 ++++
>   include/drm/drm_gem_vram_helper.h     |  21 ++----
>   10 files changed, 159 insertions(+), 100 deletions(-)
>
> --
> 2.29.2
>

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

Re: [PATCH 0/8] drm/vram-helper: Lock GEM BOs while they are mapped

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 2877 bytes --]

Hi

Am 30.11.20 um 13:04 schrieb Thomas Zimmermann:
> GEM VRAM helpers used to pin the BO in their implementation of vmap, so
> that they could not be relocated. In a recent discussion, [1] it became

[1] was supposed to point to the discussion at

   https://patchwork.freedesktop.org/patch/400054/?series=83765&rev=1



> clear that this is incorrect and that vmap should rather repend on the
> reservation lock to prevent relocation. This patchset addresses the issue.
> Besides the vram helpers, this affects ast, vboxvideo and the generic
> fbdev emulation.
> 
> Patch 1 adds a few more rules to vmap internfaces. With VRAM, it is
> necessary to keep the BO evictable, which requires soem care when mapping
> the memory. Patch 2 changes ast's cursor code accordingly.
> 
> Patch 3 adds vram helpers that acquires the reservation lock and vmap the
> memory buffer. Same for vunmap in reverse. Patches 4 and 5 convert ast
> and vboxvideo to the new helper.
> 
> Patch 6 removes pinning and locking from VRAM helper's vmap and vunmap.
> The affected users in ast and fbdev emulation acquire the reservation
> locks of the GEM objects before vmapping BOs. VRAM helpers don't support
> to export the buffer, so there are no other users of these functions.
> 
> The the pinning and locking removed, vmap can be simplified as done in
> patches 7 and 8.
> 
> Tested on ast with GEM VRAM and also on mgag200 to verify that the fbdev
> change does not interfere with GEM SHMEM.
> 
> Thomas Zimmermann (8):
>    drm/gem: Write down some rules for vmap usage
>    drm/ast: Only map cursor BOs during updates
>    drm/vram-helper: Provide drm_gem_vram_vmap_unlocked()
>    drm/ast: Use drm_gem_vram_vmap_unlocked() in ast_cursor_show()
>    drm/vboxvideo: Use drm_gem_vram_vmap_unlocked() in cursor update
>    drm/vram-helper: Remove pinning and locking from drm_gem_vram_vmap()
>    drm/vram-helper: Remove vmap reference counting
>    drm/vram-helper: Simplify vmap implementation
> 
>   drivers/gpu/drm/ast/ast_cursor.c      |  63 +++++++++-------
>   drivers/gpu/drm/ast/ast_drv.h         |   2 -
>   drivers/gpu/drm/drm_client.c          |  31 ++++++++
>   drivers/gpu/drm/drm_fb_helper.c       |  10 ++-
>   drivers/gpu/drm/drm_gem_vram_helper.c | 101 +++++++++++++-------------
>   drivers/gpu/drm/drm_prime.c           |   6 ++
>   drivers/gpu/drm/vboxvideo/vbox_mode.c |   7 +-
>   include/drm/drm_client.h              |   2 +
>   include/drm/drm_gem.h                 |  16 ++++
>   include/drm/drm_gem_vram_helper.h     |  21 ++----
>   10 files changed, 159 insertions(+), 100 deletions(-)
> 
> --
> 2.29.2
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 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: [PATCH 1/8] drm/gem: Write down some rules for vmap usage

[Daniel Vetter]

On Mon, Nov 30, 2020 at 01:04:26PM +0100, Thomas Zimmermann wrote:
> Mapping a GEM object's buffer into kernel address space prevents the
> buffer from being evicted from VRAM, which in turn may result in
> out-of-memory errors. It's therefore required to only vmap GEM BOs for
> short periods of time; unless the GEM implementation provides additional
> guarantees.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_prime.c |  6 ++++++
>  include/drm/drm_gem.h       | 16 ++++++++++++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 7db55fce35d8..9c9ece9833e0 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -669,6 +669,12 @@ EXPORT_SYMBOL(drm_gem_unmap_dma_buf);
>   * callback. Calls into &drm_gem_object_funcs.vmap for device specific handling.
>   * The kernel virtual address is returned in map.
>   *
> + * To prevent the GEM object from being relocated, callers must hold the GEM
> + * object's reservation lock from when calling this function until releasing the
> + * mapping. Holding onto a mapping and the associated reservation lock for an
> + * unbound time may result in out-of-memory errors. Calls to drm_gem_dmabuf_vmap()
> + * should therefore be accompanied by a call to drm_gem_dmabuf_vunmap().
> + *
>   * Returns 0 on success or a negative errno code otherwise.

This is a dma-buf hook, which means just documenting the rules you'd like
to have here isn't enough. We need to roll this out at the dma-buf level,
and enforce it.

Enforce it = assert_lock_held

Roll out = review everyone. Because this goes through dma-buf it'll come
back through shmem helpers (and other helpers and other subsystems) back
to any driver using vmap for gpu buffers. This includes the media
subsystem, and the media subsystem definitely doesn't cope with just
temporarily mapping buffers. So there we need to pin them, which I think
means we'll need 2 version of dma_buf_vmap - one that's temporary and
requires we hold dma_resv lock, the other requires that the buffer is
pinned.

That's what I meant with that this approach here is very sprawling :-/
-Daniel

>   */
>  int drm_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct dma_buf_map *map)
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index 5e6daa1c982f..7c34cd5ec261 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -137,7 +137,21 @@ struct drm_gem_object_funcs {
>  	 * Returns a virtual address for the buffer. Used by the
>  	 * drm_gem_dmabuf_vmap() helper.
>  	 *
> +	 * Notes to implementors:
> +	 *
> +	 * - Implementations must expect pairs of @vmap and @vunmap to be
> +	 *   called frequently and should optimize for this case.
> +	 *
> +	 * - Implemenations may expect the caller to hold the GEM object's
> +	 *   reservation lock to protect against concurrent calls and relocation
> +	 *   of the GEM object.
> +	 *
> +	 * - Implementations may provide additional guarantees (e.g., working
> +	 *   without holding the reservation lock).
> +	 *
>  	 * This callback is optional.
> +	 *
> +	 * See also drm_gem_dmabuf_vmap()
>  	 */
>  	int (*vmap)(struct drm_gem_object *obj, struct dma_buf_map *map);
>  
> @@ -148,6 +162,8 @@ struct drm_gem_object_funcs {
>  	 * drm_gem_dmabuf_vunmap() helper.
>  	 *
>  	 * This callback is optional.
> +	 *
> +	 * See also @vmap.
>  	 */
>  	void (*vunmap)(struct drm_gem_object *obj, struct dma_buf_map *map);
>  
> -- 
> 2.29.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] drm/gem: Write down some rules for vmap usage

[Christian König]

Am 30.11.20 um 16:30 schrieb Daniel Vetter:
> On Mon, Nov 30, 2020 at 01:04:26PM +0100, Thomas Zimmermann wrote:
>> Mapping a GEM object's buffer into kernel address space prevents the
>> buffer from being evicted from VRAM, which in turn may result in
>> out-of-memory errors. It's therefore required to only vmap GEM BOs for
>> short periods of time; unless the GEM implementation provides additional
>> guarantees.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/drm_prime.c |  6 ++++++
>>   include/drm/drm_gem.h       | 16 ++++++++++++++++
>>   2 files changed, 22 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>> index 7db55fce35d8..9c9ece9833e0 100644
>> --- a/drivers/gpu/drm/drm_prime.c
>> +++ b/drivers/gpu/drm/drm_prime.c
>> @@ -669,6 +669,12 @@ EXPORT_SYMBOL(drm_gem_unmap_dma_buf);
>>    * callback. Calls into &drm_gem_object_funcs.vmap for device specific handling.
>>    * The kernel virtual address is returned in map.
>>    *
>> + * To prevent the GEM object from being relocated, callers must hold the GEM
>> + * object's reservation lock from when calling this function until releasing the
>> + * mapping. Holding onto a mapping and the associated reservation lock for an
>> + * unbound time may result in out-of-memory errors. Calls to drm_gem_dmabuf_vmap()
>> + * should therefore be accompanied by a call to drm_gem_dmabuf_vunmap().
>> + *
>>    * Returns 0 on success or a negative errno code otherwise.
> This is a dma-buf hook, which means just documenting the rules you'd like
> to have here isn't enough. We need to roll this out at the dma-buf level,
> and enforce it.
>
> Enforce it = assert_lock_held
>
> Roll out = review everyone. Because this goes through dma-buf it'll come
> back through shmem helpers (and other helpers and other subsystems) back
> to any driver using vmap for gpu buffers. This includes the media
> subsystem, and the media subsystem definitely doesn't cope with just
> temporarily mapping buffers. So there we need to pin them, which I think
> means we'll need 2 version of dma_buf_vmap - one that's temporary and
> requires we hold dma_resv lock, the other requires that the buffer is
> pinned.

OR start to proper use the dma_buf_pin/dma_buf_unpin functions which I 
added to cover this use case as well.

Cheers,
Christian.

>
> That's what I meant with that this approach here is very sprawling :-/
> -Daniel
>
>>    */
>>   int drm_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct dma_buf_map *map)
>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
>> index 5e6daa1c982f..7c34cd5ec261 100644
>> --- a/include/drm/drm_gem.h
>> +++ b/include/drm/drm_gem.h
>> @@ -137,7 +137,21 @@ struct drm_gem_object_funcs {
>>   	 * Returns a virtual address for the buffer. Used by the
>>   	 * drm_gem_dmabuf_vmap() helper.
>>   	 *
>> +	 * Notes to implementors:
>> +	 *
>> +	 * - Implementations must expect pairs of @vmap and @vunmap to be
>> +	 *   called frequently and should optimize for this case.
>> +	 *
>> +	 * - Implemenations may expect the caller to hold the GEM object's
>> +	 *   reservation lock to protect against concurrent calls and relocation
>> +	 *   of the GEM object.
>> +	 *
>> +	 * - Implementations may provide additional guarantees (e.g., working
>> +	 *   without holding the reservation lock).
>> +	 *
>>   	 * This callback is optional.
>> +	 *
>> +	 * See also drm_gem_dmabuf_vmap()
>>   	 */
>>   	int (*vmap)(struct drm_gem_object *obj, struct dma_buf_map *map);
>>   
>> @@ -148,6 +162,8 @@ struct drm_gem_object_funcs {
>>   	 * drm_gem_dmabuf_vunmap() helper.
>>   	 *
>>   	 * This callback is optional.
>> +	 *
>> +	 * See also @vmap.
>>   	 */
>>   	void (*vunmap)(struct drm_gem_object *obj, struct dma_buf_map *map);
>>   
>> -- 
>> 2.29.2
>>

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

Re: [PATCH 1/8] drm/gem: Write down some rules for vmap usage

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 4335 bytes --]



Am 30.11.20 um 16:30 schrieb Daniel Vetter:
> On Mon, Nov 30, 2020 at 01:04:26PM +0100, Thomas Zimmermann wrote:
>> Mapping a GEM object's buffer into kernel address space prevents the
>> buffer from being evicted from VRAM, which in turn may result in
>> out-of-memory errors. It's therefore required to only vmap GEM BOs for
>> short periods of time; unless the GEM implementation provides additional
>> guarantees.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/drm_prime.c |  6 ++++++
>>   include/drm/drm_gem.h       | 16 ++++++++++++++++
>>   2 files changed, 22 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>> index 7db55fce35d8..9c9ece9833e0 100644
>> --- a/drivers/gpu/drm/drm_prime.c
>> +++ b/drivers/gpu/drm/drm_prime.c
>> @@ -669,6 +669,12 @@ EXPORT_SYMBOL(drm_gem_unmap_dma_buf);
>>    * callback. Calls into &drm_gem_object_funcs.vmap for device specific handling.
>>    * The kernel virtual address is returned in map.
>>    *
>> + * To prevent the GEM object from being relocated, callers must hold the GEM
>> + * object's reservation lock from when calling this function until releasing the
>> + * mapping. Holding onto a mapping and the associated reservation lock for an
>> + * unbound time may result in out-of-memory errors. Calls to drm_gem_dmabuf_vmap()
>> + * should therefore be accompanied by a call to drm_gem_dmabuf_vunmap().
>> + *
>>    * Returns 0 on success or a negative errno code otherwise.
> 
> This is a dma-buf hook, which means just documenting the rules you'd like
> to have here isn't enough. We need to roll this out at the dma-buf level,
> and enforce it.
> 

The documentation for GEM vmap callbacks point here, so it was a good 
point to start.

I know about the dependencies on dmabuf. But fixing everything now is 
unrealistic. My hope for this patch is that we can find the necessary 
rules and document them.

> Enforce it = assert_lock_held

That's probably the final step of many.

Best regards
Thomas

> 
> Roll out = review everyone. Because this goes through dma-buf it'll come
> back through shmem helpers (and other helpers and other subsystems) back
> to any driver using vmap for gpu buffers. This includes the media
> subsystem, and the media subsystem definitely doesn't cope with just
> temporarily mapping buffers. So there we need to pin them, which I think
> means we'll need 2 version of dma_buf_vmap - one that's temporary and
> requires we hold dma_resv lock, the other requires that the buffer is
> pinned.
> 
> That's what I meant with that this approach here is very sprawling :-/
> -Daniel
> 
>>    */
>>   int drm_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct dma_buf_map *map)
>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
>> index 5e6daa1c982f..7c34cd5ec261 100644
>> --- a/include/drm/drm_gem.h
>> +++ b/include/drm/drm_gem.h
>> @@ -137,7 +137,21 @@ struct drm_gem_object_funcs {
>>   	 * Returns a virtual address for the buffer. Used by the
>>   	 * drm_gem_dmabuf_vmap() helper.
>>   	 *
>> +	 * Notes to implementors:
>> +	 *
>> +	 * - Implementations must expect pairs of @vmap and @vunmap to be
>> +	 *   called frequently and should optimize for this case.
>> +	 *
>> +	 * - Implemenations may expect the caller to hold the GEM object's
>> +	 *   reservation lock to protect against concurrent calls and relocation
>> +	 *   of the GEM object.
>> +	 *
>> +	 * - Implementations may provide additional guarantees (e.g., working
>> +	 *   without holding the reservation lock).
>> +	 *
>>   	 * This callback is optional.
>> +	 *
>> +	 * See also drm_gem_dmabuf_vmap()
>>   	 */
>>   	int (*vmap)(struct drm_gem_object *obj, struct dma_buf_map *map);
>>   
>> @@ -148,6 +162,8 @@ struct drm_gem_object_funcs {
>>   	 * drm_gem_dmabuf_vunmap() helper.
>>   	 *
>>   	 * This callback is optional.
>> +	 *
>> +	 * See also @vmap.
>>   	 */
>>   	void (*vunmap)(struct drm_gem_object *obj, struct dma_buf_map *map);
>>   
>> -- 
>> 2.29.2
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 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: [PATCH 1/8] drm/gem: Write down some rules for vmap usage

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 6058 bytes --]

Hi

Am 30.11.20 um 16:33 schrieb Christian König:
> Am 30.11.20 um 16:30 schrieb Daniel Vetter:
>> On Mon, Nov 30, 2020 at 01:04:26PM +0100, Thomas Zimmermann wrote:
>>> Mapping a GEM object's buffer into kernel address space prevents the
>>> buffer from being evicted from VRAM, which in turn may result in
>>> out-of-memory errors. It's therefore required to only vmap GEM BOs for
>>> short periods of time; unless the GEM implementation provides additional
>>> guarantees.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---
>>>   drivers/gpu/drm/drm_prime.c |  6 ++++++
>>>   include/drm/drm_gem.h       | 16 ++++++++++++++++
>>>   2 files changed, 22 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>>> index 7db55fce35d8..9c9ece9833e0 100644
>>> --- a/drivers/gpu/drm/drm_prime.c
>>> +++ b/drivers/gpu/drm/drm_prime.c
>>> @@ -669,6 +669,12 @@ EXPORT_SYMBOL(drm_gem_unmap_dma_buf);
>>>    * callback. Calls into &drm_gem_object_funcs.vmap for device 
>>> specific handling.
>>>    * The kernel virtual address is returned in map.
>>>    *
>>> + * To prevent the GEM object from being relocated, callers must hold 
>>> the GEM
>>> + * object's reservation lock from when calling this function until 
>>> releasing the
>>> + * mapping. Holding onto a mapping and the associated reservation 
>>> lock for an
>>> + * unbound time may result in out-of-memory errors. Calls to 
>>> drm_gem_dmabuf_vmap()
>>> + * should therefore be accompanied by a call to 
>>> drm_gem_dmabuf_vunmap().
>>> + *
>>>    * Returns 0 on success or a negative errno code otherwise.
>> This is a dma-buf hook, which means just documenting the rules you'd like
>> to have here isn't enough. We need to roll this out at the dma-buf level,
>> and enforce it.
>>
>> Enforce it = assert_lock_held
>>
>> Roll out = review everyone. Because this goes through dma-buf it'll come
>> back through shmem helpers (and other helpers and other subsystems) back
>> to any driver using vmap for gpu buffers. This includes the media
>> subsystem, and the media subsystem definitely doesn't cope with just
>> temporarily mapping buffers. So there we need to pin them, which I think
>> means we'll need 2 version of dma_buf_vmap - one that's temporary and
>> requires we hold dma_resv lock, the other requires that the buffer is
>> pinned.
> 
> OR start to proper use the dma_buf_pin/dma_buf_unpin functions which I 
> added to cover this use case as well.

While I generally agree, here are some thoughts:

I found all generic pin functions useless, because they don't allow for 
specifying where to pin. With fbdev emulation, this means that console 
buffers might never make it to VRAM for scanout. If anything, the policy 
should be that pin always pins in HW-accessible memory.

Pin has quite a bit of overhead (more locking, buffer movement), so it 
should be the second choice after regular vmap. To make both work 
together, pin probably relies on holding the reservation lock internally.

Therefore I think we still would want some additional helpers, such as:

   pin_unlocked(), which acquires the resv lock, calls regular pin and 
then drops the resv lock. Same for unpin_unlocked()

   vmap_pinned(), which enforces that the buffer has been pinned and 
then calls regalar vmap. Same for vunmap_pinned()

A typical pattern with these functions would look like this.

	drm_gem_object bo;
	dma_buf_map map;

	init() {
		pin_unlocked(bo);
		vmap_pinned(bo, map);
	}

	worker() {
		begin_cpu_access()
		// access bo via map
		end_cpu_access()
	}

	fini() {
		vunmap_pinned(bo, map);
		unpin_unlocked(bo);
	}

	init()
	while (...) {
		worker()
	}
	fini()

Is that reasonable for media drivers?

Best regards
Thomas


> 
> Cheers,
> Christian.
> 
>>
>> That's what I meant with that this approach here is very sprawling :-/
>> -Daniel
>>
>>>    */
>>>   int drm_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct dma_buf_map 
>>> *map)
>>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
>>> index 5e6daa1c982f..7c34cd5ec261 100644
>>> --- a/include/drm/drm_gem.h
>>> +++ b/include/drm/drm_gem.h
>>> @@ -137,7 +137,21 @@ struct drm_gem_object_funcs {
>>>        * Returns a virtual address for the buffer. Used by the
>>>        * drm_gem_dmabuf_vmap() helper.
>>>        *
>>> +     * Notes to implementors:
>>> +     *
>>> +     * - Implementations must expect pairs of @vmap and @vunmap to be
>>> +     *   called frequently and should optimize for this case.
>>> +     *
>>> +     * - Implemenations may expect the caller to hold the GEM object's
>>> +     *   reservation lock to protect against concurrent calls and 
>>> relocation
>>> +     *   of the GEM object.
>>> +     *
>>> +     * - Implementations may provide additional guarantees (e.g., 
>>> working
>>> +     *   without holding the reservation lock).
>>> +     *
>>>        * This callback is optional.
>>> +     *
>>> +     * See also drm_gem_dmabuf_vmap()
>>>        */
>>>       int (*vmap)(struct drm_gem_object *obj, struct dma_buf_map *map);
>>> @@ -148,6 +162,8 @@ struct drm_gem_object_funcs {
>>>        * drm_gem_dmabuf_vunmap() helper.
>>>        *
>>>        * This callback is optional.
>>> +     *
>>> +     * See also @vmap.
>>>        */
>>>       void (*vunmap)(struct drm_gem_object *obj, struct dma_buf_map 
>>> *map);
>>> -- 
>>> 2.29.2
>>>
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 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: [PATCH 1/8] drm/gem: Write down some rules for vmap usage

[Daniel Vetter]

On Tue, Dec 1, 2020 at 9:32 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 30.11.20 um 16:33 schrieb Christian König:
> > Am 30.11.20 um 16:30 schrieb Daniel Vetter:
> >> On Mon, Nov 30, 2020 at 01:04:26PM +0100, Thomas Zimmermann wrote:
> >>> Mapping a GEM object's buffer into kernel address space prevents the
> >>> buffer from being evicted from VRAM, which in turn may result in
> >>> out-of-memory errors. It's therefore required to only vmap GEM BOs for
> >>> short periods of time; unless the GEM implementation provides additional
> >>> guarantees.
> >>>
> >>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >>> ---
> >>>   drivers/gpu/drm/drm_prime.c |  6 ++++++
> >>>   include/drm/drm_gem.h       | 16 ++++++++++++++++
> >>>   2 files changed, 22 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> >>> index 7db55fce35d8..9c9ece9833e0 100644
> >>> --- a/drivers/gpu/drm/drm_prime.c
> >>> +++ b/drivers/gpu/drm/drm_prime.c
> >>> @@ -669,6 +669,12 @@ EXPORT_SYMBOL(drm_gem_unmap_dma_buf);
> >>>    * callback. Calls into &drm_gem_object_funcs.vmap for device
> >>> specific handling.
> >>>    * The kernel virtual address is returned in map.
> >>>    *
> >>> + * To prevent the GEM object from being relocated, callers must hold
> >>> the GEM
> >>> + * object's reservation lock from when calling this function until
> >>> releasing the
> >>> + * mapping. Holding onto a mapping and the associated reservation
> >>> lock for an
> >>> + * unbound time may result in out-of-memory errors. Calls to
> >>> drm_gem_dmabuf_vmap()
> >>> + * should therefore be accompanied by a call to
> >>> drm_gem_dmabuf_vunmap().
> >>> + *
> >>>    * Returns 0 on success or a negative errno code otherwise.
> >> This is a dma-buf hook, which means just documenting the rules you'd like
> >> to have here isn't enough. We need to roll this out at the dma-buf level,
> >> and enforce it.
> >>
> >> Enforce it = assert_lock_held
> >>
> >> Roll out = review everyone. Because this goes through dma-buf it'll come
> >> back through shmem helpers (and other helpers and other subsystems) back
> >> to any driver using vmap for gpu buffers. This includes the media
> >> subsystem, and the media subsystem definitely doesn't cope with just
> >> temporarily mapping buffers. So there we need to pin them, which I think
> >> means we'll need 2 version of dma_buf_vmap - one that's temporary and
> >> requires we hold dma_resv lock, the other requires that the buffer is
> >> pinned.
> >
> > OR start to proper use the dma_buf_pin/dma_buf_unpin functions which I
> > added to cover this use case as well.
>
> While I generally agree, here are some thoughts:
>
> I found all generic pin functions useless, because they don't allow for
> specifying where to pin. With fbdev emulation, this means that console
> buffers might never make it to VRAM for scanout. If anything, the policy
> should be that pin always pins in HW-accessible memory.
>
> Pin has quite a bit of overhead (more locking, buffer movement), so it
> should be the second choice after regular vmap. To make both work
> together, pin probably relies on holding the reservation lock internally.
>
> Therefore I think we still would want some additional helpers, such as:
>
>    pin_unlocked(), which acquires the resv lock, calls regular pin and
> then drops the resv lock. Same for unpin_unlocked()
>
>    vmap_pinned(), which enforces that the buffer has been pinned and
> then calls regalar vmap. Same for vunmap_pinned()
>
> A typical pattern with these functions would look like this.
>
>         drm_gem_object bo;
>         dma_buf_map map;
>
>         init() {
>                 pin_unlocked(bo);
>                 vmap_pinned(bo, map);
>         }
>
>         worker() {
>                 begin_cpu_access()
>                 // access bo via map
>                 end_cpu_access()
>         }
>
>         fini() {
>                 vunmap_pinned(bo, map);
>                 unpin_unlocked(bo);
>         }
>
>         init()
>         while (...) {
>                 worker()
>         }
>         fini()
>
> Is that reasonable for media drivers?

So media drivers go through dma-buf, which means we always pin into
system memory. Which I guess for vram-only display drivers makes no
sense and should be rejected, but we still need somewhat consistent
rules.

The other thing is that if you do a dma_buf_attach without dynamic
mode, dma-buf will pin things for you already. So in many cases it
could be that we don't need a separate pin (but since the pin is in
the exporter, not dma-buf layer, we can't check for that). I'm also
not seeing why existing users need to split up their dma_buf_vmap into
a pin + vmap, they don't need them separately.

I think we could use what we've done for dynamic dma-buf attachment
(which also change locking rules) and just have new functions for the
new way (i.e. short term vmap protected by dma_resv lock. Maybe call
these dma_buf_vmap_local, in the spirit of the new kmap_local which
are currently under discussion. I think _local suffix is better, for
otherwise people might do something silly like

    dma_resv_lock();
    dma_buf_vmap_locked();
    dma_resv_unlock();

    /* actual access maybe even in some other thread */

   dma_buf_resv_lock();
   dma_buf_vunmap_unlocked();
   dma_resv_unlock();

_local suffix is better at telling that the resulting pointer has very
limited use (essentially just local to the calling context, if you
don't change any locking or anything).

I think encouraging importers to call dma_buf_pin/unpin isn't a good
idea. Yes dynamic ones need it, but maybe we should check for that
somehow in the exporterd interface (atm only amdgpu is using it).
-Daniel





> Best regards
> Thomas
>
>
> >
> > Cheers,
> > Christian.
> >
> >>
> >> That's what I meant with that this approach here is very sprawling :-/
> >> -Daniel
> >>
> >>>    */
> >>>   int drm_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct dma_buf_map
> >>> *map)
> >>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> >>> index 5e6daa1c982f..7c34cd5ec261 100644
> >>> --- a/include/drm/drm_gem.h
> >>> +++ b/include/drm/drm_gem.h
> >>> @@ -137,7 +137,21 @@ struct drm_gem_object_funcs {
> >>>        * Returns a virtual address for the buffer. Used by the
> >>>        * drm_gem_dmabuf_vmap() helper.
> >>>        *
> >>> +     * Notes to implementors:
> >>> +     *
> >>> +     * - Implementations must expect pairs of @vmap and @vunmap to be
> >>> +     *   called frequently and should optimize for this case.
> >>> +     *
> >>> +     * - Implemenations may expect the caller to hold the GEM object's
> >>> +     *   reservation lock to protect against concurrent calls and
> >>> relocation
> >>> +     *   of the GEM object.
> >>> +     *
> >>> +     * - Implementations may provide additional guarantees (e.g.,
> >>> working
> >>> +     *   without holding the reservation lock).
> >>> +     *
> >>>        * This callback is optional.
> >>> +     *
> >>> +     * See also drm_gem_dmabuf_vmap()
> >>>        */
> >>>       int (*vmap)(struct drm_gem_object *obj, struct dma_buf_map *map);
> >>> @@ -148,6 +162,8 @@ struct drm_gem_object_funcs {
> >>>        * drm_gem_dmabuf_vunmap() helper.
> >>>        *
> >>>        * This callback is optional.
> >>> +     *
> >>> +     * See also @vmap.
> >>>        */
> >>>       void (*vunmap)(struct drm_gem_object *obj, struct dma_buf_map
> >>> *map);
> >>> --
> >>> 2.29.2
> >>>
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] drm/gem: Write down some rules for vmap usage

[Christian König]

Am 01.12.20 um 09:32 schrieb Thomas Zimmermann:
> Hi
>
> Am 30.11.20 um 16:33 schrieb Christian König:
>> Am 30.11.20 um 16:30 schrieb Daniel Vetter:
>>> On Mon, Nov 30, 2020 at 01:04:26PM +0100, Thomas Zimmermann wrote:
>>>> Mapping a GEM object's buffer into kernel address space prevents the
>>>> buffer from being evicted from VRAM, which in turn may result in
>>>> out-of-memory errors. It's therefore required to only vmap GEM BOs for
>>>> short periods of time; unless the GEM implementation provides 
>>>> additional
>>>> guarantees.
>>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> ---
>>>>   drivers/gpu/drm/drm_prime.c |  6 ++++++
>>>>   include/drm/drm_gem.h       | 16 ++++++++++++++++
>>>>   2 files changed, 22 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>>>> index 7db55fce35d8..9c9ece9833e0 100644
>>>> --- a/drivers/gpu/drm/drm_prime.c
>>>> +++ b/drivers/gpu/drm/drm_prime.c
>>>> @@ -669,6 +669,12 @@ EXPORT_SYMBOL(drm_gem_unmap_dma_buf);
>>>>    * callback. Calls into &drm_gem_object_funcs.vmap for device 
>>>> specific handling.
>>>>    * The kernel virtual address is returned in map.
>>>>    *
>>>> + * To prevent the GEM object from being relocated, callers must 
>>>> hold the GEM
>>>> + * object's reservation lock from when calling this function until 
>>>> releasing the
>>>> + * mapping. Holding onto a mapping and the associated reservation 
>>>> lock for an
>>>> + * unbound time may result in out-of-memory errors. Calls to 
>>>> drm_gem_dmabuf_vmap()
>>>> + * should therefore be accompanied by a call to 
>>>> drm_gem_dmabuf_vunmap().
>>>> + *
>>>>    * Returns 0 on success or a negative errno code otherwise.
>>> This is a dma-buf hook, which means just documenting the rules you'd 
>>> like
>>> to have here isn't enough. We need to roll this out at the dma-buf 
>>> level,
>>> and enforce it.
>>>
>>> Enforce it = assert_lock_held
>>>
>>> Roll out = review everyone. Because this goes through dma-buf it'll 
>>> come
>>> back through shmem helpers (and other helpers and other subsystems) 
>>> back
>>> to any driver using vmap for gpu buffers. This includes the media
>>> subsystem, and the media subsystem definitely doesn't cope with just
>>> temporarily mapping buffers. So there we need to pin them, which I 
>>> think
>>> means we'll need 2 version of dma_buf_vmap - one that's temporary and
>>> requires we hold dma_resv lock, the other requires that the buffer is
>>> pinned.
>>
>> OR start to proper use the dma_buf_pin/dma_buf_unpin functions which 
>> I added to cover this use case as well.
>
> While I generally agree, here are some thoughts:
>
> I found all generic pin functions useless, because they don't allow 
> for specifying where to pin. With fbdev emulation, this means that 
> console buffers might never make it to VRAM for scanout. If anything, 
> the policy should be that pin always pins in HW-accessible memory.

Yes, completely agree. The major missing part here are some kind of 
usage flags what we want to do with the buffer.

>
> Pin has quite a bit of overhead (more locking, buffer movement), so it 
> should be the second choice after regular vmap. To make both work 
> together, pin probably relies on holding the reservation lock internally.

There is a dma_resv_assert_held() at the beginning of those two functions.

>
> Therefore I think we still would want some additional helpers, such as:
>
>   pin_unlocked(), which acquires the resv lock, calls regular pin and 
> then drops the resv lock. Same for unpin_unlocked()
>
>   vmap_pinned(), which enforces that the buffer has been pinned and 
> then calls regalar vmap. Same for vunmap_pinned()

I would rather open code that in each driver, hiding the locking logic 
in some midlayer is usually not a good idea.

Regards,
Christian.

>
> A typical pattern with these functions would look like this.
>
>     drm_gem_object bo;
>     dma_buf_map map;
>
>     init() {
>         pin_unlocked(bo);
>         vmap_pinned(bo, map);
>     }
>
>     worker() {
>         begin_cpu_access()
>         // access bo via map
>         end_cpu_access()
>     }
>
>     fini() {
>         vunmap_pinned(bo, map);
>         unpin_unlocked(bo);
>     }
>
>     init()
>     while (...) {
>         worker()
>     }
>     fini()
>
> Is that reasonable for media drivers?
>
> Best regards
> Thomas
>
>
>>
>> Cheers,
>> Christian.
>>
>>>
>>> That's what I meant with that this approach here is very sprawling :-/
>>> -Daniel

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

Re: [PATCH 1/8] drm/gem: Write down some rules for vmap usage

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 5958 bytes --]

Hi

Am 01.12.20 um 10:13 schrieb Christian König:
> Am 01.12.20 um 09:32 schrieb Thomas Zimmermann:
>> Hi
>>
>> Am 30.11.20 um 16:33 schrieb Christian König:
>>> Am 30.11.20 um 16:30 schrieb Daniel Vetter:
>>>> On Mon, Nov 30, 2020 at 01:04:26PM +0100, Thomas Zimmermann wrote:
>>>>> Mapping a GEM object's buffer into kernel address space prevents the
>>>>> buffer from being evicted from VRAM, which in turn may result in
>>>>> out-of-memory errors. It's therefore required to only vmap GEM BOs for
>>>>> short periods of time; unless the GEM implementation provides 
>>>>> additional
>>>>> guarantees.
>>>>>
>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>> ---
>>>>>   drivers/gpu/drm/drm_prime.c |  6 ++++++
>>>>>   include/drm/drm_gem.h       | 16 ++++++++++++++++
>>>>>   2 files changed, 22 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>>>>> index 7db55fce35d8..9c9ece9833e0 100644
>>>>> --- a/drivers/gpu/drm/drm_prime.c
>>>>> +++ b/drivers/gpu/drm/drm_prime.c
>>>>> @@ -669,6 +669,12 @@ EXPORT_SYMBOL(drm_gem_unmap_dma_buf);
>>>>>    * callback. Calls into &drm_gem_object_funcs.vmap for device 
>>>>> specific handling.
>>>>>    * The kernel virtual address is returned in map.
>>>>>    *
>>>>> + * To prevent the GEM object from being relocated, callers must 
>>>>> hold the GEM
>>>>> + * object's reservation lock from when calling this function until 
>>>>> releasing the
>>>>> + * mapping. Holding onto a mapping and the associated reservation 
>>>>> lock for an
>>>>> + * unbound time may result in out-of-memory errors. Calls to 
>>>>> drm_gem_dmabuf_vmap()
>>>>> + * should therefore be accompanied by a call to 
>>>>> drm_gem_dmabuf_vunmap().
>>>>> + *
>>>>>    * Returns 0 on success or a negative errno code otherwise.
>>>> This is a dma-buf hook, which means just documenting the rules you'd 
>>>> like
>>>> to have here isn't enough. We need to roll this out at the dma-buf 
>>>> level,
>>>> and enforce it.
>>>>
>>>> Enforce it = assert_lock_held
>>>>
>>>> Roll out = review everyone. Because this goes through dma-buf it'll 
>>>> come
>>>> back through shmem helpers (and other helpers and other subsystems) 
>>>> back
>>>> to any driver using vmap for gpu buffers. This includes the media
>>>> subsystem, and the media subsystem definitely doesn't cope with just
>>>> temporarily mapping buffers. So there we need to pin them, which I 
>>>> think
>>>> means we'll need 2 version of dma_buf_vmap - one that's temporary and
>>>> requires we hold dma_resv lock, the other requires that the buffer is
>>>> pinned.
>>>
>>> OR start to proper use the dma_buf_pin/dma_buf_unpin functions which 
>>> I added to cover this use case as well.
>>
>> While I generally agree, here are some thoughts:
>>
>> I found all generic pin functions useless, because they don't allow 
>> for specifying where to pin. With fbdev emulation, this means that 
>> console buffers might never make it to VRAM for scanout. If anything, 
>> the policy should be that pin always pins in HW-accessible memory.
> 
> Yes, completely agree. The major missing part here are some kind of 
> usage flags what we want to do with the buffer.

Not sure, but wasn't that not wanted by someone? I don't really remember.

Given Daniel's reply about pin, it really feels like we have 
contradicting policies for this interface.

> 
>>
>> Pin has quite a bit of overhead (more locking, buffer movement), so it 
>> should be the second choice after regular vmap. To make both work 
>> together, pin probably relies on holding the reservation lock internally.
> 
> There is a dma_resv_assert_held() at the beginning of those two functions.
> 
>>
>> Therefore I think we still would want some additional helpers, such as:
>>
>>   pin_unlocked(), which acquires the resv lock, calls regular pin and 
>> then drops the resv lock. Same for unpin_unlocked()
>>
>>   vmap_pinned(), which enforces that the buffer has been pinned and 
>> then calls regalar vmap. Same for vunmap_pinned()
> 
> I would rather open code that in each driver, hiding the locking logic 
> in some midlayer is usually not a good idea.

These helpers are less about hiding logic and more about making drivers 
do the right thing. The idea behind pin_unlocked() is that it drops the 
reservation lock immediately before returning. I suspect that some 
driver might not do that with an open-coded version. And vmap_pinned() 
does check for the BO to be pinned. Regular vmap would assert for the 
reservation lock instead.

Best regards
Thomas

> 
> Regards,
> Christian.
> 
>>
>> A typical pattern with these functions would look like this.
>>
>>     drm_gem_object bo;
>>     dma_buf_map map;
>>
>>     init() {
>>         pin_unlocked(bo);
>>         vmap_pinned(bo, map);
>>     }
>>
>>     worker() {
>>         begin_cpu_access()
>>         // access bo via map
>>         end_cpu_access()
>>     }
>>
>>     fini() {
>>         vunmap_pinned(bo, map);
>>         unpin_unlocked(bo);
>>     }
>>
>>     init()
>>     while (...) {
>>         worker()
>>     }
>>     fini()
>>
>> Is that reasonable for media drivers?
>>
>> Best regards
>> Thomas
>>
>>
>>>
>>> Cheers,
>>> Christian.
>>>
>>>>
>>>> That's what I meant with that this approach here is very sprawling :-/
>>>> -Daniel
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 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: [PATCH 1/8] drm/gem: Write down some rules for vmap usage

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 9093 bytes --]

Hi

Am 01.12.20 um 10:10 schrieb Daniel Vetter:
> On Tue, Dec 1, 2020 at 9:32 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> Hi
>>
>> Am 30.11.20 um 16:33 schrieb Christian König:
>>> Am 30.11.20 um 16:30 schrieb Daniel Vetter:
>>>> On Mon, Nov 30, 2020 at 01:04:26PM +0100, Thomas Zimmermann wrote:
>>>>> Mapping a GEM object's buffer into kernel address space prevents the
>>>>> buffer from being evicted from VRAM, which in turn may result in
>>>>> out-of-memory errors. It's therefore required to only vmap GEM BOs for
>>>>> short periods of time; unless the GEM implementation provides additional
>>>>> guarantees.
>>>>>
>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>> ---
>>>>>    drivers/gpu/drm/drm_prime.c |  6 ++++++
>>>>>    include/drm/drm_gem.h       | 16 ++++++++++++++++
>>>>>    2 files changed, 22 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>>>>> index 7db55fce35d8..9c9ece9833e0 100644
>>>>> --- a/drivers/gpu/drm/drm_prime.c
>>>>> +++ b/drivers/gpu/drm/drm_prime.c
>>>>> @@ -669,6 +669,12 @@ EXPORT_SYMBOL(drm_gem_unmap_dma_buf);
>>>>>     * callback. Calls into &drm_gem_object_funcs.vmap for device
>>>>> specific handling.
>>>>>     * The kernel virtual address is returned in map.
>>>>>     *
>>>>> + * To prevent the GEM object from being relocated, callers must hold
>>>>> the GEM
>>>>> + * object's reservation lock from when calling this function until
>>>>> releasing the
>>>>> + * mapping. Holding onto a mapping and the associated reservation
>>>>> lock for an
>>>>> + * unbound time may result in out-of-memory errors. Calls to
>>>>> drm_gem_dmabuf_vmap()
>>>>> + * should therefore be accompanied by a call to
>>>>> drm_gem_dmabuf_vunmap().
>>>>> + *
>>>>>     * Returns 0 on success or a negative errno code otherwise.
>>>> This is a dma-buf hook, which means just documenting the rules you'd like
>>>> to have here isn't enough. We need to roll this out at the dma-buf level,
>>>> and enforce it.
>>>>
>>>> Enforce it = assert_lock_held
>>>>
>>>> Roll out = review everyone. Because this goes through dma-buf it'll come
>>>> back through shmem helpers (and other helpers and other subsystems) back
>>>> to any driver using vmap for gpu buffers. This includes the media
>>>> subsystem, and the media subsystem definitely doesn't cope with just
>>>> temporarily mapping buffers. So there we need to pin them, which I think
>>>> means we'll need 2 version of dma_buf_vmap - one that's temporary and
>>>> requires we hold dma_resv lock, the other requires that the buffer is
>>>> pinned.
>>>
>>> OR start to proper use the dma_buf_pin/dma_buf_unpin functions which I
>>> added to cover this use case as well.
>>
>> While I generally agree, here are some thoughts:
>>
>> I found all generic pin functions useless, because they don't allow for
>> specifying where to pin. With fbdev emulation, this means that console
>> buffers might never make it to VRAM for scanout. If anything, the policy
>> should be that pin always pins in HW-accessible memory.
>>
>> Pin has quite a bit of overhead (more locking, buffer movement), so it
>> should be the second choice after regular vmap. To make both work
>> together, pin probably relies on holding the reservation lock internally.
>>
>> Therefore I think we still would want some additional helpers, such as:
>>
>>     pin_unlocked(), which acquires the resv lock, calls regular pin and
>> then drops the resv lock. Same for unpin_unlocked()
>>
>>     vmap_pinned(), which enforces that the buffer has been pinned and
>> then calls regalar vmap. Same for vunmap_pinned()
>>
>> A typical pattern with these functions would look like this.
>>
>>          drm_gem_object bo;
>>          dma_buf_map map;
>>
>>          init() {
>>                  pin_unlocked(bo);
>>                  vmap_pinned(bo, map);
>>          }
>>
>>          worker() {
>>                  begin_cpu_access()
>>                  // access bo via map
>>                  end_cpu_access()
>>          }
>>
>>          fini() {
>>                  vunmap_pinned(bo, map);
>>                  unpin_unlocked(bo);
>>          }
>>
>>          init()
>>          while (...) {
>>                  worker()
>>          }
>>          fini()
>>
>> Is that reasonable for media drivers?
> 
> So media drivers go through dma-buf, which means we always pin into
> system memory. Which I guess for vram-only display drivers makes no
> sense and should be rejected, but we still need somewhat consistent
> rules.
> 
> The other thing is that if you do a dma_buf_attach without dynamic
> mode, dma-buf will pin things for you already. So in many cases it

Do you have a pointer to code that illustrates this well?

> could be that we don't need a separate pin (but since the pin is in
> the exporter, not dma-buf layer, we can't check for that). I'm also
> not seeing why existing users need to split up their dma_buf_vmap into
> a pin + vmap, they don't need them separately.

It's two different operations, with pin having some possible overhead 
and failure conditions. And during the last discussion, pin was say to 
be for userspace-managed buffers. Kernel code should hold the 
reservation lock.

For my POV, the current interfaces have no clear policy or semantics. 
Looking through the different GEM implementations, each one seems to 
have its own interpretation.

> 
> I think we could use what we've done for dynamic dma-buf attachment
> (which also change locking rules) and just have new functions for the
> new way (i.e. short term vmap protected by dma_resv lock. Maybe call
> these dma_buf_vmap_local, in the spirit of the new kmap_local which
> are currently under discussion. I think _local suffix is better, for
> otherwise people might do something silly like
> 
>      dma_resv_lock();
>      dma_buf_vmap_locked();
>      dma_resv_unlock();
> 
>      /* actual access maybe even in some other thread */
> 
>     dma_buf_resv_lock();
>     dma_buf_vunmap_unlocked();
>     dma_resv_unlock();
> 
> _local suffix is better at telling that the resulting pointer has very
> limited use (essentially just local to the calling context, if you
> don't change any locking or anything).

_local sounds good.

Best regards
Thomas

> 
> I think encouraging importers to call dma_buf_pin/unpin isn't a good
> idea. Yes dynamic ones need it, but maybe we should check for that
> somehow in the exporterd interface (atm only amdgpu is using it).
> -Daniel
> 
> 
> 
> 
> 
>> Best regards
>> Thomas
>>
>>
>>>
>>> Cheers,
>>> Christian.
>>>
>>>>
>>>> That's what I meant with that this approach here is very sprawling :-/
>>>> -Daniel
>>>>
>>>>>     */
>>>>>    int drm_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct dma_buf_map
>>>>> *map)
>>>>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
>>>>> index 5e6daa1c982f..7c34cd5ec261 100644
>>>>> --- a/include/drm/drm_gem.h
>>>>> +++ b/include/drm/drm_gem.h
>>>>> @@ -137,7 +137,21 @@ struct drm_gem_object_funcs {
>>>>>         * Returns a virtual address for the buffer. Used by the
>>>>>         * drm_gem_dmabuf_vmap() helper.
>>>>>         *
>>>>> +     * Notes to implementors:
>>>>> +     *
>>>>> +     * - Implementations must expect pairs of @vmap and @vunmap to be
>>>>> +     *   called frequently and should optimize for this case.
>>>>> +     *
>>>>> +     * - Implemenations may expect the caller to hold the GEM object's
>>>>> +     *   reservation lock to protect against concurrent calls and
>>>>> relocation
>>>>> +     *   of the GEM object.
>>>>> +     *
>>>>> +     * - Implementations may provide additional guarantees (e.g.,
>>>>> working
>>>>> +     *   without holding the reservation lock).
>>>>> +     *
>>>>>         * This callback is optional.
>>>>> +     *
>>>>> +     * See also drm_gem_dmabuf_vmap()
>>>>>         */
>>>>>        int (*vmap)(struct drm_gem_object *obj, struct dma_buf_map *map);
>>>>> @@ -148,6 +162,8 @@ struct drm_gem_object_funcs {
>>>>>         * drm_gem_dmabuf_vunmap() helper.
>>>>>         *
>>>>>         * This callback is optional.
>>>>> +     *
>>>>> +     * See also @vmap.
>>>>>         */
>>>>>        void (*vunmap)(struct drm_gem_object *obj, struct dma_buf_map
>>>>> *map);
>>>>> --
>>>>> 2.29.2
>>>>>
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Felix Imendörffer
>>
> 
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 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: [PATCH 1/8] drm/gem: Write down some rules for vmap usage

[Daniel Vetter]

On Tue, Dec 1, 2020 at 10:40 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 01.12.20 um 10:10 schrieb Daniel Vetter:
> > On Tue, Dec 1, 2020 at 9:32 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >>
> >> Hi
> >>
> >> Am 30.11.20 um 16:33 schrieb Christian König:
> >>> Am 30.11.20 um 16:30 schrieb Daniel Vetter:
> >>>> On Mon, Nov 30, 2020 at 01:04:26PM +0100, Thomas Zimmermann wrote:
> >>>>> Mapping a GEM object's buffer into kernel address space prevents the
> >>>>> buffer from being evicted from VRAM, which in turn may result in
> >>>>> out-of-memory errors. It's therefore required to only vmap GEM BOs for
> >>>>> short periods of time; unless the GEM implementation provides additional
> >>>>> guarantees.
> >>>>>
> >>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >>>>> ---
> >>>>>    drivers/gpu/drm/drm_prime.c |  6 ++++++
> >>>>>    include/drm/drm_gem.h       | 16 ++++++++++++++++
> >>>>>    2 files changed, 22 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> >>>>> index 7db55fce35d8..9c9ece9833e0 100644
> >>>>> --- a/drivers/gpu/drm/drm_prime.c
> >>>>> +++ b/drivers/gpu/drm/drm_prime.c
> >>>>> @@ -669,6 +669,12 @@ EXPORT_SYMBOL(drm_gem_unmap_dma_buf);
> >>>>>     * callback. Calls into &drm_gem_object_funcs.vmap for device
> >>>>> specific handling.
> >>>>>     * The kernel virtual address is returned in map.
> >>>>>     *
> >>>>> + * To prevent the GEM object from being relocated, callers must hold
> >>>>> the GEM
> >>>>> + * object's reservation lock from when calling this function until
> >>>>> releasing the
> >>>>> + * mapping. Holding onto a mapping and the associated reservation
> >>>>> lock for an
> >>>>> + * unbound time may result in out-of-memory errors. Calls to
> >>>>> drm_gem_dmabuf_vmap()
> >>>>> + * should therefore be accompanied by a call to
> >>>>> drm_gem_dmabuf_vunmap().
> >>>>> + *
> >>>>>     * Returns 0 on success or a negative errno code otherwise.
> >>>> This is a dma-buf hook, which means just documenting the rules you'd like
> >>>> to have here isn't enough. We need to roll this out at the dma-buf level,
> >>>> and enforce it.
> >>>>
> >>>> Enforce it = assert_lock_held
> >>>>
> >>>> Roll out = review everyone. Because this goes through dma-buf it'll come
> >>>> back through shmem helpers (and other helpers and other subsystems) back
> >>>> to any driver using vmap for gpu buffers. This includes the media
> >>>> subsystem, and the media subsystem definitely doesn't cope with just
> >>>> temporarily mapping buffers. So there we need to pin them, which I think
> >>>> means we'll need 2 version of dma_buf_vmap - one that's temporary and
> >>>> requires we hold dma_resv lock, the other requires that the buffer is
> >>>> pinned.
> >>>
> >>> OR start to proper use the dma_buf_pin/dma_buf_unpin functions which I
> >>> added to cover this use case as well.
> >>
> >> While I generally agree, here are some thoughts:
> >>
> >> I found all generic pin functions useless, because they don't allow for
> >> specifying where to pin. With fbdev emulation, this means that console
> >> buffers might never make it to VRAM for scanout. If anything, the policy
> >> should be that pin always pins in HW-accessible memory.
> >>
> >> Pin has quite a bit of overhead (more locking, buffer movement), so it
> >> should be the second choice after regular vmap. To make both work
> >> together, pin probably relies on holding the reservation lock internally.
> >>
> >> Therefore I think we still would want some additional helpers, such as:
> >>
> >>     pin_unlocked(), which acquires the resv lock, calls regular pin and
> >> then drops the resv lock. Same for unpin_unlocked()
> >>
> >>     vmap_pinned(), which enforces that the buffer has been pinned and
> >> then calls regalar vmap. Same for vunmap_pinned()
> >>
> >> A typical pattern with these functions would look like this.
> >>
> >>          drm_gem_object bo;
> >>          dma_buf_map map;
> >>
> >>          init() {
> >>                  pin_unlocked(bo);
> >>                  vmap_pinned(bo, map);
> >>          }
> >>
> >>          worker() {
> >>                  begin_cpu_access()
> >>                  // access bo via map
> >>                  end_cpu_access()
> >>          }
> >>
> >>          fini() {
> >>                  vunmap_pinned(bo, map);
> >>                  unpin_unlocked(bo);
> >>          }
> >>
> >>          init()
> >>          while (...) {
> >>                  worker()
> >>          }
> >>          fini()
> >>
> >> Is that reasonable for media drivers?
> >
> > So media drivers go through dma-buf, which means we always pin into
> > system memory. Which I guess for vram-only display drivers makes no
> > sense and should be rejected, but we still need somewhat consistent
> > rules.
> >
> > The other thing is that if you do a dma_buf_attach without dynamic
> > mode, dma-buf will pin things for you already. So in many cases it
>
> Do you have a pointer to code that illustrates this well?
>
> > could be that we don't need a separate pin (but since the pin is in
> > the exporter, not dma-buf layer, we can't check for that). I'm also
> > not seeing why existing users need to split up their dma_buf_vmap into
> > a pin + vmap, they don't need them separately.
>
> It's two different operations, with pin having some possible overhead
> and failure conditions. And during the last discussion, pin was say to
> be for userspace-managed buffers. Kernel code should hold the
> reservation lock.
>
> For my POV, the current interfaces have no clear policy or semantics.
> Looking through the different GEM implementations, each one seems to
> have its own interpretation.

Yup, that's the problem really. In the past we've had vmap exclusively
for permanently pinned access, with no locking requirements. Now we're
trying to make this more dynamic, but in a somewhat ad-hoc fashion
(generic fbdev emulation charged ahead with making the fbdev
framebuffer evictable), and now it breaks at every seam. Adding more
ad-hoc semantics on top doesn't seem like a good idea.

That's why I think we should have 2 different interfaces:
- dma_buf_vmap, the one we currently have. Permanently pins the
buffer, mapping survives, no locking required.
- dma_buf_vmap_local, the new interface, the one that generic fbdev
should have used (but oh well mistakes happen), requires
dma_resv_lock, the mapping is only local to the caller

Trying to shovel both semantics into one interface, depending upon
which implementation we have backing the buffer, doesn't work indeed.

Also on the pin topic, I think neither interface should require
callers to explicitly pin anything. For existing users it should
happen automatically behind the scenes imo, that's what they're
expecting.
-Daniel


> > I think we could use what we've done for dynamic dma-buf attachment
> > (which also change locking rules) and just have new functions for the
> > new way (i.e. short term vmap protected by dma_resv lock. Maybe call
> > these dma_buf_vmap_local, in the spirit of the new kmap_local which
> > are currently under discussion. I think _local suffix is better, for
> > otherwise people might do something silly like
> >
> >      dma_resv_lock();
> >      dma_buf_vmap_locked();
> >      dma_resv_unlock();
> >
> >      /* actual access maybe even in some other thread */
> >
> >     dma_buf_resv_lock();
> >     dma_buf_vunmap_unlocked();
> >     dma_resv_unlock();
> >
> > _local suffix is better at telling that the resulting pointer has very
> > limited use (essentially just local to the calling context, if you
> > don't change any locking or anything).
>
> _local sounds good.
>
> Best regards
> Thomas
>
> >
> > I think encouraging importers to call dma_buf_pin/unpin isn't a good
> > idea. Yes dynamic ones need it, but maybe we should check for that
> > somehow in the exporterd interface (atm only amdgpu is using it).
> > -Daniel
> >
> >
> >
> >
> >
> >> Best regards
> >> Thomas
> >>
> >>
> >>>
> >>> Cheers,
> >>> Christian.
> >>>
> >>>>
> >>>> That's what I meant with that this approach here is very sprawling :-/
> >>>> -Daniel
> >>>>
> >>>>>     */
> >>>>>    int drm_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct dma_buf_map
> >>>>> *map)
> >>>>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> >>>>> index 5e6daa1c982f..7c34cd5ec261 100644
> >>>>> --- a/include/drm/drm_gem.h
> >>>>> +++ b/include/drm/drm_gem.h
> >>>>> @@ -137,7 +137,21 @@ struct drm_gem_object_funcs {
> >>>>>         * Returns a virtual address for the buffer. Used by the
> >>>>>         * drm_gem_dmabuf_vmap() helper.
> >>>>>         *
> >>>>> +     * Notes to implementors:
> >>>>> +     *
> >>>>> +     * - Implementations must expect pairs of @vmap and @vunmap to be
> >>>>> +     *   called frequently and should optimize for this case.
> >>>>> +     *
> >>>>> +     * - Implemenations may expect the caller to hold the GEM object's
> >>>>> +     *   reservation lock to protect against concurrent calls and
> >>>>> relocation
> >>>>> +     *   of the GEM object.
> >>>>> +     *
> >>>>> +     * - Implementations may provide additional guarantees (e.g.,
> >>>>> working
> >>>>> +     *   without holding the reservation lock).
> >>>>> +     *
> >>>>>         * This callback is optional.
> >>>>> +     *
> >>>>> +     * See also drm_gem_dmabuf_vmap()
> >>>>>         */
> >>>>>        int (*vmap)(struct drm_gem_object *obj, struct dma_buf_map *map);
> >>>>> @@ -148,6 +162,8 @@ struct drm_gem_object_funcs {
> >>>>>         * drm_gem_dmabuf_vunmap() helper.
> >>>>>         *
> >>>>>         * This callback is optional.
> >>>>> +     *
> >>>>> +     * See also @vmap.
> >>>>>         */
> >>>>>        void (*vunmap)(struct drm_gem_object *obj, struct dma_buf_map
> >>>>> *map);
> >>>>> --
> >>>>> 2.29.2
> >>>>>
> >>>
> >>> _______________________________________________
> >>> dri-devel mailing list
> >>> dri-devel@lists.freedesktop.org
> >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>
> >> --
> >> Thomas Zimmermann
> >> Graphics Driver Developer
> >> SUSE Software Solutions Germany GmbH
> >> Maxfeldstr. 5, 90409 Nürnberg, Germany
> >> (HRB 36809, AG Nürnberg)
> >> Geschäftsführer: Felix Imendörffer
> >>
> >
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] drm/gem: Write down some rules for vmap usage

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 11804 bytes --]

Hi

Am 01.12.20 um 11:00 schrieb Daniel Vetter:
> On Tue, Dec 1, 2020 at 10:40 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> Hi
>>
>> Am 01.12.20 um 10:10 schrieb Daniel Vetter:
>>> On Tue, Dec 1, 2020 at 9:32 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>>
>>>> Hi
>>>>
>>>> Am 30.11.20 um 16:33 schrieb Christian König:
>>>>> Am 30.11.20 um 16:30 schrieb Daniel Vetter:
>>>>>> On Mon, Nov 30, 2020 at 01:04:26PM +0100, Thomas Zimmermann wrote:
>>>>>>> Mapping a GEM object's buffer into kernel address space prevents the
>>>>>>> buffer from being evicted from VRAM, which in turn may result in
>>>>>>> out-of-memory errors. It's therefore required to only vmap GEM BOs for
>>>>>>> short periods of time; unless the GEM implementation provides additional
>>>>>>> guarantees.
>>>>>>>
>>>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>>> ---
>>>>>>>     drivers/gpu/drm/drm_prime.c |  6 ++++++
>>>>>>>     include/drm/drm_gem.h       | 16 ++++++++++++++++
>>>>>>>     2 files changed, 22 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>>>>>>> index 7db55fce35d8..9c9ece9833e0 100644
>>>>>>> --- a/drivers/gpu/drm/drm_prime.c
>>>>>>> +++ b/drivers/gpu/drm/drm_prime.c
>>>>>>> @@ -669,6 +669,12 @@ EXPORT_SYMBOL(drm_gem_unmap_dma_buf);
>>>>>>>      * callback. Calls into &drm_gem_object_funcs.vmap for device
>>>>>>> specific handling.
>>>>>>>      * The kernel virtual address is returned in map.
>>>>>>>      *
>>>>>>> + * To prevent the GEM object from being relocated, callers must hold
>>>>>>> the GEM
>>>>>>> + * object's reservation lock from when calling this function until
>>>>>>> releasing the
>>>>>>> + * mapping. Holding onto a mapping and the associated reservation
>>>>>>> lock for an
>>>>>>> + * unbound time may result in out-of-memory errors. Calls to
>>>>>>> drm_gem_dmabuf_vmap()
>>>>>>> + * should therefore be accompanied by a call to
>>>>>>> drm_gem_dmabuf_vunmap().
>>>>>>> + *
>>>>>>>      * Returns 0 on success or a negative errno code otherwise.
>>>>>> This is a dma-buf hook, which means just documenting the rules you'd like
>>>>>> to have here isn't enough. We need to roll this out at the dma-buf level,
>>>>>> and enforce it.
>>>>>>
>>>>>> Enforce it = assert_lock_held
>>>>>>
>>>>>> Roll out = review everyone. Because this goes through dma-buf it'll come
>>>>>> back through shmem helpers (and other helpers and other subsystems) back
>>>>>> to any driver using vmap for gpu buffers. This includes the media
>>>>>> subsystem, and the media subsystem definitely doesn't cope with just
>>>>>> temporarily mapping buffers. So there we need to pin them, which I think
>>>>>> means we'll need 2 version of dma_buf_vmap - one that's temporary and
>>>>>> requires we hold dma_resv lock, the other requires that the buffer is
>>>>>> pinned.
>>>>>
>>>>> OR start to proper use the dma_buf_pin/dma_buf_unpin functions which I
>>>>> added to cover this use case as well.
>>>>
>>>> While I generally agree, here are some thoughts:
>>>>
>>>> I found all generic pin functions useless, because they don't allow for
>>>> specifying where to pin. With fbdev emulation, this means that console
>>>> buffers might never make it to VRAM for scanout. If anything, the policy
>>>> should be that pin always pins in HW-accessible memory.
>>>>
>>>> Pin has quite a bit of overhead (more locking, buffer movement), so it
>>>> should be the second choice after regular vmap. To make both work
>>>> together, pin probably relies on holding the reservation lock internally.
>>>>
>>>> Therefore I think we still would want some additional helpers, such as:
>>>>
>>>>      pin_unlocked(), which acquires the resv lock, calls regular pin and
>>>> then drops the resv lock. Same for unpin_unlocked()
>>>>
>>>>      vmap_pinned(), which enforces that the buffer has been pinned and
>>>> then calls regalar vmap. Same for vunmap_pinned()
>>>>
>>>> A typical pattern with these functions would look like this.
>>>>
>>>>           drm_gem_object bo;
>>>>           dma_buf_map map;
>>>>
>>>>           init() {
>>>>                   pin_unlocked(bo);
>>>>                   vmap_pinned(bo, map);
>>>>           }
>>>>
>>>>           worker() {
>>>>                   begin_cpu_access()
>>>>                   // access bo via map
>>>>                   end_cpu_access()
>>>>           }
>>>>
>>>>           fini() {
>>>>                   vunmap_pinned(bo, map);
>>>>                   unpin_unlocked(bo);
>>>>           }
>>>>
>>>>           init()
>>>>           while (...) {
>>>>                   worker()
>>>>           }
>>>>           fini()
>>>>
>>>> Is that reasonable for media drivers?
>>>
>>> So media drivers go through dma-buf, which means we always pin into
>>> system memory. Which I guess for vram-only display drivers makes no
>>> sense and should be rejected, but we still need somewhat consistent
>>> rules.
>>>
>>> The other thing is that if you do a dma_buf_attach without dynamic
>>> mode, dma-buf will pin things for you already. So in many cases it
>>
>> Do you have a pointer to code that illustrates this well?
>>
>>> could be that we don't need a separate pin (but since the pin is in
>>> the exporter, not dma-buf layer, we can't check for that). I'm also
>>> not seeing why existing users need to split up their dma_buf_vmap into
>>> a pin + vmap, they don't need them separately.
>>
>> It's two different operations, with pin having some possible overhead
>> and failure conditions. And during the last discussion, pin was say to
>> be for userspace-managed buffers. Kernel code should hold the
>> reservation lock.
>>
>> For my POV, the current interfaces have no clear policy or semantics.
>> Looking through the different GEM implementations, each one seems to
>> have its own interpretation.
> 
> Yup, that's the problem really. In the past we've had vmap exclusively
> for permanently pinned access, with no locking requirements. Now we're
> trying to make this more dynamic, but in a somewhat ad-hoc fashion
> (generic fbdev emulation charged ahead with making the fbdev
> framebuffer evictable), and now it breaks at every seam. Adding more
> ad-hoc semantics on top doesn't seem like a good idea.
> 
> That's why I think we should have 2 different interfaces:
> - dma_buf_vmap, the one we currently have. Permanently pins the
> buffer, mapping survives, no locking required.
> - dma_buf_vmap_local, the new interface, the one that generic fbdev
> should have used (but oh well mistakes happen), requires
> dma_resv_lock, the mapping is only local to the caller

In patch 6 of this series, there's ast cursor code that acquires two 
BO's reservation locks and vmaps them afterwards. That's probably how 
you intend to use dma_buf_vmap_local.

However, I think it's more logically to have a vmap callback that only 
does the actual vmap. This is all that exporters would have to implement.

And with that, build one helper that pins before vmap and one helper 
that gets the resv lock.

I know that it might require some work on exporting drivers. But with 
your proposal, we probably need another dma-buf callback just for 
vmap_local. (?) That seems even less appealing to me.

Best regards
Thomas

> 
> Trying to shovel both semantics into one interface, depending upon
> which implementation we have backing the buffer, doesn't work indeed.
> 
> Also on the pin topic, I think neither interface should require
> callers to explicitly pin anything. For existing users it should
> happen automatically behind the scenes imo, that's what they're
> expecting.
> -Daniel
> 
> 
>>> I think we could use what we've done for dynamic dma-buf attachment
>>> (which also change locking rules) and just have new functions for the
>>> new way (i.e. short term vmap protected by dma_resv lock. Maybe call
>>> these dma_buf_vmap_local, in the spirit of the new kmap_local which
>>> are currently under discussion. I think _local suffix is better, for
>>> otherwise people might do something silly like
>>>
>>>       dma_resv_lock();
>>>       dma_buf_vmap_locked();
>>>       dma_resv_unlock();
>>>
>>>       /* actual access maybe even in some other thread */
>>>
>>>      dma_buf_resv_lock();
>>>      dma_buf_vunmap_unlocked();
>>>      dma_resv_unlock();
>>>
>>> _local suffix is better at telling that the resulting pointer has very
>>> limited use (essentially just local to the calling context, if you
>>> don't change any locking or anything).
>>
>> _local sounds good.
>>
>> Best regards
>> Thomas
>>
>>>
>>> I think encouraging importers to call dma_buf_pin/unpin isn't a good
>>> idea. Yes dynamic ones need it, but maybe we should check for that
>>> somehow in the exporterd interface (atm only amdgpu is using it).
>>> -Daniel
>>>
>>>
>>>
>>>
>>>
>>>> Best regards
>>>> Thomas
>>>>
>>>>
>>>>>
>>>>> Cheers,
>>>>> Christian.
>>>>>
>>>>>>
>>>>>> That's what I meant with that this approach here is very sprawling :-/
>>>>>> -Daniel
>>>>>>
>>>>>>>      */
>>>>>>>     int drm_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct dma_buf_map
>>>>>>> *map)
>>>>>>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
>>>>>>> index 5e6daa1c982f..7c34cd5ec261 100644
>>>>>>> --- a/include/drm/drm_gem.h
>>>>>>> +++ b/include/drm/drm_gem.h
>>>>>>> @@ -137,7 +137,21 @@ struct drm_gem_object_funcs {
>>>>>>>          * Returns a virtual address for the buffer. Used by the
>>>>>>>          * drm_gem_dmabuf_vmap() helper.
>>>>>>>          *
>>>>>>> +     * Notes to implementors:
>>>>>>> +     *
>>>>>>> +     * - Implementations must expect pairs of @vmap and @vunmap to be
>>>>>>> +     *   called frequently and should optimize for this case.
>>>>>>> +     *
>>>>>>> +     * - Implemenations may expect the caller to hold the GEM object's
>>>>>>> +     *   reservation lock to protect against concurrent calls and
>>>>>>> relocation
>>>>>>> +     *   of the GEM object.
>>>>>>> +     *
>>>>>>> +     * - Implementations may provide additional guarantees (e.g.,
>>>>>>> working
>>>>>>> +     *   without holding the reservation lock).
>>>>>>> +     *
>>>>>>>          * This callback is optional.
>>>>>>> +     *
>>>>>>> +     * See also drm_gem_dmabuf_vmap()
>>>>>>>          */
>>>>>>>         int (*vmap)(struct drm_gem_object *obj, struct dma_buf_map *map);
>>>>>>> @@ -148,6 +162,8 @@ struct drm_gem_object_funcs {
>>>>>>>          * drm_gem_dmabuf_vunmap() helper.
>>>>>>>          *
>>>>>>>          * This callback is optional.
>>>>>>> +     *
>>>>>>> +     * See also @vmap.
>>>>>>>          */
>>>>>>>         void (*vunmap)(struct drm_gem_object *obj, struct dma_buf_map
>>>>>>> *map);
>>>>>>> --
>>>>>>> 2.29.2
>>>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> dri-devel mailing list
>>>>> dri-devel@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>
>>>> --
>>>> Thomas Zimmermann
>>>> Graphics Driver Developer
>>>> SUSE Software Solutions Germany GmbH
>>>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>>>> (HRB 36809, AG Nürnberg)
>>>> Geschäftsführer: Felix Imendörffer
>>>>
>>>
>>>
>>
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Felix Imendörffer
>>
> 
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 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: [PATCH 1/8] drm/gem: Write down some rules for vmap usage

[Christian König]

Am 01.12.20 um 11:27 schrieb Thomas Zimmermann:
> Hi
>
> Am 01.12.20 um 11:00 schrieb Daniel Vetter:
>> On Tue, Dec 1, 2020 at 10:40 AM Thomas Zimmermann 
>> <tzimmermann@suse.de> wrote:
>>>
>>> Hi
>>>
>>> Am 01.12.20 um 10:10 schrieb Daniel Vetter:
>>>> On Tue, Dec 1, 2020 at 9:32 AM Thomas Zimmermann 
>>>> <tzimmermann@suse.de> wrote:
>>>>>
>>>>> Hi
>>>>>
>>>>> Am 30.11.20 um 16:33 schrieb Christian König:
>>>>>> Am 30.11.20 um 16:30 schrieb Daniel Vetter:
>>>>>>> On Mon, Nov 30, 2020 at 01:04:26PM +0100, Thomas Zimmermann wrote:
>>>>>>>> Mapping a GEM object's buffer into kernel address space 
>>>>>>>> prevents the
>>>>>>>> buffer from being evicted from VRAM, which in turn may result in
>>>>>>>> out-of-memory errors. It's therefore required to only vmap GEM 
>>>>>>>> BOs for
>>>>>>>> short periods of time; unless the GEM implementation provides 
>>>>>>>> additional
>>>>>>>> guarantees.
>>>>>>>>
>>>>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>>>> ---
>>>>>>>>     drivers/gpu/drm/drm_prime.c |  6 ++++++
>>>>>>>>     include/drm/drm_gem.h       | 16 ++++++++++++++++
>>>>>>>>     2 files changed, 22 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/drm_prime.c 
>>>>>>>> b/drivers/gpu/drm/drm_prime.c
>>>>>>>> index 7db55fce35d8..9c9ece9833e0 100644
>>>>>>>> --- a/drivers/gpu/drm/drm_prime.c
>>>>>>>> +++ b/drivers/gpu/drm/drm_prime.c
>>>>>>>> @@ -669,6 +669,12 @@ EXPORT_SYMBOL(drm_gem_unmap_dma_buf);
>>>>>>>>      * callback. Calls into &drm_gem_object_funcs.vmap for device
>>>>>>>> specific handling.
>>>>>>>>      * The kernel virtual address is returned in map.
>>>>>>>>      *
>>>>>>>> + * To prevent the GEM object from being relocated, callers 
>>>>>>>> must hold
>>>>>>>> the GEM
>>>>>>>> + * object's reservation lock from when calling this function 
>>>>>>>> until
>>>>>>>> releasing the
>>>>>>>> + * mapping. Holding onto a mapping and the associated reservation
>>>>>>>> lock for an
>>>>>>>> + * unbound time may result in out-of-memory errors. Calls to
>>>>>>>> drm_gem_dmabuf_vmap()
>>>>>>>> + * should therefore be accompanied by a call to
>>>>>>>> drm_gem_dmabuf_vunmap().
>>>>>>>> + *
>>>>>>>>      * Returns 0 on success or a negative errno code otherwise.
>>>>>>> This is a dma-buf hook, which means just documenting the rules 
>>>>>>> you'd like
>>>>>>> to have here isn't enough. We need to roll this out at the 
>>>>>>> dma-buf level,
>>>>>>> and enforce it.
>>>>>>>
>>>>>>> Enforce it = assert_lock_held
>>>>>>>
>>>>>>> Roll out = review everyone. Because this goes through dma-buf 
>>>>>>> it'll come
>>>>>>> back through shmem helpers (and other helpers and other 
>>>>>>> subsystems) back
>>>>>>> to any driver using vmap for gpu buffers. This includes the media
>>>>>>> subsystem, and the media subsystem definitely doesn't cope with 
>>>>>>> just
>>>>>>> temporarily mapping buffers. So there we need to pin them, which 
>>>>>>> I think
>>>>>>> means we'll need 2 version of dma_buf_vmap - one that's 
>>>>>>> temporary and
>>>>>>> requires we hold dma_resv lock, the other requires that the 
>>>>>>> buffer is
>>>>>>> pinned.
>>>>>>
>>>>>> OR start to proper use the dma_buf_pin/dma_buf_unpin functions 
>>>>>> which I
>>>>>> added to cover this use case as well.
>>>>>
>>>>> While I generally agree, here are some thoughts:
>>>>>
>>>>> I found all generic pin functions useless, because they don't 
>>>>> allow for
>>>>> specifying where to pin. With fbdev emulation, this means that 
>>>>> console
>>>>> buffers might never make it to VRAM for scanout. If anything, the 
>>>>> policy
>>>>> should be that pin always pins in HW-accessible memory.
>>>>>
>>>>> Pin has quite a bit of overhead (more locking, buffer movement), 
>>>>> so it
>>>>> should be the second choice after regular vmap. To make both work
>>>>> together, pin probably relies on holding the reservation lock 
>>>>> internally.
>>>>>
>>>>> Therefore I think we still would want some additional helpers, 
>>>>> such as:
>>>>>
>>>>>      pin_unlocked(), which acquires the resv lock, calls regular 
>>>>> pin and
>>>>> then drops the resv lock. Same for unpin_unlocked()
>>>>>
>>>>>      vmap_pinned(), which enforces that the buffer has been pinned 
>>>>> and
>>>>> then calls regalar vmap. Same for vunmap_pinned()
>>>>>
>>>>> A typical pattern with these functions would look like this.
>>>>>
>>>>>           drm_gem_object bo;
>>>>>           dma_buf_map map;
>>>>>
>>>>>           init() {
>>>>>                   pin_unlocked(bo);
>>>>>                   vmap_pinned(bo, map);
>>>>>           }
>>>>>
>>>>>           worker() {
>>>>>                   begin_cpu_access()
>>>>>                   // access bo via map
>>>>>                   end_cpu_access()
>>>>>           }
>>>>>
>>>>>           fini() {
>>>>>                   vunmap_pinned(bo, map);
>>>>>                   unpin_unlocked(bo);
>>>>>           }
>>>>>
>>>>>           init()
>>>>>           while (...) {
>>>>>                   worker()
>>>>>           }
>>>>>           fini()
>>>>>
>>>>> Is that reasonable for media drivers?
>>>>
>>>> So media drivers go through dma-buf, which means we always pin into
>>>> system memory. Which I guess for vram-only display drivers makes no
>>>> sense and should be rejected, but we still need somewhat consistent
>>>> rules.
>>>>
>>>> The other thing is that if you do a dma_buf_attach without dynamic
>>>> mode, dma-buf will pin things for you already. So in many cases it
>>>
>>> Do you have a pointer to code that illustrates this well?
>>>
>>>> could be that we don't need a separate pin (but since the pin is in
>>>> the exporter, not dma-buf layer, we can't check for that). I'm also
>>>> not seeing why existing users need to split up their dma_buf_vmap into
>>>> a pin + vmap, they don't need them separately.
>>>
>>> It's two different operations, with pin having some possible overhead
>>> and failure conditions. And during the last discussion, pin was say to
>>> be for userspace-managed buffers. Kernel code should hold the
>>> reservation lock.
>>>
>>> For my POV, the current interfaces have no clear policy or semantics.
>>> Looking through the different GEM implementations, each one seems to
>>> have its own interpretation.
>>
>> Yup, that's the problem really. In the past we've had vmap exclusively
>> for permanently pinned access, with no locking requirements. Now we're
>> trying to make this more dynamic, but in a somewhat ad-hoc fashion
>> (generic fbdev emulation charged ahead with making the fbdev
>> framebuffer evictable), and now it breaks at every seam. Adding more
>> ad-hoc semantics on top doesn't seem like a good idea.
>>
>> That's why I think we should have 2 different interfaces:
>> - dma_buf_vmap, the one we currently have. Permanently pins the
>> buffer, mapping survives, no locking required.
>> - dma_buf_vmap_local, the new interface, the one that generic fbdev
>> should have used (but oh well mistakes happen), requires
>> dma_resv_lock, the mapping is only local to the caller
>
> In patch 6 of this series, there's ast cursor code that acquires two 
> BO's reservation locks and vmaps them afterwards. That's probably how 
> you intend to use dma_buf_vmap_local.
>
> However, I think it's more logically to have a vmap callback that only 
> does the actual vmap. This is all that exporters would have to implement.
>
> And with that, build one helper that pins before vmap and one helper 
> that gets the resv lock.

I don't think that this is will work nor is it a good approach.

See the ast cursor handling for example. You need to acquire two BOs 
here, not just one. And this can't be done cleanly with a single vmap call.

Regards,
Christian.

>
> I know that it might require some work on exporting drivers. But with 
> your proposal, we probably need another dma-buf callback just for 
> vmap_local. (?) That seems even less appealing to me.
>
> Best regards
> Thomas
>
>>
>> Trying to shovel both semantics into one interface, depending upon
>> which implementation we have backing the buffer, doesn't work indeed.
>>
>> Also on the pin topic, I think neither interface should require
>> callers to explicitly pin anything. For existing users it should
>> happen automatically behind the scenes imo, that's what they're
>> expecting.
>> -Daniel
>>
>>
>>>> I think we could use what we've done for dynamic dma-buf attachment
>>>> (which also change locking rules) and just have new functions for the
>>>> new way (i.e. short term vmap protected by dma_resv lock. Maybe call
>>>> these dma_buf_vmap_local, in the spirit of the new kmap_local which
>>>> are currently under discussion. I think _local suffix is better, for
>>>> otherwise people might do something silly like
>>>>
>>>>       dma_resv_lock();
>>>>       dma_buf_vmap_locked();
>>>>       dma_resv_unlock();
>>>>
>>>>       /* actual access maybe even in some other thread */
>>>>
>>>>      dma_buf_resv_lock();
>>>>      dma_buf_vunmap_unlocked();
>>>>      dma_resv_unlock();
>>>>
>>>> _local suffix is better at telling that the resulting pointer has very
>>>> limited use (essentially just local to the calling context, if you
>>>> don't change any locking or anything).
>>>
>>> _local sounds good.
>>>
>>> Best regards
>>> Thomas
>>>
>>>>
>>>> I think encouraging importers to call dma_buf_pin/unpin isn't a good
>>>> idea. Yes dynamic ones need it, but maybe we should check for that
>>>> somehow in the exporterd interface (atm only amdgpu is using it).
>>>> -Daniel
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>> Best regards
>>>>> Thomas
>>>>>
>>>>>
>>>>>>
>>>>>> Cheers,
>>>>>> Christian.
>>>>>>
>>>>>>>
>>>>>>> That's what I meant with that this approach here is very 
>>>>>>> sprawling :-/
>>>>>>> -Daniel
>>>>>>>
>>>>>>>>      */
>>>>>>>>     int drm_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct 
>>>>>>>> dma_buf_map
>>>>>>>> *map)
>>>>>>>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
>>>>>>>> index 5e6daa1c982f..7c34cd5ec261 100644
>>>>>>>> --- a/include/drm/drm_gem.h
>>>>>>>> +++ b/include/drm/drm_gem.h
>>>>>>>> @@ -137,7 +137,21 @@ struct drm_gem_object_funcs {
>>>>>>>>          * Returns a virtual address for the buffer. Used by the
>>>>>>>>          * drm_gem_dmabuf_vmap() helper.
>>>>>>>>          *
>>>>>>>> +     * Notes to implementors:
>>>>>>>> +     *
>>>>>>>> +     * - Implementations must expect pairs of @vmap and 
>>>>>>>> @vunmap to be
>>>>>>>> +     *   called frequently and should optimize for this case.
>>>>>>>> +     *
>>>>>>>> +     * - Implemenations may expect the caller to hold the GEM 
>>>>>>>> object's
>>>>>>>> +     *   reservation lock to protect against concurrent calls and
>>>>>>>> relocation
>>>>>>>> +     *   of the GEM object.
>>>>>>>> +     *
>>>>>>>> +     * - Implementations may provide additional guarantees (e.g.,
>>>>>>>> working
>>>>>>>> +     *   without holding the reservation lock).
>>>>>>>> +     *
>>>>>>>>          * This callback is optional.
>>>>>>>> +     *
>>>>>>>> +     * See also drm_gem_dmabuf_vmap()
>>>>>>>>          */
>>>>>>>>         int (*vmap)(struct drm_gem_object *obj, struct 
>>>>>>>> dma_buf_map *map);
>>>>>>>> @@ -148,6 +162,8 @@ struct drm_gem_object_funcs {
>>>>>>>>          * drm_gem_dmabuf_vunmap() helper.
>>>>>>>>          *
>>>>>>>>          * This callback is optional.
>>>>>>>> +     *
>>>>>>>> +     * See also @vmap.
>>>>>>>>          */
>>>>>>>>         void (*vunmap)(struct drm_gem_object *obj, struct 
>>>>>>>> dma_buf_map
>>>>>>>> *map);
>>>>>>>> -- 
>>>>>>>> 2.29.2
>>>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> dri-devel mailing list
>>>>>> dri-devel@lists.freedesktop.org
>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>>
>>>>> -- 
>>>>> Thomas Zimmermann
>>>>> Graphics Driver Developer
>>>>> SUSE Software Solutions Germany GmbH
>>>>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>>>>> (HRB 36809, AG Nürnberg)
>>>>> Geschäftsführer: Felix Imendörffer
>>>>>
>>>>
>>>>
>>>
>>> -- 
>>> Thomas Zimmermann
>>> Graphics Driver Developer
>>> SUSE Software Solutions Germany GmbH
>>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>>> (HRB 36809, AG Nürnberg)
>>> Geschäftsführer: Felix Imendörffer
>>>
>>
>>
>

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

Re: [PATCH 1/8] drm/gem: Write down some rules for vmap usage

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 7118 bytes --]

Hi

Am 01.12.20 um 11:34 schrieb Christian König:
>> [...]
>> In patch 6 of this series, there's ast cursor code that acquires two 
>> BO's reservation locks and vmaps them afterwards. That's probably how 
>> you intend to use dma_buf_vmap_local.
>>
>> However, I think it's more logically to have a vmap callback that only 
>> does the actual vmap. This is all that exporters would have to implement.
>>
>> And with that, build one helper that pins before vmap and one helper 
>> that gets the resv lock.
> 
> I don't think that this is will work nor is it a good approach.
> 
> See the ast cursor handling for example. You need to acquire two BOs 
> here, not just one. And this can't be done cleanly with a single vmap call.

That seems to be a misunderstanding.

I don't mentioned it explicitly, but there's of course another helper 
that only vmaps and nothing else. This would be useful for cases like 
the cursor code. So there would be:

  dma_buf_vmap() - pin + vmap
  dma_buf_vmap_local() - lock + vmap
  dma_buf_vmap_locked() - only vmap; caller has set up the BOs

I did some conversion of drivers that use vram and shmem. They 
occasionally update a buffer (ast cursors) or flush a BO from system 
memory to HW (udl, cirrus, mgag200). In terms of these 3 interfaces: I 
never needed dma_buf_vmap() because pinning was never really required 
here. Almost all of the cases were handled by dma_buf_vmap_local(). And 
the ast cursor code uses the equivalent of dma_buf_vmap_locked().

The driver exporting the buffer would only have to implement vmap() and 
  pin() interfaces. Each does only its one thing and would assume that 
the caller holds the lock.

Best regards
Thomas

> 
> Regards,
> Christian.
> 
>>
>> I know that it might require some work on exporting drivers. But with 
>> your proposal, we probably need another dma-buf callback just for 
>> vmap_local. (?) That seems even less appealing to me.
>>
>> Best regards
>> Thomas
>>
>>>
>>> Trying to shovel both semantics into one interface, depending upon
>>> which implementation we have backing the buffer, doesn't work indeed.
>>>
>>> Also on the pin topic, I think neither interface should require
>>> callers to explicitly pin anything. For existing users it should
>>> happen automatically behind the scenes imo, that's what they're
>>> expecting.
>>> -Daniel
>>>
>>>
>>>>> I think we could use what we've done for dynamic dma-buf attachment
>>>>> (which also change locking rules) and just have new functions for the
>>>>> new way (i.e. short term vmap protected by dma_resv lock. Maybe call
>>>>> these dma_buf_vmap_local, in the spirit of the new kmap_local which
>>>>> are currently under discussion. I think _local suffix is better, for
>>>>> otherwise people might do something silly like
>>>>>
>>>>>       dma_resv_lock();
>>>>>       dma_buf_vmap_locked();
>>>>>       dma_resv_unlock();
>>>>>
>>>>>       /* actual access maybe even in some other thread */
>>>>>
>>>>>      dma_buf_resv_lock();
>>>>>      dma_buf_vunmap_unlocked();
>>>>>      dma_resv_unlock();
>>>>>
>>>>> _local suffix is better at telling that the resulting pointer has very
>>>>> limited use (essentially just local to the calling context, if you
>>>>> don't change any locking or anything).
>>>>
>>>> _local sounds good.
>>>>
>>>> Best regards
>>>> Thomas
>>>>
>>>>>
>>>>> I think encouraging importers to call dma_buf_pin/unpin isn't a good
>>>>> idea. Yes dynamic ones need it, but maybe we should check for that
>>>>> somehow in the exporterd interface (atm only amdgpu is using it).
>>>>> -Daniel
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>> Best regards
>>>>>> Thomas
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Christian.
>>>>>>>
>>>>>>>>
>>>>>>>> That's what I meant with that this approach here is very 
>>>>>>>> sprawling :-/
>>>>>>>> -Daniel
>>>>>>>>
>>>>>>>>>      */
>>>>>>>>>     int drm_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct 
>>>>>>>>> dma_buf_map
>>>>>>>>> *map)
>>>>>>>>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
>>>>>>>>> index 5e6daa1c982f..7c34cd5ec261 100644
>>>>>>>>> --- a/include/drm/drm_gem.h
>>>>>>>>> +++ b/include/drm/drm_gem.h
>>>>>>>>> @@ -137,7 +137,21 @@ struct drm_gem_object_funcs {
>>>>>>>>>          * Returns a virtual address for the buffer. Used by the
>>>>>>>>>          * drm_gem_dmabuf_vmap() helper.
>>>>>>>>>          *
>>>>>>>>> +     * Notes to implementors:
>>>>>>>>> +     *
>>>>>>>>> +     * - Implementations must expect pairs of @vmap and 
>>>>>>>>> @vunmap to be
>>>>>>>>> +     *   called frequently and should optimize for this case.
>>>>>>>>> +     *
>>>>>>>>> +     * - Implemenations may expect the caller to hold the GEM 
>>>>>>>>> object's
>>>>>>>>> +     *   reservation lock to protect against concurrent calls and
>>>>>>>>> relocation
>>>>>>>>> +     *   of the GEM object.
>>>>>>>>> +     *
>>>>>>>>> +     * - Implementations may provide additional guarantees (e.g.,
>>>>>>>>> working
>>>>>>>>> +     *   without holding the reservation lock).
>>>>>>>>> +     *
>>>>>>>>>          * This callback is optional.
>>>>>>>>> +     *
>>>>>>>>> +     * See also drm_gem_dmabuf_vmap()
>>>>>>>>>          */
>>>>>>>>>         int (*vmap)(struct drm_gem_object *obj, struct 
>>>>>>>>> dma_buf_map *map);
>>>>>>>>> @@ -148,6 +162,8 @@ struct drm_gem_object_funcs {
>>>>>>>>>          * drm_gem_dmabuf_vunmap() helper.
>>>>>>>>>          *
>>>>>>>>>          * This callback is optional.
>>>>>>>>> +     *
>>>>>>>>> +     * See also @vmap.
>>>>>>>>>          */
>>>>>>>>>         void (*vunmap)(struct drm_gem_object *obj, struct 
>>>>>>>>> dma_buf_map
>>>>>>>>> *map);
>>>>>>>>> -- 
>>>>>>>>> 2.29.2
>>>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> dri-devel mailing list
>>>>>>> dri-devel@lists.freedesktop.org
>>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>>>
>>>>>> -- 
>>>>>> Thomas Zimmermann
>>>>>> Graphics Driver Developer
>>>>>> SUSE Software Solutions Germany GmbH
>>>>>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>>>>>> (HRB 36809, AG Nürnberg)
>>>>>> Geschäftsführer: Felix Imendörffer
>>>>>>
>>>>>
>>>>>
>>>>
>>>> -- 
>>>> Thomas Zimmermann
>>>> Graphics Driver Developer
>>>> SUSE Software Solutions Germany GmbH
>>>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>>>> (HRB 36809, AG Nürnberg)
>>>> Geschäftsführer: Felix Imendörffer
>>>>
>>>
>>>
>>
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 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: [PATCH 1/8] drm/gem: Write down some rules for vmap usage

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 5682 bytes --]

Hi

Am 01.12.20 um 11:00 schrieb Daniel Vetter:
>> [...]
>> For my POV, the current interfaces have no clear policy or semantics.
>> Looking through the different GEM implementations, each one seems to
>> have its own interpretation.
> 
> Yup, that's the problem really. In the past we've had vmap exclusively
> for permanently pinned access, with no locking requirements. Now we're
> trying to make this more dynamic, but in a somewhat ad-hoc fashion
> (generic fbdev emulation charged ahead with making the fbdev
> framebuffer evictable), and now it breaks at every seam. Adding more
> ad-hoc semantics on top doesn't seem like a good idea.
> 
> That's why I think we should have 2 different interfaces:
> - dma_buf_vmap, the one we currently have. Permanently pins the
> buffer, mapping survives, no locking required.

I just looked at the implementation of dma_buf_vmap() and there's no 
pinning happening AFAICT. Also, none of the callback's implementations 
does pinning (except vram helpers). Do you mean dma_buf_attach() instead?

Best regards
Thomas

> - dma_buf_vmap_local, the new interface, the one that generic fbdev
> should have used (but oh well mistakes happen), requires
> dma_resv_lock, the mapping is only local to the caller
> 
> Trying to shovel both semantics into one interface, depending upon
> which implementation we have backing the buffer, doesn't work indeed.
> 
> Also on the pin topic, I think neither interface should require
> callers to explicitly pin anything. For existing users it should
> happen automatically behind the scenes imo, that's what they're
> expecting.
> -Daniel
> 
> 
>>> I think we could use what we've done for dynamic dma-buf attachment
>>> (which also change locking rules) and just have new functions for the
>>> new way (i.e. short term vmap protected by dma_resv lock. Maybe call
>>> these dma_buf_vmap_local, in the spirit of the new kmap_local which
>>> are currently under discussion. I think _local suffix is better, for
>>> otherwise people might do something silly like
>>>
>>>       dma_resv_lock();
>>>       dma_buf_vmap_locked();
>>>       dma_resv_unlock();
>>>
>>>       /* actual access maybe even in some other thread */
>>>
>>>      dma_buf_resv_lock();
>>>      dma_buf_vunmap_unlocked();
>>>      dma_resv_unlock();
>>>
>>> _local suffix is better at telling that the resulting pointer has very
>>> limited use (essentially just local to the calling context, if you
>>> don't change any locking or anything).
>>
>> _local sounds good.
>>
>> Best regards
>> Thomas
>>
>>>
>>> I think encouraging importers to call dma_buf_pin/unpin isn't a good
>>> idea. Yes dynamic ones need it, but maybe we should check for that
>>> somehow in the exporterd interface (atm only amdgpu is using it).
>>> -Daniel
>>>
>>>
>>>
>>>
>>>
>>>> Best regards
>>>> Thomas
>>>>
>>>>
>>>>>
>>>>> Cheers,
>>>>> Christian.
>>>>>
>>>>>>
>>>>>> That's what I meant with that this approach here is very sprawling :-/
>>>>>> -Daniel
>>>>>>
>>>>>>>      */
>>>>>>>     int drm_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct dma_buf_map
>>>>>>> *map)
>>>>>>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
>>>>>>> index 5e6daa1c982f..7c34cd5ec261 100644
>>>>>>> --- a/include/drm/drm_gem.h
>>>>>>> +++ b/include/drm/drm_gem.h
>>>>>>> @@ -137,7 +137,21 @@ struct drm_gem_object_funcs {
>>>>>>>          * Returns a virtual address for the buffer. Used by the
>>>>>>>          * drm_gem_dmabuf_vmap() helper.
>>>>>>>          *
>>>>>>> +     * Notes to implementors:
>>>>>>> +     *
>>>>>>> +     * - Implementations must expect pairs of @vmap and @vunmap to be
>>>>>>> +     *   called frequently and should optimize for this case.
>>>>>>> +     *
>>>>>>> +     * - Implemenations may expect the caller to hold the GEM object's
>>>>>>> +     *   reservation lock to protect against concurrent calls and
>>>>>>> relocation
>>>>>>> +     *   of the GEM object.
>>>>>>> +     *
>>>>>>> +     * - Implementations may provide additional guarantees (e.g.,
>>>>>>> working
>>>>>>> +     *   without holding the reservation lock).
>>>>>>> +     *
>>>>>>>          * This callback is optional.
>>>>>>> +     *
>>>>>>> +     * See also drm_gem_dmabuf_vmap()
>>>>>>>          */
>>>>>>>         int (*vmap)(struct drm_gem_object *obj, struct dma_buf_map *map);
>>>>>>> @@ -148,6 +162,8 @@ struct drm_gem_object_funcs {
>>>>>>>          * drm_gem_dmabuf_vunmap() helper.
>>>>>>>          *
>>>>>>>          * This callback is optional.
>>>>>>> +     *
>>>>>>> +     * See also @vmap.
>>>>>>>          */
>>>>>>>         void (*vunmap)(struct drm_gem_object *obj, struct dma_buf_map
>>>>>>> *map);
>>>>>>> --
>>>>>>> 2.29.2
>>>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> dri-devel mailing list
>>>>> dri-devel@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>
>>>> --
>>>> Thomas Zimmermann
>>>> Graphics Driver Developer
>>>> SUSE Software Solutions Germany GmbH
>>>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>>>> (HRB 36809, AG Nürnberg)
>>>> Geschäftsführer: Felix Imendörffer
>>>>
>>>
>>>
>>
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Felix Imendörffer
>>
> 
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 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: [PATCH 1/8] drm/gem: Write down some rules for vmap usage

[Christian König]

Am 01.12.20 um 12:30 schrieb Thomas Zimmermann:
> Hi
>
> Am 01.12.20 um 11:34 schrieb Christian König:
>>> [...]
>>> In patch 6 of this series, there's ast cursor code that acquires two 
>>> BO's reservation locks and vmaps them afterwards. That's probably 
>>> how you intend to use dma_buf_vmap_local.
>>>
>>> However, I think it's more logically to have a vmap callback that 
>>> only does the actual vmap. This is all that exporters would have to 
>>> implement.
>>>
>>> And with that, build one helper that pins before vmap and one helper 
>>> that gets the resv lock.
>>
>> I don't think that this is will work nor is it a good approach.
>>
>> See the ast cursor handling for example. You need to acquire two BOs 
>> here, not just one. And this can't be done cleanly with a single vmap 
>> call.
>
> That seems to be a misunderstanding.
>
> I don't mentioned it explicitly, but there's of course another helper 
> that only vmaps and nothing else. This would be useful for cases like 
> the cursor code. So there would be:
>
>  dma_buf_vmap() - pin + vmap
>  dma_buf_vmap_local() - lock + vmap
>  dma_buf_vmap_locked() - only vmap; caller has set up the BOs

Well that zoo of helpers will certainly get a NAK from my side.

See interfaces like this should implement simple functions and not hide 
what's actually needs to be done inside the drivers using this interface.

What we could do is to add a pin count to the DMA-buf and then do 
WARN_ON(dma_buf->pin_count || dma_resv_lock_help(dma_buf->resv)) in the 
vmap/vunmap calls.

>
> I did some conversion of drivers that use vram and shmem. They 
> occasionally update a buffer (ast cursors) or flush a BO from system 
> memory to HW (udl, cirrus, mgag200). In terms of these 3 interfaces: I 
> never needed dma_buf_vmap() because pinning was never really required 
> here. Almost all of the cases were handled by dma_buf_vmap_local(). 
> And the ast cursor code uses the equivalent of dma_buf_vmap_locked().

Yeah, that is kind of expected. I was already wondering as well why we 
didn't used the reservation lock more extensively.

Regards,
Christian.

>
> The driver exporting the buffer would only have to implement vmap() 
> and  pin() interfaces. Each does only its one thing and would assume 
> that the caller holds the lock.
>
> Best regards
> Thomas
>
>>
>> Regards,
>> Christian.
>>
>>>
>>> I know that it might require some work on exporting drivers. But 
>>> with your proposal, we probably need another dma-buf callback just 
>>> for vmap_local. (?) That seems even less appealing to me.
>>>
>>> Best regards
>>> Thomas
>>>
>>>>
>>>> Trying to shovel both semantics into one interface, depending upon
>>>> which implementation we have backing the buffer, doesn't work indeed.
>>>>
>>>> Also on the pin topic, I think neither interface should require
>>>> callers to explicitly pin anything. For existing users it should
>>>> happen automatically behind the scenes imo, that's what they're
>>>> expecting.
>>>> -Daniel
>>>>
>>>>
>>>>>> I think we could use what we've done for dynamic dma-buf attachment
>>>>>> (which also change locking rules) and just have new functions for 
>>>>>> the
>>>>>> new way (i.e. short term vmap protected by dma_resv lock. Maybe call
>>>>>> these dma_buf_vmap_local, in the spirit of the new kmap_local which
>>>>>> are currently under discussion. I think _local suffix is better, for
>>>>>> otherwise people might do something silly like
>>>>>>
>>>>>>       dma_resv_lock();
>>>>>>       dma_buf_vmap_locked();
>>>>>>       dma_resv_unlock();
>>>>>>
>>>>>>       /* actual access maybe even in some other thread */
>>>>>>
>>>>>>      dma_buf_resv_lock();
>>>>>>      dma_buf_vunmap_unlocked();
>>>>>>      dma_resv_unlock();
>>>>>>
>>>>>> _local suffix is better at telling that the resulting pointer has 
>>>>>> very
>>>>>> limited use (essentially just local to the calling context, if you
>>>>>> don't change any locking or anything).
>>>>>
>>>>> _local sounds good.
>>>>>
>>>>> Best regards
>>>>> Thomas
>>>>>
>>>>>>
>>>>>> I think encouraging importers to call dma_buf_pin/unpin isn't a good
>>>>>> idea. Yes dynamic ones need it, but maybe we should check for that
>>>>>> somehow in the exporterd interface (atm only amdgpu is using it).
>>>>>> -Daniel
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>> Best regards
>>>>>>> Thomas
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Cheers,
>>>>>>>> Christian.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> That's what I meant with that this approach here is very 
>>>>>>>>> sprawling :-/
>>>>>>>>> -Daniel
>>>>>>>>>
>>>>>>>>>>      */
>>>>>>>>>>     int drm_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct 
>>>>>>>>>> dma_buf_map
>>>>>>>>>> *map)
>>>>>>>>>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
>>>>>>>>>> index 5e6daa1c982f..7c34cd5ec261 100644
>>>>>>>>>> --- a/include/drm/drm_gem.h
>>>>>>>>>> +++ b/include/drm/drm_gem.h
>>>>>>>>>> @@ -137,7 +137,21 @@ struct drm_gem_object_funcs {
>>>>>>>>>>          * Returns a virtual address for the buffer. Used by the
>>>>>>>>>>          * drm_gem_dmabuf_vmap() helper.
>>>>>>>>>>          *
>>>>>>>>>> +     * Notes to implementors:
>>>>>>>>>> +     *
>>>>>>>>>> +     * - Implementations must expect pairs of @vmap and 
>>>>>>>>>> @vunmap to be
>>>>>>>>>> +     *   called frequently and should optimize for this case.
>>>>>>>>>> +     *
>>>>>>>>>> +     * - Implemenations may expect the caller to hold the 
>>>>>>>>>> GEM object's
>>>>>>>>>> +     *   reservation lock to protect against concurrent 
>>>>>>>>>> calls and
>>>>>>>>>> relocation
>>>>>>>>>> +     *   of the GEM object.
>>>>>>>>>> +     *
>>>>>>>>>> +     * - Implementations may provide additional guarantees 
>>>>>>>>>> (e.g.,
>>>>>>>>>> working
>>>>>>>>>> +     *   without holding the reservation lock).
>>>>>>>>>> +     *
>>>>>>>>>>          * This callback is optional.
>>>>>>>>>> +     *
>>>>>>>>>> +     * See also drm_gem_dmabuf_vmap()
>>>>>>>>>>          */
>>>>>>>>>>         int (*vmap)(struct drm_gem_object *obj, struct 
>>>>>>>>>> dma_buf_map *map);
>>>>>>>>>> @@ -148,6 +162,8 @@ struct drm_gem_object_funcs {
>>>>>>>>>>          * drm_gem_dmabuf_vunmap() helper.
>>>>>>>>>>          *
>>>>>>>>>>          * This callback is optional.
>>>>>>>>>> +     *
>>>>>>>>>> +     * See also @vmap.
>>>>>>>>>>          */
>>>>>>>>>>         void (*vunmap)(struct drm_gem_object *obj, struct 
>>>>>>>>>> dma_buf_map
>>>>>>>>>> *map);
>>>>>>>>>> -- 
>>>>>>>>>> 2.29.2
>>>>>>>>>>
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> dri-devel mailing list
>>>>>>>> dri-devel@lists.freedesktop.org
>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>>>>
>>>>>>> -- 
>>>>>>> Thomas Zimmermann
>>>>>>> Graphics Driver Developer
>>>>>>> SUSE Software Solutions Germany GmbH
>>>>>>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>>>>>>> (HRB 36809, AG Nürnberg)
>>>>>>> Geschäftsführer: Felix Imendörffer
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> -- 
>>>>> Thomas Zimmermann
>>>>> Graphics Driver Developer
>>>>> SUSE Software Solutions Germany GmbH
>>>>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>>>>> (HRB 36809, AG Nürnberg)
>>>>> Geschäftsführer: Felix Imendörffer
>>>>>
>>>>
>>>>
>>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

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

Re: [PATCH 1/8] drm/gem: Write down some rules for vmap usage

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 8928 bytes --]

Hi

Am 01.12.20 um 13:14 schrieb Christian König:
> Am 01.12.20 um 12:30 schrieb Thomas Zimmermann:
>> Hi
>>
>> Am 01.12.20 um 11:34 schrieb Christian König:
>>>> [...]
>>>> In patch 6 of this series, there's ast cursor code that acquires two 
>>>> BO's reservation locks and vmaps them afterwards. That's probably 
>>>> how you intend to use dma_buf_vmap_local.
>>>>
>>>> However, I think it's more logically to have a vmap callback that 
>>>> only does the actual vmap. This is all that exporters would have to 
>>>> implement.
>>>>
>>>> And with that, build one helper that pins before vmap and one helper 
>>>> that gets the resv lock.
>>>
>>> I don't think that this is will work nor is it a good approach.
>>>
>>> See the ast cursor handling for example. You need to acquire two BOs 
>>> here, not just one. And this can't be done cleanly with a single vmap 
>>> call.
>>
>> That seems to be a misunderstanding.
>>
>> I don't mentioned it explicitly, but there's of course another helper 
>> that only vmaps and nothing else. This would be useful for cases like 
>> the cursor code. So there would be:
>>
>>  dma_buf_vmap() - pin + vmap
>>  dma_buf_vmap_local() - lock + vmap
>>  dma_buf_vmap_locked() - only vmap; caller has set up the BOs
> 
> Well that zoo of helpers will certainly get a NAK from my side.
> 
> See interfaces like this should implement simple functions and not hide 
> what's actually needs to be done inside the drivers using this interface.

If 9 of 10 invocations use the same pattern, why not put that pattern in 
a helper? I see nothing wrong with that.

> 
> What we could do is to add a pin count to the DMA-buf and then do 
> WARN_ON(dma_buf->pin_count || dma_resv_lock_help(dma_buf->resv)) in the 
> vmap/vunmap calls.

Most of the vmap code is either CMA or SHMEM GEM stuff. They don't need 
to pin. It's just baggage to them. The TTM stuff that does need pinning 
is the minority.

> 
>>
>> I did some conversion of drivers that use vram and shmem. They 
>> occasionally update a buffer (ast cursors) or flush a BO from system 
>> memory to HW (udl, cirrus, mgag200). In terms of these 3 interfaces: I 
>> never needed dma_buf_vmap() because pinning was never really required 
>> here. Almost all of the cases were handled by dma_buf_vmap_local(). 
>> And the ast cursor code uses the equivalent of dma_buf_vmap_locked().
> 
> Yeah, that is kind of expected. I was already wondering as well why we 
> didn't used the reservation lock more extensively.

As a side note, I found only 6 trivial implementations of vmap outside 
of drivers/gpu/drm. I cannot find a single implementation of pin there. 
  What am I missing?

Best regards
Thomas

> 
> Regards,
> Christian.
> 
>>
>> The driver exporting the buffer would only have to implement vmap() 
>> and  pin() interfaces. Each does only its one thing and would assume 
>> that the caller holds the lock.
>>
>> Best regards
>> Thomas
>>
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> I know that it might require some work on exporting drivers. But 
>>>> with your proposal, we probably need another dma-buf callback just 
>>>> for vmap_local. (?) That seems even less appealing to me.
>>>>
>>>> Best regards
>>>> Thomas
>>>>
>>>>>
>>>>> Trying to shovel both semantics into one interface, depending upon
>>>>> which implementation we have backing the buffer, doesn't work indeed.
>>>>>
>>>>> Also on the pin topic, I think neither interface should require
>>>>> callers to explicitly pin anything. For existing users it should
>>>>> happen automatically behind the scenes imo, that's what they're
>>>>> expecting.
>>>>> -Daniel
>>>>>
>>>>>
>>>>>>> I think we could use what we've done for dynamic dma-buf attachment
>>>>>>> (which also change locking rules) and just have new functions for 
>>>>>>> the
>>>>>>> new way (i.e. short term vmap protected by dma_resv lock. Maybe call
>>>>>>> these dma_buf_vmap_local, in the spirit of the new kmap_local which
>>>>>>> are currently under discussion. I think _local suffix is better, for
>>>>>>> otherwise people might do something silly like
>>>>>>>
>>>>>>>       dma_resv_lock();
>>>>>>>       dma_buf_vmap_locked();
>>>>>>>       dma_resv_unlock();
>>>>>>>
>>>>>>>       /* actual access maybe even in some other thread */
>>>>>>>
>>>>>>>      dma_buf_resv_lock();
>>>>>>>      dma_buf_vunmap_unlocked();
>>>>>>>      dma_resv_unlock();
>>>>>>>
>>>>>>> _local suffix is better at telling that the resulting pointer has 
>>>>>>> very
>>>>>>> limited use (essentially just local to the calling context, if you
>>>>>>> don't change any locking or anything).
>>>>>>
>>>>>> _local sounds good.
>>>>>>
>>>>>> Best regards
>>>>>> Thomas
>>>>>>
>>>>>>>
>>>>>>> I think encouraging importers to call dma_buf_pin/unpin isn't a good
>>>>>>> idea. Yes dynamic ones need it, but maybe we should check for that
>>>>>>> somehow in the exporterd interface (atm only amdgpu is using it).
>>>>>>> -Daniel
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> Best regards
>>>>>>>> Thomas
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Cheers,
>>>>>>>>> Christian.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> That's what I meant with that this approach here is very 
>>>>>>>>>> sprawling :-/
>>>>>>>>>> -Daniel
>>>>>>>>>>
>>>>>>>>>>>      */
>>>>>>>>>>>     int drm_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct 
>>>>>>>>>>> dma_buf_map
>>>>>>>>>>> *map)
>>>>>>>>>>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
>>>>>>>>>>> index 5e6daa1c982f..7c34cd5ec261 100644
>>>>>>>>>>> --- a/include/drm/drm_gem.h
>>>>>>>>>>> +++ b/include/drm/drm_gem.h
>>>>>>>>>>> @@ -137,7 +137,21 @@ struct drm_gem_object_funcs {
>>>>>>>>>>>          * Returns a virtual address for the buffer. Used by the
>>>>>>>>>>>          * drm_gem_dmabuf_vmap() helper.
>>>>>>>>>>>          *
>>>>>>>>>>> +     * Notes to implementors:
>>>>>>>>>>> +     *
>>>>>>>>>>> +     * - Implementations must expect pairs of @vmap and 
>>>>>>>>>>> @vunmap to be
>>>>>>>>>>> +     *   called frequently and should optimize for this case.
>>>>>>>>>>> +     *
>>>>>>>>>>> +     * - Implemenations may expect the caller to hold the 
>>>>>>>>>>> GEM object's
>>>>>>>>>>> +     *   reservation lock to protect against concurrent 
>>>>>>>>>>> calls and
>>>>>>>>>>> relocation
>>>>>>>>>>> +     *   of the GEM object.
>>>>>>>>>>> +     *
>>>>>>>>>>> +     * - Implementations may provide additional guarantees 
>>>>>>>>>>> (e.g.,
>>>>>>>>>>> working
>>>>>>>>>>> +     *   without holding the reservation lock).
>>>>>>>>>>> +     *
>>>>>>>>>>>          * This callback is optional.
>>>>>>>>>>> +     *
>>>>>>>>>>> +     * See also drm_gem_dmabuf_vmap()
>>>>>>>>>>>          */
>>>>>>>>>>>         int (*vmap)(struct drm_gem_object *obj, struct 
>>>>>>>>>>> dma_buf_map *map);
>>>>>>>>>>> @@ -148,6 +162,8 @@ struct drm_gem_object_funcs {
>>>>>>>>>>>          * drm_gem_dmabuf_vunmap() helper.
>>>>>>>>>>>          *
>>>>>>>>>>>          * This callback is optional.
>>>>>>>>>>> +     *
>>>>>>>>>>> +     * See also @vmap.
>>>>>>>>>>>          */
>>>>>>>>>>>         void (*vunmap)(struct drm_gem_object *obj, struct 
>>>>>>>>>>> dma_buf_map
>>>>>>>>>>> *map);
>>>>>>>>>>> -- 
>>>>>>>>>>> 2.29.2
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> _______________________________________________
>>>>>>>>> dri-devel mailing list
>>>>>>>>> dri-devel@lists.freedesktop.org
>>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>>>>>
>>>>>>>> -- 
>>>>>>>> Thomas Zimmermann
>>>>>>>> Graphics Driver Developer
>>>>>>>> SUSE Software Solutions Germany GmbH
>>>>>>>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>>>>>>>> (HRB 36809, AG Nürnberg)
>>>>>>>> Geschäftsführer: Felix Imendörffer
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> -- 
>>>>>> Thomas Zimmermann
>>>>>> Graphics Driver Developer
>>>>>> SUSE Software Solutions Germany GmbH
>>>>>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>>>>>> (HRB 36809, AG Nürnberg)
>>>>>> Geschäftsführer: Felix Imendörffer
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 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: [PATCH 1/8] drm/gem: Write down some rules for vmap usage

[Christian König]

Am 01.12.20 um 13:33 schrieb Thomas Zimmermann:
> Hi
>
> Am 01.12.20 um 13:14 schrieb Christian König:
>> Am 01.12.20 um 12:30 schrieb Thomas Zimmermann:
>>> Hi
>>>
>>> Am 01.12.20 um 11:34 schrieb Christian König:
>>>>> [...]
>>>>> In patch 6 of this series, there's ast cursor code that acquires 
>>>>> two BO's reservation locks and vmaps them afterwards. That's 
>>>>> probably how you intend to use dma_buf_vmap_local.
>>>>>
>>>>> However, I think it's more logically to have a vmap callback that 
>>>>> only does the actual vmap. This is all that exporters would have 
>>>>> to implement.
>>>>>
>>>>> And with that, build one helper that pins before vmap and one 
>>>>> helper that gets the resv lock.
>>>>
>>>> I don't think that this is will work nor is it a good approach.
>>>>
>>>> See the ast cursor handling for example. You need to acquire two 
>>>> BOs here, not just one. And this can't be done cleanly with a 
>>>> single vmap call.
>>>
>>> That seems to be a misunderstanding.
>>>
>>> I don't mentioned it explicitly, but there's of course another 
>>> helper that only vmaps and nothing else. This would be useful for 
>>> cases like the cursor code. So there would be:
>>>
>>>  dma_buf_vmap() - pin + vmap
>>>  dma_buf_vmap_local() - lock + vmap
>>>  dma_buf_vmap_locked() - only vmap; caller has set up the BOs
>>
>> Well that zoo of helpers will certainly get a NAK from my side.
>>
>> See interfaces like this should implement simple functions and not 
>> hide what's actually needs to be done inside the drivers using this 
>> interface.
>
> If 9 of 10 invocations use the same pattern, why not put that pattern 
> in a helper? I see nothing wrong with that.

Because it hides the locking semantics inside the helper. See when you 
have the lock/unlock inside the driver it is obvious that you need to be 
careful not to take locks in different orders.

>> What we could do is to add a pin count to the DMA-buf and then do 
>> WARN_ON(dma_buf->pin_count || dma_resv_lock_help(dma_buf->resv)) in 
>> the vmap/vunmap calls.
>
> Most of the vmap code is either CMA or SHMEM GEM stuff. They don't 
> need to pin. It's just baggage to them. The TTM stuff that does need 
> pinning is the minority.
>
>>
>>>
>>> I did some conversion of drivers that use vram and shmem. They 
>>> occasionally update a buffer (ast cursors) or flush a BO from system 
>>> memory to HW (udl, cirrus, mgag200). In terms of these 3 interfaces: 
>>> I never needed dma_buf_vmap() because pinning was never really 
>>> required here. Almost all of the cases were handled by 
>>> dma_buf_vmap_local(). And the ast cursor code uses the equivalent of 
>>> dma_buf_vmap_locked().
>>
>> Yeah, that is kind of expected. I was already wondering as well why 
>> we didn't used the reservation lock more extensively.
>
> As a side note, I found only 6 trivial implementations of vmap outside 
> of drivers/gpu/drm. I cannot find a single implementation of pin 
> there.  What am I missing?

Amdgpu is the only one currently implementing the new interface. So far 
we didn't had the time nor the need to correctly move the locking into 
the calling drivers.

That's what the whole dynamic DMA-buf patches where all about.

Regards,
Christian.

>
> Best regards
> Thomas

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

Re: [PATCH 1/8] drm/gem: Write down some rules for vmap usage

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 3931 bytes --]

Hi

Am 01.12.20 um 13:38 schrieb Christian König:
> Am 01.12.20 um 13:33 schrieb Thomas Zimmermann:
>> Hi
>>
>> Am 01.12.20 um 13:14 schrieb Christian König:
>>> Am 01.12.20 um 12:30 schrieb Thomas Zimmermann:
>>>> Hi
>>>>
>>>> Am 01.12.20 um 11:34 schrieb Christian König:
>>>>>> [...]
>>>>>> In patch 6 of this series, there's ast cursor code that acquires 
>>>>>> two BO's reservation locks and vmaps them afterwards. That's 
>>>>>> probably how you intend to use dma_buf_vmap_local.
>>>>>>
>>>>>> However, I think it's more logically to have a vmap callback that 
>>>>>> only does the actual vmap. This is all that exporters would have 
>>>>>> to implement.
>>>>>>
>>>>>> And with that, build one helper that pins before vmap and one 
>>>>>> helper that gets the resv lock.
>>>>>
>>>>> I don't think that this is will work nor is it a good approach.
>>>>>
>>>>> See the ast cursor handling for example. You need to acquire two 
>>>>> BOs here, not just one. And this can't be done cleanly with a 
>>>>> single vmap call.
>>>>
>>>> That seems to be a misunderstanding.
>>>>
>>>> I don't mentioned it explicitly, but there's of course another 
>>>> helper that only vmaps and nothing else. This would be useful for 
>>>> cases like the cursor code. So there would be:
>>>>
>>>>  dma_buf_vmap() - pin + vmap
>>>>  dma_buf_vmap_local() - lock + vmap
>>>>  dma_buf_vmap_locked() - only vmap; caller has set up the BOs
>>>
>>> Well that zoo of helpers will certainly get a NAK from my side.
>>>
>>> See interfaces like this should implement simple functions and not 
>>> hide what's actually needs to be done inside the drivers using this 
>>> interface.
>>
>> If 9 of 10 invocations use the same pattern, why not put that pattern 
>> in a helper? I see nothing wrong with that.
> 
> Because it hides the locking semantics inside the helper. See when you 
> have the lock/unlock inside the driver it is obvious that you need to be 
> careful not to take locks in different orders.
> 
>>> What we could do is to add a pin count to the DMA-buf and then do 
>>> WARN_ON(dma_buf->pin_count || dma_resv_lock_help(dma_buf->resv)) in 
>>> the vmap/vunmap calls.
>>
>> Most of the vmap code is either CMA or SHMEM GEM stuff. They don't 
>> need to pin. It's just baggage to them. The TTM stuff that does need 
>> pinning is the minority.
>>
>>>
>>>>
>>>> I did some conversion of drivers that use vram and shmem. They 
>>>> occasionally update a buffer (ast cursors) or flush a BO from system 
>>>> memory to HW (udl, cirrus, mgag200). In terms of these 3 interfaces: 
>>>> I never needed dma_buf_vmap() because pinning was never really 
>>>> required here. Almost all of the cases were handled by 
>>>> dma_buf_vmap_local(). And the ast cursor code uses the equivalent of 
>>>> dma_buf_vmap_locked().
>>>
>>> Yeah, that is kind of expected. I was already wondering as well why 
>>> we didn't used the reservation lock more extensively.
>>
>> As a side note, I found only 6 trivial implementations of vmap outside 
>> of drivers/gpu/drm. I cannot find a single implementation of pin 
>> there.  What am I missing?
> 
> Amdgpu is the only one currently implementing the new interface. So far 
> we didn't had the time nor the need to correctly move the locking into 
> the calling drivers.
> 
> That's what the whole dynamic DMA-buf patches where all about.

Thanks for the pointer.

Best regards
Thomas

> 
> Regards,
> Christian.
> 
>>
>> Best regards
>> Thomas
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 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: [PATCH 1/8] drm/gem: Write down some rules for vmap usage

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 4223 bytes --]

Hi

Am 01.12.20 um 13:51 schrieb Thomas Zimmermann:
> Hi
> 
> Am 01.12.20 um 13:38 schrieb Christian König:
>> Am 01.12.20 um 13:33 schrieb Thomas Zimmermann:
>>> Hi
>>>
>>> Am 01.12.20 um 13:14 schrieb Christian König:
>>>> Am 01.12.20 um 12:30 schrieb Thomas Zimmermann:
>>>>> Hi
>>>>>
>>>>> Am 01.12.20 um 11:34 schrieb Christian König:
>>>>>>> [...]
>>>>>>> In patch 6 of this series, there's ast cursor code that acquires 
>>>>>>> two BO's reservation locks and vmaps them afterwards. That's 
>>>>>>> probably how you intend to use dma_buf_vmap_local.
>>>>>>>
>>>>>>> However, I think it's more logically to have a vmap callback that 
>>>>>>> only does the actual vmap. This is all that exporters would have 
>>>>>>> to implement.
>>>>>>>
>>>>>>> And with that, build one helper that pins before vmap and one 
>>>>>>> helper that gets the resv lock.
>>>>>>
>>>>>> I don't think that this is will work nor is it a good approach.
>>>>>>
>>>>>> See the ast cursor handling for example. You need to acquire two 
>>>>>> BOs here, not just one. And this can't be done cleanly with a 
>>>>>> single vmap call.
>>>>>
>>>>> That seems to be a misunderstanding.
>>>>>
>>>>> I don't mentioned it explicitly, but there's of course another 
>>>>> helper that only vmaps and nothing else. This would be useful for 
>>>>> cases like the cursor code. So there would be:
>>>>>
>>>>>  dma_buf_vmap() - pin + vmap
>>>>>  dma_buf_vmap_local() - lock + vmap
>>>>>  dma_buf_vmap_locked() - only vmap; caller has set up the BOs
>>>>
>>>> Well that zoo of helpers will certainly get a NAK from my side.
>>>>
>>>> See interfaces like this should implement simple functions and not 
>>>> hide what's actually needs to be done inside the drivers using this 
>>>> interface.
>>>
>>> If 9 of 10 invocations use the same pattern, why not put that pattern 
>>> in a helper? I see nothing wrong with that.
>>
>> Because it hides the locking semantics inside the helper. See when you 
>> have the lock/unlock inside the driver it is obvious that you need to 
>> be careful not to take locks in different orders.
>>
>>>> What we could do is to add a pin count to the DMA-buf and then do 
>>>> WARN_ON(dma_buf->pin_count || dma_resv_lock_help(dma_buf->resv)) in 
>>>> the vmap/vunmap calls.
>>>
>>> Most of the vmap code is either CMA or SHMEM GEM stuff. They don't 
>>> need to pin. It's just baggage to them. The TTM stuff that does need 
>>> pinning is the minority.
>>>
>>>>
>>>>>
>>>>> I did some conversion of drivers that use vram and shmem. They 
>>>>> occasionally update a buffer (ast cursors) or flush a BO from 
>>>>> system memory to HW (udl, cirrus, mgag200). In terms of these 3 
>>>>> interfaces: I never needed dma_buf_vmap() because pinning was never 
>>>>> really required here. Almost all of the cases were handled by 
>>>>> dma_buf_vmap_local(). And the ast cursor code uses the equivalent 
>>>>> of dma_buf_vmap_locked().
>>>>
>>>> Yeah, that is kind of expected. I was already wondering as well why 
>>>> we didn't used the reservation lock more extensively.
>>>
>>> As a side note, I found only 6 trivial implementations of vmap 
>>> outside of drivers/gpu/drm. I cannot find a single implementation of 
>>> pin there.  What am I missing?
>>
>> Amdgpu is the only one currently implementing the new interface. So 
>> far we didn't had the time nor the need to correctly move the locking 
>> into the calling drivers.
>>
>> That's what the whole dynamic DMA-buf patches where all about.
> 
> Thanks for the pointer.

That was not a snarky comment, although it might sound like one. I found 
the series in my inbox. :)

Best regards
Thomas

> 
> Best regards
> Thomas
> 
>>
>> Regards,
>> Christian.
>>
>>>
>>> Best regards
>>> Thomas
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 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: [PATCH 1/8] drm/gem: Write down some rules for vmap usage

[Christian König]

Am 01.12.20 um 13:53 schrieb Thomas Zimmermann:
> Hi
>
> Am 01.12.20 um 13:51 schrieb Thomas Zimmermann:
>> Hi
>>
>> Am 01.12.20 um 13:38 schrieb Christian König:
>>> Am 01.12.20 um 13:33 schrieb Thomas Zimmermann:
>>>> Hi
>>>>
>>>> Am 01.12.20 um 13:14 schrieb Christian König:
>>>>> Am 01.12.20 um 12:30 schrieb Thomas Zimmermann:
>>>>>> Hi
>>>>>>
>>>>>> Am 01.12.20 um 11:34 schrieb Christian König:
>>>>>>>> [...]
>>>>>>>> In patch 6 of this series, there's ast cursor code that 
>>>>>>>> acquires two BO's reservation locks and vmaps them afterwards. 
>>>>>>>> That's probably how you intend to use dma_buf_vmap_local.
>>>>>>>>
>>>>>>>> However, I think it's more logically to have a vmap callback 
>>>>>>>> that only does the actual vmap. This is all that exporters 
>>>>>>>> would have to implement.
>>>>>>>>
>>>>>>>> And with that, build one helper that pins before vmap and one 
>>>>>>>> helper that gets the resv lock.
>>>>>>>
>>>>>>> I don't think that this is will work nor is it a good approach.
>>>>>>>
>>>>>>> See the ast cursor handling for example. You need to acquire two 
>>>>>>> BOs here, not just one. And this can't be done cleanly with a 
>>>>>>> single vmap call.
>>>>>>
>>>>>> That seems to be a misunderstanding.
>>>>>>
>>>>>> I don't mentioned it explicitly, but there's of course another 
>>>>>> helper that only vmaps and nothing else. This would be useful for 
>>>>>> cases like the cursor code. So there would be:
>>>>>>
>>>>>>  dma_buf_vmap() - pin + vmap
>>>>>>  dma_buf_vmap_local() - lock + vmap
>>>>>>  dma_buf_vmap_locked() - only vmap; caller has set up the BOs
>>>>>
>>>>> Well that zoo of helpers will certainly get a NAK from my side.
>>>>>
>>>>> See interfaces like this should implement simple functions and not 
>>>>> hide what's actually needs to be done inside the drivers using 
>>>>> this interface.
>>>>
>>>> If 9 of 10 invocations use the same pattern, why not put that 
>>>> pattern in a helper? I see nothing wrong with that.
>>>
>>> Because it hides the locking semantics inside the helper. See when 
>>> you have the lock/unlock inside the driver it is obvious that you 
>>> need to be careful not to take locks in different orders.
>>>
>>>>> What we could do is to add a pin count to the DMA-buf and then do 
>>>>> WARN_ON(dma_buf->pin_count || dma_resv_lock_help(dma_buf->resv)) 
>>>>> in the vmap/vunmap calls.
>>>>
>>>> Most of the vmap code is either CMA or SHMEM GEM stuff. They don't 
>>>> need to pin. It's just baggage to them. The TTM stuff that does 
>>>> need pinning is the minority.
>>>>
>>>>>
>>>>>>
>>>>>> I did some conversion of drivers that use vram and shmem. They 
>>>>>> occasionally update a buffer (ast cursors) or flush a BO from 
>>>>>> system memory to HW (udl, cirrus, mgag200). In terms of these 3 
>>>>>> interfaces: I never needed dma_buf_vmap() because pinning was 
>>>>>> never really required here. Almost all of the cases were handled 
>>>>>> by dma_buf_vmap_local(). And the ast cursor code uses the 
>>>>>> equivalent of dma_buf_vmap_locked().
>>>>>
>>>>> Yeah, that is kind of expected. I was already wondering as well 
>>>>> why we didn't used the reservation lock more extensively.
>>>>
>>>> As a side note, I found only 6 trivial implementations of vmap 
>>>> outside of drivers/gpu/drm. I cannot find a single implementation 
>>>> of pin there.  What am I missing?
>>>
>>> Amdgpu is the only one currently implementing the new interface. So 
>>> far we didn't had the time nor the need to correctly move the 
>>> locking into the calling drivers.
>>>
>>> That's what the whole dynamic DMA-buf patches where all about.
>>
>> Thanks for the pointer.
>
> That was not a snarky comment, although it might sound like one. I 
> found the series in my inbox. :)

It wasn't recognized as such. And just to be clear your work here is 
extremely welcomed.

Regards,
Christian.

>
> Best regards
> Thomas
>
>>
>> Best regards
>> Thomas
>>
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> Best regards
>>>> Thomas
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>

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

Re: [PATCH 1/8] drm/gem: Write down some rules for vmap usage

[Daniel Vetter]

On Tue, Dec 1, 2020 at 11:27 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 01.12.20 um 11:00 schrieb Daniel Vetter:
> > On Tue, Dec 1, 2020 at 10:40 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >>
> >> Hi
> >>
> >> Am 01.12.20 um 10:10 schrieb Daniel Vetter:
> >>> On Tue, Dec 1, 2020 at 9:32 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >>>>
> >>>> Hi
> >>>>
> >>>> Am 30.11.20 um 16:33 schrieb Christian König:
> >>>>> Am 30.11.20 um 16:30 schrieb Daniel Vetter:
> >>>>>> On Mon, Nov 30, 2020 at 01:04:26PM +0100, Thomas Zimmermann wrote:
> >>>>>>> Mapping a GEM object's buffer into kernel address space prevents the
> >>>>>>> buffer from being evicted from VRAM, which in turn may result in
> >>>>>>> out-of-memory errors. It's therefore required to only vmap GEM BOs for
> >>>>>>> short periods of time; unless the GEM implementation provides additional
> >>>>>>> guarantees.
> >>>>>>>
> >>>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >>>>>>> ---
> >>>>>>>     drivers/gpu/drm/drm_prime.c |  6 ++++++
> >>>>>>>     include/drm/drm_gem.h       | 16 ++++++++++++++++
> >>>>>>>     2 files changed, 22 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> >>>>>>> index 7db55fce35d8..9c9ece9833e0 100644
> >>>>>>> --- a/drivers/gpu/drm/drm_prime.c
> >>>>>>> +++ b/drivers/gpu/drm/drm_prime.c
> >>>>>>> @@ -669,6 +669,12 @@ EXPORT_SYMBOL(drm_gem_unmap_dma_buf);
> >>>>>>>      * callback. Calls into &drm_gem_object_funcs.vmap for device
> >>>>>>> specific handling.
> >>>>>>>      * The kernel virtual address is returned in map.
> >>>>>>>      *
> >>>>>>> + * To prevent the GEM object from being relocated, callers must hold
> >>>>>>> the GEM
> >>>>>>> + * object's reservation lock from when calling this function until
> >>>>>>> releasing the
> >>>>>>> + * mapping. Holding onto a mapping and the associated reservation
> >>>>>>> lock for an
> >>>>>>> + * unbound time may result in out-of-memory errors. Calls to
> >>>>>>> drm_gem_dmabuf_vmap()
> >>>>>>> + * should therefore be accompanied by a call to
> >>>>>>> drm_gem_dmabuf_vunmap().
> >>>>>>> + *
> >>>>>>>      * Returns 0 on success or a negative errno code otherwise.
> >>>>>> This is a dma-buf hook, which means just documenting the rules you'd like
> >>>>>> to have here isn't enough. We need to roll this out at the dma-buf level,
> >>>>>> and enforce it.
> >>>>>>
> >>>>>> Enforce it = assert_lock_held
> >>>>>>
> >>>>>> Roll out = review everyone. Because this goes through dma-buf it'll come
> >>>>>> back through shmem helpers (and other helpers and other subsystems) back
> >>>>>> to any driver using vmap for gpu buffers. This includes the media
> >>>>>> subsystem, and the media subsystem definitely doesn't cope with just
> >>>>>> temporarily mapping buffers. So there we need to pin them, which I think
> >>>>>> means we'll need 2 version of dma_buf_vmap - one that's temporary and
> >>>>>> requires we hold dma_resv lock, the other requires that the buffer is
> >>>>>> pinned.
> >>>>>
> >>>>> OR start to proper use the dma_buf_pin/dma_buf_unpin functions which I
> >>>>> added to cover this use case as well.
> >>>>
> >>>> While I generally agree, here are some thoughts:
> >>>>
> >>>> I found all generic pin functions useless, because they don't allow for
> >>>> specifying where to pin. With fbdev emulation, this means that console
> >>>> buffers might never make it to VRAM for scanout. If anything, the policy
> >>>> should be that pin always pins in HW-accessible memory.
> >>>>
> >>>> Pin has quite a bit of overhead (more locking, buffer movement), so it
> >>>> should be the second choice after regular vmap. To make both work
> >>>> together, pin probably relies on holding the reservation lock internally.
> >>>>
> >>>> Therefore I think we still would want some additional helpers, such as:
> >>>>
> >>>>      pin_unlocked(), which acquires the resv lock, calls regular pin and
> >>>> then drops the resv lock. Same for unpin_unlocked()
> >>>>
> >>>>      vmap_pinned(), which enforces that the buffer has been pinned and
> >>>> then calls regalar vmap. Same for vunmap_pinned()
> >>>>
> >>>> A typical pattern with these functions would look like this.
> >>>>
> >>>>           drm_gem_object bo;
> >>>>           dma_buf_map map;
> >>>>
> >>>>           init() {
> >>>>                   pin_unlocked(bo);
> >>>>                   vmap_pinned(bo, map);
> >>>>           }
> >>>>
> >>>>           worker() {
> >>>>                   begin_cpu_access()
> >>>>                   // access bo via map
> >>>>                   end_cpu_access()
> >>>>           }
> >>>>
> >>>>           fini() {
> >>>>                   vunmap_pinned(bo, map);
> >>>>                   unpin_unlocked(bo);
> >>>>           }
> >>>>
> >>>>           init()
> >>>>           while (...) {
> >>>>                   worker()
> >>>>           }
> >>>>           fini()
> >>>>
> >>>> Is that reasonable for media drivers?
> >>>
> >>> So media drivers go through dma-buf, which means we always pin into
> >>> system memory. Which I guess for vram-only display drivers makes no
> >>> sense and should be rejected, but we still need somewhat consistent
> >>> rules.
> >>>
> >>> The other thing is that if you do a dma_buf_attach without dynamic
> >>> mode, dma-buf will pin things for you already. So in many cases it
> >>
> >> Do you have a pointer to code that illustrates this well?
> >>
> >>> could be that we don't need a separate pin (but since the pin is in
> >>> the exporter, not dma-buf layer, we can't check for that). I'm also
> >>> not seeing why existing users need to split up their dma_buf_vmap into
> >>> a pin + vmap, they don't need them separately.
> >>
> >> It's two different operations, with pin having some possible overhead
> >> and failure conditions. And during the last discussion, pin was say to
> >> be for userspace-managed buffers. Kernel code should hold the
> >> reservation lock.
> >>
> >> For my POV, the current interfaces have no clear policy or semantics.
> >> Looking through the different GEM implementations, each one seems to
> >> have its own interpretation.
> >
> > Yup, that's the problem really. In the past we've had vmap exclusively
> > for permanently pinned access, with no locking requirements. Now we're
> > trying to make this more dynamic, but in a somewhat ad-hoc fashion
> > (generic fbdev emulation charged ahead with making the fbdev
> > framebuffer evictable), and now it breaks at every seam. Adding more
> > ad-hoc semantics on top doesn't seem like a good idea.
> >
> > That's why I think we should have 2 different interfaces:
> > - dma_buf_vmap, the one we currently have. Permanently pins the
> > buffer, mapping survives, no locking required.
> > - dma_buf_vmap_local, the new interface, the one that generic fbdev
> > should have used (but oh well mistakes happen), requires
> > dma_resv_lock, the mapping is only local to the caller
>
> In patch 6 of this series, there's ast cursor code that acquires two
> BO's reservation locks and vmaps them afterwards. That's probably how
> you intend to use dma_buf_vmap_local.
>
> However, I think it's more logically to have a vmap callback that only
> does the actual vmap. This is all that exporters would have to implement.
>
> And with that, build one helper that pins before vmap and one helper
> that gets the resv lock.
>
> I know that it might require some work on exporting drivers. But with
> your proposal, we probably need another dma-buf callback just for
> vmap_local. (?) That seems even less appealing to me.

The stuff I was explaining is what I expect the interface to look like
for the importers.

For exporters I think there's going to be two modes, and there I think
a single vmap plus separate (optional) pin as needed sounds
reasonable. Again that's pretty much what we've done for dynamic
exporters too. So sounds all good to me.

Btw I don't think we need a dma_buf_vmap_local_unlocked helper, imo
better to inflict the dma_resv_lock onto callers explicitly. It's a
bit more code, but the dma_resv is a very tricky lock which is highly
constrained. Surfacing it instead of hiding it feels better. In
general best practice is that the dma_resv is locked by the top most
caller possible, and lower functions and callbacks just have an
dma_resv_assert_locked.
-Daniel
>
> Best regards
> Thomas
>
> >
> > Trying to shovel both semantics into one interface, depending upon
> > which implementation we have backing the buffer, doesn't work indeed.
> >
> > Also on the pin topic, I think neither interface should require
> > callers to explicitly pin anything. For existing users it should
> > happen automatically behind the scenes imo, that's what they're
> > expecting.
> > -Daniel
> >
> >
> >>> I think we could use what we've done for dynamic dma-buf attachment
> >>> (which also change locking rules) and just have new functions for the
> >>> new way (i.e. short term vmap protected by dma_resv lock. Maybe call
> >>> these dma_buf_vmap_local, in the spirit of the new kmap_local which
> >>> are currently under discussion. I think _local suffix is better, for
> >>> otherwise people might do something silly like
> >>>
> >>>       dma_resv_lock();
> >>>       dma_buf_vmap_locked();
> >>>       dma_resv_unlock();
> >>>
> >>>       /* actual access maybe even in some other thread */
> >>>
> >>>      dma_buf_resv_lock();
> >>>      dma_buf_vunmap_unlocked();
> >>>      dma_resv_unlock();
> >>>
> >>> _local suffix is better at telling that the resulting pointer has very
> >>> limited use (essentially just local to the calling context, if you
> >>> don't change any locking or anything).
> >>
> >> _local sounds good.
> >>
> >> Best regards
> >> Thomas
> >>
> >>>
> >>> I think encouraging importers to call dma_buf_pin/unpin isn't a good
> >>> idea. Yes dynamic ones need it, but maybe we should check for that
> >>> somehow in the exporterd interface (atm only amdgpu is using it).
> >>> -Daniel
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>> Best regards
> >>>> Thomas
> >>>>
> >>>>
> >>>>>
> >>>>> Cheers,
> >>>>> Christian.
> >>>>>
> >>>>>>
> >>>>>> That's what I meant with that this approach here is very sprawling :-/
> >>>>>> -Daniel
> >>>>>>
> >>>>>>>      */
> >>>>>>>     int drm_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct dma_buf_map
> >>>>>>> *map)
> >>>>>>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> >>>>>>> index 5e6daa1c982f..7c34cd5ec261 100644
> >>>>>>> --- a/include/drm/drm_gem.h
> >>>>>>> +++ b/include/drm/drm_gem.h
> >>>>>>> @@ -137,7 +137,21 @@ struct drm_gem_object_funcs {
> >>>>>>>          * Returns a virtual address for the buffer. Used by the
> >>>>>>>          * drm_gem_dmabuf_vmap() helper.
> >>>>>>>          *
> >>>>>>> +     * Notes to implementors:
> >>>>>>> +     *
> >>>>>>> +     * - Implementations must expect pairs of @vmap and @vunmap to be
> >>>>>>> +     *   called frequently and should optimize for this case.
> >>>>>>> +     *
> >>>>>>> +     * - Implemenations may expect the caller to hold the GEM object's
> >>>>>>> +     *   reservation lock to protect against concurrent calls and
> >>>>>>> relocation
> >>>>>>> +     *   of the GEM object.
> >>>>>>> +     *
> >>>>>>> +     * - Implementations may provide additional guarantees (e.g.,
> >>>>>>> working
> >>>>>>> +     *   without holding the reservation lock).
> >>>>>>> +     *
> >>>>>>>          * This callback is optional.
> >>>>>>> +     *
> >>>>>>> +     * See also drm_gem_dmabuf_vmap()
> >>>>>>>          */
> >>>>>>>         int (*vmap)(struct drm_gem_object *obj, struct dma_buf_map *map);
> >>>>>>> @@ -148,6 +162,8 @@ struct drm_gem_object_funcs {
> >>>>>>>          * drm_gem_dmabuf_vunmap() helper.
> >>>>>>>          *
> >>>>>>>          * This callback is optional.
> >>>>>>> +     *
> >>>>>>> +     * See also @vmap.
> >>>>>>>          */
> >>>>>>>         void (*vunmap)(struct drm_gem_object *obj, struct dma_buf_map
> >>>>>>> *map);
> >>>>>>> --
> >>>>>>> 2.29.2
> >>>>>>>
> >>>>>
> >>>>> _______________________________________________
> >>>>> dri-devel mailing list
> >>>>> dri-devel@lists.freedesktop.org
> >>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>>>
> >>>> --
> >>>> Thomas Zimmermann
> >>>> Graphics Driver Developer
> >>>> SUSE Software Solutions Germany GmbH
> >>>> Maxfeldstr. 5, 90409 Nürnberg, Germany
> >>>> (HRB 36809, AG Nürnberg)
> >>>> Geschäftsführer: Felix Imendörffer
> >>>>
> >>>
> >>>
> >>
> >> --
> >> Thomas Zimmermann
> >> Graphics Driver Developer
> >> SUSE Software Solutions Germany GmbH
> >> Maxfeldstr. 5, 90409 Nürnberg, Germany
> >> (HRB 36809, AG Nürnberg)
> >> Geschäftsführer: Felix Imendörffer
> >>
> >
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
>


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