Re: [PATCH v4 3/6] dmaengine: Add support for repeating transactions

[Peter Ujfalusi <peter.ujfalusi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 3673 bytes --]

Hi Laurent,

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?

>>> 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).

>> 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?

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.

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[-- Attachment #2: pEpkey.asc --]
[-- Type: application/pgp-keys, Size: 1783 bytes --]