From mboxrd@z Thu Jan 1 00:00:00 1970 From: Markus Armbruster Subject: Re: [RFC] PVFB: Add refresh period to XenStore parameters? Date: Fri, 09 May 2008 10:43:15 +0200 Message-ID: <87hcd7c1gc.fsf@pike.pond.sub.org> References: <20080505091808.GC4497@implementation.uk.xensource.com> <87ej8hysv6.fsf@pike.pond.sub.org> <20080505165008.GP4497@implementation.uk.xensource.com> <87ve1rv8xb.fsf@pike.pond.sub.org> <20080506163255.GP4430@implementation.uk.xensource.com> <87lk2ntm0z.fsf@pike.pond.sub.org> <20080506172959.GR4430@implementation.uk.xensource.com> <871w4emaxx.fsf@pike.pond.sub.org> <20080507145426.GL4562@implementation.uk.xensource.com> <877ie5i4n7.fsf@pike.pond.sub.org> <20080508150146.GO4365@implementation.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <20080508150146.GO4365@implementation.uk.xensource.com> (Samuel Thibault's message of "Thu\, 8 May 2008 16\:01\:46 +0100") List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Samuel Thibault Cc: xen-devel@lists.xensource.com List-Id: xen-devel@lists.xenproject.org Samuel Thibault writes: > Hello, > > Markus Armbruster, le Thu 08 May 2008 10:25:32 +0200, a =C3=A9crit : >> 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 > [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 */ > }; >=20=20 > @@ -536,6 +537,41 @@ static void xenfb_on_fb_event(struct xen > xc_evtchn_notify(xenfb->evt_xch, xenfb->fb.port); > } >=20=20 > +static int xenfb_queue_full(struct xenfb *xenfb) > +{ > + struct xenfb_page *page =3D xenfb->fb.page; > + uint32_t cons, prod; > + > + prod =3D page->in_prod; > + cons =3D page->in_cons; > + return prod - cons =3D=3D XENFB_IN_RING_LEN; > +} > + > +static void xenfb_send_event(struct xenfb *xenfb, union xenfb_in_event *= event) > +{ > + uint32_t prod; > + struct xenfb_page *page =3D xenfb->fb.page; > + > + prod =3D page->in_prod; > + /* caller ensures !xenfb_queue_full() */ > + xen_mb(); /* ensure ring space available */ > + XENFB_IN_RING_REF(page, prod) =3D *event; > + xen_wmb(); /* ensure ring contents visible */ > + page->in_prod =3D 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 =3D XENFB_TYPE_REFRESH_PERIOD; > + event.refresh_period.period =3D period; > + xenfb_send_event(xenfb, &event); > +} > + > static void xenfb_on_kbd_event(struct xenfb *xenfb) > { > struct xenkbd_page *page =3D xenfb->kbd.page; > @@ -707,6 +743,7 @@ static int xenfb_read_frontend_fb_config > xenfb->protocol) < 0) > xenfb->protocol[0] =3D '\0'; > xenfb_xs_printf(xenfb->xsh, xenfb->fb.nodename, "request-update"= , "1"); > + xenfb->refresh_period =3D -1; >=20=20 > /* TODO check for permitted ranges */ > fb_page =3D xenfb->fb.page; > @@ -1185,10 +1222,28 @@ static void xenfb_guest_copy(struct xenf > dpy_update(xenfb->ds, x, y, w, h); > } >=20=20 > -/* Periodic update of display, no need for any in our case */ > +/* Periodic update of display, transmit the refresh interval to the fron= tend */ > static void xenfb_update(void *opaque) > { > struct xenfb *xenfb =3D opaque; > + int period; > + > + if (xenfb_queue_full(xenfb)) > + return; > + > + if (xenfb->ds->idle) > + period =3D 0; > + else { > + period =3D xenfb->ds->gui_timer_interval; > + if (!period) > + period =3D 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 !=3D period) { > + xenfb_send_refresh_period(xenfb, period); > + xenfb->refresh_period =3D period; > + } > } >=20=20 > /* 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) */ >=20=20 > /* > - * 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 shou= ld 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 nee= ded */ > +}; >=20=20 > #define XENFB_IN_EVENT_SIZE 40 >=20=20 > 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]; > }; >=20=20