|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC] PVFB: Add refresh period to XenStore parameters?
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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |