Pat Campbell
2008-Mar-09 21:19 UTC
[Xen-devel][RFC] Dynamic modes support for PV xenfb (included)
Patches are against xen-unstable tip
What changed since last comment round. (No major structural
surprises this time:-).
Backend:
1. In the resize event handler
Disabled shared memory (Need to fix)
Invalidate the buffer
2. Added videoram config variable to limit hostile front
end frame buffer memory size.
3. Sets feature-resize and width/height in xenstore
before frontend connected. width/height are for xenkbd
abs input config values
Frontend:
1. Variable usage cleanup. Now using the fb variables
in most cases. Only two fields added to main struct
2. Removed resize event send in xenfb_resume(). There is
a general race condition where the vnc view will be left
at 600x400 if attached to too early. (Not resize related)
3. Removed refresh call in xenfb_resize_screen(), back end
invalidates buffer in resize event handler now
4. xenkbd gets width and height for abs input in Connected
After I get some feed back, will wait a day or two, I will
prepare a proper patch set.
Pat
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Samuel Thibault
2008-Mar-09 21:29 UTC
Re: [Xen-devel][RFC] Dynamic modes support for PV xenfb (included)
Pat Campbell, le Sun 09 Mar 2008 15:19:59 -0600, a écrit :> 1. In the resize event handler > Disabled shared memory (Need to fix)(Note: that''s not a problem, we''ll fix that after it gets commited) Samuel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Samuel Thibault
2008-Mar-10 12:36 UTC
Re: [Xen-devel][RFC] Dynamic modes support for PV xenfb (included)
Pat Campbell, le Sun 09 Mar 2008 15:19:59 -0600, a écrit :> + dpy_resize(xenfb->ds, xenfb->width, xenfb->height);Note: in today''s tip, there is a 4th parameter which is the stride. Samuel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Samuel Thibault
2008-Mar-10 16:16 UTC
Re: [Xen-devel][RFC] Dynamic modes support for PV xenfb (included)
Pat Campbell, le Sun 09 Mar 2008 15:19:59 -0600, a écrit :> diff -r 854b0704962b tools/ioemu/hw/xenfb.c > --- a/tools/ioemu/hw/xenfb.c Wed Mar 05 09:43:03 2008 +0000 > +++ b/tools/ioemu/hw/xenfb.c Fri Mar 07 13:44:09 2008 -0600 > @@ -516,6 +516,15 @@ static void xenfb_on_fb_event(struct xen > } > xenfb_guest_copy(xenfb, x, y, w, h); > break; > + case XENFB_TYPE_RESIZE: > + xenfb->width = event->resize.width; > + xenfb->height = event->resize.height; > + xenfb->row_stride = event->resize.stride; > + /* Disable video buf sharing, not compatable with resizing */ > + dpy_colourdepth(xenfb->ds, 0); > + dpy_resize(xenfb->ds, xenfb->width, xenfb->height);Insert if (xenfb->ds->shared_buf) dpy_setdata(xenfb->ds, xenfb->pixels); just after dpy_colourdepth() and dpy_resize(), and the video buf sharing will work (so you can replace 0 with xenfb->depth in the colourdepth call).> + xenfb_invalidate(xenfb); > + break;Samuel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Samuel Thibault
2008-Mar-10 16:36 UTC
Re: [Xen-devel][RFC] Dynamic modes support for PV xenfb (included)
Samuel Thibault, le Mon 10 Mar 2008 16:16:57 +0000, a écrit :> Pat Campbell, le Sun 09 Mar 2008 15:19:59 -0600, a écrit : > > diff -r 854b0704962b tools/ioemu/hw/xenfb.c > > --- a/tools/ioemu/hw/xenfb.c Wed Mar 05 09:43:03 2008 +0000 > > +++ b/tools/ioemu/hw/xenfb.c Fri Mar 07 13:44:09 2008 -0600 > > @@ -516,6 +516,15 @@ static void xenfb_on_fb_event(struct xen > > } > > xenfb_guest_copy(xenfb, x, y, w, h); > > break; > > + case XENFB_TYPE_RESIZE: > > + xenfb->width = event->resize.width; > > + xenfb->height = event->resize.height; > > + xenfb->row_stride = event->resize.stride; > > + /* Disable video buf sharing, not compatable with resizing */ > > + dpy_colourdepth(xenfb->ds, 0); > > + dpy_resize(xenfb->ds, xenfb->width, xenfb->height); > > Insert if (xenfb->ds->shared_buf) dpy_setdata(xenfb->ds, xenfb->pixels); > just after dpy_colourdepth() and dpy_resize(), and the video buf sharing > will work (so you can replace 0 with xenfb->depth in the colourdepth > call).That actually leads me to the the question of an "offset" support in the resize event; it would actually be very easy: just add xenfb->offset to xenfb->pixels in the dpy_setdata call, as well as in the BLT macro and memcpy call in xenfb_guest_copy. Samuel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Markus Armbruster
2008-Mar-12 17:04 UTC
Re: [Xen-devel][RFC] Dynamic modes support for PV xenfb (included)
Pat Campbell <plc@novell.com> writes:> Patches are against xen-unstable tip > > What changed since last comment round. (No major structural > surprises this time:-). > > Backend: > 1. In the resize event handler > Disabled shared memory (Need to fix) > Invalidate the buffer > 2. Added videoram config variable to limit hostile front > end frame buffer memory size. > 3. Sets feature-resize and width/height in xenstore > before frontend connected. width/height are for xenkbd > abs input config values > > Frontend: > 1. Variable usage cleanup. Now using the fb variables > in most cases. Only two fields added to main struct > 2. Removed resize event send in xenfb_resume(). There is > a general race condition where the vnc view will be left > at 600x400 if attached to too early. (Not resize related)Could you elaborate on this race?> 3. Removed refresh call in xenfb_resize_screen(), back end > invalidates buffer in resize event handler now > 4. xenkbd gets width and height for abs input in Connected > > After I get some feed back, will wait a day or two, I will > prepare a proper patch set. > > PatI like this much better. A couple of questions remain; see comments inline.> diff -r 854b0704962b tools/ioemu/hw/xenfb.c > --- a/tools/ioemu/hw/xenfb.c Wed Mar 05 09:43:03 2008 +0000 > +++ b/tools/ioemu/hw/xenfb.c Fri Mar 07 13:44:09 2008 -0600 > @@ -516,6 +516,15 @@ static void xenfb_on_fb_event(struct xen > } > xenfb_guest_copy(xenfb, x, y, w, h); > break; > + case XENFB_TYPE_RESIZE: > + xenfb->width = event->resize.width; > + xenfb->height = event->resize.height; > + xenfb->row_stride = event->resize.stride; > + /* Disable video buf sharing, not compatable with resizing */Spelling of compatible.> + dpy_colourdepth(xenfb->ds, 0); > + dpy_resize(xenfb->ds, xenfb->width, xenfb->height); > + xenfb_invalidate(xenfb); > + break; > } > } > xen_mb(); /* ensure we''re done with ring contents */ > @@ -680,6 +689,7 @@ static int xenfb_read_frontend_fb_config > static int xenfb_read_frontend_fb_config(struct xenfb *xenfb) { > struct xenfb_page *fb_page; > int val; > + int videoram; > > if (xenfb_xs_scanf1(xenfb->xsh, xenfb->fb.otherend, "feature-update", > "%d", &val) < 0) > @@ -702,10 +712,30 @@ static int xenfb_read_frontend_fb_config > /* TODO check for consistency with the above */ > xenfb->fb_len = fb_page->mem_length; > xenfb->row_stride = fb_page->line_length; > + > + /* Protect against hostile frontend, limit fb_len to max allowed */ > + if (xenfb_xs_scanf1(xenfb->xsh, xenfb->fb.nodename, "videoram", "%d", > + &videoram) < 0) > + videoram = 0; > + videoram = videoram * 1024 * 1024; > + if (videoram && xenfb->fb_len > videoram) { > + fprintf(stderr, "Framebuffer requested length of %d exceded allowed %d\n",Spelling of exceeded.> + xenfb->fb_len, videoram); > + xenfb->fb_len = videoram; > + if (xenfb->row_stride * xenfb->height > xenfb->fb_len) > + xenfb->height = xenfb->fb_len / xenfb->row_stride; > + }This isn''t quite watertight, but we can fix that later.> fprintf(stderr, "Framebuffer depth %d width %d height %d line %d\n", > fb_page->depth, fb_page->width, fb_page->height, fb_page->line_length); > if (xenfb_map_fb(xenfb, xenfb->fb.otherend_id) < 0) > return -1; > + > + /* Indicate we have the frame buffer resize feature */ > + xenfb_xs_printf(xenfb->xsh, xenfb->fb.nodename, "feature-resize", "1"); > + > + /* Tell kbd pointer the screen geometry */ > + xenfb_xs_printf(xenfb->xsh, xenfb->kbd.nodename, "width", "%d", xenfb->width); > + xenfb_xs_printf(xenfb->xsh, xenfb->kbd.nodename, "height", "%d", xenfb->height); > > if (xenfb_switch_state(&xenfb->fb, XenbusStateConnected)) > return -1;Context not shown here, but useful below to convince ourselves this is correct: if (xenfb_switch_state(&xenfb->kbd, XenbusStateConnected)) return -1;> diff -r 854b0704962b tools/python/xen/xend/server/vfbif.py > --- a/tools/python/xen/xend/server/vfbif.py Wed Mar 05 09:43:03 2008 +0000 > +++ b/tools/python/xen/xend/server/vfbif.py Thu Mar 06 11:12:17 2008 -0600 > @@ -6,7 +6,7 @@ import os > import os > > CONFIG_ENTRIES = [''type'', ''vncdisplay'', ''vnclisten'', ''vncpasswd'', ''vncunused'', > - ''display'', ''xauthority'', ''keymap'', > + ''videoram'', ''display'', ''xauthority'', ''keymap'', > ''uuid'', ''location'', ''protocol''] > > class VfbifController(DevController): > diff -r 854b0704962b tools/python/xen/xm/create.py > --- a/tools/python/xen/xm/create.py Wed Mar 05 09:43:03 2008 +0000 > +++ b/tools/python/xen/xm/create.py Fri Mar 07 13:12:52 2008 -0600 > @@ -500,6 +500,11 @@ gopts.var(''vncunused'', val='''', > use="""Try to find an unused port for the VNC server. > Only valid when vnc=1.""") > > +gopts.var(''videoram'', val='''', > + fn=set_value, default=None, > + use="""Maximum amount of videoram PV guest can allocate > + for frame buffer.""") > + > gopts.var(''sdl'', val='''', > fn=set_value, default=None, > use="""Should the device model use SDL?""") > @@ -641,7 +646,7 @@ def configure_vfbs(config_devs, vals): > d[''type''] = ''sdl'' > for (k,v) in d.iteritems(): > if not k in [ ''vnclisten'', ''vncunused'', ''vncdisplay'', ''display'', > - ''xauthority'', ''type'', ''vncpasswd'' ]: > + ''videoram'', ''xauthority'', ''type'', ''vncpasswd'' ]: > err("configuration option %s unknown to vfbs" % k) > config.append([k,v]) > if not d.has_key("keymap"): > diff -r 854b0704962b xen/include/public/io/fbif.h > --- a/xen/include/public/io/fbif.h Wed Mar 05 09:43:03 2008 +0000 > +++ b/xen/include/public/io/fbif.h Thu Mar 06 11:12:17 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. > */You still need this wart because you still set the default resolution in xenkbd_probe(). You later reset it to the real maximum resolution in xenkbd_backend_changed(), after frontend went to state Connected. I wonder whether the first one is necessary.> #ifdef __KERNEL__ > #define XENFB_WIDTH 800 > diff -r 1cf7ba68d855 drivers/xen/fbfront/xenfb.c > --- a/drivers/xen/fbfront/xenfb.c Mon Mar 03 13:36:57 2008 +0000 > +++ b/drivers/xen/fbfront/xenfb.c Fri Mar 07 21:21:44 2008 -0600 > @@ -62,6 +62,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 */ > + int resize_dpy; /* XENFB_TYPE_RESIZE needed */ > > struct xenbus_device *xbdev; > }; > @@ -129,20 +131,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 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); > + > +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 +174,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->fb_info->var.xres; > + event.resize.height = info->fb_info->var.yres; > + event.resize.stride = info->fb_info->fix.line_length; > + event.resize.depth = info->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) > @@ -209,11 +241,23 @@ 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;I think you can info->dirty = 0 here. Saves an update event.> + xenfb_do_resize(info); > +} > + > static int xenfb_thread(void *data) > { > struct xenfb_info *info = data; > > while (!kthread_should_stop()) { > + if (info->resize_dpy) { > + xenfb_resize_screen(info); > + } > if (info->dirty) { > info->dirty = 0; > xenfb_update_screen(info); > @@ -413,6 +457,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; > + 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) {Avoidable long lines due to excessive indentation.> + return 0; > + } > + 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) &&Ditto.> + 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; > + > + xenfb_info = info->par; > + > + info->var.xres_virtual = info->var.xres; > + info->var.yres_virtual = info->var.yres; > + xenfb_info->resize_dpy = 1; > + return 0; > +}How is this synchronized with xenfb_do_resize()? If that runs on another processor, it could see the new value of resize_dpy, and old values of var.xres and var.yres.> + > static struct fb_ops xenfb_fb_ops = { > .owner = THIS_MODULE, > .fb_setcolreg = xenfb_setcolreg, > @@ -420,6 +503,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 +535,8 @@ static int __devinit xenfb_probe(struct > { > struct xenfb_info *info; > struct fb_info *fb_info; > + int fb_size; > + int val = 0; > int ret; > > info = kzalloc(sizeof(*info), GFP_KERNEL); > @@ -457,6 +544,21 @@ 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; > @@ -469,12 +571,12 @@ static int __devinit xenfb_probe(struct > 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 +591,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);You move xenfb_init_shared_page() down because you initialize it from fb_info. Okay.> > fb_info = framebuffer_alloc(sizeof(u32) * 256, NULL); > /* see fishy hackery below */ > @@ -504,9 +604,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 +618,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; > @@ -534,14 +634,18 @@ static int __devinit xenfb_probe(struct > goto error; > } > > + /* init shared page, uses fb_info attributes */ > + info->fb_info = fb_info; > + xenfb_init_shared_page(info); > + > ret = register_framebuffer(fb_info); > if (ret) { > fb_dealloc_cmap(&info->fb_info->cmap); > framebuffer_release(fb_info); > + info->fb_info = NULL; > xenbus_dev_fatal(dev, ret, "register_framebuffer"); > goto error; > } > - info->fb_info = fb_info;You set info->fb_info earlier because you need it in xenfb_init_shared_page(). But it''s not yet ready for xenfb_remove() then, and that''s why you need to zap it in the error path. I don''t like this, it''s too complex. What about passing it to xenfb_init_shared_page() as argument instead?> > /* FIXME should this be delayed until backend XenbusStateConnected? */ > info->kthread = kthread_run(xenfb_thread, info, "xenfb thread"); > @@ -600,6 +704,7 @@ static void xenfb_init_shared_page(struc > static void xenfb_init_shared_page(struct xenfb_info *info) > { > int i; > + int epd = PAGE_SIZE/sizeof(info->mfns[0]);Suggest space around /> > for (i = 0; i < info->nr_pages; i++) > info->pages[i] = vmalloc_to_page(info->fb + i * PAGE_SIZE); > @@ -607,13 +712,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 = info->fb_info->var.xres; > + info->page->height = info->fb_info->var.yres; > + info->page->depth = info->fb_info->var.bits_per_pixel; > + info->page->line_length = info->fb_info->fix.line_length; > + info->page->mem_length = info->fb_info->fix.smem_len; > info->page->in_cons = info->page->in_prod = 0; > info->page->out_cons = info->page->out_prod = 0; > } > @@ -710,6 +816,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 1cf7ba68d855 drivers/xen/fbfront/xenkbd.c > --- a/drivers/xen/fbfront/xenkbd.c Mon Mar 03 13:36:57 2008 +0000 > +++ b/drivers/xen/fbfront/xenkbd.c Fri Mar 07 16:57:34 2008 -0600 > @@ -295,6 +295,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; > > case XenbusStateClosing:The combined fb/kbd backend writes width and height right before entering state Connected for both devices. Therefore, we can read it here in the kbd frontend after observing the kbd backend transitioning to Conntected. Okay. But what about the race work-around? If the backend goes through InitWait to Connected fast enough, we don''t see the InitWait, and we work around the race with the goto InitWait. But are we guaranteed to get called a second time for the transition to Connected? If not, width and height are never read. Why don''t we have to set the parameters on dynamic resolution change?> diff -r 1cf7ba68d855 include/xen/interface/io/fbif.h > --- a/include/xen/interface/io/fbif.h Mon Mar 03 13:36:57 2008 +0000 > +++ b/include/xen/interface/io/fbif.h Fri Mar 07 13:46:35 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
Pat Campbell
2008-Mar-13 13:05 UTC
Re: [Xen-devel][RFC] Dynamic modes support for PV xenfb (included)
Markus Armbruster wrote:> Pat Campbell <plc@novell.com> writes: > > >> Patches are against xen-unstable tip >> >> What changed since last comment round. (No major structural >> surprises this time:-). >> >> Backend: >> 1. In the resize event handler >> Disabled shared memory (Need to fix) >> Invalidate the buffer >> 2. Added videoram config variable to limit hostile front >> end frame buffer memory size. >> 3. Sets feature-resize and width/height in xenstore >> before frontend connected. width/height are for xenkbd >> abs input config values >> >> Frontend: >> 1. Variable usage cleanup. Now using the fb variables >> in most cases. Only two fields added to main struct >> 2. Removed resize event send in xenfb_resume(). There is >> a general race condition where the vnc view will be left >> at 600x400 if attached to too early. (Not resize related) >> > > Could you elaborate on this race? > >From the command line I suspend the guest >From a perl script I:Resume the guest, xm resume <guest-name> Loop checking for vnc port in xenstore, when found immediately attach. 9 times out of 10 times the vncviewer will be at 600,400 instead of the default 800x600 This happens in Xen 3.2 without any of my code changes and before the recent shared_buf changes. I intend to investigate this further.> >> 3. Removed refresh call in xenfb_resize_screen(), back end >> invalidates buffer in resize event handler now >> 4. xenkbd gets width and height for abs input in Connected >> >> After I get some feed back, will wait a day or two, I will >> prepare a proper patch set. >> >> Pat >> > > I like this much better. A couple of questions remain; see comments > inline. > > >> diff -r 854b0704962b tools/ioemu/hw/xenfb.c >> --- a/tools/ioemu/hw/xenfb.c Wed Mar 05 09:43:03 2008 +0000 >> +++ b/tools/ioemu/hw/xenfb.c Fri Mar 07 13:44:09 2008 -0600 >> @@ -516,6 +516,15 @@ static void xenfb_on_fb_event(struct xen >> } >> xenfb_guest_copy(xenfb, x, y, w, h); >> break; >> + case XENFB_TYPE_RESIZE: >> + xenfb->width = event->resize.width; >> + xenfb->height = event->resize.height; >> + xenfb->row_stride = event->resize.stride; >> + /* Disable video buf sharing, not compatable with resizing */ >> > > Spelling of compatible. >Replaced with code suggestion from Samuel.> >> + dpy_colourdepth(xenfb->ds, 0); >> + dpy_resize(xenfb->ds, xenfb->width, xenfb->height); >> + xenfb_invalidate(xenfb); >> + break; >> } >> } >> xen_mb(); /* ensure we''re done with ring contents */ >> @@ -680,6 +689,7 @@ static int xenfb_read_frontend_fb_config >> static int xenfb_read_frontend_fb_config(struct xenfb *xenfb) { >> struct xenfb_page *fb_page; >> int val; >> + int videoram; >> >> if (xenfb_xs_scanf1(xenfb->xsh, xenfb->fb.otherend, "feature-update", >> "%d", &val) < 0) >> @@ -702,10 +712,30 @@ static int xenfb_read_frontend_fb_config >> /* TODO check for consistency with the above */ >> xenfb->fb_len = fb_page->mem_length; >> xenfb->row_stride = fb_page->line_length; >> + >> + /* Protect against hostile frontend, limit fb_len to max allowed */ >> + if (xenfb_xs_scanf1(xenfb->xsh, xenfb->fb.nodename, "videoram", "%d", >> + &videoram) < 0) >> + videoram = 0; >> + videoram = videoram * 1024 * 1024; >> + if (videoram && xenfb->fb_len > videoram) { >> + fprintf(stderr, "Framebuffer requested length of %d exceded allowed %d\n", >> > > Spelling of exceeded. > >Fixed that. Spelling was never my strong suite.>> + xenfb->fb_len, videoram); >> + xenfb->fb_len = videoram; >> + if (xenfb->row_stride * xenfb->height > xenfb->fb_len) >> + xenfb->height = xenfb->fb_len / xenfb->row_stride; >> + } >> > > This isn''t quite watertight, but we can fix that later. > > >> fprintf(stderr, "Framebuffer depth %d width %d height %d line %d\n", >> fb_page->depth, fb_page->width, fb_page->height, fb_page->line_length); >> if (xenfb_map_fb(xenfb, xenfb->fb.otherend_id) < 0) >> return -1; >> + >> + /* Indicate we have the frame buffer resize feature */ >> + xenfb_xs_printf(xenfb->xsh, xenfb->fb.nodename, "feature-resize", "1"); >> + >> + /* Tell kbd pointer the screen geometry */ >> + xenfb_xs_printf(xenfb->xsh, xenfb->kbd.nodename, "width", "%d", xenfb->width); >> + xenfb_xs_printf(xenfb->xsh, xenfb->kbd.nodename, "height", "%d", xenfb->height); >> >> if (xenfb_switch_state(&xenfb->fb, XenbusStateConnected)) >> return -1; >> > > Context not shown here, but useful below to convince ourselves this is > correct: > > if (xenfb_switch_state(&xenfb->kbd, XenbusStateConnected)) > return -1; > > >> diff -r 854b0704962b tools/python/xen/xend/server/vfbif.py >> --- a/tools/python/xen/xend/server/vfbif.py Wed Mar 05 09:43:03 2008 +0000 >> +++ b/tools/python/xen/xend/server/vfbif.py Thu Mar 06 11:12:17 2008 -0600 >> @@ -6,7 +6,7 @@ import os >> import os >> >> CONFIG_ENTRIES = [''type'', ''vncdisplay'', ''vnclisten'', ''vncpasswd'', ''vncunused'', >> - ''display'', ''xauthority'', ''keymap'', >> + ''videoram'', ''display'', ''xauthority'', ''keymap'', >> ''uuid'', ''location'', ''protocol''] >> >> class VfbifController(DevController): >> diff -r 854b0704962b tools/python/xen/xm/create.py >> --- a/tools/python/xen/xm/create.py Wed Mar 05 09:43:03 2008 +0000 >> +++ b/tools/python/xen/xm/create.py Fri Mar 07 13:12:52 2008 -0600 >> @@ -500,6 +500,11 @@ gopts.var(''vncunused'', val='''', >> use="""Try to find an unused port for the VNC server. >> Only valid when vnc=1.""") >> >> +gopts.var(''videoram'', val='''', >> + fn=set_value, default=None, >> + use="""Maximum amount of videoram PV guest can allocate >> + for frame buffer.""") >> + >> gopts.var(''sdl'', val='''', >> fn=set_value, default=None, >> use="""Should the device model use SDL?""") >> @@ -641,7 +646,7 @@ def configure_vfbs(config_devs, vals): >> d[''type''] = ''sdl'' >> for (k,v) in d.iteritems(): >> if not k in [ ''vnclisten'', ''vncunused'', ''vncdisplay'', ''display'', >> - ''xauthority'', ''type'', ''vncpasswd'' ]: >> + ''videoram'', ''xauthority'', ''type'', ''vncpasswd'' ]: >> err("configuration option %s unknown to vfbs" % k) >> config.append([k,v]) >> if not d.has_key("keymap"): >> diff -r 854b0704962b xen/include/public/io/fbif.h >> --- a/xen/include/public/io/fbif.h Wed Mar 05 09:43:03 2008 +0000 >> +++ b/xen/include/public/io/fbif.h Thu Mar 06 11:12:17 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. >> */ >> > > You still need this wart because you still set the default resolution > in xenkbd_probe(). You later reset it to the real maximum resolution > in xenkbd_backend_changed(), after frontend went to state Connected. > I wonder whether the first one is necessary. > >Yes I think so. Handles the case of the new VM running against an older vnc-xenfb or qemu that does not send a new value.>> #ifdef __KERNEL__ >> #define XENFB_WIDTH 800 >> diff -r 1cf7ba68d855 drivers/xen/fbfront/xenfb.c >> --- a/drivers/xen/fbfront/xenfb.c Mon Mar 03 13:36:57 2008 +0000 >> +++ b/drivers/xen/fbfront/xenfb.c Fri Mar 07 21:21:44 2008 -0600 >> @@ -62,6 +62,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 */ >> + int resize_dpy; /* XENFB_TYPE_RESIZE needed */ >> >> struct xenbus_device *xbdev; >> }; >> @@ -129,20 +131,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 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); >> + >> +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 +174,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->fb_info->var.xres; >> + event.resize.height = info->fb_info->var.yres; >> + event.resize.stride = info->fb_info->fix.line_length; >> + event.resize.depth = info->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) >> @@ -209,11 +241,23 @@ 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; >> > > I think you can info->dirty = 0 here. Saves an update event. >Added that and then took it back out. Somehow that caused the GUI to have a terrible delay, minutes, when starting up before you could enter your user name password. Changing screens work fast enough. Will have to investigate this later.> >> + xenfb_do_resize(info); >> +} >> + >> static int xenfb_thread(void *data) >> { >> struct xenfb_info *info = data; >> >> while (!kthread_should_stop()) { >> + if (info->resize_dpy) { >> + xenfb_resize_screen(info); >> + } >> if (info->dirty) { >> info->dirty = 0; >> xenfb_update_screen(info); >> @@ -413,6 +457,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; >> + 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) { >> > > Avoidable long lines due to excessive indentation. >Ok, vim auto indent is my defense.> >> + return 0; >> + } >> + 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) && >> > > Ditto. >Ok> >> + 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; >> + >> + xenfb_info = info->par; >> + >> + info->var.xres_virtual = info->var.xres; >> + info->var.yres_virtual = info->var.yres; >> + xenfb_info->resize_dpy = 1; >> + return 0; >> +} >> > > How is this synchronized with xenfb_do_resize()? > > If that runs on another processor, it could see the new value of > resize_dpy, and old values of var.xres and var.yres. > >By the time xenfb_set_par() is called the values are already in the fb struct. They are set sometime between the successful xenfb_check_var() and xenfb_set_par() calls. I can see a possible condition where if we are doing back to back resizes utilizing multiple procs we could get a new xres and an old yres. I will add into xenfb_check_var() the following test: if ( xenfb_info->resize_dpy) return -EINVAL; and move the resize_dpy clear to below the xenfb_do_resize() call>> + >> static struct fb_ops xenfb_fb_ops = { >> .owner = THIS_MODULE, >> .fb_setcolreg = xenfb_setcolreg, >> @@ -420,6 +503,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 +535,8 @@ static int __devinit xenfb_probe(struct >> { >> struct xenfb_info *info; >> struct fb_info *fb_info; >> + int fb_size; >> + int val = 0; >> int ret; >> >> info = kzalloc(sizeof(*info), GFP_KERNEL); >> @@ -457,6 +544,21 @@ 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; >> @@ -469,12 +571,12 @@ static int __devinit xenfb_probe(struct >> 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 +591,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); >> > > You move xenfb_init_shared_page() down because you initialize it from > fb_info. Okay. > > >> >> fb_info = framebuffer_alloc(sizeof(u32) * 256, NULL); >> /* see fishy hackery below */ >> @@ -504,9 +604,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 +618,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; >> @@ -534,14 +634,18 @@ static int __devinit xenfb_probe(struct >> goto error; >> } >> >> + /* init shared page, uses fb_info attributes */ >> + info->fb_info = fb_info; >> + xenfb_init_shared_page(info); >> + >> ret = register_framebuffer(fb_info); >> if (ret) { >> fb_dealloc_cmap(&info->fb_info->cmap); >> framebuffer_release(fb_info); >> + info->fb_info = NULL; >> xenbus_dev_fatal(dev, ret, "register_framebuffer"); >> goto error; >> } >> - info->fb_info = fb_info; >> > > > You set info->fb_info earlier because you need it in > xenfb_init_shared_page(). But it''s not yet ready for xenfb_remove() > then, and that''s why you need to zap it in the error path. I don''t > like this, it''s too complex. What about passing it to > xenfb_init_shared_page() as argument instead? >Ok, changed that.> >> >> /* FIXME should this be delayed until backend XenbusStateConnected? */ >> info->kthread = kthread_run(xenfb_thread, info, "xenfb thread"); >> @@ -600,6 +704,7 @@ static void xenfb_init_shared_page(struc >> static void xenfb_init_shared_page(struct xenfb_info *info) >> { >> int i; >> + int epd = PAGE_SIZE/sizeof(info->mfns[0]); >> > > Suggest space around / >Ok>> >> for (i = 0; i < info->nr_pages; i++) >> info->pages[i] = vmalloc_to_page(info->fb + i * PAGE_SIZE); >> @@ -607,13 +712,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 = info->fb_info->var.xres; >> + info->page->height = info->fb_info->var.yres; >> + info->page->depth = info->fb_info->var.bits_per_pixel; >> + info->page->line_length = info->fb_info->fix.line_length; >> + info->page->mem_length = info->fb_info->fix.smem_len; >> info->page->in_cons = info->page->in_prod = 0; >> info->page->out_cons = info->page->out_prod = 0; >> } >> @@ -710,6 +816,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 1cf7ba68d855 drivers/xen/fbfront/xenkbd.c >> --- a/drivers/xen/fbfront/xenkbd.c Mon Mar 03 13:36:57 2008 +0000 >> +++ b/drivers/xen/fbfront/xenkbd.c Fri Mar 07 16:57:34 2008 -0600 >> @@ -295,6 +295,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; >> >> case XenbusStateClosing: >> > > The combined fb/kbd backend writes width and height right before > entering state Connected for both devices. Therefore, we can read it > here in the kbd frontend after observing the kbd backend transitioning > to Conntected. Okay. > > But what about the race work-around? If the backend goes through > InitWait to Connected fast enough, we don''t see the InitWait, and we > work around the race with the goto InitWait. But are we guaranteed to > get called a second time for the transition to Connected? If not, > width and height are never read. >I debated this. Should those parameters be read before the work around? Qemu xenfb did reordered things, is the work around still necessary? Should I work around the work around? I never saw that case during my testing, not to say it still does not happen. I would like to see if this is still an issue in a larger audience before changing.> Why don''t we have to set the parameters on dynamic resolution change? >I debated this. Should those parameters be read before the work around? Qemu xenfb did reorder things, is the work around still necessary? Should I work around the work around? I never saw that case during my testing, not to say it still does not happen. I would like to see if this is still an issue in a larger audience before changing.> >> diff -r 1cf7ba68d855 include/xen/interface/io/fbif.h >> --- a/include/xen/interface/io/fbif.h Mon Mar 03 13:36:57 2008 +0000 >> +++ b/include/xen/interface/io/fbif.h Fri Mar 07 13:46:35 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 >>I will send out a real patch incorporating your suggestions later today. Thanks Pat _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Pat Campbell
2008-Mar-13 19:53 UTC
Re: [Xen-devel][RFC] Dynamic modes support for PV xenfb (included)
Hi, Stripped out a bunch of stuff Markus Armbruster wrote: How is this synchronized with xenfb_do_resize()?> > If that runs on another processor, it could see the new value of > resize_dpy, and old values of var.xres and var.yres. > >My suggested fix did not work. My current solution, included in the upcoming patch post was to remove function xenfb_resize_screen() and restored xenfb_thread() to it''s original state and put a lock around the event queue. Resize event is now directly sent from xenfb_set_par(). You might want to have a look at that code. Pat _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Markus Armbruster
2008-Mar-14 16:39 UTC
Re: [Xen-devel][RFC] Dynamic modes support for PV xenfb (included)
Pat Campbell <plc@novell.com> writes:> Markus Armbruster wrote: >> Pat Campbell <plc@novell.com> writes: >> >> >>> Patches are against xen-unstable tip >>> >>> What changed since last comment round. (No major structural >>> surprises this time:-). >>> >>> Backend: >>> 1. In the resize event handler >>> Disabled shared memory (Need to fix) >>> Invalidate the buffer >>> 2. Added videoram config variable to limit hostile front >>> end frame buffer memory size. >>> 3. Sets feature-resize and width/height in xenstore >>> before frontend connected. width/height are for xenkbd >>> abs input config values >>> >>> Frontend: >>> 1. Variable usage cleanup. Now using the fb variables >>> in most cases. Only two fields added to main struct >>> 2. Removed resize event send in xenfb_resume(). There is >>> a general race condition where the vnc view will be left >>> at 600x400 if attached to too early. (Not resize related) >>> >> >> Could you elaborate on this race? >> >>From the command line I suspend the guest >>From a perl script I: > Resume the guest, xm resume <guest-name> > Loop checking for vnc port in xenstore, when found > immediately attach. 9 times out of 10 times the vncviewer will > be at 600,400 instead of the default 800x600 > > This happens in Xen 3.2 without any of my code changes and before the > recent shared_buf changes. I intend to investigate this further.Interesting. Please keep us posted.>> >>> 3. Removed refresh call in xenfb_resize_screen(), back end >>> invalidates buffer in resize event handler now >>> 4. xenkbd gets width and height for abs input in Connected >>> >>> After I get some feed back, will wait a day or two, I will >>> prepare a proper patch set. >>> >>> Pat >>> >> >> I like this much better. A couple of questions remain; see comments >> inline. >> >> >>> diff -r 854b0704962b tools/ioemu/hw/xenfb.c[...]>>> diff -r 854b0704962b xen/include/public/io/fbif.h >>> --- a/xen/include/public/io/fbif.h Wed Mar 05 09:43:03 2008 +0000 >>> +++ b/xen/include/public/io/fbif.h Thu Mar 06 11:12:17 2008 -0600[...]>>> /* >>> - * 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. >>> */ >>> >> >> You still need this wart because you still set the default resolution >> in xenkbd_probe(). You later reset it to the real maximum resolution >> in xenkbd_backend_changed(), after frontend went to state Connected. >> I wonder whether the first one is necessary. >> >> > Yes I think so. Handles the case of the new VM running against an older > vnc-xenfb or qemu that does not send a new value.You''re right.>>> #ifdef __KERNEL__ >>> #define XENFB_WIDTH 800 >>> diff -r 1cf7ba68d855 drivers/xen/fbfront/xenfb.c >>> --- a/drivers/xen/fbfront/xenfb.c Mon Mar 03 13:36:57 2008 +0000 >>> +++ b/drivers/xen/fbfront/xenfb.c Fri Mar 07 21:21:44 2008 -0600[...]>>> @@ -209,11 +241,23 @@ 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; >>> >> >> I think you can info->dirty = 0 here. Saves an update event. >> > Added that and then took it back out. Somehow that caused the GUI to > have a terrible delay, minutes, when starting up before you could enter > your user name password. Changing screens work fast enough. Will have > to investigate this later.Weird. It would be good to know what exactly goes wrong there.>>> + xenfb_do_resize(info); >>> +} >>> +[...]>>> +static int xenfb_set_par(struct fb_info *info) >>> +{ >>> + struct xenfb_info *xenfb_info; >>> + >>> + xenfb_info = info->par; >>> + >>> + info->var.xres_virtual = info->var.xres; >>> + info->var.yres_virtual = info->var.yres; >>> + xenfb_info->resize_dpy = 1; >>> + return 0; >>> +} >>> >> >> How is this synchronized with xenfb_do_resize()? >> >> If that runs on another processor, it could see the new value of >> resize_dpy, and old values of var.xres and var.yres. >> >> > By the time xenfb_set_par() is called the values are already in the fb > struct. They are set sometime between the successful xenfb_check_var() > and xenfb_set_par() calls. I can see a possible condition where if we > are doing back to back resizes utilizing multiple procs we could get a > new xres and an old yres. > > I will add into xenfb_check_var() the following test: > if ( xenfb_info->resize_dpy) > return -EINVAL; > and move the resize_dpy clear to below the xenfb_do_resize() callFurther discussed in another message, will reply there. [...]>>> diff -r 1cf7ba68d855 drivers/xen/fbfront/xenkbd.c >>> --- a/drivers/xen/fbfront/xenkbd.c Mon Mar 03 13:36:57 2008 +0000 >>> +++ b/drivers/xen/fbfront/xenkbd.c Fri Mar 07 16:57:34 2008 -0600 >>> @@ -295,6 +295,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; >>> >>> case XenbusStateClosing: >>> >> >> The combined fb/kbd backend writes width and height right before >> entering state Connected for both devices. Therefore, we can read it >> here in the kbd frontend after observing the kbd backend transitioning >> to Conntected. Okay. >> >> But what about the race work-around? If the backend goes through >> InitWait to Connected fast enough, we don''t see the InitWait, and we >> work around the race with the goto InitWait. But are we guaranteed to >> get called a second time for the transition to Connected? If not, >> width and height are never read. >> > I debated this. Should those parameters be read before the work > around? Qemu xenfb did reordered things, is the work around still > necessary? Should I work around the work around? I never saw that > case during my testing, not to say it still does not happen. I would > like to see if this is still an issue in a larger audience before changing.The work-around is necessary if the backend can go through InitWait to Connected without synchronizing with the frontend. It actually synchronizes by waiting for the frontend changing state to Initialised. I figure the work-around can be removed.>> Why don''t we have to set the parameters on dynamic resolution change? >> > I debated this. Should those parameters be read before the work > around? Qemu xenfb did reorder things, is the work around still > necessary? Should I work around the work around? I never saw that > case during my testing, not to say it still does not happen. I would > like to see if this is still an issue in a larger audience before changing.Pardon? [...]> I will send out a real patch incorporating your suggestions later today. > > Thanks > > PatGot that, working on it. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel