WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: Samuel Thibault <samuel.thibault@xxxxxxxxxxxxx>
Subject: Re: [Xen-devel] [RFC] PVFB: Add refresh period to XenStore parameters?
From: Markus Armbruster <armbru@xxxxxxxxxx>
Date: Fri, 09 May 2008 10:43:15 +0200
Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
Delivery-date: Fri, 09 May 2008 01:43:41 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <20080508150146.GO4365@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> (Samuel Thibault's message of "Thu\, 8 May 2008 16\:01\:46 +0100")
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <20080505091808.GC4497@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> <87ej8hysv6.fsf@xxxxxxxxxxxxxxxxx> <20080505165008.GP4497@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> <87ve1rv8xb.fsf@xxxxxxxxxxxxxxxxx> <20080506163255.GP4430@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> <87lk2ntm0z.fsf@xxxxxxxxxxxxxxxxx> <20080506172959.GR4430@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> <871w4emaxx.fsf@xxxxxxxxxxxxxxxxx> <20080507145426.GL4562@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> <877ie5i4n7.fsf@xxxxxxxxxxxxxxxxx> <20080508150146.GO4365@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Gnus/5.11 (Gnus v5.11) Emacs/22.1 (gnu/linux)
Samuel Thibault <samuel.thibault@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
>
[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];
>  };
>  

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel