Remove explicit locking and kmap arguments from GEM VRAM interface

Remove explicit locking and kmap arguments from GEM VRAM interface

[Thomas Zimmermann]

Drivers should not have to care about internal locking of GEM VRAM objects
and their memory-mapping structures. This patch set removes both from the
GEM VRAM interface.

This affects the ast and mgag200 drivers. In places where GEM objects are
being locked by the driver, the patch converts the lock operation to a pin
operation. The locking prevented the memory manager from moving the object,
so pinning is more appropriate.

For the memory mappings, all book-keeping is done by the implementation
of GEM VRAM. Explicit kmap objects are removed from the module's public
interfaces. This change mostly affects the cursor handling in ast and
mgag200, which is being simplified by this patch set.

Future directions: with these patches in place, more code in mode setting
and fbdev emulation can be shared between ast and mgag200.

The patches have been tested on ast and mgag200 hardware.


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

[PATCH 1/8] drm/ast: Unpin cursor BO during cleanup

[Thomas Zimmermann]

The unpin operation was missing from ast_cursor_fini(). Fixed now.

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

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index fb700d620b64..41741cd6cd15 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -959,6 +959,7 @@ static void ast_cursor_fini(struct drm_device *dev)
 	struct drm_gem_vram_object *gbo =
 		drm_gem_vram_of_gem(ast->cursor_cache);
 	drm_gem_vram_kunmap_at(gbo, &ast->cache_kmap);
+	drm_gem_vram_unpin(gbo);
 	drm_gem_object_put_unlocked(ast->cursor_cache);
 }
 
-- 
2.21.0

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

Re: [PATCH 1/8] drm/ast: Unpin cursor BO during cleanup

[Maarten Lankhorst]

Op 04-06-2019 om 17:41 schreef Thomas Zimmermann:
> The unpin operation was missing from ast_cursor_fini(). Fixed now.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/ast/ast_mode.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index fb700d620b64..41741cd6cd15 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -959,6 +959,7 @@ static void ast_cursor_fini(struct drm_device *dev)
>  	struct drm_gem_vram_object *gbo =
>  		drm_gem_vram_of_gem(ast->cursor_cache);
>  	drm_gem_vram_kunmap_at(gbo, &ast->cache_kmap);
> +	drm_gem_vram_unpin(gbo);
>  	drm_gem_object_put_unlocked(ast->cursor_cache);
>  }
>  

Fixes tag?

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

Re: [PATCH 1/8] drm/ast: Unpin cursor BO during cleanup

[Thomas Zimmermann]

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

Hi

Am 05.06.19 um 09:45 schrieb Maarten Lankhorst:
> Op 04-06-2019 om 17:41 schreef Thomas Zimmermann:
>> The unpin operation was missing from ast_cursor_fini(). Fixed now.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>  drivers/gpu/drm/ast/ast_mode.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
>> index fb700d620b64..41741cd6cd15 100644
>> --- a/drivers/gpu/drm/ast/ast_mode.c
>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>> @@ -959,6 +959,7 @@ static void ast_cursor_fini(struct drm_device *dev)
>>  	struct drm_gem_vram_object *gbo =
>>  		drm_gem_vram_of_gem(ast->cursor_cache);
>>  	drm_gem_vram_kunmap_at(gbo, &ast->cache_kmap);
>> +	drm_gem_vram_unpin(gbo);
>>  	drm_gem_object_put_unlocked(ast->cursor_cache);
>>  }
>>  
> 
> Fixes tag?

I didn't add one as it would be the original commit 312fec1405dd5. Since
the code is only called during driver shutdown, I don't think it ever
was a problem. Unpinning the cursor is still the correct thing to do.

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

[PATCH 2/8] drm/ast: Remove obsolete or unused cursor state

[Thomas Zimmermann]

