Re: Depreciated spi_master.transfer and "prepared spi messages" for an optimized pipelined-SPI-DMA-driver

[Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 4102 bytes --]

On Thu, Nov 14, 2013 at 08:47:35PM +0100, Martin Sperl wrote:
> > Meh, it doesn't seem that urgently different to be honest.  The basic
> > flow of interactions with the device is very much typical of SPI
> > controllers.

> Well - if it was not for the requirement to call 
> spi_finalize_current_message prior to leaving transfer_one message to get
> the next one - I might have continued this route with pipelining.
> But hacking my way around those limitations (by taking the next item of the 
> list, etc.) seemed a bit absurd to be honest.

The thing to do there is extend the interface, you're treating things
like you're working on a proprietary OS where the APIs are fixed.

> Well - if you think this "approach" is also applicable to other devices,
> then this is one more argument to allow device drivers to prepare its 
> messages (with the is_dma_mapped requirement to keep cost low). 

No, this isn't something which should be being open coded in individual
drivers - it's far too much redundant work especially once you get into
doing the tradeoffs for no queue vs queue cases.  Using regmap MMIO may
be helpful but we need to do that in a way that allows us to integrate
it into the DMA pipeline...

This is part of what I keep saying about doing things in a way that
makes them generally applicable.

> Note that if you do that and want to leverage the "prepare_message",
> then I would recommend you add an additional flag field to these calls.
> Because then you may pass some hints to the engine (like if it can
> use GFP_KERNEL or GFP_ATOMIC during allocations). And prepare 
> could get done with GFP_KERNEL, while a "prepare" during spi_async
> would need to be done via GFP_ATOMIC.

Or in the async case we just punt the prepare to the thread.

> > That's fine for experimentation but for an actual merged API I think it
> > does need to be generally applicable so that it's worth the effort for
> > client drivers to support it.

> First of all it is a bus-driver that should be working.
> (also to avoid the expected "short transfer issue" for some use-cases I 
> would propose to run it in parallel to the existing version)

> Then I believe we can start discussing any approach to refactor things
> after we get that far and we see the performance improvements and 
> drawbacks for each of those steps.

It'll definitely need some experimentation and tuning.

> Well, actually if you do it right - like I did in my version of the
> mcp2515 driver, I am actually scheduling 2 spi_messages back to back.
> and only the first has a callback. 

> The first reads the status registers on which to base decisions what
> to do and also pulls the interrupt high.

> While the second message does a speculative transfer of the 
> first inbound message buffer and reads some error counters.

> So ideally while the callback of the 1st message is running the 2nd
> message is running in HW.

Yes, this is exactly the sort of thing I'm talking about - it's cutting
out dead time on the bus.

> For any driver that is _not_ optimized for latencies and where one 
> needs low latency then a rewrite is required.

Right, and the big concern with the way you're currently proposing to
implement the optimisation is that it's leaving essentially the whole
thing up to the individual driver.  This is going to result in limited
adoption and is going to mean there's no sharing of learning and fixes
between drivers.

> I believe I should finish my POC to get fully up to my expectations
> and then I can show you the advantage of having this "minimal" change,
> which has also an overall positive impact (even though slight).

To repeat yet again, something that is just a tunnel through to this one
heavily optimised master driver isn't good.  The problem with the
proposed implementation is that it is essentially just tunnelling
through to a heavily optimised master, it's not generic enough.  If
other devices can make use of it it's more interesting but then those
uses ought to be being factored out.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]