Re: [Spice-devel] [PATCH] drm/qxl: add spice-devel list to MAINTAINERS

[Christophe Fergeau]

On Wed, Nov 21, 2018 at 10:01:29AM +0100, Gerd Hoffmann wrote:
> So qxl kernel patches are sent to the spice-devel list for review.
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4caac2f6b0..d9945d57eb 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4728,6 +4728,7 @@ DRM DRIVER FOR QXL VIRTUAL GPU
>  M:   Dave Airlie 
>  M:   Gerd Hoffmann 
>  L:   virtualizat...@lists.linux-foundation.org
> +L:   spice-de...@lists.freedesktop.org

Should this also list dri-devel@lists.freedesktop.org ?

Christophe


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] qxl: Make sure qxl_cursor memory is pinned

[Christophe Fergeau]

QEMU keeps a vram reference to the last QXLCursorCmd it received.
This QXLCursorCmd command points to a QXLCursor instance (stored in vram
too). However, while the QXLCursorCmd memory is pinned, the QXLCursor
memory is not.

When booting a recent Fedora to its login screen while monitoring the
QXLCursorCmd QEMU holds, it's possible to see the QXLCursor memory
becoming invalid shortly after boot. Pinning that memory ensures that
that QXLCursor memory is not going to be moved by the guest kernel.

Moving the pin/unpin to qxl_release_list_add()/qxl_release_free_list()
would be a more generic fix. However, doing this quickly exhausts QXL
video memory, so more fixing would be needed before this is workable.

Signed-off-by: Christophe Fergeau 
---
 drivers/gpu/drm/qxl/qxl_display.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index 61bb88f576f7..6b6a89015813 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -620,10 +620,14 @@ static void qxl_cursor_atomic_update(struct drm_plane 
*plane,
if (ret)
goto out_kunmap;
 
-   ret = qxl_release_reserve_list(release, true);
+   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_kmap(cursor_bo, (void **));
if (ret)
goto out_backoff;
@@ -668,6 +672,8 @@ static void qxl_cursor_atomic_update(struct drm_plane 
*plane,
qxl_push_cursor_ring_release(qdev, release, QXL_CMD_CURSOR, false);
qxl_release_fence_buffer_objects(release);
 
+   if (old_cursor_bo != NULL)
+   qxl_bo_unpin(old_cursor_bo);
qxl_bo_unref(_cursor_bo);
qxl_bo_unref(_bo);
 
@@ -675,6 +681,8 @@ static void qxl_cursor_atomic_update(struct drm_plane 
*plane,
 
 out_backoff:
qxl_release_backoff_reserve_list(release);
+out_unpin:
+   qxl_bo_unpin(cursor_bo);
 out_free_bo:
qxl_bo_unref(_bo);
 out_kunmap:
-- 
2.19.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/3] qxl: No need for NULL check before calling qxl_bo_unref()

[Christophe Fergeau]

qxl_bo_unref() is already performing a NULL check.

Signed-off-by: Christophe Fergeau 
---
 drivers/gpu/drm/qxl/qxl_display.c | 4 +---
 drivers/gpu/drm/qxl/qxl_draw.c| 3 +--
 drivers/gpu/drm/qxl/qxl_kms.c | 6 ++
 3 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index 87d16a0ce01e..9d5558aad780 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -668,9 +668,7 @@ static void qxl_cursor_atomic_update(struct drm_plane 
*plane,
qxl_push_cursor_ring_release(qdev, release, QXL_CMD_CURSOR, false);
qxl_release_fence_buffer_objects(release);
 
-   if (old_cursor_bo)
-   qxl_bo_unref(_cursor_bo);
-
+   qxl_bo_unref(_cursor_bo);
qxl_bo_unref(_bo);
 
return;
diff --git a/drivers/gpu/drm/qxl/qxl_draw.c b/drivers/gpu/drm/qxl/qxl_draw.c
index cc5b32e749ce..dff1581d0bdc 100644
--- a/drivers/gpu/drm/qxl/qxl_draw.c
+++ b/drivers/gpu/drm/qxl/qxl_draw.c
@@ -245,8 +245,7 @@ void qxl_draw_opaque_fb(const struct qxl_fb_image 
*qxl_fb_image,
qxl_release_fence_buffer_objects(release);
 
 out_free_palette:
-   if (palette_bo)
-   qxl_bo_unref(_bo);
+   qxl_bo_unref(_bo);
 out_free_image:
qxl_image_free_objects(qdev, dimage);
 out_free_drawable:
diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
index e25c589d5f50..d07f75444542 100644
--- a/drivers/gpu/drm/qxl/qxl_kms.c
+++ b/drivers/gpu/drm/qxl/qxl_kms.c
@@ -313,10 +313,8 @@ int qxl_device_init(struct qxl_device *qdev,
 
 void qxl_device_fini(struct qxl_device *qdev)
 {
-   if (qdev->current_release_bo[0])
-   qxl_bo_unref(>current_release_bo[0]);
-   if (qdev->current_release_bo[1])
-   qxl_bo_unref(>current_release_bo[1]);
+   qxl_bo_unref(>current_release_bo[0]);
+   qxl_bo_unref(>current_release_bo[1]);
flush_work(>gc_work);
qxl_ring_free(qdev->command_ring);
qxl_ring_free(qdev->cursor_ring);
-- 
2.19.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] qxl: Remove unused qxl_bo_pin arguments

[Christophe Fergeau]

The 'domain' argument to qxl_bo_pin is redundant with 'bo', and
'gpu_addr' is unused, so we can remove both.

Signed-off-by: Christophe Fergeau 
---
 drivers/gpu/drm/qxl/qxl_display.c |  4 ++--
 drivers/gpu/drm/qxl/qxl_fb.c  |  2 +-
 drivers/gpu/drm/qxl/qxl_object.c  | 12 
 drivers/gpu/drm/qxl/qxl_object.h  |  2 +-
 4 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index 9d5558aad780..61bb88f576f7 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -753,7 +753,7 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane,
}
}
 
-   ret = qxl_bo_pin(user_bo, QXL_GEM_DOMAIN_CPU, NULL);
+   ret = qxl_bo_pin(user_bo);
if (ret)
return ret;
 
@@ -1101,7 +1101,7 @@ 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, QXL_GEM_DOMAIN_VRAM, NULL);
+   ret = qxl_bo_pin(qdev->monitors_config_bo);
if (ret)
return ret;
 
diff --git a/drivers/gpu/drm/qxl/qxl_fb.c b/drivers/gpu/drm/qxl/qxl_fb.c
index 2294b7f14fdf..34904ecee67d 100644
--- a/drivers/gpu/drm/qxl/qxl_fb.c
+++ b/drivers/gpu/drm/qxl/qxl_fb.c
@@ -111,7 +111,7 @@ static int qxlfb_create_pinned_object(struct qxl_device 
*qdev,
qbo->surf.stride = mode_cmd->pitches[0];
qbo->surf.format = SPICE_SURFACE_FMT_32_xRGB;
 
-   ret = qxl_bo_pin(qbo, QXL_GEM_DOMAIN_SURFACE, NULL);
+   ret = qxl_bo_pin(qbo);
if (ret) {
goto out_unref;
}
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index 6a30196e9d6c..2abd609d415b 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -221,7 +221,7 @@ struct qxl_bo *qxl_bo_ref(struct qxl_bo *bo)
return bo;
 }
 
-static int __qxl_bo_pin(struct qxl_bo *bo, u32 domain, u64 *gpu_addr)
+static int __qxl_bo_pin(struct qxl_bo *bo)
 {
struct ttm_operation_ctx ctx = { false, false };
struct drm_device *ddev = bo->gem_base.dev;
@@ -229,16 +229,12 @@ static int __qxl_bo_pin(struct qxl_bo *bo, u32 domain, 
u64 *gpu_addr)
 
if (bo->pin_count) {
bo->pin_count++;
-   if (gpu_addr)
-   *gpu_addr = qxl_bo_gpu_offset(bo);
return 0;
}
-   qxl_ttm_placement_from_domain(bo, domain, true);
+   qxl_ttm_placement_from_domain(bo, bo->type, true);
r = ttm_bo_validate(>tbo, >placement, );
if (likely(r == 0)) {
bo->pin_count = 1;
-   if (gpu_addr != NULL)
-   *gpu_addr = qxl_bo_gpu_offset(bo);
}
if (unlikely(r != 0))
dev_err(ddev->dev, "%p pin failed\n", bo);
@@ -272,7 +268,7 @@ static int __qxl_bo_unpin(struct qxl_bo *bo)
  * beforehand, use the internal version directly __qxl_bo_pin.
  *
  */
-int qxl_bo_pin(struct qxl_bo *bo, u32 domain, u64 *gpu_addr)
+int qxl_bo_pin(struct qxl_bo *bo)
 {
int r;
 
@@ -280,7 +276,7 @@ int qxl_bo_pin(struct qxl_bo *bo, u32 domain, u64 *gpu_addr)
if (r)
return r;
 
-   r = __qxl_bo_pin(bo, bo->type, NULL);
+   r = __qxl_bo_pin(bo);
qxl_bo_unreserve(bo);
return r;
 }
diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h
index 0374fd93f4d6..2ae999a26b9e 100644
--- a/drivers/gpu/drm/qxl/qxl_object.h
+++ b/drivers/gpu/drm/qxl/qxl_object.h
@@ -95,7 +95,7 @@ void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, struct 
qxl_bo *bo, int pa
 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);
 extern void qxl_bo_unref(struct qxl_bo **bo);
-extern int qxl_bo_pin(struct qxl_bo *bo, u32 domain, u64 *gpu_addr);
+extern int qxl_bo_pin(struct qxl_bo *bo);
 extern int qxl_bo_unpin(struct qxl_bo *bo);
 extern void qxl_ttm_placement_from_domain(struct qxl_bo *qbo, u32 domain, bool 
pinned);
 extern bool qxl_ttm_bo_is_qxl_bo(struct ttm_buffer_object *bo);
-- 
2.19.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH drm/qxl v4 7/7] qxl: Allow resolution which are not multiple of 8

[Christophe Fergeau]

The use of drm_cvt_mode() in qxl_add_monitors_config_modes() means that
the resolutions we are going to present to user-space are going to be
rounded down to a multiple of 8. In the QXL arbitrary resolution case,
this is not useful.
This commit forces the actual width/height that was requested by the
client in the drm_display_mode structure rather than keeping the
rounded version.

Signed-off-by: Christophe Fergeau 
---
 drivers/gpu/drm/qxl/qxl_display.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index 518333c..fa99481 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -199,6 +199,9 @@ static int qxl_add_monitors_config_modes(struct 
drm_connector *connector,
mode = drm_cvt_mode(dev, head->width, head->height, 60, false, false,
false);
mode->type |= DRM_MODE_TYPE_PREFERRED;
+   mode->hdisplay = head->width;
+   mode->vdisplay = head->height;
+   drm_mode_set_name(mode);
*pwidth = head->width;
*pheight = head->height;
drm_mode_probed_add(connector, mode);
-- 
2.9.3

[PATCH drm/qxl v4 6/7] qxl: Don't notify userspace when monitors config is unchanged

[Christophe Fergeau]

When the QXL driver receives a QXL_INTERRUPT_CLIENT_MONITORS_CONFIG interrupt,
we currently always notify userspace that there was some hotplug event.

However, gnome-shell/mutter is reacting to this event by attempting a
resolution change, which it does by issueing drmModeRmFB, drmModeAddFB,
and then drmModeSetCrtc. This has the side-effect of causing
qxl_crtc_mode_set() to tell the QXL virtual hardware that a primary
surface was destroyed and created. After going through QEMU and then the
remote SPICE client, a new identical monitors config message will be
sent, resulting in a QXL_INTERRUPT_CLIENT_MONITORS_CONFIG interrupt to
be emitted, and the same scenario occurring again.

As destroying/creating the primary surface causes a visible screen
flicker, this makes the guest hard to use (
https://bugzilla.redhat.com/show_bug.cgi?id=1266484 ).

This commit checks if the screen configuration we received is the same
one as the current one, and does not notify userspace about it if that's
the case.

Signed-off-by: Christophe Fergeau 
---
 drivers/gpu/drm/qxl/qxl_display.c | 62 ---
 1 file changed, 52 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index 8cf5177..518333c 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -57,11 +57,18 @@ static void qxl_alloc_client_monitors_config(struct 
qxl_device *qdev, unsigned c
qdev->client_monitors_config->count = count;
 }

+enum {
+   MONITORS_CONFIG_MODIFIED,
+   MONITORS_CONFIG_UNCHANGED,
+   MONITORS_CONFIG_BAD_CRC,
+};
+
 static int qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
 {
int i;
int num_monitors;
uint32_t crc;
+   int status = MONITORS_CONFIG_UNCHANGED;

num_monitors = qdev->rom->client_monitors_config.count;
crc = crc32(0, (const uint8_t *)>rom->client_monitors_config,
@@ -70,7 +77,7 @@ static int qxl_display_copy_rom_client_monitors_config(struct 
qxl_device *qdev)
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;
+   return MONITORS_CONFIG_BAD_CRC;
}
if (num_monitors > qdev->monitors_config->max_allowed) {
DRM_DEBUG_KMS("client monitors list will be truncated: %d < 
%d\n",
@@ -79,6 +86,10 @@ static int 
qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
} else {
num_monitors = qdev->rom->client_monitors_config.count;
}
+   if (qdev->client_monitors_config
+ && (num_monitors != qdev->client_monitors_config->count)) {
+   status = MONITORS_CONFIG_MODIFIED;
+   }
qxl_alloc_client_monitors_config(qdev, num_monitors);
/* we copy max from the client but it isn't used */
qdev->client_monitors_config->max_allowed =
@@ -88,17 +99,39 @@ static int 
qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
>rom->client_monitors_config.heads[i];
struct qxl_head *client_head =
>client_monitors_config->heads[i];
-   client_head->x = c_rect->left;
-   client_head->y = c_rect->top;
-   client_head->width = c_rect->right - c_rect->left;
-   client_head->height = c_rect->bottom - c_rect->top;
-   client_head->surface_id = 0;
-   client_head->id = i;
-   client_head->flags = 0;
+   if (client_head->x != c_rect->left) {
+   client_head->x = c_rect->left;
+   status = MONITORS_CONFIG_MODIFIED;
+   }
+   if (client_head->y != c_rect->top) {
+   client_head->y = c_rect->top;
+   status = MONITORS_CONFIG_MODIFIED;
+   }
+   if (client_head->width != c_rect->right - c_rect->left) {
+   client_head->width = c_rect->right - c_rect->left;
+   status = MONITORS_CONFIG_MODIFIED;
+   }
+   if (client_head->height != c_rect->bottom - c_rect->top) {
+   client_head->height = c_rect->bottom - c_rect->top;
+   status = MONITORS_CONFIG_MODIFIED;
+   }
+   if (client_head->surface_id != 0) {
+   client_head->surface_id = 0;
+   status = MONITORS_CONFIG_MODIFIED;
+   }
+   if (client_head->id != i) {
+   client_head-

[PATCH drm/qxl v4 5/7] qxl: Remove qxl_bo_init() return value

[Christophe Fergeau]

It's always returning 0, and it's always ignored.

Signed-off-by: Christophe Fergeau 
Acked-by: Frediano Ziglio 
---
 drivers/gpu/drm/qxl/qxl_drv.h | 2 +-
 drivers/gpu/drm/qxl/qxl_gem.c | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 5a4720a..feac7e6 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -398,7 +398,7 @@ int qxl_create_monitors_object(struct qxl_device *qdev);
 int qxl_destroy_monitors_object(struct qxl_device *qdev);

 /* qxl_gem.c */
-int qxl_gem_init(struct qxl_device *qdev);
+void qxl_gem_init(struct qxl_device *qdev);
 void qxl_gem_fini(struct qxl_device *qdev);
 int qxl_gem_object_create(struct qxl_device *qdev, int size,
  int alignment, int initial_domain,
diff --git a/drivers/gpu/drm/qxl/qxl_gem.c b/drivers/gpu/drm/qxl/qxl_gem.c
index d9746e9..3f185c4 100644
--- a/drivers/gpu/drm/qxl/qxl_gem.c
+++ b/drivers/gpu/drm/qxl/qxl_gem.c
@@ -111,10 +111,9 @@ void qxl_gem_object_close(struct drm_gem_object *obj,
 {
 }

-int qxl_gem_init(struct qxl_device *qdev)
+void qxl_gem_init(struct qxl_device *qdev)
 {
INIT_LIST_HEAD(>gem.objects);
-   return 0;
 }

 void qxl_gem_fini(struct qxl_device *qdev)
-- 
2.9.3

[PATCH drm/qxl v4 4/7] qxl: Call qxl_gem_{init,fini}

[Christophe Fergeau]

qdev->gem.objects was initialized directly in qxl_device_init() rather
than going through qxl_gem_init(), and qxl_gem_fini() was never called.

Signed-off-by: Christophe Fergeau 
---
 drivers/gpu/drm/qxl/qxl_kms.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
index e642242..af685f1 100644
--- a/drivers/gpu/drm/qxl/qxl_kms.c
+++ b/drivers/gpu/drm/qxl/qxl_kms.c
@@ -131,7 +131,7 @@ static int qxl_device_init(struct qxl_device *qdev,
mutex_init(>update_area_mutex);
mutex_init(>release_mutex);
mutex_init(>surf_evict_mutex);
-   INIT_LIST_HEAD(>gem.objects);
+   qxl_gem_init(qdev);

qdev->rom_base = pci_resource_start(pdev, 2);
qdev->rom_size = pci_resource_len(pdev, 2);
@@ -273,6 +273,7 @@ static void qxl_device_fini(struct qxl_device *qdev)
qxl_ring_free(qdev->command_ring);
qxl_ring_free(qdev->cursor_ring);
qxl_ring_free(qdev->release_ring);
+   qxl_gem_fini(qdev);
qxl_bo_fini(qdev);
io_mapping_free(qdev->surface_mapping);
io_mapping_free(qdev->vram_mapping);
-- 
2.9.3

[PATCH drm/qxl v4 3/7] qxl: Add missing '\n' to qxl_io_log() call

[Christophe Fergeau]

The message has to be terminated by a newline as it's not going to get
added automatically.

Signed-off-by: Christophe Fergeau 
Acked-by: Frediano Ziglio 
---
 drivers/gpu/drm/qxl/qxl_fb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/qxl/qxl_fb.c b/drivers/gpu/drm/qxl/qxl_fb.c
index 28c1423..969c7c1 100644
--- a/drivers/gpu/drm/qxl/qxl_fb.c
+++ b/drivers/gpu/drm/qxl/qxl_fb.c
@@ -198,7 +198,7 @@ static int qxlfb_framebuffer_dirty(struct drm_framebuffer 
*fb,
/*
 * we are using a shadow draw buffer, at qdev->surface0_shadow
 */
-   qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]", clips->x1, clips->x2,
+   qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]\n", clips->x1, clips->x2,
   clips->y1, clips->y2);
image->dx = clips->x1;
image->dy = clips->y1;
-- 
2.9.3

[PATCH drm/qxl v4 2/7] qxl: Remove unused prototype

[Christophe Fergeau]

qxl_crtc_set_from_monitors_config() is defined in qxl_drv.h but never
implemented.

Signed-off-by: Christophe Fergeau 
Acked-by: Frediano Ziglio 
---
 drivers/gpu/drm/qxl/qxl_drv.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index a3c1131..5a4720a 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -397,9 +397,6 @@ void qxl_display_read_client_monitors_config(struct 
qxl_device *qdev);
 int qxl_create_monitors_object(struct qxl_device *qdev);
 int qxl_destroy_monitors_object(struct qxl_device *qdev);

-/* used by qxl_debugfs only */
-void qxl_crtc_set_from_monitors_config(struct qxl_device *qdev);
-
 /* qxl_gem.c */
 int qxl_gem_init(struct qxl_device *qdev);
 void qxl_gem_fini(struct qxl_device *qdev);
-- 
2.9.3

[PATCH drm/qxl v4 1/7] qxl: Mark some internal functions as static

[Christophe Fergeau]

They are not used outside of their respective source file

Signed-off-by: Christophe Fergeau 
Acked-by: Frediano Ziglio 
---
 drivers/gpu/drm/qxl/qxl_cmd.c | 2 +-
 drivers/gpu/drm/qxl/qxl_display.c | 4 ++--
 drivers/gpu/drm/qxl/qxl_drv.h | 3 ---
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index 04270f5..74fc936 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -578,7 +578,7 @@ int qxl_hw_surface_dealloc(struct qxl_device *qdev,
return 0;
 }

-int qxl_update_surface(struct qxl_device *qdev, struct qxl_bo *surf)
+static int qxl_update_surface(struct qxl_device *qdev, struct qxl_bo *surf)
 {
struct qxl_rect rect;
int ret;
diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index 3aef127..8cf5177 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -36,7 +36,7 @@ static bool qxl_head_enabled(struct qxl_head *head)
return head->width && head->height;
 }

-void qxl_alloc_client_monitors_config(struct qxl_device *qdev, unsigned count)
+static void qxl_alloc_client_monitors_config(struct qxl_device *qdev, unsigned 
count)
 {
if (qdev->client_monitors_config &&
count > qdev->client_monitors_config->count) {
@@ -559,7 +559,7 @@ static bool qxl_crtc_mode_fixup(struct drm_crtc *crtc,
return true;
 }

-void
+static void
 qxl_send_monitors_config(struct qxl_device *qdev)
 {
int i;
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 8e633ca..a3c1131 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -394,13 +394,11 @@ qxl_framebuffer_init(struct drm_device *dev,
 struct drm_gem_object *obj,
 const struct drm_framebuffer_funcs *funcs);
 void qxl_display_read_client_monitors_config(struct qxl_device *qdev);
-void qxl_send_monitors_config(struct qxl_device *qdev);
 int qxl_create_monitors_object(struct qxl_device *qdev);
 int qxl_destroy_monitors_object(struct qxl_device *qdev);

 /* used by qxl_debugfs only */
 void qxl_crtc_set_from_monitors_config(struct qxl_device *qdev);
-void qxl_alloc_client_monitors_config(struct qxl_device *qdev, unsigned count);

 /* qxl_gem.c */
 int qxl_gem_init(struct qxl_device *qdev);
@@ -573,6 +571,5 @@ int qxl_bo_check_id(struct qxl_device *qdev, struct qxl_bo 
*bo);
 struct qxl_drv_surface *
 qxl_surface_lookup(struct drm_device *dev, int surface_id);
 void qxl_surface_evict(struct qxl_device *qdev, struct qxl_bo *surf, bool 
freeing);
-int qxl_update_surface(struct qxl_device *qdev, struct qxl_bo *surf);

 #endif
-- 
2.9.3

[PATCH drm/qxl v4 0/7] qxl: Various fixes

[Christophe Fergeau]

Hey,

Here is what should hopefully be the final version of this series, only change
since v3 is that I made sure to keep "PATCH" in the subject line, and I added
missing Acked-by/Signed-of-by annotations to the logs.

Christophe

[Spice-devel] [drm/qxl v3 6/7] qxl: Don't notify userspace when monitors config is unchanged

[Christophe Fergeau]

On Mon, Nov 07, 2016 at 04:08:48AM -0500, Frediano Ziglio wrote:
> > @@ -124,9 +157,18 @@ void qxl_display_read_client_monitors_config(struct
> > qxl_device *qdev)
> >  {
> >  
> > struct drm_device *dev = qdev->ddev;
> > -   while (qxl_display_copy_rom_client_monitors_config(qdev)) {
> > +   int status;
> > +
> > +   status = qxl_display_copy_rom_client_monitors_config(qdev);
> > +   while (status == MONITORS_CONFIG_BAD_CRC) {
> > qxl_io_log(qdev, "failed crc check for client_monitors_config,"
> >  " retrying\n");
> > +   status = qxl_display_copy_rom_client_monitors_config(qdev);
> > +   }
> > +   if (status == MONITORS_CONFIG_UNCHANGED) {
> > +   qxl_io_log(qdev, "config unchanged\n");
> > +   DRM_DEBUG("ignoring unchanged client monitors config");
> 
> Why log and debug? Looks like a missing debug cleanup.

qxl_io_log is going to be output host-side, DRM_DEBUG is guest-side,
having both was intentional.

Christophe
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: 

[drm/qxl v3 7/7] qxl: Allow resolution which are not multiple of 8

[Christophe Fergeau]

The use of drm_cvt_mode() in qxl_add_monitors_config_modes() means that
the resolutions we are going to present to user-space are going to be
rounded down to a multiple of 8. In the QXL arbitrary resolution case,
this is not useful.
This commit forces the actual width/height that was requested by the
client in the drm_display_mode structure rather than keeping the
rounded version.

Signed-off-by: Christophe Fergeau 
---
 drivers/gpu/drm/qxl/qxl_display.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index 518333c..fa99481 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -199,6 +199,9 @@ static int qxl_add_monitors_config_modes(struct 
drm_connector *connector,
mode = drm_cvt_mode(dev, head->width, head->height, 60, false, false,
false);
mode->type |= DRM_MODE_TYPE_PREFERRED;
+   mode->hdisplay = head->width;
+   mode->vdisplay = head->height;
+   drm_mode_set_name(mode);
*pwidth = head->width;
*pheight = head->height;
drm_mode_probed_add(connector, mode);
-- 
2.9.3

[drm/qxl v3 6/7] qxl: Don't notify userspace when monitors config is unchanged

[Christophe Fergeau]

When the QXL driver receives a QXL_INTERRUPT_CLIENT_MONITORS_CONFIG interrupt,
we currently always notify userspace that there was some hotplug event.

However, gnome-shell/mutter is reacting to this event by attempting a
resolution change, which it does by issueing drmModeRmFB, drmModeAddFB,
and then drmModeSetCrtc. This has the side-effect of causing
qxl_crtc_mode_set() to tell the QXL virtual hardware that a primary
surface was destroyed and created. After going through QEMU and then the
remote SPICE client, a new identical monitors config message will be
sent, resulting in a QXL_INTERRUPT_CLIENT_MONITORS_CONFIG interrupt to
be emitted, and the same scenario occurring again.

As destroying/creating the primary surface causes a visible screen
flicker, this makes the guest hard to use (
https://bugzilla.redhat.com/show_bug.cgi?id=1266484 ).

This commit checks if the screen configuration we received is the same
one as the current one, and does not notify userspace about it if that's
the case.

Signed-off-by: Christophe Fergeau 
---
 drivers/gpu/drm/qxl/qxl_display.c | 62 ---
 1 file changed, 52 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index 8cf5177..518333c 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -57,11 +57,18 @@ static void qxl_alloc_client_monitors_config(struct 
qxl_device *qdev, unsigned c
qdev->client_monitors_config->count = count;
 }

+enum {
+   MONITORS_CONFIG_MODIFIED,
+   MONITORS_CONFIG_UNCHANGED,
+   MONITORS_CONFIG_BAD_CRC,
+};
+
 static int qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
 {
int i;
int num_monitors;
uint32_t crc;
+   int status = MONITORS_CONFIG_UNCHANGED;

num_monitors = qdev->rom->client_monitors_config.count;
crc = crc32(0, (const uint8_t *)>rom->client_monitors_config,
@@ -70,7 +77,7 @@ static int qxl_display_copy_rom_client_monitors_config(struct 
qxl_device *qdev)
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;
+   return MONITORS_CONFIG_BAD_CRC;
}
if (num_monitors > qdev->monitors_config->max_allowed) {
DRM_DEBUG_KMS("client monitors list will be truncated: %d < 
%d\n",
@@ -79,6 +86,10 @@ static int 
qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
} else {
num_monitors = qdev->rom->client_monitors_config.count;
}
+   if (qdev->client_monitors_config
+ && (num_monitors != qdev->client_monitors_config->count)) {
+   status = MONITORS_CONFIG_MODIFIED;
+   }
qxl_alloc_client_monitors_config(qdev, num_monitors);
/* we copy max from the client but it isn't used */
qdev->client_monitors_config->max_allowed =
@@ -88,17 +99,39 @@ static int 
qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
>rom->client_monitors_config.heads[i];
struct qxl_head *client_head =
>client_monitors_config->heads[i];
-   client_head->x = c_rect->left;
-   client_head->y = c_rect->top;
-   client_head->width = c_rect->right - c_rect->left;
-   client_head->height = c_rect->bottom - c_rect->top;
-   client_head->surface_id = 0;
-   client_head->id = i;
-   client_head->flags = 0;
+   if (client_head->x != c_rect->left) {
+   client_head->x = c_rect->left;
+   status = MONITORS_CONFIG_MODIFIED;
+   }
+   if (client_head->y != c_rect->top) {
+   client_head->y = c_rect->top;
+   status = MONITORS_CONFIG_MODIFIED;
+   }
+   if (client_head->width != c_rect->right - c_rect->left) {
+   client_head->width = c_rect->right - c_rect->left;
+   status = MONITORS_CONFIG_MODIFIED;
+   }
+   if (client_head->height != c_rect->bottom - c_rect->top) {
+   client_head->height = c_rect->bottom - c_rect->top;
+   status = MONITORS_CONFIG_MODIFIED;
+   }
+   if (client_head->surface_id != 0) {
+   client_head->surface_id = 0;
+   status = MONITORS_CONFIG_MODIFIED;
+   }
+   if (client_head->id != i) {
+   client_head-

[drm/qxl v3 5/7] qxl: Remove qxl_bo_init() return value

[Christophe Fergeau]

It's always returning 0, and it's always ignored.
---
 drivers/gpu/drm/qxl/qxl_drv.h | 2 +-
 drivers/gpu/drm/qxl/qxl_gem.c | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 5a4720a..feac7e6 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -398,7 +398,7 @@ int qxl_create_monitors_object(struct qxl_device *qdev);
 int qxl_destroy_monitors_object(struct qxl_device *qdev);

 /* qxl_gem.c */
-int qxl_gem_init(struct qxl_device *qdev);
+void qxl_gem_init(struct qxl_device *qdev);
 void qxl_gem_fini(struct qxl_device *qdev);
 int qxl_gem_object_create(struct qxl_device *qdev, int size,
  int alignment, int initial_domain,
diff --git a/drivers/gpu/drm/qxl/qxl_gem.c b/drivers/gpu/drm/qxl/qxl_gem.c
index d9746e9..3f185c4 100644
--- a/drivers/gpu/drm/qxl/qxl_gem.c
+++ b/drivers/gpu/drm/qxl/qxl_gem.c
@@ -111,10 +111,9 @@ void qxl_gem_object_close(struct drm_gem_object *obj,
 {
 }

-int qxl_gem_init(struct qxl_device *qdev)
+void qxl_gem_init(struct qxl_device *qdev)
 {
INIT_LIST_HEAD(>gem.objects);
-   return 0;
 }

 void qxl_gem_fini(struct qxl_device *qdev)
-- 
2.9.3

[drm/qxl v3 4/7] qxl: Call qxl_gem_{init,fini}

[Christophe Fergeau]

qdev->gem.objects was initialized directly in qxl_device_init() rather
than going through qxl_gem_init(), and qxl_gem_fini() was never called.

Signed-off-by: Christophe Fergeau 
---
 drivers/gpu/drm/qxl/qxl_kms.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
index e642242..af685f1 100644
--- a/drivers/gpu/drm/qxl/qxl_kms.c
+++ b/drivers/gpu/drm/qxl/qxl_kms.c
@@ -131,7 +131,7 @@ static int qxl_device_init(struct qxl_device *qdev,
mutex_init(>update_area_mutex);
mutex_init(>release_mutex);
mutex_init(>surf_evict_mutex);
-   INIT_LIST_HEAD(>gem.objects);
+   qxl_gem_init(qdev);

qdev->rom_base = pci_resource_start(pdev, 2);
qdev->rom_size = pci_resource_len(pdev, 2);
@@ -273,6 +273,7 @@ static void qxl_device_fini(struct qxl_device *qdev)
qxl_ring_free(qdev->command_ring);
qxl_ring_free(qdev->cursor_ring);
qxl_ring_free(qdev->release_ring);
+   qxl_gem_fini(qdev);
qxl_bo_fini(qdev);
io_mapping_free(qdev->surface_mapping);
io_mapping_free(qdev->vram_mapping);
-- 
2.9.3

[drm/qxl v3 3/7] qxl: Add missing '\n' to qxl_io_log() call

[Christophe Fergeau]

The message has to be terminated by a newline as it's not going to get
added automatically.

Signed-off-by: Christophe Fergeau 
---
 drivers/gpu/drm/qxl/qxl_fb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/qxl/qxl_fb.c b/drivers/gpu/drm/qxl/qxl_fb.c
index 28c1423..969c7c1 100644
--- a/drivers/gpu/drm/qxl/qxl_fb.c
+++ b/drivers/gpu/drm/qxl/qxl_fb.c
@@ -198,7 +198,7 @@ static int qxlfb_framebuffer_dirty(struct drm_framebuffer 
*fb,
/*
 * we are using a shadow draw buffer, at qdev->surface0_shadow
 */
-   qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]", clips->x1, clips->x2,
+   qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]\n", clips->x1, clips->x2,
   clips->y1, clips->y2);
image->dx = clips->x1;
image->dy = clips->y1;
-- 
2.9.3

[drm/qxl v3 2/7] qxl: Remove unused prototype

[Christophe Fergeau]

qxl_crtc_set_from_monitors_config() is defined in qxl_drv.h but never
implemented.

Signed-off-by: Christophe Fergeau 
---
 drivers/gpu/drm/qxl/qxl_drv.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index a3c1131..5a4720a 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -397,9 +397,6 @@ void qxl_display_read_client_monitors_config(struct 
qxl_device *qdev);
 int qxl_create_monitors_object(struct qxl_device *qdev);
 int qxl_destroy_monitors_object(struct qxl_device *qdev);

-/* used by qxl_debugfs only */
-void qxl_crtc_set_from_monitors_config(struct qxl_device *qdev);
-
 /* qxl_gem.c */
 int qxl_gem_init(struct qxl_device *qdev);
 void qxl_gem_fini(struct qxl_device *qdev);
-- 
2.9.3

[drm/qxl v3 1/7] qxl: Mark some internal functions as static

[Christophe Fergeau]

They are not used outside of their respective source file

Signed-off-by: Christophe Fergeau 
---
 drivers/gpu/drm/qxl/qxl_cmd.c | 2 +-
 drivers/gpu/drm/qxl/qxl_display.c | 4 ++--
 drivers/gpu/drm/qxl/qxl_drv.h | 3 ---
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index 04270f5..74fc936 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -578,7 +578,7 @@ int qxl_hw_surface_dealloc(struct qxl_device *qdev,
return 0;
 }

-int qxl_update_surface(struct qxl_device *qdev, struct qxl_bo *surf)
+static int qxl_update_surface(struct qxl_device *qdev, struct qxl_bo *surf)
 {
struct qxl_rect rect;
int ret;
diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index 3aef127..8cf5177 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -36,7 +36,7 @@ static bool qxl_head_enabled(struct qxl_head *head)
return head->width && head->height;
 }

-void qxl_alloc_client_monitors_config(struct qxl_device *qdev, unsigned count)
+static void qxl_alloc_client_monitors_config(struct qxl_device *qdev, unsigned 
count)
 {
if (qdev->client_monitors_config &&
count > qdev->client_monitors_config->count) {
@@ -559,7 +559,7 @@ static bool qxl_crtc_mode_fixup(struct drm_crtc *crtc,
return true;
 }

-void
+static void
 qxl_send_monitors_config(struct qxl_device *qdev)
 {
int i;
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 8e633ca..a3c1131 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -394,13 +394,11 @@ qxl_framebuffer_init(struct drm_device *dev,
 struct drm_gem_object *obj,
 const struct drm_framebuffer_funcs *funcs);
 void qxl_display_read_client_monitors_config(struct qxl_device *qdev);
-void qxl_send_monitors_config(struct qxl_device *qdev);
 int qxl_create_monitors_object(struct qxl_device *qdev);
 int qxl_destroy_monitors_object(struct qxl_device *qdev);

 /* used by qxl_debugfs only */
 void qxl_crtc_set_from_monitors_config(struct qxl_device *qdev);
-void qxl_alloc_client_monitors_config(struct qxl_device *qdev, unsigned count);

 /* qxl_gem.c */
 int qxl_gem_init(struct qxl_device *qdev);
@@ -573,6 +571,5 @@ int qxl_bo_check_id(struct qxl_device *qdev, struct qxl_bo 
*bo);
 struct qxl_drv_surface *
 qxl_surface_lookup(struct drm_device *dev, int surface_id);
 void qxl_surface_evict(struct qxl_device *qdev, struct qxl_bo *surf, bool 
freeing);
-int qxl_update_surface(struct qxl_device *qdev, struct qxl_bo *surf);

 #endif
-- 
2.9.3

[drm/qxl v3 0/7] qxl: Various cleanups/fixes

[Christophe Fergeau]

Hey,

Same series as v2 except that I removed the use of camel case in patch 6/7.
Now it's only using an anonymous enum + int.

Christophe

[Spice-devel] [drm/qxl 5/6] qxl: Don't notify userspace when monitors config is unchanged

[Christophe Fergeau]

On Thu, Nov 03, 2016 at 06:42:38PM +, Emil Velikov wrote:
> Hi guys,
> 
> On 2 November 2016 at 16:31, Christophe Fergeau  
> wrote:
> > Hey,
> >
> > On Mon, Oct 31, 2016 at 08:00:09AM -0400, Frediano Ziglio wrote:
> >> > diff --git a/drivers/gpu/drm/qxl/qxl_display.c
> >> > b/drivers/gpu/drm/qxl/qxl_display.c
> >> > index 156b7de..edb90f6 100644
> >> > --- a/drivers/gpu/drm/qxl/qxl_display.c
> >> > +++ b/drivers/gpu/drm/qxl/qxl_display.c
> >> > @@ -57,11 +57,18 @@ static void qxl_alloc_client_monitors_config(struct
> >> > qxl_device *qdev, unsigned c
> >> > qdev->client_monitors_config->count = count;
> >> >  }
> >> >
> >> > +enum MonitorsConfigCopyStatus {
> >> > +   MONITORS_CONFIG_COPIED,
> >> > +   MONITORS_CONFIG_UNCHANGED,
> >> > +   MONITORS_CONFIG_BAD_CRC,
> >> > +};
> >> > +
> >>
> >> I don't remember exactly kernel style, a
> >>
> >> typedef enum {
> >>   MONITORS_CONFIG_COPIED,
> >>   MONITORS_CONFIG_UNCHANGED,
> >>   MONITORS_CONFIG_BAD_CRC,
> >> } MonitorsConfigCopyStatus;
> >>
> >> could make following code shorter.
> >
> > A git grep enum in qxl/ returns a dozen results, none of these using
> > typedef, I guess I just followed that style.
> >
> Kernel coding style advises against both typedefs and CamelCase. We do
> have a few "offenders" but it's better to not add more.
> 
> Ftw when in doubt do search/grep through the document - I don't thinks
> there's many people who've read the thing in one go :-)

Ah thanks, I'll get rid of the CamelCase!

Christophe
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: 
<https://lists.freedesktop.org/archives/dri-devel/attachments/20161104/8881115e/attachment.sig>

[Spice-devel] [drm/qxl v2 7/7] qxl: Allow resolution which are not multiple of 8

[Christophe Fergeau]

On Thu, Nov 03, 2016 at 06:08:39PM +0100, Gerd Hoffmann wrote:
> > Or maybe other parts of the
> > kernel/userspace rely on this rounding down.
> 
> This is where I suspect we could run in trouble.  Odd resolutions simply
> don't happen on physical hardware, all usual resolutions are a multiple
> of 8, most of them are even a multiple of 16.
> 
> Various image/video formats use 16x16 blocks.
> The qemu vnc server operates on 16x16 blocks too (and we had bugs in the
> past with odd resolutions).
> 
> Also scanlines and cachelines align nicely if you don't use odd
> resolutions.
> 
> > I unfortunately don't know
> > :(
> 
> I don't have definitive answers too, just a gut feeling that this might
> cause trouble.

I think this might be fine actually, there is already one such
resolution in the kernel, which is 1366x768 (1366 is only a multiple of
2). There is already a bit of a hack to handle it anyway, see
fixup_mode_1366x768() in drm_edid.c.

> 
> Maybe we should add a module option for this?  So there is an easy
> (as-in: doesn't require a kernel rebuild) way out in case it causes
> trouble in certain setups?

This seems a bit overkill to me, but I can look into it if needed.

Christophe
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: 

[drm/qxl v2 7/7] qxl: Allow resolution which are not multiple of 8

[Christophe Fergeau]

On Thu, Nov 03, 2016 at 09:53:48AM +0100, Gerd Hoffmann wrote:
> On Mi, 2016-11-02 at 18:00 +0100, Christophe Fergeau wrote:
> > The use of drm_cvt_mode() in qxl_add_monitors_config_modes() means that
> > the resolutions we are going to present to user-space are going to be
> > rounded down to a multiple of 8. In the QXL arbitrary resolution case,
> > this is not useful.
> > This commit forces the actual width/height that was requested by the
> > client in the drm_display_mode structure rather than keeping the
> > rounded version.
> 
> Hmm, not sure this is a good idea.  There are probably reasons why
> drm_cvt_mode is rounding down ...

Yeah, I'm sure there are reasons, but I don't know what they are.
drm_cvt_mode seems to be calculating various frequencies and timings
related to refreshing real world monitors, and this seems to be defined
by some VESA standard. Maybe the rounding down is because the real-world
monitors or VESA require it. Or maybe other parts of the
kernel/userspace rely on this rounding down. I unfortunately don't know
:( Any guidance there whether that's ok, or whether I should approach
this differently would be very useful.

Christophe
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: 
<https://lists.freedesktop.org/archives/dri-devel/attachments/20161103/013280ea/attachment.sig>

[Spice-devel] [drm/qxl 5/6] qxl: Don't notify userspace when monitors config is unchanged

[Christophe Fergeau]

On Wed, Nov 02, 2016 at 05:31:28PM +0100, Christophe Fergeau wrote:
> > 
> > > + if (client_head->flags != 0) {
> > > + client_head->flags = 0;
> > > + changed = true;
> > > + }
> > 
> > why testing flags change if is always 0 ?
> 
> Yeah, same for surface_id above actually. Just mechanically changed
> everything ;)

At first I changed the commit to exclude flags and surface_id from these
checks, but on second though, I'd keep them the same way as the rest,
this way if these can get changed to a different value outside of
copy_rom, this code will still work as intended.

Christophe
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: 
<https://lists.freedesktop.org/archives/dri-devel/attachments/20161102/2b5b804f/attachment.sig>

[drm/qxl v2 7/7] qxl: Allow resolution which are not multiple of 8

[Christophe Fergeau]

The use of drm_cvt_mode() in qxl_add_monitors_config_modes() means that
the resolutions we are going to present to user-space are going to be
rounded down to a multiple of 8. In the QXL arbitrary resolution case,
this is not useful.
This commit forces the actual width/height that was requested by the
client in the drm_display_mode structure rather than keeping the
rounded version.

Signed-off-by: Christophe Fergeau 
---
 drivers/gpu/drm/qxl/qxl_display.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index 2f1d738..2241954 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -200,6 +200,9 @@ static int qxl_add_monitors_config_modes(struct 
drm_connector *connector,
mode = drm_cvt_mode(dev, head->width, head->height, 60, false, false,
false);
mode->type |= DRM_MODE_TYPE_PREFERRED;
+   mode->hdisplay = head->width;
+   mode->vdisplay = head->height;
+   drm_mode_set_name(mode);
*pwidth = head->width;
*pheight = head->height;
drm_mode_probed_add(connector, mode);
-- 
2.9.3

[drm/qxl v2 6/7] qxl: Don't notify userspace when monitors config is unchanged

[Christophe Fergeau]

When the QXL driver receives a QXL_INTERRUPT_CLIENT_MONITORS_CONFIG interrupt,
we currently always notify userspace that there was some hotplug event.

However, gnome-shell/mutter is reacting to this event by attempting a
resolution change, which it does by issueing drmModeRmFB, drmModeAddFB,
and then drmModeSetCrtc. This has the side-effect of causing
qxl_crtc_mode_set() to tell the QXL virtual hardware that a primary
surface was destroyed and created. After going through QEMU and then the
remote SPICE client, a new identical monitors config message will be
sent, resulting in a QXL_INTERRUPT_CLIENT_MONITORS_CONFIG interrupt to
be emitted, and the same scenario occurring again.

As destroying/creating the primary surface causes a visible screen
flicker, this makes the guest hard to use (
https://bugzilla.redhat.com/show_bug.cgi?id=1266484 ).

This commit checks if the screen configuration we received is the same
one as the current one, and does not notify userspace about it if that's
the case.

Signed-off-by: Christophe Fergeau 
---
Changes since v1:
  - made qxl_display_copy_rom_client_monitors_config return an enum
MonitorsConfigCopyStatus
  - remove 'bool changed' from +qxl_display_copy_rom_client_monitors_config and 
directly
set the return value instead

 drivers/gpu/drm/qxl/qxl_display.c | 65 ---
 1 file changed, 54 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index 8cf5177..2f1d738 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -57,11 +57,19 @@ static void qxl_alloc_client_monitors_config(struct 
qxl_device *qdev, unsigned c
qdev->client_monitors_config->count = count;
 }

-static int qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
+enum MonitorsConfigCopyStatus {
+   MONITORS_CONFIG_MODIFIED,
+   MONITORS_CONFIG_UNCHANGED,
+   MONITORS_CONFIG_BAD_CRC,
+};
+
+static enum MonitorsConfigCopyStatus
+qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
 {
int i;
int num_monitors;
uint32_t crc;
+   enum MonitorsConfigCopyStatus status = MONITORS_CONFIG_UNCHANGED;

num_monitors = qdev->rom->client_monitors_config.count;
crc = crc32(0, (const uint8_t *)>rom->client_monitors_config,
@@ -70,7 +78,7 @@ static int qxl_display_copy_rom_client_monitors_config(struct 
qxl_device *qdev)
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;
+   return MONITORS_CONFIG_BAD_CRC;
}
if (num_monitors > qdev->monitors_config->max_allowed) {
DRM_DEBUG_KMS("client monitors list will be truncated: %d < 
%d\n",
@@ -79,6 +87,10 @@ static int 
qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
} else {
num_monitors = qdev->rom->client_monitors_config.count;
}
+   if (qdev->client_monitors_config
+ && (num_monitors != qdev->client_monitors_config->count)) {
+   status = MONITORS_CONFIG_MODIFIED;
+   }
qxl_alloc_client_monitors_config(qdev, num_monitors);
/* we copy max from the client but it isn't used */
qdev->client_monitors_config->max_allowed =
@@ -88,17 +100,39 @@ static int 
qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
>rom->client_monitors_config.heads[i];
struct qxl_head *client_head =
>client_monitors_config->heads[i];
-   client_head->x = c_rect->left;
-   client_head->y = c_rect->top;
-   client_head->width = c_rect->right - c_rect->left;
-   client_head->height = c_rect->bottom - c_rect->top;
-   client_head->surface_id = 0;
-   client_head->id = i;
-   client_head->flags = 0;
+   if (client_head->x != c_rect->left) {
+   client_head->x = c_rect->left;
+   status = MONITORS_CONFIG_MODIFIED;
+   }
+   if (client_head->y != c_rect->top) {
+   client_head->y = c_rect->top;
+   status = MONITORS_CONFIG_MODIFIED;
+   }
+   if (client_head->width != c_rect->right - c_rect->left) {
+   client_head->width = c_rect->right - c_rect->left;
+   status = MONITORS_CONFIG_MODIFIED;
+   }
+   if (client_head->height != c_rect->bottom - c_rect->top) {
+   

[drm/qxl v2 5/7] qxl: Remove qxl_bo_init() return value

[Christophe Fergeau]

It's always returning 0, and it's always ignored.

Signed-off-by: Christophe Fergeau 
---
Changes since v1: new patch

 drivers/gpu/drm/qxl/qxl_drv.h | 2 +-
 drivers/gpu/drm/qxl/qxl_gem.c | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 5a4720a..feac7e6 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -398,7 +398,7 @@ int qxl_create_monitors_object(struct qxl_device *qdev);
 int qxl_destroy_monitors_object(struct qxl_device *qdev);

 /* qxl_gem.c */
-int qxl_gem_init(struct qxl_device *qdev);
+void qxl_gem_init(struct qxl_device *qdev);
 void qxl_gem_fini(struct qxl_device *qdev);
 int qxl_gem_object_create(struct qxl_device *qdev, int size,
  int alignment, int initial_domain,
diff --git a/drivers/gpu/drm/qxl/qxl_gem.c b/drivers/gpu/drm/qxl/qxl_gem.c
index d9746e9..3f185c4 100644
--- a/drivers/gpu/drm/qxl/qxl_gem.c
+++ b/drivers/gpu/drm/qxl/qxl_gem.c
@@ -111,10 +111,9 @@ void qxl_gem_object_close(struct drm_gem_object *obj,
 {
 }

-int qxl_gem_init(struct qxl_device *qdev)
+void qxl_gem_init(struct qxl_device *qdev)
 {
INIT_LIST_HEAD(>gem.objects);
-   return 0;
 }

 void qxl_gem_fini(struct qxl_device *qdev)
-- 
2.9.3

[drm/qxl v2 4/7] qxl: Call qxl_gem_{init,fini}

[Christophe Fergeau]

qdev->gem.objects was initialized directly in qxl_device_init() rather
than going through qxl_gem_init(), and qxl_gem_fini() was never called.

Signed-off-by: Christophe Fergeau 
---
 drivers/gpu/drm/qxl/qxl_kms.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
index e642242..af685f1 100644
--- a/drivers/gpu/drm/qxl/qxl_kms.c
+++ b/drivers/gpu/drm/qxl/qxl_kms.c
@@ -131,7 +131,7 @@ static int qxl_device_init(struct qxl_device *qdev,
mutex_init(>update_area_mutex);
mutex_init(>release_mutex);
mutex_init(>surf_evict_mutex);
-   INIT_LIST_HEAD(>gem.objects);
+   qxl_gem_init(qdev);

qdev->rom_base = pci_resource_start(pdev, 2);
qdev->rom_size = pci_resource_len(pdev, 2);
@@ -273,6 +273,7 @@ static void qxl_device_fini(struct qxl_device *qdev)
qxl_ring_free(qdev->command_ring);
qxl_ring_free(qdev->cursor_ring);
qxl_ring_free(qdev->release_ring);
+   qxl_gem_fini(qdev);
qxl_bo_fini(qdev);
io_mapping_free(qdev->surface_mapping);
io_mapping_free(qdev->vram_mapping);
-- 
2.9.3

[drm/qxl v2 3/7] qxl: Add missing '\n' to qxl_io_log() call

[Christophe Fergeau]

The message has to be terminated by a newline as it's not going to get
added automatically.

Signed-off-by: Christophe Fergeau 
Acked-by: Frediano Ziglio 
---
 drivers/gpu/drm/qxl/qxl_fb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/qxl/qxl_fb.c b/drivers/gpu/drm/qxl/qxl_fb.c
index 28c1423..969c7c1 100644
--- a/drivers/gpu/drm/qxl/qxl_fb.c
+++ b/drivers/gpu/drm/qxl/qxl_fb.c
@@ -198,7 +198,7 @@ static int qxlfb_framebuffer_dirty(struct drm_framebuffer 
*fb,
/*
 * we are using a shadow draw buffer, at qdev->surface0_shadow
 */
-   qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]", clips->x1, clips->x2,
+   qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]\n", clips->x1, clips->x2,
   clips->y1, clips->y2);
image->dx = clips->x1;
image->dy = clips->y1;
-- 
2.9.3

[drm/qxl v2 2/7] qxl: Remove unused prototype

[Christophe Fergeau]

qxl_crtc_set_from_monitors_config() is defined in qxl_drv.h but never
implemented.

Signed-off-by: Christophe Fergeau 
Acked-by: Frediano Ziglio 
---
 drivers/gpu/drm/qxl/qxl_drv.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index a3c1131..5a4720a 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -397,9 +397,6 @@ void qxl_display_read_client_monitors_config(struct 
qxl_device *qdev);
 int qxl_create_monitors_object(struct qxl_device *qdev);
 int qxl_destroy_monitors_object(struct qxl_device *qdev);

-/* used by qxl_debugfs only */
-void qxl_crtc_set_from_monitors_config(struct qxl_device *qdev);
-
 /* qxl_gem.c */
 int qxl_gem_init(struct qxl_device *qdev);
 void qxl_gem_fini(struct qxl_device *qdev);
-- 
2.9.3

[drm/qxl v2 1/7] qxl: Mark some internal functions as static

[Christophe Fergeau]

They are not used outside of their respective source file

Signed-off-by: Christophe Fergeau 
Acked-by: Frediano Ziglio 
---
 drivers/gpu/drm/qxl/qxl_cmd.c | 2 +-
 drivers/gpu/drm/qxl/qxl_display.c | 4 ++--
 drivers/gpu/drm/qxl/qxl_drv.h | 3 ---
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index 04270f5..74fc936 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -578,7 +578,7 @@ int qxl_hw_surface_dealloc(struct qxl_device *qdev,
return 0;
 }

-int qxl_update_surface(struct qxl_device *qdev, struct qxl_bo *surf)
+static int qxl_update_surface(struct qxl_device *qdev, struct qxl_bo *surf)
 {
struct qxl_rect rect;
int ret;
diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index 3aef127..8cf5177 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -36,7 +36,7 @@ static bool qxl_head_enabled(struct qxl_head *head)
return head->width && head->height;
 }

-void qxl_alloc_client_monitors_config(struct qxl_device *qdev, unsigned count)
+static void qxl_alloc_client_monitors_config(struct qxl_device *qdev, unsigned 
count)
 {
if (qdev->client_monitors_config &&
count > qdev->client_monitors_config->count) {
@@ -559,7 +559,7 @@ static bool qxl_crtc_mode_fixup(struct drm_crtc *crtc,
return true;
 }

-void
+static void
 qxl_send_monitors_config(struct qxl_device *qdev)
 {
int i;
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 8e633ca..a3c1131 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -394,13 +394,11 @@ qxl_framebuffer_init(struct drm_device *dev,
 struct drm_gem_object *obj,
 const struct drm_framebuffer_funcs *funcs);
 void qxl_display_read_client_monitors_config(struct qxl_device *qdev);
-void qxl_send_monitors_config(struct qxl_device *qdev);
 int qxl_create_monitors_object(struct qxl_device *qdev);
 int qxl_destroy_monitors_object(struct qxl_device *qdev);

 /* used by qxl_debugfs only */
 void qxl_crtc_set_from_monitors_config(struct qxl_device *qdev);
-void qxl_alloc_client_monitors_config(struct qxl_device *qdev, unsigned count);

 /* qxl_gem.c */
 int qxl_gem_init(struct qxl_device *qdev);
@@ -573,6 +571,5 @@ int qxl_bo_check_id(struct qxl_device *qdev, struct qxl_bo 
*bo);
 struct qxl_drv_surface *
 qxl_surface_lookup(struct drm_device *dev, int surface_id);
 void qxl_surface_evict(struct qxl_device *qdev, struct qxl_bo *surf, bool 
freeing);
-int qxl_update_surface(struct qxl_device *qdev, struct qxl_bo *surf);

 #endif
-- 
2.9.3

[drm/qxl v2 0/7] qxl: Various cleanups/fixes

[Christophe Fergeau]

Hey,

Here is the v2 of my patch series. It improves a bit patch 6/7 readability
following Frediano's review, and patch 5/7 is new and was suggested during
review. The other patches are unchanged.

Christophe

[Spice-devel] [drm/qxl 5/6] qxl: Don't notify userspace when monitors config is unchanged

[Christophe Fergeau]

Hey,

On Mon, Oct 31, 2016 at 08:00:09AM -0400, Frediano Ziglio wrote:
> > diff --git a/drivers/gpu/drm/qxl/qxl_display.c
> > b/drivers/gpu/drm/qxl/qxl_display.c
> > index 156b7de..edb90f6 100644
> > --- a/drivers/gpu/drm/qxl/qxl_display.c
> > +++ b/drivers/gpu/drm/qxl/qxl_display.c
> > @@ -57,11 +57,18 @@ static void qxl_alloc_client_monitors_config(struct
> > qxl_device *qdev, unsigned c
> > qdev->client_monitors_config->count = count;
> >  }
> >  
> > +enum MonitorsConfigCopyStatus {
> > +   MONITORS_CONFIG_COPIED,
> > +   MONITORS_CONFIG_UNCHANGED,
> > +   MONITORS_CONFIG_BAD_CRC,
> > +};
> > +
> 
> I don't remember exactly kernel style, a 
> 
> typedef enum {
>   MONITORS_CONFIG_COPIED,
>   MONITORS_CONFIG_UNCHANGED,
>   MONITORS_CONFIG_BAD_CRC,
> } MonitorsConfigCopyStatus;
> 
> could make following code shorter.

A git grep enum in qxl/ returns a dozen results, none of these using
typedef, I guess I just followed that style.

> 
> >  static int qxl_display_copy_rom_client_monitors_config(struct qxl_device
> >  *qdev)
> 
> why not returning MonitorsConfigCopyStatus ?

No idea ;) I'll change the patch.

> 
> >  {
> > int i;
> > int num_monitors;
> > uint32_t crc;
> > +   bool changed = false;
> >  
> 
> using a "MonitorsConfigCopyStatus res = MONITORS_CONFIG_UNCHANGED" here
> could make return statement shorter.
> 
> > num_monitors = qdev->rom->client_monitors_config.count;
> > crc = crc32(0, (const uint8_t *)>rom->client_monitors_config,
> > @@ -88,17 +99,42 @@ static int
> > qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
> > >rom->client_monitors_config.heads[i];
> > struct qxl_head *client_head =
> > >client_monitors_config->heads[i];
> > -   client_head->x = c_rect->left;
> > -   client_head->y = c_rect->top;
> > -   client_head->width = c_rect->right - c_rect->left;
> > -   client_head->height = c_rect->bottom - c_rect->top;
> > -   client_head->surface_id = 0;
> > -   client_head->id = i;
> > -   client_head->flags = 0;
> > +   if (client_head->x != c_rect->left) {
> > +   client_head->x = c_rect->left;
> > +   changed = true;
> > +   }
> > +   if (client_head->y != c_rect->top) {
> > +   client_head->y = c_rect->top;
> > +   changed = true;
> > +   }
> > +   if (client_head->width != c_rect->right - c_rect->left) {
> > +   client_head->width = c_rect->right - c_rect->left;
> > +   changed = true;
> > +   }
> > +   if (client_head->height != c_rect->bottom - c_rect->top) {
> > +   client_head->height = c_rect->bottom - c_rect->top;
> > +   changed = true;
> > +   }
> > +   if (client_head->surface_id != 0) {
> > +   client_head->surface_id = 0;
> > +   changed = true;
> > +   }
> > +   if (client_head->id != i) {
> > +   client_head->id = i;
> > +   changed = true;
> > +   }
> 
> quite similar code... I would write a macro but I'm a too macro fun :)
> Would be something like this
> 
>   if (client_head->id != i)
>   res = MONITORS_CONFIG_COPIED;
>   client_head->id = i;
> 
> make sense?

I'm not a big macro fan, especially if they have side effects, so I
preferred to keep things explicit, even though I am annoyed by the
repetitive code too /o\


> 
> > +   if (client_head->flags != 0) {
> > +   client_head->flags = 0;
> > +   changed = true;
> > +   }
> 
> why testing flags change if is always 0 ?

Yeah, same for surface_id above actually. Just mechanically changed
everything ;)

> 
> Usually I would write something like
> 
>   for (;;) {
>   status = qxl_display_copy_rom_client_monitors_config(qdev);
>   if (status != MONITORS_CONFIG_BAD_CRC)
>   break;
>   qxl_io_log(qdev, "failed crc check for client_monitors_config,"
>" retrying\n");
>   }
> 
> or
> 
>   while ((status = qxl_display_copy_rom_client_monitors_config(qdev))
>   ==  MONITORS_CONFIG_BAD_CRC) {
>   qxl_io_log(qdev, "failed crc check for client_monitors_config,"
>" retrying\n");
>   }
> 
> (just style and probably indentation is even wrong)

Same as above, I don't like either, first one obscures the loop exit
condition, and second one makes the assignment/test harder to read.

Christophe
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: 

[Spice-devel] [drm/qxl 4/6] qxl: Call qxl_gem_{init,fini}

[Christophe Fergeau]

On Mon, Oct 31, 2016 at 07:42:35AM -0400, Frediano Ziglio wrote:
> > 
> > qdev->gem.objects was initialized directly in qxl_device_init() rather
> > than going through qxl_gem_init(), and qxl_gem_fini() was never called.
> > 
> 
> Considering "qxl_gem_fini() was never called" did we have a leak?

Maybe, as this forces the freeing of some pending bo's if they are still in
use. This would only happen when unloading/reloading the module
repeatedly, which I don't expect to happen on production systems.

> 
> > Signed-off-by: Christophe Fergeau 
> > ---
> >  drivers/gpu/drm/qxl/qxl_kms.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
> > index e642242..af685f1 100644
> > --- a/drivers/gpu/drm/qxl/qxl_kms.c
> > +++ b/drivers/gpu/drm/qxl/qxl_kms.c
> > @@ -131,7 +131,7 @@ static int qxl_device_init(struct qxl_device *qdev,
> > mutex_init(>update_area_mutex);
> > mutex_init(>release_mutex);
> > mutex_init(>surf_evict_mutex);
> > -   INIT_LIST_HEAD(>gem.objects);
> > +   qxl_gem_init(qdev);
> >  
> 
> Here qxl_gem_init returns a value that is always ignored, perhaps
> would be better to return void from qxl_gem_init if it cannot
> fails.

Good suggestion, I'll add that to a separate patch.

Christophe
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: 
<https://lists.freedesktop.org/archives/dri-devel/attachments/20161102/06aaea44/attachment.sig>

[drm/qxl 6/6] qxl: Allow resolution which are not multiple of 8

[Christophe Fergeau]

The use of drm_cvt_mode() in qxl_add_monitors_config_modes() means that
the resolutions we are going to present to user-space are going to be
rounded down to a multiple of 8. In the QXL arbitrary resolution case,
this is not useful.
This commit forces the actual width/height that was requested by the
client in the drm_display_mode structure rather than keeping the
rounded version.

Signed-off-by: Christophe Fergeau 
---

I know this is very hacky, but I have no idea what is important to be set in 
the mode struct,
if there is a better way to create it without getting the rounding to a 
multiple of 8, ...


 drivers/gpu/drm/qxl/qxl_display.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index edb90f6..fc5b01e 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -202,6 +202,9 @@ static int qxl_add_monitors_config_modes(struct 
drm_connector *connector,
mode = drm_cvt_mode(dev, head->width, head->height, 60, false, false,
false);
mode->type |= DRM_MODE_TYPE_PREFERRED;
+   mode->hdisplay = head->width;
+   mode->vdisplay = head->height;
+   drm_mode_set_name(mode);
*pwidth = head->width;
*pheight = head->height;
drm_mode_probed_add(connector, mode);
-- 
2.9.3

[drm/qxl 5/6] qxl: Don't notify userspace when monitors config is unchanged

[Christophe Fergeau]

When the QXL driver receives a QXL_INTERRUPT_CLIENT_MONITORS_CONFIG interrupt,
we currently always notify userspace that there was some hotplug event.

However, gnome-shell/mutter is reacting to this event by attempting a
resolution change, which it does by issueing drmModeRmFB, drmModeAddFB,
and then drmModeSetCrtc. This has the side-effect of causing
qxl_crtc_mode_set() to tell the QXL virtual hardware that a primary
surface was destroyed and created. After going through QEMU and then the
remote SPICE client, a new identical monitors config message will be
sent, resulting in a QXL_INTERRUPT_CLIENT_MONITORS_CONFIG interrupt to
be emitted, and the same scenario occurring again.

As destroying/creating the primary surface causes a visible screen
flicker, this makes the guest hard to use (
https://bugzilla.redhat.com/show_bug.cgi?id=1266484 ).

This commit checks if the screen configuration we received is the same
one as the current one, and does not notify userspace about it if that's
the case.

Signed-off-by: Christophe Fergeau 
---
 drivers/gpu/drm/qxl/qxl_display.c | 65 +--
 1 file changed, 55 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index 156b7de..edb90f6 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -57,11 +57,18 @@ static void qxl_alloc_client_monitors_config(struct 
qxl_device *qdev, unsigned c
qdev->client_monitors_config->count = count;
 }

+enum MonitorsConfigCopyStatus {
+   MONITORS_CONFIG_COPIED,
+   MONITORS_CONFIG_UNCHANGED,
+   MONITORS_CONFIG_BAD_CRC,
+};
+
 static int qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
 {
int i;
int num_monitors;
uint32_t crc;
+   bool changed = false;

num_monitors = qdev->rom->client_monitors_config.count;
crc = crc32(0, (const uint8_t *)>rom->client_monitors_config,
@@ -70,7 +77,7 @@ static int qxl_display_copy_rom_client_monitors_config(struct 
qxl_device *qdev)
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;
+   return MONITORS_CONFIG_BAD_CRC;
}
if (num_monitors > qdev->monitors_config->max_allowed) {
DRM_DEBUG_KMS("client monitors list will be truncated: %d < 
%d\n",
@@ -79,6 +86,10 @@ static int 
qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
} else {
num_monitors = qdev->rom->client_monitors_config.count;
}
+   if (qdev->client_monitors_config
+ && (num_monitors != qdev->client_monitors_config->count)) {
+   changed = true;
+   }
qxl_alloc_client_monitors_config(qdev, num_monitors);
/* we copy max from the client but it isn't used */
qdev->client_monitors_config->max_allowed =
@@ -88,17 +99,42 @@ static int 
qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
>rom->client_monitors_config.heads[i];
struct qxl_head *client_head =
>client_monitors_config->heads[i];
-   client_head->x = c_rect->left;
-   client_head->y = c_rect->top;
-   client_head->width = c_rect->right - c_rect->left;
-   client_head->height = c_rect->bottom - c_rect->top;
-   client_head->surface_id = 0;
-   client_head->id = i;
-   client_head->flags = 0;
+   if (client_head->x != c_rect->left) {
+   client_head->x = c_rect->left;
+   changed = true;
+   }
+   if (client_head->y != c_rect->top) {
+   client_head->y = c_rect->top;
+   changed = true;
+   }
+   if (client_head->width != c_rect->right - c_rect->left) {
+   client_head->width = c_rect->right - c_rect->left;
+   changed = true;
+   }
+   if (client_head->height != c_rect->bottom - c_rect->top) {
+   client_head->height = c_rect->bottom - c_rect->top;
+   changed = true;
+   }
+   if (client_head->surface_id != 0) {
+   client_head->surface_id = 0;
+   changed = true;
+   }
+   if (client_head->id != i) {
+   client_head->id = i;
+   changed = true;
+   }
+   if (client_head

[drm/qxl 4/6] qxl: Call qxl_gem_{init,fini}

[Christophe Fergeau]

qdev->gem.objects was initialized directly in qxl_device_init() rather
than going through qxl_gem_init(), and qxl_gem_fini() was never called.

Signed-off-by: Christophe Fergeau 
---
 drivers/gpu/drm/qxl/qxl_kms.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
index e642242..af685f1 100644
--- a/drivers/gpu/drm/qxl/qxl_kms.c
+++ b/drivers/gpu/drm/qxl/qxl_kms.c
@@ -131,7 +131,7 @@ static int qxl_device_init(struct qxl_device *qdev,
mutex_init(>update_area_mutex);
mutex_init(>release_mutex);
mutex_init(>surf_evict_mutex);
-   INIT_LIST_HEAD(>gem.objects);
+   qxl_gem_init(qdev);

qdev->rom_base = pci_resource_start(pdev, 2);
qdev->rom_size = pci_resource_len(pdev, 2);
@@ -273,6 +273,7 @@ static void qxl_device_fini(struct qxl_device *qdev)
qxl_ring_free(qdev->command_ring);
qxl_ring_free(qdev->cursor_ring);
qxl_ring_free(qdev->release_ring);
+   qxl_gem_fini(qdev);
qxl_bo_fini(qdev);
io_mapping_free(qdev->surface_mapping);
io_mapping_free(qdev->vram_mapping);
-- 
2.9.3

[drm/qxl 3/6] qxl: Add missing '\n' to qxl_io_log() call

[Christophe Fergeau]

The message has to be terminated by a newline as it's not going to get
added automatically.

Signed-off-by: Christophe Fergeau 
---
 drivers/gpu/drm/qxl/qxl_fb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/qxl/qxl_fb.c b/drivers/gpu/drm/qxl/qxl_fb.c
index 2cd879a..0d16107 100644
--- a/drivers/gpu/drm/qxl/qxl_fb.c
+++ b/drivers/gpu/drm/qxl/qxl_fb.c
@@ -197,7 +197,7 @@ static int qxlfb_framebuffer_dirty(struct drm_framebuffer 
*fb,
/*
 * we are using a shadow draw buffer, at qdev->surface0_shadow
 */
-   qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]", clips->x1, clips->x2,
+   qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]\n", clips->x1, clips->x2,
   clips->y1, clips->y2);
image->dx = clips->x1;
image->dy = clips->y1;
-- 
2.9.3

[drm/qxl 2/6] qxl: Remove unused prototype

[Christophe Fergeau]

qxl_crtc_set_from_monitors_config() is defined in qxl_drv.h but never
implemented.

Signed-off-by: Christophe Fergeau 
---
 drivers/gpu/drm/qxl/qxl_drv.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index da41e1f..590ba25 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -398,9 +398,6 @@ void qxl_display_read_client_monitors_config(struct 
qxl_device *qdev);
 int qxl_create_monitors_object(struct qxl_device *qdev);
 int qxl_destroy_monitors_object(struct qxl_device *qdev);

-/* used by qxl_debugfs only */
-void qxl_crtc_set_from_monitors_config(struct qxl_device *qdev);
-
 /* qxl_gem.c */
 int qxl_gem_init(struct qxl_device *qdev);
 void qxl_gem_fini(struct qxl_device *qdev);
-- 
2.9.3

[drm/qxl 1/6] qxl: Mark some internal functions as static

[Christophe Fergeau]

They are not used outside of their respective source file

Signed-off-by: Christophe Fergeau 
---
 drivers/gpu/drm/qxl/qxl_cmd.c | 2 +-
 drivers/gpu/drm/qxl/qxl_display.c | 4 ++--
 drivers/gpu/drm/qxl/qxl_drv.h | 3 ---
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index 04270f5..74fc936 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -578,7 +578,7 @@ int qxl_hw_surface_dealloc(struct qxl_device *qdev,
return 0;
 }

-int qxl_update_surface(struct qxl_device *qdev, struct qxl_bo *surf)
+static int qxl_update_surface(struct qxl_device *qdev, struct qxl_bo *surf)
 {
struct qxl_rect rect;
int ret;
diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index a61c0d4..156b7de 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -36,7 +36,7 @@ static bool qxl_head_enabled(struct qxl_head *head)
return head->width && head->height;
 }

-void qxl_alloc_client_monitors_config(struct qxl_device *qdev, unsigned count)
+static void qxl_alloc_client_monitors_config(struct qxl_device *qdev, unsigned 
count)
 {
if (qdev->client_monitors_config &&
count > qdev->client_monitors_config->count) {
@@ -607,7 +607,7 @@ static bool qxl_crtc_mode_fixup(struct drm_crtc *crtc,
return true;
 }

-void
+static void
 qxl_send_monitors_config(struct qxl_device *qdev)
 {
int i;
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 5f3e5ad..da41e1f 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -395,13 +395,11 @@ qxl_framebuffer_init(struct drm_device *dev,
 struct drm_gem_object *obj,
 const struct drm_framebuffer_funcs *funcs);
 void qxl_display_read_client_monitors_config(struct qxl_device *qdev);
-void qxl_send_monitors_config(struct qxl_device *qdev);
 int qxl_create_monitors_object(struct qxl_device *qdev);
 int qxl_destroy_monitors_object(struct qxl_device *qdev);

 /* used by qxl_debugfs only */
 void qxl_crtc_set_from_monitors_config(struct qxl_device *qdev);
-void qxl_alloc_client_monitors_config(struct qxl_device *qdev, unsigned count);

 /* qxl_gem.c */
 int qxl_gem_init(struct qxl_device *qdev);
@@ -574,6 +572,5 @@ int qxl_bo_check_id(struct qxl_device *qdev, struct qxl_bo 
*bo);
 struct qxl_drv_surface *
 qxl_surface_lookup(struct drm_device *dev, int surface_id);
 void qxl_surface_evict(struct qxl_device *qdev, struct qxl_bo *surf, bool 
freeing);
-int qxl_update_surface(struct qxl_device *qdev, struct qxl_bo *surf);

 #endif
-- 
2.9.3

[drm/qxl 0/6] Various cleanups/fixes

[Christophe Fergeau]

Hey,

This patch series starts by doing some various small cleanups (patch 1 to 4), 
and
then there are 2 fixes for issues which were noticed in fedora 25, the more 
annoying one
being the one fixed by patch 5 where the VM screen would constantly flicker
when wayland is used. Patch 6 is very hacky, I'm not familiar at all with that 
code,
so I don't know what is the correct way of doing it.

Christophe

[Spice-devel] [PATCH] Do not loop on ERESTARTSYS using interruptible waits

[Christophe Fergeau]

Hey,

On Fri, May 22, 2015 at 09:58:10AM -0400, Frediano Ziglio wrote:
> > >  }
> > >  
> > >  int qxl_io_update_area(struct qxl_device *qdev, struct qxl_bo *surf,
> > > - const struct qxl_rect *area)
> > > + const struct qxl_rect *area, bool intr)
> > >  {
> > >   int surface_id;
> > >   uint32_t surface_width, surface_height;
> > > @@ -350,7 +347,7 @@ int qxl_io_update_area(struct qxl_device *qdev, struct
> > > qxl_bo *surf,
> > >   mutex_lock(>update_area_mutex);
> > >   qdev->ram_header->update_area = *area;
> > >   qdev->ram_header->update_surface = surface_id;
> > > - ret = wait_for_io_cmd_user(qdev, 0, QXL_IO_UPDATE_AREA_ASYNC, true);
> > > + ret = wait_for_io_cmd_user(qdev, 0, QXL_IO_UPDATE_AREA_ASYNC, intr);
> > >   mutex_unlock(>update_area_mutex);
> > >   return ret;
> > >  }
> > > @@ -588,10 +585,7 @@ int qxl_update_surface(struct qxl_device *qdev, 
> > > struct
> > > qxl_bo *surf)
> > >   rect.right = surf->surf.width;
> > >   rect.top = 0;
> > >   rect.bottom = surf->surf.height;
> > > -retry:
> > > - ret = qxl_io_update_area(qdev, surf, );
> > > - if (ret == -ERESTARTSYS)
> > > - goto retry;
> > > + ret = qxl_io_update_area(qdev, surf, , false);
> > 
> > My understanding is that the fix is this hunk? If so, this could be made
> > more obvious with an intermediate commit adding the 'bool intr' arg to
> > qxl_io_update_area and only calling it with 'true' in the appropriate
> > places.
> > This code path is only triggered from qxl_surface_evict() which I assume
> > is not necessarily easily interruptible, so this change makes sense to
> > me. However it would be much better to get a review from Dave Airlie ;)
> > 
> > Christophe
> > 
> 
> Are you asking if just removing the loop would fix the issue?
> So you are proposing a first patch that add the argument always
> passing true and another that change some calls to false? It make
> sense but still the loop should be removed.

Sorry, I was not clear, removing the loop is fine, I was not suggesting
to keep it.

Christophe
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: 

[Spice-devel] [PATCH] Do not loop on ERESTARTSYS using interruptible waits

[Christophe Fergeau]

Hey,

On Tue, May 19, 2015 at 05:54:54AM -0400, Frediano Ziglio wrote:
> This problem happens using KMS surfaces and QXL driver.
> To easy reproduce use KDE Plasma (which use surfaces a lot) and assure
> you are using KMS surfaces (QXL driver on Fedora/RedHat has a patch to
> stop using them). Open some complex application like LibreOffice and
> after a while your machine get stuck using 100% CPU on Xorg.
> The problem occurs as creating new surfaces not interruptible wait
> are used however instead of returning ERESTARTSYS back to userspace
> you try to loop but wait routines always keep returning ERESTARTSYS
> once the signal is marked.
> On out of memory conditions TTM module try to move objects to system
> memory and QXL assure surface is updated before the move.
> The fix handle differently this case using no interruptible wait so
> wait functions will wait instead of returning ERESTARTSYS.
> Note the when the loop occurs driver will send a lot of update requests
> causing more CPU usage on Qemu side too.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  qxl/qxl_cmd.c   | 12 +++-
>  qxl/qxl_drv.h   |  2 +-
>  qxl/qxl_ioctl.c |  2 +-
>  3 files changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drivers/gpu/drm/qxl/qxl_cmd.c b/qxl/qxl_cmd.c
> index 9782364..bd5404e 100644
> --- a/drivers/gpu/drm/qxl/qxl_cmd.c
> +++ b/drivers/gpu/drm/qxl/qxl_cmd.c
> @@ -317,14 +317,11 @@ static void wait_for_io_cmd(struct qxl_device *qdev, 
> uint8_t val, long port)
>  {
>   int ret;
>  
> -restart:
>   ret = wait_for_io_cmd_user(qdev, val, port, false);
> - if (ret == -ERESTARTSYS)
> - goto restart;

I think this one is not directly related to the fix, but can be removed
because wait_for_io_cmd_user(qdev, val, port, false); will call
wait_event_timeout() which cannot return ERESTARTSYS? Or was this loop
causing issues too?

>  }
>  
>  int qxl_io_update_area(struct qxl_device *qdev, struct qxl_bo *surf,
> - const struct qxl_rect *area)
> + const struct qxl_rect *area, bool intr)
>  {
>   int surface_id;
>   uint32_t surface_width, surface_height;
> @@ -350,7 +347,7 @@ int qxl_io_update_area(struct qxl_device *qdev, struct 
> qxl_bo *surf,
>   mutex_lock(>update_area_mutex);
>   qdev->ram_header->update_area = *area;
>   qdev->ram_header->update_surface = surface_id;
> - ret = wait_for_io_cmd_user(qdev, 0, QXL_IO_UPDATE_AREA_ASYNC, true);
> + ret = wait_for_io_cmd_user(qdev, 0, QXL_IO_UPDATE_AREA_ASYNC, intr);
>   mutex_unlock(>update_area_mutex);
>   return ret;
>  }
> @@ -588,10 +585,7 @@ int qxl_update_surface(struct qxl_device *qdev, struct 
> qxl_bo *surf)
>   rect.right = surf->surf.width;
>   rect.top = 0;
>   rect.bottom = surf->surf.height;
> -retry:
> - ret = qxl_io_update_area(qdev, surf, );
> - if (ret == -ERESTARTSYS)
> - goto retry;
> + ret = qxl_io_update_area(qdev, surf, , false);

My understanding is that the fix is this hunk? If so, this could be made
more obvious with an intermediate commit adding the 'bool intr' arg to
qxl_io_update_area and only calling it with 'true' in the appropriate
places.
This code path is only triggered from qxl_surface_evict() which I assume
is not necessarily easily interruptible, so this change makes sense to
me. However it would be much better to get a review from Dave Airlie ;)

Christophe
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: