(was: drm/vram: Add VRAM buffers for HW cursors)
This patchset cleans up the memory management of HW cursors in ast and mgag200. It further moves the allocated cursor BOs to the of the video RAM to reduce memory fragmentation.
The ast and mgag200 drivers support HW cursors of uncommon pixel formats. Both drivers manage cursor memory in dedicated GEM VRAM buffer objects with a double buffering scheme; either by alternating between two GEM BOs, or by alternating between offsets within the same GEM BO. The code is convoluted and can lead to memory fragmentation if the BO is stored the middle of VRAM. This is especially a problem on devices with only a small amount of video memory.
With this patchset, the cursor handling in ast and mgag200 is first split up into separate functions for copying cursor images, managing buffer objects, setting scanout addresses, and moving and hiding the cursor. Furthermore, each driver dedicates a few KiB at the high end of each device's video memory to storing the cursor's buffer objects. This reduces memory fragmentation, which may prevent displaying the framebuffer on low-memory devices.
The patchset has been tested on ast and mgag200 hardware.
v2: * remove VRAM buffers in favor of GEM BOs * manage BO placement with pin flag
Thomas Zimmermann (12): drm/vram: Support top-down placement flag drm/ast: Don't call ast_show_cursor() from ast_cursor_move() drm/ast: Move cursor update code to ast_show_cursor() drm/ast: Move cursor offset swapping into ast_show_cursor() drm/ast: Allocate cursor BOs at high end of video memory drm/mgag200: Rename cursor functions to use mgag200_ prefix drm/mgag200: Add init and fini functions for cursor handling drm/mgag200: Add separate move-cursor function drm/mgag200: Move cursor-image update to mgag200_show_cursor() drm/mgag200: Move cursor BO swapping into mgag200_show_cursor() drm/mgag200: Reserve video memory for cursor plane drm/mgag200: Allocate cursor BOs at high end of video memory
drivers/gpu/drm/ast/ast_drv.h | 43 +-- drivers/gpu/drm/ast/ast_mode.c | 235 +++++++++-------- drivers/gpu/drm/drm_gem_vram_helper.c | 14 +- drivers/gpu/drm/mgag200/mgag200_cursor.c | 316 ++++++++++++++--------- drivers/gpu/drm/mgag200/mgag200_drv.h | 22 +- drivers/gpu/drm/mgag200/mgag200_main.c | 20 +- drivers/gpu/drm/mgag200/mgag200_mode.c | 6 +- drivers/gpu/drm/mgag200/mgag200_ttm.c | 4 + include/drm/drm_gem_vram_helper.h | 1 + 9 files changed, 387 insertions(+), 274 deletions(-)
-- 2.23.0
Pinning lots of small buffer objects, such as cursors or sprites, to video memory can lead to fragmentation, which is a problem for devices with only a small amount of memory. As a result, framebuffer images might not get pinned, even though there's enough space available overall.
The flag DRM_GEM_VRAM_PL_FLAG_TOPDOWN marks buffer objects to be pinned at the high end of video memory. This leaves contiguous space available at the memory's low end.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/drm_gem_vram_helper.c | 14 ++++++++++---- include/drm/drm_gem_vram_helper.h | 1 + 2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index 49588de88959..91baf3f279f1 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -58,6 +58,7 @@ static void drm_gem_vram_placement(struct drm_gem_vram_object *gbo, { unsigned int i; unsigned int c = 0; + u32 invariant_flags = pl_flag & TTM_PL_FLAG_TOPDOWN;
gbo->placement.placement = gbo->placements; gbo->placement.busy_placement = gbo->placements; @@ -65,15 +66,18 @@ static void drm_gem_vram_placement(struct drm_gem_vram_object *gbo, if (pl_flag & TTM_PL_FLAG_VRAM) gbo->placements[c++].flags = TTM_PL_FLAG_WC | TTM_PL_FLAG_UNCACHED | - TTM_PL_FLAG_VRAM; + TTM_PL_FLAG_VRAM | + invariant_flags;
if (pl_flag & TTM_PL_FLAG_SYSTEM) gbo->placements[c++].flags = TTM_PL_MASK_CACHING | - TTM_PL_FLAG_SYSTEM; + TTM_PL_FLAG_SYSTEM | + invariant_flags;
if (!c) gbo->placements[c++].flags = TTM_PL_MASK_CACHING | - TTM_PL_FLAG_SYSTEM; + TTM_PL_FLAG_SYSTEM | + invariant_flags;
gbo->placement.num_placement = c; gbo->placement.num_busy_placement = c; @@ -236,7 +240,9 @@ static int drm_gem_vram_pin_locked(struct drm_gem_vram_object *gbo, * a memory region. A pinned buffer object has to be unpinned before * it can be pinned to another region. If the pl_flag argument is 0, * the buffer is pinned at its current location (video RAM or system - * memory). + * memory). The modifier DRM_GEM_VRAM_PL_FLAG_TOPDOWN marks the buffer + * object to be pinned at the high end of the memory region. Set this + * flag to avoid memory fragmentation. * * Returns: * 0 on success, or diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h index 418eb1122861..354a9cd358a3 100644 --- a/include/drm/drm_gem_vram_helper.h +++ b/include/drm/drm_gem_vram_helper.h @@ -19,6 +19,7 @@ struct vm_area_struct;
#define DRM_GEM_VRAM_PL_FLAG_VRAM TTM_PL_FLAG_VRAM #define DRM_GEM_VRAM_PL_FLAG_SYSTEM TTM_PL_FLAG_SYSTEM +#define DRM_GEM_VRAM_PL_FLAG_TOPDOWN TTM_PL_FLAG_TOPDOWN
/* * Buffer-object helpers
Hi,
- object to be pinned at the high end of the memory region. Set this
- flag to avoid memory fragmentation.
That is confusing, sounds like the flag should be set on all objects which is not correct. The description from the cover letter is better.
Otherwise the patch is fine, so with that fixed: Reviewed-by: Gerd Hoffmann kraxel@redhat.com
cheers, Gerd
Separating the cursor's move() function from the show() function in preparation of further rework of the cursor update code.
'Showing' the cursor from within the move() function is required to update the cursor position.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/ast/ast_mode.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 6caa6ebfeaa8..a4cbf2d5ee0a 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -1236,6 +1236,7 @@ static int ast_cursor_move(struct drm_crtc *crtc, struct ast_private *ast = crtc->dev->dev_private; int x_offset, y_offset; u8 *sig; + u8 jreg;
sig = drm_gem_vram_kmap(drm_gem_vram_of_gem(ast->cursor_cache), false, NULL); @@ -1262,7 +1263,9 @@ static int ast_cursor_move(struct drm_crtc *crtc, ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xc7, ((y >> 8) & 0x07));
/* dummy write to fire HWC */ - ast_show_cursor(crtc); + jreg = 0x02 | + 0x01; /* enable ARGB4444 cursor */ + ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xcb, 0xfc, jreg);
return 0; }
A call to ast's show-cursor function now receives the cursor image and updates the buffer. The change splits off image update and base-address update into separate functions.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/ast/ast_mode.c | 120 +++++++++++++++++++-------------- 1 file changed, 69 insertions(+), 51 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index a4cbf2d5ee0a..1294f0612fd5 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -1064,23 +1064,6 @@ static void ast_i2c_destroy(struct ast_i2c_chan *i2c) kfree(i2c); }
-static void ast_show_cursor(struct drm_crtc *crtc) -{ - struct ast_private *ast = crtc->dev->dev_private; - u8 jreg; - - jreg = 0x2; - /* enable ARGB cursor */ - jreg |= 1; - ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xcb, 0xfc, jreg); -} - -static void ast_hide_cursor(struct drm_crtc *crtc) -{ - struct ast_private *ast = crtc->dev->dev_private; - ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xcb, 0xfc, 0x00); -} - static u32 copy_cursor_image(u8 *src, u8 *dst, int width, int height) { union { @@ -1137,6 +1120,72 @@ static u32 copy_cursor_image(u8 *src, u8 *dst, int width, int height) return csum; }
+static int ast_cursor_update(void *dst, void *src, unsigned int width, + unsigned int height) +{ + u32 csum; + + /* do data transfer to cursor cache */ + csum = copy_cursor_image(src, dst, width, height); + + /* write checksum + signature */ + dst += AST_HWC_SIZE; + writel(csum, dst); + writel(width, dst + AST_HWC_SIGNATURE_SizeX); + writel(height, dst + AST_HWC_SIGNATURE_SizeY); + writel(0, dst + AST_HWC_SIGNATURE_HOTSPOTX); + writel(0, dst + AST_HWC_SIGNATURE_HOTSPOTY); + + return 0; +} + +static void ast_cursor_set_base(struct ast_private *ast, u64 address) +{ + u8 addr0 = (address >> 3) & 0xff; + u8 addr1 = (address >> 11) & 0xff; + u8 addr2 = (address >> 19) & 0xff; + + ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xc8, addr0); + ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xc9, addr1); + ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xca, addr2); +} + +static int ast_show_cursor(struct drm_crtc *crtc, void *dst, void *src, + unsigned int width, unsigned int height, + u64 dst_gpu) +{ + struct ast_private *ast = crtc->dev->dev_private; + struct ast_crtc *ast_crtc = to_ast_crtc(crtc); + int ret; + u8 jreg; + + dst += (AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE)*ast->next_cursor; + + ret = ast_cursor_update(dst, src, width, height); + if (ret) + return ret; + ast_cursor_set_base(ast, dst_gpu); + + ast->next_cursor = (ast->next_cursor + 1) % AST_DEFAULT_HWC_NUM; + + ast_crtc->offset_x = AST_MAX_HWC_WIDTH - width; + ast_crtc->offset_y = AST_MAX_HWC_WIDTH - height; + + jreg = 0x2; + /* enable ARGB cursor */ + jreg |= 1; + ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xcb, 0xfc, jreg); + + return 0; +} + +static void ast_hide_cursor(struct drm_crtc *crtc) +{ + struct ast_private *ast = crtc->dev->dev_private; + + ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xcb, 0xfc, 0x00); +} + static int ast_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv, uint32_t handle, @@ -1144,12 +1193,9 @@ static int ast_cursor_set(struct drm_crtc *crtc, uint32_t height) { struct ast_private *ast = crtc->dev->dev_private; - struct ast_crtc *ast_crtc = to_ast_crtc(crtc); struct drm_gem_object *obj; struct drm_gem_vram_object *gbo; s64 dst_gpu; - u64 gpu_addr; - u32 csum; int ret; u8 *src, *dst;
@@ -1185,37 +1231,9 @@ static int ast_cursor_set(struct drm_crtc *crtc, goto err_drm_gem_vram_vunmap; }
- dst += (AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE)*ast->next_cursor; - - /* do data transfer to cursor cache */ - csum = copy_cursor_image(src, dst, width, height); - - /* write checksum + signature */ - { - 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); - writel(height, dst + AST_HWC_SIGNATURE_SizeY); - writel(0, dst + AST_HWC_SIGNATURE_HOTSPOTX); - writel(0, dst + AST_HWC_SIGNATURE_HOTSPOTY); - - /* set pattern offset */ - 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->offset_x = AST_MAX_HWC_WIDTH - width; - ast_crtc->offset_y = AST_MAX_HWC_WIDTH - height; - - ast->next_cursor = (ast->next_cursor + 1) % AST_DEFAULT_HWC_NUM; - - ast_show_cursor(crtc); + ret = ast_show_cursor(crtc, dst, src, width, height, dst_gpu); + if (ret) + goto err_drm_gem_vram_kunmap;
drm_gem_vram_vunmap(gbo, src); drm_gem_object_put_unlocked(obj);
Hi,
-static void ast_hide_cursor(struct drm_crtc *crtc) -{
- struct ast_private *ast = crtc->dev->dev_private;
- ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xcb, 0xfc, 0x00);
-}
+static void ast_hide_cursor(struct drm_crtc *crtc) +{
- struct ast_private *ast = crtc->dev->dev_private;
- ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xcb, 0xfc, 0x00);
+}
Please split this into two patches, one which only moves the functions and one which does the code changes. Then it is easier to see the actual code changes for ast_{show,hide}_cursor in the diff output of the second patch.
cheers, Gerd
Selecting the correct offset for the new cursor image is not relevant outside of ast_show_cursor(). Let the function do the work.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/ast/ast_mode.c | 57 ++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 1294f0612fd5..6a517ffb1c5c 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -1150,23 +1150,34 @@ static void ast_cursor_set_base(struct ast_private *ast, u64 address) ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xca, addr2); }
-static int ast_show_cursor(struct drm_crtc *crtc, void *dst, void *src, - unsigned int width, unsigned int height, - u64 dst_gpu) +static int ast_show_cursor(struct drm_crtc *crtc, void *src, + unsigned int width, unsigned int height) { struct ast_private *ast = crtc->dev->dev_private; struct ast_crtc *ast_crtc = to_ast_crtc(crtc); + struct drm_gem_vram_object *gbo; + u8 *dst, *dst_next; + s64 off; int ret; u8 jreg;
- dst += (AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE)*ast->next_cursor; + gbo = drm_gem_vram_of_gem(ast->cursor_cache); + dst = drm_gem_vram_vmap(gbo); + if (IS_ERR(dst)) + return PTR_ERR(dst); + off = drm_gem_vram_offset(gbo); + if (off < 0) { + ret = (int)off; + goto err_drm_gem_vram_vunmap; + }
- ret = ast_cursor_update(dst, src, width, height); - if (ret) - return ret; - ast_cursor_set_base(ast, dst_gpu); + dst_next = dst + (AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE) * + ast->next_cursor;
- ast->next_cursor = (ast->next_cursor + 1) % AST_DEFAULT_HWC_NUM; + ret = ast_cursor_update(dst_next, src, width, height); + if (ret) + goto err_drm_gem_vram_vunmap; + ast_cursor_set_base(ast, off);
ast_crtc->offset_x = AST_MAX_HWC_WIDTH - width; ast_crtc->offset_y = AST_MAX_HWC_WIDTH - height; @@ -1176,7 +1187,15 @@ static int ast_show_cursor(struct drm_crtc *crtc, void *dst, void *src, jreg |= 1; ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xcb, 0xfc, jreg);
+ ast->next_cursor = (ast->next_cursor + 1) % AST_DEFAULT_HWC_NUM; + + drm_gem_vram_vunmap(gbo, dst); + return 0; + +err_drm_gem_vram_vunmap: + drm_gem_vram_vunmap(gbo, dst); + return ret; }
static void ast_hide_cursor(struct drm_crtc *crtc) @@ -1192,12 +1211,10 @@ static int ast_cursor_set(struct drm_crtc *crtc, uint32_t width, uint32_t height) { - struct ast_private *ast = crtc->dev->dev_private; struct drm_gem_object *obj; struct drm_gem_vram_object *gbo; - s64 dst_gpu; + u8 *src; int ret; - u8 *src, *dst;
if (!handle) { ast_hide_cursor(crtc); @@ -1219,21 +1236,9 @@ static int ast_cursor_set(struct drm_crtc *crtc, goto err_drm_gem_object_put_unlocked; }
- dst = drm_gem_vram_kmap(drm_gem_vram_of_gem(ast->cursor_cache), - false, NULL); - if (IS_ERR(dst)) { - ret = PTR_ERR(dst); - goto err_drm_gem_vram_vunmap; - } - dst_gpu = drm_gem_vram_offset(drm_gem_vram_of_gem(ast->cursor_cache)); - if (dst_gpu < 0) { - ret = (int)dst_gpu; - goto err_drm_gem_vram_vunmap; - } - - ret = ast_show_cursor(crtc, dst, src, width, height, dst_gpu); + ret = ast_show_cursor(crtc, src, width, height); if (ret) - goto err_drm_gem_vram_kunmap; + goto err_drm_gem_vram_vunmap;
drm_gem_vram_vunmap(gbo, src); drm_gem_object_put_unlocked(obj);
By putting cursor BOs at the high end of the video memory, we can avoid memory fragmentation. Starting at the low end, contiguous video memory is available for framebuffers.
The patch also simplifies the buffer swapping by splitting struct ast_private.cursor_cache BO into two separate boffer objects. Cursor images alternate between these buffers instead of offsets within cursor_cache.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/ast/ast_drv.h | 43 +++++++++------- drivers/gpu/drm/ast/ast_mode.c | 91 ++++++++++++++++++---------------- 2 files changed, 73 insertions(+), 61 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h index 244cc7c382af..fc35163c7541 100644 --- a/drivers/gpu/drm/ast/ast_drv.h +++ b/drivers/gpu/drm/ast/ast_drv.h @@ -82,6 +82,25 @@ enum ast_tx_chip { #define AST_DRAM_4Gx16 7 #define AST_DRAM_8Gx16 8
+ +#define AST_MAX_HWC_WIDTH 64 +#define AST_MAX_HWC_HEIGHT 64 + +#define AST_HWC_SIZE (AST_MAX_HWC_WIDTH * AST_MAX_HWC_HEIGHT * 2) +#define AST_HWC_SIGNATURE_SIZE 32 + +#define AST_DEFAULT_HWC_NUM 2 + +/* define for signature structure */ +#define AST_HWC_SIGNATURE_CHECKSUM 0x00 +#define AST_HWC_SIGNATURE_SizeX 0x04 +#define AST_HWC_SIGNATURE_SizeY 0x08 +#define AST_HWC_SIGNATURE_X 0x0C +#define AST_HWC_SIGNATURE_Y 0x10 +#define AST_HWC_SIGNATURE_HOTSPOTX 0x14 +#define AST_HWC_SIGNATURE_HOTSPOTY 0x18 + + struct ast_private { struct drm_device *dev;
@@ -97,8 +116,11 @@ struct ast_private {
int fb_mtrr;
- struct drm_gem_object *cursor_cache; - int next_cursor; + struct { + struct drm_gem_vram_object *gbo[AST_DEFAULT_HWC_NUM]; + unsigned int next_index; + } cursor; + bool support_wide_screen; enum { ast_use_p2a, @@ -199,23 +221,6 @@ static inline void ast_open_key(struct ast_private *ast)
#define AST_VIDMEM_DEFAULT_SIZE AST_VIDMEM_SIZE_8M
-#define AST_MAX_HWC_WIDTH 64 -#define AST_MAX_HWC_HEIGHT 64 - -#define AST_HWC_SIZE (AST_MAX_HWC_WIDTH*AST_MAX_HWC_HEIGHT*2) -#define AST_HWC_SIGNATURE_SIZE 32 - -#define AST_DEFAULT_HWC_NUM 2 -/* define for signature structure */ -#define AST_HWC_SIGNATURE_CHECKSUM 0x00 -#define AST_HWC_SIGNATURE_SizeX 0x04 -#define AST_HWC_SIGNATURE_SizeY 0x08 -#define AST_HWC_SIGNATURE_X 0x0C -#define AST_HWC_SIGNATURE_Y 0x10 -#define AST_HWC_SIGNATURE_HOTSPOTX 0x14 -#define AST_HWC_SIGNATURE_HOTSPOTY 0x18 - - struct ast_i2c_chan { struct i2c_adapter adapter; struct drm_device *dev; diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 6a517ffb1c5c..b13eaa2619ab 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -883,50 +883,53 @@ static int ast_connector_init(struct drm_device *dev) static int ast_cursor_init(struct drm_device *dev) { struct ast_private *ast = dev->dev_private; - int size; - int ret; - struct drm_gem_object *obj; + size_t size, i; struct drm_gem_vram_object *gbo; - s64 gpu_addr; - void *base; + int ret;
- size = (AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE) * AST_DEFAULT_HWC_NUM; + size = roundup(AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE, PAGE_SIZE);
- ret = ast_gem_create(dev, size, true, &obj); - if (ret) - return ret; - gbo = drm_gem_vram_of_gem(obj); - ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM); - if (ret) - goto fail; - gpu_addr = drm_gem_vram_offset(gbo); - if (gpu_addr < 0) { - drm_gem_vram_unpin(gbo); - ret = (int)gpu_addr; - goto fail; - } + for (i = 0; i < ARRAY_SIZE(ast->cursor.gbo); ++i) { + gbo = drm_gem_vram_create(dev, &dev->vram_mm->bdev, + size, 0, false); + if (IS_ERR(gbo)) { + ret = PTR_ERR(gbo); + goto err_drm_gem_vram_put; + } + ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM | + DRM_GEM_VRAM_PL_FLAG_TOPDOWN); + if (ret) { + drm_gem_vram_put(gbo); + goto err_drm_gem_vram_put; + }
- /* kmap the object */ - base = drm_gem_vram_kmap(gbo, true, NULL); - if (IS_ERR(base)) { - ret = PTR_ERR(base); - goto fail; + ast->cursor.gbo[i] = gbo; }
- ast->cursor_cache = obj; return 0; -fail: + +err_drm_gem_vram_put: + while (i) { + --i; + gbo = ast->cursor.gbo[i]; + drm_gem_vram_unpin(gbo); + drm_gem_vram_put(gbo); + ast->cursor.gbo[i] = NULL; + } return ret; }
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(gbo); - drm_gem_vram_unpin(gbo); - drm_gem_object_put_unlocked(ast->cursor_cache); + size_t i; + struct drm_gem_vram_object *gbo; + + for (i = 0; i < ARRAY_SIZE(ast->cursor.gbo); ++i) { + gbo = ast->cursor.gbo[i]; + drm_gem_vram_unpin(gbo); + drm_gem_vram_put(gbo); + } }
int ast_mode_init(struct drm_device *dev) @@ -1156,12 +1159,12 @@ static int ast_show_cursor(struct drm_crtc *crtc, void *src, struct ast_private *ast = crtc->dev->dev_private; struct ast_crtc *ast_crtc = to_ast_crtc(crtc); struct drm_gem_vram_object *gbo; - u8 *dst, *dst_next; + void *dst; s64 off; int ret; u8 jreg;
- gbo = drm_gem_vram_of_gem(ast->cursor_cache); + gbo = ast->cursor.gbo[ast->cursor.next_index]; dst = drm_gem_vram_vmap(gbo); if (IS_ERR(dst)) return PTR_ERR(dst); @@ -1171,10 +1174,7 @@ static int ast_show_cursor(struct drm_crtc *crtc, void *src, goto err_drm_gem_vram_vunmap; }
- dst_next = dst + (AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE) * - ast->next_cursor; - - ret = ast_cursor_update(dst_next, src, width, height); + ret = ast_cursor_update(dst, src, width, height); if (ret) goto err_drm_gem_vram_vunmap; ast_cursor_set_base(ast, off); @@ -1187,7 +1187,8 @@ static int ast_show_cursor(struct drm_crtc *crtc, void *src, jreg |= 1; ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xcb, 0xfc, jreg);
- ast->next_cursor = (ast->next_cursor + 1) % AST_DEFAULT_HWC_NUM; + ++ast->cursor.next_index; + ast->cursor.next_index %= ARRAY_SIZE(ast->cursor.gbo);
drm_gem_vram_vunmap(gbo, dst);
@@ -1257,13 +1258,17 @@ static int ast_cursor_move(struct drm_crtc *crtc, { struct ast_crtc *ast_crtc = to_ast_crtc(crtc); struct ast_private *ast = crtc->dev->dev_private; + struct drm_gem_vram_object *gbo; int x_offset, y_offset; - u8 *sig; + u8 *dst, *sig; u8 jreg;
- 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; + gbo = ast->cursor.gbo[ast->cursor.next_index]; + dst = drm_gem_vram_vmap(gbo); + if (IS_ERR(dst)) + return PTR_ERR(dst); + + sig = dst + AST_HWC_SIZE; writel(x, sig + AST_HWC_SIGNATURE_X); writel(y, sig + AST_HWC_SIGNATURE_Y);
@@ -1290,5 +1295,7 @@ static int ast_cursor_move(struct drm_crtc *crtc, 0x01; /* enable ARGB4444 cursor */ ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xcb, 0xfc, jreg);
+ drm_gem_vram_vunmap(gbo, dst); + return 0; }
Although the driver source code is fairly inconsistent wrt naming, the prefix should be mgag200. Rename cursor functions accordingly.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/mgag200/mgag200_cursor.c | 19 ++++++++----------- drivers/gpu/drm/mgag200/mgag200_drv.h | 6 +++--- drivers/gpu/drm/mgag200/mgag200_mode.c | 4 ++-- 3 files changed, 13 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c index 89f61573a497..3df70d86af21 100644 --- a/drivers/gpu/drm/mgag200/mgag200_cursor.c +++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c @@ -13,10 +13,10 @@ static bool warn_transparent = true; static bool warn_palette = true;
/* - Hide the cursor off screen. We can't disable the cursor hardware because it - takes too long to re-activate and causes momentary corruption -*/ -static void mga_hide_cursor(struct mga_device *mdev) + * Hide the cursor off screen. We can't disable the cursor hardware because + * it takes too long to re-activate and causes momentary corruption. + */ +static void mgag200_hide_cursor(struct mga_device *mdev) { WREG8(MGA_CURPOSXL, 0); WREG8(MGA_CURPOSXH, 0); @@ -25,11 +25,8 @@ static void mga_hide_cursor(struct mga_device *mdev) mdev->cursor.pixels_current = NULL; }
-int mga_crtc_cursor_set(struct drm_crtc *crtc, - struct drm_file *file_priv, - uint32_t handle, - uint32_t width, - uint32_t height) +int mgag200_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv, + uint32_t handle, uint32_t width, uint32_t height) { struct drm_device *dev = crtc->dev; struct mga_device *mdev = (struct mga_device *)dev->dev_private; @@ -66,7 +63,7 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc, }
if (!handle || !file_priv) { - mga_hide_cursor(mdev); + mgag200_hide_cursor(mdev); return 0; }
@@ -224,7 +221,7 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc, return ret; }
-int mga_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) +int mgag200_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) { struct mga_device *mdev = (struct mga_device *)crtc->dev->dev_private; /* Our origin is at (64,64) */ diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index 37c003ed57c0..5244e3fa4203 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -203,8 +203,8 @@ int mgag200_mm_init(struct mga_device *mdev); void mgag200_mm_fini(struct mga_device *mdev); int mgag200_mmap(struct file *filp, struct vm_area_struct *vma);
-int mga_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv, - uint32_t handle, uint32_t width, uint32_t height); -int mga_crtc_cursor_move(struct drm_crtc *crtc, int x, int y); +int mgag200_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv, + uint32_t handle, uint32_t width, uint32_t height); +int mgag200_crtc_cursor_move(struct drm_crtc *crtc, int x, int y);
#endif /* __MGAG200_DRV_H__ */ diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 68226556044b..0cf5608c3644 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -1413,8 +1413,8 @@ static void mga_crtc_disable(struct drm_crtc *crtc)
/* These provide the minimum set of functions required to handle a CRTC */ static const struct drm_crtc_funcs mga_crtc_funcs = { - .cursor_set = mga_crtc_cursor_set, - .cursor_move = mga_crtc_cursor_move, + .cursor_set = mgag200_crtc_cursor_set, + .cursor_move = mgag200_crtc_cursor_move, .gamma_set = mga_crtc_gamma_set, .set_config = drm_crtc_helper_set_config, .destroy = mga_crtc_destroy,
Moving the cursor initialization and cleanup into separate functions makes the overall code slightly more readable.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/mgag200/mgag200_cursor.c | 28 ++++++++++++++++++++++++ drivers/gpu/drm/mgag200/mgag200_drv.h | 2 ++ drivers/gpu/drm/mgag200/mgag200_main.c | 18 +++++---------- 3 files changed, 35 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c index 3df70d86af21..d39e2bc57a70 100644 --- a/drivers/gpu/drm/mgag200/mgag200_cursor.c +++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c @@ -25,6 +25,34 @@ static void mgag200_hide_cursor(struct mga_device *mdev) mdev->cursor.pixels_current = NULL; }
+int mgag200_cursor_init(struct mga_device *mdev) +{ + struct drm_device *dev = mdev->dev; + + /* + * Make small buffers to store a hardware cursor (double + * buffered icon updates) + */ + mdev->cursor.pixels_1 = drm_gem_vram_create(dev, &dev->vram_mm->bdev, + roundup(48*64, PAGE_SIZE), + 0, 0); + mdev->cursor.pixels_2 = drm_gem_vram_create(dev, &dev->vram_mm->bdev, + roundup(48*64, PAGE_SIZE), + 0, 0); + if (IS_ERR(mdev->cursor.pixels_2) || IS_ERR(mdev->cursor.pixels_1)) { + mdev->cursor.pixels_1 = NULL; + mdev->cursor.pixels_2 = NULL; + dev_warn(&dev->pdev->dev, + "Could not allocate space for cursors. Not doing hardware cursors.\n"); + } + mdev->cursor.pixels_current = NULL; + + return 0; +} + +void mgag200_cursor_fini(struct mga_device *mdev) +{ } + int mgag200_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv, uint32_t handle, uint32_t width, uint32_t height) { diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index 5244e3fa4203..01243fa6397c 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -203,6 +203,8 @@ int mgag200_mm_init(struct mga_device *mdev); void mgag200_mm_fini(struct mga_device *mdev); int mgag200_mmap(struct file *filp, struct vm_area_struct *vma);
+int mgag200_cursor_init(struct mga_device *mdev); +void mgag200_cursor_fini(struct mga_device *mdev); int mgag200_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv, uint32_t handle, uint32_t width, uint32_t height); int mgag200_crtc_cursor_move(struct drm_crtc *crtc, int x, int y); diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c index a9773334dedf..2b59280777a5 100644 --- a/drivers/gpu/drm/mgag200/mgag200_main.c +++ b/drivers/gpu/drm/mgag200/mgag200_main.c @@ -171,20 +171,10 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags) goto err_modeset; }
- /* Make small buffers to store a hardware cursor (double buffered icon updates) */ - mdev->cursor.pixels_1 = drm_gem_vram_create(dev, &dev->vram_mm->bdev, - roundup(48*64, PAGE_SIZE), - 0, 0); - mdev->cursor.pixels_2 = drm_gem_vram_create(dev, &dev->vram_mm->bdev, - roundup(48*64, PAGE_SIZE), - 0, 0); - if (IS_ERR(mdev->cursor.pixels_2) || IS_ERR(mdev->cursor.pixels_1)) { - mdev->cursor.pixels_1 = NULL; - mdev->cursor.pixels_2 = NULL; + r = mgag200_cursor_init(mdev); + if (r) dev_warn(&dev->pdev->dev, - "Could not allocate space for cursors. Not doing hardware cursors.\n"); - } - mdev->cursor.pixels_current = NULL; + "Could not initialize cursors. Not doing hardware cursors.\n");
r = drm_fbdev_generic_setup(mdev->dev, 0); if (r) @@ -194,6 +184,7 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
err_modeset: drm_mode_config_cleanup(dev); + mgag200_cursor_fini(mdev); mgag200_mm_fini(mdev); err_mm: dev->dev_private = NULL; @@ -209,6 +200,7 @@ void mgag200_driver_unload(struct drm_device *dev) return; mgag200_modeset_fini(mdev); drm_mode_config_cleanup(dev); + mgag200_cursor_fini(mdev); mgag200_mm_fini(mdev); dev->dev_private = NULL; }
Adding mgag200_move_cursor() makes the cursor code more consistent and will become handy when we move to universal cursor planes.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/mgag200/mgag200_cursor.c | 29 ++++++++++++++++-------- 1 file changed, 20 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c index d39e2bc57a70..621960723a3a 100644 --- a/drivers/gpu/drm/mgag200/mgag200_cursor.c +++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c @@ -25,6 +25,24 @@ static void mgag200_hide_cursor(struct mga_device *mdev) mdev->cursor.pixels_current = NULL; }
+static void mgag200_move_cursor(struct mga_device *mdev, int x, int y) +{ + if (WARN_ON(x <= 0)) + return; + if (WARN_ON(y <= 0)) + return; + if (WARN_ON(x & ~0xffff)) + return; + if (WARN_ON(y & ~0xffff)) + return; + + WREG8(MGA_CURPOSXL, x & 0xff); + WREG8(MGA_CURPOSXH, (x>>8) & 0xff); + + WREG8(MGA_CURPOSYL, y & 0xff); + WREG8(MGA_CURPOSYH, (y>>8) & 0xff); +} + int mgag200_cursor_init(struct mga_device *mdev) { struct drm_device *dev = mdev->dev; @@ -252,19 +270,12 @@ int mgag200_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv, int mgag200_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) { struct mga_device *mdev = (struct mga_device *)crtc->dev->dev_private; + /* Our origin is at (64,64) */ x += 64; y += 64;
- BUG_ON(x <= 0); - BUG_ON(y <= 0); - BUG_ON(x & ~0xffff); - BUG_ON(y & ~0xffff); + mgag200_move_cursor(mdev, x, y);
- WREG8(MGA_CURPOSXL, x & 0xff); - WREG8(MGA_CURPOSXH, (x>>8) & 0xff); - - WREG8(MGA_CURPOSYL, y & 0xff); - WREG8(MGA_CURPOSYH, (y>>8) & 0xff); return 0; }
Separating the management of buffer objects from updating the hardware cursor buffer gives the code more structure. While doing this, we can further split the image-update code into code for writing the buffer, setting the base scan-out address, and enabling the cursor. The first two operations are in dedicated functions update() and set_base().
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/mgag200/mgag200_cursor.c | 221 +++++++++++++---------- 1 file changed, 126 insertions(+), 95 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c index 621960723a3a..13daa0ce1c9e 100644 --- a/drivers/gpu/drm/mgag200/mgag200_cursor.c +++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c @@ -12,6 +12,128 @@ static bool warn_transparent = true; static bool warn_palette = true;
+static int mgag200_cursor_update(struct mga_device *mdev, void *dst, void *src, + unsigned int width, unsigned int height) +{ + struct drm_device *dev = mdev->dev; + unsigned int i, row, col; + uint32_t colour_set[16]; + uint32_t *next_space = &colour_set[0]; + uint32_t *palette_iter; + uint32_t this_colour; + bool found = false; + int colour_count = 0; + u8 reg_index; + u8 this_row[48]; + + memset(&colour_set[0], 0, sizeof(uint32_t)*16); + /* width*height*4 = 16384 */ + for (i = 0; i < 16384; i += 4) { + this_colour = ioread32(src + i); + /* No transparency */ + if (this_colour>>24 != 0xff && + this_colour>>24 != 0x0) { + if (warn_transparent) { + dev_info(&dev->pdev->dev, "Video card doesn't support cursors with partial transparency.\n"); + dev_info(&dev->pdev->dev, "Not enabling hardware cursor.\n"); + warn_transparent = false; /* Only tell the user once. */ + } + return -EINVAL; + } + /* Don't need to store transparent pixels as colours */ + if (this_colour>>24 == 0x0) + continue; + found = false; + for (palette_iter = &colour_set[0]; palette_iter != next_space; palette_iter++) { + if (*palette_iter == this_colour) { + found = true; + break; + } + } + if (found) + continue; + /* We only support 4bit paletted cursors */ + if (colour_count >= 16) { + if (warn_palette) { + dev_info(&dev->pdev->dev, "Video card only supports cursors with up to 16 colours.\n"); + dev_info(&dev->pdev->dev, "Not enabling hardware cursor.\n"); + warn_palette = false; /* Only tell the user once. */ + } + return -EINVAL; + } + *next_space = this_colour; + next_space++; + colour_count++; + } + + /* Program colours from cursor icon into palette */ + for (i = 0; i < colour_count; i++) { + if (i <= 2) + reg_index = 0x8 + i*0x4; + else + reg_index = 0x60 + i*0x3; + WREG_DAC(reg_index, colour_set[i] & 0xff); + WREG_DAC(reg_index+1, colour_set[i]>>8 & 0xff); + WREG_DAC(reg_index+2, colour_set[i]>>16 & 0xff); + if (WARN_ON((colour_set[i]>>24 & 0xff) != 0xff)) + return -EINVAL; + } + + /* now write colour indices into hardware cursor buffer */ + for (row = 0; row < 64; row++) { + memset(&this_row[0], 0, 48); + for (col = 0; col < 64; col++) { + this_colour = ioread32(src + 4*(col + 64*row)); + /* write transparent pixels */ + if (this_colour>>24 == 0x0) { + this_row[47 - col/8] |= 0x80>>(col%8); + continue; + } + + /* write colour index here */ + for (i = 0; i < colour_count; i++) { + if (colour_set[i] == this_colour) { + if (col % 2) + this_row[col/2] |= i<<4; + else + this_row[col/2] |= i; + break; + } + } + } + memcpy_toio(dst + row*48, &this_row[0], 48); + } + + return 0; +} + +static void mgag200_cursor_set_base(struct mga_device *mdev, u64 address) +{ + u8 addrl = (address >> 10) & 0xff; + u8 addrh = (address >> 18) & 0x3f; + + /* Program gpu address of cursor buffer */ + WREG_DAC(MGA1064_CURSOR_BASE_ADR_LOW, addrl); + WREG_DAC(MGA1064_CURSOR_BASE_ADR_HI, addrh); +} + +static int mgag200_show_cursor(struct mga_device *mdev, void *dst, void *src, + unsigned int width, unsigned int height, + u64 dst_gpu) +{ + int ret; + + ret = mgag200_cursor_update(mdev, dst, src, width, height); + if (ret) + return ret; + mgag200_cursor_set_base(mdev, dst_gpu); + + /* Adjust cursor control register to turn on the cursor */ + WREG_DAC(MGA1064_CURSOR_CTL, 4); /* 16-colour palletized cursor mode */ + + return 0; +} + /* * Hide the cursor off screen. We can't disable the cursor hardware because * it takes too long to re-activate and causes momentary corruption. @@ -82,19 +204,10 @@ int mgag200_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv, struct drm_gem_vram_object *pixels_next; struct drm_gem_object *obj; struct drm_gem_vram_object *gbo = NULL; - int ret = 0; + int ret; u8 *src, *dst; - unsigned int i, row, col; - uint32_t colour_set[16]; - uint32_t *next_space = &colour_set[0]; - uint32_t *palette_iter; - uint32_t this_colour; - bool found = false; - int colour_count = 0; s64 gpu_addr; u64 dst_gpu; - u8 reg_index; - u8 this_row[48];
if (!pixels_1 || !pixels_2) { WREG8(MGA_CURPOSXL, 0); @@ -159,91 +272,9 @@ int mgag200_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv, } dst_gpu = (u64)gpu_addr;
- memset(&colour_set[0], 0, sizeof(uint32_t)*16); - /* width*height*4 = 16384 */ - for (i = 0; i < 16384; i += 4) { - this_colour = ioread32(src + i); - /* No transparency */ - if (this_colour>>24 != 0xff && - this_colour>>24 != 0x0) { - if (warn_transparent) { - dev_info(&dev->pdev->dev, "Video card doesn't support cursors with partial transparency.\n"); - dev_info(&dev->pdev->dev, "Not enabling hardware cursor.\n"); - warn_transparent = false; /* Only tell the user once. */ - } - ret = -EINVAL; - goto err_drm_gem_vram_kunmap_dst; - } - /* Don't need to store transparent pixels as colours */ - if (this_colour>>24 == 0x0) - continue; - found = false; - for (palette_iter = &colour_set[0]; palette_iter != next_space; palette_iter++) { - if (*palette_iter == this_colour) { - found = true; - break; - } - } - if (found) - continue; - /* We only support 4bit paletted cursors */ - if (colour_count >= 16) { - if (warn_palette) { - dev_info(&dev->pdev->dev, "Video card only supports cursors with up to 16 colours.\n"); - dev_info(&dev->pdev->dev, "Not enabling hardware cursor.\n"); - warn_palette = false; /* Only tell the user once. */ - } - ret = -EINVAL; - goto err_drm_gem_vram_kunmap_dst; - } - *next_space = this_colour; - next_space++; - colour_count++; - } - - /* Program colours from cursor icon into palette */ - for (i = 0; i < colour_count; i++) { - if (i <= 2) - reg_index = 0x8 + i*0x4; - else - reg_index = 0x60 + i*0x3; - WREG_DAC(reg_index, colour_set[i] & 0xff); - WREG_DAC(reg_index+1, colour_set[i]>>8 & 0xff); - WREG_DAC(reg_index+2, colour_set[i]>>16 & 0xff); - BUG_ON((colour_set[i]>>24 & 0xff) != 0xff); - } - - /* now write colour indices into hardware cursor buffer */ - for (row = 0; row < 64; row++) { - memset(&this_row[0], 0, 48); - for (col = 0; col < 64; col++) { - this_colour = ioread32(src + 4*(col + 64*row)); - /* write transparent pixels */ - if (this_colour>>24 == 0x0) { - this_row[47 - col/8] |= 0x80>>(col%8); - continue; - } - - /* write colour index here */ - for (i = 0; i < colour_count; i++) { - if (colour_set[i] == this_colour) { - if (col % 2) - this_row[col/2] |= i<<4; - else - this_row[col/2] |= i; - break; - } - } - } - memcpy_toio(dst + row*48, &this_row[0], 48); - } - - /* Program gpu address of cursor buffer */ - 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 */ + ret = mgag200_show_cursor(mdev, dst, src, width, height, dst_gpu); + if (ret) + goto err_drm_gem_vram_kunmap_dst;
/* Now update internal buffer pointers */ if (pixels_current)
Selecting the correct BO for the new cursor image is not relevant outside of mgag200_show_cursor(). Let the function do the work.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/mgag200/mgag200_cursor.c | 120 +++++++++++------------ 1 file changed, 56 insertions(+), 64 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c index 13daa0ce1c9e..4a5b1aa921e0 100644 --- a/drivers/gpu/drm/mgag200/mgag200_cursor.c +++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c @@ -117,21 +117,69 @@ static void mgag200_cursor_set_base(struct mga_device *mdev, u64 address) WREG_DAC(MGA1064_CURSOR_BASE_ADR_HI, addrh); }
-static int mgag200_show_cursor(struct mga_device *mdev, void *dst, void *src, - unsigned int width, unsigned int height, - u64 dst_gpu) +static int mgag200_show_cursor(struct mga_device *mdev, void *src, + unsigned int width, unsigned int height) { + struct drm_device *dev = mdev->dev; + 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_next; + void *dst; + s64 off; int ret;
+ if (!pixels_1 || !pixels_2) { + WREG8(MGA_CURPOSXL, 0); + WREG8(MGA_CURPOSXH, 0); + return -ENOTSUPP; /* Didn't allocate space for cursors */ + } + + if (WARN_ON(pixels_current && + pixels_1 != pixels_current && + pixels_2 != pixels_current)) { + return -ENOTSUPP; /* inconsistent state */ + } + + if (pixels_current == pixels_1) + pixels_next = pixels_2; + else + pixels_next = pixels_1; + + dst = drm_gem_vram_vmap(pixels_next); + if (IS_ERR(dst)) { + ret = PTR_ERR(dst); + dev_err(&dev->pdev->dev, + "failed to map cursor updates: %d\n", ret); + return ret; + } + off = drm_gem_vram_offset(pixels_next); + if (off < 0) { + ret = (int)off; + dev_err(&dev->pdev->dev, + "failed to get cursor scanout address: %d\n", ret); + goto err_drm_gem_vram_vunmap; + } + ret = mgag200_cursor_update(mdev, dst, src, width, height); if (ret) - return ret; - mgag200_cursor_set_base(mdev, dst_gpu); + goto err_drm_gem_vram_vunmap; + mgag200_cursor_set_base(mdev, off);
/* Adjust cursor control register to turn on the cursor */ WREG_DAC(MGA1064_CURSOR_CTL, 4); /* 16-colour palletized cursor mode */
+ if (pixels_current) + drm_gem_vram_unpin(pixels_current); + mdev->cursor.pixels_current = pixels_next; + + drm_gem_vram_vunmap(pixels_next, dst); + return 0; + +err_drm_gem_vram_vunmap: + drm_gem_vram_vunmap(pixels_next, dst); + return ret; }
/* @@ -198,28 +246,10 @@ int mgag200_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv, { struct drm_device *dev = crtc->dev; struct mga_device *mdev = (struct mga_device *)dev->dev_private; - 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_next; struct drm_gem_object *obj; struct drm_gem_vram_object *gbo = NULL; int ret; - u8 *src, *dst; - s64 gpu_addr; - u64 dst_gpu; - - if (!pixels_1 || !pixels_2) { - WREG8(MGA_CURPOSXL, 0); - WREG8(MGA_CURPOSXH, 0); - return -ENOTSUPP; /* Didn't allocate space for cursors */ - } - - if (WARN_ON(pixels_current && - pixels_1 != pixels_current && - pixels_2 != pixels_current)) { - return -ENOTSUPP; /* inconsistent state */ - } + u8 *src;
if (!handle || !file_priv) { mgag200_hide_cursor(mdev); @@ -232,11 +262,6 @@ int mgag200_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv, return -EINVAL; }
- 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; @@ -249,48 +274,15 @@ int mgag200_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv, goto err_drm_gem_object_put_unlocked; }
- /* 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_vunmap; - } - 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_next); - 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; - - ret = mgag200_show_cursor(mdev, dst, src, width, height, dst_gpu); + ret = mgag200_show_cursor(mdev, src, width, height); if (ret) - goto err_drm_gem_vram_kunmap_dst; + goto err_drm_gem_vram_vunmap;
/* 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_next); drm_gem_vram_vunmap(gbo, src); 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_vunmap: drm_gem_vram_vunmap(gbo, src); err_drm_gem_object_put_unlocked:
The double-buffered cursor image is currently stored in video memory by creating two BOs and pinning them to VRAM. The exact location is chosen by VRAM helpers. The pinned cursor BOs can conflict with framebuffer BOs and prevent the primary plane from displaying its framebuffer.
As a first step to solving this problem, we reserve dedicated space at the high end of the video memory for the cursor images. As the amount of video memory now differs from the amount of available framebuffer memory, size tests are adapted accordingly.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/mgag200/mgag200_cursor.c | 19 +++++++++++++++---- drivers/gpu/drm/mgag200/mgag200_drv.h | 2 ++ drivers/gpu/drm/mgag200/mgag200_main.c | 2 +- drivers/gpu/drm/mgag200/mgag200_mode.c | 2 +- drivers/gpu/drm/mgag200/mgag200_ttm.c | 4 ++++ 5 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c index 4a5b1aa921e0..7f48abf80a6a 100644 --- a/drivers/gpu/drm/mgag200/mgag200_cursor.c +++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c @@ -216,17 +216,20 @@ static void mgag200_move_cursor(struct mga_device *mdev, int x, int y) int mgag200_cursor_init(struct mga_device *mdev) { struct drm_device *dev = mdev->dev; + size_t size; + + size = roundup(64 * 48, PAGE_SIZE); + if (size * 2 > mdev->vram_fb_available) + return -ENOMEM;
/* * Make small buffers to store a hardware cursor (double * buffered icon updates) */ mdev->cursor.pixels_1 = drm_gem_vram_create(dev, &dev->vram_mm->bdev, - roundup(48*64, PAGE_SIZE), - 0, 0); + size, 0, 0); mdev->cursor.pixels_2 = drm_gem_vram_create(dev, &dev->vram_mm->bdev, - roundup(48*64, PAGE_SIZE), - 0, 0); + size, 0, 0); if (IS_ERR(mdev->cursor.pixels_2) || IS_ERR(mdev->cursor.pixels_1)) { mdev->cursor.pixels_1 = NULL; mdev->cursor.pixels_2 = NULL; @@ -235,6 +238,14 @@ int mgag200_cursor_init(struct mga_device *mdev) } mdev->cursor.pixels_current = NULL;
+ /* + * At the high end of video memory, we reserve space for + * buffer objects. The cursor plane uses this memory to store + * a double-buffered image of the current cursor. Hence, it's + * not available for framebuffers. + */ + mdev->vram_fb_available -= 2 * size; + return 0; }
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index 01243fa6397c..5d6cfc88697a 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -173,6 +173,8 @@ struct mga_device {
struct mga_cursor cursor;
+ size_t vram_fb_available; + bool suspended; int num_crtc; enum mga_type type; diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c index 2b59280777a5..5f74aabcd3df 100644 --- a/drivers/gpu/drm/mgag200/mgag200_main.c +++ b/drivers/gpu/drm/mgag200/mgag200_main.c @@ -159,7 +159,7 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
drm_mode_config_init(dev); dev->mode_config.funcs = (void *)&mga_mode_funcs; - if (IS_G200_SE(mdev) && mdev->mc.vram_size < (2048*1024)) + if (IS_G200_SE(mdev) && mdev->vram_fb_available < (2048*1024)) dev->mode_config.preferred_depth = 16; else dev->mode_config.preferred_depth = 32; diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 0cf5608c3644..5ec697148fc1 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -1629,7 +1629,7 @@ static enum drm_mode_status mga_vga_mode_valid(struct drm_connector *connector, bpp = connector->cmdline_mode.bpp; }
- if ((mode->hdisplay * mode->vdisplay * (bpp/8)) > mdev->mc.vram_size) { + if ((mode->hdisplay * mode->vdisplay * (bpp/8)) > mdev->vram_fb_available) { if (connector->cmdline_mode.specified) connector->cmdline_mode.specified = false; return MODE_BAD; diff --git a/drivers/gpu/drm/mgag200/mgag200_ttm.c b/drivers/gpu/drm/mgag200/mgag200_ttm.c index 69c81ebf3745..99997d737362 100644 --- a/drivers/gpu/drm/mgag200/mgag200_ttm.c +++ b/drivers/gpu/drm/mgag200/mgag200_ttm.c @@ -50,6 +50,8 @@ int mgag200_mm_init(struct mga_device *mdev) mdev->fb_mtrr = arch_phys_wc_add(pci_resource_start(dev->pdev, 0), pci_resource_len(dev->pdev, 0));
+ mdev->vram_fb_available = mdev->mc.vram_size; + return 0; }
@@ -57,6 +59,8 @@ void mgag200_mm_fini(struct mga_device *mdev) { struct drm_device *dev = mdev->dev;
+ mdev->vram_fb_available = 0; + drm_vram_helper_release_mm(dev);
arch_io_free_memtype_wc(pci_resource_start(dev->pdev, 0),
Hi,
- /*
* At the high end of video memory, we reserve space for* buffer objects. The cursor plane uses this memory to store* a double-buffered image of the current cursor. Hence, it's* not available for framebuffers.*/- mdev->vram_fb_available -= 2 * size;
Hmm, that looks like a leftover from the previous version of the patch series ...
cheers, Gerd
Hi
Am 24.09.19 um 09:55 schrieb Gerd Hoffmann:
Hi,
- /*
* At the high end of video memory, we reserve space for* buffer objects. The cursor plane uses this memory to store* a double-buffered image of the current cursor. Hence, it's* not available for framebuffers.*/- mdev->vram_fb_available -= 2 * size;
Hmm, that looks like a leftover from the previous version of the patch series ...
This belongs here. For several tests the size of the available framebuffer memory needs to be known. It's stored in mdev->vram_fb_available. mgag200_mm_init() sets the value of mdev->vram_fb_available to the full size of the video RAM. The line above subtracts the size of 2 cursor buffers.
Best regards Thomas
cheers, Gerd
By putting cursor BOs at the high end of the video memory, we can avoid memory fragmentation. Starting at the low end, contiguous video memory is available for framebuffers.
The patch also simplifies the buffer swapping and aligns it with the ast driver. If there are more drivers with similar requirements, the code could be moved into a shared place.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/mgag200/mgag200_cursor.c | 94 +++++++++++++----------- drivers/gpu/drm/mgag200/mgag200_drv.h | 12 +-- 2 files changed, 52 insertions(+), 54 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c index 7f48abf80a6a..d65ee94d540c 100644 --- a/drivers/gpu/drm/mgag200/mgag200_cursor.c +++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c @@ -121,39 +121,25 @@ static int mgag200_show_cursor(struct mga_device *mdev, void *src, unsigned int width, unsigned int height) { struct drm_device *dev = mdev->dev; - 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_next; + struct drm_gem_vram_object *gbo; void *dst; s64 off; int ret;
- if (!pixels_1 || !pixels_2) { + gbo = mdev->cursor.gbo[mdev->cursor.next_index]; + if (!gbo) { WREG8(MGA_CURPOSXL, 0); WREG8(MGA_CURPOSXH, 0); return -ENOTSUPP; /* Didn't allocate space for cursors */ } - - if (WARN_ON(pixels_current && - pixels_1 != pixels_current && - pixels_2 != pixels_current)) { - return -ENOTSUPP; /* inconsistent state */ - } - - if (pixels_current == pixels_1) - pixels_next = pixels_2; - else - pixels_next = pixels_1; - - dst = drm_gem_vram_vmap(pixels_next); + dst = drm_gem_vram_vmap(gbo); if (IS_ERR(dst)) { ret = PTR_ERR(dst); dev_err(&dev->pdev->dev, "failed to map cursor updates: %d\n", ret); return ret; } - off = drm_gem_vram_offset(pixels_next); + off = drm_gem_vram_offset(gbo); if (off < 0) { ret = (int)off; dev_err(&dev->pdev->dev, @@ -169,16 +155,15 @@ static int mgag200_show_cursor(struct mga_device *mdev, void *src, /* Adjust cursor control register to turn on the cursor */ WREG_DAC(MGA1064_CURSOR_CTL, 4); /* 16-colour palletized cursor mode */
- if (pixels_current) - drm_gem_vram_unpin(pixels_current); - mdev->cursor.pixels_current = pixels_next; + drm_gem_vram_vunmap(gbo, dst);
- drm_gem_vram_vunmap(pixels_next, dst); + ++mdev->cursor.next_index; + mdev->cursor.next_index %= ARRAY_SIZE(mdev->cursor.gbo);
return 0;
err_drm_gem_vram_vunmap: - drm_gem_vram_vunmap(pixels_next, dst); + drm_gem_vram_vunmap(gbo, dst); return ret; }
@@ -190,9 +175,6 @@ static void mgag200_hide_cursor(struct mga_device *mdev) { WREG8(MGA_CURPOSXL, 0); WREG8(MGA_CURPOSXH, 0); - if (mdev->cursor.pixels_current) - drm_gem_vram_unpin(mdev->cursor.pixels_current); - mdev->cursor.pixels_current = NULL; }
static void mgag200_move_cursor(struct mga_device *mdev, int x, int y) @@ -216,27 +198,32 @@ static void mgag200_move_cursor(struct mga_device *mdev, int x, int y) int mgag200_cursor_init(struct mga_device *mdev) { struct drm_device *dev = mdev->dev; + size_t ncursors = ARRAY_SIZE(mdev->cursor.gbo); size_t size; + int ret; + size_t i; + struct drm_gem_vram_object *gbo;
size = roundup(64 * 48, PAGE_SIZE); - if (size * 2 > mdev->vram_fb_available) + if (size * ncursors > mdev->vram_fb_available) return -ENOMEM;
- /* - * Make small buffers to store a hardware cursor (double - * buffered icon updates) - */ - mdev->cursor.pixels_1 = drm_gem_vram_create(dev, &dev->vram_mm->bdev, - size, 0, 0); - mdev->cursor.pixels_2 = drm_gem_vram_create(dev, &dev->vram_mm->bdev, - size, 0, 0); - if (IS_ERR(mdev->cursor.pixels_2) || IS_ERR(mdev->cursor.pixels_1)) { - mdev->cursor.pixels_1 = NULL; - mdev->cursor.pixels_2 = NULL; - dev_warn(&dev->pdev->dev, - "Could not allocate space for cursors. Not doing hardware cursors.\n"); + for (i = 0; i < ncursors; ++i) { + gbo = drm_gem_vram_create(dev, &dev->vram_mm->bdev, + size, 0, false); + if (IS_ERR(gbo)) { + ret = PTR_ERR(gbo); + goto err_drm_gem_vram_put; + } + ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM | + DRM_GEM_VRAM_PL_FLAG_TOPDOWN); + if (ret) { + drm_gem_vram_put(gbo); + goto err_drm_gem_vram_put; + } + + mdev->cursor.gbo[i] = gbo; } - mdev->cursor.pixels_current = NULL;
/* * At the high end of video memory, we reserve space for @@ -244,13 +231,32 @@ int mgag200_cursor_init(struct mga_device *mdev) * a double-buffered image of the current cursor. Hence, it's * not available for framebuffers. */ - mdev->vram_fb_available -= 2 * size; + mdev->vram_fb_available -= ncursors * size;
return 0; + +err_drm_gem_vram_put: + while (i) { + --i; + gbo = mdev->cursor.gbo[i]; + drm_gem_vram_unpin(gbo); + drm_gem_vram_put(gbo); + mdev->cursor.gbo[i] = NULL; + } + return ret; }
void mgag200_cursor_fini(struct mga_device *mdev) -{ } +{ + size_t i; + struct drm_gem_vram_object *gbo; + + for (i = 0; i < ARRAY_SIZE(mdev->cursor.gbo); ++i) { + gbo = mdev->cursor.gbo[i]; + drm_gem_vram_unpin(gbo); + drm_gem_vram_put(gbo); + } +}
int mgag200_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv, uint32_t handle, uint32_t width, uint32_t height) diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index 5d6cfc88697a..0ea9a525e57d 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -129,16 +129,8 @@ struct mga_connector { };
struct mga_cursor { - /* - We have to have 2 buffers for the cursor to avoid occasional - corruption while switching cursor icons. - If either of these is NULL, then don't do hardware cursors, and - fall back to software. - */ - struct drm_gem_vram_object *pixels_1; - struct drm_gem_vram_object *pixels_2; - /* The currently displayed icon, this points to one of pixels_1, or pixels_2 */ - struct drm_gem_vram_object *pixels_current; + struct drm_gem_vram_object *gbo[2]; + unsigned int next_index; };
struct mga_mc {
dri-devel@lists.freedesktop.org
-
Gerd Hoffmann -
Thomas Zimmermann