Re: [PATCH v1 19/19] hid-ft260: uart: improve write performance

[Michael Zaidman <michael.zaidman@gmail.com>]

On Sat, Feb 10, 2024 at 11:51:47PM +0200, Michael Zaidman wrote:
> Tx performance with the current buffer size of 256 bytes is lower when
> the data length exceeds the xmit buf size.
> 
> [134331.147978] ft260_uart_write: count: 288, len: 256
> [134331.157945] ft260_uart_write: count: 32, len: 32
> [134331.159977] ft260_uart_write: count: 288, len: 256
> [134331.169990] ft260_uart_write: count: 32, len: 32
> 
> 1. Increase the xmit buffer size to page size as used in the serial core
> and other tty drivers.
> 
> 2. Remove the xmit buffer fulness against the watermark checking and the
> tty_wakeup calling in the ft260_uart_transmit_chars routine. This code is
> taken from other drivers, but other drivers may call the routine from the
> interrupt context. In our case, this condition is always True since xmit
> buffer filling and emptying are serialized and done synchronously.
> 
> Tested with picocom ASCII file transfer by 288-byte chunks at 921600
> bauds rate with above 20% performance improvement.
> 
> Before:
> 2821.7 Kbytes transferred at 47367 CPS... Done.
> 
> After:
> 2821.7 Kbytes transferred at 57788 CPS... Done.
> 

Besides the performance improvement, it fixes the bug of outputting
characters to the local terminal when local echo is disabled and
printing every character twice with local echo enabled.

> Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com>
> ---
>  drivers/hid/hid-ft260.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index 6b172bfa4f98..1188b8e09938 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c
> @@ -308,8 +308,7 @@ enum {
>  #define FT260_UART_EN_PW_SAVE_BAUD	(4800)
>  
>  #define UART_COUNT_MAX (4) /* Number of supported UARTs */
> -#define XMIT_FIFO_SIZE (256)
> -#define TTY_WAKEUP_WATERMARK (XMIT_FIFO_SIZE / 2)
> +#define XMIT_FIFO_SIZE (PAGE_SIZE)
>  
>  static const struct hid_device_id ft260_devices[] = {
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_FUTURE_TECHNOLOGY,
> @@ -1211,7 +1210,7 @@ static int ft260_uart_transmit_chars(struct ft260_device *port)
>  
>  		len = kfifo_out_spinlocked(xmit, rep->data, len, &port->xmit_fifo_lock);
>  
> -		ret = ft260_hid_output_report(hdev, (u8 *)rep, len + sizeof(*rep));
> +		ret = ft260_hid_output_report(hdev, (u8 *)rep, len + 2);
>  		if (ret < 0)
>  			goto tty_out;
>  
> @@ -1219,10 +1218,6 @@ static int ft260_uart_transmit_chars(struct ft260_device *port)
>  		port->icount.tx += len;
>  	} while (data_len > 0);
>  
> -	len = kfifo_len(xmit);
> -	if ((XMIT_FIFO_SIZE - len) > TTY_WAKEUP_WATERMARK)
> -		tty_wakeup(tty);
> -
>  	ret = 0;
>  
>  tty_out: