date:20150527

[Spice-devel] [PATCH 04/11] Avoid double free on error

2015-05-27 Thread Frediano Ziglio

Is we are not able to get source bo object from handle we free
destination bo object and call cleanup code however destination
object was already inserted in reloc_info array (num_relocs was
already incremented) so on cleanup we free destination again.

Signed-off-by: Frediano Ziglio fzig...@redhat.com
---
 qxl/qxl_ioctl.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/qxl/qxl_ioctl.c b/qxl/qxl_ioctl.c
index 230ab84..85b3808 100644
--- a/qxl/qxl_ioctl.c
+++ b/qxl/qxl_ioctl.c
@@ -240,8 +240,6 @@ static int qxl_process_single_command(struct qxl_device 
*qdev,
qxlhw_handle_to_bo(qdev, file_priv,
   reloc.src_handle, release);
if (!reloc_info[i].src_bo) {
-   if (reloc_info[i].dst_bo != cmd_bo)
-   
drm_gem_object_unreference_unlocked(reloc_info[i].dst_bo-gem_base);
ret = -EINVAL;
goto out_free_bos;
}
-- 
2.1.0

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH 05/11] Handle all errors in qxl_surface_evict

2015-05-27 Thread Frediano Ziglio

Only EBUSY error was handled. This could cause code to believe
reserve was successful while it failed.

Signed-off-by: Frediano Ziglio fzig...@redhat.com
---
 qxl/qxl_cmd.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/qxl/qxl_cmd.c b/qxl/qxl_cmd.c
index 85ed5dc..b18f84c 100644
--- a/qxl/qxl_cmd.c
+++ b/qxl/qxl_cmd.c
@@ -612,8 +612,8 @@ static int qxl_reap_surf(struct qxl_device *qdev, struct 
qxl_bo *surf, bool stal
int ret;
 
ret = qxl_bo_reserve(surf, false);
-   if (ret == -EBUSY)
-   return -EBUSY;
+   if (ret)
+   return ret;
 
if (stall)
mutex_unlock(qdev-surf_evict_mutex);
@@ -622,9 +622,9 @@ static int qxl_reap_surf(struct qxl_device *qdev, struct 
qxl_bo *surf, bool stal
 
if (stall)
mutex_lock(qdev-surf_evict_mutex);
-   if (ret == -EBUSY) {
+   if (ret) {
qxl_bo_unreserve(surf);
-   return -EBUSY;
+   return ret;
}
 
qxl_surface_evict_locked(qdev, surf, true);
-- 
2.1.0

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH 09/11] Move main reference counter to GEM object instead of TTM ones

2015-05-27 Thread Frediano Ziglio

qxl_bo structure has two reference counters, one in the GEM object and
another in the TTM object. The GEM object keep a counter to the TTM object
so when GEM counter reached zero the TTM counter (using qxl_bo_unref) was
decremented. The qxl object is fully freed (both GEM and TTM part are cleaned)
when the TTM counter reach zero.
One issue was that surface idr structure has no owning on qxl_bo objects however
it contains a pointer to qxl_bo object. This caused some nasty race condition
for instance qxl_bo object was reaped even after counter was already zero.
This patch fix these races moving main counter (the one used by qxl_bo_(un)ref)
to GEM object which cleanup routine (qxl_gem_object_free) remove the idr pointer
(using qxl_surface_evict) when the counters are still valid.

Signed-off-by: Frediano Ziglio fzig...@redhat.com
---
 qxl/qxl_gem.c| 10 --
 qxl/qxl_object.c | 11 ---
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/qxl/qxl_gem.c b/qxl/qxl_gem.c
index b96f0c9..d9746e9 100644
--- a/qxl/qxl_gem.c
+++ b/qxl/qxl_gem.c
@@ -31,9 +31,15 @@
 void qxl_gem_object_free(struct drm_gem_object *gobj)
 {
struct qxl_bo *qobj = gem_to_qxl_bo(gobj);
+   struct qxl_device *qdev;
+   struct ttm_buffer_object *tbo;
 
-   if (qobj)
-   qxl_bo_unref(qobj);
+   qdev = (struct qxl_device *)gobj-dev-dev_private;
+
+   qxl_surface_evict(qdev, qobj, false);
+
+   tbo = qobj-tbo;
+   ttm_bo_unref(tbo);
 }
 
 int qxl_gem_object_create(struct qxl_device *qdev, int size,
diff --git a/qxl/qxl_object.c b/qxl/qxl_object.c
index cdeaf08..6d6f33d 100644
--- a/qxl/qxl_object.c
+++ b/qxl/qxl_object.c
@@ -208,19 +208,16 @@ void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev,
 
 void qxl_bo_unref(struct qxl_bo **bo)
 {
-   struct ttm_buffer_object *tbo;
-
if ((*bo) == NULL)
return;
-   tbo = ((*bo)-tbo);
-   ttm_bo_unref(tbo);
-   if (tbo == NULL)
-   *bo = NULL;
+
+   drm_gem_object_unreference_unlocked((*bo)-gem_base);
+   *bo = NULL;
 }
 
 struct qxl_bo *qxl_bo_ref(struct qxl_bo *bo)
 {
-   ttm_bo_reference(bo-tbo);
+   drm_gem_object_reference(bo-gem_base);
return bo;
 }
 
-- 
2.1.0

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH 06/11] Fix return for qxl_release_alloc

2015-05-27 Thread Frediano Ziglio

This function return handle to allocated release object which is an int.

Signed-off-by: Frediano Ziglio fzig...@redhat.com
---
 qxl/qxl_release.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qxl/qxl_release.c b/qxl/qxl_release.c
index d9b2568..6fd8e50 100644
--- a/qxl/qxl_release.c
+++ b/qxl/qxl_release.c
@@ -122,7 +122,7 @@ static const struct fence_ops qxl_fence_ops = {
.wait = qxl_fence_wait,
 };
 
-static uint64_t
+static int
 qxl_release_alloc(struct qxl_device *qdev, int type,
  struct qxl_release **ret)
 {
-- 
2.1.0

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH 11/11] Propagate correctly errors from qxlhw_handle_to_bo

2015-05-27 Thread Frediano Ziglio

This function could return a NULL pointer in case of handle not
present and in case of out of memory conditions however caller
function always returned EINVAL error hiding a possible ENOMEM.
This patch change the function to return the error instead to
be able to propagate the error instead of assuming EINVAL.

Signed-off-by: Frediano Ziglio fzig...@redhat.com
---
 qxl/qxl_ioctl.c | 33 ++---
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/qxl/qxl_ioctl.c b/qxl/qxl_ioctl.c
index bb326ff..37f1faf 100644
--- a/qxl/qxl_ioctl.c
+++ b/qxl/qxl_ioctl.c
@@ -107,9 +107,9 @@ apply_surf_reloc(struct qxl_device *qdev, struct 
qxl_reloc_info *info)
 }
 
 /* return holding the reference to this object */
-static struct qxl_bo *qxlhw_handle_to_bo(struct qxl_device *qdev,
-struct drm_file *file_priv, uint64_t 
handle,
-struct qxl_release *release)
+static int qxlhw_handle_to_bo(struct qxl_device *qdev,
+ struct drm_file *file_priv, uint64_t handle,
+ struct qxl_release *release, struct qxl_bo 
**qbo_p)
 {
struct drm_gem_object *gobj;
struct qxl_bo *qobj;
@@ -117,16 +117,17 @@ static struct qxl_bo *qxlhw_handle_to_bo(struct 
qxl_device *qdev,
 
gobj = drm_gem_object_lookup(qdev-ddev, file_priv, handle);
if (!gobj)
-   return NULL;
+   return -EINVAL;
 
qobj = gem_to_qxl_bo(gobj);
 
ret = qxl_release_list_add(release, qobj);
drm_gem_object_unreference_unlocked(gobj);
if (ret)
-   return NULL;
+   return ret;
 
-   return qobj;
+   *qbo_p = qobj;
+   return 0;
 }
 
 /*
@@ -219,13 +220,10 @@ static int qxl_process_single_command(struct qxl_device 
*qdev,
reloc_info[i].type = reloc.reloc_type;
 
if (reloc.dst_handle) {
-   reloc_info[i].dst_bo = qxlhw_handle_to_bo(qdev, 
file_priv,
- 
reloc.dst_handle, release);
-   if (!reloc_info[i].dst_bo) {
-   ret = -EINVAL;
-   reloc_info[i].src_bo = NULL;
+   ret = qxlhw_handle_to_bo(qdev, file_priv, 
reloc.dst_handle, release,
+reloc_info[i].dst_bo);
+   if (ret)
goto out_free_bos;
-   }
reloc_info[i].dst_offset = reloc.dst_offset;
} else {
reloc_info[i].dst_bo = cmd_bo;
@@ -234,14 +232,11 @@ static int qxl_process_single_command(struct qxl_device 
*qdev,
num_relocs++;
 
/* reserve and validate the reloc dst bo */
-   if (reloc.reloc_type == QXL_RELOC_TYPE_BO || reloc.src_handle  
0) {
-   reloc_info[i].src_bo =
-   qxlhw_handle_to_bo(qdev, file_priv,
-  reloc.src_handle, release);
-   if (!reloc_info[i].src_bo) {
-   ret = -EINVAL;
+   if (reloc.reloc_type == QXL_RELOC_TYPE_BO || reloc.src_handle) {
+   ret = qxlhw_handle_to_bo(qdev, file_priv, 
reloc.src_handle, release,
+reloc_info[i].src_bo);
+   if (ret)
goto out_free_bos;
-   }
reloc_info[i].src_offset = reloc.src_offset;
} else {
reloc_info[i].src_bo = NULL;
-- 
2.1.0

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH 03/11] Fix print statement not using uninitialized variable

2015-05-27 Thread Frediano Ziglio

reloc_info[i] is not still initialized in the print statement.

Signed-off-by: Frediano Ziglio fzig...@redhat.com
---
 qxl/qxl_ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qxl/qxl_ioctl.c b/qxl/qxl_ioctl.c
index e8b5207..230ab84 100644
--- a/qxl/qxl_ioctl.c
+++ b/qxl/qxl_ioctl.c
@@ -212,7 +212,7 @@ static int qxl_process_single_command(struct qxl_device 
*qdev,
/* add the bos to the list of bos to validate -
   need to validate first then process relocs? */
if (reloc.reloc_type != QXL_RELOC_TYPE_BO  reloc.reloc_type 
!= QXL_RELOC_TYPE_SURF) {
-   DRM_DEBUG(unknown reloc type %d\n, 
reloc_info[i].type);
+   DRM_DEBUG(unknown reloc type %d\n, reloc.reloc_type);
 
ret = -EINVAL;
goto out_free_bos;
-- 
2.1.0

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH 10/11] Simplify cleaning qxl processing command

2015-05-27 Thread Frediano Ziglio

In qxlhw_handle_to_bo we incremented counters twice, one time for release object
and one for reloc_info.
In the main function however reloc_info references was drop much earlier than
release so keeping the pointer only on release is safe and make cleaning
process easier.

Signed-off-by: Frediano Ziglio fzig...@redhat.com
---
 qxl/qxl_ioctl.c | 13 +++--
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/qxl/qxl_ioctl.c b/qxl/qxl_ioctl.c
index 85b3808..bb326ff 100644
--- a/qxl/qxl_ioctl.c
+++ b/qxl/qxl_ioctl.c
@@ -122,10 +122,9 @@ static struct qxl_bo *qxlhw_handle_to_bo(struct qxl_device 
*qdev,
qobj = gem_to_qxl_bo(gobj);
 
ret = qxl_release_list_add(release, qobj);
-   if (ret) {
-   drm_gem_object_unreference_unlocked(gobj);
+   drm_gem_object_unreference_unlocked(gobj);
+   if (ret)
return NULL;
-   }
 
return qobj;
 }
@@ -145,7 +144,7 @@ static int qxl_process_single_command(struct qxl_device 
*qdev,
struct qxl_release *release;
struct qxl_bo *cmd_bo;
void *fb_cmd;
-   int i, j, ret, num_relocs;
+   int i, ret, num_relocs;
int unwritten;
 
switch (cmd-type) {
@@ -269,12 +268,6 @@ static int qxl_process_single_command(struct qxl_device 
*qdev,
qxl_release_fence_buffer_objects(release);
 
 out_free_bos:
-   for (j = 0; j  num_relocs; j++) {
-   if (reloc_info[j].dst_bo != cmd_bo)
-   
drm_gem_object_unreference_unlocked(reloc_info[j].dst_bo-gem_base);
-   if (reloc_info[j].src_bo  reloc_info[j].src_bo != cmd_bo)
-   
drm_gem_object_unreference_unlocked(reloc_info[j].src_bo-gem_base);
-   }
 out_free_release:
if (ret)
qxl_release_free(qdev, release);
-- 
2.1.0

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH 07/11] Handle correctly failures in qxl_alloc_relase_reserved

2015-05-27 Thread Frediano Ziglio

Free resources correctly if function fails

Signed-off-by: Frediano Ziglio fzig...@redhat.com
---
 qxl/qxl_release.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/qxl/qxl_release.c b/qxl/qxl_release.c
index 6fd8e50..00604ed 100644
--- a/qxl/qxl_release.c
+++ b/qxl/qxl_release.c
@@ -363,6 +363,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, 
unsigned long size,
ret = qxl_release_bo_alloc(qdev, 
qdev-current_release_bo[cur_idx]);
if (ret) {
mutex_unlock(qdev-release_mutex);
+   qxl_release_free(qdev, *release);
return ret;
}
}
@@ -377,13 +378,17 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, 
unsigned long size,
 
mutex_unlock(qdev-release_mutex);
 
-   qxl_release_list_add(*release, bo);
+   ret = qxl_release_list_add(*release, bo);
+   qxl_bo_unref(bo);
+   if (ret) {
+   qxl_release_free(qdev, *release);
+   return ret;
+   }
 
info = qxl_release_map(qdev, *release);
info-id = idr_ret;
qxl_release_unmap(qdev, *release, info);
 
-   qxl_bo_unref(bo);
return ret;
 }
 
-- 
2.1.0

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH 01/11] Do not cause spice-server to clean our objects

2015-05-27 Thread Frediano Ziglio

If objects are moved back from system memory to VRAM (and spice id
created again) memory is already initialized so we need to set flag
to not clear memory.
If you don't do it after a while using desktop many images turns to
black or transparents.

Signed-off-by: Frediano Ziglio fzig...@redhat.com
---
 qxl/qxl_cmd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/qxl/qxl_cmd.c b/qxl/qxl_cmd.c
index bd5404e..85ed5dc 100644
--- a/qxl/qxl_cmd.c
+++ b/qxl/qxl_cmd.c
@@ -502,6 +502,7 @@ int qxl_hw_surface_alloc(struct qxl_device *qdev,
 
cmd = (struct qxl_surface_cmd *)qxl_release_map(qdev, release);
cmd-type = QXL_SURFACE_CMD_CREATE;
+   cmd-flags = QXL_SURF_FLAG_KEEP_DATA;
cmd-u.surface_create.format = surf-surf.format;
cmd-u.surface_create.width = surf-surf.width;
cmd-u.surface_create.height = surf-surf.height;
-- 
2.1.0

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH 00/11] Miscellaneous stability patches

2015-05-27 Thread Frediano Ziglio

This set of patches mainly contains fix for some memory issues
using quite aggressively surfaces and other minor problems like
images going black after a while.

Frediano Ziglio (11):
  Do not cause spice-server to clean our objects
  Do not leak memory if qxl_release_list_add fails
  Fix print statement not using uninitialized variable
  Avoid double free on error
  Handle all errors in qxl_surface_evict
  Fix return for qxl_release_alloc
  Handle correctly failures in qxl_alloc_relase_reserved
  Remove format string errors
  Move main reference counter to GEM object instead of TTM ones
  Simplify cleaning qxl processing command
  Propagate correctly errors from qxlhw_handle_to_bo

 qxl/qxl_cmd.c | 11 ++-
 qxl/qxl_display.c |  2 +-
 qxl/qxl_drv.h |  2 +-
 qxl/qxl_gem.c | 10 --
 qxl/qxl_ioctl.c   | 46 +-
 qxl/qxl_object.c  | 11 ---
 qxl/qxl_release.c | 13 +
 7 files changed, 46 insertions(+), 49 deletions(-)

-- 
2.1.0

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH 02/11] Do not leak memory if qxl_release_list_add fails

2015-05-27 Thread Frediano Ziglio

If the function fails reference counter to the object is not decremented
causing leaks.
This is hard to spot as it happens only on very low memory situations.

Signed-off-by: Frediano Ziglio fzig...@redhat.com
---
 qxl/qxl_ioctl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/qxl/qxl_ioctl.c b/qxl/qxl_ioctl.c
index afd7297..e8b5207 100644
--- a/qxl/qxl_ioctl.c
+++ b/qxl/qxl_ioctl.c
@@ -122,8 +122,10 @@ static struct qxl_bo *qxlhw_handle_to_bo(struct qxl_device 
*qdev,
qobj = gem_to_qxl_bo(gobj);
 
ret = qxl_release_list_add(release, qobj);
-   if (ret)
+   if (ret) {
+   drm_gem_object_unreference_unlocked(gobj);
return NULL;
+   }
 
return qobj;
 }
-- 
2.1.0

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [vdagent-linux] Prepare for 0.16.0 release

2015-05-27 Thread Christophe Fergeau

---
Hey,

As suggested by poma, it might be a good time to have a 0.16.0 release of
spice-vdagent. Let me know what you think :)

Christophe


 Makefile.am  |  1 +
 NEWS | 10 ++
 configure.ac |  2 +-
 3 files changed, 12 insertions(+), 1 deletion(-)
 create mode 100644 NEWS

diff --git a/Makefile.am b/Makefile.am
index cf5d0f5..8c55b43 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -80,6 +80,7 @@ manpage_DATA = data/spice-vdagent.1   \
data/spice-vdagentd.1
 
 EXTRA_DIST =   \
+   NEWS\
README.RHEL-5   \
data/70-spice-vdagentd.rules\
data/spice-vdagent.desktop  \
diff --git a/NEWS b/NEWS
new file mode 100644
index 000..a437978
--- /dev/null
+++ b/NEWS
@@ -0,0 +1,10 @@
+Overview of changes in SPICE vdagent 0.16.0
+===
+
+- Add audio volume synchronization support
+- Add support for maximum clipboard size
+- Add support for more clipboard targets (STRING and TIMESTAMP)
+- Improve handling of transfers of multiple files
+- Fix transfer of 2GB files on 32 bit systems
+- XSpice improvements
+- Various bug fixes related to resolution changes
diff --git a/configure.ac b/configure.ac
index 37ae160..f559c2e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1,5 +1,5 @@
 AC_PREREQ(2.59)
-AC_INIT([spice-vdagent], [0.15.0])
+AC_INIT([spice-vdagent], [0.16.0])
 AC_CONFIG_SRCDIR([configure.ac])
 
 AM_CONFIG_HEADER([src/config.h])
-- 
2.4.1

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH] Prepare for spice-protocol 0.12.8 release

2015-05-27 Thread Christophe Fergeau

---
This is needed if we are to do a spice-vdagent release. There are a few pending
patches on the mailing list doing more additions to that file, but we can aim
at making a 0.12.9 release soon after they are merged.

Christophe


 NEWS | 5 +
 1 file changed, 5 insertions(+)

diff --git a/NEWS b/NEWS
index 8fcc327..ba0014d 100644
--- a/NEWS
+++ b/NEWS
@@ -1,3 +1,8 @@
+Major changes in 0.12.8
+===
+* add LZ4 support
+* add audio volume synchronization
+
 Major changes in 0.12.7
 ===
 * add support for Webdav channel
-- 
2.4.1

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 00/11] Miscellaneous stability patches

2015-05-27 Thread Josh Boyer

On Wed, May 27, 2015 at 8:47 AM, Josh Boyer jwbo...@fedoraproject.org wrote:
 On Wed, May 27, 2015 at 6:03 AM, Frediano Ziglio fzig...@redhat.com wrote:
 This set of patches mainly contains fix for some memory issues
 using quite aggressively surfaces and other minor problems like
 images going black after a while.

 Frediano Ziglio (11):
   Do not cause spice-server to clean our objects
   Do not leak memory if qxl_release_list_add fails
   Fix print statement not using uninitialized variable
   Avoid double free on error
   Handle all errors in qxl_surface_evict
   Fix return for qxl_release_alloc
   Handle correctly failures in qxl_alloc_relase_reserved
   Remove format string errors
   Move main reference counter to GEM object instead of TTM ones
   Simplify cleaning qxl processing command
   Propagate correctly errors from qxlhw_handle_to_bo

  qxl/qxl_cmd.c | 11 ++-
  qxl/qxl_display.c |  2 +-
  qxl/qxl_drv.h |  2 +-
  qxl/qxl_gem.c | 10 --
  qxl/qxl_ioctl.c   | 46 +-
  qxl/qxl_object.c  | 11 ---
  qxl/qxl_release.c | 13 +
  7 files changed, 46 insertions(+), 49 deletions(-)

 The strip level on these patches is rather odd.  Normally one would
 see a strip level of 1 at the top of the kernel dir.  E.g.

 drivers/gpu/drm/qxl/qxl_gem.c

 in the diffstat, etc.

(Sorry for the double reply.)

Also, are any of these commits something that should be queued for
stable kernel releases?  There are a handful that look like they
should be to me.

josh
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 00/11] Miscellaneous stability patches

2015-05-27 Thread Josh Boyer

On Wed, May 27, 2015 at 6:03 AM, Frediano Ziglio fzig...@redhat.com wrote:
 This set of patches mainly contains fix for some memory issues
 using quite aggressively surfaces and other minor problems like
 images going black after a while.

 Frediano Ziglio (11):
   Do not cause spice-server to clean our objects
   Do not leak memory if qxl_release_list_add fails
   Fix print statement not using uninitialized variable
   Avoid double free on error
   Handle all errors in qxl_surface_evict
   Fix return for qxl_release_alloc
   Handle correctly failures in qxl_alloc_relase_reserved
   Remove format string errors
   Move main reference counter to GEM object instead of TTM ones
   Simplify cleaning qxl processing command
   Propagate correctly errors from qxlhw_handle_to_bo

  qxl/qxl_cmd.c | 11 ++-
  qxl/qxl_display.c |  2 +-
  qxl/qxl_drv.h |  2 +-
  qxl/qxl_gem.c | 10 --
  qxl/qxl_ioctl.c   | 46 +-
  qxl/qxl_object.c  | 11 ---
  qxl/qxl_release.c | 13 +
  7 files changed, 46 insertions(+), 49 deletions(-)

The strip level on these patches is rather odd.  Normally one would
see a strip level of 1 at the top of the kernel dir.  E.g.

drivers/gpu/drm/qxl/qxl_gem.c

in the diffstat, etc.

josh
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 00/11] Miscellaneous stability patches

2015-05-27 Thread Frediano Ziglio

 
 On Wed, May 27, 2015 at 8:47 AM, Josh Boyer jwbo...@fedoraproject.org
 wrote:
  On Wed, May 27, 2015 at 6:03 AM, Frediano Ziglio fzig...@redhat.com
  wrote:
  This set of patches mainly contains fix for some memory issues
  using quite aggressively surfaces and other minor problems like
  images going black after a while.
 
  Frediano Ziglio (11):
Do not cause spice-server to clean our objects
Do not leak memory if qxl_release_list_add fails
Fix print statement not using uninitialized variable
Avoid double free on error
Handle all errors in qxl_surface_evict
Fix return for qxl_release_alloc
Handle correctly failures in qxl_alloc_relase_reserved
Remove format string errors
Move main reference counter to GEM object instead of TTM ones
Simplify cleaning qxl processing command
Propagate correctly errors from qxlhw_handle_to_bo
 
   qxl/qxl_cmd.c | 11 ++-
   qxl/qxl_display.c |  2 +-
   qxl/qxl_drv.h |  2 +-
   qxl/qxl_gem.c | 10 --
   qxl/qxl_ioctl.c   | 46 +-
   qxl/qxl_object.c  | 11 ---
   qxl/qxl_release.c | 13 +
   7 files changed, 46 insertions(+), 49 deletions(-)
 
  The strip level on these patches is rather odd.  Normally one would
  see a strip level of 1 at the top of the kernel dir.  E.g.
 
  drivers/gpu/drm/qxl/qxl_gem.c
 
  in the diffstat, etc.
 
 (Sorry for the double reply.)
 
 Also, are any of these commits something that should be queued for
 stable kernel releases?  There are a handful that look like they
 should be to me.
 
 josh
 

Hi,
  no problem for double reply.

I was using a different repository with only QXL driver. I tested and all 
patches apply and compile perfectly even with Linus master branch.

About which patches should be applied surely (attempting to put a priority)
- Move main reference counter to GEM object instead of TTM ones this can 
causes memory corruption even not wanting to;
- Avoid double free on error this can be cause leaks in kernel if user space 
wants, mitigated by the fact that usually DRM inodes are owned by root;
- Handle all errors in qxl_surface_evict could cause corruption too, not 
really probable but taking into account that Xorg implementation use a lot 
signals is not so impossible;
- Handle correctly failures in qxl_alloc_relase_reserved, Do not leak memory 
if qxl_release_list_add fails just cause leaks on situation where memory is 
already REALLY low, can be omitted;
- Fix print statement not using uninitialized variable, Remove format string 
errors should just print garbage and debugging is disabled by default, not 
necessary.

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

[Spice-devel] [PATCH v2] Report invalid password as a special auth error

2015-05-27 Thread Cédric Bosdonnat

Provide a special authentication error message for too long passwords.
Also check for too long passwords before sending them over the wire.
---

 Diff to v1:
   * Added a check in spice_channel_send_spice_ticket
   * moved spice_channel_failed_authentication before 
spice_channel_send_spice_ticket
 in order to reuse it there.

 gtk/spice-channel.c | 64 +
 gtk/spice-client.h  |  2 ++
 2 files changed, 42 insertions(+), 24 deletions(-)

diff --git a/gtk/spice-channel.c b/gtk/spice-channel.c
index 4e7d8b7..c4a18f5 100644
--- a/gtk/spice-channel.c
+++ b/gtk/spice-channel.c
@@ -1010,6 +1010,33 @@ static int spice_channel_read(SpiceChannel *channel, 
void *data, size_t length)
 }
 
 /* coroutine context */
+static void spice_channel_failed_authentication(SpiceChannel *channel,
+gboolean invalidPassword)
+{
+SpiceChannelPrivate *c = channel-priv;
+
+if (c-auth_needs_username_and_password)
+g_set_error_literal(c-error,
+SPICE_CLIENT_ERROR,
+
SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD_AND_USERNAME,
+_(Authentication failed: password and username 
are required));
+else if (invalidPassword)
+g_set_error_literal(c-error,
+SPICE_CLIENT_ERROR,
+SPICE_CLIENT_ERROR_AUTH_INVALID_PASSWORD,
+_(Authentication failed: password is too long));
+else
+g_set_error_literal(c-error,
+SPICE_CLIENT_ERROR,
+SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD,
+_(Authentication failed: password is required));
+
+c-event = SPICE_CHANNEL_ERROR_AUTH;
+
+c-has_error = TRUE; /* force disconnect */
+}
+
+/* coroutine context */
 static void spice_channel_send_spice_ticket(SpiceChannel *channel)
 {
 SpiceChannelPrivate *c = channel-priv;
@@ -1039,11 +1066,17 @@ static void 
spice_channel_send_spice_ticket(SpiceChannel *channel)
 g_object_get(c-session, password, password, NULL);
 if (password == NULL)
 password = g_strdup();
+if (strlen(password)  SPICE_MAX_PASSWORD_LENGTH) {
+spice_channel_failed_authentication(channel, TRUE);
+goto cleanup;
+}
 rc = RSA_public_encrypt(strlen(password) + 1, (uint8_t*)password,
 encrypted, rsa, RSA_PKCS1_OAEP_PADDING);
 g_warn_if_fail(rc  0);
 
 spice_channel_write(channel, encrypted, nRSASize);
+
+cleanup:
 memset(encrypted, 0, nRSASize);
 EVP_PKEY_free(pubkey);
 BIO_free(bioKey);
@@ -1051,27 +1084,6 @@ static void spice_channel_send_spice_ticket(SpiceChannel 
*channel)
 }
 
 /* coroutine context */
-static void spice_channel_failed_authentication(SpiceChannel *channel)
-{
-SpiceChannelPrivate *c = channel-priv;
-
-if (c-auth_needs_username_and_password)
-g_set_error_literal(c-error,
-SPICE_CLIENT_ERROR,
-
SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD_AND_USERNAME,
-_(Authentication failed: password and username 
are required));
-else
-g_set_error_literal(c-error,
-SPICE_CLIENT_ERROR,
-SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD,
-_(Authentication failed: password is required));
-
-c-event = SPICE_CHANNEL_ERROR_AUTH;
-
-c-has_error = TRUE; /* force disconnect */
-}
-
-/* coroutine context */
 static gboolean spice_channel_recv_auth(SpiceChannel *channel)
 {
 SpiceChannelPrivate *c = channel-priv;
@@ -1086,9 +1098,13 @@ static gboolean spice_channel_recv_auth(SpiceChannel 
*channel)
 return FALSE;
 }
 
-if (link_res != SPICE_LINK_ERR_OK) {
+if (link_res == SPICE_LINK_ERR_INVALID_PASSWORD) {
+CHANNEL_DEBUG(channel, link result: invalid password);
+spice_channel_failed_authentication(channel, TRUE);
+return FALSE;
+} if (link_res != SPICE_LINK_ERR_OK) {
 CHANNEL_DEBUG(channel, link result: reply %d, link_res);
-spice_channel_failed_authentication(channel);
+spice_channel_failed_authentication(channel, FALSE);
 return FALSE;
 }
 
@@ -1662,7 +1678,7 @@ error:
 if (saslconn)
 sasl_dispose(saslconn);
 
-spice_channel_failed_authentication(channel);
+spice_channel_failed_authentication(channel, FALSE);
 ret = FALSE;
 
 cleanup:
diff --git a/gtk/spice-client.h b/gtk/spice-client.h
index c2474d1..58d1f76 100644
--- a/gtk/spice-client.h
+++ b/gtk/spice-client.h
@@ -60,6 +60,7 @@ G_BEGIN_DECLS
  * @SPICE_CLIENT_USB_DEVICE_LOST: usb device disconnected (fatal IO error)
  * @SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD: password is required
  * @SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD_AND_USERNAME: password and username 
are required
+ *

[Spice-devel] [PATCH spice 3/7] server: Refactor the Spice server video encoding so alternative implementations can be added. (take 2)

2015-05-27 Thread Francois Gouget

This patch simply replaces the mjpeg_encoder_xxx() call points with a more 
object oriented design.
---

Changes since take 1:
- I fixed the width/height comments and they now state that they always 
  match src. Unless some reason not to is discovered, I'll submit a 
  separate patch to remove the width/height parameters to avoid 
  confusion.

 server/Makefile.am |   2 +-
 server/mjpeg_encoder.c | 134 +---
 server/mjpeg_encoder.h | 114 --
 server/red_worker.c| 156 ++-
 server/video_encoder.h | 162 +
 5 files changed, 331 insertions(+), 237 deletions(-)
 delete mode 100644 server/mjpeg_encoder.h
 create mode 100644 server/video_encoder.h

diff --git a/server/Makefile.am b/server/Makefile.am
index 89a375c..36b6d45 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -81,7 +81,6 @@ libspice_server_la_SOURCES =  \
main_channel.c  \
main_channel.h  \
mjpeg_encoder.c \
-   mjpeg_encoder.h \
red_bitmap_utils.h  \
red_channel.c   \
red_channel.h   \
@@ -115,6 +114,7 @@ libspice_server_la_SOURCES =\
spicevmc.c  \
spice_timer_queue.c \
spice_timer_queue.h \
+   video_encoder.h \
zlib_encoder.c  \
zlib_encoder.h  \
spice_bitmap_utils.h\
diff --git a/server/mjpeg_encoder.c b/server/mjpeg_encoder.c
index 9a41ef3..d734520 100644
--- a/server/mjpeg_encoder.c
+++ b/server/mjpeg_encoder.c
@@ -20,7 +20,11 @@
 #endif
 
 #include red_common.h
-#include mjpeg_encoder.h
+
+typedef struct MJpegEncoder MJpegEncoder;
+#define VIDEO_ENCODER_T MJpegEncoder
+#include video_encoder.h
+
 #include jerror.h
 #include jpeglib.h
 #include inttypes.h
@@ -154,6 +158,7 @@ typedef struct MJpegEncoderRateControl {
 } MJpegEncoderRateControl;
 
 struct MJpegEncoder {
+VideoEncoder base;
 uint8_t *row;
 uint32_t row_size;
 int first_frame;
@@ -165,7 +170,7 @@ struct MJpegEncoder {
 void (*pixel_converter)(uint8_t *src, uint8_t *dest);
 
 MJpegEncoderRateControl rate_control;
-MJpegEncoderRateControlCbs cbs;
+VideoEncoderRateControlCbs cbs;
 void *cbs_opaque;
 
 /* stats */
@@ -174,6 +179,22 @@ struct MJpegEncoder {
 uint32_t num_frames;
 };
 
+static void mjpeg_encoder_destroy(MJpegEncoder *encoder);
+static int mjpeg_encoder_encode_frame(MJpegEncoder *encoder,
+const SpiceBitmap *bitmap, int width, int height,
+const SpiceRect *src, int top_down, uint32_t frame_mm_time,
+uint8_t **outbuf, size_t *outbuf_size, int *data_size);
+static void mjpeg_encoder_client_stream_report(MJpegEncoder *encoder,
+uint32_t num_frames,
+uint32_t num_drops,
+uint32_t start_frame_mm_time,
+uint32_t end_frame_mm_time,
+int32_t end_frame_delay,
+uint32_t audio_delay);
+static void mjpeg_encoder_notify_server_frame_drop(MJpegEncoder *encoder);
+static uint64_t mjpeg_encoder_get_bit_rate(MJpegEncoder *encoder);
+static void mjpeg_encoder_get_stats(MJpegEncoder *encoder, VideoEncoderStats 
*stats);
+
 static inline void mjpeg_encoder_reset_quality(MJpegEncoder *encoder,
int quality_id,
uint32_t fps,
@@ -189,8 +210,9 @@ static inline int rate_control_is_active(MJpegEncoder* 
encoder)
 return encoder-cbs.get_roundtrip_ms != NULL;
 }
 
-MJpegEncoder *mjpeg_encoder_new(uint64_t starting_bit_rate,
-MJpegEncoderRateControlCbs *cbs, void *opaque)
+MJpegEncoder *create_mjpeg_encoder(uint64_t starting_bit_rate,
+   VideoEncoderRateControlCbs *cbs,
+   void *cbs_opaque)
 {
 MJpegEncoder *enc;
 
@@ -198,6 +220,12 @@ MJpegEncoder *mjpeg_encoder_new(uint64_t starting_bit_rate,
 
 enc = spice_new0(MJpegEncoder, 1);
 
+enc-base.destroy = mjpeg_encoder_destroy;
+enc-base.encode_frame = mjpeg_encoder_encode_frame;
+enc-base.client_stream_report = mjpeg_encoder_client_stream_report;
+enc-base.notify_server_frame_drop = 
mjpeg_encoder_notify_server_frame_drop;
+enc-base.get_bit_rate = mjpeg_encoder_get_bit_rate;
+enc-base.get_stats = mjpeg_encoder_get_stats;
 enc-first_frame = TRUE;

[Spice-devel] [PATCH spice 2/7] server: Remove the rate_control_is_active field from MJpegEncoder. (take 2)

2015-05-27 Thread Francois Gouget

It is redundant with the corresponding callbacks.
---

As far as I can tell this patch was ok in the previous round so no 
change this time around.

 server/mjpeg_encoder.c | 22 +-
 server/mjpeg_encoder.h |  2 +-
 server/red_worker.c|  4 ++--
 3 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/server/mjpeg_encoder.c b/server/mjpeg_encoder.c
index 95d841f..9a41ef3 100644
--- a/server/mjpeg_encoder.c
+++ b/server/mjpeg_encoder.c
@@ -164,7 +164,6 @@ struct MJpegEncoder {
 unsigned int bytes_per_pixel; /* bytes per pixel of the input buffer */
 void (*pixel_converter)(uint8_t *src, uint8_t *dest);
 
-int rate_control_is_active;
 MJpegEncoderRateControl rate_control;
 MJpegEncoderRateControlCbs cbs;
 void *cbs_opaque;
@@ -185,21 +184,25 @@ static uint32_t get_min_required_playback_delay(uint64_t 
frame_enc_size,
 uint64_t byte_rate,
 uint32_t latency);
 
-MJpegEncoder *mjpeg_encoder_new(int bit_rate_control, uint64_t 
starting_bit_rate,
+static inline int rate_control_is_active(MJpegEncoder* encoder)
+{
+return encoder-cbs.get_roundtrip_ms != NULL;
+}
+
+MJpegEncoder *mjpeg_encoder_new(uint64_t starting_bit_rate,
 MJpegEncoderRateControlCbs *cbs, void *opaque)
 {
 MJpegEncoder *enc;
 
-spice_assert(!bit_rate_control || (cbs  cbs-get_roundtrip_ms  
cbs-get_source_fps));
+spice_assert(!cbs || (cbs  cbs-get_roundtrip_ms  
cbs-get_source_fps));
 
 enc = spice_new0(MJpegEncoder, 1);
 
 enc-first_frame = TRUE;
-enc-rate_control_is_active = bit_rate_control;
 enc-rate_control.byte_rate = starting_bit_rate / 8;
 enc-starting_bit_rate = starting_bit_rate;
 
-if (bit_rate_control) {
+if (cbs) {
 struct timespec time;
 
 clock_gettime(CLOCK_MONOTONIC, time);
@@ -211,6 +214,7 @@ MJpegEncoder *mjpeg_encoder_new(int bit_rate_control, 
uint64_t starting_bit_rate
 enc-rate_control.quality_eval_data.reason = 
MJPEG_QUALITY_EVAL_REASON_RATE_CHANGE;
 enc-rate_control.warmup_start_time = ((uint64_t) time.tv_sec) * 
10 + time.tv_nsec;
 } else {
+enc-cbs.get_roundtrip_ms = NULL;
 mjpeg_encoder_reset_quality(enc, MJPEG_LEGACY_STATIC_QUALITY_ID, 
MJPEG_MAX_FPS, 0);
 }
 
@@ -607,7 +611,7 @@ static void 
mjpeg_encoder_adjust_params_to_bit_rate(MJpegEncoder *encoder)
 uint32_t latency = 0;
 uint32_t src_fps;
 
-spice_assert(encoder-rate_control_is_active);
+spice_assert(rate_control_is_active(encoder));
 
 rate_control = encoder-rate_control;
 quality_eval = rate_control-quality_eval_data;
@@ -692,7 +696,7 @@ static void mjpeg_encoder_adjust_fps(MJpegEncoder *encoder, 
uint64_t now)
 MJpegEncoderRateControl *rate_control = encoder-rate_control;
 uint64_t adjusted_fps_time_passed;
 
-spice_assert(encoder-rate_control_is_active);
+spice_assert(rate_control_is_active(encoder));
 
 adjusted_fps_time_passed = (now - rate_control-adjusted_fps_start_time) / 
1000 / 1000;
 
@@ -734,7 +738,7 @@ int mjpeg_encoder_start_frame(MJpegEncoder *encoder, 
SpiceBitmapFmt format,
 {
 uint32_t quality;
 
-if (encoder-rate_control_is_active) {
+if (rate_control_is_active(encoder)) {
 MJpegEncoderRateControl *rate_control = encoder-rate_control;
 struct timespec time;
 uint64_t now;
@@ -1131,7 +1135,7 @@ void mjpeg_encoder_client_stream_report(MJpegEncoder 
*encoder,
 end_frame_mm_time - start_frame_mm_time,
 end_frame_delay, audio_delay);
 
-if (!encoder-rate_control_is_active) {
+if (!rate_control_is_active(encoder)) {
 spice_debug(rate control was not activated: ignoring);
 return;
 }
diff --git a/server/mjpeg_encoder.h b/server/mjpeg_encoder.h
index 741ea1c..d584b92 100644
--- a/server/mjpeg_encoder.h
+++ b/server/mjpeg_encoder.h
@@ -49,7 +49,7 @@ typedef struct MJpegEncoderStats {
 double avg_quality;
 } MJpegEncoderStats;
 
-MJpegEncoder *mjpeg_encoder_new(int bit_rate_control, uint64_t 
starting_bit_rate,
+MJpegEncoder *mjpeg_encoder_new(uint64_t starting_bit_rate,
 MJpegEncoderRateControlCbs *cbs, void *opaque);
 void mjpeg_encoder_destroy(MJpegEncoder *encoder);
 
diff --git a/server/red_worker.c b/server/red_worker.c
index 5deb30b..e0ff8e9 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -3100,9 +3100,9 @@ static void 
red_display_create_stream(DisplayChannelClient *dcc, Stream *stream)
 mjpeg_cbs.update_client_playback_delay = 
red_stream_update_client_playback_latency;
 
 initial_bit_rate = red_stream_get_initial_bit_rate(dcc, stream);
-agent-mjpeg_encoder = mjpeg_encoder_new(TRUE, initial_bit_rate, 
mjpeg_cbs, agent);
+agent-mjpeg_encoder = mjpeg_encoder_new(initial_bit_rate, mjpeg_cbs, 
agent);
 } else {
-

[Spice-devel] [PATCH common 5/7] Add support for the VP8 video codec. (take 2)

2015-05-27 Thread Francois Gouget

---

Thanks to Fabio Fantoni for noticing that spice.proto needed updating 
too. I think I did not notice the issue because the top-level autogen.sh 
keeps rechecking out spice-protocol so spice-protocol/spice/enums.h 
ended up always being newer than spice.proto.

 spice.proto | 1 +
 1 file changed, 1 insertion(+)

diff --git a/spice.proto b/spice.proto
index 01493c9..ecc6308 100644
--- a/spice.proto
+++ b/spice.proto
@@ -329,6 +329,7 @@ flags8 path_flags { /* TODO: C enum names changes */
 
 enum8 video_codec_type {
 MJPEG = 1,
+VP8,
 };
 
 flags8 stream_flags {
-- 
2.1.4
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH spice 7/7] server: Add VP8 support, a video codec preference list and compatibility checks with the Spice client. (take 2)

2015-05-27 Thread Francois Gouget

The video encoder preferences can be expressed by building a semi-colon 
separated list of encoder:codec pairs. For instance 'gstreamer:vp8;spice:mjpeg' 
to pick first the GStreamer VP8 video encoder first and used the original MJPEG 
video encoder one as a fallback.
The server has a default preference list which can also be selected by 
specifying 'auto' as the preferences list.
The client compatibility check relies on the following new capabilities:
 * SPICE_DISPLAY_CAP_MULTI_CODEC which denotes a recent client that supports 
multiple codecs. This capability is needed to not have to hardcode that MJPEG 
is supported. This makes it possible to write clients that don't support MJPEG.
 * SPICE_DISPLAY_CAP_CODEC_XXX, where XXX is a supported codec, for now MJPEG 
and VP8.
---

Changes since take 1:
- Only set the parameters supported by the current encoder.
- reconfigure_pipeline() handling has been fixed in the previous patch.
- The video codecs list now has a more sensible default and it's 
  possible to explicitly request this default by specifying 'auto' in 
  the configuration files.

The buffer timestamping is unchanged because I don't see a better 
solution at this time. 

Note that the xf86-video-qxl, spice-gtk and spice-html5 patches work 
just fine with this new series so I'm not reposting them to limit 
redundant emails. If this series can go in then I can resubmit them. For 
now, anyone who needs them for testing is invited to grab them there:

* [PATCH qxl 7/11] Add SpiceVideoCodecs and $XSPICE_VIDEO_CODECS for specifying 
video codec preferences.
  http://lists.freedesktop.org/archives/spice-devel/2015-May/019770.html

* [PATCH qemu 08/11] Add the ability to specify Spice video codecs.
  http://lists.freedesktop.org/archives/spice-devel/2015-May/019771.html

* [PATCH spice-gtk 9/11] Add the ability to use gstreamer for vp8 decoding.
  http://lists.freedesktop.org/archives/spice-devel/2015-May/019772.html

* [PATCH spice-html5 10/11] Revise the webm files to more correctly identify 
audio tracks.
  http://lists.freedesktop.org/archives/spice-devel/2015-May/019773.html

* [PATCH spice-html5 11/12] Add support for the vp8 codec type.
  (this is really patch 11/11)
  http://lists.freedesktop.org/archives/spice-devel/2015-May/019774.html



 server/gstreamer_encoder.c |  68 ++-
 server/mjpeg_encoder.c |   6 +-
 server/red_dispatcher.c| 165 -
 server/red_dispatcher.h|   8 +++
 server/red_worker.c|  62 ++---
 server/red_worker.h|  18 +
 server/reds.c  |  12 
 server/spice-server.h  |   1 +
 server/spice-server.syms   |   1 +
 server/video_encoder.h |  18 +++--
 10 files changed, 312 insertions(+), 47 deletions(-)

diff --git a/server/gstreamer_encoder.c b/server/gstreamer_encoder.c
index 15ddd82..01b4f5d 100644
--- a/server/gstreamer_encoder.c
+++ b/server/gstreamer_encoder.c
@@ -42,6 +42,7 @@ typedef struct {
 
 struct GstEncoder {
 VideoEncoder base;
+const gchar* gstenc_name;
 
 /* The GStreamer pipeline. If pipeline is NULL then the other pointers are
  * invalid.
@@ -139,11 +140,11 @@ static void adjust_bit_rate(GstEncoder *encoder)
 {
 /* Even MJPEG should achieve good quality with a 10x compression level 
*/
 encoder-bit_rate = raw_bit_rate / 10;
-spice_debug(capping the bit rate to %.2fMbps for a 10x compression 
level, ((double)encoder-bit_rate) / 1024 / 1024);
+spice_debug(capping the %s bit rate to %.2fMbps for a 10x compression 
level, encoder-gstenc_name, ((double)encoder-bit_rate) / 1024 / 1024);
 }
 else
 {
-spice_debug(setting the bit rate to %.2fMbps for a %dx compression 
level, ((double)encoder-bit_rate) / 1024 / 1024, compression);
+spice_debug(setting the %s bit rate to %.2fMbps for a %dx compression 
level, encoder-gstenc_name, ((double)encoder-bit_rate) / 1024 / 1024, 
compression);
 }
 }
 
@@ -177,9 +178,12 @@ static int construct_pipeline(GstEncoder *encoder, const 
SpiceBitmap *bitmap)
 {
 GstStateChangeReturn ret;
 GError *err;
+gchar *desc;
 
 err = NULL;
-encoder-pipeline = gst_parse_launch_full(appsrc name=src is-live=1 ! 
autovideoconvert ! ffenc_mjpeg name=encoder ! appsink name=sink, NULL, 
GST_PARSE_FLAG_FATAL_ERRORS, err);
+desc = g_strdup_printf(appsrc name=src is-live=1 ! autovideoconvert ! %s 
name=encoder ! appsink name=sink, encoder-gstenc_name);
+encoder-pipeline = gst_parse_launch_full(desc, NULL, 
GST_PARSE_FLAG_FATAL_ERRORS, err);
+g_free(desc);
 if (!encoder-pipeline)
 {
 spice_warning(GStreamer error: %s, err-message);
@@ -190,15 +194,21 @@ static int construct_pipeline(GstEncoder *encoder, const 
SpiceBitmap *bitmap)
 encoder-gstenc = gst_bin_get_by_name(GST_BIN(encoder-pipeline), 
encoder);
 encoder-appsink = 
GST_APP_SINK(gst_bin_get_by_name(GST_BIN(encoder-pipeline), sink));
 
+/* Set

[Spice-devel] [PATCH protocol 6/7] Add support for the VP8 video codec and for advertising supported video codecs. (take 2)

2015-05-27 Thread Francois Gouget

Clients that support multiple codecs should advertise the 
SPICE_DISPLAY_CAP_MULTI_CODEC capability and one SPICE_DISPLAY_CAP_CODEC_XXX 
per supported codec.
---

As far as I can tell this patch was ok in the previous round so no 
change this time around.

 spice/enums.h| 1 +
 spice/protocol.h | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/spice/enums.h b/spice/enums.h
index 18e2f74..14675f9 100644
--- a/spice/enums.h
+++ b/spice/enums.h
@@ -139,6 +139,7 @@ typedef enum SpicePathFlags {
 
 typedef enum SpiceVideoCodecType {
 SPICE_VIDEO_CODEC_TYPE_MJPEG = 1,
+SPICE_VIDEO_CODEC_TYPE_VP8,
 
 SPICE_VIDEO_CODEC_TYPE_ENUM_END
 } SpiceVideoCodecType;
diff --git a/spice/protocol.h b/spice/protocol.h
index bea376c..7f0b322 100644
--- a/spice/protocol.h
+++ b/spice/protocol.h
@@ -134,6 +134,9 @@ enum {
 SPICE_DISPLAY_CAP_A8_SURFACE,
 SPICE_DISPLAY_CAP_STREAM_REPORT,
 SPICE_DISPLAY_CAP_LZ4_COMPRESSION,
+SPICE_DISPLAY_CAP_MULTI_CODEC,
+SPICE_DISPLAY_CAP_CODEC_MJPEG,
+SPICE_DISPLAY_CAP_CODEC_VP8,
 };
 
 enum {
-- 
2.1.4
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH spice 4/7] server: Add a GStreamer 0.10 MJPEG video encoder and use it by default. (take 2)

2015-05-27 Thread Francois Gouget

The GStreamer video encoder is quite primitive and mostly meant to introduce, 
test and debug the basic infrastructure.
The main limitation is the lack of rate control: the bitrate is set at startup 
and will not react to changes in the network conditions. Still it should work 
fine on LANs.
---

Changes since take 1:
- Use autovideoconvert instead of ffmpegcolorspace.
- The width/height comments are now correct in the previous patch of
  the series.
- Added a comment suggesting to avoid copying the compressed buffer as 
  a future improvement.


 configure.ac   |  18 ++
 server/Makefile.am |   8 +
 server/gstreamer_encoder.c | 440 +
 server/red_worker.c|  11 +-
 server/video_encoder.h |   6 +
 5 files changed, 481 insertions(+), 2 deletions(-)
 create mode 100644 server/gstreamer_encoder.c

diff --git a/configure.ac b/configure.ac
index 7a9fc4b..919bf88 100644
--- a/configure.ac
+++ b/configure.ac
@@ -91,6 +91,15 @@ if test x$enable_smartcard = xyes; then
AC_DEFINE([USE_SMARTCARD], [1], [Define if supporting smartcard proxying])
 fi
 
+AC_ARG_ENABLE(gstreamer-0.10,
+[  --enable-gstreamer-0.10   Enable GStreamer 0.10 support],,
+[enable_gstreamer_0_10=yes])
+AS_IF([test x$enable_gstreamer_0_10 != xno], [enable_gstreamer_0_10=yes])
+AM_CONDITIONAL(SUPPORT_GSTREAMER_0_10, test x$enable_gstreamer_0_10 != xno)
+if test x$enable_gstreamer_0_10 = xyes; then
+   AC_DEFINE([HAVE_GSTREAMER_0_10], [1], [Define if supporting GStreamer 0.10])
+fi
+
 AC_ARG_ENABLE(automated_tests,
 [  --enable-automated-tests Enable automated tests using spicy-screenshot 
(part of spice--gtk)],,
 [enable_automated_tests=no])
@@ -127,6 +136,13 @@ if test x$enable_smartcard = xyes; then
 AS_VAR_APPEND([SPICE_REQUIRES], [ libcacard = 0.1.2])
 fi
 
+if test x$enable_gstreamer_0_10 = xyes; then
+PKG_CHECK_MODULES(GSTREAMER_0_10, [gstreamer-0.10, gstreamer-app-0.10])
+AC_SUBST(GSTREAMER_0_10_LIBS)
+AC_SUBST(GSTREAMER_0_10_CFLAGS)
+AS_VAR_APPEND([SPICE_REQUIRES], [ gstreamer-0.10])
+fi
+
 
 PKG_CHECK_MODULES([GLIB2], [glib-2.0 = 2.22])
 AS_VAR_APPEND([SPICE_REQUIRES], [ glib-2.0 = 2.22])
@@ -359,6 +375,8 @@ echo 
 
 Smartcard:${enable_smartcard}
 
+GStreamer-0.10:   ${enable_gstreamer_0_10}
+
 SASL support: ${enable_sasl}
 
 Automated tests:  ${enable_automated_tests}
diff --git a/server/Makefile.am b/server/Makefile.am
index 36b6d45..28c4a46 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -11,6 +11,7 @@ AM_CPPFLAGS = \
$(SASL_CFLAGS)  \
$(SLIRP_CFLAGS) \
$(SMARTCARD_CFLAGS) \
+   $(GSTREAMER_0_10_CFLAGS)\
$(SSL_CFLAGS)   \
$(VISIBILITY_HIDDEN_CFLAGS) \
$(WARN_CFLAGS)  \
@@ -41,6 +42,7 @@ libspice_server_la_LIBADD =   
\
$(PIXMAN_LIBS)  \
$(SASL_LIBS)\
$(SLIRP_LIBS)   \
+   $(GSTREAMER_0_10_LIBS)  
\
$(SSL_LIBS) \
$(Z_LIBS)   \
$(SPICE_NONPKGCONFIG_LIBS)  \
@@ -138,6 +140,12 @@ libspice_server_la_SOURCES +=  \
$(NULL)
 endif
 
+if SUPPORT_GSTREAMER_0_10
+libspice_server_la_SOURCES +=  \
+   gstreamer_encoder.c \
+   $(NULL)
+endif
+
 EXTRA_DIST =   \
glz_encode_match_tmpl.c \
glz_encode_tmpl.c   \
diff --git a/server/gstreamer_encoder.c b/server/gstreamer_encoder.c
new file mode 100644
index 000..cd80cc9
--- /dev/null
+++ b/server/gstreamer_encoder.c
@@ -0,0 +1,440 @@
+/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
+/*
+   Copyright (C) 2015 Jeremy White
+   Copyright (C) 2015 Francois Gouget
+
+   This library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   This library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with this library; if not, see A

Re: [Spice-devel] [PATCH 0/11] Add GStreamer and VP8 support

2015-05-27 Thread Francois Gouget

On Thu, 21 May 2015, Fabio Fantoni wrote:
[...]
 Thanks for improving spice, I tried your patches, I applied the
 spice-gtk, spice-protocol and I updated the git submodule to use right
 spice-protocol but make of spice-common/common generate a new enum.h and
 override the spice-protocol patch failing the spice-gtk build with vp8
 codec undefined.
 Seems that also a spice-common patch (at least in spice.proto) is needed
 but missed in the patch serie posted.

Thanks for pointing this out. I could not see it initially because the 
top-level autogen.sh keeps rechecking out spice-protocol so 
spice-protocol/spice/enums.h ended up always being newer than 
spice.proto. I have added a spice-common patch in the new series.


-- 
Francois Gouget fgou...@free.fr  http://fgouget.free.fr/
A particle is an irreducible representation of the Poincaré Group - Eugene 
Wigner___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice] server: Remove an unused structure.

2015-05-27 Thread Jonathon Jongsma

In addition, the name has a typo :)

ACK

On Wed, 2015-05-27 at 16:21 +0200, Francois Gouget wrote:
 ---
 
 This struct was introduced in the first commit, c1b79eb0, about 6 years 
 ago, but has never been used. So I think it's safe to remove.
 
  server/red_dispatcher.c | 10 --
  1 file changed, 10 deletions(-)
 
 diff --git a/server/red_dispatcher.c b/server/red_dispatcher.c
 index e8a1347..9d02d20 100644
 --- a/server/red_dispatcher.c
 +++ b/server/red_dispatcher.c
 @@ -69,16 +69,6 @@ struct RedDispatcher {
  QXLDevSurfaceCreate surface_create;
  };
  
 -typedef struct RedWorkeState {
 -uint8_t *io_base;
 -unsigned long phys_delta;
 -
 -uint32_t x_res;
 -uint32_t y_res;
 -uint32_t bits;
 -uint32_t stride;
 -} RedWorkeState;
 -
  extern uint32_t streaming_video;
  extern spice_image_compression_t image_compression;
  extern spice_wan_compression_t jpeg_state;


___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH spice] server: Remove an unused structure.

2015-05-27 Thread Francois Gouget

---

This struct was introduced in the first commit, c1b79eb0, about 6 years 
ago, but has never been used. So I think it's safe to remove.

 server/red_dispatcher.c | 10 --
 1 file changed, 10 deletions(-)

diff --git a/server/red_dispatcher.c b/server/red_dispatcher.c
index e8a1347..9d02d20 100644
--- a/server/red_dispatcher.c
+++ b/server/red_dispatcher.c
@@ -69,16 +69,6 @@ struct RedDispatcher {
 QXLDevSurfaceCreate surface_create;
 };
 
-typedef struct RedWorkeState {
-uint8_t *io_base;
-unsigned long phys_delta;
-
-uint32_t x_res;
-uint32_t y_res;
-uint32_t bits;
-uint32_t stride;
-} RedWorkeState;
-
 extern uint32_t streaming_video;
 extern spice_image_compression_t image_compression;
 extern spice_wan_compression_t jpeg_state;
-- 
2.1.4
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice 7/7] server: Add VP8 support, a video codec preference list and compatibility checks with the Spice client. (take 2)

2015-05-27 Thread Fabio Fantoni

Il 27/05/2015 15:13, Francois Gouget ha scritto:
 The video encoder preferences can be expressed by building a semi-colon 
 separated list of encoder:codec pairs. For instance 
 'gstreamer:vp8;spice:mjpeg' to pick first the GStreamer VP8 video encoder 
 first and used the original MJPEG video encoder one as a fallback.
 The server has a default preference list which can also be selected by 
 specifying 'auto' as the preferences list.
 The client compatibility check relies on the following new capabilities:
  * SPICE_DISPLAY_CAP_MULTI_CODEC which denotes a recent client that supports 
 multiple codecs. This capability is needed to not have to hardcode that MJPEG 
 is supported. This makes it possible to write clients that don't support 
 MJPEG.
  * SPICE_DISPLAY_CAP_CODEC_XXX, where XXX is a supported codec, for now MJPEG 
 and VP8.
 ---

 Changes since take 1:
 - Only set the parameters supported by the current encoder.
 - reconfigure_pipeline() handling has been fixed in the previous patch.
 - The video codecs list now has a more sensible default and it's 
   possible to explicitly request this default by specifying 'auto' in 
   the configuration files.

 The buffer timestamping is unchanged because I don't see a better 
 solution at this time. 

 Note that the xf86-video-qxl, spice-gtk and spice-html5 patches work 
 just fine with this new series so I'm not reposting them to limit 
 redundant emails. If this series can go in then I can resubmit them. For 
 now, anyone who needs them for testing is invited to grab them there:

 * [PATCH qxl 7/11] Add SpiceVideoCodecs and $XSPICE_VIDEO_CODECS for 
 specifying video codec preferences.
   http://lists.freedesktop.org/archives/spice-devel/2015-May/019770.html

 * [PATCH qemu 08/11] Add the ability to specify Spice video codecs.
   http://lists.freedesktop.org/archives/spice-devel/2015-May/019771.html

Qemu patch have an error:
http://lists.freedesktop.org/archives/spice-devel/2015-May/019901.html

Thanks for this serie update, I'll try probably tomorrow.


 * [PATCH spice-gtk 9/11] Add the ability to use gstreamer for vp8 decoding.
   http://lists.freedesktop.org/archives/spice-devel/2015-May/019772.html

 * [PATCH spice-html5 10/11] Revise the webm files to more correctly identify 
 audio tracks.
   http://lists.freedesktop.org/archives/spice-devel/2015-May/019773.html

 * [PATCH spice-html5 11/12] Add support for the vp8 codec type.
   (this is really patch 11/11)
   http://lists.freedesktop.org/archives/spice-devel/2015-May/019774.html



  server/gstreamer_encoder.c |  68 ++-
  server/mjpeg_encoder.c |   6 +-
  server/red_dispatcher.c| 165 
 -
  server/red_dispatcher.h|   8 +++
  server/red_worker.c|  62 ++---
  server/red_worker.h|  18 +
  server/reds.c  |  12 
  server/spice-server.h  |   1 +
  server/spice-server.syms   |   1 +
  server/video_encoder.h |  18 +++--
  10 files changed, 312 insertions(+), 47 deletions(-)

 diff --git a/server/gstreamer_encoder.c b/server/gstreamer_encoder.c
 index 15ddd82..01b4f5d 100644
 --- a/server/gstreamer_encoder.c
 +++ b/server/gstreamer_encoder.c
 @@ -42,6 +42,7 @@ typedef struct {
  
  struct GstEncoder {
  VideoEncoder base;
 +const gchar* gstenc_name;
  
  /* The GStreamer pipeline. If pipeline is NULL then the other pointers 
 are
   * invalid.
 @@ -139,11 +140,11 @@ static void adjust_bit_rate(GstEncoder *encoder)
  {
  /* Even MJPEG should achieve good quality with a 10x compression 
 level */
  encoder-bit_rate = raw_bit_rate / 10;
 -spice_debug(capping the bit rate to %.2fMbps for a 10x compression 
 level, ((double)encoder-bit_rate) / 1024 / 1024);
 +spice_debug(capping the %s bit rate to %.2fMbps for a 10x 
 compression level, encoder-gstenc_name, ((double)encoder-bit_rate) / 1024 
 / 1024);
  }
  else
  {
 -spice_debug(setting the bit rate to %.2fMbps for a %dx compression 
 level, ((double)encoder-bit_rate) / 1024 / 1024, compression);
 +spice_debug(setting the %s bit rate to %.2fMbps for a %dx 
 compression level, encoder-gstenc_name, ((double)encoder-bit_rate) / 1024 
 / 1024, compression);
  }
  }
  
 @@ -177,9 +178,12 @@ static int construct_pipeline(GstEncoder *encoder, const 
 SpiceBitmap *bitmap)
  {
  GstStateChangeReturn ret;
  GError *err;
 +gchar *desc;
  
  err = NULL;
 -encoder-pipeline = gst_parse_launch_full(appsrc name=src is-live=1 ! 
 autovideoconvert ! ffenc_mjpeg name=encoder ! appsink name=sink, NULL, 
 GST_PARSE_FLAG_FATAL_ERRORS, err);
 +desc = g_strdup_printf(appsrc name=src is-live=1 ! autovideoconvert ! 
 %s name=encoder ! appsink name=sink, encoder-gstenc_name);
 +encoder-pipeline = gst_parse_launch_full(desc, NULL, 
 GST_PARSE_FLAG_FATAL_ERRORS, err);
 +g_free(desc);
  if (!encoder-pipeline)
  {

Re: [Spice-devel] [PATCH spice 7/7] server: Add VP8 support, a video codec preference list and compatibility checks with the Spice client. (take 2)

2015-05-27 Thread Jeremy White

 * [PATCH qemu 08/11] Add the ability to specify Spice video codecs.
   http://lists.freedesktop.org/archives/spice-devel/2015-May/019771.html
 
 Qemu patch have an error:
 http://lists.freedesktop.org/archives/spice-devel/2015-May/019901.html

Yes, thanks.  There is an error with spice-html5 as well.

I think I'll plan to resubmit once the server parts are in, and my
patches can be smaller sets.

Cheers,

Jeremy
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH xf86-video-qxl] Do not process watches on select() error.

2015-05-27 Thread Jeremy White

This enables a kill of an Xorg process to propogate further.
Without this, the read masks would be set, and we could end up
blocking in an accept() call and not exiting from the signal.

Signed-off-by: Jeremy White jwh...@codeweavers.com
---
 src/spiceqxl_main_loop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/spiceqxl_main_loop.c b/src/spiceqxl_main_loop.c
index 92579eb..ac9e43f 100644
--- a/src/spiceqxl_main_loop.c
+++ b/src/spiceqxl_main_loop.c
@@ -296,7 +296,7 @@ static void select_and_check_watches(void)
 watch = (SpiceWatch*)watches.next;
 timeout.tv_sec = timeout.tv_usec = 0;
 retval = select(max_fd + 1, rfds, wfds, NULL, timeout);
-if (retval) {
+if (retval  0) {
 RING_FOREACH_SAFE(link, next, watches) {
 watch = (SpiceWatch*)link;
 if (!watch-remove  (watch-event_mask  SPICE_WATCH_EVENT_READ)
-- 
2.1.4

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 05/11] Handle all errors in qxl_surface_evict

2015-05-27 Thread Dave Airlie

On 27 May 2015 at 20:04, Frediano Ziglio fzig...@redhat.com wrote:
 Only EBUSY error was handled. This could cause code to believe
 reserve was successful while it failed.

 Signed-off-by: Frediano Ziglio fzig...@redhat.com

This has been there since I wrote qxl, so I expect I had some reason
for it, but I can't remember it
now..

Most likely is something had the bo reserved already and we reentered
from somewhere we shouldn't
but I think a lot of the horrible code went away.

so because I can't justify the hack,
Reviewed-by: Dave Airlie airl...@redhat.com
Dave.
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 06/11] Fix return for qxl_release_alloc

2015-05-27 Thread Dave Airlie

On 27 May 2015 at 20:04, Frediano Ziglio fzig...@redhat.com wrote:
 This function return handle to allocated release object which is an int.

Reviewed-by: Dave Airlie airl...@redhat.com

 Signed-off-by: Frediano Ziglio fzig...@redhat.com
 ---
  qxl/qxl_release.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/qxl/qxl_release.c b/qxl/qxl_release.c
 index d9b2568..6fd8e50 100644
 --- a/qxl/qxl_release.c
 +++ b/qxl/qxl_release.c
 @@ -122,7 +122,7 @@ static const struct fence_ops qxl_fence_ops = {
 .wait = qxl_fence_wait,
  };

 -static uint64_t
 +static int
  qxl_release_alloc(struct qxl_device *qdev, int type,
   struct qxl_release **ret)
  {
 --
 2.1.0

 ___
 Spice-devel mailing list
 Spice-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 01/11] Do not cause spice-server to clean our objects

2015-05-27 Thread Dave Airlie

On 27 May 2015 at 20:03, Frediano Ziglio fzig...@redhat.com wrote:
 If objects are moved back from system memory to VRAM (and spice id
 created again) memory is already initialized so we need to set flag
 to not clear memory.
 If you don't do it after a while using desktop many images turns to
 black or transparents.

Good point,

Reviewed-by: Dave Airlie airl...@redhat.com

 Signed-off-by: Frediano Ziglio fzig...@redhat.com
 ---
  qxl/qxl_cmd.c | 1 +
  1 file changed, 1 insertion(+)

 diff --git a/qxl/qxl_cmd.c b/qxl/qxl_cmd.c
 index bd5404e..85ed5dc 100644
 --- a/qxl/qxl_cmd.c
 +++ b/qxl/qxl_cmd.c
 @@ -502,6 +502,7 @@ int qxl_hw_surface_alloc(struct qxl_device *qdev,

 cmd = (struct qxl_surface_cmd *)qxl_release_map(qdev, release);
 cmd-type = QXL_SURFACE_CMD_CREATE;
 +   cmd-flags = QXL_SURF_FLAG_KEEP_DATA;
 cmd-u.surface_create.format = surf-surf.format;
 cmd-u.surface_create.width = surf-surf.width;
 cmd-u.surface_create.height = surf-surf.height;
 --
 2.1.0

 ___
 Spice-devel mailing list
 Spice-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 02/11] Do not leak memory if qxl_release_list_add fails

2015-05-27 Thread Dave Airlie

On 27 May 2015 at 20:03, Frediano Ziglio fzig...@redhat.com wrote:
 If the function fails reference counter to the object is not decremented
 causing leaks.
 This is hard to spot as it happens only on very low memory situations.

 Signed-off-by: Frediano Ziglio fzig...@redhat.com

Looks good,

Reviewed-by: Dave Airlie airl...@redhat.com

Dave.
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 08/11] Remove format string errors

2015-05-27 Thread Dave Airlie

On 27 May 2015 at 20:04, Frediano Ziglio fzig...@redhat.com wrote:
 Enable format string checks for qxl_io_log and remove resulting warnings
 which could lead to memory errors on different platform or just printing
 wrong information.

 Signed-off-by: Frediano Ziglio fzig...@redhat.com

Reviewed-by: Dave Airlie airl...@redhat.com
 ---
  qxl/qxl_cmd.c | 2 +-
  qxl/qxl_display.c | 2 +-
  qxl/qxl_drv.h | 2 +-
  qxl/qxl_release.c | 2 +-
  4 files changed, 4 insertions(+), 4 deletions(-)

 diff --git a/qxl/qxl_cmd.c b/qxl/qxl_cmd.c
 index b18f84c..edc1eec 100644
 --- a/qxl/qxl_cmd.c
 +++ b/qxl/qxl_cmd.c
 @@ -248,7 +248,7 @@ int qxl_garbage_collect(struct qxl_device *qdev)
 }
 }

 -   QXL_INFO(qdev, %s: %lld\n, __func__, i);
 +   QXL_INFO(qdev, %s: %d\n, __func__, i);

 return i;
  }
 diff --git a/qxl/qxl_display.c b/qxl/qxl_display.c
 index 4a0a8b2..a8dbb3e 100644
 --- a/qxl/qxl_display.c
 +++ b/qxl/qxl_display.c
 @@ -67,7 +67,7 @@ static int 
 qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
 crc = crc32(0, (const uint8_t *)qdev-rom-client_monitors_config,
   sizeof(qdev-rom-client_monitors_config));
 if (crc != qdev-rom-client_monitors_config_crc) {
 -   qxl_io_log(qdev, crc mismatch: have %X (%d) != %X\n, crc,
 +   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;
 diff --git a/qxl/qxl_drv.h b/qxl/qxl_drv.h
 index 6745c44..62ef8be 100644
 --- a/qxl/qxl_drv.h
 +++ b/qxl/qxl_drv.h
 @@ -328,7 +328,7 @@ struct qxl_device {
  };

  /* forward declaration for QXL_INFO_IO */
 -void qxl_io_log(struct qxl_device *qdev, const char *fmt, ...);
 +__printf(2,3) void qxl_io_log(struct qxl_device *qdev, const char *fmt, ...);

  extern const struct drm_ioctl_desc qxl_ioctls[];
  extern int qxl_max_ioctl;
 diff --git a/qxl/qxl_release.c b/qxl/qxl_release.c
 index 00604ed..b66ec33 100644
 --- a/qxl/qxl_release.c
 +++ b/qxl/qxl_release.c
 @@ -153,7 +153,7 @@ qxl_release_alloc(struct qxl_device *qdev, int type,
 return handle;
 }
 *ret = release;
 -   QXL_INFO(qdev, allocated release %lld\n, handle);
 +   QXL_INFO(qdev, allocated release %d\n, handle);
 release-id = handle;
 return handle;
  }
 --
 2.1.0

 ___
 Spice-devel mailing list
 Spice-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] Do not loop on ERESTARTSYS using interruptible waits

2015-05-27 Thread Dave Airlie

On 19 May 2015 at 19:54, Frediano Ziglio fzig...@redhat.com wrote:
 This problem happens using KMS surfaces and QXL driver.
 To easy reproduce use KDE Plasma (which use surfaces a lot) and assure
 you are using KMS surfaces (QXL driver on Fedora/RedHat has a patch to
 stop using them). Open some complex application like LibreOffice and
 after a while your machine get stuck using 100% CPU on Xorg.
 The problem occurs as creating new surfaces not interruptible wait
 are used however instead of returning ERESTARTSYS back to userspace
 you try to loop but wait routines always keep returning ERESTARTSYS
 once the signal is marked.
 On out of memory conditions TTM module try to move objects to system
 memory and QXL assure surface is updated before the move.
 The fix handle differently this case using no interruptible wait so
 wait functions will wait instead of returning ERESTARTSYS.
 Note the when the loop occurs driver will send a lot of update requests
 causing more CPU usage on Qemu side too.

I actually don't think we should be enabling surfaces upstream, I
don't mind fixing
the kernel driver to not be crap, but I really don't think surfaces
really help the
SPICE protocol.

I should h ave pushed the disable surface in all cases upstream, feel free to do
so, they were a bad experiment, and nobody ever showed they were faster, or
at least when they hit eviction paths they didn't plummet down the
side of a massive
cliff.

The reason this loops in -ERESTARTSYS is that the hw craps itself if you try
and redo an operation, so you can't go back out to userspace and re-enter the
kernel, just one of many bad design points in the QXL hw.

you should probably drop wait_for_io_cmd completely.

Dave.


 Signed-off-by: Frediano Ziglio fzig...@redhat.com
 ---
  qxl/qxl_cmd.c   | 12 +++-
  qxl/qxl_drv.h   |  2 +-
  qxl/qxl_ioctl.c |  2 +-
  3 files changed, 5 insertions(+), 11 deletions(-)

 diff --git a/drivers/gpu/drm/drivers/gpu/drm/qxl/qxl_cmd.c b/qxl/qxl_cmd.c
 index 9782364..bd5404e 100644
 --- a/drivers/gpu/drm/qxl/qxl_cmd.c
 +++ b/drivers/gpu/drm/qxl/qxl_cmd.c
 @@ -317,14 +317,11 @@ static void wait_for_io_cmd(struct qxl_device *qdev, 
 uint8_t val, long port)
  {
 int ret;

 -restart:
 ret = wait_for_io_cmd_user(qdev, val, port, false);
 -   if (ret == -ERESTARTSYS)
 -   goto restart;
  }

  int qxl_io_update_area(struct qxl_ddevice *qdev, struct qxl_bo *surf,
 -   const struct qxl_rect *area)
 +   const struct qxl_rect *area, bool intr)
  {
 int surface_id;
 uint32_t surface_width, surface_height;
 @@ -350,7 +347,7 @@ int qxl_io_update_area(struct qxl_device *qdev, struct 
 qxl_bo *surf,
 mutex_lock(qdev-update_area_mutex);
 qdev-ram_header-update_area = *area;
 qdev-ram_header-update_surface = surface_id;
 -   ret = wait_for_io_cmd_user(qdev, 0, QXL_IO_UPDATE_AREA_ASYNC, true);
 +   ret = wait_for_io_cmd_user(qdev, 0, QXL_IO_UPDATE_AREA_ASYNC, intr);
 mutex_unlock(qdev-update_area_mutex);
 return ret;
  }
 @@ -588,10 +585,7 @@ int qxl_update_surface(struct qxl_device *qdev, struct 
 qxl_bo *surf)
 rect.right = surf-surf.width;
 rect.top = 0;
 rect.bottom = surf-surf.height;
 -retry:
 -   ret = qxl_io_update_area(qdev, surf, rect);
 -   if (ret == -ERESTARTSYS)
 -   goto retry;
 +   ret = qxl_io_update_area(qdev, surf, rect, false);
 return ret;
  }

 diff --git a/drivers/gpu/drm/drivers/gpu/drm/qxl/qxl_drv.h b/qxl/qxl_drv.h
 index 7c6cafe..6745c44 100644
 --- a/drivers/gpu/drm/qxl/qxl_drv.h
 +++ b/drivers/gpu/drm/qxl/qxl_drv.h
 @@ -462,7 +462,7 @@ void qxl_io_memslot_add(struct qxl_device *qdev, uint8_t 
 id);
  void qxl_io_notify_oom(struct qxl_device *qdev);

  int qxl_io_update_area(struct qxl_device *qdev, struct qxl_bo *surf,
 -  const struct qxl_rect *area);
 +  const struct qxl_rect *area, bool intr);

  void qxl_io_reset(struct qxl_device *qdev);
  void qxl_io_monitors_config(struct qxl_device *qdev);
 diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/qxl/qxl_ioctl.c
 index b110883..afd7297 100644
 --- a/drivers/gpu/drm/qxl/qxl_ioctl.c
 +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
 @@ -348,7 +348,7 @@ static int qxl_update_area_ioctl(struct drm_device *dev, 
 void *data,
 goto out2;
 if (!qobj-surface_id)
 DRM_ERROR(got update area for surface with no id %d\n, 
 update_area-handle);
 -   ret = qxl_io_update_area(qdev, qobj, area);
 +   ret = qxl_io_update_area(qdev, qobj, area, true);

  out2:
 qxl_bo_unreserve(qobj);
 --
 2.1.0
 ___
 Spice-devel mailing list
 Spice-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org

Re: [Spice-devel] [PATCH 07/11] Handle correctly failures in qxl_alloc_relase_reserved

2015-05-27 Thread Dave Airlie

On 27 May 2015 at 20:04, Frediano Ziglio fzig...@redhat.com wrote:
 Free resources correctly if function fails

 Signed-off-by: Frediano Ziglio fzig...@redhat.com

Reviewed-by: Dave Airlie airl...@redhat.com
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 00/11] Miscellaneous stability patches

2015-05-27 Thread Dave Airlie

 I was using a different repository with only QXL driver. I tested and all 
 patches apply and compile perfectly even with Linus master branch.

Lets only post patches people can apply, it makes it harder to figure
out stuff. I'll take a look at the patches, but it would be good to
get them resent base on drm-next.

also indicating in each patch what is a right now fix and what isn't.

Dave.
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 09/11] Move main reference counter to GEM object instead of TTM ones

2015-05-27 Thread Dave Airlie

On 27 May 2015 at 20:04, Frediano Ziglio fzig...@redhat.com wrote:
 qxl_bo structure has two reference counters, one in the GEM object and
 another in the TTM object. The GEM object keep a counter to the TTM object
 so when GEM counter reached zero the TTM counter (using qxl_bo_unref) was
 decremented. The qxl object is fully freed (both GEM and TTM part are cleaned)
 when the TTM counter reach zero.
 One issue was that surface idr structure has no owning on qxl_bo objects 
 however
 it contains a pointer to qxl_bo object. This caused some nasty race condition
 for instance qxl_bo object was reaped even after counter was already zero.
 This patch fix these races moving main counter (the one used by 
 qxl_bo_(un)ref)
 to GEM object which cleanup routine (qxl_gem_object_free) remove the idr 
 pointer
 (using qxl_surface_evict) when the counters are still valid.

Uggh, but yes, not sure I like this fix for the problem, but if it works,

Reviewed-by: Dave Airlie airl...@redhat.com
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 10/11] Simplify cleaning qxl processing command

2015-05-27 Thread Dave Airlie

On 27 May 2015 at 20:04, Frediano Ziglio fzig...@redhat.com wrote:
 In qxlhw_handle_to_bo we incremented counters twice, one time for release 
 object
 and one for reloc_info.
 In the main function however reloc_info references was drop much earlier than
 release so keeping the pointer only on release is safe and make cleaning
 process easier.

Seems fine,

Reviewed-by: Dave Airlie airl...@redhat.com

 Signed-off-by: Frediano Ziglio fzig...@redhat.com
 ---
  qxl/qxl_ioctl.c | 13 +++--
  1 file changed, 3 insertions(+), 10 deletions(-)

 diff --git a/qxl/qxl_ioctl.c b/qxl/qxl_ioctl.c
 index 85b3808..bb326ff 100644
 --- a/qxl/qxl_ioctl.c
 +++ b/qxl/qxl_ioctl.c
 @@ -122,10 +122,9 @@ static struct qxl_bo *qxlhw_handle_to_bo(struct 
 qxl_device *qdev,
 qobj = gem_to_qxl_bo(gobj);

 ret = qxl_release_list_add(release, qobj);
 -   if (ret) {
 -   drm_gem_object_unreference_unlocked(gobj);
 +   drm_gem_object_unreference_unlocked(gobj);
 +   if (ret)
 return NULL;
 -   }

 return qobj;
  }
 @@ -145,7 +144,7 @@ static int qxl_process_single_command(struct qxl_device 
 *qdev,
 struct qxl_release *release;
 struct qxl_bo *cmd_bo;
 void *fb_cmd;
 -   int i, j, ret, num_relocs;
 +   int i, ret, num_relocs;
 int unwritten;

 switch (cmd-type) {
 @@ -269,12 +268,6 @@ static int qxl_process_single_command(struct qxl_device 
 *qdev,
 qxl_release_fence_buffer_objects(release);

  out_free_bos:
 -   for (j = 0; j  num_relocs; j++) {
 -   if (reloc_info[j].dst_bo != cmd_bo)
 -   
 drm_gem_object_unreference_unlocked(reloc_info[j].dst_bo-gem_base);
 -   if (reloc_info[j].src_bo  reloc_info[j].src_bo != cmd_bo)
 -   
 drm_gem_object_unreference_unlocked(reloc_info[j].src_bo-gem_base);
 -   }
  out_free_release:
 if (ret)
 qxl_release_free(qdev, release);
 --
 2.1.0

 ___
 Spice-devel mailing list
 Spice-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH usbdk] UsbDk: Add support for USB 3.0 controllers by MS, Intel and Renesas

2015-05-27 Thread Jay.han

Add search strategy on USB3 controller, NUSB3
represents Renesas Electronics  usb 3 controller, IUSB3 represents to Intel
USB 3 controller

Signed-off-by: Jay.Han ezzze...@gmail.com
---
 UsbDk/FilterDevice.cpp | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/UsbDk/FilterDevice.cpp b/UsbDk/FilterDevice.cpp
index b4433db..e02dd98 100644
--- a/UsbDk/FilterDevice.cpp
+++ b/UsbDk/FilterDevice.cpp
@@ -485,7 +485,9 @@ bool
CUsbDkFilterDevice::CStrategist::SelectStrategy(PDEVICE_OBJECT DevObj)
 DevID-Dump();

 // 2. Root hubs - Hub strategy
-if ((DevID-Match(LUSB\\ROOT_HUB) ||
DevID-Match(LUSB\\ROOT_HUB20)))
+if ((DevID-Match(LUSB\\ROOT_HUB) ||
DevID-Match(LUSB\\ROOT_HUB20)
+|| DevID-Match(LUSB\\ROOT_HUB30) ||
DevID-Match(LNUSB3\\ROOT_HUB30)
+|| DevID-Match(LIUSB3\\ROOT_HUB30)))
 {
 TraceEvents(TRACE_LEVEL_INFORMATION, TRACE_FILTERDEVICE, %!FUNC!
Assigning HUB strategy);
 m_Strategy-Delete();
-- 
1.8.3.1

-- 
Jay.Han
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 03/11] Fix print statement not using uninitialized variable

2015-05-27 Thread Dave Airlie

On 27 May 2015 at 20:03, Frediano Ziglio fzig...@redhat.com wrote:
 reloc_info[i] is not still initialized in the print statement.

 Signed-off-by: Frediano Ziglio fzig...@redhat.com

Reviewed-by: Dave Airlie airl...@redhat.com
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 04/11] Avoid double free on error

2015-05-27 Thread Dave Airlie

On 27 May 2015 at 20:03, Frediano Ziglio fzig...@redhat.com wrote:
 Is we are not able to get source bo object from handle we free
 destination bo object and call cleanup code however destination
 object was already inserted in reloc_info array (num_relocs was
 already incremented) so on cleanup we free destination again.

 Signed-off-by: Frediano Ziglio fzig...@redhat.com

Reviewed-by: Dave Airlie airl...@redhat.com
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 11/11] Propagate correctly errors from qxlhw_handle_to_bo

2015-05-27 Thread Dave Airlie

On 27 May 2015 at 20:04, Frediano Ziglio fzig...@redhat.com wrote:
 This function could return a NULL pointer in case of handle not
 present and in case of out of memory conditions however caller
 function always returned EINVAL error hiding a possible ENOMEM.
 This patch change the function to return the error instead to
 be able to propagate the error instead of assuming EINVAL.

 Signed-off-by: Frediano Ziglio fzig...@redhat.com

Reviewed-by: Dave Airlie airl...@redhat.com
 ---
  qxl/qxl_ioctl.c | 33 ++---
  1 file changed, 14 insertions(+), 19 deletions(-)

 diff --git a/qxl/qxl_ioctl.c b/qxl/qxl_ioctl.c
 index bb326ff..37f1faf 100644
 --- a/qxl/qxl_ioctl.c
 +++ b/qxl/qxl_ioctl.c
 @@ -107,9 +107,9 @@ apply_surf_reloc(struct qxl_device *qdev, struct 
 qxl_reloc_info *info)
  }

  /* return holding the reference to this object */
 -static struct qxl_bo *qxlhw_handle_to_bo(struct qxl_device *qdev,
 -struct drm_file *file_priv, uint64_t 
 handle,
 -struct qxl_release *release)
 +static int qxlhw_handle_to_bo(struct qxl_device *qdev,
 + struct drm_file *file_priv, uint64_t handle,
 + struct qxl_release *release, struct qxl_bo 
 **qbo_p)
  {
 struct drm_gem_object *gobj;
 struct qxl_bo *qobj;
 @@ -117,16 +117,17 @@ static struct qxl_bo *qxlhw_handle_to_bo(struct 
 qxl_device *qdev,

 gobj = drm_gem_object_lookup(qdev-ddev, file_priv, handle);
 if (!gobj)
 -   return NULL;
 +   return -EINVAL;

 qobj = gem_to_qxl_bo(gobj);

 ret = qxl_release_list_add(release, qobj);
 drm_gem_object_unreference_unlocked(gobj);
 if (ret)
 -   return NULL;
 +   return ret;

 -   return qobj;
 +   *qbo_p = qobj;
 +   return 0;
  }

  /*
 @@ -219,13 +220,10 @@ static int qxl_process_single_command(struct qxl_device 
 *qdev,
 reloc_info[i].type = reloc.reloc_type;

 if (reloc.dst_handle) {
 -   reloc_info[i].dst_bo = qxlhw_handle_to_bo(qdev, 
 file_priv,
 - 
 reloc.dst_handle, release);
 -   if (!reloc_info[i].dst_bo) {
 -   ret = -EINVAL;
 -   reloc_info[i].src_bo = NULL;
 +   ret = qxlhw_handle_to_bo(qdev, file_priv, 
 reloc.dst_handle, release,
 +reloc_info[i].dst_bo);
 +   if (ret)
 goto out_free_bos;
 -   }
 reloc_info[i].dst_offset = reloc.dst_offset;
 } else {
 reloc_info[i].dst_bo = cmd_bo;
 @@ -234,14 +232,11 @@ static int qxl_process_single_command(struct qxl_device 
 *qdev,
 num_relocs++;

 /* reserve and validate the reloc dst bo */
 -   if (reloc.reloc_type == QXL_RELOC_TYPE_BO || reloc.src_handle 
  0) {
 -   reloc_info[i].src_bo =
 -   qxlhw_handle_to_bo(qdev, file_priv,
 -  reloc.src_handle, release);
 -   if (!reloc_info[i].src_bo) {
 -   ret = -EINVAL;
 +   if (reloc.reloc_type == QXL_RELOC_TYPE_BO || 
 reloc.src_handle) {
 +   ret = qxlhw_handle_to_bo(qdev, file_priv, 
 reloc.src_handle, release,
 +reloc_info[i].src_bo);
 +   if (ret)
 goto out_free_bos;
 -   }
 reloc_info[i].src_offset = reloc.src_offset;
 } else {
 reloc_info[i].src_bo = NULL;
 --
 2.1.0

 ___
 Spice-devel mailing list
 Spice-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

43 matches

Mail list logo