Hi Vinod and Peter, Gentle ping. On Wed, Jun 17, 2020 at 12:39:09AM +0300, Laurent Pinchart wrote: > On Wed, Jun 03, 2020 at 09:36:09PM +0530, Vinod Koul wrote: > > On 03-06-20, 13:51, Peter Ujfalusi wrote: > >> On 01/06/2020 14.49, Laurent Pinchart wrote: > >>> On Mon, Jun 01, 2020 at 02:14:03PM +0300, Peter Ujfalusi wrote: > >>>> On 28/05/2020 5.10, Laurent Pinchart wrote: > >>>>>>> As mentioned in the commit message, I plan to extend that, I just didn't > >>>>>>> want to add the checks to all the prepare operation wrappers until an > >>>>>>> agreement on the approach would be reached. I also thought it would be > >>>>>>> good to not allow this API for other transaction types until use cases > >>>>>>> arise, in order to force upstream discussions instead of silently > >>>>>>> abusing the API :-) > >>>>>> > >>>>>> I would not object if slave_sg and memcpy got the same treatment. If the > >>>>>> DMA driver did not set the DMA_REPEAT then clients can not use this > >>>>>> feature anyways. > >>>>> > >>>>> Would you not object, or would you prefer if it was done in v5 ? :-) > >>>> > >>>> DMA_REPEAT is a generic flag, not limited to only interleaved, but you > >>>> are going to be the first user of it with interleaved. > >>>> > >>>>> Overall I think that enabling APIs that have no user isn't necessarily > >>>>> the best idea, as it's prone to design issues, but I don't mind doing so > >>>>> if you think it needs to be done now. > >>>> > >>>> We would get the support in one go with the same commit. I don't think > >>>> it makes much sense to add slave_sg later, then memcpy another time. > >>>> True, there might be no users for them for some time, but their presents > >>>> might invite users? > >>> > >>> My approach to API design is that an API designed without (at least) one > >>> user is very prone to be a bad API. As I said before I don't mind > >>> enabling support for slave_sg and memcpy today already, even if I don't > >>> think it's a good idea. I want to get my use case supported, and I've > >>> given up on what I would consider a good API :-) That's fine, > >>> maintainers are the ones who have to support APIs and the design choices > >>> behind them in the longer term, and I'm not a subsystem maintainer here. > >>> I tried to prevent what I think may become a case of shooting in the > >>> foot, but I could be wrong. Only the future will tell. > >> > >> Yes, we will see in the longer run. > > > > I am not sure I would like to add an API without a user, we can add some > > notes in documentation for this and future ideas on how to add this, but > > an API without user doesn't sound right to me. > > That's my preference as well. Peter, are you OK with that ? > > >>>>>>> I can extend the flag to all other transaction types > >>>>>>> (except for the cyclic transaction, as it doesn't make sense there). > >>>>>> > >>>>>> Yep, cyclic is a different type of transfer, it is for circular buffers. > >>>>>> It could be seen as a special case of slave_sg. Some drivers actually > >>>>>> create temporary sg_list in case of cyclic and use the same setup > >>>>>> function to set up the transfer for slave_sg/cyclic... > >>>>> > >>>>> Cyclic is different for historical reasons, but if I had to redesign it > >>>>> today, I'd make it slave_sg + DMA_PREP_REPEAT. We obviously can't, and I > >>>>> have no issue with that. > >>>> > >>>> Which should be accompanied with a flag to tell that the sg_list is > >>>> covering a circular buffer to save all drivers to check the sg_list that > >>>> it is circular buffer (current cyclic) or really sg. > >>>> Some DMA can only do repeat on circular buffers (omap-dma, tegra, etc). > >>> > >>> Isn't DMA_PREP_REPEAT that flag ? > >> > >> Not really. It tells that the descriptor should be repeated. In case of > >> slave_sg the list could describe one block of memory, split up to > >> 'periods' or it could be a list scattered chunks all over the place. > >> > >> circular buffer can be described with sg_list. > >> sg_list is not necessary describes a circular buffer. > >> > >>>>>> But, DMA drivers might support neither of them, either of them or both. > >>>>>> It is up to the client to pick the preferred method for it's use. > >>>>>> It is not far fetched that the next DMA the client is going to be > >>>>>> serviced will have different capabilities and the client needs to handle > >>>>>> EOT or NOW or it might even need to have fallback to case when neither > >>>>>> is supported. > >>>>>> > >>>>>> I don't like excessive flags either, but based on my experience > >>>>>> under-flagging can bite back sooner than later. > >>>>>> > >>>>>> I'm aware that at the moment it feels like it is too explicit, but never > >>>>>> underestimate the creativity of the design - and in some cases the > >>>>>> constraint the design must fulfill. > >>>>> > >>>>> I'm still very puzzled by why you think adding DMA_PREP_LOAD_EOT now is > >>>>> a good idea, given that there's no existing and no foreseen use case for > >>>>> not setting it. Creating an API element that is completely disconnected > >>>>> from any known use case doesn't seem like good API design to me, > >>>>> especially for an in-kernel API. > >>>> > >>>> If we document that DMA_REPEAT covers REPEAT _and_ LOAD_EOT with one > >>>> flag then how would other drivers can implement REPEAT if they can not > >>>> support LOAD_EOT? > >>>> They should do DMA_REPEAT | NOT_LOAD_EOT | LOAD_ASAP? > >>> > >>> As stated before, I think a DMA_LOAD_EOT capability is useful. My > >>> concern is about DMA_PREP_LOAD_EOT for which I can't see use cases. I've > >>> added DMA_PREP_LOAD_EOT in the last patch series, and my DMA engine > >>> driver ignores the transaction when DMA_PREP_LOAD_EOT is not set, as > >>> required. It works fine as the my client always sets it. > >> > >> Thanks. > >> > >>> I'd expect Vinod or you to write the documentation though, as writing > >>> code for an API I don't believe in is one thing, writing documentation > >>> to explain the rationale behind the API design will be more complex > >> > >> Vinod can correct me, but for the capabilities: > >> DMA_REPEAT: the controller (and driver) supports repeating the > >> descriptor. It can be terminated with terminate_all > >> DMA_LOAD_EOT: the controller (and driver) supports loading the next > >> issued transfer on a channel which is running DMA_REPEAT > >> descriptor. Iow, instead of reloading the running transfer, it > >> moves to the next one. > >> DMA_LOAD_NOW: the controller (and driver) supports aborting the > >> active descriptor (either DMA_REPEAT or non repeated one) and > >> moving to the next issued transfer without clients needing to > >> use terminate_all. > > > > Sounds right to me. > > For the same reason as above, my latest patch series doesn't include > DMA_LOAD_NOW, as that would be an API with no user. Vinod, is that OK > with you ? > > >>> when I don't believe there's any rationale :-) > >> > >> Sure, you have a specific DMA, which does one thing and one thing only. > >> When a subsystem decides to create a generic DMA layer on top of > >> DMAengine for example to get rid of the duplicated code in the drivers > >> then this generic code does need information to decide how the servicing > >> DMA should be used for optimal performance and quality. > >> Some DMAs (and drivers) might have slightly different capabilities. > >> > >>>> LOAD_EOT is a feature the HW can or can not support and it is an > >>>> operation mode that you want to use or do not want to use. > >>> > >>> DMA_PREP_REPEAT for the EOT mode, DRM_PREP_REPEAT | DMA_PREP_LOAD_NOW > >>> for the immediate mode would work too, and wouldn't have the drawback of > >>> artificially creating a case (!EOT && !NOW) that would fail. > >> > >> But if a DMA does not support LOAD_EOT at all? If it did not support > >> LOAD_NOW either? > > If the driver doesn't support EOT, then REPEAT without EOT would be > rejected by the prepare operation. If the driver doesn't support EOT not > NOW, then it wouldn't support REPEAT :-) In any case, both EOT and NOW > would be rejected, and so would REPEAT (REPEAT without EOT or NOW > doesn't make much sense). > > >> But if anything the LOAD_NOW sounds more of a default expectation than > >> LOAD_EOT. > >> Yes, I know. The display use case needs LOAD_EOT to avoid artifacts on > >> screen, but DMA_REPEAT is not only for displays. > > > > Correct, a user can request LOAD_NOW or LOAD_EOT, driver should be able > > to handle (as long as h/w supports) and act accordingly. > > > > Dmaengine layer and drivers and not specific to one interface or one > > user, the idea is to write generic dmaengine driver catering to > > different users, so supporting different flags from driver pov as well > > dmaengine framework pov is required. > > Let's skip the lectures on API design, I think we're way past that > point. > > Could you please review the latest patch series > (https://lore.kernel.org/dmaengine/20200528025228.31638-1-laurent.pinchart@xxxxxxxxxxxxxxxx/T/#t) > ? -- Regards, Laurent Pinchart
