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

Framebuffer configuration #179

Merged
merged 10 commits into from
Oct 7, 2021
Merged

Framebuffer configuration #179

merged 10 commits into from
Oct 7, 2021

Conversation

aurelilia
Copy link
Contributor

@aurelilia aurelilia commented Jun 20, 2021

This pull request adds minimum_framebuffer_height and minimum_framebuffer_width to the bootloader configuration, and makes the UEFI/BIOS bootloader select and set a GOP/VESA mode with those parameters if one is available.

Some things I'm not quite happy with yet, but am unsure how to fix:

  • This does handle BIOS/VESA. I sadly don't have much knowledge of x86 assembly and don't think I'd be able to add this
  • Config gets written twice now, so that bin/uefi.rs has access to the config. Seems not ideal, but unsure how to improve this.

This is my first PR and I hope it is useful. Thank you for this great project! It has been a great help in learning about OS development.

Copy link

@Andy-Python-Programmer Andy-Python-Programmer left a comment

Choose a reason for hiding this comment

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

For the BIOS video mode, the crate actually does set it up from what I see, I personally have not tried it but you can see it right here:

.ascii ": Is this OK? (s)ave/(y)es/(n)o "
Just from a quick view.

src/bin/uefi.rs Outdated Show resolved Hide resolved
@aurelilia
Copy link
Contributor Author

aurelilia commented Jun 21, 2021

Read through vesa.s a bit more throughly and experimented a bit, VESA is also configurable now! Thanks for the hint Andy, I didn't see the vesa_minx/y at first.

@aurelilia aurelilia changed the title UEFI GOP mode configuration Framebuffer configuration Jun 21, 2021
@aurelilia
Copy link
Contributor Author

Just updated the PR to current main. @phil-opp would it be possible to get some feedback on this and hopefully merge it?

@phil-opp
Copy link
Member

Sorry for the delay and thanks a lot for tackling this!

Looks good overall. Some ideas for improvements:

  • It looks like you're trying to get an exact resolution match on UEFI, but only use the supplied resolution as minimum bounds on BIOS. This seems a bit inconsistent. The easiest solution would be to set treat the numbers as minima for UEFI too (and change the config field names accordingly).
  • The only reason for the duplicated config write is that you need different import paths for the Config struct, right? Perhaps it is possible to instead make the parsed_config module public and then access it directly from bin/uefi.rs (via use bootloader::binary::parsed_config::CONFIG) instead of including the config there too?

@aurelilia
Copy link
Contributor Author

Alright, got both of them done! UEFI now correctly treats it as minima (+ rename to minimum_framebuffer_*) and your idea of accessing the config module worked perfectly. Updated to current main as well.

@aurelilia
Copy link
Contributor Author

@phil-opp Any updates? This is ready for merge.

Copy link
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

Thanks for the ping! The code looks good to me, thanks a lot! Have you tested both the BIOS and UEFI variants in QEMU?

It seems like the CI did not start for some reason. Could you push an additional dummy commit (or rebase the PR) to trigger it?

@aurelilia
Copy link
Contributor Author

I pushed another commit, however GitHub is telling me that the CI workflow is awaiting approval ("First-time contributors need a maintainer to approve running workflows", it links to this article).
Could you approve it? @phil-opp

@phil-opp
Copy link
Member

phil-opp commented Oct 7, 2021

Thanks!

@phil-opp phil-opp merged commit 0c16754 into rust-osdev:main Oct 7, 2021
@phil-opp
Copy link
Member

phil-opp commented Oct 7, 2021

Published as v0.10.9

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.

3 participants