On Tue, Jun 11, 2019 at 03:03:42PM +0200, Thomas Zimmermann wrote:
> The cursor handling in mgag200 is complicated to understand. It touches a
> number of different BOs, but doesn't really use all of them.
>
> Rewriting the cursor update reduces the amount of cursor state. There are
> two BOs for double-buffered HW updates. The source BO updates the one that
> is currently not displayed and then switches buffers. Explicit BO locking
> has been removed from the code. BOs are simply pinned and unpinned in video
> RAM.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> Acked-by: Gerd Hoffmann <kraxel at redhat.com>
> ---
> drivers/gpu/drm/mgag200/mgag200_cursor.c | 165 ++++++++++-------------
> drivers/gpu/drm/mgag200/mgag200_drv.h | 3 -
> drivers/gpu/drm/mgag200/mgag200_main.c | 4 +-
> 3 files changed, 69 insertions(+), 103 deletions(-)
>
> diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c
> index de94a650077b..5a22ef825588 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_cursor.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c
> @@ -19,10 +19,9 @@ static void mga_hide_cursor(struct mga_device *mdev)
> {
> WREG8(MGA_CURPOSXL, 0);
> WREG8(MGA_CURPOSXH, 0);
> - if (mdev->cursor.pixels_1->pin_count)
> - drm_gem_vram_unpin_locked(mdev->cursor.pixels_1);
> - if (mdev->cursor.pixels_2->pin_count)
> - drm_gem_vram_unpin_locked(mdev->cursor.pixels_2);
> + if (mdev->cursor.pixels_current)
> + drm_gem_vram_unpin(mdev->cursor.pixels_current);
> + mdev->cursor.pixels_current = NULL;
> }
>
> int mga_crtc_cursor_set(struct drm_crtc *crtc,
> @@ -36,7 +35,7 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc,
> struct drm_gem_vram_object *pixels_1 = mdev->cursor.pixels_1;
> struct drm_gem_vram_object *pixels_2 = mdev->cursor.pixels_2;
> struct drm_gem_vram_object *pixels_current = mdev->cursor.pixels_current;
> - struct drm_gem_vram_object *pixels_prev = mdev->cursor.pixels_prev;
> + struct drm_gem_vram_object *pixels_next;
> struct drm_gem_object *obj;
> struct drm_gem_vram_object *gbo = NULL;
> int ret = 0;
> @@ -49,6 +48,7 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc,
> bool found = false;
> int colour_count = 0;
> s64 gpu_addr;
> + u64 dst_gpu;
> u8 reg_index;
> u8 this_row[48];
>
> @@ -58,80 +58,66 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc,
> return -ENOTSUPP; /* Didn't allocate space for cursors */
> }
>
> - if ((width != 64 || height != 64) && handle) {
> - WREG8(MGA_CURPOSXL, 0);
> - WREG8(MGA_CURPOSXH, 0);
> - return -EINVAL;
> + if (WARN_ON(pixels_current &&
> + pixels_1 != pixels_current &&
> + pixels_2 != pixels_current)) {
> + return -ENOTSUPP; /* inconsistent state */
> }
>
> - BUG_ON(pixels_1 != pixels_current && pixels_1 != pixels_prev);
> - BUG_ON(pixels_2 != pixels_current && pixels_2 != pixels_prev);
> - BUG_ON(pixels_current == pixels_prev);
> -
> if (!handle || !file_priv) {
> mga_hide_cursor(mdev);
> return 0;
> }
>
> - obj = drm_gem_object_lookup(file_priv, handle);
> - if (!obj)
> - return -ENOENT;
> -
> - ret = drm_gem_vram_lock(pixels_1, true);
> - if (ret) {
> + if (width != 64 || height != 64) {
> WREG8(MGA_CURPOSXL, 0);
> WREG8(MGA_CURPOSXH, 0);
> - goto out_unref;
> - }
> - ret = drm_gem_vram_lock(pixels_2, true);
> - if (ret) {
> - WREG8(MGA_CURPOSXL, 0);
> - WREG8(MGA_CURPOSXH, 0);
> - drm_gem_vram_unlock(pixels_1);
> - goto out_unlock1;
> + return -EINVAL;
> }
>
> - /* Move cursor buffers into VRAM if they aren't already */
> - if (!pixels_1->pin_count) {
> - ret = drm_gem_vram_pin_locked(pixels_1,
> - DRM_GEM_VRAM_PL_FLAG_VRAM);
> - if (ret)
> - goto out1;
> - gpu_addr = drm_gem_vram_offset(pixels_1);
> - if (gpu_addr < 0) {
> - drm_gem_vram_unpin_locked(pixels_1);
> - goto out1;
> - }
> - mdev->cursor.pixels_1_gpu_addr = gpu_addr;
> - }
> - if (!pixels_2->pin_count) {
> - ret = drm_gem_vram_pin_locked(pixels_2,
> - DRM_GEM_VRAM_PL_FLAG_VRAM);
> - if (ret) {
> - drm_gem_vram_unpin_locked(pixels_1);
> - goto out1;
> - }
> - gpu_addr = drm_gem_vram_offset(pixels_2);
> - if (gpu_addr < 0) {
> - drm_gem_vram_unpin_locked(pixels_1);
> - drm_gem_vram_unpin_locked(pixels_2);
> - goto out1;
> - }
> - mdev->cursor.pixels_2_gpu_addr = gpu_addr;
> - }
> + if (pixels_current == pixels_1)
> + pixels_next = pixels_2;
> + else
> + pixels_next = pixels_1;
>
> + obj = drm_gem_object_lookup(file_priv, handle);
> + if (!obj)
> + return -ENOENT;
> gbo = drm_gem_vram_of_gem(obj);
> - ret = drm_gem_vram_lock(gbo, true);
> + ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_SYSTEM);
pl_flag = 0 should be fine here too.
cheers,
Gerd