WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel][PATCH][LINUX] Dynamic modes support for PV xenfb (1 of 2

To: "Pat Campbell" <plc@xxxxxxxxxx>
Subject: Re: [Xen-devel][PATCH][LINUX] Dynamic modes support for PV xenfb (1 of 2)
From: Markus Armbruster <armbru@xxxxxxxxxx>
Date: Sat, 12 Jan 2008 09:03:14 +0100
Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
Delivery-date: Sat, 12 Jan 2008 00:03:39 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <4787222C.3E48.0018.0@xxxxxxxxxx> (Pat Campbell's message of "Fri\, 11 Jan 2008 08\:06\:24 -0700")
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <47833551.3E48.0018.0@xxxxxxxxxx> <47833551.3E48.0018.0@xxxxxxxxxx> <87abnehtfn.fsf@xxxxxxxxxxxxxxxxx> <4787222C.3E48.0018.0@xxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Gnus/5.11 (Gnus v5.11) Emacs/22.1 (gnu/linux)
"Pat Campbell" <plc@xxxxxxxxxx> writes:

>>>> On Thu, Jan 10, 2008 at  3:21 AM, in message
> <87abnehtfn.fsf@xxxxxxxxxxxxxxxxx>, Markus Armbruster <armbru@xxxxxxxxxx>
> wrote: 
>> "Pat Campbell" <plc@xxxxxxxxxx> writes:
>> 
>>> Attached patch adds multiple frame buffer resolution support to 
>>> the PV xenfb frame buffer driver.
>>>
>>> Code is essentially the same as I sent in the previous RFC except
>>> the frame buffer size is now 800x600 if the backend does not
>>> support feature-    resize,  same memory footprint.
>>>
>>> Corresponding backend IOEMU patch is required for functionality but 
>>> this patch is not dependent on it, preserving backwards compatibility.
[...]
>>>  static int xenfb_thread(void *data)
>>>  {
>>>     struct xenfb_info *info = data;
>>> @@ -    217,6 +253,9 @@ static int xenfb_thread(void *data)
>>>             if (info-    >dirty) {
>>>                     info-    >dirty = 0;
>>>                     xenfb_update_screen(info);
>>> +           }
>>> +           if (info-    >resize_dpy) {
>>> +                   xenfb_resize_screen(info);
>>>             }
>> 
>> Hmm.  If both dirty and resize_dpy are set, the update event goes out
>> before the resize event.  What if the update is for the resized
>> screen?  If whatever caused the update happened after the screen was
>> resized, then the update event overtakes the resize event, and its
>> coordinates will not make sense, will they?
>
> An update event for a rectangle outside the new size will be clipped and
> then discarded in the backend.  Wasted time sending it but it should
> have no ill effect.

Yes.  It's still ugly.  Hmm, the resize makes the backend refresh the
whole screen, doesn't it?  That makes the update *always* redundant.
We could suppress the event.  I guess that's not quite worth the
trouble.  What about a comment explaining this?

>>>             wait_event_interruptible(info-    >wq,
>>>                     kthread_should_stop() || info-    >dirty);
>>> @@ -    413,6 +452,45 @@ 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;
>>> +
>>> +   xenfb_info = info-    >par;
>> 
>> Style nitpick:
>> 
>>      struct xenfb_info *xenfb_info = info-    >par;
>> 
>>> +
>>> +   if (!xenfb_info-    >feature_resize) {
>>> +           if (var-    >xres == XENFB_WIDTH && var-    >yres == 
>>> XENFB_HEIGHT
>>> +                           && var-    >bits_per_pixel == xenfb_info-    
>>> >page-    >depth) {
>>> +                   return 0;
>>> +           }
>>> +           return -    EINVAL;
>>> +   }
>>> +   if (var-    >bits_per_pixel == xenfb_info-    >page-    >depth && 
>>> +                   XENFB_SCAN_LINE_LEN >= var-    >xres && 
>>> +                   xenfb_mem_len >= (var-    >xres * var-    >yres * 
>>> (xenfb_info-    >page-    >depth / 8))) {
>>> +           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;
>>> +
>>> +   xenfb_info = info-    >par;
>> 
>> Style nitpick:
>> 
>>      struct xenfb_info *xenfb_info = info-    >par;
>> 
>>> +
>>> +   if (xenfb_info-    >xres != info-    >var.xres || 
>>> +                   xenfb_info-    >yres != info-    >var.yres) {
>>> +           xenfb_info-    >resize_dpy = 1;
>>> +           xenfb_info-    >xres = info-    >var.xres_virtual = info-    
>>> >var.xres;
>>> +           xenfb_info-    >yres = info-    >var.yres_virtual = info-    
>>> >var.yres;
>>> +   }
>>> +   return 0;
>>> +}
>> 
>> How is the resolution change synchronized between guest user land,
>> pvfb frontend and pvfb backend?
>> 
>
> Resolution change is asynchronous.  Short of making fb_set_par() 
> a blocking call I see no way to make this synchronized.
>
>> I guess method fb_set_par() changes the resolution instantly as far as
>> the guest is concerned.  The framebuffer then contains junk, until
>> guest user land fixes it up.  The notification of the backend is
>> delayed until xenfb_thread() next checks xenfb_info-    >resize_dpy.
>> 
>> By the way, this delay is arbitrary; we could call
>> xenfb_resize_screen() here.  We route screen updates through
>> xenfb_thread() because we can't do that work from xenfb_vm_nopage().
>> Welcome side effect: also collapses multiple quick updates into one.
>> None of this is of concern for screen resize.
>
> Sending the resize event from within xenfb_thread protects against the
> possibility of the queue being full.

You're right.

>                                        resize_dpy will not be cleared until 
> the
> event is successfully inserted in the queue unlike update events which
> are silently discarded if the queue is full. Probably will never happen but
> that is why I put it there.

Update events are not discarded, they're accumulated in the dirty
rectangle.

>> Anyway, the backend continues to interpret the framebuffer according
>> to the old resolution until it receives the notification.  The
>> notification also makes it redraw the whole screen.  There's no
>> guarantee that guest user land finished fixing up the framebuffer
>> contents by then.  If it hasn't, the redraw draws junk.  This junk can
>> then remain on the screen indefinitely, because no further screen
>> update requests need to arrive from the frontend.  Correct?
>> 
>
> Yes, although in testing I have not seen artifacts of the old resolution
> on a resized screen.  A simple VNC client refresh by the user would
> correct that  :)

I guess (hope?) guest user land saves us there by triggering suitable
update events.

[...]

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel

<Prev in Thread] Current Thread [Next in Thread>