Improving SPI driver latency (vs v3.8.13.14-rt31)
Subsystems (3)
cover Improving SPI driver latency (vs v3.8.13.14-rt31)
I recently became interested in realtime access to an SPI device on the Odroid U3 platform, with a goal of running a repetitive task every 1ms that performs two SPI transactions totalling around 200 bytes. (for
http://linuxcnc.org/ interfacing to a http://mesanet.com/ "Anything I/O" card)
Unfortunately, I found that there were frequent large latencies, some over 10ms, when using /dev/spidev. This seems to be typical of others' experience using it (for instance, one can find threads discussing disappointing RT performance of SPI on the beaglebone and pandaboard; at least one Raspberry PI project chose to implement a pure userspace SPI driver instead of using spidev)
At all levels of the SPI stack, I found things that could be improved if lowest delays are the goal. I doubt that in their current form these changes are suitable to be incorporated in linux, but I hope that this might spur some discussion that would ultimately lead to better realtime performance of SPI particularly in the preempt-rt kernel.
As you may know, the kernel's spi driver stack consists of
spidev - implementation of the /dev/spidev* device
spi - hardware-independent kernel layer
spi-xyzzy - hardware-dependent driver for xyzzy hardware
(s3c64xx in my device)
I fixed causes of latency at each layer
* First, I eliminated per-ioctl memory allocations in spidev
* Second, I made __spi_sync *actually* synchronous, rather than being a wrapper over spi_async + wait_for_completion; and changed spidev to use spi_sync. This is probably the most controversial aspect of my patches.
* Third, in the hardware-dependent code I moved DMA acquisition to device initialization time rather than transaction time
With these changes, I have been able to run at a 500us repetitive rate, performing two transactions per repetition, with no transaction over 250us, for over 24 hours. This is in contrast to the original performance, in which transactions taking over 10ms were seen multiple times per hour. (24 hours is about 340 million spi transactions) This means there's plenty of margin for the other activities LinuxCNC needs to perform while still running at a 1ms repetitive rate.
I know that 3.8 is by no means current, but 3.8.y is the default kernel shipped by hardkernel for their U3 devices so it was a rational version choice for me. I did skim spi changes from version 3.8 to the present and didn't see anything that looked like it was directed at improving SPI latency, though the underlying code has probably changed enough over time that I assume my patches wouldn't actually apply at the tip of the latest and greatest branches.
Jeff Epler (4):
spi: reenable sync SPI transfers
spidev: Avoid runtime memory allocations
spidev: actually use synchronous transfers
spidev-s3c64xx: allocate dma channel at startup
drivers/spi/spi-s3c64xx.c | 15 +++++++-------- drivers/spi/spi.c | 22 ++++++++++++++++------ drivers/spi/spidev.c | 43 ++++++++++++++++++++++++++++++------------- 3 files changed, 53 insertions(+), 27 deletions(-)
Signature · 6 lines
2.1.0 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
RFC 1/4 spi: reenable sync SPI transfers
In reply to a message not in the archiveone source of excess latency in hm2_spi / hal_spidev is latency due to async transfers. Make the __spi_sync primitive actually synchronous, rather than building on an asynchronous primitive.
--- drivers/spi/spi.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)
drivers/spi/spi.c +16 -6
@@ -664,10 +664,18 @@ void spi_finalize_current_message(struct spi_master *master)struct spi_message *mesg;unsigned long flags; +if (!master->cur_msg) +return ; +spin_lock_irqsave (&master->queue_lock, flags); mesg = master->cur_msg; master->cur_msg =NULL ; +if (!mesg) { +spin_unlock_irqrestore (&master->queue_lock, flags); +return ; + } +queue_kthread_work (&master->kworker, &master->pump_messages);spin_unlock_irqrestore (&master->queue_lock, flags);
@@ -1490,24 +1498,26 @@ static int __spi_sync(struct spi_device *spi, struct spi_message *message, int bus_locked) {DECLARE_COMPLETION_ONSTACK (done); -int status; +int status =0 ;struct spi_master *master = spi->master; message->complete = spi_complete; message->context = &done; + message->spi = spi;if (!bus_locked)mutex_lock (&master->bus_lock_mutex); - status =spi_async_locked (spi, message); +if (master->prepare_transfer_hardware) + status = master->prepare_transfer_hardware (master); +if (status >=0 ) + status = master->transfer_one_message (master, message); +if (status >=0 && master->unprepare_transfer_hardware) + status = master->unprepare_transfer_hardware (master);if (!bus_locked)mutex_unlock (&master->bus_lock_mutex); -if (status ==0 ) { -wait_for_completion (&done); - status = message->status; - } message->context =NULL ;return status; }
Signature · 6 lines
2.1.0 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
RFC 2/4 spidev: Avoid runtime memory allocations
In reply to a message not in the archiveallocating bounce and k_xfers at every iteration was a source of excess delays.
--- drivers/spi/spidev.c | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-)
drivers/spi/spidev.c +29 -6
@@ -85,6 +85,8 @@ struct spidev_data {struct mutex buf_lock;unsigned users;u8 *buffer; +struct spi_ioc_transfer *bounce; +struct spi_transfer *k_xfers; };static LIST_HEAD (device_list);
@@ -227,9 +229,9 @@ static int spidev_message(struct spidev_data *spidev,int status = -EFAULT ;spi_message_init (&msg); - k_xfers =kcalloc (n_xfers,sizeof (*k_tmp),GFP_KERNEL ); -if (k_xfers ==NULL ) -return -ENOMEM ; +if (n_xfers *sizeof (*k_tmp) > bufsiz) +return -ENOMEM ; + k_xfers = spidev->k_xfers;/* Construct spi_message, copying any tx data to bounce buffer. * We walk the array of user-provided transfers, using each one
@@ -302,7 +304,6 @@ static int spidev_message(struct spidev_data *spidev, status = total; done: -kfree (k_xfers);return status; }
@@ -451,8 +452,12 @@ spidev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)if (n_ioc ==0 )break ; +if (tmp > bufsiz) { + retval = -EINVAL ; +break ; + }/* copy into scratch area */ - ioc =kmalloc (tmp,GFP_KERNEL ); + ioc = spidev->bounce;if (!ioc) { retval = -ENOMEM ;break ;
@@ -465,7 +470,6 @@ spidev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)/* translate to spi_message, execute */ retval =spidev_message (spidev, ioc, n_ioc); -kfree (ioc);break ; }
@@ -504,6 +508,19 @@ static int spidev_open(struct inode *inode, struct file *filp)dev_dbg (&spidev->spi->dev,"open/ENOMEM\n" ); status = -ENOMEM ; } + spidev->bounce =kmalloc (bufsiz,GFP_KERNEL ); +if (!spidev->bounce) { +dev_dbg (&spidev->spi->dev,"open/ENOMEM\n" ); +kfree (spidev->buffer); + status = -ENOMEM ; + } + spidev->k_xfers =kmalloc (bufsiz,GFP_KERNEL ); +if (!spidev->k_xfers) { +dev_dbg (&spidev->spi->dev,"open/ENOMEM\n" ); +kfree (spidev->buffer); +kfree (spidev->bounce); + status = -ENOMEM ; + } }if (status ==0 ) { spidev->users++;
@@ -534,6 +551,12 @@ static int spidev_release(struct inode *inode, struct file *filp)kfree (spidev->buffer); spidev->buffer =NULL ; +kfree (spidev->bounce); + spidev->bounce =NULL ; + +kfree (spidev->k_xfers); + spidev->k_xfers =NULL ; +/* ... after we unbound from the underlying device? */ spin_lock_irq (&spidev->spi_lock); dofree = (spidev->spi ==NULL );
Signature · 6 lines
2.1.0 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
RFC 3/4 spidev: actually use synchronous transfers
In reply to a message not in the archivethis reduces a source of latency
--- drivers/spi/spidev.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
drivers/spi/spidev.c +1 -7
@@ -120,15 +120,9 @@ spidev_sync(struct spidev_data *spidev, struct spi_message *message)if (spidev->spi ==NULL ) status = -ESHUTDOWN ;else - status =spi_async (spidev->spi, message); + status =spi_sync (spidev->spi, message);spin_unlock_irq (&spidev->spi_lock); -if (status ==0 ) { -wait_for_completion (&done); - status = message->status; -if (status ==0 ) - status = message->actual_length; - }return status; }
Signature · 6 lines
2.1.0 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
RFC 4/4 spidev-s3c64xx: allocate dma channel at startup
In reply to a message not in the archiveit appears that acquire_dma introduces intolerable delays; the system is fine with the dma channnels allocated all the time.
--- drivers/spi/spi-s3c64xx.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)
drivers/spi/spi-s3c64xx.c +7 -8
@@ -797,10 +797,6 @@ static int s3c64xx_spi_prepare_transfer(struct spi_master *spi) {struct s3c64xx_spi_driver_data *sdd =spi_master_get_devdata (spi); -/* Acquire DMA channels */ -while (!acquire_dma (sdd)) -msleep (10 ); -pm_runtime_get_sync (&sdd->pdev->dev);return 0 ;
@@ -810,10 +806,6 @@ static int s3c64xx_spi_unprepare_transfer(struct spi_master *spi) {struct s3c64xx_spi_driver_data *sdd =spi_master_get_devdata (spi); -/* Free DMA channels */ - sdd->ops->release (sdd->rx_dma.ch, &s3c64xx_spi_dma_client); - sdd->ops->release (sdd->tx_dma.ch, &s3c64xx_spi_dma_client); -pm_runtime_put (&sdd->pdev->dev);return 0 ;
@@ -1344,6 +1336,9 @@ static int __init s3c64xx_spi_probe(struct platform_device *pdev) goto err7; } +// cannot fail(?) +acquire_dma (sdd); +writel (S3C64XX_SPI_INT_RX_OVERRUN_EN |S3C64XX_SPI_INT_RX_UNDERRUN_EN |S3C64XX_SPI_INT_TX_OVERRUN_EN |S3C64XX_SPI_INT_TX_UNDERRUN_EN , sdd->regs +S3C64XX_SPI_INT_EN );
@@ -1396,6 +1391,10 @@ static int s3c64xx_spi_remove(struct platform_device *pdev)spi_unregister_master (master); +/* Free DMA channels */ + sdd->ops->release (sdd->rx_dma.ch, &s3c64xx_spi_dma_client); + sdd->ops->release (sdd->tx_dma.ch, &s3c64xx_spi_dma_client); +writel (0 , sdd->regs +S3C64XX_SPI_INT_EN );free_irq (platform_get_irq (pdev,0 ), sdd);
Signature · 6 lines
2.1.0 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 1/4] spi: reenable sync SPI transfers
In reply to a message not in the archiveQuoted 4 lines · Jeff Epler
one source of excess latency in
hm2_spi/hal_spidevis latency due to async transfers. Make the__spi_syncprimitive actually synchronous, rather than building on an asynchronous primitive.
---
Please submit patches using the process that is documented in Documentation/SubmittingPatches, in particular it is *essential* that you sign off your patches and you need CC maintainers otherwise it's likely your patch will be missed.
drivers/spi/spi.c void spi_finalize_current_message(struct spi_master *master) · line 669
Quoted 3 lines
drivers/spi/spi.c +3
-0+if (!master->cur_msg) +return ; +
Please also follow the kernel coding style.
Quoted 6 lines
drivers/spi/spi.c +4
-1- status =spi_async_locked (spi, message); +if (master->prepare_transfer_hardware) + status = master->prepare_transfer_hardware (master); +if (status >=0 ) + status = master->transfer_one_message (master, message);
drivers/spi/spi.c static int __spi_sync(struct spi_device *spi, struct spi_message *message, · line 1510
Quoted 10 lines
drivers/spi/spi.c +2
-4+if (status >=0 && master->unprepare_transfer_hardware) + status = master->unprepare_transfer_hardware (master);if (!bus_locked)mutex_unlock (&master->bus_lock_mutex); -if (status ==0 ) { -wait_for_completion (&done); - status = message->status; - }
There's many problems with this - the most critical are that it is broken for multithreaded use, it's not even trying to do locking so both other callers and the thread will break, and it's not waiting for the transfer to complete.
It would be good to do as much as we can inline but doing so needs much more work than this and needs to consider other aspects of performance - your patch will also make performance worse in many situations. My talk at ELC this year covers a lot of this, the slides should be googleable.