Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[config] Allow configuration of default screen height #1761

Closed

Conversation

and3rson
Copy link
Contributor

@and3rson and3rson commented Dec 2, 2023

I'm using BIOS console in my system with a non-standard screen size (40x8).
However, screen height of 25 rows is hard-coded in setup.S, which messes up screen scrolling.

This PR adds a config parameter to specify a value different than 25 if VGA is disabled.

@and3rson and3rson force-pushed the ad/config-screen-height-novga branch 2 times, most recently from e443e3c to d9e4918 Compare December 2, 2023 14:00
@and3rson and3rson force-pushed the ad/config-screen-height-novga branch from d9e4918 to 899902a Compare December 2, 2023 14:01
@and3rson and3rson changed the title boot: allow configuration of default screen height [config] Allow configuration of default screen height Dec 2, 2023
@ghaerr
Copy link
Owner

ghaerr commented Dec 2, 2023

Hello @and3rson,

Thanks for posting this PR.

I'm having a little trouble figuring the best way to handle the problem; the proposed solution adds a new CONFIG_DEF_SCREEN_HEIGHT, which is sometimes defined by config but then checked for not being defined later, which introduces maintenance issues, and config.h already defines CONFIG_VID_LINES, which is actually used in the kernel. Also, handling columns != 80 in the same way will just get messier.

The current solution for handling the mess of a bunch of less-used but importantly configurable items is the config.h file, which sets lots of SETUP_xxx defines for overriding settings found in the hardware-dependent (or missing) setup.S pre-kernel-boot execution.

Given that the proposed solution already modifies config.h to add the #ifdndef CONFIG_DEF_SCREEN_LINES, I'm thinking that jumping further into defining a sub-platform for your work in config.h, and setting that from elks/config.in, might be a better way to go. This could work the same way that the Russian MK-88 was handled, such as:

elks/config.in:
...
        if [ "$CONFIG_ARCH_IBMPC" = "y" ]; then

                bool 'Compaq DeskPro Hardware (fast mode)' CONFIG_HW_COMPAQFAST n
                bool 'MK-88 Hardware (IRQ 3 keyboard)' CONFIG_HW_MK88 n
                bool 'PK-88 SBC Hardware' CONFIG_HW_PK88 n
include/linuxmt/config.h:
#ifdef CONFIG_ARCH_IBMPC
... (after #ifdef CONFIG_HW_MK88)

#ifdef CONFIG_HW_PK88 /* SBC w/KM1810VM88 CPU from Ukraine */
#undef SETUP_VID_LINES
#undef SETUP_VID_COLS
#define SETUP_VID_COLS 40
#define SET_VID_LINES 8
#endif
...

After this, special considerations for your SBC could be made using #ifdef CONFIG_HW_PK88, at least for the time being.

What do you think?

Thank you!

@and3rson
Copy link
Contributor Author

and3rson commented Dec 2, 2023

Thanks for feedback!
I like this idea, but there's one thing that worries me: since my SBC is being actively developed, I might be swapping lots of things in and out - such as replacing an LCD with a bigger LCD, adding more I/O stuff, etc. Thus my biggest concern is that I'll be "polluting" ELKS codebase & commit history with some changes that are relevant only to my machine...

I personally find overridable params more portable (and probably less annoying to others), since anyone who has their own flavor of x86 SBC will be able to run menuconfig and enter values that are relevant for their device, without needing to add a new sub-platform.

But hey, if you're OK with people like me having their own configurations directly in ELKS codebase - I'm all for it, that works for me. In fact, it's an honor to leave a trace of my doings in ELKS codebase. :)

@and3rson
Copy link
Contributor Author

and3rson commented Dec 2, 2023

Some more thoughts:

I briefly mentioned in this thread - #1751 (reply in thread) - that another possible portable solution would be to add a new sub-platform, e. g. CONFIG_HW_HOMEBREW.

If defined, it could change certain features of ELKS to use some pre-defined hackable contract with the underlying hardware: for example, interrupt 0x80.
Here's how it could be used with cases like this, i.e. SBC with non-standard display height:

; setup.S, line 774:
#ifdef CONFIG_HW_HOMEBREW
        ; Fetch display height from SBC's BIOS
        mov 0x01, %ah  ; Function 0x01: get display size, CH<-rows, CL<-columns
        int 0x80  ; Generic Homebrew API
        mov %ch, %al
#else
        ; Here goes the original behavior currently implemented in ELKS.
        mov   $25,%al		// height of display in rows
#ifdef

There are some big benefits:

  • Homebrew SBCs can define their own handler for int 0x80 before booting ELKS, and thus modify behavior of ELKS
  • No need to add sub-platforms for each new device

Cons:

  • A brand new API that needs documenting! :D

@ghaerr
Copy link
Owner

ghaerr commented Dec 2, 2023

Good points on codebase/commit issues as well as menuconfig being favorable. Agreed, except that frankly, we don't have that many SBC design(er)s being developed on ELKS and at this point in early development, I'm unsure as to exactly which way to go for your custom port on an IBM PC-like architecture.

The last couple of years have been spent decreasing the amount of options throughout ELKS as many were put in decades ago and never maintained - thus decreasing the understandability of the project as a whole. Also, it became important to decrease the dependencies ELKS required for porting, as in the decades past, all sorts of dependencies were added all over the source base, which IMO would make for lots of work for new ports. As a result, I think we've succeeded in making ELKS easy to port (although config.h is still a bit messy, but at least its only in one place). (Actually, I'd prefer to get rid of lots/most #ifdefs throughout but that's a far bigger job than it seems at the moment).

if you're OK with people like me having their own configurations directly in ELKS codebase

Lets go with adding configuration-specific settings in config.h for your SBC for now. That'll allow you to proceed quickly with exploring what kinds of things need to be more configurable in ELKS, and allowing for commits to happen quickly without regard to system-wide or other port/platform testing. I'm far more concerned with allowing for port-specific exploratory development than worrying about ELKS commit history.

personally find overridable params more portable (and probably less annoying to others), since anyone who has their own flavor of x86 SBC will be able to run menuconfig and enter values that are relevant for their device

Sure. After you feel your port is done, we can revisit how many options there are, and to see whether some options would be better off in config.in file(s).

@and3rson
Copy link
Contributor Author

and3rson commented Dec 2, 2023

As a result, I think we've succeeded in making ELKS easy to port

You sure did - ELKS ran almost out of the box for my SBC. And the docs are really good!

Lets go with adding configuration-specific settings in config.h for your SBC for now

Just to make sure I understand correctly - you mean adding CONFIG_HW_PK88, correct?

@ghaerr
Copy link
Owner

ghaerr commented Dec 2, 2023

another possible portable solution would be to add a new sub-platform, e. g. CONFIG_HW_HOMEBREW.

You're welcome to use CONFIG_HW_HOMEBREW rather than CONFIG_HW_PK88, if you like.

If defined, it could change certain features of ELKS to use some pre-defined hackable contract with the underlying hardware: for example, interrupt 0x80.

IMO, the problem with this approach is twofold: setup.S is already too complicated, should be in C not ASM, and hides information that rightfully should be performed in the kernel itself. Any number of ROM-only ports don't use setup.S at all (or at least shouldn't, if we could finally get rid of it). setup.S is a hack from the original Linux days that probably should go away, and be replaced with a more understandable kernel init routine. It was originally written because the kernel jumped to protected mode and the information wasn't available after real mode switched over. It remains required to relocate the kernel, which at this point is still so complicated you don't want to know how it does it lol.

