ui/cocoa.m | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)
Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
---
ui/cocoa.m | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 49 insertions(+)
diff --git a/ui/cocoa.m b/ui/cocoa.m
index 995301502be..8b83f91723a 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -540,6 +540,43 @@ - (void) setContentDimensions
}
}
+- (void) updateUIInfo
+{
+ NSSize frameSize;
+ QemuUIInfo info;
+
+ if (!qemu_console_is_graphic(dcl.con)) {
+ return;
+ }
+
+ if ([self window]) {
+ NSDictionary *description = [[[self window] screen] deviceDescription];
+ CGDirectDisplayID display = [[description objectForKey:@"NSScreenNumber"] unsignedIntValue];
+ NSSize screenSize = [[[self window] screen] frame].size;
+ CGSize screenPhysicalSize = CGDisplayScreenSize(display);
+
+ frameSize = isFullscreen ? screenSize : [self frame].size;
+ info.width_mm = frameSize.width / screenSize.width * screenPhysicalSize.width;
+ info.height_mm = frameSize.height / screenSize.height * screenPhysicalSize.height;
+ } else {
+ frameSize = [self frame].size;
+ info.width_mm = 0;
+ info.height_mm = 0;
+ }
+
+ info.xoff = 0;
+ info.yoff = 0;
+ info.width = frameSize.width;
+ info.height = frameSize.height;
+
+ dpy_set_ui_info(dcl.con, &info);
+}
+
+- (void)viewDidMoveToWindow
+{
+ [self updateUIInfo];
+}
+
- (void) switchSurface:(pixman_image_t *)image
{
COCOA_DEBUG("QemuCocoaView: switchSurface\n");
@@ -1258,6 +1295,16 @@ - (NSApplicationTerminateReply)applicationShouldTerminate:
return [self verifyQuit];
}
+- (void)windowDidChangeScreen:(NSNotification *)notification
+{
+ [cocoaView updateUIInfo];
+}
+
+- (void)windowDidResize:(NSNotification *)notification
+{
+ [cocoaView updateUIInfo];
+}
+
/* Called when the user clicks on a window's close button */
- (BOOL)windowShouldClose:(id)sender
{
@@ -1936,6 +1983,8 @@ static void cocoa_switch(DisplayChangeListener *dcl,
COCOA_DEBUG("qemu_cocoa: cocoa_switch\n");
+ [cocoaView updateUIInfo];
+
// The DisplaySurface will be freed as soon as this callback returns.
// We take a reference to the underlying pixman image here so it does
// not disappear from under our feet; the switchSurface method will
--
2.30.1 (Apple Git-130)
On Wed, Jun 16, 2021 at 11:19:10PM +0900, Akihiko Odaki wrote: > Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com> > --- > ui/cocoa.m | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > > diff --git a/ui/cocoa.m b/ui/cocoa.m > index 995301502be..8b83f91723a 100644 > --- a/ui/cocoa.m > +++ b/ui/cocoa.m > @@ -540,6 +540,43 @@ - (void) setContentDimensions > } > } Added to UI queue. thanks, Gerd
On Wed, 16 Jun 2021 at 15:19, Akihiko Odaki <akihiko.odaki@gmail.com> wrote: > > Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com> > --- > ui/cocoa.m | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) Hi; I was looking at the cocoa.m code to see about maybe deleting the unnecessary autorelease pools, and I ran into some code I was a bit unsure about that was added in this patch. In particular I'm wondering about the interactions across threads here. > +- (void) updateUIInfo > +{ > + NSSize frameSize; > + QemuUIInfo info; > + > + if (!qemu_console_is_graphic(dcl.con)) { > + return; > + } > + > + if ([self window]) { > + NSDictionary *description = [[[self window] screen] deviceDescription]; > + CGDirectDisplayID display = [[description objectForKey:@"NSScreenNumber"] unsignedIntValue]; > + NSSize screenSize = [[[self window] screen] frame].size; > + CGSize screenPhysicalSize = CGDisplayScreenSize(display); > + > + frameSize = isFullscreen ? screenSize : [self frame].size; > + info.width_mm = frameSize.width / screenSize.width * screenPhysicalSize.width; > + info.height_mm = frameSize.height / screenSize.height * screenPhysicalSize.height; > + } else { > + frameSize = [self frame].size; > + info.width_mm = 0; > + info.height_mm = 0; > + } > + > + info.xoff = 0; > + info.yoff = 0; > + info.width = frameSize.width; > + info.height = frameSize.height; > + > + dpy_set_ui_info(dcl.con, &info); This function makes some cocoa calls, and it also calls a QEMU UI layer function, dpy_set_ui_info(). > +- (void)windowDidChangeScreen:(NSNotification *)notification > +{ > + [cocoaView updateUIInfo]; We call it from the cocoa UI thread in callbacks like this. > /* Called when the user clicks on a window's close button */ > - (BOOL)windowShouldClose:(id)sender > { > @@ -1936,6 +1983,8 @@ static void cocoa_switch(DisplayChangeListener *dcl, > > COCOA_DEBUG("qemu_cocoa: cocoa_switch\n"); > > + [cocoaView updateUIInfo]; We also call it from the QEMU thread, when the QEMU thread calls this cocoa_switch callback function. (1) A question for Akihiko: Are all the cocoa calls we make in updateUIInfo safe to make from a non-UI thread? Would it be preferable for this call in cocoa_switch() to be moved inside the dispatch_async block? (Moving it would mean that I wouldn't have to think about whether any of the code in it needs to have an autorelease pool :-)) (2) A question for Gerd: Is it safe to call dpy_set_ui_info() from a non-QEMU-main-thread? It doesn't appear to do any locking that would protect against multiple threads calling it simultaneously. thanks -- PMM
On Sat, Feb 5, 2022 at 1:19 AM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Wed, 16 Jun 2021 at 15:19, Akihiko Odaki <akihiko.odaki@gmail.com> wrote: > > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com> > > --- > > ui/cocoa.m | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 49 insertions(+) > > Hi; I was looking at the cocoa.m code to see about maybe deleting the > unnecessary autorelease pools, and I ran into some code I was a bit > unsure about that was added in this patch. > > In particular I'm wondering about the interactions across threads here. > > > +- (void) updateUIInfo > > +{ > > + NSSize frameSize; > > + QemuUIInfo info; > > + > > + if (!qemu_console_is_graphic(dcl.con)) { > > + return; > > + } > > + > > + if ([self window]) { > > + NSDictionary *description = [[[self window] screen] deviceDescription]; > > + CGDirectDisplayID display = [[description objectForKey:@"NSScreenNumber"] unsignedIntValue]; > > + NSSize screenSize = [[[self window] screen] frame].size; > > + CGSize screenPhysicalSize = CGDisplayScreenSize(display); > > + > > + frameSize = isFullscreen ? screenSize : [self frame].size; > > + info.width_mm = frameSize.width / screenSize.width * screenPhysicalSize.width; > > + info.height_mm = frameSize.height / screenSize.height * screenPhysicalSize.height; > > + } else { > > + frameSize = [self frame].size; > > + info.width_mm = 0; > > + info.height_mm = 0; > > + } > > + > > + info.xoff = 0; > > + info.yoff = 0; > > + info.width = frameSize.width; > > + info.height = frameSize.height; > > + > > + dpy_set_ui_info(dcl.con, &info); > > This function makes some cocoa calls, and it also calls a QEMU > UI layer function, dpy_set_ui_info(). > > > +- (void)windowDidChangeScreen:(NSNotification *)notification > > +{ > > + [cocoaView updateUIInfo]; > > We call it from the cocoa UI thread in callbacks like this. > > > /* Called when the user clicks on a window's close button */ > > - (BOOL)windowShouldClose:(id)sender > > { > > @@ -1936,6 +1983,8 @@ static void cocoa_switch(DisplayChangeListener *dcl, > > > > COCOA_DEBUG("qemu_cocoa: cocoa_switch\n"); > > > > + [cocoaView updateUIInfo]; > > We also call it from the QEMU thread, when the QEMU thread calls > this cocoa_switch callback function. > > (1) A question for Akihiko: > Are all the cocoa calls we make in updateUIInfo safe to > make from a non-UI thread? Would it be preferable for this > call in cocoa_switch() to be moved inside the dispatch_async block? > (Moving it would mean that I wouldn't have to think about whether > any of the code in it needs to have an autorelease pool :-)) It is not safe. Apparently I totally forgot about threads when I wrote this. It should be moved in the dispatch_async block as you suggest. Should I write a patch, or will you write one before you delete autorelease pools? Regards, Akihiko Odaki > > (2) A question for Gerd: > Is it safe to call dpy_set_ui_info() from a non-QEMU-main-thread? > It doesn't appear to do any locking that would protect against > multiple threads calling it simultaneously. > > thanks > -- PMM
On Sat, 5 Feb 2022 at 02:06, Akihiko Odaki <akihiko.odaki@gmail.com> wrote: > > On Sat, Feb 5, 2022 at 1:19 AM Peter Maydell <peter.maydell@linaro.org> wrote > > (1) A question for Akihiko: > > Are all the cocoa calls we make in updateUIInfo safe to > > make from a non-UI thread? Would it be preferable for this > > call in cocoa_switch() to be moved inside the dispatch_async block? > > (Moving it would mean that I wouldn't have to think about whether > > any of the code in it needs to have an autorelease pool :-)) > > It is not safe. Apparently I totally forgot about threads when I wrote this. > > It should be moved in the dispatch_async block as you suggest. Should > I write a patch, or will you write one before you delete autorelease > pools? I'll write a patchset. If you have time to test it when I send it out that would be great. Incidentally, I think the answer to my other question > > Is it safe to call dpy_set_ui_info() from a non-QEMU-main-thread? is "no, and so the body of updateUIInfo should be enclosed in a with_iothread_lock block". -- PMM
On Wed, Feb 9, 2022 at 3:07 AM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Sat, 5 Feb 2022 at 02:06, Akihiko Odaki <akihiko.odaki@gmail.com> wrote: > > > > On Sat, Feb 5, 2022 at 1:19 AM Peter Maydell <peter.maydell@linaro.org> wrote > > > > (1) A question for Akihiko: > > > Are all the cocoa calls we make in updateUIInfo safe to > > > make from a non-UI thread? Would it be preferable for this > > > call in cocoa_switch() to be moved inside the dispatch_async block? > > > (Moving it would mean that I wouldn't have to think about whether > > > any of the code in it needs to have an autorelease pool :-)) > > > > It is not safe. Apparently I totally forgot about threads when I wrote this. > > > > It should be moved in the dispatch_async block as you suggest. Should > > I write a patch, or will you write one before you delete autorelease > > pools? > > I'll write a patchset. If you have time to test it when I send it out > that would be great. Thanks, I will test the patchset soon after I receive it. > > Incidentally, I think the answer to my other question > > > Is it safe to call dpy_set_ui_info() from a non-QEMU-main-thread? > > is "no, and so the body of updateUIInfo should be enclosed in a > with_iothread_lock block". > > -- PMM
> (2) A question for Gerd: > Is it safe to call dpy_set_ui_info() from a non-QEMU-main-thread? No. take care, Gerd
On Mon, 14 Feb 2022 at 10:27, Gerd Hoffmann <kraxel@redhat.com> wrote: > > > (2) A question for Gerd: > > Is it safe to call dpy_set_ui_info() from a non-QEMU-main-thread? > > No. Is it OK to do so if the other thread is holding the iothread lock? (This is how we do a lot of the other "need to call a QEMU function" work from the cocoa UI thread.) thanks -- PMM
On Mon, Feb 14, 2022 at 10:43:32AM +0000, Peter Maydell wrote: > On Mon, 14 Feb 2022 at 10:27, Gerd Hoffmann <kraxel@redhat.com> wrote: > > > > > (2) A question for Gerd: > > > Is it safe to call dpy_set_ui_info() from a non-QEMU-main-thread? > > > > No. > > Is it OK to do so if the other thread is holding the iothread lock? > (This is how we do a lot of the other "need to call a QEMU function" > work from the cocoa UI thread.) That should work. take care, Gerd
© 2016 - 2024 Red Hat, Inc.