[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] ui/cocoa: Set UI information
From: |
Akihiko Odaki |
Subject: |
Re: [PATCH] ui/cocoa: Set UI information |
Date: |
Sat, 5 Feb 2022 11:06:12 +0900 |
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