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

[Frediano Ziglio <fziglio@redhat.com>]

> 
> 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
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel