Hi Eric
On 10/28/14 08:01, Dong, Eric wrote:
> Laszlo,
>
> I refine the code in ShowProgress(). when TimeoutDefault is zero, BDS
> also read key. I have verified it works. Please check this new code.
>
> Thanks,
> Eric
> -----Original Message-----
> From: Laszlo Ersek [mailto:[email protected]]
> Sent: Thursday, October 23, 2014 7:34 PM
> To: [email protected]
> Subject: [edk2] [PATCH v2 2/9] IntelFrameworkModulePkg: BdsDxe: poll keyboard
> at front page with zero timeout
>
> When PlatformBdsEnterFrontPage() is called with a zero TimeoutDefault value,
> PlatformBdsEnterFrontPage() --> ShowProgress() returns EFI_TIMEOUT
> immediately, without looking at the keyboard at all.
>
> This is slightly incorrect, because a keypress might already be pending at
> that time. Change ShowProgress() such that it polls the keyboard once even if
> TimeoutDefault equals zero, using a 100 nanosecond event timeout.
>
> (The UEFI specification explicitly allows a zero TriggerTime argument in
> gBS->SetTimer() -- meaning "next timer tick" for TimerRelative --, but
> passing zero to gBS->SetTimer() would require a reorganization of the
> WaitForSingleEvent() utility function. So let's just use a 100ns timeout
> here: it is indistinguishable from "next timer tick" for the purposes of
> polling the keyboard for an already pending keypress.)
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <[email protected]>
> ---
>
> Notes:
> v2:
> - new in v2
>
> IntelFrameworkModulePkg/Universal/BdsDxe/FrontPage.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/FrontPage.c
> b/IntelFrameworkModulePkg/Universal/BdsDxe/FrontPage.c
> index 219f691..c093416 100644
> --- a/IntelFrameworkModulePkg/Universal/BdsDxe/FrontPage.c
> +++ b/IntelFrameworkModulePkg/Universal/BdsDxe/FrontPage.c
> @@ -931,7 +931,15 @@ ShowProgress (
> EFI_GRAPHICS_OUTPUT_BLT_PIXEL Color;
>
> if (TimeoutDefault == 0) {
> - return EFI_TIMEOUT;
> + //
> + // this amounts to a poll -- 1 * 100ns timeout
> + //
> + Status = WaitForSingleEvent (gST->ConIn->WaitForKey, 1);
> +
> + if (Status == EFI_TIMEOUT) {
> + return EFI_TIMEOUT;
> + }
> + return HandleKeyPress ();
> }
>
> DEBUG ((EFI_D_INFO, "\n\nStart showing progress bar... Press any key to
> stop it! ...Zzz....\n"));
> --
> 1.8.3.1
>
>
>
> ------------------------------------------------------------------------------
> _______________________________________________
> edk2-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>
>
> FrontPage.c.patch
>
>
> Index: Universal/BdsDxe/FrontPage.c
> ===================================================================
> --- Universal/BdsDxe/FrontPage.c (revision 16227)
> +++ Universal/BdsDxe/FrontPage.c (working copy)
> @@ -891,64 +891,62 @@
> EFI_GRAPHICS_OUTPUT_BLT_PIXEL Background;
> EFI_GRAPHICS_OUTPUT_BLT_PIXEL Color;
>
> - if (TimeoutDefault == 0) {
> - return EFI_TIMEOUT;
> - }
> + if (TimeoutDefault != 0) {
> + DEBUG ((EFI_D_INFO, "\n\nStart showing progress bar... Press any key to
> stop it! ...Zzz....\n"));
>
> - DEBUG ((EFI_D_INFO, "\n\nStart showing progress bar... Press any key to
> stop it! ...Zzz....\n"));
> + SetMem (&Foreground, sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL), 0xff);
> + SetMem (&Background, sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL), 0x0);
> + SetMem (&Color, sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL), 0xff);
> +
> + TmpStr = GetStringById (STRING_TOKEN (STR_START_BOOT_OPTION));
>
> - SetMem (&Foreground, sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL), 0xff);
> - SetMem (&Background, sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL), 0x0);
> - SetMem (&Color, sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL), 0xff);
> -
> - TmpStr = GetStringById (STRING_TOKEN (STR_START_BOOT_OPTION));
> -
> - if (!FeaturePcdGet(PcdBootlogoOnlyEnable)) {
> - //
> - // Clear the progress status bar first
> - //
> - if (TmpStr != NULL) {
> - PlatformBdsShowProgress (Foreground, Background, TmpStr, Color, 0, 0);
> - }
> - }
> -
> -
> - TimeoutRemain = TimeoutDefault;
> - while (TimeoutRemain != 0) {
> - DEBUG ((EFI_D_INFO, "Showing progress bar...Remaining %d second!\n",
> TimeoutRemain));
> -
> - Status = WaitForSingleEvent (gST->ConIn->WaitForKey, ONE_SECOND);
> - if (Status != EFI_TIMEOUT) {
> - break;
> - }
> - TimeoutRemain--;
> -
> if (!FeaturePcdGet(PcdBootlogoOnlyEnable)) {
> //
> - // Show progress
> + // Clear the progress status bar first
> //
> if (TmpStr != NULL) {
> - PlatformBdsShowProgress (
> - Foreground,
> - Background,
> - TmpStr,
> - Color,
> - ((TimeoutDefault - TimeoutRemain) * 100 / TimeoutDefault),
> - 0
> - );
> + PlatformBdsShowProgress (Foreground, Background, TmpStr, Color, 0,
> 0);
> }
> }
> - }
> -
> - if (TmpStr != NULL) {
> - gBS->FreePool (TmpStr);
> - }
> +
>
> - //
> - // Timeout expired
> - //
> - if (TimeoutRemain == 0) {
> - return EFI_TIMEOUT;
> + TimeoutRemain = TimeoutDefault;
> + while (TimeoutRemain != 0) {
> + DEBUG ((EFI_D_INFO, "Showing progress bar...Remaining %d second!\n",
> TimeoutRemain));
> +
> + Status = WaitForSingleEvent (gST->ConIn->WaitForKey, ONE_SECOND);
> + if (Status != EFI_TIMEOUT) {
> + break;
> + }
> + TimeoutRemain--;
> +
> + if (!FeaturePcdGet(PcdBootlogoOnlyEnable)) {
> + //
> + // Show progress
> + //
> + if (TmpStr != NULL) {
> + PlatformBdsShowProgress (
> + Foreground,
> + Background,
> + TmpStr,
> + Color,
> + ((TimeoutDefault - TimeoutRemain) * 100 / TimeoutDefault),
> + 0
> + );
> + }
> + }
> + }
> +
> + if (TmpStr != NULL) {
> + gBS->FreePool (TmpStr);
> + }
> +
> + //
> + // Timeout expired
> + //
> + if (TimeoutRemain == 0) {
> + return EFI_TIMEOUT;
> + }
> }
>
> //
Sorry about the late response.
So, first of all, it's easier to see what your patch does if one
generates the diff without regard to whitespace (git diff -b). Then it
becomes:
------------
diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/FrontPage.c
b/IntelFrameworkModulePkg/Universal/BdsDxe/FrontPage.c
index f69b17c..a0c6381 100644
--- a/IntelFrameworkModulePkg/Universal/BdsDxe/FrontPage.c
+++ b/IntelFrameworkModulePkg/Universal/BdsDxe/FrontPage.c
@@ -891,10 +891,7 @@ ShowProgress (
EFI_GRAPHICS_OUTPUT_BLT_PIXEL Background;
EFI_GRAPHICS_OUTPUT_BLT_PIXEL Color;
- if (TimeoutDefault == 0) {
- return EFI_TIMEOUT;
- }
-
+ if (TimeoutDefault != 0) {
DEBUG ((EFI_D_INFO, "\n\nStart showing progress bar... Press any
key to stop it! ...Zzz....\n"));
SetMem (&Foreground, sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL), 0xff);
@@ -950,6 +947,7 @@ ShowProgress (
if (TimeoutRemain == 0) {
return EFI_TIMEOUT;
}
+ }
//
// User pressed some key
------------
This is easier to verify.
Basically, if TimeoutDefault equals 0, then we jump immediately to
gST->ConIn->ReadKeyStroke(), without waiting for the
gST->ConIn->WaitForKey event.
This way ReadKeyStroke() can return EFI_SUCCESS (same as before, if a
keypress has been pending by the time we reach this code), or
ReadKeyStroke() can return EFI_NOT_READY (also immediately).
This changes the return code for this case from EFI_TIMEOUT to
EFI_NOT_READY. However, the only caller, PlatformBdsEnterFrontPage(),
handles this status code identically to EFI_TIMEOUT, so it's fine.
Reviewed-by: Laszlo Ersek <[email protected]>
Tested-by: Laszlo Ersek <[email protected]>
Jordan, if you drop the first two patches form this series, then the
rest (patches v2 3/9 to 9/9) apply just as fine, and work as intended
(on top of Eric's patch). If you're OK with the series in that form (as
in, R-b), then I'll await Eric's commit, and then push patches 3 to 9 on
top.
Thanks!
Laszlo
------------------------------------------------------------------------------
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
