On 29 March 2016 at 06:09, Kinney, Michael D <[email protected]> wrote: > Ard, > > What is the timer rate in the system you are using? >
I am going to let Heyi respond with the actual numbers. > The timer event rate of a UEFI driver can not go any faster or a smaller > granularity than the system timer rate. > > The original formula may be correct, but may not be compensating for the > granularity of the timer rate. > > Is the issue also resolved if you increase the system timer tick rate? > I agree that there is likely a significant quantization error once the poll rate approaches the system tick rate. But I think Heyi's analysis is correct that handlers executing at the same TPL may cause jitter that needs to be compensated for by bias towards zero on the poll rate. Ard. >> -----Original Message----- >> From: edk2-devel [mailto:[email protected]] On Behalf Of Heyi >> Guo >> Sent: Monday, March 28, 2016 8:06 PM >> To: Ard Biesheuvel <[email protected]>; Ni, Ruiyu >> <[email protected]> >> Cc: [email protected]; Tian, Feng <[email protected]>; Zeng, Star >> <[email protected]> >> Subject: Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling >> rate by serial >> IO mode >> >> >> >> On 03/25/2016 01:56 PM, Ard Biesheuvel wrote: >> > On 25 March 2016 at 03:52, Ni, Ruiyu <[email protected]> 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. >> >> Thanks Ard for helping me to reply; that's really the case I want to fix :) >> >> > >> >> 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. >> OK, I'll try that and let you know the result. >> >> Thanks and regards. >> >> Heyi >> >> >> >> >> 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:[email protected]] >> >> Sent: Thursday, March 24, 2016 10:59 PM >> >> To: Ni, Ruiyu <[email protected]>; [email protected] >> >> Cc: Tian, Feng <[email protected]>; Zeng, Star <[email protected]> >> >> 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:[email protected]] >> >>> Sent: Wednesday, March 23, 2016 5:44 PM >> >>> To: Ni, Ruiyu <[email protected]><mailto:[email protected]>; edk2- >> [email protected]<mailto:[email protected]> >> >>> Cc: Tian, Feng <[email protected]><mailto:[email protected]>; Zeng, >> >>> Star >> <[email protected]><mailto:[email protected]>; Kinney, Michael D >> <[email protected]><mailto:[email protected]> >> >>> 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:[email protected]] On Behalf Of >> >>>>> Heyi Guo >> >>>>> Sent: Thursday, March 17, 2016 10:37 PM >> >>>>> To: [email protected]<mailto:[email protected]> >> >>>>> Cc: Heyi Guo <[email protected]><mailto:[email protected]>; Tian, >> >>>>> Feng >> <[email protected]><mailto:[email protected]>; Zeng, Star >> <[email protected]><mailto:[email protected]> >> >>>>> 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 >> >>>>> <[email protected]><mailto:[email protected]> >> >>>>> Cc: Feng Tian <[email protected]><mailto:[email protected]> >> >>>>> Cc: Star Zeng <[email protected]><mailto:[email protected]> >> >>>>> --- >> >>>>> .../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 >> >>>>> [email protected]<mailto:[email protected]> >> >>>>> https://lists.01.org/mailman/listinfo/edk2-devel >> >> _______________________________________________ >> >> edk2-devel mailing list >> >> [email protected] >> >> https://lists.01.org/mailman/listinfo/edk2-devel >> >> _______________________________________________ >> edk2-devel mailing list >> [email protected] >> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
