|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel][PATCH][LINUX] Dynamic modes support for PV xenfb (1 of 2)
"Pat Campbell" <plc@xxxxxxxxxx> writes:
> Attached patch adds multiple frame buffer resolution support to
> the PV xenfb frame buffer driver.
>
> Code is essentially the same as I sent in the previous RFC except
> the frame buffer size is now 800x600 if the backend does not
> support feature-resize, same memory footprint.
>
> Corresponding backend IOEMU patch is required for functionality but
> this patch is not dependent on it, preserving backwards compatibility.
>
> Please apply to tip of linux-2.6.18-xen
>
> Signed- off- by: Pat Campbell <plc@xxxxxxxxxx>
>
>
>
>
>
>
>
> diff -r 61c96456a3e1 drivers/xen/fbfront/xenfb.c
> --- a/drivers/xen/fbfront/xenfb.c Thu Dec 20 16:58:14 2007 +0000
> +++ b/drivers/xen/fbfront/xenfb.c Sat Jan 05 11:08:03 2008 -0700
> @@ -62,6 +62,9 @@ struct xenfb_info
> struct xenfb_page *page;
> unsigned long *mfns;
> int update_wanted; /* XENFB_TYPE_UPDATE wanted */
> + int feature_resize; /* Backend has resize feature */
> + int resize_dpy;
> + int xres, yres;
>
> struct xenbus_device *xbdev;
> };
> @@ -130,13 +133,17 @@ struct xenfb_info
> * Oh well, we wont be updating the writes to this page anytime soon.
> */
>
> +#define XENFB_PIXCLOCK 9025 /* Xorg "1280x1024" 110.80 to FB
> 1000000/110.80 */
> +#define XENFB_SCAN_LINE_LEN 1280
> +#define XENFB_FRAME_BUFFER_LEN (1024 * 5120) // 5 MB framebuffer
> static int xenfb_fps = 20;
> -static unsigned long xenfb_mem_len = XENFB_WIDTH * XENFB_HEIGHT *
> XENFB_DEPTH / 8;
> +static unsigned long xenfb_mem_len = XENFB_FRAME_BUFFER_LEN;
>
> static int xenfb_remove(struct xenbus_device *);
> static void xenfb_init_shared_page(struct xenfb_info *);
> static int xenfb_connect_backend(struct xenbus_device *, struct xenfb_info
> *);
> static void xenfb_disconnect_backend(struct xenfb_info *);
> +static void xenfb_refresh(struct xenfb_info *info, int x1, int y1, int w,
> int h);
>
> static void xenfb_do_update(struct xenfb_info *info,
> int x, int y, int w, int h)
> @@ -155,6 +162,25 @@ static void xenfb_do_update(struct xenfb
> mb(); /* ensure ring space available */
> XENFB_OUT_RING_REF(info->page, prod) = event;
> wmb(); /* ensure ring contents visible */
> + info->page->out_prod = prod + 1;
> +
> + notify_remote_via_irq(info->irq);
> +}
> +
> +static void xenfb_do_resize(struct xenfb_info *info)
> +{
> + union xenfb_out_event event;
> + __u32 prod;
> +
> + event.type = XENFB_TYPE_RESIZE;
> + event.resize.width = info->xres;
> + event.resize.height = info->yres;
> +
> + prod = info->page->out_prod;
> + /* caller ensures !xenfb_queue_full() */
> + mb(); /* ensure ring space available */
> + XENFB_OUT_RING_REF(info->page, prod) = event;
> + wmb(); /* ensure ring contents visible */
> info->page->out_prod = prod + 1;
>
> notify_remote_via_irq(info->irq);
xenfb_do_update() and xenfb_do_resize() share the short, but hairy
code to put an event into the ring buffer. Please create a separate
helper function for that.
> @@ -209,6 +235,16 @@ static void xenfb_update_screen(struct x
> xenfb_do_update(info, x1, y1, x2 - x1, y2 - y1);
> }
>
> +static void xenfb_resize_screen(struct xenfb_info *info)
> +{
> + if (xenfb_queue_full(info))
> + return;
> +
> + info->resize_dpy = 0;
> + xenfb_do_resize(info);
> + xenfb_refresh(info, 0, 0, info->xres, info->yres);
> +}
> +
> static int xenfb_thread(void *data)
> {
> struct xenfb_info *info = data;
> @@ -217,6 +253,9 @@ static int xenfb_thread(void *data)
> if (info->dirty) {
> info->dirty = 0;
> xenfb_update_screen(info);
> + }
> + if (info->resize_dpy) {
> + xenfb_resize_screen(info);
> }
Hmm. If both dirty and resize_dpy are set, the update event goes out
before the resize event. What if the update is for the resized
screen? If whatever caused the update happened after the screen was
resized, then the update event overtakes the resize event, and its
coordinates will not make sense, will they?
> wait_event_interruptible(info->wq,
> kthread_should_stop() || info->dirty);
> @@ -413,6 +452,45 @@ static int xenfb_mmap(struct fb_info *fb
> return 0;
> }
>
> +static int
> +xenfb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
> +{
> + struct xenfb_info *xenfb_info;
> +
> + xenfb_info = info->par;
Style nitpick:
struct xenfb_info *xenfb_info = info->par;
> +
> + if (!xenfb_info->feature_resize) {
> + if (var->xres == XENFB_WIDTH && var->yres == XENFB_HEIGHT
> + && var->bits_per_pixel ==
> xenfb_info->page->depth) {
> + return 0;
> + }
> + return -EINVAL;
> + }
> + if (var->bits_per_pixel == xenfb_info->page->depth &&
> + XENFB_SCAN_LINE_LEN >= var->xres &&
> + xenfb_mem_len >= (var->xres * var->yres *
> (xenfb_info->page->depth / 8))) {
> + var->xres_virtual = var->xres;
> + var->yres_virtual = var->yres;
> + return 0;
> + }
> + return -EINVAL;
> +}
> +
> +static int xenfb_set_par(struct fb_info *info)
> +{
> + struct xenfb_info *xenfb_info;
> +
> + xenfb_info = info->par;
Style nitpick:
struct xenfb_info *xenfb_info = info->par;
> +
> + if (xenfb_info->xres != info->var.xres ||
> + xenfb_info->yres != info->var.yres) {
> + xenfb_info->resize_dpy = 1;
> + xenfb_info->xres = info->var.xres_virtual = info->var.xres;
> + xenfb_info->yres = info->var.yres_virtual = info->var.yres;
> + }
> + return 0;
> +}
How is the resolution change synchronized between guest user land,
pvfb frontend and pvfb backend?
I guess method fb_set_par() changes the resolution instantly as far as
the guest is concerned. The framebuffer then contains junk, until
guest user land fixes it up. The notification of the backend is
delayed until xenfb_thread() next checks xenfb_info->resize_dpy.
By the way, this delay is arbitrary; we could call
xenfb_resize_screen() here. We route screen updates through
xenfb_thread() because we can't do that work from xenfb_vm_nopage().
Welcome side effect: also collapses multiple quick updates into one.
None of this is of concern for screen resize.
Anyway, the backend continues to interpret the framebuffer according
to the old resolution until it receives the notification. The
notification also makes it redraw the whole screen. There's no
guarantee that guest user land finished fixing up the framebuffer
contents by then. If it hasn't, the redraw draws junk. This junk can
then remain on the screen indefinitely, because no further screen
update requests need to arrive from the frontend. Correct?
> +
> static struct fb_ops xenfb_fb_ops = {
> .owner = THIS_MODULE,
> .fb_setcolreg = xenfb_setcolreg,
> @@ -420,6 +498,8 @@ static struct fb_ops xenfb_fb_ops = {
> .fb_copyarea = xenfb_copyarea,
> .fb_imageblit = xenfb_imageblit,
> .fb_mmap = xenfb_mmap,
> + .fb_check_var = xenfb_check_var,
> + .fb_set_par = xenfb_set_par,
> };
>
> static irqreturn_t xenfb_event_handler(int rq, void *dev_id,
> @@ -451,16 +531,29 @@ static int __devinit xenfb_probe(struct
> struct xenfb_info *info;
> struct fb_info *fb_info;
> int ret;
> + int val;
>
> info = kzalloc(sizeof(*info), GFP_KERNEL);
> if (info == NULL) {
> xenbus_dev_fatal(dev, -ENOMEM, "allocating info structure");
> return -ENOMEM;
> }
> +
> + // Determine video memory size based on feature support
> + xenfb_mem_len = XENFB_WIDTH * XENFB_HEIGHT * XENFB_DEPTH / 8;
> + if (xenbus_scanf(XBT_NIL, dev->otherend,
> + "xenfb-feature-resize", "%d", &val) < 0)
> + val = 0;
> + info->feature_resize = val;
> + if (info->feature_resize)
> + xenfb_mem_len = XENFB_FRAME_BUFFER_LEN;
> +
> dev->dev.driver_data = info;
> info->xbdev = dev;
> info->irq = -1;
> info->x1 = info->y1 = INT_MAX;
> + info->xres = XENFB_WIDTH;
> + info->yres = XENFB_HEIGHT;
> spin_lock_init(&info->dirty_lock);
> mutex_init(&info->mm_lock);
> init_waitqueue_head(&info->wq);
> @@ -504,9 +597,10 @@ static int __devinit xenfb_probe(struct
> fb_info->screen_base = info->fb;
>
> fb_info->fbops = &xenfb_fb_ops;
> - fb_info->var.xres_virtual = fb_info->var.xres = info->page->width;
> - fb_info->var.yres_virtual = fb_info->var.yres = info->page->height;
> + fb_info->var.xres_virtual = fb_info->var.xres = info->xres;
> + fb_info->var.yres_virtual = fb_info->var.yres = info->yres;
> fb_info->var.bits_per_pixel = info->page->depth;
> + fb_info->var.pixclock = XENFB_PIXCLOCK;
I have no idea why we need this :)
>
> fb_info->var.red = (struct fb_bitfield){16, 8, 0};
> fb_info->var.green = (struct fb_bitfield){8, 8, 0};
> @@ -572,6 +666,8 @@ static int xenfb_resume(struct xenbus_de
>
> xenfb_disconnect_backend(info);
> xenfb_init_shared_page(info);
> + if (info->feature_resize)
> + info->resize_dpy = 1;
Why is this needed? Not obvious to me; need a comment?
> return xenfb_connect_backend(dev, info);
> }
>
> @@ -600,19 +696,30 @@ static void xenfb_init_shared_page(struc
> static void xenfb_init_shared_page(struct xenfb_info *info)
> {
> int i;
> -
> + int j;
> + int entries_per_pd;
> +
> + entries_per_pd = PAGE_SIZE/sizeof(unsigned long);
> +
Style nitpick:
int i, j;
int entries_per_pd = PAGE_SIZE / sizeof(unsigned long);
> for (i = 0; i < info->nr_pages; i++)
> info->pages[i] = vmalloc_to_page(info->fb + i * PAGE_SIZE);
>
> - for (i = 0; i < info->nr_pages; i++)
> - info->mfns[i] = vmalloc_to_mfn(info->fb + i * PAGE_SIZE);
> -
> info->page->pd[0] = vmalloc_to_mfn(info->mfns);
> info->page->pd[1] = 0;
> + for ( j = i = 0; i < info->nr_pages; i++) {
> + info->mfns[i] = vmalloc_to_mfn(info->fb + i * PAGE_SIZE);
> + if ( i && i % entries_per_pd == 0 ) {
> + j++;
> + info->page->pd[j] = vmalloc_to_mfn(&info->mfns[i]);
> + }
> + }
I don't like this loop. What about something like this:
{
int i;
int epd = PAGE_SIZE / sizeof(info->mfns[0]);
for (i = 0; i < info->nr_pages; i++)
info->pages[i] = vmalloc_to_page(info->fb + i * PAGE_SIZE);
for (i = 0; i < info->nr_pages; i++)
info->mfns[i] = vmalloc_to_mfn(info->fb + i * PAGE_SIZE);
for (i = 0; i * epd < info->nr_pages; i++)
info->page->pd[i] = vmalloc_to_mfn(&info->mfns[i * epd]);
}
> info->page->width = XENFB_WIDTH;
> info->page->height = XENFB_HEIGHT;
> info->page->depth = XENFB_DEPTH;
> - info->page->line_length = (info->page->depth / 8) * info->page->width;
> + if (info->feature_resize)
> + info->page->line_length = (info->page->depth / 8) *
> XENFB_SCAN_LINE_LEN;
> + else
> + info->page->line_length = (info->page->depth / 8) * XENFB_WIDTH;
> info->page->mem_length = xenfb_mem_len;
> info->page->in_cons = info->page->in_prod = 0;
> info->page->out_cons = info->page->out_prod = 0;
> diff -r 61c96456a3e1 include/xen/interface/io/fbif.h
> --- a/include/xen/interface/io/fbif.h Thu Dec 20 16:58:14 2007 +0000
> +++ b/include/xen/interface/io/fbif.h Sat Jan 05 11:08:35 2008 -0700
> @@ -50,12 +50,26 @@ struct xenfb_update
> int32_t height; /* rect height */
> };
>
> +/*
> + * Framebuffer resize notification event
> + * Capable backend sets feature-resize in xenstore.
> + */
> +#define XENFB_TYPE_RESIZE 3
> +
> +struct xenfb_resize
> +{
> + uint8_t type; /* XENFB_TYPE_RESIZE */
> + int32_t width; /* xres */
> + int32_t height; /* yres */
> +};
> +
> #define XENFB_OUT_EVENT_SIZE 40
>
> union xenfb_out_event
> {
> uint8_t type;
> struct xenfb_update update;
> + struct xenfb_resize resize;
> char pad[XENFB_OUT_EVENT_SIZE];
> };
>
> @@ -111,8 +125,12 @@ struct xenfb_page
> * PAGE_SIZE / sizeof(*pd) bytes. With PAGE_SIZE == 4096 and
> * sizeof(unsigned long) == 4, that's 4 Megs. Two directory
> * pages should be enough for a while.
> + *
> + * Increased to 3 to support 1280x1024 resolution on a 64bit system
> + * (1280*1024*4)/PAGE_SIZE = 1280 pages required
> + * PAGE_SIZE/64bit long = 512 pages per page directory
> */
> - unsigned long pd[2];
> + unsigned long pd[3];
> };
>
> /*
Still in fbif.h:
/*
* Wart: xenkbd needs to know resolution. Put it here until a better
* solution is found, but don't leak it to the backend.
*/
#ifdef __KERNEL__
#define XENFB_WIDTH 800
#define XENFB_HEIGHT 600
#define XENFB_DEPTH 32
#endif
The comment is now misleading: if xenkbd *needs* to know *the*
resolution, why is it happy with an *incorrect* resolution?
I don't quite understand how your pointer scaling works, care to
explain?
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |