[Spice-devel] [PATCH v2 1/2] display-channel: Join drawables to improve rhel7 behaviour
Frediano Ziglio
fziglio at redhat.com
Fri Mar 3 11:38:26 UTC 2017
>
> On Thu, 2017-03-02 at 11:34 +0000, Frediano Ziglio wrote:
> > Due to the way RHEL7 works the images came out from guest using
> > multiple
> > commands. This increase the commands to the client and cause the
> > video code to create and handle multiple streams creating some
> > visual glitches.
> > This patch attempt to detect and join the multiple commands to
> > avoid these issues.
> >
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> > server/display-channel-private.h | 2 +-
> > server/display-channel.c | 173
> > +++++++++++++++++++++++++++++++-
> > server/red-parse-qxl.h | 1 +-
> > server/red-worker.c | 14 +--
> > 4 files changed, 183 insertions(+), 7 deletions(-)
> >
> > diff --git a/server/display-channel-private.h b/server/display-
> > channel-private.h
> > index da807d1..62e03b6 100644
> > --- a/server/display-channel-private.h
> > +++ b/server/display-channel-private.h
> > @@ -69,6 +69,8 @@ struct DisplayChannelPrivate
> >
> > int gl_draw_async_count;
> >
> > + RedDrawable *joinable_drawable;
> > +
> > /* TODO: some day unify this, make it more runtime.. */
> > stat_info_t add_stat;
> > stat_info_t exclude_stat;
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index fa2b281..37d1703 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -1175,8 +1175,147 @@ static void
> > display_channel_add_drawable(DisplayChannel *display, Drawable *draw
> > #endif
> > }
> >
> > -void display_channel_process_draw(DisplayChannel *display,
> > RedDrawable *red_drawable,
> > - uint32_t
> > process_commands_generation)
> > +/* Check that a given drawable it's a simple copy that can be
>
> - it's -> is
> - remove the extra 'be' at the end of the line above
>
> > + * possibly be joined to next one.
> > + * This is used to undo some engine which split images into
> > + * chunks causing more commands and creating multiple streams.
> > + * One example is RHEL 7.
>
> maybe reword this:
>
> "This is used to work around some drivers which split larger image
> updates into smaller chunks. These chunks are generally horizontal
> strips. This can cause our stream-detection heuristics to generate
> multiple streams instead of a single stream, and results in more
> commands being sent to the client."
>
> > + */
> > +static gboolean red_drawable_joinable(const RedDrawable *drawable)
> > +{
> > + /* these 3 initial tests are arranged to minimize checks as
> > + * they are in reverse order of occurrences */
>
> "reverse order of occurrence" implies to me that the first one is the
> least likely to occur. Is that what you intended? I think it should be
> the opposite, right?
>
> (also: occurrences should just be occurrence (singular))
>
First is more probable.
I probably needs some help with the comment. Maybe:
"these 3 initial tests are arranged to minimize checks as
they are arranged from more probable to less probable."
> > + if (drawable->clip.type != SPICE_CLIP_TYPE_NONE) {
> > + return FALSE;
> > + }
> > + if (drawable->type != QXL_DRAW_COPY) {
> > + return FALSE;
> > + }
> > + if (drawable->effect != QXL_EFFECT_OPAQUE) {
> > + return FALSE;
> > + }
> > + if (drawable->self_bitmap) {
> > + return FALSE;
> > + }
> > + if (drawable->surface_deps[0] != -1 ||
> > + drawable->surface_deps[1] != -1 ||
> > + drawable->surface_deps[2] != -1) {
> > + return FALSE;
> > + }
> > +
> > + const SpiceCopy *copy = &drawable->u.copy;
> > + if (copy->src_bitmap == NULL) {
> > + return FALSE;
> > + }
> > + if (copy->rop_descriptor != SPICE_ROPD_OP_PUT) {
> > + return FALSE;
> > + }
> > + if (copy->mask.bitmap != NULL) {
> > + return FALSE;
> > + }
> > +
> > + const SpiceImage *image = copy->src_bitmap;
> > + if (image->descriptor.type != SPICE_IMAGE_TYPE_BITMAP) {
> > + return FALSE;
> > + }
> > + const SpiceBitmap *bitmap = &image->u.bitmap;
> > + if (bitmap->format != SPICE_BITMAP_FMT_RGBA && bitmap->format !=
> > SPICE_BITMAP_FMT_32BIT) {
> > + return FALSE;
> > + }
> > + if (bitmap->flags != SPICE_BITMAP_FLAGS_TOP_DOWN) {
> > + return FALSE;
> > + }
> > + if (bitmap->palette != NULL) {
> > + return FALSE;
> > + }
> > + if (bitmap->data == NULL) {
> > + return FALSE;
> > + }
> > + if (bitmap->data->flags & SPICE_CHUNKS_FLAGS_UNSTABLE) {
> > + return FALSE;
> > + }
> > + if (bitmap->x != image->descriptor.width ||
> > + bitmap->y != image->descriptor.height) {
> > + return FALSE;
> > + }
> > + /* area should specify all image */
> > + if (copy->src_area.left != 0 || copy->src_area.top != 0 ||
> > + copy->src_area.right != bitmap->x || copy->src_area.bottom
> > != bitmap->y) {
> > + return FALSE;
> > + }
> > + if (drawable->bbox.right - drawable->bbox.left != bitmap->x ||
> > + drawable->bbox.bottom - drawable->bbox.top != bitmap->y) {
> > + return FALSE;
> > + }
>
> It's not really clear to me why all of these checks are necessary, or
> whether there are any checks missing. How did you decide which checks
> were required? Can any comments be added?
>
Yes
>
> > +
> > + return TRUE;
> > +}
> > +
> > +static gboolean red_drawable_can_join(const RedDrawable *first,
> > const RedDrawable *second)
> > +{
> > + if (!red_drawable_joinable(first) ||
> > !red_drawable_joinable(second)) {
> > + return FALSE;
> > + }
> > + if (first->surface_id != second->surface_id) {
> > + return FALSE;
> > + }
> > + if (first->u.copy.src_bitmap->u.bitmap.format != second-
> > >u.copy.src_bitmap->u.bitmap.format) {
> > + return FALSE;
> > + }
> > + // they must have the same width
> > + if (first->u.copy.src_bitmap->u.bitmap.x != second-
> > >u.copy.src_bitmap->u.bitmap.x ||
> > + first->u.copy.src_bitmap->u.bitmap.stride != second-
> > >u.copy.src_bitmap->u.bitmap.stride) {
> > + return FALSE;
> > + }
> > + // check that second is exactly under the first
> > + if (first->bbox.left != second->bbox.left) {
> > + return FALSE;
> > + }
> > + if (first->bbox.bottom != second->bbox.top) {
> > + return FALSE;
> > + }
> > + // check we can join the chunks
> > + if (first->u.copy.src_bitmap->u.bitmap.data->flags !=
> > + second->u.copy.src_bitmap->u.bitmap.data->flags) {
> > + return FALSE;
> > + }
> > + return TRUE;
> > +}
> > +
>
> I think that the following function deserves a few comments because
> it's a pretty easy function to misuse. First of all, the returned
> object will contain references to data held by the two arguments, so
> care must be taken to avoid double-freeing that data. Also, I think
> it's useful to indicate in a comment that a return value is newly-
> allocated and must be freed, and that the data from 'second' will be
> appended to 'first'.
> > +static SpiceChunks *chunks_join(const SpiceChunks *first, const
> > SpiceChunks *second)
> > +{
> > + // TODO use realloc to optimize
> > + SpiceChunks *new_chunks = spice_chunks_new(first->num_chunks +
> > second->num_chunks);
> > + new_chunks->flags = first->flags;
> > + new_chunks->data_size = first->data_size + second->data_size;
> > + memcpy(new_chunks->chunk, first->chunk, sizeof(first->chunk[0])
> > * first->num_chunks);
> > + memcpy(new_chunks->chunk + first->num_chunks, second->chunk,
> > sizeof(second->chunk[0]) * second->num_chunks);
> > + return new_chunks;
> > +}
> > +
>
>
> This function also could use some basic documentation. It doesn't
> allocate and return a new drawable like the previous _join() function.
> Rather it copies all of the data into the second one, links them
> together, and then returns the second one.
> > +static RedDrawable *red_drawable_join(RedDrawable *first,
> > RedDrawable *second)
> > +{
> > + uint32_t first_height = first->u.copy.src_bitmap->u.bitmap.y;
> > + second->u.copy.src_bitmap->descriptor.flags &=
> > ~SPICE_IMAGE_FLAGS_CACHE_ME;
> > + second->bbox.top = first->bbox.top;
> > + second->u.copy.src_bitmap->descriptor.height += first_height;
> > + second->u.copy.src_bitmap->u.bitmap.y += first_height;
> > + second->u.copy.src_area.bottom += first_height;
> > + // join chunks
> > + SpiceChunks *new_chunks = chunks_join(first->u.copy.src_bitmap-
> > >u.bitmap.data, second->u.copy.src_bitmap->u.bitmap.data);
> > + // prevent to free chunks copied in new structure
> > + second->u.copy.src_bitmap->u.bitmap.data->num_chunks = 0;
> > + first->u.copy.src_bitmap->u.bitmap.data->num_chunks = 0;
> > + // replace second one
> > + spice_chunks_destroy(second->u.copy.src_bitmap->u.bitmap.data);
> > + second->u.copy.src_bitmap->u.bitmap.data = new_chunks;
> > + second->joined = first;
>
> So this stores a reference from the most recent drawable to the (empty)
> drawable that it was just joined with. That one may have links to
> earlier (empty) joined drawables. This link is necessary only to be
> able to free that drawable later. I suppose it's not possible to simply
> free it now because that would release some resources within the driver
> and cause problems?
>
> > + return second;
> > +}
> > +
> > +static void
> > +display_channel_process_draw_single(DisplayChannel *display,
> > RedDrawable *red_drawable,
> > + uint32_t
> > process_commands_generation)
> > {
> > Drawable *drawable =
> > display_channel_get_drawable(display, red_drawable->effect,
> > red_drawable,
> > @@ -1191,6 +1330,36 @@ void
> > display_channel_process_draw(DisplayChannel *display, RedDrawable
> > *red_draw
> > drawable_unref(drawable);
> > }
> >
> > +void display_channel_process_draw(DisplayChannel *display,
> > RedDrawable *red_drawable,
> > + uint32_t
> > process_commands_generation)
> > +{
> > + if (!red_drawable_joinable(red_drawable)) {
> > + // not joinable, process all
> > + if (display->priv->joinable_drawable) {
> > + display_channel_process_draw_single(display, display-
> > >priv->joinable_drawable,
> > + process_commands_gen
> > eration);
>
> Isn't it possible that the earlier invocation of
> display_channel_proces_draw() (when we saved priv->joinable_drawable)
> may have used a different value for process_commands_generation? Should
> that value be saved with the joinable_drawable and passed to this
> function call? Or are you certain that it's OK to use the current value
> here?
>
> > + red_drawable_unref(display->priv->joinable_drawable);
> > + display->priv->joinable_drawable = NULL;
> > + }
> > + display_channel_process_draw_single(display, red_drawable,
> > process_commands_generation);
> > + return;
> > + }
> > +
>
> add comment:
> // drawable is joinable
> > + if (display->priv->joinable_drawable == NULL) {
> > + // try to join with next one
> > + } else if (red_drawable_can_join(display->priv-
> > >joinable_drawable, red_drawable)) {
> > + red_drawable = red_drawable_join(display->priv-
> > >joinable_drawable, red_drawable);
> > + } else {
> > + // they can't be joined
> > + display_channel_process_draw_single(display, display->priv-
> > >joinable_drawable,
> > + process_commands_generat
> > ion);
> > + red_drawable_unref(display->priv->joinable_drawable);
> > + }
> > + // try to join with next one
> > + red_drawable_ref(red_drawable);
> > + display->priv->joinable_drawable = red_drawable;
> > +}
> > +
>
> I find the above section a little bit confusing. I think it would be a
> bit easier to follow like this:
>
>
> if (display->priv->joinable_drawable != NULL) {
> if (red_drawable_can_join(...)) {
> red_drawable = red_drawable_join(...);
> } else {
> // the can't be joined
> display_channel_process_draw_single(...);
> red_drawable_unref(display->priv->joinable_drawable);
> }
> }
>
> // try to join with next one
> red_drawable_ref()
> display->priv->joinable_drawable = red_drawable;
>
>
>
> > int display_channel_wait_for_migrate_data(DisplayChannel *display)
> > {
> > uint64_t end_time = spice_get_monotonic_time_ns() +
> > DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT;
> > diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
> > index 86a2d93..56cc906 100644
> > --- a/server/red-parse-qxl.h
> > +++ b/server/red-parse-qxl.h
> > @@ -57,6 +57,7 @@ typedef struct RedDrawable {
> > SpiceWhiteness whiteness;
> > SpiceComposite composite;
> > } u;
> > + struct RedDrawable *joined;
> > } RedDrawable;
> >
> > static inline RedDrawable *red_drawable_ref(RedDrawable *drawable)
> > diff --git a/server/red-worker.c b/server/red-worker.c
> > index 8735cd1..b0d2955 100644
> > --- a/server/red-worker.c
> > +++ b/server/red-worker.c
> > @@ -94,12 +94,16 @@ static int display_is_connected(RedWorker
> > *worker)
> >
> > void red_drawable_unref(RedDrawable *red_drawable)
> > {
> > - if (--red_drawable->refs) {
> > - return;
> > + while (red_drawable) {
> > + if (--red_drawable->refs) {
> > + return;
> > + }
> > + red_qxl_release_resource(red_drawable->qxl, red_drawable-
> > >release_info_ext);
> > + red_put_drawable(red_drawable);
> > + RedDrawable *next = red_drawable->joined;
> > + free(red_drawable);
> > + red_drawable = next;
> > }
> > - red_qxl_release_resource(red_drawable->qxl, red_drawable-
> > >release_info_ext);
> > - red_put_drawable(red_drawable);
> > - free(red_drawable);
> > }
> >
> > static gboolean red_process_cursor_cmd(RedWorker *worker, const
> > QXLCommandExt *ext)
>
Thanks for reviewing this.
Frediano
More information about the Spice-devel
mailing list