On 01/18/16 12:41, Ryan Harkin wrote: > On 18 January 2016 at 11:33, Laszlo Ersek <ler...@redhat.com> wrote: >> On 01/18/16 11:24, Zeng, Star wrote: >>> [...] >>> >>>>> >>>>> The above analysis is very clear, thanks. I am a little concern about if >>>>> the code changes below follow the comments in the code. >>>>> >>>>> In Terminal.c: >>>>> // >>>>> // Set the timeout value of serial buffer for >>>>> // keystroke response performance issue >>>>> // >>>>> In TerminalConIn.c: >>>>> // >>>>> // if current timeout value for serial device is not identical with >>>>> // the value saved in TERMINAL_DEV structure, then recalculate the >>>>> // timeout value again and set serial attribute according to this >>>>> value. >>>>> // >>> >>> Any comments about above concern? >> >> Not really. >> >> I don't know what the purpose of the Timeout calculation is (what is the >> "keystroke response performance issue"?), but in any case, it is a good >> sign that TerminalDxe sets *some* Timeout explicitly. >> >> Plus, it has worked reliably until now, so we shouldn't change the >> Timeout argument. >> > > I think "reliably" is a bit generous ;-) The most common complaint I > get about EDK2 is that it can't handle more than a FIFO's worth of > pasted characters on the serial port.
I tend to think this comes, at least partly, from the fact that UEFI doesn't do interrupt-driven drivers. There's timer based polling (in drivers), and there are busy loops (in applications, I guess). I think you'll see the same behavior with network packet reception. I said "reliably" beause in my environment I've had practically zero issues with the serial terminal (beyond the one, very lacking, textual resolution list -- but I patched that for myself permanently). Thanks Laszlo > >>> >>>>> >>>>> And I also checked the >>>>> IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/Serial.c although ARM >>>>> platforms do not use it, the SERIAL_PORT_DEFAULT_RECEIVE_FIFO_DEPTH >>>>> value is 1. if SetAttributes() is called with ReceiveFifoDepth = 0, then >>>>> SERIAL_PORT_DEFAULT_RECEIVE_FIFO_DEPTH is selected as the description of >>>>> the functions is "A ReceiveFifoDepth value of 0 will use the device's >>>>> default FIFO depth", so what's the device's default FIFO depth? >>>> >>>> Well, that's exactly what SerialDxe delegates to SerialPortLib :) >>>> SerialDxe need not be aware of the device-specific value; the platform >>>> will provide a device-specific library instance. >>>> >>>>> I have a thought that updates SerialDxe to call SerialSetAttributes(with >>>>> 0, 0, 0, DefaultParity, 0 and DefaultStopBits) in SerialDxeInitialize() >>>>> and SerialReset() to get the real default values of the device, then the >>>>> driver can also eliminate the use of PcdUartDefault####. >>>>> >>>>> What's your opinion? >>>> >>>> I don't think this is a good idea -- the UEFI spec is very clear on >>>> this. Under "11.8 Serial I/O Protocol", in the general description part, >>>> you find: >>>> >>>> The default attributes for all UART-style serial device interfaces >>>> are: 115,200 baud, a 1 byte receive FIFO, a 1,000,000 microsecond >>>> timeout per character, no parity, 8 data bits, and 1 stop bit. [...] >>>> >>>> So this is what must be in effect right after initialization, and -- >>>> with all likelihood -- after re-set as well. >>>> >>>> If a Serial IO protocol client is not content with these settings, then >>>> it must explicitly change them, with the SetAttributes() call. >>> >>> So TerminalDxe could know it is not content with the settings? >> >> I think so, yes. TerminalDxe knows in advance that it will handle escape >> sequences (= bursts of scan codes). The longest such sequence (3 or 4 >> scan codes I guess?) should fit in the receive FIFO. >> >> So TerminalDxe could try to figure out the longest sequence it wishes to >> handle, then configure such a receive FIFO depth with SerialIO. >> >> Or else, what I think would be better, just call SetAttributes() from >> the platform's SerialPortLib instance, with ReceiveFifoDepth=0: >> - in practice, this would restore the previous behavior, >> - I expect that this value should enable the FIFO on most UARTs, with a >> sufficient depth for the escape sequence processing >> >>> Should platform BDS code to explicitly change them with the >>> SetAttributes() call? >> >> Well, PlatformBDS is the ultimate fallback, always :), but I think >> TerminalDxe has much more business dealing with the Serial IO >> attributes. TerminalDxe already modifies the Timeout attribute, and in >> the end, the FIFO depth too is important to TerminalDxe (for escape seq >> processing). I think it's not a BDS responsibility. >> >> Consider for example >> "MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxe.inf". This is a >> UEFI driver (unlike SerialDxe, which is a DXE_DRIVER). >> >> If PlatformBDS wanted to control the receive FIFO depth of the SerialIo >> protocol instance procuded by PciSioSerialDxe, then it would have to >> connect PciIo instances (so SerialIo would exist), then set the >> attribute, then connect SerialIo recursively (so that TerminalDxe would >> sit on top). In other words, PlatformBds would have to intrude to the >> UEFI driver model -- a full bottom-up recursive ConnectAll would no >> longer work. >> >> Also, the user could disconnect/reconnect the SerialIo protocol provider >> in the UEFI shell; maybe even unload/reload the driver. So I think >> setting the attributes is the responsibility of the driver that sets >> directly atop. >> >> Thanks >> Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