The ast driver's data structures store unused or uncecessary cursor
state. Most of the cursor state is already stored elsewhere and can
be retrieved when necessary. Remove the obsolete fields and adapt
users accordingly.

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

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index b6cac9511796..684e15e64a62 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -101,10 +101,6 @@ struct ast_private {
 	int fb_mtrr;
 
 	struct drm_gem_object *cursor_cache;
-	uint64_t cursor_cache_gpu_addr;
-	/* Acces to this cache is protected by the crtc->mutex of the only crtc
-	 * we have. */
-	struct ttm_bo_kmap_obj cache_kmap;
 	int next_cursor;
 	bool support_wide_screen;
 	enum {
@@ -236,9 +232,6 @@ struct ast_connector {
 
 struct ast_crtc {
 	struct drm_crtc base;
-	struct drm_gem_object *cursor_bo;
-	uint64_t cursor_addr;
-	int cursor_width, cursor_height;
 	u8 offset_x, offset_y;
 };
 
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 41741cd6cd15..cb6e8249a7db 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -939,15 +939,13 @@ static int ast_cursor_init(struct drm_device *dev)
 	}
 
 	/* kmap the object */
-	base = drm_gem_vram_kmap_at(gbo, true, NULL, &ast->cache_kmap);
+	base = drm_gem_vram_kmap(gbo, true, NULL);
 	if (IS_ERR(base)) {
 		ret = PTR_ERR(base);
 		goto fail;
 	}
 
 	ast->cursor_cache = obj;
-	ast->cursor_cache_gpu_addr = gpu_addr;
-	DRM_DEBUG_KMS("pinned cursor cache at %llx\n", ast->cursor_cache_gpu_addr);
 	return 0;
 fail:
 	return ret;
@@ -958,7 +956,7 @@ static void ast_cursor_fini(struct drm_device *dev)
 	struct ast_private *ast = dev->dev_private;
 	struct drm_gem_vram_object *gbo =
 		drm_gem_vram_of_gem(ast->cursor_cache);
-	drm_gem_vram_kunmap_at(gbo, &ast->cache_kmap);
+	drm_gem_vram_kunmap(gbo);
 	drm_gem_vram_unpin(gbo);
 	drm_gem_object_put_unlocked(ast->cursor_cache);
 }
@@ -1181,7 +1179,8 @@ static int ast_cursor_set(struct drm_crtc *crtc,
 	struct ast_crtc *ast_crtc = to_ast_crtc(crtc);
 	struct drm_gem_object *obj;
 	struct drm_gem_vram_object *gbo;
-	s64 gpu_addr;
+	s64 dst_gpu;
+	u64 gpu_addr;
 	u32 csum;
 	int ret;
 	struct ttm_bo_kmap_obj uobj_map;
@@ -1215,14 +1214,19 @@ static int ast_cursor_set(struct drm_crtc *crtc,
 	if (src_isiomem == true)
 		DRM_ERROR("src cursor bo should be in main memory\n");
 
-	dst = drm_gem_vram_kmap_at(drm_gem_vram_of_gem(ast->cursor_cache),
-				   false, &dst_isiomem, &ast->cache_kmap);
+	dst = drm_gem_vram_kmap(drm_gem_vram_of_gem(ast->cursor_cache),
+				false, &dst_isiomem);
 	if (IS_ERR(dst)) {
 		ret = PTR_ERR(dst);
 		goto fail_unlock;
 	}
 	if (dst_isiomem == false)
 		DRM_ERROR("dst bo should be in VRAM\n");
+	dst_gpu = drm_gem_vram_offset(drm_gem_vram_of_gem(ast->cursor_cache));
+	if (dst_gpu < 0) {
+		ret = (int)dst_gpu;
+		goto fail_unlock;
+	}
 
 	dst += (AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE)*ast->next_cursor;
 
@@ -1234,8 +1238,9 @@ static int ast_cursor_set(struct drm_crtc *crtc,
 
 	/* write checksum + signature */
 	{
-		u8 *dst = drm_gem_vram_kmap_at(drm_gem_vram_of_gem(ast->cursor_cache),
-					       false, NULL, &ast->cache_kmap);
+		struct drm_gem_vram_object *dst_gbo =
+			drm_gem_vram_of_gem(ast->cursor_cache);
+		u8 *dst = drm_gem_vram_kmap(dst_gbo, false, NULL);
 		dst += (AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE)*ast->next_cursor + AST_HWC_SIZE;
 		writel(csum, dst);
 		writel(width, dst + AST_HWC_SIGNATURE_SizeX);
@@ -1244,15 +1249,13 @@ static int ast_cursor_set(struct drm_crtc *crtc,
 		writel(0, dst + AST_HWC_SIGNATURE_HOTSPOTY);
 
 		/* set pattern offset */
-		gpu_addr = ast->cursor_cache_gpu_addr;
+		gpu_addr = (u64)dst_gpu;
 		gpu_addr += (AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE)*ast->next_cursor;
 		gpu_addr >>= 3;
 		ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xc8, gpu_addr & 0xff);
 		ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xc9, (gpu_addr >> 8) & 0xff);
 		ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xca, (gpu_addr >> 16) & 0xff);
 	}
-	ast_crtc->cursor_width = width;
-	ast_crtc->cursor_height = height;
 	ast_crtc->offset_x = AST_MAX_HWC_WIDTH - width;
 	ast_crtc->offset_y = AST_MAX_HWC_WIDTH - height;
 
@@ -1278,8 +1281,8 @@ static int ast_cursor_move(struct drm_crtc *crtc,
 	int x_offset, y_offset;
 	u8 *sig;
 
-	sig = drm_gem_vram_kmap_at(drm_gem_vram_of_gem(ast->cursor_cache),
-				   false, NULL, &ast->cache_kmap);
+	sig = drm_gem_vram_kmap(drm_gem_vram_of_gem(ast->cursor_cache),
+				   false, NULL);
 	sig += (AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE)*ast->next_cursor + AST_HWC_SIZE;
 	writel(x, sig + AST_HWC_SIGNATURE_X);
 	writel(y, sig + AST_HWC_SIGNATURE_Y);
-- 
2.21.0

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

[PATCH 3/8] drm/ast: Pin and map cursor source BO during update

[Thomas Zimmermann]

The ast driver used to lock the cursor source BO during updates. Locking
should be done internally by the BO's implementation, so we pin it instead
to system memory. The mapping information is also stored in the BO. No
need to have an extra argument to the kmap function.

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

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index cb6e8249a7db..be3d91d7fde5 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1183,7 +1183,6 @@ static int ast_cursor_set(struct drm_crtc *crtc,
 	u64 gpu_addr;
 	u32 csum;
 	int ret;
-	struct ttm_bo_kmap_obj uobj_map;
 	u8 *src, *dst;
 	bool src_isiomem, dst_isiomem;
 	if (!handle) {
@@ -1201,15 +1200,13 @@ static int ast_cursor_set(struct drm_crtc *crtc,
 	}
 	gbo = drm_gem_vram_of_gem(obj);
 
-	ret = drm_gem_vram_lock(gbo, false);
+	ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_SYSTEM);
 	if (ret)
-		goto fail;
-
-	memset(&uobj_map, 0, sizeof(uobj_map));
-	src = drm_gem_vram_kmap_at(gbo, true, &src_isiomem, &uobj_map);
+		goto err_drm_gem_object_put_unlocked;
+	src = drm_gem_vram_kmap(gbo, true, &src_isiomem);
 	if (IS_ERR(src)) {
 		ret = PTR_ERR(src);
-		goto fail_unlock;
+		goto err_drm_gem_vram_unpin;
 	}
 	if (src_isiomem == true)
 		DRM_ERROR("src cursor bo should be in main memory\n");
@@ -1218,14 +1215,14 @@ static int ast_cursor_set(struct drm_crtc *crtc,
 				false, &dst_isiomem);
 	if (IS_ERR(dst)) {
 		ret = PTR_ERR(dst);
-		goto fail_unlock;
+		goto err_drm_gem_vram_kunmap;
 	}
 	if (dst_isiomem == false)
 		DRM_ERROR("dst bo should be in VRAM\n");
 	dst_gpu = drm_gem_vram_offset(drm_gem_vram_of_gem(ast->cursor_cache));
 	if (dst_gpu < 0) {
 		ret = (int)dst_gpu;
-		goto fail_unlock;
+		goto err_drm_gem_vram_kunmap;
 	}
 
 	dst += (AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE)*ast->next_cursor;
@@ -1233,9 +1230,6 @@ static int ast_cursor_set(struct drm_crtc *crtc,
 	/* do data transfer to cursor cache */
 	csum = copy_cursor_image(src, dst, width, height);
 
-	drm_gem_vram_kunmap_at(gbo, &uobj_map);
-	drm_gem_vram_unlock(gbo);
-
 	/* write checksum + signature */
 	{
 		struct drm_gem_vram_object *dst_gbo =
@@ -1263,12 +1257,17 @@ static int ast_cursor_set(struct drm_crtc *crtc,
 
 	ast_show_cursor(crtc);
 
+	drm_gem_vram_kunmap(gbo);
+	drm_gem_vram_unpin(gbo);
 	drm_gem_object_put_unlocked(obj);
+
 	return 0;
 
-fail_unlock:
-	drm_gem_vram_unlock(gbo);
-fail:
+err_drm_gem_vram_kunmap:
+	drm_gem_vram_kunmap(gbo);
+err_drm_gem_vram_unpin:
+	drm_gem_vram_unpin(gbo);
+err_drm_gem_object_put_unlocked:
 	drm_gem_object_put_unlocked(obj);
 	return ret;
 }
-- 
2.21.0

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

[PATCH 4/8] drm/ast: Pin framebuffer BO during dirty update

[Thomas Zimmermann]

Another explicit lock operation of a GEM VRAM BO is located in AST's
framebuffer update code. Instead of locking the BO, we pin it to wherever
it is.

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

diff --git a/drivers/gpu/drm/ast/ast_fb.c b/drivers/gpu/drm/ast/ast_fb.c
index 05f45222b702..7d911391c9cf 100644
--- a/drivers/gpu/drm/ast/ast_fb.c
+++ b/drivers/gpu/drm/ast/ast_fb.c
@@ -48,32 +48,32 @@ static void ast_dirty_update(struct ast_fbdev *afbdev,
 			     int x, int y, int width, int height)
 {
 	int i;
-	struct drm_gem_object *obj;
 	struct drm_gem_vram_object *gbo;
 	int src_offset, dst_offset;
 	int bpp = afbdev->afb.base.format->cpp[0];
-	int ret = -EBUSY;
+	int ret;
 	u8 *dst;
 	bool unmap = false;
 	bool store_for_later = false;
 	int x2, y2;
 	unsigned long flags;
 
-	obj = afbdev->afb.obj;
-	gbo = drm_gem_vram_of_gem(obj);
+	gbo = drm_gem_vram_of_gem(afbdev->afb.obj);
 
-	/* 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.
+	/* We pin the BO to system memory so it won't be moved during
+	 * the update and doesn't waste video ram. If the BO is already
+	 * located in VRAM, the pin operation will simply increment the
+	 * pin count.
 	 */
-	if (drm_can_sleep())
-		ret = drm_gem_vram_lock(gbo, true);
-	if (ret) {
-		if (ret != -EBUSY)
-			return;
-
+	if (drm_can_sleep()) {
+		ret = drm_gem_vram_pin(gbo, TTM_PL_FLAG_SYSTEM);
+		if (ret) {
+			if (ret != -EBUSY)
+				return;
+			store_for_later = true;
+		}
+	} else
 		store_for_later = true;
-	}
 
 	x2 = x + width - 1;
 	y2 = y + height - 1;
@@ -126,7 +126,7 @@ static void ast_dirty_update(struct ast_fbdev *afbdev,
 		drm_gem_vram_kunmap(gbo);
 
 out:
-	drm_gem_vram_unlock(gbo);
+	drm_gem_vram_unpin(gbo);
 }
 
 static void ast_fillrect(struct fb_info *info,
-- 
2.21.0

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

Re: [PATCH 4/8] drm/ast: Pin framebuffer BO during dirty update

[Gerd Hoffmann]

> +	/* We pin the BO to system memory so it won't be moved during
> +	 * the update and doesn't waste video ram. If the BO is already
> +	 * located in VRAM, the pin operation will simply increment the
> +	 * pin count.
>  	 */

s/located/pinned/ I guess?

Maybe we should update drm_gem_vram_pin() to skip the
drm_gem_vram_placement() call in case pl_flag is zero, so you can pin
the bo where it happens to be at the moment.

This will avoid moving around the bo for no good reason.  We don't care
where the bo is, we only want make sure it doesn't move while we access
it.

(same goes for the cursor patch, and I guess the mga patches too but I
havn't looked yet).

cheers,
  Gerd

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

[PATCH 5/8] drm/mgag200: Pin framebuffer BO during dirty update

[Thomas Zimmermann]

Another explicit lock operation of a GEM VRAM BO is located in mgag200's
framebuffer update code. Instead of locking the BO, we pin it to wherever
it is.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_fb.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_fb.c b/drivers/gpu/drm/mgag200/mgag200_fb.c
index 97c575a9a86f..dbcf179a120c 100644
--- a/drivers/gpu/drm/mgag200/mgag200_fb.c
+++ b/drivers/gpu/drm/mgag200/mgag200_fb.c
@@ -26,7 +26,7 @@ static void mga_dirty_update(struct mga_fbdev *mfbdev,
 	struct drm_gem_vram_object *gbo;
 	int src_offset, dst_offset;
 	int bpp = mfbdev->mfb.base.format->cpp[0];
-	int ret = -EBUSY;
+	int ret;
 	u8 *dst;
 	bool unmap = false;
 	bool store_for_later = false;
@@ -36,18 +36,20 @@ static void mga_dirty_update(struct mga_fbdev *mfbdev,
 	obj = mfbdev->mfb.obj;
 	gbo = drm_gem_vram_of_gem(obj);
 
-	/* 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.
+	/* We pin the BO to system memory so it won't be moved during
+	 * the update and doesn't waste video ram. If the BO is already
+	 * located in VRAM, the pin operation will simply increment the
+	 * pin count.
 	 */
-	if (drm_can_sleep())
-		ret = drm_gem_vram_lock(gbo, true);
-	if (ret) {
-		if (ret != -EBUSY)
-			return;
-
+	if (drm_can_sleep()) {
+		ret = drm_gem_vram_pin(gbo, TTM_PL_FLAG_SYSTEM);
+		if (ret) {
+			if (ret != -EBUSY)
+				return;
+			store_for_later = true;
+		}
+	} else
 		store_for_later = true;
-	}
 
 	x2 = x + width - 1;
 	y2 = y + height - 1;
@@ -100,7 +102,7 @@ static void mga_dirty_update(struct mga_fbdev *mfbdev,
 		drm_gem_vram_kunmap(gbo);
 
 out:
-	drm_gem_vram_unlock(gbo);
+	drm_gem_vram_unpin(gbo);
 }
 
 static void mga_fillrect(struct fb_info *info,
-- 
2.21.0

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

[PATCH 6/8] drm/mgag200: Rewrite cursor handling

[Thomas Zimmermann]

The cursor handling in mgag200 is complicated to understand. It touches a
number of different BOs, but doesn't really use all of them.

Rewriting the cursor update reduces the amount of cursor state. There are
two BOs for double-buffered HW updates. The source BO updates the one that
is currently not displayed and then switches buffers. Explicit BO locking
has been removed from the code. BOs are simply pinned and unpinned in video
RAM.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_cursor.c | 165 ++++++++++-------------
 drivers/gpu/drm/mgag200/mgag200_drv.h    |   3 -
 drivers/gpu/drm/mgag200/mgag200_main.c   |   4 +-
 3 files changed, 69 insertions(+), 103 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c
index 06a8c076cb33..165895d00fcb 100644
--- a/drivers/gpu/drm/mgag200/mgag200_cursor.c
+++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c
@@ -22,10 +22,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_locked(mdev->cursor.pixels_1);
-	if (mdev->cursor.pixels_2->pin_count)
-		drm_gem_vram_unpin_locked(mdev->cursor.pixels_2);
+	if (mdev->cursor.pixels_current)
+		drm_gem_vram_unpin(mdev->cursor.pixels_current);
+	mdev->cursor.pixels_current = NULL;
 }
 
 int mga_crtc_cursor_set(struct drm_crtc *crtc,
@@ -39,7 +38,7 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc,
 	struct drm_gem_vram_object *pixels_1 = mdev->cursor.pixels_1;
 	struct drm_gem_vram_object *pixels_2 = mdev->cursor.pixels_2;
 	struct drm_gem_vram_object *pixels_current = mdev->cursor.pixels_current;
-	struct drm_gem_vram_object *pixels_prev = mdev->cursor.pixels_prev;
+	struct drm_gem_vram_object *pixels_next;
 	struct drm_gem_object *obj;
 	struct drm_gem_vram_object *gbo = NULL;
 	int ret = 0;
@@ -52,6 +51,7 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc,
 	bool found = false;
 	int colour_count = 0;
 	s64 gpu_addr;
+	u64 dst_gpu;
 	u8 reg_index;
 	u8 this_row[48];
 
@@ -61,80 +61,66 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc,
 		return -ENOTSUPP; /* Didn't allocate space for cursors */
 	}
 
-	if ((width != 64 || height != 64) && handle) {
-		WREG8(MGA_CURPOSXL, 0);
-		WREG8(MGA_CURPOSXH, 0);
-		return -EINVAL;
+	if (WARN_ON(pixels_current &&
+		    pixels_1 != pixels_current &&
+		    pixels_2 != pixels_current)) {
+		return -ENOTSUPP; /* inconsistent state */
 	}
 
-	BUG_ON(pixels_1 != pixels_current && pixels_1 != pixels_prev);
-	BUG_ON(pixels_2 != pixels_current && pixels_2 != pixels_prev);
-	BUG_ON(pixels_current == pixels_prev);
-
 	if (!handle || !file_priv) {
 		mga_hide_cursor(mdev);
 		return 0;
 	}
 
-	obj = drm_gem_object_lookup(file_priv, handle);
-	if (!obj)
-		return -ENOENT;
-
-	ret = drm_gem_vram_lock(pixels_1, true);
-	if (ret) {
+	if (width != 64 || height != 64) {
 		WREG8(MGA_CURPOSXL, 0);
 		WREG8(MGA_CURPOSXH, 0);
-		goto out_unref;
-	}
-	ret = drm_gem_vram_lock(pixels_2, true);
-	if (ret) {
-		WREG8(MGA_CURPOSXL, 0);
-		WREG8(MGA_CURPOSXH, 0);
-		drm_gem_vram_unlock(pixels_1);
-		goto out_unlock1;
+		return -EINVAL;
 	}
 
-	/* Move cursor buffers into VRAM if they aren't already */
-	if (!pixels_1->pin_count) {
-		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_locked(pixels_1);
-			goto out1;
-		}
-		mdev->cursor.pixels_1_gpu_addr = gpu_addr;
-	}
-	if (!pixels_2->pin_count) {
-		ret = drm_gem_vram_pin_locked(pixels_2,
-					      DRM_GEM_VRAM_PL_FLAG_VRAM);
-		if (ret) {
-			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_locked(pixels_1);
-			drm_gem_vram_unpin_locked(pixels_2);
-			goto out1;
-		}
-		mdev->cursor.pixels_2_gpu_addr = gpu_addr;
-	}
+	if (pixels_current == pixels_1)
+		pixels_next = pixels_2;
+	else
+		pixels_next = pixels_1;
 
+	obj = drm_gem_object_lookup(file_priv, handle);
+	if (!obj)
+		return -ENOENT;
 	gbo = drm_gem_vram_of_gem(obj);
-	ret = drm_gem_vram_lock(gbo, true);
+	ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_SYSTEM);
 	if (ret) {
 		dev_err(&dev->pdev->dev, "failed to lock user bo\n");
-		goto out1;
+		goto err_drm_gem_object_put_unlocked;
 	}
 	src = drm_gem_vram_kmap(gbo, true, NULL);
 	if (IS_ERR(src)) {
 		ret = PTR_ERR(src);
-		dev_err(&dev->pdev->dev, "failed to kmap user buffer updates\n");
-		goto out2;
+		dev_err(&dev->pdev->dev,
+			"failed to kmap user buffer updates\n");
+		goto err_drm_gem_vram_unpin_src;
+	}
+
+	/* Pin and map up-coming buffer to write colour indices */
+	ret = drm_gem_vram_pin(pixels_next, DRM_GEM_VRAM_PL_FLAG_VRAM);
+	if (ret)
+		dev_err(&dev->pdev->dev,
+			"failed to pin cursor buffer: %d\n", ret);
+		goto err_drm_gem_vram_kunmap_src;
+	dst = drm_gem_vram_kmap(pixels_next, true, NULL);
+	if (IS_ERR(dst)) {
+		ret = PTR_ERR(dst);
+		dev_err(&dev->pdev->dev,
+			"failed to kmap cursor updates: %d\n", ret);
+		goto err_drm_gem_vram_unpin_dst;
+	}
+	gpu_addr = drm_gem_vram_offset(pixels_2);
+	if (gpu_addr < 0) {
+		ret = (int)gpu_addr;
+		dev_err(&dev->pdev->dev,
+			"failed to get cursor scanout address: %d\n", ret);
+		goto err_drm_gem_vram_kunmap_dst;
 	}
+	dst_gpu = (u64)gpu_addr;
 
 	memset(&colour_set[0], 0, sizeof(uint32_t)*16);
 	/* width*height*4 = 16384 */
@@ -149,7 +135,7 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc,
 				warn_transparent = false; /* Only tell the user once. */
 			}
 			ret = -EINVAL;
-			goto out3;
+			goto err_drm_gem_vram_kunmap_dst;
 		}
 		/* Don't need to store transparent pixels as colours */
 		if (this_colour>>24 == 0x0)
@@ -171,7 +157,7 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc,
 				warn_palette = false; /* Only tell the user once. */
 			}
 			ret = -EINVAL;
-			goto out3;
+			goto err_drm_gem_vram_kunmap_dst;
 		}
 		*next_space = this_colour;
 		next_space++;
@@ -190,14 +176,6 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc,
 		BUG_ON((colour_set[i]>>24 & 0xff) != 0xff);
 	}
 
-	/* Map up-coming buffer to write colour indices */
-	dst = drm_gem_vram_kmap(pixels_prev, true, NULL);
-	if (IS_ERR(dst)) {
-		ret = PTR_ERR(dst);
-		dev_err(&dev->pdev->dev, "failed to kmap cursor updates\n");
-		goto out3;
-	}
-
 	/* now write colour indices into hardware cursor buffer */
 	for (row = 0; row < 64; row++) {
 		memset(&this_row[0], 0, 48);
@@ -224,42 +202,35 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc,
 	}
 
 	/* Program gpu address of cursor buffer */
-	if (pixels_prev == pixels_1)
-		gpu_addr = mdev->cursor.pixels_1_gpu_addr;
-	else
-		gpu_addr = mdev->cursor.pixels_2_gpu_addr;
-	WREG_DAC(MGA1064_CURSOR_BASE_ADR_LOW, (u8)((gpu_addr>>10) & 0xff));
-	WREG_DAC(MGA1064_CURSOR_BASE_ADR_HI, (u8)((gpu_addr>>18) & 0x3f));
+	WREG_DAC(MGA1064_CURSOR_BASE_ADR_LOW, (u8)((dst_gpu>>10) & 0xff));
+	WREG_DAC(MGA1064_CURSOR_BASE_ADR_HI, (u8)((dst_gpu>>18) & 0x3f));
 
 	/* Adjust cursor control register to turn on the cursor */
 	WREG_DAC(MGA1064_CURSOR_CTL, 4); /* 16-colour palletized cursor mode */
 
-	/* Now swap internal buffer pointers */
-	if (mdev->cursor.pixels_1 == mdev->cursor.pixels_prev) {
-		mdev->cursor.pixels_prev = mdev->cursor.pixels_2;
-		mdev->cursor.pixels_current = mdev->cursor.pixels_1;
-	} else if (mdev->cursor.pixels_1 == mdev->cursor.pixels_current) {
-		mdev->cursor.pixels_prev = mdev->cursor.pixels_1;
-		mdev->cursor.pixels_current = mdev->cursor.pixels_2;
-	} else {
-		BUG();
-	}
-	ret = 0;
+	/* Now update internal buffer pointers */
+	if (pixels_current)
+		drm_gem_vram_unpin(pixels_current);
+	mdev->cursor.pixels_current = pixels_next;
 
-	drm_gem_vram_kunmap(pixels_prev);
- out3:
+	drm_gem_vram_kunmap(pixels_next);
+	drm_gem_vram_unpin(pixels_next);
 	drm_gem_vram_kunmap(gbo);
- out2:
-	drm_gem_vram_unlock(gbo);
- out1:
-	if (ret)
-		mga_hide_cursor(mdev);
-	drm_gem_vram_unlock(pixels_1);
-out_unlock1:
-	drm_gem_vram_unlock(pixels_2);
-out_unref:
+	drm_gem_vram_unpin(gbo);
 	drm_gem_object_put_unlocked(obj);
 
+	return 0;
+
+err_drm_gem_vram_kunmap_dst:
+	drm_gem_vram_kunmap(pixels_next);
+err_drm_gem_vram_unpin_dst:
+	drm_gem_vram_unpin(pixels_next);
+err_drm_gem_vram_kunmap_src:
+	drm_gem_vram_kunmap(gbo);
+err_drm_gem_vram_unpin_src:
+	drm_gem_vram_unpin(gbo);
+err_drm_gem_object_put_unlocked:
+	drm_gem_object_put_unlocked(obj);
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 6180acbca7ca..d39f2a567b3f 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -158,11 +158,8 @@ struct mga_cursor {
 	*/
 	struct drm_gem_vram_object *pixels_1;
 	struct drm_gem_vram_object *pixels_2;
-	u64 pixels_1_gpu_addr, pixels_2_gpu_addr;
 	/* The currently displayed icon, this points to one of pixels_1, or pixels_2 */
 	struct drm_gem_vram_object *pixels_current;
-	/* The previously displayed icon */
-	struct drm_gem_vram_object *pixels_prev;
 };
 
 struct mga_mc {
diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
index f3687fed4075..0d7fc00e5d8a 100644
--- a/drivers/gpu/drm/mgag200/mgag200_main.c
+++ b/drivers/gpu/drm/mgag200/mgag200_main.c
@@ -241,10 +241,8 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
 		mdev->cursor.pixels_2 = NULL;
 		dev_warn(&dev->pdev->dev,
 			"Could not allocate space for cursors. Not doing hardware cursors.\n");
-	} else {
-		mdev->cursor.pixels_current = mdev->cursor.pixels_1;
-		mdev->cursor.pixels_prev = mdev->cursor.pixels_2;
 	}
+	mdev->cursor.pixels_current = NULL;
 
 	return 0;
 
-- 
2.21.0

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

Re: [PATCH 6/8] drm/mgag200: Rewrite cursor handling

[Gerd Hoffmann]

On Tue, Jun 04, 2019 at 05:41:59PM +0200, Thomas Zimmermann wrote:
> The cursor handling in mgag200 is complicated to understand. It touches a
> number of different BOs, but doesn't really use all of them.
> 
> Rewriting the cursor update reduces the amount of cursor state. There are
> two BOs for double-buffered HW updates. The source BO updates the one that
> is currently not displayed and then switches buffers. Explicit BO locking
> has been removed from the code. BOs are simply pinned and unpinned in video
> RAM.

Cursors are not that big after all, so maybe pin the two BOs for
double-buffering permanently in vram to simplify things further?

Also factoring out the code which updates the two BOs to a separate
function should help making the code more readable.

But even as-is the patch is a step into the right direction.

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

cheers,
  Gerd

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

Re: [PATCH 6/8] drm/mgag200: Rewrite cursor handling

[Thomas Zimmermann]

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

Hi

Am 05.06.19 um 11:58 schrieb Gerd Hoffmann:
> On Tue, Jun 04, 2019 at 05:41:59PM +0200, Thomas Zimmermann wrote:
>> The cursor handling in mgag200 is complicated to understand. It touches a
>> number of different BOs, but doesn't really use all of them.
>>
>> Rewriting the cursor update reduces the amount of cursor state. There are
>> two BOs for double-buffered HW updates. The source BO updates the one that
>> is currently not displayed and then switches buffers. Explicit BO locking
>> has been removed from the code. BOs are simply pinned and unpinned in video
>> RAM.
> 
> Cursors are not that big after all, so maybe pin the two BOs for
> double-buffering permanently in vram to simplify things further?
> 
> Also factoring out the code which updates the two BOs to a separate
> function should help making the code more readable.

The cursor handling in the ast driver is similar, but uses one single BO
to hold both cursor buffers. I'm thinking about how to turn both
implementations into a generic helper for legacy cursors (i.e., low
number of colors or palette). This would also be helpful for my work on
fbdev porting.

One idea is to adapt deferred I/O. DRM would expose an ARGB shadow
buffer to userspace and let the mmap implementation update the HW
buffers (including dithering, palette setup, etc.).

Best regards
Thomas

> But even as-is the patch is a step into the right direction.
> 
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> 
> cheers,
>   Gerd
> 

-- 
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 6/8] drm/mgag200: Rewrite cursor handling

[Daniel Vetter]

On Tue, Jun 11, 2019 at 2:32 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 05.06.19 um 11:58 schrieb Gerd Hoffmann:
> > On Tue, Jun 04, 2019 at 05:41:59PM +0200, Thomas Zimmermann wrote:
> >> The cursor handling in mgag200 is complicated to understand. It touches a
> >> number of different BOs, but doesn't really use all of them.
> >>
> >> Rewriting the cursor update reduces the amount of cursor state. There are
> >> two BOs for double-buffered HW updates. The source BO updates the one that
> >> is currently not displayed and then switches buffers. Explicit BO locking
> >> has been removed from the code. BOs are simply pinned and unpinned in video
> >> RAM.
> >
> > Cursors are not that big after all, so maybe pin the two BOs for
> > double-buffering permanently in vram to simplify things further?
> >
> > Also factoring out the code which updates the two BOs to a separate
> > function should help making the code more readable.
>
> The cursor handling in the ast driver is similar, but uses one single BO
> to hold both cursor buffers. I'm thinking about how to turn both
> implementations into a generic helper for legacy cursors (i.e., low
> number of colors or palette). This would also be helpful for my work on
> fbdev porting.
>
> One idea is to adapt deferred I/O. DRM would expose an ARGB shadow
> buffer to userspace and let the mmap implementation update the HW
> buffers (including dithering, palette setup, etc.).

No mmap games needed with kms, we expect userspace to give us an ioctl
call in all cases. fbdev is the legacy horrors that works differently.

So for cursors, assuming you have universal cursors, you just need to
update the shadowed copy in the prepare_fb plane hook. For
non-universal plane drivers you need to do that somewhere in your
set/move_cursor hooks (I think both of them). Aside: For non-cursors
there's also the dirtyfb ioctl, which serves the same purpose.

Cheers, Daniel

>
> Best regards
> Thomas
>
> > But even as-is the patch is a step into the right direction.
> >
> > Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> >
> > cheers,
> >   Gerd
> >
>
> --
> 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 6/8] drm/mgag200: Rewrite cursor handling

[Thomas Zimmermann]

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

Hi

Am 11.06.19 um 17:33 schrieb Daniel Vetter:
> On Tue, Jun 11, 2019 at 2:32 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> Hi
>>
>> Am 05.06.19 um 11:58 schrieb Gerd Hoffmann:
>>> On Tue, Jun 04, 2019 at 05:41:59PM +0200, Thomas Zimmermann wrote:
>>>> The cursor handling in mgag200 is complicated to understand. It touches a
>>>> number of different BOs, but doesn't really use all of them.
>>>>
>>>> Rewriting the cursor update reduces the amount of cursor state. There are
>>>> two BOs for double-buffered HW updates. The source BO updates the one that
>>>> is currently not displayed and then switches buffers. Explicit BO locking
>>>> has been removed from the code. BOs are simply pinned and unpinned in video
>>>> RAM.
>>>
>>> Cursors are not that big after all, so maybe pin the two BOs for
>>> double-buffering permanently in vram to simplify things further?
>>>
>>> Also factoring out the code which updates the two BOs to a separate
>>> function should help making the code more readable.
>>
>> The cursor handling in the ast driver is similar, but uses one single BO
>> to hold both cursor buffers. I'm thinking about how to turn both
>> implementations into a generic helper for legacy cursors (i.e., low
>> number of colors or palette). This would also be helpful for my work on
>> fbdev porting.
>>
>> One idea is to adapt deferred I/O. DRM would expose an ARGB shadow
>> buffer to userspace and let the mmap implementation update the HW
>> buffers (including dithering, palette setup, etc.).
> 
> No mmap games needed with kms, we expect userspace to give us an ioctl
> call in all cases. fbdev is the legacy horrors that works differently.

Thanks for clarifying this. Conversion should be much easier this way. I
saw the dirty callback and the DIRTYFB ioctl, but I don't saw anything
in Weston that calls it. So I assumed that it's obsolete or optional.

Best regards
Thomas

> So for cursors, assuming you have universal cursors, you just need to
> update the shadowed copy in the prepare_fb plane hook. For
> non-universal plane drivers you need to do that somewhere in your
> set/move_cursor hooks (I think both of them). Aside: For non-cursors
> there's also the dirtyfb ioctl, which serves the same purpose.
> 
> Cheers, Daniel
> 
>>
>> Best regards
>> Thomas
>>
>>> But even as-is the patch is a step into the right direction.
>>>
>>> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
>>>
>>> cheers,
>>>   Gerd
>>>
>>
>> --
>> 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)
>>
> 
> 

-- 
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 6/8] drm/mgag200: Rewrite cursor handling

[Daniel Vetter]

On Wed, Jun 12, 2019 at 09:27:21AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 11.06.19 um 17:33 schrieb Daniel Vetter:
> > On Tue, Jun 11, 2019 at 2:32 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >>
> >> Hi
> >>
> >> Am 05.06.19 um 11:58 schrieb Gerd Hoffmann:
> >>> On Tue, Jun 04, 2019 at 05:41:59PM +0200, Thomas Zimmermann wrote:
> >>>> The cursor handling in mgag200 is complicated to understand. It touches a
> >>>> number of different BOs, but doesn't really use all of them.
> >>>>
> >>>> Rewriting the cursor update reduces the amount of cursor state. There are
> >>>> two BOs for double-buffered HW updates. The source BO updates the one that
> >>>> is currently not displayed and then switches buffers. Explicit BO locking
> >>>> has been removed from the code. BOs are simply pinned and unpinned in video
> >>>> RAM.
> >>>
> >>> Cursors are not that big after all, so maybe pin the two BOs for
> >>> double-buffering permanently in vram to simplify things further?
> >>>
> >>> Also factoring out the code which updates the two BOs to a separate
> >>> function should help making the code more readable.
> >>
> >> The cursor handling in the ast driver is similar, but uses one single BO
> >> to hold both cursor buffers. I'm thinking about how to turn both
> >> implementations into a generic helper for legacy cursors (i.e., low
> >> number of colors or palette). This would also be helpful for my work on
> >> fbdev porting.
> >>
> >> One idea is to adapt deferred I/O. DRM would expose an ARGB shadow
> >> buffer to userspace and let the mmap implementation update the HW
> >> buffers (including dithering, palette setup, etc.).
> > 
> > No mmap games needed with kms, we expect userspace to give us an ioctl
> > call in all cases. fbdev is the legacy horrors that works differently.
> 
> Thanks for clarifying this. Conversion should be much easier this way. I
> saw the dirty callback and the DIRTYFB ioctl, but I don't saw anything
> in Weston that calls it. So I assumed that it's obsolete or optional.

It's not optional nor obsolete, but you only need to call dirtyfb if you
do frontbuffer rendering. Which weston doesnt do. As long as you pageflip,
the pageflip will make sure the buffer contents gets updated.
-Daniel

> 
> Best regards
> Thomas
> 
> > So for cursors, assuming you have universal cursors, you just need to
> > update the shadowed copy in the prepare_fb plane hook. For
> > non-universal plane drivers you need to do that somewhere in your
> > set/move_cursor hooks (I think both of them). Aside: For non-cursors
> > there's also the dirtyfb ioctl, which serves the same purpose.
> > 
> > Cheers, Daniel
> > 
> >>
> >> Best regards
> >> Thomas
> >>
> >>> But even as-is the patch is a step into the right direction.
> >>>
> >>> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> >>>
> >>> cheers,
> >>>   Gerd
> >>>
> >>
> >> --
> >> 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)
> >>
> > 
> > 
> 
> -- 
> 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
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 7/8] drm: Remove lock interfaces from GEM VRAM helpers

[Thomas Zimmermann]

The lock functions and the locked-pin/unpin functions of GEM VRAM are not
requried any longer. Remove them.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_vram_helper.c | 109 --------------------------
 include/drm/drm_gem_vram_helper.h     |   5 --
 2 files changed, 114 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index 42ad80888df7..3dfccdb744da 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -151,36 +151,6 @@ void drm_gem_vram_put(struct drm_gem_vram_object *gbo)
 }
 EXPORT_SYMBOL(drm_gem_vram_put);
 
-/**
- * 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
- *
- * See ttm_bo_reserve() for more information.
- *
- * Returns:
- * 0 on success, or
- * a negative error code otherwise
- */
-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_lock);
-
-/**
- * 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_unlock(struct drm_gem_vram_object *gbo)
-{
-	ttm_bo_unreserve(&gbo->bo);
-}
-EXPORT_SYMBOL(drm_gem_vram_unlock);
-
 /**
  * drm_gem_vram_mmap_offset() - Returns a GEM VRAM object's mmap offset
  * @gbo:	the GEM VRAM object
@@ -262,49 +232,6 @@ 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_locked() - Pins a GEM VRAM object in a region.
- * @gbo:	the GEM VRAM object
- * @pl_flag:	a bitmask of possible memory regions
- *
- * Pinning a buffer object ensures that it is not evicted from
- * a memory region. A pinned buffer object has to be unpinned before
- * it can be pinned to another region.
- *
- * This function pins a GEM VRAM object that has already been
- * locked. Use drm_gem_vram_pin() if possible.
- *
- * Returns:
- * 0 on success, or
- * a negative error code otherwise.
- */
-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 };
-
-	lockdep_assert_held(&gbo->bo.resv->lock.base);
-
-	if (gbo->pin_count) {
-		++gbo->pin_count;
-		return 0;
-	}
-
-	drm_gem_vram_placement(gbo, pl_flag);
-	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 < 0)
-		return ret;
-
-	gbo->pin_count = 1;
-
-	return 0;
-}
-EXPORT_SYMBOL(drm_gem_vram_pin_locked);
-
 /**
  * drm_gem_vram_unpin() - Unpins a GEM VRAM object
  * @gbo:	the GEM VRAM object
@@ -347,42 +274,6 @@ int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo)
 }
 EXPORT_SYMBOL(drm_gem_vram_unpin);
 
-/**
- * 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
- * locked. Use drm_gem_vram_unpin() if possible.
- *
- * Returns:
- * 0 on success, or
- * a negative error code otherwise.
- */
-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;
-
-	--gbo->pin_count;
-	if (gbo->pin_count)
-		return 0;
-
-	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 < 0)
-		return ret;
-
-	return 0;
-}
-EXPORT_SYMBOL(drm_gem_vram_unpin_locked);
-
 /**
  * drm_gem_vram_kmap_at() - Maps a GEM VRAM object into kernel address space
  * @gbo:	the GEM VRAM object
diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
index 4d1d2c1bf32b..1d4aa87f8dfa 100644
--- a/include/drm/drm_gem_vram_helper.h
+++ b/include/drm/drm_gem_vram_helper.h
@@ -77,15 +77,10 @@ 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_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_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_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 8/8] drm: Remove functions with kmap-object argument from GEM VRAM helpers

[Thomas Zimmermann]

The GEM VRAM functions with kmap-object argument are not requried any
longer. Remove them.

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

diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index 3dfccdb744da..4a5ff274289e 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -275,12 +275,11 @@ int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo)
 EXPORT_SYMBOL(drm_gem_vram_unpin);
 
 /**
- * drm_gem_vram_kmap_at() - Maps a GEM VRAM object into kernel address space
+ * drm_gem_vram_kmap() - Maps a GEM VRAM object into kernel address space
  * @gbo:	the GEM VRAM object
  * @map:	establish a mapping if necessary
  * @is_iomem:	returns true if the mapped memory is I/O memory, or false \
 	otherwise; can be NULL
- * @kmap:	the mapping's kmap object
  *
  * This function maps the buffer object into the kernel's address space
  * or returns the current mapping. If the parameter map is false, the
@@ -292,10 +291,11 @@ EXPORT_SYMBOL(drm_gem_vram_unpin);
  * NULL if not mapped, or
  * an ERR_PTR()-encoded error code otherwise.
  */
