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

Add a simple frame buffer console #25

Closed
wants to merge 9 commits into from

Conversation

svenpeter42
Copy link
Member

@svenpeter42 svenpeter42 commented Feb 12, 2021

  • cleanup code some more
  • add font license and sign-off the commit
  • integrate makefont.sh into Makefile
  • figure out how to deal with the logo

src/fb.c Outdated Show resolved Hide resolved
makefont.sh Outdated Show resolved Hide resolved
src/fb.c Outdated Show resolved Hide resolved
src/fb.c Outdated Show resolved Hide resolved
src/fb.c Outdated Show resolved Hide resolved
Copy link
Contributor

@Lunaphied Lunaphied left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think things are mostly good other than a few style and commenting tweaks here.

Also this isn't SMP safe as you've noticed, and you'll probably need to add some form of locking to make sure access to the framebuffer's console state isn't racey.

src/fb.c Outdated Show resolved Hide resolved
src/fb.c Outdated Show resolved Hide resolved
src/fb.h Outdated Show resolved Hide resolved
src/fb.c Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
src/fb.c Outdated Show resolved Hide resolved
@marcan
Copy link
Member

marcan commented Feb 13, 2021

I think things are mostly good other than a few style and commenting tweaks here.

Also this isn't SMP safe as you've noticed, and you'll probably need to add some form of locking to make sure access to the framebuffer's console state isn't racey.

SMP boot already should effectively synchronize with the CPUs booting, so we can probably get away without locking here at this time (the UART code has this problem too), but it would be ideal to add locking globally to printf to solve this - however, there are subtleties there with IRQs, so we probably need to disable IRQs around printf to avoid deadlocks there.

@Lunaphied
Copy link
Contributor

I think things are mostly good other than a few style and commenting tweaks here.

Also this isn't SMP safe as you've noticed, and you'll probably need to add some form of locking to make sure access to the framebuffer's console state isn't racey.

SMP boot already should effectively synchronize with the CPUs booting, so we can probably get away without locking here at this time (the UART code has this problem too), but it would be ideal to add locking globally to printf to solve this - however, there are subtleties there with IRQs, so we probably need to disable IRQs around printf to avoid deadlocks there.

Other people noted that testing on physical hardware was causing SMP startup to fail with the new print code. I was mostly referencing that, although I'm not positive what the actual cause if that was.

@marcan
Copy link
Member

marcan commented Feb 13, 2021

Something to look out for is that right now we map the FB as cached, though given that this code works I assume the GPU (and display controller) are cache-coherent with the CPUs. But since the other CPUs will have caches disabled, and in fact we never enable the MMU on those, I could see this going wrong if different CPUs try to access the same memory with different caching attributes.

Given that, it might be prudent to disable FB prints entirely from CPUs other than CPU#0; that sounds like it's likely to save us a whole bunch of trouble anyway. We can either just not show those messages on the FB, or buffer them and catch up on CPU#0 on the next print from there.

Signed-off-by: Sven Peter <sven@svenpeter.dev>
@svenpeter42
Copy link
Member Author

Something to look out for is that right now we map the FB as cached, though given that this code works I assume the GPU (and display controller) are cache-coherent with the CPUs. But since the other CPUs will have caches disabled, and in fact we never enable the MMU on those, I could see this going wrong if different CPUs try to access the same memory with different caching attributes.

Ack, I think that's what is causing these failures.

Given that, it might be prudent to disable FB prints entirely from CPUs other than CPU#0; that sounds like it's likely to save us a whole bunch of trouble anyway. We can either just not show those messages on the FB, or buffer them and catch up on CPU#0 on the next print from there.

Good idea, I'll just disable frame buffer printfs from secondary cores entirely.

@svenpeter42 svenpeter42 marked this pull request as ready for review February 13, 2021 13:43
@svenpeter42 svenpeter42 force-pushed the fb-console branch 2 times, most recently from b955fa9 to 8ab4912 Compare February 13, 2021 14:04
@svenpeter42 svenpeter42 changed the title Fb console Add a simple frame buffer console Feb 13, 2021
Copy link
Contributor

@Lunaphied Lunaphied left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few nitpicks, but overall besides the one change in the fb_console_init function to fix the bug with row offsets I think this is pretty good. Works well in the emulator even with different size displays and everything.

I like the way it splits the text at the logo. It's simple and prevents the logo from being overwritten or anything. I was going to suggest changing that a bit, I think that just using a small logo in the upper right corner when using fb console might be nice (maybe display the big logo, delay 0.5 sec then replace with small logo and just use the full screen?) but that's a personal preference thing (I think it makes split hex values a little annoying to read).

src/fb.c Outdated Show resolved Hide resolved
src/fb.c Show resolved Hide resolved
src/fb.c Show resolved Hide resolved
src/fb.h Show resolved Hide resolved
src/fb.c Outdated Show resolved Hide resolved
src/fb.c Show resolved Hide resolved
Signed-off-by: Sven Peter <sven@svenpeter.dev>
Signed-off-by: Sven Peter <sven@svenpeter.dev>
This font is licensed under the OFL-1.1 License and copyright:

Copyright 2010-2019 Adobe (http://www.adobe.com/), with Reserved Font Name 'Source'.
All Rights Reserved. Source is a trademark of Adobe in the United States and/or other countries.

Signed-off-by: Sven Peter <sven@svenpeter.dev>
Signed-off-by: Sven Peter <sven@svenpeter.dev>
Signed-off-by: Sven Peter <sven@svenpeter.dev>
Signed-off-by: Sven Peter <sven@svenpeter.dev>
Signed-off-by: Sven Peter <sven@svenpeter.dev>
@Lunaphied
Copy link
Contributor

Github's review system is very strange and seems to break completely when someone force pushes (I would remove my previous change requests or like to mark resolved ones that have been discussed), but overall I think once those last few changes are made this is probably good to merge. Thanks for putting up with my confusion @svenpeter42.

@svenpeter42
Copy link
Member Author

thanks for the review and the tests on qemu @modwizcode! those definitely improved the code :)

@maximus64
Copy link
Contributor

I tried out on my Macbook Air and SMP boot is working. Font also much bigger than before heh. lgtm

Let's not resize and squish the rendered fonts :-)

Also, it's faster to render the whole font all at once.

Signed-off-by: Hector Martin <marcan@marcan.st>
@marcan
Copy link
Member

marcan commented Feb 18, 2021

The terminal looks good; the font looks hideous. drmr would've never let us get away with that in BootMii :-)

I figured out what went wrong with the converter, so now we're good.

Before:

image

After:

image

@marcan
Copy link
Member

marcan commented Feb 18, 2021

Hm, the "logo avoidance" is kind of wonky:

image

(This is with Retina mode forced on a non-Retina output).

Looks like you need to check the right side of the character, not the left. But... do we really want this kind of weird "wrap-through" behavior?

Personally, when I was thinking about the console, I was just considering limiting the console to the left half of the screen, never going beyond the left edge of the logo. I can see how this approach lets the logo be where it is while letting use use more width but... it feels kind of odd.

One nice thing about not playing games with the logo is that you can scroll by just doing block memmoves; you need to do it linewise to avoid moving the logo, but at least you don't need to repaint every character.

I've also noticed that, as you would expect, once the MMU shuts down the framebuffer becomes Linux-on-crappy-GPU-fbcon-in-the-2000s-quality slow. This is chiefly due to the scroll; there are only a couple lines being printed at that point. This might or might not be improved by doing row-wise scrolling, but either way, can we add a hack for this? Something like a console function that scrolls without moving the true cursor, opening up, say, 3 lines, so that the MMU code can call that prior to MMU shutdown in anticipation that there will be one or two lines printed after that, so we avoid having to scroll.

@marcan
Copy link
Member

marcan commented Feb 18, 2021

Also, aren't we cropping off long lines instead of wrapping, the way it's done now?

It feels wrong dealing with the console as a buffer of text lines that then get blitted as-is. Text needs to be wrapped (on a character boundary, not word wrap) as it is "reshaped" into console size. If we do scrolling the way I suggested, we don't need to keep the concept of text lines around; writing text just becomes a matter of keeping track of the cursor position, blitting characters, and moving down on newline/overflow, and scrolling once we reach the bottom edge.

To print early text before console init, at that point, you could just buffer it as plain text (though you might still want to count backwards the console height to avoid wasting time rendering lines that will scroll off, but you still might waste a bit of time with overflow). Then it would be wrapped at the right width when the console is initialized and the backlog rendered.

@Lunaphied
Copy link
Contributor

These are good points that I assumed were somewhat unimportant in my review because I can’t see how much more output would be required and the issue with wrapping around the logo is honestly triggered very rarely. However you’re right, we can simply blit one character at a time and it fixes every issue. It also lets you just overwrite only the part of the logo that you would encroach onto, which was going to be my suggestion originally before realizing it was hard to implement without a different rendering approach.

Is the reason for the lagginess with the MMU turned off related to that disabling caching/write combining? I’ve always wondered why otherwise fast processors struggle with rendering high-res console text.

@marcan
Copy link
Member

marcan commented Feb 19, 2021

Yes, the lag is what happens when the framebuffer is uncached, which it is on many platforms (M1 is actually kind of an exception since it is a unified memory coherent system).

@marcan
Copy link
Member

marcan commented Feb 19, 2021

Oh, one more thing: there should be a proxy command to enable/disable the fbcon. Obviously printing to the FB adds latency, and that might slow down some experiments (e.g. where exception handlers are spamming dumps) or be otherwise undesirable (e.g. if we're playing with the framebuffer anyway).

@svenpeter42
Copy link
Member Author

fixed in #38

@svenpeter42 svenpeter42 deleted the fb-console branch April 14, 2021 15:54
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.

None yet

4 participants