| Message ID | 20161107080103.22299-7-cfergeau@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> > 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> > --- > 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 *)&qdev->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) > &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 +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. > + return; > } > > drm_modeset_lock_all(dev); Frediano
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
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 *)&qdev->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) &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 +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"); + return; } drm_modeset_lock_all(dev);
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> --- drivers/gpu/drm/qxl/qxl_display.c | 62 ++++++++++++++++++++++++++++++++------- 1 file changed, 52 insertions(+), 10 deletions(-)