Pat Campbell
2008-Jan-08 15:34 UTC
[Xen-devel][PATCH][LINUX] Dynamic modes support for PV xenfb (1 of 2)
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@novell.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Markus Armbruster
2008-Jan-10 10:21 UTC
Re: [Xen-devel][PATCH][LINUX] Dynamic modes support for PV xenfb (1 of 2)
"Pat Campbell" <plc@novell.com> 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@novell.com> > > > > > > > > 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@lists.xensource.com http://lists.xensource.com/xen-devel
Pat Campbell
2008-Jan-11 15:06 UTC
Re: [Xen-devel][PATCH][LINUX] Dynamic modes support for PV xenfb (1 of 2)
>>> On Thu, Jan 10, 2008 at 3:21 AM, in message<87abnehtfn.fsf@pike.pond.sub.org>, Markus Armbruster <armbru@redhat.com> wrote:> "Pat Campbell" <plc@novell.com> 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@novell.com> >> >> >> >> >> >> >> >> 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.OK> >> @@ - 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?An update event for a rectangle outside the new size will be clipped and then discarded in the backend. Wasted time sending it but it should have no ill effect.> >> 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? >Resolution change is asynchronous. Short of making fb_set_par() a blocking call I see no way to make this synchronized.> 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.Sending the resize event from within xenfb_thread protects against the possibility of the queue being full. resize_dpy will not be cleared until the event is successfully inserted in the queue unlike update events which are silently discarded if the queue is full. Probably will never happen but that is why I put it there.> > 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? >Yes, although in testing I have not seen artifacts of the old resolution on a resized screen. A simple VNC client refresh by the user would correct that :)>> + >> 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 :)pixclock is read by X config utilities to determine what monitor settings will work. We don''t need it but the user land apps can use it.> >> >> 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?VNC client window upon restore will be at the default 800x600 resolution. This will cause the backend to change to the resolution the guest is running with.> >> 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]); > }Ok> >> 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 will change it to say: xenkbd needs to know the default initial resolution. Put it here ...> I don''t quite understand how your pointer scaling works, care to > explain?Even though it works I have dropped the whole scale stuff. I have sent a patch request into SuSE to have X evdev_drv handle a SYN_CONFIG event in their older distros. Fedora8 and OpenSuSE already handle this event. I looked briefly at RHEL5 X code and it could use the same patch with perhaps a small tweak. Will send updated patch when I have incorporated your suggestions Thanks for your feedback Pat _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Markus Armbruster
2008-Jan-12 08:03 UTC
Re: [Xen-devel][PATCH][LINUX] Dynamic modes support for PV xenfb (1 of 2)
"Pat Campbell" <plc@novell.com> writes:>>>> On Thu, Jan 10, 2008 at 3:21 AM, in message > <87abnehtfn.fsf@pike.pond.sub.org>, Markus Armbruster <armbru@redhat.com> > wrote: >> "Pat Campbell" <plc@novell.com> 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.[...]>>> 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? > > An update event for a rectangle outside the new size will be clipped and > then discarded in the backend. Wasted time sending it but it should > have no ill effect.Yes. It''s still ugly. Hmm, the resize makes the backend refresh the whole screen, doesn''t it? That makes the update *always* redundant. We could suppress the event. I guess that''s not quite worth the trouble. What about a comment explaining this?>>> 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? >> > > Resolution change is asynchronous. Short of making fb_set_par() > a blocking call I see no way to make this synchronized. > >> 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. > > Sending the resize event from within xenfb_thread protects against the > possibility of the queue being full.You''re right.> resize_dpy will not be cleared until the > event is successfully inserted in the queue unlike update events which > are silently discarded if the queue is full. Probably will never happen but > that is why I put it there.Update events are not discarded, they''re accumulated in the dirty rectangle.>> 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? >> > > Yes, although in testing I have not seen artifacts of the old resolution > on a resized screen. A simple VNC client refresh by the user would > correct that :)I guess (hope?) guest user land saves us there by triggering suitable update events. [...] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel