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

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

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

Hi Laurent, Vinod,

On 15/05/2020 17.11, Laurent Pinchart wrote:
> On Fri, May 15, 2020 at 02:08:17PM +0530, Vinod Koul wrote:
>> Hi Laurent,
>>
>> On 14-05-20, 23:07, Laurent Pinchart wrote:
>>> On Thu, May 14, 2020 at 11:53:44PM +0530, Vinod Koul wrote:
>>>> On 13-05-20, 19:59, Laurent Pinchart wrote:
>>>>> DMA engines used with displays perform 2D interleaved transfers to read
>>>>> framebuffers from memory and feed the data to the display engine. As the
>>>>> same framebuffer can be displayed for multiple frames, the DMA
>>>>> transactions need to be repeated until a new framebuffer replaces the
>>>>> current one. This feature is implemented natively by some DMA engines
>>>>> that have the ability to repeat transactions and switch to a new
>>>>> transaction at the end of a transfer without any race condition or frame
>>>>> loss.
>>>>>
>>>>> This patch implements support for this feature in the DMA engine API. A
>>>>> new DMA_PREP_REPEAT transaction flag allows DMA clients to instruct the
>>>>> DMA channel to repeat the transaction automatically until one or more
>>>>> new transactions are issued on the channel (or until all active DMA
>>>>> transfers are explicitly terminated with the dmaengine_terminate_*()
>>>>> functions). A new DMA_REPEAT transaction type is also added for DMA
>>>>> engine drivers to report their support of the DMA_PREP_REPEAT flag.
>>>>>
>>>>> The DMA_PREP_REPEAT flag is currently supported for interleaved
>>>>> transactions only. Its usage can easily be extended to cover more
>>>>> transaction types simply by adding an appropriate check in the
>>>>> corresponding dmaengine_prep_*() function.
>>>>>
>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>> ---
>>>>> If this approach is accepted I can send a new version that updates
>>>>> documentation in Documentation/driver-api/dmaengine/, and extend support
>>>>> of DMA_PREP_REPEAT to the other transaction types, if desired already.
>>>>>
>>>>>  include/linux/dmaengine.h | 10 ++++++++++
>>>>>  1 file changed, 10 insertions(+)
>>>>>
>>>>> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>>>>> index 64461fc64e1b..9fa00bdbf583 100644
>>>>> --- a/include/linux/dmaengine.h
>>>>> +++ b/include/linux/dmaengine.h
>>>>> @@ -61,6 +61,7 @@ enum dma_transaction_type {
>>>>>  	DMA_SLAVE,
>>>>>  	DMA_CYCLIC,
>>>>>  	DMA_INTERLEAVE,
>>>>> +	DMA_REPEAT,
>>>>>  /* last transaction type for creation of the capabilities mask */
>>>>>  	DMA_TX_TYPE_END,
>>>>>  };
>>>>> @@ -176,6 +177,11 @@ struct dma_interleaved_template {
>>>>>   * @DMA_PREP_CMD: tell the driver that the data passed to DMA API is command
>>>>>   *  data and the descriptor should be in different format from normal
>>>>>   *  data descriptors.
>>>>> + * @DMA_PREP_REPEAT: tell the driver that the transaction shall be automatically
>>>>> + *  repeated when it ends if no other transaction has been issued on the same
>>>>> + *  channel. If other transactions have been issued, this transaction completes
>>>>> + *  normally. This flag is only applicable to interleaved transactions and is
>>>>> + *  ignored for all other transaction types.

It should not be restricted to interleaved, slave_sg/memcpy/etc can be
repeated if the DMA driver implements it (a user on a given platform
needs it).

>>>>>   */
>>>>>  enum dma_ctrl_flags {
>>>>>  	DMA_PREP_INTERRUPT = (1 << 0),
>>>>> @@ -186,6 +192,7 @@ enum dma_ctrl_flags {
>>>>>  	DMA_PREP_FENCE = (1 << 5),
>>>>>  	DMA_CTRL_REUSE = (1 << 6),
>>>>>  	DMA_PREP_CMD = (1 << 7),
>>>>> +	DMA_PREP_REPEAT = (1 << 8),
>>>>
>>>> Thanks for sending this. I think this is a good proposal which Peter
>>>> made for solving this issue and it has great merits, but this is
>>>> incomplete.
>>>>
>>>> DMA_PREP_REPEAT|RELOAD should only imply repeating of transactions,
>>>> nothing else. I would like to see APIs having explicit behaviour, so let
>>>> us also add another flag DMA_PREP_LOAD_NEXT|NEW to indicate that the
>>>> next transactions will replace the current one when submitted after calling
>>>> .issue_pending().
>>>>
>>>> Also it makes sense to explicitly specify when the transaction should be
>>>> reloaded. Rather than make a guesswork based on hardware support, we
>>>> should specify the EOB/EOT in these flags as well.
>>>>
>>>> Next is callback notification mechanism and when it should be invoked.
>>>> EOT is today indicated by DMA_PREP_INTERRUPT, EOB needs to be added.
>>>>
>>>> So to summarize your driver needs to invoke
>>>> DMA_PREP_REPEAT|DMA_PREP_LOAD_NEXT|DMA_LOAD_EOT|DMA_PREP_INTERRUPT
>>>> specifying that the transactions are repeated untill next one pops up
>>>> and replaced at EOT with callbacks being invoked at EOT boundaries.
> 
> Peter, what do you think ?

Well, I'm in between ;)

You have a dedicated DMA which can do one thing - to service display.
DMAengine provides generic API for DMA use users.

The DMA_PREP_REPEAT is a new flag for a descriptor, imho it can be
introduced without breaking anything which exists today.

DMA_PREP_REPEAT - the descriptor should be repeated until the channel is
terminated with terminate_all.

DMA_PREP_LOAD_EOT - the descriptor should be loaded at the next EOT of
the currently running transfer, if any, otherwise start.

DMA_PREP_INTERRUPT - as it is today. Callback at EOT (for
slave_sg/interleaved/memcpy/etc, cyclic interprets this differently -
callback at period elapse time).

So you would set DMA_PREP_REPEAT | DMA_PREP_LOAD_EOT (|
DMA_PREP_INTERRUPT if you need callbacks at EOT).

The capabilities of the device/channel should tell the user if it is
capable of REPEAT and LOAD_EOT.
It is possible that a DMA can do repeat, but lacks the ability to do any
type of LOAD_*

I think this would give a nice starting point to extend on later.

- Péter

>>> Are you *serious* ? I feel trapped in a cross-over of Groundhog Day and
>>> Brazil.
>>
>> Sorry, I don't understand that reference!
>>
>> Nevertheless, you want a behaviour which is somehow defined by your use
>> and magically implies certain conditions. I do not want it that way.
>> I would rather see all the flag required.
>>
>>>> @Peter, did I miss anything else in this..? Please send the patch for
>>>> this (to start with just the headers so that Laurent can start
>>>> using them) and detailed patch with documentation as follow up, I trust
>>>> you two can coordinate :)
>>>
>>> I won't call that coordination, no. If you want to design something
>>> absurd that's your call, not mine, I don't want to get involved.
>>
>> Your wish!
> 

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