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

[Noralf Trønnes <noralf@xxxxxxxxxxx>]

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
>