[Spice-devel] [PATCH spice-server v2] red-parse-qxl: Avoid invalid flag usage

[Christophe Fergeau]

On Mon, Jun 25, 2018 at 10:50:27AM +0100, Frediano Ziglio wrote:
> self_bitmap flag is used for some complex drawing not possible
> by QXL_DRAW_COPY commands. Having this flag set causes
> spice-server do draw part of the screen, copy that part on new
> allocated image and reduce network optimisations with no visual
> changes.
> Some drivers (like Windows 10 DOD) set this flag by mistake for
> this command so reset it.
> 
> More details follow.
> 
> The self_bitmap flag is used for some drawing command requiring to mix
> the frame buffer with some other image. For this specific
> QXL_DRAW_COPY command self_bitmap is used by spice-server code during
> cachine/sending (the reason for the cache is to cache images sent to
> client so the relationship between the two parts of the code).
> However the self_bitmap_image (an image created in spice-server if
> this flags is set) is used only if src_bitmap of SpiceCopy structure
> (the structure used to store the QXL_DRAW_COPY command inside
> spice-server) is NULL. But in red_get_copy_ptr (red-parse-qxl.c, the
> function that parse the QXL_DRAW_COPY command form the QXL device)
> not having a src_bitmap is considered an error so the
> self_bitmap_image won't be used.
> 
> Why this flag affects network performance?
> When spice-server see this flag it update the frame buffer according
> to the pending commands (commands to be sent or still to be drawn on
> frame buffer). spice-server maintain a tree of commands used to reduce
> rendering and command to send. More or less if a command is covering
> other commands (for instance filling the entire screen with a single
> color) the pending commands can be removed from the queue and not sent
> to the client. However when an update of the frame buffer is requested
> spice-server update the frame buffer removing the commands from the
> tree but not from the client queue.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/red-parse-qxl.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> Changes since v1:
> - handle all QXL_DRAW_COPY paths parsing the command;
> - more long explanation.
> 
> diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
> index 4a45c0f7..ca9b27a3 100644
> --- a/server/red-parse-qxl.c
> +++ b/server/red-parse-qxl.c
> @@ -677,8 +677,15 @@ static void red_put_opaque(SpiceOpaque *red)
>  }
>  
>  static bool red_get_copy_ptr(RedMemSlotInfo *slots, int group_id,
> -                             SpiceCopy *red, QXLCopy *qxl, uint32_t flags)
> +                             RedDrawable *red_drawable, QXLCopy *qxl, uint32_t flags)
>  {
> +    /* there's no sense to have this true, this will just waste CPU and reduce optimizations
> +     * for this command. Due to some bugs however some driver set self_bitmap field for this
> +     * command so reset it. */
> +    red_drawable->self_bitmap = false;

Looks to me, ideally we could warn once when this is unexpectedly set,
but we are missing helpers for that at the moment.

Looks good otherwise, 
Acked-by: Christophe Fergeau <cfergeau at redhat.com>

Christophe

> +
> +    SpiceCopy *red = &red_drawable->u.copy;
> +
>      red->src_bitmap      = red_get_image(slots, group_id, qxl->src_bitmap, flags, false);
>      if (!red->src_bitmap) {
>          return false;
> @@ -1049,9 +1056,9 @@ static bool red_get_native_drawable(RedMemSlotInfo *slots, int group_id,
>                                &red->u.blackness, &qxl->u.blackness, flags);
>          break;
>      case QXL_DRAW_BLEND:
> -        return red_get_blend_ptr(slots, group_id, &red->u.blend, &qxl->u.blend, flags);
> +        return red_get_blend_ptr(slots, group_id, red, &qxl->u.blend, flags);
>      case QXL_DRAW_COPY:
> -        return red_get_copy_ptr(slots, group_id, &red->u.copy, &qxl->u.copy, flags);
> +        return red_get_copy_ptr(slots, group_id, red, &qxl->u.copy, flags);
>      case QXL_COPY_BITS:
>          red_get_point_ptr(&red->u.copy_bits.src_pos, &qxl->u.copy_bits.src_pos);
>          break;
> @@ -1128,9 +1135,9 @@ static bool red_get_compat_drawable(RedMemSlotInfo *slots, int group_id,
>                                &red->u.blackness, &qxl->u.blackness, flags);
>          break;
>      case QXL_DRAW_BLEND:
> -        return red_get_blend_ptr(slots, group_id, &red->u.blend, &qxl->u.blend, flags);
> +        return red_get_blend_ptr(slots, group_id, red, &qxl->u.blend, flags);
>      case QXL_DRAW_COPY:
> -        return red_get_copy_ptr(slots, group_id, &red->u.copy, &qxl->u.copy, flags);
> +        return red_get_copy_ptr(slots, group_id, red, &qxl->u.copy, flags);
>      case QXL_COPY_BITS:
>          red_get_point_ptr(&red->u.copy_bits.src_pos, &qxl->u.copy_bits.src_pos);
>          red->surface_deps[0] = 0;
> -- 
> 2.17.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180703/31ff1eee/attachment.sig>