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

OcConsoleLib: Prevent verbose boot text overwriting pre-existing graphics #426

Merged
merged 1 commit into from Feb 21, 2023

Conversation

mikebeaton
Copy link
Contributor

@mikebeaton mikebeaton commented Feb 15, 2023

Was not needed/was done by default, prior to a189bd5

I appreciate that this is yet another 'fix' for a problem which was not present prior to recent changes.

However, I also believe that in this case, too, the change + fix is a genuine improvement over the previous code.

In this case:

  • Previous code: Full screen was always cleared to console background colour, on setting up OC console mode, even if we were in and staying in graphics mode. This was noticeable when entering OpenCore from set up and working graphics mode with UI theme colour, e.g. when entering OpenCore from native picker on MacPro with EnableGop driver, and manifested as an unwanted clear screen to black and then back to grey again
  • Current code, before this fix: No such artefact, but if we switch to console mode and then clear screen with graphics (such as BIOS system logo) already on the screen, these are not always cleared
  • Current code after this fix: Neither artefact

The current version of this PR has two commits, and includes #424 .

@mikebeaton
Copy link
Contributor Author

FYI I have just double-checked this, and both commits are required, i.e. the second commit here alone does not fix acidanthera/bugtracker#2225 .

@vandroiy2013
Copy link
Contributor

I'll check in a few hours. Thank you

@mikebeaton
Copy link
Contributor Author

mikebeaton commented Feb 16, 2023

I have detected yet another relevant case, addressed in a revised second commit 583621c.

The is required because, while the code in 2974fbb detects -v in NVRAM boot-args and when passed by OpenCore via loaded image options, it does not (cannot) detect the case where boot.efi itself detects CMD+V and enters verbose boot. This becomes visibly the case if, e.g., we hold down CMD+V while booting without entering the picker, on a hack which has its own BIOS logo; in which case the first lines of verbose boot will write over/next to the logo, in the similar manner to that shown in the videos here: acidanthera/bugtracker#2225 .

After further research on this and in the native picker, I suspect that the reason this works on Macs, is because of some other difference between their Console implementation and ours, as regards when they do/do not clear screen. I no longer think they have a specific work-around for verbose mode in their picker. As far as I can tell, they don't (can't) do (the equivalent of) the clear screen on reset which I removed, however. But all this - probably - does not really matter. Based on how it actually looks, when it transitions into the early lines of verbose boot, on a fully native system, it occurs to me that this may be because they have a true ConSplitter.

@mikebeaton mikebeaton marked this pull request as draft February 16, 2023 11:07
@mikebeaton
Copy link
Contributor Author

mikebeaton commented Feb 16, 2023

I've seen a way to reduce the hackery of commit 3 of this series, so moving the PR back to draft temporarily.

EDIT: Now included in revised second commit of two.

@mikebeaton
Copy link
Contributor Author

mikebeaton commented Feb 16, 2023

@vit9696 - Are you able to review AfterBootCompatLib changes in 583621c .

EDIT: I'm now, instead, verifying an improved, simpler approach, which doesn't require any ABC code.

@mikebeaton mikebeaton marked this pull request as draft February 16, 2023 13:02
@mikebeaton
Copy link
Contributor Author

mikebeaton commented Feb 16, 2023

I'm getting major linkage errors when attempting to call OCABC where I'm calling it from. Moved to draft again.

EDIT: I believe I've resolved this. If so, was not an issue in new code in these commits. Waiting for rebuild

@@ -65,6 +65,7 @@
OcCryptoLib
OcDeviceTreeLib
OcGuardLib
OcLogAggregatorLib
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?

@@ -0,0 +1,25 @@
## @file
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the point of this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't need this to fix the screen clear now, and am going to do a revised version of this PR without it. But I still believe this is wrong and should be fixed to save someone else having to work it out in future.

The Null and Serial versions of DebugLib also included null functions for OcLogAggregatorLib. But OcLogAggregator lib is a separate library from DebugLib.

Because of further library dependencies, this error (as I believe it to be) was making it impossible to link OcAfterBootCompatLib anywhere except where it was already linked. After these changes - which look correct to me anyway - I could link it to another another library, OcBootManagementLib, as I needed to in this current (to be replaced) version of the PR, just fine.

@mikebeaton mikebeaton changed the title OcConsoleLib: Ensure full screen is cleared at least once in console mode OcConsoleLib: Prevent verbose boot text overwriting pre-existing graphics Feb 19, 2023
…hics

 - Clear console screen on change to console mode
 - Ensure entire screen is cleared at least once in console mode
   o Was not needed/was done by default, prior to a189bd5
   o Remove GOP clear screen work-around no longer required with this change.
 - These changes improve EnableGop driver in the same way, so update version number
   o Add EnableGop version in UI section, to enable tool builders to track it
@mikebeaton
Copy link
Contributor Author

mikebeaton commented Feb 19, 2023

This drops all the AfterBootCompatLib stuff.

I think this is a complete, correct, pretty fully tested, and simple solution.

The most significant change is clearing screen when we enter console mode from graphics mode. This is the key to not needing any strange ABC stuff, as it means boot.efi itself tells us whether it is running in console mode, and we clear the screen, or not, in response. (The Apple native system is visually very slightly different - a less than full screen console mode is centred on the screen rather than in the top left - but otherwise looks pretty much identical to how this behaves now.)

@mhaeuser
Copy link
Member

Looks great now - @vit9696 imo test for artifacts and then merge.

@mikebeaton
Copy link
Contributor Author

mikebeaton commented Feb 19, 2023

@vandroiy2013 I don't know if you have a moment to retest this one? I'm pretty sure I could replicate your issue and it is fully fixed now.

This one is tested by me on (hack|mac native|mac+EnableGop driver)x(-v in boot-args|cmd+v in OC|cmd+v detected by boot.efi)x(console picker|canopy)x(ShowPicker:true|false).

@vit9696 vit9696 merged commit 2439d58 into acidanthera:master Feb 21, 2023
@mikebeaton mikebeaton deleted the console branch April 12, 2023 22:14
mikebeaton added a commit to mikebeaton/OpenCorePkg that referenced this pull request Apr 22, 2023
…hics (acidanthera#426)

- Clear console screen on change to console mode
 - Ensure entire screen is cleared at least once in console mode
   o Was not needed/was done by default, prior to a189bd5
   o Remove GOP clear screen work-around no longer required with this change.
 - These changes improve EnableGop driver in the same way, so update version number
   o Add EnableGop version in UI section, to enable tool builders to track it
mikebeaton added a commit to mikebeaton/OpenCorePkg that referenced this pull request Apr 22, 2023
…hics (acidanthera#426)

- Clear console screen on change to console mode
 - Ensure entire screen is cleared at least once in console mode
   o Was not needed/was done by default, prior to a189bd5
   o Remove GOP clear screen work-around no longer required with this change.
 - These changes improve EnableGop driver in the same way, so update version number
   o Add EnableGop version in UI section, to enable tool builders to track it
mikebeaton added a commit to mikebeaton/OpenCorePkg that referenced this pull request Apr 22, 2023
…hics (acidanthera#426)

- Clear console screen on change to console mode
 - Ensure entire screen is cleared at least once in console mode
   o Was not needed/was done by default, prior to a189bd5
   o Remove GOP clear screen work-around no longer required with this change.
 - These changes improve EnableGop driver in the same way, so update version number
   o Add EnableGop version in UI section, to enable tool builders to track it
mikebeaton added a commit to mikebeaton/OpenCorePkg that referenced this pull request Apr 23, 2023
…hics (acidanthera#426)

- Clear console screen on change to console mode
 - Ensure entire screen is cleared at least once in console mode
   o Was not needed/was done by default, prior to a189bd5
   o Remove GOP clear screen work-around no longer required with this change.
mikebeaton added a commit to mikebeaton/OpenCorePkg that referenced this pull request Apr 23, 2023
…hics (acidanthera#426)

- Clear console screen on change to console mode
 - Ensure entire screen is cleared at least once in console mode
   o Was not needed/was done by default, prior to a189bd5
   o Remove GOP clear screen when no valid entry from Apple picker (was added in a653620), no longer required with this change
mikebeaton added a commit to mikebeaton/OpenCorePkg that referenced this pull request Apr 23, 2023
…hics (acidanthera#426)

- Clear console screen on change to console mode
 - Ensure entire screen is cleared at least once in console mode
   o Was not needed/was done by default, prior to a189bd5
   o Remove GOP clear screen when no valid entry from Apple picker (was added in a653620), no longer required with this change
mikebeaton added a commit to mikebeaton/OpenCorePkg that referenced this pull request Apr 23, 2023
…hics (acidanthera#426)

- Clear console screen on change to console mode
 - Ensure entire screen is cleared at least once in console mode
   o Was not needed/was done by default, prior to a189bd5
   o Remove GOP clear screen when no valid entry from Apple picker (was added in a653620), no longer required with this change
mikebeaton added a commit to mikebeaton/OpenCorePkg that referenced this pull request Apr 23, 2023
…hics (acidanthera#426)

- Clear console screen on change to console mode
 - Ensure entire screen is cleared at least once in console mode
   o Was not needed/was done by default, prior to a189bd5
   o Remove GOP clear screen when no valid entry from Apple picker (was added in a653620), no longer required with this change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants