> On Oct 11, 2019, at 9:33 AM, Laszlo Ersek <ler...@redhat.com> wrote:
>
> Hi Andrew,
>
> On 10/11/19 15:13, Andrew Fish wrote:
>
>> I'm with Pete on this as my expectation would be the progress bar
>> completes when you are done. I'd also point out that a common progress
>> bar UI implementation is to show the area that is going to be updated.
>> I don't think our UI does that, but it could get updated at some
>> point. Thus I would posit that if PlatformBootManagerWaitCallback() is
>> called by passing in PcdPlatformBootTimeOut you should always call
>> PlatformBootManagerWaitCallback() with zero to terminate the progress
>> bar.
>
> OK.
>
>> What makes sense to me is to skip the UI if PcdPlatformBootTimeOut is
>> zero. The PlatformBootManagerWaitCallback (0) is only really needed if
>> PlatformBootManagerWaitCallback (0) has not been called (TimeoutRemain
>> != 0).
>
> Let me paste your patch below, formatted with the
> "--ignore-space-change" option (it hides changes due to simple
> re-indentation of code):
>
>> diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>> b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>> index 7968a58f3454..fd551d17e15d 100644
>> --- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>> +++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>> @@ -310,48 +310,52 @@ VOID
>> BdsWait (
>> IN EFI_EVENT HotkeyTriggered
>> )
>> {
>> EFI_STATUS Status;
>> UINT16 TimeoutRemain;
>>
>> DEBUG ((EFI_D_INFO, "[Bds]BdsWait ...Zzzzzzzzzzzz...\n"));
>>
>> TimeoutRemain = PcdGet16 (PcdPlatformBootTimeOut);
>> + if (TimeoutRemain != 0) {
>> while (TimeoutRemain != 0) {
>> DEBUG ((EFI_D_INFO, "[Bds]BdsWait(%d)..Zzzz...\n", (UINTN)
>> TimeoutRemain));
>> PlatformBootManagerWaitCallback (TimeoutRemain);
>>
>> BdsReadKeys (); // BUGBUG: Only reading can signal HotkeyTriggered
>> // Can be removed after all keyboard drivers
>> invoke callback in timer callback.
>>
>> if (HotkeyTriggered != NULL) {
>> Status = BdsWaitForSingleEvent (HotkeyTriggered,
>> EFI_TIMER_PERIOD_SECONDS (1));
>> if (!EFI_ERROR (Status)) {
>> break;
>> }
>> } else {
>> gBS->Stall (1000000);
>> }
>>
>> //
>> // 0xffff means waiting forever
>> // BDS with no hotkey provided and 0xffff as timeout will "hang" in
>> the loop
>> //
>> if (TimeoutRemain != 0xffff) {
>> TimeoutRemain--;
>> }
>> }
>> + if (TimeoutRemain != 0) {
>> PlatformBootManagerWaitCallback (0);
>> + }
>> + }
>> DEBUG ((EFI_D_INFO, "[Bds]Exit the waiting!\n"));
>> }
>
> This would solve the (PcdPlatformBootTimeOut==0) case well.
>
> However, it would regress Pete's use case. Namely, if you allow the loop
> to time out (the "while" statement sees a zero "TimeoutRemain"), then
> the final PlatformBootManagerWaitCallback (0) is skipped.
>
> And that's the problem Pete intended to fix in the first place: when the
> loop times out, PlatformBootManagerWaitCallback (0) is *never* called,
> inside the loop. So we start booting e.g. an OS, without the progress
> bar filling the screen to the right edge.
>
> I think if we eliminate the *inner* "if" statement -- call
> PlatformBootManagerWaitCallback (0) unconditionally --, and preserve the
> outer "if" statement, it should work.
>
Laszlo,
Good point. I was trying to prevent 2 calls with 0 but now I notice that is not
possible in the while loop. So I agree with your suggestion. I also like Pete's
idea of adding the error check in PlatformBootManagerWaitCallback().
Thanks,
Andrew Fish
> Thanks
> Laszlo
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48852): https://edk2.groups.io/g/devel/message/48852
Mute This Topic: https://groups.io/mt/34479934/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
