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
