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