> >> >> + /* 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.
I'm not bothered. Just do whatever's easier.
> >> >> + 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.
Ah, okay. Thanks for clearing that up for me.
> >> > 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.
I'm quite happy to ignore this problem. Certainly 8 bit types which
need more than 8 bit alignment are pretty unexpected, and I'd be
amazed if we any of the standard integer types need more than 8 byte
alignment on any reasonable architecture. If you're planning on
sending weird SSE types across or something like that then you might
have problems, but frankly at that point you deserve everything you
get.
> Even if we dismiss the portability problem as theoretical: isn't this
> padding game too clever by half?
You do need to think about padding to some extent when doing this sort
of thing. One of the things which we'd like for new IO protocols is
to have the same byte layout between 32 and 64 bit guests, so that you
don't sabotage the ongoing work to get 32 bit guests working on 64 bit
hosts. In the past, we've screwed up by relying on the compiler to
insert implicit padding in e.g. the block request structure, and then
discovered later on that it does it differently in 32 and 64 bit mode.
Making the padding explicit also makes it more obviously where you can
insert fields if we decide that we want e.g. per-request flags in a
few months' time.
Anyway, this whole issue is a bikeshed. Do whichever you prefer.
> >> >> +/*
> >> >> + * 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.
Okay.
Steven.
signature.asc
Description: Digital signature
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home