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

[Mark Cave-Ayland]

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.

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'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?


ATB,

Mark.