-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,
+			bool *is_iomem)
 {
 	int ret;
+	struct ttm_bo_kmap_obj *kmap = &gbo->kmap;
 
 	if (kmap->virtual || !map)
 		goto out;
@@ -313,56 +313,22 @@ void *drm_gem_vram_kmap_at(struct drm_gem_vram_object *gbo, bool map,
 	}
 	return ttm_kmap_obj_virtual(kmap, is_iomem);
 }
-EXPORT_SYMBOL(drm_gem_vram_kmap_at);
-
-/**
- * drm_gem_vram_kmap() - Maps a GEM VRAM object into kernel address space
- * @gbo:	the GEM VRAM object
- * @map:	establish a mapping if necessary
- * @is_iomem:	returns true if the mapped memory is I/O memory, or false \
-	otherwise; can be NULL
- *
- * This function maps the buffer object into the kernel's address space
- * or returns the current mapping. If the parameter map is false, the
- * function only queries the current mapping, but does not establish a
- * new one.
- *
- * Returns:
- * The buffers virtual address if mapped, or
- * NULL if not mapped, or
- * an ERR_PTR()-encoded error code otherwise.
- */
-void *drm_gem_vram_kmap(struct drm_gem_vram_object *gbo, bool map,
-			bool *is_iomem)
-{
-	return drm_gem_vram_kmap_at(gbo, map, is_iomem, &gbo->kmap);
-}
 EXPORT_SYMBOL(drm_gem_vram_kmap);
 
 /**
- * drm_gem_vram_kunmap_at() - Unmaps a GEM VRAM object
+ * drm_gem_vram_kunmap() - Unmaps a GEM VRAM object
  * @gbo:	the GEM VRAM object
- * @kmap:	the mapping's kmap object
  */
