Re: [Xen-devel] [PATCH 1/2] Virtual frame buffer: frontend

["Christian Limpach" <christian.limpach@xxxxxxxxx>]

On 6/26/06, Markus Armbruster <armbru@xxxxxxxxxx> wrote:
> Derived from
> http://www.cs.utexas.edu/users/aliguori/vfb-20060124.bundle
>
> Converted to Xenstore and ported to current kernels.
This looks good.

Could you address the following issues:

- the patch which defined struct xenfb_page seems to be missing

- handling of the ring indexes:
 + we generally prefer to store ring indexes in their unmasked form,
it makes debugging easier and it allows the ring to be used to its
full capacity instead of requiring leaving one slot empty to be able
distinguish empty/full
 + unless I misread the code, the producer (for frontend to backend
messages) seems to write to out_prod + 1 > out_cons, then increment
out_prob (xenfb_do_update) and then the consumer processes entries
from out_cons upto but not include the updated out_prod
(xenfb_on_fb_event) -- the producer for the other way around (backend
to frontend, xenfb_fb_event) seems to be correcter.
 + the lack of barriers -- /* FIXME barriers */ doesn't really cut it ;-)
==> I would suggest looking at the block or net frontend/backend
drivers and copy/pasting some code from there...

- xenbus transactions can fail and there's no code to retry failed transactions

Additionally, I think that the dirty region protocol doesn't really
perform too well in quite simple usage cases like having video play in
one corner of the screen and the mouse being moved in the opposite
corner.  It's probably good enough for this version and supporting
this protocol in the future isn't too bad.

I'm looking forward to seeing a new version of this patch, thanks!

   christian

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