Re: [Qemu-devel] [PATCH 1/4] qxl/spice-display: move pipe to ssd

[Hans de Goede]

Hi,

Looks good, except for one small issue, see my comment at
the end of the patch.

On 03/15/2011 09:17 PM, Alon Levy wrote:
> this moves the int pipe[2] and pthread_t main data from the
> PCIQXLDevice struct to the SimpleSpiceDisplay. This will let us
> reuse it in the next patch for both -spice with no -qxl usage and
> for vga mode from qxl.
> ---
>   hw/qxl.c           |   32 ++++++++++++++++++++------------
>   hw/qxl.h           |    4 ----
>   ui/spice-display.c |    1 +
>   ui/spice-display.h |   10 ++++++++++
>   4 files changed, 31 insertions(+), 16 deletions(-)
>
> diff --git a/hw/qxl.c b/hw/qxl.c
> index fe4212b..d1d3c9c 100644
> --- a/hw/qxl.c
> +++ b/hw/qxl.c
> @@ -1062,7 +1062,7 @@ static void pipe_read(void *opaque)
>       int len;
>
>       do {
> -        len = read(d->pipe[0],&dummy, sizeof(dummy));
> +        len = read(d->ssd.pipe[0],&dummy, sizeof(dummy));
>       } while (len == sizeof(dummy));
>       qxl_set_irq(d);
>   }
> @@ -1078,31 +1078,39 @@ static void qxl_send_events(PCIQXLDevice *d, uint32_t 
> events)
>       if ((old_pending&  le_events) == le_events) {
>           return;
>       }
> -    if (pthread_self() == d->main) {
> +    if (pthread_self() == d->ssd.main) {
> +        /* running in io_thread thread */
>           qxl_set_irq(d);
>       } else {
> -        if (write(d->pipe[1], d, 1) != 1) {
> +        if (write(d->ssd.pipe[1], d, 1) != 1) {
>               dprint(d, 1, "%s: write to pipe failed\n", __FUNCTION__);
>           }
>       }
>   }
>
> -static void init_pipe_signaling(PCIQXLDevice *d)
> +void qxl_create_vcpu_to_iothread_pipe(SimpleSpiceDisplay *ssd)
>   {
> -   if (pipe(d->pipe)<  0) {
> -       dprint(d, 1, "%s: pipe creation failed\n", __FUNCTION__);
> +   if (pipe(ssd->pipe)<  0) {
> +       fprintf(stderr, "%s: pipe creation failed\n", __FUNCTION__);
>          return;
>      }
> +
>   #ifdef CONFIG_IOTHREAD
> -   fcntl(d->pipe[0], F_SETFL, O_NONBLOCK);
> +   fcntl(ssd->pipe[0], F_SETFL, O_NONBLOCK);
>   #else
> -   fcntl(d->pipe[0], F_SETFL, O_NONBLOCK /* | O_ASYNC */);
> +   fcntl(ssd->pipe[0], F_SETFL, O_NONBLOCK /* | O_ASYNC */);
>   #endif
> -   fcntl(d->pipe[1], F_SETFL, O_NONBLOCK);
> -   fcntl(d->pipe[0], F_SETOWN, getpid());
> +   fcntl(ssd->pipe[1], F_SETFL, O_NONBLOCK);
>
> -   d->main = pthread_self();
> -   qemu_set_fd_handler(d->pipe[0], pipe_read, NULL, d);
> +   fcntl(ssd->pipe[0], F_SETOWN, getpid());
> +
> +   qemu_set_fd_handler(ssd->pipe[0], pipe_read, NULL, ssd);
> +   ssd->main = pthread_self();
> +}
> +
> +static void init_pipe_signaling(PCIQXLDevice *d)
> +{
> +   qxl_create_vcpu_to_iothread_pipe(&d->ssd);
>   }
>
>   /* graphics console */
> diff --git a/hw/qxl.h b/hw/qxl.h
> index f6c450d..701245f 100644
> --- a/hw/qxl.h
> +++ b/hw/qxl.h
> @@ -55,10 +55,6 @@ typedef struct PCIQXLDevice {
>       } guest_surfaces;
>       QXLPHYSICAL        guest_cursor;
>
> -    /* thread signaling */
> -    pthread_t          main;
> -    int                pipe[2];
> -
>       /* ram pci bar */
>       QXLRam             *ram;
>       VGACommonState     vga;
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 020b423..e0ac616 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -406,6 +406,7 @@ void qemu_spice_display_init(DisplayState *ds)
>       qemu_spice_add_interface(&sdpy.qxl.base);
>       assert(sdpy.worker);
>
> +    qxl_create_vcpu_to_iothread_pipe(&sdpy);
>       
> qemu_add_vm_change_state_handler(qemu_spice_vm_change_state_handler,&sdpy);
>       qemu_spice_create_host_memslot(&sdpy);
>       qemu_spice_create_host_primary(&sdpy);
> diff --git a/ui/spice-display.h b/ui/spice-display.h
> index aef0464..a708d59 100644
> --- a/ui/spice-display.h
> +++ b/ui/spice-display.h
> @@ -43,6 +43,11 @@ typedef struct SimpleSpiceDisplay {
>       QXLRect dirty;
>       int notify;
>       int running;
> +
> +    /* thread signaling - used both in qxl (in vga mode
> +     * and in native mode) and without qxl */
> +    pthread_t          main;
> +    int                pipe[2];     /* to iothread */
>   } SimpleSpiceDisplay;
>
>   typedef struct SimpleSpiceUpdate {
> @@ -66,3 +71,8 @@ void qemu_spice_display_update(SimpleSpiceDisplay *ssd,
>                                  int x, int y, int w, int h);
>   void qemu_spice_display_resize(SimpleSpiceDisplay *ssd);
>   void qemu_spice_display_refresh(SimpleSpiceDisplay *ssd);
> +/* shared with qxl.c in vga mode and ui/spice-display (no qxl mode) */
> +int qxl_vga_mode_get_command(
> +    SimpleSpiceDisplay *ssd, struct QXLCommandExt *ext);

This function does not get defined until the next patch, so the adding
of the prototype should be done there too.

> +/* used by both qxl and spice-display */
> +void qxl_create_vcpu_to_iothread_pipe(SimpleSpiceDisplay *ssd);

Regards,

Hans