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

[Laurent Vivier]

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.

> 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...

And GNOME desktop is not available on debian/sid m68k (some packages are 
missing). Perhaps I should
try Xfce.

So, don't worry about that...

Thanks,
Laurent