| Message ID | 20161028121227.16904-6-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> 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@redhat.com> (feel free to take into account some or none of them). Frediano
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 *)&qdev->rom->client_monitors_config, > > @@ -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? 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
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
Hi guys, On 2 November 2016 at 16:31, Christophe Fergeau <cfergeau@redhat.com> 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 :-) Thanks Emil
On Thu, Nov 03, 2016 at 06:42:38PM +0000, Emil Velikov wrote: > Hi guys, > > On 2 November 2016 at 16:31, Christophe Fergeau <cfergeau@redhat.com> 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
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 *)&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; + } + if (client_head->flags != 0) { + client_head->flags = 0; + changed = true; + } 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; + } } 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); + } + 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 | 65 +++++++++++++++++++++++++++++++++------ 1 file changed, 55 insertions(+), 10 deletions(-)