[PATCH 0/3] Various clean-up patches for GEM VRAM

[PATCH 0/3] Various clean-up patches for GEM VRAM

[Thomas Zimmermann]

Replacing drm_gem_vram_push_to_system() moves policy from drivers back
to the memory manager. Now, unused BOs are only evicted when the space
is required.

The lock/unlock-renaming patch aligns the interface with other names
in DRM. No functional changes are done.

Finally, there's now a lockdep assert that ensures we don't call the
GEM VRAM _locked() functions with an unlocked BO.

Patches are against a recent drm-tip and tested on mgag200 and ast HW.

Thomas Zimmermann (3):
  drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin
  drm: Rename reserve/unreserve to lock/unlock in GEM VRAM helpers
  drm: Assert that BO is locked in drm_gem_vram_{pin,unpin}_locked()

 drivers/gpu/drm/ast/ast_fb.c             | 11 ++-
 drivers/gpu/drm/ast/ast_mode.c           | 26 ++++---
 drivers/gpu/drm/drm_gem_vram_helper.c    | 86 ++++++------------------
 drivers/gpu/drm/drm_vram_helper_common.c |  2 -
 drivers/gpu/drm/mgag200/mgag200_cursor.c | 40 +++++------
 drivers/gpu/drm/mgag200/mgag200_fb.c     | 11 ++-
 drivers/gpu/drm/mgag200/mgag200_mode.c   | 15 +++--
 include/drm/drm_gem_vram_helper.h        |  9 ++-
 8 files changed, 80 insertions(+), 120 deletions(-)

--
2.21.0

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

[PATCH 1/3] drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin

[Thomas Zimmermann]

The push-to-system function forces a buffer out of video RAM. This decision
should rather be made by the memory manager. By replacing the function with
calls to the kunmap and unpin functions, the buffer's memory becomes available,
but the buffer remains in VRAM until it's evicted by a pin operation.

This patch replaces the remaining instances of drm_gem_vram_push_to_system()
in ast and mgag200, and removes the function from DRM.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_mode.c           | 14 +++++--
 drivers/gpu/drm/drm_gem_vram_helper.c    | 50 ------------------------
 drivers/gpu/drm/drm_vram_helper_common.c |  2 -
 drivers/gpu/drm/mgag200/mgag200_mode.c   | 15 ++++---
 include/drm/drm_gem_vram_helper.h        |  1 -
 5 files changed, 20 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 9aca9135a5cc..fbafe3994654 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -521,7 +521,6 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, int mode)
 	}
 }
 
-/* ast is different - we will force move buffers out of VRAM */
 static int ast_crtc_do_set_base(struct drm_crtc *crtc,
 				struct drm_framebuffer *fb,
 				int x, int y, int atomic)
@@ -534,12 +533,15 @@ static int ast_crtc_do_set_base(struct drm_crtc *crtc,
 	s64 gpu_addr;
 	void *base;
 
-	/* push the previous fb to system ram */
 	if (!atomic && fb) {
 		ast_fb = to_ast_framebuffer(fb);
 		obj = ast_fb->obj;
 		gbo = drm_gem_vram_of_gem(obj);
-		drm_gem_vram_push_to_system(gbo);
+
+		/* unmap if console */
+		if (&ast->fbdev->afb == ast_fb)
+			drm_gem_vram_kunmap(gbo);
+		drm_gem_vram_unpin(gbo);
 	}
 
 	ast_fb = to_ast_framebuffer(crtc->primary->fb);
@@ -622,11 +624,15 @@ static void ast_crtc_disable(struct drm_crtc *crtc)
 	DRM_DEBUG_KMS("\n");
 	ast_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
 	if (crtc->primary->fb) {
+		struct ast_private *ast = crtc->dev->dev_private;
 		struct ast_framebuffer *ast_fb = to_ast_framebuffer(crtc->primary->fb);
 		struct drm_gem_object *obj = ast_fb->obj;
 		struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(obj);
 
-		drm_gem_vram_push_to_system(gbo);
+		/* unmap if console */
+		if (&ast->fbdev->afb == ast_fb)
+			drm_gem_vram_kunmap(gbo);
+		drm_gem_vram_unpin(gbo);
 	}
 	crtc->primary->fb = NULL;
 }
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index bde8237e8021..b341b70832be 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -379,56 +379,6 @@ int drm_gem_vram_unpin_reserved(struct drm_gem_vram_object *gbo)
 }
 EXPORT_SYMBOL(drm_gem_vram_unpin_reserved);
 
