Pat Campbell <plc@xxxxxxxxxx> writes:
> Markus Armbruster wrote:
>> Pat Campbell <plc@xxxxxxxxxx> writes:
>>
>>
>>> Markus Armbruster wrote:
>>>
>>>> Pat Campbell <plc@xxxxxxxxxx> writes:
>>>>
>>>>
>>>>
>>>>> Markus Armbruster wrote:
>>>>>
>>>>>
>>>>>> Pat Campbell <plc@xxxxxxxxxx> 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,
[...]
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home