Re: [PATCH v3 2/6] dmaengine: Add interleaved cyclic transaction type

[Vinod Koul <vkoul@kernel.org>]

Hi Laurent,

On 26-02-20, 18:30, Laurent Pinchart wrote:
> On Wed, Feb 19, 2020 at 02:55:14PM +0530, Vinod Koul wrote:
> > On 17-02-20, 12:00, Peter Ujfalusi wrote:
> > > On 14/02/2020 18.22, Laurent Pinchart wrote:
> > >>>> It does, but I really wonder why we need a new terminate operation that
> > >>>> would terminate a single transfer. If we call issue_pending at step B.3,
> > >>>> when the new txn submitted, we can terminate the current transfer at the
> > >>>> point. It changes the semantics of issue_pending, but only for cyclic
> > >>>> transfers (this whole discussions it only about cyclic transfers). As a
> > >>>> cyclic transfer will be repeated forever until terminated, there's no
> > >>>> use case for issuing a new transfer without terminating the one in
> > >>>> progress. I thus don't think we need a new terminate operation: the only
> > >>>> thing that makes sense to do when submitting a new cyclic transfer is to
> > >>>> terminate the current one and switch to the new one, and we already have
> > >>>> all the APIs we need to enable this behaviour.
> > >>>
> > >>> The issue_pending() is a NOP when engine is already running.
> > >> 
> > >> That's not totally right. issue_pending() still moves submitted but not
> > >> issued transactions from the submitted queue to the issued queue. The
> > >> DMA engine only considers the issued queue, so issue_pending()
> > >> essentially tells the DMA engine to consider the submitted transaction
> > >> for processing after the already issued transactions complete (in the
> > >> non-cyclic case).
> > > 
> > > Vinod's point is for the cyclic case at the current state. It is NOP
> > > essentially as we don't have way to not kill the whole channel.
> > 
> > Or IOW there is no descriptor movement to hardware..
> > 
> > > Just a sidenote: it is not even that clean cut for slave transfers
> > > either as the slave_config must _not_ change between the issued
> > > transfers. Iow, you can not switch between 16bit and 32bit word lengths
> > > with some DMA. EDMA, sDMA can do that, but UDMA can not for example...
> > > 
> > >>> The design of APIs is that we submit a txn to pending_list and then the
> > >>> pending_list is started when issue_pending() is called.
> > >>> Or if the engine is already running, it will take next txn from
> > >>> pending_list() when current txn completes.
> > >>>
> > >>> The only consideration here in this case is that the cyclic txn never
> > >>> completes. Do we really treat a new txn submission as an 'indication' of
> > >>> completeness? That is indeed a point to ponder upon.
> > >> 
> > >> The reason why I think we should is two-fold:
> > >> 
> > >> 1. I believe it's semantically aligned with the existing behaviour of
> > >> issue_pending(). As explained above, the operation tells the DMA engine
> > >> to consider submitted transactions for processing when the current (and
> > >> other issued) transactions complete. If we extend the definition of
> > >> complete to cover cyclic transactions, I think it's a good match.
> > > 
> > > We will end up with different behavior between cyclic and non cyclic
> > > transfers and the new behavior should be somehow supported by existing
> > > drivers.
> > > Yes, issue_pending is moving the submitted tx to the issued queue to be
> > > executed on HW when the current transfer finished.
> > > We only needed this for non cyclic uses so far. Some DMA hw can replace
> > > the current transfer with a new one (re-trigger to fetch the new
> > > configuration, like your's), but some can not (none of the system DMAs
> > > on TI platforms can).
> > > If we say that this is the behavior the DMA drivers must follow then we
> > > will have non compliant DMA drivers. You can not move simply to other
> > > DMA or can not create generic DMA code shared by drivers.
> > 
> > That is very important point for API. We want no implicit behaviour, so
> > if we want an behaviour let us do that explicitly.
> 
> As I've just explained in my reply to Peter, there's nothing implicit in
> my proposal :-) It's however missing a flag to report if the DMA engine
> driver supports this feature, put apart from that, it makes the API
> *more* consistent by making issue_pending() cover *all* transfer types
> with the *same* semantics.

