On 25 March 2016 at 03:52, Ni, Ruiyu <ruiyu...@intel.com> wrote: > Heyi, > I was not able to remove the blue bar in the beginning of every line if I > embedded my reply. > So I will directly write down here. > > > 1. I would like to know in what circumstance the key loss happens. > Because your UART hardware has 32 FIFO buffer to receive the keys. > A typical escape keys (corresponding to Arrow Left, Arrow Right, or F1-F12) > combination > would only need 3-4 bytes. The FIFO buffer is sufficient to hold about 10 > combinations. >
The loss of input characters occurs typically when you copy-paste something into the terminal window. > > 2. Your system's timer hardware is fast enough so your timer callback > can be triggered > every 2 (2.78) ms, then terminal driver should be able to capture all the > escape keys. > Now since terminal driver cannot capture all the escape keys, so you guess > the callback is > not triggered every 2ms. > Can you find a way to prove it? for example, save the time every time the > callback is called. > Then after like 100 rounds of callback, dump all the 100 time values. > Let's see the real time interval value. > > > 3. I forget about the FIFO depth change. The FIFO depth change doesn't > cause the device > > path reinstallation. I agree we need to detect the FIFO depth change in timer > call back > > to update the interval. > > > Regards, > Ray > > From: Heyi Guo [mailto:heyi....@linaro.org] > Sent: Thursday, March 24, 2016 10:59 PM > To: Ni, Ruiyu <ruiyu...@intel.com>; edk2-devel@lists.01.org > Cc: Tian, Feng <feng.t...@intel.com>; Zeng, Star <star.z...@intel.com> > Subject: Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate > by serial IO mode > > Hi Ruiyu, > > I had seen the other comments and I just needed more time to think about them > :) > > Please see my comments below. > On 03/24/2016 03:26 PM, Ni, Ruiyu wrote: > Heyi, > I had 7 comments in previous mail. I guess you may miss the other comments. > > What Timer driver are you using? How many ms does one system tick cost? > If you are using a less-precise timer driver/hardware, one tick may be larger > than > what you expected. > We are using ArmPlatformPkg/Drivers/SP804TimerDxe/SP804TimerDxe.inf as timer > driver. > The frequency of the timer is 200MHz. Timer interrupt period is set to 1ms. > > > > And can you tell me all the UART parameters in your case? What timer interval > did you get using the equation? > Baud rate = 115200 > FIFO depth = 32 > Parity = 0 > stop = 1 > Data size = 8 > Polling interval = (1 + 8 + 0 + 1) * 32 / 115200 = 0.00278 (2.78ms) > > And please continue to see my opinions for other comments below: > > > > Regards, > Ray > > >>-----Original Message----- >>From: Heyi Guo [mailto:heyi....@linaro.org] >>Sent: Wednesday, March 23, 2016 5:44 PM >>To: Ni, Ruiyu <ruiyu...@intel.com><mailto:ruiyu...@intel.com>; >>edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>Cc: Tian, Feng <feng.t...@intel.com><mailto:feng.t...@intel.com>; Zeng, Star >><star.z...@intel.com><mailto:star.z...@intel.com>; Kinney, Michael D >><michael.d.kin...@intel.com><mailto:michael.d.kin...@intel.com> >>Subject: Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate >>by serial IO mode >> >>Hi Ruiyu, >> >>Many thanks for your review. >> >>For questions 2#, I tested equation #1 with copy-paste on serial >>terminal, and found it still missing some characters when the FIFO was >>almost full (i.e. near the end of input). I think it is because time >>event is not real-time interrupt; it may be pending for equal or higher TPL. >> >>And a little faster polling rate tested well on real environment. >> >>Please let me know your suggestion. >> >>Regards. >> >>Heyi >> >>On 03/23/2016 04:03 PM, Ni, Ruiyu wrote: >>> Heyi, >>> I have comments in below. >>> >>> Regards, >>> Ray >>> >>> >>>> -----Original Message----- >>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of >>>> Heyi Guo >>>> Sent: Thursday, March 17, 2016 10:37 PM >>>> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>>> Cc: Heyi Guo <heyi....@linaro.org><mailto:heyi....@linaro.org>; Tian, Feng >>>> <feng.t...@intel.com><mailto:feng.t...@intel.com>; Zeng, Star >>>> <star.z...@intel.com><mailto:star.z...@intel.com> >>>> Subject: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate >>>> by serial IO mode >>>> >>>> Calculate serial input polling rate according to parameters from >>>> serial IO mode as below, to fix potential input truncation with fixed >>>> polling interval 0.02s. >>>> >>>> Polling interval (100ns) = >>>> FifoDepth * (ParityBits + StopBits + DataBits) * 10,000,000 / BaudRate >>> 1. The above equation (let's call it equation #1) looks good. >>> > Sorry I think I missed the start bit, which should always be 1 bit. > > >>>> However, as UEFI events will probably delayed by other code of higher >>>> TPL, we use below equation to make polling rate fast enough: >>>> >>>> FifoDepth * DataBits * 10,000,000 * 2 / (BaudRate * 3) >>> 2. Can you tell me why you multiple two in the left of "/" and multiple 3 >>> in the right? >>> Did you meet any real problem when using the original equation (#1)? >>> >>> I have more comments in below. >>> >>>> Signed-off-by: Heyi Guo <heyi....@linaro.org><mailto:heyi....@linaro.org> >>>> Cc: Feng Tian <feng.t...@intel.com><mailto:feng.t...@intel.com> >>>> Cc: Star Zeng <star.z...@intel.com><mailto:star.z...@intel.com> >>>> --- >>>> .../Universal/Console/TerminalDxe/Terminal.c | 5 +- >>>> .../Universal/Console/TerminalDxe/Terminal.h | 27 ++++++++- >>>> .../Universal/Console/TerminalDxe/TerminalConIn.c | 68 >>>> ++++++++++++++++++++++ >>>> 3 files changed, 98 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c >>>> b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c >>>> index 5adaa97..db790f3 100644 >>>> --- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c >>>> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c >>>> @@ -71,6 +71,7 @@ TERMINAL_DEV mTerminalDevTemplate = { >>>> }, >>>> NULL, // TerminalConsoleModeData >>>> 0, // SerialInTimeOut >>>> + DEFAULT_KEYBOARD_TIMER_INTERVAL, // KeyboardTimerInterval >>> 3. Can you remove the default timer interval? >>> We should be able to calculate the timer interval from the default >>> setting (DataBits, StopBits, BaudRate, etc). >>> > See below. > >>>> NULL, // RawFifo >>>> NULL, // UnicodeFiFo >>>> @@ -984,10 +985,12 @@ TerminalDriverBindingStart ( >>>> ); >>>> ASSERT_EFI_ERROR (Status); >>>> >>>> + TerminalDevice->KeyboardTimerInterval = GetKeyboardTimerInterval >>>> (Mode); >>>> + >>>> Status = gBS->SetTimer ( >>>> TerminalDevice->TimerEvent, >>>> TimerPeriodic, >>>> - KEYBOARD_TIMER_INTERVAL >>>> + TerminalDevice->KeyboardTimerInterval >>>> ); >>>> ASSERT_EFI_ERROR (Status); >>>> >>>> diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h >>>> b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h >>>> index 269d2ae..58d5664 100644 >>>> --- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h >>>> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h >>>> @@ -68,7 +68,7 @@ typedef struct { >>>> UINTN Rows; >>>> } TERMINAL_CONSOLE_MODE_DATA; >>>> >>>> -#define KEYBOARD_TIMER_INTERVAL 200000 // 0.02s >>>> +#define DEFAULT_KEYBOARD_TIMER_INTERVAL 200000 // 0.02s >>> >>> 4. Same question as #3. >>> >>>> #define TERMINAL_DEV_SIGNATURE SIGNATURE_32 ('t', 'm', 'n', 'l') >>>> >>>> @@ -91,6 +91,7 @@ typedef struct { >>>> EFI_SIMPLE_TEXT_OUTPUT_MODE SimpleTextOutputMode; >>>> TERMINAL_CONSOLE_MODE_DATA *TerminalConsoleModeData; >>>> UINTN SerialInTimeOut; >>>> + UINT64 KeyboardTimerInterval; >>>> RAW_DATA_FIFO *RawFiFo; >>>> UNICODE_FIFO *UnicodeFiFo; >>>> EFI_KEY_FIFO *EfiKeyFiFo; >>>> @@ -1358,4 +1359,28 @@ TerminalConInTimerHandler ( >>>> IN EFI_EVENT Event, >>>> IN VOID *Context >>>> ); >>>> + >>>> +/** >>>> + Calculate input polling timer interval by serial IO mode. >>>> + >>>> + @param Mode Pointer to serial IO mode. >>>> + >>>> + @retval The required polling timer interval in 100ns. >>>> + >>>> +**/ >>>> +UINT64 >>>> +GetKeyboardTimerInterval ( >>>> + IN EFI_SERIAL_IO_MODE *Mode >>>> + ); >>>> + >>>> +/** >>>> + Update period of polling timer event. >>>> + >>>> + @param TerminalDevice The terminal device to update. >>>> +**/ >>>> +VOID >>>> +UpdatePollingRate ( >>>> + IN TERMINAL_DEV *TerminalDevice >>>> + ); >>>> + >>>> #endif >>>> diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c >>>> b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c >>>> index 2215df6..22349a0 100644 >>>> --- a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c >>>> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c >>>> @@ -502,6 +502,71 @@ TerminalConInWaitForKey ( >>>> } >>>> >>>> /** >>>> + Calculate input polling timer interval by serial IO mode. >>>> + >>>> + @param Mode Pointer to serial IO mode. >>>> + >>>> + @retval The required polling timer interval in 100ns. >>>> + >>>> +**/ >>>> +UINT64 >>>> +GetKeyboardTimerInterval ( >>>> + IN EFI_SERIAL_IO_MODE *Mode >>>> + ) >>>> +{ >>>> + UINT32 FifoDepth; >>>> + >>>> + if (Mode->BaudRate == 0) { >>>> + return DEFAULT_KEYBOARD_TIMER_INTERVAL; >>>> + } >>> 5. Now I understand you need the default timer interval for the default >>> setting. >>> Because UEFI spec says when the baud rate has the value of 0 to indicate >>> the device >>> runs at the device's *designed speed*. >>> But I still think you can calculate the timer interval assuming the >>> baudrate is 115200. >>> Because if using 115200 the timer interval is about 0.001s which is still >>> shorter than 0.02s. >>> > Sounds good for me. > > >>>> + >>>> + FifoDepth = Mode->ReceiveFifoDepth; >>>> + // Fix incorrect FIFO depth >>>> + if (FifoDepth == 0) { >>>> + FifoDepth = 1; >>>> + } >>>> + >>>> + // We ignore stop bits and parity bit, and reduce the interval by 1/3, >>>> + // to make polling rate fast enough to avoid serial input truncation. >>>> + return DivU64x64Remainder ( >>>> + FifoDepth * Mode->DataBits * 10000000 * 2, >>>> + Mode->BaudRate * 3, >>>> + NULL >>>> + ); >>>> +} >>> 6. Same question. Can you use the original equation #1? >>> >>>> + >>>> + >>>> +/** >>>> + Update period of polling timer event. >>>> + >>>> + @param TerminalDevice The terminal device to update. >>>> +**/ >>>> +VOID >>>> +UpdatePollingRate ( >>>> + IN TERMINAL_DEV *TerminalDevice >>>> + ) >>>> +{ >>>> + UINT64 NewInterval; >>>> + EFI_STATUS Status; >>>> + >>>> + NewInterval = GetKeyboardTimerInterval (TerminalDevice->SerialIo->Mode); >>>> + >>>> + if (TerminalDevice->KeyboardTimerInterval == NewInterval) { >>>> + return; >>>> + } >>>> + >>>> + Status = gBS->SetTimer ( >>>> + TerminalDevice->TimerEvent, >>>> + TimerPeriodic, >>>> + NewInterval >>>> + ); >>>> + ASSERT_EFI_ERROR (Status); >>>> + >>>> + TerminalDevice->KeyboardTimerInterval = NewInterval; >>>> +} >>>> + >>>> + >>>> +/** >>>> Timer handler to poll the key from serial. >>>> >>>> @param Event Indicates the event that invoke this >>>> function. >>>> @@ -560,6 +625,9 @@ TerminalConInTimerHandler ( >>>> TerminalDevice->SerialInTimeOut = SerialInTimeOut; >>>> } >>>> } >>>> + >>>> + UpdatePollingRate (TerminalDevice); >>> 7. All the parameters (baudrate, databits, stopbits,etc) are stored in the >>> UART device path. >>> Any change to these parameters triggers the UART device path >>> reinstallation, ultimately >>> triggers the Terminal driver's restart. >>> So the polling rate doesn't need to update here. >>> > How about FIFO depth change? > > Thanks for your review :) > > Heyi > > > >>>> + >>>> // >>>> // Check whether serial buffer is empty. >>>> // Skip the key transfer loop only if the SerialIo protocol instance >>>> -- >>>> 2.7.0 >>>> >>>> _______________________________________________ >>>> edk2-devel mailing list >>>> edk2-devel@lists.01.org<mailto: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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
