[PATCH v4 0/4] Chunk splitting of spi transfers

[PATCH v4 0/4] Chunk splitting of spi transfers

[Noralf Trønnes]

spi-bcm2835 has a ~64kB upper limit on DMA transfers. Drivers in
drivers/gpu/drm/tinydrm work around this limitation by splitting the
buffer into multiple transfers. This patchset lifts this limitation by
splitting affected transfers in the SPI core using
spi_split_transfers_maxsize().

This work[1] was begun by Meghana Madhyastha.

Main changes in this version:
- Remove warning in spi_split_transfers_maxsize()
- Split SPI patch into core patch and driver patch

Noralf.

[1] https://patchwork.freedesktop.org/series/38913/

Meghana Madhyastha (3):
  spi: Split spi message into max_dma_len size chunks
  spi/spi-bcm2835: Remove DMA transfer size cap
  drm/tinydrm: Remove chunk splitting in tinydrm_spi_transfer

Noralf Trønnes (1):
  spi: Remove warning in spi_split_transfers_maxsize()

 .../gpu/drm/tinydrm/core/tinydrm-helpers.c    | 83 ++-----------------
 drivers/gpu/drm/tinydrm/mipi-dbi.c            | 10 +--
 drivers/spi/spi-bcm2835.c                     | 15 +---
 drivers/spi/spi.c                             | 10 +--
 4 files changed, 13 insertions(+), 105 deletions(-)

-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4 1/4] spi: Remove warning in spi_split_transfers_maxsize()

[Noralf Trønnes]

Don't warn about splitting transfers, the info is available in the
statistics if needed.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/spi/spi.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 9a7def7c3237..05875e63be43 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2692,11 +2692,6 @@ static int __spi_split_transfer_maxsize(struct spi_controller *ctlr,
 	size_t offset;
 	size_t count, i;
 
-	/* warn once about this fact that we are splitting a transfer */
-	dev_warn_once(&msg->spi->dev,
-		      "spi_transfer of length %i exceed max length of %zu - needed to split transfers\n",
-		      xfer->len, maxsize);
-
 	/* calculate how many we have to replace */
 	count = DIV_ROUND_UP(xfer->len, maxsize);
 
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4 2/4] spi: Split spi message into max_dma_len size chunks

[Noralf Trønnes]

From: Meghana Madhyastha <meghana.madhyastha@gmail.com>

Some drivers like spi_bcm2835 have a max size on DMA transfers. Work
around this by splitting up the transfer if necessary.

->max_transfer_size is MAX_INT if the driver doesn't set it, so this change
will only affect drivers that set the value.

Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/spi/spi.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 05875e63be43..22bc658032b3 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1299,6 +1299,11 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread)
 
 	trace_spi_message_start(ctlr->cur_msg);
 
+	ret = spi_split_transfers_maxsize(ctlr, ctlr->cur_msg, ctlr->max_dma_len,
+					  GFP_KERNEL | GFP_DMA);
+	if (ret)
+		goto out;
+
 	if (ctlr->prepare_message) {
 		ret = ctlr->prepare_message(ctlr, ctlr->cur_msg);
 		if (ret) {
-- 
2.20.1


_______________________________________________
linux-rpi-kernel mailing list
linux-rpi-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rpi-kernel

[PATCH v4 3/4] spi/spi-bcm2835: Remove DMA transfer size cap

[Noralf Trønnes]

From: Meghana Madhyastha <meghana.madhyastha@gmail.com>

The spi core splits up transfers larger than ->max_dma_len now so we can
remove the upper bound on DMA transfers. Limit max_dma_len to 65532,
because the scatter gather segment is required to be a multiple of 4.

Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/spi/spi-bcm2835.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index 35aebdfd3b4e..caf33da01ac1 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -630,19 +630,6 @@ static bool bcm2835_spi_can_dma(struct spi_master *master,
 	if (tfr->len < BCM2835_SPI_DMA_MIN_LENGTH)
 		return false;
 
-	/* BCM2835_SPI_DLEN has defined a max transfer size as
-	 * 16 bit, so max is 65535
-	 * we can revisit this by using an alternative transfer
-	 * method - ideally this would get done without any more
-	 * interaction...
-	 */
-	if (tfr->len > 65535) {
-		dev_warn_once(&spi->dev,
-			      "transfer size of %d too big for dma-transfer\n",
-			      tfr->len);
-		return false;
-	}
-
 	/* return OK */
 	return true;
 }
@@ -707,7 +694,7 @@ static void bcm2835_dma_init(struct spi_master *master, struct device *dev)
 
 	/* all went well, so set can_dma */
 	master->can_dma = bcm2835_spi_can_dma;
-	master->max_dma_len = 65535; /* limitation by BCM2835_SPI_DLEN */
+	master->max_dma_len = 65532; /* limitation by BCM2835_SPI_DLEN */
 	/* need to do TX AND RX DMA, so we need dummy buffers */
 	master->flags = SPI_MASTER_MUST_RX | SPI_MASTER_MUST_TX;
 
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4 4/4] drm/tinydrm: Remove chunk splitting in tinydrm_spi_transfer

[Noralf Trønnes]

From: Meghana Madhyastha <meghana.madhyastha@gmail.com>

Remove chunk splitting in tinydrm_spi_transfer in tinydrm-helpers as the
spi core will split a buffer into max_dma_len chunks for the spi
controller driver to handle, automatic byte swapping in
tinydrm_spi_transfer as it doesn't have users.

Remove the spi_max module argument that now has lost its cause.

The 16kB buffer size for Type C Option 1 (9-bit) interface is somewhat
arbitrary, but a bigger buffer will have a miniscule impact on transfer
speed, so it's probably fine.

Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 .../gpu/drm/tinydrm/core/tinydrm-helpers.c    | 83 ++-----------------
 drivers/gpu/drm/tinydrm/mipi-dbi.c            | 10 +--
 2 files changed, 7 insertions(+), 86 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
index 6d540d93758f..a32dc859a9c1 100644
--- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
@@ -22,41 +22,8 @@
 #include <drm/drm_rect.h>
 #include <drm/tinydrm/tinydrm-helpers.h>
 
-static unsigned int spi_max;
-module_param(spi_max, uint, 0400);
-MODULE_PARM_DESC(spi_max, "Set a lower SPI max transfer size");
-
 #if IS_ENABLED(CONFIG_SPI)
 
-/**
- * tinydrm_spi_max_transfer_size - Determine max SPI transfer size
- * @spi: SPI device
- * @max_len: Maximum buffer size needed (optional)
- *
- * This function returns the maximum size to use for SPI transfers. It checks
- * the SPI master, the optional @max_len and the module parameter spi_max and
- * returns the smallest.
- *
- * Returns:
- * Maximum size for SPI transfers
- */
-size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t max_len)
-{
-	size_t ret;
-
-	ret = min(spi_max_transfer_size(spi), spi->master->max_dma_len);
-	if (max_len)
-		ret = min(ret, max_len);
-	if (spi_max)
-		ret = min_t(size_t, ret, spi_max);
-	ret &= ~0x3;
-	if (ret < 4)
-		ret = 4;
-
-	return ret;
-}
-EXPORT_SYMBOL(tinydrm_spi_max_transfer_size);
-
 /**
  * tinydrm_spi_bpw_supported - Check if bits per word is supported
  * @spi: SPI device
@@ -147,62 +114,22 @@ int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz,
 	struct spi_transfer tr = {
 		.bits_per_word = bpw,
 		.speed_hz = speed_hz,
+		.tx_buf = buf,
+		.len = len
 	};
 	struct spi_message m;
-	u16 *swap_buf = NULL;
-	size_t max_chunk;
-	size_t chunk;
-	int ret = 0;
-
-	if (WARN_ON_ONCE(bpw != 8 && bpw != 16))
-		return -EINVAL;
-
-	max_chunk = tinydrm_spi_max_transfer_size(spi, 0);
 
 	if (drm_debug & DRM_UT_DRIVER)
-		pr_debug("[drm:%s] bpw=%u, max_chunk=%zu, transfers:\n",
-			 __func__, bpw, max_chunk);
-
-	if (bpw == 16 && !tinydrm_spi_bpw_supported(spi, 16)) {
-		tr.bits_per_word = 8;
-		if (tinydrm_machine_little_endian()) {
-			swap_buf = kmalloc(min(len, max_chunk), GFP_KERNEL);
-			if (!swap_buf)
-				return -ENOMEM;
-		}
-	}
+		pr_debug("[drm:%s] bpw=%u, transfers:\n", __func__, bpw);
 
 	spi_message_init(&m);
 	if (header)
 		spi_message_add_tail(header, &m);
 	spi_message_add_tail(&tr, &m);
 
-	while (len) {
-		chunk = min(len, max_chunk);
+	tinydrm_dbg_spi_message(spi, &m);
 
-		tr.tx_buf = buf;
-		tr.len = chunk;
-
-		if (swap_buf) {
-			const u16 *buf16 = buf;
-			unsigned int i;
-
-			for (i = 0; i < chunk / 2; i++)
-				swap_buf[i] = swab16(buf16[i]);
-
-			tr.tx_buf = swap_buf;
-		}
-
-		buf += chunk;
-		len -= chunk;
-
-		tinydrm_dbg_spi_message(spi, &m);
-		ret = spi_sync(spi, &m);
-		if (ret)
-			return ret;
-	}
-
-	return 0;
+	return spi_sync(spi, &m);
 }
 EXPORT_SYMBOL(tinydrm_spi_transfer);
 
diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
index 85761b4abb83..0bcf6c764893 100644
--- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
+++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
@@ -975,15 +975,9 @@ static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 *cmd,
 int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi,
 		      struct gpio_desc *dc)
 {
-	size_t tx_size = tinydrm_spi_max_transfer_size(spi, 0);
 	struct device *dev = &spi->dev;
 	int ret;
 
-	if (tx_size < 16) {
-		DRM_ERROR("SPI transmit buffer too small: %zu\n", tx_size);
-		return -EINVAL;
-	}
-
 	/*
 	 * Even though it's not the SPI device that does DMA (the master does),
 	 * the dma mask is necessary for the dma_alloc_wc() in
@@ -1013,8 +1007,8 @@ int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi,
 			mipi->swap_bytes = true;
 	} else {
 		mipi->command = mipi_dbi_typec1_command;
-		mipi->tx_buf9_len = tx_size;
-		mipi->tx_buf9 = devm_kmalloc(dev, tx_size, GFP_KERNEL);
+		mipi->tx_buf9_len = SZ_16K;
+		mipi->tx_buf9 = devm_kmalloc(dev, mipi->tx_buf9_len, GFP_KERNEL);
 		if (!mipi->tx_buf9)
 			return -ENOMEM;
 	}
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/4] spi: Split spi message into max_dma_len size chunks

[Lukas Wunner]

On Thu, Apr 11, 2019 at 06:42:33PM +0200, Noralf Trønnes wrote:
> @@ -1299,6 +1299,11 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread)
>  
>  	trace_spi_message_start(ctlr->cur_msg);
>  
> +	ret = spi_split_transfers_maxsize(ctlr, ctlr->cur_msg, ctlr->max_dma_len,
> +					  GFP_KERNEL | GFP_DMA);

Um, shouldn't this be ifdef'ed to CONFIG_HAS_DMA?


> Some drivers like spi_bcm2835 have a max size on DMA transfers. Work
> around this by splitting up the transfer if necessary.

This feels like a very expensive solution to the problem:  Large transfers
are split into multiple smaller transfers (requiring lots of overhead to
allocate and populate the structures) and the split transfers seem to be
cached for later reuse.

I'm wondering, for a frame buffer, doesn't the buffer (and hence the SPI
message) change on every transmission?  In other words, does a frame
buffer benefit from the caching?  If not, then the caching seems to incur
unnecessary overhead.

Even the normal case when no transfers exceed the limit is affected by
additional overhead, namely an iteration over the transfer list to
check each transfers' length. :-(

Note that spi_map_buf() already splits every transfer's sglist into
segments that are smaller than ctlr->max_dma_len.  Now all that needs
to be done is to amend spi-bcm2835.c to iterate over the sglist
and transmit it in portions which do not exceed 65535.  Addressing the
problem at this lower level would drastically reduce the overhead
compared to the approach in the present patch and hence appears to be
more recommendable.

Thanks,

Lukas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/4] spi: Split spi message into max_dma_len size chunks

[Noralf Trønnes]

Den 11.04.2019 18.42, skrev Noralf Trønnes:
> From: Meghana Madhyastha <meghana.madhyastha@gmail.com>
> 
> Some drivers like spi_bcm2835 have a max size on DMA transfers. Work
> around this by splitting up the transfer if necessary.
> 
> ->max_transfer_size is MAX_INT if the driver doesn't set it, so this change
> will only affect drivers that set the value.
> 
> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  drivers/spi/spi.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 05875e63be43..22bc658032b3 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1299,6 +1299,11 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread)
>  
>  	trace_spi_message_start(ctlr->cur_msg);
>  
> +	ret = spi_split_transfers_maxsize(ctlr, ctlr->cur_msg, ctlr->max_dma_len,
> +					  GFP_KERNEL | GFP_DMA);
> +	if (ret)
> +		goto out;
> +
>  	if (ctlr->prepare_message) {
>  		ret = ctlr->prepare_message(ctlr, ctlr->cur_msg);
>  		if (ret) {
> 

This doesn't work.

I started to wonder why it was necessary to lift the upper bound in
spi-bcm2835, so I put the test back and it triggered. Adding some
printk's show that the split transfer are mapped correctly, but when
unmap happens there's only the one big transfer:

[   70.935524] __spi_map_msg: msg=ae711dd4
[   70.935572]     xfer=95c5acd8, xfer->len=65532
[   70.955800]     xfer=aee3f198, xfer->len=65532
[   70.956034]     xfer=7fc3464e, xfer->len=22536
[   70.982802] __spi_unmap_msg: msg=ae711dd4
[   70.982889]     xfer=d5b5dbbf, xfer->len=153600

I need to study this more tomorrow to find out why this is, unless
someone knows off hand what the problem is.

Noralf.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/4] spi: Split spi message into max_dma_len size chunks

[Noralf Trønnes]

Den 11.04.2019 20.18, skrev Lukas Wunner:
> On Thu, Apr 11, 2019 at 06:42:33PM +0200, Noralf Trønnes wrote:
>> @@ -1299,6 +1299,11 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread)
>>  
>>  	trace_spi_message_start(ctlr->cur_msg);
>>  
>> +	ret = spi_split_transfers_maxsize(ctlr, ctlr->cur_msg, ctlr->max_dma_len,
>> +					  GFP_KERNEL | GFP_DMA);
> 
> Um, shouldn't this be ifdef'ed to CONFIG_HAS_DMA?
> 

Maybe, I don't know. Mark didn't mentioned it when he commented on a
previous version of this. Some hate ifdef's and want to avoid them, some
don't.

> 
>> Some drivers like spi_bcm2835 have a max size on DMA transfers. Work
>> around this by splitting up the transfer if necessary.
> 
> This feels like a very expensive solution to the problem:  Large transfers
> are split into multiple smaller transfers (requiring lots of overhead to
> allocate and populate the structures) and the split transfers seem to be
> cached for later reuse.
> 

Only drivers that set ->max_dma_len will be split, and only when the
length is bigger. I can't see any caching with a quick glance, where do
you see it?

> I'm wondering, for a frame buffer, doesn't the buffer (and hence the SPI
> message) change on every transmission?  In other words, does a frame
> buffer benefit from the caching?  If not, then the caching seems to incur
> unnecessary overhead.
> 
> Even the normal case when no transfers exceed the limit is affected by
> additional overhead, namely an iteration over the transfer list to
> check each transfers' length. :-(
> 
> Note that spi_map_buf() already splits every transfer's sglist into
> segments that are smaller than ctlr->max_dma_len.  Now all that needs
> to be done is to amend spi-bcm2835.c to iterate over the sglist
> and transmit it in portions which do not exceed 65535.  Addressing the
> problem at this lower level would drastically reduce the overhead
> compared to the approach in the present patch and hence appears to be
> more recommendable.
> 

In a previous version of this I suggested to Meghana to put this in the
driver, but Mark wanted it in the core.

Noralf.

> Thanks,
> 
> Lukas
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/4] spi: Split spi message into max_dma_len size chunks

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 550 bytes --]

On Thu, Apr 11, 2019 at 06:42:33PM +0200, Noralf Trønnes wrote:

> +++ b/drivers/spi/spi.c
> @@ -1299,6 +1299,11 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread)
>  
>  	trace_spi_message_start(ctlr->cur_msg);
>  
> +	ret = spi_split_transfers_maxsize(ctlr, ctlr->cur_msg, ctlr->max_dma_len,
> +					  GFP_KERNEL | GFP_DMA);
> +	if (ret)
> +		goto out;
> +

We should do any message maipulation in __spi_validate() with all the
other mainpulation, the message pump should just be pushing messages
out.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 206 bytes --]

_______________________________________________
linux-rpi-kernel mailing list
linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
http://lists.infradead.org/mailman/listinfo/linux-rpi-kernel

Re: [PATCH v4 2/4] spi: Split spi message into max_dma_len size chunks

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2669 bytes --]

On Thu, Apr 11, 2019 at 11:02:26PM +0200, Noralf Trønnes wrote:
> Den 11.04.2019 20.18, skrev Lukas Wunner:
> > On Thu, Apr 11, 2019 at 06:42:33PM +0200, Noralf Trønnes wrote:
> >> @@ -1299,6 +1299,11 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread)

> >>  	trace_spi_message_start(ctlr->cur_msg);

> >> +	ret = spi_split_transfers_maxsize(ctlr, ctlr->cur_msg, ctlr->max_dma_len,
> >> +					  GFP_KERNEL | GFP_DMA);

> > Um, shouldn't this be ifdef'ed to CONFIG_HAS_DMA?

> Maybe, I don't know. Mark didn't mentioned it when he commented on a
> previous version of this. Some hate ifdef's and want to avoid them, some
> don't.

I *think* we managed to fix all the architectures to at least stub out
the DMA interfaces, it's such a pointless thing to have conditional -
it really only makes a difference for build coverage.

> >> Some drivers like spi_bcm2835 have a max size on DMA transfers. Work
> >> around this by splitting up the transfer if necessary.

> > This feels like a very expensive solution to the problem:  Large transfers
> > are split into multiple smaller transfers (requiring lots of overhead to
> > allocate and populate the structures) and the split transfers seem to be
> > cached for later reuse.

> Only drivers that set ->max_dma_len will be split, and only when the
> length is bigger. I can't see any caching with a quick glance, where do
> you see it?

It *is* more expensive to post-process the transfers and have to do
allocations rather than having them set up as we want them on the way
in.

> > Even the normal case when no transfers exceed the limit is affected by
> > additional overhead, namely an iteration over the transfer list to
> > check each transfers' length. :-(

We already have the iterations in the message validation, if the code is
integrated there the overhead will be reduced.

> > Note that spi_map_buf() already splits every transfer's sglist into
> > segments that are smaller than ctlr->max_dma_len.  Now all that needs
> > to be done is to amend spi-bcm2835.c to iterate over the sglist
> > and transmit it in portions which do not exceed 65535.  Addressing the
> > problem at this lower level would drastically reduce the overhead
> > compared to the approach in the present patch and hence appears to be
> > more recommendable.

> In a previous version of this I suggested to Meghana to put this in the
> driver, but Mark wanted it in the core.

If we want to do this at a lower level the DMA code could hide this
limitation from the upper layers; presumably the SPI driver isn't the
only thing that might run into this.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/4] spi: Split spi message into max_dma_len size chunks

[kernel-TqfNSX0MhmxHKSADF0wUEw]

> On 12.04.2019, at 11:47, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> 
>> In a previous version of this I suggested to Meghana to put this in the
>> driver, but Mark wanted it in the core.
> 
> If we want to do this at a lower level the DMA code could hide this
> limitation from the upper layers; presumably the SPI driver isn't the
> only thing that might run into this.

For clarification:
There is a register of the SPI controller where you have to configure the
number of bytes that it will request via DMA (primarily support transfers
that are not a multiple of 4 - the data is transferred by DMA as words).
So it is not really related to the general DMA implementation but to the
DMA (request) support of the SPI controller.

Martin

Re: [PATCH v4 2/4] spi: Split spi message into max_dma_len size chunks

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 858 bytes --]

On Fri, Apr 12, 2019 at 12:03:35PM +0200, kernel@martin.sperl.org wrote:
> > On 12.04.2019, at 11:47, Mark Brown <broonie@kernel.org> wrote:

> >> In a previous version of this I suggested to Meghana to put this in the
> >> driver, but Mark wanted it in the core.

> > If we want to do this at a lower level the DMA code could hide this
> > limitation from the upper layers; presumably the SPI driver isn't the
> > only thing that might run into this.

> For clarification:
> There is a register of the SPI controller where you have to configure the
> number of bytes that it will request via DMA (primarily support transfers
> that are not a multiple of 4 - the data is transferred by DMA as words).
> So it is not really related to the general DMA implementation but to the
> DMA (request) support of the SPI controller.

Ah, I see.  That's unfortunate :/

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/4] spi: Split spi message into max_dma_len size chunks

[Lukas Wunner]

On Thu, Apr 11, 2019 at 11:02:26PM +0200, Noralf Trønnes wrote:
> Den 11.04.2019 20.18, skrev Lukas Wunner:
> > On Thu, Apr 11, 2019 at 06:42:33PM +0200, Noralf Trønnes wrote:
> >> @@ -1299,6 +1299,11 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread)
> >>  
> >>  	trace_spi_message_start(ctlr->cur_msg);
> >>  
> >> +	ret = spi_split_transfers_maxsize(ctlr, ctlr->cur_msg, ctlr->max_dma_len,
> >> +					  GFP_KERNEL | GFP_DMA);
> > 
> > Um, shouldn't this be ifdef'ed to CONFIG_HAS_DMA?
> 
> Maybe, I don't know. Mark didn't mentioned it when he commented on a
> previous version of this. Some hate ifdef's and want to avoid them, some
> don't.

The above call is clearly only necessary if DMA is present and
all the DMA-specific functionality in spi.c is ifdef'ed to
CONFIG_HAS_DMA, so the above should likewise only be compiled
in if DMA is present.

Regardless whether this is achieved with an #ifdef or an empty
inline stub for the non-DMA case.


> > > Some drivers like spi_bcm2835 have a max size on DMA transfers. Work
> > > around this by splitting up the transfer if necessary.
> > 
> > This feels like a very expensive solution to the problem:  Large transfers
> > are split into multiple smaller transfers (requiring lots of overhead to
> > allocate and populate the structures) and the split transfers seem to be
> > cached for later reuse.
> > 
> 
> Only drivers that set ->max_dma_len will be split, and only when the
> length is bigger.

The issue on the BCM2385 is that the (non-Lite) DMA channels have a
max length of 1 GByte but the SPI controller has a 65535 byte limit.

There are three other drivers which set max_dma_len, I'm not sure if
they suffer from the same issue as the BCM2835, but this patch causes
additional overhead for them so the driver maintainers / main authors
should at least be cc'ed.


> I can't see any caching with a quick glance, where do you see it?

Misunderstanding of the code on my part, sorry.  I thought
spi_res_release() wouldn't be called until the spi_message is freed,
in which case the split transfers would be kept around (cached) for
retransmissions of the same message.  But I notice now that
spi_res_release() is already called upon finishing the first
transmission of the spi_message, hence the split transfers are gone
and need to be reconstructed on retransmission.


> > Note that spi_map_buf() already splits every transfer's sglist into
> > segments that are smaller than ctlr->max_dma_len.  Now all that needs
> > to be done is to amend spi-bcm2835.c to iterate over the sglist
> > and transmit it in portions which do not exceed 65535.  Addressing the
> > problem at this lower level would drastically reduce the overhead
> > compared to the approach in the present patch and hence appears to be
> > more recommendable.
> 
> In a previous version of this I suggested to Meghana to put this in the
> driver, but Mark wanted it in the core.

Do you have a link to these comments of Mark?  The first version of
this patchset that I have here is v2 of March 2018 and it already
uses spi_split_transfers_maxsize().  I've never seen a version which
splits the sglist in spi-bcm2835.c (instead of splitting the transfers
in spi.c, which, again, is significantly more expensive).

Thanks,

Lukas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/4] spi: Split spi message into max_dma_len size chunks

[Lukas Wunner]

On Fri, Apr 12, 2019 at 10:47:21AM +0100, Mark Brown wrote:
> On Thu, Apr 11, 2019 at 11:02:26PM +0200, Noralf Trønnes wrote:
> > Den 11.04.2019 20.18, skrev Lukas Wunner:
> > > On Thu, Apr 11, 2019 at 06:42:33PM +0200, Noralf Trønnes wrote:
> > > > @@ -1299,6 +1299,11 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread)
> 
> > > >  	trace_spi_message_start(ctlr->cur_msg);
> 
> > > > +	ret = spi_split_transfers_maxsize(ctlr, ctlr->cur_msg, ctlr->max_dma_len,
> > > > +					  GFP_KERNEL | GFP_DMA);
> 
> > > Um, shouldn't this be ifdef'ed to CONFIG_HAS_DMA?
> 
> > Maybe, I don't know. Mark didn't mentioned it when he commented on a
> > previous version of this. Some hate ifdef's and want to avoid them, some
> > don't.
> 
> I *think* we managed to fix all the architectures to at least stub out
> the DMA interfaces, it's such a pointless thing to have conditional -
> it really only makes a difference for build coverage.

My point was that the call to spi_split_transfers_maxsize() shouldn't
be called on non-DMA-capable platforms in the first place (because it's
DMA-specific).


> On Fri, Apr 12, 2019 at 12:03:35PM +0200, kernel@martin.sperl.org wrote:
> > There is a register of the SPI controller where you have to configure the
> > number of bytes that it will request via DMA (primarily support transfers
> > that are not a multiple of 4 - the data is transferred by DMA as words).
> > So it is not really related to the general DMA implementation but to the
> > DMA (request) support of the SPI controller.

Right, if the transfer is not a multiple of 4, the DMA engine will
write up to 3 surplus bytes to the TX FIFO.  The SPI controller
knows how many bytes it has to send because the length is set via
its DLEN register first.  Internally it counts down the DLEN register
to zero and then stops clocking out bytes.  The surplus bytes written
by the DMA engine are thus left as residue in the TX FIFO and have
to be flushed by setting the CLEAR_TX bit in the CS register.

Thanks,

Lukas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/4] spi: Split spi message into max_dma_len size chunks

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 594 bytes --]

On Fri, Apr 12, 2019 at 12:54:48PM +0200, Lukas Wunner wrote:
> On Fri, Apr 12, 2019 at 10:47:21AM +0100, Mark Brown wrote:

> > I *think* we managed to fix all the architectures to at least stub out
> > the DMA interfaces, it's such a pointless thing to have conditional -
> > it really only makes a difference for build coverage.

> My point was that the call to spi_split_transfers_maxsize() shouldn't
> be called on non-DMA-capable platforms in the first place (because it's
> DMA-specific).

It's not a DMA specific problem - there can be SPI controller
limitations on transfer sizes too.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/4] spi: Split spi message into max_dma_len size chunks

[Lukas Wunner]

On Fri, Apr 12, 2019 at 12:09:34PM +0100, Mark Brown wrote:
> On Fri, Apr 12, 2019 at 12:54:48PM +0200, Lukas Wunner wrote:
> > On Fri, Apr 12, 2019 at 10:47:21AM +0100, Mark Brown wrote:
> > > I *think* we managed to fix all the architectures to at least stub out
> > > the DMA interfaces, it's such a pointless thing to have conditional -
> > > it really only makes a difference for build coverage.
> 
> > My point was that the call to spi_split_transfers_maxsize() shouldn't
> > be called on non-DMA-capable platforms in the first place (because it's
> > DMA-specific).
> 
> It's not a DMA specific problem - there can be SPI controller
> limitations on transfer sizes too.

The call does pass in ctlr->max_dma_len though, so is clearly motivated
by a DMA limitation.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/4] spi: Split spi message into max_dma_len size chunks

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 624 bytes --]

On Fri, Apr 12, 2019 at 01:16:15PM +0200, Lukas Wunner wrote:
> On Fri, Apr 12, 2019 at 12:09:34PM +0100, Mark Brown wrote:
> > On Fri, Apr 12, 2019 at 12:54:48PM +0200, Lukas Wunner wrote:

> > > My point was that the call to spi_split_transfers_maxsize() shouldn't
> > > be called on non-DMA-capable platforms in the first place (because it's
> > > DMA-specific).

> > It's not a DMA specific problem - there can be SPI controller
> > limitations on transfer sizes too.

> The call does pass in ctlr->max_dma_len though, so is clearly motivated
> by a DMA limitation.

Yeah, that bit is but we can have other limitations.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/4] spi: Split spi message into max_dma_len size chunks

[kernel-TqfNSX0MhmxHKSADF0wUEw]

> On 12.04.2019, at 13:16, Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> wrote:
> 
> On Fri, Apr 12, 2019 at 12:09:34PM +0100, Mark Brown wrote:
>> On Fri, Apr 12, 2019 at 12:54:48PM +0200, Lukas Wunner wrote:
>>> On Fri, Apr 12, 2019 at 10:47:21AM +0100, Mark Brown wrote:
>>>> I *think* we managed to fix all the architectures to at least stub out
>>>> the DMA interfaces, it's such a pointless thing to have conditional -
>>>> it really only makes a difference for build coverage.
>> 
>>> My point was that the call to spi_split_transfers_maxsize() shouldn't
>>> be called on non-DMA-capable platforms in the first place (because it's
>>> DMA-specific).
>> 
>> It's not a DMA specific problem - there can be SPI controller
>> limitations on transfer sizes too.
> 
> The call does pass in ctlr->max_dma_len though, so is clearly motivated
> by a DMA limitation.
The limitation is in this register: BCM2835_SPI_DLEN

Where the bcm2835-SDK (Brcm_Android_ICS_Graphics_Stack.tar.gz)defines:
bit 0-15 for DLEN

See transformed data at:
https://github.com/msperl/rpi-registers/blob/master/md/Region_SPI.md

Or the Arm Periperial Document on page 156 with regards to DLEN register.

This is where the DMA length limit comes from - so it is NOT
really DMA block related but SPI block related.

More specifically to DMA requests are only triggered when this counter
is > 0.

That is unless the Documentation has another errata and more bits are
actually allowed.

Martin

Re: [PATCH v4 2/4] spi: Split spi message into max_dma_len size chunks

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1780 bytes --]

On Fri, Apr 12, 2019 at 12:46:44PM +0200, Lukas Wunner wrote:
> On Thu, Apr 11, 2019 at 11:02:26PM +0200, Noralf Trønnes wrote:
> > Den 11.04.2019 20.18, skrev Lukas Wunner:

> > > Note that spi_map_buf() already splits every transfer's sglist into
> > > segments that are smaller than ctlr->max_dma_len.  Now all that needs
> > > to be done is to amend spi-bcm2835.c to iterate over the sglist
> > > and transmit it in portions which do not exceed 65535.  Addressing the
> > > problem at this lower level would drastically reduce the overhead
> > > compared to the approach in the present patch and hence appears to be
> > > more recommendable.

> > In a previous version of this I suggested to Meghana to put this in the
> > driver, but Mark wanted it in the core.

> Do you have a link to these comments of Mark?  The first version of
> this patchset that I have here is v2 of March 2018 and it already
> uses spi_split_transfers_maxsize().  I've never seen a version which
> splits the sglist in spi-bcm2835.c (instead of splitting the transfers
> in spi.c, which, again, is significantly more expensive).

The basic theory is that we shouldn't be open coding the same handling
in multiple drivers, they should just be able to declare their
capabilities and have the core handle things as far as possible.  Since
we're already iterating over the whole message I'd be surprised if there
were a particularly big hit in cases where we don't need to split things
out, though obviously that's not been tested yet.

There have also been discussions in the past about pre-cooking some of
the messages so that if the client is sending the same thing a lot (or
things that vary only by data) we can do a lot of the validation one
time which would help.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel