Re: [Qemu-devel] [PATCH] DisplayState interface change

[Anthony Liguori <anthony@codemonkey.ws>]

Stefano Stabellini wrote:
> diff -r 5c78dd111aae console.h
> --- a/console.h	Tue Nov 04 09:04:41 2008 +0000
> +++ b/console.h	Mon Nov 17 12:15:40 2008 +0000
> @@ -73,45 +73,111 @@
>  
>  /* consoles */
>  
> -struct DisplayState {
> -    uint8_t *data;
> -    int linesize;
> -    int depth;
> -    int bgr; /* BGR color order instead of RGB. Only valid for depth == 32 */
> -    int width;
> -    int height;
> -    void *opaque;
> -    struct QEMUTimer *gui_timer;
> +struct DisplayChangeListener {
> +    int idle;
>      uint64_t gui_timer_interval;
> -    int idle; /* there is nothing to update (window invisible), set by vnc/sdl */
>  
>      void (*dpy_update)(struct DisplayState *s, int x, int y, int w, int h);
> -    void (*dpy_resize)(struct DisplayState *s, int w, int h);
> +    void (*dpy_resize)(struct DisplayState *s);
> +    void (*dpy_setdata)(struct DisplayState *s);
>      void (*dpy_refresh)(struct DisplayState *s);
>      void (*dpy_copy)(struct DisplayState *s, int src_x, int src_y,
>                       int dst_x, int dst_y, int w, int h);
>      void (*dpy_fill)(struct DisplayState *s, int x, int y,
>                       int w, int h, uint32_t c);
>      void (*dpy_text_cursor)(struct DisplayState *s, int x, int y);
> +
> +    struct DisplayChangeListener *next;
> +};
> +
> +struct DisplayState {
> +    uint8_t *data;
> +    int shared_buf;
> +    int linesize;
> +    int depth;
> +    int width;
> +    int height;
> +    void *opaque;
> +    struct QEMUTimer *gui_timer;
> +
> +    struct DisplayChangeListener* listeners;
> +
>      void (*mouse_set)(int x, int y, int on);
>      void (*cursor_define)(int width, int height, int bpp, int hot_x, int hot_y,
>                            uint8_t *image, uint8_t *mask);
> +
> +    void (*data_resize)(struct DisplayState *s);
>  }

If we're going to change DisplayState, and I think it's long over due 
that we do, then I think we should try to make sure we get it right.

What I would like to see is the buffer information extracted into a 
separate structure.  That buffer should also describe the pixel format 
in a much more thorough way.  There should be, in the very least, an 
endianness flag, information about the per-color shift and per-color 
mask, the bits per pixel, the bytes per pixel, the width and the height.

We should also have generic conversion functions to convert from one 
buffer to another.  This would allow us to simplify a lot of things 
(like screen shot).  It also ties in better to other display tool kits.  
Right now we have too many assumptions about what the pixel layout is 
from the depth.

The other thing to consider is that we do need some level of 
bidirectional communication.  vmware VGA supports the ability to tell 
the guest what resolution/depth it ought to be using.  You really want 
to get that information from SDL (at least the depth bits) so that we 
can avoid blitting.  With KVM, we can pretty trivially directly map the 
SDL buffer directly into the guest's physical memory provided it uses 
the right format.

Regards,

Anthony Liguori