This set of patches mainly contains fix for some memory issues using quite aggressively surfaces and other minor problems like images going black after a while.
There are no code change from v1, changes: - fix path prefix - rebased on drm-next branch; - add "drm/qxl" prefix on subject; - add reviewed by.
About which patches should be applied surely (attempting to put a priority) - "Move main reference counter to GEM object instead of TTM ones" this can causes memory corruption even not wanting to; - "Avoid double free on error" this can be cause leaks in kernel if user space wants, mitigated by the fact that usually DRM inodes are owned by root; - "Handle all errors in qxl_surface_evict" could cause corruption too, not really probable but taking into account that Xorg implementation use a lot signals is not so impossible; - "Handle correctly failures in qxl_alloc_relase_reserved", "Do not leak memory if qxl_release_list_add fails" just cause leaks on situation where memory is already REALLY low, can be omitted; - "Fix print statement not using uninitialized variable", "Remove format string errors" should just print garbage and debugging is disabled by default, not necessary.
Frediano Ziglio (11): drm/qxl: Do not cause spice-server to clean our objects drm/qxl: Do not leak memory if qxl_release_list_add fails drm/qxl: Fix print statement not using uninitialized variable drm/qxl: Avoid double free on error drm/qxl: Handle all errors in qxl_surface_evict drm/qxl: Fix return for qxl_release_alloc drm/qxl: Handle correctly failures in qxl_alloc_relase_reserved drm/qxl: Remove format string errors drm/qxl: Move main reference counter to GEM object instead of TTM ones drm/qxl: Simplify cleaning qxl processing command drm/qxl: Propagate correctly errors from qxlhw_handle_to_bo
drivers/gpu/drm/qxl/qxl_cmd.c | 11 +++++----- drivers/gpu/drm/qxl/qxl_display.c | 2 +- drivers/gpu/drm/qxl/qxl_drv.h | 2 +- drivers/gpu/drm/qxl/qxl_gem.c | 10 +++++++-- drivers/gpu/drm/qxl/qxl_ioctl.c | 46 +++++++++++++++------------------------ drivers/gpu/drm/qxl/qxl_object.c | 11 ++++------ drivers/gpu/drm/qxl/qxl_release.c | 13 +++++++---- 7 files changed, 46 insertions(+), 49 deletions(-)
If objects are moved back from system memory to VRAM (and spice id created again) memory is already initialized so we need to set flag to not clear memory. If you don't do it after a while using desktop many images turns to black or transparents.
Signed-off-by: Frediano Ziglio fziglio@redhat.com Reviewed-by: Dave Airlie airlied@redhat.com --- drivers/gpu/drm/qxl/qxl_cmd.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c index bd5404e..85ed5dc 100644 --- a/drivers/gpu/drm/qxl/qxl_cmd.c +++ b/drivers/gpu/drm/qxl/qxl_cmd.c @@ -502,6 +502,7 @@ int qxl_hw_surface_alloc(struct qxl_device *qdev,
cmd = (struct qxl_surface_cmd *)qxl_release_map(qdev, release); cmd->type = QXL_SURFACE_CMD_CREATE; + cmd->flags = QXL_SURF_FLAG_KEEP_DATA; cmd->u.surface_create.format = surf->surf.format; cmd->u.surface_create.width = surf->surf.width; cmd->u.surface_create.height = surf->surf.height;
If the function fails reference counter to the object is not decremented causing leaks. This is hard to spot as it happens only on very low memory situations.
Signed-off-by: Frediano Ziglio fziglio@redhat.com Reviewed-by: Dave Airlie airlied@redhat.com --- drivers/gpu/drm/qxl/qxl_ioctl.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c index afd7297..e8b5207 100644 --- a/drivers/gpu/drm/qxl/qxl_ioctl.c +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c @@ -122,8 +122,10 @@ static struct qxl_bo *qxlhw_handle_to_bo(struct qxl_device *qdev, qobj = gem_to_qxl_bo(gobj);
ret = qxl_release_list_add(release, qobj); - if (ret) + if (ret) { + drm_gem_object_unreference_unlocked(gobj); return NULL; + }
return qobj; }
reloc_info[i] is not still initialized in the print statement.
Signed-off-by: Frediano Ziglio fziglio@redhat.com Reviewed-by: Dave Airlie airlied@redhat.com --- drivers/gpu/drm/qxl/qxl_ioctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c index e8b5207..230ab84 100644 --- a/drivers/gpu/drm/qxl/qxl_ioctl.c +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c @@ -212,7 +212,7 @@ static int qxl_process_single_command(struct qxl_device *qdev, /* add the bos to the list of bos to validate - need to validate first then process relocs? */ if (reloc.reloc_type != QXL_RELOC_TYPE_BO && reloc.reloc_type != QXL_RELOC_TYPE_SURF) { - DRM_DEBUG("unknown reloc type %d\n", reloc_info[i].type); + DRM_DEBUG("unknown reloc type %d\n", reloc.reloc_type);
ret = -EINVAL; goto out_free_bos;
Is we are not able to get source bo object from handle we free destination bo object and call cleanup code however destination object was already inserted in reloc_info array (num_relocs was already incremented) so on cleanup we free destination again.
Signed-off-by: Frediano Ziglio fziglio@redhat.com Reviewed-by: Dave Airlie airlied@redhat.com --- drivers/gpu/drm/qxl/qxl_ioctl.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c index 230ab84..85b3808 100644 --- a/drivers/gpu/drm/qxl/qxl_ioctl.c +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c @@ -240,8 +240,6 @@ static int qxl_process_single_command(struct qxl_device *qdev, qxlhw_handle_to_bo(qdev, file_priv, reloc.src_handle, release); if (!reloc_info[i].src_bo) { - if (reloc_info[i].dst_bo != cmd_bo) - drm_gem_object_unreference_unlocked(&reloc_info[i].dst_bo->gem_base); ret = -EINVAL; goto out_free_bos; }
Only EBUSY error was handled. This could cause code to believe reserve was successful while it failed.
Signed-off-by: Frediano Ziglio fziglio@redhat.com Reviewed-by: Dave Airlie airlied@redhat.com --- drivers/gpu/drm/qxl/qxl_cmd.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c index 85ed5dc..b18f84c 100644 --- a/drivers/gpu/drm/qxl/qxl_cmd.c +++ b/drivers/gpu/drm/qxl/qxl_cmd.c @@ -612,8 +612,8 @@ static int qxl_reap_surf(struct qxl_device *qdev, struct qxl_bo *surf, bool stal int ret;
ret = qxl_bo_reserve(surf, false); - if (ret == -EBUSY) - return -EBUSY; + if (ret) + return ret;
if (stall) mutex_unlock(&qdev->surf_evict_mutex); @@ -622,9 +622,9 @@ static int qxl_reap_surf(struct qxl_device *qdev, struct qxl_bo *surf, bool stal
if (stall) mutex_lock(&qdev->surf_evict_mutex); - if (ret == -EBUSY) { + if (ret) { qxl_bo_unreserve(surf); - return -EBUSY; + return ret; }
qxl_surface_evict_locked(qdev, surf, true);
This function return handle to allocated release object which is an int.
Signed-off-by: Frediano Ziglio fziglio@redhat.com Reviewed-by: Dave Airlie airlied@redhat.com --- drivers/gpu/drm/qxl/qxl_release.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c index d9b2568..6fd8e50 100644 --- a/drivers/gpu/drm/qxl/qxl_release.c +++ b/drivers/gpu/drm/qxl/qxl_release.c @@ -122,7 +122,7 @@ static const struct fence_ops qxl_fence_ops = { .wait = qxl_fence_wait, };
-static uint64_t +static int qxl_release_alloc(struct qxl_device *qdev, int type, struct qxl_release **ret) {
Free resources correctly if function fails
Signed-off-by: Frediano Ziglio fziglio@redhat.com Reviewed-by: Dave Airlie airlied@redhat.com --- drivers/gpu/drm/qxl/qxl_release.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c index 6fd8e50..00604ed 100644 --- a/drivers/gpu/drm/qxl/qxl_release.c +++ b/drivers/gpu/drm/qxl/qxl_release.c @@ -363,6 +363,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size, ret = qxl_release_bo_alloc(qdev, &qdev->current_release_bo[cur_idx]); if (ret) { mutex_unlock(&qdev->release_mutex); + qxl_release_free(qdev, *release); return ret; } } @@ -377,13 +378,17 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size,
mutex_unlock(&qdev->release_mutex);
- qxl_release_list_add(*release, bo); + ret = qxl_release_list_add(*release, bo); + qxl_bo_unref(&bo); + if (ret) { + qxl_release_free(qdev, *release); + return ret; + }
info = qxl_release_map(qdev, *release); info->id = idr_ret; qxl_release_unmap(qdev, *release, info);
- qxl_bo_unref(&bo); return ret; }
Enable format string checks for qxl_io_log and remove resulting warnings which could lead to memory errors on different platform or just printing wrong information.
Signed-off-by: Frediano Ziglio fziglio@redhat.com Reviewed-by: Dave Airlie airlied@redhat.com --- drivers/gpu/drm/qxl/qxl_cmd.c | 2 +- drivers/gpu/drm/qxl/qxl_display.c | 2 +- drivers/gpu/drm/qxl/qxl_drv.h | 2 +- drivers/gpu/drm/qxl/qxl_release.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c index b18f84c..edc1eec 100644 --- a/drivers/gpu/drm/qxl/qxl_cmd.c +++ b/drivers/gpu/drm/qxl/qxl_cmd.c @@ -248,7 +248,7 @@ int qxl_garbage_collect(struct qxl_device *qdev) } }
- QXL_INFO(qdev, "%s: %lld\n", __func__, i); + QXL_INFO(qdev, "%s: %d\n", __func__, i);
return i; } diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index 4a0a8b2..a8dbb3e 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -67,7 +67,7 @@ static int qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev) crc = crc32(0, (const uint8_t *)&qdev->rom->client_monitors_config, sizeof(qdev->rom->client_monitors_config)); if (crc != qdev->rom->client_monitors_config_crc) { - qxl_io_log(qdev, "crc mismatch: have %X (%d) != %X\n", crc, + qxl_io_log(qdev, "crc mismatch: have %X (%zd) != %X\n", crc, sizeof(qdev->rom->client_monitors_config), qdev->rom->client_monitors_config_crc); return 1; diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h index 6745c44..62ef8be 100644 --- a/drivers/gpu/drm/qxl/qxl_drv.h +++ b/drivers/gpu/drm/qxl/qxl_drv.h @@ -328,7 +328,7 @@ struct qxl_device { };
/* forward declaration for QXL_INFO_IO */ -void qxl_io_log(struct qxl_device *qdev, const char *fmt, ...); +__printf(2,3) void qxl_io_log(struct qxl_device *qdev, const char *fmt, ...);
extern const struct drm_ioctl_desc qxl_ioctls[]; extern int qxl_max_ioctl; diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c index 00604ed..b66ec33 100644 --- a/drivers/gpu/drm/qxl/qxl_release.c +++ b/drivers/gpu/drm/qxl/qxl_release.c @@ -153,7 +153,7 @@ qxl_release_alloc(struct qxl_device *qdev, int type, return handle; } *ret = release; - QXL_INFO(qdev, "allocated release %lld\n", handle); + QXL_INFO(qdev, "allocated release %d\n", handle); release->id = handle; return handle; }
qxl_bo structure has two reference counters, one in the GEM object and another in the TTM object. The GEM object keep a counter to the TTM object so when GEM counter reached zero the TTM counter (using qxl_bo_unref) was decremented. The qxl object is fully freed (both GEM and TTM part are cleaned) when the TTM counter reach zero. One issue was that surface idr structure has no owning on qxl_bo objects however it contains a pointer to qxl_bo object. This caused some nasty race condition for instance qxl_bo object was reaped even after counter was already zero. This patch fix these races moving main counter (the one used by qxl_bo_(un)ref) to GEM object which cleanup routine (qxl_gem_object_free) remove the idr pointer (using qxl_surface_evict) when the counters are still valid.
Signed-off-by: Frediano Ziglio fziglio@redhat.com Reviewed-by: Dave Airlie airlied@redhat.com --- drivers/gpu/drm/qxl/qxl_gem.c | 10 ++++++++-- drivers/gpu/drm/qxl/qxl_object.c | 11 ++++------- 2 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_gem.c b/drivers/gpu/drm/qxl/qxl_gem.c index b96f0c9..d9746e9 100644 --- a/drivers/gpu/drm/qxl/qxl_gem.c +++ b/drivers/gpu/drm/qxl/qxl_gem.c @@ -31,9 +31,15 @@ void qxl_gem_object_free(struct drm_gem_object *gobj) { struct qxl_bo *qobj = gem_to_qxl_bo(gobj); + struct qxl_device *qdev; + struct ttm_buffer_object *tbo;
- if (qobj) - qxl_bo_unref(&qobj); + qdev = (struct qxl_device *)gobj->dev->dev_private; + + qxl_surface_evict(qdev, qobj, false); + + tbo = &qobj->tbo; + ttm_bo_unref(&tbo); }
int qxl_gem_object_create(struct qxl_device *qdev, int size, diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c index cdeaf08..6d6f33d 100644 --- a/drivers/gpu/drm/qxl/qxl_object.c +++ b/drivers/gpu/drm/qxl/qxl_object.c @@ -208,19 +208,16 @@ void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev,
void qxl_bo_unref(struct qxl_bo **bo) { - struct ttm_buffer_object *tbo; - if ((*bo) == NULL) return; - tbo = &((*bo)->tbo); - ttm_bo_unref(&tbo); - if (tbo == NULL) - *bo = NULL; + + drm_gem_object_unreference_unlocked(&(*bo)->gem_base); + *bo = NULL; }
struct qxl_bo *qxl_bo_ref(struct qxl_bo *bo) { - ttm_bo_reference(&bo->tbo); + drm_gem_object_reference(&bo->gem_base); return bo; }
In qxlhw_handle_to_bo we incremented counters twice, one time for release object and one for reloc_info. In the main function however reloc_info references was drop much earlier than release so keeping the pointer only on release is safe and make cleaning process easier.
Signed-off-by: Frediano Ziglio fziglio@redhat.com Reviewed-by: Dave Airlie airlied@redhat.com --- drivers/gpu/drm/qxl/qxl_ioctl.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c index 85b3808..bb326ff 100644 --- a/drivers/gpu/drm/qxl/qxl_ioctl.c +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c @@ -122,10 +122,9 @@ static struct qxl_bo *qxlhw_handle_to_bo(struct qxl_device *qdev, qobj = gem_to_qxl_bo(gobj);
ret = qxl_release_list_add(release, qobj); - if (ret) { - drm_gem_object_unreference_unlocked(gobj); + drm_gem_object_unreference_unlocked(gobj); + if (ret) return NULL; - }
return qobj; } @@ -145,7 +144,7 @@ static int qxl_process_single_command(struct qxl_device *qdev, struct qxl_release *release; struct qxl_bo *cmd_bo; void *fb_cmd; - int i, j, ret, num_relocs; + int i, ret, num_relocs; int unwritten;
switch (cmd->type) { @@ -269,12 +268,6 @@ static int qxl_process_single_command(struct qxl_device *qdev, qxl_release_fence_buffer_objects(release);
out_free_bos: - for (j = 0; j < num_relocs; j++) { - if (reloc_info[j].dst_bo != cmd_bo) - drm_gem_object_unreference_unlocked(&reloc_info[j].dst_bo->gem_base); - if (reloc_info[j].src_bo && reloc_info[j].src_bo != cmd_bo) - drm_gem_object_unreference_unlocked(&reloc_info[j].src_bo->gem_base); - } out_free_release: if (ret) qxl_release_free(qdev, release);
This function could return a NULL pointer in case of handle not present and in case of out of memory conditions however caller function always returned EINVAL error hiding a possible ENOMEM. This patch change the function to return the error instead to be able to propagate the error instead of assuming EINVAL.
Signed-off-by: Frediano Ziglio fziglio@redhat.com Reviewed-by: Dave Airlie airlied@redhat.com --- drivers/gpu/drm/qxl/qxl_ioctl.c | 33 ++++++++++++++------------------- 1 file changed, 14 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c index bb326ff..37f1faf 100644 --- a/drivers/gpu/drm/qxl/qxl_ioctl.c +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c @@ -107,9 +107,9 @@ apply_surf_reloc(struct qxl_device *qdev, struct qxl_reloc_info *info) }
/* return holding the reference to this object */ -static struct qxl_bo *qxlhw_handle_to_bo(struct qxl_device *qdev, - struct drm_file *file_priv, uint64_t handle, - struct qxl_release *release) +static int qxlhw_handle_to_bo(struct qxl_device *qdev, + struct drm_file *file_priv, uint64_t handle, + struct qxl_release *release, struct qxl_bo **qbo_p) { struct drm_gem_object *gobj; struct qxl_bo *qobj; @@ -117,16 +117,17 @@ static struct qxl_bo *qxlhw_handle_to_bo(struct qxl_device *qdev,
gobj = drm_gem_object_lookup(qdev->ddev, file_priv, handle); if (!gobj) - return NULL; + return -EINVAL;
qobj = gem_to_qxl_bo(gobj);
ret = qxl_release_list_add(release, qobj); drm_gem_object_unreference_unlocked(gobj); if (ret) - return NULL; + return ret;
- return qobj; + *qbo_p = qobj; + return 0; }
/* @@ -219,13 +220,10 @@ static int qxl_process_single_command(struct qxl_device *qdev, reloc_info[i].type = reloc.reloc_type;
if (reloc.dst_handle) { - reloc_info[i].dst_bo = qxlhw_handle_to_bo(qdev, file_priv, - reloc.dst_handle, release); - if (!reloc_info[i].dst_bo) { - ret = -EINVAL; - reloc_info[i].src_bo = NULL; + ret = qxlhw_handle_to_bo(qdev, file_priv, reloc.dst_handle, release, + &reloc_info[i].dst_bo); + if (ret) goto out_free_bos; - } reloc_info[i].dst_offset = reloc.dst_offset; } else { reloc_info[i].dst_bo = cmd_bo; @@ -234,14 +232,11 @@ static int qxl_process_single_command(struct qxl_device *qdev, num_relocs++;
/* reserve and validate the reloc dst bo */ - if (reloc.reloc_type == QXL_RELOC_TYPE_BO || reloc.src_handle > 0) { - reloc_info[i].src_bo = - qxlhw_handle_to_bo(qdev, file_priv, - reloc.src_handle, release); - if (!reloc_info[i].src_bo) { - ret = -EINVAL; + if (reloc.reloc_type == QXL_RELOC_TYPE_BO || reloc.src_handle) { + ret = qxlhw_handle_to_bo(qdev, file_priv, reloc.src_handle, release, + &reloc_info[i].src_bo); + if (ret) goto out_free_bos; - } reloc_info[i].src_offset = reloc.src_offset; } else { reloc_info[i].src_bo = NULL;
dri-devel@lists.freedesktop.org
-
Frediano Ziglio