-/**
- * drm_gem_vram_push_to_system() - \
-	Unpins a GEM VRAM object and moves it to system memory
- * @gbo:	the GEM VRAM object
- *
- * This operation only works if the caller holds the final pin on the
- * buffer object.
- *
- * Returns:
- * 0 on success, or
- * a negative error code otherwise.
- */
-int drm_gem_vram_push_to_system(struct drm_gem_vram_object *gbo)
-{
-	int i, ret;
-	struct ttm_operation_ctx ctx = { false, false };
-
-	ret = ttm_bo_reserve(&gbo->bo, true, false, NULL);
-	if (ret < 0)
-		return ret;
-
-	if (WARN_ON_ONCE(!gbo->pin_count))
-		goto out;
-
-	--gbo->pin_count;
-	if (gbo->pin_count)
-		goto out;
-
-	if (gbo->kmap.virtual)
-		ttm_bo_kunmap(&gbo->kmap);
-
-	drm_gem_vram_placement(gbo, TTM_PL_FLAG_SYSTEM);
-	for (i = 0; i < gbo->placement.num_placement ; ++i)
-		gbo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
-
-	ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx);
-	if (ret)
-		goto err_ttm_bo_unreserve;
-
-out:
-	ttm_bo_unreserve(&gbo->bo);
-
-	return 0;
-
-err_ttm_bo_unreserve:
-	ttm_bo_unreserve(&gbo->bo);
-	return ret;
-}
-EXPORT_SYMBOL(drm_gem_vram_push_to_system);
-
 /**
  * drm_gem_vram_kmap_at() - Maps a GEM VRAM object into kernel address space
  * @gbo:	the GEM VRAM object
diff --git a/drivers/gpu/drm/drm_vram_helper_common.c b/drivers/gpu/drm/drm_vram_helper_common.c
index 3b47f7002115..e9c9f9a80ba3 100644
--- a/drivers/gpu/drm/drm_vram_helper_common.c
+++ b/drivers/gpu/drm/drm_vram_helper_common.c
@@ -79,8 +79,6 @@
  * RAM. Call drm_gem_vram_pin() with &DRM_GEM_VRAM_PL_FLAG_VRAM or
  * &DRM_GEM_VRAM_PL_FLAG_SYSTEM to pin a buffer object in video RAM or system
  * memory. Call drm_gem_vram_unpin() to release the pinned object afterwards.
- * If you have to evict a buffer object from video RAM (e.g., for freeing up
- * memory), unpin the buffer and call drm_gem_vram_push_to_system().
  *
  * A buffer object that is pinned in video RAM has a fixed address within that
  * memory region. Call drm_gem_vram_offset() to retrieve this value. Typically
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index e79872c968bf..1c8e0bfac015 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -858,8 +858,6 @@ static void mga_set_start_address(struct drm_crtc *crtc, unsigned offset)
 	WREG_ECRT(0x0, ((u8)(addr >> 16) & 0xf) | crtcext0);
 }
 
-
-/* ast is different - we will force move buffers out of VRAM */
 static int mga_crtc_do_set_base(struct drm_crtc *crtc,
 				struct drm_framebuffer *fb,
 				int x, int y, int atomic)
@@ -872,12 +870,15 @@ static int mga_crtc_do_set_base(struct drm_crtc *crtc,
 	s64 gpu_addr;
 	void *base;
 
-	/* push the previous fb to system ram */
 	if (!atomic && fb) {
 		mga_fb = to_mga_framebuffer(fb);
 		obj = mga_fb->obj;
 		gbo = drm_gem_vram_of_gem(obj);
-		drm_gem_vram_push_to_system(gbo);
+
+		/* unmap if console */
+		if (&mdev->mfbdev->mfb == mga_fb)
+			drm_gem_vram_kunmap(gbo);
+		drm_gem_vram_unpin(gbo);
 	}
 
 	mga_fb = to_mga_framebuffer(crtc->primary->fb);
@@ -1425,11 +1426,15 @@ static void mga_crtc_disable(struct drm_crtc *crtc)
 	DRM_DEBUG_KMS("\n");
 	mga_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
 	if (crtc->primary->fb) {
+		struct mga_device *mdev = crtc->dev->dev_private;
 		struct mga_framebuffer *mga_fb = to_mga_framebuffer(crtc->primary->fb);
 		struct drm_gem_object *obj = mga_fb->obj;
 		struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(obj);
 
-		drm_gem_vram_push_to_system(gbo);
+		/* unmap if console */
+		if (&mdev->mfbdev->mfb == mga_fb)
+			drm_gem_vram_kunmap(gbo);
+		drm_gem_vram_unpin(gbo);
 	}
 	crtc->primary->fb = NULL;
 }
diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
index ff1a81761543..22e52d7dfbb7 100644
--- a/include/drm/drm_gem_vram_helper.h
+++ b/include/drm/drm_gem_vram_helper.h
@@ -86,7 +86,6 @@ int drm_gem_vram_pin_reserved(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_unpin_reserved(struct drm_gem_vram_object *gbo);
-int drm_gem_vram_push_to_system(struct drm_gem_vram_object *gbo);
 void *drm_gem_vram_kmap_at(struct drm_gem_vram_object *gbo, bool map,
 			   bool *is_iomem, struct ttm_bo_kmap_obj *kmap);
 void *drm_gem_vram_kmap(struct drm_gem_vram_object *gbo, bool map,
-- 
2.21.0

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

[PATCH 2/3] drm: Rename reserve/unreserve to lock/unlock in GEM VRAM helpers

[Thomas Zimmermann]

To align with the rest of DRM terminology, the GEM VRAM helpers now use
lock and unlock in places where reserve and unreserve where used before.
All callers have been adapted.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_fb.c             | 11 +++----
 drivers/gpu/drm/ast/ast_mode.c           | 12 +++----
 drivers/gpu/drm/drm_gem_vram_helper.c    | 32 +++++++++----------
 drivers/gpu/drm/mgag200/mgag200_cursor.c | 40 ++++++++++++------------
 drivers/gpu/drm/mgag200/mgag200_fb.c     | 11 +++----
 include/drm/drm_gem_vram_helper.h        |  8 ++---
 6 files changed, 56 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_fb.c b/drivers/gpu/drm/ast/ast_fb.c
index 505e602855c0..05f45222b702 100644
--- a/drivers/gpu/drm/ast/ast_fb.c
+++ b/drivers/gpu/drm/ast/ast_fb.c
@@ -62,13 +62,12 @@ static void ast_dirty_update(struct ast_fbdev *afbdev,
 	obj = afbdev->afb.obj;
 	gbo = drm_gem_vram_of_gem(obj);
 
-	/*
-	 * try and reserve the BO, if we fail with busy
-	 * then the BO is being moved and we should
-	 * store up the damage until later.
+	/* Try to lock the BO. If we fail with -EBUSY then
+	 * the BO is being moved and we should store up the
+	 * damage until later.
 	 */
 	if (drm_can_sleep())
-		ret = drm_gem_vram_reserve(gbo, true);
+		ret = drm_gem_vram_lock(gbo, true);
 	if (ret) {
 		if (ret != -EBUSY)
 			return;
@@ -127,7 +126,7 @@ static void ast_dirty_update(struct ast_fbdev *afbdev,
 		drm_gem_vram_kunmap(gbo);
 
 out:
-	drm_gem_vram_unreserve(gbo);
+	drm_gem_vram_unlock(gbo);
 }
 
 static void ast_fillrect(struct fb_info *info,
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index fbafe3994654..fb700d620b64 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1201,7 +1201,7 @@ static int ast_cursor_set(struct drm_crtc *crtc,
 	}
 	gbo = drm_gem_vram_of_gem(obj);
 
-	ret = drm_gem_vram_reserve(gbo, false);
+	ret = drm_gem_vram_lock(gbo, false);
 	if (ret)
 		goto fail;
 
@@ -1209,7 +1209,7 @@ static int ast_cursor_set(struct drm_crtc *crtc,
 	src = drm_gem_vram_kmap_at(gbo, true, &src_isiomem, &uobj_map);
 	if (IS_ERR(src)) {
 		ret = PTR_ERR(src);
-		goto fail_unreserve;
+		goto fail_unlock;
 	}
 	if (src_isiomem == true)
 		DRM_ERROR("src cursor bo should be in main memory\n");
@@ -1218,7 +1218,7 @@ static int ast_cursor_set(struct drm_crtc *crtc,
 				   false, &dst_isiomem, &ast->cache_kmap);
 	if (IS_ERR(dst)) {
 		ret = PTR_ERR(dst);
-		goto fail_unreserve;
+		goto fail_unlock;
 	}
 	if (dst_isiomem == false)
 		DRM_ERROR("dst bo should be in VRAM\n");
@@ -1229,7 +1229,7 @@ static int ast_cursor_set(struct drm_crtc *crtc,
 	csum = copy_cursor_image(src, dst, width, height);
 
 	drm_gem_vram_kunmap_at(gbo, &uobj_map);
-	drm_gem_vram_unreserve(gbo);
+	drm_gem_vram_unlock(gbo);
 
 	/* write checksum + signature */
 	{
@@ -1262,8 +1262,8 @@ static int ast_cursor_set(struct drm_crtc *crtc,
 	drm_gem_object_put_unlocked(obj);
 	return 0;
 
-fail_unreserve:
-	drm_gem_vram_unreserve(gbo);
+fail_unlock:
+	drm_gem_vram_unlock(gbo);
 fail:
 	drm_gem_object_put_unlocked(obj);
 	return ret;
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index b341b70832be..aefb0c361486 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -152,7 +152,7 @@ void drm_gem_vram_put(struct drm_gem_vram_object *gbo)
 EXPORT_SYMBOL(drm_gem_vram_put);
 
 /**
- * drm_gem_vram_reserve() - Reserves a VRAM-backed GEM object
+ * drm_gem_vram_lock() - Locks a VRAM-backed GEM object
  * @gbo:	the GEM VRAM object
  * @no_wait:	don't wait for buffer object to become available
  *
@@ -162,24 +162,24 @@ EXPORT_SYMBOL(drm_gem_vram_put);
  * 0 on success, or
  * a negative error code otherwise
  */
-int drm_gem_vram_reserve(struct drm_gem_vram_object *gbo, bool no_wait)
+int drm_gem_vram_lock(struct drm_gem_vram_object *gbo, bool no_wait)
 {
 	return ttm_bo_reserve(&gbo->bo, true, no_wait, NULL);
 }
-EXPORT_SYMBOL(drm_gem_vram_reserve);
+EXPORT_SYMBOL(drm_gem_vram_lock);
 
 /**
- * drm_gem_vram_unreserve() - \
-	Release a reservation acquired by drm_gem_vram_reserve()
+ * drm_gem_vram_unlock() - \
+	Release a reservation acquired by drm_gem_vram_lock()
  * @gbo:	the GEM VRAM object
  *
  * See ttm_bo_unreserve() for more information.
  */
-void drm_gem_vram_unreserve(struct drm_gem_vram_object *gbo)
+void drm_gem_vram_unlock(struct drm_gem_vram_object *gbo)
 {
 	ttm_bo_unreserve(&gbo->bo);
 }
-EXPORT_SYMBOL(drm_gem_vram_unreserve);
+EXPORT_SYMBOL(drm_gem_vram_unlock);
 
 /**
  * drm_gem_vram_mmap_offset() - Returns a GEM VRAM object's mmap offset
@@ -263,7 +263,7 @@ int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, unsigned long pl_flag)
 EXPORT_SYMBOL(drm_gem_vram_pin);
 
 /**
- * drm_gem_vram_pin_reserved() - Pins a GEM VRAM object in a region.
+ * drm_gem_vram_pin_locked() - Pins a GEM VRAM object in a region.
  * @gbo:	the GEM VRAM object
  * @pl_flag:	a bitmask of possible memory regions
  *
@@ -272,14 +272,14 @@ EXPORT_SYMBOL(drm_gem_vram_pin);
  * it can be pinned to another region.
  *
  * This function pins a GEM VRAM object that has already been
- * reserved. Use drm_gem_vram_pin() if possible.
+ * locked. Use drm_gem_vram_pin() if possible.
  *
  * Returns:
  * 0 on success, or
  * a negative error code otherwise.
  */
-int drm_gem_vram_pin_reserved(struct drm_gem_vram_object *gbo,
-			      unsigned long pl_flag)
+int drm_gem_vram_pin_locked(struct drm_gem_vram_object *gbo,
+			    unsigned long pl_flag)
 {
 	int i, ret;
 	struct ttm_operation_ctx ctx = { false, false };
@@ -301,7 +301,7 @@ int drm_gem_vram_pin_reserved(struct drm_gem_vram_object *gbo,
 
 	return 0;
 }
-EXPORT_SYMBOL(drm_gem_vram_pin_reserved);
+EXPORT_SYMBOL(drm_gem_vram_pin_locked);
 
 /**
  * drm_gem_vram_unpin() - Unpins a GEM VRAM object
@@ -346,17 +346,17 @@ int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo)
 EXPORT_SYMBOL(drm_gem_vram_unpin);
 
 /**
- * drm_gem_vram_unpin_reserved() - Unpins a GEM VRAM object
+ * drm_gem_vram_unpin_locked() - Unpins a GEM VRAM object
  * @gbo:	the GEM VRAM object
  *
  * This function unpins a GEM VRAM object that has already been
- * reserved. Use drm_gem_vram_unpin() if possible.
+ * locked. Use drm_gem_vram_unpin() if possible.
  *
  * Returns:
  * 0 on success, or
  * a negative error code otherwise.
  */
-int drm_gem_vram_unpin_reserved(struct drm_gem_vram_object *gbo)
+int drm_gem_vram_unpin_locked(struct drm_gem_vram_object *gbo)
 {
 	int i, ret;
 	struct ttm_operation_ctx ctx = { false, false };
@@ -377,7 +377,7 @@ int drm_gem_vram_unpin_reserved(struct drm_gem_vram_object *gbo)
 
 	return 0;
 }
-EXPORT_SYMBOL(drm_gem_vram_unpin_reserved);
+EXPORT_SYMBOL(drm_gem_vram_unpin_locked);
 
 /**
  * drm_gem_vram_kmap_at() - Maps a GEM VRAM object into kernel address space
diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c
index 1c4fc85315a0..06a8c076cb33 100644
--- a/drivers/gpu/drm/mgag200/mgag200_cursor.c
+++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c
@@ -23,9 +23,9 @@ static void mga_hide_cursor(struct mga_device *mdev)
 	WREG8(MGA_CURPOSXL, 0);
 	WREG8(MGA_CURPOSXH, 0);
 	if (mdev->cursor.pixels_1->pin_count)
-		drm_gem_vram_unpin_reserved(mdev->cursor.pixels_1);
+		drm_gem_vram_unpin_locked(mdev->cursor.pixels_1);
 	if (mdev->cursor.pixels_2->pin_count)
-		drm_gem_vram_unpin_reserved(mdev->cursor.pixels_2);
+		drm_gem_vram_unpin_locked(mdev->cursor.pixels_2);
 }
 
 int mga_crtc_cursor_set(struct drm_crtc *crtc,
@@ -80,53 +80,53 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc,
 	if (!obj)
 		return -ENOENT;
 
-	ret = drm_gem_vram_reserve(pixels_1, true);
+	ret = drm_gem_vram_lock(pixels_1, true);
 	if (ret) {
 		WREG8(MGA_CURPOSXL, 0);
 		WREG8(MGA_CURPOSXH, 0);
 		goto out_unref;
 	}
-	ret = drm_gem_vram_reserve(pixels_2, true);
+	ret = drm_gem_vram_lock(pixels_2, true);
 	if (ret) {
 		WREG8(MGA_CURPOSXL, 0);
 		WREG8(MGA_CURPOSXH, 0);
-		drm_gem_vram_unreserve(pixels_1);
-		goto out_unreserve1;
+		drm_gem_vram_unlock(pixels_1);
+		goto out_unlock1;
 	}
 
 	/* Move cursor buffers into VRAM if they aren't already */
 	if (!pixels_1->pin_count) {
-		ret = drm_gem_vram_pin_reserved(pixels_1,
-						DRM_GEM_VRAM_PL_FLAG_VRAM);
+		ret = drm_gem_vram_pin_locked(pixels_1,
+					      DRM_GEM_VRAM_PL_FLAG_VRAM);
 		if (ret)
 			goto out1;
 		gpu_addr = drm_gem_vram_offset(pixels_1);
 		if (gpu_addr < 0) {
-			drm_gem_vram_unpin_reserved(pixels_1);
+			drm_gem_vram_unpin_locked(pixels_1);
 			goto out1;
 		}
 		mdev->cursor.pixels_1_gpu_addr = gpu_addr;
 	}
 	if (!pixels_2->pin_count) {
-		ret = drm_gem_vram_pin_reserved(pixels_2,
-						DRM_GEM_VRAM_PL_FLAG_VRAM);
+		ret = drm_gem_vram_pin_locked(pixels_2,
+					      DRM_GEM_VRAM_PL_FLAG_VRAM);
 		if (ret) {
-			drm_gem_vram_unpin_reserved(pixels_1);
+			drm_gem_vram_unpin_locked(pixels_1);
 			goto out1;
 		}
 		gpu_addr = drm_gem_vram_offset(pixels_2);
 		if (gpu_addr < 0) {
-			drm_gem_vram_unpin_reserved(pixels_1);
-			drm_gem_vram_unpin_reserved(pixels_2);
+			drm_gem_vram_unpin_locked(pixels_1);
+			drm_gem_vram_unpin_locked(pixels_2);
 			goto out1;
 		}
 		mdev->cursor.pixels_2_gpu_addr = gpu_addr;
 	}
 
 	gbo = drm_gem_vram_of_gem(obj);
-	ret = drm_gem_vram_reserve(gbo, true);
+	ret = drm_gem_vram_lock(gbo, true);
 	if (ret) {
-		dev_err(&dev->pdev->dev, "failed to reserve user bo\n");
+		dev_err(&dev->pdev->dev, "failed to lock user bo\n");
 		goto out1;
 	}
 	src = drm_gem_vram_kmap(gbo, true, NULL);
@@ -250,13 +250,13 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc,
  out3:
 	drm_gem_vram_kunmap(gbo);
  out2:
-	drm_gem_vram_unreserve(gbo);
+	drm_gem_vram_unlock(gbo);
  out1:
 	if (ret)
 		mga_hide_cursor(mdev);
-	drm_gem_vram_unreserve(pixels_1);
-out_unreserve1:
-	drm_gem_vram_unreserve(pixels_2);
+	drm_gem_vram_unlock(pixels_1);
+out_unlock1:
+	drm_gem_vram_unlock(pixels_2);
 out_unref:
 	drm_gem_object_put_unlocked(obj);
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_fb.c b/drivers/gpu/drm/mgag200/mgag200_fb.c
index 87217bdce9f8..97c575a9a86f 100644
--- a/drivers/gpu/drm/mgag200/mgag200_fb.c
+++ b/drivers/gpu/drm/mgag200/mgag200_fb.c
@@ -36,13 +36,12 @@ static void mga_dirty_update(struct mga_fbdev *mfbdev,
 	obj = mfbdev->mfb.obj;
 	gbo = drm_gem_vram_of_gem(obj);
 
-	/*
-	 * try and reserve the BO, if we fail with busy
-	 * then the BO is being moved and we should
-	 * store up the damage until later.
+	/* Try to lock the BO. If we fail with -EBUSY then
+	 * the BO is being moved and we should store up the
+	 * damage until later.
 	 */
 	if (drm_can_sleep())
-		ret = drm_gem_vram_reserve(gbo, true);
+		ret = drm_gem_vram_lock(gbo, true);
 	if (ret) {
 		if (ret != -EBUSY)
 			return;
@@ -101,7 +100,7 @@ static void mga_dirty_update(struct mga_fbdev *mfbdev,
 		drm_gem_vram_kunmap(gbo);
 
 out:
-	drm_gem_vram_unreserve(gbo);
+	drm_gem_vram_unlock(gbo);
 }
 
 static void mga_fillrect(struct fb_info *info,
diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
index 22e52d7dfbb7..4d1d2c1bf32b 100644
--- a/include/drm/drm_gem_vram_helper.h
+++ b/include/drm/drm_gem_vram_helper.h
@@ -77,15 +77,15 @@ struct drm_gem_vram_object *drm_gem_vram_create(struct drm_device *dev,
 						unsigned long pg_align,
 						bool interruptible);
 void drm_gem_vram_put(struct drm_gem_vram_object *gbo);
-int drm_gem_vram_reserve(struct drm_gem_vram_object *gbo, bool no_wait);
-void drm_gem_vram_unreserve(struct drm_gem_vram_object *gbo);
+int drm_gem_vram_lock(struct drm_gem_vram_object *gbo, bool no_wait);
+void drm_gem_vram_unlock(struct drm_gem_vram_object *gbo);
 u64 drm_gem_vram_mmap_offset(struct drm_gem_vram_object *gbo);
 s64 drm_gem_vram_offset(struct drm_gem_vram_object *gbo);
 int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, unsigned long pl_flag);
-int drm_gem_vram_pin_reserved(struct drm_gem_vram_object *gbo,
+int drm_gem_vram_pin_locked(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_unpin_reserved(struct drm_gem_vram_object *gbo);
+int drm_gem_vram_unpin_locked(struct drm_gem_vram_object *gbo);
 void *drm_gem_vram_kmap_at(struct drm_gem_vram_object *gbo, bool map,
 			   bool *is_iomem, struct ttm_bo_kmap_obj *kmap);
 void *drm_gem_vram_kmap(struct drm_gem_vram_object *gbo, bool map,
-- 
2.21.0

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

[PATCH 3/3] drm: Assert that BO is locked in drm_gem_vram_{pin, unpin}_locked()

[Thomas Zimmermann]

We may not call drm_gem_vram_{pin,unpin}_locked() with an unlocked
BO. Now test for this.

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

diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index aefb0c361486..7380a06a582c 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -284,6 +284,8 @@ int drm_gem_vram_pin_locked(struct drm_gem_vram_object *gbo,
 	int i, ret;
 	struct ttm_operation_ctx ctx = { false, false };
 
+	lockdep_assert_held(&gbo->bo.resv->lock.base);
+
 	if (gbo->pin_count) {
 		++gbo->pin_count;
 		return 0;
@@ -361,6 +363,8 @@ int drm_gem_vram_unpin_locked(struct drm_gem_vram_object *gbo)
 	int i, ret;
 	struct ttm_operation_ctx ctx = { false, false };
 
+	lockdep_assert_held(&gbo->bo.resv->lock.base);
+
 	if (WARN_ON_ONCE(!gbo->pin_count))
 		return 0;
 
-- 
2.21.0

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

Re: [PATCH 0/3] Various clean-up patches for GEM VRAM

[Daniel Vetter]

On Tue, May 21, 2019 at 01:08:28PM +0200, Thomas Zimmermann wrote:
> Replacing drm_gem_vram_push_to_system() moves policy from drivers back
> to the memory manager. Now, unused BOs are only evicted when the space
> is required.
> 
> The lock/unlock-renaming patch aligns the interface with other names
> in DRM. No functional changes are done.
> 
> Finally, there's now a lockdep assert that ensures we don't call the
> GEM VRAM _locked() functions with an unlocked BO.
> 
> Patches are against a recent drm-tip and tested on mgag200 and ast HW.
> 
> Thomas Zimmermann (3):
>   drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin
>   drm: Rename reserve/unreserve to lock/unlock in GEM VRAM helpers
>   drm: Assert that BO is locked in drm_gem_vram_{pin,unpin}_locked()

Awesome, thanks a lot for quickly working on this. On the series:

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

But definitely get someone with more knowledge of the details to check
this all again.

Aside: Do you plan to continue working on drm drivers, i.e. any need for
drm-misc commit rights?

Cheers, Daniel

> 
>  drivers/gpu/drm/ast/ast_fb.c             | 11 ++-
>  drivers/gpu/drm/ast/ast_mode.c           | 26 ++++---
>  drivers/gpu/drm/drm_gem_vram_helper.c    | 86 ++++++------------------
>  drivers/gpu/drm/drm_vram_helper_common.c |  2 -
>  drivers/gpu/drm/mgag200/mgag200_cursor.c | 40 +++++------
>  drivers/gpu/drm/mgag200/mgag200_fb.c     | 11 ++-
>  drivers/gpu/drm/mgag200/mgag200_mode.c   | 15 +++--
>  include/drm/drm_gem_vram_helper.h        |  9 ++-
>  8 files changed, 80 insertions(+), 120 deletions(-)
> 
> --
> 2.21.0
> 

-- 
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 0/3] Various clean-up patches for GEM VRAM

[Thomas Zimmermann]

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

Hi

Am 21.05.19 um 14:40 schrieb Daniel Vetter:
> On Tue, May 21, 2019 at 01:08:28PM +0200, Thomas Zimmermann wrote:
>> Replacing drm_gem_vram_push_to_system() moves policy from drivers back
>> to the memory manager. Now, unused BOs are only evicted when the space
>> is required.
>>
>> The lock/unlock-renaming patch aligns the interface with other names
>> in DRM. No functional changes are done.
>>
>> Finally, there's now a lockdep assert that ensures we don't call the
>> GEM VRAM _locked() functions with an unlocked BO.
>>
>> Patches are against a recent drm-tip and tested on mgag200 and ast HW.
>>
>> Thomas Zimmermann (3):
>>   drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin
>>   drm: Rename reserve/unreserve to lock/unlock in GEM VRAM helpers
>>   drm: Assert that BO is locked in drm_gem_vram_{pin,unpin}_locked()
> 
> Awesome, thanks a lot for quickly working on this. On the series:
> 
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> But definitely get someone with more knowledge of the details to check
> this all again.
> 
> Aside: Do you plan to continue working on drm drivers,

Yes, that's my job at SUSE.

> i.e. any need for
> drm-misc commit rights?

Sure. Thank you for your trust. From what I could found online, I guess
[1] and [2] applies?

Best regards
Thomas

[1]
https://drm.pages.freedesktop.org/maintainer-tools/committer-drm-misc.html
[2] https://drm.pages.freedesktop.org/maintainer-tools/commit-access.html

> Cheers, Daniel
> 
>>
>>  drivers/gpu/drm/ast/ast_fb.c             | 11 ++-
>>  drivers/gpu/drm/ast/ast_mode.c           | 26 ++++---
>>  drivers/gpu/drm/drm_gem_vram_helper.c    | 86 ++++++------------------
>>  drivers/gpu/drm/drm_vram_helper_common.c |  2 -
>>  drivers/gpu/drm/mgag200/mgag200_cursor.c | 40 +++++------
>>  drivers/gpu/drm/mgag200/mgag200_fb.c     | 11 ++-
>>  drivers/gpu/drm/mgag200/mgag200_mode.c   | 15 +++--
>>  include/drm/drm_gem_vram_helper.h        |  9 ++-
>>  8 files changed, 80 insertions(+), 120 deletions(-)
>>
>> --
>> 2.21.0
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

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

Re: [PATCH 0/3] Various clean-up patches for GEM VRAM

[Daniel Vetter]

On Tue, May 21, 2019 at 4:26 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 21.05.19 um 14:40 schrieb Daniel Vetter:
> > On Tue, May 21, 2019 at 01:08:28PM +0200, Thomas Zimmermann wrote:
> >> Replacing drm_gem_vram_push_to_system() moves policy from drivers back
> >> to the memory manager. Now, unused BOs are only evicted when the space
> >> is required.
> >>
> >> The lock/unlock-renaming patch aligns the interface with other names
> >> in DRM. No functional changes are done.
> >>
> >> Finally, there's now a lockdep assert that ensures we don't call the
> >> GEM VRAM _locked() functions with an unlocked BO.
> >>
> >> Patches are against a recent drm-tip and tested on mgag200 and ast HW.
> >>
> >> Thomas Zimmermann (3):
> >>   drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin
> >>   drm: Rename reserve/unreserve to lock/unlock in GEM VRAM helpers
> >>   drm: Assert that BO is locked in drm_gem_vram_{pin,unpin}_locked()
> >
> > Awesome, thanks a lot for quickly working on this. On the series:
> >
> > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> > But definitely get someone with more knowledge of the details to check
> > this all again.
> >
> > Aside: Do you plan to continue working on drm drivers,
>
> Yes, that's my job at SUSE.
>
> > i.e. any need for
> > drm-misc commit rights?
>
> Sure. Thank you for your trust. From what I could found online, I guess
> [1] and [2] applies?
>
> Best regards
> Thomas
>
> [1]
> https://drm.pages.freedesktop.org/maintainer-tools/committer-drm-misc.html
> [2] https://drm.pages.freedesktop.org/maintainer-tools/commit-access.html

The getting started page is pretty useful too:

https://drm.pages.freedesktop.org/maintainer-tools/getting-started.html

Wrt account you need a legacy ssh account from here:

https://www.freedesktop.org/wiki/AccountRequests/

Just highlighting that, it's all in the docs too, but at least myself
I glossed over the right link a few times :-)

Cheers, Daniel

>
> > Cheers, Daniel
> >
> >>
> >>  drivers/gpu/drm/ast/ast_fb.c             | 11 ++-
> >>  drivers/gpu/drm/ast/ast_mode.c           | 26 ++++---
> >>  drivers/gpu/drm/drm_gem_vram_helper.c    | 86 ++++++------------------
> >>  drivers/gpu/drm/drm_vram_helper_common.c |  2 -
> >>  drivers/gpu/drm/mgag200/mgag200_cursor.c | 40 +++++------
> >>  drivers/gpu/drm/mgag200/mgag200_fb.c     | 11 ++-
> >>  drivers/gpu/drm/mgag200/mgag200_mode.c   | 15 +++--
> >>  include/drm/drm_gem_vram_helper.h        |  9 ++-
> >>  8 files changed, 80 insertions(+), 120 deletions(-)
> >>
> >> --
> >> 2.21.0
> >>
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
> HRB 21284 (AG Nürnberg)
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] Various clean-up patches for GEM VRAM

[Gerd Hoffmann]

On Tue, May 21, 2019 at 02:40:22PM +0200, Daniel Vetter wrote:
> On Tue, May 21, 2019 at 01:08:28PM +0200, Thomas Zimmermann wrote:
> > Replacing drm_gem_vram_push_to_system() moves policy from drivers back
> > to the memory manager. Now, unused BOs are only evicted when the space
> > is required.
> > 
> > The lock/unlock-renaming patch aligns the interface with other names
> > in DRM. No functional changes are done.
> > 
> > Finally, there's now a lockdep assert that ensures we don't call the
> > GEM VRAM _locked() functions with an unlocked BO.
> > 
> > Patches are against a recent drm-tip and tested on mgag200 and ast HW.
> > 
> > Thomas Zimmermann (3):
> >   drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin
> >   drm: Rename reserve/unreserve to lock/unlock in GEM VRAM helpers
> >   drm: Assert that BO is locked in drm_gem_vram_{pin,unpin}_locked()
> 
> Awesome, thanks a lot for quickly working on this. On the series:
> 
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> But definitely get someone with more knowledge of the details to check
> this all again.

Done & pushed to drm-misc-next.

thanks,
  Gerd

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

Re: [PATCH 0/3] Various clean-up patches for GEM VRAM

[Daniel Vetter]

On Tue, May 21, 2019 at 01:08:28PM +0200, Thomas Zimmermann wrote:
> Replacing drm_gem_vram_push_to_system() moves policy from drivers back
> to the memory manager. Now, unused BOs are only evicted when the space
> is required.
> 
> The lock/unlock-renaming patch aligns the interface with other names
> in DRM. No functional changes are done.
> 
> Finally, there's now a lockdep assert that ensures we don't call the
> GEM VRAM _locked() functions with an unlocked BO.
> 
> Patches are against a recent drm-tip and tested on mgag200 and ast HW.
> 
> Thomas Zimmermann (3):
>   drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin
>   drm: Rename reserve/unreserve to lock/unlock in GEM VRAM helpers
>   drm: Assert that BO is locked in drm_gem_vram_{pin,unpin}_locked()

$ make htmldocs

results in new warnings:

./drivers/gpu/drm/drm_gem_vram_helper.c:595: warning: cannot understand function prototype: 'const struct drm_vram_mm_funcs drm_gem_vram_mm_funcs = '
./drivers/gpu/drm/drm_gem_vram_helper.c:596: warning: cannot understand function prototype: 'const struct drm_vram_mm_funcs drm_gem_vram_mm_funcs = '

You might want to review the entire output and make sure all the links
work and everything looks pretty and reasonable.

Thanks, Daniel
> 
>  drivers/gpu/drm/ast/ast_fb.c             | 11 ++-
>  drivers/gpu/drm/ast/ast_mode.c           | 26 ++++---
>  drivers/gpu/drm/drm_gem_vram_helper.c    | 86 ++++++------------------
>  drivers/gpu/drm/drm_vram_helper_common.c |  2 -
>  drivers/gpu/drm/mgag200/mgag200_cursor.c | 40 +++++------
>  drivers/gpu/drm/mgag200/mgag200_fb.c     | 11 ++-
>  drivers/gpu/drm/mgag200/mgag200_mode.c   | 15 +++--
>  include/drm/drm_gem_vram_helper.h        |  9 ++-
>  8 files changed, 80 insertions(+), 120 deletions(-)
> 
> --
> 2.21.0
> 

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