Dynamic modes support for xenfb (2 of 2)

Attached patch adds dynamic frame buffer resolution support to the PV xenfb frame buffer driver.

Corresponding backend IOEMU patch is required for functionality but this patch is not dependent on it, preserving backwards compatibility.

Please apply to tip of linux-2.6.18-xen

Signed-off-by Pat Campbell <[email protected]>

diff -r 3983b041fc51 drivers/xen/fbfront/xenfb.c

drivers/xen/fbfront/xenfb.c +11 -0

@@ -31,6 +31,7 @@
 #include <xen/interface/io/protocols.h>
 #include <xen/xenbus.h>
 #include <linux/kthread.h>
+#include <linux/delay.h>
 
 struct xenfb_mapping
 {

@@ -62,6 +63,8 @@ struct xenfb_info
 	struct xenfb_page	*page;
 	unsigned long 		*mfns;
 	int			update_wanted; /* XENFB_TYPE_UPDATE wanted */
+	int			feature_resize; /* Backend has resize feature */
+	struct mutex		queue_lock;
 
 	struct xenbus_device	*xbdev;
 };

@@ -129,20 +132,42 @@ struct xenfb_info
  *
  * Oh well, we wont be updating the writes to this page anytime soon.
  */
+#define MB_ (1024*1024)
+#define XENFB_DEFAULT_FB_LEN (XENFB_WIDTH * XENFB_HEIGHT * XENFB_DEPTH / 8)
+
+enum {KPARAM_MEM, KPARAM_WIDTH, KPARAM_HEIGHT, KPARAM_CNT};
+static int video[KPARAM_CNT] = {2, XENFB_WIDTH, XENFB_HEIGHT};
+module_param_array(video, int, NULL, 0);
+MODULE_PARM_DESC(video,
+		"Size of video memory in MB and width,height in pixels, default = (2,800,600)");

drivers/xen/fbfront/xenfb.c +31 -8

+
+static void xenfb_send_event(struct xenfb_info *info,
+		union xenfb_out_event *event)
+{
+	__u32 prod;
+
+	prod = info->page->out_prod;
+	/* caller ensures !xenfb_queue_full() */
+	mb();			/* ensure ring space available */
+	XENFB_OUT_RING_REF(info->page, prod) = *event;
+	wmb();			/* ensure ring contents visible */
+	info->page->out_prod = prod + 1;
+
+	notify_remote_via_irq(info->irq);
+}
 
 static void xenfb_do_update(struct xenfb_info *info,
 			    int x, int y, int w, int h)
 {
 	union xenfb_out_event event;
-	__u32 prod;
 
 	event.type = XENFB_TYPE_UPDATE;
 	event.update.x = x;

@@ -150,14 +175,23 @@ static void xenfb_do_update(struct xenfb
 	event.update.width = w;
 	event.update.height = h;
 
-	prod = info->page->out_prod;
 	/* caller ensures !xenfb_queue_full() */
-	mb();			/* ensure ring space available */
-	XENFB_OUT_RING_REF(info->page, prod) = event;
-	wmb();			/* ensure ring contents visible */
-	info->page->out_prod = prod + 1;
-
-	notify_remote_via_irq(info->irq);
+	xenfb_send_event(info, &event);
+}
+
+static void xenfb_do_resize(struct xenfb_info *info,
+			    struct fb_info *fb_info)
+{
+	union xenfb_out_event event;
+
+	event.type = XENFB_TYPE_RESIZE;
+	event.resize.width = fb_info->var.xres;
+	event.resize.height = fb_info->var.yres;
+	event.resize.stride = fb_info->fix.line_length;
+	event.resize.depth = fb_info->var.bits_per_pixel;
+
+	/* caller ensures !xenfb_queue_full() */
+	xenfb_send_event(info, &event);
 }
 
 static int xenfb_queue_full(struct xenfb_info *info)

drivers/xen/fbfront/xenfb.c +44 -0

+	if (xenfb_queue_full(info)) {
+		mutex_unlock(&info->queue_lock);
 		return;
+	}
 
 	mutex_lock(&info->mm_lock);
 

@@ -207,6 +245,7 @@ static void xenfb_update_screen(struct x
 		WARN_ON(1);
 	}
 	xenfb_do_update(info, x1, y1, x2 - x1, y2 - y1);
+	mutex_unlock(&info->queue_lock);
 }
 
 static int xenfb_thread(void *data)

@@ -413,6 +452,60 @@ static int xenfb_mmap(struct fb_info *fb
 	return 0;
 }
 
+static int
+xenfb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
+{
+	struct xenfb_info *xenfb_info;
+	int required_mem_len;
+
+	xenfb_info = info->par;
+
+	if (!xenfb_info->feature_resize) {
+		if (var->xres == video[KPARAM_WIDTH] &&
+			var->yres == video[KPARAM_HEIGHT] &&
+			var->bits_per_pixel == xenfb_info->page->depth) {
+			return 0;
+		}
+		return -EINVAL;
+	}
+
+	/* Can't resize past initial width and height */
+	if (var->xres > video[KPARAM_WIDTH] || var->yres > video[KPARAM_HEIGHT])
+		return -EINVAL;
+
+	required_mem_len = var->xres * var->yres * (xenfb_info->page->depth / 8);
+	if (var->bits_per_pixel == xenfb_info->page->depth &&
+		var->xres <= info->fix.line_length / (XENFB_DEPTH / 8) &&
+		required_mem_len <= info->fix.smem_len) {
+		var->xres_virtual = var->xres;
+		var->yres_virtual = var->yres;
+		return 0;
+	}
+	return -EINVAL;
+}
+
+static int xenfb_set_par(struct fb_info *info)
+{
+	struct xenfb_info *xenfb_info;
+	unsigned long timeout = jiffies + HZ;
+	int status;
+
+	xenfb_info = info->par;
+

drivers/xen/fbfront/xenfb.c +3 -0

+
 static struct fb_ops xenfb_fb_ops = {
 	.owner		= THIS_MODULE,
 	.fb_setcolreg	= xenfb_setcolreg,

@@ -420,6 +513,8 @@ static struct fb_ops xenfb_fb_ops = {
 	.fb_copyarea	= xenfb_copyarea,
 	.fb_imageblit	= xenfb_imageblit,
 	.fb_mmap	= xenfb_mmap,
+	.fb_check_var	= xenfb_check_var,
+	.fb_set_par     = xenfb_set_par,
 };
 
 static irqreturn_t xenfb_event_handler(int rq, void *dev_id,

drivers/xen/fbfront/xenfb.c +25 -11

 	int ret;
 
 	info = kzalloc(sizeof(*info), GFP_KERNEL);

@@ -457,24 +554,40 @@ static int __devinit xenfb_probe(struct 
 		xenbus_dev_fatal(dev, -ENOMEM, "allocating info structure");
 		return -ENOMEM;
 	}
+
+	/* Limit kernel param videoram amount to what is in xenstore */
+	if (xenbus_scanf(XBT_NIL, dev->otherend, "videoram", "%d", &val) == 1) {
+		if (val < video[KPARAM_MEM])
+			video[KPARAM_MEM] = val;
+	}
+
+	/* If requested res does not fit in available memory, use default */
+	fb_size = video[KPARAM_MEM] * MB_;
+	if (video[KPARAM_WIDTH] * video[KPARAM_HEIGHT] * XENFB_DEPTH/8 > fb_size) {
+		video[KPARAM_WIDTH] = XENFB_WIDTH;
+		video[KPARAM_HEIGHT] = XENFB_HEIGHT;
+		fb_size = XENFB_DEFAULT_FB_LEN;
+	}
+
 	dev->dev.driver_data = info;
 	info->xbdev = dev;
 	info->irq = -1;
 	info->x1 = info->y1 = INT_MAX;
 	spin_lock_init(&info->dirty_lock);
 	mutex_init(&info->mm_lock);
+	mutex_init(&info->queue_lock);
 	init_waitqueue_head(&info->wq);
 	init_timer(&info->refresh);
 	info->refresh.function = xenfb_timer;
 	info->refresh.data = (unsigned long)info;
 	INIT_LIST_HEAD(&info->mappings);
 
-	info->fb = vmalloc(xenfb_mem_len);
+	info->fb = vmalloc(fb_size);
 	if (info->fb == NULL)
 		goto error_nomem;
-	memset(info->fb, 0, xenfb_mem_len);
-
-	info->nr_pages = (xenfb_mem_len + PAGE_SIZE - 1) >> PAGE_SHIFT;
+	memset(info->fb, 0, fb_size);
+
+	info->nr_pages = (fb_size + PAGE_SIZE - 1) >> PAGE_SHIFT;
 
 	info->pages = kmalloc(sizeof(struct page *) * info->nr_pages,
 			      GFP_KERNEL);

@@ -489,8 +602,6 @@ static int __devinit xenfb_probe(struct 
 	info->page = (void *)__get_free_page(GFP_KERNEL | __GFP_ZERO);
 	if (!info->page)
 		goto error_nomem;
-
-	xenfb_init_shared_page(info);
 
 	fb_info = framebuffer_alloc(sizeof(u32) * 256, NULL);
 				/* see fishy hackery below */

@@ -504,9 +615,9 @@ static int __devinit xenfb_probe(struct 
 	fb_info->screen_base = info->fb;
 
 	fb_info->fbops = &xenfb_fb_ops;
-	fb_info->var.xres_virtual = fb_info->var.xres = info->page->width;
-	fb_info->var.yres_virtual = fb_info->var.yres = info->page->height;
-	fb_info->var.bits_per_pixel = info->page->depth;
+	fb_info->var.xres_virtual = fb_info->var.xres = video[KPARAM_WIDTH];
+	fb_info->var.yres_virtual = fb_info->var.yres = video[KPARAM_HEIGHT];
+	fb_info->var.bits_per_pixel = XENFB_DEPTH;
 
 	fb_info->var.red = (struct fb_bitfield){16, 8, 0};
 	fb_info->var.green = (struct fb_bitfield){8, 8, 0};

@@ -518,9 +629,9 @@ static int __devinit xenfb_probe(struct 
 	fb_info->var.vmode = FB_VMODE_NONINTERLACED;
 
 	fb_info->fix.visual = FB_VISUAL_TRUECOLOR;
-	fb_info->fix.line_length = info->page->line_length;
+	fb_info->fix.line_length = fb_info->var.xres * (XENFB_DEPTH / 8);
 	fb_info->fix.smem_start = 0;
-	fb_info->fix.smem_len = xenfb_mem_len;
+	fb_info->fix.smem_len = fb_size;
 	strcpy(fb_info->fix.id, "xen");
 	fb_info->fix.type = FB_TYPE_PACKED_PIXELS;
 	fb_info->fix.accel = FB_ACCEL_NONE;

drivers/xen/fbfront/xenfb.c +17 -9

 	ret = register_framebuffer(fb_info);
 	if (ret) {

@@ -571,7 +685,7 @@ static int xenfb_resume(struct xenbus_de
 	struct xenfb_info *info = dev->dev.driver_data;
 
 	xenfb_disconnect_backend(info);
-	xenfb_init_shared_page(info);
+	xenfb_init_shared_page(info, info->fb_info);
 	return xenfb_connect_backend(dev, info);
 }
 

@@ -597,9 +711,11 @@ static int xenfb_remove(struct xenbus_de
 	return 0;
 }
 
-static void xenfb_init_shared_page(struct xenfb_info *info)
+static void xenfb_init_shared_page(struct xenfb_info *info,
+                                   struct fb_info * fb_info)
 {
 	int i;
+	int epd = PAGE_SIZE / sizeof(info->mfns[0]);
 
 	for (i = 0; i < info->nr_pages; i++)
 		info->pages[i] = vmalloc_to_page(info->fb + i * PAGE_SIZE);

@@ -607,13 +723,14 @@ static void xenfb_init_shared_page(struc
 	for (i = 0; i < info->nr_pages; i++)
 		info->mfns[i] = vmalloc_to_mfn(info->fb + i * PAGE_SIZE);
 
-	info->page->pd[0] = vmalloc_to_mfn(info->mfns);
-	info->page->pd[1] = 0;
-	info->page->width = XENFB_WIDTH;
-	info->page->height = XENFB_HEIGHT;
-	info->page->depth = XENFB_DEPTH;
-	info->page->line_length = (info->page->depth / 8) * info->page->width;
-	info->page->mem_length = xenfb_mem_len;
+	for (i = 0; i * epd < info->nr_pages; i++)
+		info->page->pd[i] = vmalloc_to_mfn(&info->mfns[i * epd]);
+
+	info->page->width = fb_info->var.xres;
+	info->page->height = fb_info->var.yres;
+	info->page->depth = fb_info->var.bits_per_pixel;
+	info->page->line_length = fb_info->fix.line_length;
+	info->page->mem_length = fb_info->fix.smem_len;
 	info->page->in_cons = info->page->in_prod = 0;
 	info->page->out_cons = info->page->out_prod = 0;
 }

@@ -712,6 +829,11 @@ static void xenfb_backend_changed(struct
 			val = 0;
 		if (val)
 			info->update_wanted = 1;
+
+		if (xenbus_scanf(XBT_NIL, dev->otherend,
+					"feature-resize", "%d", &val) < 0)
+			val = 0;
+		info->feature_resize = val;
 		break;
 
 	case XenbusStateClosing:

diff -r 3983b041fc51 drivers/xen/fbfront/xenkbd.c

drivers/xen/fbfront/xenkbd.c +1 -0

@@ -297,6 +297,16 @@ static void xenkbd_backend_changed(struc
 		 */
 		if (dev->state != XenbusStateConnected)
 			goto InitWait; /* no InitWait seen yet, fudge it */
+

drivers/xen/fbfront/xenkbd.c +0 -0

 	case XenbusStateClosing:

diff -r 3983b041fc51 include/xen/interface/io/fbif.h

include/xen/interface/io/fbif.h +23 -5

@@ -50,12 +50,28 @@ struct xenfb_update
     int32_t height; /* rect height */
 };
 
+/*
+ * Framebuffer resize notification event
+ * Capable backend sets feature-resize in xenstore.
+ */
+#define XENFB_TYPE_RESIZE 3
+
+struct xenfb_resize
+{
+    uint8_t type;    /* XENFB_TYPE_RESIZE */
+    int32_t width;   /* width in pixels */
+    int32_t height;  /* height in pixels */
+    int32_t stride;  /* stride in bytes */
+    int32_t depth;   /* depth in bits */
+};
+
 #define XENFB_OUT_EVENT_SIZE 40
 
 union xenfb_out_event
 {
     uint8_t type;
     struct xenfb_update update;
+    struct xenfb_resize resize;
     char pad[XENFB_OUT_EVENT_SIZE];
 };
 

@@ -109,15 +125,17 @@ struct xenfb_page
      * Each directory page holds PAGE_SIZE / sizeof(*pd)
      * framebuffer pages, and can thus map up to PAGE_SIZE *
      * PAGE_SIZE / sizeof(*pd) bytes.  With PAGE_SIZE == 4096 and
-     * sizeof(unsigned long) == 4, that's 4 Megs.  Two directory
-     * pages should be enough for a while.
+     * sizeof(unsigned long) == 4/8, that's 4 Megs 32 bit and 2 Megs
+     * 64 bit.  256 directories give enough room for a 512 Meg
+     * framebuffer with a max resolution of 12,800x10,240.  Should
+     * be enough for a while with room leftover for expansion.
      */
-    unsigned long pd[2];
+    unsigned long pd[256];
 };
 
 /*
- * Wart: xenkbd needs to know resolution.  Put it here until a better
- * solution is found, but don't leak it to the backend.
+ * Wart: xenkbd needs to know default resolution.  Put it here until a
+ * better solution is found, but don't leak it to the backend.
  */
 #ifdef __KERNEL__
 #define XENFB_WIDTH 800

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Quoted 10 lines · Pat Campbell

Attached patch adds dynamic frame buffer resolution support to the PV xenfb frame buffer driver.

Corresponding backend IOEMU patch is required for functionality but this patch is not dependent on it, preserving backwards compatibility.

Please apply to tip of linux-2.6.18-xen

Signed-off-by Pat Campbell <[email protected]>

Adding another lock (queue_lock) to our already ticklish locking gives me a queasy feeling...

The purpose of the lock is not obvious to me. Please explain that, in the code. Likewise, it's not entirely obvious that the locking works. Please update the "How the locks work together" comment.

But before you do that, I suggest you tell us exactly what problem you're attempting to solve with queue_lock. Maybe we can come up with a less scary solution. Maybe not. But it's worth a try. If it's just to ensure the changes made by xenfb_set_par() are seen in xenfb_do_resize() correctly, a memory barrier should to the trick more easily.

Quoted 36 lines

diff -r 3983b041fc51 drivers/xen/fbfront/xenfb.c

drivers/xen/fbfront/xenfb.c +11 -1

@@ -31,6 +31,7 @@
 #include <xen/interface/io/protocols.h>
 #include <xen/xenbus.h>
 #include <linux/kthread.h>
+#include <linux/delay.h>
 
 struct xenfb_mapping
 {

@@ -62,6 +63,8 @@ struct xenfb_info
 	struct xenfb_page	*page;
 	unsigned long 		*mfns;
 	int			update_wanted; /* XENFB_TYPE_UPDATE wanted */
+	int			feature_resize; /* Backend has resize feature */
+	struct mutex		queue_lock;
 
 	struct xenbus_device	*xbdev;
 };

@@ -129,20 +132,42 @@ struct xenfb_info
  *
  * Oh well, we wont be updating the writes to this page anytime soon.
  */
+#define MB_ (1024*1024)
+#define XENFB_DEFAULT_FB_LEN (XENFB_WIDTH * XENFB_HEIGHT * XENFB_DEPTH / 8)
+
+enum {KPARAM_MEM, KPARAM_WIDTH, KPARAM_HEIGHT, KPARAM_CNT};
+static int video[KPARAM_CNT] = {2, XENFB_WIDTH, XENFB_HEIGHT};
+module_param_array(video, int, NULL, 0);
+MODULE_PARM_DESC(video,
+		"Size of video memory in MB and width,height in pixels, default = (2,800,600)");
 
 static int xenfb_fps = 20;
-static unsigned long xenfb_mem_len = XENFB_WIDTH * XENFB_HEIGHT * XENFB_DEPTH / 8;
 

drivers/xen/fbfront/xenfb.c struct xenfb_info · line 166

Quoted 7 lines

drivers/xen/fbfront/xenfb.c +2 -1

 static int xenfb_remove(struct xenbus_device *);
-static void xenfb_init_shared_page(struct xenfb_info *);
+static void xenfb_init_shared_page(struct xenfb_info *, struct fb_info *);
 static int xenfb_connect_backend(struct xenbus_device *, struct xenfb_info *);
 static void xenfb_disconnect_backend(struct xenfb_info *);
+static void xenfb_refresh(struct xenfb_info *, int, int, int, int);
 

Is this forward declaration needed?

Quoted 58 lines

drivers/xen/fbfront/xenfb.c +15 -1

+
+static void xenfb_send_event(struct xenfb_info *info,
+		union xenfb_out_event *event)
+{
+	__u32 prod;
+
+	prod = info->page->out_prod;
+	/* caller ensures !xenfb_queue_full() */
+	mb();			/* ensure ring space available */
+	XENFB_OUT_RING_REF(info->page, prod) = *event;
+	wmb();			/* ensure ring contents visible */
+	info->page->out_prod = prod + 1;
+
+	notify_remote_via_irq(info->irq);
+}
 
 static void xenfb_do_update(struct xenfb_info *info,
 			    int x, int y, int w, int h)
 {
 	union xenfb_out_event event;
-	__u32 prod;
 
 	event.type = XENFB_TYPE_UPDATE;

drivers/xen/fbfront/xenfb.c +16 -7

 	event.update.x = x;

@@ -150,14 +175,23 @@ static void xenfb_do_update(struct xenfb
 	event.update.width = w;
 	event.update.height = h;
 
-	prod = info->page->out_prod;
 	/* caller ensures !xenfb_queue_full() */
-	mb();			/* ensure ring space available */
-	XENFB_OUT_RING_REF(info->page, prod) = event;
-	wmb();			/* ensure ring contents visible */
-	info->page->out_prod = prod + 1;
-
-	notify_remote_via_irq(info->irq);
+	xenfb_send_event(info, &event);
+}
+
+static void xenfb_do_resize(struct xenfb_info *info,
+			    struct fb_info *fb_info)
+{
+	union xenfb_out_event event;
+
+	event.type = XENFB_TYPE_RESIZE;
+	event.resize.width = fb_info->var.xres;
+	event.resize.height = fb_info->var.yres;
+	event.resize.stride = fb_info->fix.line_length;
+	event.resize.depth = fb_info->var.bits_per_pixel;
+
+	/* caller ensures !xenfb_queue_full() */
+	xenfb_send_event(info, &event);
 }
 
 static int xenfb_queue_full(struct xenfb_info *info)

@@ -177,8 +211,12 @@ static void xenfb_update_screen(struct x
 

drivers/xen/fbfront/xenfb.c static void xenfb_update_screen(struct x · line 220

Quoted 6 lines

drivers/xen/fbfront/xenfb.c +2 -1

 	if (!info->update_wanted)
 		return;
-	if (xenfb_queue_full(info))
+
+	mutex_lock(&info->queue_lock);
 

This can block the thread running xenfb_thread() indefinitely. I guess that's okay. We don't do S4 (save-to-disk) in domU anyway.

Quoted 61 lines

drivers/xen/fbfront/xenfb.c +45 -0

+	if (xenfb_queue_full(info)) {
+		mutex_unlock(&info->queue_lock);
 		return;
+	}
 
 	mutex_lock(&info->mm_lock);

@@ -207,6 +245,7 @@ static void xenfb_update_screen(struct x
 		WARN_ON(1);
 	}
 	xenfb_do_update(info, x1, y1, x2 - x1, y2 - y1);
+	mutex_unlock(&info->queue_lock);
 }
 
 static int xenfb_thread(void *data)

@@ -413,6 +452,60 @@ static int xenfb_mmap(struct fb_info *fb
 	return 0;
 }
 
+static int
+xenfb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
+{
+	struct xenfb_info *xenfb_info;
+	int required_mem_len;
+
+	xenfb_info = info->par;
+
+	if (!xenfb_info->feature_resize) {
+		if (var->xres == video[KPARAM_WIDTH] &&
+			var->yres == video[KPARAM_HEIGHT] &&
+			var->bits_per_pixel == xenfb_info->page->depth) {
+			return 0;
+		}
+		return -EINVAL;
+	}
+
+	/* Can't resize past initial width and height */
+	if (var->xres > video[KPARAM_WIDTH] || var->yres > video[KPARAM_HEIGHT])
+		return -EINVAL;
+
+	required_mem_len = var->xres * var->yres * (xenfb_info->page->depth / 8);
+	if (var->bits_per_pixel == xenfb_info->page->depth &&
+		var->xres <= info->fix.line_length / (XENFB_DEPTH / 8) &&
+		required_mem_len <= info->fix.smem_len) {
+		var->xres_virtual = var->xres;
+		var->yres_virtual = var->yres;
+		return 0;
+	}
+	return -EINVAL;
+}
+
+static int xenfb_set_par(struct fb_info *info)
+{
+	struct xenfb_info *xenfb_info;
+	unsigned long timeout = jiffies + HZ;
+	int status;
+
+	xenfb_info = info->par;
+
+	mutex_lock(&xenfb_info->queue_lock);

drivers/xen/fbfront/xenfb.c static int xenfb_mmap(struct fb_info *fb · line 505

Quoted 10 lines

drivers/xen/fbfront/xenfb.c +9 -0

+	while ((status = xenfb_queue_full(xenfb_info)) && 
+		time_before(jiffies, timeout)) {
+		msleep(10);
+	}
+	if (status == 0) {
+		info->var.xres_virtual = info->var.xres;
+		info->var.yres_virtual = info->var.yres;
+		xenfb_do_resize(xenfb_info, info);
+	}
 

else fail silently by doing nothing

drivers/xen/fbfront/xenfb.c static int xenfb_mmap(struct fb_info *fb

Quoted 4 lines

drivers/xen/fbfront/xenfb.c +3 -0

+	mutex_unlock(&xenfb_info->queue_lock);
+	return 0;
+}
 

Does the framebuffer code handle the fb_set_par() callback doing nothing?

Even if it does: is silently failing acceptable?

Quoted 13 lines

drivers/xen/fbfront/xenfb.c +1 -0

+
 static struct fb_ops xenfb_fb_ops = {

drivers/xen/fbfront/xenfb.c +2 -0

 	.owner		= THIS_MODULE,
 	.fb_setcolreg	= xenfb_setcolreg,

@@ -420,6 +513,8 @@ static struct fb_ops xenfb_fb_ops = {
 	.fb_copyarea	= xenfb_copyarea,
 	.fb_imageblit	= xenfb_imageblit,
 	.fb_mmap	= xenfb_mmap,
+	.fb_check_var	= xenfb_check_var,
+	.fb_set_par     = xenfb_set_par,
 };
 

drivers/xen/fbfront/xenfb.c static int __devinit xenfb_probe(struct · line 550

Quoted 8 lines

drivers/xen/fbfront/xenfb.c +2 -0

 static irqreturn_t xenfb_event_handler(int rq, void *dev_id,

@@ -450,6 +545,8 @@ static int __devinit xenfb_probe(struct 
 {
 	struct xenfb_info *info;
 	struct fb_info *fb_info;
+	int fb_size;
+	int val = 0;
 

Why do you initialize val?

Quoted 85 lines

drivers/xen/fbfront/xenfb.c +0 -0

 	int ret;
 

drivers/xen/fbfront/xenfb.c +25 -11

 	info = kzalloc(sizeof(*info), GFP_KERNEL);

@@ -457,24 +554,40 @@ static int __devinit xenfb_probe(struct 
 		xenbus_dev_fatal(dev, -ENOMEM, "allocating info structure");
 		return -ENOMEM;
 	}
+
+	/* Limit kernel param videoram amount to what is in xenstore */
+	if (xenbus_scanf(XBT_NIL, dev->otherend, "videoram", "%d", &val) == 1) {
+		if (val < video[KPARAM_MEM])
+			video[KPARAM_MEM] = val;
+	}
+
+	/* If requested res does not fit in available memory, use default */
+	fb_size = video[KPARAM_MEM] * MB_;
+	if (video[KPARAM_WIDTH] * video[KPARAM_HEIGHT] * XENFB_DEPTH/8 > fb_size) {
+		video[KPARAM_WIDTH] = XENFB_WIDTH;
+		video[KPARAM_HEIGHT] = XENFB_HEIGHT;
+		fb_size = XENFB_DEFAULT_FB_LEN;
+	}
+
 	dev->dev.driver_data = info;
 	info->xbdev = dev;
 	info->irq = -1;
 	info->x1 = info->y1 = INT_MAX;
 	spin_lock_init(&info->dirty_lock);
 	mutex_init(&info->mm_lock);
+	mutex_init(&info->queue_lock);
 	init_waitqueue_head(&info->wq);
 	init_timer(&info->refresh);
 	info->refresh.function = xenfb_timer;
 	info->refresh.data = (unsigned long)info;
 	INIT_LIST_HEAD(&info->mappings);
 
-	info->fb = vmalloc(xenfb_mem_len);
+	info->fb = vmalloc(fb_size);
 	if (info->fb == NULL)
 		goto error_nomem;
-	memset(info->fb, 0, xenfb_mem_len);
-
-	info->nr_pages = (xenfb_mem_len + PAGE_SIZE - 1) >> PAGE_SHIFT;
+	memset(info->fb, 0, fb_size);
+
+	info->nr_pages = (fb_size + PAGE_SIZE - 1) >> PAGE_SHIFT;
 
 	info->pages = kmalloc(sizeof(struct page *) * info->nr_pages,
 			      GFP_KERNEL);

@@ -489,8 +602,6 @@ static int __devinit xenfb_probe(struct 
 	info->page = (void *)__get_free_page(GFP_KERNEL | __GFP_ZERO);
 	if (!info->page)
 		goto error_nomem;
-
-	xenfb_init_shared_page(info);
 
 	fb_info = framebuffer_alloc(sizeof(u32) * 256, NULL);
 				/* see fishy hackery below */

@@ -504,9 +615,9 @@ static int __devinit xenfb_probe(struct 
 	fb_info->screen_base = info->fb;
 
 	fb_info->fbops = &xenfb_fb_ops;
-	fb_info->var.xres_virtual = fb_info->var.xres = info->page->width;
-	fb_info->var.yres_virtual = fb_info->var.yres = info->page->height;
-	fb_info->var.bits_per_pixel = info->page->depth;
+	fb_info->var.xres_virtual = fb_info->var.xres = video[KPARAM_WIDTH];
+	fb_info->var.yres_virtual = fb_info->var.yres = video[KPARAM_HEIGHT];
+	fb_info->var.bits_per_pixel = XENFB_DEPTH;
 
 	fb_info->var.red = (struct fb_bitfield){16, 8, 0};
 	fb_info->var.green = (struct fb_bitfield){8, 8, 0};

@@ -518,9 +629,9 @@ static int __devinit xenfb_probe(struct 
 	fb_info->var.vmode = FB_VMODE_NONINTERLACED;
 
 	fb_info->fix.visual = FB_VISUAL_TRUECOLOR;
-	fb_info->fix.line_length = info->page->line_length;
+	fb_info->fix.line_length = fb_info->var.xres * (XENFB_DEPTH / 8);
 	fb_info->fix.smem_start = 0;
-	fb_info->fix.smem_len = xenfb_mem_len;
+	fb_info->fix.smem_len = fb_size;
 	strcpy(fb_info->fix.id, "xen");
 	fb_info->fix.type = FB_TYPE_PACKED_PIXELS;
 	fb_info->fix.accel = FB_ACCEL_NONE;

@@ -533,6 +644,9 @@ static int __devinit xenfb_probe(struct 
 		xenbus_dev_fatal(dev, ret, "fb_alloc_cmap");

drivers/xen/fbfront/xenfb.c static int __devinit xenfb_probe(struct · line 650

Quoted 6 lines

drivers/xen/fbfront/xenfb.c +3 -0

 		goto error;
 	}
+
+	/* init shared page, uses fb_info attributes */
+	xenfb_init_shared_page(info, fb_info);
 

The comment is somewhat redundant.

Quoted 75 lines

drivers/xen/fbfront/xenfb.c +17 -9

 	ret = register_framebuffer(fb_info);
 	if (ret) {

@@ -571,7 +685,7 @@ static int xenfb_resume(struct xenbus_de
 	struct xenfb_info *info = dev->dev.driver_data;
 
 	xenfb_disconnect_backend(info);
-	xenfb_init_shared_page(info);
+	xenfb_init_shared_page(info, info->fb_info);
 	return xenfb_connect_backend(dev, info);
 }
 

@@ -597,9 +711,11 @@ static int xenfb_remove(struct xenbus_de
 	return 0;
 }
 
-static void xenfb_init_shared_page(struct xenfb_info *info)
+static void xenfb_init_shared_page(struct xenfb_info *info,
+                                   struct fb_info * fb_info)
 {
 	int i;
+	int epd = PAGE_SIZE / sizeof(info->mfns[0]);
 
 	for (i = 0; i < info->nr_pages; i++)
 		info->pages[i] = vmalloc_to_page(info->fb + i * PAGE_SIZE);

@@ -607,13 +723,14 @@ static void xenfb_init_shared_page(struc
 	for (i = 0; i < info->nr_pages; i++)
 		info->mfns[i] = vmalloc_to_mfn(info->fb + i * PAGE_SIZE);
 
-	info->page->pd[0] = vmalloc_to_mfn(info->mfns);
-	info->page->pd[1] = 0;
-	info->page->width = XENFB_WIDTH;
-	info->page->height = XENFB_HEIGHT;
-	info->page->depth = XENFB_DEPTH;
-	info->page->line_length = (info->page->depth / 8) * info->page->width;
-	info->page->mem_length = xenfb_mem_len;
+	for (i = 0; i * epd < info->nr_pages; i++)
+		info->page->pd[i] = vmalloc_to_mfn(&info->mfns[i * epd]);
+
+	info->page->width = fb_info->var.xres;
+	info->page->height = fb_info->var.yres;
+	info->page->depth = fb_info->var.bits_per_pixel;
+	info->page->line_length = fb_info->fix.line_length;
+	info->page->mem_length = fb_info->fix.smem_len;
 	info->page->in_cons = info->page->in_prod = 0;
 	info->page->out_cons = info->page->out_prod = 0;
 }

@@ -712,6 +829,11 @@ static void xenfb_backend_changed(struct
 			val = 0;
 		if (val)
 			info->update_wanted = 1;
+
+		if (xenbus_scanf(XBT_NIL, dev->otherend,
+					"feature-resize", "%d", &val) < 0)
+			val = 0;
+		info->feature_resize = val;
 		break;
 
 	case XenbusStateClosing:

diff -r 3983b041fc51 drivers/xen/fbfront/xenkbd.c

drivers/xen/fbfront/xenkbd.c +8 -0

@@ -297,6 +297,16 @@ static void xenkbd_backend_changed(struc
 		 */
 		if (dev->state != XenbusStateConnected)
 			goto InitWait; /* no InitWait seen yet, fudge it */
+
+		/* Set input abs params to match backend screen res */
+		if (xenbus_scanf(XBT_NIL, info->xbdev->otherend,
+				   "width", "%d", &val) > 0 )
+			input_set_abs_params(info->ptr, ABS_X, 0, val, 0, 0);
+
+		if (xenbus_scanf(XBT_NIL, info->xbdev->otherend,
+				   "height", "%d", &val) > 0 )

drivers/xen/fbfront/xenkbd.c static void xenkbd_backend_changed(struc · line 311

Quoted 4 lines

drivers/xen/fbfront/xenkbd.c +2 -0

+			input_set_abs_params(info->ptr, ABS_Y, 0, val, 0, 0);
+
 		break;
 

Open question: why do we have to set the parameters to the actual initial resolution here, but don't have to set them when the resolution changes dynamically?

Quoted 58 lines

drivers/xen/fbfront/xenkbd.c +0 -0

 	case XenbusStateClosing:

diff -r 3983b041fc51 include/xen/interface/io/fbif.h

include/xen/interface/io/fbif.h +23 -5

@@ -50,12 +50,28 @@ struct xenfb_update
     int32_t height; /* rect height */
 };
 
+/*
+ * Framebuffer resize notification event
+ * Capable backend sets feature-resize in xenstore.
+ */
+#define XENFB_TYPE_RESIZE 3
+
+struct xenfb_resize
+{
+    uint8_t type;    /* XENFB_TYPE_RESIZE */
+    int32_t width;   /* width in pixels */
+    int32_t height;  /* height in pixels */
+    int32_t stride;  /* stride in bytes */
+    int32_t depth;   /* depth in bits */
+};
+
 #define XENFB_OUT_EVENT_SIZE 40
 
 union xenfb_out_event
 {
     uint8_t type;
     struct xenfb_update update;
+    struct xenfb_resize resize;
     char pad[XENFB_OUT_EVENT_SIZE];
 };
 

@@ -109,15 +125,17 @@ struct xenfb_page
      * Each directory page holds PAGE_SIZE / sizeof(*pd)
      * framebuffer pages, and can thus map up to PAGE_SIZE *
      * PAGE_SIZE / sizeof(*pd) bytes.  With PAGE_SIZE == 4096 and
-     * sizeof(unsigned long) == 4, that's 4 Megs.  Two directory
-     * pages should be enough for a while.
+     * sizeof(unsigned long) == 4/8, that's 4 Megs 32 bit and 2 Megs
+     * 64 bit.  256 directories give enough room for a 512 Meg
+     * framebuffer with a max resolution of 12,800x10,240.  Should
+     * be enough for a while with room leftover for expansion.
      */
-    unsigned long pd[2];
+    unsigned long pd[256];
 };
 
 /*
- * Wart: xenkbd needs to know resolution.  Put it here until a better
- * solution is found, but don't leak it to the backend.
+ * Wart: xenkbd needs to know default resolution.  Put it here until a
+ * better solution is found, but don't leak it to the backend.
  */
 #ifdef __KERNEL__
 #define XENFB_WIDTH 800

Quoted 5 lines

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Quoted 21 lines · Markus Armbruster

Quoted 10 lines · Pat Campbell

Attached patch adds dynamic frame buffer resolution support to the PV xenfb frame buffer driver.

Corresponding backend IOEMU patch is required for functionality but this patch is not dependent on it, preserving backwards compatibility.

Please apply to tip of linux-2.6.18-xen

Signed-off-by Pat Campbell <[email protected]>

Adding another lock (queue_lock) to our already ticklish locking gives me a queasy feeling...

The purpose of the lock is not obvious to me. Please explain that, in the code. Likewise, it's not entirely obvious that the locking works. Please update the "How the locks work together" comment.

Quoted 8 lines

But before you do that, I suggest you tell us exactly what problem you're attempting to solve with queue_lock. Maybe we can come up with a less scary solution. Maybe not. But it's worth a try. If it's just to ensure the changes made by xenfb_set_par() are seen in xenfb_do_resize() correctly, a memory barrier should to the trick more easily.

xenfb_set_par() needs to complete it's work before returning to the caller so I need to send the resize event from within the xenfb_set_par() function. This would mean we now have two event queue
writers, xenfb_update_screen() and xenfb_set_par(). Very simple 2
writer one reader problem easily solved by the addition of the queue_lock.

Previously I handled the resize in xenfb_thread(). This could lead to us missing a resize and or using the incorrect values on a multi processor system.

IE: Wrong resolution
P1:1 set_par sets resize_dpy w, h already in fb struct
P2:1 thread wakeup sees resize_dpy set
P2:2 do_resize()
P2:3 read fb width
P1:3 check_var/set_par changes fb height
P2:4 read fb height (2nd value here)

IE: Missed resolution change
P1:1 set_par sets resize_dpy w, h already in fb struct
P2:1 thread wakeup sees resize_dpy set
P2:2 do_resize()
P2:3 read fb width
P2:4 read fb height
P1:3 set_par sets resize_dpy
P2:5 clears resize_dpy (Missed the second resize)

Adding in a queue lock should be safe because:
1. xenfb_update_screen() can hold a mutex without interfering with
zap_page_range
2. xenfb_set_par() will timeout releasing the lock so update can run

I have verified that the lock works as expected and times out if necessary.

Quoted 60 lines · Pat Campbell

Quoted 22 lines · Markus Armbruster

Quoted 11 lines · Pat Campbell

Attached patch adds dynamic frame buffer resolution support to the PV xenfb frame buffer driver.

Corresponding backend IOEMU patch is required for functionality but this patch is not dependent on it, preserving backwards compatibility.

Please apply to tip of linux-2.6.18-xen

Signed-off-by Pat Campbell <[email protected]>

Adding another lock (queue_lock) to our already ticklish locking gives me a queasy feeling...

The purpose of the lock is not obvious to me. Please explain that, in the code. Likewise, it's not entirely obvious that the locking works. Please update the "How the locks work together" comment.

Quoted 9 lines

But before you do that, I suggest you tell us exactly what problem you're attempting to solve with queue_lock. Maybe we can come up with a less scary solution. Maybe not. But it's worth a try. If it's just to ensure the changes made by xenfb_set_par() are seen in xenfb_do_resize() correctly, a memory barrier should to the trick more easily.

xenfb_set_par() needs to complete it's work before returning to the caller so I need to send the resize event from within the xenfb_set_par() function. This would mean we now have two event queue
writers, xenfb_update_screen() and xenfb_set_par(). Very simple 2
writer one reader problem easily solved by the addition of the queue_lock.

Previously I handled the resize in xenfb_thread(). This could lead to us missing a resize and or using the incorrect values on a multi processor system.

IE: Wrong resolution
P1:1 set_par sets resize_dpy w, h already in fb struct
P2:1 thread wakeup sees resize_dpy set
P2:2 do_resize()
P2:3 read fb width
P1:3 check_var/set_par changes fb height
P2:4 read fb height (2nd value here)

IE: Missed resolution change
P1:1 set_par sets resize_dpy w, h already in fb struct
P2:1 thread wakeup sees resize_dpy set
P2:2 do_resize()
P2:3 read fb width
P2:4 read fb height
P1:3 set_par sets resize_dpy
P2:5 clears resize_dpy (Missed the second resize)

Quoted 7 lines

Adding in a queue lock should be safe because:
1. xenfb_update_screen() can hold a mutex without interfering with
zap_page_range
2. xenfb_set_par() will timeout releasing the lock so update can run

I have verified that the lock works as expected and times out if necessary.

Your race is real.

Your old code shared the resolution state (info->resize_dpy and the resolution variables in info->fb_info) between xenfb_do_resize() (running in xenfb_thread) and xenfb_set_par() (running in another thread).

Your new code shares the ring buffer instead.

Both methods can be made race-free with suitable synchronization.

With your old code, xenfb_set_par() always succeeds. Until the backend gets around to process the XENFB_TYPE_RESIZE message, it interprets the frame buffer for the old resolution. As you argue below, this isn't fatal, it can just mess up the display somewhat.

With your new code, xenfb_set_par() can take a long time (one second), and it can fail, leaving the display in a deranged state until the next resolution change. It still doesn't fully synchronize with the
backend: it returns successfully after sending the message, leaving the time window until the backend receives the message open.

I'd prefer the old behavior. But I could be missing something here. Am I?

I think you could fix the old code by protecting access to the shared resolution state by a spin lock.

If I am missing something, and your new code is the way to go, you still need to migrate your explanation why it works from e-mail to source file, where the poor maintainer can find it.

[...]

Looks like this is the last issue that you haven't addressed / explained fully yet :)

Quoted 27 lines · Markus Armbruster

Quoted 24 lines · Pat Campbell

Quoted 22 lines · Markus Armbruster

Quoted 11 lines · Pat Campbell

Attached patch adds dynamic frame buffer resolution support to the PV xenfb frame buffer driver.

Corresponding backend IOEMU patch is required for functionality but this patch is not dependent on it, preserving backwards compatibility.

Please apply to tip of linux-2.6.18-xen

Signed-off-by Pat Campbell <[email protected]>

Adding another lock (queue_lock) to our already ticklish locking gives me a queasy feeling...

The purpose of the lock is not obvious to me. Please explain that, in the code. Likewise, it's not entirely obvious that the locking works. Please update the "How the locks work together" comment.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

But before you do that, I suggest you tell us exactly what problem you're attempting to solve with queue_lock. Maybe we can come up with a less scary solution. Maybe not. But it's worth a try. If it's just to ensure the changes made by xenfb_set_par() are seen in xenfb_do_resize() correctly, a memory barrier should to the trick more easily.

Quoted 17 lines

Your race is real.

Your old code shared the resolution state (info->resize_dpy and the resolution variables in info->fb_info) between xenfb_do_resize() (running in xenfb_thread) and xenfb_set_par() (running in another thread).

Your new code shares the ring buffer instead.

Both methods can be made race-free with suitable synchronization.

With your old code, xenfb_set_par() always succeeds. Until the backend gets around to process the XENFB_TYPE_RESIZE message, it interprets the frame buffer for the old resolution. As you argue below, this isn't fatal, it can just mess up the display somewhat.

Quoted 6 lines

With your new code, xenfb_set_par() can take a long time (one second), and it can fail, leaving the display in a deranged state until the next resolution change. It still doesn't fully synchronize with the
backend: it returns successfully after sending the message, leaving the time window until the backend receives the message open.

Synchronizing with the backend? I don't think this is necessary. The update events that follow the resize event will be for the new size, as long as the backend gets the resize event we should be ok.

Quoted 4 lines

I'd prefer the old behavior. But I could be missing something here. Am I?

I like the new behavior, it seems more straight forward and does not rely on the xenfb_thread() being active. Our first set_par() happens during frame buffer registration which is before xenfb_thread() is
started. I disliked adding new code into the xenfb_update() function
for an event that happens rarely so will revert back to the sharing of resize_dpy flag code.

Is this how you envision xenfb_set_par() synchronization?

static int xenfb_set_par(struct fb_info *info)

{
    struct xenfb_info *xenfb_info;
    unsigned long flags;

    xenfb_info = info->par;

    if (xenfb_info->kthread) {
        spin_lock_irqsave(&xenfb_info->resize_lock, flags);
        info->var.xres_virtual = info->var.xres;
        info->var.yres_virtual = info->var.yres;
        xenfb_info->resize_dpy = 1;
        /* Wake up xenfb_thread() */
        xenfb_refresh(xenfb_info, 0, 0, 1, 1);
        while (xenfb_info->kthread && xenfb_info->resize_dpy == 1) {
            msleep(10);
        }
        spin_unlock_irqrestore(&xenfb_info->resize_lock, flags);
    }
    return 0;
}

To explain the code a liitle bit.
1. Need to test for kthread because first xenfb_set_par() happens
before xenfb_thread() is started

2. Need to wakeup xenfb_thread via refresh as application screen events
are blocked waiting on the set_par to complete. Can't just set dirty, will get a bogus nasty gram.

3. Testing xenfb_thread in the while loop in case xenfb_remove() is
called and thread is shutdown. Protects against a system shutdown hang. Don't know if that can happen, defensive code.

-------------------
New lock comment

/*
* There are three locks: spinlock resize_lock protecting resize_dpy,

* spinlock dirty_lock protecting the dirty rectangle and mutex
* mm_lock protecting mappings.
*
* resize_lock is used to synchronize resize_dpy access between
* xenfb_set_par() and xenfb_do_resize() running in xenfb_thread(). *
* How the dirty and mapping locks work together
*
..........

Quoted 13 lines

I think you could fix the old code by protecting access to the shared resolution state by a spin lock.

If I am missing something, and your new code is the way to go, you still need to migrate your explanation why it works from e-mail to source file, where the poor maintainer can find it.

[...]

Looks like this is the last issue that you haven't addressed / explained fully yet :)

Yahoo, getting close. Thanks

Just an FYI, I am at Brainshare this week so my responses might be a little slow but I will try to turn around any comments I receive as soon as possible. Like to put this to bed before any more staging changes take place.

Quoted 33 lines · Pat Campbell

Quoted 31 lines · Markus Armbruster

Quoted 27 lines · Pat Campbell

Quoted 24 lines · Markus Armbruster

Quoted 12 lines · Pat Campbell

Attached patch adds dynamic frame buffer resolution support to the PV xenfb frame buffer driver.

Corresponding backend IOEMU patch is required for functionality but this patch is not dependent on it, preserving backwards compatibility.

Please apply to tip of linux-2.6.18-xen

Signed-off-by Pat Campbell <[email protected]>

Adding another lock (queue_lock) to our already ticklish locking gives me a queasy feeling...

The purpose of the lock is not obvious to me. Please explain that, in the code. Likewise, it's not entirely obvious that the locking works. Please update the "How the locks work together" comment.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

But before you do that, I suggest you tell us exactly what problem you're attempting to solve with queue_lock. Maybe we can come up with a less scary solution. Maybe not. But it's worth a try. If it's just to ensure the changes made by xenfb_set_par() are seen in xenfb_do_resize() correctly, a memory barrier should to the trick more easily.

[Pat explains the race...]

Quoted 23 lines

Quoted 16 lines

Your race is real.

Your old code shared the resolution state (info->resize_dpy and the resolution variables in info->fb_info) between xenfb_do_resize() (running in xenfb_thread) and xenfb_set_par() (running in another thread).

Your new code shares the ring buffer instead.

Both methods can be made race-free with suitable synchronization.

With your old code, xenfb_set_par() always succeeds. Until the backend gets around to process the XENFB_TYPE_RESIZE message, it interprets the frame buffer for the old resolution. As you argue below, this isn't fatal, it can just mess up the display somewhat.

Quoted 7 lines

With your new code, xenfb_set_par() can take a long time (one second), and it can fail, leaving the display in a deranged state until the next resolution change. It still doesn't fully synchronize with the
backend: it returns successfully after sending the message, leaving the time window until the backend receives the message open.

Quoted 4 lines

Synchronizing with the backend? I don't think this is necessary. The update events that follow the resize event will be for the new size, as long as the backend gets the resize event we should be ok.

I didn't mean to say that synchronizing with the backend is necessary. I just wanted to point out that your new method pays the price of synchronization, namely a possibly significant delay in xenfb_set_par(), plus an ugly failure mode there, without actually achieving synchronization.

Quoted 36 lines

Quoted 5 lines

I'd prefer the old behavior. But I could be missing something here. Am I?

I like the new behavior, it seems more straight forward and does not rely on the xenfb_thread() being active. Our first set_par() happens during frame buffer registration which is before xenfb_thread() is
started. I disliked adding new code into the xenfb_update() function
for an event that happens rarely so will revert back to the sharing of resize_dpy flag code.

Is this how you envision xenfb_set_par() synchronization?

static int xenfb_set_par(struct fb_info *info)

{
    struct xenfb_info *xenfb_info;
    unsigned long flags;

    xenfb_info = info->par;

    if (xenfb_info->kthread) {
        spin_lock_irqsave(&xenfb_info->resize_lock, flags);
        info->var.xres_virtual = info->var.xres;
        info->var.yres_virtual = info->var.yres;
        xenfb_info->resize_dpy = 1;
        /* Wake up xenfb_thread() */
        xenfb_refresh(xenfb_info, 0, 0, 1, 1);
        while (xenfb_info->kthread && xenfb_info->resize_dpy == 1) {
            msleep(10);
        }
        spin_unlock_irqrestore(&xenfb_info->resize_lock, flags);
    }
    return 0;
}

Quoted 4 lines

To explain the code a liitle bit.
1. Need to test for kthread because first xenfb_set_par() happens
before xenfb_thread() is started

Why can you just ignore the mode change then?

Quoted 4 lines

2. Need to wakeup xenfb_thread via refresh as application screen events
are blocked waiting on the set_par to complete. Can't just set dirty, will get a bogus nasty gram.

I'd pass an empty rectangle there:

        xenfb_refresh(xenfb_info, INT_MAX, INT_MAX, 0, 0);

Alternatively, factor out the code to wake up the thread into a new function, and call that here and from __xenfb_refresh(). That would be cleaner.

Quoted 4 lines

3. Testing xenfb_thread in the while loop in case xenfb_remove() is
called and thread is shutdown. Protects against a system shutdown hang. Don't know if that can happen, defensive code.

Sleeps while holding a spinlock. Not good. And I didn't get the hang you're trying to defend against.

xenfb_resize_screen() still accesses the size unsynchronized, and can thus see intermediate states of resolution changes.

No, that's not what I had in mind. Let me sketch it.

The first question to answer for a mutex is: what shared state does it protect? resize_lock protects the screen size fb_info->var.xres, fb_info->var.yres and the flag resize_dpy.

Once that's clear, the use of the mutex becomes obvious: wrap it around any access of the shared state, i.e. the updating of the shared state it protects in xenfb_set_par() and the reading in xenfb_thread(). Code sketch:

static void xenfb_resize_screen(struct xenfb_info *info)

{
	if (xenfb_queue_full(info))
		return;

        /* caller holds info->resize_lock */
	info->resize_dpy = 0;
	xenfb_do_resize(info);
}

static int xenfb_thread(void *data)

{
	struct xenfb_info *info = data;
	unsigned long flags;

	while (!kthread_should_stop()) {
		spin_lock_irqsave(info->resize_lock, flags);
		if (info->resize_dpy)
			xenfb_resize_screen(info);
		spin_unlock_irqrestore(info->resize_lock, flags);

[...]
}
[...]
static int xenfb_set_par(struct fb_info *info)

{
	struct xenfb_info *xenfb_info = info->par;
	unsigned long flags;

	spin_lock_irqsave(info->resize_lock, flags);
	info->var.xres_virtual = info->var.xres;
	info->var.yres_virtual = info->var.yres;
	xenfb_info->resize_dpy = 1;
	spin_unlock_irqrestore(info->resize_lock, flags);

	if (xenfb_info->kthread) {
		/* Wake up xenfb_thread() */
		xenfb_refresh(xenfb_info, INT_MAX, INT_MAX, 0, 0);
	}
	return 0;
}

Note that xenfb_set_par() can schedule resolution changes just fine before xenfb_thread() runs. It'll pick them up right when it starts.

I used xenfb_refresh() to wake up xenfb_thread() only to keep things simple, not to express a preference for that method.

Don't just copy my code sketch! Review it carefully, please.

Quoted 6 lines

-------------------
New lock comment

/*
* There are three locks: spinlock resize_lock protecting resize_dpy,

It actually protects the screen size and resize_dpy.

Quoted 26 lines

* spinlock dirty_lock protecting the dirty rectangle and mutex
* mm_lock protecting mappings.
*
* resize_lock is used to synchronize resize_dpy access between
* xenfb_set_par() and xenfb_do_resize() running in xenfb_thread(). *
* How the dirty and mapping locks work together
*
..........

Quoted 14 lines

I think you could fix the old code by protecting access to the shared resolution state by a spin lock.

If I am missing something, and your new code is the way to go, you still need to migrate your explanation why it works from e-mail to source file, where the poor maintainer can find it.

[...]

Looks like this is the last issue that you haven't addressed / explained fully yet :)

Quoted 7 lines

Yahoo, getting close. Thanks

Just an FYI, I am at Brainshare this week so my responses might be a little slow but I will try to turn around any comments I receive as soon as possible. Like to put this to bed before any more staging changes take place.

Me too! And thanks for pushing this feature all the way.

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Quoted 19 lines · Markus Armbruster

Quoted 16 lines · Pat Campbell

Quoted 14 lines · Markus Armbruster

Quoted 10 lines · Pat Campbell

Quoted 7 lines · Markus Armbruster

Quoted 2 lines · Pat Campbell

Attached patch adds dynamic frame buffer resolution support to the PV xenfb frame buffer driver.

I have attached front and back for your review. Back has not changed except to include opengl changes. Front has synchronization fix. I added a spin lock and struct xenfb_resize into struct xenfb. struct xenfb_resize was added to protect the screen size values from changes outside the driver. IE User application calls to ioctl(fb, FBIOPUT_VSCREENINFO, &fb_var); while the driver is processing a previous call.

diff -r de57c3f218fb drivers/xen/fbfront/xenfb.c

drivers/xen/fbfront/xenfb.c +4 -0

@@ -62,15 +62,21 @@ struct xenfb_info
 	struct xenfb_page	*page;
 	unsigned long 		*mfns;
 	int			update_wanted; /* XENFB_TYPE_UPDATE wanted */
+	int			feature_resize; /* Backend has resize feature */
+	struct xenfb_resize	resize;
+	int			resize_dpy;
+	spinlock_t		resize_lock;
 

drivers/xen/fbfront/xenfb.c +4 -0

+ *    spinlock dirty_lock protecting the dirty rectangle
+ *    mutex mm_lock protecting mappings.
+ *
+ * How the dirty and mapping locks work together
  *
  * The problem is that dirty rectangle and mappings aren't
  * independent: the dirty rectangle must cover all faulted pages in

drivers/xen/fbfront/xenfb.c +2 -7

@@ -150,14 +177,22 @@ static void xenfb_do_update(struct xenfb
 	event.update.width = w;
 	event.update.height = h;
 
-	prod = info->page->out_prod;
 	/* caller ensures !xenfb_queue_full() */
-	mb();			/* ensure ring space available */
-	XENFB_OUT_RING_REF(info->page, prod) = event;
-	wmb();			/* ensure ring contents visible */
-	info->page->out_prod = prod + 1;
-
-	notify_remote_via_irq(info->irq);
+	xenfb_send_event(info, &event);
+}

drivers/xen/fbfront/xenfb.c +47 -0

+
+	spin_lock_irqsave(&info->resize_lock, flags);
+	if (info->resize_dpy) {
+		if (!xenfb_queue_full(info)) {
+			info->resize_dpy = 0;
+			xenfb_do_resize(info);
+		}
+	}
+	spin_unlock_irqrestore(&info->resize_lock, flags);
+}
+
 static int xenfb_thread(void *data)
 {
 	struct xenfb_info *info = data;
 
 	while (!kthread_should_stop()) {
+		xenfb_handle_resize_dpy(info);
 		if (info->dirty) {
 			info->dirty = 0;
 			xenfb_update_screen(info);

@@ -413,6 +463,55 @@ static int xenfb_mmap(struct fb_info *fb
 	return 0;
 }
 
+static int
+xenfb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
+{
+	struct xenfb_info *xenfb_info;
+	int required_mem_len;
+
+	xenfb_info = info->par;
+
+	if (!xenfb_info->feature_resize) {
+		if (var->xres == video[KPARAM_WIDTH] &&
+			var->yres == video[KPARAM_HEIGHT] &&
+			var->bits_per_pixel == xenfb_info->page->depth) {
+			return 0;
+		}
+		return -EINVAL;
+	}
+
+	/* Can't resize past initial width and height */
+	if (var->xres > video[KPARAM_WIDTH] || var->yres > video[KPARAM_HEIGHT])
+		return -EINVAL;
+
+	required_mem_len = var->xres * var->yres * (xenfb_info->page->depth / 8);
+	if (var->bits_per_pixel == xenfb_info->page->depth &&
+		var->xres <= info->fix.line_length / (XENFB_DEPTH / 8) &&
+		required_mem_len <= info->fix.smem_len) {
+		var->xres_virtual = var->xres;
+		var->yres_virtual = var->yres;
+		return 0;
+	}
+	return -EINVAL;
+}
+
+static int xenfb_set_par(struct fb_info *info)
+{
+	struct xenfb_info *xenfb_info;

drivers/xen/fbfront/xenfb.c +4 -0

+	spin_unlock_irqrestore(&xenfb_info->resize_lock, flags);
+	return 0;
+}
+
 static struct fb_ops xenfb_fb_ops = {
 	.owner		= THIS_MODULE,
 	.fb_setcolreg	= xenfb_setcolreg,

Looks good! One easily fixed bug and a few remarks inline.

Hello,

Quoted 1 line · Markus Armbruster, le Tue 25 Mar 2008 15:43:28 +0100, a écrit :

Looks good! One easily fixed bug and a few remarks inline.

Cool!

Since there were no remarks on the backend part, could it be commited? We need it for the stubdomains.

Samuel