Re: [RFC] PVFB: Add refresh period to XenStore parameters?

[Markus Armbruster <armbru@redhat.com>]

Samuel Thibault <samuel.thibault@eu.citrix.com> writes:

> Hello,
>
> Markus Armbruster, le Thu 08 May 2008 10:25:32 +0200, a écrit :
>> Strictly speaking, frame buffer update and update notification events
>> are separate things.
>
> Right, let's call the first one "refresh".
>
>> What you seem to need is *not* a way to control *notifications*, but a
>> way to control *updates*.  Because your shared framebuffer isn't
>> really a framebuffer, but some shadow of the real framebuffer.
>> Correct?
>
> Yes.  Here is an updated patch.

Looks like we're down to clarifying comments.  See inline.

> pvfb/ioemu: transmit refresh interval advice from backend to frontend
> which permits the frontend to avoid useless polls.
>
> Signed-off-by: Samuel Thibault <samuel.thibault@eu.citrix.com>
>
[diff extras/mini-os/... I got nothing useful to say about that...]
> diff -r b0d7780794eb tools/ioemu/hw/xenfb.c
> --- a/tools/ioemu/hw/xenfb.c	Thu May 08 13:40:40 2008 +0100
> +++ b/tools/ioemu/hw/xenfb.c	Thu May 08 15:47:13 2008 +0100
> @@ -59,6 +59,7 @@ struct xenfb {
>  	int offset;             /* offset of the framebuffer */
>  	int abs_pointer_wanted; /* Whether guest supports absolute pointer */
>  	int button_state;       /* Last seen pointer button state */
> +	int refresh_period;     /* The refresh period we have advised */
>  	char protocol[64];	/* frontend protocol */
>  };
>  
> @@ -536,6 +537,41 @@ static void xenfb_on_fb_event(struct xen
>  	xc_evtchn_notify(xenfb->evt_xch, xenfb->fb.port);
>  }
>  
> +static int xenfb_queue_full(struct xenfb *xenfb)
> +{
> +	struct xenfb_page *page = xenfb->fb.page;
> +	uint32_t cons, prod;
> +
> +	prod = page->in_prod;
> +	cons = page->in_cons;
> +	return prod - cons == XENFB_IN_RING_LEN;
> +}
> +
> +static void xenfb_send_event(struct xenfb *xenfb, union xenfb_in_event *event)
> +{
> +	uint32_t prod;
> +	struct xenfb_page *page = xenfb->fb.page;
> +
> +	prod = page->in_prod;
> +	/* caller ensures !xenfb_queue_full() */
> +	xen_mb();                   /* ensure ring space available */
> +	XENFB_IN_RING_REF(page, prod) = *event;
> +	xen_wmb();                  /* ensure ring contents visible */
> +	page->in_prod = prod + 1;
> +
> +	xc_evtchn_notify(xenfb->evt_xch, xenfb->fb.port);
> +}
> +
> +static void xenfb_send_refresh_period(struct xenfb *xenfb, int period)
> +{
> +	union xenfb_in_event event;
> +
> +	memset(&event, 0, sizeof(event));
> +	event.type = XENFB_TYPE_REFRESH_PERIOD;
> +	event.refresh_period.period = period;
> +	xenfb_send_event(xenfb, &event);
> +}
> +
>  static void xenfb_on_kbd_event(struct xenfb *xenfb)
>  {
>  	struct xenkbd_page *page = xenfb->kbd.page;
> @@ -707,6 +743,7 @@ static int xenfb_read_frontend_fb_config
>                              xenfb->protocol) < 0)
>                  xenfb->protocol[0] = '\0';
>          xenfb_xs_printf(xenfb->xsh, xenfb->fb.nodename, "request-update", "1");
> +        xenfb->refresh_period = -1;
>  
>          /* TODO check for permitted ranges */
>          fb_page = xenfb->fb.page;
> @@ -1185,10 +1222,28 @@ static void xenfb_guest_copy(struct xenf
>      dpy_update(xenfb->ds, x, y, w, h);
>  }
>  
> -/* Periodic update of display, no need for any in our case */
> +/* Periodic update of display, transmit the refresh interval to the frontend */
>  static void xenfb_update(void *opaque)
>  {
>      struct xenfb *xenfb = opaque;
> +    int period;
> +
> +    if (xenfb_queue_full(xenfb))
> +        return;
> +
> +    if (xenfb->ds->idle)
> +        period = 0;
> +    else {
> +        period = xenfb->ds->gui_timer_interval;
> +        if (!period)
> +            period = GUI_REFRESH_INTERVAL;
> +    }
> +
> +    /* Will have to be disabled for frontends without feature-update */

I think I asked you to put this comment here, but is it still correct
for current XENFB_TYPE_REFRESH_PERIOD semantics?

> +    if (xenfb->refresh_period != period) {
> +        xenfb_send_refresh_period(xenfb, period);
> +        xenfb->refresh_period = period;
> +    }
>  }
>  
>  /* QEMU display state changed, so refresh the framebuffer copy */
[CONFIG_STUBDOM stuff snipped, I'm too ignorant about that to comment...]
> diff -r b0d7780794eb tools/ioemu/sdl.c
[more snippage due to ignorance...]
> diff -r b0d7780794eb tools/ioemu/vl.c
[ditto...]
> diff -r b0d7780794eb tools/ioemu/vl.h
[ditto...]
> diff -r b0d7780794eb tools/ioemu/vnc.c
[ditto...]
> diff -r b0d7780794eb xen/include/public/io/fbif.h
> --- a/xen/include/public/io/fbif.h	Thu May 08 13:40:40 2008 +0100
> +++ b/xen/include/public/io/fbif.h	Thu May 08 15:47:13 2008 +0100
> @@ -79,15 +79,27 @@ union xenfb_out_event
>  /* In events (backend -> frontend) */
>  
>  /*
> - * Frontends should ignore unknown in events.
> - * No in events currently defined.
> + * Framebuffer refresh period advice
> + * Backend sends it to advise the frontend the period of refresh it should use,
> + * i.e how often it should take care to update the FB with its internal state.
> + *
> + * If the frontend uses the advice, it should refresh and send an update event
> + * in response to this event.
>   */

You delete the bit about ignoring unknown events.  Oops.

What about this:

/* In events (backend -> frontend) */

/*
 * Frontends should ignore unknown in events.
 */

/*
 * Framebuffer refresh period advice
 * Backend sends it to advise the frontend their preferred period of
 * refresh.  Frontends that keep the framebuffer constantly up-to-date
 * just ignore it.  Frontends that use the advice should immediately
 * refresh the framebuffer (and send an update notification event if
 * those have been requested), then use the update frequency to guide
 * their periodical refreshs.
 */
#define XENFB_TYPE_REFRESH_PERIOD 1

> +#define XENFB_TYPE_REFRESH_PERIOD 1
> +
> +struct xenfb_refresh_period
> +{
> +    uint8_t type;    /* XENFB_TYPE_UPDATE_PERIOD */
> +    uint32_t period; /* period of refresh, in ms, 0 if no refresh is needed */
> +};
>  
>  #define XENFB_IN_EVENT_SIZE 40
>  
>  union xenfb_in_event
>  {
>      uint8_t type;
> +    struct xenfb_refresh_period refresh_period;

Time unit?  Needs a comment!  Make sure to explain the special value
for "no updates please".

I'd be tempted to use a frequency instead of a period, just because
that doesn't require a special value for "no updates".  Strictly a
matter of taste.

>      char pad[XENFB_IN_EVENT_SIZE];
>  };
>