[Spice-devel] [PATCH spice-server] Release cursor as soon as possible

[Christophe Fergeau]

On Tue, Feb 28, 2017 at 03:20:09PM +0000, Frediano Ziglio wrote:
> Cursor resources (basically the shape of it) was retained till
> it was used however it was copied so there were no reason to not release
> this resource.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/cursor-channel.c         | 14 ++++----------
>  server/cursor-channel.h         |  4 ++--
>  server/red-parse-qxl.c          | 10 +++++++---
>  server/red-parse-qxl.h          |  4 ++--
>  server/red-worker.c             |  5 +++--
>  server/tests/test-qxl-parsing.c | 26 +++++++++++++++++++++++---
>  6 files changed, 41 insertions(+), 22 deletions(-)
> 
> diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
> index 89cb120..5ed36df 100644
> --- a/server/red-parse-qxl.c
> +++ b/server/red-parse-qxl.c
> @@ -27,6 +27,7 @@
>  #include "red-common.h"
>  #include "memslot.h"
>  #include "red-parse-qxl.h"
> +#include "red-qxl.h"
>  
>  /* Max size in bytes for any data field used in a QXL command.
>   * This will for example be useful to prevent the guest from saturating the
> @@ -1470,8 +1471,10 @@ static void red_put_cursor(SpiceCursor *red)
>  }
>  
>  int red_get_cursor_cmd(RedMemSlotInfo *slots, int group_id,
> -                       RedCursorCmd *red, QXLPHYSICAL addr)
> +                       RedCursorCmd *red, QXLPHYSICAL addr,
> +                       QXLInstance *qxl_instance)

Not convinced about this patch, mainly I think because of the added
QXLInstance dependency which gets added to red-parse-qxl.h,
red_get_cursor_cmd() becoming odd compared to the other similar
functions, ...

I think it would work to have the red_qxl_release_resource() call in
red_process_cursor_cmd()?

>  {
> +    QXLReleaseInfoExt release_info_ext;
>      QXLCursorCmd *qxl;
>      int error;
>  
> @@ -1479,8 +1482,8 @@ int red_get_cursor_cmd(RedMemSlotInfo *slots, int group_id,
>      if (error) {
>          return error;
>      }
> -    red->release_info_ext.info      = &qxl->release_info;
> -    red->release_info_ext.group_id  = group_id;
> +    release_info_ext.info      = &qxl->release_info;
> +    release_info_ext.group_id  = group_id;
>  
>      red->type = qxl->type;
>      switch (red->type) {
> @@ -1497,6 +1500,7 @@ int red_get_cursor_cmd(RedMemSlotInfo *slots, int group_id,
>          red->u.trail.frequency = qxl->u.trail.frequency;
>          break;
>      }
> +    red_qxl_release_resource(qxl_instance, release_info_ext);
>      return error;
>  }
>  
> diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
> index 86a2d93..9aa1077 100644
> --- a/server/red-parse-qxl.h
> +++ b/server/red-parse-qxl.h
> @@ -99,7 +99,6 @@ typedef struct RedSurfaceCmd {
>  } RedSurfaceCmd;
>  
>  typedef struct RedCursorCmd {
> -    QXLReleaseInfoExt release_info_ext;
>      uint8_t type;
>      union {
>          struct {
> @@ -138,7 +137,8 @@ int red_get_surface_cmd(RedMemSlotInfo *slots, int group_id,
>  void red_put_surface_cmd(RedSurfaceCmd *red);
>  
>  int red_get_cursor_cmd(RedMemSlotInfo *slots, int group_id,
> -                       RedCursorCmd *red, QXLPHYSICAL addr);
> +                       RedCursorCmd *red, QXLPHYSICAL addr,
> +                       QXLInstance *qxl);
>  void red_put_cursor_cmd(RedCursorCmd *red);
>  
>  #endif
> diff --git a/server/red-worker.c b/server/red-worker.c
> index e5adbaa..20b15bc 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -107,7 +107,8 @@ static gboolean red_process_cursor_cmd(RedWorker *worker, const QXLCommandExt *e
>      RedCursorCmd *cursor_cmd;
>  
>      cursor_cmd = spice_new0(RedCursorCmd, 1);
> -    if (red_get_cursor_cmd(&worker->mem_slots, ext->group_id, cursor_cmd, ext->cmd.data)) {
> +    if (red_get_cursor_cmd(&worker->mem_slots, ext->group_id, cursor_cmd, ext->cmd.data,
> +                           worker->qxl)) {
>          free(cursor_cmd);
>          return FALSE;
>      }
> @@ -1364,7 +1365,7 @@ RedWorker* red_worker_new(QXLInstance *qxl,
>  
>      worker->event_timeout = INF_EVENT_WAIT;
>  
> -    worker->cursor_channel = cursor_channel_new(reds, qxl,
> +    worker->cursor_channel = cursor_channel_new(reds, qxl->id,
>                                                  &worker->core);

This change is not strictly related to what you are doing in this
commit, and in my opinion this belongs more in a follow-up patch moving
CommonGraphicsChannel::qxl to a DisplayChannel::qxl property as after
this change, it will always be NULL.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170301/7380e062/attachment-0001.sig>