Hi Vinod, Ping. We need a solution to this problem, it's been way too long already. If you don't want to accept my proposal, please provide me with an implementation or a very detailed spec I can implement. On Wed, Apr 08, 2020 at 08:00:49PM +0300, Laurent Pinchart wrote: > On Thu, Mar 26, 2020 at 12:32:34PM +0530, Vinod Koul wrote: > > On 11-03-20, 17:52, Laurent Pinchart wrote: > >> On Wed, Mar 04, 2020 at 09:54:26PM +0530, Vinod Koul wrote: > >>>>>>> Second in error handling where some engines do not support > >>>>>>> aborting (unless we reset the whole controller) > >>>>>> > >>>>>> Could you explain that one ? I'm not sure to understand it. > >>>>> > >>>>> So I have dma to a slow peripheral and it is stuck for some reason. I > >>>>> want to abort the cookie and let subsequent ones runs (btw this is for > >>>>> non cyclic case), so I would use that here. Today we terminate_all and > >>>>> then resubmit... > >>>> > >>>> That's also for immediate abort, right ? > >>> > >>> Right > >>> > >>>> For this to work properly we need very accurate residue reporting, as > >>>> the client will usually need to know exactly what has been transferred. > >>>> The device would need to support DMA_RESIDUE_GRANULARITY_BURST when > >>>> aborting an ongoing transfer. What hardware supports this ? > >>> > >>> git grep DMA_RESIDUE_GRANULARITY_BURST drivers/dma/ |wc -l > >>> 27 > >>> > >>> So it seems many do support the burst reporting. > >> > >> Yes, but not all of those may support aborting a transfer *and* > >> reporting the exact residue of cancelled transfers. We need both to > >> implement your proposal. > > > > Reporting residue is already implemented, please see struct > > dmaengine_result. This can be passed by a callback > > dma_async_tx_callback_result() in struct dma_async_tx_descriptor. > > I mean that I don't know if the driver that support > DMA_RESIDUE_GRANULARITY_BURST only support reporting the residue when > the transfer is active, or also support reporting it when cancelling a > transfer. Maybe all of them do, maybe a subset of them do, so I can't > tell if this would be a feature that could be widely supported. > > >>>>>>> But yes the .terminate_cookie() semantics should indicate if the > >>>>>>> termination should be immediate or end of current txn. I see people > >>>>>>> using it for both. > >>>>>> > >>>>>> Immediate termination is *not* something I'll implement as I have no > >>>>>> good way to test that semantics. I assume you would be fine with leaving > >>>>>> that for later, when someone will need it ? > >>>>> > >>>>> Sure, if you have hw to support please test. If not, you will not > >>>>> implement that. > >>>>> > >>>>> The point is that API should support it and people can add support in > >>>>> the controllers and test :) > >>>> > >>>> I still think this is a different API. We'll have > >>>> > >>>> 1. Existing .issue_pending(), queueing the next transfer for non-cyclic > >>>> cases, and being a no-op for cyclic cases. > >>>> 2. New .terminate_cookie(AT_END_OF_TRANSFER), being a no-op for > >>>> non-cyclic cases, and moving to the next transfer for cyclic cases. > >>>> 3. New .terminate_cookie(ABORT_IMMEDIATELY), applicable to both cyclic > >>>> and non-cyclic cases. > >>>> > >>>> 3. is an API I don't need, and can't easily test. I agree that it can > >>>> have use cases (provided the DMA device can abort an ongoing transfer > >>>> *and* still support DMA_RESIDUE_GRANULARITY_BURST in that case). > >>>> > >>>> I'm troubled by my inability to convince you that 1. and 2. are really > >>>> the same, with 1. addressing the non-cyclic case and 2. addressing the > >>>> cyclic case :-) This is why I think they should both be implemeted using > >>>> .issue_pending() (no other option for 1., that's what it uses today). > >>>> This wouldn't prevent implementing 3. with a new .terminate_cookie() > >>>> operation, that wouldn't need to take a flag as it would always operate > >>>> in ABORT_IMMEDIATELY mode. There would also be no need to report a new > >>>> capability for 3., as the presence of the .terminate_cookie() handler > >>>> would be enough to tell clients that the API is supported. Only a new > >>>> capability for 2. would be needed. > >>> > >>> Well I agree 1 & 2 seem similar but I would like to define the behaviour > >>> not dependent on the txn being cyclic or not. That is my concern and > >>> hence the idea that: > >>> > >>> 1. .issue_pending() will push txn to pending_queue, you may have a case > >>> where that is done only once (due to nature of txn), but no other > >>> implication > >>> > >>> 2. .terminate_cookie(EOT) will abort the transfer at the end. Maybe not > >>> used for cyclic but irrespective of that, the behaviour would be abort > >>> at end of cyclic > >> > >> Did you mean "maybe not used for non-cyclic" ? > > > > Yes I think so.. > > > >>> 3. .terminate_cookie(IMMEDIATE) will abort immediately. If there is > >>> anything in pending_queue that will get pushed to hardware. > >>> > >>> 4. Cyclic by nature never completes > >>> - as a consequence needs to be stopped by terminate_all/terminate_cookie > >>> > >>> Does these rules make sense :) > >> > >> It's a set of rules that I think can handle my use case, but I still > >> believe my proposal based on just .issue_pending() would be simpler, in > >> line with the existing API concepts, and wouldn't preclude the addition > >> of .terminate_cookie(IMMEDIATE) at a later point. It's your call though, > >> especially if you provide the implementation :-) When do you think you > >> will be able to do so ? > > > > I will try to take a stab at it once merge window opens.. will let you > > and Peter for sneak preview once I start on it :) > > I started giving it a try as this has been blocked for two months and a > half now. > > I very quickly ran into issues as the interface is ill-defined as it > stands. > > - What should happen when .terminate_cookie(EOT) is called with no other > transfer issued, and a new transfer is issued before the current > transfer terminates ? > > - I expect .terminate_cookie() to be asynchronous, as .terminate_all(). > This means that actual termination of cyclic transfers will actually > be handled at end of transfer, in the interrupt handler. This creates > race conditions with other operations. It would also make it much more > difficult to support this feature for devices that require sleeping > when stopping the DMA engine at the end of a cyclic transfer. > > If we have to go forward with this new API, I need a detailed > explanation of how all this should be handled. I still truly believe > this is a case of yak shaving that introduces additional complexity for > absolutely no valid reason, when a solution that is aligned with the > existing API and its concepts exists already. It's your decision as the > subsystem maintainer, but if you want something more complex, please > provide it soon. I don't want to wait another three months to see > progress on this issue. > > >>>>>>> And with this I think it would make sense to also add this to > >>>>>>> capabilities :) > >>>>>> > >>>>>> I'll repeat the comment I made to Peter: you want me to implement a > >>>>>> feature that you think would be useful, but is completely unrelated to > >>>>>> my use case, while there's a more natural way to handle my issue with > >>>>>> the current API, without precluding in any way the addition of your new > >>>>>> feature in the future. Not fair. > >>>>> > >>>>> So from API design pov, I would like this to support both the features. > >>>>> This helps us to not rework the API again for the immediate abort. > >>>>> > >>>>> I am not expecting this to be implemented by you if your hw doesn't > >>>>> support it. The core changes are pretty minimal and callback in the > >>>>> driver is the one which does the job and yours wont do this > >>>> > >>>> Xilinx DMA drivers don't support DMA_RESIDUE_GRANULARITY_BURST so I > >>>> can't test this indeed. > >>> > >>> Sure I understand that! Am sure folks will respond to CFT and I guess > >>> Peter will also be interested in testing. > >> > >> s/testing/implementing it/ :-) > > > > Even better :) -- Regards, Laurent Pinchart
