[RFC][PATCH v1] xen-fbfront: replace deferred io with buffer queue

[RFC][PATCH v1] xen-fbfront: replace deferred io with buffer queue

[Sergiy Kibrik]

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.

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.

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*/
 #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,
+};
+
 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;
+	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);
+	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;
+	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 */
+	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

Re: [RFC][PATCH v1] xen-fbfront: replace deferred io with buffer queue

[Stefano Stabellini]

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.

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
> 

Re: [RFC][PATCH v1] xen-fbfront: replace deferred io with buffer queue

[Stefano Stabellini]

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
> 

Re: [RFC][PATCH v1] xen-fbfront: replace deferred io with buffer queue

[Ian Campbell]

On Mon, 2015-01-19 at 14:03 +0200, Sergiy Kibrik wrote:
>  include/xen/interface/io/fbif.h   |    9 +-

Please get the any protocol changes reviewed and accepted into xen.git
first, including e.g. the switch to grant tables, if that requires
front/back end coordination.

Ian.

Re: [RFC][PATCH v1] xen-fbfront: replace deferred io with buffer queue

[Sergiy Kibrik]

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
>>

Re: [RFC][PATCH v1] xen-fbfront: replace deferred io with buffer queue

[Sergiy Kibrik]

On 1/19/2015 3:25 PM, Stefano Stabellini wrote:
>> 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?
> 
AFAIK there's no backend code in xen-4.x tree anymore, I found it in
old releases. But I'll find a way to present backend as well.

num_bufs should be set by xl tool according to domain config file, but
we don't have patch for xl yet (it's done manually via xenstore-write in
our setup. Fairly ugly)

-- 
regards,
Sergey

Re: [RFC][PATCH v1] xen-fbfront: replace deferred io with buffer queue

[Sergiy Kibrik]

On 1/19/2015 3:31 PM, Ian Campbell wrote:
> On Mon, 2015-01-19 at 14:03 +0200, Sergiy Kibrik wrote:
>>  include/xen/interface/io/fbif.h   |    9 +-
> 
> Please get the any protocol changes reviewed and accepted into xen.git
> first, including e.g. the switch to grant tables, if that requires
> front/back end coordination.
> 
> Ian.
> 

hi Ian,
I surely can post a patch for xen.git as well, however my major
intention is not just to have all the stuff accepted, but rather to
receive comments about design itself, whether such work is appropriate
or not, or probably the community thinks of some other ways to implement
this.
Current state is really too far from being acceptable.

-- 
regards,
Sergey

Re: [RFC][PATCH v1] xen-fbfront: replace deferred io with buffer queue

[Stefano Stabellini]

On Mon, 19 Jan 2015, Sergiy Kibrik wrote:
> On 1/19/2015 3:25 PM, Stefano Stabellini wrote:
> >> 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?
> > 
> AFAIK there's no backend code in xen-4.x tree anymore, I found it in
> old releases. But I'll find a way to present backend as well.

The backend is in QEMU: hw/display/xenfb.c.


> num_bufs should be set by xl tool according to domain config file, but
> we don't have patch for xl yet (it's done manually via xenstore-write in
> our setup. Fairly ugly)

OK, we need that patch :-)