| Message ID | 20220316160300.85438-1-philippe.mathieu.daude@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,PATCH-for-7.0,v2] cocoa: run qemu_init in the main thread | expand |
On 2022/03/17 1:03, Philippe Mathieu-Daudé wrote: > From: Paolo Bonzini <pbonzini@redhat.com> > > Simplify the initialization dance by running qemu_init() in the main > thread before the Cocoa event loop starts. The cocoa_display_init() > code that is post-applicationDidFinishLaunching: moves to the > application delegate itself, and the secondary thread only runs > the rest of qemu_main(), namely qemu_main_loop() and qemu_cleanup(). > > This fixes a case where addRemovableDevicesMenuItems() calls > qmp_query_block() while expecting the main thread to still hold > the BQL. The newly-introduced assertions in the block layer > complain about this. I was thinking that it may be better to let softmmu/main.c do the details if it involves the internals of qemu_main() like qemu_main_loop(). More concretely, softmmu/main.c would provide a function to register a function pointer to take over the main thread. main() implemented in softmmu/main.c would call qemu_init(). If a function pointer gets registered in qemu_init(), it would create a thread for main loop and call the registered function pointer. Otherwise, it would directly call qemu_main_loop(). It would be a semantically appropriate division of ui/cocoa.m and softmmu/main.c. It would also be beneficial for end-users as it would also allow to isolate ui/cocoa.m into a separate module when --enable-modules in the future. (With "In the future", I mean sometime when we have time to hack Meson build files and some details we cannot fill by 7.0.) Regards, Akihiko Odaki > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > Message-Id: <20220307151004.578069-1-pbonzini@redhat.com> > [PMD: Fixed trivial build failures & rebased] > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > Cc: Akihiko Odaki <akihiko.odaki@gmail.com> > Cc: Peter Maydell <peter.maydell@linaro.org> > Cc: Emanuele Giuseppe Esposito <eesposit@redhat.com> > > v1: https://lore.kernel.org/qemu-devel/20220307151004.578069-1-pbonzini@redhat.com/ > --- > softmmu/main.c | 12 ++-- > ui/cocoa.m | 153 ++++++++++++++++++++++--------------------------- > 2 files changed, 74 insertions(+), 91 deletions(-) > > diff --git a/softmmu/main.c b/softmmu/main.c > index 639c67ff48..0c4384e980 100644 > --- a/softmmu/main.c > +++ b/softmmu/main.c > @@ -39,16 +39,18 @@ int main(int argc, char **argv) > #endif > #endif /* CONFIG_SDL */ > > -#ifdef CONFIG_COCOA > -#undef main > -#define main qemu_main > -#endif /* CONFIG_COCOA */ > - > +#ifndef CONFIG_COCOA > int main(int argc, char **argv, char **envp) > { > + /* > + * ui/cocoa.m relies on this being the exact content of main(), > + * because it has to run everything after qemu_init in a secondary > + * thread. > + */ > qemu_init(argc, argv, envp); > qemu_main_loop(); > qemu_cleanup(); > > return 0; > } > +#endif /* CONFIG_COCOA */ > diff --git a/ui/cocoa.m b/ui/cocoa.m > index cb6e7c41dc..e69ce97f44 100644 > --- a/ui/cocoa.m > +++ b/ui/cocoa.m > @@ -75,6 +75,9 @@ typedef struct { > int height; > } QEMUScreen; > > +@interface QemuCocoaPasteboardTypeOwner : NSObject<NSPasteboardTypeOwner> > +@end > + > static void cocoa_update(DisplayChangeListener *dcl, > int x, int y, int w, int h); > > @@ -97,20 +100,23 @@ static int last_buttons; > static int cursor_hide = 1; > static int left_command_key_enabled = 1; > static bool swap_opt_cmd; > +static bool full_screen; > +static bool full_grab; > +static bool have_cocoa_ui; > > -static int gArgc; > -static char **gArgv; > static bool stretch_video; > static NSTextField *pauseLabel; > > -static QemuSemaphore display_init_sem; > -static QemuSemaphore app_started_sem; > static bool allow_events; > > static NSInteger cbchangecount = -1; > static QemuClipboardInfo *cbinfo; > static QemuEvent cbevent; > > +static QemuCocoaPasteboardTypeOwner *cbowner; > +static QemuClipboardPeer cbpeer; > +static QemuThread main_thread; > + > // Utility functions to run specified code block with iothread lock held > typedef void (^CodeBlock)(void); > typedef bool (^BoolCodeBlock)(void); > @@ -142,6 +148,33 @@ static bool bool_with_iothread_lock(BoolCodeBlock block) > return val; > } > > +/* > + * The startup process for the OSX/Cocoa UI is complicated, because > + * OSX insists that the UI runs on the initial main thread, and so we > + * need to start a second thread which runs qemu_main_loop(): > + * > + * Initial thread: 2nd thread: > + * in main(): > + * qemu_init() > + * create application, menus, etc > + * enter OSX run loop > + * in applicationDidFinishLaunching: > + * fullscreen if needed > + * create main loop thread > + * call qemu_main_loop() > + */ > + > +static void *call_qemu_main_loop(void *opaque) > +{ > + COCOA_DEBUG("Second thread: calling qemu_main()\n"); > + qemu_mutex_lock_iothread(); > + qemu_main_loop(); > + COCOA_DEBUG("Second thread: qemu_main_loop() returned, exiting\n"); > + qemu_cleanup(); > + [cbowner release]; > + exit(0); > +} > + > // Mac to QKeyCode conversion > static const int mac_to_qkeycode_map[] = { > [kVK_ANSI_A] = Q_KEY_CODE_A, > @@ -780,9 +813,7 @@ static CGEventRef handleTapEvent(CGEventTapProxy proxy, CGEventType type, CGEven > /* > * Just let OSX have all events that arrive before > * applicationDidFinishLaunching. > - * This avoids a deadlock on the iothread lock, which cocoa_display_init() > - * will not drop until after the app_started_sem is posted. (In theory > - * there should not be any such events, but OSX Catalina now emits some.) > + * This may not be needed anymore? > */ > return false; > } > @@ -1280,8 +1311,22 @@ static CGEventRef handleTapEvent(CGEventTapProxy proxy, CGEventType type, CGEven > { > COCOA_DEBUG("QemuCocoaAppController: applicationDidFinishLaunching\n"); > allow_events = true; > - /* Tell cocoa_display_init to proceed */ > - qemu_sem_post(&app_started_sem); > + > + // register vga output callbacks > + register_displaychangelistener(&dcl); > + > + qemu_clipboard_peer_register(&cbpeer); > + qemu_mutex_unlock_iothread(); > + qemu_thread_create(&main_thread, "qemu_main_loop", call_qemu_main_loop, > + NULL, QEMU_THREAD_DETACHED); > + > + if (full_screen) { > + [NSApp activateIgnoringOtherApps: YES]; > + [self toggleFullScreen: nil]; > + } > + if (full_grab) { > + [self setFullGrab: nil]; > + } > } > > - (void)applicationWillTerminate:(NSNotification *)aNotification > @@ -1804,9 +1849,6 @@ static void addRemovableDevicesMenuItems(void) > qapi_free_BlockInfoList(pointerToFree); > } > > -@interface QemuCocoaPasteboardTypeOwner : NSObject<NSPasteboardTypeOwner> > -@end > - > @implementation QemuCocoaPasteboardTypeOwner > > - (void)pasteboard:(NSPasteboard *)sender provideDataForType:(NSPasteboardType)type > @@ -1841,8 +1883,6 @@ static void addRemovableDevicesMenuItems(void) > > @end > > -static QemuCocoaPasteboardTypeOwner *cbowner; > - > static void cocoa_clipboard_notify(Notifier *notifier, void *data); > static void cocoa_clipboard_request(QemuClipboardInfo *info, > QemuClipboardType type); > @@ -1903,60 +1943,18 @@ static void cocoa_clipboard_request(QemuClipboardInfo *info, > } > } > > -/* > - * The startup process for the OSX/Cocoa UI is complicated, because > - * OSX insists that the UI runs on the initial main thread, and so we > - * need to start a second thread which runs the vl.c qemu_main(): > - * > - * Initial thread: 2nd thread: > - * in main(): > - * create qemu-main thread > - * wait on display_init semaphore > - * call qemu_main() > - * ... > - * in cocoa_display_init(): > - * post the display_init semaphore > - * wait on app_started semaphore > - * create application, menus, etc > - * enter OSX run loop > - * in applicationDidFinishLaunching: > - * post app_started semaphore > - * tell main thread to fullscreen if needed > - * [...] > - * run qemu main-loop > - * > - * We do this in two stages so that we don't do the creation of the > - * GUI application menus and so on for command line options like --help > - * where we want to just print text to stdout and exit immediately. > - */ > - > -static void *call_qemu_main(void *opaque) > +int main(int argc, char **argv, char **envp) > { > - int status; > - > - COCOA_DEBUG("Second thread: calling qemu_main()\n"); > - status = qemu_main(gArgc, gArgv, *_NSGetEnviron()); > - COCOA_DEBUG("Second thread: qemu_main() returned, exiting\n"); > - [cbowner release]; > - exit(status); > -} > - > -int main (int argc, char **argv) { > - QemuThread thread; > - > COCOA_DEBUG("Entered main()\n"); > - gArgc = argc; > - gArgv = argv; > + qemu_event_init(&cbevent, false); > > - qemu_sem_init(&display_init_sem, 0); > - qemu_sem_init(&app_started_sem, 0); > - > - qemu_thread_create(&thread, "qemu_main", call_qemu_main, > - NULL, QEMU_THREAD_DETACHED); > - > - COCOA_DEBUG("Main thread: waiting for display_init_sem\n"); > - qemu_sem_wait(&display_init_sem); > - COCOA_DEBUG("Main thread: initializing app\n"); > + /* Takes iothread lock, released in applicationDidFinishLaunching:. */ > + qemu_init(argc, argv, envp); > + if (!have_cocoa_ui) { > + qemu_main_loop(); > + qemu_cleanup(); > + return 0; > + } > > NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init]; > > @@ -1978,6 +1976,7 @@ int main (int argc, char **argv) { > */ > add_console_menu_entries(); > addRemovableDevicesMenuItems(); > + cbowner = [[QemuCocoaPasteboardTypeOwner alloc] init]; > > // Create an Application controller > QemuCocoaAppController *appController = [[QemuCocoaAppController alloc] init]; > @@ -2071,24 +2070,13 @@ static void cocoa_refresh(DisplayChangeListener *dcl) > static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts) > { > COCOA_DEBUG("qemu_cocoa: cocoa_display_init\n"); > + have_cocoa_ui = 1; > > - /* Tell main thread to go ahead and create the app and enter the run loop */ > - qemu_sem_post(&display_init_sem); > - qemu_sem_wait(&app_started_sem); > - COCOA_DEBUG("cocoa_display_init: app start completed\n"); > - > - QemuCocoaAppController *controller = (QemuCocoaAppController *)[[NSApplication sharedApplication] delegate]; > - /* if fullscreen mode is to be used */ > if (opts->has_full_screen && opts->full_screen) { > - dispatch_async(dispatch_get_main_queue(), ^{ > - [NSApp activateIgnoringOtherApps: YES]; > - [controller toggleFullScreen: nil]; > - }); > + full_screen = 1; > } > if (opts->u.cocoa.has_full_grab && opts->u.cocoa.full_grab) { > - dispatch_async(dispatch_get_main_queue(), ^{ > - [controller setFullGrab: nil]; > - }); > + full_grab = 1; > } > > if (opts->has_show_cursor && opts->show_cursor) { > @@ -2101,13 +2089,6 @@ static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts) > if (opts->u.cocoa.has_left_command_key && !opts->u.cocoa.left_command_key) { > left_command_key_enabled = 0; > } > - > - // register vga output callbacks > - register_displaychangelistener(&dcl); > - > - qemu_event_init(&cbevent, false); > - cbowner = [[QemuCocoaPasteboardTypeOwner alloc] init]; > - qemu_clipboard_peer_register(&cbpeer); > } > > static QemuDisplay qemu_display_cocoa = {
On 2022/03/17 1:03, Philippe Mathieu-Daudé wrote: > From: Paolo Bonzini <pbonzini@redhat.com> > > Simplify the initialization dance by running qemu_init() in the main > thread before the Cocoa event loop starts. The cocoa_display_init() > code that is post-applicationDidFinishLaunching: moves to the > application delegate itself, and the secondary thread only runs > the rest of qemu_main(), namely qemu_main_loop() and qemu_cleanup(). > > This fixes a case where addRemovableDevicesMenuItems() calls > qmp_query_block() while expecting the main thread to still hold > the BQL. The newly-introduced assertions in the block layer > complain about this. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > Message-Id: <20220307151004.578069-1-pbonzini@redhat.com> > [PMD: Fixed trivial build failures & rebased] > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > Cc: Akihiko Odaki <akihiko.odaki@gmail.com> > Cc: Peter Maydell <peter.maydell@linaro.org> > Cc: Emanuele Giuseppe Esposito <eesposit@redhat.com> > > v1: https://lore.kernel.org/qemu-devel/20220307151004.578069-1-pbonzini@redhat.com/ > --- > softmmu/main.c | 12 ++-- > ui/cocoa.m | 153 ++++++++++++++++++++++--------------------------- > 2 files changed, 74 insertions(+), 91 deletions(-) > > diff --git a/softmmu/main.c b/softmmu/main.c > index 639c67ff48..0c4384e980 100644 > --- a/softmmu/main.c > +++ b/softmmu/main.c > @@ -39,16 +39,18 @@ int main(int argc, char **argv) > #endif > #endif /* CONFIG_SDL */ > > -#ifdef CONFIG_COCOA > -#undef main > -#define main qemu_main > -#endif /* CONFIG_COCOA */ > - > +#ifndef CONFIG_COCOA > int main(int argc, char **argv, char **envp) > { > + /* > + * ui/cocoa.m relies on this being the exact content of main(), > + * because it has to run everything after qemu_init in a secondary > + * thread. > + */ > qemu_init(argc, argv, envp); > qemu_main_loop(); > qemu_cleanup(); > > return 0; > } > +#endif /* CONFIG_COCOA */ > diff --git a/ui/cocoa.m b/ui/cocoa.m > index cb6e7c41dc..e69ce97f44 100644 > --- a/ui/cocoa.m > +++ b/ui/cocoa.m > @@ -75,6 +75,9 @@ typedef struct { > int height; > } QEMUScreen; > > +@interface QemuCocoaPasteboardTypeOwner : NSObject<NSPasteboardTypeOwner> > +@end > + > static void cocoa_update(DisplayChangeListener *dcl, > int x, int y, int w, int h); > > @@ -97,20 +100,23 @@ static int last_buttons; > static int cursor_hide = 1; > static int left_command_key_enabled = 1; > static bool swap_opt_cmd; > +static bool full_screen; > +static bool full_grab; > +static bool have_cocoa_ui; > > -static int gArgc; > -static char **gArgv; > static bool stretch_video; > static NSTextField *pauseLabel; > > -static QemuSemaphore display_init_sem; > -static QemuSemaphore app_started_sem; > static bool allow_events; > > static NSInteger cbchangecount = -1; > static QemuClipboardInfo *cbinfo; > static QemuEvent cbevent; > > +static QemuCocoaPasteboardTypeOwner *cbowner; > +static QemuClipboardPeer cbpeer; > +static QemuThread main_thread; > + > // Utility functions to run specified code block with iothread lock held > typedef void (^CodeBlock)(void); > typedef bool (^BoolCodeBlock)(void); > @@ -142,6 +148,33 @@ static bool bool_with_iothread_lock(BoolCodeBlock block) > return val; > } > > +/* > + * The startup process for the OSX/Cocoa UI is complicated, because > + * OSX insists that the UI runs on the initial main thread, and so we > + * need to start a second thread which runs qemu_main_loop(): > + * > + * Initial thread: 2nd thread: > + * in main(): > + * qemu_init() > + * create application, menus, etc > + * enter OSX run loop > + * in applicationDidFinishLaunching: > + * fullscreen if needed > + * create main loop thread > + * call qemu_main_loop() > + */ > + > +static void *call_qemu_main_loop(void *opaque) > +{ > + COCOA_DEBUG("Second thread: calling qemu_main()\n"); > + qemu_mutex_lock_iothread(); > + qemu_main_loop(); > + COCOA_DEBUG("Second thread: qemu_main_loop() returned, exiting\n"); > + qemu_cleanup(); > + [cbowner release]; > + exit(0); > +} > + > // Mac to QKeyCode conversion > static const int mac_to_qkeycode_map[] = { > [kVK_ANSI_A] = Q_KEY_CODE_A, > @@ -780,9 +813,7 @@ static CGEventRef handleTapEvent(CGEventTapProxy proxy, CGEventType type, CGEven > /* > * Just let OSX have all events that arrive before > * applicationDidFinishLaunching. > - * This avoids a deadlock on the iothread lock, which cocoa_display_init() > - * will not drop until after the app_started_sem is posted. (In theory > - * there should not be any such events, but OSX Catalina now emits some.) > + * This may not be needed anymore? > */ > return false; > } > @@ -1280,8 +1311,22 @@ static CGEventRef handleTapEvent(CGEventTapProxy proxy, CGEventType type, CGEven > { > COCOA_DEBUG("QemuCocoaAppController: applicationDidFinishLaunching\n"); > allow_events = true; > - /* Tell cocoa_display_init to proceed */ > - qemu_sem_post(&app_started_sem); > + > + // register vga output callbacks > + register_displaychangelistener(&dcl); > + > + qemu_clipboard_peer_register(&cbpeer); > + qemu_mutex_unlock_iothread(); > + qemu_thread_create(&main_thread, "qemu_main_loop", call_qemu_main_loop, > + NULL, QEMU_THREAD_DETACHED); > + > + if (full_screen) { > + [NSApp activateIgnoringOtherApps: YES]; > + [self toggleFullScreen: nil]; > + } > + if (full_grab) { > + [self setFullGrab: nil]; > + } > } > > - (void)applicationWillTerminate:(NSNotification *)aNotification > @@ -1804,9 +1849,6 @@ static void addRemovableDevicesMenuItems(void) > qapi_free_BlockInfoList(pointerToFree); > } > > -@interface QemuCocoaPasteboardTypeOwner : NSObject<NSPasteboardTypeOwner> > -@end > - > @implementation QemuCocoaPasteboardTypeOwner > > - (void)pasteboard:(NSPasteboard *)sender provideDataForType:(NSPasteboardType)type > @@ -1841,8 +1883,6 @@ static void addRemovableDevicesMenuItems(void) > > @end > > -static QemuCocoaPasteboardTypeOwner *cbowner; > - > static void cocoa_clipboard_notify(Notifier *notifier, void *data); > static void cocoa_clipboard_request(QemuClipboardInfo *info, > QemuClipboardType type); > @@ -1903,60 +1943,18 @@ static void cocoa_clipboard_request(QemuClipboardInfo *info, > } > } > > -/* > - * The startup process for the OSX/Cocoa UI is complicated, because > - * OSX insists that the UI runs on the initial main thread, and so we > - * need to start a second thread which runs the vl.c qemu_main(): > - * > - * Initial thread: 2nd thread: > - * in main(): > - * create qemu-main thread > - * wait on display_init semaphore > - * call qemu_main() > - * ... > - * in cocoa_display_init(): > - * post the display_init semaphore > - * wait on app_started semaphore > - * create application, menus, etc > - * enter OSX run loop > - * in applicationDidFinishLaunching: > - * post app_started semaphore > - * tell main thread to fullscreen if needed > - * [...] > - * run qemu main-loop > - * > - * We do this in two stages so that we don't do the creation of the > - * GUI application menus and so on for command line options like --help > - * where we want to just print text to stdout and exit immediately. > - */ > - > -static void *call_qemu_main(void *opaque) > +int main(int argc, char **argv, char **envp) > { > - int status; > - > - COCOA_DEBUG("Second thread: calling qemu_main()\n"); > - status = qemu_main(gArgc, gArgv, *_NSGetEnviron()); > - COCOA_DEBUG("Second thread: qemu_main() returned, exiting\n"); > - [cbowner release]; > - exit(status); > -} > - > -int main (int argc, char **argv) { > - QemuThread thread; > - > COCOA_DEBUG("Entered main()\n"); > - gArgc = argc; > - gArgv = argv; > + qemu_event_init(&cbevent, false); > > - qemu_sem_init(&display_init_sem, 0); > - qemu_sem_init(&app_started_sem, 0); > - > - qemu_thread_create(&thread, "qemu_main", call_qemu_main, > - NULL, QEMU_THREAD_DETACHED); > - > - COCOA_DEBUG("Main thread: waiting for display_init_sem\n"); > - qemu_sem_wait(&display_init_sem); > - COCOA_DEBUG("Main thread: initializing app\n"); > + /* Takes iothread lock, released in applicationDidFinishLaunching:. */ > + qemu_init(argc, argv, envp); > + if (!have_cocoa_ui) { > + qemu_main_loop(); > + qemu_cleanup(); > + return 0; > + } > > NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init]; > > @@ -1978,6 +1976,7 @@ int main (int argc, char **argv) { > */ > add_console_menu_entries(); > addRemovableDevicesMenuItems(); > + cbowner = [[QemuCocoaPasteboardTypeOwner alloc] init]; > > // Create an Application controller > QemuCocoaAppController *appController = [[QemuCocoaAppController alloc] init]; > @@ -2071,24 +2070,13 @@ static void cocoa_refresh(DisplayChangeListener *dcl) > static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts) > { > COCOA_DEBUG("qemu_cocoa: cocoa_display_init\n"); > + have_cocoa_ui = 1; > > - /* Tell main thread to go ahead and create the app and enter the run loop */ > - qemu_sem_post(&display_init_sem); > - qemu_sem_wait(&app_started_sem); > - COCOA_DEBUG("cocoa_display_init: app start completed\n"); > - > - QemuCocoaAppController *controller = (QemuCocoaAppController *)[[NSApplication sharedApplication] delegate]; > - /* if fullscreen mode is to be used */ > if (opts->has_full_screen && opts->full_screen) { > - dispatch_async(dispatch_get_main_queue(), ^{ > - [NSApp activateIgnoringOtherApps: YES]; > - [controller toggleFullScreen: nil]; > - }); > + full_screen = 1; We could just save opts and use it later. sdl2 does this. Regards, Akihiko Odaki > } > if (opts->u.cocoa.has_full_grab && opts->u.cocoa.full_grab) { > - dispatch_async(dispatch_get_main_queue(), ^{ > - [controller setFullGrab: nil]; > - }); > + full_grab = 1; > } > > if (opts->has_show_cursor && opts->show_cursor) { > @@ -2101,13 +2089,6 @@ static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts) > if (opts->u.cocoa.has_left_command_key && !opts->u.cocoa.left_command_key) { > left_command_key_enabled = 0; > } > - > - // register vga output callbacks > - register_displaychangelistener(&dcl); > - > - qemu_event_init(&cbevent, false); > - cbowner = [[QemuCocoaPasteboardTypeOwner alloc] init]; > - qemu_clipboard_peer_register(&cbpeer); > } > > static QemuDisplay qemu_display_cocoa = {
On 3/16/22 17:22, Akihiko Odaki wrote: > I was thinking that it may be better to let softmmu/main.c do the > details if it involves the internals of qemu_main() like qemu_main_loop(). > > More concretely, softmmu/main.c would provide a function to register a > function pointer to take over the main thread. main() implemented in > softmmu/main.c would call qemu_init(). If a function pointer gets > registered in qemu_init(), it would create a thread for main loop and > call the registered function pointer. Otherwise, it would directly call > qemu_main_loop(). > > It would be a semantically appropriate division of ui/cocoa.m and > softmmu/main.c. It would also be beneficial for end-users as it would > also allow to isolate ui/cocoa.m into a separate module when > --enable-modules in the future. (With "In the future", I mean sometime > when we have time to hack Meson build files and some details we cannot > fill by 7.0.) I would like this for 7.1. Basically rename qemu_main_loop to qemu_default_main_loop, and cocoa_display_init would do qemu_main_loop = qemu_cocoa_main_loop; qemu_cocoa_main_loop would include the bulk of the current main of ui/cocoa.m. Seems like a good idea. Paolo
On Wed, 16 Mar 2022 at 17:31, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 3/16/22 17:22, Akihiko Odaki wrote: > > I was thinking that it may be better to let softmmu/main.c do the > > details if it involves the internals of qemu_main() like qemu_main_loop(). > > > > More concretely, softmmu/main.c would provide a function to register a > > function pointer to take over the main thread. main() implemented in > > softmmu/main.c would call qemu_init(). If a function pointer gets > > registered in qemu_init(), it would create a thread for main loop and > > call the registered function pointer. Otherwise, it would directly call > > qemu_main_loop(). > > > > It would be a semantically appropriate division of ui/cocoa.m and > > softmmu/main.c. It would also be beneficial for end-users as it would > > also allow to isolate ui/cocoa.m into a separate module when > > --enable-modules in the future. (With "In the future", I mean sometime > > when we have time to hack Meson build files and some details we cannot > > fill by 7.0.) > > I would like this for 7.1. > > Basically rename qemu_main_loop to qemu_default_main_loop, and > cocoa_display_init would do > > qemu_main_loop = qemu_cocoa_main_loop; > > qemu_cocoa_main_loop would include the bulk of the current main of > ui/cocoa.m. Seems like a good idea. Speaking of 7.1, is cocoa currently completely broken, ie in need of an interim fix for 7.0 ? If so, which of the various patches/approaches should it be ? thanks -- PMM
On Wed, 16 Mar 2022 at 19:29, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Wed, 16 Mar 2022 at 17:31, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > On 3/16/22 17:22, Akihiko Odaki wrote: > > > I was thinking that it may be better to let softmmu/main.c do the > > > details if it involves the internals of qemu_main() like qemu_main_loop(). > > > > > > More concretely, softmmu/main.c would provide a function to register a > > > function pointer to take over the main thread. main() implemented in > > > softmmu/main.c would call qemu_init(). If a function pointer gets > > > registered in qemu_init(), it would create a thread for main loop and > > > call the registered function pointer. Otherwise, it would directly call > > > qemu_main_loop(). > > > > > > It would be a semantically appropriate division of ui/cocoa.m and > > > softmmu/main.c. It would also be beneficial for end-users as it would > > > also allow to isolate ui/cocoa.m into a separate module when > > > --enable-modules in the future. (With "In the future", I mean sometime > > > when we have time to hack Meson build files and some details we cannot > > > fill by 7.0.) > > > > I would like this for 7.1. > > > > Basically rename qemu_main_loop to qemu_default_main_loop, and > > cocoa_display_init would do > > > > qemu_main_loop = qemu_cocoa_main_loop; > > > > qemu_cocoa_main_loop would include the bulk of the current main of > > ui/cocoa.m. Seems like a good idea. > > Speaking of 7.1, is cocoa currently completely broken, ie in need > of an interim fix for 7.0 ? If so, which of the various patches/approaches > should it be ? To answer the first half of my question, yes, the cocoa UI is currently completely broken as it asserts on startup. -- PMM
On 3/16/22 22:06, Peter Maydell wrote: >> Speaking of 7.1, is cocoa currently completely broken, ie in need >> of an interim fix for 7.0 ? If so, which of the various patches/approaches >> should it be ? > > To answer the first half of my question, yes, the cocoa UI is > currently completely broken as it asserts on startup. I think this patch is a good interim fix for 7.0 and an improvement in general. Any other changes can be deferred to 7.1. For example it's trivial for Cocoa not to block GTK+/SDL anymore, but no need to have that in 7.0. Same for removing main() from ui/cocoa.m. Paolo
Just one change to aid future reading of the code, possibly. Move this
line:
On 3/16/22 17:03, Philippe Mathieu-Daudé wrote:
> + qemu_event_init(&cbevent, false);
just before
+ cbowner = [[QemuCocoaPasteboardTypeOwner alloc] init];
i.e. the place where it was before the patch, in cocoa_display_init.
Paolo
On 17/3/22 12:57, Paolo Bonzini wrote: > Just one change to aid future reading of the code, possibly. Move this > line: > > On 3/16/22 17:03, Philippe Mathieu-Daudé wrote: >> + qemu_event_init(&cbevent, false); > > just before > > + cbowner = [[QemuCocoaPasteboardTypeOwner alloc] init]; > > i.e. the place where it was before the patch, in cocoa_display_init. OK. I'll also include this change (s/cocoa_display_init/applicationDidFinishLaunching): @@ -611,17 +611,17 @@ - (void) updateUIInfo { if (!allow_events) { /* * Don't try to tell QEMU about UI information in the application * startup phase -- we haven't yet registered dcl with the QEMU UI * layer, and also trying to take the iothread lock would deadlock. - * When cocoa_display_init() does register the dcl, the UI layer - * will call cocoa_switch(), which will call updateUIInfo, so - * we don't lose any information here. + * When applicationDidFinishLaunching() does register the dcl, the + * UI layer will call cocoa_switch(), which will call updateUIInfo, + * so we don't lose any information here. */ return; }
diff --git a/softmmu/main.c b/softmmu/main.c index 639c67ff48..0c4384e980 100644 --- a/softmmu/main.c +++ b/softmmu/main.c @@ -39,16 +39,18 @@ int main(int argc, char **argv) #endif #endif /* CONFIG_SDL */ -#ifdef CONFIG_COCOA -#undef main -#define main qemu_main -#endif /* CONFIG_COCOA */ - +#ifndef CONFIG_COCOA int main(int argc, char **argv, char **envp) { + /* + * ui/cocoa.m relies on this being the exact content of main(), + * because it has to run everything after qemu_init in a secondary + * thread. + */ qemu_init(argc, argv, envp); qemu_main_loop(); qemu_cleanup(); return 0; } +#endif /* CONFIG_COCOA */ diff --git a/ui/cocoa.m b/ui/cocoa.m index cb6e7c41dc..e69ce97f44 100644 --- a/ui/cocoa.m +++ b/ui/cocoa.m @@ -75,6 +75,9 @@ typedef struct { int height; } QEMUScreen; +@interface QemuCocoaPasteboardTypeOwner : NSObject<NSPasteboardTypeOwner> +@end + static void cocoa_update(DisplayChangeListener *dcl, int x, int y, int w, int h); @@ -97,20 +100,23 @@ static int last_buttons; static int cursor_hide = 1; static int left_command_key_enabled = 1; static bool swap_opt_cmd; +static bool full_screen; +static bool full_grab; +static bool have_cocoa_ui; -static int gArgc; -static char **gArgv; static bool stretch_video; static NSTextField *pauseLabel; -static QemuSemaphore display_init_sem; -static QemuSemaphore app_started_sem; static bool allow_events; static NSInteger cbchangecount = -1; static QemuClipboardInfo *cbinfo; static QemuEvent cbevent; +static QemuCocoaPasteboardTypeOwner *cbowner; +static QemuClipboardPeer cbpeer; +static QemuThread main_thread; + // Utility functions to run specified code block with iothread lock held typedef void (^CodeBlock)(void); typedef bool (^BoolCodeBlock)(void); @@ -142,6 +148,33 @@ static bool bool_with_iothread_lock(BoolCodeBlock block) return val; } +/* + * The startup process for the OSX/Cocoa UI is complicated, because + * OSX insists that the UI runs on the initial main thread, and so we + * need to start a second thread which runs qemu_main_loop(): + * + * Initial thread: 2nd thread: + * in main(): + * qemu_init() + * create application, menus, etc + * enter OSX run loop + * in applicationDidFinishLaunching: + * fullscreen if needed + * create main loop thread + * call qemu_main_loop() + */ + +static void *call_qemu_main_loop(void *opaque) +{ + COCOA_DEBUG("Second thread: calling qemu_main()\n"); + qemu_mutex_lock_iothread(); + qemu_main_loop(); + COCOA_DEBUG("Second thread: qemu_main_loop() returned, exiting\n"); + qemu_cleanup(); + [cbowner release]; + exit(0); +} + // Mac to QKeyCode conversion static const int mac_to_qkeycode_map[] = { [kVK_ANSI_A] = Q_KEY_CODE_A, @@ -780,9 +813,7 @@ static CGEventRef handleTapEvent(CGEventTapProxy proxy, CGEventType type, CGEven /* * Just let OSX have all events that arrive before * applicationDidFinishLaunching. - * This avoids a deadlock on the iothread lock, which cocoa_display_init() - * will not drop until after the app_started_sem is posted. (In theory - * there should not be any such events, but OSX Catalina now emits some.) + * This may not be needed anymore? */ return false; } @@ -1280,8 +1311,22 @@ static CGEventRef handleTapEvent(CGEventTapProxy proxy, CGEventType type, CGEven { COCOA_DEBUG("QemuCocoaAppController: applicationDidFinishLaunching\n"); allow_events = true; - /* Tell cocoa_display_init to proceed */ - qemu_sem_post(&app_started_sem); + + // register vga output callbacks + register_displaychangelistener(&dcl); + + qemu_clipboard_peer_register(&cbpeer); + qemu_mutex_unlock_iothread(); + qemu_thread_create(&main_thread, "qemu_main_loop", call_qemu_main_loop, + NULL, QEMU_THREAD_DETACHED); + + if (full_screen) { + [NSApp activateIgnoringOtherApps: YES]; + [self toggleFullScreen: nil]; + } + if (full_grab) { + [self setFullGrab: nil]; + } } - (void)applicationWillTerminate:(NSNotification *)aNotification @@ -1804,9 +1849,6 @@ static void addRemovableDevicesMenuItems(void) qapi_free_BlockInfoList(pointerToFree); } -@interface QemuCocoaPasteboardTypeOwner : NSObject<NSPasteboardTypeOwner> -@end - @implementation QemuCocoaPasteboardTypeOwner - (void)pasteboard:(NSPasteboard *)sender provideDataForType:(NSPasteboardType)type @@ -1841,8 +1883,6 @@ static void addRemovableDevicesMenuItems(void) @end -static QemuCocoaPasteboardTypeOwner *cbowner; - static void cocoa_clipboard_notify(Notifier *notifier, void *data); static void cocoa_clipboard_request(QemuClipboardInfo *info, QemuClipboardType type); @@ -1903,60 +1943,18 @@ static void cocoa_clipboard_request(QemuClipboardInfo *info, } } -/* - * The startup process for the OSX/Cocoa UI is complicated, because - * OSX insists that the UI runs on the initial main thread, and so we - * need to start a second thread which runs the vl.c qemu_main(): - * - * Initial thread: 2nd thread: - * in main(): - * create qemu-main thread - * wait on display_init semaphore - * call qemu_main() - * ... - * in cocoa_display_init(): - * post the display_init semaphore - * wait on app_started semaphore - * create application, menus, etc - * enter OSX run loop - * in applicationDidFinishLaunching: - * post app_started semaphore - * tell main thread to fullscreen if needed - * [...] - * run qemu main-loop - * - * We do this in two stages so that we don't do the creation of the - * GUI application menus and so on for command line options like --help - * where we want to just print text to stdout and exit immediately. - */ - -static void *call_qemu_main(void *opaque) +int main(int argc, char **argv, char **envp) { - int status; - - COCOA_DEBUG("Second thread: calling qemu_main()\n"); - status = qemu_main(gArgc, gArgv, *_NSGetEnviron()); - COCOA_DEBUG("Second thread: qemu_main() returned, exiting\n"); - [cbowner release]; - exit(status); -} - -int main (int argc, char **argv) { - QemuThread thread; - COCOA_DEBUG("Entered main()\n"); - gArgc = argc; - gArgv = argv; + qemu_event_init(&cbevent, false); - qemu_sem_init(&display_init_sem, 0); - qemu_sem_init(&app_started_sem, 0); - - qemu_thread_create(&thread, "qemu_main", call_qemu_main, - NULL, QEMU_THREAD_DETACHED); - - COCOA_DEBUG("Main thread: waiting for display_init_sem\n"); - qemu_sem_wait(&display_init_sem); - COCOA_DEBUG("Main thread: initializing app\n"); + /* Takes iothread lock, released in applicationDidFinishLaunching:. */ + qemu_init(argc, argv, envp); + if (!have_cocoa_ui) { + qemu_main_loop(); + qemu_cleanup(); + return 0; + } NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init]; @@ -1978,6 +1976,7 @@ int main (int argc, char **argv) { */ add_console_menu_entries(); addRemovableDevicesMenuItems(); + cbowner = [[QemuCocoaPasteboardTypeOwner alloc] init]; // Create an Application controller QemuCocoaAppController *appController = [[QemuCocoaAppController alloc] init]; @@ -2071,24 +2070,13 @@ static void cocoa_refresh(DisplayChangeListener *dcl) static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts) { COCOA_DEBUG("qemu_cocoa: cocoa_display_init\n"); + have_cocoa_ui = 1; - /* Tell main thread to go ahead and create the app and enter the run loop */ - qemu_sem_post(&display_init_sem); - qemu_sem_wait(&app_started_sem); - COCOA_DEBUG("cocoa_display_init: app start completed\n"); - - QemuCocoaAppController *controller = (QemuCocoaAppController *)[[NSApplication sharedApplication] delegate]; - /* if fullscreen mode is to be used */ if (opts->has_full_screen && opts->full_screen) { - dispatch_async(dispatch_get_main_queue(), ^{ - [NSApp activateIgnoringOtherApps: YES]; - [controller toggleFullScreen: nil]; - }); + full_screen = 1; } if (opts->u.cocoa.has_full_grab && opts->u.cocoa.full_grab) { - dispatch_async(dispatch_get_main_queue(), ^{ - [controller setFullGrab: nil]; - }); + full_grab = 1; } if (opts->has_show_cursor && opts->show_cursor) { @@ -2101,13 +2089,6 @@ static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts) if (opts->u.cocoa.has_left_command_key && !opts->u.cocoa.left_command_key) { left_command_key_enabled = 0; } - - // register vga output callbacks - register_displaychangelistener(&dcl); - - qemu_event_init(&cbevent, false); - cbowner = [[QemuCocoaPasteboardTypeOwner alloc] init]; - qemu_clipboard_peer_register(&cbpeer); } static QemuDisplay qemu_display_cocoa = {