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][RFC] Dynamic modes support for PV xenfb (included)

To: Pat Campbell <plc@xxxxxxxxxx>
Subject: Re: [Xen-devel][RFC] Dynamic modes support for PV xenfb (included)
From: Markus Armbruster <armbru@xxxxxxxxxx>
Date: Fri, 14 Mar 2008 17:39:21 +0100
Cc: xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxx>, "Daniel P. Berrange" <berrange@xxxxxxxxxx>, Samuel Thibault <samuel.thibault@xxxxxxxxxxxxx>
Delivery-date: Fri, 14 Mar 2008 09:39:47 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <47D9269B.9030500@xxxxxxxxxx> (Pat Campbell's message of "Thu\, 13 Mar 2008 07\:05\:31 -0600")
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: <47D4547F.1060305@xxxxxxxxxx> <8763vryiu6.fsf@xxxxxxxxxxxxxxxxx> <47D9269B.9030500@xxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Gnus/5.11 (Gnus v5.11) Emacs/22.1 (gnu/linux)
Pat Campbell <plc@xxxxxxxxxx> writes:

> Markus Armbruster wrote:
>> Pat Campbell <plc@xxxxxxxxxx> writes:
>>
>>   
>>> Patches are against xen-unstable tip
>>>
>>> What changed since last comment round. (No major structural
>>> surprises this time:-).
>>>
>>>  Backend:
>>>   1. In the resize event handler 
>>>        Disabled shared memory (Need to fix)
>>>        Invalidate the buffer 
>>>   2. Added videoram config variable to limit hostile front 
>>>      end frame buffer memory size.  
>>>   3. Sets feature-resize and width/height in xenstore 
>>>      before frontend connected. width/height are for xenkbd
>>>      abs input config values
>>>   
>>>  Frontend:
>>>   1. Variable usage cleanup.  Now using the fb variables
>>>      in most cases.  Only two fields added to main struct
>>>   2. Removed resize event send in xenfb_resume().  There is 
>>>      a general race condition where the vnc view will be left 
>>>      at 600x400 if attached to too early.  (Not resize related)
>>>     
>>
>> Could you elaborate on this race?
>>   
>>From the command line I suspend the guest
>>From a perl script I:
>    Resume the guest, xm resume <guest-name>
>    Loop checking for vnc port in xenstore, when found
>      immediately attach.  9 times out of 10 times the vncviewer will
>      be at 600,400 instead of the default 800x600
>
> This happens in Xen 3.2 without any of my code changes and before the
> recent shared_buf changes.  I intend to investigate this further.

Interesting.  Please keep us posted.

>>   
>>>   3. Removed refresh call in xenfb_resize_screen(), back end 
>>>      invalidates buffer in resize event handler now
>>>   4. xenkbd gets width and height for abs input in Connected
>>>
>>> After I get some feed back, will wait a day or two, I will 
>>> prepare a proper patch set.
>>>
>>> Pat
>>>     
>>
>> I like this much better.  A couple of questions remain; see comments
>> inline.
>>
>>   
>>> diff -r 854b0704962b tools/ioemu/hw/xenfb.c
[...]
>>> diff -r 854b0704962b xen/include/public/io/fbif.h
>>> --- a/xen/include/public/io/fbif.h  Wed Mar 05 09:43:03 2008 +0000
>>> +++ b/xen/include/public/io/fbif.h  Thu Mar 06 11:12:17 2008 -0600
[...]
>>>  /*
>>> - * 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.
>>>   */
>>>     
>>
>> You still need this wart because you still set the default resolution
>> in xenkbd_probe().  You later reset it to the real maximum resolution
>> in xenkbd_backend_changed(), after frontend went to state Connected.
>> I wonder whether the first one is necessary.
>>
>>   
> Yes I think so.  Handles the case of the new VM running against an older
> vnc-xenfb or qemu that does not send  a new value.

You're right.

>>>  #ifdef __KERNEL__
>>>  #define XENFB_WIDTH 800
>>> diff -r 1cf7ba68d855 drivers/xen/fbfront/xenfb.c
>>> --- a/drivers/xen/fbfront/xenfb.c   Mon Mar 03 13:36:57 2008 +0000
>>> +++ b/drivers/xen/fbfront/xenfb.c   Fri Mar 07 21:21:44 2008 -0600
[...]
>>> @@ -209,11 +241,23 @@ static void xenfb_update_screen(struct x
>>>     xenfb_do_update(info, x1, y1, x2 - x1, y2 - y1);
>>>  }
>>>  
>>> +static void xenfb_resize_screen(struct xenfb_info *info)
>>> +{
>>> +   if (xenfb_queue_full(info))
>>> +           return;
>>> +
>>> +   info->resize_dpy = 0;
>>>     
>>
>> I think you can info->dirty = 0 here.  Saves an update event.
>>   
> Added that and then took it back out.  Somehow that caused the GUI to
> have a terrible delay, minutes, when starting up before you could enter
> your user name password.  Changing screens work fast enough.  Will have
> to investigate this later.

Weird.  It would be good to know what exactly goes wrong there.

>>> +   xenfb_do_resize(info);
>>> +}
>>> +
[...]
>>> +static int xenfb_set_par(struct fb_info *info)
>>> +{
>>> +   struct xenfb_info *xenfb_info;
>>> +
>>> +   xenfb_info = info->par;
>>> +
>>> +   info->var.xres_virtual = info->var.xres;
>>> +   info->var.yres_virtual = info->var.yres;
>>> +   xenfb_info->resize_dpy = 1;
>>> +   return 0;
>>> +}
>>>     
>>
>> How is this synchronized with xenfb_do_resize()?
>>
>> If that runs on another processor, it could see the new value of
>> resize_dpy, and old values of var.xres and var.yres.
>>
>>   
> By the time xenfb_set_par() is called the values are already in the fb
> struct.  They are set sometime between the successful xenfb_check_var()
> and xenfb_set_par() calls. I can see a possible condition where if we
> are doing back to back resizes utilizing multiple procs we could get a
> new xres and an old yres.   
>
> I will add into xenfb_check_var() the following test:
>   if ( xenfb_info->resize_dpy)
>      return -EINVAL;
> and move the resize_dpy clear to below the xenfb_do_resize() call

Further discussed in another message, will reply there.

[...]
>>> diff -r 1cf7ba68d855 drivers/xen/fbfront/xenkbd.c
>>> --- a/drivers/xen/fbfront/xenkbd.c  Mon Mar 03 13:36:57 2008 +0000
>>> +++ b/drivers/xen/fbfront/xenkbd.c  Fri Mar 07 16:57:34 2008 -0600
>>> @@ -295,6 +295,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:
>>>     
>>
>> The combined fb/kbd backend writes width and height right before
>> entering state Connected for both devices.  Therefore, we can read it
>> here in the kbd frontend after observing the kbd backend transitioning
>> to Conntected.  Okay.
>>
>> But what about the race work-around?  If the backend goes through
>> InitWait to Connected fast enough, we don't see the InitWait, and we
>> work around the race with the goto InitWait.  But are we guaranteed to
>> get called a second time for the transition to Connected?  If not,
>> width and height are never read.
>>   
> I debated this.  Should those parameters be read before the work
> around?  Qemu xenfb did reordered things, is the work around still
> necessary?  Should I work around the work around?   I never saw that
> case during my testing, not to say it still does not happen.  I would
> like to see if this is still an issue in a larger audience before changing.

The work-around is necessary if the backend can go through InitWait to
Connected without synchronizing with the frontend.  It actually
synchronizes by waiting for the frontend changing state to
Initialised.  I figure the work-around can be removed.

>> Why don't we have to set the parameters on dynamic resolution change?
>>   
> I debated this.  Should those parameters be read before the work
> around?  Qemu xenfb did reorder things, is the work around still
> necessary?  Should I work around the work around?   I never saw that
> case during my testing, not to say it still does not happen.  I would
> like to see if this is still an issue in a larger audience before changing.

Pardon?

[...]
> I will send out a real patch incorporating your suggestions later today.
>
> Thanks
>
> Pat

Got that, working on it.

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