[Spice-devel] [PATCH v3 1/2] display-channel: Join drawables to improve rhel7 behaviour
Christophe de Dinechin
cdupontd at redhat.com
Fri Mar 3 12:07:57 UTC 2017
> On 3 Mar 2017, at 12:46, Frediano Ziglio <fziglio at redhat.com> 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 | 220 +++++++++++++++++++++++++++++++-
> server/red-parse-qxl.h | 1 +-
> server/red-worker.c | 14 +-
> 4 files changed, 230 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 67a77ef..e95741f 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -78,6 +78,8 @@ display_channel_finalize(GObject *object)
> {
> DisplayChannel *self = DISPLAY_CHANNEL(object);
>
> + red_drawable_unref(self->priv->joinable_drawable);
> +
> display_channel_destroy_surfaces(self);
> image_cache_reset(&self->priv->image_cache);
> monitors_config_unref(self->priv->monitors_config);
> @@ -1176,8 +1178,190 @@ 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 is a simple copy that can
> + * possibly be joined to next one.
> + * 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.
> + * One example is RHEL 7.
> + * Some of the check in this function are just to check that the
> + * operation is a opaque bitmap copy operations, some other just
> + * avoid to make the joining code more complicated just to support
> + * unseen commands.
> + */
> +static bool red_drawable_joinable(const RedDrawable *drawable)
> +{
> + // These 3 initial tests are arranged to minimize checks as
> + // they are arranged from more probable to less probable
Any reason not to group all the tests with || ?
> + 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;
> + }
> + // Never seen, avoid to support during join
> + if (drawable->self_bitmap) {
> + return false;
> + }
> + // Consistency, a copy should not have dependencies
> + if (drawable->surface_deps[0] != -1 ||
> + drawable->surface_deps[1] != -1 ||
> + drawable->surface_deps[2] != -1) {
> + return false;
> + }
> +
> + const SpiceCopy *copy = &drawable->u.copy;
> + // We need a bitmap
> + if (copy->src_bitmap == NULL) {
> + return false;
> + }
> + if (copy->rop_descriptor != SPICE_ROPD_OP_PUT) {
> + return false;
> + }
> + // Never seen, avoid to support during join
> + if (copy->mask.bitmap != NULL) {
> + return false;
> + }
> +
> + // Limit image support to 32bit bitmap
> + const SpiceImage *image = copy->src_bitmap;
For my education: I thought the style was to declare variables ahead of time (which I personally don’t like much). But I prefer the way you wrote it.
> + 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 you restrict to 32-bit bitmaps, it might be worth adding to the comment, and explaining why. Is it “never seen”, “can’t do” or “don’t want to do”?
> + // Never seen anything else, avoid to support during join.
> + // We could not easily join bitmap with different orientations
> + if (bitmap->flags != SPICE_BITMAP_FLAGS_TOP_DOWN) {
> + return false;
> + }
> + // Consistency, a 32bit image have no reason to have a palette
> + if (bitmap->palette != NULL) {
> + return false;
> + }
> + if (bitmap->data == NULL) {
> + return false;
> + }
> + // Never seen, avoid to support during join.
> + // Wouldn't be too hard to implement but looking at driver code this flag
> + // is not used so no worth implementing it.
> + if (bitmap->data->flags & SPICE_CHUNKS_FLAGS_UNSTABLE) {
> + return false;
> + }
> + // Consistency, the bitmap inside the image should fill the image
> + if (bitmap->x != image->descriptor.width ||
> + bitmap->y != image->descriptor.height) {
> + return false;
> + }
> + // Area should specify all image. If not we would have to
> + // adjust the memory chunks to take into account this or make other
> + // complicated cropping.
> + 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;
> + }
> +
> + return true;
> +}
> +
> +static bool 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;
> + }
> + // We can't join 2 different format
> + 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;
> +}
> +
> +/* Create a new set of chunks containing the chunks of first+second set.
> + * Note that the resulting chunks will point to original data so
> + * you should pay attention in order to avoid double free or other
> + * corruptions.
> + * For instance you could set to zero the number of chunks of first and second
> + * just after calling this function or free the two sets paying attention
> + * they don't release the data.
> + */
> +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;
> +}
> +
> +/* Join the first drawable with the second.
> + * The resulting drawable is returned.
> + * Once joined you should avoid to use first and second drawables.
> + */
> +static RedDrawable *red_drawable_join(RedDrawable *first, RedDrawable *second)
> +{
> + // Join drawing areas. The first is exactly on top of the 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 set of chunks with the sum computed above
> + spice_chunks_destroy(second->u.copy.src_bitmap->u.bitmap.data);
> + second->u.copy.src_bitmap->u.bitmap.data = new_chunks;
> +
> + // Create a chain of drawables. This is necessary to retain needed memory
> + // resources.
> + second->joined = first;
> +
> + 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,
> @@ -1192,6 +1376,38 @@ 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_generation);
> + red_drawable_unref(display->priv->joinable_drawable);
> + display->priv->joinable_drawable = NULL;
> + }
> + display_channel_process_draw_single(display, red_drawable, process_commands_generation);
> + return;
> + }
> + // Drawable is joinable
> +
> + if (display->priv->joinable_drawable != NULL) {
> + 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_generation);
> + red_drawable_unref(display->priv->joinable_drawable);
> + }
> + }
> +
> + // Try to join with next one
> + red_drawable_ref(red_drawable);
> + 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 c88034b..897566e 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)
> --
> git-series 0.9.1
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list