| Message ID | 20200528025228.31638-4-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | dma: Add Xilinx ZynqMP DPDMA driver | expand |
Hi Laurent, On 28-05-20, 05:52, Laurent Pinchart wrote: > @@ -176,6 +178,18 @@ 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. 1. Let us not restrict this to only interleave (hint we can in future replace cyclic API) 2. DMA_PREP_REPEAT telling the transaction shall be automatically repeated is okay. No issues with that > + * @DMA_PREP_LOAD_EOT: tell the driver that the transaction shall replaced any s/replaced/replace > + * active repeated (as indicated by DMA_PREP_REPEAT) transaction when the > + * repeated transaction terminate. Not setting this flag when the previously > + * queued transaction is marked with DMA_PREP_REPEAT will cause the new > + * transaction to never be processed and stay in the issued queue forever. > + * The flag is ignored if the previous transaction is not a repeated > + * transaction. I am happy with this bit, I think we dont need to specify something like DMA_PREP_LOAD_NEXT given the explanation here, so adding DMA_PREP_LOAD_EOT would mean that. Can we add a corresponding EOB as well to complete this
Hi Vinod, On Fri, Jul 03, 2020 at 10:40:39PM +0530, Vinod Koul wrote: > Hi Laurent, > > On 28-05-20, 05:52, Laurent Pinchart wrote: > > > @@ -176,6 +178,18 @@ 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. > > 1. Let us not restrict this to only interleave (hint we can in future > replace cyclic API) Peter wanted to already implement support for DMA_PREP_REPEAT in other transaction types, and you replied that you would prefer not enabling APIs without users, waiting for the first user of DMA_PREP_REPEAT with a non-interleaved transaction to do so. Your comment here seems to contract that. Which way do you want to go ? > 2. DMA_PREP_REPEAT telling the transaction shall be automatically > repeated is okay. No issues with that > > > + * @DMA_PREP_LOAD_EOT: tell the driver that the transaction shall replaced any > > s/replaced/replace > > > + * active repeated (as indicated by DMA_PREP_REPEAT) transaction when the > > + * repeated transaction terminate. Not setting this flag when the previously > > + * queued transaction is marked with DMA_PREP_REPEAT will cause the new > > + * transaction to never be processed and stay in the issued queue forever. > > + * The flag is ignored if the previous transaction is not a repeated > > + * transaction. > > I am happy with this bit, I think we dont need to specify something like > DMA_PREP_LOAD_NEXT given the explanation here, so adding > DMA_PREP_LOAD_EOT would mean that. Just to clarify, does that mean I don't need to add a DMA_PREP_LOAD_NEXT flag in the API ? > Can we add a corresponding EOB as well to complete this What's EOB ?
Hi Laurent, On 03-07-20, 20:22, Laurent Pinchart wrote: > On Fri, Jul 03, 2020 at 10:40:39PM +0530, Vinod Koul wrote: > > On 28-05-20, 05:52, Laurent Pinchart wrote: > > > > > @@ -176,6 +178,18 @@ 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. > > > > 1. Let us not restrict this to only interleave (hint we can in future > > replace cyclic API) > > Peter wanted to already implement support for DMA_PREP_REPEAT in other > transaction types, and you replied that you would prefer not enabling > APIs without users, waiting for the first user of DMA_PREP_REPEAT with a > non-interleaved transaction to do so. Your comment here seems to > contract that. Which way do you want to go ? I would like to change the language of the explanation to not forbid other uses, but they would be enabled in txn when we have users.. > > 2. DMA_PREP_REPEAT telling the transaction shall be automatically > > repeated is okay. No issues with that > > > > > + * @DMA_PREP_LOAD_EOT: tell the driver that the transaction shall replaced any > > > > s/replaced/replace > > > > > + * active repeated (as indicated by DMA_PREP_REPEAT) transaction when the > > > + * repeated transaction terminate. Not setting this flag when the previously > > > + * queued transaction is marked with DMA_PREP_REPEAT will cause the new > > > + * transaction to never be processed and stay in the issued queue forever. > > > + * The flag is ignored if the previous transaction is not a repeated > > > + * transaction. > > > > I am happy with this bit, I think we dont need to specify something like > > DMA_PREP_LOAD_NEXT given the explanation here, so adding > > DMA_PREP_LOAD_EOT would mean that. > > Just to clarify, does that mean I don't need to add a DMA_PREP_LOAD_NEXT > flag in the API ? > > > Can we add a corresponding EOB as well to complete this > > What's EOB ? End of Burst, DMA would complete current burst and then terminate. I do understand it is not applicable for your case as your hw doesn't support it and would be unused but this is another case which would be useful so for completeness we should add this
Hi Vinod, On Fri, Jul 03, 2020 at 11:07:10PM +0530, Vinod Koul wrote: > On 03-07-20, 20:22, Laurent Pinchart wrote: > > On Fri, Jul 03, 2020 at 10:40:39PM +0530, Vinod Koul wrote: > > > On 28-05-20, 05:52, Laurent Pinchart wrote: > > > > > > > @@ -176,6 +178,18 @@ 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. > > > > > > 1. Let us not restrict this to only interleave (hint we can in future > > > replace cyclic API) > > > > Peter wanted to already implement support for DMA_PREP_REPEAT in other > > transaction types, and you replied that you would prefer not enabling > > APIs without users, waiting for the first user of DMA_PREP_REPEAT with a > > non-interleaved transaction to do so. Your comment here seems to > > contract that. Which way do you want to go ? > > I would like to change the language of the explanation to not forbid > other uses, but they would be enabled in txn when we have users.. The documentation isn't set in stone, it can be updated when support for DMA_PREP_REPEAT will be enabled in other transaction types. I think it's better to have the documentation and code match, the documentation should describe the current implementation. I can add an additional sentence at the end of the paragraph to state "Support for this flag in other transaction types may be added in the future if the need arises." if that makes you feel better about it. > > > 2. DMA_PREP_REPEAT telling the transaction shall be automatically > > > repeated is okay. No issues with that > > > > > > > + * @DMA_PREP_LOAD_EOT: tell the driver that the transaction shall replaced any > > > > > > s/replaced/replace > > > > > > > + * active repeated (as indicated by DMA_PREP_REPEAT) transaction when the > > > > + * repeated transaction terminate. Not setting this flag when the previously > > > > + * queued transaction is marked with DMA_PREP_REPEAT will cause the new > > > > + * transaction to never be processed and stay in the issued queue forever. > > > > + * The flag is ignored if the previous transaction is not a repeated > > > > + * transaction. > > > > > > I am happy with this bit, I think we dont need to specify something like > > > DMA_PREP_LOAD_NEXT given the explanation here, so adding > > > DMA_PREP_LOAD_EOT would mean that. > > > > Just to clarify, does that mean I don't need to add a DMA_PREP_LOAD_NEXT > > flag in the API ? > > > > > Can we add a corresponding EOB as well to complete this > > > > What's EOB ? > > End of Burst, DMA would complete current burst and then terminate. I do > understand it is not applicable for your case as your hw doesn't support > it and would be unused but this is another case which would be useful so > for completeness we should add this Why not add it when we'll have a user for it ? How can you otherwise guarantee that it's a correct API if there's not even a single user that we can test the design with ?
HI Laurent, On 03-07-20, 20:47, Laurent Pinchart wrote: > On Fri, Jul 03, 2020 at 11:07:10PM +0530, Vinod Koul wrote: > > On 03-07-20, 20:22, Laurent Pinchart wrote: > > > On Fri, Jul 03, 2020 at 10:40:39PM +0530, Vinod Koul wrote: > > > > On 28-05-20, 05:52, Laurent Pinchart wrote: > > > > > > > > > @@ -176,6 +178,18 @@ 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. > > > > > > > > 1. Let us not restrict this to only interleave (hint we can in future > > > > replace cyclic API) > > > > > > Peter wanted to already implement support for DMA_PREP_REPEAT in other > > > transaction types, and you replied that you would prefer not enabling > > > APIs without users, waiting for the first user of DMA_PREP_REPEAT with a > > > non-interleaved transaction to do so. Your comment here seems to > > > contract that. Which way do you want to go ? > > > > I would like to change the language of the explanation to not forbid > > other uses, but they would be enabled in txn when we have users.. > > The documentation isn't set in stone, it can be updated when support for > DMA_PREP_REPEAT will be enabled in other transaction types. I think it's > better to have the documentation and code match, the documentation > should describe the current implementation. I can add an additional > sentence at the end of the paragraph to state "Support for this flag in > other transaction types may be added in the future if the need arises." > if that makes you feel better about it. Okay I will be okay with that caveat > > > > 2. DMA_PREP_REPEAT telling the transaction shall be automatically > > > > repeated is okay. No issues with that > > > > > > > > > + * @DMA_PREP_LOAD_EOT: tell the driver that the transaction shall replaced any > > > > > > > > s/replaced/replace > > > > > > > > > + * active repeated (as indicated by DMA_PREP_REPEAT) transaction when the > > > > > + * repeated transaction terminate. Not setting this flag when the previously > > > > > + * queued transaction is marked with DMA_PREP_REPEAT will cause the new > > > > > + * transaction to never be processed and stay in the issued queue forever. > > > > > + * The flag is ignored if the previous transaction is not a repeated > > > > > + * transaction. > > > > > > > > I am happy with this bit, I think we dont need to specify something like > > > > DMA_PREP_LOAD_NEXT given the explanation here, so adding > > > > DMA_PREP_LOAD_EOT would mean that. > > > > > > Just to clarify, does that mean I don't need to add a DMA_PREP_LOAD_NEXT > > > flag in the API ? > > > > > > > Can we add a corresponding EOB as well to complete this > > > > > > What's EOB ? > > > > End of Burst, DMA would complete current burst and then terminate. I do > > understand it is not applicable for your case as your hw doesn't support > > it and would be unused but this is another case which would be useful so > > for completeness we should add this > > Why not add it when we'll have a user for it ? How can you otherwise > guarantee that it's a correct API if there's not even a single user that > we can test the design with ? Okay lets not add but add a note that additional flags can be added when required like EOB Also can we add a note in Documentation about these flags and how they should be used for repeating transactions. Thanks
Hi Vinod, On Fri, Jul 03, 2020 at 11:24:41PM +0530, Vinod Koul wrote: > On 03-07-20, 20:47, Laurent Pinchart wrote: > > On Fri, Jul 03, 2020 at 11:07:10PM +0530, Vinod Koul wrote: > >> On 03-07-20, 20:22, Laurent Pinchart wrote: > >>> On Fri, Jul 03, 2020 at 10:40:39PM +0530, Vinod Koul wrote: > >>>> On 28-05-20, 05:52, Laurent Pinchart wrote: > >>>> > >>>>> @@ -176,6 +178,18 @@ 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. > >>>> > >>>> 1. Let us not restrict this to only interleave (hint we can in future > >>>> replace cyclic API) > >>> > >>> Peter wanted to already implement support for DMA_PREP_REPEAT in other > >>> transaction types, and you replied that you would prefer not enabling > >>> APIs without users, waiting for the first user of DMA_PREP_REPEAT with a > >>> non-interleaved transaction to do so. Your comment here seems to > >>> contract that. Which way do you want to go ? > >> > >> I would like to change the language of the explanation to not forbid > >> other uses, but they would be enabled in txn when we have users.. > > > > The documentation isn't set in stone, it can be updated when support for > > DMA_PREP_REPEAT will be enabled in other transaction types. I think it's > > better to have the documentation and code match, the documentation > > should describe the current implementation. I can add an additional > > sentence at the end of the paragraph to state "Support for this flag in > > other transaction types may be added in the future if the need arises." > > if that makes you feel better about it. > > Okay I will be okay with that caveat > > >>>> 2. DMA_PREP_REPEAT telling the transaction shall be automatically > >>>> repeated is okay. No issues with that > >>>> > >>>>> + * @DMA_PREP_LOAD_EOT: tell the driver that the transaction shall replaced any > >>>> > >>>> s/replaced/replace > >>>> > >>>>> + * active repeated (as indicated by DMA_PREP_REPEAT) transaction when the > >>>>> + * repeated transaction terminate. Not setting this flag when the previously > >>>>> + * queued transaction is marked with DMA_PREP_REPEAT will cause the new > >>>>> + * transaction to never be processed and stay in the issued queue forever. > >>>>> + * The flag is ignored if the previous transaction is not a repeated > >>>>> + * transaction. > >>>> > >>>> I am happy with this bit, I think we dont need to specify something like > >>>> DMA_PREP_LOAD_NEXT given the explanation here, so adding > >>>> DMA_PREP_LOAD_EOT would mean that. > >>> > >>> Just to clarify, does that mean I don't need to add a DMA_PREP_LOAD_NEXT > >>> flag in the API ? > >>> > >>>> Can we add a corresponding EOB as well to complete this > >>> > >>> What's EOB ? > >> > >> End of Burst, DMA would complete current burst and then terminate. I do > >> understand it is not applicable for your case as your hw doesn't support > >> it and would be unused but this is another case which would be useful so > >> for completeness we should add this > > > > Why not add it when we'll have a user for it ? How can you otherwise > > guarantee that it's a correct API if there's not even a single user that > > we can test the design with ? > > Okay lets not add but add a note that additional flags can be added when > required like EOB > > Also can we add a note in Documentation about these flags and how they > should be used for repeating transactions. Done, and I've added the notes regarding future extensions (support of non-interleaved repeated transactions, and EOB) there too, as it made more sense than in the header file. The new patches are in your mailbox :-)
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h index 21065c04c4ac..8e3769c2b841 100644 --- a/include/linux/dmaengine.h +++ b/include/linux/dmaengine.h @@ -61,6 +61,8 @@ enum dma_transaction_type { DMA_SLAVE, DMA_CYCLIC, DMA_INTERLEAVE, + DMA_REPEAT, + DMA_LOAD_EOT, /* last transaction type for creation of the capabilities mask */ DMA_TX_TYPE_END, }; @@ -176,6 +178,18 @@ 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. + * @DMA_PREP_LOAD_EOT: tell the driver that the transaction shall replaced any + * active repeated (as indicated by DMA_PREP_REPEAT) transaction when the + * repeated transaction terminate. Not setting this flag when the previously + * queued transaction is marked with DMA_PREP_REPEAT will cause the new + * transaction to never be processed and stay in the issued queue forever. + * The flag is ignored if the previous transaction is not a repeated + * transaction. */ enum dma_ctrl_flags { DMA_PREP_INTERRUPT = (1 << 0), @@ -186,6 +200,8 @@ enum dma_ctrl_flags { DMA_PREP_FENCE = (1 << 5), DMA_CTRL_REUSE = (1 << 6), DMA_PREP_CMD = (1 << 7), + DMA_PREP_REPEAT = (1 << 8), + DMA_PREP_LOAD_EOT = (1 << 9), }; /** @@ -980,6 +996,9 @@ static inline struct dma_async_tx_descriptor *dmaengine_prep_interleaved_dma( { if (!chan || !chan->device || !chan->device->device_prep_interleaved_dma) return NULL; + if (flags & DMA_PREP_REPEAT && + !test_bit(DMA_REPEAT, chan->device->cap_mask.bits)) + return NULL; return chan->device->device_prep_interleaved_dma(chan, xt, flags); }
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. A new DMA_PREP_LOAD_EOT transaction flag is also introduced (with a corresponding DMA_LOAD_EOT capability bit), as requested during the review of v4. The flag instructs the DMA channel that the transaction being queued should replace the active repeated transaction when the latter terminates (at End Of Transaction). Not setting the flag will result in the active repeated transaction to continue being repeated, and the new transaction being silently ignored. 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> --- Changes since v4: - Add DMA_LOAD_EOT and DMA_PREP_LOAD_EOT flags --- include/linux/dmaengine.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)