> > 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@xxxxxxxxxx> Great message! > --- > 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, > +}; > + 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. > static int qxl_display_copy_rom_client_monitors_config(struct qxl_device > *qdev) why not returning MonitorsConfigCopyStatus ? > { > 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 *)&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)) { > + 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) > &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; > + 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? > + if (client_head->flags != 0) { > + client_head->flags = 0; > + changed = true; > + } why testing flags change if is always 0 ? > DRM_DEBUG_KMS("read %dx%d+%d+%d\n", client_head->width, > client_head->height, > client_head->x, client_head->y); > } > - return 0; > + if (changed) { > + return MONITORS_CONFIG_COPIED; > + } else { > + return MONITORS_CONFIG_UNCHANGED; > + } Here it would be just "return res;". > } > > static void qxl_update_offset_props(struct qxl_device *qdev) > @@ -124,9 +160,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); > + } 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) > + 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); Beside all that style paranoia/opinions: Acked-by: Frediano Ziglio <fziglio@xxxxxxxxxx> (feel free to take into account some or none of them). Frediano _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