I would be more comfortable in calling an API to do so :)
The flow I am thinking is:

- prep cyclic1 txn
- submit cyclic1 txn
- call issue_pending() (cyclic one starts)

- prep cyclic2 txn
- submit cyclic2 txn
- signal_cyclic1_txn aka terminate_cookie()
- cyclic1 completes, switch to cyclic2 (dmaengine driver)
- get callback for cyclic1 (optional)

To check if hw supports terminate_cookie() or not we can check if the
callback support is implemented

> 
> > >> 2. There's really nothing else we could do with cyclic transactions.
> > >> They never complete today and have to be terminated manually with
> > >> terminate_all(). Using issue_pending() to move to a next cyclic
> > >> transaction doesn't change the existing behaviour by replacing a useful
> > >> (and used) feature, as issue_pending() is currently a no-op for cyclic
> > >> transactions. The newly issued transaction is never considered, and
> > >> calling terminate_all() will cancel the issued transactions. By
> > >> extending the behaviour of issue_pending(), we're making a new use case
> > >> possible, without restricting any other feature, and without "stealing"
> > >> issue_pending() and preventing it from implementing another useful
> > >> behaviour.
> > > 
> > > But at the same time we make existing drivers non compliant...
> > > 
> > > Imo a new callback to 'kill' / 'terminate' / 'replace' / 'abort' an
> > > issued cookie would be cleaner.
> > > 
> > > cookie1 = dmaengine_issue_pending();
> > > // will start the transfer
> > > cookie2 = dmaengine_issue_pending();
> > > // cookie1 still runs, cookie2 is waiting to be executed
> > > dmaengine_abort_tx(chan);
> > > // will kill cookie1 and executes cookie2
> > 
> > Right and we need a kill mode which kills the cookie1 at the end of
> > transfer (conditional to hw supporting that)
> > 
> > I think it should be generic API and usable in both the cyclic and
> > non-cyclic case
> 
> I have no issue with an API that can abort ongoing transfers without
> killing the whole queue of pending transfers, but that's not what I'm
> after, it's not my use case. Again, as explained in my reply to Peter,
> I'm not looking for a way to abort a transfer immediately, but to move
> to the next transfer at the end of the current one. It's very different,
> and the DMA engine API already supports this for all transfers but
> cyclic transfers. I'd go as far as saying that my proposal is fixing a
> bug in the current implementation :-)
> 
> > > dmaengine_abort_tx() could take a cookie as parameter if we wish, so you
> > > can say selectively which issued tx you want to remove, if it is the
> > > running one, then stop it and move to the next one.
> > > In place of the cookie parameter a 0 could imply that I don't know the
> > > cookie, but kill the running one.
> > > 
> > > We would preserve what issue_pending does atm and would give us a
> > > generic flow of how other drivers should handle such cases.
> > > 
> > > Note that this is not only useful for cyclic cases. Any driver which
> > > currently uses brute-force termination can be upgraded.
> > > Prime example is UART RX. We issue an RX buffer to receive data, but it
> > > is not guarantied that the remote will send data which would fill the
> > > buffer and we hit a timeout waiting. We could issue the next buffer and
> > > kill the stale transfer to reclaim the received data.
> > > 
> > > I think this can be even implemented for DMAs which can not do the same
> > > thing as your DMA can.
> > > 
> > >> In a nutshell, an important reason why I like using issue_pending() for
> > >> this purpose is because it makes cyclic and non-cyclic transactions
> > >> behave more similarly, which I think is good from an API consistency
> > >> point of view.
> > >> 
> > >>> Also, we need to keep in mind that the dmaengine wont stop a cyclic
> > >>> txn. It would be running and start next transfer (in this case do
> > >>> from start) while it also gives you an interrupt. Here we would be
> > >>> required to stop it and then start a new one...
> > >> 
> > >> We wouldn't be required to stop it in the middle, the expected behaviour
> > >> is for the DMA engine to complete the cyclic transaction until the end
> > >> of the cycle and then replace it by the new one. That's exactly what
> > >> happens for non-cyclic transactions when you call issue_pending(), which
> > >> makes me like this solution.
> > > 
> > > Right, so we have two different use cases. Replace the current transfers
> > > with the next issued one and abort the current transfer now and arm the
> > > next issued one.
> > > dmaengine_abort_tx(chan, cookie, forced) ?
> > > forced == false: replace it at cyclic boundary
> > > forced == true: right away (as HW allows), do not wait for cyclic round
> > > 
> > >>> Or perhaps remove the cyclic setting from the txn when a new one
> > >>> arrives and that behaviour IMO is controller dependent, not sure if
> > >>> all controllers support it..
> > >> 
> > >> At the very least I would assume controllers to be able to stop a cyclic
> > >> transaction forcefully, otherwise terminate_all() could never be
> > >> implemented. This may not lead to a gracefully switch from one cyclic
> > >> transaction to another one if the hardware doesn't allow doing so. In
> > >> that case I think tx_submit() could return an error, or we could turn
> > >> issue_pending() into an int operation to signal the error. Note that
> > >> there's no need to mass-patch drivers here, if a DMA engine client
> > >> issues a second cyclic transaction while one is in progress, the second
> > >> transaction won't be considered today. Signalling an error is in my
> > >> opinion a useful feature, but not doing so in DMA engine drivers can't
> > >> be a regression. We could also add a flag to tell whether this mode of
> > >> operation is supported.
> > > 
> > > My problems is that it is changing the behavior of issue_pending() for
> > > cyclic. If we document this than all existing DMA drivers are broken
> > > (not complaint with the API documentation) as they don't do this.
> > > 
> > > 
> > >>>>> That would be a clean way to handle it. We were missing this API for a
> > >>>>> long time to be able to cancel the ongoing transfer (whether it is
> > >>>>> cyclic or slave_sg, or memcpy) and move to the next one if there is one
> > >>>>> pending.
> > >>>>
> > >>>> Note that this new terminate API wouldn't terminate the ongoing transfer
> > >>>> immediately, it would complete first, until the end of the cycle for
> > >>>> cyclic transfers, and until the end of the whole transfer otherwise.
> > >>>> This new operation would thus essentially be a no-op for non-cyclic
> > >>>> transfers. I don't see how it would help :-) Do you have any particular
> > >>>> use case in mind ?
> > >>>
> > >>> Yeah that is something more to think about. Do we really abort here or
> > >>> wait for the txn to complete. I think Peter needs the former and your
> > >>> falls in the latter category
> > >> 
> > >> I definitely need the latter, otherwise the display will flicker (or
> > >> completely misoperate) every time a new frame is displayed, which isn't
> > >> a good idea :-)
> > > 
> > > Sure, and it is a great feature.
> > > 
> > >> I'm not sure about Peter's use cases, but it seems to me
> > >> that aborting a transaction immediately is racy in most cases, unless
> > >> the DMA engine supports byte-level residue reporting.
> > > 
> > > Sort of yes. With EDMA, sDMA I can just kill the channel and set up a
> > > new one right away.
> > > UDMA on the other hand is not that forgiving... I would need to kill the
> > > channel, wait for the termination to complete, reconfigure the channel
> > > and execute the new transfer.
> > > 
> > > But with a separate callback API at least there will be an entry point
> > > when this can be initiated and handled.
> > > Fwiw, I think it should be simple to add this functionality to them, the
> > > code is kind of handling it in other parts, but implementing it in the
> > > issue_pending() is not really a clean solution.
> > > 
> > > In a channel you can run slave_sg transfers followed by cyclic if you
> > > wish. A slave channel is what it is, slave channel which can be capable
> > > to execute slave_sg and/or cyclic (and/or interleaved).
> > > If issue_pending() is to take care then we need to check if the current
> > > transfer is cyclic or not and decide based on that.
> > > 
> > > With a separate callback we in the DMA driver just need to do what the
> > > client is asking for and no need to think.
> > > 
> > >> One non-intrusive
> > >> option would be to add a flag to signal that a newly issued transaction
> > >> should interrupt the current transaction immediately.
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
~Vinod