Secondly, we're adding even more ifdefs into code, making the overall intent and maintenance harder to follow.

LOL - if you're getting the idea that I have a problem with setup.S, yes, probably true - it took way too long for me to initially figure it out, and then pull most of the stuff out of it we could so that porting became much easier. At the end of the day, it's still filled with unportable stuff.

Here's how it could be used with cases like this, i.e. SBC with non-standard display height:
Homebrew SBCs can define their own handler for int 0x80 before booting ELKS, and thus modify behavior of ELKS

These are potentially good ideas, but IMO should not live in setup.S. I would rather see something like

#define SETUP_VID_COLS int80(1)
#define SETUP_VID_LINES int80(2)

along with a separate .S or .c file that would keep the mess out of the setup or mainline kernel. The CONFIG_HW_PK88 or CONFIG_HW_HOMEBREW could make a single call in kernel_init() (or setup.S if absolutely necessary) to set things up, with no ifdefs. The idea here is to keep all the SBC or system-specific stuff in a single place, which doesn't interfere with understanding the rest of the system for ports that don't need it.

BTW, the SETUP_xxx defines are called quite infrequently, so it wouldn't be a problem to make a function call when they are used elsewhere in the kernel. Also, don't use INT 0x80 because that's the ELKS system call!

Just to make sure I understand correctly - you mean adding CONFIG_HW_PK88, correct?

Yes.

@ghaerr
Copy link
Owner

ghaerr commented Dec 2, 2023

then pull most of the stuff out of it we could so that porting became much easier.

The idea here is to keep all the SBC or system-specific stuff in a single place, which doesn't interfere with understanding the rest of the system for ports that don't need it.

Another quick point - config.h was added specifically for new ports that would/could not use setup.S; the defines override the default calls to setup(x) which grab values passed in a low memory segment from setup.S to the kernel. So what you're seeing in config.h are all the settings that eventually should be moved from setup.S into a (preferably) C kernel init routine, for maximum portability. This would have been done already except that its messy to wrap all the various PC INT xxH interrupts that might be called from C, just to get values like lines, columns, max mem, etc, especially when setup.S is still required for IBM PC boots anyways.

At some point we could consider reengineering (removing most of) setup.S and config.h, and adding those settings to menuconfig. This might be confusing to PC users to have so many options. Perhaps a separate page entirely in menuconfig that pops up would be a good idea. IMO, the whole thing is an exercise in managing complexity vs usability.

@and3rson
Copy link
Contributor Author

and3rson commented Dec 2, 2023

This makes a lot of sense. Thanks for taking time to explain your motivation, I highly appreciate it!
I'll go with CONFIG_HW_PK88 for now to make things more explicit & simple, and if we'll have more people doing the similar things with their own builds, maybe we'll be able to find a more universal solution that both doesn't increase the complexity of the already complex setup code and doesn't add too much implicit ifdef entanglement to the code.

I'll close this PR for now and will spend some time to see if there are any other places in ELKS that need ifdef guards for my SBC, and then I'll submit a new PR. Though I anticipate the screen height to be the only place requiring override for my build.

EDIT:

#define SETUP_VID_COLS int80(1)
#define SETUP_VID_LINES int80(2)

This looks nice and clean, and I get your idea - maybe I'll try doing this! Of course, I'll use a different int - for a split second I totally forgot ELKS is *nix, and it uses 0x80 for sycalls!

@and3rson
Copy link
Contributor Author

and3rson commented Dec 3, 2023

BTW, fun fact: MK-88 was actually a joint Ukrainian-Belarussian development. Both countries were part of Soviet Union at that time, however it wasn't a Russian product. I'll try to find more info about it, since it seems to have been driven by the same KM1810VM88 CPU made in Kyiv!

@and3rson and3rson closed this Dec 4, 2023
@and3rson and3rson mentioned this pull request Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants