Pat Campbell
2008-Mar-13 19:59 UTC
[Xen-devel][PATCH][LINUX] Dynamic modes support for xenfb (2 of 2)
Attached patch adds dynamic frame buffer resolution support to the PV xenfb frame buffer driver. 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-Mar-14 16:41 UTC
Re: [Xen-devel][PATCH][LINUX] Dynamic modes support for xenfb (2 of 2)
Pat Campbell <plc@novell.com> writes:> Attached patch adds dynamic frame buffer resolution support to > the PV xenfb frame buffer driver. > > 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>Adding another lock (queue_lock) to our already ticklish locking gives me a queasy feeling... The purpose of the lock is not obvious to me. Please explain that, in the code. Likewise, it''s not entirely obvious that the locking works. Please update the "How the locks work together" comment. But before you do that, I suggest you tell us exactly what problem you''re attempting to solve with queue_lock. Maybe we can come up with a less scary solution. Maybe not. But it''s worth a try. If it''s just to ensure the changes made by xenfb_set_par() are seen in xenfb_do_resize() correctly, a memory barrier should to the trick more easily.> diff -r 3983b041fc51 drivers/xen/fbfront/xenfb.c > --- a/drivers/xen/fbfront/xenfb.c Mon Mar 10 22:53:07 2008 +0000 > +++ b/drivers/xen/fbfront/xenfb.c Thu Mar 13 12:57:30 2008 -0600 > @@ -31,6 +31,7 @@ > #include <xen/interface/io/protocols.h> > #include <xen/xenbus.h> > #include <linux/kthread.h> > +#include <linux/delay.h> > > struct xenfb_mapping > { > @@ -62,6 +63,8 @@ struct xenfb_info > struct xenfb_page *page; > unsigned long *mfns; > int update_wanted; /* XENFB_TYPE_UPDATE wanted */ > + int feature_resize; /* Backend has resize feature */ > + struct mutex queue_lock; > > struct xenbus_device *xbdev; > }; > @@ -129,20 +132,42 @@ struct xenfb_info > * > * Oh well, we wont be updating the writes to this page anytime soon. > */ > +#define MB_ (1024*1024) > +#define XENFB_DEFAULT_FB_LEN (XENFB_WIDTH * XENFB_HEIGHT * XENFB_DEPTH / 8) > + > +enum {KPARAM_MEM, KPARAM_WIDTH, KPARAM_HEIGHT, KPARAM_CNT}; > +static int video[KPARAM_CNT] = {2, XENFB_WIDTH, XENFB_HEIGHT}; > +module_param_array(video, int, NULL, 0); > +MODULE_PARM_DESC(video, > + "Size of video memory in MB and width,height in pixels, default = (2,800,600)"); > > static int xenfb_fps = 20; > -static unsigned long xenfb_mem_len = XENFB_WIDTH * XENFB_HEIGHT * XENFB_DEPTH / 8; > > static int xenfb_remove(struct xenbus_device *); > -static void xenfb_init_shared_page(struct xenfb_info *); > +static void xenfb_init_shared_page(struct xenfb_info *, struct fb_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 *, int, int, int, int);Is this forward declaration needed?> + > +static void xenfb_send_event(struct xenfb_info *info, > + union xenfb_out_event *event) > +{ > + __u32 prod; > + > + 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); > +} > > static void xenfb_do_update(struct xenfb_info *info, > int x, int y, int w, int h) > { > union xenfb_out_event event; > - __u32 prod; > > event.type = XENFB_TYPE_UPDATE; > event.update.x = x; > @@ -150,14 +175,23 @@ static void xenfb_do_update(struct xenfb > event.update.width = w; > event.update.height = h; > > - 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_send_event(info, &event); > +} > + > +static void xenfb_do_resize(struct xenfb_info *info, > + struct fb_info *fb_info) > +{ > + union xenfb_out_event event; > + > + event.type = XENFB_TYPE_RESIZE; > + event.resize.width = fb_info->var.xres; > + event.resize.height = fb_info->var.yres; > + event.resize.stride = fb_info->fix.line_length; > + event.resize.depth = fb_info->var.bits_per_pixel; > + > + /* caller ensures !xenfb_queue_full() */ > + xenfb_send_event(info, &event); > } > > static int xenfb_queue_full(struct xenfb_info *info) > @@ -177,8 +211,12 @@ static void xenfb_update_screen(struct x > > if (!info->update_wanted) > return; > - if (xenfb_queue_full(info)) > + > + mutex_lock(&info->queue_lock);This can block the thread running xenfb_thread() indefinitely. I guess that''s okay. We don''t do S4 (save-to-disk) in domU anyway.> + if (xenfb_queue_full(info)) { > + mutex_unlock(&info->queue_lock); > return; > + } > > mutex_lock(&info->mm_lock); > > @@ -207,6 +245,7 @@ static void xenfb_update_screen(struct x > WARN_ON(1); > } > xenfb_do_update(info, x1, y1, x2 - x1, y2 - y1); > + mutex_unlock(&info->queue_lock); > } > > static int xenfb_thread(void *data) > @@ -413,6 +452,60 @@ 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; > + int required_mem_len; > + > + xenfb_info = info->par; > + > + if (!xenfb_info->feature_resize) { > + if (var->xres == video[KPARAM_WIDTH] && > + var->yres == video[KPARAM_HEIGHT] && > + var->bits_per_pixel == xenfb_info->page->depth) { > + return 0; > + } > + return -EINVAL; > + } > + > + /* Can''t resize past initial width and height */ > + if (var->xres > video[KPARAM_WIDTH] || var->yres > video[KPARAM_HEIGHT]) > + return -EINVAL; > + > + required_mem_len = var->xres * var->yres * (xenfb_info->page->depth / 8); > + if (var->bits_per_pixel == xenfb_info->page->depth && > + var->xres <= info->fix.line_length / (XENFB_DEPTH / 8) && > + required_mem_len <= info->fix.smem_len) { > + 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; > + unsigned long timeout = jiffies + HZ; > + int status; > + > + xenfb_info = info->par; > + > + mutex_lock(&xenfb_info->queue_lock); > + while ((status = xenfb_queue_full(xenfb_info)) && > + time_before(jiffies, timeout)) { > + msleep(10); > + } > + if (status == 0) { > + info->var.xres_virtual = info->var.xres; > + info->var.yres_virtual = info->var.yres; > + xenfb_do_resize(xenfb_info, info); > + }else fail silently by doing nothing> + mutex_unlock(&xenfb_info->queue_lock); > + return 0; > +}Does the framebuffer code handle the fb_set_par() callback doing nothing? Even if it does: is silently failing acceptable?> + > static struct fb_ops xenfb_fb_ops = { > .owner = THIS_MODULE, > .fb_setcolreg = xenfb_setcolreg, > @@ -420,6 +513,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, > @@ -450,6 +545,8 @@ static int __devinit xenfb_probe(struct > { > struct xenfb_info *info; > struct fb_info *fb_info; > + int fb_size; > + int val = 0;Why do you initialize val?> int ret; > > info = kzalloc(sizeof(*info), GFP_KERNEL); > @@ -457,24 +554,40 @@ static int __devinit xenfb_probe(struct > xenbus_dev_fatal(dev, -ENOMEM, "allocating info structure"); > return -ENOMEM; > } > + > + /* Limit kernel param videoram amount to what is in xenstore */ > + if (xenbus_scanf(XBT_NIL, dev->otherend, "videoram", "%d", &val) == 1) { > + if (val < video[KPARAM_MEM]) > + video[KPARAM_MEM] = val; > + } > + > + /* If requested res does not fit in available memory, use default */ > + fb_size = video[KPARAM_MEM] * MB_; > + if (video[KPARAM_WIDTH] * video[KPARAM_HEIGHT] * XENFB_DEPTH/8 > fb_size) { > + video[KPARAM_WIDTH] = XENFB_WIDTH; > + video[KPARAM_HEIGHT] = XENFB_HEIGHT; > + fb_size = XENFB_DEFAULT_FB_LEN; > + } > + > dev->dev.driver_data = info; > info->xbdev = dev; > info->irq = -1; > info->x1 = info->y1 = INT_MAX; > spin_lock_init(&info->dirty_lock); > mutex_init(&info->mm_lock); > + mutex_init(&info->queue_lock); > init_waitqueue_head(&info->wq); > init_timer(&info->refresh); > info->refresh.function = xenfb_timer; > info->refresh.data = (unsigned long)info; > INIT_LIST_HEAD(&info->mappings); > > - info->fb = vmalloc(xenfb_mem_len); > + info->fb = vmalloc(fb_size); > if (info->fb == NULL) > goto error_nomem; > - memset(info->fb, 0, xenfb_mem_len); > - > - info->nr_pages = (xenfb_mem_len + PAGE_SIZE - 1) >> PAGE_SHIFT; > + memset(info->fb, 0, fb_size); > + > + info->nr_pages = (fb_size + PAGE_SIZE - 1) >> PAGE_SHIFT; > > info->pages = kmalloc(sizeof(struct page *) * info->nr_pages, > GFP_KERNEL); > @@ -489,8 +602,6 @@ static int __devinit xenfb_probe(struct > info->page = (void *)__get_free_page(GFP_KERNEL | __GFP_ZERO); > if (!info->page) > goto error_nomem; > - > - xenfb_init_shared_page(info); > > fb_info = framebuffer_alloc(sizeof(u32) * 256, NULL); > /* see fishy hackery below */ > @@ -504,9 +615,9 @@ 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.bits_per_pixel = info->page->depth; > + fb_info->var.xres_virtual = fb_info->var.xres = video[KPARAM_WIDTH]; > + fb_info->var.yres_virtual = fb_info->var.yres = video[KPARAM_HEIGHT]; > + fb_info->var.bits_per_pixel = XENFB_DEPTH; > > fb_info->var.red = (struct fb_bitfield){16, 8, 0}; > fb_info->var.green = (struct fb_bitfield){8, 8, 0}; > @@ -518,9 +629,9 @@ static int __devinit xenfb_probe(struct > fb_info->var.vmode = FB_VMODE_NONINTERLACED; > > fb_info->fix.visual = FB_VISUAL_TRUECOLOR; > - fb_info->fix.line_length = info->page->line_length; > + fb_info->fix.line_length = fb_info->var.xres * (XENFB_DEPTH / 8); > fb_info->fix.smem_start = 0; > - fb_info->fix.smem_len = xenfb_mem_len; > + fb_info->fix.smem_len = fb_size; > strcpy(fb_info->fix.id, "xen"); > fb_info->fix.type = FB_TYPE_PACKED_PIXELS; > fb_info->fix.accel = FB_ACCEL_NONE; > @@ -533,6 +644,9 @@ static int __devinit xenfb_probe(struct > xenbus_dev_fatal(dev, ret, "fb_alloc_cmap"); > goto error; > } > + > + /* init shared page, uses fb_info attributes */ > + xenfb_init_shared_page(info, fb_info);The comment is somewhat redundant.> > ret = register_framebuffer(fb_info); > if (ret) { > @@ -571,7 +685,7 @@ static int xenfb_resume(struct xenbus_de > struct xenfb_info *info = dev->dev.driver_data; > > xenfb_disconnect_backend(info); > - xenfb_init_shared_page(info); > + xenfb_init_shared_page(info, info->fb_info); > return xenfb_connect_backend(dev, info); > } > > @@ -597,9 +711,11 @@ static int xenfb_remove(struct xenbus_de > return 0; > } > > -static void xenfb_init_shared_page(struct xenfb_info *info) > +static void xenfb_init_shared_page(struct xenfb_info *info, > + struct fb_info * fb_info) > { > 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); > @@ -607,13 +723,14 @@ static void xenfb_init_shared_page(struc > 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; > - 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; > - info->page->mem_length = xenfb_mem_len; > + for (i = 0; i * epd < info->nr_pages; i++) > + info->page->pd[i] = vmalloc_to_mfn(&info->mfns[i * epd]); > + > + info->page->width = fb_info->var.xres; > + info->page->height = fb_info->var.yres; > + info->page->depth = fb_info->var.bits_per_pixel; > + info->page->line_length = fb_info->fix.line_length; > + info->page->mem_length = fb_info->fix.smem_len; > info->page->in_cons = info->page->in_prod = 0; > info->page->out_cons = info->page->out_prod = 0; > } > @@ -712,6 +829,11 @@ static void xenfb_backend_changed(struct > val = 0; > if (val) > info->update_wanted = 1; > + > + if (xenbus_scanf(XBT_NIL, dev->otherend, > + "feature-resize", "%d", &val) < 0) > + val = 0; > + info->feature_resize = val; > break; > > case XenbusStateClosing: > diff -r 3983b041fc51 drivers/xen/fbfront/xenkbd.c > --- a/drivers/xen/fbfront/xenkbd.c Mon Mar 10 22:53:07 2008 +0000 > +++ b/drivers/xen/fbfront/xenkbd.c Thu Mar 13 08:26:22 2008 -0600 > @@ -297,6 +297,16 @@ static void xenkbd_backend_changed(struc > */ > if (dev->state != XenbusStateConnected) > goto InitWait; /* no InitWait seen yet, fudge it */ > + > + /* Set input abs params to match backend screen res */ > + if (xenbus_scanf(XBT_NIL, info->xbdev->otherend, > + "width", "%d", &val) > 0 ) > + input_set_abs_params(info->ptr, ABS_X, 0, val, 0, 0); > + > + if (xenbus_scanf(XBT_NIL, info->xbdev->otherend, > + "height", "%d", &val) > 0 ) > + input_set_abs_params(info->ptr, ABS_Y, 0, val, 0, 0); > + > break;Open question: why do we have to set the parameters to the actual initial resolution here, but don''t have to set them when the resolution changes dynamically?> > case XenbusStateClosing: > diff -r 3983b041fc51 include/xen/interface/io/fbif.h > --- a/include/xen/interface/io/fbif.h Mon Mar 10 22:53:07 2008 +0000 > +++ b/include/xen/interface/io/fbif.h Tue Mar 11 17:39:38 2008 -0600 > @@ -50,12 +50,28 @@ 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; /* width in pixels */ > + int32_t height; /* height in pixels */ > + int32_t stride; /* stride in bytes */ > + int32_t depth; /* depth in bits */ > +}; > + > #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]; > }; > > @@ -109,15 +125,17 @@ struct xenfb_page > * Each directory page holds PAGE_SIZE / sizeof(*pd) > * framebuffer pages, and can thus map up to PAGE_SIZE * > * 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. > + * sizeof(unsigned long) == 4/8, that''s 4 Megs 32 bit and 2 Megs > + * 64 bit. 256 directories give enough room for a 512 Meg > + * framebuffer with a max resolution of 12,800x10,240. Should > + * be enough for a while with room leftover for expansion. > */ > - unsigned long pd[2]; > + unsigned long pd[256]; > }; > > /* > - * Wart: xenkbd needs to know resolution. Put it here until a better > - * solution is found, but don''t leak it to the backend. > + * Wart: xenkbd needs to know default resolution. Put it here until a > + * better solution is found, but don''t leak it to the backend. > */ > #ifdef __KERNEL__ > #define XENFB_WIDTH 800 > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Pat Campbell
2008-Mar-16 19:09 UTC
Re: [Xen-devel][PATCH][LINUX] Dynamic modes support for xenfb (2 of 2)
Markus Armbruster wrote:> Pat Campbell <plc@novell.com> writes: > > >> Attached patch adds dynamic frame buffer resolution support to >> the PV xenfb frame buffer driver. >> >> 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> >> > > Adding another lock (queue_lock) to our already ticklish locking gives > me a queasy feeling... > > The purpose of the lock is not obvious to me. Please explain that, in > the code. Likewise, it''s not entirely obvious that the locking works. > Please update the "How the locks work together" comment. > > But before you do that, I suggest you tell us exactly what problem > you''re attempting to solve with queue_lock. Maybe we can come up with > a less scary solution. Maybe not. But it''s worth a try. If it''s > just to ensure the changes made by xenfb_set_par() are seen in > xenfb_do_resize() correctly, a memory barrier should to the trick more > easily. > >xenfb_set_par() needs to complete it''s work before returning to the caller so I need to send the resize event from within the xenfb_set_par() function. This would mean we now have two event queue writers, xenfb_update_screen() and xenfb_set_par(). Very simple 2 writer one reader problem easily solved by the addition of the queue_lock. Previously I handled the resize in xenfb_thread(). This could lead to us missing a resize and or using the incorrect values on a multi processor system. IE: Wrong resolution P1:1 set_par sets resize_dpy w, h already in fb struct P2:1 thread wakeup sees resize_dpy set P2:2 do_resize() P2:3 read fb width P1:3 check_var/set_par changes fb height P2:4 read fb height (2nd value here) IE: Missed resolution change P1:1 set_par sets resize_dpy w, h already in fb struct P2:1 thread wakeup sees resize_dpy set P2:2 do_resize() P2:3 read fb width P2:4 read fb height P1:3 set_par sets resize_dpy P2:5 clears resize_dpy (Missed the second resize) Adding in a queue lock should be safe because: 1. xenfb_update_screen() can hold a mutex without interfering with zap_page_range 2. xenfb_set_par() will timeout releasing the lock so update can run I have verified that the lock works as expected and times out if necessary.>> diff -r 3983b041fc51 drivers/xen/fbfront/xenfb.c >> --- a/drivers/xen/fbfront/xenfb.c Mon Mar 10 22:53:07 2008 +0000 >> +++ b/drivers/xen/fbfront/xenfb.c Thu Mar 13 12:57:30 2008 -0600 >> @@ -31,6 +31,7 @@ >> #include <xen/interface/io/protocols.h> >> #include <xen/xenbus.h> >> #include <linux/kthread.h> >> +#include <linux/delay.h> >> >> struct xenfb_mapping >> { >> @@ -62,6 +63,8 @@ struct xenfb_info >> struct xenfb_page *page; >> unsigned long *mfns; >> int update_wanted; /* XENFB_TYPE_UPDATE wanted */ >> + int feature_resize; /* Backend has resize feature */ >> + struct mutex queue_lock; >> >> struct xenbus_device *xbdev; >> }; >> @@ -129,20 +132,42 @@ struct xenfb_info >> * >> * Oh well, we wont be updating the writes to this page anytime soon. >> */ >> +#define MB_ (1024*1024) >> +#define XENFB_DEFAULT_FB_LEN (XENFB_WIDTH * XENFB_HEIGHT * XENFB_DEPTH / 8) >> + >> +enum {KPARAM_MEM, KPARAM_WIDTH, KPARAM_HEIGHT, KPARAM_CNT}; >> +static int video[KPARAM_CNT] = {2, XENFB_WIDTH, XENFB_HEIGHT}; >> +module_param_array(video, int, NULL, 0); >> +MODULE_PARM_DESC(video, >> + "Size of video memory in MB and width,height in pixels, default = (2,800,600)"); >> >> static int xenfb_fps = 20; >> -static unsigned long xenfb_mem_len = XENFB_WIDTH * XENFB_HEIGHT * XENFB_DEPTH / 8; >> >> static int xenfb_remove(struct xenbus_device *); >> -static void xenfb_init_shared_page(struct xenfb_info *); >> +static void xenfb_init_shared_page(struct xenfb_info *, struct fb_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 *, int, int, int, int); >> > > Is this forward declaration needed? >No, not anymore.> >> + >> +static void xenfb_send_event(struct xenfb_info *info, >> + union xenfb_out_event *event) >> +{ >> + __u32 prod; >> + >> + 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); >> +} >> >> static void xenfb_do_update(struct xenfb_info *info, >> int x, int y, int w, int h) >> { >> union xenfb_out_event event; >> - __u32 prod; >> >> event.type = XENFB_TYPE_UPDATE; >> event.update.x = x; >> @@ -150,14 +175,23 @@ static void xenfb_do_update(struct xenfb >> event.update.width = w; >> event.update.height = h; >> >> - 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_send_event(info, &event); >> +} >> + >> +static void xenfb_do_resize(struct xenfb_info *info, >> + struct fb_info *fb_info) >> +{ >> + union xenfb_out_event event; >> + >> + event.type = XENFB_TYPE_RESIZE; >> + event.resize.width = fb_info->var.xres; >> + event.resize.height = fb_info->var.yres; >> + event.resize.stride = fb_info->fix.line_length; >> + event.resize.depth = fb_info->var.bits_per_pixel; >> + >> + /* caller ensures !xenfb_queue_full() */ >> + xenfb_send_event(info, &event); >> } >> >> static int xenfb_queue_full(struct xenfb_info *info) >> @@ -177,8 +211,12 @@ static void xenfb_update_screen(struct x >> >> if (!info->update_wanted) >> return; >> - if (xenfb_queue_full(info)) >> + >> + mutex_lock(&info->queue_lock); >> > > This can block the thread running xenfb_thread() indefinitely. I > guess that''s okay. We don''t do S4 (save-to-disk) in domU anyway. >No, not indefinitely. xenfb_set_par(), the only other holder, times out in one second if the queue does not drain enough to allow the addition of a resize event into the queue. I think a good case could be made for something drastically wrong in the backend if we ever hit the timeout.> >> + if (xenfb_queue_full(info)) { >> + mutex_unlock(&info->queue_lock); >> return; >> + } >> >> mutex_lock(&info->mm_lock); >> >> @@ -207,6 +245,7 @@ static void xenfb_update_screen(struct x >> WARN_ON(1); >> } >> xenfb_do_update(info, x1, y1, x2 - x1, y2 - y1); >> + mutex_unlock(&info->queue_lock); >> } >> >> static int xenfb_thread(void *data) >> @@ -413,6 +452,60 @@ 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; >> + int required_mem_len; >> + >> + xenfb_info = info->par; >> + >> + if (!xenfb_info->feature_resize) { >> + if (var->xres == video[KPARAM_WIDTH] && >> + var->yres == video[KPARAM_HEIGHT] && >> + var->bits_per_pixel == xenfb_info->page->depth) { >> + return 0; >> + } >> + return -EINVAL; >> + } >> + >> + /* Can''t resize past initial width and height */ >> + if (var->xres > video[KPARAM_WIDTH] || var->yres > video[KPARAM_HEIGHT]) >> + return -EINVAL; >> + >> + required_mem_len = var->xres * var->yres * (xenfb_info->page->depth / 8); >> + if (var->bits_per_pixel == xenfb_info->page->depth && >> + var->xres <= info->fix.line_length / (XENFB_DEPTH / 8) && >> + required_mem_len <= info->fix.smem_len) { >> + 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; >> + unsigned long timeout = jiffies + HZ; >> + int status; >> + >> + xenfb_info = info->par; >> + >> + mutex_lock(&xenfb_info->queue_lock); >> + while ((status = xenfb_queue_full(xenfb_info)) && >> + time_before(jiffies, timeout)) { >> + msleep(10); >> + } >> + if (status == 0) { >> + info->var.xres_virtual = info->var.xres; >> + info->var.yres_virtual = info->var.yres; >> + xenfb_do_resize(xenfb_info, info); >> + } >> > > else fail silently by doing nothing > > >> + mutex_unlock(&xenfb_info->queue_lock); >> + return 0; >> +} >> > > Does the framebuffer code handle the fb_set_par() callback doing > nothing? >As far as the frame buffer is concerned the resolution has been changed and it goes merrily on it''s way. VNC server, backend, will still happily believe the screen is at the old resolution. Worse case the user sees either to much or not enough screen. Hanging here would result in an unusable display. This is really no different than a real video controller that could not adjust to the new setting leaving the hardware settings at the current values. As long as memory bounds are not violated everything still works. xenfb memory bounds can not be violated as the max is allocated at startup and never reduced.> Even if it does: is silently failing acceptable? >I will add an error message.> >> + >> static struct fb_ops xenfb_fb_ops = { >> .owner = THIS_MODULE, >> .fb_setcolreg = xenfb_setcolreg, >> @@ -420,6 +513,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, >> @@ -450,6 +545,8 @@ static int __devinit xenfb_probe(struct >> { >> struct xenfb_info *info; >> struct fb_info *fb_info; >> + int fb_size; >> + int val = 0; >> > > Why do you initialize val? >Removed initialization.> >> int ret; >> >> info = kzalloc(sizeof(*info), GFP_KERNEL); >> @@ -457,24 +554,40 @@ static int __devinit xenfb_probe(struct >> xenbus_dev_fatal(dev, -ENOMEM, "allocating info structure"); >> return -ENOMEM; >> } >> + >> + /* Limit kernel param videoram amount to what is in xenstore */ >> + if (xenbus_scanf(XBT_NIL, dev->otherend, "videoram", "%d", &val) == 1) { >> + if (val < video[KPARAM_MEM]) >> + video[KPARAM_MEM] = val; >> + } >> + >> + /* If requested res does not fit in available memory, use default */ >> + fb_size = video[KPARAM_MEM] * MB_; >> + if (video[KPARAM_WIDTH] * video[KPARAM_HEIGHT] * XENFB_DEPTH/8 > fb_size) { >> + video[KPARAM_WIDTH] = XENFB_WIDTH; >> + video[KPARAM_HEIGHT] = XENFB_HEIGHT; >> + fb_size = XENFB_DEFAULT_FB_LEN; >> + } >> + >> dev->dev.driver_data = info; >> info->xbdev = dev; >> info->irq = -1; >> info->x1 = info->y1 = INT_MAX; >> spin_lock_init(&info->dirty_lock); >> mutex_init(&info->mm_lock); >> + mutex_init(&info->queue_lock); >> init_waitqueue_head(&info->wq); >> init_timer(&info->refresh); >> info->refresh.function = xenfb_timer; >> info->refresh.data = (unsigned long)info; >> INIT_LIST_HEAD(&info->mappings); >> >> - info->fb = vmalloc(xenfb_mem_len); >> + info->fb = vmalloc(fb_size); >> if (info->fb == NULL) >> goto error_nomem; >> - memset(info->fb, 0, xenfb_mem_len); >> - >> - info->nr_pages = (xenfb_mem_len + PAGE_SIZE - 1) >> PAGE_SHIFT; >> + memset(info->fb, 0, fb_size); >> + >> + info->nr_pages = (fb_size + PAGE_SIZE - 1) >> PAGE_SHIFT; >> >> info->pages = kmalloc(sizeof(struct page *) * info->nr_pages, >> GFP_KERNEL); >> @@ -489,8 +602,6 @@ static int __devinit xenfb_probe(struct >> info->page = (void *)__get_free_page(GFP_KERNEL | __GFP_ZERO); >> if (!info->page) >> goto error_nomem; >> - >> - xenfb_init_shared_page(info); >> >> fb_info = framebuffer_alloc(sizeof(u32) * 256, NULL); >> /* see fishy hackery below */ >> @@ -504,9 +615,9 @@ 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.bits_per_pixel = info->page->depth; >> + fb_info->var.xres_virtual = fb_info->var.xres = video[KPARAM_WIDTH]; >> + fb_info->var.yres_virtual = fb_info->var.yres = video[KPARAM_HEIGHT]; >> + fb_info->var.bits_per_pixel = XENFB_DEPTH; >> >> fb_info->var.red = (struct fb_bitfield){16, 8, 0}; >> fb_info->var.green = (struct fb_bitfield){8, 8, 0}; >> @@ -518,9 +629,9 @@ static int __devinit xenfb_probe(struct >> fb_info->var.vmode = FB_VMODE_NONINTERLACED; >> >> fb_info->fix.visual = FB_VISUAL_TRUECOLOR; >> - fb_info->fix.line_length = info->page->line_length; >> + fb_info->fix.line_length = fb_info->var.xres * (XENFB_DEPTH / 8); >> fb_info->fix.smem_start = 0; >> - fb_info->fix.smem_len = xenfb_mem_len; >> + fb_info->fix.smem_len = fb_size; >> strcpy(fb_info->fix.id, "xen"); >> fb_info->fix.type = FB_TYPE_PACKED_PIXELS; >> fb_info->fix.accel = FB_ACCEL_NONE; >> @@ -533,6 +644,9 @@ static int __devinit xenfb_probe(struct >> xenbus_dev_fatal(dev, ret, "fb_alloc_cmap"); >> goto error; >> } >> + >> + /* init shared page, uses fb_info attributes */ >> + xenfb_init_shared_page(info, fb_info); >> > > The comment is somewhat redundant. >Ok> >> >> ret = register_framebuffer(fb_info); >> if (ret) { >> @@ -571,7 +685,7 @@ static int xenfb_resume(struct xenbus_de >> struct xenfb_info *info = dev->dev.driver_data; >> >> xenfb_disconnect_backend(info); >> - xenfb_init_shared_page(info); >> + xenfb_init_shared_page(info, info->fb_info); >> return xenfb_connect_backend(dev, info); >> } >> >> @@ -597,9 +711,11 @@ static int xenfb_remove(struct xenbus_de >> return 0; >> } >> >> -static void xenfb_init_shared_page(struct xenfb_info *info) >> +static void xenfb_init_shared_page(struct xenfb_info *info, >> + struct fb_info * fb_info) >> { >> 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); >> @@ -607,13 +723,14 @@ static void xenfb_init_shared_page(struc >> 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; >> - 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; >> - info->page->mem_length = xenfb_mem_len; >> + for (i = 0; i * epd < info->nr_pages; i++) >> + info->page->pd[i] = vmalloc_to_mfn(&info->mfns[i * epd]); >> + >> + info->page->width = fb_info->var.xres; >> + info->page->height = fb_info->var.yres; >> + info->page->depth = fb_info->var.bits_per_pixel; >> + info->page->line_length = fb_info->fix.line_length; >> + info->page->mem_length = fb_info->fix.smem_len; >> info->page->in_cons = info->page->in_prod = 0; >> info->page->out_cons = info->page->out_prod = 0; >> } >> @@ -712,6 +829,11 @@ static void xenfb_backend_changed(struct >> val = 0; >> if (val) >> info->update_wanted = 1; >> + >> + if (xenbus_scanf(XBT_NIL, dev->otherend, >> + "feature-resize", "%d", &val) < 0) >> + val = 0; >> + info->feature_resize = val; >> break; >> >> case XenbusStateClosing: >> diff -r 3983b041fc51 drivers/xen/fbfront/xenkbd.c >> --- a/drivers/xen/fbfront/xenkbd.c Mon Mar 10 22:53:07 2008 +0000 >> +++ b/drivers/xen/fbfront/xenkbd.c Thu Mar 13 08:26:22 2008 -0600 >> @@ -297,6 +297,16 @@ static void xenkbd_backend_changed(struc >> */ >> if (dev->state != XenbusStateConnected) >> goto InitWait; /* no InitWait seen yet, fudge it */ >> + >> + /* Set input abs params to match backend screen res */ >> + if (xenbus_scanf(XBT_NIL, info->xbdev->otherend, >> + "width", "%d", &val) > 0 ) >> + input_set_abs_params(info->ptr, ABS_X, 0, val, 0, 0); >> + >> + if (xenbus_scanf(XBT_NIL, info->xbdev->otherend, >> + "height", "%d", &val) > 0 ) >> + input_set_abs_params(info->ptr, ABS_Y, 0, val, 0, 0); >> + >> break; >> > > Open question: why do we have to set the parameters to the actual > initial resolution here, but don''t have to set them when the > resolution changes dynamically? >We are dynamically resizing the portal into the video memory, not the video memory itself. The abs values are essentially static once the frame buffer has been allocated and only need to be set to their true value once.> >> >> case XenbusStateClosing: >> diff -r 3983b041fc51 include/xen/interface/io/fbif.h >> --- a/include/xen/interface/io/fbif.h Mon Mar 10 22:53:07 2008 +0000 >> +++ b/include/xen/interface/io/fbif.h Tue Mar 11 17:39:38 2008 -0600 >> @@ -50,12 +50,28 @@ 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; /* width in pixels */ >> + int32_t height; /* height in pixels */ >> + int32_t stride; /* stride in bytes */ >> + int32_t depth; /* depth in bits */ >> +}; >> + >> #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]; >> }; >> >> @@ -109,15 +125,17 @@ struct xenfb_page >> * Each directory page holds PAGE_SIZE / sizeof(*pd) >> * framebuffer pages, and can thus map up to PAGE_SIZE * >> * 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. >> + * sizeof(unsigned long) == 4/8, that''s 4 Megs 32 bit and 2 Megs >> + * 64 bit. 256 directories give enough room for a 512 Meg >> + * framebuffer with a max resolution of 12,800x10,240. Should >> + * be enough for a while with room leftover for expansion. >> */ >> - unsigned long pd[2]; >> + unsigned long pd[256]; >> }; >> >> /* >> - * Wart: xenkbd needs to know resolution. Put it here until a better >> - * solution is found, but don''t leak it to the backend. >> + * Wart: xenkbd needs to know default resolution. Put it here until a >> + * better solution is found, but don''t leak it to the backend. >> */ >> #ifdef __KERNEL__ >> #define XENFB_WIDTH 800 >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xensource.com >> http://lists.xensource.com/xen-devel >> > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Markus Armbruster
2008-Mar-17 17:35 UTC
Re: [Xen-devel][PATCH][LINUX] Dynamic modes support for xenfb (2 of 2)
Pat Campbell <plc@novell.com> writes:> Markus Armbruster wrote: >> Pat Campbell <plc@novell.com> writes: >> >> >>> Attached patch adds dynamic frame buffer resolution support to >>> the PV xenfb frame buffer driver. >>> >>> 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> >>> >> >> Adding another lock (queue_lock) to our already ticklish locking gives >> me a queasy feeling... >> >> The purpose of the lock is not obvious to me. Please explain that, in >> the code. Likewise, it''s not entirely obvious that the locking works. >> Please update the "How the locks work together" comment. >> >> But before you do that, I suggest you tell us exactly what problem >> you''re attempting to solve with queue_lock. Maybe we can come up with >> a less scary solution. Maybe not. But it''s worth a try. If it''s >> just to ensure the changes made by xenfb_set_par() are seen in >> xenfb_do_resize() correctly, a memory barrier should to the trick more >> easily. >> >> > xenfb_set_par() needs to complete it''s work before returning to the > caller so I need to send the resize event from within the > xenfb_set_par() function. This would mean we now have two event queue > writers, xenfb_update_screen() and xenfb_set_par(). Very simple 2 > writer one reader problem easily solved by the addition of the queue_lock. > > Previously I handled the resize in xenfb_thread(). This could lead to > us missing a resize and or using the incorrect values on a multi > processor system. > > IE: Wrong resolution > P1:1 set_par sets resize_dpy w, h already in fb struct > P2:1 thread wakeup sees resize_dpy set > P2:2 do_resize() > P2:3 read fb width > P1:3 check_var/set_par changes fb height > P2:4 read fb height (2nd value here) > > IE: Missed resolution change > P1:1 set_par sets resize_dpy w, h already in fb struct > P2:1 thread wakeup sees resize_dpy set > P2:2 do_resize() > P2:3 read fb width > P2:4 read fb height > P1:3 set_par sets resize_dpy > P2:5 clears resize_dpy (Missed the second resize) > > Adding in a queue lock should be safe because: > 1. xenfb_update_screen() can hold a mutex without interfering with > zap_page_range > 2. xenfb_set_par() will timeout releasing the lock so update can run > > I have verified that the lock works as expected and times out if necessary.Your race is real. Your old code shared the resolution state (info->resize_dpy and the resolution variables in info->fb_info) between xenfb_do_resize() (running in xenfb_thread) and xenfb_set_par() (running in another thread). Your new code shares the ring buffer instead. Both methods can be made race-free with suitable synchronization. With your old code, xenfb_set_par() always succeeds. Until the backend gets around to process the XENFB_TYPE_RESIZE message, it interprets the frame buffer for the old resolution. As you argue below, this isn''t fatal, it can just mess up the display somewhat. With your new code, xenfb_set_par() can take a long time (one second), and it can fail, leaving the display in a deranged state until the next resolution change. It still doesn''t fully synchronize with the backend: it returns successfully after sending the message, leaving the time window until the backend receives the message open. I''d prefer the old behavior. But I could be missing something here. Am I? I think you could fix the old code by protecting access to the shared resolution state by a spin lock. If I am missing something, and your new code is the way to go, you still need to migrate your explanation why it works from e-mail to source file, where the poor maintainer can find it. [...] Looks like this is the last issue that you haven''t addressed / explained fully yet :) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Pat Campbell
2008-Mar-18 19:11 UTC
Re: [Xen-devel][PATCH][LINUX] Dynamic modes support for xenfb (2 of 2)
Markus Armbruster wrote:> Pat Campbell <plc@novell.com> writes: > > >> Markus Armbruster wrote: >> >>> Pat Campbell <plc@novell.com> writes: >>> >>> >>> >>>> Attached patch adds dynamic frame buffer resolution support to >>>> the PV xenfb frame buffer driver. >>>> >>>> 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> >>>> >>>> >>> Adding another lock (queue_lock) to our already ticklish locking gives >>> me a queasy feeling... >>> >>> The purpose of the lock is not obvious to me. Please explain that, in >>> the code. Likewise, it''s not entirely obvious that the locking works. >>> Please update the "How the locks work together" comment. >>> >>> But before you do that, I suggest you tell us exactly what problem >>> you''re attempting to solve with queue_lock. Maybe we can come up with >>> a less scary solution. Maybe not. But it''s worth a try. If it''s >>> just to ensure the changes made by xenfb_set_par() are seen in >>> xenfb_do_resize() correctly, a memory barrier should to the trick more >>> easily. >>> >>> >>> >> xenfb_set_par() needs to complete it''s work before returning to the >> caller so I need to send the resize event from within the >> xenfb_set_par() function. This would mean we now have two event queue >> writers, xenfb_update_screen() and xenfb_set_par(). Very simple 2 >> writer one reader problem easily solved by the addition of the queue_lock. >> >> Previously I handled the resize in xenfb_thread(). This could lead to >> us missing a resize and or using the incorrect values on a multi >> processor system. >> >> IE: Wrong resolution >> P1:1 set_par sets resize_dpy w, h already in fb struct >> P2:1 thread wakeup sees resize_dpy set >> P2:2 do_resize() >> P2:3 read fb width >> P1:3 check_var/set_par changes fb height >> P2:4 read fb height (2nd value here) >> >> IE: Missed resolution change >> P1:1 set_par sets resize_dpy w, h already in fb struct >> P2:1 thread wakeup sees resize_dpy set >> P2:2 do_resize() >> P2:3 read fb width >> P2:4 read fb height >> P1:3 set_par sets resize_dpy >> P2:5 clears resize_dpy (Missed the second resize) >> >> Adding in a queue lock should be safe because: >> 1. xenfb_update_screen() can hold a mutex without interfering with >> zap_page_range >> 2. xenfb_set_par() will timeout releasing the lock so update can run >> >> I have verified that the lock works as expected and times out if necessary. >> > > Your race is real. > > Your old code shared the resolution state (info->resize_dpy and the > resolution variables in info->fb_info) between xenfb_do_resize() > (running in xenfb_thread) and xenfb_set_par() (running in another > thread). > > Your new code shares the ring buffer instead. > > Both methods can be made race-free with suitable synchronization. > > With your old code, xenfb_set_par() always succeeds. Until the > backend gets around to process the XENFB_TYPE_RESIZE message, it > interprets the frame buffer for the old resolution. As you argue > below, this isn''t fatal, it can just mess up the display somewhat. > > With your new code, xenfb_set_par() can take a long time (one second), > and it can fail, leaving the display in a deranged state until the > next resolution change. It still doesn''t fully synchronize with the > backend: it returns successfully after sending the message, leaving > the time window until the backend receives the message open. >Synchronizing with the backend? I don''t think this is necessary. The update events that follow the resize event will be for the new size, as long as the backend gets the resize event we should be ok.> I''d prefer the old behavior. But I could be missing something here. > Am I? > >I like the new behavior, it seems more straight forward and does not rely on the xenfb_thread() being active. Our first set_par() happens during frame buffer registration which is before xenfb_thread() is started. I disliked adding new code into the xenfb_update() function for an event that happens rarely so will revert back to the sharing of resize_dpy flag code. Is this how you envision xenfb_set_par() synchronization? static int xenfb_set_par(struct fb_info *info) { struct xenfb_info *xenfb_info; unsigned long flags; xenfb_info = info->par; if (xenfb_info->kthread) { spin_lock_irqsave(&xenfb_info->resize_lock, flags); info->var.xres_virtual = info->var.xres; info->var.yres_virtual = info->var.yres; xenfb_info->resize_dpy = 1; /* Wake up xenfb_thread() */ xenfb_refresh(xenfb_info, 0, 0, 1, 1); while (xenfb_info->kthread && xenfb_info->resize_dpy == 1) { msleep(10); } spin_unlock_irqrestore(&xenfb_info->resize_lock, flags); } return 0; } To explain the code a liitle bit. 1. Need to test for kthread because first xenfb_set_par() happens before xenfb_thread() is started 2. Need to wakeup xenfb_thread via refresh as application screen events are blocked waiting on the set_par to complete. Can''t just set dirty, will get a bogus nasty gram. 3. Testing xenfb_thread in the while loop in case xenfb_remove() is called and thread is shutdown. Protects against a system shutdown hang. Don''t know if that can happen, defensive code. ------------------- New lock comment /* * There are three locks: spinlock resize_lock protecting resize_dpy, * spinlock dirty_lock protecting the dirty rectangle and mutex * mm_lock protecting mappings. * * resize_lock is used to synchronize resize_dpy access between * xenfb_set_par() and xenfb_do_resize() running in xenfb_thread(). * * How the dirty and mapping locks work together * ..........> I think you could fix the old code by protecting access to the shared > resolution state by a spin lock. > > If I am missing something, and your new code is the way to go, you > still need to migrate your explanation why it works from e-mail to > source file, where the poor maintainer can find it. > > [...] > > Looks like this is the last issue that you haven''t addressed / > explained fully yet :) > >Yahoo, getting close. Thanks Just an FYI, I am at Brainshare this week so my responses might be a little slow but I will try to turn around any comments I receive as soon as possible. Like to put this to bed before any more staging changes take place. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Markus Armbruster
2008-Mar-19 10:21 UTC
Re: [Xen-devel][PATCH][LINUX] Dynamic modes support for xenfb (2 of 2)
Pat Campbell <plc@novell.com> writes:> Markus Armbruster wrote: >> Pat Campbell <plc@novell.com> writes: >> >> >>> Markus Armbruster wrote: >>> >>>> Pat Campbell <plc@novell.com> writes: >>>> >>>> >>>> >>>>> Attached patch adds dynamic frame buffer resolution support to >>>>> the PV xenfb frame buffer driver. >>>>> >>>>> 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> >>>>> >>>>> >>>> Adding another lock (queue_lock) to our already ticklish locking gives >>>> me a queasy feeling... >>>> >>>> The purpose of the lock is not obvious to me. Please explain that, in >>>> the code. Likewise, it''s not entirely obvious that the locking works. >>>> Please update the "How the locks work together" comment. >>>> >>>> But before you do that, I suggest you tell us exactly what problem >>>> you''re attempting to solve with queue_lock. Maybe we can come up with >>>> a less scary solution. Maybe not. But it''s worth a try. If it''s >>>> just to ensure the changes made by xenfb_set_par() are seen in >>>> xenfb_do_resize() correctly, a memory barrier should to the trick more >>>> easily. >>>>[Pat explains the race...]>> >> Your race is real. >> >> Your old code shared the resolution state (info->resize_dpy and the >> resolution variables in info->fb_info) between xenfb_do_resize() >> (running in xenfb_thread) and xenfb_set_par() (running in another >> thread). >> >> Your new code shares the ring buffer instead. >> >> Both methods can be made race-free with suitable synchronization. >> >> With your old code, xenfb_set_par() always succeeds. Until the >> backend gets around to process the XENFB_TYPE_RESIZE message, it >> interprets the frame buffer for the old resolution. As you argue >> below, this isn''t fatal, it can just mess up the display somewhat. >> >> With your new code, xenfb_set_par() can take a long time (one second), >> and it can fail, leaving the display in a deranged state until the >> next resolution change. It still doesn''t fully synchronize with the >> backend: it returns successfully after sending the message, leaving >> the time window until the backend receives the message open. >> > Synchronizing with the backend? I don''t think this is necessary. The > update events that follow the resize event will be for the new size, as > long as the backend gets the resize event we should be ok.I didn''t mean to say that synchronizing with the backend is necessary. I just wanted to point out that your new method pays the price of synchronization, namely a possibly significant delay in xenfb_set_par(), plus an ugly failure mode there, without actually achieving synchronization.>> I''d prefer the old behavior. But I could be missing something here. >> Am I? >> >> > I like the new behavior, it seems more straight forward and does not > rely on the xenfb_thread() being active. Our first set_par() happens > during frame buffer registration which is before xenfb_thread() is > started. I disliked adding new code into the xenfb_update() function > for an event that happens rarely so will revert back to the sharing of > resize_dpy flag code. > > Is this how you envision xenfb_set_par() synchronization? > > static int xenfb_set_par(struct fb_info *info) > { > struct xenfb_info *xenfb_info; > unsigned long flags; > > xenfb_info = info->par; > > if (xenfb_info->kthread) { > spin_lock_irqsave(&xenfb_info->resize_lock, flags); > info->var.xres_virtual = info->var.xres; > info->var.yres_virtual = info->var.yres; > xenfb_info->resize_dpy = 1; > /* Wake up xenfb_thread() */ > xenfb_refresh(xenfb_info, 0, 0, 1, 1); > while (xenfb_info->kthread && xenfb_info->resize_dpy == 1) { > msleep(10); > } > spin_unlock_irqrestore(&xenfb_info->resize_lock, flags); > } > return 0; > } > > To explain the code a liitle bit. > 1. Need to test for kthread because first xenfb_set_par() happens > before xenfb_thread() is startedWhy can you just ignore the mode change then?> 2. Need to wakeup xenfb_thread via refresh as application screen events > are blocked waiting on the set_par to complete. Can''t just set dirty, > will get a bogus nasty gram.I''d pass an empty rectangle there: xenfb_refresh(xenfb_info, INT_MAX, INT_MAX, 0, 0); Alternatively, factor out the code to wake up the thread into a new function, and call that here and from __xenfb_refresh(). That would be cleaner.> 3. Testing xenfb_thread in the while loop in case xenfb_remove() is > called and thread is shutdown. Protects against a system shutdown > hang. Don''t know if that can happen, defensive code.Sleeps while holding a spinlock. Not good. And I didn''t get the hang you''re trying to defend against. xenfb_resize_screen() still accesses the size unsynchronized, and can thus see intermediate states of resolution changes. No, that''s not what I had in mind. Let me sketch it. The first question to answer for a mutex is: what shared state does it protect? resize_lock protects the screen size fb_info->var.xres, fb_info->var.yres and the flag resize_dpy. Once that''s clear, the use of the mutex becomes obvious: wrap it around any access of the shared state, i.e. the updating of the shared state it protects in xenfb_set_par() and the reading in xenfb_thread(). Code sketch: static void xenfb_resize_screen(struct xenfb_info *info) { if (xenfb_queue_full(info)) return; /* caller holds info->resize_lock */ info->resize_dpy = 0; xenfb_do_resize(info); } static int xenfb_thread(void *data) { struct xenfb_info *info = data; unsigned long flags; while (!kthread_should_stop()) { spin_lock_irqsave(info->resize_lock, flags); if (info->resize_dpy) xenfb_resize_screen(info); spin_unlock_irqrestore(info->resize_lock, flags); [...] } [...] static int xenfb_set_par(struct fb_info *info) { struct xenfb_info *xenfb_info = info->par; unsigned long flags; spin_lock_irqsave(info->resize_lock, flags); info->var.xres_virtual = info->var.xres; info->var.yres_virtual = info->var.yres; xenfb_info->resize_dpy = 1; spin_unlock_irqrestore(info->resize_lock, flags); if (xenfb_info->kthread) { /* Wake up xenfb_thread() */ xenfb_refresh(xenfb_info, INT_MAX, INT_MAX, 0, 0); } return 0; } Note that xenfb_set_par() can schedule resolution changes just fine before xenfb_thread() runs. It''ll pick them up right when it starts. I used xenfb_refresh() to wake up xenfb_thread() only to keep things simple, not to express a preference for that method. Don''t just copy my code sketch! Review it carefully, please.> > > ------------------- > New lock comment > > /* > * There are three locks: spinlock resize_lock protecting resize_dpy,It actually protects the screen size and resize_dpy.> * spinlock dirty_lock protecting the dirty rectangle and mutex > * mm_lock protecting mappings. > * > * resize_lock is used to synchronize resize_dpy access between > * xenfb_set_par() and xenfb_do_resize() running in xenfb_thread(). > * > * How the dirty and mapping locks work together > * > .......... > > >> I think you could fix the old code by protecting access to the shared >> resolution state by a spin lock. >> >> If I am missing something, and your new code is the way to go, you >> still need to migrate your explanation why it works from e-mail to >> source file, where the poor maintainer can find it. >> >> [...] >> >> Looks like this is the last issue that you haven''t addressed / >> explained fully yet :) >> >> > Yahoo, getting close. Thanks > > Just an FYI, I am at Brainshare this week so my responses might be a > little slow but I will try to turn around any comments I receive as soon > as possible. Like to put this to bed before any more staging changes > take place.Me too! And thanks for pushing this feature all the way. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Pat Campbell
2008-Mar-24 03:40 UTC
Re: [Xen-devel][PATCH][LINUX] Dynamic modes support for xenfb (2 of 2)
Markus Armbruster wrote:> Pat Campbell <plc@novell.com> writes: > > >> Markus Armbruster wrote: >> >>> Pat Campbell <plc@novell.com> writes: >>> >>> >>> >>>> Markus Armbruster wrote: >>>> >>>> >>>>> Pat Campbell <plc@novell.com> writes: >>>>> >>>>> >>>>> >>>>> >>>>>> Attached patch adds dynamic frame buffer resolution support to >>>>>> the PV xenfb frame buffer driver. >>>>>> >>>>>> 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> >>>>>> >>>>>> >>>>>> >>>>> Adding another lock (queue_lock) to our already ticklish locking gives >>>>> me a queasy feeling... >>>>> >>>>> The purpose of the lock is not obvious to me. Please explain that, in >>>>> the code. Likewise, it''s not entirely obvious that the locking works. >>>>> Please update the "How the locks work together" comment. >>>>> >>>>> But before you do that, I suggest you tell us exactly what problem >>>>> you''re attempting to solve with queue_lock. Maybe we can come up with >>>>> a less scary solution. Maybe not. But it''s worth a try. If it''s >>>>> just to ensure the changes made by xenfb_set_par() are seen in >>>>> xenfb_do_resize() correctly, a memory barrier should to the trick more >>>>> easily. >>>>> >>>>> > [Pat explains the race...] > >>> Your race is real. >>> >>> Your old code shared the resolution state (info->resize_dpy and the >>> resolution variables in info->fb_info) between xenfb_do_resize() >>> (running in xenfb_thread) and xenfb_set_par() (running in another >>> thread). >>> >>> Your new code shares the ring buffer instead. >>> >>> Both methods can be made race-free with suitable synchronization. >>> >>> With your old code, xenfb_set_par() always succeeds. Until the >>> backend gets around to process the XENFB_TYPE_RESIZE message, it >>> interprets the frame buffer for the old resolution. As you argue >>> below, this isn''t fatal, it can just mess up the display somewhat. >>> >>> With your new code, xenfb_set_par() can take a long time (one second), >>> and it can fail, leaving the display in a deranged state until the >>> next resolution change. It still doesn''t fully synchronize with the >>> backend: it returns successfully after sending the message, leaving >>> the time window until the backend receives the message open. >>> >>> >> Synchronizing with the backend? I don''t think this is necessary. The >> update events that follow the resize event will be for the new size, as >> long as the backend gets the resize event we should be ok. >> > > I didn''t mean to say that synchronizing with the backend is necessary. > I just wanted to point out that your new method pays the price of > synchronization, namely a possibly significant delay in > xenfb_set_par(), plus an ugly failure mode there, without actually > achieving synchronization. > > >>> I''d prefer the old behavior. But I could be missing something here. >>> Am I? >>> >>> >>> >> I like the new behavior, it seems more straight forward and does not >> rely on the xenfb_thread() being active. Our first set_par() happens >> during frame buffer registration which is before xenfb_thread() is >> started. I disliked adding new code into the xenfb_update() function >> for an event that happens rarely so will revert back to the sharing of >> resize_dpy flag code. >> >> Is this how you envision xenfb_set_par() synchronization? >> >> static int xenfb_set_par(struct fb_info *info) >> { >> struct xenfb_info *xenfb_info; >> unsigned long flags; >> >> xenfb_info = info->par; >> >> if (xenfb_info->kthread) { >> spin_lock_irqsave(&xenfb_info->resize_lock, flags); >> info->var.xres_virtual = info->var.xres; >> info->var.yres_virtual = info->var.yres; >> xenfb_info->resize_dpy = 1; >> /* Wake up xenfb_thread() */ >> xenfb_refresh(xenfb_info, 0, 0, 1, 1); >> while (xenfb_info->kthread && xenfb_info->resize_dpy == 1) { >> msleep(10); >> } >> spin_unlock_irqrestore(&xenfb_info->resize_lock, flags); >> } >> return 0; >> } >> >> To explain the code a liitle bit. >> 1. Need to test for kthread because first xenfb_set_par() happens >> before xenfb_thread() is started >> > > Why can you just ignore the mode change then? > > >> 2. Need to wakeup xenfb_thread via refresh as application screen events >> are blocked waiting on the set_par to complete. Can''t just set dirty, >> will get a bogus nasty gram. >> > > I''d pass an empty rectangle there: > > xenfb_refresh(xenfb_info, INT_MAX, INT_MAX, 0, 0); > > Alternatively, factor out the code to wake up the thread into a new > function, and call that here and from __xenfb_refresh(). That would > be cleaner. > > >> 3. Testing xenfb_thread in the while loop in case xenfb_remove() is >> called and thread is shutdown. Protects against a system shutdown >> hang. Don''t know if that can happen, defensive code. >> > > Sleeps while holding a spinlock. Not good. And I didn''t get the hang > you''re trying to defend against. > > xenfb_resize_screen() still accesses the size unsynchronized, and can > thus see intermediate states of resolution changes. > > > No, that''s not what I had in mind. Let me sketch it. > > The first question to answer for a mutex is: what shared state does it > protect? resize_lock protects the screen size fb_info->var.xres, > fb_info->var.yres and the flag resize_dpy. > > Once that''s clear, the use of the mutex becomes obvious: wrap it > around any access of the shared state, i.e. the updating of the shared > state it protects in xenfb_set_par() and the reading in > xenfb_thread(). Code sketch: > > static void xenfb_resize_screen(struct xenfb_info *info) > { > if (xenfb_queue_full(info)) > return; > > /* caller holds info->resize_lock */ > info->resize_dpy = 0; > xenfb_do_resize(info); > } > > static int xenfb_thread(void *data) > { > struct xenfb_info *info = data; > unsigned long flags; > > while (!kthread_should_stop()) { > spin_lock_irqsave(info->resize_lock, flags); > if (info->resize_dpy) > xenfb_resize_screen(info); > spin_unlock_irqrestore(info->resize_lock, flags); > [...] > } > [...] > static int xenfb_set_par(struct fb_info *info) > { > struct xenfb_info *xenfb_info = info->par; > unsigned long flags; > > spin_lock_irqsave(info->resize_lock, flags); > info->var.xres_virtual = info->var.xres; > info->var.yres_virtual = info->var.yres; > xenfb_info->resize_dpy = 1; > spin_unlock_irqrestore(info->resize_lock, flags); > > if (xenfb_info->kthread) { > /* Wake up xenfb_thread() */ > xenfb_refresh(xenfb_info, INT_MAX, INT_MAX, 0, 0); > } > return 0; > } > > Note that xenfb_set_par() can schedule resolution changes just fine > before xenfb_thread() runs. It''ll pick them up right when it starts. > > I used xenfb_refresh() to wake up xenfb_thread() only to keep things > simple, not to express a preference for that method. > > Don''t just copy my code sketch! Review it carefully, please. > > >> ------------------- >> New lock comment >> >> /* >> * There are three locks: spinlock resize_lock protecting resize_dpy, >> > > It actually protects the screen size and resize_dpy. > > >> * spinlock dirty_lock protecting the dirty rectangle and mutex >> * mm_lock protecting mappings. >> * >> * resize_lock is used to synchronize resize_dpy access between >> * xenfb_set_par() and xenfb_do_resize() running in xenfb_thread(). >> * >> * How the dirty and mapping locks work together >> * >> .......... >> >> >> >>> I think you could fix the old code by protecting access to the shared >>> resolution state by a spin lock. >>> >>> If I am missing something, and your new code is the way to go, you >>> still need to migrate your explanation why it works from e-mail to >>> source file, where the poor maintainer can find it. >>> >>> [...] >>> >>> Looks like this is the last issue that you haven''t addressed / >>> explained fully yet :) >>> >>> >>> >> Yahoo, getting close. Thanks >> >> Just an FYI, I am at Brainshare this week so my responses might be a >> little slow but I will try to turn around any comments I receive as soon >> as possible. Like to put this to bed before any more staging changes >> take place. >> > > Me too! And thanks for pushing this feature all the way. > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel >I have attached front and back for your review. Back has not changed except to include opengl changes. Front has synchronization fix. I added a spin lock and struct xenfb_resize into struct xenfb. struct xenfb_resize was added to protect the screen size values from changes outside the driver. IE User application calls to ioctl(fb, FBIOPUT_VSCREENINFO, &fb_var); while the driver is processing a previous call. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Markus Armbruster
2008-Mar-25 14:43 UTC
Re: [Xen-devel][PATCH][LINUX] Dynamic modes support for xenfb (2 of 2)
Pat Campbell <plc@novell.com> writes:> Markus Armbruster wrote: >> Pat Campbell <plc@novell.com> writes: >> >> >>> Markus Armbruster wrote: >>> >>>> Pat Campbell <plc@novell.com> writes: >>>> >>>> >>>> >>>>> Markus Armbruster wrote: >>>>> >>>>> >>>>>> Pat Campbell <plc@novell.com> writes: >>>>>> >>>>>> >>>>>> >>>>>> >>>>>>> Attached patch adds dynamic frame buffer resolution support to >>>>>>> the PV xenfb frame buffer driver.[...]> I have attached front and back for your review. Back has not changed > except to include opengl changes. Front has synchronization fix. I > added a spin lock and struct xenfb_resize into struct xenfb. struct > xenfb_resize was added to protect the screen size values from changes > outside the driver. IE User application calls to ioctl(fb, > FBIOPUT_VSCREENINFO, &fb_var); while the driver is processing a previous > call.Yup, protection against that is required. Looks good! One easily fixed bug and a few remarks inline. [...]> diff -r de57c3f218fb drivers/xen/fbfront/xenfb.c > --- a/drivers/xen/fbfront/xenfb.c Thu Mar 20 11:35:25 2008 +0000 > +++ b/drivers/xen/fbfront/xenfb.c Sun Mar 23 19:52:17 2008 -0600 > @@ -62,15 +62,21 @@ struct xenfb_info > struct xenfb_page *page; > unsigned long *mfns; > int update_wanted; /* XENFB_TYPE_UPDATE wanted */ > + int feature_resize; /* Backend has resize feature */ > + struct xenfb_resize resize; > + int resize_dpy; > + spinlock_t resize_lock; > > struct xenbus_device *xbdev; > }; > > /* > - * How the locks work together > - * > - * There are two locks: spinlock dirty_lock protecting the dirty > - * rectangle, and mutex mm_lock protecting mappings. > + * There are three locks: > + * spinlock resize_lock protecting resize_dpy and screen sizeSuggest * spinlock resize_lock protecting resize_dpy and resize because with the new screen size in one place now we can be both both specific and concise here.> + * spinlock dirty_lock protecting the dirty rectangle > + * mutex mm_lock protecting mappings. > + * > + * How the dirty and mapping locks work together > * > * The problem is that dirty rectangle and mappings aren''t > * independent: the dirty rectangle must cover all faulted pages in[...]> @@ -150,14 +177,22 @@ static void xenfb_do_update(struct xenfb > event.update.width = w; > event.update.height = h; > > - 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_send_event(info, &event); > +} > + > +static void xenfb_do_resize(struct xenfb_info *info) > +{ > + union xenfb_out_event event; > + > + event.type = XENFB_TYPE_RESIZE; > + event.resize.width = info->resize.width; > + event.resize.height = info->resize.height; > + event.resize.stride = info->resize.stride; > + event.resize.depth = info->resize.depth;Could do event.resize = info->resize, except info->resize.type isn''t set up for that (see below).> + > + /* caller ensures !xenfb_queue_full() */ > + xenfb_send_event(info, &event); > } > > static int xenfb_queue_full(struct xenfb_info *info) > @@ -209,11 +244,26 @@ static void xenfb_update_screen(struct x > xenfb_do_update(info, x1, y1, x2 - x1, y2 - y1); > } > > +static void xenfb_handle_resize_dpy(struct xenfb_info *info) > +{ > + int flags;Type error! unsigned long flags> + > + spin_lock_irqsave(&info->resize_lock, flags); > + if (info->resize_dpy) { > + if (!xenfb_queue_full(info)) { > + info->resize_dpy = 0; > + xenfb_do_resize(info); > + } > + } > + spin_unlock_irqrestore(&info->resize_lock, flags); > +} > + > static int xenfb_thread(void *data) > { > struct xenfb_info *info = data; > > while (!kthread_should_stop()) { > + xenfb_handle_resize_dpy(info); > if (info->dirty) { > info->dirty = 0; > xenfb_update_screen(info); > @@ -413,6 +463,55 @@ 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; > + int required_mem_len; > + > + xenfb_info = info->par; > + > + if (!xenfb_info->feature_resize) { > + if (var->xres == video[KPARAM_WIDTH] && > + var->yres == video[KPARAM_HEIGHT] && > + var->bits_per_pixel == xenfb_info->page->depth) { > + return 0; > + } > + return -EINVAL; > + } > + > + /* Can''t resize past initial width and height */ > + if (var->xres > video[KPARAM_WIDTH] || var->yres > video[KPARAM_HEIGHT]) > + return -EINVAL; > + > + required_mem_len = var->xres * var->yres * (xenfb_info->page->depth / 8); > + if (var->bits_per_pixel == xenfb_info->page->depth && > + var->xres <= info->fix.line_length / (XENFB_DEPTH / 8) && > + required_mem_len <= info->fix.smem_len) { > + 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; > + unsigned long flags; > + > + xenfb_info = info->par; > + > + spin_lock_irqsave(&xenfb_info->resize_lock, flags); > + xenfb_info->resize.width = info->var.xres; > + xenfb_info->resize.height = info->var.yres; > + xenfb_info->resize.stride = info->fix.line_length; > + xenfb_info->resize.depth = info->var.bits_per_pixel; > + xenfb_info->resize_dpy = 1;Not a bug, just a little trap for the unwary: xenfb_info->resize.type remains zero.> + spin_unlock_irqrestore(&xenfb_info->resize_lock, flags); > + return 0; > +} > + > static struct fb_ops xenfb_fb_ops = { > .owner = THIS_MODULE, > .fb_setcolreg = xenfb_setcolreg,[...] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Samuel Thibault
2008-Mar-25 16:31 UTC
Re: [Xen-devel][PATCH][LINUX] Dynamic modes support for xenfb (2 of 2)
Hello, Markus Armbruster, le Tue 25 Mar 2008 15:43:28 +0100, a écrit :> Looks good! One easily fixed bug and a few remarks inline.Cool! Since there were no remarks on the backend part, could it be commited? We need it for the stubdomains. Samuel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Markus Armbruster
2008-Mar-25 16:46 UTC
Re: [Xen-devel][PATCH][LINUX] Dynamic modes support for xenfb (2 of 2)
Samuel Thibault <samuel.thibault@eu.citrix.com> writes:> Hello, > > Markus Armbruster, le Tue 25 Mar 2008 15:43:28 +0100, a écrit : >> Looks good! One easily fixed bug and a few remarks inline. > > Cool! > > Since there were no remarks on the backend part, could it be commited? > We need it for the stubdomains. > > SamuelFine with me. The backend looked okay in the last iteration already[*], and it didn''t substantially change. [*] It could still use a few tastefull line-breaks, though ;) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Pat Campbell
2008-Mar-25 20:10 UTC
[Xen-devel][PATCH][LINUX] Dynamic modes support for xenfb (2 of 2)
Attached patch adds dynamic frame buffer resolution support to the PV xenfb frame buffer driver. 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-Mar-26 09:45 UTC
Re: [Xen-devel][PATCH][LINUX] Dynamic modes support for xenfb (2 of 2)
Pat Campbell <plc@novell.com> writes:> Attached patch adds dynamic frame buffer resolution support to > the PV xenfb frame buffer driver. > > 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>Acked-by: Markus Armbruster <armbru@redhat.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel