diff mbox series

[1/1] serial: 8250_dma: Rearm DMA Rx if more data is pending

Message ID 20221107102126.56481-1-ilpo.jarvinen@linux.intel.com
State New
Headers
Series [1/1] serial: 8250_dma: Rearm DMA Rx if more data is pending |

Commit Message

Ilpo Järvinen Nov. 7, 2022, 10:21 a.m. UTC 
  When DMA Rx completes, the current behavior is to just exit the DMA
completion handler without future actions. If the transfer is still
on-going, UART will trigger an interrupt and that eventually rearms the
DMA Rx. The extra interrupt round-trip has an inherent latency cost
that increases the risk of FIFO overrun. In such situations, the
latency margin tends to already be less due to FIFO not being empty.

Add check into DMA Rx completion handler to detect if LSR has DR (Data
Ready) still set. DR indicates there will be more characters pending
and DMA Rx can be rearmed right away to handle them.

Cc: Gilles BULOZ <gilles.buloz@kontron.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250_dma.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

Comments

Andy Shevchenko Nov. 7, 2022, 11:31 a.m. UTC | #1 
On Mon, Nov 07, 2022 at 12:21:26PM +0200, Ilpo Järvinen wrote:
> When DMA Rx completes, the current behavior is to just exit the DMA
> completion handler without future actions. If the transfer is still
> on-going, UART will trigger an interrupt and that eventually rearms the
> DMA Rx. The extra interrupt round-trip has an inherent latency cost
> that increases the risk of FIFO overrun. In such situations, the
> latency margin tends to already be less due to FIFO not being empty.
> 
> Add check into DMA Rx completion handler to detect if LSR has DR (Data
> Ready) still set. DR indicates there will be more characters pending
> and DMA Rx can be rearmed right away to handle them.

Yep, I used to have something like draft of the below change locally.
Thanks for putting it in shape and upstreamimg!

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Cc: Gilles BULOZ <gilles.buloz@kontron.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/tty/serial/8250/8250_dma.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
> index b85c82616e8c..37d6af2ec427 100644
> --- a/drivers/tty/serial/8250/8250_dma.c
> +++ b/drivers/tty/serial/8250/8250_dma.c
> @@ -38,9 +38,8 @@ static void __dma_tx_complete(void *param)
>  	spin_unlock_irqrestore(&p->port.lock, flags);
>  }
>  
> -static void __dma_rx_complete(void *param)
> +static void __dma_rx_complete(struct uart_8250_port *p)
>  {
> -	struct uart_8250_port	*p = param;
>  	struct uart_8250_dma	*dma = p->dma;
>  	struct tty_port		*tty_port = &p->port.state->port;
>  	struct dma_tx_state	state;
> @@ -57,6 +56,20 @@ static void __dma_rx_complete(void *param)
>  	tty_flip_buffer_push(tty_port);
>  }
>  
> +static void dma_rx_complete(void *param)
> +{
> +	struct uart_8250_port *p = param;
> +	struct uart_8250_dma *dma = p->dma;
> +	unsigned long flags;
> +
> +	__dma_rx_complete(p);
> +
> +	spin_lock_irqsave(&p->port.lock, flags);
> +	if (!dma->rx_running && (serial_lsr_in(p) & UART_LSR_DR))
> +		p->dma->rx_dma(p);
> +	spin_unlock_irqrestore(&p->port.lock, flags);
> +}
> +
>  int serial8250_tx_dma(struct uart_8250_port *p)
>  {
>  	struct uart_8250_dma		*dma = p->dma;
> @@ -130,7 +143,7 @@ int serial8250_rx_dma(struct uart_8250_port *p)
>  		return -EBUSY;
>  
>  	dma->rx_running = 1;
> -	desc->callback = __dma_rx_complete;
> +	desc->callback = dma_rx_complete;
>  	desc->callback_param = p;
>  
>  	dma->rx_cookie = dmaengine_submit(desc);
> -- 
> 2.30.2
>
Andy Shevchenko Nov. 8, 2022, 12:57 p.m. UTC | #2 
On Mon, Nov 07, 2022 at 01:31:52PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 07, 2022 at 12:21:26PM +0200, Ilpo Järvinen wrote:

...

> Yep, I used to have something like draft of the below change locally.
> Thanks for putting it in shape and upstreamimg!
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

...

> > -static void __dma_rx_complete(void *param)
> > +static void __dma_rx_complete(struct uart_8250_port *p)
> >  {
> > -	struct uart_8250_port	*p = param;
> >  	struct uart_8250_dma	*dma = p->dma;

Btw, looking into my patch in archives I noticed that dma can also be passed as
a parameter...

...

> > +static void dma_rx_complete(void *param)
> > +{
> > +	struct uart_8250_port *p = param;

> > +	struct uart_8250_dma *dma = p->dma;

...since you have it already here.

> > +	unsigned long flags;
> > +
> > +	__dma_rx_complete(p);
diff mbox series

Patch

diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index b85c82616e8c..37d6af2ec427 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -38,9 +38,8 @@  static void __dma_tx_complete(void *param)
 	spin_unlock_irqrestore(&p->port.lock, flags);
 }
 
-static void __dma_rx_complete(void *param)
+static void __dma_rx_complete(struct uart_8250_port *p)
 {
-	struct uart_8250_port	*p = param;
 	struct uart_8250_dma	*dma = p->dma;
 	struct tty_port		*tty_port = &p->port.state->port;
 	struct dma_tx_state	state;
@@ -57,6 +56,20 @@  static void __dma_rx_complete(void *param)
 	tty_flip_buffer_push(tty_port);
 }
 
+static void dma_rx_complete(void *param)
+{
+	struct uart_8250_port *p = param;
+	struct uart_8250_dma *dma = p->dma;
+	unsigned long flags;
+
+	__dma_rx_complete(p);
+
+	spin_lock_irqsave(&p->port.lock, flags);
+	if (!dma->rx_running && (serial_lsr_in(p) & UART_LSR_DR))
+		p->dma->rx_dma(p);
+	spin_unlock_irqrestore(&p->port.lock, flags);
+}
+
 int serial8250_tx_dma(struct uart_8250_port *p)
 {
 	struct uart_8250_dma		*dma = p->dma;
@@ -130,7 +143,7 @@  int serial8250_rx_dma(struct uart_8250_port *p)
 		return -EBUSY;
 
 	dma->rx_running = 1;
-	desc->callback = __dma_rx_complete;
+	desc->callback = dma_rx_complete;
 	desc->callback_param = p;
 
 	dma->rx_cookie = dmaengine_submit(desc);