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

[Markus Armbruster <armbru@xxxxxxxxxx>]

"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