>
> 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@xxxxxxxxxx>
> > ---
> > 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()?
>
This would be just a step before this...
Memory management and resources are two sides of the coin.
red-parse-qxl.c already deals with release_info information so
adding a free is not really a big jump.
Actually already some structures in red-parse-qxl.h has a QXLInstance
which is initialized by red-worker.c. Maybe this is the issue, some
structures are basically initialized half in red-parse-qxl.c and
half in red-worker.c.
I think I have 3/4 branches trying in different way to do
some sort of this but I think this is the best even considering
guest resources. Resources are not used, just retained more
(the cursor is copied in host allocated memory in any way).
> > {
> > + 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
>
Yes, I can split. And also start follow ups.
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel