[edk2,v2,2/9] IntelFrameworkModulePkg: BdsDxe: poll keyboard at front page with zero timeout

From: Jordan Justen [mailto:jordan.l.justen@intel.com]

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:lersek@redhat.com] 
> Sent: Thursday, October 23, 2014 7:34 PM
> To: edk2-devel@lists.sourceforge.net
> 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 <lersek@redhat.com>
> ---
> 
> 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
> edk2-devel@lists.sourceforge.net
> 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:

------------
     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 <lersek@redhat.com>
Tested-by: Laszlo Ersek <lersek@redhat.com>

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,v2,2/9] IntelFrameworkModulePkg: BdsDxe: poll keyboard at front page with zero timeout

Commit Message

Comments

Patch