> On Apr 28, 2016, at 7:19 AM, Heyi Guo <heyi....@linaro.org> wrote:
>
>
>
> On 04/28/2016 11:17 AM, Andrew Fish wrote:
>>> On Apr 27, 2016, at 8:10 PM, Ni, Ruiyu <ruiyu...@intel.com> wrote:
>>>
>>>> -----Original Message-----
>>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>>>> Heyi Guo
>>>> Sent: Saturday, April 23, 2016 4:54 PM
>>>> To: edk2-devel@lists.01.org
>>>> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Heyi Guo
>>>> <heyi....@linaro.org>; Tian, Feng <feng.t...@intel.com>;
>>>> Zeng, Star <star.z...@intel.com>
>>>> Subject: [edk2] [PATCH] 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.
>>>>
>>>> Polling interval (100ns) =
>>>> FifoDepth * (StartBit + DataBits + StopBits + ParityBits) * 10,000,000
>>>> / BaudRate
>>>> (StopBits is assumed to be 1 and ParityBits is ignored for simplicity.
>>>>
>>>> However, as the event is time sensitive, we need to align the interval
>>>> to timer interrupt period to make sure the event will be triggered at
>>>> the expected rate. E.g. if the interval is 2.7ms and timer interrupt
>>>> period is 1ms, the event will be triggered by time slice sequence as
>>>> below:
>>>> 3ms 3ms 2ms 3ms 3ms 2ms...
>>>>
>>>> In such case we will adjust the polling interval to be 2ms.
>>> Heyi,
>>> I remember we had a discussion about the polling interval calculation
>>> equation a month ago. The equation used in the patch looks good to me.
>>> Because the timer handler is called sometimes every 3ms but not 2.7ms
>>> resulting the key loss?
>>> Just want to confirm with you.
>>>
>> Is it safe to make assumptions based on an implementation of an AP? Other
>> platforms could be using different code?
>>
>> Thanks,
>>
>> Andrew Fish
> Michael has similar concern and we will try using standard UEFI services or
> protocols to achieve the timer interrupt period.
>
I know Kinney mentioned not to use PI APIs, but this sounds like you are making
assumptions about the implementation. A lot of the EFI APIs are specified as
will delay at least X 100 ns, and with no quality of service guarantee. So you
need to code to that assumption, not how a specific timer driver works.
Thanks,
Andrew Fish
> Thanks.
>
> Heyi
>>
>>> Regards,
>>> Ray
>>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>> Signed-off-by: Heyi Guo <heyi....@linaro.org>
>>>> Cc: Feng Tian <feng.t...@intel.com>
>>>> Cc: Star Zeng <star.z...@intel.com>
>>>> Cc: Michael D Kinney <michael.d.kin...@intel.com>
>>>> ---
>>>> .../Universal/Console/TerminalDxe/Terminal.c | 5 +-
>>>> .../Universal/Console/TerminalDxe/Terminal.h | 28 ++++++-
>>>> .../Universal/Console/TerminalDxe/TerminalConIn.c | 92
>>>> ++++++++++++++++++++++
>>>> .../Universal/Console/TerminalDxe/TerminalDxe.inf | 1 +
>>>> 4 files changed, 123 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
>>>> b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
>>>> index 6fde3b2..2944707 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
>>>> + 0, // KeyboardTimerInterval
>>>>
>>>> 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..a1ff595 100644
>>>> --- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
>>>> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
>>>> @@ -28,6 +28,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
>>>> EITHER EXPRESS OR IMPLIED.
>>>> #include <Protocol/DevicePath.h>
>>>> #include <Protocol/SimpleTextIn.h>
>>>> #include <Protocol/SimpleTextInEx.h>
>>>> +#include <Protocol/Timer.h>
>>>>
>>>> #include <Library/DebugLib.h>
>>>> #include <Library/UefiDriverEntryPoint.h>
>>>> @@ -68,8 +69,6 @@ typedef struct {
>>>> UINTN Rows;
>>>> } TERMINAL_CONSOLE_MODE_DATA;
>>>>
>>>> -#define KEYBOARD_TIMER_INTERVAL 200000 // 0.02s
>>>> -
>>>> #define TERMINAL_DEV_SIGNATURE SIGNATURE_32 ('t', 'm', 'n', 'l')
>>>>
>>>> #define TERMINAL_CONSOLE_IN_EX_NOTIFY_SIGNATURE SIGNATURE_32 ('t', 'm',
>>>> 'e', 'n')
>>>> @@ -91,6 +90,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 +1358,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 3be877b..e7788a0 100644
>>>> --- a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
>>>> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
>>>> @@ -15,6 +15,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
>>>> EITHER EXPRESS OR IMPLIED.
>>>>
>>>> #include "Terminal.h"
>>>>
>>>> +EFI_TIMER_ARCH_PROTOCOL *gTimer;
>>>>
>>>> /**
>>>> Reads the next keystroke from the input device. The WaitForKey Event can
>>>> @@ -502,6 +503,94 @@ 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;
>>>> + UINT64 BaudRate;
>>>> + UINT64 Interval;
>>>> + UINT64 TimerPeriod;
>>>> + EFI_STATUS Status;
>>>> +
>>>> + // Make some assumption if the values are not suitable for calculating.
>>>> + BaudRate = Mode->BaudRate;
>>>> + if (BaudRate == 0) {
>>>> + BaudRate = 115200;
>>>> + }
>>>> + FifoDepth = Mode->ReceiveFifoDepth;
>>>> + if (FifoDepth == 0) {
>>>> + FifoDepth = 1;
>>>> + }
>>>> +
>>>> + // We assume stop bits to be 1 and ignore parity bit to make it simple
>>>> + // and fast enough to poll.
>>>> + Interval = DivU64x64Remainder (
>>>> + FifoDepth * (1 + Mode->DataBits + 1) * 10000000,
>>>> + Mode->BaudRate,
>>>> + NULL
>>>> + );
>>>> +
>>>> + // As this is a time sensitive event, we still need to align the
>>>> + // interval to timer interrupt period.
>>>> + if (gTimer == NULL) {
>>>> + Status = gBS->LocateProtocol (
>>>> + &gEfiTimerArchProtocolGuid,
>>>> + EFI_NATIVE_INTERFACE,
>>>> + (VOID **) &gTimer
>>>> + );
>>>> + ASSERT_EFI_ERROR (Status);
>>>> + }
>>>> +
>>>> + Status = gTimer->GetTimerPeriod (gTimer, &TimerPeriod);
>>>> + ASSERT_EFI_ERROR (Status);
>>>> +
>>>> + if (Interval <= TimerPeriod) {
>>>> + return TimerPeriod;
>>>> + }
>>>> + return MultU64x64 (DivU64x64Remainder (Interval, TimerPeriod, NULL),
>>>> TimerPeriod);
>>>> +}
>>>> +
>>>> +
>>>> +/**
>>>> + 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 +649,9 @@ TerminalConInTimerHandler (
>>>> TerminalDevice->SerialInTimeOut = SerialInTimeOut;
>>>> }
>>>> }
>>>> +
>>>> + UpdatePollingRate (TerminalDevice);
>>>> +
>>>> //
>>>> // Check whether serial buffer is empty.
>>>> // Skip the key transfer loop only if the SerialIo protocol instance
>>>> diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf
>>>> b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf
>>>> index 0780296..dfd5035 100644
>>>> --- a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf
>>>> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf
>>>> @@ -84,6 +84,7 @@
>>>> gEfiSimpleTextInProtocolGuid ## BY_START
>>>> gEfiSimpleTextInputExProtocolGuid ## BY_START
>>>> gEfiSimpleTextOutProtocolGuid ## BY_START
>>>> + gEfiTimerArchProtocolGuid ## CONSUMES
>>>>
>>>> [Pcd]
>>>> gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType ##
>>>> SOMETIMES_CONSUMES
>>>> --
>>>> 2.7.0
>>>>
>>>> _______________________________________________
>>>> 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
>
> _______________________________________________
> 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
