|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel][PATCH][LINUX] Dynamic modes support for xenfb (2 of 2)
Markus Armbruster wrote:
> Pat Campbell <plc@xxxxxxxxxx> 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@xxxxxxxxxx>
>>
>
> 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@xxxxxxxxxxxxxxxxxxx
>> http://lists.xensource.com/xen-devel
>>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |