Hi Vinod, On Mon, Mar 02, 2020 at 09:17:35AM +0530, Vinod Koul wrote: > 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 Two questions though: - Where is .issue_pending() called for cyclic2 in your above sequence ? Surely it should be called somewhere, as the DMA engine API requires .issue_pending() to be called for a transfer to be executed, otherwise it stays in the submitted but not pending queue. - With the introduction of a new .terminate_cookie() operation, we need to specify that operation for all transfer types. What's its envisioned semantics for non-cyclic transfers ? And how do DMA engine drivers report that they support .terminate_cookie() for cyclic transfers but not for other transfer types (the counterpart of reporting, in my proposition, that .issue_pending() isn't supported replace the current cyclic transfer) ? > >>>> 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
