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

[Christophe de Dinechin]

Hey Frediano,


Glad you found the bug, but a bit puzzled by the explanation. Three things confuse me:

- You seem to imply that self_bitmap and QXL_DRAW_COPY don’t make sense together, but it’s not clear from the description why not, and looking at the code, it was not entirely obvious to me. Naively, I’d think it might make sense to use image caching during copies, e.g. to repeat patterns. But I can’t say I really understand that whole “self_image” stuff.

- Why does it reduce network optimizations? Do you mean to say that it makes them less effective (but then I don’t understand why) or, more likely, that it causes a lot of additional traffic (but then I’m not sure why? Just sending unnecessary bitmaps, or something worse? A bit unclear to me.

- Based on not really understanding the above two, I don’t understand why the problem would be specific to QXL_DRAW_COPY (except if the Win10 driver only has a bug with that specific operation).


Thanks,
Christophe

> On 22 Jun 2018, at 18:19, Frediano Ziglio <fziglio at redhat.com> 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.


> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
> server/red-parse-qxl.c | 4 ++++
> 1 file changed, 4 insertions(+)
> 
> The only concern about this patch is that the flag could be reset
> from all calls to red_get_copy_ptr/red_get_blend_ptr
> 
> diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
> index 4a45c0f7..0bf022d5 100644
> --- a/server/red-parse-qxl.c
> +++ b/server/red-parse-qxl.c
> @@ -1051,6 +1051,10 @@ static bool red_get_native_drawable(RedMemSlotInfo *slots, int group_id,
>     case QXL_DRAW_BLEND:
>         return red_get_blend_ptr(slots, group_id, &red->u.blend, &qxl->u.blend, flags);
>     case QXL_DRAW_COPY:
> +        /* 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->self_bitmap = false;

>         return red_get_copy_ptr(slots, group_id, &red->u.copy, &qxl->u.copy, flags);
>     case QXL_COPY_BITS:
>         red_get_point_ptr(&red->u.copy_bits.src_pos, &qxl->u.copy_bits.src_pos);
> -- 
> 2.17.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel