Re: [PATCH 1/2] PV framebuffer

[Steven Smith <sos22-xen@srcf.ucam.org>]

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

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

> >> +	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;
> > goto error will end up calling xenfb_remove, which will notice that
> > info->kthread is non-NULL and try to kthread_stop it.  That strikes me
> > as a bad idea.
> > ...
> Oops.  Thanks for catching.
np.

> >> +static int __devexit xenfb_remove(struct xenbus_device *dev)
> > This gets called from some error paths even when the driver is
> > remaining loaded.  Is __devexit safe?  I'm thinking particularly of
> > the case where the kernel is compiled without module unloading and
> > discards exit text segments.
> Insufficient understanding of __devexit on my part.  Fixing.
Yeah, I had to go and look it up.

> >> +{
> >> +	struct xenfb_info *info = dev->dev.driver_data;
> >> +
> >> +	del_timer(&info->refresh);
> >> +	if (info->kthread)
> >> +		kthread_stop(info->kthread);
> >> +	if (info->irq >= 0)
> >> +		unbind_from_irqhandler(info->irq, info);
> >> +	if (info->fb_info) {
> >> +		unregister_framebuffer(info->fb_info);
> >> +		fb_dealloc_cmap(&info->fb_info->cmap);
> >> +		framebuffer_release(info->fb_info);
> >> +	}
> >> +	free_page((unsigned long)info->page);
> >> +	vfree(info->mfns);
> >> +	kfree(info->pages);
> >> +	vfree(info->fb);
> >> +	kfree(info);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static struct xenbus_driver xenfb = {
> >> +	.name = "vfb",
> >> +	.owner = THIS_MODULE,
> >> +	.ids = xenfb_ids,
> >> +	.probe = xenfb_probe,
> >> +	.remove = xenfb_remove,
> > I think this wants to be __devexit_p.
> Only if xenfb is __devexit, right?
Yes, so if xenfb_remove loses __devexit this is correct.

> >> +	/* TODO .resume = xenfb_resume, */
> >> +	.otherend_changed = xenfb_backend_changed,
> >> +};
> >> +
> >> +static int __init xenfb_init(void)
> >> +{
> >> +	int ret;
> >> +
> >> +	if (!is_running_on_xen())
> >> +		return -ENODEV;
> >> +
> >> +	/* Nothing to do if running in dom0. */
> >> +	if (is_initial_xendomain())
> >> +		return -ENODEV;
> >> +	/* if we're not set up to use graphics mode, then don't initialize */
> >> +	if (xenbus_scanf(XBT_NIL, "console", "use_graphics", "%d", &ret) < 0)
> >> +		return -ENODEV;
> >> +	if (ret == 0)
> >> +		return -ENODEV;
> > Not sure about this.  It seems to me like it might be useful to
> > sometimes have domains which have a PVFB but still use xencons for
> > kernel printks etc.  Perhaps I'm just confused.
> I guess you're right.
Thanks.

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

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

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

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

Steven.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

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