[console] Add dual monitor support to console-direct driver

[vkoskiv]

I wanted to use two displays at the same time on ELKS, so I rewrote most of the console-direct driver to make this happen.

Still very much a WIP, but this is already working quite nicely on my hardware.
IBM folks were clever enough to ensure that I/O ports and vram segments didn't collide between the MDA and CGA cards, so both could be used at once.
Not too many programs in the DOS era supported this. Lotus 123 and Borland's wonderful Turbo Debugger are good examples.
Now ELKS, with its full preemptive multitasking can take advantage of this feature, for some really cool dual-terminal UNIX sessions on even the most ancient IBM hardware.

This PR introduces the concept of an output to the console drivers, and makes use of that in console-direct.c, which now has logic for probing, initializing and managing multiple output devices.
Switching between displays is fairly straightforward - Each display gets allocated 0-N consoles, and the active display is indicated by hiding the cursor on the inactive display. Switch with CTRL+ALT+F<N>, like before.

Few more things require attention before merging:

• The disk busy indicator relies on the VideoSeg global, which is no more. I'll probably just hook that up so the indicator is displayed on the primary display. Thoughts?
• I have not tested, or even considered EGA + VGA. As far as I know, EGA should be pretty straightforward, and may even be able to coexist with both a CGA and and MDA card, but that needs to be checked. VGA is still //TODO, similarly.
• I have only tested this on a working MDA + CGA configuration on my 5150+5160 setup. More testing with different hardware configurations is needed.

Below are some photos of the thing in action on my hardware.

[vkoskiv]

It should be noted that the addition of this new stuff to console-direct.c bloats up that driver by ~1.7kB, so it might be desirable to move this implementation into a separate driver, and have it as a compile-time option.

Before:
> du -b $(find . -name 'console-direct.o')
16504	./elks/arch/i86/drivers/char/console-direct.o
After:
> du -b $(find . -name 'console-direct.o')
18216	./elks/arch/i86/drivers/char/console-direct.o

[ghaerr]

Wow @vkoskiv this is really amazing! Great work!!

Give me a bit to find some time to see how you've made this work, but I'm super impressed with the screenshots.

And I have to say, running matrix on top of your IBM whilst in the middle of an vi editing session on another monitor?! That's really too much, a total classic!!!! I think I'll hang that picture on my wall :)

I haven't had but two seconds to look at the code, but I'm seeing lots of C->op->var. The double indirection all the time is expensive. How about the idea of just including the struct output members directly in struct console, or something like that. I realize that design might be a bit different, given that you have identified Consoles and Outputs. If they can be merged into a single struct Console, even at the expense of using a bit more data because of a larger array, that would likely still be preferable to multiple structs and indirections.

The above suggestion would also help for adding a config option for this, as I think the extra 1.7k bytes is too much to force on people that don't have multiple monitors, and, in some configurations, we're within about that much space before running out of our near kernel CS address space! The extra data required in struct Console could be ifdefed out when not configured, along with any unneeded routines.

I'll get back to you shortly with more input, thank you for this amazing contribution!

[vkoskiv]

I haven't had but two seconds to look at the code, but I'm seeing lots of C->op->var. The double indirection all the time is expensive.

Ah! I had a hunch that that may be the case as I was adding those in, I just didn't look at the generated code yet :]
Merging struct output with struct console should be quite doable. The main goal was to just get rid of that global state that assumed a single display, and I guess I just figured this might be how this abstraction would be expressed on modern Linux, but of course, modern Linux this is not. Bytes are expensive!

[ghaerr]

Merging struct output with struct console should be quite doable.

Great - please go ahead with that and push another version after you have that working. I should hopefully have more time soon.

If you want to take a pass at a configuration option, a very simple method is to define a file-local config option, say CONFIG_CONSOLE_DUAL, that is defined near the top of console-direct.c. Then you can play with adding (or not) the struct Console variables right during development, and we can then afterwards put the config option into the appropriate place inside the configuration management system.

Turning the config option on and off within the file allows for a simple comparison of text/data sizes. BTW, for that, I would use ia16-elf-size console-direct.o. That shows the exact text/data/bss values immediately, which helps.

[ghaerr]

I just didn't look at the generated code yet

For that, try ia16-elf-objdump -D -r -Mi8086 console-direct.o. Its definitely worth getting used to, although a bit complicated at first.

[ghaerr]

a very simple method is to define a file-local config option, say CONFIG_CONSOLE_DUAL, that is defined near the top of console-direct.c.

Now that I see you've gone ahead and implemented this for many of our various console*.c drivers, a better place for the temp config option for the moment would be in console.h.

The disk busy indicator relies on the VideoSeg global, which is no more. I'll probably just hook that up so the indicator is displayed on the primary display.

Sure, you can do that provided it doesn't get messy, or just ifdef it out entirely when CONFIG_CONSOLE_DUAL is defined, your choice.

[tyama501]

Wow!! This is amazing and very cool!

console.c is used in pc-98 too so let me know if there is build errors:)
Thank you.

[toncho11]

Seeing the two apps simultaneously is priceless!
From what I heard the old console driver (current in ELKS) is a bit slow, so maybe this rewrite can also help with that in the future.

[ghaerr]

From what I heard the old console driver (current in ELKS) is a bit slow, so maybe this rewrite can also help with that in the future.

Yes, while the ELKS kernel overhead in processing tons of ANSI escape sequences emitted from sl has proved quite slow on an 8088, this enhancement actually might technically make the console just slightly slower. The speed issue isn't just within the ANSI console driver though. But with the double indirection removed, I don't think the proposed changes will make ELKS measurably slower :)

@vkoskiv, you're welcome to run sl on your vintage 8088 and report back just how fast/slow it seems. I would be surprised if your code made any noticeable difference.

[ghaerr]

Hello @vkoskiv,

I've been thinking more about some ideas for the possible minor (re-)implementation of this PR. As mentioned above, we definitely don't want to use double indirection. And we want as much speed as possible, but certainly the "fixed" values of things like Width, MaxCol etc would need deferencing via C->MaxCol etc (BTW, perhaps don't rename all the member variables, just use the same name in the extended struct Console for less confusion with old code).

Since struct Output would be going away, we really only need as may struct Consoles as there are max consoles * monitors, which is 6, and possibly only 4 if MDA hardware doesn't support multiple pages? So we need /dev/tty1 through /dev/tty6, or through /dev/tty4 for a single MDA page. Each struct Console basically maps directly into a /dev/ttyN, just the ordering of the mapping may differ between hardware installed:

In the MDA-only case, /dev/tty1 (though N when more pages) would be allocated, so systems programs can always assume a working /dev/tty1. For the EGA-only case, /dev/tty1 through 3, just like it is now, and for the multiple monitor case, map EGA to /dev/tty1 - 3 and MDA tty4 onwards. This may be how you have it designed now.

For the glock global, it may not be needed as a struct Console member, as it's only used by Nano-X for switching between text and graphics modes which MDA doesn't support, so a single global value should suffice.

For EGA+CGA, the EGA consoles would be allocated first, then CGA. For VGA, ELKS treats VGA as EGA and doesn't support any VGA-specific features, so its probably not worth bothering to add options to ELKS to use special (non 0xB800) hardware address for EGA/VGA. I am assuming that your initialization routine can basically differentiate between hardware video memory at B000, B800 and A000 all in some kind of text mode. Are you relying on the BIOS to initialize each monitor's text mode? I haven't looked at that part yet.

I haven't fully studied your code, so some of what I've suggested may not quite be the way you've implemented things, but thought to discuss this more before/while you're refactoring the code.

Thank you!

[vkoskiv]

Hey @ghaerr ! Took a few days longer than expected to get back to this, as I fell ill with the flu the very next day after opening this PR. Feeling much better now, so I'm excited to get back to working on this!
I finished up and pushed the patch I was working on earlier. It does what it says on the tin, and gets rid of the struct output stuff, and instead includes the necessary bits in struct console directly.
That change, plus a few extra days of thinking time, enabled me to simplify the init logic in console-direct.c quite a bit. I still need to gate some of the extra stuff behind that CONFIG_CONSOLE_DUAL flag, but it's already looking quite good.

(BTW, perhaps don't rename all the member variables, just use the same name in the extended struct Console for less confusion with old code).

Agreed, I'll change those back.

Since struct Output would be going away, we really only need as may struct Consoles as there are max consoles * monitors, which is 6, and possibly only 4 if MDA hardware doesn't support multiple pages?

The max amount of consoles possible, as far as I can tell, is 5. We can't have multiple CGA cards in the system, as their CRTCs and vram conflict. CGA should support 4 console pages, but console-direct in master currently only sets up 3 pages, I'm not sure why. I didn't try bumping that up to 4 yet, as there are a lot of other things to test.
So the theoretical max is 5 (4x CGA + 1x MDA), but currently the max is 4 (3x CGA + 1x MDA).
I haven't investigated EGA yet, I don't even know if it can coexist with a CGA card, though I currently suspect that it may.
And yes, you are correct that the 4K of RAM in a MDA card only supports a maximum of 1 80x25 console pages (2k for text, 2k for attribs)

So we need /dev/tty1 through /dev/tty6, or through /dev/tty4 for a single MDA page. Each struct Console basically maps directly into a /dev/ttyN, just the ordering of the mapping may differ between hardware installed:

In the MDA-only case, /dev/tty1 (though N when more pages) would be allocated, so systems programs can always assume a working /dev/tty1. For the EGA-only case, /dev/tty1 through 3, just like it is now, and for the multiple monitor case, map EGA to /dev/tty1 - 3 and MDA tty4 onwards. This may be how you have it designed now.

The way that it's designed now is that we ask the BIOS to tell us which display the user has selected as the 'primary' one. This happens with switches on the IBM 5150:

and I'm assuming BIOS options on more modern motherboards.

So /dev/tty1 always ends up on that primary display, and the rest are allocated based on availability. So if we only have a CGA card, we get 3 TTYs. CGA + MDA allocates the extra /dev/tty4 on the MDA display.
I still need to test this, but the logic should also work if I flip the switches to select my MDA card as the primary display, I'd get /dev/tty1 on the MDA, and /dev/tty[2,3,4] on the CGA.
Do you think this makes sense?

For the glock global, it may not be needed as a struct Console member, as it's only used by Nano-X for switching between text and graphics modes which MDA doesn't support, so a single global value should suffice.

Side note, I remember trying the Nano-X stuff earlier, and as far as I can tell, it's not intended to work with CGA or MDA? In any case, I propose a check in the DCGET_GRAPH ioctl to return an error code, at least if that console is assigned to a MDA screen that doesn't support graphics.

I am assuming that your initialization routine can basically differentiate between hardware video memory at B000, B800 and A000 all in some kind of text mode.

Right, so I'm just dealing with the specified video memory addresses and CRTC I/O bases. probe_crtc() can check if a CRTC is present, and that info is then used when setting up hardware.

Are you relying on the BIOS to initialize each monitor's text mode? I haven't looked at that part yet.

No, so the primary source of 'bloat' coming from these changes is that we now have logic for setting up video hardware, as we cannot rely on the BIOS to initialize anything but the primary one specified by the user. init_output() handles this. It programs the CRTC to a known good state for 80x25 text, tests and clears the VRAM, and then enables video at the end. I checked this against what the BIOS does, and it's quite similar. I didn't investigate yet if there is a BIOS interrupt we could call to set up arbitrary adapters.

[vkoskiv]

@tyama501

Wow!! This is amazing and very cool!

Thanks!

console.c is used in pc-98 too so let me know if there is build errors:) Thank you.

I've taken care to update console-direct-pc98.c and checked that there are no build errors, but I don't have hardware to test that my changes didn't break something at runtime.

[vkoskiv]

The bloat of console-direct.o was slightly reduced here:

Original:
> du -b $(find . -name 'console-direct.o')
16504	./elks/arch/i86/drivers/char/console-direct.o
With struct output:
> du -b $(find . -name 'console-direct.o')
18216	./elks/arch/i86/drivers/char/console-direct.o
Currently:
> du -b $(find . -name 'console-direct.o')
17752	./elks/arch/i86/drivers/char/console-direct.o

So 464 bytes smaller, but still 1248 bytes of bloat when compared to how it was originally. I'd expect gating with CONFIG_CONSOLE_DUAL will constrain most of this bloat to only when that option is enabled, though.

[vkoskiv]

@ghaerr What should we do about the spinning I/O indicator that lives in arch/i86/kernel/timer.c?

It relies on the VideoSeg global, and it's a bit unclear now which video page on which display that should be. Intuitively I'd just assign the first page of the primary display there, but then the spinner only displays if that page is up on that display.
I think the spinner is cute, but I'd like to see it documented, and perhaps gated behind a config option, so it may be disabled if needed.

[vkoskiv]

Cracked open the 5150 to test this with the MDA card selected as primary, and it works as expected. The assignment ends up as:

/dev/tty1 -> MDA
/dev/tty2 -> CGA
/dev/tty3 -> CGA
/dev/tty4 -> CGA

[ghaerr]

Agreed, I'll change those back.

Having the same member names in struct Console will help, thanks. For those (new) names in struct hw_params, the new member names are probably better. This will be more descriptive and keep the HW changes only in console-direct.c, whereas console.c is shared between the other console drivers/platforms and their member names not changing will help.

need to gate some of the extra stuff behind that CONFIG_CONSOLE_DUAL flag

Great, probably just as well to starting gating right away, some of my comments will mention more detail about it.

but console-direct in master currently only sets up 3 pages, I'm not sure why.

It was probably done a long time ago for space reasons. There are other complicated issues though, for instance the "fake" FAT filesystem /dev emulation is limited as to how many /dev entries there can be. So for now keeping /dev/tty1 - 3 as CGA is a good idea to reduce complications that can be dealt with later.

The way that it's designed now is that we ask the BIOS to tell us which display the user has selected as the 'primary' one.

Ok. What BIOS Data Address (BDA 0x40:XXXX) is being used for that info?

So if we only have a CGA card, we get 3 TTYs. CGA + MDA allocates the extra /dev/tty4 on the MDA display.
if I flip the switches to select my MDA card as the primary display,
Do you think this makes sense?

Yes, sounds good.

I remember trying the Nano-X stuff earlier, and as far as I can tell, it's not intended to work with CGA or MDA?

@tyama501 just added CGA support to Nano-X. MDA is not supported.

In any case, I propose a check in the DCGET_GRAPH ioctl to return an error code, at least if that console is assigned to a MDA screen that doesn't support graphics.

Lets not change the logic yet on glock or its ioctl issues until we've had a chance to get the first revision of this PR committed and can start testing before/after with Nano-X graphics. What I'd like any new hardware-initialization or code determining monitor types to be fully gated with CONFIG_CONSOLE_DUAL for the first cut. The console driver currently uses the current BIOS BDA video mode to determine whether the console is already running in MDA or CGA/EGA (text) mode, and for compatibility reasons needs to stay that way if this option is not set.

so the primary source of 'bloat' coming from these changes is that we now have logic for setting up video hardware, as we cannot rely on the BIOS to initialize anything but the primary one specified by the user.

Got it. That's fine, as long as all hardware-specific logic is gated. That will ensure full boot compatibility without the option. (It has taken us a while to achieve booting on almost any 8088 system, which is why we're using only a single BDA video mode for the current startup).

with the MDA card selected as primary, and it works as expected

Great!

It relies on the VideoSeg global, and it's a bit unclear now which video page on which display that should be.

Perhaps leave a VideoSeg global which is set unmodified outside the gate, but set to the video segment of what your new code determines the primary monitor is when CONFIG_CONSOLE_DUAL is defined.

I have some other comments on the last commit, will comment directly on the code for that.

[vkoskiv]

Ok. What BIOS Data Address (BDA 0x40:XXXX) is being used for that info?

From here:

40:63 word Base port address for active 6845 CRT controller
3B4h = mono, 3D4h = color

[vkoskiv]

If the rebase approach is okay, I could apply the rest of the fixes like that, too, so we'd end up with 2-3 good commits, and no more extra fixup/feedback commits. Both ways are fine by me.

[ghaerr]

I think we're OK with the rebase commits, but git is not my forte (nor do I want it to be!). Agreed there's been probably too many fixup commits to be of much interest to people, so a rewritten history is fine by me for this PR.

I plan on either a squash or merge commit at the end of this; a merge commit would be nice if you want to keep a few good commits showing changes.

The Width/Height/MaxRow/MaxCol rename is looking good - it is showing exactly what i wanted to see and making it easy to ensure no bugs. The big question now is whether to optimize out Width/Height, or MaxRow/MaxCol. I want to take a slightly closer look, but it seems that if you went with moving MaxRow/MaxCol to locals variables, it could save quite a bit of code generation in the shared console.c routines. As far as speed goes, either way won't matter with the Scroll/Clear routines etc, as they take way longer than what we're concerned with.

[ghaerr]

@vkoskiv: I just reviewed our large diff. With regards to the Width/Height/MaxCol/MaxRow we've been talking about: I say you should go ahead and remove MaxCol and MaxRow as members of the Console struct, and replace them at the top of any function which needs them with a local variable of the same name computed from Width or Height. This should lessen the amount of code produced in console.c::AnsiCmd() by quite a bit, and frankly, Width and Height are much more commonly understood than the zero-based equivalent MaxCol/MaxRow. This method also allows us to ensure porting errors to be at a minimum, as the diff will be simple to inspect. And if we're lucky, they'll be some savings as a result of the to balance the added bloat when CONFIG_CONSOLE_DUAL isn't defined.

What do you think about that?

[vkoskiv]

@ghaerr Sounds good to me! I have a patch ready for it, I'll just do some checks to ensure I didn't break anything. In the meantime, I addressed most (all?) of the other feedback, mostly as fixup commits to the existing commits.
Ideally I'd like to try squashing the Remove output struct to reduce indirection commit into the initial commit to reduce code churn in the git history, I'll see if I can pull that off a bit later.
Really happy with how this PR is turning out, thanks again for the excellent feedback so far. Learned some really useful things!

[ghaerr]

Ok @vkoskiv, glad to see you back and get this finished up. Thanks for all the changes. I'll do another code review and then a hopefully final one after the MaxRow/MaxCol elimination. I am anxious to learn how much the code size changes up or down for the console driver when the DUAL option is not selected, given some of these later optimizations.

[ghaerr]

Ideally I'd like to try squashing the Remove output struct to reduce indirection commit into the initial commit to reduce code churn in the git history, I'll see if I can pull that off a bit later.

Don't worry about that if your time is short - I'm happy with the commits you've pushed and plan to do a merge commit.

[ghaerr]

All of this is looking very nice indeed! The removal of C->MaxCol and C->MaxRow really helped make the diff smaller, which definitely helps regression testing via eyeballs. I am going to fly my drones for a bit but will take a final look after and hopefully get this committed today :)

The build is failing because crtc_init and crtc_probe are undefined - please test without CONFIG_CONSOLE_DUAL defined, that's likely the problem.

[vkoskiv]

The build is failing because crtc_init and crtc_probe are undefined - please test without CONFIG_CONSOLE_DUAL defined, that's likely the problem.

I've done test builds with different sets of options, and it builds fine on my system with CONFIG_CONSOLE_DUAL disabled in the kernel config. Not sure what's tripping up the CI build yet 🤔

[ghaerr]

Try running ./buildimages.sh or ./build.sh auto allimages in the main directory, it may be related to building the 8018x port. In any case, the latter runs the same stuff that the CI does. You can check

[vkoskiv]

I can repro with ./buildimages.sh. Not sure why, but autoconf.h gets #define CONFIG_CONSOLE_DUAL 1 even though the current .configure doesn't even have that config option in it to begin with.

[vkoskiv]

I changed the y to an n in elks/arch/i86/drivers/char/config.in for the CONFIG_CONSOLE_DUAL line. I don't know why, but that seems to fix the issue.

[ghaerr]

@vkoskiv: You've done a great job on this, thank you for the wonderful enhancement! I'm going to commit this now.

I will follow up with a PR to make the change from unsigned short to int for Width and Height, with a few other minor changes, as well as an answer to your above question. I'm anxious to see what the kernel code and data size change for this with DUAL on and off. We can continue discussion on that PR if needed.

Thank you!