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
They are not used outside of their respective source file
Signed-off-by: Christophe Fergeau cfergeau@redhat.com Acked-by: Frediano Ziglio fziglio@redhat.com --- 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
qxl_crtc_set_from_monitors_config() is defined in qxl_drv.h but never implemented.
Signed-off-by: Christophe Fergeau cfergeau@redhat.com Acked-by: Frediano Ziglio fziglio@redhat.com --- 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);
The message has to be terminated by a newline as it's not going to get added automatically.
Signed-off-by: Christophe Fergeau cfergeau@redhat.com Acked-by: Frediano Ziglio fziglio@redhat.com --- 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;
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 cfergeau@redhat.com --- 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(&qdev->update_area_mutex); mutex_init(&qdev->release_mutex); mutex_init(&qdev->surf_evict_mutex); - INIT_LIST_HEAD(&qdev->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);
It's always returning 0, and it's always ignored.
Signed-off-by: Christophe Fergeau cfergeau@redhat.com --- 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(&qdev->gem.objects); - return 0; }
void qxl_gem_fini(struct qxl_device *qdev)
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 cfergeau@redhat.com --- 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 *)&qdev->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) &qdev->rom->client_monitors_config.heads[i]; struct qxl_head *client_head = &qdev->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->id = i; + status = MONITORS_CONFIG_MODIFIED; + } + if (client_head->flags != 0) { + client_head->flags = 0; + status = MONITORS_CONFIG_MODIFIED; + } DRM_DEBUG_KMS("read %dx%d+%d+%d\n", client_head->width, client_head->height, client_head->x, client_head->y); } - return 0; + + return status; }
static void qxl_update_offset_props(struct qxl_device *qdev) @@ -124,9 +158,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)) { + enum MonitorsConfigCopyStatus 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"); + return; }
drm_modeset_lock_all(dev);
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 cfergeau@redhat.com --- 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);
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 ...
cheers, Gerd
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
On Do, 2016-11-03 at 12:41 +0100, Christophe Fergeau wrote:
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.
No worries here, we are in the virtual world, it for sure wouldn't break monitors ;)
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.
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?
cheers, Gerd
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
On 4 November 2016 at 20:41, Christophe Fergeau cfergeau@redhat.com wrote:
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.
I think we should try it an see, if we see problems you could enforce the framebuffer would have a stride aligned to whatever value, and just the view into the framebuffer could be whatever.
The CVT stuff is just due to how hw is programmed and monitors are described.
Dave.
dri-devel@lists.freedesktop.org
-
Christophe Fergeau -
Dave Airlie -
Gerd Hoffmann