256-color VESA modes broken

[Martin Decky]

As of 64f85f54a27d11f2ce4cf0aef2e2fb35918fe5cd, HelenOS support for 256-color (8 bpp) VESA modes on amd64 and ia32 is broken. Despite the fact that the boot loader sets up the mode correctly, the lack of a well-defined color palette cripples the usability of the system.

For example, using the HelenOS-supplied GRUB boot loader and the QEMU-supplied (2.11) VGA BIOS leads to an all-black default color palette, thus rendering the output completely black. The original real-mode VESA code that has been removed in e3fa1720e83c1029187354ec92323608505ed7e3 explicitly configured a 3:2:3 palette.

In the future, I would suggest more thorough regression testing in cases of removing code that has been in place for a good reason for more than 10 years.

[Jiří Zárevúcky]

In the future, I would suggest more thorough regression testing

Honestly, I've never once used 256-color mode in any system in my entire life.
It wouldn't occur to me as a thing to test if you tortured me.
Creating (or documenting) appropriate tests is responsibility of the author.

[Martin Decky]

Honestly, I've never once used 256-color mode in any system in my entire life.

You probably have, but maybe just not explicitly

More seriously speaking, a 256-color mode is indeed rarely needed on a PC nowadays. But it was a simple test that 256-color modes are generally still working fine for the purpose of some more exotic architectures that require them.

Creating (or documenting) appropriate tests is responsibility of the author.

Sure. But testing the bootstrap process automatically is not trivial and we naturally just resort to booting the system manually and observing the behavior. You are right, we should have a checklist for that.

My point is that removing code should not be taken lightly. A code usually does something. This something is usually done for a reason. That should have been the push for a second thought regarding why the palette setting code was there in the first place, whether removing it without a functional replacement might not break something and ultimately to test a 256-color mode where this code path was used. Despite not being part of a formal checklist.

But this is obviously just my personal opinion.

[Jiří Zárevúcky]

I'm more concerned about the purpose of the code. You consider 256-color framebuffer modes at boot to be a useful feature? Why? To me it's just pointless maintenance burden. Do you actually have a machine that only works with it, or is it another case of "we support it because we can"?

[Martin Decky]

Please let me make a few comments.

(1) Your argument about the maintenance burden sounds really fundamentalistic to me. If you would apply the same logic to the entire HelenOS code base, you might as well remove almost everything except amd64 and arm32, because from purely practical point of view the other architectures are quite pointless.

(2) But I believe we have always aspired to be a general-purpose operating system. This is IMHO the main reason why we sometimes implement fringe features and support obscure architectures. First, to be more generic and not to fall into the trap of biases. Second, yes, to show that we can do it. I don't see a problem with the second motivation unless it severely limits us. Does the 256-color mode support limit us?

(3) Yes, I consider the 256-color framebuffer mode support in general a useful feature. These modes are still present in many hardware and not just as a compatibility feature. E.g. some OFW-based machines provide only 256-color modes via the firmware. Sure, you can always say "Just implement a proper hardware-specific GPU driver for that machine and you don't need any such limited firmware support", but this would turn such machines completely useless for HelenOS currently.

(4) Like Jiří Svoboda mentioned elsewhere, causing regressions in the name of progress is not a popular approach. I understand that it is sometimes necessary, but then it should be clearly justified. This is IMHO not the case. Setting a palette if a 256-color VESA mode is enabled is hardly a critical piece of code that needs extreme maintenance burden. Actually, nobody was required to touch the code in many years, until you decided to refactor the boot code. Let me stress that I have nothing against your effort (on the contrary), but that does not give you a general license to just strip previous functionality for no apparent reason.

(5) The palette-setting code was also not on the critical path because in all other cases when not running in a 256-color mode the code is simply skipped. I don't think that "the piece of code was just in my way when I wanted to do my changes" is the definition of maintenance burden.

(6) I would understand if you would remove the 8 bpp VESA support explicitly and completely, i.e. making sure that if the user selects such a mode, they will get an error message and not end up with a black screen. Instead, you are saying that you have removed somebody's code without much consideration for the consequences because you personally don't care for it. Well, I really see your approach a bit disrespectful.

[Jiří Zárevúcky]

You are right. I'm sorry, I didn't think it through. I opened a pull request to gather feedback and avoid this kind of problem, but then I merged it too soon and didn't give you time to comment.

Now, I'm not convinced the issue is in GRUB giving us an unusable framebuffer mode. Multiboot parsing (either version) ignores indexed color mode. I'll see if initializing bfb correctly fixes the issue.

[Jiří Zárevúcky]

Ah, I see, our framebuffer code doesn't actually support arbitrary pallete, and multiboot doesn't support setting the pallete. Perhaps we should consider adding support for arbitrary pallete.

[Jiri Svoboda]

Based on above discussion I suggest the VESA code should be put back. You can rewrite it to C later, but in any case I like this piece of functionality, please bring it back.

As far as support for old PCs and other vintage platforms, I completely agree with Martin - if we can support an unusal or old platform without it limiting us, we should. I would actually like to *extend* our support of old PCs, bringing back ISA bus support was just the first step…

Having good support of vintage HW can be a differentiator. Very few up-to-date operating systems support these and the demand is growing due to increased interest in vintage computers.

[Martin Decky]

When time permits, I will try to get back at least the basic palette setup code. The VESA VBE should indicate whether the GPU is actually VGA compatible or not (in the majority of cases it should be, but of course this is not universal). If yes, it should be legal to setup the palette via the standard VGA ports as done previously. But I am currently not sure if this VGA-compatibility flag is passed from the multiboot boot loader.

[Jiri Svoboda]

The change which caused this problem was reverted in commit ​8be323084b791c62927f9cbb14696265246370dd.