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

[Laurent Vivier]

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

Thanks,
Laurent

xorg.conf
Description: Text document