Add helper functions to create and move the cursor.
Create the cursor_bo in prepare_fb callback, in the
atomic_commit callback we only send the update command
to the host.
Signed-off-by: Gerd Hoffmann <[email protected]>
---
drivers/gpu/drm/qxl/qxl_display.c | 248 ++++++++++++++++--------------
1 file changed, 133 insertions(+), 115 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index b315d7484e21..4a3d272e8d6c 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -476,12 +476,11 @@ static int qxl_primary_atomic_check(struct drm_plane *plane,
return qxl_check_framebuffer(qdev, bo);
}
-static int qxl_primary_apply_cursor(struct drm_plane *plane)
+static int qxl_primary_apply_cursor(struct qxl_device *qdev,
+ struct drm_plane_state *plane_state)
{
- struct drm_device *dev = plane->dev;
- struct qxl_device *qdev = to_qxl(dev);
- struct drm_framebuffer *fb = plane->state->fb;
- struct qxl_crtc *qcrtc = to_qxl_crtc(plane->state->crtc);
+ struct drm_framebuffer *fb = plane_state->fb;
+ struct qxl_crtc *qcrtc = to_qxl_crtc(plane_state->crtc);
struct qxl_cursor_cmd *cmd;
struct qxl_release *release;
int ret = 0;
@@ -505,8 +504,8 @@ static int qxl_primary_apply_cursor(struct drm_plane *plane)
cmd = (struct qxl_cursor_cmd *)qxl_release_map(qdev, release);
cmd->type = QXL_CURSOR_SET;
- cmd->u.set.position.x = plane->state->crtc_x + fb->hot_x;
- cmd->u.set.position.y = plane->state->crtc_y + fb->hot_y;
+ cmd->u.set.position.x = plane_state->crtc_x + fb->hot_x;
+ cmd->u.set.position.y = plane_state->crtc_y + fb->hot_y;
cmd->u.set.shape = qxl_bo_physical_address(qdev, qcrtc->cursor_bo, 0);
@@ -523,6 +522,113 @@ static int qxl_primary_apply_cursor(struct drm_plane *plane)
return ret;
}
+static int qxl_primary_move_cursor(struct qxl_device *qdev,
+ struct drm_plane_state *plane_state)
+{
+ struct drm_framebuffer *fb = plane_state->fb;
+ struct qxl_crtc *qcrtc = to_qxl_crtc(plane_state->crtc);
+ struct qxl_cursor_cmd *cmd;
+ struct qxl_release *release;
+ int ret = 0;
+
+ if (!qcrtc->cursor_bo)
+ return 0;
+
+ ret = qxl_alloc_release_reserved(qdev, sizeof(*cmd),
+ QXL_RELEASE_CURSOR_CMD,
+ &release, NULL);
+ if (ret)
+ return ret;
+
+ ret = qxl_release_reserve_list(release, true);
+ if (ret) {
+ qxl_release_free(qdev, release);
+ return ret;
+ }
+
+ cmd = (struct qxl_cursor_cmd *)qxl_release_map(qdev, release);
+ cmd->type = QXL_CURSOR_MOVE;
+ cmd->u.position.x = plane_state->crtc_x + fb->hot_x;
+ cmd->u.position.y = plane_state->crtc_y + fb->hot_y;
+ qxl_release_unmap(qdev, release, &cmd->release_info);
+
+ qxl_release_fence_buffer_objects(release);
+ qxl_push_cursor_ring_release(qdev, release, QXL_CMD_CURSOR, false);
+ return ret;
+}
+
+static struct qxl_bo *qxl_create_cursor(struct qxl_device *qdev,
+ struct qxl_bo *user_bo,
+ int hot_x, int hot_y)
+{
+ static const u32 size = 64 * 64 * 4;
+ struct qxl_bo *cursor_bo;
+ struct dma_buf_map cursor_map;
+ struct dma_buf_map user_map;
+ struct qxl_cursor cursor;
+ int ret;
+
+ if (!user_bo)
+ return NULL;
+
+ ret = qxl_bo_create(qdev, sizeof(struct qxl_cursor) + size,
+ false, true, QXL_GEM_DOMAIN_VRAM, 1,
+ NULL, &cursor_bo);
+ if (ret)
+ goto err;
+
+ ret = qxl_bo_vmap(cursor_bo, &cursor_map);
+ if (ret)
+ goto err_unref;
+
+ ret = qxl_bo_vmap(user_bo, &user_map);
+ if (ret)
+ goto err_unmap;
+
+ cursor.header.unique = 0;
+ cursor.header.type = SPICE_CURSOR_TYPE_ALPHA;
+ cursor.header.width = 64;
+ cursor.header.height = 64;
+ cursor.header.hot_spot_x = hot_x;
+ cursor.header.hot_spot_y = hot_y;
+ cursor.data_size = size;
+ cursor.chunk.next_chunk = 0;
+ cursor.chunk.prev_chunk = 0;
+ cursor.chunk.data_size = size;
+ if (cursor_map.is_iomem) {
+ memcpy_toio(cursor_map.vaddr_iomem,
+ &cursor, sizeof(cursor));
+ memcpy_toio(cursor_map.vaddr_iomem + sizeof(cursor),
+ user_map.vaddr, size);
+ } else {
+ memcpy(cursor_map.vaddr,
+ &cursor, sizeof(cursor));
+ memcpy(cursor_map.vaddr + sizeof(cursor),
+ user_map.vaddr, size);
+ }
+
+ qxl_bo_vunmap(user_bo);
+ qxl_bo_vunmap(cursor_bo);
+ return cursor_bo;
+
+err_unmap:
+ qxl_bo_vunmap(cursor_bo);
+err_unref:
+ qxl_bo_unpin(cursor_bo);
+ qxl_bo_unref(&cursor_bo);
+err:
+ return NULL;
+}
+
+static void qxl_free_cursor(struct qxl_bo *cursor_bo)
+{
+ if (!cursor_bo)
+ return;
+
+ qxl_bo_unpin(cursor_bo);
+ qxl_bo_unref(&cursor_bo);
+}
+
static void qxl_primary_atomic_update(struct drm_plane *plane,
struct drm_plane_state *old_state)
{
@@ -543,7 +649,7 @@ static void qxl_primary_atomic_update(struct drm_plane *plane,
if (qdev->primary_bo)
qxl_io_destroy_primary(qdev);
qxl_io_create_primary(qdev, primary);
- qxl_primary_apply_cursor(plane);
+ qxl_primary_apply_cursor(qdev, plane->state);
}
if (bo->is_dumb)
@@ -574,124 +680,21 @@ static void qxl_primary_atomic_disable(struct drm_plane *plane,
static void qxl_cursor_atomic_update(struct drm_plane *plane,
struct drm_plane_state *old_state)
{
- struct drm_device *dev = plane->dev;
- struct qxl_device *qdev = to_qxl(dev);
+ struct qxl_device *qdev = to_qxl(plane->dev);
struct drm_framebuffer *fb = plane->state->fb;
- struct qxl_crtc *qcrtc = to_qxl_crtc(plane->state->crtc);
- struct qxl_release *release;
- struct qxl_cursor_cmd *cmd;
- struct qxl_cursor *cursor;
- struct drm_gem_object *obj;
- struct qxl_bo *cursor_bo = NULL, *user_bo = NULL, *old_cursor_bo = NULL;
- int ret;
- struct dma_buf_map user_map;
- struct dma_buf_map cursor_map;
- void *user_ptr;
- int size = 64*64*4;
-
- ret = qxl_alloc_release_reserved(qdev, sizeof(*cmd),
- QXL_RELEASE_CURSOR_CMD,
- &release, NULL);
- if (ret)
- return;
if (fb != old_state->fb) {
- obj = fb->obj[0];
- user_bo = gem_to_qxl_bo(obj);
-
- /* pinning is done in the prepare/cleanup framevbuffer */
- ret = qxl_bo_vmap_locked(user_bo, &user_map);
- if (ret)
- goto out_free_release;
- user_ptr = user_map.vaddr; /* TODO: Use mapping abstraction properly */
-
- ret = qxl_alloc_bo_reserved(qdev, release,
- sizeof(struct qxl_cursor) + size,
- &cursor_bo);
- if (ret)
- goto out_kunmap;
-
- ret = qxl_bo_pin(cursor_bo);
- if (ret)
- goto out_free_bo;
-
- ret = qxl_release_reserve_list(release, true);
- if (ret)
- goto out_unpin;
-
- ret = qxl_bo_vmap_locked(cursor_bo, &cursor_map);
- if (ret)
- goto out_backoff;
- if (cursor_map.is_iomem) /* TODO: Use mapping abstraction properly */
- cursor = (struct qxl_cursor __force *)cursor_map.vaddr_iomem;
- else
- cursor = (struct qxl_cursor *)cursor_map.vaddr;
-
- cursor->header.unique = 0;
- cursor->header.type = SPICE_CURSOR_TYPE_ALPHA;
- cursor->header.width = 64;
- cursor->header.height = 64;
- cursor->header.hot_spot_x = fb->hot_x;
- cursor->header.hot_spot_y = fb->hot_y;
- cursor->data_size = size;
- cursor->chunk.next_chunk = 0;
- cursor->chunk.prev_chunk = 0;
- cursor->chunk.data_size = size;
- memcpy(cursor->chunk.data, user_ptr, size);
- qxl_bo_vunmap_locked(cursor_bo);
- qxl_bo_vunmap_locked(user_bo);
-
- cmd = (struct qxl_cursor_cmd *) qxl_release_map(qdev, release);
- cmd->u.set.visible = 1;
- cmd->u.set.shape = qxl_bo_physical_address(qdev,
- cursor_bo, 0);
- cmd->type = QXL_CURSOR_SET;
-
- old_cursor_bo = qcrtc->cursor_bo;
- qcrtc->cursor_bo = cursor_bo;
- cursor_bo = NULL;
+ qxl_primary_apply_cursor(qdev, plane->state);
} else {
-
- ret = qxl_release_reserve_list(release, true);
- if (ret)
- goto out_free_release;
-
- cmd = (struct qxl_cursor_cmd *) qxl_release_map(qdev, release);
- cmd->type = QXL_CURSOR_MOVE;
+ qxl_primary_move_cursor(qdev, plane->state);
}
-
- cmd->u.position.x = plane->state->crtc_x + fb->hot_x;
- cmd->u.position.y = plane->state->crtc_y + fb->hot_y;
-
- qxl_release_unmap(qdev, release, &cmd->release_info);
- qxl_release_fence_buffer_objects(release);
- qxl_push_cursor_ring_release(qdev, release, QXL_CMD_CURSOR, false);
-
- if (old_cursor_bo != NULL)
- qxl_bo_unpin(old_cursor_bo);
- qxl_bo_unref(&old_cursor_bo);
- qxl_bo_unref(&cursor_bo);
-
- return;
-
-out_backoff:
- qxl_release_backoff_reserve_list(release);
-out_unpin:
- qxl_bo_unpin(cursor_bo);
-out_free_bo:
- qxl_bo_unref(&cursor_bo);
-out_kunmap:
- qxl_bo_vunmap_locked(user_bo);
-out_free_release:
- qxl_release_free(qdev, release);
- return;
-
}
static void qxl_cursor_atomic_disable(struct drm_plane *plane,
struct drm_plane_state *old_state)
{
struct qxl_device *qdev = to_qxl(plane->dev);
+ struct qxl_crtc *qcrtc;
struct qxl_release *release;
struct qxl_cursor_cmd *cmd;
int ret;
@@ -714,6 +717,10 @@ static void qxl_cursor_atomic_disable(struct drm_plane *plane,
qxl_release_fence_buffer_objects(release);
qxl_push_cursor_ring_release(qdev, release, QXL_CMD_CURSOR, false);
+
+ qcrtc = to_qxl_crtc(old_state->crtc);
+ qxl_free_cursor(qcrtc->cursor_bo);
+ qcrtc->cursor_bo = NULL;
}
static void qxl_update_dumb_head(struct qxl_device *qdev,
@@ -822,6 +829,17 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane,
qxl_prepare_shadow(qdev, user_bo, new_state->crtc->index);
}
+ if (plane->type == DRM_PLANE_TYPE_CURSOR &&
+ plane->state->fb != new_state->fb) {
+ struct qxl_crtc *qcrtc = to_qxl_crtc(new_state->crtc);
+ struct qxl_bo *old_cursor_bo = qcrtc->cursor_bo;
+
+ qcrtc->cursor_bo = qxl_create_cursor(qdev, user_bo,
+ new_state->fb->hot_x,
+ new_state->fb->hot_y);
+ qxl_free_cursor(old_cursor_bo);
+ }
+
return qxl_bo_pin(user_bo);
}
--
2.29.2
Hi
Am 17.02.21 um 13:32 schrieb Gerd Hoffmann:
> Add helper functions to create and move the cursor.
> Create the cursor_bo in prepare_fb callback, in the
> atomic_commit callback we only send the update command
> to the host.
I'm still trying to wrap my head around the qxl cursor code.
Getting vmap out of the commit tail is good, but I feel like this isn't
going in the right direction overall.
In ast, these helper functions were only good when converting the drvier
to atomic modesetting. So I removed them in the latst patchset and did
all the updates in the plane helpers directly.
For cursor_bo itself, it seems to be transitional state that is only
used during the plane update and crtc update . It should probably be
stored in a plane-state structure.
Some of the primary plane's functions seem to deal with cursor handling.
What's the role of the primary plane in cursor handling?
For now, I suggest to merge patch 1 to 8 and 11; and move the cursor
patches into a new patchset. I'd like ot hear Daniel's opinion on this.
Do you have further plans here?
If you absolutely want patches 9 and 10, I'd rubber-stamp an A-b on them.
Best regards
Thomas
>
> Signed-off-by: Gerd Hoffmann <[email protected]>
> ---
> drivers/gpu/drm/qxl/qxl_display.c | 248 ++++++++++++++++--------------
> 1 file changed, 133 insertions(+), 115 deletions(-)
>
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
> index b315d7484e21..4a3d272e8d6c 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -476,12 +476,11 @@ static int qxl_primary_atomic_check(struct drm_plane *plane,
> return qxl_check_framebuffer(qdev, bo);
> }
>
> -static int qxl_primary_apply_cursor(struct drm_plane *plane)
> +static int qxl_primary_apply_cursor(struct qxl_device *qdev,
> + struct drm_plane_state *plane_state)
> {
> - struct drm_device *dev = plane->dev;
> - struct qxl_device *qdev = to_qxl(dev);
> - struct drm_framebuffer *fb = plane->state->fb;
> - struct qxl_crtc *qcrtc = to_qxl_crtc(plane->state->crtc);
> + struct drm_framebuffer *fb = plane_state->fb;
> + struct qxl_crtc *qcrtc = to_qxl_crtc(plane_state->crtc);
> struct qxl_cursor_cmd *cmd;
> struct qxl_release *release;
> int ret = 0;
> @@ -505,8 +504,8 @@ static int qxl_primary_apply_cursor(struct drm_plane *plane)
>
> cmd = (struct qxl_cursor_cmd *)qxl_release_map(qdev, release);
> cmd->type = QXL_CURSOR_SET;
> - cmd->u.set.position.x = plane->state->crtc_x + fb->hot_x;
> - cmd->u.set.position.y = plane->state->crtc_y + fb->hot_y;
> + cmd->u.set.position.x = plane_state->crtc_x + fb->hot_x;
> + cmd->u.set.position.y = plane_state->crtc_y + fb->hot_y;
>
> cmd->u.set.shape = qxl_bo_physical_address(qdev, qcrtc->cursor_bo, 0);
>
> @@ -523,6 +522,113 @@ static int qxl_primary_apply_cursor(struct drm_plane *plane)
> return ret;
> }
>
> +static int qxl_primary_move_cursor(struct qxl_device *qdev,
> + struct drm_plane_state *plane_state)
> +{
> + struct drm_framebuffer *fb = plane_state->fb;
> + struct qxl_crtc *qcrtc = to_qxl_crtc(plane_state->crtc);
> + struct qxl_cursor_cmd *cmd;
> + struct qxl_release *release;
> + int ret = 0;
> +
> + if (!qcrtc->cursor_bo)
> + return 0;
> +
> + ret = qxl_alloc_release_reserved(qdev, sizeof(*cmd),
> + QXL_RELEASE_CURSOR_CMD,
> + &release, NULL);
> + if (ret)
> + return ret;
> +
> + ret = qxl_release_reserve_list(release, true);
> + if (ret) {
> + qxl_release_free(qdev, release);
> + return ret;
> + }
> +
> + cmd = (struct qxl_cursor_cmd *)qxl_release_map(qdev, release);
> + cmd->type = QXL_CURSOR_MOVE;
> + cmd->u.position.x = plane_state->crtc_x + fb->hot_x;
> + cmd->u.position.y = plane_state->crtc_y + fb->hot_y;
> + qxl_release_unmap(qdev, release, &cmd->release_info);
> +
> + qxl_release_fence_buffer_objects(release);
> + qxl_push_cursor_ring_release(qdev, release, QXL_CMD_CURSOR, false);
> + return ret;
> +}
> +
> +static struct qxl_bo *qxl_create_cursor(struct qxl_device *qdev,
> + struct qxl_bo *user_bo,
> + int hot_x, int hot_y)
> +{
> + static const u32 size = 64 * 64 * 4;
> + struct qxl_bo *cursor_bo;
> + struct dma_buf_map cursor_map;
> + struct dma_buf_map user_map;
> + struct qxl_cursor cursor;
> + int ret;
> +
> + if (!user_bo)
> + return NULL;
> +
> + ret = qxl_bo_create(qdev, sizeof(struct qxl_cursor) + size,
> + false, true, QXL_GEM_DOMAIN_VRAM, 1,
> + NULL, &cursor_bo);
> + if (ret)
> + goto err;
> +
> + ret = qxl_bo_vmap(cursor_bo, &cursor_map);
> + if (ret)
> + goto err_unref;
> +
> + ret = qxl_bo_vmap(user_bo, &user_map);
> + if (ret)
> + goto err_unmap;
> +
> + cursor.header.unique = 0;
> + cursor.header.type = SPICE_CURSOR_TYPE_ALPHA;
> + cursor.header.width = 64;
> + cursor.header.height = 64;
> + cursor.header.hot_spot_x = hot_x;
> + cursor.header.hot_spot_y = hot_y;
> + cursor.data_size = size;
> + cursor.chunk.next_chunk = 0;
> + cursor.chunk.prev_chunk = 0;
> + cursor.chunk.data_size = size;
> + if (cursor_map.is_iomem) {
> + memcpy_toio(cursor_map.vaddr_iomem,
> + &cursor, sizeof(cursor));
> + memcpy_toio(cursor_map.vaddr_iomem + sizeof(cursor),
> + user_map.vaddr, size);
> + } else {
> + memcpy(cursor_map.vaddr,
> + &cursor, sizeof(cursor));
> + memcpy(cursor_map.vaddr + sizeof(cursor),
> + user_map.vaddr, size);
> + }
> +
> + qxl_bo_vunmap(user_bo);
> + qxl_bo_vunmap(cursor_bo);
> + return cursor_bo;
> +
> +err_unmap:
> + qxl_bo_vunmap(cursor_bo);
> +err_unref:
> + qxl_bo_unpin(cursor_bo);
> + qxl_bo_unref(&cursor_bo);
> +err:
> + return NULL;
> +}
> +
> +static void qxl_free_cursor(struct qxl_bo *cursor_bo)
> +{
> + if (!cursor_bo)
> + return;
> +
> + qxl_bo_unpin(cursor_bo);
> + qxl_bo_unref(&cursor_bo);
> +}
> +
> static void qxl_primary_atomic_update(struct drm_plane *plane,
> struct drm_plane_state *old_state)
> {
> @@ -543,7 +649,7 @@ static void qxl_primary_atomic_update(struct drm_plane *plane,
> if (qdev->primary_bo)
> qxl_io_destroy_primary(qdev);
> qxl_io_create_primary(qdev, primary);
> - qxl_primary_apply_cursor(plane);
> + qxl_primary_apply_cursor(qdev, plane->state);
> }
>
> if (bo->is_dumb)
> @@ -574,124 +680,21 @@ static void qxl_primary_atomic_disable(struct drm_plane *plane,
> static void qxl_cursor_atomic_update(struct drm_plane *plane,
> struct drm_plane_state *old_state)
> {
> - struct drm_device *dev = plane->dev;
> - struct qxl_device *qdev = to_qxl(dev);
> + struct qxl_device *qdev = to_qxl(plane->dev);
> struct drm_framebuffer *fb = plane->state->fb;
> - struct qxl_crtc *qcrtc = to_qxl_crtc(plane->state->crtc);
> - struct qxl_release *release;
> - struct qxl_cursor_cmd *cmd;
> - struct qxl_cursor *cursor;
> - struct drm_gem_object *obj;
> - struct qxl_bo *cursor_bo = NULL, *user_bo = NULL, *old_cursor_bo = NULL;
> - int ret;
> - struct dma_buf_map user_map;
> - struct dma_buf_map cursor_map;
> - void *user_ptr;
> - int size = 64*64*4;
> -
> - ret = qxl_alloc_release_reserved(qdev, sizeof(*cmd),
> - QXL_RELEASE_CURSOR_CMD,
> - &release, NULL);
> - if (ret)
> - return;
>
> if (fb != old_state->fb) {
> - obj = fb->obj[0];
> - user_bo = gem_to_qxl_bo(obj);
> -
> - /* pinning is done in the prepare/cleanup framevbuffer */
> - ret = qxl_bo_vmap_locked(user_bo, &user_map);
> - if (ret)
> - goto out_free_release;
> - user_ptr = user_map.vaddr; /* TODO: Use mapping abstraction properly */
> -
> - ret = qxl_alloc_bo_reserved(qdev, release,
> - sizeof(struct qxl_cursor) + size,
> - &cursor_bo);
> - if (ret)
> - goto out_kunmap;
> -
> - ret = qxl_bo_pin(cursor_bo);
> - if (ret)
> - goto out_free_bo;
> -
> - ret = qxl_release_reserve_list(release, true);
> - if (ret)
> - goto out_unpin;
> -
> - ret = qxl_bo_vmap_locked(cursor_bo, &cursor_map);
> - if (ret)
> - goto out_backoff;
> - if (cursor_map.is_iomem) /* TODO: Use mapping abstraction properly */
> - cursor = (struct qxl_cursor __force *)cursor_map.vaddr_iomem;
> - else
> - cursor = (struct qxl_cursor *)cursor_map.vaddr;
> -
> - cursor->header.unique = 0;
> - cursor->header.type = SPICE_CURSOR_TYPE_ALPHA;
> - cursor->header.width = 64;
> - cursor->header.height = 64;
> - cursor->header.hot_spot_x = fb->hot_x;
> - cursor->header.hot_spot_y = fb->hot_y;
> - cursor->data_size = size;
> - cursor->chunk.next_chunk = 0;
> - cursor->chunk.prev_chunk = 0;
> - cursor->chunk.data_size = size;
> - memcpy(cursor->chunk.data, user_ptr, size);
> - qxl_bo_vunmap_locked(cursor_bo);
> - qxl_bo_vunmap_locked(user_bo);
> -
> - cmd = (struct qxl_cursor_cmd *) qxl_release_map(qdev, release);
> - cmd->u.set.visible = 1;
> - cmd->u.set.shape = qxl_bo_physical_address(qdev,
> - cursor_bo, 0);
> - cmd->type = QXL_CURSOR_SET;
> -
> - old_cursor_bo = qcrtc->cursor_bo;
> - qcrtc->cursor_bo = cursor_bo;
> - cursor_bo = NULL;
> + qxl_primary_apply_cursor(qdev, plane->state);
> } else {
> -
> - ret = qxl_release_reserve_list(release, true);
> - if (ret)
> - goto out_free_release;
> -
> - cmd = (struct qxl_cursor_cmd *) qxl_release_map(qdev, release);
> - cmd->type = QXL_CURSOR_MOVE;
> + qxl_primary_move_cursor(qdev, plane->state);
> }
> -
> - cmd->u.position.x = plane->state->crtc_x + fb->hot_x;
> - cmd->u.position.y = plane->state->crtc_y + fb->hot_y;
> -
> - qxl_release_unmap(qdev, release, &cmd->release_info);
> - qxl_release_fence_buffer_objects(release);
> - qxl_push_cursor_ring_release(qdev, release, QXL_CMD_CURSOR, false);
> -
> - if (old_cursor_bo != NULL)
> - qxl_bo_unpin(old_cursor_bo);
> - qxl_bo_unref(&old_cursor_bo);
> - qxl_bo_unref(&cursor_bo);
> -
> - return;
> -
> -out_backoff:
> - qxl_release_backoff_reserve_list(release);
> -out_unpin:
> - qxl_bo_unpin(cursor_bo);
> -out_free_bo:
> - qxl_bo_unref(&cursor_bo);
> -out_kunmap:
> - qxl_bo_vunmap_locked(user_bo);
> -out_free_release:
> - qxl_release_free(qdev, release);
> - return;
> -
> }
>
> static void qxl_cursor_atomic_disable(struct drm_plane *plane,
> struct drm_plane_state *old_state)
> {
> struct qxl_device *qdev = to_qxl(plane->dev);
> + struct qxl_crtc *qcrtc;
> struct qxl_release *release;
> struct qxl_cursor_cmd *cmd;
> int ret;
> @@ -714,6 +717,10 @@ static void qxl_cursor_atomic_disable(struct drm_plane *plane,
>
> qxl_release_fence_buffer_objects(release);
> qxl_push_cursor_ring_release(qdev, release, QXL_CMD_CURSOR, false);
> +
> + qcrtc = to_qxl_crtc(old_state->crtc);
> + qxl_free_cursor(qcrtc->cursor_bo);
> + qcrtc->cursor_bo = NULL;
> }
>
> static void qxl_update_dumb_head(struct qxl_device *qdev,
> @@ -822,6 +829,17 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane,
> qxl_prepare_shadow(qdev, user_bo, new_state->crtc->index);
> }
>
> + if (plane->type == DRM_PLANE_TYPE_CURSOR &&
> + plane->state->fb != new_state->fb) {
> + struct qxl_crtc *qcrtc = to_qxl_crtc(new_state->crtc);
> + struct qxl_bo *old_cursor_bo = qcrtc->cursor_bo;
> +
> + qcrtc->cursor_bo = qxl_create_cursor(qdev, user_bo,
> + new_state->fb->hot_x,
> + new_state->fb->hot_y);
> + qxl_free_cursor(old_cursor_bo);
> + }
> +
> return qxl_bo_pin(user_bo);
> }
>
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
Hi,
> I'm still trying to wrap my head around the qxl cursor code.
>
> Getting vmap out of the commit tail is good, but I feel like this isn't
> going in the right direction overall.
>
> In ast, these helper functions were only good when converting the drvier to
> atomic modesetting. So I removed them in the latst patchset and did all the
> updates in the plane helpers directly.
I see the helper functions more as a way to get some structure into the
code flow. The callbacks are easier to read if they just call helper
functions for stuff which needs more than a handful lines of code
(patch 9/11 exists for the same reason).
The helpers also make it easier move work from one callback to another,
but that is just a useful side-effect.
I had considered making that two separate patches, one factor out code
into functions and one moving the calls. Turned out to not be that easy
though, because the old qxl_cursor_atomic_update() code was a rather
hairy mix of qxl_create_cursor() + qxl_primary_apply_cursor() +
qxl_primary_move_cursor().
> For cursor_bo itself, it seems to be transitional state that is only used
> during the plane update and crtc update . It should probably be stored in a
> plane-state structure.
>
> Some of the primary plane's functions seem to deal with cursor handling.
> What's the role of the primary plane in cursor handling?
It's a quirk. The qxl device will forget the cursor state on
qxl_io_create_primary(), so I have to remember the cursor state
and re-establish it by calling qxl_primary_apply_cursor() again.
So I'm not sure sticking this into plane state would work. Because of
the quirk this is more than just a handover from prepare to commit.
> For now, I suggest to merge patch 1 to 8 and 11; and move the cursor patches
> into a new patchset.
I can merge 1-8, but 11 has to wait until the cursor is sorted.
There is a reason why 11 is last in the series ;)
> I'd like ot hear Daniel's opinion on this. Do you have
> further plans here?
Well. I suspect I could easily spend a month cleaning up and party
redesign the qxl driver (specifically qxl_draw.c + qxl_image.c).
I'm not sure I'll find the time to actually do that anytime soon.
I have plenty of other stuff on my TODO list, and given that the
world is transitioning to virtio-gpu the priority for qxl isn't
that high.
So, no, I have no short-term plans for qxl beyond fixing pins +
reservations + lockdep.
take care,
Gerd
Hi
Am 18.02.21 um 12:50 schrieb Gerd Hoffmann:
> Hi,
>
>> I'm still trying to wrap my head around the qxl cursor code.
>>
>> Getting vmap out of the commit tail is good, but I feel like this isn't
>> going in the right direction overall.
>>
>> In ast, these helper functions were only good when converting the drvier to
>> atomic modesetting. So I removed them in the latst patchset and did all the
>> updates in the plane helpers directly.
>
> I see the helper functions more as a way to get some structure into the
> code flow. The callbacks are easier to read if they just call helper
> functions for stuff which needs more than a handful lines of code
> (patch 9/11 exists for the same reason).
>
> The helpers also make it easier move work from one callback to another,
> but that is just a useful side-effect.
>
> I had considered making that two separate patches, one factor out code
> into functions and one moving the calls. Turned out to not be that easy
> though, because the old qxl_cursor_atomic_update() code was a rather
> hairy mix of qxl_create_cursor() + qxl_primary_apply_cursor() +
> qxl_primary_move_cursor().
>
>> For cursor_bo itself, it seems to be transitional state that is only used
>> during the plane update and crtc update . It should probably be stored in a
>> plane-state structure.
>>
>> Some of the primary plane's functions seem to deal with cursor handling.
>> What's the role of the primary plane in cursor handling?
>
> It's a quirk. The qxl device will forget the cursor state on
> qxl_io_create_primary(), so I have to remember the cursor state
> and re-establish it by calling qxl_primary_apply_cursor() again.
>
> So I'm not sure sticking this into plane state would work. Because of
> the quirk this is more than just a handover from prepare to commit.
>
>> For now, I suggest to merge patch 1 to 8 and 11; and move the cursor patches
>> into a new patchset.
>
> I can merge 1-8, but 11 has to wait until the cursor is sorted.
> There is a reason why 11 is last in the series ;)
>
>> I'd like ot hear Daniel's opinion on this. Do you have
>> further plans here?
>
> Well. I suspect I could easily spend a month cleaning up and party
> redesign the qxl driver (specifically qxl_draw.c + qxl_image.c).
>
> I'm not sure I'll find the time to actually do that anytime soon.
> I have plenty of other stuff on my TODO list, and given that the
> world is transitioning to virtio-gpu the priority for qxl isn't
> that high.
Well, in that case:
Acked-by: Thomas Zimmermann <[email protected]>
for patches 9 and 10. Having the vmap calls fixed is at least worth it.
Best regards
Thomas
>
> So, no, I have no short-term plans for qxl beyond fixing pins +
> reservations + lockdep.
>
> take care,
> Gerd
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
Hi,
> > Well. I suspect I could easily spend a month cleaning up and party
> > redesign the qxl driver (specifically qxl_draw.c + qxl_image.c).
> >
> > I'm not sure I'll find the time to actually do that anytime soon.
> > I have plenty of other stuff on my TODO list, and given that the
> > world is transitioning to virtio-gpu the priority for qxl isn't
> > that high.
>
> Well, in that case:
>
> Acked-by: Thomas Zimmermann <[email protected]>
>
> for patches 9 and 10. Having the vmap calls fixed is at least worth it.
Thanks. Any chance you can ack 8/11 too (only patch missing an ack).
take care,
Gerd