Re: [Xen-devel] [PATCH 1/2] PV framebuffer

[Markus Armbruster <armbru@xxxxxxxxxx>]

Steven Smith <sos22-xen@xxxxxxxxxxxxx> writes:

>> >> +    ret = fb_alloc_cmap(&fb_info->cmap, 256, 0);
>> >> +    if (ret < 0) {
>> >> +            framebuffer_release(fb_info);
>> >> +            xenbus_dev_fatal(dev, ret, "fb_alloc_cmap");
>> >> +            goto error;
>> >> +    }
>> >> +
>> >> +    /* FIXME should this be delayed until backend XenbusStateConnected? 
>> >> */
>> > I would, but I'm not sure it matters too much.  You have a potentially
>> > slightly confusing situation where someone starts using the
>> > framebuffer before the backend connects, so the backend has to be able
>> > to handle there being events on the ring when it starts, but I think
>> > it probably works as it is.
>> 
>> Yes, it works.
>> 
>> I might move it eventually just for cleanliness.
> Actually, thinking about it, it probably does want to stay where it
> is, at least if you want to support backend disconnect/reconnect
> cleanly.  If you kill the backend and restart it, _probe shouldn't get
> run a second time, whereas the state machine transitions probably will
> be.  You probably want to present the same device to Linux, so
> re-registering might not be a good idea.

Yeah, that makes sense.

What about the kernel thread?  I figure letting it run only while a
backend is connected would make some sense.  Anyway, no flawless
diamond needed here for the initial merge.

>> >> + info->kthread = kthread_run(xenfb_thread, info, "xenfb thread");
>> >> + if (IS_ERR(info->kthread)) {
>> >> +         ret = PTR_ERR(info->kthread);
>> >> +         xenbus_dev_fatal(dev, ret, "register_framebuffer");
>> >> +         goto error;
[...]
>> >> +int __devinit xenkbd_probe(struct xenbus_device *dev,
>> >> +                    const struct xenbus_device_id *id)
>> >> +{
>> > ...
>> >> + input_dev->evbit[0] = BIT(EV_KEY) | BIT(EV_REL) | BIT(EV_ABS);
>> >> + input_dev->keybit[LONG(BTN_MOUSE)]
>> >> +         = BIT(BTN_LEFT) | BIT(BTN_MIDDLE) | BIT(BTN_RIGHT);
>> >> + /* FIXME more buttons? */
>> >> + input_dev->relbit[0] = BIT(REL_X) | BIT(REL_Y);
>> > Do you need to set some bits in absbit, as well, since you can
>> > generate absolute coordinate messages?
>> We missed that.  Curiously, everything seems to work all the same (as
>> tested with evdev).  Apparently, no consumer actually tests the bits
>> correctly.
> Figures.
>
>> Fixing anyway.
> Thanks.

Haha, I found it: input_set_abs_params() takes care of absbit[] [...]
automatically.

>> >> diff -rupN -x '*.orig' 
>> >> linux-2.6.16.29-xen/include/xen/interface/io/xenfb.h 
>> >> linux-2.6.16.29-xen-vfb/include/xen/interface/io/xenfb.h
>> >> --- linux-2.6.16.29-xen/include/xen/interface/io/xenfb.h  1970-01-01 
>> >> 01:00:00.000000000 +0100
>> >> +++ linux-2.6.16.29-xen-vfb/include/xen/interface/io/xenfb.h      
>> >> 2006-11-10 09:43:37.000000000 +0100
>> > I think this really wants to be in xen/include/public/io, but I'd
>> > guess this is just an artifact of the diff.
>> Where in the *Linux* tree you want it?  include/xen/interface or
>> include/xen/public or somewhere else?
> The IO header files all go in xen/include/public/io, with symlinks
> from linux/include/xen/interface/io.  Your tree may be different due
> to de-sparseification.  If that's all it is then I'll just ignore this
> from now on.

Yes, that's all it is.

>> >> +struct xenfb_update
>> >> +{
>> >> + __u8 type;              /* XENFB_TYPE_UPDATE */
>> >> + __s32 x;                /* source x */
>> >> + __s32 y;                /* source y */
>> >> + __s32 width;            /* rect width */
>> >> + __s32 height;           /* rect height */
>> >> +};
>> >> +
>> >> +#define XENFB_OUT_EVENT_SIZE 40
>> >> +
>> >> +union xenfb_out_event
>> >> +{
>> >> + __u8 type;
>> >> + struct xenfb_update update;
>> >> + char pad[XENFB_OUT_EVENT_SIZE];
>> >> +};
>> > I still think you'd be better off doing tagged unions the usual way,
>> > but your funeral.
>> While I personally prefer Anthony's way to do it, I'd convert to yours
>> if I could ensure the type's size remains exactly XENFB_OUT_EVENT_SIZE
>> portably and without undue ugliness.  Alternatively, if I could ensure
>> the ring memory layout remains the same even when the type size
>> changes.
> How about something like this:
>
> struct xenfb_out_event {
>       __u8 type;
>       __u8 pad1[7];
>       union {
>               struct xenfb_update update;
>               __u8 pad[XENFB_OUT_SIZE-8];
>       } u;
> };
>
> Does this satisfy your requirements?  Admittedly, I've had to move
> some padding around to get good alignment, which I'd forgotten about
> before and makes it look a bit uglier than I was hoping, but I'd still
> say this is a little nicer.

Umm, doesn't this make non-portable assumptions?  What if
__alignof(update) > 8?  What if __alignof__(pad1) != 1?  I admit I'd
be surprised by the latter.

Even if we dismiss the portability problem as theoretical: isn't this
padding game too clever by half?

>> >> +/*
>> >> + * Wart: xenkbd needs to know resolution.  Put it here until a better
>> >> + * solution is found, but don't leak it to the backend.
>> >> + */
>> > Why does xenkbd need to know the resolution?  Do you mean xenfb?
>> >
>> > As far as I can see, these only get used in xenfb.c, so the #defines
>> > could go there rather than in the IO description header.
>> That's where they used to be.  But xenkbd needs to tell the input
>> layer about the resolution to make absolute pointer work.
> That's really quite unfortunate, and is going to cause problems when
> you start supporting changes in resolution.
>
> Does the resolution reported by xenkbd actually need to match the
> resolution of the framebuffer?  Could you report a resolution of
> 65536x65536 and then have the backend scale it?  Presumably, a real
> tablet won't scale its output to match the display resolution.

I'm not worried about this right now.  When we start supporting
resolution change, we can figure out whether lying about the
resolution works.  If it doesn't, I'm sure we'll find a reasonable way
to communicate the real resolution to xenkbd.

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