Hi Laurent, Sorry for delay in replying.. 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. > > >>>> 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 :) > > >>>> 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 :) -- ~Vinod
