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:
signature.asc
Description: PGP signature
