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

[Mark Brown <broonie@kernel.org>]

[-- 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