It makes sense that F2 to F9 behaves like ESC because the Serial device doesn't have enough buffer (FIFO) to hold all the escape sequence keys.
Because the certain platform cannot handle escape sequence keys like F2-F9 or cursor movement, I think it's reasonable to use a bigger FIFO other 1 in that certain platform. Regards, Ray -----Original Message----- From: Ryan Harkin [mailto:ryan.har...@linaro.org] Sent: Thursday, January 21, 2016 4:54 PM To: Laszlo Ersek <ler...@redhat.com> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Leif Lindholm <leif.lindh...@linaro.org>; edk2-de...@ml01.01.org; Zeng, Star <star.z...@intel.com>; Ard Biesheuvel <ard.biesheu...@linaro.org> Subject: Re: [edk2] [PATCH 3/3] MdeModulePkg: TerminalDxe: select the UART's default receive FIFO depth On 21 January 2016 at 08:42, Laszlo Ersek <ler...@redhat.com> wrote: > On 01/21/16 09:20, Ryan Harkin wrote: >> On 21 January 2016 at 08:14, Laszlo Ersek <ler...@redhat.com> wrote: >>> On 01/21/16 06:42, Ni, Ruiyu wrote: >>>> Laszlo, >>>> When I re-think about the cursor issue, I am curious whether the >>>> function keys (F1/F2/...) work well in your platform. >>>> I guess any keys that will translate to multiple bytes don't work. >>>> Correct? >>> >>> I never experienced this issue personally. Based on the original report, >>> it emerged only on ARM FVP (which is a software emulator) and ARM Juno. >>> So I hope the question about function keys can be answered by one of the >>> CC'd guys. >>> >> >> I don't know how to test F1/F2, etc. as I don't of any software that >> consumes them. I'm currently using Intel BDS and that only appears to >> use cursor key, enter and regular ASCII printable characters. > > No, the Intel BDS is actually a good utility to test this. Go into one > of the setup dialogs, and change something, then press F9 or F10 to save > or throw away changes. (Writing from memory, so I'm not sure which does > which, but you can definitely use those function keys on at least some > of the forms.) > Ha! Foiled by my terminal - F10 opens the menu bar in the terminal app. I'm not sure F9 "Reset to Defaults". I was adding a new boot option, and on the "Modify Boot Option Description" page, ESC and F2 to F9 all do the same thing: drop me back to the previous page where I can choose the file I want to boot. The "Input the description" field isn't reset and retains the text I've typed into it. > Thanks > Laszlo > >> >> >>>> But I never heard of such issue in X86 platforms so far even the >>>> IsaSerialDxe driver in IntelFrameworkModulePkg uses FIFO depth >>>> as 1. >>>> Is it because your platform uses a very higher baud rate? >>>> Or the time handler (supposed to be triggered every 0.02s) isn't >>>> triggered every 0.02s but a longer time? >>> >>> No clue, sorry. All we determined was that the FIFO depth of 1 caused >>> the PL011Uart library to disable queueing on the device completely. >>> Apparently, for handling isolated scan codes, no FIFO logic is necessary >>> on the PL011Uart. (And isolated keystrokes did work well, only bursts of >>> scan codes didn't.) >>> >> >> I don't know if it's related, but when looking into why copy/paste >> doesn't work past one FIFO length, another Linaro team proposed that >> the timeout was too short for our baud rate. I never managed to get >> this to work by changing the timeout. >> >>> Thanks >>> Laszlo >>> >>>> >>>> Regards, >>>> Ray >>>> >>>> -----Original Message----- >>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of >>>> Laszlo Ersek >>>> Sent: Thursday, January 21, 2016 11:23 AM >>>> To: Ni, Ruiyu <ruiyu...@intel.com> >>>> Cc: Leif Lindholm <leif.lindh...@linaro.org>; edk2-de...@ml01.01.org; Ryan >>>> Harkin <ryan.har...@linaro.org>; Zeng, Star <star.z...@intel.com>; Ard >>>> Biesheuvel <ard.biesheu...@linaro.org> >>>> Subject: Re: [edk2] [PATCH 3/3] MdeModulePkg: TerminalDxe: select the >>>> UART's default receive FIFO depth >>>> >>>> On 01/21/16 03:20, Ni, Ruiyu wrote: >>>>> Laszlo, >>>>> I may not agree with your change to the Terminal driver. >>>>> Firstly it's not a good code practice to not preserve the other fields >>>>> when changing >>>>> the time out value. People may forget that the FIFO depth was changed in >>>>> Terminal >>>>> driver after a long enough period. >>>> >>>> I don't understand this argument. People may forget anything at all; >>>> that's why they should look at the code. >>>> >>>> Do you mean that the receive FIFO depth parameter should be changed in a >>>> different / separate SetAttributes() call, in the BindingStart() >>>> function of the terminal driver? I.e., independently of the Timeout >>>> setting? >>>> >>>>> Secondly your change resets the FIFO depth but >>>>> it's possible that the device may not in the default FIFO depth which may >>>>> bring >>>>> functionality issue. >>>> >>>> I think I disagree. The serial IO protocol is not a service >>>> binding-style protocol, so when you open it with the BY_DRIVER >>>> attribute, and another agent already has it open with the BY_DRIVER >>>> attribute, EFI_ACCESS_DENIED is returned. This means in practice that >>>> TerminalDxe gets exclusive access to the underlying Serial IO protocol, >>>> and it can configure it whichever way it sees suitable. >>>> >>>> (Same as when a BlockIO or other driver sits atop of a PciIo instance -- >>>> we don't care what state the underlying PciIo instance might be at that >>>> point; we'll just set it up the way we need it for implementing BlockIo >>>> on top.) >>>> >>>>> I have offline talked with Star and have understood the intention of the >>>>> change. >>>>> How about we create a new PCD PcdUartDefaultReceiveFifoDepth and change >>>>> the SerialDxe >>>>> driver to use the PCD value in its entrypoint and Reset() function? So >>>>> that any platform can >>>>> override the default PCD in platform DSC. >>>> >>>> Sure, technically that could work. But if you look at the UEFI >>>> specification, the SerialIO protocol instance, produced by SerialDxe >>>> with such a PCD override, would not conform to the spec. >>>> >>>> (Similarly to the case when you change the already existing PCDs from >>>> their default values -- for example, if you change the baud rate PCD >>>> from 115200, then the resultant SerialIO protocol instance will *also* >>>> not conform.) >>>> >>>>> >>>>> Through this, we also limit the change to SerialDxe driver. After all >>>>> Terminal driver can start >>>>> above any SerialIo instance, not just the one created by the >>>>> MdeModulePkg/Universal/SerialDxe. >>>> >>>> Yes, I fully agree that TerminalDxe should be able to work with any >>>> SerialIO protocol instance, regardless of what UART driver produced it. >>>> >>>> However, the consequence that I draw from this requirement is different >>>> from yours. My argument goes, if TerminalDxe cannot (and it should not) >>>> assume anything particular about the underlying SerialIO protocol >>>> instance, then what it *should* assume, is *only* what the UEFI spec >>>> dictates about the SerialIO protocol defaults. >>>> >>>> Those defaults include "receive FIFO depth = 1", at least right after >>>> Reset(). >>>> >>>> Since TerminalDxe knows for sure that "receive FIFO depth = 1" will not >>>> be appropriate for parsing escape sequences, it can be argued that >>>> TerminalDxe simply cannot avoid setting the receive FIFO depth to >>>> something more suitable than the spec-mandated default. >>>> >>>> Anyway, I don't insist. The only reason I chimed in was that the UEFI >>>> spec explicitly regulates this question, as far as I could see, so we >>>> should try to conform in the reference implementation. >>>> >>>> But, whatever works. Feel free to revert my patches, or restore those >>>> bits as part of another solution. >>>> >>>> Thanks >>>> Laszlo >>>> >>>>> >>>>> Regards, >>>>> Ray >>>>> >>>>> -----Original Message----- >>>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of >>>>> Laszlo Ersek >>>>> Sent: Tuesday, January 19, 2016 9:54 PM >>>>> To: edk2-de...@ml01.01.org >>>>> Cc: Ryan Harkin <ryan.har...@linaro.org>; Zeng, Star >>>>> <star.z...@intel.com>; Leif Lindholm <leif.lindh...@linaro.org>; Ard >>>>> Biesheuvel <ard.biesheu...@linaro.org> >>>>> Subject: [edk2] [PATCH 3/3] MdeModulePkg: TerminalDxe: select the UART's >>>>> default receive FIFO depth >>>>> >>>>> The Serial IO protocol instances provided by SerialDxe and consumed by >>>>> TerminalDxe come with a Mode.ReceiveFifoDepth=1 default setting, as >>>>> required by UEFI 2.5. >>>>> >>>>> Although TerminalDxe calls EFI_SERIAL_IO_PROTOCOL.SetAttributes() in the >>>>> TerminalDriverBindingStart() and TerminalConInTimerHandler() functions, it >>>>> only does so to change the Mode.Timeout member. Other members of Mode, >>>>> including Mode.ReceiveFifoDepth, are preserved. >>>>> >>>>> On some platforms this causes the UART that underlies TerminalDxe not to >>>>> have enough room for bursts of scan codes, which translates to broken >>>>> parsing of escape sequences, e.g. cursor movement keys. >>>>> >>>>> According to the UEFI spec, passing ReceiveFifoDepth=0 to >>>>> EFI_SERIAL_IO_PROTOCOL.SetAttributes() "will use the device's default FIFO >>>>> depth". While TerminalDxe could try to configure a receive FIFO depth that >>>>> matches the longest escape sequence it wishes to parse, in practice the >>>>> device-specific default FIFO depth -- which may well differ from the >>>>> spec-mandated SerialIo->Mode.ReceiveFifoDepth=1 default -- seems to work. >>>>> Hence let's just set that. >>>>> >>>>> This issue was exposed by SVN r18971 / git commit 921e987b2b >>>>> ("ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg"). >>>>> In that conversion, MdeModulePkg's SerialDxe started to initialize >>>>> Mode.ReceiveFifoDepth to 1 (in conformance with the spec), unlike the >>>>> prior, non-conformant initialization to 0 in EmbeddedPkg's SerialDxe. >>>>> >>>>> Since TerminalDxe would never change ReceiveFifoDepth from the new default >>>>> value 1, and the ArmPlatformPkg/Drivers/PL011Uart library instance, >>>>> underlying SerialDxe through SerialPortLib, would obey it too, they would >>>>> collectively effect a receive queue depth of 1, rather than the default 16 >>>>> or 32. This broke cursor keys on the ARM FVP and Juno platforms. >>>>> >>>>> It is the client of EFI_SERIAL_IO_PROTOCOL that is responsible for >>>>> modifying the attributes, if the defaults are not appropriate, hence this >>>>> patch modifies TerminalDxe. >>>>> >>>>> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> >>>>> Cc: Ryan Harkin <ryan.har...@linaro.org> >>>>> Cc: Leif Lindholm <leif.lindh...@linaro.org> >>>>> Cc: Star Zeng <star.z...@intel.com> >>>>> Reported-by: Ryan Harkin <ryan.har...@linaro.org> >>>>> Reference: >>>>> http://thread.gmane.org/gmane.comp.bios.edk2.devel/4779/focus=6553 >>>>> Reference: http://thread.gmane.org/gmane.comp.bios.edk2.devel/6594 >>>>> Contributed-under: TianoCore Contribution Agreement 1.0 >>>>> Signed-off-by: Laszlo Ersek <ler...@redhat.com> >>>>> --- >>>>> MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c | 2 +- >>>>> MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c | 2 +- >>>>> 2 files changed, 2 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c >>>>> b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c >>>>> index f6cee4e..8238a93 100644 >>>>> --- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c >>>>> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c >>>>> @@ -841,7 +841,7 @@ TerminalDriverBindingStart ( >>>>> Status = TerminalDevice->SerialIo->SetAttributes ( >>>>> TerminalDevice->SerialIo, >>>>> Mode->BaudRate, >>>>> - Mode->ReceiveFifoDepth, >>>>> + 0, // the device's default FIFO >>>>> depth >>>>> (UINT32) SerialInTimeOut, >>>>> (EFI_PARITY_TYPE) (Mode->Parity), >>>>> (UINT8) Mode->DataBits, >>>>> diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c >>>>> b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c >>>>> index 3be877b..2215df6 100644 >>>>> --- a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c >>>>> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c >>>>> @@ -547,7 +547,7 @@ TerminalConInTimerHandler ( >>>>> Status = SerialIo->SetAttributes ( >>>>> SerialIo, >>>>> Mode->BaudRate, >>>>> - Mode->ReceiveFifoDepth, >>>>> + 0, // the device's default FIFO depth >>>>> (UINT32) SerialInTimeOut, >>>>> (EFI_PARITY_TYPE) (Mode->Parity), >>>>> (UINT8) Mode->DataBits, >>>>> >>>> >>>> _______________________________________________ >>>> edk2-devel mailing list >>>> edk2-devel@lists.01.org >>>> https://lists.01.org/mailman/listinfo/edk2-devel >>>> >>> > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
