[drm/qxl 0/6] Various cleanups/fixes

[drm/qxl 0/6] Various cleanups/fixes

[Christophe Fergeau]

Hey,

This patch series starts by doing some various small cleanups (patch 1 to 4), and
then there are 2 fixes for issues which were noticed in fedora 25, the more annoying one
being the one fixed by patch 5 where the VM screen would constantly flicker
when wayland is used. Patch 6 is very hacky, I'm not familiar at all with that code,
so I don't know what is the correct way of doing it.

Christophe

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[drm/qxl 1/6] qxl: Mark some internal functions as static

[Christophe Fergeau]

They are not used outside of their respective source file

Signed-off-by: Christophe Fergeau <cfergeau@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 a61c0d4..156b7de 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) {
@@ -607,7 +607,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 5f3e5ad..da41e1f 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -395,13 +395,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);
@@ -574,6 +572,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
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[drm/qxl 2/6] qxl: Remove unused prototype

[Christophe Fergeau]

qxl_crtc_set_from_monitors_config() is defined in qxl_drv.h but never
implemented.

Signed-off-by: Christophe Fergeau <cfergeau@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 da41e1f..590ba25 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -398,9 +398,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);
-- 
2.9.3

_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[drm/qxl 3/6] qxl: Add missing '\n' to qxl_io_log() call

[Christophe Fergeau]

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>
---
 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 2cd879a..0d16107 100644
--- a/drivers/gpu/drm/qxl/qxl_fb.c
+++ b/drivers/gpu/drm/qxl/qxl_fb.c
@@ -197,7 +197,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;
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[drm/qxl 4/6] qxl: Call qxl_gem_{init,fini}

[Christophe Fergeau]

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);
-- 
2.9.3

_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Christophe Fergeau]

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(-)

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);
-- 
2.9.3

_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[drm/qxl 6/6] qxl: Allow resolution which are not multiple of 8

[Christophe Fergeau]

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>
---

I know this is very hacky, but I have no idea what is important to be set in the mode struct,
if there is a better way to create it without getting the rounding to a multiple of 8, ...


 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 edb90f6..fc5b01e 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -202,6 +202,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);
-- 
2.9.3

_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [drm/qxl 1/6] qxl: Mark some internal functions as static

[Frediano Ziglio]

> 
> 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 a61c0d4..156b7de 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) {
> @@ -607,7 +607,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 5f3e5ad..da41e1f 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.h
> +++ b/drivers/gpu/drm/qxl/qxl_drv.h
> @@ -395,13 +395,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);
> @@ -574,6 +572,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

Frediano
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Spice-devel] [drm/qxl 3/6] qxl: Add missing '\n' to qxl_io_log() call

[Frediano Ziglio]

> 
> 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 2cd879a..0d16107 100644
> --- a/drivers/gpu/drm/qxl/qxl_fb.c
> +++ b/drivers/gpu/drm/qxl/qxl_fb.c
> @@ -197,7 +197,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;

Frediano
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [drm/qxl 2/6] qxl: Remove unused prototype

[Frediano Ziglio]

> 
> 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 da41e1f..590ba25 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.h
> +++ b/drivers/gpu/drm/qxl/qxl_drv.h
> @@ -398,9 +398,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);

Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [drm/qxl 6/6] qxl: Allow resolution which are not multiple of 8

[Frediano Ziglio]

> 
> 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>
> ---
> 
> I know this is very hacky, but I have no idea what is important to be set in
> the mode struct,
> if there is a better way to create it without getting the rounding to a
> multiple of 8, ...
> 
> 
>  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 edb90f6..fc5b01e 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -202,6 +202,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);

If I remember I was discussing with Pavel some time ago about the implementation
of this.
And if I remember he tested some code.
Pavel, do you remember something about?

Frediano
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Spice-devel] [drm/qxl 4/6] qxl: Call qxl_gem_{init,fini}

[Frediano Ziglio]

> 
> qdev->gem.objects was initialized directly in qxl_device_init() rather
> than going through qxl_gem_init(), and qxl_gem_fini() was never called.
> 

Considering "qxl_gem_fini() was never called" did we have a leak?

> 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);
>  

Here qxl_gem_init returns a value that is always ignored, perhaps
would be better to return void from qxl_gem_init if it cannot
fails.

>  	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);

Frediano
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [drm/qxl 6/6] qxl: Allow resolution which are not multiple of 8

[Pavel Grunt]

On Mon, 2016-10-31 at 07:35 -0400, Frediano Ziglio 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.
> > 
> > Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
> > ---
> > 
> > I know this is very hacky, but I have no idea what is important to
> > be set in
> > the mode struct,
> > if there is a better way to create it without getting the rounding
> > to a
> > multiple of 8, ...
> > 
> > 
> >  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 edb90f6..fc5b01e 100644
> > --- a/drivers/gpu/drm/qxl/qxl_display.c
> > +++ b/drivers/gpu/drm/qxl/qxl_display.c
> > @@ -202,6 +202,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);
> 
> If I remember I was discussing with Pavel some time ago about the
> implementation
> of this.
> And if I remember he tested some code.
> Pavel, do you remember something about?

I found out that there is a function fixup_mode_1366x768() in
drivers/gpu/drm/drm_edid.c setting the mode->hdisplay and also
adjusting the mode->hsync

This patch is doing something similar. It just ignores the sync part.

Pavel


_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Frediano Ziglio]

> 
> 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

Re: [Spice-devel] [drm/qxl 4/6] qxl: Call qxl_gem_{init,fini}

[Christophe Fergeau]

[-- Attachment #1.1: Type: text/plain, Size: 1392 bytes --]

On Mon, Oct 31, 2016 at 07:42:35AM -0400, Frediano Ziglio wrote:
> > 
> > qdev->gem.objects was initialized directly in qxl_device_init() rather
> > than going through qxl_gem_init(), and qxl_gem_fini() was never called.
> > 
> 
> Considering "qxl_gem_fini() was never called" did we have a leak?

Maybe, as this forces the freeing of some pending bo's if they are still in
use. This would only happen when unloading/reloading the module
repeatedly, which I don't expect to happen on production systems.

> 
> > 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);
> >  
> 
> Here qxl_gem_init returns a value that is always ignored, perhaps
> would be better to return void from qxl_gem_init if it cannot
> fails.

Good suggestion, I'll add that to a separate patch.

Christophe

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Christophe Fergeau]

[-- Attachment #1.1: Type: text/plain, Size: 4216 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Christophe Fergeau]

[-- Attachment #1.1: Type: text/plain, Size: 613 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 166 bytes --]

_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Emil Velikov]

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

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

[Christophe Fergeau]

[-- Attachment #1.1: Type: text/plain, Size: 1581 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel