Skip to content

Commit

Permalink
drm/qxl: Pin buffer objects for internal mappings
Browse files Browse the repository at this point in the history
Add qxl_bo_pin_and_vmap() that pins and vmaps a buffer object in one
step. Update callers of the regular qxl_bo_vmap(). Fixes a bug where
qxl accesses an unpinned buffer object while it is being moved; such
as with the monitor-description BO. An typical error is shown below.

[    4.303586] [drm:drm_atomic_helper_commit_planes] *ERROR* head 1 wrong: 65376256x16777216+0+0
[    4.586883] [drm:drm_atomic_helper_commit_planes] *ERROR* head 1 wrong: 65376256x16777216+0+0
[    4.904036] [drm:drm_atomic_helper_commit_planes] *ERROR* head 1 wrong: 65335296x16777216+0+0
[    5.374347] [drm:qxl_release_from_id_locked] *ERROR* failed to find id in release_idr

Commit b33651a ("drm/qxl: Do not pin buffer objects for vmap")
removed the implicit pin operation from qxl's vmap code. This is the
correct behavior for GEM and PRIME interfaces, but the pin is still
needed for qxl internal operation.

Also add a corresponding function qxl_bo_vunmap_and_unpin() and remove
the old qxl_bo_vmap() helpers.

Future directions: BOs should not be pinned or vmapped unnecessarily.
The pin-and-vmap operation should be removed from the driver and a
temporary mapping should be established with a vmap_local-like helper.
See the client helper drm_client_buffer_vmap_local() for semantics.

v2:
- unreserve BO on errors in qxl_bo_pin_and_vmap() (Dmitry)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: b33651a ("drm/qxl: Do not pin buffer objects for vmap")
Reported-by: David Kaplan <david.kaplan@amd.com>
Closes: https://lore.kernel.org/dri-devel/ab0fb17d-0f96-4ee6-8b21-65d02bb02655@suse.de/
Tested-by: David Kaplan <david.kaplan@amd.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Zack Rusin <zack.rusin@broadcom.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: virtualization@lists.linux.dev
Cc: spice-devel@lists.freedesktop.org
Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Reviewed-by: Zack Rusin <zack.rusin@broadcom.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20240708142208.194361-1-tzimmermann@suse.de
  • Loading branch information
Thomas Zimmermann committed Jul 10, 2024
1 parent ec85147 commit c537fb4
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 11 deletions.
14 changes: 7 additions & 7 deletions drivers/gpu/drm/qxl/qxl_display.c
Original file line number Diff line number Diff line change
Expand Up @@ -584,11 +584,11 @@ static struct qxl_bo *qxl_create_cursor(struct qxl_device *qdev,
if (ret)
goto err;

ret = qxl_bo_vmap(cursor_bo, &cursor_map);
ret = qxl_bo_pin_and_vmap(cursor_bo, &cursor_map);
if (ret)
goto err_unref;

ret = qxl_bo_vmap(user_bo, &user_map);
ret = qxl_bo_pin_and_vmap(user_bo, &user_map);
if (ret)
goto err_unmap;

Expand All @@ -614,12 +614,12 @@ static struct qxl_bo *qxl_create_cursor(struct qxl_device *qdev,
user_map.vaddr, size);
}

qxl_bo_vunmap(user_bo);
qxl_bo_vunmap(cursor_bo);
qxl_bo_vunmap_and_unpin(user_bo);
qxl_bo_vunmap_and_unpin(cursor_bo);
return cursor_bo;

err_unmap:
qxl_bo_vunmap(cursor_bo);
qxl_bo_vunmap_and_unpin(cursor_bo);
err_unref:
qxl_bo_unpin(cursor_bo);
qxl_bo_unref(&cursor_bo);
Expand Down Expand Up @@ -1205,7 +1205,7 @@ int qxl_create_monitors_object(struct qxl_device *qdev)
}
qdev->monitors_config_bo = gem_to_qxl_bo(gobj);

ret = qxl_bo_vmap(qdev->monitors_config_bo, &map);
ret = qxl_bo_pin_and_vmap(qdev->monitors_config_bo, &map);
if (ret)
return ret;

Expand Down Expand Up @@ -1236,7 +1236,7 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev)
qdev->monitors_config = NULL;
qdev->ram_header->monitors_config = 0;

ret = qxl_bo_vunmap(qdev->monitors_config_bo);
ret = qxl_bo_vunmap_and_unpin(qdev->monitors_config_bo);
if (ret)
return ret;

Expand Down
13 changes: 11 additions & 2 deletions drivers/gpu/drm/qxl/qxl_object.c
Original file line number Diff line number Diff line change
Expand Up @@ -182,15 +182,23 @@ int qxl_bo_vmap_locked(struct qxl_bo *bo, struct iosys_map *map)
return 0;
}

int qxl_bo_vmap(struct qxl_bo *bo, struct iosys_map *map)
int qxl_bo_pin_and_vmap(struct qxl_bo *bo, struct iosys_map *map)
{
int r;

r = qxl_bo_reserve(bo);
if (r)
return r;

r = qxl_bo_pin_locked(bo);
if (r) {
qxl_bo_unreserve(bo);
return r;
}

r = qxl_bo_vmap_locked(bo, map);
if (r)
qxl_bo_unpin_locked(bo);
qxl_bo_unreserve(bo);
return r;
}
Expand Down Expand Up @@ -241,7 +249,7 @@ void qxl_bo_vunmap_locked(struct qxl_bo *bo)
ttm_bo_vunmap(&bo->tbo, &bo->map);
}

int qxl_bo_vunmap(struct qxl_bo *bo)
int qxl_bo_vunmap_and_unpin(struct qxl_bo *bo)
{
int r;

Expand All @@ -250,6 +258,7 @@ int qxl_bo_vunmap(struct qxl_bo *bo)
return r;

qxl_bo_vunmap_locked(bo);
qxl_bo_unpin_locked(bo);
qxl_bo_unreserve(bo);
return 0;
}
Expand Down
4 changes: 2 additions & 2 deletions drivers/gpu/drm/qxl/qxl_object.h
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,9 @@ extern int qxl_bo_create(struct qxl_device *qdev,
u32 priority,
struct qxl_surface *surf,
struct qxl_bo **bo_ptr);
int qxl_bo_vmap(struct qxl_bo *bo, struct iosys_map *map);
int qxl_bo_pin_and_vmap(struct qxl_bo *bo, struct iosys_map *map);
int qxl_bo_vmap_locked(struct qxl_bo *bo, struct iosys_map *map);
int qxl_bo_vunmap(struct qxl_bo *bo);
int qxl_bo_vunmap_and_unpin(struct qxl_bo *bo);
void qxl_bo_vunmap_locked(struct qxl_bo *bo);
void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, int page_offset);
void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, void *map);
Expand Down

0 comments on commit c537fb4

Please sign in to comment.