-void drm_gem_vram_kunmap_at(struct drm_gem_vram_object *gbo,
-			    struct ttm_bo_kmap_obj *kmap)
+void drm_gem_vram_kunmap(struct drm_gem_vram_object *gbo)
 {
+	struct ttm_bo_kmap_obj *kmap = &gbo->kmap;
+
 	if (!kmap->virtual)
 		return;
 
 	ttm_bo_kunmap(kmap);
 	kmap->virtual = NULL;
 }
-EXPORT_SYMBOL(drm_gem_vram_kunmap_at);
-
-/**
- * drm_gem_vram_kunmap() - Unmaps a GEM VRAM object
- * @gbo:	the GEM VRAM object
- */
-void drm_gem_vram_kunmap(struct drm_gem_vram_object *gbo)
-{
-	drm_gem_vram_kunmap_at(gbo, &gbo->kmap);
-}
 EXPORT_SYMBOL(drm_gem_vram_kunmap);
 
 /**
diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
index 1d4aa87f8dfa..9581ea0a4f7e 100644
--- a/include/drm/drm_gem_vram_helper.h
+++ b/include/drm/drm_gem_vram_helper.h
@@ -81,12 +81,8 @@ 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_unpin(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,
 			bool *is_iomem);
-void drm_gem_vram_kunmap_at(struct drm_gem_vram_object *gbo,
-			    struct ttm_bo_kmap_obj *kmap);
 void drm_gem_vram_kunmap(struct drm_gem_vram_object *gbo);
 
 int drm_gem_vram_fill_create_dumb(struct drm_file *file,
-- 
2.21.0

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