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 --- qxl/qxl_cmd.c | 1 + 1 file changed, 1 insertion(+)

diff --git a/qxl/qxl_cmd.c b/qxl/qxl_cmd.c index bd5404e..85ed5dc 100644 --- a/qxl/qxl_cmd.c +++ b/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 --- qxl/qxl_ioctl.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/qxl/qxl_ioctl.c b/qxl/qxl_ioctl.c index afd7297..e8b5207 100644 --- a/qxl/qxl_ioctl.c +++ b/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 --- qxl/qxl_ioctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qxl/qxl_ioctl.c b/qxl/qxl_ioctl.c index e8b5207..230ab84 100644 --- a/qxl/qxl_ioctl.c +++ b/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 --- qxl/qxl_ioctl.c | 2 -- 1 file changed, 2 deletions(-)

diff --git a/qxl/qxl_ioctl.c b/qxl/qxl_ioctl.c index 230ab84..85b3808 100644 --- a/qxl/qxl_ioctl.c +++ b/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 --- qxl/qxl_cmd.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/qxl/qxl_cmd.c b/qxl/qxl_cmd.c index 85ed5dc..b18f84c 100644 --- a/qxl/qxl_cmd.c +++ b/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 --- qxl/qxl_release.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qxl/qxl_release.c b/qxl/qxl_release.c index d9b2568..6fd8e50 100644 --- a/qxl/qxl_release.c +++ b/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 --- qxl/qxl_release.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/qxl/qxl_release.c b/qxl/qxl_release.c index 6fd8e50..00604ed 100644 --- a/qxl/qxl_release.c +++ b/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 --- qxl/qxl_cmd.c | 2 +- qxl/qxl_display.c | 2 +- qxl/qxl_drv.h | 2 +- qxl/qxl_release.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/qxl/qxl_cmd.c b/qxl/qxl_cmd.c index b18f84c..edc1eec 100644 --- a/qxl/qxl_cmd.c +++ b/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/qxl/qxl_display.c b/qxl/qxl_display.c index 4a0a8b2..a8dbb3e 100644 --- a/qxl/qxl_display.c +++ b/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/qxl/qxl_drv.h b/qxl/qxl_drv.h index 6745c44..62ef8be 100644 --- a/qxl/qxl_drv.h +++ b/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/qxl/qxl_release.c b/qxl/qxl_release.c index 00604ed..b66ec33 100644 --- a/qxl/qxl_release.c +++ b/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 --- qxl/qxl_gem.c | 10 ++++++++-- qxl/qxl_object.c | 11 ++++------- 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/qxl/qxl_gem.c b/qxl/qxl_gem.c index b96f0c9..d9746e9 100644 --- a/qxl/qxl_gem.c +++ b/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/qxl/qxl_object.c b/qxl/qxl_object.c index cdeaf08..6d6f33d 100644 --- a/qxl/qxl_object.c +++ b/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; }