Hi Peter, On Fri, Mar 06, 2020 at 04:49:01PM +0200, Peter Ujfalusi wrote: > On 04/03/2020 18.00, Laurent Pinchart wrote: > > 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. > > Let's see the two cases, AT_END_OF_TRANSFER and ABORT_IMMEDIATELY > against cyclic and slave for simplicity: > - AT_END_OF_TRANSFER > ... > issue_pending(1) > issue_pending(2) > terminate_cookie(AT_END_OF_TRANSFER) > > In case of cyclic: > When cookie1 finishes a tx cookie2 will start. > > Same sequence in case of slave: > When cookie1 finishes a tx cookie2 will start. > Yes, terminate_cookie(AT_END_OF_TRANSFER) is NOP > > - ABORT_IMMEDIATELY > ... > issue_pending(1) > issue_pending(2) > terminate_cookie(ABORT_IMMEDIATELY) > > In case of cyclic and slave: > Abort cookie1 right away and start cookie2. > > In case of cyclic: > When cookie1 finishes a tx cookie2 will start. Is this paragraph a copy & paste leftover ? > True, we have NOP operation, but as you can see the semantics of the two > cases are well defined and consistent among different operations. I'm not disputing that, but I still think that the semantics for the proposal based solely on issue_pending() is well-defined too and consistent among different operations :-) My point is that terminate_cookie() is only required for the ABORT_IMMEDIATELY case, which could be implemented on top of my proposal. Anyway, I seem to have failed in my attempt to convincing Vinod, and he proposed providing the implementation of terminate_cookie() in the DMA engine core and doc, so I'll rebase the driver on top of that and submit the two together after testing. > Imho the only thing which is not really defined is the > AT_END_OF_TRANSFER, is it after the current period, or when finishing > the buffer / after a frame or all frames are consumed in the current tx > for interleaved. For 2D interleaved cyclic transfers, there's a single period, so that's not an issue. For the existing cyclic API it's up to us to decide, and I don't have enough insight on the expected usage and hardware features to answer that question. > >>>> 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. > > All TI DMA supports it ;) Great, so you can implement this feature ;-) > >>>>>> So, the .terminate_cookie() should be a feature for all type of txn's. > >>>>>> If for some reason (dont discount what hw designers can do) a controller > >>>>>> supports this for some specific type(s), then they should return > >>>>>> -ENOTSUPP for cookies that do not support and let the caller know. > >>>>> > >>>>> But then the caller can't know ahead of time, it will only find out when > >>>>> it's too late, and can't decide not to use the DMA engine if it doesn't > >>>>> support the feature. I don't think that's a very good option. > >>>> > >>>> Agreed so lets go with adding these in caps. > >>> > >>> So if there's a need for caps anyway, why not a cap that marks > >>> .issue_pending() as moving from the current cyclic transfer to the next > >>> one ? > >> > >> Is the overhead really too much on that :) If you like I can send the > >> core patches and you would need to implement the driver side? > > > > We can try that as a compromise. One of main concerns with developing > > the core patches myself is that the .terminate_cookie() API still seems > > ill-defined to me, so it would be much more efficient if you translate > > the idea you have in your idea into code than trying to communicate it > > to me in all details (one of the grey areas is what should > > .terminate_cookie() do if the cookie passed to the function corresponds > > to an already terminated or, more tricky from a completion callback > > point of view, an issued but not-yet-started transfer, or also a > > submitted but not issued transfer). If you implement the core part, then > > that problem will go away. > > > > How about the implementation in virt-dma.[ch] by the way ? -- Regards, Laurent Pinchart
