* Re: [RFC][PATCH v1] xen-fbfront: replace deferred io with buffer queue
2015-01-19 13:21 ` Stefano Stabellini
@ 2015-01-19 13:25 ` Stefano Stabellini
2015-01-19 16:39 ` Sergiy Kibrik
2015-01-19 16:33 ` Sergiy Kibrik
1 sibling, 1 reply; 8+ messages in thread
From: Stefano Stabellini @ 2015-01-19 13:25 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Sergiy Kibrik
On Mon, 19 Jan 2015, Stefano Stabellini wrote:
> 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@globallogic.com>
> > ---
> >
> > 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.
Sorry, I realize now that fortunately it is the other way around.
It would be nice to see a correspondent patch for xenfb (the backend).
Who configures num_bufs initially in your system?
> 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@lists.xen.org
> > http://lists.xen.org/xen-devel
> >
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC][PATCH v1] xen-fbfront: replace deferred io with buffer queue
2015-01-19 13:21 ` Stefano Stabellini
2015-01-19 13:25 ` Stefano Stabellini
@ 2015-01-19 16:33 ` Sergiy Kibrik
1 sibling, 0 replies; 8+ messages in thread
From: Sergiy Kibrik @ 2015-01-19 16:33 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel
hi Stefano, thank you for comments,
On 1/19/2015 3:21 PM, Stefano Stabellini wrote:
> 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.
>
ok, will do it
>
>> Signed-off-by: Sergiy Kibrik <sergiy.kibrik@globallogic.com>
>> ---
>>
>> 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?
yes, it is. E.g. on DRA7xx evaluation board with 800x480 display we
adopted quad-buffering, this means we allocate around 6.8MB overall.
However for traditional double-buffered case it's gonna be around 3.4Mb,
affordable size for graphics use.
>
>
>> 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?
no, it was left intentionally as a hint for turning debug messages on.
But if it's redundant I can remove one.
>
>
>> #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.
>
will do that
>
>> 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.
lock renamed because it protects the whole xenfb_info object, not just
rectangle attributes. I'll make up separate patch.
>
>
>> + 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?
>
no specific need for vmalloc_32, will correct that
>
>> + 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?
>
>
xenfb_in_event is a union.
I just did it in the way union xenfb_out_event already implemented.
--
regards,
Sergey
>> + 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@lists.xen.org
>> http://lists.xen.org/xen-devel
>>
^ permalink raw reply [flat|nested] 8+ messages in thread