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

[Markus Armbruster <armbru@redhat.com>]

"Christian Limpach" <christian.limpach@gmail.com> writes:

> On 6/26/06, Markus Armbruster <armbru@redhat.com> 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

D'oh!

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

Understood.

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

I'll look into this.

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

Will do.

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

Will do.

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

There's been some discussion on this.  I'm not sure what bottom line
emerged, if any.  Do we have to improve this before it can be merged?
Or before it can be declared stable?  Or just eventually?  Unless it's
the latter: do we already know in which direction to go?