[Spice-devel] [PATCH v2 1/2] display-channel: Join drawables to improve rhel7 behaviour
Jonathon Jongsma
jjongsma at redhat.com
Thu Mar 2 21:10:21 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))
> + 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?
> +
> + 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)
More information about the Spice-devel
mailing list