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

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

[Samuel Thibault]

Hello,

Sometimes the backend of PVFB knows that it doesn't need permanent
refresh, when the window is minimized for instance (no refresh at all),
or the administration tools know that the window is thumnailed, and so a
slow refresh rate is fine.  Also, some users may want to tune the
refresh rate according to the smoothness they would like, balanced with
the CPU time that requires.

I've played with that idea a bit and it seems to work fine, saving
computations and communications.  I'm now wondering about the interface:
it looks to me like it could be as simple as a "refresh-period" node in
the backend part of XenStore: the front-end would watch it, and update
the timing of its internal refresh loop, xenfb_fps in the case of Linux'
xenfb for instance.  A period of 0 would mean that no refresh is needed
(e.g. minimized window)

Samuel

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

[Samuel Thibault]

Hello,

There was no comment on my proposition below, should I consider that it
means people are fine with it? :)

Samuel

Samuel Thibault, le Fri 29 Feb 2008 12:08:07 +0000, a écrit :
> Sometimes the backend of PVFB knows that it doesn't need permanent
> refresh, when the window is minimized for instance (no refresh at all),
> or the administration tools know that the window is thumnailed, and so a
> slow refresh rate is fine.  Also, some users may want to tune the
> refresh rate according to the smoothness they would like, balanced with
> the CPU time that requires.
> 
> I've played with that idea a bit and it seems to work fine, saving
> computations and communications.  I'm now wondering about the interface:
> it looks to me like it could be as simple as a "refresh-period" node in
> the backend part of XenStore: the front-end would watch it, and update
> the timing of its internal refresh loop, xenfb_fps in the case of Linux'
> xenfb for instance.  A period of 0 would mean that no refresh is needed
> (e.g. minimized window)

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

[Markus Armbruster]

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

> Hello,
>
> Sometimes the backend of PVFB knows that it doesn't need permanent
> refresh, when the window is minimized for instance (no refresh at all),
> or the administration tools know that the window is thumnailed, and so a
> slow refresh rate is fine.  Also, some users may want to tune the
> refresh rate according to the smoothness they would like, balanced with
> the CPU time that requires.

Can you quantify the CPU time savings?  Are you sure they're worth the
extra complexity?

Are you sure the ability to control the rate is required?  Why isn't
it sufficient to be able to switch updates off?

> I've played with that idea a bit and it seems to work fine, saving
> computations and communications.  I'm now wondering about the interface:
> it looks to me like it could be as simple as a "refresh-period" node in
> the backend part of XenStore: the front-end would watch it, and update
> the timing of its internal refresh loop, xenfb_fps in the case of Linux'
> xenfb for instance.  A period of 0 would mean that no refresh is needed
> (e.g. minimized window)
>
> Samuel

Another option is to send a suitable message through the ring.  That's
how the dynamic mode patch (not yet merged) communicates resolution
change, albeit in the other direction.

The pvops PVFB uses fb_defio.  I think we can change the refresh
period there by changing xenfb_defio.delay, but that doesn't exactly
look like something the API wants us to do.

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

[Samuel Thibault]

Hello,

Markus Armbruster, le Mon 03 Mar 2008 19:03:46 +0100, a écrit :
> Samuel Thibault <samuel.thibault@eu.citrix.com> writes:
> > Sometimes the backend of PVFB knows that it doesn't need permanent
> > refresh, when the window is minimized for instance (no refresh at all),
> > or the administration tools know that the window is thumnailed, and so a
> > slow refresh rate is fine.  Also, some users may want to tune the
> > refresh rate according to the smoothness they would like, balanced with
> > the CPU time that requires.
> 
> Can you quantify the CPU time savings?

Something like 6% CPU on my test machine (by just slowing down from 30ms
to 1000ms interval).

In my case, I'm using PVFB to expose the stubdomain qemu display.  The
problem is that every 30ms, qemu wakes up to memcmp() the whole video
memory with a shadow buffer so as to track changes.  If it knew that the
window is minimized or reduced, it could stop or increase that polling
interval.  With SDL and vnc, it can, but when going through PVFB that
information is lost.

> Are you sure they're worth the extra complexity?

At least watching a simple integer in XenStore is not very complex.

Note that this may not be a requirement, just the backend telling the
frontend what he'd prefer to see.  If it's difficult for the frontend to
change the rate, then it can just ignore it, and the user won't be so
happy, that's all.

> Are you sure the ability to control the rate is required?  Why isn't
> it sufficient to be able to switch updates off?

Being able to choose the smoothness of the interface is really a good
user experience.  To my feeling, the current 30ms default rate of qemu
(7% CPU) is not so smooth (people don't use 30Hz monitors, to they? ;).
I usually prefer spending e.g. 14% CPU to get a 10ms rate, but of course
I don't want that CPU time to be used when the viewer is off screen.
Other people won't feel that need and can save CPU% by slowing it down.

Also, in other cases, you just need to have a snapshot of the VMs, so a
1s rate (or even 10s) makes sense.

> Another option is to send a suitable message through the ring.

Yes, but then it's hard for management tools (e.g. a gui that manages
VMs) to tune it, while a xenstore value is pretty easy to tinker with.

> The pvops PVFB uses fb_defio.  I think we can change the refresh
> period there by changing xenfb_defio.delay, but that doesn't exactly
> look like something the API wants us to do.

Then that frontend may just ignore the rate.  It's much less of a
concern, since that frontend doesn't use an active polling loop, and
thus consumes no CPU if nothing happens in the guest.

Samuel

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

[Trolle Selander]

[-- Attachment #1.1: Type: text/plain, Size: 4082 bytes --]

Turning off refresh on minimized windows certainly sounds worth it to me.
Let's not forget that even though it currently may save "only" 6% CPU, the
current VGA emulation only has 4 MB of video RAM. This will need to increase
in the future up to at least a factor of 8 if we want to be able to support
all resolutions that modern monitors can handle. I know there are more
efficient update code than a complete memcmp() in the works, and that will
probably be needed before we can do 2560x1600x32 double-buffered in guests
at any reasonable CPU usage, but with those in place, needless updates is
probably going to be a good thing to avoid.

Moreover, since I've been working with Xen in a client environment I know
that being able to set the refresh rate higher than 30 Hz would also be a
very welcome thing. The current unaccelerated VESA is actually surprisingly
fast, and I the 30 Hz refresh is probably  the main  cause of people
perceiving the client graphics as slow.

Just my 2 cents.

On Mon, Mar 3, 2008 at 8:18 PM, Samuel Thibault <
samuel.thibault@eu.citrix.com> wrote:

> Hello,
>
> Markus Armbruster, le Mon 03 Mar 2008 19:03:46 +0100, a écrit :
> > Samuel Thibault <samuel.thibault@eu.citrix.com> writes:
> > > Sometimes the backend of PVFB knows that it doesn't need permanent
> > > refresh, when the window is minimized for instance (no refresh at
> all),
> > > or the administration tools know that the window is thumnailed, and so
> a
> > > slow refresh rate is fine.  Also, some users may want to tune the
> > > refresh rate according to the smoothness they would like, balanced
> with
> > > the CPU time that requires.
> >
> > Can you quantify the CPU time savings?
>
> Something like 6% CPU on my test machine (by just slowing down from 30ms
> to 1000ms interval).
>
> In my case, I'm using PVFB to expose the stubdomain qemu display.  The
> problem is that every 30ms, qemu wakes up to memcmp() the whole video
> memory with a shadow buffer so as to track changes.  If it knew that the
> window is minimized or reduced, it could stop or increase that polling
> interval.  With SDL and vnc, it can, but when going through PVFB that
> information is lost.
>
> > Are you sure they're worth the extra complexity?
>
> At least watching a simple integer in XenStore is not very complex.
>
> Note that this may not be a requirement, just the backend telling the
> frontend what he'd prefer to see.  If it's difficult for the frontend to
> change the rate, then it can just ignore it, and the user won't be so
> happy, that's all.
>
> > Are you sure the ability to control the rate is required?  Why isn't
> > it sufficient to be able to switch updates off?
>
> Being able to choose the smoothness of the interface is really a good
> user experience.  To my feeling, the current 30ms default rate of qemu
> (7% CPU) is not so smooth (people don't use 30Hz monitors, to they? ;).
> I usually prefer spending e.g. 14% CPU to get a 10ms rate, but of course
> I don't want that CPU time to be used when the viewer is off screen.
> Other people won't feel that need and can save CPU% by slowing it down.
>
> Also, in other cases, you just need to have a snapshot of the VMs, so a
> 1s rate (or even 10s) makes sense.
>
> > Another option is to send a suitable message through the ring.
>
> Yes, but then it's hard for management tools (e.g. a gui that manages
> VMs) to tune it, while a xenstore value is pretty easy to tinker with.
>
> > The pvops PVFB uses fb_defio.  I think we can change the refresh
> > period there by changing xenfb_defio.delay, but that doesn't exactly
> > look like something the API wants us to do.
>
> Then that frontend may just ignore the rate.  It's much less of a
> concern, since that frontend doesn't use an active polling loop, and
> thus consumes no CPU if nothing happens in the guest.
>
> Samuel
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>

[-- Attachment #1.2: Type: text/html, Size: 5060 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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

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

[Markus Armbruster]

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

> Hello,
>
> Markus Armbruster, le Mon 03 Mar 2008 19:03:46 +0100, a écrit :
>> Samuel Thibault <samuel.thibault@eu.citrix.com> writes:
>> > Sometimes the backend of PVFB knows that it doesn't need permanent
>> > refresh, when the window is minimized for instance (no refresh at all),
>> > or the administration tools know that the window is thumnailed, and so a
>> > slow refresh rate is fine.  Also, some users may want to tune the
>> > refresh rate according to the smoothness they would like, balanced with
>> > the CPU time that requires.

I'm not sure I understand how this is supposed to work.  Commonly, my
display is somewhere else, and all the backend knows of it is a VNC
connection.  It has no idea what I do with my VNC viewer window.  Can
you explain?

>> Can you quantify the CPU time savings?
>
> Something like 6% CPU on my test machine (by just slowing down from 30ms
> to 1000ms interval).
>
> In my case, I'm using PVFB to expose the stubdomain qemu display.  The
> problem is that every 30ms, qemu wakes up to memcmp() the whole video
> memory with a shadow buffer so as to track changes.  If it knew that the

Wait a second!  You're talking about *your* frontend here, aren't you?

Maintaining a shadow copy in the frontend (which requires compare and
copy) to minimize copying in the backend sounds pretty self-defeating
to me.  Why is this a good idea?

Why not refrain from sending update messages and have the backend
simply redisplay everything?  tools/ioemu/hw/xenfb.c doesn't implement
that mode, but it shouldn't be hard.  And then you could control the
refresh rate solely in the backend, without having to communicate with
the frontend.

> window is minimized or reduced, it could stop or increase that polling
> interval.  With SDL and vnc, it can, but when going through PVFB that
> information is lost.
>
>> Are you sure they're worth the extra complexity?
>
> At least watching a simple integer in XenStore is not very complex.
>
> Note that this may not be a requirement, just the backend telling the
> frontend what he'd prefer to see.  If it's difficult for the frontend to
> change the rate, then it can just ignore it, and the user won't be so
> happy, that's all.

I'm concerned that we're papering over performance problems instead of
solving them.

>> Are you sure the ability to control the rate is required?  Why isn't
>> it sufficient to be able to switch updates off?
>
> Being able to choose the smoothness of the interface is really a good
> user experience.  To my feeling, the current 30ms default rate of qemu
> (7% CPU) is not so smooth (people don't use 30Hz monitors, to they? ;).
> I usually prefer spending e.g. 14% CPU to get a 10ms rate, but of course
> I don't want that CPU time to be used when the viewer is off screen.
> Other people won't feel that need and can save CPU% by slowing it down.
>
> Also, in other cases, you just need to have a snapshot of the VMs, so a
> 1s rate (or even 10s) makes sense.
>
>> Another option is to send a suitable message through the ring.
>
> Yes, but then it's hard for management tools (e.g. a gui that manages
> VMs) to tune it, while a xenstore value is pretty easy to tinker with.

Depends on who does the tinkering.  If its the backend, then nothing
is easier than sending a message through the ring.

On the other hand, if this is just a hack to make a particular
frontend less inefficient, then using XenStore at least keeps it out
of the PVFB protocol.

>> The pvops PVFB uses fb_defio.  I think we can change the refresh
>> period there by changing xenfb_defio.delay, but that doesn't exactly
>> look like something the API wants us to do.
>
> Then that frontend may just ignore the rate.  It's much less of a
> concern, since that frontend doesn't use an active polling loop, and
> thus consumes no CPU if nothing happens in the guest.
>
> Samuel

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

[Samuel Thibault]

Markus Armbruster, le Tue 04 Mar 2008 15:32:57 +0100, a écrit :
> Samuel Thibault <samuel.thibault@eu.citrix.com> writes:
> > Markus Armbruster, le Mon 03 Mar 2008 19:03:46 +0100, a écrit :
> >> Samuel Thibault <samuel.thibault@eu.citrix.com> writes:
> >> > Sometimes the backend of PVFB knows that it doesn't need permanent
> >> > refresh, when the window is minimized for instance (no refresh at all),
> >> > or the administration tools know that the window is thumnailed, and so a
> >> > slow refresh rate is fine.  Also, some users may want to tune the
> >> > refresh rate according to the smoothness they would like, balanced with
> >> > the CPU time that requires.
> 
> I'm not sure I understand how this is supposed to work.  Commonly, my
> display is somewhere else, and all the backend knows of it is a VNC
> connection

Ok.

> It has no idea what I do with my VNC viewer window.

There is already a heuristic in that case, see the backoff: label in
vnc.c.

> >> Can you quantify the CPU time savings?
> >
> > Something like 6% CPU on my test machine (by just slowing down from 30ms
> > to 1000ms interval).
> >
> > In my case, I'm using PVFB to expose the stubdomain qemu display.  The
> > problem is that every 30ms, qemu wakes up to memcmp() the whole video
> > memory with a shadow buffer so as to track changes.  If it knew that the
> 
> Wait a second!  You're talking about *your* frontend here, aren't you?
> 
> Maintaining a shadow copy in the frontend (which requires compare and
> copy) to minimize copying in the backend sounds pretty self-defeating
> to me.  Why is this a good idea?

Because in my case the domain is HVM, and I don't want to trap the
writes to the video RAM since that's awfully slow.

> Why not refrain from sending update messages and have the backend
> simply redisplay everything?

Because as explained above, I can't avoid a shadow copy.

> >> Are you sure they're worth the extra complexity?
> >
> > At least watching a simple integer in XenStore is not very complex.
> >
> > Note that this may not be a requirement, just the backend telling the
> > frontend what he'd prefer to see.  If it's difficult for the frontend to
> > change the rate, then it can just ignore it, and the user won't be so
> > happy, that's all.
> 
> I'm concerned that we're papering over performance problems instead of
> solving them.

Mmm, I'm afraid my english couldn't understand that sentence.

> > Yes, but then it's hard for management tools (e.g. a gui that manages
> > VMs) to tune it, while a xenstore value is pretty easy to tinker with.
> 
> Depends on who does the tinkering.  If its the backend, then nothing
> is easier than sending a message through the ring.
> 
> On the other hand, if this is just a hack to make a particular
> frontend less inefficient, then using XenStore at least keeps it out
> of the PVFB protocol.

But the backend still needs to notify the frontend when it should switch
modes.  Another way to go would be to only add to the PVFB protocol some
"expose on/off" events, and let the front-end decide of the rate to
adopt.

Samuel

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

[Samuel Thibault]

Samuel Thibault, le Tue 04 Mar 2008 14:49:52 +0000, a écrit :
> Markus Armbruster, le Tue 04 Mar 2008 15:32:57 +0100, a écrit :
> > Depends on who does the tinkering.  If its the backend, then nothing
> > is easier than sending a message through the ring.
> > 
> > On the other hand, if this is just a hack to make a particular
> > frontend less inefficient, then using XenStore at least keeps it out
> > of the PVFB protocol.
> 
> But the backend still needs to notify the frontend when it should switch
> modes.  Another way to go would be to only add to the PVFB protocol some
> "expose on/off" events, and let the front-end decide of the rate to
> adopt.

Mmm, that said, the backend remains the most close for the user to
select the refresh rate.

Samuel

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

[Markus Armbruster]

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

> Markus Armbruster, le Tue 04 Mar 2008 15:32:57 +0100, a écrit :
>> Samuel Thibault <samuel.thibault@eu.citrix.com> writes:
>> > Markus Armbruster, le Mon 03 Mar 2008 19:03:46 +0100, a écrit :
>> >> Samuel Thibault <samuel.thibault@eu.citrix.com> writes:
>> >> > Sometimes the backend of PVFB knows that it doesn't need permanent
>> >> > refresh, when the window is minimized for instance (no refresh at all),
>> >> > or the administration tools know that the window is thumnailed, and so a
>> >> > slow refresh rate is fine.  Also, some users may want to tune the
>> >> > refresh rate according to the smoothness they would like, balanced with
>> >> > the CPU time that requires.
>> 
>> I'm not sure I understand how this is supposed to work.  Commonly, my
>> display is somewhere else, and all the backend knows of it is a VNC
>> connection
>
> Ok.
>
>> It has no idea what I do with my VNC viewer window.
>
> There is already a heuristic in that case, see the backoff: label in
> vnc.c.
>
>> >> Can you quantify the CPU time savings?
>> >
>> > Something like 6% CPU on my test machine (by just slowing down from 30ms
>> > to 1000ms interval).
>> >
>> > In my case, I'm using PVFB to expose the stubdomain qemu display.  The
>> > problem is that every 30ms, qemu wakes up to memcmp() the whole video
>> > memory with a shadow buffer so as to track changes.  If it knew that the
>> 
>> Wait a second!  You're talking about *your* frontend here, aren't you?
>> 
>> Maintaining a shadow copy in the frontend (which requires compare and
>> copy) to minimize copying in the backend sounds pretty self-defeating
>> to me.  Why is this a good idea?
>
> Because in my case the domain is HVM, and I don't want to trap the
> writes to the video RAM since that's awfully slow.
>
>> Why not refrain from sending update messages and have the backend
>> simply redisplay everything?
>
> Because as explained above, I can't avoid a shadow copy.

I imagine (perhaps naively):

* The domU writes to a framebuffer provided by the frontend.

* The framebuffer (not a copy of it) can be shared with the backend,
  which only reads.

* If the frontend can track changes efficiently, it sends update
  messages to the backend, which can use them to only redisplay
  changed areas.

  Else the backend needs to assume the worst and periodically
  redisplay everything.  It is free to maintain a shadow copy to track
  changes and minimize redisplay, if that turns out to be faster.

What's wrong with that?

>> >> Are you sure they're worth the extra complexity?
>> >
>> > At least watching a simple integer in XenStore is not very complex.
>> >
>> > Note that this may not be a requirement, just the backend telling the
>> > frontend what he'd prefer to see.  If it's difficult for the frontend to
>> > change the rate, then it can just ignore it, and the user won't be so
>> > happy, that's all.
>> 
>> I'm concerned that we're papering over performance problems instead of
>> solving them.
>
> Mmm, I'm afraid my english couldn't understand that sentence.

Sorry about that, let me try again.  I'm concerned that we're working
around serious performance problems instead of solving them properly.

[...]

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

[Samuel Thibault]

Markus Armbruster, le Tue 04 Mar 2008 16:48:20 +0100, a écrit :
> Samuel Thibault <samuel.thibault@eu.citrix.com> writes:
> > Markus Armbruster, le Tue 04 Mar 2008 15:32:57 +0100, a écrit :
> >> > In my case, I'm using PVFB to expose the stubdomain qemu display.  The
> >> > problem is that every 30ms, qemu wakes up to memcmp() the whole video
> >> > memory with a shadow buffer so as to track changes.  If it knew that the
> >> 
> >> Wait a second!  You're talking about *your* frontend here, aren't you?
> >> 
> >> Maintaining a shadow copy in the frontend (which requires compare and
> >> copy) to minimize copying in the backend sounds pretty self-defeating
> >> to me.  Why is this a good idea?
> >
> > Because in my case the domain is HVM, and I don't want to trap the
> > writes to the video RAM since that's awfully slow.
> >
> >> Why not refrain from sending update messages and have the backend
> >> simply redisplay everything?
> >
> > Because as explained above, I can't avoid a shadow copy.
> 
> I imagine (perhaps naively):
> 
> * The domU writes to a framebuffer provided by the frontend.
>
> * The framebuffer (not a copy of it) can be shared with the backend,
>   which only reads.

Well, that's not always the case, when the guest is in text mode for
instance, the PV shared buffer is converted from the guest text buffer.

> * If the frontend can track changes efficiently, it sends update
>   messages to the backend, which can use them to only redisplay
>   changed areas.
> 
>   Else the backend needs to assume the worst and periodically
>   redisplay everything.  It is free to maintain a shadow copy to track
>   changes and minimize redisplay, if that turns out to be faster.
> 
> What's wrong with that?

Well, that was actually my next target :)
(but for that we badly need the resize/redepth support).

However, that doesn't solve something that still bugs me, which is
that the VGA emulation layer wakes up every 30ms to just notice that
the width/height/depth registers didn't change etc. even if the actual
window is not shown.

Samuel

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

[Markus Armbruster]

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

> Markus Armbruster, le Tue 04 Mar 2008 16:48:20 +0100, a écrit :
>> Samuel Thibault <samuel.thibault@eu.citrix.com> writes:
>> > Markus Armbruster, le Tue 04 Mar 2008 15:32:57 +0100, a écrit :
>> >> > In my case, I'm using PVFB to expose the stubdomain qemu display.  The
>> >> > problem is that every 30ms, qemu wakes up to memcmp() the whole video
>> >> > memory with a shadow buffer so as to track changes.  If it knew that the
>> >> 
>> >> Wait a second!  You're talking about *your* frontend here, aren't you?
>> >> 
>> >> Maintaining a shadow copy in the frontend (which requires compare and
>> >> copy) to minimize copying in the backend sounds pretty self-defeating
>> >> to me.  Why is this a good idea?
>> >
>> > Because in my case the domain is HVM, and I don't want to trap the
>> > writes to the video RAM since that's awfully slow.
>> >
>> >> Why not refrain from sending update messages and have the backend
>> >> simply redisplay everything?
>> >
>> > Because as explained above, I can't avoid a shadow copy.
>> 
>> I imagine (perhaps naively):
>> 
>> * The domU writes to a framebuffer provided by the frontend.
>>
>> * The framebuffer (not a copy of it) can be shared with the backend,
>>   which only reads.
>
> Well, that's not always the case, when the guest is in text mode for
> instance, the PV shared buffer is converted from the guest text buffer.

Optimize for the common case.  Which I figure is a 32 bpp framebuffer.

Anyway, compare and copy for some legacy 80x25 text mode shouldn't eat
that much CPU.

>> * If the frontend can track changes efficiently, it sends update
>>   messages to the backend, which can use them to only redisplay
>>   changed areas.
>> 
>>   Else the backend needs to assume the worst and periodically
>>   redisplay everything.  It is free to maintain a shadow copy to track
>>   changes and minimize redisplay, if that turns out to be faster.
>> 
>> What's wrong with that?
>
> Well, that was actually my next target :)
> (but for that we badly need the resize/redepth support).

We'll see another iteration of the patch soon, I hope.

> However, that doesn't solve something that still bugs me, which is
> that the VGA emulation layer wakes up every 30ms to just notice that
> the width/height/depth registers didn't change etc. even if the actual
> window is not shown.
>
> Samuel

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

[Samuel Thibault]

Markus Armbruster, le Tue 04 Mar 2008 18:06:26 +0100, a écrit :
> Samuel Thibault <samuel.thibault@eu.citrix.com> writes:
> > Markus Armbruster, le Tue 04 Mar 2008 16:48:20 +0100, a écrit :
> >> * The domU writes to a framebuffer provided by the frontend.
> >>
> >> * The framebuffer (not a copy of it) can be shared with the backend,
> >>   which only reads.
> >
> > Well, that's not always the case, when the guest is in text mode for
> > instance, the PV shared buffer is converted from the guest text buffer.
> 
> Optimize for the common case.  Which I figure is a 32 bpp framebuffer.

Cirrus VGA is 24bpp max :)

> Anyway, compare and copy for some legacy 80x25 text mode shouldn't eat
> that much CPU.

Yes.

By my "that's not always the case", I actually expressed the need for
the offset feature.

Samuel

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

[Markus Armbruster]

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

> Markus Armbruster, le Tue 04 Mar 2008 18:06:26 +0100, a écrit :
>> Samuel Thibault <samuel.thibault@eu.citrix.com> writes:
>> > Markus Armbruster, le Tue 04 Mar 2008 16:48:20 +0100, a écrit :
>> >> * The domU writes to a framebuffer provided by the frontend.
>> >>
>> >> * The framebuffer (not a copy of it) can be shared with the backend,
>> >>   which only reads.
>> >
>> > Well, that's not always the case, when the guest is in text mode for
>> > instance, the PV shared buffer is converted from the guest text buffer.
>> 
>> Optimize for the common case.  Which I figure is a 32 bpp framebuffer.
>
> Cirrus VGA is 24bpp max :)

I read that as a sign that you're abusing the PVFB stuff for something
it wasn't meant to do: emulating some craptastic piece of vintage junk
:)

>> Anyway, compare and copy for some legacy 80x25 text mode shouldn't eat
>> that much CPU.
>
> Yes.
>
> By my "that's not always the case", I actually expressed the need for
> the offset feature.
>
> Samuel

Let's await the next round of the dynamic mode patch, then see how an
offset could fit in there.

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

[Samuel Thibault]

Markus Armbruster, le Wed 05 Mar 2008 09:03:51 +0100, a écrit :
> Samuel Thibault <samuel.thibault@eu.citrix.com> writes:
> 
> > Markus Armbruster, le Tue 04 Mar 2008 18:06:26 +0100, a écrit :
> >> Samuel Thibault <samuel.thibault@eu.citrix.com> writes:
> >> > Markus Armbruster, le Tue 04 Mar 2008 16:48:20 +0100, a écrit :
> >> >> * The domU writes to a framebuffer provided by the frontend.
> >> >>
> >> >> * The framebuffer (not a copy of it) can be shared with the backend,
> >> >>   which only reads.
> >> >
> >> > Well, that's not always the case, when the guest is in text mode for
> >> > instance, the PV shared buffer is converted from the guest text buffer.
> >> 
> >> Optimize for the common case.  Which I figure is a 32 bpp framebuffer.
> >
> > Cirrus VGA is 24bpp max :)
> 
> I read that as a sign that you're abusing the PVFB stuff for something
> it wasn't meant to do: emulating some craptastic piece of vintage junk
> :)

Yep, ask the qemu developper who chose that model.

Samuel

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

[Markus Armbruster]

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

> Hello,
>
> Markus Armbruster, le Mon 03 Mar 2008 19:03:46 +0100, a écrit :
[...]
>> Are you sure the ability to control the rate is required?  Why isn't
>> it sufficient to be able to switch updates off?
>
> Being able to choose the smoothness of the interface is really a good
> user experience.  To my feeling, the current 30ms default rate of qemu
> (7% CPU) is not so smooth (people don't use 30Hz monitors, to they? ;).
> I usually prefer spending e.g. 14% CPU to get a 10ms rate, but of course
> I don't want that CPU time to be used when the viewer is off screen.
> Other people won't feel that need and can save CPU% by slowing it down.

If PVFB is not smooth enough, why not use some real remote graphics
tech?  Plain X11 (with NX for high latency connections) and VNC come
to mind.  PVFB is neat to run graphical installers and as an emergency
console, but I wouldn't want to use it for a desktop.

[...]

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

[Samuel Thibault]

Markus Armbruster, le Wed 05 Mar 2008 12:19:40 +0100, a écrit :
> Samuel Thibault <samuel.thibault@eu.citrix.com> writes:
> 
> > Hello,
> >
> > Markus Armbruster, le Mon 03 Mar 2008 19:03:46 +0100, a écrit :
> [...]
> >> Are you sure the ability to control the rate is required?  Why isn't
> >> it sufficient to be able to switch updates off?
> >
> > Being able to choose the smoothness of the interface is really a good
> > user experience.  To my feeling, the current 30ms default rate of qemu
> > (7% CPU) is not so smooth (people don't use 30Hz monitors, to they? ;).
> > I usually prefer spending e.g. 14% CPU to get a 10ms rate, but of course
> > I don't want that CPU time to be used when the viewer is off screen.
> > Other people won't feel that need and can save CPU% by slowing it down.
> 
> If PVFB is not smooth enough, why not use some real remote graphics
> tech?  Plain X11 (with NX for high latency connections) and VNC come
> to mind.  PVFB is neat to run graphical installers and as an emergency
> console, but I wouldn't want to use it for a desktop.

PVFB is by design just faster than X11 or VNC.  The problem here is not
in PVFB, but in the VGA engine, which needs information (minization)
that X11 or VNC provide, but PVFB doesn't.

Samuel

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

[Samuel Thibault]

Hello,

Getting back to that old issue.

Samuel Thibault, le Tue 04 Mar 2008 16:12:20 +0000, a écrit :
> > * If the frontend can track changes efficiently, it sends update
> >   messages to the backend, which can use them to only redisplay
> >   changed areas.
> 
> Well, that was actually my next target :)
> (but for that we badly need the resize/redepth support).
> 
> However, that doesn't solve something that still bugs me, which is
> that the VGA emulation layer wakes up every 30ms to just notice that
> the width/height/depth registers didn't change etc. even if the actual
> window is not shown.

What could be done is to make the backend set request-update to 0 when
the window is minimized, which makes the frontend understand that there
is temporarily no need to send updates any more, and set it back to 1
when the window is restored.  Would that be OK?

Samuel

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

[Samuel Thibault]

Samuel Thibault, le Thu 01 May 2008 18:55:36 +0100, a écrit :
> Getting back to that old issue.
> 
> Samuel Thibault, le Tue 04 Mar 2008 16:12:20 +0000, a écrit :
> > > * If the frontend can track changes efficiently, it sends update
> > >   messages to the backend, which can use them to only redisplay
> > >   changed areas.
> > 
> > Well, that was actually my next target :)
> > (but for that we badly need the resize/redepth support).
> > 
> > However, that doesn't solve something that still bugs me, which is
> > that the VGA emulation layer wakes up every 30ms to just notice that
> > the width/height/depth registers didn't change etc. even if the actual
> > window is not shown.
> 
> What could be done is to make the backend set request-update to 0 when
> the window is minimized, which makes the frontend understand that there
> is temporarily no need to send updates any more, and set it back to 1
> when the window is restored.  Would that be OK?

Here is how it would be implemented:


ioemu: transmit idleness information to avoid useless polls

Signed-off-by: Samuel Thibault <samuel.thibault@eu.citrix.com>

diff -r 32342f2b5742 extras/mini-os/fbfront.c
--- a/extras/mini-os/fbfront.c	Fri May 02 15:12:43 2008 +0100
+++ b/extras/mini-os/fbfront.c	Fri May 02 17:05:17 2008 +0100
@@ -243,6 +243,8 @@ struct fbfront_dev {
     char *backend;
     int request_update;
 
+    struct xenbus_event *volatile events;
+
     int width;
     int height;
     int depth;
@@ -377,6 +379,8 @@ done:
         xenbus_unwatch_path(XBT_NIL, path);
 
         snprintf(path, sizeof(path), "%s/request-update", dev->backend);
+        dev->events = NULL;
+        xenbus_watch_path_token(XBT_NIL, path, path, &dev->events);
         dev->request_update = xenbus_read_integer(path);
 
         err = xenbus_printf(XBT_NIL, nodename, "state", "%u", 4); /* connected */
@@ -407,12 +411,20 @@ static void fbfront_out_event(struct fbf
     notify_remote_via_evtchn(dev->evtchn);
 }
 
+int fbfront_update_requested(struct fbfront_dev *dev)
+{
+    struct xenbus_event *event;
+    while ((event = dev->events)) {
+        dev->events = event->next;
+        dev->request_update = xenbus_read_integer(event->path);
+        xfree(event);
+    }
+    return dev->request_update;
+}
+
 void fbfront_update(struct fbfront_dev *dev, int x, int y, int width, int height)
 {
     struct xenfb_update update;
-
-    if (dev->request_update <= 0)
-        return;
 
     if (x < 0) {
         width += x;
@@ -461,6 +473,7 @@ void shutdown_fbfront(struct fbfront_dev
 
     printk("close fb: backend at %s\n",dev->backend);
 
+    xenbus_unwatch_path_token(XBT_NIL, path, path);
     snprintf(path, sizeof(path), "%s/state", dev->backend);
     err = xenbus_printf(XBT_NIL, nodename, "state", "%u", 5); /* closing */
     xenbus_wait_for_value(path,"5");
diff -r 32342f2b5742 extras/mini-os/include/fbfront.h
--- a/extras/mini-os/include/fbfront.h	Fri May 02 15:12:43 2008 +0100
+++ b/extras/mini-os/include/fbfront.h	Fri May 02 17:05:17 2008 +0100
@@ -36,6 +36,7 @@ int fbfront_open(struct fbfront_dev *dev
 int fbfront_open(struct fbfront_dev *dev);
 #endif
 
+int fbfront_update_requested(struct fbfront_dev *dev);
 void fbfront_update(struct fbfront_dev *dev, int x, int y, int width, int height);
 void fbfront_resize(struct fbfront_dev *dev, int width, int height, int stride, int depth, int offset);
 
diff -r 32342f2b5742 tools/ioemu/hw/xenfb.c
--- a/tools/ioemu/hw/xenfb.c	Fri May 02 15:12:43 2008 +0100
+++ b/tools/ioemu/hw/xenfb.c	Fri May 02 17:05:17 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 requested_update;   /* Did we request update */
 	char protocol[64];	/* frontend protocol */
 };
 
@@ -706,7 +707,8 @@ static int xenfb_read_frontend_fb_config
         if (xenfb_xs_scanf1(xenfb->xsh, xenfb->fb.otherend, "protocol", "%63s",
                             xenfb->protocol) < 0)
                 xenfb->protocol[0] = '\0';
-        xenfb_xs_printf(xenfb->xsh, xenfb->fb.nodename, "request-update", "1");
+        xenfb_xs_printf(xenfb->xsh, xenfb->fb.nodename, "request-update", xenfb->ds->idle ? "0" : "1");
+        xenfb->requested_update = !xenfb->ds->idle;
 
         /* TODO check for permitted ranges */
         fb_page = xenfb->fb.page;
@@ -1189,6 +1191,17 @@ static void xenfb_update(void *opaque)
 static void xenfb_update(void *opaque)
 {
     struct xenfb *xenfb = opaque;
+    if (xenfb->ds->idle) {
+        if (xenfb->requested_update) {
+            xenfb_xs_printf(xenfb->xsh, xenfb->fb.nodename, "request-update", "0");
+            xenfb->requested_update = 0;
+        }
+    } else {
+        if (!xenfb->requested_update) {
+            xenfb_xs_printf(xenfb->xsh, xenfb->fb.nodename, "request-update", "1");
+            xenfb->requested_update = 1;
+        }
+    }
 }
 
 /* QEMU display state changed, so refresh the framebuffer copy */
@@ -1318,7 +1331,22 @@ static void xenfb_pv_setdata(DisplayStat
 
 static void xenfb_pv_refresh(DisplayState *ds)
 {
+    struct fbfront_dev *fb_dev = ds->opaque;
+
     vga_hw_update();
+
+    if (!fb_dev)
+        return;
+
+    if (fbfront_update_requested(fb_dev)) {
+        /* Back to default interval */
+        ds->gui_timer_interval = 0;
+        ds->idle = 0;
+    } else {
+        /* Sleeping interval */
+        ds->gui_timer_interval = 500;
+        ds->idle = 1;
+    }
 }
 
 static void xenfb_kbd_handler(void *opaque)
@@ -1447,6 +1475,7 @@ int xenfb_pv_display_init(DisplayState *
     ds->dpy_colourdepth = xenfb_pv_colourdepth;
     ds->dpy_setdata = xenfb_pv_setdata;
     ds->dpy_refresh = xenfb_pv_refresh;
+    ds->opaque = NULL;
     return 0;
 }
 
diff -r 32342f2b5742 tools/ioemu/sdl.c
--- a/tools/ioemu/sdl.c	Fri May 02 15:12:43 2008 +0100
+++ b/tools/ioemu/sdl.c	Fri May 02 17:05:17 2008 +0100
@@ -696,9 +696,11 @@ static void sdl_refresh(DisplayState *ds
 		if (ev->active.gain) {
 		    /* Back to default interval */
 		    ds->gui_timer_interval = 0;
+		    ds->idle = 0;
 		} else {
 		    /* Sleeping interval */
 		    ds->gui_timer_interval = 500;
+		    ds->idle = 1;
 		}
 	    }
             break;
diff -r 32342f2b5742 tools/ioemu/vl.c
--- a/tools/ioemu/vl.c	Fri May 02 15:12:43 2008 +0100
+++ b/tools/ioemu/vl.c	Fri May 02 17:05:17 2008 +0100
@@ -4467,6 +4467,8 @@ void dumb_display_init(DisplayState *ds)
     ds->dpy_resize = dumb_resize;
     ds->dpy_colourdepth = NULL;
     ds->dpy_refresh = dumb_refresh;
+    ds->gui_timer_interval = 500;
+    ds->idle = 1;
 }
 
 /***********************************************************/
diff -r 32342f2b5742 tools/ioemu/vl.h
--- a/tools/ioemu/vl.h	Fri May 02 15:12:43 2008 +0100
+++ b/tools/ioemu/vl.h	Fri May 02 17:05:17 2008 +0100
@@ -939,6 +939,7 @@ struct DisplayState {
     void *opaque;
     uint32_t *palette;
     uint64_t gui_timer_interval;
+    int idle;
 
     int shared_buf;
     
diff -r 32342f2b5742 tools/ioemu/vnc.c
--- a/tools/ioemu/vnc.c	Fri May 02 15:12:43 2008 +0100
+++ b/tools/ioemu/vnc.c	Fri May 02 17:05:17 2008 +0100
@@ -778,6 +778,7 @@ static void _vnc_update_client(void *opa
     vs->has_update = 0;
     vnc_flush(vs);
     vs->last_update_time = now;
+    vs->ds->idle = 0;
 
     vs->timer_interval /= 2;
     if (vs->timer_interval < VNC_REFRESH_INTERVAL_BASE)
@@ -790,26 +791,29 @@ static void _vnc_update_client(void *opa
     vs->timer_interval += VNC_REFRESH_INTERVAL_INC;
     if (vs->timer_interval > VNC_REFRESH_INTERVAL_MAX) {
 	vs->timer_interval = VNC_REFRESH_INTERVAL_MAX;
-	if (now - vs->last_update_time >= VNC_MAX_UPDATE_INTERVAL &&
-            vs->update_requested) {
-	    /* Send a null update.  If the client is no longer
-	       interested (e.g. minimised) it'll ignore this, and we
-	       can stop scanning the buffer until it sends another
-	       update request. */
-	    /* It turns out that there's a bug in realvncviewer 4.1.2
-	       which means that if you send a proper null update (with
-	       no update rectangles), it gets a bit out of sync and
-	       never sends any further requests, regardless of whether
-	       it needs one or not.  Fix this by sending a single 1x1
-	       update rectangle instead. */
-	    vnc_write_u8(vs, 0);
-	    vnc_write_u8(vs, 0);
-	    vnc_write_u16(vs, 1);
-	    send_framebuffer_update(vs, 0, 0, 1, 1);
-	    vnc_flush(vs);
-	    vs->last_update_time = now;
-            vs->update_requested--;
-	    return;
+	if (now - vs->last_update_time >= VNC_MAX_UPDATE_INTERVAL) {
+            if (!vs->update_requested) {
+                vs->ds->idle = 1;
+            } else {
+                /* Send a null update.  If the client is no longer
+                   interested (e.g. minimised) it'll ignore this, and we
+                   can stop scanning the buffer until it sends another
+                   update request. */
+                /* It turns out that there's a bug in realvncviewer 4.1.2
+                   which means that if you send a proper null update (with
+                   no update rectangles), it gets a bit out of sync and
+                   never sends any further requests, regardless of whether
+                   it needs one or not.  Fix this by sending a single 1x1
+                   update rectangle instead. */
+                vnc_write_u8(vs, 0);
+                vnc_write_u8(vs, 0);
+                vnc_write_u16(vs, 1);
+                send_framebuffer_update(vs, 0, 0, 1, 1);
+                vnc_flush(vs);
+                vs->last_update_time = now;
+                vs->update_requested--;
+                return;
+            }
 	}
     }
     qemu_mod_timer(vs->timer, now + vs->timer_interval);
@@ -970,6 +974,7 @@ static int vnc_client_io_error(VncState 
 	qemu_set_fd_handler2(vs->csock, NULL, NULL, NULL, NULL);
 	closesocket(vs->csock);
 	vs->csock = -1;
+        vs->ds->idle = 1;
 	buffer_reset(&vs->input);
 	buffer_reset(&vs->output);
         free_queue(vs);
@@ -2443,6 +2448,7 @@ static void vnc_listen_read(void *opaque
     vs->csock = accept(vs->lsock, (struct sockaddr *)&addr, &addrlen);
     if (vs->csock != -1) {
 	VNC_DEBUG("New client on socket %d\n", vs->csock);
+        vs->ds->idle = 0;
         socket_set_nonblock(vs->csock);
 	qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, NULL, opaque);
 	vnc_write(vs, "RFB 003.008\n", 12);
@@ -2468,6 +2474,7 @@ void vnc_display_init(DisplayState *ds)
 	exit(1);
 
     ds->opaque = vs;
+    ds->idle = 1;
     vnc_state = vs;
     vs->display = NULL;
     vs->password = NULL;

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

[Markus Armbruster]

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

> Samuel Thibault, le Thu 01 May 2008 18:55:36 +0100, a écrit :
>> Getting back to that old issue.
>> 
>> Samuel Thibault, le Tue 04 Mar 2008 16:12:20 +0000, a écrit :
>> > > * If the frontend can track changes efficiently, it sends update
>> > >   messages to the backend, which can use them to only redisplay
>> > >   changed areas.
>> > 
>> > Well, that was actually my next target :)
>> > (but for that we badly need the resize/redepth support).
>> > 
>> > However, that doesn't solve something that still bugs me, which is
>> > that the VGA emulation layer wakes up every 30ms to just notice that
>> > the width/height/depth registers didn't change etc. even if the actual
>> > window is not shown.
>> 
>> What could be done is to make the backend set request-update to 0 when
>> the window is minimized, which makes the frontend understand that there
>> is temporarily no need to send updates any more, and set it back to 1
>> when the window is restored.  Would that be OK?
>
> Here is how it would be implemented:
>
>
> ioemu: transmit idleness information to avoid useless polls
>
> Signed-off-by: Samuel Thibault <samuel.thibault@eu.citrix.com>

Any particular reason for going through xenstore instead of the event
rings?

If I read your patch correctly, xenfb_read_frontend_fb_config() sets
request-update in xenstore depending on xenfb->ds->idle, during
startup.  The only place that changes it is xenfb_update(), which is
#ifdef CONFIG_STUBDOM.

What if ds->idle is true when xenfb_read_frontend_fb_config() runs
during startup of the ordinary (!CONFIG_STUBDOM) backend?  Can this
happen?  If not, why?

Other than that, the change seems to affect only Mini-OS and
CONFIG_STUBDOM, which are both outside the area of my expertise (and
frankly, interest).

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

[Samuel Thibault]

Markus Armbruster, le Mon 05 May 2008 10:26:14 +0200, a écrit :
> Any particular reason for going through xenstore instead of the event
> rings?

Just because it exists.

> If I read your patch correctly, xenfb_read_frontend_fb_config() sets
> request-update in xenstore depending on xenfb->ds->idle, during
> startup.  The only place that changes it is xenfb_update(), which is
> #ifdef CONFIG_STUBDOM.

Err, no, it is not #ifdef CONFIG_STUBDOM.

> What if ds->idle is true when xenfb_read_frontend_fb_config() runs
> during startup of the ordinary (!CONFIG_STUBDOM) backend?  Can this
> happen?

Yes it can, when running a vnc server for instance.  And when later on a
client connects, on next iteration of xenfb_update() the frontend will
be notified.

> Other than that, the change seems to affect only Mini-OS and
> CONFIG_STUBDOM, which are both outside the area of my expertise (and
> frankly, interest).

Well, that could be used in the linux frontend to disable the timer too.
That may interest e.g. laptop users.

Samuel

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

[Markus Armbruster]

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

> Markus Armbruster, le Mon 05 May 2008 10:26:14 +0200, a écrit :
>> Any particular reason for going through xenstore instead of the event
>> rings?
>
> Just because it exists.

Sufficient reason for climbing mountains, if you're so inclined, but
is it sane guidance for software development?

So far, PVFB uses xenstore *only* for device initialization and
shutdown.  For normal operations, we communicate through the event
rings.  Therefore, there's just one place dealing with events related
to device initialization and shutdown (in the fb frontend:
xenfb_backend_changed()), and just one place dealing with all other
events (fb frontend: xenfb_event_handler()).

I fear mixing in more communication channels without a technical need
just adds complexity for no real gain.

>> If I read your patch correctly, xenfb_read_frontend_fb_config() sets
>> request-update in xenstore depending on xenfb->ds->idle, during
>> startup.  The only place that changes it is xenfb_update(), which is
>> #ifdef CONFIG_STUBDOM.
>
> Err, no, it is not #ifdef CONFIG_STUBDOM.

Oops, you're right.  You'll have to update its comment, by the way.

>> What if ds->idle is true when xenfb_read_frontend_fb_config() runs
>> during startup of the ordinary (!CONFIG_STUBDOM) backend?  Can this
>> happen?
>
> Yes it can, when running a vnc server for instance.  And when later on a
> client connects, on next iteration of xenfb_update() the frontend will
> be notified.
>
>> Other than that, the change seems to affect only Mini-OS and
>> CONFIG_STUBDOM, which are both outside the area of my expertise (and
>> frankly, interest).
>
> Well, that could be used in the linux frontend to disable the timer too.
> That may interest e.g. laptop users.
>
> Samuel

The frontend should not send any events unless something's drawing on
the display.  And when something's drawing on the display, the guest,
and therefore the laptop, is already burning processor cycles, and
whether disabling sending updates saves any battery power is not
obvious.  Of course, if you can demonstrate it does...

Anyway, I'm not opposed to the feature.

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

[Samuel Thibault]

Markus Armbruster, le Mon 05 May 2008 11:58:53 +0200, a écrit :
> >> Other than that, the change seems to affect only Mini-OS and
> >> CONFIG_STUBDOM, which are both outside the area of my expertise (and
> >> frankly, interest).
> >
> > Well, that could be used in the linux frontend to disable the timer too.
> > That may interest e.g. laptop users.
> 
> The frontend should not send any events unless something's drawing on
> the display.

Agreed.

> And when something's drawing on the display, the guest,
> and therefore the laptop, is already burning processor cycles,

Agreed.

> and whether disabling sending updates saves any battery power is not
> obvious.

Disabling _sending_ updates does not, yes.

> Of course, if you can demonstrate it does...

Disabling _having to care_ whether some updates should be sent _do_ save
battery.  Now, contrary to what I thought, it happens that linux' xenfb
is not using a periodic timer but a triggered timer, so it does not in
that case indeed.

Samuel

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

[Samuel Thibault]

Hello,

Something like this then?



ioemu: transmit idleness information to avoid useless polls

Signed-off-by: Samuel Thibault <samuel.thibault@eu.citrix.com>

diff -r 32342f2b5742 extras/mini-os/fbfront.c
--- a/extras/mini-os/fbfront.c	Fri May 02 15:12:43 2008 +0100
+++ b/extras/mini-os/fbfront.c	Mon May 05 17:44:16 2008 +0100
@@ -249,11 +249,41 @@ struct fbfront_dev {
     int stride;
     int mem_length;
     int offset;
+
+    int status;
 };
 
 void fbfront_handler(evtchn_port_t port, struct pt_regs *regs, void *data)
 {
+    struct fbfront_dev *dev = data;
+    struct xenfb_page *page = dev->page;
+    uint32_t prod, cons;
+    int i;
+
     wake_up(&fbfront_queue);
+    prod = page->in_prod;
+
+    if (prod == page->in_cons)
+        return;
+
+    rmb();      /* ensure we see ring contents up to prod */
+
+    for (i = 0, cons = page->in_cons; cons != prod; i++, cons++) {
+        union xenfb_in_event *event = &XENFB_IN_RING_REF(page, cons);
+        switch (event->type) {
+        case XENFB_TYPE_BACKEND_STATUS:
+            dev->status = event->status.status;
+            printk("got status %d\n", dev->status);
+            break;
+        default:
+            /* ignore */
+            break;
+        }
+    }
+    mb();       /* ensure we got ring contents */
+    page->in_cons = cons;
+
+    notify_remote_via_evtchn(dev->evtchn);
 }
 
 struct fbfront_dev *init_fbfront(char *nodename, unsigned long *mfns, int width, int height, int depth, int stride, int n)
@@ -292,6 +322,7 @@ struct fbfront_dev *init_fbfront(char *n
     dev->stride = s->line_length = stride;
     dev->mem_length = s->mem_length = n * PAGE_SIZE;
     dev->offset = 0;
+    dev->status = XENFB_BACKEND_STATUS_ACTIVE;
 
     const int max_pd = sizeof(s->pd) / sizeof(s->pd[0]);
     unsigned long mapped = 0;
@@ -405,6 +436,11 @@ static void fbfront_out_event(struct fbf
     wmb(); /* ensure ring contents visible */
     page->out_prod = prod + 1;
     notify_remote_via_evtchn(dev->evtchn);
+}
+
+int fbfront_status(struct fbfront_dev *dev)
+{
+    return dev->status;
 }
 
 void fbfront_update(struct fbfront_dev *dev, int x, int y, int width, int height)
diff -r 32342f2b5742 extras/mini-os/include/fbfront.h
--- a/extras/mini-os/include/fbfront.h	Fri May 02 15:12:43 2008 +0100
+++ b/extras/mini-os/include/fbfront.h	Mon May 05 17:44:16 2008 +0100
@@ -36,6 +36,7 @@ int fbfront_open(struct fbfront_dev *dev
 int fbfront_open(struct fbfront_dev *dev);
 #endif
 
+int fbfront_status(struct fbfront_dev *dev);
 void fbfront_update(struct fbfront_dev *dev, int x, int y, int width, int height);
 void fbfront_resize(struct fbfront_dev *dev, int width, int height, int stride, int depth, int offset);
 
diff -r 32342f2b5742 tools/ioemu/hw/xenfb.c
--- a/tools/ioemu/hw/xenfb.c	Fri May 02 15:12:43 2008 +0100
+++ b/tools/ioemu/hw/xenfb.c	Mon May 05 17:44:16 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 notified_active;   /* Did we request update */
 	char protocol[64];	/* frontend protocol */
 };
 
@@ -536,6 +537,40 @@ 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_status(struct xenfb *xenfb, int active)
+{
+        union xenfb_in_event event;
+        event.type = XENFB_TYPE_BACKEND_STATUS;
+        event.status.status = active ? XENFB_BACKEND_STATUS_ACTIVE
+                                     : XENFB_BACKEND_STATUS_IDLE ;
+        xenfb_send_event(xenfb, &event);
+}
+
 static void xenfb_on_kbd_event(struct xenfb *xenfb)
 {
 	struct xenkbd_page *page = xenfb->kbd.page;
@@ -707,6 +742,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->notified_active = 1;
 
         /* TODO check for permitted ranges */
         fb_page = xenfb->fb.page;
@@ -1185,10 +1221,21 @@ 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, just announce idleness to the front end */
 static void xenfb_update(void *opaque)
 {
     struct xenfb *xenfb = opaque;
+    if (xenfb->ds->idle) {
+        if (xenfb->notified_active && !xenfb_queue_full(xenfb)) {
+            xenfb_send_status(xenfb, 0);
+            xenfb->notified_active = 0;
+        }
+    } else {
+        if (!xenfb->notified_active && !xenfb_queue_full(xenfb)) {
+            xenfb_send_status(xenfb, 1);
+            xenfb->notified_active = 1;
+        }
+    }
 }
 
 /* QEMU display state changed, so refresh the framebuffer copy */
@@ -1318,7 +1365,22 @@ static void xenfb_pv_setdata(DisplayStat
 
 static void xenfb_pv_refresh(DisplayState *ds)
 {
+    struct fbfront_dev *fb_dev = ds->opaque;
+
     vga_hw_update();
+
+    if (!fb_dev)
+        return;
+
+    if (fbfront_status(fb_dev)) {
+        /* Back to default interval */
+        ds->gui_timer_interval = 0;
+        ds->idle = 0;
+    } else {
+        /* Sleeping interval */
+        ds->gui_timer_interval = 500;
+        ds->idle = 1;
+    }
 }
 
 static void xenfb_kbd_handler(void *opaque)
@@ -1447,6 +1509,7 @@ int xenfb_pv_display_init(DisplayState *
     ds->dpy_colourdepth = xenfb_pv_colourdepth;
     ds->dpy_setdata = xenfb_pv_setdata;
     ds->dpy_refresh = xenfb_pv_refresh;
+    ds->opaque = NULL;
     return 0;
 }
 
diff -r 32342f2b5742 tools/ioemu/sdl.c
--- a/tools/ioemu/sdl.c	Fri May 02 15:12:43 2008 +0100
+++ b/tools/ioemu/sdl.c	Mon May 05 17:44:16 2008 +0100
@@ -696,9 +696,11 @@ static void sdl_refresh(DisplayState *ds
 		if (ev->active.gain) {
 		    /* Back to default interval */
 		    ds->gui_timer_interval = 0;
+		    ds->idle = 0;
 		} else {
 		    /* Sleeping interval */
 		    ds->gui_timer_interval = 500;
+		    ds->idle = 1;
 		}
 	    }
             break;
diff -r 32342f2b5742 tools/ioemu/vl.c
--- a/tools/ioemu/vl.c	Fri May 02 15:12:43 2008 +0100
+++ b/tools/ioemu/vl.c	Mon May 05 17:44:16 2008 +0100
@@ -4467,6 +4467,8 @@ void dumb_display_init(DisplayState *ds)
     ds->dpy_resize = dumb_resize;
     ds->dpy_colourdepth = NULL;
     ds->dpy_refresh = dumb_refresh;
+    ds->gui_timer_interval = 500;
+    ds->idle = 1;
 }
 
 /***********************************************************/
diff -r 32342f2b5742 tools/ioemu/vl.h
--- a/tools/ioemu/vl.h	Fri May 02 15:12:43 2008 +0100
+++ b/tools/ioemu/vl.h	Mon May 05 17:44:16 2008 +0100
@@ -939,6 +939,7 @@ struct DisplayState {
     void *opaque;
     uint32_t *palette;
     uint64_t gui_timer_interval;
+    int idle;
 
     int shared_buf;
     
diff -r 32342f2b5742 tools/ioemu/vnc.c
--- a/tools/ioemu/vnc.c	Fri May 02 15:12:43 2008 +0100
+++ b/tools/ioemu/vnc.c	Mon May 05 17:44:16 2008 +0100
@@ -778,6 +778,7 @@ static void _vnc_update_client(void *opa
     vs->has_update = 0;
     vnc_flush(vs);
     vs->last_update_time = now;
+    vs->ds->idle = 0;
 
     vs->timer_interval /= 2;
     if (vs->timer_interval < VNC_REFRESH_INTERVAL_BASE)
@@ -790,26 +791,29 @@ static void _vnc_update_client(void *opa
     vs->timer_interval += VNC_REFRESH_INTERVAL_INC;
     if (vs->timer_interval > VNC_REFRESH_INTERVAL_MAX) {
 	vs->timer_interval = VNC_REFRESH_INTERVAL_MAX;
-	if (now - vs->last_update_time >= VNC_MAX_UPDATE_INTERVAL &&
-            vs->update_requested) {
-	    /* Send a null update.  If the client is no longer
-	       interested (e.g. minimised) it'll ignore this, and we
-	       can stop scanning the buffer until it sends another
-	       update request. */
-	    /* It turns out that there's a bug in realvncviewer 4.1.2
-	       which means that if you send a proper null update (with
-	       no update rectangles), it gets a bit out of sync and
-	       never sends any further requests, regardless of whether
-	       it needs one or not.  Fix this by sending a single 1x1
-	       update rectangle instead. */
-	    vnc_write_u8(vs, 0);
-	    vnc_write_u8(vs, 0);
-	    vnc_write_u16(vs, 1);
-	    send_framebuffer_update(vs, 0, 0, 1, 1);
-	    vnc_flush(vs);
-	    vs->last_update_time = now;
-            vs->update_requested--;
-	    return;
+	if (now - vs->last_update_time >= VNC_MAX_UPDATE_INTERVAL) {
+            if (!vs->update_requested) {
+                vs->ds->idle = 1;
+            } else {
+                /* Send a null update.  If the client is no longer
+                   interested (e.g. minimised) it'll ignore this, and we
+                   can stop scanning the buffer until it sends another
+                   update request. */
+                /* It turns out that there's a bug in realvncviewer 4.1.2
+                   which means that if you send a proper null update (with
+                   no update rectangles), it gets a bit out of sync and
+                   never sends any further requests, regardless of whether
+                   it needs one or not.  Fix this by sending a single 1x1
+                   update rectangle instead. */
+                vnc_write_u8(vs, 0);
+                vnc_write_u8(vs, 0);
+                vnc_write_u16(vs, 1);
+                send_framebuffer_update(vs, 0, 0, 1, 1);
+                vnc_flush(vs);
+                vs->last_update_time = now;
+                vs->update_requested--;
+                return;
+            }
 	}
     }
     qemu_mod_timer(vs->timer, now + vs->timer_interval);
@@ -970,6 +974,7 @@ static int vnc_client_io_error(VncState 
 	qemu_set_fd_handler2(vs->csock, NULL, NULL, NULL, NULL);
 	closesocket(vs->csock);
 	vs->csock = -1;
+        vs->ds->idle = 1;
 	buffer_reset(&vs->input);
 	buffer_reset(&vs->output);
         free_queue(vs);
@@ -2443,6 +2448,7 @@ static void vnc_listen_read(void *opaque
     vs->csock = accept(vs->lsock, (struct sockaddr *)&addr, &addrlen);
     if (vs->csock != -1) {
 	VNC_DEBUG("New client on socket %d\n", vs->csock);
+        vs->ds->idle = 0;
         socket_set_nonblock(vs->csock);
 	qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, NULL, opaque);
 	vnc_write(vs, "RFB 003.008\n", 12);
@@ -2468,6 +2474,7 @@ void vnc_display_init(DisplayState *ds)
 	exit(1);
 
     ds->opaque = vs;
+    ds->idle = 1;
     vnc_state = vs;
     vs->display = NULL;
     vs->password = NULL;
diff -r 32342f2b5742 xen/include/public/io/fbif.h
--- a/xen/include/public/io/fbif.h	Fri May 02 15:12:43 2008 +0100
+++ b/xen/include/public/io/fbif.h	Mon May 05 17:44:16 2008 +0100
@@ -80,14 +80,30 @@ union xenfb_out_event
 
 /*
  * Frontends should ignore unknown in events.
- * No in events currently defined.
  */
+
+/*
+ * Backend idleness report
+ * Backend sends it when the output window is somehow non visible
+ * (minimized, no client, etc.)
+ */
+#define XENFB_TYPE_BACKEND_STATUS 1
+
+#define XENFB_BACKEND_STATUS_IDLE 0
+#define XENFB_BACKEND_STATUS_ACTIVE 1
+
+struct xenfb_backend_status
+{
+    uint8_t type;    /* XENFB_TYPE_BACKEND_STATUS */
+    uint8_t status;  /* XENFB_BACKEND_STATUS_* */
+};
 
 #define XENFB_IN_EVENT_SIZE 40
 
 union xenfb_in_event
 {
     uint8_t type;
+    struct xenfb_backend_status status;
     char pad[XENFB_IN_EVENT_SIZE];
 };

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

[Markus Armbruster]

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

> Hello,
>
> Something like this then?

Yes.  Comments inline.

> ioemu: transmit idleness information to avoid useless polls
>
> Signed-off-by: Samuel Thibault <samuel.thibault@eu.citrix.com>
>
> diff -r 32342f2b5742 extras/mini-os/fbfront.c
> --- a/extras/mini-os/fbfront.c	Fri May 02 15:12:43 2008 +0100
> +++ b/extras/mini-os/fbfront.c	Mon May 05 17:44:16 2008 +0100
> @@ -249,11 +249,41 @@ struct fbfront_dev {
>      int stride;
>      int mem_length;
>      int offset;
> +
> +    int status;

Suggest a more descriptive name, or, if you can't think of any, a
descriptive comment.

Oh, see comments on fbif.h below.

>  };
>  
>  void fbfront_handler(evtchn_port_t port, struct pt_regs *regs, void *data)
>  {
> +    struct fbfront_dev *dev = data;
> +    struct xenfb_page *page = dev->page;
> +    uint32_t prod, cons;
> +    int i;
> +
>      wake_up(&fbfront_queue);
> +    prod = page->in_prod;
> +
> +    if (prod == page->in_cons)
> +        return;
> +
> +    rmb();      /* ensure we see ring contents up to prod */
> +
> +    for (i = 0, cons = page->in_cons; cons != prod; i++, cons++) {

Purpose of i ?

> +        union xenfb_in_event *event = &XENFB_IN_RING_REF(page, cons);
> +        switch (event->type) {
> +        case XENFB_TYPE_BACKEND_STATUS:
> +            dev->status = event->status.status;
> +            printk("got status %d\n", dev->status);
> +            break;
> +        default:
> +            /* ignore */

Suggest /* ignore unknown events */

> +            break;
> +        }
> +    }
> +    mb();       /* ensure we got ring contents */
> +    page->in_cons = cons;
> +
> +    notify_remote_via_evtchn(dev->evtchn);
>  }
>  
>  struct fbfront_dev *init_fbfront(char *nodename, unsigned long *mfns, int width, int height, int depth, int stride, int n)
> @@ -292,6 +322,7 @@ struct fbfront_dev *init_fbfront(char *n
>      dev->stride = s->line_length = stride;
>      dev->mem_length = s->mem_length = n * PAGE_SIZE;
>      dev->offset = 0;
> +    dev->status = XENFB_BACKEND_STATUS_ACTIVE;
>  
>      const int max_pd = sizeof(s->pd) / sizeof(s->pd[0]);
>      unsigned long mapped = 0;
> @@ -405,6 +436,11 @@ static void fbfront_out_event(struct fbf
>      wmb(); /* ensure ring contents visible */
>      page->out_prod = prod + 1;
>      notify_remote_via_evtchn(dev->evtchn);
> +}
> +
> +int fbfront_status(struct fbfront_dev *dev)
> +{
> +    return dev->status;
>  }
>  
>  void fbfront_update(struct fbfront_dev *dev, int x, int y, int width, int height)
> diff -r 32342f2b5742 extras/mini-os/include/fbfront.h
> --- a/extras/mini-os/include/fbfront.h	Fri May 02 15:12:43 2008 +0100
> +++ b/extras/mini-os/include/fbfront.h	Mon May 05 17:44:16 2008 +0100
> @@ -36,6 +36,7 @@ int fbfront_open(struct fbfront_dev *dev
>  int fbfront_open(struct fbfront_dev *dev);
>  #endif
>  
> +int fbfront_status(struct fbfront_dev *dev);
>  void fbfront_update(struct fbfront_dev *dev, int x, int y, int width, int height);
>  void fbfront_resize(struct fbfront_dev *dev, int width, int height, int stride, int depth, int offset);
>  
> diff -r 32342f2b5742 tools/ioemu/hw/xenfb.c
> --- a/tools/ioemu/hw/xenfb.c	Fri May 02 15:12:43 2008 +0100
> +++ b/tools/ioemu/hw/xenfb.c	Mon May 05 17:44:16 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 notified_active;   /* Did we request update */

Tab, please, if it's not too much trouble.  Mixing tabs and spaces for
indentation makes diffs unnecessarily hard to read.

>  	char protocol[64];	/* frontend protocol */
>  };
>  
> @@ -536,6 +537,40 @@ 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);

Ditto.

> +}
> +
> +static void xenfb_send_status(struct xenfb *xenfb, int active)
> +{
> +        union xenfb_in_event event;

Please memset(&event, 0, sizeof(event))

Makes backward-compatible evolution of the protocol so much easier,
and removes all doubts about leaking potentially sensitive bits.

> +        event.type = XENFB_TYPE_BACKEND_STATUS;
> +        event.status.status = active ? XENFB_BACKEND_STATUS_ACTIVE
> +                                     : XENFB_BACKEND_STATUS_IDLE ;
> +        xenfb_send_event(xenfb, &event);
> +}
> +
>  static void xenfb_on_kbd_event(struct xenfb *xenfb)
>  {
>  	struct xenkbd_page *page = xenfb->kbd.page;
> @@ -707,6 +742,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->notified_active = 1;
>  
>          /* TODO check for permitted ranges */
>          fb_page = xenfb->fb.page;
> @@ -1185,10 +1221,21 @@ 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, just announce idleness to the front end */
>  static void xenfb_update(void *opaque)
>  {
>      struct xenfb *xenfb = opaque;
> +    if (xenfb->ds->idle) {
> +        if (xenfb->notified_active && !xenfb_queue_full(xenfb)) {
> +            xenfb_send_status(xenfb, 0);
> +            xenfb->notified_active = 0;
> +        }
> +    } else {
> +        if (!xenfb->notified_active && !xenfb_queue_full(xenfb)) {
> +            xenfb_send_status(xenfb, 1);
> +            xenfb->notified_active = 1;
> +        }
> +    }

This breaks if we ever support frontends without feature-update.
Worth a comment.

>  }
>  
>  /* QEMU display state changed, so refresh the framebuffer copy */

[CONFIG_STUBDOM stuff snipped, I'm too ignorant about that to comment...]
> diff -r 32342f2b5742 tools/ioemu/sdl.c
> --- a/tools/ioemu/sdl.c	Fri May 02 15:12:43 2008 +0100
> +++ b/tools/ioemu/sdl.c	Mon May 05 17:44:16 2008 +0100
[more snippage due to ignorance...]
> diff -r 32342f2b5742 tools/ioemu/vl.h
> --- a/tools/ioemu/vl.h	Fri May 02 15:12:43 2008 +0100
> +++ b/tools/ioemu/vl.h	Mon May 05 17:44:16 2008 +0100
[ditto...]
> diff -r 32342f2b5742 tools/ioemu/vnc.c
[ditto...]
> diff -r 32342f2b5742 xen/include/public/io/fbif.h
> --- a/xen/include/public/io/fbif.h	Fri May 02 15:12:43 2008 +0100
> +++ b/xen/include/public/io/fbif.h	Mon May 05 17:44:16 2008 +0100
> @@ -80,14 +80,30 @@ union xenfb_out_event
>  
>  /*
>   * Frontends should ignore unknown in events.
> - * No in events currently defined.
>   */
> +
> +/*
> + * Backend idleness report
> + * Backend sends it when the output window is somehow non visible
> + * (minimized, no client, etc.)
> + */
> +#define XENFB_TYPE_BACKEND_STATUS 1
> +
> +#define XENFB_BACKEND_STATUS_IDLE 0
> +#define XENFB_BACKEND_STATUS_ACTIVE 1
> +
> +struct xenfb_backend_status
> +{
> +    uint8_t type;    /* XENFB_TYPE_BACKEND_STATUS */
> +    uint8_t status;  /* XENFB_BACKEND_STATUS_* */
> +};

I'm not entirely happy with the protocol defined here.

1. Encoding of idleness

   If idleness is and will remain boolean, I'd rather encode it the
   old-fashioned C way as zero / non-zero, because that removes all
   doubt on how to interpret funny status values.

   If you want to reserve funny status values for future extensions of
   the notion "idleness", then you need to document that here, along
   with what frontends should do for values they don't know.

2. Names

   If the message is intended just for reporting idleness, it should
   be called something like BACKEND_IDLE instead of STATUS.

   If it is meant to be extensible for reporting status other than
   idleness, the member status isn't named properly.  If the member
   holds just idleness, it should be named is_idle (if yes/no),
   idleness (if more than two values), or something like that.  If
   idleness is yes/no, you could also encode it as a bit in a status
   word instead.

Aside: going through the ring requires us to write down the protocol
here.  You didn't (have to) do that when you went through xenstore.
Guess what I like better :)

>  
>  #define XENFB_IN_EVENT_SIZE 40
>  
>  union xenfb_in_event
>  {
>      uint8_t type;
> +    struct xenfb_backend_status status;
>      char pad[XENFB_IN_EVENT_SIZE];
>  };
>  

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

[Keir Fraser]

On 6/5/08 14:50, "Markus Armbruster" <armbru@redhat.com> wrote:

> Samuel Thibault <samuel.thibault@eu.citrix.com> writes:
> 
>> Hello,
>> 
>> Something like this then?
> 
> Yes.  Comments inline.

Also, needs to be re-synced with tip now that the minios "fix thread safety"
changeset 17576 is in xen-unstable.

 -- Keir

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

[Samuel Thibault]

Markus Armbruster, le Tue 06 May 2008 15:50:08 +0200, a écrit :
> > +    for (i = 0, cons = page->in_cons; cons != prod; i++, cons++) {
> 
> Purpose of i ?

It was needed in the case of kbd, but not here indeed.

> > +        int notified_active;   /* Did we request update */
> 
> Tab, please, if it's not too much trouble.  Mixing tabs and spaces for
> indentation makes diffs unnecessarily hard to read.

I agree and fixed it, the problem is just that xen has various
indentation/tab practices, so no default configuration can work :)

> > +/*
> > + * Backend idleness report
> > + * Backend sends it when the output window is somehow non visible
> > + * (minimized, no client, etc.)
> > + */
> > +#define XENFB_TYPE_BACKEND_STATUS 1
> > +
> > +#define XENFB_BACKEND_STATUS_IDLE 0
> > +#define XENFB_BACKEND_STATUS_ACTIVE 1
> > +
> > +struct xenfb_backend_status
> > +{
> > +    uint8_t type;    /* XENFB_TYPE_BACKEND_STATUS */
> > +    uint8_t status;  /* XENFB_BACKEND_STATUS_* */
> > +};
> 
> I'm not entirely happy with the protocol defined here.

Right, I'm not sure of what we would ideally want to express. I can see
three use cases:

- The output is fully active, we want frequent update notification
  (that is the assumed permanent state up to now)
- The output is not visible, update notification is useless.
- The output is visible in reduced conditions, for instance a thumbnail
  in a VMs management tool, update notification don't really need to be
  sent often.  We could have the backend explicitely request updates
  from the frontend when it wants a new thumbnail (this is needed e.g.
  in HVM text mode, in which the guest output is not directly mapped
  through PVFB, so an explicit refresh is needed).

Instead of expressing idleness or "status", maybe we could rather
express whether periodic update notifications are wanted or not, and let
the backend request an explicit update notification when it feels the
need for one (low-frequency thumbnail update). It has the advantage of
only talking about the PVFB protocol itself and not something around it
(idleness of the actual output).  That is also backward compatible in
that a frontend which doesn't know these two events will just continue
sending periodic update notifications, which is fine for the backend.

Samuel

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

[Markus Armbruster]

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

> Markus Armbruster, le Tue 06 May 2008 15:50:08 +0200, a écrit :
>> > +    for (i = 0, cons = page->in_cons; cons != prod; i++, cons++) {
>> 
>> Purpose of i ?
>
> It was needed in the case of kbd, but not here indeed.
>
>> > +        int notified_active;   /* Did we request update */
>> 
>> Tab, please, if it's not too much trouble.  Mixing tabs and spaces for
>> indentation makes diffs unnecessarily hard to read.
>
> I agree and fixed it, the problem is just that xen has various
> indentation/tab practices, so no default configuration can work :)

I know...

>> > +/*
>> > + * Backend idleness report
>> > + * Backend sends it when the output window is somehow non visible
>> > + * (minimized, no client, etc.)
>> > + */
>> > +#define XENFB_TYPE_BACKEND_STATUS 1
>> > +
>> > +#define XENFB_BACKEND_STATUS_IDLE 0
>> > +#define XENFB_BACKEND_STATUS_ACTIVE 1
>> > +
>> > +struct xenfb_backend_status
>> > +{
>> > +    uint8_t type;    /* XENFB_TYPE_BACKEND_STATUS */
>> > +    uint8_t status;  /* XENFB_BACKEND_STATUS_* */
>> > +};
>> 
>> I'm not entirely happy with the protocol defined here.
>
> Right, I'm not sure of what we would ideally want to express. I can see
> three use cases:
>
> - The output is fully active, we want frequent update notification
>   (that is the assumed permanent state up to now)
> - The output is not visible, update notification is useless.
> - The output is visible in reduced conditions, for instance a thumbnail
>   in a VMs management tool, update notification don't really need to be
>   sent often.  We could have the backend explicitely request updates
>   from the frontend when it wants a new thumbnail (this is needed e.g.
>   in HVM text mode, in which the guest output is not directly mapped
>   through PVFB, so an explicit refresh is needed).
>
> Instead of expressing idleness or "status", maybe we could rather
> express whether periodic update notifications are wanted or not, and let
> the backend request an explicit update notification when it feels the
> need for one (low-frequency thumbnail update). It has the advantage of
> only talking about the PVFB protocol itself and not something around it
> (idleness of the actual output).  That is also backward compatible in
> that a frontend which doesn't know these two events will just continue
> sending periodic update notifications, which is fine for the backend.
>
> Samuel

I think that's a better way to define this feature.

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

[Samuel Thibault]

Markus Armbruster, le Tue 06 May 2008 18:50:04 +0200, a écrit :
> > Instead of expressing idleness or "status", maybe we could rather
> > express whether periodic update notifications are wanted or not, and let
> > the backend request an explicit update notification when it feels the
> > need for one (low-frequency thumbnail update). It has the advantage of
> > only talking about the PVFB protocol itself and not something around it
> > (idleness of the actual output).  That is also backward compatible in
> > that a frontend which doesn't know these two events will just continue
> > sending periodic update notifications, which is fine for the backend.
> 
> I think that's a better way to define this feature.

More precisely, we could have an UPDATE_PERIOD event which carries
an advice from the backend about how often the frontend should sent
update notifications (0 if periodic notification is not useful), and a
REQUEST_UPDATE event that requests a one-time update notification?  The
latter could even contain the requested area? (not sure whether it'd be
really useful considering the added complexity, though).

Samuel

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

[Markus Armbruster]

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

> Markus Armbruster, le Tue 06 May 2008 18:50:04 +0200, a écrit :
>> > Instead of expressing idleness or "status", maybe we could rather
>> > express whether periodic update notifications are wanted or not, and let
>> > the backend request an explicit update notification when it feels the
>> > need for one (low-frequency thumbnail update). It has the advantage of
>> > only talking about the PVFB protocol itself and not something around it
>> > (idleness of the actual output).  That is also backward compatible in
>> > that a frontend which doesn't know these two events will just continue
>> > sending periodic update notifications, which is fine for the backend.
>> 
>> I think that's a better way to define this feature.
>
> More precisely, we could have an UPDATE_PERIOD event which carries
> an advice from the backend about how often the frontend should sent
> update notifications (0 if periodic notification is not useful), and a
> REQUEST_UPDATE event that requests a one-time update notification?  The
> latter could even contain the requested area? (not sure whether it'd be
> really useful considering the added complexity, though).
>
> Samuel

I figure your UPDATE_PERIOD advice event is a sensible way to solve
your problem.  Frontends are free to use the advice as they see fit.

Why do you need REQUEST_UPDATE?  Perhaps because your frontend doesn't
want to keep the shared framebuffer up-to-date?

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

[Samuel Thibault]

Markus Armbruster, le Wed 07 May 2008 16:43:38 +0200, a écrit :
> Why do you need REQUEST_UPDATE?  Perhaps because your frontend doesn't
> want to keep the shared framebuffer up-to-date?

Yes, because it is expensive.

Samuel

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

[Markus Armbruster]

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

> Markus Armbruster, le Wed 07 May 2008 16:43:38 +0200, a écrit :
>> Why do you need REQUEST_UPDATE?  Perhaps because your frontend doesn't
>> want to keep the shared framebuffer up-to-date?
>
> Yes, because it is expensive.
>
> Samuel

Strictly speaking, frame buffer update and update notification events
are separate things.

The PVFB protocol (tacitly) assumes that the framebuffer shared by the
frontend gets updated as the guest draws in it.  The update
notification event is (designed to be) optional.  The backend doesn't
actually implement the optionality, it simply bails out when it can't
get update notifications.

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?

This is of course all semantic fine print, but getting that wrong can
be very confusing.

So, if I'm guessing right and you need to control updates, then what
about this: have an fb in event to advise on updates.  It contains a
suggested update frequency.  Frontends that always keep the
framebuffer in sync ignore it.  Frontends that don't keep it in sync
should immediately update it (and send an update notification for
that), and use the update frequency to guide updates until further
notice.

One could put the area to be updated into the event, but I can't see
practical applications for that.

I believe this is pretty close to what you have in mind.  But I could
be wrong.

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

[Samuel Thibault]

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.



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 -r b0d7780794eb extras/mini-os/fbfront.c
--- a/extras/mini-os/fbfront.c	Thu May 08 13:40:40 2008 +0100
+++ b/extras/mini-os/fbfront.c	Thu May 08 15:47:13 2008 +0100
@@ -255,11 +255,55 @@ struct fbfront_dev {
     int offset;
 
     xenbus_event_queue events;
+
+#ifdef HAVE_LIBC
+    int fd;
+#endif
 };
 
 void fbfront_handler(evtchn_port_t port, struct pt_regs *regs, void *data)
 {
+#ifdef HAVE_LIBC
+    struct fbfront_dev *dev = data;
+    int fd = dev->fd;
+
+    files[fd].read = 1;
+#endif
     wake_up(&fbfront_queue);
+}
+
+int fbfront_receive(struct fbfront_dev *dev, union xenfb_in_event *buf, int n)
+{
+    struct xenfb_page *page = dev->page;
+    uint32_t prod, cons;
+    int i;
+
+#ifdef HAVE_LIBC
+    files[dev->fd].read = 0;
+    mb(); /* Make sure to let the handler set read to 1 before we start looking at the ring */
+#endif
+
+    prod = page->in_prod;
+
+    if (prod == page->in_cons)
+        return 0;
+
+    rmb();      /* ensure we see ring contents up to prod */
+
+    for (i = 0, cons = page->in_cons; i < n && cons != prod; i++, cons++)
+        memcpy(buf + i, &XENFB_IN_RING_REF(page, cons), sizeof(*buf));
+
+    mb();       /* ensure we got ring contents */
+    page->in_cons = cons;
+    notify_remote_via_evtchn(dev->evtchn);
+
+#ifdef HAVE_LIBC
+    if (cons != prod)
+        /* still some events to read */
+        files[dev->fd].read = 1;
+#endif
+
+    return i;
 }
 
 struct fbfront_dev *init_fbfront(char *nodename, unsigned long *mfns, int width, int height, int depth, int stride, int n)
@@ -482,3 +526,14 @@ void shutdown_fbfront(struct fbfront_dev
     free(dev->backend);
     free(dev);
 }
+
+#ifdef HAVE_LIBC
+int fbfront_open(struct fbfront_dev *dev)
+{
+    dev->fd = alloc_fd(FTYPE_FB);
+    printk("fb_open(%s) -> %d\n", dev->nodename, dev->fd);
+    files[dev->fd].fb.dev = dev;
+    return dev->fd;
+}
+#endif
+
diff -r b0d7780794eb extras/mini-os/include/fbfront.h
--- a/extras/mini-os/include/fbfront.h	Thu May 08 13:40:40 2008 +0100
+++ b/extras/mini-os/include/fbfront.h	Thu May 08 15:47:13 2008 +0100
@@ -1,4 +1,5 @@
 #include <xen/io/kbdif.h>
+#include <xen/io/fbif.h>
 #include <wait.h>
 
 /* from <linux/input.h> */
@@ -36,6 +37,8 @@ int fbfront_open(struct fbfront_dev *dev
 int fbfront_open(struct fbfront_dev *dev);
 #endif
 
+int fbfront_receive(struct fbfront_dev *dev, union xenfb_in_event *buf, int n);
+extern struct wait_queue_head fbfront_queue;
 void fbfront_update(struct fbfront_dev *dev, int x, int y, int width, int height);
 void fbfront_resize(struct fbfront_dev *dev, int width, int height, int stride, int depth, int offset);
 
diff -r b0d7780794eb extras/mini-os/include/lib.h
--- a/extras/mini-os/include/lib.h	Thu May 08 13:40:40 2008 +0100
+++ b/extras/mini-os/include/lib.h	Thu May 08 15:47:13 2008 +0100
@@ -141,6 +141,7 @@ enum fd_type {
     FTYPE_TAP,
     FTYPE_BLK,
     FTYPE_KBD,
+    FTYPE_FB,
 };
 
 #define MAX_EVTCHN_PORTS 16
@@ -175,6 +176,9 @@ extern struct file {
 	struct {
 	    struct kbdfront_dev *dev;
 	} kbd;
+	struct {
+	    struct fbfront_dev *dev;
+	} fb;
         struct {
             /* To each xenbus FD is associated a queue of watch events for this
              * FD.  */
diff -r b0d7780794eb extras/mini-os/kernel.c
--- a/extras/mini-os/kernel.c	Thu May 08 13:40:40 2008 +0100
+++ b/extras/mini-os/kernel.c	Thu May 08 15:47:13 2008 +0100
@@ -260,6 +260,7 @@ static void blkfront_thread(void *p)
 #define DEPTH 32
 
 static uint32_t *fb;
+static int refresh_period = 50;
 static struct fbfront_dev *fb_dev;
 static struct semaphore fbfront_sem = __SEMAPHORE_INITIALIZER(fbfront_sem, 0);
 
@@ -333,6 +334,10 @@
 static void refresh_cursor(int new_x, int new_y)
 {
     static int old_x = -1, old_y = -1;
+
+    if (!refresh_period)
+        return;
+
     if (old_x != -1 && old_y != -1) {
         fbfront_drawvert(old_x, old_y + 1, old_y + 8, 0xffffffff);
         fbfront_drawhoriz(old_x + 1, old_x + 8, old_y, 0xffffffff);
@@ -358,43 +363,46 @@ static void kbdfront_thread(void *p)
     down(&fbfront_sem);
     refresh_cursor(x, y);
     while (1) {
-        union xenkbd_in_event event;
+        union xenkbd_in_event kbdevent;
+        union xenfb_in_event fbevent;
+        int sleep = 1;
 
         add_waiter(w, kbdfront_queue);
+        add_waiter(w, fbfront_queue);
 
-        if (kbdfront_receive(kbd_dev, &event, 1) == 0)
-            schedule();
-        else switch(event.type) {
+        while (kbdfront_receive(kbd_dev, &kbdevent, 1) != 0) {
+            sleep = 0;
+            switch(kbdevent.type) {
             case XENKBD_TYPE_MOTION:
                 printk("motion x:%d y:%d z:%d\n",
-                        event.motion.rel_x,
-                        event.motion.rel_y,
-                        event.motion.rel_z);
-                x += event.motion.rel_x;
-                y += event.motion.rel_y;
-                z += event.motion.rel_z;
+                        kbdevent.motion.rel_x,
+                        kbdevent.motion.rel_y,
+                        kbdevent.motion.rel_z);
+                x += kbdevent.motion.rel_x;
+                y += kbdevent.motion.rel_y;
+                z += kbdevent.motion.rel_z;
                 clip_cursor(&x, &y);
                 refresh_cursor(x, y);
                 break;
             case XENKBD_TYPE_POS:
                 printk("pos x:%d y:%d dz:%d\n",
-                        event.pos.abs_x,
-                        event.pos.abs_y,
-                        event.pos.rel_z);
-                x = event.pos.abs_x;
-                y = event.pos.abs_y;
-                z = event.pos.rel_z;
+                        kbdevent.pos.abs_x,
+                        kbdevent.pos.abs_y,
+                        kbdevent.pos.rel_z);
+                x = kbdevent.pos.abs_x;
+                y = kbdevent.pos.abs_y;
+                z = kbdevent.pos.rel_z;
                 clip_cursor(&x, &y);
                 refresh_cursor(x, y);
                 break;
             case XENKBD_TYPE_KEY:
                 printk("key %d %s\n",
-                        event.key.keycode,
-                        event.key.pressed ? "pressed" : "released");
-                if (event.key.keycode == BTN_LEFT) {
+                        kbdevent.key.keycode,
+                        kbdevent.key.pressed ? "pressed" : "released");
+                if (kbdevent.key.keycode == BTN_LEFT) {
                     printk("mouse %s at (%d,%d,%d)\n",
-                            event.key.pressed ? "clic" : "release", x, y, z);
-                    if (event.key.pressed) {
+                            kbdevent.key.pressed ? "clic" : "release", x, y, z);
+                    if (kbdevent.key.pressed) {
                         uint32_t color = rand();
                         fbfront_drawvert(x - 16, y - 16, y + 15, color);
                         fbfront_drawhoriz(x - 16, x + 15, y + 16, color);
@@ -402,13 +410,26 @@ static void kbdfront_thread(void *p)
                         fbfront_drawhoriz(x - 15, x + 16, y - 16, color);
                         fbfront_update(fb_dev, x - 16, y - 16, 33, 33);
                     }
-                } else if (event.key.keycode == KEY_Q) {
+                } else if (kbdevent.key.keycode == KEY_Q) {
                     struct sched_shutdown sched_shutdown = { .reason = SHUTDOWN_poweroff };
                     HYPERVISOR_sched_op(SCHEDOP_shutdown, &sched_shutdown);
                     do_exit();
                 }
                 break;
+            }
         }
+        while (fbfront_receive(fb_dev, &fbevent, 1) != 0) {
+            sleep = 0;
+            switch(fbevent.type) {
+            case XENFB_TYPE_REFRESH_PERIOD:
+                refresh_period = fbevent.refresh_period.period;
+                printk("refresh period %d\n", refresh_period);
+                refresh_cursor(x, y);
+                break;
+            }
+        }
+        if (sleep)
+            schedule();
     }
 }
 
diff -r b0d7780794eb extras/mini-os/lib/sys.c
--- a/extras/mini-os/lib/sys.c	Thu May 08 13:40:40 2008 +0100
+++ b/extras/mini-os/lib/sys.c	Thu May 08 15:47:13 2008 +0100
@@ -249,6 +251,16 @@ int read(int fd, void *buf, size_t nbyte
 	    }
 	    return ret * sizeof(union xenkbd_in_event);
         }
+        case FTYPE_FB: {
+            int ret, n;
+            n = nbytes / sizeof(union xenfb_in_event);
+            ret = fbfront_receive(files[fd].fb.dev, buf, n);
+	    if (ret <= 0) {
+		errno = EAGAIN;
+		return -1;
+	    }
+	    return ret * sizeof(union xenfb_in_event);
+        }
 	case FTYPE_NONE:
 	case FTYPE_XENBUS:
 	case FTYPE_EVTCHN:
@@ -290,6 +302,7 @@ int write(int fd, const void *buf, size_
 	case FTYPE_EVTCHN:
 	case FTYPE_BLK:
 	case FTYPE_KBD:
+	case FTYPE_FB:
 	    break;
     }
     printk("write(%d): Bad descriptor\n", fd);
@@ -348,6 +361,7 @@ int fsync(int fd) {
 	case FTYPE_TAP:
 	case FTYPE_BLK:
 	case FTYPE_KBD:
+	case FTYPE_FB:
 	    break;
     }
     printk("fsync(%d): Bad descriptor\n", fd);
@@ -392,6 +406,10 @@ int close(int fd)
 	    return 0;
 	case FTYPE_KBD:
             shutdown_kbdfront(files[fd].kbd.dev);
+            files[fd].type = FTYPE_NONE;
+            return 0;
+	case FTYPE_FB:
+            shutdown_fbfront(files[fd].fb.dev);
             files[fd].type = FTYPE_NONE;
             return 0;
 	case FTYPE_NONE:
@@ -485,6 +503,7 @@ int fstat(int fd, struct stat *buf)
 	case FTYPE_TAP:
 	case FTYPE_BLK:
 	case FTYPE_KBD:
+	case FTYPE_FB:
 	    break;
     }
 
@@ -513,6 +532,7 @@ int ftruncate(int fd, off_t length)
 	case FTYPE_TAP:
 	case FTYPE_BLK:
 	case FTYPE_KBD:
+	case FTYPE_FB:
 	    break;
     }
 
@@ -624,6 +644,7 @@ static const char file_types[] = {
     [FTYPE_TAP]		= 'T',
     [FTYPE_BLK]		= 'B',
     [FTYPE_KBD]		= 'K',
+    [FTYPE_FB]		= 'G',
 };
 #ifdef LIBC_DEBUG
 static void dump_set(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds, struct timeval *timeout)
@@ -732,6 +753,7 @@ static int select_poll(int nfds, fd_set 
 	case FTYPE_TAP:
 	case FTYPE_BLK:
 	case FTYPE_KBD:
+	case FTYPE_FB:
 	    if (FD_ISSET(i, readfds)) {
 		if (files[i].read)
 		    n++;
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 */
+    if (xenfb->refresh_period != period) {
+        xenfb_send_refresh_period(xenfb, period);
+        xenfb->refresh_period = period;
+    }
 }
 
 /* QEMU display state changed, so refresh the framebuffer copy */
@@ -1232,11 +1287,17 @@ static int xenfb_register_console(struct
 }
 
 #ifdef CONFIG_STUBDOM
-static struct semaphore kbd_sem = __SEMAPHORE_INITIALIZER(kbd_sem, 0);
-static struct kbdfront_dev *kbd_dev;
+typedef struct XenFBState {
+    struct semaphore kbd_sem;
+    struct kbdfront_dev *kbd_dev;
+    struct fbfront_dev *fb_dev;
+    void *vga_vram, *nonshared_vram;
+    DisplayState *ds;
+} XenFBState;
+
+XenFBState *xs;
+
 static char *kbd_path, *fb_path;
-static void *vga_vram, *nonshared_vram;
-static DisplayState *xenfb_ds;
 
 static unsigned char linux2scancode[KEY_MAX + 1];
 
@@ -1254,7 +1315,8 @@ int xenfb_connect_vfb(const char *path)
 
 static void xenfb_pv_update(DisplayState *ds, int x, int y, int w, int h)
 {
-    struct fbfront_dev *fb_dev = ds->opaque;
+    XenFBState *xs = ds->opaque;
+    struct fbfront_dev *fb_dev = xs->fb_dev;
     if (!fb_dev)
         return;
     fbfront_update(fb_dev, x, y, w, h);
@@ -1262,7 +1324,8 @@ static void xenfb_pv_update(DisplayState
 
 static void xenfb_pv_resize(DisplayState *ds, int w, int h, int linesize)
 {
-    struct fbfront_dev *fb_dev = ds->opaque;
+    XenFBState *xs = ds->opaque;
+    struct fbfront_dev *fb_dev = xs->fb_dev;
     fprintf(stderr,"resize to %dx%d, %d required\n", w, h, linesize);
     ds->width = w;
     ds->height = h;
@@ -1276,14 +1339,15 @@ static void xenfb_pv_resize(DisplayState
     if (ds->shared_buf) {
         ds->data = NULL;
     } else {
-        ds->data = nonshared_vram;
+        ds->data = xs->nonshared_vram;
         fbfront_resize(fb_dev, w, h, linesize, ds->depth, VGA_RAM_SIZE);
     }
 }
 
 static void xenfb_pv_colourdepth(DisplayState *ds, int depth)
 {
-    struct fbfront_dev *fb_dev = ds->opaque;
+    XenFBState *xs = ds->opaque;
+    struct fbfront_dev *fb_dev = xs->fb_dev;
     static int lastdepth = -1;
     if (!depth) {
         ds->shared_buf = 0;
@@ -1301,15 +1365,16 @@ static void xenfb_pv_colourdepth(Display
     if (ds->shared_buf) {
         ds->data = NULL;
     } else {
-        ds->data = nonshared_vram;
+        ds->data = xs->nonshared_vram;
         fbfront_resize(fb_dev, ds->width, ds->height, ds->linesize, ds->depth, VGA_RAM_SIZE);
     }
 }
 
 static void xenfb_pv_setdata(DisplayState *ds, void *pixels)
 {
-    struct fbfront_dev *fb_dev = ds->opaque;
-    int offset = pixels - vga_vram;
+    XenFBState *xs = ds->opaque;
+    struct fbfront_dev *fb_dev = xs->fb_dev;
+    int offset = pixels - xs->vga_vram;
     ds->data = pixels;
     if (!fb_dev)
         return;
@@ -1321,16 +1386,45 @@ static void xenfb_pv_refresh(DisplayStat
     vga_hw_update();
 }
 
+static void xenfb_fb_handler(void *opaque)
+{
+#define FB_NUM_BATCH 4
+    union xenfb_in_event buf[FB_NUM_BATCH];
+    int n, i;
+    XenFBState *xs = opaque;
+    DisplayState *ds = xs->ds;
+
+    n = fbfront_receive(xs->fb_dev, buf, FB_NUM_BATCH);
+    for (i = 0; i < n; i++) {
+        switch (buf[i].type) {
+        case XENFB_TYPE_REFRESH_PERIOD:
+            if (buf[i].refresh_period.period) {
+                /* Set interval */
+                ds->idle = 0;
+                ds->gui_timer_interval = buf[i].refresh_period.period;
+            } else {
+                /* Sleeping interval */
+                ds->idle = 1;
+                ds->gui_timer_interval = 500;
+            }
+        default:
+            /* ignore unknown events */
+            break;
+        }
+    }
+}
+
 static void xenfb_kbd_handler(void *opaque)
 {
 #define KBD_NUM_BATCH 64
     union xenkbd_in_event buf[KBD_NUM_BATCH];
     int n, i;
-    DisplayState *s = opaque;
+    XenFBState *xs = opaque;
+    DisplayState *s = xs->ds;
     static int buttons;
     static int x, y;
 
-    n = kbdfront_receive(kbd_dev, buf, KBD_NUM_BATCH);
+    n = kbdfront_receive(xs->kbd_dev, buf, KBD_NUM_BATCH);
     for (i = 0; i < n; i++) {
         switch (buf[i].type) {
 
@@ -1412,12 +1506,13 @@ static void kbdfront_thread(void *p)
 static void kbdfront_thread(void *p)
 {
     int scancode, keycode;
-    kbd_dev = init_kbdfront(p, 1);
-    if (!kbd_dev) {
+    XenFBState *xs = p;
+    xs->kbd_dev = init_kbdfront(kbd_path, 1);
+    if (!xs->kbd_dev) {
         fprintf(stderr,"can't open keyboard\n");
         exit(1);
     }
-    up(&kbd_sem);
+    up(&xs->kbd_sem);
     for (scancode = 0; scancode < 128; scancode++) {
         keycode = atkbd_set2_keycode[atkbd_unxlate_table[scancode]];
         linux2scancode[keycode] = scancode;
@@ -1431,12 +1526,18 @@ int xenfb_pv_display_init(DisplayState *
     if (!fb_path || !kbd_path)
         return -1;
 
-    create_thread("kbdfront", kbdfront_thread, (void*) kbd_path);
+    xs = qemu_mallocz(sizeof(XenFBState));
+    if (!xs)
+        return -1;
 
-    xenfb_ds = ds;
+    init_SEMAPHORE(&xs->kbd_sem, 0);
+    xs->ds = ds;
 
-    ds->data = nonshared_vram = qemu_memalign(PAGE_SIZE, VGA_RAM_SIZE);
+    create_thread("kbdfront", kbdfront_thread, (void*) xs);
+
+    ds->data = xs->nonshared_vram = qemu_memalign(PAGE_SIZE, VGA_RAM_SIZE);
     memset(ds->data, 0, VGA_RAM_SIZE);
+    ds->opaque = xs;
     ds->depth = 32;
     ds->bgr = 0;
     ds->width = 640;
@@ -1452,9 +1553,9 @@ int xenfb_pv_display_init(DisplayState *
 
 int xenfb_pv_display_start(void *data)
 {
-    DisplayState *ds = xenfb_ds;
+    DisplayState *ds = xs->ds;
     struct fbfront_dev *fb_dev;
-    int kbd_fd;
+    int kbd_fd, fb_fd;
     int offset = 0;
     unsigned long *mfns;
     int n = VGA_RAM_SIZE / PAGE_SIZE;
@@ -1463,12 +1564,12 @@ int xenfb_pv_display_start(void *data)
     if (!fb_path || !kbd_path)
         return 0;
 
-    vga_vram = data;
+    xs->vga_vram = data;
     mfns = malloc(2 * n * sizeof(*mfns));
     for (i = 0; i < n; i++)
-        mfns[i] = virtual_to_mfn(vga_vram + i * PAGE_SIZE);
+        mfns[i] = virtual_to_mfn(xs->vga_vram + i * PAGE_SIZE);
     for (i = 0; i < n; i++)
-        mfns[n + i] = virtual_to_mfn(nonshared_vram + i * PAGE_SIZE);
+        mfns[n + i] = virtual_to_mfn(xs->nonshared_vram + i * PAGE_SIZE);
 
     fb_dev = init_fbfront(fb_path, mfns, ds->width, ds->height, ds->depth, ds->linesize, 2 * n);
     free(mfns);
@@ -1479,21 +1580,24 @@ int xenfb_pv_display_start(void *data)
     free(fb_path);
 
     if (ds->shared_buf) {
-        offset = (void*) ds->data - vga_vram;
+        offset = (void*) ds->data - xs->vga_vram;
     } else {
         offset = VGA_RAM_SIZE;
-        ds->data = nonshared_vram;
+        ds->data = xs->nonshared_vram;
     }
     if (offset)
         fbfront_resize(fb_dev, ds->width, ds->height, ds->linesize, ds->depth, offset);
 
-    down(&kbd_sem);
+    down(&xs->kbd_sem);
     free(kbd_path);
 
-    kbd_fd = kbdfront_open(kbd_dev);
-    qemu_set_fd_handler(kbd_fd, xenfb_kbd_handler, NULL, ds);
+    kbd_fd = kbdfront_open(xs->kbd_dev);
+    qemu_set_fd_handler(kbd_fd, xenfb_kbd_handler, NULL, xs);
 
-    xenfb_ds->opaque = fb_dev;
+    fb_fd = fbfront_open(fb_dev);
+    qemu_set_fd_handler(fb_fd, xenfb_fb_handler, NULL, xs);
+
+    xs->fb_dev = fb_dev;
     return 0;
 }
 #endif
diff -r b0d7780794eb tools/ioemu/sdl.c
--- a/tools/ioemu/sdl.c	Thu May 08 13:40:40 2008 +0100
+++ b/tools/ioemu/sdl.c	Thu May 08 15:47:13 2008 +0100
@@ -696,9 +696,11 @@ static int select_poll(int nfds, fd_set 
 		if (ev->active.gain) {
 		    /* Back to default interval */
 		    ds->gui_timer_interval = 0;
+		    ds->idle = 0;
 		} else {
 		    /* Sleeping interval */
 		    ds->gui_timer_interval = 500;
+		    ds->idle = 1;
 		}
 	    }
             break;
diff -r b0d7780794eb tools/ioemu/vl.c
--- a/tools/ioemu/vl.c	Thu May 08 13:40:40 2008 +0100
+++ b/tools/ioemu/vl.c	Thu May 08 15:47:13 2008 +0100
@@ -130,8 +130,6 @@
 #else
 #define DEFAULT_RAM_SIZE 128
 #endif
-/* in ms */
-#define GUI_REFRESH_INTERVAL 30
 
 /* Max number of USB devices that can be specified on the commandline.  */
 #define MAX_USB_CMDLINE 8
@@ -4467,6 +4465,8 @@ void dumb_display_init(DisplayState *ds)
     ds->dpy_resize = dumb_resize;
     ds->dpy_colourdepth = NULL;
     ds->dpy_refresh = dumb_refresh;
+    ds->gui_timer_interval = 500;
+    ds->idle = 1;
 }
 
 /***********************************************************/
diff -r b0d7780794eb tools/ioemu/vl.h
--- a/tools/ioemu/vl.h	Thu May 08 13:40:40 2008 +0100
+++ b/tools/ioemu/vl.h	Thu May 08 15:47:13 2008 +0100
@@ -929,6 +929,9 @@ extern struct soundhw soundhw[];
 
 #define VGA_RAM_SIZE (8192 * 1024)
 
+/* in ms */
+#define GUI_REFRESH_INTERVAL 30
+
 struct DisplayState {
     uint8_t *data;
     int linesize;
@@ -939,6 +942,7 @@ struct DisplayState {
     void *opaque;
     uint32_t *palette;
     uint64_t gui_timer_interval;
+    int idle;
 
     int shared_buf;
     
diff -r b0d7780794eb tools/ioemu/vnc.c
--- a/tools/ioemu/vnc.c	Thu May 08 13:40:40 2008 +0100
+++ b/tools/ioemu/vnc.c	Thu May 08 15:47:13 2008 +0100
@@ -778,6 +778,7 @@ static void _vnc_update_client(void *opa
     vs->has_update = 0;
     vnc_flush(vs);
     vs->last_update_time = now;
+    vs->ds->idle = 0;
 
     vs->timer_interval /= 2;
     if (vs->timer_interval < VNC_REFRESH_INTERVAL_BASE)
@@ -790,26 +791,29 @@ static void _vnc_update_client(void *opa
     vs->timer_interval += VNC_REFRESH_INTERVAL_INC;
     if (vs->timer_interval > VNC_REFRESH_INTERVAL_MAX) {
 	vs->timer_interval = VNC_REFRESH_INTERVAL_MAX;
-	if (now - vs->last_update_time >= VNC_MAX_UPDATE_INTERVAL &&
-            vs->update_requested) {
-	    /* Send a null update.  If the client is no longer
-	       interested (e.g. minimised) it'll ignore this, and we
-	       can stop scanning the buffer until it sends another
-	       update request. */
-	    /* It turns out that there's a bug in realvncviewer 4.1.2
-	       which means that if you send a proper null update (with
-	       no update rectangles), it gets a bit out of sync and
-	       never sends any further requests, regardless of whether
-	       it needs one or not.  Fix this by sending a single 1x1
-	       update rectangle instead. */
-	    vnc_write_u8(vs, 0);
-	    vnc_write_u8(vs, 0);
-	    vnc_write_u16(vs, 1);
-	    send_framebuffer_update(vs, 0, 0, 1, 1);
-	    vnc_flush(vs);
-	    vs->last_update_time = now;
-            vs->update_requested--;
-	    return;
+	if (now - vs->last_update_time >= VNC_MAX_UPDATE_INTERVAL) {
+            if (!vs->update_requested) {
+                vs->ds->idle = 1;
+            } else {
+                /* Send a null update.  If the client is no longer
+                   interested (e.g. minimised) it'll ignore this, and we
+                   can stop scanning the buffer until it sends another
+                   update request. */
+                /* It turns out that there's a bug in realvncviewer 4.1.2
+                   which means that if you send a proper null update (with
+                   no update rectangles), it gets a bit out of sync and
+                   never sends any further requests, regardless of whether
+                   it needs one or not.  Fix this by sending a single 1x1
+                   update rectangle instead. */
+                vnc_write_u8(vs, 0);
+                vnc_write_u8(vs, 0);
+                vnc_write_u16(vs, 1);
+                send_framebuffer_update(vs, 0, 0, 1, 1);
+                vnc_flush(vs);
+                vs->last_update_time = now;
+                vs->update_requested--;
+                return;
+            }
 	}
     }
     qemu_mod_timer(vs->timer, now + vs->timer_interval);
@@ -970,6 +974,7 @@ static int vnc_client_io_error(VncState 
 	qemu_set_fd_handler2(vs->csock, NULL, NULL, NULL, NULL);
 	closesocket(vs->csock);
 	vs->csock = -1;
+	vs->ds->idle = 1;
 	buffer_reset(&vs->input);
 	buffer_reset(&vs->output);
         free_queue(vs);
@@ -2443,6 +2448,7 @@ static void vnc_listen_read(void *opaque
     vs->csock = accept(vs->lsock, (struct sockaddr *)&addr, &addrlen);
     if (vs->csock != -1) {
 	VNC_DEBUG("New client on socket %d\n", vs->csock);
+	vs->ds->idle = 0;
         socket_set_nonblock(vs->csock);
 	qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, NULL, opaque);
 	vnc_write(vs, "RFB 003.008\n", 12);
@@ -2468,6 +2474,7 @@ void vnc_display_init(DisplayState *ds)
 	exit(1);
 
     ds->opaque = vs;
+    ds->idle = 1;
     vnc_state = vs;
     vs->display = NULL;
     vs->password = NULL;
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.
  */
+#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;
     char pad[XENFB_IN_EVENT_SIZE];
 };

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

[Markus Armbruster]

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];
>  };
>  

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

[Samuel Thibault]

Markus Armbruster, le Fri 09 May 2008 10:43:15 +0200, a écrit :
> > +    /* 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?

Considering the clarification of the semantic, it can be dropped indeed.

> > + *
> > + * 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.

Oops.

> What about this:

Ok.

> > +#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?

There is one in the structure above.

> 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.

The "problem" of an integer frequency is that it does not permit a
period of more than one second, which may become a limit in some odd
situations (thousands of VMs waking every second?)

Samuel

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

[Markus Armbruster]

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

> Markus Armbruster, le Fri 09 May 2008 10:43:15 +0200, a écrit :
[...]
>> >  union xenfb_in_event
>> >  {
>> >      uint8_t type;
>> > +    struct xenfb_refresh_period refresh_period;
>> 
>> Time unit?
>
> There is one in the structure above.

I was confused.

>> 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.
>
> The "problem" of an integer frequency is that it does not permit a
> period of more than one second, which may become a limit in some odd
> situations (thousands of VMs waking every second?)
>
> Samuel

Okay.  Any convenient encoding for the values you're interested in
should do, as long as it's documented clearly.

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

[Samuel Thibault]

Updated patch, the only changes are the addition of a XENFB_NO_REFRESH
macro and comment fixup (as well as a small fixup for the VNC case).

Samuel



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 -r b0d7780794eb extras/mini-os/fbfront.c
--- a/extras/mini-os/fbfront.c	Thu May 08 13:40:40 2008 +0100
+++ b/extras/mini-os/fbfront.c	Fri May 09 14:35:17 2008 +0100
@@ -255,11 +255,55 @@
     int offset;
 
     xenbus_event_queue events;
+
+#ifdef HAVE_LIBC
+    int fd;
+#endif
 };
 
 void fbfront_handler(evtchn_port_t port, struct pt_regs *regs, void *data)
 {
+#ifdef HAVE_LIBC
+    struct fbfront_dev *dev = data;
+    int fd = dev->fd;
+
+    files[fd].read = 1;
+#endif
     wake_up(&fbfront_queue);
+}
+
+int fbfront_receive(struct fbfront_dev *dev, union xenfb_in_event *buf, int n)
+{
+    struct xenfb_page *page = dev->page;
+    uint32_t prod, cons;
+    int i;
+
+#ifdef HAVE_LIBC
+    files[dev->fd].read = 0;
+    mb(); /* Make sure to let the handler set read to 1 before we start looking at the ring */
+#endif
+
+    prod = page->in_prod;
+
+    if (prod == page->in_cons)
+        return 0;
+
+    rmb();      /* ensure we see ring contents up to prod */
+
+    for (i = 0, cons = page->in_cons; i < n && cons != prod; i++, cons++)
+        memcpy(buf + i, &XENFB_IN_RING_REF(page, cons), sizeof(*buf));
+
+    mb();       /* ensure we got ring contents */
+    page->in_cons = cons;
+    notify_remote_via_evtchn(dev->evtchn);
+
+#ifdef HAVE_LIBC
+    if (cons != prod)
+        /* still some events to read */
+        files[dev->fd].read = 1;
+#endif
+
+    return i;
 }
 
 struct fbfront_dev *init_fbfront(char *nodename, unsigned long *mfns, int width, int height, int depth, int stride, int n)
@@ -482,3 +526,14 @@
     free(dev->backend);
     free(dev);
 }
+
+#ifdef HAVE_LIBC
+int fbfront_open(struct fbfront_dev *dev)
+{
+    dev->fd = alloc_fd(FTYPE_FB);
+    printk("fb_open(%s) -> %d\n", dev->nodename, dev->fd);
+    files[dev->fd].fb.dev = dev;
+    return dev->fd;
+}
+#endif
+
--- a/extras/mini-os/include/fbfront.h	Thu May 08 13:40:40 2008 +0100
+++ b/extras/mini-os/include/fbfront.h	Fri May 09 14:35:17 2008 +0100
@@ -1,4 +1,5 @@
 #include <xen/io/kbdif.h>
+#include <xen/io/fbif.h>
 #include <wait.h>
 
 /* from <linux/input.h> */
@@ -36,6 +37,8 @@
 int fbfront_open(struct fbfront_dev *dev);
 #endif
 
+int fbfront_receive(struct fbfront_dev *dev, union xenfb_in_event *buf, int n);
+extern struct wait_queue_head fbfront_queue;
 void fbfront_update(struct fbfront_dev *dev, int x, int y, int width, int height);
 void fbfront_resize(struct fbfront_dev *dev, int width, int height, int stride, int depth, int offset);
 
--- a/extras/mini-os/include/lib.h	Thu May 08 13:40:40 2008 +0100
+++ b/extras/mini-os/include/lib.h	Fri May 09 14:35:17 2008 +0100
@@ -141,6 +141,7 @@
     FTYPE_TAP,
     FTYPE_BLK,
     FTYPE_KBD,
+    FTYPE_FB,
 };
 
 #define MAX_EVTCHN_PORTS 16
@@ -175,6 +176,9 @@
 	struct {
 	    struct kbdfront_dev *dev;
 	} kbd;
+	struct {
+	    struct fbfront_dev *dev;
+	} fb;
         struct {
             /* To each xenbus FD is associated a queue of watch events for this
              * FD.  */
--- a/extras/mini-os/kernel.c	Thu May 08 13:40:40 2008 +0100
+++ b/extras/mini-os/kernel.c	Fri May 09 14:35:18 2008 +0100
@@ -260,6 +260,7 @@
 #define DEPTH 32
 
 static uint32_t *fb;
+static int refresh_period = 50;
 static struct fbfront_dev *fb_dev;
 static struct semaphore fbfront_sem = __SEMAPHORE_INITIALIZER(fbfront_sem, 0);
 
@@ -333,6 +334,10 @@
 static void refresh_cursor(int new_x, int new_y)
 {
     static int old_x = -1, old_y = -1;
+
+    if (!refresh_period)
+        return;
+
     if (old_x != -1 && old_y != -1) {
         fbfront_drawvert(old_x, old_y + 1, old_y + 8, 0xffffffff);
         fbfront_drawhoriz(old_x + 1, old_x + 8, old_y, 0xffffffff);
@@ -358,43 +363,46 @@
     down(&fbfront_sem);
     refresh_cursor(x, y);
     while (1) {
-        union xenkbd_in_event event;
+        union xenkbd_in_event kbdevent;
+        union xenfb_in_event fbevent;
+        int sleep = 1;
 
         add_waiter(w, kbdfront_queue);
+        add_waiter(w, fbfront_queue);
 
-        if (kbdfront_receive(kbd_dev, &event, 1) == 0)
-            schedule();
-        else switch(event.type) {
+        while (kbdfront_receive(kbd_dev, &kbdevent, 1) != 0) {
+            sleep = 0;
+            switch(kbdevent.type) {
             case XENKBD_TYPE_MOTION:
                 printk("motion x:%d y:%d z:%d\n",
-                        event.motion.rel_x,
-                        event.motion.rel_y,
-                        event.motion.rel_z);
-                x += event.motion.rel_x;
-                y += event.motion.rel_y;
-                z += event.motion.rel_z;
+                        kbdevent.motion.rel_x,
+                        kbdevent.motion.rel_y,
+                        kbdevent.motion.rel_z);
+                x += kbdevent.motion.rel_x;
+                y += kbdevent.motion.rel_y;
+                z += kbdevent.motion.rel_z;
                 clip_cursor(&x, &y);
                 refresh_cursor(x, y);
                 break;
             case XENKBD_TYPE_POS:
                 printk("pos x:%d y:%d dz:%d\n",
-                        event.pos.abs_x,
-                        event.pos.abs_y,
-                        event.pos.rel_z);
-                x = event.pos.abs_x;
-                y = event.pos.abs_y;
-                z = event.pos.rel_z;
+                        kbdevent.pos.abs_x,
+                        kbdevent.pos.abs_y,
+                        kbdevent.pos.rel_z);
+                x = kbdevent.pos.abs_x;
+                y = kbdevent.pos.abs_y;
+                z = kbdevent.pos.rel_z;
                 clip_cursor(&x, &y);
                 refresh_cursor(x, y);
                 break;
             case XENKBD_TYPE_KEY:
                 printk("key %d %s\n",
-                        event.key.keycode,
-                        event.key.pressed ? "pressed" : "released");
-                if (event.key.keycode == BTN_LEFT) {
+                        kbdevent.key.keycode,
+                        kbdevent.key.pressed ? "pressed" : "released");
+                if (kbdevent.key.keycode == BTN_LEFT) {
                     printk("mouse %s at (%d,%d,%d)\n",
-                            event.key.pressed ? "clic" : "release", x, y, z);
-                    if (event.key.pressed) {
+                            kbdevent.key.pressed ? "clic" : "release", x, y, z);
+                    if (kbdevent.key.pressed) {
                         uint32_t color = rand();
                         fbfront_drawvert(x - 16, y - 16, y + 15, color);
                         fbfront_drawhoriz(x - 16, x + 15, y + 16, color);
@@ -402,13 +410,26 @@
                         fbfront_drawhoriz(x - 15, x + 16, y - 16, color);
                         fbfront_update(fb_dev, x - 16, y - 16, 33, 33);
                     }
-                } else if (event.key.keycode == KEY_Q) {
+                } else if (kbdevent.key.keycode == KEY_Q) {
                     struct sched_shutdown sched_shutdown = { .reason = SHUTDOWN_poweroff };
                     HYPERVISOR_sched_op(SCHEDOP_shutdown, &sched_shutdown);
                     do_exit();
                 }
                 break;
+            }
         }
+        while (fbfront_receive(fb_dev, &fbevent, 1) != 0) {
+            sleep = 0;
+            switch(fbevent.type) {
+            case XENFB_TYPE_REFRESH_PERIOD:
+                refresh_period = fbevent.refresh_period.period;
+                printk("refresh period %d\n", refresh_period);
+                refresh_cursor(x, y);
+                break;
+            }
+        }
+        if (sleep)
+            schedule();
     }
 }
 
--- a/extras/mini-os/lib/sys.c	Thu May 08 13:40:40 2008 +0100
+++ b/extras/mini-os/lib/sys.c	Fri May 09 14:35:18 2008 +0100
@@ -249,6 +249,16 @@
 	    }
 	    return ret * sizeof(union xenkbd_in_event);
         }
+        case FTYPE_FB: {
+            int ret, n;
+            n = nbytes / sizeof(union xenfb_in_event);
+            ret = fbfront_receive(files[fd].fb.dev, buf, n);
+	    if (ret <= 0) {
+		errno = EAGAIN;
+		return -1;
+	    }
+	    return ret * sizeof(union xenfb_in_event);
+        }
 	case FTYPE_NONE:
 	case FTYPE_XENBUS:
 	case FTYPE_EVTCHN:
@@ -290,6 +300,7 @@
 	case FTYPE_EVTCHN:
 	case FTYPE_BLK:
 	case FTYPE_KBD:
+	case FTYPE_FB:
 	    break;
     }
     printk("write(%d): Bad descriptor\n", fd);
@@ -348,6 +359,7 @@
 	case FTYPE_TAP:
 	case FTYPE_BLK:
 	case FTYPE_KBD:
+	case FTYPE_FB:
 	    break;
     }
     printk("fsync(%d): Bad descriptor\n", fd);
@@ -392,6 +404,10 @@
 	    return 0;
 	case FTYPE_KBD:
             shutdown_kbdfront(files[fd].kbd.dev);
+            files[fd].type = FTYPE_NONE;
+            return 0;
+	case FTYPE_FB:
+            shutdown_fbfront(files[fd].fb.dev);
             files[fd].type = FTYPE_NONE;
             return 0;
 	case FTYPE_NONE:
@@ -485,6 +501,7 @@
 	case FTYPE_TAP:
 	case FTYPE_BLK:
 	case FTYPE_KBD:
+	case FTYPE_FB:
 	    break;
     }
 
@@ -513,6 +530,7 @@
 	case FTYPE_TAP:
 	case FTYPE_BLK:
 	case FTYPE_KBD:
+	case FTYPE_FB:
 	    break;
     }
 
@@ -624,6 +642,7 @@
     [FTYPE_TAP]		= 'T',
     [FTYPE_BLK]		= 'B',
     [FTYPE_KBD]		= 'K',
+    [FTYPE_FB]		= 'G',
 };
 #ifdef LIBC_DEBUG
 static void dump_set(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds, struct timeval *timeout)
@@ -732,6 +751,7 @@
 	case FTYPE_TAP:
 	case FTYPE_BLK:
 	case FTYPE_KBD:
+	case FTYPE_FB:
 	    if (FD_ISSET(i, readfds)) {
 		if (files[i].read)
 		    n++;
--- a/tools/ioemu/hw/xenfb.c	Thu May 08 13:40:40 2008 +0100
+++ b/tools/ioemu/hw/xenfb.c	Fri May 09 14:35:19 2008 +0100
@@ -59,6 +59,7 @@
 	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 @@
 	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 @@
                             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 @@
     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 = XENFB_NO_REFRESH;
+    else {
+        period = xenfb->ds->gui_timer_interval;
+        if (!period)
+            period = GUI_REFRESH_INTERVAL;
+    }
+
+    /* Will have to be disabled for frontends without feature-update */
+    if (xenfb->refresh_period != period) {
+        xenfb_send_refresh_period(xenfb, period);
+        xenfb->refresh_period = period;
+    }
 }
 
 /* QEMU display state changed, so refresh the framebuffer copy */
@@ -1232,11 +1287,17 @@
 }
 
 #ifdef CONFIG_STUBDOM
-static struct semaphore kbd_sem = __SEMAPHORE_INITIALIZER(kbd_sem, 0);
-static struct kbdfront_dev *kbd_dev;
+typedef struct XenFBState {
+    struct semaphore kbd_sem;
+    struct kbdfront_dev *kbd_dev;
+    struct fbfront_dev *fb_dev;
+    void *vga_vram, *nonshared_vram;
+    DisplayState *ds;
+} XenFBState;
+
+XenFBState *xs;
+
 static char *kbd_path, *fb_path;
-static void *vga_vram, *nonshared_vram;
-static DisplayState *xenfb_ds;
 
 static unsigned char linux2scancode[KEY_MAX + 1];
 
@@ -1254,7 +1315,8 @@
 
 static void xenfb_pv_update(DisplayState *ds, int x, int y, int w, int h)
 {
-    struct fbfront_dev *fb_dev = ds->opaque;
+    XenFBState *xs = ds->opaque;
+    struct fbfront_dev *fb_dev = xs->fb_dev;
     if (!fb_dev)
         return;
     fbfront_update(fb_dev, x, y, w, h);
@@ -1262,7 +1324,8 @@
 
 static void xenfb_pv_resize(DisplayState *ds, int w, int h, int linesize)
 {
-    struct fbfront_dev *fb_dev = ds->opaque;
+    XenFBState *xs = ds->opaque;
+    struct fbfront_dev *fb_dev = xs->fb_dev;
     fprintf(stderr,"resize to %dx%d, %d required\n", w, h, linesize);
     ds->width = w;
     ds->height = h;
@@ -1276,14 +1339,15 @@
     if (ds->shared_buf) {
         ds->data = NULL;
     } else {
-        ds->data = nonshared_vram;
+        ds->data = xs->nonshared_vram;
         fbfront_resize(fb_dev, w, h, linesize, ds->depth, VGA_RAM_SIZE);
     }
 }
 
 static void xenfb_pv_colourdepth(DisplayState *ds, int depth)
 {
-    struct fbfront_dev *fb_dev = ds->opaque;
+    XenFBState *xs = ds->opaque;
+    struct fbfront_dev *fb_dev = xs->fb_dev;
     static int lastdepth = -1;
     if (!depth) {
         ds->shared_buf = 0;
@@ -1301,15 +1365,16 @@
     if (ds->shared_buf) {
         ds->data = NULL;
     } else {
-        ds->data = nonshared_vram;
+        ds->data = xs->nonshared_vram;
         fbfront_resize(fb_dev, ds->width, ds->height, ds->linesize, ds->depth, VGA_RAM_SIZE);
     }
 }
 
 static void xenfb_pv_setdata(DisplayState *ds, void *pixels)
 {
-    struct fbfront_dev *fb_dev = ds->opaque;
-    int offset = pixels - vga_vram;
+    XenFBState *xs = ds->opaque;
+    struct fbfront_dev *fb_dev = xs->fb_dev;
+    int offset = pixels - xs->vga_vram;
     ds->data = pixels;
     if (!fb_dev)
         return;
@@ -1321,16 +1386,45 @@
     vga_hw_update();
 }
 
+static void xenfb_fb_handler(void *opaque)
+{
+#define FB_NUM_BATCH 4
+    union xenfb_in_event buf[FB_NUM_BATCH];
+    int n, i;
+    XenFBState *xs = opaque;
+    DisplayState *ds = xs->ds;
+
+    n = fbfront_receive(xs->fb_dev, buf, FB_NUM_BATCH);
+    for (i = 0; i < n; i++) {
+        switch (buf[i].type) {
+        case XENFB_TYPE_REFRESH_PERIOD:
+            if (buf[i].refresh_period.period == XENFB_NO_REFRESH) {
+                /* Sleeping interval */
+                ds->idle = 1;
+                ds->gui_timer_interval = 500;
+            } else {
+                /* Set interval */
+                ds->idle = 0;
+                ds->gui_timer_interval = buf[i].refresh_period.period;
+            }
+        default:
+            /* ignore unknown events */
+            break;
+        }
+    }
+}
+
 static void xenfb_kbd_handler(void *opaque)
 {
 #define KBD_NUM_BATCH 64
     union xenkbd_in_event buf[KBD_NUM_BATCH];
     int n, i;
-    DisplayState *s = opaque;
+    XenFBState *xs = opaque;
+    DisplayState *s = xs->ds;
     static int buttons;
     static int x, y;
 
-    n = kbdfront_receive(kbd_dev, buf, KBD_NUM_BATCH);
+    n = kbdfront_receive(xs->kbd_dev, buf, KBD_NUM_BATCH);
     for (i = 0; i < n; i++) {
         switch (buf[i].type) {
 
@@ -1412,12 +1506,13 @@
 static void kbdfront_thread(void *p)
 {
     int scancode, keycode;
-    kbd_dev = init_kbdfront(p, 1);
-    if (!kbd_dev) {
+    XenFBState *xs = p;
+    xs->kbd_dev = init_kbdfront(kbd_path, 1);
+    if (!xs->kbd_dev) {
         fprintf(stderr,"can't open keyboard\n");
         exit(1);
     }
-    up(&kbd_sem);
+    up(&xs->kbd_sem);
     for (scancode = 0; scancode < 128; scancode++) {
         keycode = atkbd_set2_keycode[atkbd_unxlate_table[scancode]];
         linux2scancode[keycode] = scancode;
@@ -1431,12 +1526,18 @@
     if (!fb_path || !kbd_path)
         return -1;
 
-    create_thread("kbdfront", kbdfront_thread, (void*) kbd_path);
+    xs = qemu_mallocz(sizeof(XenFBState));
+    if (!xs)
+        return -1;
 
-    xenfb_ds = ds;
+    init_SEMAPHORE(&xs->kbd_sem, 0);
+    xs->ds = ds;
 
-    ds->data = nonshared_vram = qemu_memalign(PAGE_SIZE, VGA_RAM_SIZE);
+    create_thread("kbdfront", kbdfront_thread, (void*) xs);
+
+    ds->data = xs->nonshared_vram = qemu_memalign(PAGE_SIZE, VGA_RAM_SIZE);
     memset(ds->data, 0, VGA_RAM_SIZE);
+    ds->opaque = xs;
     ds->depth = 32;
     ds->bgr = 0;
     ds->width = 640;
@@ -1452,9 +1553,9 @@
 
 int xenfb_pv_display_start(void *data)
 {
-    DisplayState *ds = xenfb_ds;
+    DisplayState *ds;
     struct fbfront_dev *fb_dev;
-    int kbd_fd;
+    int kbd_fd, fb_fd;
     int offset = 0;
     unsigned long *mfns;
     int n = VGA_RAM_SIZE / PAGE_SIZE;
@@ -1463,12 +1564,13 @@
     if (!fb_path || !kbd_path)
         return 0;
 
-    vga_vram = data;
+    ds = xs->ds;
+    xs->vga_vram = data;
     mfns = malloc(2 * n * sizeof(*mfns));
     for (i = 0; i < n; i++)
-        mfns[i] = virtual_to_mfn(vga_vram + i * PAGE_SIZE);
+        mfns[i] = virtual_to_mfn(xs->vga_vram + i * PAGE_SIZE);
     for (i = 0; i < n; i++)
-        mfns[n + i] = virtual_to_mfn(nonshared_vram + i * PAGE_SIZE);
+        mfns[n + i] = virtual_to_mfn(xs->nonshared_vram + i * PAGE_SIZE);
 
     fb_dev = init_fbfront(fb_path, mfns, ds->width, ds->height, ds->depth, ds->linesize, 2 * n);
     free(mfns);
@@ -1479,21 +1581,24 @@
     free(fb_path);
 
     if (ds->shared_buf) {
-        offset = (void*) ds->data - vga_vram;
+        offset = (void*) ds->data - xs->vga_vram;
     } else {
         offset = VGA_RAM_SIZE;
-        ds->data = nonshared_vram;
+        ds->data = xs->nonshared_vram;
     }
     if (offset)
         fbfront_resize(fb_dev, ds->width, ds->height, ds->linesize, ds->depth, offset);
 
-    down(&kbd_sem);
+    down(&xs->kbd_sem);
     free(kbd_path);
 
-    kbd_fd = kbdfront_open(kbd_dev);
-    qemu_set_fd_handler(kbd_fd, xenfb_kbd_handler, NULL, ds);
+    kbd_fd = kbdfront_open(xs->kbd_dev);
+    qemu_set_fd_handler(kbd_fd, xenfb_kbd_handler, NULL, xs);
 
-    xenfb_ds->opaque = fb_dev;
+    fb_fd = fbfront_open(fb_dev);
+    qemu_set_fd_handler(fb_fd, xenfb_fb_handler, NULL, xs);
+
+    xs->fb_dev = fb_dev;
     return 0;
 }
 #endif
--- a/tools/ioemu/sdl.c	Thu May 08 13:40:40 2008 +0100
+++ b/tools/ioemu/sdl.c	Fri May 09 14:35:19 2008 +0100
@@ -696,9 +696,11 @@
 		if (ev->active.gain) {
 		    /* Back to default interval */
 		    ds->gui_timer_interval = 0;
+		    ds->idle = 0;
 		} else {
 		    /* Sleeping interval */
 		    ds->gui_timer_interval = 500;
+		    ds->idle = 1;
 		}
 	    }
             break;
--- a/tools/ioemu/vl.c	Thu May 08 13:40:40 2008 +0100
+++ b/tools/ioemu/vl.c	Fri May 09 14:35:20 2008 +0100
@@ -130,8 +130,6 @@
 #else
 #define DEFAULT_RAM_SIZE 128
 #endif
-/* in ms */
-#define GUI_REFRESH_INTERVAL 30
 
 /* Max number of USB devices that can be specified on the commandline.  */
 #define MAX_USB_CMDLINE 8
@@ -4467,6 +4465,8 @@
     ds->dpy_resize = dumb_resize;
     ds->dpy_colourdepth = NULL;
     ds->dpy_refresh = dumb_refresh;
+    ds->gui_timer_interval = 500;
+    ds->idle = 1;
 }
 
 /***********************************************************/
--- a/tools/ioemu/vl.h	Thu May 08 13:40:40 2008 +0100
+++ b/tools/ioemu/vl.h	Fri May 09 14:35:20 2008 +0100
@@ -929,6 +929,9 @@
 
 #define VGA_RAM_SIZE (8192 * 1024)
 
+/* in ms */
+#define GUI_REFRESH_INTERVAL 30
+
 struct DisplayState {
     uint8_t *data;
     int linesize;
@@ -939,6 +942,7 @@
     void *opaque;
     uint32_t *palette;
     uint64_t gui_timer_interval;
+    int idle;
 
     int shared_buf;
     
--- a/tools/ioemu/vnc.c	Thu May 08 13:40:40 2008 +0100
+++ b/tools/ioemu/vnc.c	Fri May 09 14:35:20 2008 +0100
@@ -778,6 +778,7 @@
     vs->has_update = 0;
     vnc_flush(vs);
     vs->last_update_time = now;
+    vs->ds->idle = 0;
 
     vs->timer_interval /= 2;
     if (vs->timer_interval < VNC_REFRESH_INTERVAL_BASE)
@@ -790,26 +791,29 @@
     vs->timer_interval += VNC_REFRESH_INTERVAL_INC;
     if (vs->timer_interval > VNC_REFRESH_INTERVAL_MAX) {
 	vs->timer_interval = VNC_REFRESH_INTERVAL_MAX;
-	if (now - vs->last_update_time >= VNC_MAX_UPDATE_INTERVAL &&
-            vs->update_requested) {
-	    /* Send a null update.  If the client is no longer
-	       interested (e.g. minimised) it'll ignore this, and we
-	       can stop scanning the buffer until it sends another
-	       update request. */
-	    /* It turns out that there's a bug in realvncviewer 4.1.2
-	       which means that if you send a proper null update (with
-	       no update rectangles), it gets a bit out of sync and
-	       never sends any further requests, regardless of whether
-	       it needs one or not.  Fix this by sending a single 1x1
-	       update rectangle instead. */
-	    vnc_write_u8(vs, 0);
-	    vnc_write_u8(vs, 0);
-	    vnc_write_u16(vs, 1);
-	    send_framebuffer_update(vs, 0, 0, 1, 1);
-	    vnc_flush(vs);
-	    vs->last_update_time = now;
-            vs->update_requested--;
-	    return;
+	if (now - vs->last_update_time >= VNC_MAX_UPDATE_INTERVAL) {
+            if (!vs->update_requested) {
+                vs->ds->idle = 1;
+            } else {
+                /* Send a null update.  If the client is no longer
+                   interested (e.g. minimised) it'll ignore this, and we
+                   can stop scanning the buffer until it sends another
+                   update request. */
+                /* It turns out that there's a bug in realvncviewer 4.1.2
+                   which means that if you send a proper null update (with
+                   no update rectangles), it gets a bit out of sync and
+                   never sends any further requests, regardless of whether
+                   it needs one or not.  Fix this by sending a single 1x1
+                   update rectangle instead. */
+                vnc_write_u8(vs, 0);
+                vnc_write_u8(vs, 0);
+                vnc_write_u16(vs, 1);
+                send_framebuffer_update(vs, 0, 0, 1, 1);
+                vnc_flush(vs);
+                vs->last_update_time = now;
+                vs->update_requested--;
+                return;
+            }
 	}
     }
     qemu_mod_timer(vs->timer, now + vs->timer_interval);
@@ -970,6 +974,7 @@
 	qemu_set_fd_handler2(vs->csock, NULL, NULL, NULL, NULL);
 	closesocket(vs->csock);
 	vs->csock = -1;
+	vs->ds->idle = 1;
 	buffer_reset(&vs->input);
 	buffer_reset(&vs->output);
         free_queue(vs);
@@ -2443,6 +2448,7 @@
     vs->csock = accept(vs->lsock, (struct sockaddr *)&addr, &addrlen);
     if (vs->csock != -1) {
 	VNC_DEBUG("New client on socket %d\n", vs->csock);
+	vs->ds->idle = 0;
         socket_set_nonblock(vs->csock);
 	qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, NULL, opaque);
 	vnc_write(vs, "RFB 003.008\n", 12);
@@ -2468,6 +2474,7 @@
 	exit(1);
 
     ds->opaque = vs;
+    ds->idle = 1;
     vnc_state = vs;
     vs->display = NULL;
     vs->password = NULL;
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	Fri May 09 14:39:43 2008 +0100
@@ -80,14 +80,33 @@
 
 /*
  * Frontends should ignore unknown in events.
- * No in events currently defined.
  */
+
+/*
+ * 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_NO_REFRESH 0
+
+struct xenfb_refresh_period
+{
+    uint8_t type;    /* XENFB_TYPE_UPDATE_PERIOD */
+    uint32_t period; /* period of refresh, in ms,
+                      * XENFB_NO_REFRESH if no refresh is needed */
+};
 
 #define XENFB_IN_EVENT_SIZE 40
 
 union xenfb_in_event
 {
     uint8_t type;
+    struct xenfb_refresh_period refresh_period;
     char pad[XENFB_IN_EVENT_SIZE];
 };