|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC][PATCH v1] xen-fbfront: replace deferred io with buffer queue
On Mon, 19 Jan 2015, Sergiy Kibrik wrote: > Use N-buffering instead of old deferred I/O, which is not suitable for high > frame rates. This includes new event type -- xenfb_in_released, > to track buffers not being used by backend. > Also use grants for fb pages, as they allow backend to map them > to video devices. Please make the grant change separately in another patch. > Signed-off-by: Sergiy Kibrik <sergiy.kibrik@xxxxxxxxxxxxxxx> > --- > > Hi, > Here I'd like to present changes to xen-fbfront driver as example of > how it can possibly be modified to support high frame rates. > With this changes plus modified xenfb backend I was able to achieve 60 FPS on > ARM based DRA7xx SoC, but this is rather display limitation, not driver > itself. > > The idea is to send full resolution shared buffers to backend -- modern UI and > multimedia applications cause heavy screen redraw. For this purpose driver > allocates N-times bigger buffer than actual framebuffer resolution, each chunk > assigned ID (0-N), which is carried within event message to backend. > As soon as buffers displayed & released on host side, backend sends back > release > event with ID of released chunk, which can be used as framebuffer again. Isn't it going to use a lot of memory? Could you write down some example configurations and relative memory consumption? > For my setup I used modified Xen framebuffer backend [1], DRA7xx SoC and > OMAP Display Subsystem for video output (omap_vout V4L2 driver). > > [1] http://www-archive.xenproject.org/files/summit_3/Xenpvfb.pdf > > drivers/video/fbdev/xen-fbfront.c | 344 > ++++++++++++++++++++++++++----------- > include/xen/interface/io/fbif.h | 9 +- > 2 files changed, 250 insertions(+), 103 deletions(-) > > diff --git a/drivers/video/fbdev/xen-fbfront.c > b/drivers/video/fbdev/xen-fbfront.c > index 09dc447..a3e40ac 100644 > --- a/drivers/video/fbdev/xen-fbfront.c > +++ b/drivers/video/fbdev/xen-fbfront.c > @@ -11,13 +11,7 @@ > * more details. > */ > > -/* > - * TODO: > - * > - * Switch to grant tables when they become capable of dealing with the > - * frame buffer. > - */ > - > +/*#define DEBUG*/ spurious change? > #include <linux/console.h> > #include <linux/kernel.h> > #include <linux/errno.h> > @@ -32,20 +26,34 @@ > #include <xen/xen.h> > #include <xen/events.h> > #include <xen/page.h> > +#include <xen/grant_table.h> > #include <xen/interface/io/fbif.h> > #include <xen/interface/io/protocols.h> > #include <xen/xenbus.h> > #include <xen/platform_pci.h> > > +enum xenfb_buf_state { > + XENFB_BUF_RELEASED = 0, > + XENFB_BUF_ACTIVE, > + XENFB_BUF_NEXT, > +}; Could you please add a comment to explain what are these states for? Maybe you could also explain exactly how the new protocol works. > struct xenfb_info { > unsigned char *fb; > + int fb_size; > struct fb_info *fb_info; > int x1, y1, x2, y2; /* dirty rectangle, > protected by dirty_lock */ > - spinlock_t dirty_lock; > + > + spinlock_t b_lock; Why are you renaming the spinlock? If it is really necessary, please do it in a separate patch. Mixing all the changes together makes the patch difficult to read. > + enum xenfb_buf_state *buffers; > + int nr_buffers; > int nr_pages; > + struct completion completion; > + > int irq; > struct xenfb_page *page; > + int ring_ref; > unsigned long *mfns; > int update_wanted; /* XENFB_TYPE_UPDATE wanted */ > int feature_resize; /* XENFB_TYPE_RESIZE ok */ > @@ -70,6 +78,43 @@ 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 *rvmalloc(unsigned long size) > +{ > + void *mem; > + unsigned long adr; > + > + size = PAGE_ALIGN(size); > + mem = vmalloc_32(size); Is vmalloc_32 really needed? Could vmalloc be used? > + if (!mem) > + return NULL; > + > + memset(mem, 0, size); /* Clear the ram out, no junk to the user */ > + adr = (unsigned long) mem; > + while (size > 0) { > + SetPageReserved(vmalloc_to_page((void *)adr)); > + adr += PAGE_SIZE; > + size -= PAGE_SIZE; > + } > + > + return mem; > +} > + > +static void rvfree(void *mem, unsigned long size) > +{ > + unsigned long adr; > + > + if (!mem) > + return; > + > + adr = (unsigned long) mem; > + while ((long) size > 0) { > + ClearPageReserved(vmalloc_to_page((void *)adr)); > + adr += PAGE_SIZE; > + size -= PAGE_SIZE; > + } > + vfree(mem); > +} > + > static void xenfb_send_event(struct xenfb_info *info, > union xenfb_out_event *event) > { > @@ -147,7 +192,7 @@ static void xenfb_refresh(struct xenfb_info *info, > if (!info->update_wanted) > return; > > - spin_lock_irqsave(&info->dirty_lock, flags); > + spin_lock_irqsave(&info->b_lock, flags); > > /* Combine with dirty rectangle: */ > if (info->y1 < y1) > @@ -165,7 +210,7 @@ static void xenfb_refresh(struct xenfb_info *info, > info->x2 = x2; > info->y1 = y1; > info->y2 = y2; > - spin_unlock_irqrestore(&info->dirty_lock, flags); > + spin_unlock_irqrestore(&info->b_lock, flags); > return; > } > > @@ -173,42 +218,12 @@ static void xenfb_refresh(struct xenfb_info *info, > info->x1 = info->y1 = INT_MAX; > info->x2 = info->y2 = 0; > > - spin_unlock_irqrestore(&info->dirty_lock, flags); > + spin_unlock_irqrestore(&info->b_lock, flags); > > if (x1 <= x2 && y1 <= y2) > xenfb_do_update(info, x1, y1, x2 - x1 + 1, y2 - y1 + 1); > } > > -static void xenfb_deferred_io(struct fb_info *fb_info, > - struct list_head *pagelist) > -{ > - struct xenfb_info *info = fb_info->par; > - struct page *page; > - unsigned long beg, end; > - int y1, y2, miny, maxy; > - > - miny = INT_MAX; > - maxy = 0; > - list_for_each_entry(page, pagelist, lru) { > - beg = page->index << PAGE_SHIFT; > - end = beg + PAGE_SIZE - 1; > - y1 = beg / fb_info->fix.line_length; > - y2 = end / fb_info->fix.line_length; > - if (y2 >= fb_info->var.yres) > - y2 = fb_info->var.yres - 1; > - if (miny > y1) > - miny = y1; > - if (maxy < y2) > - maxy = y2; > - } > - xenfb_refresh(info, 0, miny, fb_info->var.xres, maxy - miny + 1); > -} > - > -static struct fb_deferred_io xenfb_defio = { > - .delay = HZ / 20, > - .deferred_io = xenfb_deferred_io, > -}; > - > static int xenfb_setcolreg(unsigned regno, unsigned red, unsigned green, > unsigned blue, unsigned transp, > struct fb_info *info) > @@ -275,26 +290,40 @@ static ssize_t xenfb_write(struct fb_info *p, const > char __user *buf, > return res; > } > > +static int find_released(struct xenfb_info *info) > +{ > + int i; > + for (i = 0; i < info->nr_buffers; i++) > + if (info->buffers[i] == XENFB_BUF_RELEASED) > + return i; > + return -1; > +} > + > static int > xenfb_check_var(struct fb_var_screeninfo *var, struct fb_info *info) > { > struct xenfb_info *xenfb_info; > int required_mem_len; > + int buffer; > + unsigned long flags; > > 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; > - } > + if (var->xres != video[KPARAM_WIDTH] || > + var->yres != video[KPARAM_HEIGHT] || > + var->bits_per_pixel != xenfb_info->page->depth) > return -EINVAL; > - } > > - /* Can't resize past initial width and height */ > - if (var->xres > video[KPARAM_WIDTH] || var->yres > video[KPARAM_HEIGHT]) > - return -EINVAL; > + if (xenfb_queue_full(xenfb_info)) > + return -EBUSY; > + > + spin_lock_irqsave(&xenfb_info->b_lock, flags); > + buffer = var->yoffset / var->yres; > + if (find_released(xenfb_info) == -1) { > + spin_unlock_irqrestore(&xenfb_info->b_lock, flags); > + return -EBUSY; > + } > + spin_unlock_irqrestore(&xenfb_info->b_lock, flags); > > required_mem_len = var->xres * var->yres * xenfb_info->page->depth / 8; > if (var->bits_per_pixel == xenfb_info->page->depth && > @@ -307,25 +336,85 @@ xenfb_check_var(struct fb_var_screeninfo *var, struct > fb_info *info) > return -EINVAL; > } > > -static int xenfb_set_par(struct fb_info *info) > +static int xenfb_set_par(struct fb_info *fbi) > { > - struct xenfb_info *xenfb_info; > + struct xenfb_info *info = fbi->par; > + struct xenbus_device *xbdev = info->xbdev; > + struct fb_var_screeninfo *var = &fbi->var; > + int buffer = var->yoffset / var->yres; > unsigned long flags; > > - xenfb_info = info->par; > + BUG_ON(buffer < 0 || buffer >= info->nr_buffers); > + > + xenfb_do_update(info, var->xoffset, var->yoffset, > + var->xres, var->yres); > + dev_dbg(&xbdev->dev, "sent buf %d\n", buffer); > + > + spin_lock_irqsave(&info->b_lock, flags); > + info->buffers[buffer] = XENFB_BUF_NEXT; > + buffer = find_released(info); > + if (buffer != -1) { > + var->yoffset = var->yres * buffer; > + dev_dbg(&xbdev->dev, "giving out %d\n", buffer); > + spin_unlock_irqrestore(&info->b_lock, flags); > + return 0; > + } > > - spin_lock_irqsave(&xenfb_info->resize_lock, flags); > - xenfb_info->resize.type = XENFB_TYPE_RESIZE; > - 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.offset = 0; > - xenfb_info->resize_dpy = 1; > - spin_unlock_irqrestore(&xenfb_info->resize_lock, flags); > + /* If no buffers available, e.g. in case of double-buffering, > + * we have to wait for backend to release some. If backend is > + * already dead, we return last buffer and hope > + * client knows what to do > + */ > + INIT_COMPLETION(info->completion); > + spin_unlock_irqrestore(&info->b_lock, flags); > + > + if (wait_for_completion_interruptible_timeout(&info->completion, > + msecs_to_jiffies(40)) <= 0) { > + dev_dbg(&xbdev->dev, "no buffer after timeout\n"); > + return -ETIMEDOUT; > + } > + > + spin_lock_irqsave(&info->b_lock, flags); > + buffer = find_released(info); > + spin_unlock_irqrestore(&info->b_lock, flags); > + BUG_ON(buffer == -1); /* wtf is going on? */ > + var->yoffset = var->yres * buffer; > + > + dev_dbg(&xbdev->dev, "given %d after wait\n", buffer); > return 0; > } > > +static int xenfb_mmap(struct fb_info *fbi, > + struct vm_area_struct *vma) > +{ > + struct xenfb_info *info = fbi->par; > + unsigned long start = vma->vm_start; > + unsigned long size = vma->vm_end - vma->vm_start; > + unsigned long offset = vma->vm_pgoff << PAGE_SHIFT; > + unsigned long page, pos; > + > + if (offset + size > fbi->fix.smem_len) > + return -EINVAL; > + > + pos = (unsigned long)info->fb + offset; > + > + while (size > 0) { > + page = vmalloc_to_pfn((void *)pos); > + if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) > + return -EAGAIN; > + > + start += PAGE_SIZE; > + pos += PAGE_SIZE; > + if (size > PAGE_SIZE) > + size -= PAGE_SIZE; > + else > + size = 0; > + } > + > + return 0; > + > +} > + > static struct fb_ops xenfb_fb_ops = { > .owner = THIS_MODULE, > .fb_read = fb_sys_read, > @@ -336,26 +425,36 @@ static struct fb_ops xenfb_fb_ops = { > .fb_imageblit = xenfb_imageblit, > .fb_check_var = xenfb_check_var, > .fb_set_par = xenfb_set_par, > + .fb_mmap = xenfb_mmap, > }; > > static irqreturn_t xenfb_event_handler(int rq, void *dev_id) > { > - /* > - * No in events recognized, simply ignore them all. > - * If you need to recognize some, see xen-kbdfront's > - * input_handler() for how to do that. > - */ > struct xenfb_info *info = dev_id; > struct xenfb_page *page = info->page; > + uint32_t cons, in_prod; > + unsigned long flags; > > - if (page->in_cons != page->in_prod) { > - info->page->in_cons = info->page->in_prod; > - notify_remote_via_irq(info->irq); > + rmb(); > + in_prod = page->in_prod; > + dev_dbg(&info->xbdev->dev, "handle %d in events\n", in_prod); > + > + spin_lock_irqsave(&info->b_lock, flags); > + for (cons = page->in_cons; cons != in_prod; cons++) { > + union xenfb_in_event *event = &XENFB_IN_RING_REF(page, cons); > + if (event->type == XENFB_TYPE_RELEASE) { > + int buffer = event->release.buffer; > + if (buffer < 0 || buffer >= info->nr_buffers) > + continue; > + info->buffers[buffer] = XENFB_BUF_RELEASED; > + dev_dbg(&info->xbdev->dev, "released %d\n", buffer); > + complete(&info->completion); > + } > } > + page->in_cons = cons; > + spin_unlock_irqrestore(&info->b_lock, flags); > > - /* Flush dirty rectangle: */ > - xenfb_refresh(info, INT_MAX, INT_MAX, -INT_MAX, -INT_MAX); > - > + mb(); > return IRQ_HANDLED; > } > > @@ -364,8 +463,6 @@ static int xenfb_probe(struct xenbus_device *dev, > { > struct xenfb_info *info; > struct fb_info *fb_info; > - int fb_size; > - int val; > int ret = 0; > > info = kzalloc(sizeof(*info), GFP_KERNEL); > @@ -375,42 +472,50 @@ static int xenfb_probe(struct xenbus_device *dev, > } > > /* 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] * 1024 * 1024; > - if (video[KPARAM_WIDTH] * video[KPARAM_HEIGHT] * XENFB_DEPTH / 8 > - > fb_size) { > + if (xenbus_scanf(XBT_NIL, dev->otherend, > + "num_bufs", "%d", &info->nr_buffers) != 1) > + info->nr_buffers = 2; This doesn't sound like a secure interface: a potentially very significant and unbound memory allocation in dom0 is caused by a parameter configured by the guest. It can be trivial for a malicious guest to take down the system. > + if (xenbus_scanf(XBT_NIL, dev->otherend, "size", "%dx%d", > + &video[KPARAM_WIDTH], &video[KPARAM_HEIGHT]) != 2) { > video[KPARAM_WIDTH] = XENFB_WIDTH; > video[KPARAM_HEIGHT] = XENFB_HEIGHT; > - fb_size = XENFB_DEFAULT_FB_LEN; > } > > dev_set_drvdata(&dev->dev, info); > info->xbdev = dev; > info->irq = -1; > info->x1 = info->y1 = INT_MAX; > - spin_lock_init(&info->dirty_lock); > + spin_lock_init(&info->b_lock); > spin_lock_init(&info->resize_lock); > + init_completion(&info->completion); > > - info->fb = vzalloc(fb_size); > + info->fb_size = video[KPARAM_WIDTH] * video[KPARAM_HEIGHT] * > + (XENFB_DEPTH / 8) * info->nr_buffers; > + info->fb = rvmalloc(info->fb_size); > if (info->fb == NULL) > goto error_nomem; > > - info->nr_pages = (fb_size + PAGE_SIZE - 1) >> PAGE_SHIFT; > + info->nr_pages = (info->fb_size + PAGE_SIZE - 1) >> PAGE_SHIFT; > > info->mfns = vmalloc(sizeof(unsigned long) * info->nr_pages); > if (!info->mfns) > goto error_nomem; > + info->buffers = kzalloc(sizeof(info->buffers[0]) * info->nr_buffers, > + GFP_KERNEL); > + if (!info->buffers) > + goto error_nomem; > > /* set up shared page */ > info->page = (void *)__get_free_page(GFP_KERNEL | __GFP_ZERO); > if (!info->page) > goto error_nomem; > > + memset(info->page->pd, -1, sizeof(info->page->pd)); > + > + info->ring_ref = xenbus_grant_ring(dev, virt_to_mfn(info->page)); > + if (info->ring_ref < 0) > + goto error; > + > /* abusing framebuffer_alloc() to allocate pseudo_palette */ > fb_info = framebuffer_alloc(sizeof(u32) * 256, NULL); > if (fb_info == NULL) > @@ -438,11 +543,13 @@ static int xenfb_probe(struct xenbus_device *dev, > > fb_info->fix.visual = FB_VISUAL_TRUECOLOR; > fb_info->fix.line_length = fb_info->var.xres * XENFB_DEPTH / 8; > - fb_info->fix.smem_start = 0; > - fb_info->fix.smem_len = fb_size; > + fb_info->fix.smem_start = > + (unsigned long)page_to_phys(vmalloc_to_page(info->fb)); > + fb_info->fix.smem_len = info->fb_size; > strcpy(fb_info->fix.id, "xen"); > fb_info->fix.type = FB_TYPE_PACKED_PIXELS; > fb_info->fix.accel = FB_ACCEL_NONE; > + fb_info->fix.ypanstep = fb_info->var.yres; > > fb_info->flags = FBINFO_FLAG_DEFAULT | FBINFO_VIRTFB; > > @@ -453,9 +560,6 @@ static int xenfb_probe(struct xenbus_device *dev, > goto error; > } > > - fb_info->fbdefio = &xenfb_defio; > - fb_deferred_io_init(fb_info); > - > xenfb_init_shared_page(info, fb_info); > > ret = xenfb_connect_backend(dev, info); > @@ -521,6 +625,7 @@ static int xenfb_resume(struct xenbus_device *dev) > static int xenfb_remove(struct xenbus_device *dev) > { > struct xenfb_info *info = dev_get_drvdata(&dev->dev); > + int i; > > xenfb_disconnect_backend(info); > if (info->fb_info) { > @@ -529,9 +634,26 @@ static int xenfb_remove(struct xenbus_device *dev) > fb_dealloc_cmap(&info->fb_info->cmap); > framebuffer_release(info->fb_info); > } > + > + for (i = 0; i < sizeof(info->page->pd); i++) > + if (info->page->pd[i] != -1) { > + gnttab_end_foreign_access( > + info->page->pd[i], 0, 0UL); > + info->page->pd[i] = -1; > + } > + > + if (info->ring_ref >= 0) { > + gnttab_end_foreign_access(info->ring_ref, 0, 0UL); > + info->ring_ref = -1; > + } > + > free_page((unsigned long)info->page); > + > + for (i = 0; i < info->nr_pages; i++) > + gnttab_end_foreign_access(info->mfns[i], 0, 0UL); > vfree(info->mfns); > - vfree(info->fb); > + rvfree(info->fb, info->fb_size); > + kfree(info->buffers); > kfree(info); > > return 0; > @@ -545,14 +667,32 @@ static unsigned long vmalloc_to_mfn(void *address) > static void xenfb_init_shared_page(struct xenfb_info *info, > struct fb_info *fb_info) > { > - int i; > + int i, ref; > + grant_ref_t gref_head; > int epd = PAGE_SIZE / sizeof(info->mfns[0]); > > - for (i = 0; i < info->nr_pages; i++) > - info->mfns[i] = vmalloc_to_mfn(info->fb + i * PAGE_SIZE); > + if (gnttab_alloc_grant_references(info->nr_pages + > + DIV_ROUND_UP(info->nr_pages, epd), > + &gref_head)) { > + WARN(1, "failed to allocate %u grants\n", info->nr_pages); > + return; > + } > > - for (i = 0; i * epd < info->nr_pages; i++) > - info->page->pd[i] = vmalloc_to_mfn(&info->mfns[i * epd]); > + for (i = 0; i < info->nr_pages; i++) { > + ref = gnttab_claim_grant_reference(&gref_head); > + BUG_ON(ref == -ENOSPC); > + gnttab_grant_foreign_access_ref(ref, info->xbdev->otherend_id, > + vmalloc_to_mfn(info->fb + i * PAGE_SIZE), 0); > + info->mfns[i] = ref; > + } > + > + for (i = 0; i * epd < info->nr_pages; i++) { > + ref = gnttab_claim_grant_reference(&gref_head); > + BUG_ON(ref == -ENOSPC); > + gnttab_grant_foreign_access_ref(ref, info->xbdev->otherend_id, > + vmalloc_to_mfn(&info->mfns[i * epd]), 0); > + info->page->pd[i] = ref; > + } > > info->page->width = fb_info->var.xres; > info->page->height = fb_info->var.yres; > @@ -585,8 +725,8 @@ static int xenfb_connect_backend(struct xenbus_device > *dev, > xenbus_dev_fatal(dev, ret, "starting transaction"); > goto unbind_irq; > } > - ret = xenbus_printf(xbt, dev->nodename, "page-ref", "%lu", > - virt_to_mfn(info->page)); > + ret = xenbus_printf(xbt, dev->nodename, "ring-ref", "%u", > + info->ring_ref); > if (ret) > goto error_xenbus; > ret = xenbus_printf(xbt, dev->nodename, "event-channel", "%u", > diff --git a/include/xen/interface/io/fbif.h b/include/xen/interface/io/fbif.h > index 974a51e..471d867 100644 > --- a/include/xen/interface/io/fbif.h > +++ b/include/xen/interface/io/fbif.h > @@ -77,13 +77,20 @@ union xenfb_out_event { > > /* > * Frontends should ignore unknown in events. > - * No in events currently defined. > */ > > +#define XENFB_TYPE_RELEASE 4 > +struct xenfb_release { > + uint8_t type; /* XENFB_TYPE_RELEASE */ Isn't the type already stored in struct xenfb_in_event? Why do you need it here too? Are there two different types? > + int32_t buffer; /* buffer # */ > +}; > + > + > #define XENFB_IN_EVENT_SIZE 40 > > union xenfb_in_event { > uint8_t type; > + struct xenfb_release release; > char pad[XENFB_IN_EVENT_SIZE]; > }; > > -- > 1.7.9.5 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@xxxxxxxxxxxxx > http://lists.xen.org/xen-devel > _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |