(was: drm/ast/mgag200: Place cursor BOs at VRAM high-end)
This patchset cleans up the memory management of HW cursors in mgag200. It further moves the allocated cursor BOs to the of the video RAM to reduce memory fragmentation.
The mgag200 driver manages cursor memory in dedicated GEM VRAM buffer objects. It uses a double-buffering scheme of alternating between two GEM BOs The code is convoluted and can lead to memory fragmentation if a BO is stored the middle of VRAM. This is especially a problem as mgag200 devices only contain a small amount of video memory (e.g., 16 MiB).
With this patchset, the cursor handling in 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, the driver dedicates a few KiB at the high end of a device's video memory to storing the cursor's buffer objects. This prevents memory fragmentation.
The patchset has been tested on mgag200 hardware.
v3: * split-off mgag200 patches into separate series v2: * remove VRAM buffers in favor of GEM BOs * manage BO placement with pin flag
Thomas Zimmermann (7): 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/mgag200/mgag200_cursor.c | 313 ++++++++++++++--------- 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 + 5 files changed, 216 insertions(+), 149 deletions(-)
-- 2.23.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 | 220 +++++++++++++---------- 1 file changed, 125 insertions(+), 95 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c index 621960723a3a..5fc47ab0e8fd 100644 --- a/drivers/gpu/drm/mgag200/mgag200_cursor.c +++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c @@ -12,6 +12,127 @@ 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); + 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); + } + + 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 +203,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 +271,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 5fc47ab0e8fd..ed69b396ac02 100644 --- a/drivers/gpu/drm/mgag200/mgag200_cursor.c +++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c @@ -116,21 +116,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; }
/* @@ -197,28 +245,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); @@ -231,11 +261,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; @@ -248,48 +273,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 ed69b396ac02..318e434f2d40 100644 --- a/drivers/gpu/drm/mgag200/mgag200_cursor.c +++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c @@ -215,17 +215,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; @@ -234,6 +237,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),
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 318e434f2d40..79711dbb5b03 100644 --- a/drivers/gpu/drm/mgag200/mgag200_cursor.c +++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c @@ -120,39 +120,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, @@ -168,16 +154,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; }
@@ -189,9 +174,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) @@ -215,27 +197,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 @@ -243,13 +230,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 {
On Fri, Sep 27, 2019 at 11:12:54AM +0200, Thomas Zimmermann wrote:
(was: drm/ast/mgag200: Place cursor BOs at VRAM high-end)
This patchset cleans up the memory management of HW cursors in mgag200. It further moves the allocated cursor BOs to the of the video RAM to reduce memory fragmentation.
The mgag200 driver manages cursor memory in dedicated GEM VRAM buffer objects. It uses a double-buffering scheme of alternating between two GEM BOs The code is convoluted and can lead to memory fragmentation if a BO is stored the middle of VRAM. This is especially a problem as mgag200 devices only contain a small amount of video memory (e.g., 16 MiB).
With this patchset, the cursor handling in 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, the driver dedicates a few KiB at the high end of a device's video memory to storing the cursor's buffer objects. This prevents memory fragmentation.
The patchset has been tested on mgag200 hardware.
Acked-by: Gerd Hoffmann kraxel@redhat.com
v3:
- split-off mgag200 patches into separate series
v2:
- remove VRAM buffers in favor of GEM BOs
- manage BO placement with pin flag
Thomas Zimmermann (7): 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/mgag200/mgag200_cursor.c | 313 ++++++++++++++--------- 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 + 5 files changed, 216 insertions(+), 149 deletions(-)
-- 2.23.0
dri-devel@lists.freedesktop.org
-
Gerd Hoffmann -
Thomas Zimmermann