Re: [PATCH v2 08/12] macfb: add common monitor modes supported by the MacOS toolbox ROM

[Mark Cave-Ayland]

On 06/10/2021 22:16, Laurent Vivier wrote:

> Le 06/10/2021 à 21:24, Laurent Vivier a écrit :
> > Le 06/10/2021 à 18:09, Mark Cave-Ayland a écrit :
> > > On 06/10/2021 16:46, Laurent Vivier wrote:
> > > > Le 06/10/2021 à 15:54, Mark Cave-Ayland a écrit :
> > > > > On 06/10/2021 13:24, Laurent Vivier wrote:
> > > > >
> > > > > > > This is where it becomes a bit trickier, since technically booting Linux with 
> > > > > > > -kernel you can use
> > > > > > > any supported values as long as everything fits in the video RAM which is why 
> > > > > > > there isn't
> > > > > > > currently
> > > > > > > a hard-coded list :)
> > > > > >
> > > > > > We need the list of "supported values". I don't want to read the code or try 
> > > > > > values combination
> > > > > > until it works.
> > > > > >
> > > > > > In a perfect world, I would like to be able to use any value I want with 
> > > > > > "-kernel".
> > > > > >
> > > > > > For instance I was using "-g 1200x800x24" and it was working fine.
> > > > > >
> > > > > > Now I have:
> > > > > >
> > > > > > qemu-system-m68k: unknown display mode: width 1200, height 800, depth 24
> > > > > >
> > > > > > If it's not possible (because the original hardware cannot provide it, and we 
> > > > > > don't know the base
> > > > > > address or things like that), we don't need the list of the display id, but the 
> > > > > > list of available
> > > > > > modes: (width,height,depth).
> > > > > >
> > > > > > Rougly, something like:
> > > > > >
> > > > > > qemu-system-m68k: unknown display mode: width 1200, height 800, depth 24
> > > > > > Available modes:
> > > > > >        1152x870x8
> > > > > >        1152x870x4
> > > > > >        1152x870x2
> > > > > >        1152x870x1
> > > > > >        800x600x24
> > > > > >        800x600x8
> > > > > >        800x600x4
> > > > > >        800x600x2
> > > > > >        800x600x1
> > > > > >        640x480x24
> > > > > >        640x480x8
> > > > > >        640x480x4
> > > > > >        640x480x2
> > > > > >        640x480x1
> > > > > >
> > > > > > diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> > > > > > index 5b8812e9e7d8..4b352eb89c3f 100644
> > > > > > --- a/hw/display/macfb.c
> > > > > > +++ b/hw/display/macfb.c
> > > > > > @@ -438,6 +438,26 @@ static MacFbMode *macfb_find_mode(MacfbDisplayType 
> > > > > > display_type,
> > > > > >         return NULL;
> > > > > >     }
> > > > > >
> > > > > > +static gchar *macfb_mode_list(void)
> > > > > > +{
> > > > > > +    gchar *list = NULL;
> > > > > > +    gchar *mode;
> > > > > > +    MacFbMode *macfb_mode;
> > > > > > +    int i;
> > > > > > +
> > > > > > +    for (i = 0; i < ARRAY_SIZE(macfb_mode_table); i++) {
> > > > > > +        macfb_mode = &macfb_mode_table[i];
> > > > > > +
> > > > > > +        mode = g_strdup_printf("    %dx%dx%d\n", macfb_mode->width,
> > > > > > +                               macfb_mode->height, macfb_mode->depth);
> > > > > > +        list = g_strconcat(mode, list, NULL);
> > > > > > +        g_free(mode);
> > > > > > +    }
> > > > > > +
> > > > > > +    return list;
> > > > > > +}
> > > > > > +
> > > > > > +
> > > > > >     static void macfb_update_display(void *opaque)
> > > > > >     {
> > > > > >         MacfbState *s = opaque;
> > > > > > @@ -620,8 +640,13 @@ static bool macfb_common_realize(DeviceState *dev, 
> > > > > > MacfbState *s, Error
> > > > > > **errp)
> > > > > >
> > > > > >         s->mode = macfb_find_mode(s->type, s->width, s->height, s->depth);
> > > > > >         if (!s->mode) {
> > > > > > +        gchar *list;
> > > > > >             error_setg(errp, "unknown display mode: width %d, height %d, depth 
> > > > > > %d",
> > > > > >                        s->width, s->height, s->depth);
> > > > > > +        list =  macfb_mode_list();
> > > > > > +        error_append_hint(errp, "Available modes:\n%s", list);
> > > > > > +        g_free(list);
> > > > > > +
> > > > > >             return false;
> > > > > >         }
> > > > >
> > > > > Hi Laurent,
> > > > >
> > > > > Thanks for the example - I can certainly squash this into patch 8.
> > > >
> > > > yes, please.
> > >
> > > Okay I'll do that for a v3 (and also split the 1st patch that Phil suggested).
> > >
> > > > > As for allowing extra resolutions via -kernel, since the check is being done in
> > > > > macfb_common_realize() then it would be possible to add a qdev property that 
> > > > > only gets set when
> > > > > -kernel is used on the command line which bypasses the mode check if you prefer?
> > > >
> > > > I think it can wait and be done by a patch later. For the moment we can focus 
> > > > on MacOS.
> > > >
> > > > > I'm not sure that your existing example of "-g 1200x800x24" (or indeed any 
> > > > > resolution with 24-bit
> > > > > depth) with -kernel will still work after this patchset given that the 24-bit 
> > > > > encoding scheme has
> > > > > changed. Presumably this would also need a corresponding change in the 
> > > > > bootinfo/kernel framebuffer/X
> > > > > configuration somewhere?
> > > >
> > > > The kernel framebuffer should be easy to fix, if needed, normally we pass the 
> > > > needed information via
> > > > the bootinfo structure.
> > > >
> > > > My X configuration is broken for a while. With debian/sid I've never been able 
> > > > to start X (even on a
> > > > real q800, I think), and with debian/etch we need a special kernel as the ADB 
> > > > stack has been broken
> > > > with old kernel. I was not able to start X for a while now...
> > >
> > > FWIW I found that the last set of ADB fixes in mac_via.c actually fixed ADB on 
> > > old kernels again (I
> > > was able to use keyboard and mouse on the 4.15 kernel you used for the original 
> > > patches), so you may
> > > be able to get debian/etch working in QEMU. I'd expect forcing EMILE into a 
> > > 24-bit depth on a real
> > > Quadra 800 would also show the same issue here.
> >
> > In fact, I was trying the etch kernel, 2.6.32 that was never able to boot 
> > before because VBL
> > interrupt was not implemented. Now, with your series it boots (congratulation) 
> > but there is some
> > other issues in ADB but I don't think it's related to the changes in QEMU.

That's definitely progress! There are a couple of extra ADB fixes in my gitlab 
q800.upstream branch for NetBSD which may or may not help here?

> > I've found another kernel on my disk (4.1.39) and I'm able to start a 
> > gnome-session with 1152x870x8
> > mode. ADB works well. There is an issue with gdm that doesn't take characters 
> > from the keyboard but
> > if I start X manually and then gnome-sessions I'm able to play with it.
>
> And I'm able to run a 800x600x24 session with the attached xorg.conf
>
> To run 1152x870, the DefaultDepth and DefaultFbBpp must be changed to 8.

Excellent - thanks for the example config. Assuming that this is matching the 
behaviour on the real Quadra 800 then I'm happy these patches are moving the 
emulation in the right direction :)


ATB,

Mark.