Mostly around locking.
Gerd Hoffmann (10): drm/qxl: properly handle device init failures drm/qxl: more fence wait rework drm/qxl: use ttm bo priorities drm/qxl: fix lockdep issue in qxl_alloc_release_reserved drm/qxl: rename qxl_bo_kmap -> qxl_bo_kmap_locked drm/qxl: add qxl_bo_kmap/qxl_bo_kunmap drm/qxl: fix prime kmap drm/qxl: fix monitors object kmap drm/qxl: map/unmap framebuffers in prepare_fb+cleanup_fb callbacks. drm/qxl: add lock asserts to qxl_bo_kmap_locked + qxl_bo_kunmap_locked
drivers/gpu/drm/qxl/qxl_object.h | 5 ++- drivers/gpu/drm/qxl/qxl_cmd.c | 2 +- drivers/gpu/drm/qxl/qxl_display.c | 34 +++++++++----------- drivers/gpu/drm/qxl/qxl_draw.c | 8 ++--- drivers/gpu/drm/qxl/qxl_gem.c | 2 +- drivers/gpu/drm/qxl/qxl_image.c | 2 +- drivers/gpu/drm/qxl/qxl_kms.c | 4 +++ drivers/gpu/drm/qxl/qxl_object.c | 53 +++++++++++++++++++++++++++---- drivers/gpu/drm/qxl/qxl_release.c | 41 +++++++++++++++--------- 9 files changed, 103 insertions(+), 48 deletions(-)
Specifically do not try release resources which where not allocated in the first place.
Cc: Tong Zhang ztong0001@gmail.com Tested-by: Tong Zhang ztong0001@gmail.com Signed-off-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/qxl/qxl_display.c | 3 +++ drivers/gpu/drm/qxl/qxl_kms.c | 4 ++++ 2 files changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index c326412136c5..ec50d2cfd4e1 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -1183,6 +1183,9 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev) { int ret;
+ if (!qdev->monitors_config_bo) + return 0; + qdev->monitors_config = NULL; qdev->ram_header->monitors_config = 0;
diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c index 66d74aaaee06..4dc5ad13f12c 100644 --- a/drivers/gpu/drm/qxl/qxl_kms.c +++ b/drivers/gpu/drm/qxl/qxl_kms.c @@ -288,6 +288,10 @@ void qxl_device_fini(struct qxl_device *qdev) { int cur_idx;
+ /* check if qxl_device_init() was successful (gc_work is initialized last) */ + if (!qdev->gc_work.func) + return; + for (cur_idx = 0; cur_idx < 3; cur_idx++) { if (!qdev->current_release_bo[cur_idx]) continue;
Am 16.02.21 um 12:37 schrieb Gerd Hoffmann:
Specifically do not try release resources which where not allocated in the first place.
I still think this should eventually be resolved by using managed code. But for now
Acked-by: Thomas Zimmermann tzimmermann@suse.de
Cc: Tong Zhang ztong0001@gmail.com Tested-by: Tong Zhang ztong0001@gmail.com Signed-off-by: Gerd Hoffmann kraxel@redhat.com
drivers/gpu/drm/qxl/qxl_display.c | 3 +++ drivers/gpu/drm/qxl/qxl_kms.c | 4 ++++ 2 files changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index c326412136c5..ec50d2cfd4e1 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -1183,6 +1183,9 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev) { int ret;
- if (!qdev->monitors_config_bo)
return 0;- qdev->monitors_config = NULL; qdev->ram_header->monitors_config = 0;
diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c index 66d74aaaee06..4dc5ad13f12c 100644 --- a/drivers/gpu/drm/qxl/qxl_kms.c +++ b/drivers/gpu/drm/qxl/qxl_kms.c @@ -288,6 +288,10 @@ void qxl_device_fini(struct qxl_device *qdev) { int cur_idx;
- /* check if qxl_device_init() was successful (gc_work is initialized last) */
- if (!qdev->gc_work.func)
return;- for (cur_idx = 0; cur_idx < 3; cur_idx++) { if (!qdev->current_release_bo[cur_idx]) continue;
Move qxl_io_notify_oom() call into wait condition. That way the driver will call it again if one call wasn't enough.
Also allows to remove the extra dma_fence_is_signaled() check and the goto.
Fixes: 5a838e5d5825 ("drm/qxl: simplify qxl_fence_wait") Signed-off-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/qxl/qxl_release.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c index 6ed673d75f9f..63818b56218c 100644 --- a/drivers/gpu/drm/qxl/qxl_release.c +++ b/drivers/gpu/drm/qxl/qxl_release.c @@ -62,16 +62,13 @@ static long qxl_fence_wait(struct dma_fence *fence, bool intr,
qdev = container_of(fence->lock, struct qxl_device, release_lock);
- if (dma_fence_is_signaled(fence)) - goto signaled; - - qxl_io_notify_oom(qdev); if (!wait_event_timeout(qdev->release_event, - dma_fence_is_signaled(fence), + (dma_fence_is_signaled(fence) || ( + qxl_io_notify_oom(qdev), + 0)), timeout)) return 0;
-signaled: cur = jiffies; if (time_after(cur, end)) return 0;
Allow to set priorities for buffer objects. Use priority 1 for surface and cursor command releases. Use priority 0 for drawing command releases. That way the short-living drawing commands are first in line when it comes to eviction, making it *much* less likely that ttm_bo_mem_force_space() picks something which can't be evicted and throws an error after waiting a while without success.
Signed-off-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/qxl/qxl_object.h | 1 + drivers/gpu/drm/qxl/qxl_cmd.c | 2 +- drivers/gpu/drm/qxl/qxl_display.c | 4 ++-- drivers/gpu/drm/qxl/qxl_gem.c | 2 +- drivers/gpu/drm/qxl/qxl_object.c | 5 +++-- drivers/gpu/drm/qxl/qxl_release.c | 19 +++++++++++++------ 6 files changed, 21 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h index e60a8f88e226..dc1659e717f1 100644 --- a/drivers/gpu/drm/qxl/qxl_object.h +++ b/drivers/gpu/drm/qxl/qxl_object.h @@ -61,6 +61,7 @@ static inline u64 qxl_bo_mmap_offset(struct qxl_bo *bo) extern int qxl_bo_create(struct qxl_device *qdev, unsigned long size, bool kernel, bool pinned, u32 domain, + u32 priority, struct qxl_surface *surf, struct qxl_bo **bo_ptr); extern int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map); diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c index 7e22a81bfb36..7b00c955cd82 100644 --- a/drivers/gpu/drm/qxl/qxl_cmd.c +++ b/drivers/gpu/drm/qxl/qxl_cmd.c @@ -269,7 +269,7 @@ int qxl_alloc_bo_reserved(struct qxl_device *qdev, int ret;
ret = qxl_bo_create(qdev, size, false /* not kernel - device */, - false, QXL_GEM_DOMAIN_VRAM, NULL, &bo); + false, QXL_GEM_DOMAIN_VRAM, 0, NULL, &bo); if (ret) { DRM_ERROR("failed to allocate VRAM BO\n"); return ret; diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index ec50d2cfd4e1..a1b5cc5918bc 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -799,8 +799,8 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane, qdev->dumb_shadow_bo = NULL; } qxl_bo_create(qdev, surf.height * surf.stride, - true, true, QXL_GEM_DOMAIN_SURFACE, &surf, - &qdev->dumb_shadow_bo); + true, true, QXL_GEM_DOMAIN_SURFACE, 0, + &surf, &qdev->dumb_shadow_bo); } if (user_bo->shadow != qdev->dumb_shadow_bo) { if (user_bo->shadow) { diff --git a/drivers/gpu/drm/qxl/qxl_gem.c b/drivers/gpu/drm/qxl/qxl_gem.c index 48e096285b4c..a08da0bd9098 100644 --- a/drivers/gpu/drm/qxl/qxl_gem.c +++ b/drivers/gpu/drm/qxl/qxl_gem.c @@ -55,7 +55,7 @@ int qxl_gem_object_create(struct qxl_device *qdev, int size, /* At least align on page size */ if (alignment < PAGE_SIZE) alignment = PAGE_SIZE; - r = qxl_bo_create(qdev, size, kernel, false, initial_domain, surf, &qbo); + r = qxl_bo_create(qdev, size, kernel, false, initial_domain, 0, surf, &qbo); if (r) { if (r != -ERESTARTSYS) DRM_ERROR( diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c index 705b51535492..7eada4ad217b 100644 --- a/drivers/gpu/drm/qxl/qxl_object.c +++ b/drivers/gpu/drm/qxl/qxl_object.c @@ -103,8 +103,8 @@ static const struct drm_gem_object_funcs qxl_object_funcs = { .print_info = drm_gem_ttm_print_info, };
-int qxl_bo_create(struct qxl_device *qdev, - unsigned long size, bool kernel, bool pinned, u32 domain, +int qxl_bo_create(struct qxl_device *qdev, unsigned long size, + bool kernel, bool pinned, u32 domain, u32 priority, struct qxl_surface *surf, struct qxl_bo **bo_ptr) { @@ -137,6 +137,7 @@ int qxl_bo_create(struct qxl_device *qdev,
qxl_ttm_placement_from_domain(bo, domain);
+ bo->tbo.priority = priority; r = ttm_bo_init_reserved(&qdev->mman.bdev, &bo->tbo, size, type, &bo->placement, 0, &ctx, NULL, NULL, &qxl_ttm_bo_destroy); diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c index 63818b56218c..a831184e014a 100644 --- a/drivers/gpu/drm/qxl/qxl_release.c +++ b/drivers/gpu/drm/qxl/qxl_release.c @@ -161,11 +161,12 @@ qxl_release_free(struct qxl_device *qdev, }
static int qxl_release_bo_alloc(struct qxl_device *qdev, - struct qxl_bo **bo) + struct qxl_bo **bo, + u32 priority) { /* pin releases bo's they are too messy to evict */ return qxl_bo_create(qdev, PAGE_SIZE, false, true, - QXL_GEM_DOMAIN_VRAM, NULL, bo); + QXL_GEM_DOMAIN_VRAM, priority, NULL, bo); }
int qxl_release_list_add(struct qxl_release *release, struct qxl_bo *bo) @@ -288,13 +289,19 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size, int ret = 0; union qxl_release_info *info; int cur_idx; + u32 priority;
- if (type == QXL_RELEASE_DRAWABLE) + if (type == QXL_RELEASE_DRAWABLE) { cur_idx = 0; - else if (type == QXL_RELEASE_SURFACE_CMD) + priority = 0; + } else if (type == QXL_RELEASE_SURFACE_CMD) { cur_idx = 1; - else if (type == QXL_RELEASE_CURSOR_CMD) + priority = 1; + } + else if (type == QXL_RELEASE_CURSOR_CMD) { cur_idx = 2; + priority = 1; + } else { DRM_ERROR("got illegal type: %d\n", type); return -EINVAL; @@ -316,7 +323,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size, qdev->current_release_bo[cur_idx] = NULL; } if (!qdev->current_release_bo[cur_idx]) { - ret = qxl_release_bo_alloc(qdev, &qdev->current_release_bo[cur_idx]); + ret = qxl_release_bo_alloc(qdev, &qdev->current_release_bo[cur_idx], priority); if (ret) { mutex_unlock(&qdev->release_mutex); qxl_release_free(qdev, *release);
Call qxl_bo_unpin (which does a reservation) without holding the release_mutex lock. Fixes lockdep (correctly) warning on a possible deadlock.
Fixes: 65ffea3c6e73 ("drm/qxl: unpin release objects") Signed-off-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/qxl/qxl_release.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c index a831184e014a..352a11a8485b 100644 --- a/drivers/gpu/drm/qxl/qxl_release.c +++ b/drivers/gpu/drm/qxl/qxl_release.c @@ -284,7 +284,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size, int type, struct qxl_release **release, struct qxl_bo **rbo) { - struct qxl_bo *bo; + struct qxl_bo *bo, *free_bo = NULL; int idr_ret; int ret = 0; union qxl_release_info *info; @@ -317,8 +317,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size,
mutex_lock(&qdev->release_mutex); if (qdev->current_release_bo_offset[cur_idx] + 1 >= releases_per_bo[cur_idx]) { - qxl_bo_unpin(qdev->current_release_bo[cur_idx]); - qxl_bo_unref(&qdev->current_release_bo[cur_idx]); + free_bo = qdev->current_release_bo[cur_idx]; qdev->current_release_bo_offset[cur_idx] = 0; qdev->current_release_bo[cur_idx] = NULL; } @@ -326,6 +325,10 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size, ret = qxl_release_bo_alloc(qdev, &qdev->current_release_bo[cur_idx], priority); if (ret) { mutex_unlock(&qdev->release_mutex); + if (free_bo) { + qxl_bo_unpin(free_bo); + qxl_bo_unref(&free_bo); + } qxl_release_free(qdev, *release); return ret; } @@ -341,6 +344,10 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size, *rbo = bo;
mutex_unlock(&qdev->release_mutex); + if (free_bo) { + qxl_bo_unpin(free_bo); + qxl_bo_unref(&free_bo); + }
ret = qxl_release_list_add(*release, bo); qxl_bo_unref(&bo);
Make clear that these functions should be called with reserved bo's only. No functional change.
Signed-off-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/qxl/qxl_object.h | 4 ++-- drivers/gpu/drm/qxl/qxl_display.c | 14 +++++++------- drivers/gpu/drm/qxl/qxl_draw.c | 8 ++++---- drivers/gpu/drm/qxl/qxl_image.c | 2 +- drivers/gpu/drm/qxl/qxl_object.c | 8 ++++---- drivers/gpu/drm/qxl/qxl_prime.c | 4 ++-- 6 files changed, 20 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h index dc1659e717f1..5bd33650183f 100644 --- a/drivers/gpu/drm/qxl/qxl_object.h +++ b/drivers/gpu/drm/qxl/qxl_object.h @@ -64,8 +64,8 @@ extern int qxl_bo_create(struct qxl_device *qdev, u32 priority, struct qxl_surface *surf, struct qxl_bo **bo_ptr); -extern int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map); -extern void qxl_bo_kunmap(struct qxl_bo *bo); +extern int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map); +extern void qxl_bo_kunmap_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); extern struct qxl_bo *qxl_bo_ref(struct qxl_bo *bo); diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index a1b5cc5918bc..8672385a1caf 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -600,7 +600,7 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, user_bo = gem_to_qxl_bo(obj);
/* pinning is done in the prepare/cleanup framevbuffer */ - ret = qxl_bo_kmap(user_bo, &user_map); + ret = qxl_bo_kmap_locked(user_bo, &user_map); if (ret) goto out_free_release; user_ptr = user_map.vaddr; /* TODO: Use mapping abstraction properly */ @@ -619,7 +619,7 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, if (ret) goto out_unpin;
- ret = qxl_bo_kmap(cursor_bo, &cursor_map); + ret = qxl_bo_kmap_locked(cursor_bo, &cursor_map); if (ret) goto out_backoff; if (cursor_map.is_iomem) /* TODO: Use mapping abstraction properly */ @@ -638,8 +638,8 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, cursor->chunk.prev_chunk = 0; cursor->chunk.data_size = size; memcpy(cursor->chunk.data, user_ptr, size); - qxl_bo_kunmap(cursor_bo); - qxl_bo_kunmap(user_bo); + qxl_bo_kunmap_locked(cursor_bo); + qxl_bo_kunmap_locked(user_bo);
cmd = (struct qxl_cursor_cmd *) qxl_release_map(qdev, release); cmd->u.set.visible = 1; @@ -681,7 +681,7 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, out_free_bo: qxl_bo_unref(&cursor_bo); out_kunmap: - qxl_bo_kunmap(user_bo); + qxl_bo_kunmap_locked(user_bo); out_free_release: qxl_release_free(qdev, release); return; @@ -1163,7 +1163,7 @@ int qxl_create_monitors_object(struct qxl_device *qdev) if (ret) return ret;
- qxl_bo_kmap(qdev->monitors_config_bo, &map); + qxl_bo_kmap_locked(qdev->monitors_config_bo, &map);
qdev->monitors_config = qdev->monitors_config_bo->kptr; qdev->ram_header->monitors_config = @@ -1189,7 +1189,7 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev) qdev->monitors_config = NULL; qdev->ram_header->monitors_config = 0;
- qxl_bo_kunmap(qdev->monitors_config_bo); + qxl_bo_kunmap_locked(qdev->monitors_config_bo); ret = qxl_bo_unpin(qdev->monitors_config_bo); if (ret) return ret; diff --git a/drivers/gpu/drm/qxl/qxl_draw.c b/drivers/gpu/drm/qxl/qxl_draw.c index 7b7acb910780..294542d49015 100644 --- a/drivers/gpu/drm/qxl/qxl_draw.c +++ b/drivers/gpu/drm/qxl/qxl_draw.c @@ -48,7 +48,7 @@ static struct qxl_rect *drawable_set_clipping(struct qxl_device *qdev, struct qxl_clip_rects *dev_clips; int ret;
- ret = qxl_bo_kmap(clips_bo, &map); + ret = qxl_bo_kmap_locked(clips_bo, &map); if (ret) return NULL; dev_clips = map.vaddr; /* TODO: Use mapping abstraction properly */ @@ -202,7 +202,7 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev, if (ret) goto out_release_backoff;
- ret = qxl_bo_kmap(bo, &surface_map); + ret = qxl_bo_kmap_locked(bo, &surface_map); if (ret) goto out_release_backoff; surface_base = surface_map.vaddr; /* TODO: Use mapping abstraction properly */ @@ -210,7 +210,7 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev, ret = qxl_image_init(qdev, release, dimage, surface_base, left - dumb_shadow_offset, top, width, height, depth, stride); - qxl_bo_kunmap(bo); + qxl_bo_kunmap_locked(bo); if (ret) goto out_release_backoff;
@@ -247,7 +247,7 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev, rects[i].top = clips_ptr->y1; rects[i].bottom = clips_ptr->y2; } - qxl_bo_kunmap(clips_bo); + qxl_bo_kunmap_locked(clips_bo);
qxl_release_fence_buffer_objects(release); qxl_push_command_ring_release(qdev, release, QXL_CMD_DRAW, false); diff --git a/drivers/gpu/drm/qxl/qxl_image.c b/drivers/gpu/drm/qxl/qxl_image.c index 60ab7151b84d..d4db21ca1d5d 100644 --- a/drivers/gpu/drm/qxl/qxl_image.c +++ b/drivers/gpu/drm/qxl/qxl_image.c @@ -186,7 +186,7 @@ qxl_image_init_helper(struct qxl_device *qdev, } } } - qxl_bo_kunmap(chunk_bo); + qxl_bo_kunmap_locked(chunk_bo);
image_bo = dimage->bo; ptr = qxl_bo_kmap_atomic_page(qdev, image_bo, 0); diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c index 7eada4ad217b..b56d4dfc28cb 100644 --- a/drivers/gpu/drm/qxl/qxl_object.c +++ b/drivers/gpu/drm/qxl/qxl_object.c @@ -155,7 +155,7 @@ int qxl_bo_create(struct qxl_device *qdev, unsigned long size, return 0; }
-int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map) +int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map) { int r;
@@ -203,7 +203,7 @@ void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, return rptr; }
- ret = qxl_bo_kmap(bo, &bo_map); + ret = qxl_bo_kmap_locked(bo, &bo_map); if (ret) return NULL; rptr = bo_map.vaddr; /* TODO: Use mapping abstraction properly */ @@ -212,7 +212,7 @@ void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, return rptr; }
-void qxl_bo_kunmap(struct qxl_bo *bo) +void qxl_bo_kunmap_locked(struct qxl_bo *bo) { if (bo->kptr == NULL) return; @@ -233,7 +233,7 @@ void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev, io_mapping_unmap_atomic(pmap); return; fallback: - qxl_bo_kunmap(bo); + qxl_bo_kunmap_locked(bo); }
void qxl_bo_unref(struct qxl_bo **bo) diff --git a/drivers/gpu/drm/qxl/qxl_prime.c b/drivers/gpu/drm/qxl/qxl_prime.c index 4aa949799446..dc295b2e2b52 100644 --- a/drivers/gpu/drm/qxl/qxl_prime.c +++ b/drivers/gpu/drm/qxl/qxl_prime.c @@ -59,7 +59,7 @@ int qxl_gem_prime_vmap(struct drm_gem_object *obj, struct dma_buf_map *map) struct qxl_bo *bo = gem_to_qxl_bo(obj); int ret;
- ret = qxl_bo_kmap(bo, map); + ret = qxl_bo_kmap_locked(bo, map); if (ret < 0) return ret;
@@ -71,7 +71,7 @@ void qxl_gem_prime_vunmap(struct drm_gem_object *obj, { struct qxl_bo *bo = gem_to_qxl_bo(obj);
- qxl_bo_kunmap(bo); + qxl_bo_kunmap_locked(bo); }
int qxl_gem_prime_mmap(struct drm_gem_object *obj,
Hi
Am 16.02.21 um 12:37 schrieb Gerd Hoffmann:
Make clear that these functions should be called with reserved bo's only. No functional change.
Signed-off-by: Gerd Hoffmann kraxel@redhat.com
drivers/gpu/drm/qxl/qxl_object.h | 4 ++-- drivers/gpu/drm/qxl/qxl_display.c | 14 +++++++------- drivers/gpu/drm/qxl/qxl_draw.c | 8 ++++---- drivers/gpu/drm/qxl/qxl_image.c | 2 +- drivers/gpu/drm/qxl/qxl_object.c | 8 ++++---- drivers/gpu/drm/qxl/qxl_prime.c | 4 ++-- 6 files changed, 20 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h index dc1659e717f1..5bd33650183f 100644 --- a/drivers/gpu/drm/qxl/qxl_object.h +++ b/drivers/gpu/drm/qxl/qxl_object.h @@ -64,8 +64,8 @@ extern int qxl_bo_create(struct qxl_device *qdev, u32 priority, struct qxl_surface *surf, struct qxl_bo **bo_ptr); -extern int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map); -extern void qxl_bo_kunmap(struct qxl_bo *bo); +extern int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map); +extern void qxl_bo_kunmap_locked(struct qxl_bo *bo);
Nowadaways kmap/kunmap is a misnomer. Since you're renaming; maybe rename them to vmap/vunmap.
Best regards Thomas
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); extern struct qxl_bo *qxl_bo_ref(struct qxl_bo *bo); diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index a1b5cc5918bc..8672385a1caf 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -600,7 +600,7 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, user_bo = gem_to_qxl_bo(obj);
/* pinning is done in the prepare/cleanup framevbuffer */
ret = qxl_bo_kmap(user_bo, &user_map);
ret = qxl_bo_kmap_locked(user_bo, &user_map);@@ -619,7 +619,7 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, if (ret) goto out_unpin;
ret = qxl_bo_kmap(cursor_bo, &cursor_map);
ret = qxl_bo_kmap_locked(cursor_bo, &cursor_map);@@ -638,8 +638,8 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, cursor->chunk.prev_chunk = 0; cursor->chunk.data_size = size; memcpy(cursor->chunk.data, user_ptr, size);
qxl_bo_kunmap(cursor_bo);qxl_bo_kunmap(user_bo);
qxl_bo_kunmap_locked(cursor_bo);qxl_bo_kunmap_locked(user_bo);cmd = (struct qxl_cursor_cmd *) qxl_release_map(qdev, release); cmd->u.set.visible = 1;
@@ -681,7 +681,7 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, out_free_bo: qxl_bo_unref(&cursor_bo); out_kunmap:
- qxl_bo_kunmap(user_bo);
- qxl_bo_kunmap_locked(user_bo); out_free_release: qxl_release_free(qdev, release); return;
@@ -1163,7 +1163,7 @@ int qxl_create_monitors_object(struct qxl_device *qdev) if (ret) return ret;
- qxl_bo_kmap(qdev->monitors_config_bo, &map);
qxl_bo_kmap_locked(qdev->monitors_config_bo, &map);
qdev->monitors_config = qdev->monitors_config_bo->kptr; qdev->ram_header->monitors_config =
@@ -1189,7 +1189,7 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev) qdev->monitors_config = NULL; qdev->ram_header->monitors_config = 0;
- qxl_bo_kunmap(qdev->monitors_config_bo);
- qxl_bo_kunmap_locked(qdev->monitors_config_bo); ret = qxl_bo_unpin(qdev->monitors_config_bo); if (ret) return ret;
diff --git a/drivers/gpu/drm/qxl/qxl_draw.c b/drivers/gpu/drm/qxl/qxl_draw.c index 7b7acb910780..294542d49015 100644 --- a/drivers/gpu/drm/qxl/qxl_draw.c +++ b/drivers/gpu/drm/qxl/qxl_draw.c @@ -48,7 +48,7 @@ static struct qxl_rect *drawable_set_clipping(struct qxl_device *qdev, struct qxl_clip_rects *dev_clips; int ret;
- ret = qxl_bo_kmap(clips_bo, &map);
- ret = qxl_bo_kmap_locked(clips_bo, &map); if (ret) return NULL; dev_clips = map.vaddr; /* TODO: Use mapping abstraction properly */
@@ -202,7 +202,7 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev, if (ret) goto out_release_backoff;
- ret = qxl_bo_kmap(bo, &surface_map);
- ret = qxl_bo_kmap_locked(bo, &surface_map); if (ret) goto out_release_backoff; surface_base = surface_map.vaddr; /* TODO: Use mapping abstraction properly */
@@ -210,7 +210,7 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev, ret = qxl_image_init(qdev, release, dimage, surface_base, left - dumb_shadow_offset, top, width, height, depth, stride);
- qxl_bo_kunmap(bo);
- qxl_bo_kunmap_locked(bo); if (ret) goto out_release_backoff;
@@ -247,7 +247,7 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev, rects[i].top = clips_ptr->y1; rects[i].bottom = clips_ptr->y2; }
- qxl_bo_kunmap(clips_bo);
qxl_bo_kunmap_locked(clips_bo);
qxl_release_fence_buffer_objects(release); qxl_push_command_ring_release(qdev, release, QXL_CMD_DRAW, false);
diff --git a/drivers/gpu/drm/qxl/qxl_image.c b/drivers/gpu/drm/qxl/qxl_image.c index 60ab7151b84d..d4db21ca1d5d 100644 --- a/drivers/gpu/drm/qxl/qxl_image.c +++ b/drivers/gpu/drm/qxl/qxl_image.c @@ -186,7 +186,7 @@ qxl_image_init_helper(struct qxl_device *qdev, } } }
- qxl_bo_kunmap(chunk_bo);
qxl_bo_kunmap_locked(chunk_bo);
image_bo = dimage->bo; ptr = qxl_bo_kmap_atomic_page(qdev, image_bo, 0);
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c index 7eada4ad217b..b56d4dfc28cb 100644 --- a/drivers/gpu/drm/qxl/qxl_object.c +++ b/drivers/gpu/drm/qxl/qxl_object.c @@ -155,7 +155,7 @@ int qxl_bo_create(struct qxl_device *qdev, unsigned long size, return 0; }
-int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map) +int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map) { int r;
@@ -203,7 +203,7 @@ void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, return rptr; }
- ret = qxl_bo_kmap(bo, &bo_map);
- ret = qxl_bo_kmap_locked(bo, &bo_map); if (ret) return NULL; rptr = bo_map.vaddr; /* TODO: Use mapping abstraction properly */
@@ -212,7 +212,7 @@ void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, return rptr; }
-void qxl_bo_kunmap(struct qxl_bo *bo) +void qxl_bo_kunmap_locked(struct qxl_bo *bo) { if (bo->kptr == NULL) return; @@ -233,7 +233,7 @@ void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev, io_mapping_unmap_atomic(pmap); return; fallback:
- qxl_bo_kunmap(bo);
qxl_bo_kunmap_locked(bo); }
void qxl_bo_unref(struct qxl_bo **bo)
diff --git a/drivers/gpu/drm/qxl/qxl_prime.c b/drivers/gpu/drm/qxl/qxl_prime.c index 4aa949799446..dc295b2e2b52 100644 --- a/drivers/gpu/drm/qxl/qxl_prime.c +++ b/drivers/gpu/drm/qxl/qxl_prime.c @@ -59,7 +59,7 @@ int qxl_gem_prime_vmap(struct drm_gem_object *obj, struct dma_buf_map *map) struct qxl_bo *bo = gem_to_qxl_bo(obj); int ret;
- ret = qxl_bo_kmap(bo, map);
- ret = qxl_bo_kmap_locked(bo, map); if (ret < 0) return ret;
@@ -71,7 +71,7 @@ void qxl_gem_prime_vunmap(struct drm_gem_object *obj, { struct qxl_bo *bo = gem_to_qxl_bo(obj);
- qxl_bo_kunmap(bo);
qxl_bo_kunmap_locked(bo); }
int qxl_gem_prime_mmap(struct drm_gem_object *obj,
Add kmap/kunmap variants which reserve (and pin) the bo. They can be used in case the caller doesn't hold a reservation for the bo.
Signed-off-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/qxl/qxl_object.h | 2 ++ drivers/gpu/drm/qxl/qxl_object.c | 36 ++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h index 5bd33650183f..360972ae4869 100644 --- a/drivers/gpu/drm/qxl/qxl_object.h +++ b/drivers/gpu/drm/qxl/qxl_object.h @@ -64,7 +64,9 @@ extern int qxl_bo_create(struct qxl_device *qdev, u32 priority, struct qxl_surface *surf, struct qxl_bo **bo_ptr); +extern int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map); extern int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map); +extern int qxl_bo_kunmap(struct qxl_bo *bo); extern void qxl_bo_kunmap_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); diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c index b56d4dfc28cb..22748b9566af 100644 --- a/drivers/gpu/drm/qxl/qxl_object.c +++ b/drivers/gpu/drm/qxl/qxl_object.c @@ -29,6 +29,9 @@ #include "qxl_drv.h" #include "qxl_object.h"
+static int __qxl_bo_pin(struct qxl_bo *bo); +static void __qxl_bo_unpin(struct qxl_bo *bo); + static void qxl_ttm_bo_destroy(struct ttm_buffer_object *tbo) { struct qxl_bo *bo; @@ -179,6 +182,25 @@ int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map) return 0; }
+int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map) +{ + int r; + + r = qxl_bo_reserve(bo); + if (r) + return r; + + r = __qxl_bo_pin(bo); + if (r) { + qxl_bo_unreserve(bo); + return r; + } + + r = qxl_bo_kmap_locked(bo, map); + qxl_bo_unreserve(bo); + return r; +} + void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, int page_offset) { @@ -223,6 +245,20 @@ void qxl_bo_kunmap_locked(struct qxl_bo *bo) ttm_bo_vunmap(&bo->tbo, &bo->map); }
+int qxl_bo_kunmap(struct qxl_bo *bo) +{ + int r; + + r = qxl_bo_reserve(bo); + if (r) + return r; + + qxl_bo_kunmap_locked(bo); + __qxl_bo_unpin(bo); + qxl_bo_unreserve(bo); + return 0; +} + void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, void *pmap) {
Hi
Am 16.02.21 um 12:37 schrieb Gerd Hoffmann:
Add kmap/kunmap variants which reserve (and pin) the bo. They can be used in case the caller doesn't hold a reservation for the bo.
Again, these functions should rather be called vmap/vunamp.
Signed-off-by: Gerd Hoffmann kraxel@redhat.com
drivers/gpu/drm/qxl/qxl_object.h | 2 ++ drivers/gpu/drm/qxl/qxl_object.c | 36 ++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h index 5bd33650183f..360972ae4869 100644 --- a/drivers/gpu/drm/qxl/qxl_object.h +++ b/drivers/gpu/drm/qxl/qxl_object.h @@ -64,7 +64,9 @@ extern int qxl_bo_create(struct qxl_device *qdev, u32 priority, struct qxl_surface *surf, struct qxl_bo **bo_ptr); +extern int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map); extern int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map); +extern int qxl_bo_kunmap(struct qxl_bo *bo); extern void qxl_bo_kunmap_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); diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c index b56d4dfc28cb..22748b9566af 100644 --- a/drivers/gpu/drm/qxl/qxl_object.c +++ b/drivers/gpu/drm/qxl/qxl_object.c @@ -29,6 +29,9 @@ #include "qxl_drv.h" #include "qxl_object.h"
+static int __qxl_bo_pin(struct qxl_bo *bo); +static void __qxl_bo_unpin(struct qxl_bo *bo);
- static void qxl_ttm_bo_destroy(struct ttm_buffer_object *tbo) { struct qxl_bo *bo;
@@ -179,6 +182,25 @@ int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map) return 0; }
+int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map) +{
- int r;
- r = qxl_bo_reserve(bo);
- if (r)
return r;- r = __qxl_bo_pin(bo);
- if (r) {
qxl_bo_unreserve(bo);return r;- }
- r = qxl_bo_kmap_locked(bo, map);
- qxl_bo_unreserve(bo);
- return r;
+}
- void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, int page_offset) {
@@ -223,6 +245,20 @@ void qxl_bo_kunmap_locked(struct qxl_bo *bo) ttm_bo_vunmap(&bo->tbo, &bo->map); }
+int qxl_bo_kunmap(struct qxl_bo *bo) +{
- int r;
- r = qxl_bo_reserve(bo);
- if (r)
return r;- qxl_bo_kunmap_locked(bo);
- __qxl_bo_unpin(bo);
- qxl_bo_unreserve(bo);
- return 0;
+}
- void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, void *pmap) {
Use the correct kmap variant. We don't have a reservation here, so we can't use the _locked version.
Signed-off-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/qxl/qxl_prime.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_prime.c b/drivers/gpu/drm/qxl/qxl_prime.c index dc295b2e2b52..4aa949799446 100644 --- a/drivers/gpu/drm/qxl/qxl_prime.c +++ b/drivers/gpu/drm/qxl/qxl_prime.c @@ -59,7 +59,7 @@ int qxl_gem_prime_vmap(struct drm_gem_object *obj, struct dma_buf_map *map) struct qxl_bo *bo = gem_to_qxl_bo(obj); int ret;
- ret = qxl_bo_kmap_locked(bo, map); + ret = qxl_bo_kmap(bo, map); if (ret < 0) return ret;
@@ -71,7 +71,7 @@ void qxl_gem_prime_vunmap(struct drm_gem_object *obj, { struct qxl_bo *bo = gem_to_qxl_bo(obj);
- qxl_bo_kunmap_locked(bo); + qxl_bo_kunmap(bo); }
int qxl_gem_prime_mmap(struct drm_gem_object *obj,
Hi
Am 16.02.21 um 12:37 schrieb Gerd Hoffmann:
Use the correct kmap variant. We don't have a reservation here, so we can't use the _locked version.
I'd merge this change into patch 6. So the new functions come with a caller.
Best regards Thomas
Signed-off-by: Gerd Hoffmann kraxel@redhat.com
drivers/gpu/drm/qxl/qxl_prime.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_prime.c b/drivers/gpu/drm/qxl/qxl_prime.c index dc295b2e2b52..4aa949799446 100644 --- a/drivers/gpu/drm/qxl/qxl_prime.c +++ b/drivers/gpu/drm/qxl/qxl_prime.c @@ -59,7 +59,7 @@ int qxl_gem_prime_vmap(struct drm_gem_object *obj, struct dma_buf_map *map) struct qxl_bo *bo = gem_to_qxl_bo(obj); int ret;
- ret = qxl_bo_kmap_locked(bo, map);
- ret = qxl_bo_kmap(bo, map); if (ret < 0) return ret;
@@ -71,7 +71,7 @@ void qxl_gem_prime_vunmap(struct drm_gem_object *obj, { struct qxl_bo *bo = gem_to_qxl_bo(obj);
- qxl_bo_kunmap_locked(bo);
qxl_bo_kunmap(bo); }
int qxl_gem_prime_mmap(struct drm_gem_object *obj,
Use the correct kmap variant. We don't hold a reservation here, so we can't use the _locked variant. We can drop the pin because qxl_bo_kmap will do that for us.
Signed-off-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/qxl/qxl_display.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index 8672385a1caf..7500560db8e4 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -1159,12 +1159,10 @@ int qxl_create_monitors_object(struct qxl_device *qdev) } qdev->monitors_config_bo = gem_to_qxl_bo(gobj);
- ret = qxl_bo_pin(qdev->monitors_config_bo); + ret = qxl_bo_kmap(qdev->monitors_config_bo, &map); if (ret) return ret;
- qxl_bo_kmap_locked(qdev->monitors_config_bo, &map); - qdev->monitors_config = qdev->monitors_config_bo->kptr; qdev->ram_header->monitors_config = qxl_bo_physical_address(qdev, qdev->monitors_config_bo, 0); @@ -1189,8 +1187,7 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev) qdev->monitors_config = NULL; qdev->ram_header->monitors_config = 0;
- qxl_bo_kunmap_locked(qdev->monitors_config_bo); - ret = qxl_bo_unpin(qdev->monitors_config_bo); + ret = qxl_bo_kunmap(qdev->monitors_config_bo); if (ret) return ret;
Am 16.02.21 um 12:37 schrieb Gerd Hoffmann:
Use the correct kmap variant. We don't hold a reservation here, so we can't use the _locked variant. We can drop the pin because qxl_bo_kmap will do that for us.
Signed-off-by: Gerd Hoffmann kraxel@redhat.com
Acked-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/qxl/qxl_display.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index 8672385a1caf..7500560db8e4 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -1159,12 +1159,10 @@ int qxl_create_monitors_object(struct qxl_device *qdev) } qdev->monitors_config_bo = gem_to_qxl_bo(gobj);
- ret = qxl_bo_pin(qdev->monitors_config_bo);
- ret = qxl_bo_kmap(qdev->monitors_config_bo, &map); if (ret) return ret;
- qxl_bo_kmap_locked(qdev->monitors_config_bo, &map);
- qdev->monitors_config = qdev->monitors_config_bo->kptr; qdev->ram_header->monitors_config = qxl_bo_physical_address(qdev, qdev->monitors_config_bo, 0);
@@ -1189,8 +1187,7 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev) qdev->monitors_config = NULL; qdev->ram_header->monitors_config = 0;
- qxl_bo_kunmap_locked(qdev->monitors_config_bo);
- ret = qxl_bo_unpin(qdev->monitors_config_bo);
- ret = qxl_bo_kunmap(qdev->monitors_config_bo); if (ret) return ret;
We don't have to map in atomic_update callback then, making locking a bit less complicated.
Signed-off-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/qxl/qxl_display.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index 7500560db8e4..39b8c5116d34 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -584,7 +584,6 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, 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; @@ -599,11 +598,8 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, obj = fb->obj[0]; user_bo = gem_to_qxl_bo(obj);
- /* pinning is done in the prepare/cleanup framevbuffer */ - ret = qxl_bo_kmap_locked(user_bo, &user_map); - if (ret) - goto out_free_release; - user_ptr = user_map.vaddr; /* TODO: Use mapping abstraction properly */ + /* mapping is done in the prepare/cleanup framevbuffer */ + user_ptr = user_bo->map.vaddr; /* TODO: Use mapping abstraction properly */
ret = qxl_alloc_bo_reserved(qdev, release, sizeof(struct qxl_cursor) + size, @@ -639,7 +635,6 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, cursor->chunk.data_size = size; memcpy(cursor->chunk.data, user_ptr, size); qxl_bo_kunmap_locked(cursor_bo); - qxl_bo_kunmap_locked(user_bo);
cmd = (struct qxl_cursor_cmd *) qxl_release_map(qdev, release); cmd->u.set.visible = 1; @@ -778,6 +773,7 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane, struct drm_gem_object *obj; struct qxl_bo *user_bo; struct qxl_surface surf; + struct dma_buf_map unused;
if (!new_state->fb) return 0; @@ -815,7 +811,7 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane, } }
- return qxl_bo_pin(user_bo); + return qxl_bo_kmap(user_bo, &unused); }
static void qxl_plane_cleanup_fb(struct drm_plane *plane, @@ -834,7 +830,7 @@ static void qxl_plane_cleanup_fb(struct drm_plane *plane,
obj = old_state->fb->obj[0]; user_bo = gem_to_qxl_bo(obj); - qxl_bo_unpin(user_bo); + qxl_bo_kunmap(user_bo);
if (old_state->fb != plane->state->fb && user_bo->shadow) { qxl_bo_unpin(user_bo->shadow);
Hi
this is a shadow-buffered plane. Did you consider using the new helpers for shadow-buffered planes? They will map the user BO for you and provide the mapping in the plane state.
From there, you should implement your own plane state on top of struct drm_shadow_plane_state, and also move all the other allocations and vmaps into prepare_fb and cleanup_fb. Most of this is not actually allowed in commit tails. All we'd have to do is to export the reset, duplicate and destroy code; similar to what __drm_atomic_helper_plane_reset() does.
Best regards Thomas
Am 16.02.21 um 12:37 schrieb Gerd Hoffmann:
We don't have to map in atomic_update callback then, making locking a bit less complicated.
Signed-off-by: Gerd Hoffmann kraxel@redhat.com
drivers/gpu/drm/qxl/qxl_display.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index 7500560db8e4..39b8c5116d34 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -584,7 +584,6 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, 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;
@@ -599,11 +598,8 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, obj = fb->obj[0]; user_bo = gem_to_qxl_bo(obj);
/* pinning is done in the prepare/cleanup framevbuffer */ret = qxl_bo_kmap_locked(user_bo, &user_map);if (ret)goto out_free_release;user_ptr = user_map.vaddr; /* TODO: Use mapping abstraction properly */
/* mapping is done in the prepare/cleanup framevbuffer */user_ptr = user_bo->map.vaddr; /* TODO: Use mapping abstraction properly */ret = qxl_alloc_bo_reserved(qdev, release, sizeof(struct qxl_cursor) + size,
@@ -639,7 +635,6 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, cursor->chunk.data_size = size; memcpy(cursor->chunk.data, user_ptr, size); qxl_bo_kunmap_locked(cursor_bo);
qxl_bo_kunmap_locked(user_bo);cmd = (struct qxl_cursor_cmd *) qxl_release_map(qdev, release); cmd->u.set.visible = 1;
@@ -778,6 +773,7 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane, struct drm_gem_object *obj; struct qxl_bo *user_bo; struct qxl_surface surf;
struct dma_buf_map unused;
if (!new_state->fb) return 0;
@@ -815,7 +811,7 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane, } }
- return qxl_bo_pin(user_bo);
return qxl_bo_kmap(user_bo, &unused); }
static void qxl_plane_cleanup_fb(struct drm_plane *plane,
@@ -834,7 +830,7 @@ static void qxl_plane_cleanup_fb(struct drm_plane *plane,
obj = old_state->fb->obj[0]; user_bo = gem_to_qxl_bo(obj);
- qxl_bo_unpin(user_bo);
qxl_bo_kunmap(user_bo);
if (old_state->fb != plane->state->fb && user_bo->shadow) { qxl_bo_unpin(user_bo->shadow);
Am 16.02.21 um 14:27 schrieb Thomas Zimmermann:
Hi
this is a shadow-buffered plane. Did you consider using the new helpers for shadow-buffered planes? They will map the user BO for you and provide the mapping in the plane state.
From there, you should implement your own plane state on top of struct drm_shadow_plane_state, and also move all the other allocations and vmaps into prepare_fb and cleanup_fb. Most of this is not actually allowed in commit tails. All we'd have to do is to export the reset, duplicate and destroy code; similar to what __drm_atomic_helper_plane_reset() does.
AFAICT the cursor_bo is used to implement double buffering for the cursor image.
Ideally, you can do what ast does: pre-allocate/vmap 2 BOs at the end of the vram. Then pageflip between them in atomic_update(). Resolves all the allocation and mapping headaches.
Best regards Thomas
Best regards Thomas
Am 16.02.21 um 12:37 schrieb Gerd Hoffmann:
We don't have to map in atomic_update callback then, making locking a bit less complicated.
Signed-off-by: Gerd Hoffmann kraxel@redhat.com
drivers/gpu/drm/qxl/qxl_display.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index 7500560db8e4..39b8c5116d34 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -584,7 +584,6 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, 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; @@ -599,11 +598,8 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, obj = fb->obj[0]; user_bo = gem_to_qxl_bo(obj); - /* pinning is done in the prepare/cleanup framevbuffer */ - ret = qxl_bo_kmap_locked(user_bo, &user_map); - if (ret) - goto out_free_release; - user_ptr = user_map.vaddr; /* TODO: Use mapping abstraction properly */ + /* mapping is done in the prepare/cleanup framevbuffer */ + user_ptr = user_bo->map.vaddr; /* TODO: Use mapping abstraction properly */ ret = qxl_alloc_bo_reserved(qdev, release, sizeof(struct qxl_cursor) + size, @@ -639,7 +635,6 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, cursor->chunk.data_size = size; memcpy(cursor->chunk.data, user_ptr, size); qxl_bo_kunmap_locked(cursor_bo); - qxl_bo_kunmap_locked(user_bo); cmd = (struct qxl_cursor_cmd *) qxl_release_map(qdev, release); cmd->u.set.visible = 1; @@ -778,6 +773,7 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane, struct drm_gem_object *obj; struct qxl_bo *user_bo; struct qxl_surface surf; + struct dma_buf_map unused; if (!new_state->fb) return 0; @@ -815,7 +811,7 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane, } } - return qxl_bo_pin(user_bo); + return qxl_bo_kmap(user_bo, &unused); } static void qxl_plane_cleanup_fb(struct drm_plane *plane, @@ -834,7 +830,7 @@ static void qxl_plane_cleanup_fb(struct drm_plane *plane, obj = old_state->fb->obj[0]; user_bo = gem_to_qxl_bo(obj); - qxl_bo_unpin(user_bo); + qxl_bo_kunmap(user_bo); if (old_state->fb != plane->state->fb && user_bo->shadow) { qxl_bo_unpin(user_bo->shadow);
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Feb 16, 2021 at 02:46:21PM +0100, Thomas Zimmermann wrote:
Am 16.02.21 um 14:27 schrieb Thomas Zimmermann:
Hi
this is a shadow-buffered plane. Did you consider using the new helpers for shadow-buffered planes? They will map the user BO for you and provide the mapping in the plane state.
From there, you should implement your own plane state on top of struct drm_shadow_plane_state, and also move all the other allocations and vmaps into prepare_fb and cleanup_fb. Most of this is not actually allowed in commit tails. All we'd have to do is to export the reset, duplicate and destroy code; similar to what __drm_atomic_helper_plane_reset() does.
AFAICT the cursor_bo is used to implement double buffering for the cursor image.
Ideally, you can do what ast does: pre-allocate/vmap 2 BOs at the end of the vram. Then pageflip between them in atomic_update(). Resolves all the allocation and mapping headaches.
Just waded through the ast patches.
It is not that simple for qxl. You have to send a command to the virtualization host and take care of the host accessing that memory when processing the command, so you can't reuse the memory until the host signals it is fine to do so.
But, yes, it should be possible to handle cursor_bo creation in prepare_fb without too much effort.
take care, Gerd
Hi
Am 17.02.21 um 11:02 schrieb Gerd Hoffmann:
On Tue, Feb 16, 2021 at 02:46:21PM +0100, Thomas Zimmermann wrote:
Am 16.02.21 um 14:27 schrieb Thomas Zimmermann:
Hi
this is a shadow-buffered plane. Did you consider using the new helpers for shadow-buffered planes? They will map the user BO for you and provide the mapping in the plane state.
From there, you should implement your own plane state on top of struct drm_shadow_plane_state, and also move all the other allocations and vmaps into prepare_fb and cleanup_fb. Most of this is not actually allowed in commit tails. All we'd have to do is to export the reset, duplicate and destroy code; similar to what __drm_atomic_helper_plane_reset() does.
AFAICT the cursor_bo is used to implement double buffering for the cursor image.
Ideally, you can do what ast does: pre-allocate/vmap 2 BOs at the end of the vram. Then pageflip between them in atomic_update(). Resolves all the allocation and mapping headaches.
Just waded through the ast patches.
I just received your ack. Thanks a lot for looking at the ast patches.
It is not that simple for qxl. You have to send a command to the virtualization host and take care of the host accessing that memory when processing the command, so you can't reuse the memory until the host signals it is fine to do so.
But, yes, it should be possible to handle cursor_bo creation in prepare_fb without too much effort.
I've been thinking about this issue and here's an idea:
If you take the ast code as a blueprint, you'd store two cursor bo in a cursor-plane structure. Aditionally each of these BOs would have a pointer to a fence associated with it.
One idea for the fencing code would be to allocate each new fence in prepare_fb and store it in the cursor plane state. In atomic_update, pick the unused BO in the cursor plane and wait on its fence. This should guarantee that the BO is available. (?) Then swap the BO's fence with the one in the cursor plane state. Setup the new fence for synchronization with the host. Next time you pick this cursor BO, the fence will be there for synchronization. The old fence from the cursor BO will now be stored in the cursor-plane state and can be freed in cleanup_fb().
My main interest here is to move all fail-able/locking calls out of the atomic_update function. I might be missing some crucial corner case, but this should resolve the issue. (?) In any case, it's maybe worth a separate patchset.
Best regards Thomas
take care, Gerd
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Try avoid re-introducing locking bugs.
Signed-off-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/qxl/qxl_object.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c index 22748b9566af..90d5e5b7f927 100644 --- a/drivers/gpu/drm/qxl/qxl_object.c +++ b/drivers/gpu/drm/qxl/qxl_object.c @@ -162,6 +162,8 @@ int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map) { int r;
+ dma_resv_assert_held(bo->tbo.base.resv); + if (bo->kptr) { bo->map_count++; goto out; @@ -236,6 +238,8 @@ void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev,
void qxl_bo_kunmap_locked(struct qxl_bo *bo) { + dma_resv_assert_held(bo->tbo.base.resv); + if (bo->kptr == NULL) return; bo->map_count--;
Hi
Am 16.02.21 um 12:37 schrieb Gerd Hoffmann:
Try avoid re-introducing locking bugs.
Signed-off-by: Gerd Hoffmann kraxel@redhat.com
Acked-by: Thomas Zimmermann tzimmermann@suse.de
IMHO this should eventually be done in the rsp TTM functions. But so far, I'd expect this to break some drivers.
Best regards Thomas
drivers/gpu/drm/qxl/qxl_object.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c index 22748b9566af..90d5e5b7f927 100644 --- a/drivers/gpu/drm/qxl/qxl_object.c +++ b/drivers/gpu/drm/qxl/qxl_object.c @@ -162,6 +162,8 @@ int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map) { int r;
- dma_resv_assert_held(bo->tbo.base.resv);
- if (bo->kptr) { bo->map_count++; goto out;
@@ -236,6 +238,8 @@ void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev,
void qxl_bo_kunmap_locked(struct qxl_bo *bo) {
- dma_resv_assert_held(bo->tbo.base.resv);
- if (bo->kptr == NULL) return; bo->map_count--;
dri-devel@lists.freedesktop.org
-
Gerd Hoffmann -
Thomas Zimmermann