* [PATCH][LINUX] Dynamic modes support for xenfb (2 of 2)
@ 2008-03-13 19:59 Pat Campbell
2008-03-14 16:41 ` Markus Armbruster
0 siblings, 1 reply; 12+ messages in thread
From: Pat Campbell @ 2008-03-13 19:59 UTC (permalink / raw)
To: xen-devel
[-- Attachment #1: Type: text/plain, Size: 325 bytes --]
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 <plc@novell.com>
[-- Attachment #2: fbfront-resize.patch --]
[-- Type: text/x-patch, Size: 13335 bytes --]
diff -r 3983b041fc51 drivers/xen/fbfront/xenfb.c
--- a/drivers/xen/fbfront/xenfb.c Mon Mar 10 22:53:07 2008 +0000
+++ b/drivers/xen/fbfront/xenfb.c Thu Mar 13 12:57:30 2008 -0600
@@ -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;
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);
+
+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)
@@ -177,8 +211,12 @@ static void xenfb_update_screen(struct x
if (!info->update_wanted)
return;
- if (xenfb_queue_full(info))
+
+ mutex_lock(&info->queue_lock);
+ 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);
+ 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);
+ }
+ mutex_unlock(&xenfb_info->queue_lock);
+ return 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,
@@ -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;
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;
@@ -533,6 +644,9 @@ static int __devinit xenfb_probe(struct
xenbus_dev_fatal(dev, ret, "fb_alloc_cmap");
goto error;
}
+
+ /* init shared page, uses fb_info attributes */
+ xenfb_init_shared_page(info, fb_info);
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
--- a/drivers/xen/fbfront/xenkbd.c Mon Mar 10 22:53:07 2008 +0000
+++ b/drivers/xen/fbfront/xenkbd.c Thu Mar 13 08:26:22 2008 -0600
@@ -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 )
+ input_set_abs_params(info->ptr, ABS_Y, 0, val, 0, 0);
+
break;
case XenbusStateClosing:
diff -r 3983b041fc51 include/xen/interface/io/fbif.h
--- a/include/xen/interface/io/fbif.h Mon Mar 10 22:53:07 2008 +0000
+++ b/include/xen/interface/io/fbif.h Tue Mar 11 17:39:38 2008 -0600
@@ -50,12 +50,28 @@ struct xenfb_update
int32_t height; /* rect height */
};
+/*
+ * Framebuffer resize notification event
+ * Capable backend sets feature-resize in xenstore.
+ */
+#define XENFB_TYPE_RESIZE 3
+
+struct xenfb_resize
+{
+ uint8_t type; /* XENFB_TYPE_RESIZE */
+ int32_t width; /* width in pixels */
+ int32_t height; /* height in pixels */
+ int32_t stride; /* stride in bytes */
+ int32_t depth; /* depth in bits */
+};
+
#define XENFB_OUT_EVENT_SIZE 40
union xenfb_out_event
{
uint8_t type;
struct xenfb_update update;
+ struct xenfb_resize resize;
char pad[XENFB_OUT_EVENT_SIZE];
};
@@ -109,15 +125,17 @@ struct xenfb_page
* Each directory page holds PAGE_SIZE / sizeof(*pd)
* framebuffer pages, and can thus map up to PAGE_SIZE *
* PAGE_SIZE / sizeof(*pd) bytes. With PAGE_SIZE == 4096 and
- * sizeof(unsigned long) == 4, that's 4 Megs. Two directory
- * pages should be enough for a while.
+ * sizeof(unsigned long) == 4/8, that's 4 Megs 32 bit and 2 Megs
+ * 64 bit. 256 directories give enough room for a 512 Meg
+ * framebuffer with a max resolution of 12,800x10,240. Should
+ * be enough for a while with room leftover for expansion.
*/
- unsigned long pd[2];
+ unsigned long pd[256];
};
/*
- * Wart: xenkbd needs to know resolution. Put it here until a better
- * solution is found, but don't leak it to the backend.
+ * Wart: xenkbd needs to know default resolution. Put it here until a
+ * better solution is found, but don't leak it to the backend.
*/
#ifdef __KERNEL__
#define XENFB_WIDTH 800
[-- Attachment #3: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH][LINUX] Dynamic modes support for xenfb (2 of 2)
2008-03-13 19:59 [PATCH][LINUX] Dynamic modes support for xenfb (2 of 2) Pat Campbell
@ 2008-03-14 16:41 ` Markus Armbruster
2008-03-16 19:09 ` Pat Campbell
0 siblings, 1 reply; 12+ messages in thread
From: Markus Armbruster @ 2008-03-14 16:41 UTC (permalink / raw)
To: Pat Campbell; +Cc: xen-devel
Pat Campbell <plc@novell.com> writes:
> 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 <plc@novell.com>
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.
> diff -r 3983b041fc51 drivers/xen/fbfront/xenfb.c
> --- a/drivers/xen/fbfront/xenfb.c Mon Mar 10 22:53:07 2008 +0000
> +++ b/drivers/xen/fbfront/xenfb.c Thu Mar 13 12:57:30 2008 -0600
> @@ -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;
>
> 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?
> +
> +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)
> @@ -177,8 +211,12 @@ static void xenfb_update_screen(struct x
>
> 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.
> + 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);
> + 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
> + 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?
> +
> 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,
> @@ -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?
> 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;
> @@ -533,6 +644,9 @@ static int __devinit xenfb_probe(struct
> xenbus_dev_fatal(dev, ret, "fb_alloc_cmap");
> goto error;
> }
> +
> + /* init shared page, uses fb_info attributes */
> + xenfb_init_shared_page(info, fb_info);
The comment is somewhat redundant.
>
> 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
> --- a/drivers/xen/fbfront/xenkbd.c Mon Mar 10 22:53:07 2008 +0000
> +++ b/drivers/xen/fbfront/xenkbd.c Thu Mar 13 08:26:22 2008 -0600
> @@ -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 )
> + 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?
>
> case XenbusStateClosing:
> diff -r 3983b041fc51 include/xen/interface/io/fbif.h
> --- a/include/xen/interface/io/fbif.h Mon Mar 10 22:53:07 2008 +0000
> +++ b/include/xen/interface/io/fbif.h Tue Mar 11 17:39:38 2008 -0600
> @@ -50,12 +50,28 @@ struct xenfb_update
> int32_t height; /* rect height */
> };
>
> +/*
> + * Framebuffer resize notification event
> + * Capable backend sets feature-resize in xenstore.
> + */
> +#define XENFB_TYPE_RESIZE 3
> +
> +struct xenfb_resize
> +{
> + uint8_t type; /* XENFB_TYPE_RESIZE */
> + int32_t width; /* width in pixels */
> + int32_t height; /* height in pixels */
> + int32_t stride; /* stride in bytes */
> + int32_t depth; /* depth in bits */
> +};
> +
> #define XENFB_OUT_EVENT_SIZE 40
>
> union xenfb_out_event
> {
> uint8_t type;
> struct xenfb_update update;
> + struct xenfb_resize resize;
> char pad[XENFB_OUT_EVENT_SIZE];
> };
>
> @@ -109,15 +125,17 @@ struct xenfb_page
> * Each directory page holds PAGE_SIZE / sizeof(*pd)
> * framebuffer pages, and can thus map up to PAGE_SIZE *
> * PAGE_SIZE / sizeof(*pd) bytes. With PAGE_SIZE == 4096 and
> - * sizeof(unsigned long) == 4, that's 4 Megs. Two directory
> - * pages should be enough for a while.
> + * sizeof(unsigned long) == 4/8, that's 4 Megs 32 bit and 2 Megs
> + * 64 bit. 256 directories give enough room for a 512 Meg
> + * framebuffer with a max resolution of 12,800x10,240. Should
> + * be enough for a while with room leftover for expansion.
> */
> - unsigned long pd[2];
> + unsigned long pd[256];
> };
>
> /*
> - * Wart: xenkbd needs to know resolution. Put it here until a better
> - * solution is found, but don't leak it to the backend.
> + * Wart: xenkbd needs to know default resolution. Put it here until a
> + * better solution is found, but don't leak it to the backend.
> */
> #ifdef __KERNEL__
> #define XENFB_WIDTH 800
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH][LINUX] Dynamic modes support for xenfb (2 of 2)
2008-03-14 16:41 ` Markus Armbruster
@ 2008-03-16 19:09 ` Pat Campbell
2008-03-17 17:35 ` Markus Armbruster
0 siblings, 1 reply; 12+ messages in thread
From: Pat Campbell @ 2008-03-16 19:09 UTC (permalink / raw)
To: Markus Armbruster; +Cc: xen-devel
Markus Armbruster wrote:
> Pat Campbell <plc@novell.com> writes:
>
>
>> 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 <plc@novell.com>
>>
>
> 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.
>
>
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.
>> diff -r 3983b041fc51 drivers/xen/fbfront/xenfb.c
>> --- a/drivers/xen/fbfront/xenfb.c Mon Mar 10 22:53:07 2008 +0000
>> +++ b/drivers/xen/fbfront/xenfb.c Thu Mar 13 12:57:30 2008 -0600
>> @@ -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;
>>
>> 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?
>
No, not anymore.
>
>> +
>> +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)
>> @@ -177,8 +211,12 @@ static void xenfb_update_screen(struct x
>>
>> 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.
>
No, not indefinitely. xenfb_set_par(), the only other holder, times out
in one second if the queue does not drain enough to allow the addition
of a resize event into the queue. I think a good case could be made for
something drastically wrong in the backend if we ever hit the timeout.
>
>> + 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);
>> + 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
>
>
>> + mutex_unlock(&xenfb_info->queue_lock);
>> + return 0;
>> +}
>>
>
> Does the framebuffer code handle the fb_set_par() callback doing
> nothing?
>
As far as the frame buffer is concerned the resolution has been changed
and it goes merrily on it's way. VNC server, backend, will still
happily believe the screen is at the old resolution. Worse case the
user sees either to much or not enough screen. Hanging here would
result in an unusable display. This is really no different than a real
video controller that could not adjust to the new setting leaving the
hardware settings at the current values. As long as memory bounds are
not violated everything still works. xenfb memory bounds can not be
violated as the max is allocated at startup and never reduced.
> Even if it does: is silently failing acceptable?
>
I will add an error message.
>
>> +
>> 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,
>> @@ -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?
>
Removed initialization.
>
>> 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;
>> @@ -533,6 +644,9 @@ static int __devinit xenfb_probe(struct
>> xenbus_dev_fatal(dev, ret, "fb_alloc_cmap");
>> goto error;
>> }
>> +
>> + /* init shared page, uses fb_info attributes */
>> + xenfb_init_shared_page(info, fb_info);
>>
>
> The comment is somewhat redundant.
>
Ok
>
>>
>> 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
>> --- a/drivers/xen/fbfront/xenkbd.c Mon Mar 10 22:53:07 2008 +0000
>> +++ b/drivers/xen/fbfront/xenkbd.c Thu Mar 13 08:26:22 2008 -0600
>> @@ -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 )
>> + 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?
>
We are dynamically resizing the portal into the video memory, not the
video memory itself. The abs values are essentially static once the
frame buffer has been allocated and only need to be set to their true
value once.
>
>>
>> case XenbusStateClosing:
>> diff -r 3983b041fc51 include/xen/interface/io/fbif.h
>> --- a/include/xen/interface/io/fbif.h Mon Mar 10 22:53:07 2008 +0000
>> +++ b/include/xen/interface/io/fbif.h Tue Mar 11 17:39:38 2008 -0600
>> @@ -50,12 +50,28 @@ struct xenfb_update
>> int32_t height; /* rect height */
>> };
>>
>> +/*
>> + * Framebuffer resize notification event
>> + * Capable backend sets feature-resize in xenstore.
>> + */
>> +#define XENFB_TYPE_RESIZE 3
>> +
>> +struct xenfb_resize
>> +{
>> + uint8_t type; /* XENFB_TYPE_RESIZE */
>> + int32_t width; /* width in pixels */
>> + int32_t height; /* height in pixels */
>> + int32_t stride; /* stride in bytes */
>> + int32_t depth; /* depth in bits */
>> +};
>> +
>> #define XENFB_OUT_EVENT_SIZE 40
>>
>> union xenfb_out_event
>> {
>> uint8_t type;
>> struct xenfb_update update;
>> + struct xenfb_resize resize;
>> char pad[XENFB_OUT_EVENT_SIZE];
>> };
>>
>> @@ -109,15 +125,17 @@ struct xenfb_page
>> * Each directory page holds PAGE_SIZE / sizeof(*pd)
>> * framebuffer pages, and can thus map up to PAGE_SIZE *
>> * PAGE_SIZE / sizeof(*pd) bytes. With PAGE_SIZE == 4096 and
>> - * sizeof(unsigned long) == 4, that's 4 Megs. Two directory
>> - * pages should be enough for a while.
>> + * sizeof(unsigned long) == 4/8, that's 4 Megs 32 bit and 2 Megs
>> + * 64 bit. 256 directories give enough room for a 512 Meg
>> + * framebuffer with a max resolution of 12,800x10,240. Should
>> + * be enough for a while with room leftover for expansion.
>> */
>> - unsigned long pd[2];
>> + unsigned long pd[256];
>> };
>>
>> /*
>> - * Wart: xenkbd needs to know resolution. Put it here until a better
>> - * solution is found, but don't leak it to the backend.
>> + * Wart: xenkbd needs to know default resolution. Put it here until a
>> + * better solution is found, but don't leak it to the backend.
>> */
>> #ifdef __KERNEL__
>> #define XENFB_WIDTH 800
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH][LINUX] Dynamic modes support for xenfb (2 of 2)
2008-03-16 19:09 ` Pat Campbell
@ 2008-03-17 17:35 ` Markus Armbruster
2008-03-18 19:11 ` Pat Campbell
0 siblings, 1 reply; 12+ messages in thread
From: Markus Armbruster @ 2008-03-17 17:35 UTC (permalink / raw)
To: Pat Campbell; +Cc: xen-devel
Pat Campbell <plc@novell.com> writes:
> Markus Armbruster wrote:
>> Pat Campbell <plc@novell.com> writes:
>>
>>
>>> 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 <plc@novell.com>
>>>
>>
>> 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.
>>
>>
> 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.
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 :)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH][LINUX] Dynamic modes support for xenfb (2 of 2)
2008-03-17 17:35 ` Markus Armbruster
@ 2008-03-18 19:11 ` Pat Campbell
2008-03-19 10:21 ` Markus Armbruster
0 siblings, 1 reply; 12+ messages in thread
From: Pat Campbell @ 2008-03-18 19:11 UTC (permalink / raw)
To: Markus Armbruster; +Cc: xen-devel
Markus Armbruster wrote:
> Pat Campbell <plc@novell.com> writes:
>
>
>> Markus Armbruster wrote:
>>
>>> Pat Campbell <plc@novell.com> writes:
>>>
>>>
>>>
>>>> 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 <plc@novell.com>
>>>>
>>>>
>>> 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.
>>>
>>>
>>>
>> 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.
>>
>
> 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.
>
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'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
*
..........
> 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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH][LINUX] Dynamic modes support for xenfb (2 of 2)
2008-03-18 19:11 ` Pat Campbell
@ 2008-03-19 10:21 ` Markus Armbruster
2008-03-24 3:40 ` Pat Campbell
0 siblings, 1 reply; 12+ messages in thread
From: Markus Armbruster @ 2008-03-19 10:21 UTC (permalink / raw)
To: Pat Campbell; +Cc: xen-devel
Pat Campbell <plc@novell.com> writes:
> Markus Armbruster wrote:
>> Pat Campbell <plc@novell.com> writes:
>>
>>
>>> Markus Armbruster wrote:
>>>
>>>> Pat Campbell <plc@novell.com> writes:
>>>>
>>>>
>>>>
>>>>> 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 <plc@novell.com>
>>>>>
>>>>>
>>>> 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.
>>>>
[Pat explains the race...]
>>
>> 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.
>>
> 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.
>> 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
Why can you just ignore the mode change then?
> 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.
> 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.
>
>
> -------------------
> New lock comment
>
> /*
> * There are three locks: spinlock resize_lock protecting resize_dpy,
It actually protects the screen size and 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
> *
> ..........
>
>
>> 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.
Me too! And thanks for pushing this feature all the way.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH][LINUX] Dynamic modes support for xenfb (2 of 2)
2008-03-19 10:21 ` Markus Armbruster
@ 2008-03-24 3:40 ` Pat Campbell
2008-03-25 14:43 ` Markus Armbruster
0 siblings, 1 reply; 12+ messages in thread
From: Pat Campbell @ 2008-03-24 3:40 UTC (permalink / raw)
To: Markus Armbruster; +Cc: xen-devel
[-- Attachment #1: Type: text/plain, Size: 9208 bytes --]
Markus Armbruster wrote:
> Pat Campbell <plc@novell.com> writes:
>
>
>> Markus Armbruster wrote:
>>
>>> Pat Campbell <plc@novell.com> writes:
>>>
>>>
>>>
>>>> Markus Armbruster wrote:
>>>>
>>>>
>>>>> Pat Campbell <plc@novell.com> writes:
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>> 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 <plc@novell.com>
>>>>>>
>>>>>>
>>>>>>
>>>>> 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.
>>>>>
>>>>>
> [Pat explains the race...]
>
>>> 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.
>>>
>>>
>> 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.
>
>
>>> 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
>>
>
> Why can you just ignore the mode change then?
>
>
>> 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.
>
>
>> 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.
>
>
>> -------------------
>> New lock comment
>>
>> /*
>> * There are three locks: spinlock resize_lock protecting resize_dpy,
>>
>
> It actually protects the screen size and 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
>> *
>> ..........
>>
>>
>>
>>> 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.
>>
>
> Me too! And thanks for pushing this feature all the way.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>
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.
[-- Attachment #2: fbback-resize.patch --]
[-- Type: text/x-patch, Size: 6210 bytes --]
diff -r 76c9cf11ce23 tools/ioemu/hw/xenfb.c
--- a/tools/ioemu/hw/xenfb.c Fri Mar 21 09:45:34 2008 +0000
+++ b/tools/ioemu/hw/xenfb.c Sun Mar 23 19:38:17 2008 -0600
@@ -516,6 +516,16 @@ static void xenfb_on_fb_event(struct xen
}
xenfb_guest_copy(xenfb, x, y, w, h);
break;
+ case XENFB_TYPE_RESIZE:
+ xenfb->width = event->resize.width;
+ xenfb->height = event->resize.height;
+ xenfb->row_stride = event->resize.stride;
+ dpy_colourdepth(xenfb->ds, xenfb->depth);
+ dpy_resize(xenfb->ds, xenfb->width, xenfb->height, xenfb->row_stride);
+ if (xenfb->ds->shared_buf)
+ dpy_setdata(xenfb->ds, xenfb->pixels);
+ xenfb_invalidate(xenfb);
+ break;
}
}
xen_mb(); /* ensure we're done with ring contents */
@@ -680,6 +690,7 @@ static int xenfb_read_frontend_fb_config
static int xenfb_read_frontend_fb_config(struct xenfb *xenfb) {
struct xenfb_page *fb_page;
int val;
+ int videoram;
if (xenfb_xs_scanf1(xenfb->xsh, xenfb->fb.otherend, "feature-update",
"%d", &val) < 0)
@@ -702,10 +713,30 @@ static int xenfb_read_frontend_fb_config
/* TODO check for consistency with the above */
xenfb->fb_len = fb_page->mem_length;
xenfb->row_stride = fb_page->line_length;
+
+ /* Protect against hostile frontend, limit fb_len to max allowed */
+ if (xenfb_xs_scanf1(xenfb->xsh, xenfb->fb.nodename, "videoram", "%d",
+ &videoram) < 0)
+ videoram = 0;
+ videoram = videoram * 1024 * 1024;
+ if (videoram && xenfb->fb_len > videoram) {
+ fprintf(stderr, "Framebuffer requested length of %zd exceeded allowed %d\n",
+ xenfb->fb_len, videoram);
+ xenfb->fb_len = videoram;
+ if (xenfb->row_stride * xenfb->height > xenfb->fb_len)
+ xenfb->height = xenfb->fb_len / xenfb->row_stride;
+ }
fprintf(stderr, "Framebuffer depth %d width %d height %d line %d\n",
fb_page->depth, fb_page->width, fb_page->height, fb_page->line_length);
if (xenfb_map_fb(xenfb, xenfb->fb.otherend_id) < 0)
return -1;
+
+ /* Indicate we have the frame buffer resize feature */
+ xenfb_xs_printf(xenfb->xsh, xenfb->fb.nodename, "feature-resize", "1");
+
+ /* Tell kbd pointer the screen geometry */
+ xenfb_xs_printf(xenfb->xsh, xenfb->kbd.nodename, "width", "%d", xenfb->width);
+ xenfb_xs_printf(xenfb->xsh, xenfb->kbd.nodename, "height", "%d", xenfb->height);
if (xenfb_switch_state(&xenfb->fb, XenbusStateConnected))
return -1;
diff -r 76c9cf11ce23 tools/python/xen/xend/server/vfbif.py
--- a/tools/python/xen/xend/server/vfbif.py Fri Mar 21 09:45:34 2008 +0000
+++ b/tools/python/xen/xend/server/vfbif.py Sun Mar 23 08:08:09 2008 -0600
@@ -6,7 +6,7 @@ import os
import os
CONFIG_ENTRIES = ['type', 'vncdisplay', 'vnclisten', 'vncpasswd', 'vncunused',
- 'display', 'xauthority', 'keymap',
+ 'videoram', 'display', 'xauthority', 'keymap',
'uuid', 'location', 'protocol', 'opengl']
class VfbifController(DevController):
diff -r 76c9cf11ce23 tools/python/xen/xm/create.py
--- a/tools/python/xen/xm/create.py Fri Mar 21 09:45:34 2008 +0000
+++ b/tools/python/xen/xm/create.py Sun Mar 23 08:10:23 2008 -0600
@@ -500,6 +500,11 @@ gopts.var('vncunused', val='',
use="""Try to find an unused port for the VNC server.
Only valid when vnc=1.""")
+gopts.var('videoram', val='',
+ fn=set_value, default=None,
+ use="""Maximum amount of videoram PV guest can allocate
+ for frame buffer.""")
+
gopts.var('sdl', val='',
fn=set_value, default=None,
use="""Should the device model use SDL?""")
@@ -645,7 +650,8 @@ def configure_vfbs(config_devs, vals):
d['type'] = 'sdl'
for (k,v) in d.iteritems():
if not k in [ 'vnclisten', 'vncunused', 'vncdisplay', 'display',
- 'xauthority', 'type', 'vncpasswd', 'opengl' ]:
+ 'videoram', 'xauthority', 'type', 'vncpasswd',
+ 'opengl' ]:
err("configuration option %s unknown to vfbs" % k)
config.append([k,v])
if not d.has_key("keymap"):
diff -r 76c9cf11ce23 xen/include/public/io/fbif.h
--- a/xen/include/public/io/fbif.h Fri Mar 21 09:45:34 2008 +0000
+++ b/xen/include/public/io/fbif.h Sun Mar 23 08:05:53 2008 -0600
@@ -50,12 +50,28 @@ struct xenfb_update
int32_t height; /* rect height */
};
+/*
+ * Framebuffer resize notification event
+ * Capable backend sets feature-resize in xenstore.
+ */
+#define XENFB_TYPE_RESIZE 3
+
+struct xenfb_resize
+{
+ uint8_t type; /* XENFB_TYPE_RESIZE */
+ int32_t width; /* width in pixels */
+ int32_t height; /* height in pixels */
+ int32_t stride; /* stride in bytes */
+ int32_t depth; /* depth in bits */
+};
+
#define XENFB_OUT_EVENT_SIZE 40
union xenfb_out_event
{
uint8_t type;
struct xenfb_update update;
+ struct xenfb_resize resize;
char pad[XENFB_OUT_EVENT_SIZE];
};
@@ -109,15 +125,17 @@ struct xenfb_page
* Each directory page holds PAGE_SIZE / sizeof(*pd)
* framebuffer pages, and can thus map up to PAGE_SIZE *
* PAGE_SIZE / sizeof(*pd) bytes. With PAGE_SIZE == 4096 and
- * sizeof(unsigned long) == 4, that's 4 Megs. Two directory
- * pages should be enough for a while.
+ * sizeof(unsigned long) == 4/8, that's 4 Megs 32 bit and 2 Megs
+ * 64 bit. 256 directories give enough room for a 512 Meg
+ * framebuffer with a max resolution of 12,800x10,240. Should
+ * be enough for a while with room leftover for expansion.
*/
- unsigned long pd[2];
+ unsigned long pd[256];
};
/*
- * Wart: xenkbd needs to know resolution. Put it here until a better
- * solution is found, but don't leak it to the backend.
+ * Wart: xenkbd needs to know default resolution. Put it here until a
+ * better solution is found, but don't leak it to the backend.
*/
#ifdef __KERNEL__
#define XENFB_WIDTH 800
[-- Attachment #3: fbfront-resize.patch --]
[-- Type: text/x-patch, Size: 13749 bytes --]
diff -r de57c3f218fb drivers/xen/fbfront/xenfb.c
--- a/drivers/xen/fbfront/xenfb.c Thu Mar 20 11:35:25 2008 +0000
+++ b/drivers/xen/fbfront/xenfb.c Sun Mar 23 19:52:17 2008 -0600
@@ -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;
struct xenbus_device *xbdev;
};
/*
- * How the locks work together
- *
- * There are two locks: spinlock dirty_lock protecting the dirty
- * rectangle, and mutex mm_lock protecting mappings.
+ * There are three locks:
+ * spinlock resize_lock protecting resize_dpy and screen size
+ * 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
@@ -129,20 +135,41 @@ struct xenfb_info
*
* Oh well, we wont be updating the writes to this page anytime soon.
*/
+#define MB_ (1024*1024)
+#define XENFB_DEFAULT_FB_LEN (XENFB_WIDTH * XENFB_HEIGHT * XENFB_DEPTH / 8)
+
+enum {KPARAM_MEM, KPARAM_WIDTH, KPARAM_HEIGHT, KPARAM_CNT};
+static int video[KPARAM_CNT] = {2, XENFB_WIDTH, XENFB_HEIGHT};
+module_param_array(video, int, NULL, 0);
+MODULE_PARM_DESC(video,
+ "Size of video memory in MB and width,height in pixels, default = (2,800,600)");
static int xenfb_fps = 20;
-static unsigned long xenfb_mem_len = XENFB_WIDTH * XENFB_HEIGHT * XENFB_DEPTH / 8;
static int xenfb_remove(struct xenbus_device *);
-static void xenfb_init_shared_page(struct xenfb_info *);
+static 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_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 +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);
+}
+
+static void xenfb_do_resize(struct xenfb_info *info)
+{
+ union xenfb_out_event event;
+
+ event.type = XENFB_TYPE_RESIZE;
+ event.resize.width = info->resize.width;
+ event.resize.height = info->resize.height;
+ event.resize.stride = info->resize.stride;
+ event.resize.depth = info->resize.depth;
+
+ /* caller ensures !xenfb_queue_full() */
+ xenfb_send_event(info, &event);
}
static int xenfb_queue_full(struct xenfb_info *info)
@@ -209,11 +244,26 @@ static void xenfb_update_screen(struct x
xenfb_do_update(info, x1, y1, x2 - x1, y2 - y1);
}
+static void xenfb_handle_resize_dpy(struct xenfb_info *info)
+{
+ int flags;
+
+ 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;
+ unsigned long flags;
+
+ xenfb_info = info->par;
+
+ spin_lock_irqsave(&xenfb_info->resize_lock, flags);
+ 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_dpy = 1;
+ spin_unlock_irqrestore(&xenfb_info->resize_lock, flags);
+ return 0;
+}
+
static struct fb_ops xenfb_fb_ops = {
.owner = THIS_MODULE,
.fb_setcolreg = xenfb_setcolreg,
@@ -420,6 +519,8 @@ static struct fb_ops xenfb_fb_ops = {
.fb_copyarea = xenfb_copyarea,
.fb_imageblit = xenfb_imageblit,
.fb_mmap = xenfb_mmap,
+ .fb_check_var = xenfb_check_var,
+ .fb_set_par = xenfb_set_par,
};
static irqreturn_t xenfb_event_handler(int rq, void *dev_id,
@@ -450,6 +551,8 @@ static int __devinit xenfb_probe(struct
{
struct xenfb_info *info;
struct fb_info *fb_info;
+ int fb_size;
+ int val;
int ret;
info = kzalloc(sizeof(*info), GFP_KERNEL);
@@ -457,11 +560,27 @@ 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);
+ spin_lock_init(&info->resize_lock);
mutex_init(&info->mm_lock);
init_waitqueue_head(&info->wq);
init_timer(&info->refresh);
@@ -469,12 +588,12 @@ static int __devinit xenfb_probe(struct
info->refresh.data = (unsigned long)info;
INIT_LIST_HEAD(&info->mappings);
- info->fb = vmalloc(xenfb_mem_len);
+ info->fb = vmalloc(fb_size);
if (info->fb == NULL)
goto error_nomem;
- memset(info->fb, 0, xenfb_mem_len);
-
- info->nr_pages = (xenfb_mem_len + PAGE_SIZE - 1) >> PAGE_SHIFT;
+ memset(info->fb, 0, fb_size);
+
+ info->nr_pages = (fb_size + PAGE_SIZE - 1) >> PAGE_SHIFT;
info->pages = kmalloc(sizeof(struct page *) * info->nr_pages,
GFP_KERNEL);
@@ -489,8 +608,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 +621,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 +635,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 +650,8 @@ static int __devinit xenfb_probe(struct
xenbus_dev_fatal(dev, ret, "fb_alloc_cmap");
goto error;
}
+
+ xenfb_init_shared_page(info, fb_info);
ret = register_framebuffer(fb_info);
if (ret) {
@@ -571,7 +690,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 +716,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 +728,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 +834,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 de57c3f218fb drivers/xen/fbfront/xenkbd.c
--- a/drivers/xen/fbfront/xenkbd.c Thu Mar 20 11:35:25 2008 +0000
+++ b/drivers/xen/fbfront/xenkbd.c Sun Mar 23 08:04:40 2008 -0600
@@ -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 )
+ input_set_abs_params(info->ptr, ABS_Y, 0, val, 0, 0);
+
break;
case XenbusStateClosing:
diff -r de57c3f218fb include/xen/interface/io/fbif.h
--- a/include/xen/interface/io/fbif.h Thu Mar 20 11:35:25 2008 +0000
+++ b/include/xen/interface/io/fbif.h Sun Mar 23 08:04:40 2008 -0600
@@ -50,12 +50,28 @@ struct xenfb_update
int32_t height; /* rect height */
};
+/*
+ * Framebuffer resize notification event
+ * Capable backend sets feature-resize in xenstore.
+ */
+#define XENFB_TYPE_RESIZE 3
+
+struct xenfb_resize
+{
+ uint8_t type; /* XENFB_TYPE_RESIZE */
+ int32_t width; /* width in pixels */
+ int32_t height; /* height in pixels */
+ int32_t stride; /* stride in bytes */
+ int32_t depth; /* depth in bits */
+};
+
#define XENFB_OUT_EVENT_SIZE 40
union xenfb_out_event
{
uint8_t type;
struct xenfb_update update;
+ struct xenfb_resize resize;
char pad[XENFB_OUT_EVENT_SIZE];
};
@@ -109,15 +125,17 @@ struct xenfb_page
* Each directory page holds PAGE_SIZE / sizeof(*pd)
* framebuffer pages, and can thus map up to PAGE_SIZE *
* PAGE_SIZE / sizeof(*pd) bytes. With PAGE_SIZE == 4096 and
- * sizeof(unsigned long) == 4, that's 4 Megs. Two directory
- * pages should be enough for a while.
+ * sizeof(unsigned long) == 4/8, that's 4 Megs 32 bit and 2 Megs
+ * 64 bit. 256 directories give enough room for a 512 Meg
+ * framebuffer with a max resolution of 12,800x10,240. Should
+ * be enough for a while with room leftover for expansion.
*/
- unsigned long pd[2];
+ unsigned long pd[256];
};
/*
- * Wart: xenkbd needs to know resolution. Put it here until a better
- * solution is found, but don't leak it to the backend.
+ * Wart: xenkbd needs to know default resolution. Put it here until a
+ * better solution is found, but don't leak it to the backend.
*/
#ifdef __KERNEL__
#define XENFB_WIDTH 800
[-- Attachment #4: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH][LINUX] Dynamic modes support for xenfb (2 of 2)
2008-03-24 3:40 ` Pat Campbell
@ 2008-03-25 14:43 ` Markus Armbruster
2008-03-25 16:31 ` Samuel Thibault
0 siblings, 1 reply; 12+ messages in thread
From: Markus Armbruster @ 2008-03-25 14:43 UTC (permalink / raw)
To: Pat Campbell; +Cc: xen-devel
Pat Campbell <plc@novell.com> writes:
> Markus Armbruster wrote:
>> Pat Campbell <plc@novell.com> writes:
>>
>>
>>> Markus Armbruster wrote:
>>>
>>>> Pat Campbell <plc@novell.com> writes:
>>>>
>>>>
>>>>
>>>>> Markus Armbruster wrote:
>>>>>
>>>>>
>>>>>> Pat Campbell <plc@novell.com> writes:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>> 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.
Yup, protection against that is required.
Looks good! One easily fixed bug and a few remarks inline.
[...]
> diff -r de57c3f218fb drivers/xen/fbfront/xenfb.c
> --- a/drivers/xen/fbfront/xenfb.c Thu Mar 20 11:35:25 2008 +0000
> +++ b/drivers/xen/fbfront/xenfb.c Sun Mar 23 19:52:17 2008 -0600
> @@ -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;
>
> struct xenbus_device *xbdev;
> };
>
> /*
> - * How the locks work together
> - *
> - * There are two locks: spinlock dirty_lock protecting the dirty
> - * rectangle, and mutex mm_lock protecting mappings.
> + * There are three locks:
> + * spinlock resize_lock protecting resize_dpy and screen size
Suggest
* spinlock resize_lock protecting resize_dpy and resize
because with the new screen size in one place now we can be both both
specific and concise here.
> + * 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
[...]
> @@ -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);
> +}
> +
> +static void xenfb_do_resize(struct xenfb_info *info)
> +{
> + union xenfb_out_event event;
> +
> + event.type = XENFB_TYPE_RESIZE;
> + event.resize.width = info->resize.width;
> + event.resize.height = info->resize.height;
> + event.resize.stride = info->resize.stride;
> + event.resize.depth = info->resize.depth;
Could do event.resize = info->resize, except info->resize.type isn't
set up for that (see below).
> +
> + /* caller ensures !xenfb_queue_full() */
> + xenfb_send_event(info, &event);
> }
>
> static int xenfb_queue_full(struct xenfb_info *info)
> @@ -209,11 +244,26 @@ static void xenfb_update_screen(struct x
> xenfb_do_update(info, x1, y1, x2 - x1, y2 - y1);
> }
>
> +static void xenfb_handle_resize_dpy(struct xenfb_info *info)
> +{
> + int flags;
Type error! unsigned long flags
> +
> + 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;
> + unsigned long flags;
> +
> + xenfb_info = info->par;
> +
> + spin_lock_irqsave(&xenfb_info->resize_lock, flags);
> + 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_dpy = 1;
Not a bug, just a little trap for the unwary: xenfb_info->resize.type
remains zero.
> + spin_unlock_irqrestore(&xenfb_info->resize_lock, flags);
> + return 0;
> +}
> +
> static struct fb_ops xenfb_fb_ops = {
> .owner = THIS_MODULE,
> .fb_setcolreg = xenfb_setcolreg,
[...]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH][LINUX] Dynamic modes support for xenfb (2 of 2)
2008-03-25 14:43 ` Markus Armbruster
@ 2008-03-25 16:31 ` Samuel Thibault
2008-03-25 16:46 ` Markus Armbruster
0 siblings, 1 reply; 12+ messages in thread
From: Samuel Thibault @ 2008-03-25 16:31 UTC (permalink / raw)
To: Markus Armbruster; +Cc: xen-devel, Pat Campbell
Hello,
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH][LINUX] Dynamic modes support for xenfb (2 of 2)
2008-03-25 16:31 ` Samuel Thibault
@ 2008-03-25 16:46 ` Markus Armbruster
0 siblings, 0 replies; 12+ messages in thread
From: Markus Armbruster @ 2008-03-25 16:46 UTC (permalink / raw)
To: Samuel Thibault; +Cc: xen-devel, Pat Campbell
Samuel Thibault <samuel.thibault@eu.citrix.com> writes:
> Hello,
>
> 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
Fine with me. The backend looked okay in the last iteration
already[*], and it didn't substantially change.
[*] It could still use a few tastefull line-breaks, though ;)
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH][LINUX] Dynamic modes support for xenfb (2 of 2)
@ 2008-03-25 20:10 Pat Campbell
2008-03-26 9:45 ` Markus Armbruster
0 siblings, 1 reply; 12+ messages in thread
From: Pat Campbell @ 2008-03-25 20:10 UTC (permalink / raw)
To: xen-devel
[-- Attachment #1: Type: text/plain, Size: 325 bytes --]
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 <plc@novell.com>
[-- Attachment #2: fbfront-resize.patch --]
[-- Type: text/x-patch, Size: 13622 bytes --]
diff -r de57c3f218fb drivers/xen/fbfront/xenfb.c
--- a/drivers/xen/fbfront/xenfb.c Thu Mar 20 11:35:25 2008 +0000
+++ b/drivers/xen/fbfront/xenfb.c Tue Mar 25 10:20:01 2008 -0600
@@ -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;
struct xenbus_device *xbdev;
};
/*
- * How the locks work together
- *
- * There are two locks: spinlock dirty_lock protecting the dirty
- * rectangle, and mutex mm_lock protecting mappings.
+ * There are three locks:
+ * spinlock resize_lock protecting resize_dpy and resize
+ * 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
@@ -129,20 +135,41 @@ struct xenfb_info
*
* Oh well, we wont be updating the writes to this page anytime soon.
*/
+#define MB_ (1024*1024)
+#define XENFB_DEFAULT_FB_LEN (XENFB_WIDTH * XENFB_HEIGHT * XENFB_DEPTH / 8)
+
+enum {KPARAM_MEM, KPARAM_WIDTH, KPARAM_HEIGHT, KPARAM_CNT};
+static int video[KPARAM_CNT] = {2, XENFB_WIDTH, XENFB_HEIGHT};
+module_param_array(video, int, NULL, 0);
+MODULE_PARM_DESC(video,
+ "Size of video memory in MB and width,height in pixels, default = (2,800,600)");
static int xenfb_fps = 20;
-static unsigned long xenfb_mem_len = XENFB_WIDTH * XENFB_HEIGHT * XENFB_DEPTH / 8;
static int xenfb_remove(struct xenbus_device *);
-static void xenfb_init_shared_page(struct xenfb_info *);
+static 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_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 +177,18 @@ static void xenfb_do_update(struct xenfb
event.update.width = w;
event.update.height = h;
- prod = info->page->out_prod;
/* caller ensures !xenfb_queue_full() */
- mb(); /* ensure ring space available */
- XENFB_OUT_RING_REF(info->page, prod) = event;
- wmb(); /* ensure ring contents visible */
- info->page->out_prod = prod + 1;
-
- notify_remote_via_irq(info->irq);
+ xenfb_send_event(info, &event);
+}
+
+static void xenfb_do_resize(struct xenfb_info *info)
+{
+ union xenfb_out_event event;
+
+ event.resize = info->resize;
+
+ /* caller ensures !xenfb_queue_full() */
+ xenfb_send_event(info, &event);
}
static int xenfb_queue_full(struct xenfb_info *info)
@@ -209,11 +240,26 @@ static void xenfb_update_screen(struct x
xenfb_do_update(info, x1, y1, x2 - x1, y2 - y1);
}
+static void xenfb_handle_resize_dpy(struct xenfb_info *info)
+{
+ unsigned long flags;
+
+ 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 +459,56 @@ 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 flags;
+
+ xenfb_info = info->par;
+
+ 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_dpy = 1;
+ spin_unlock_irqrestore(&xenfb_info->resize_lock, flags);
+ return 0;
+}
+
static struct fb_ops xenfb_fb_ops = {
.owner = THIS_MODULE,
.fb_setcolreg = xenfb_setcolreg,
@@ -420,6 +516,8 @@ static struct fb_ops xenfb_fb_ops = {
.fb_copyarea = xenfb_copyarea,
.fb_imageblit = xenfb_imageblit,
.fb_mmap = xenfb_mmap,
+ .fb_check_var = xenfb_check_var,
+ .fb_set_par = xenfb_set_par,
};
static irqreturn_t xenfb_event_handler(int rq, void *dev_id,
@@ -450,6 +548,8 @@ static int __devinit xenfb_probe(struct
{
struct xenfb_info *info;
struct fb_info *fb_info;
+ int fb_size;
+ int val;
int ret;
info = kzalloc(sizeof(*info), GFP_KERNEL);
@@ -457,11 +557,27 @@ 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);
+ spin_lock_init(&info->resize_lock);
mutex_init(&info->mm_lock);
init_waitqueue_head(&info->wq);
init_timer(&info->refresh);
@@ -469,12 +585,12 @@ static int __devinit xenfb_probe(struct
info->refresh.data = (unsigned long)info;
INIT_LIST_HEAD(&info->mappings);
- info->fb = vmalloc(xenfb_mem_len);
+ info->fb = vmalloc(fb_size);
if (info->fb == NULL)
goto error_nomem;
- memset(info->fb, 0, xenfb_mem_len);
-
- info->nr_pages = (xenfb_mem_len + PAGE_SIZE - 1) >> PAGE_SHIFT;
+ memset(info->fb, 0, fb_size);
+
+ info->nr_pages = (fb_size + PAGE_SIZE - 1) >> PAGE_SHIFT;
info->pages = kmalloc(sizeof(struct page *) * info->nr_pages,
GFP_KERNEL);
@@ -489,8 +605,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 +618,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 +632,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 +647,8 @@ static int __devinit xenfb_probe(struct
xenbus_dev_fatal(dev, ret, "fb_alloc_cmap");
goto error;
}
+
+ xenfb_init_shared_page(info, fb_info);
ret = register_framebuffer(fb_info);
if (ret) {
@@ -571,7 +687,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 +713,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 +725,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 +831,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 de57c3f218fb drivers/xen/fbfront/xenkbd.c
--- a/drivers/xen/fbfront/xenkbd.c Thu Mar 20 11:35:25 2008 +0000
+++ b/drivers/xen/fbfront/xenkbd.c Tue Mar 25 10:20:01 2008 -0600
@@ -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 )
+ input_set_abs_params(info->ptr, ABS_Y, 0, val, 0, 0);
+
break;
case XenbusStateClosing:
diff -r de57c3f218fb include/xen/interface/io/fbif.h
--- a/include/xen/interface/io/fbif.h Thu Mar 20 11:35:25 2008 +0000
+++ b/include/xen/interface/io/fbif.h Tue Mar 25 10:20:01 2008 -0600
@@ -50,12 +50,28 @@ struct xenfb_update
int32_t height; /* rect height */
};
+/*
+ * Framebuffer resize notification event
+ * Capable backend sets feature-resize in xenstore.
+ */
+#define XENFB_TYPE_RESIZE 3
+
+struct xenfb_resize
+{
+ uint8_t type; /* XENFB_TYPE_RESIZE */
+ int32_t width; /* width in pixels */
+ int32_t height; /* height in pixels */
+ int32_t stride; /* stride in bytes */
+ int32_t depth; /* depth in bits */
+};
+
#define XENFB_OUT_EVENT_SIZE 40
union xenfb_out_event
{
uint8_t type;
struct xenfb_update update;
+ struct xenfb_resize resize;
char pad[XENFB_OUT_EVENT_SIZE];
};
@@ -109,15 +125,17 @@ struct xenfb_page
* Each directory page holds PAGE_SIZE / sizeof(*pd)
* framebuffer pages, and can thus map up to PAGE_SIZE *
* PAGE_SIZE / sizeof(*pd) bytes. With PAGE_SIZE == 4096 and
- * sizeof(unsigned long) == 4, that's 4 Megs. Two directory
- * pages should be enough for a while.
+ * sizeof(unsigned long) == 4/8, that's 4 Megs 32 bit and 2 Megs
+ * 64 bit. 256 directories give enough room for a 512 Meg
+ * framebuffer with a max resolution of 12,800x10,240. Should
+ * be enough for a while with room leftover for expansion.
*/
- unsigned long pd[2];
+ unsigned long pd[256];
};
/*
- * Wart: xenkbd needs to know resolution. Put it here until a better
- * solution is found, but don't leak it to the backend.
+ * Wart: xenkbd needs to know default resolution. Put it here until a
+ * better solution is found, but don't leak it to the backend.
*/
#ifdef __KERNEL__
#define XENFB_WIDTH 800
[-- Attachment #3: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH][LINUX] Dynamic modes support for xenfb (2 of 2)
2008-03-25 20:10 Pat Campbell
@ 2008-03-26 9:45 ` Markus Armbruster
0 siblings, 0 replies; 12+ messages in thread
From: Markus Armbruster @ 2008-03-26 9:45 UTC (permalink / raw)
To: Pat Campbell; +Cc: xen-devel
Pat Campbell <plc@novell.com> writes:
> 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 <plc@novell.com>
Acked-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2008-03-26 9:45 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-03-13 19:59 [PATCH][LINUX] Dynamic modes support for xenfb (2 of 2) Pat Campbell
2008-03-14 16:41 ` Markus Armbruster
2008-03-16 19:09 ` Pat Campbell
2008-03-17 17:35 ` Markus Armbruster
2008-03-18 19:11 ` Pat Campbell
2008-03-19 10:21 ` Markus Armbruster
2008-03-24 3:40 ` Pat Campbell
2008-03-25 14:43 ` Markus Armbruster
2008-03-25 16:31 ` Samuel Thibault
2008-03-25 16:46 ` Markus Armbruster
-- strict thread matches above, loose matches on Subject: below --
2008-03-25 20:10 Pat Campbell
2008-03-26 9:45 ` Markus Armbruster
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.